Received-SPF: pass (google.com: domain of linux-kernel+bounces-105302-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240314-mainline-ad7944-3-wire-mode-v2-1-d469da0705d2@baylibre.com>
 <ZfX5jynjW4M9pvw1@surfacebook.localdomain>
In-Reply-To: <ZfX5jynjW4M9pvw1@surfacebook.localdomain>
From: David Lechner <dlechner@baylibre.com>
Date: Sat, 16 Mar 2024 18:10:28 -0500
Message-ID: <CAMknhBGMUQFoQ9TxTTgy0dxHoyXkt+5tS93tpwz5Wo=h1UQD3Q@mail.gmail.com>
Subject: Re: [PATCH v2] iio: adc: ad7944: Add support for "3-wire mode"
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Jonathan Cameron <jic23@kernel.org>, Michael Hennerich <michael.hennerich@analog.com>, 
	=?UTF-8?B?TnVubyBTw6E=?= <nuno.sa@analog.com>, linux-iio@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Sat, Mar 16, 2024 at 2:57=E2=80=AFPM Andy Shevchenko
<andy.shevchenko@gmail.com> 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 conversi=
on 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 co=
ntroller.
> > + *
> > + * Upon successful return adc->sample.raw will contain the conversion =
result.
> > + */
>
> ...
>
> > +     struct spi_transfer xfers[] =3D {
> > +             {
> > +                     /*
> > +                      * 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 =3D chan->scan_type.realbits,
> > +                     /*
> > +                      * CS is tied to CNV and we need a low to high
> > +                      * transition to start the conversion, so place C=
NV
> > +                      * 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 =3D {
> > +                             .value =3D T_QUIET_NS,
> > +                             .unit =3D SPI_DELAY_UNIT_NSECS,
> > +                     },
> > +
> > +             },
> > +             {
> > +                     .bits_per_word =3D chan->scan_type.realbits,
> > +                     /*
> > +                      * CS has to be high for full conversion time to =
avoid
> > +                      * triggering the busy indication.
> > +                      */
> > +                     .cs_off =3D 1,
> > +                     .delay =3D {
> > +                             .value =3D t_conv_ns,
> > +                             .unit =3D SPI_DELAY_UNIT_NSECS,
> > +                     },
> > +             },
> > +             {
> > +                     /* Then we can read the data during the acquisiti=
on phase */
> > +                     .rx_buf =3D &adc->sample.raw,
> > +                     .len =3D BITS_TO_BYTES(chan->scan_type.storagebit=
s),
> > +                     .bits_per_word =3D chan->scan_type.realbits,
> > +             },
> > +     };
>
> ...
>
> > +     case AD7944_SPI_MODE_SINGLE:
> > +             ret =3D 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) =
=3D=3D 0) {
> > +             ret =3D 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 t=
his.

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 =3D ret;
> > +     } else {
> > +             /* absence of adi,spi-mode property means default mode */
> > +             adc->spi_mode =3D AD7944_SPI_MODE_DEFAULT;
> > +     }
>
> --
> With Best Regards,
> Andy Shevchenko
>
>