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

[Andy Shevchenko]

On Sun, Mar 17, 2024 at 1:10 AM David Lechner <dlechner@xxxxxxxxxxxx> wrote:
> 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:

..

> > > +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 doesn't matter.

> It would just add redundant enum member names.

These comments make it harder to follow in my opinion.

..

> > > +/*
> >
> > The below is mimicking 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.

Was it the IIO subsystem? (I believe not).
Again, that feedback is bogus as we control what to share and what not
in the documentation (when importing we may say internal or external
or even on the function granularity), however, even for static
functions it's good to have documentation in the approved format as it
makes it easier to render.

> > > + * 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.
> > > + */

..

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

return IRQ_NONE?

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

Yes, but here is the device_ variant of it.

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

As far as I remember he was planning to rebase, so I believe he can
easily fold fixups.

> > > +                                          "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