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] [day] [month] [year] [list]
Date: Thu, 14 Mar 2024 15:41:37 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: David Lechner <dlechner@...libre.com>
Cc: Nuno Sá <noname.nuno@...il.com>, 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, 12 Mar 2024 09:13:56 -0500
David Lechner <dlechner@...libre.com> wrote:

> 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.
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



Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ