Received-SPF: pass (google.com: domain of linux-kernel+bounces-103522-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Thu, 14 Mar 2024 15:41:37 +0000
From: Jonathan Cameron <jic23@kernel.org>
To: David Lechner <dlechner@baylibre.com>
Cc: Nuno =?UTF-8?B?U8Oh?= <noname.nuno@gmail.com>, Michael Hennerich
 <michael.hennerich@analog.com>, Nuno =?UTF-8?B?U8Oh?= <nuno.sa@analog.com>,
 linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] iio: adc: ad7944: Add support for "3-wire mode"
Message-ID: <20240314154137.2fdf82ba@jic23-huawei>
In-Reply-To: <CAMknhBE8OwrtbJ9xYVZ8ObsZTnxmn9Fpk2a-gj1aCSaN-whDRg@mail.gmail.com>
References: <20240311-mainline-ad7944-3-wire-mode-v1-1-8e8199efa1f7@baylibre.com>
	<8ee551edeff9c4c959a4dbda53d1a2a26a9bb62c.camel@gmail.com>
	<CAMknhBE8OwrtbJ9xYVZ8ObsZTnxmn9Fpk2a-gj1aCSaN-whDRg@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On Tue, 12 Mar 2024 09:13:56 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On Tue, Mar 12, 2024 at 4:08=E2=80=AFAM Nuno S=C3=A1 <noname.nuno@gmail.c=
om> wrote:
> >
> > On Mon, 2024-03-11 at 16:26 -0500, David Lechner wrote: =20
>=20
> ...
>=20
> > >  /*
> > >   * ad7944_4wire_mode_conversion - Perform a 4-wire mode conversion a=
nd acquisition
> > >   * @adc: The ADC device structure
> > > @@ -167,9 +246,22 @@ static int ad7944_single_conversion(struct ad794=
4_adc *adc,
> > >  {
> > >       int ret;
> > >
> > > -     ret =3D ad7944_4wire_mode_conversion(adc, chan);
> > > -     if (ret)
> > > -             return ret;
> > > +     switch (adc->spi_mode) {
> > > +     case AD7944_SPI_MODE_DEFAULT:
> > > +             ret =3D ad7944_4wire_mode_conversion(adc, chan);
> > > +             if (ret)
> > > +                     return ret;
> > > +
> > > +             break;
> > > +     case AD7944_SPI_MODE_SINGLE:
> > > +             ret =3D ad7944_3wire_cs_mode_conversion(adc, chan);
> > > +             if (ret)
> > > +                     return ret;
> > > +
> > > +             break;
> > > +     case AD7944_SPI_MODE_CHAIN:
> > > +             return -EOPNOTSUPP; =20
> >
> > This mode is not really supported for now and in theory we can't really=
 have
> > adc->spi_mode =3D AD7944_SPI_MODE_CHAIN, right? So, I would just make t=
his the
> > 'default' branch and not care about chain mode (implementing it when ad=
ding it). =20
>=20
> The compiler was happy with this, but yeah, default: is probably safer.
>=20
> ...
>=20
> > > +     if (!adc->cnv && adc->spi_mode =3D=3D AD7944_SPI_MODE_DEFAULT)
> > > +             return dev_err_probe(&spi->dev, -EINVAL, "CNV GPIO is
> > > required\n");
> > > +     else if (adc->cnv && adc->spi_mode !=3D AD7944_SPI_MODE_DEFAULT)
> > > +             return dev_err_probe(&spi->dev, -EINVAL,
> > > +                                  "CNV GPIO in single and chain mode=
 is not
> > > currently supported\n");
> > > + =20
> >
> > Redundant else... =20
>=20
> yup
>=20
> > =20
> > >       adc->turbo =3D devm_gpiod_get_optional(dev, "turbo", GPIOD_OUT_=
LOW);
> > >       if (IS_ERR(adc->turbo))
> > >               return dev_err_probe(dev, PTR_ERR(adc->turbo),
> > > @@ -369,6 +486,10 @@ static int ad7944_probe(struct spi_device *spi)
> > >               return dev_err_probe(dev, -EINVAL,
> > >                       "cannot have both turbo-gpios and adi,always-tu=
rbo\n");
> > >
> > > +     if (adc->spi_mode =3D=3D AD7944_SPI_MODE_CHAIN && adc->always_t=
urbo)
> > > +             return dev_err_probe(dev, -EINVAL,
> > > +                     "cannot have both chain mode and always turbo\n=
");
> > > + =20
> >
> >
> > I'm fine in having this now but shouldn't we only have the above when w=
e do support
> > chain mode? A bit odd having it when we don't even allow chain mode.
> > =20
>=20
> Yeah, we could wait to add this. It seemed like something easy to
> overlook though if we don't add chain mode right away, so I just went
> ahead and added it now.
Seems reasonable - maybe just mention it in the patch description.

Other than the tidying up Nuno pointed out looks good to me, so I'll pick up
v2 once posted.

Jonathan