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

[David Lechner]

On Sat, Mar 16, 2024 at 2:57 PM Andy Shevchenko
<andy.shevchenko@xxxxxxxxx> wrote:
>
> Thu, Mar 14, 2024 at 12:43:38PM -0500, David Lechner kirjoitti:
> > This adds support for AD7944 ADCs wired in "3-wire mode". (NOTE: 3-wire
> > is the datasheet name for this wiring configuration and has nothing to
> > do with SPI_3WIRE.)
> >
> > In the 3-wire mode, the SPI controller CS line can be wired to the CNV
> > line on the ADC and used to trigger conversions rather that using a
> > separate GPIO line.
> >
> > The turbo/chain mode compatibility check at the end of the probe
> > function is technically can't be triggered right now but adding it now
> > anyway so that we don't forget to add it later when support for
> > daisy-chaining is added.
>
> ...
>
> > +enum ad7944_spi_mode {
> > +     /* datasheet calls this "4-wire mode" */
> > +     AD7944_SPI_MODE_DEFAULT,
> > +     /* datasheet calls this "3-wire mode" (not related to SPI_3WIRE!) */
> > +     AD7944_SPI_MODE_SINGLE,
> > +     /* datasheet calls this "chain mode" */
> > +     AD7944_SPI_MODE_CHAIN,
>
> Why not kernel doc?

This isn't a public/shared enum so it doesn't seem like it needs it.
It would just add redundant enum member names.

>
> > +};
>
> ...
>
> >  struct ad7944_adc {
> >       struct spi_device *spi;
> > +     enum ad7944_spi_mode spi_mode;
> >       /* Chip-specific timing specifications. */
> >       const struct ad7944_timing_spec *timing_spec;
> >       /* GPIO connected to CNV pin. */
> > @@ -58,6 +75,9 @@ struct ad7944_adc {
> >        } sample __aligned(IIO_DMA_MINALIGN);
> >  };
>
> Have you run `pahole` to see if there is a better place for a new member?

Nope. Not familiar with this tool. I can address this in a planned
patch that will be adding more members to this struct.

>
> ...
>
> > +/*
>
> The below is mimicing the kernel doc, but there is no marker for this.
> Why?

I received feedback on another patch in a different subsystem that
static functions shouldn't use /** since they aren't used outside of
the file where they are.

>
> > + * ad7944_3wire_cs_mode_conversion - Perform a 3-wire CS mode conversion and
> > + *                                   acquisition
> > + * @adc: The ADC device structure
> > + * @chan: The channel specification
> > + * Return: 0 on success, a negative error code on failure
> > + *
> > + * This performs a conversion and reads data when the chip is wired in 3-wire
> > + * mode with the CNV line on the ADC tied to the CS line on the SPI controller.
> > + *
> > + * Upon successful return adc->sample.raw will contain the conversion result.
> > + */
>
> ...
>
> > +     struct spi_transfer xfers[] = {
> > +             {
> > +                     /*
> > +                      * NB: can get better performance from some SPI
> > +                      * controllers if we use the same bits_per_word
> > +                      * in every transfer.
>
> I believe you may reformat this to reduce by 1 line.
>
> > +                      */
> > +                     .bits_per_word = chan->scan_type.realbits,
> > +                     /*
> > +                      * CS is tied to CNV and we need a low to high
> > +                      * transition to start the conversion, so place CNV
> > +                      * low for t_QUIET to prepare for this.
>
> This also seems narrow.

I have another patch in the works that will be moving these, so it can
be addressed then.

>
> > +                      */
> > +                     .delay = {
> > +                             .value = T_QUIET_NS,
> > +                             .unit = SPI_DELAY_UNIT_NSECS,
> > +                     },
> > +
> > +             },
> > +             {
> > +                     .bits_per_word = chan->scan_type.realbits,
> > +                     /*
> > +                      * CS has to be high for full conversion time to avoid
> > +                      * triggering the busy indication.
> > +                      */
> > +                     .cs_off = 1,
> > +                     .delay = {
> > +                             .value = t_conv_ns,
> > +                             .unit = SPI_DELAY_UNIT_NSECS,
> > +                     },
> > +             },
> > +             {
> > +                     /* Then we can read the data during the acquisition phase */
> > +                     .rx_buf = &adc->sample.raw,
> > +                     .len = BITS_TO_BYTES(chan->scan_type.storagebits),
> > +                     .bits_per_word = chan->scan_type.realbits,
> > +             },
> > +     };
>
> ...
>
> > +     case AD7944_SPI_MODE_SINGLE:
> > +             ret = ad7944_3wire_cs_mode_conversion(adc, &indio_dev->channels[0]);
> > +             if (ret)
> > +                     goto out;
> > +
> > +             break;
> > +     default:
> > +             /* not supported */
>
> No error code set?

This is in an interrupt handler, so I didn't think there was anything
we can do with an error.

>
> >               goto out;
> > +     }
>
> ...
>
> > +     if (device_property_read_string(dev, "adi,spi-mode", &str_val) == 0) {
> > +             ret = sysfs_match_string(ad7944_spi_modes, str_val);
>
> Don't you want use new fwnode/device property API for making these two in
> one go?

I didn't know there was one. I assume you mean
fwnode_property_match_property_string().

>
> > +             if (ret < 0)
> > +                     return dev_err_probe(dev, -EINVAL,
>
> Why shadowing the error code?

Cargo culted from one of a few of users of sysfs_match_string() that does this.

Jonathan already picked this patch up so I can follow up with a patch
to clean up these two items.

>
> > +                                          "unsupported adi,spi-mode\n");
> > +
> > +             adc->spi_mode = ret;
> > +     } else {
> > +             /* absence of adi,spi-mode property means default mode */
> > +             adc->spi_mode = AD7944_SPI_MODE_DEFAULT;
> > +     }
>
> --
> With Best Regards,
> Andy Shevchenko
>
>