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?
> +};
...
> 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?
...
> +/*
The below is mimicing the kernel doc, but there is no marker for this.
Why?
> + * 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.
> + */
> + .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?
> 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?
> + if (ret < 0)
> + return dev_err_probe(dev, -EINVAL,
Why shadowing the error code?
> + "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
