On Tue, 12 Mar 2024 09:13:56 -0500
David Lechner <dlechner@xxxxxxxxxxxx> wrote:
> On Tue, Mar 12, 2024 at 4:08 AM Nuno Sá <noname.nuno@xxxxxxxxx> 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
