Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
MIME-Version: 1.0
References: <20231121-dev-iio-backend-v1-0-6a3d542eba35@analog.com> <20231121-dev-iio-backend-v1-4-6a3d542eba35@analog.com>
In-Reply-To: <20231121-dev-iio-backend-v1-4-6a3d542eba35@analog.com>
From:   David Lechner <dlechner@baylibre.com>
Date:   Thu, 30 Nov 2023 15:41:01 -0600
Message-ID: <CAMknhBGCqnzCp6vQ+59Z-SybScvbtU7aWdAD6KnP1e6=q60gVQ@mail.gmail.com>
Subject: Re: [PATCH 04/12] iio: adc: ad9467: fix reset gpio handling
To:     nuno.sa@analog.com
Cc:     linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
        linux-iio@vger.kernel.org,
        Olivier MOYSAN <olivier.moysan@foss.st.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Frank Rowand <frowand.list@gmail.com>,
        Jonathan Cameron <jic23@kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Michael Hennerich <Michael.Hennerich@analog.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Nov 21, 2023 at 4:17=E2=80=AFAM Nuno Sa via B4 Relay
<devnull+nuno.sa.analog.com@kernel.org> wrote:
>
> From: Nuno Sa <nuno.sa@analog.com>
>
> The reset gpio was being requested with GPIOD_OUT_LOW which means, not
> asserted. Then it was being asserted but never de-asserted which means
> the devices was left in reset. Fix it by de-asserting the gpio.

It could be helpful to update the devicetree bindings to state the
expected active-high or active-low setting for this gpio so it is
clear which state means asserted.

>
> While at it, moved the handling to it's own function and dropped
> 'reset_gpio' from the 'struct ad9467_state' as we only need it during
> probe. On top of that, refactored things so that we now request the gpio
> asserted (i.e in reset) and then de-assert it.
>
> Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> ---
>  drivers/iio/adc/ad9467.c | 33 ++++++++++++++++++++-------------
>  1 file changed, 20 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
> index 39eccc28debe..368ea57be117 100644
> --- a/drivers/iio/adc/ad9467.c
> +++ b/drivers/iio/adc/ad9467.c
> @@ -121,7 +121,6 @@ struct ad9467_state {
>         unsigned int                    output_mode;
>
>         struct gpio_desc                *pwrdown_gpio;
> -       struct gpio_desc                *reset_gpio;
>  };
>
>  static int ad9467_spi_read(struct spi_device *spi, unsigned int reg)
> @@ -378,6 +377,23 @@ static int ad9467_preenable_setup(struct adi_axi_adc=
_conv *conv)
>         return ad9467_outputmode_set(st->spi, st->output_mode);
>  }
>
> +static int ad9467_reset(struct device *dev)
> +{
> +       struct gpio_desc *gpio;
> +
> +       gpio =3D devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +       if (IS_ERR(gpio))
> +               return PTR_ERR(gpio);
> +       if (!gpio)
> +               return 0;

can be done in one test instead of 2:

if (IS_ERR_OR_NULL(gpio))
        return PTR_ERR_OR_ZERO(gpio);

> +
> +       fsleep(1);
> +       gpiod_direction_output(gpio, 0);
> +       fsleep(10);

Previous version was 10 milliseconds instead of 10 microseconds. Was
this change intentional? If yes, it should be mentioned it in the
commit message.

> +
> +       return 0;
> +}
> +
>  static int ad9467_probe(struct spi_device *spi)
>  {
>         const struct ad9467_chip_info *info;
> @@ -408,18 +424,9 @@ static int ad9467_probe(struct spi_device *spi)
>         if (IS_ERR(st->pwrdown_gpio))
>                 return PTR_ERR(st->pwrdown_gpio);
>
> -       st->reset_gpio =3D devm_gpiod_get_optional(&spi->dev, "reset",
> -                                                GPIOD_OUT_LOW);
> -       if (IS_ERR(st->reset_gpio))
> -               return PTR_ERR(st->reset_gpio);
> -
> -       if (st->reset_gpio) {
> -               udelay(1);
> -               ret =3D gpiod_direction_output(st->reset_gpio, 1);
> -               if (ret)
> -                       return ret;
> -               mdelay(10);
> -       }
> +       ret =3D ad9467_reset(&spi->dev);
> +       if (ret)
> +               return ret;
>
>         conv->chip_info =3D &info->axi_adc_info;
>
>
> --
> 2.42.1
>
>