linux-kernel - Re: [PATCH] iio: adc: ad7944: Add support for "3-wire mode"

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 12 Mar 2024 09:13:56 -0500
From: David Lechner <dlechner@...libre.com>
To: Nuno Sá <noname.nuno@...il.com>
Cc: Jonathan Cameron <jic23@...nel.org>, Michael Hennerich <michael.hennerich@...log.com>, 
	Nuno Sá <nuno.sa@...log.com>, linux-iio@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iio: adc: ad7944: Add support for "3-wire mode"

On Tue, Mar 12, 2024 at 4:08 AM Nuno Sá <noname.nuno@...il.com> wrote:
>
> On Mon, 2024-03-11 at 16:26 -0500, David Lechner wrote:

..

> >  /*
> >   * ad7944_4wire_mode_conversion - Perform a 4-wire mode conversion and acquisition
> >   * @adc: The ADC device structure
> > @@ -167,9 +246,22 @@ static int ad7944_single_conversion(struct ad7944_adc *adc,
> >  {
> >       int ret;
> >
> > -     ret = ad7944_4wire_mode_conversion(adc, chan);
> > -     if (ret)
> > -             return ret;
> > +     switch (adc->spi_mode) {
> > +     case AD7944_SPI_MODE_DEFAULT:
> > +             ret = ad7944_4wire_mode_conversion(adc, chan);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             break;
> > +     case AD7944_SPI_MODE_SINGLE:
> > +             ret = ad7944_3wire_cs_mode_conversion(adc, chan);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             break;
> > +     case AD7944_SPI_MODE_CHAIN:
> > +             return -EOPNOTSUPP;
>
> This mode is not really supported for now and in theory we can't really have
> adc->spi_mode = AD7944_SPI_MODE_CHAIN, right? So, I would just make this the
> 'default' branch and not care about chain mode (implementing it when adding it).

The compiler was happy with this, but yeah, default: is probably safer.

..

> > +     if (!adc->cnv && adc->spi_mode == AD7944_SPI_MODE_DEFAULT)
> > +             return dev_err_probe(&spi->dev, -EINVAL, "CNV GPIO is
> > required\n");
> > +     else if (adc->cnv && adc->spi_mode != AD7944_SPI_MODE_DEFAULT)
> > +             return dev_err_probe(&spi->dev, -EINVAL,
> > +                                  "CNV GPIO in single and chain mode is not
> > currently supported\n");
> > +
>
> Redundant else...

yup

>
> >       adc->turbo = 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-turbo\n");
> >
> > +     if (adc->spi_mode == AD7944_SPI_MODE_CHAIN && adc->always_turbo)
> > +             return dev_err_probe(dev, -EINVAL,
> > +                     "cannot have both chain mode and always turbo\n");
> > +
>
>
> I'm fine in having this now but shouldn't we only have the above when we do support
> chain mode? A bit odd having it when we don't even allow chain mode.
>

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.