On 06/26, Alexandru Ardelean wrote:
> On Wed, Jun 26, 2024 at 12:56 AM Marcelo Schmitt
> <marcelo.schmitt@xxxxxxxxxx> wrote:
> >
> > Add support for AD4000 series of low noise, low power, high speed,
> > successive approximation register (SAR) ADCs.
> >
>
> Hello :)
Hey Alexandru, nice to hear from you.
>
> Looks good overall.
> Just a few comments.
> The only one where I am not sure is about the enum-to-string mapping.
> If that's fine, we can leave this unchanged (from my side).
>
> > Signed-off-by: Marcelo Schmitt <marcelo.schmitt@xxxxxxxxxx>
> > ---
> > MAINTAINERS | 1 +
> > drivers/iio/adc/Kconfig | 12 +
> > drivers/iio/adc/Makefile | 1 +
> > drivers/iio/adc/ad4000.c | 711 +++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 725 insertions(+)
> > create mode 100644 drivers/iio/adc/ad4000.c
> >
...
> > +enum ad4000_sdi {
> > + /* datasheet calls this "4-wire mode" (controller CS goes to ADC SDI!) */
> > + AD4000_SDI_MOSI,
> > + /* datasheet calls this "3-wire mode" (not related to SPI_3WIRE!) */
> > + AD4000_SDI_VIO,
> > + AD4000_SDI_CS,
> > +};
> > +
> > +/* maps adi,sdi-pin property value to enum */
> > +static const char * const ad4000_sdi_pin[] = {
> > + [AD4000_SDI_MOSI] = "",
>
> Maybe I missed a previous comment.
> And I'm also a little fuzzy on the details here, but in the DT this
> property has "high", "low", "cs".
> Is "low" the default if unspecified?
> Or should this string be "low"?
The default is to have MOSI connected to ADC SDI pin which was empty adi,sdi-pin
dt property in v5.
Will make the defalut explicit as "sdi" as suggested in dt-binding review.
>
> > + [AD4000_SDI_VIO] = "high",
> > + [AD4000_SDI_CS] = "cs",
> > +};
> > +
...
> > +
> > + st->gain_milli = 1000;
> > + if (chip->has_hardware_gain) {
> > + if (device_property_present(dev, "adi,gain-milli")) {
>
> Only if there is another version, it may be neat to reduce indentation
> here (a bit).
> Something like:
> if (chip->has_hardware_gain &&
> device_property_present(dev, "adi,gain-milli")) {
>
> }
>
looks good, will do.
Thanks
