| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAMknhBGMUQFoQ9TxTTgy0dxHoyXkt+5tS93tpwz5Wo=h1UQD3Q@mail.gmail.com>
Date: Sat, 16 Mar 2024 18:10:28 -0500
From: David Lechner <dlechner@...libre.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: Jonathan Cameron <jic23@...nel.org>, Michael Hennerich <michael.hennerich@...log.com>,
Nuno Sá <nuno.sa@...log.com>, linux-iio@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] iio: adc: ad7944: Add support for "3-wire mode"
On Sat, Mar 16, 2024 at 2:57 PM Andy Shevchenko
<andy.shevchenko@...il.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 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
>
>
Powered by blists - more mailing lists