Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Message-ID: <d534c3323c32d4ed2aedae19a9f101be90ef0cc7.camel@gmail.com>
Subject: Re: [PATCH 04/12] iio: adc: ad9467: fix reset gpio handling
From:   Nuno =?ISO-8859-1?Q?S=E1?= <noname.nuno@gmail.com>
To:     David Lechner <dlechner@baylibre.com>, 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>
Date:   Fri, 01 Dec 2023 09:47:24 +0100
In-Reply-To: <CAMknhBGCqnzCp6vQ+59Z-SybScvbtU7aWdAD6KnP1e6=q60gVQ@mail.gmail.com>
References: <20231121-dev-iio-backend-v1-0-6a3d542eba35@analog.com>
         <20231121-dev-iio-backend-v1-4-6a3d542eba35@analog.com>
         <CAMknhBGCqnzCp6vQ+59Z-SybScvbtU7aWdAD6KnP1e6=q60gVQ@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.48.4 (3.48.4-1.fc38) 
MIME-Version: 1.0
Precedence: bulk

On Thu, 2023-11-30 at 15:41 -0600, David Lechner wrote:
> On Tue, Nov 21, 2023 at 4:17=E2=80=AFAM Nuno Sa via B4 Relay
> <devnull+nuno.sa.analog.com@kernel.org> wrote:
> >=20
> > From: Nuno Sa <nuno.sa@analog.com>
> >=20
> > 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.
>=20
> 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.
>=20

You could state that the chip is active low but I don't see that change tha=
t
important for now. Not sure if this is clear and maybe that's why your comm=
ent.
GPIOD_OUT_HIGH has nothing to do with active high or low. It just means, "g=
et me the
pin in the asserted state".

> > 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 gpi=
o
> > asserted (i.e in reset) and then de-assert it.
> >=20
> > Fixes: ad6797120238 ("iio: adc: ad9467: add support AD9467 ADC")
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > ---
> > =C2=A0drivers/iio/adc/ad9467.c | 33 ++++++++++++++++++++-------------
> > =C2=A01 file changed, 20 insertions(+), 13 deletions(-)
> >=20
> > 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 {
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 unsigned int=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0 output_mode;
> >=20
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct gpio_desc=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0 *pwrdown_gpio;
> > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct gpio_desc=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
 *reset_gpio;
> > =C2=A0};
> >=20
> > =C2=A0static int ad9467_spi_read(struct spi_device *spi, unsigned int r=
eg)
> > @@ -378,6 +377,23 @@ static int ad9467_preenable_setup(struct adi_axi_a=
dc_conv
> > *conv)
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return ad9467_outputmode_set=
(st->spi, st->output_mode);
> > =C2=A0}
> >=20
> > +static int ad9467_reset(struct device *dev)
> > +{
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct gpio_desc *gpio;
> > +
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 gpio =3D devm_gpiod_get_optional(=
dev, "reset", GPIOD_OUT_HIGH);
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (IS_ERR(gpio))
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0 return PTR_ERR(gpio);
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (!gpio)
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0 return 0;
>=20
> can be done in one test instead of 2:
>=20
> if (IS_ERR_OR_NULL(gpio))
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return PTR_ERR_OR_ZERO(gpio);
>=20

Yep, better that way...

> > +
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 fsleep(1);
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 gpiod_direction_output(gpio, 0);
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 fsleep(10);
>=20
> Previous version was 10 milliseconds instead of 10 microseconds. Was
> this change intentional? If yes, it should be mentioned it in the
> commit message.

Oh, good catch! Copy past thing with even realizing the differences in the =
arguments
:face_palm:

- Nuno S=C3=A1


>=20