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.
Reviewed-by: Nuno Sa <[email protected]>
Signed-off-by: David Lechner <[email protected]>
---
Changes in v2:
- Use default: in case statements.
- Remove redundant else.
- Explain turbo/chain mode check in commit message.
- Link to v1: https://lore.kernel.org/r/20240311-mainline-ad7944-3-wire-mode-v1-1-8e8199efa1f7@baylibre.com
---
drivers/iio/adc/ad7944.c | 157 +++++++++++++++++++++++++++++++++++++++++------
1 file changed, 139 insertions(+), 18 deletions(-)
diff --git a/drivers/iio/adc/ad7944.c b/drivers/iio/adc/ad7944.c
index adb007cdd287..d5ec6b5a41c7 100644
--- a/drivers/iio/adc/ad7944.c
+++ b/drivers/iio/adc/ad7944.c
@@ -32,8 +32,25 @@ struct ad7944_timing_spec {
unsigned int turbo_conv_ns;
};
+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,
+};
+
+/* maps adi,spi-mode property value to enum */
+static const char * const ad7944_spi_modes[] = {
+ [AD7944_SPI_MODE_DEFAULT] = "",
+ [AD7944_SPI_MODE_SINGLE] = "single",
+ [AD7944_SPI_MODE_CHAIN] = "chain",
+};
+
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);
};
+/* quite time before CNV rising edge */
+#define T_QUIET_NS 20
+
static const struct ad7944_timing_spec ad7944_timing_spec = {
.conv_ns = 420,
.turbo_conv_ns = 320,
@@ -110,6 +130,65 @@ AD7944_DEFINE_CHIP_INFO(ad7985, ad7944, 16, 0);
/* fully differential */
AD7944_DEFINE_CHIP_INFO(ad7986, ad7986, 18, 1);
+/*
+ * 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.
+ */
+static int ad7944_3wire_cs_mode_conversion(struct ad7944_adc *adc,
+ const struct iio_chan_spec *chan)
+{
+ unsigned int t_conv_ns = adc->always_turbo ? adc->timing_spec->turbo_conv_ns
+ : adc->timing_spec->conv_ns;
+ struct spi_transfer xfers[] = {
+ {
+ /*
+ * NB: can get better performance from some SPI
+ * controllers if we use the same bits_per_word
+ * in every transfer.
+ */
+ .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.
+ */
+ .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,
+ },
+ };
+
+ return spi_sync_transfer(adc->spi, xfers, ARRAY_SIZE(xfers));
+}
+
/*
* ad7944_4wire_mode_conversion - Perform a 4-wire mode conversion and acquisition
* @adc: The ADC device structure
@@ -167,9 +246,22 @@ static int ad7944_single_conversion(struct ad7944_adc *adc,
{
int ret;
- ret = ad7944_4wire_mode_conversion(adc, chan);
- if (ret)
- return ret;
+ switch (adc->spi_mode) {
+ case AD7944_SPI_MODE_DEFAULT:
+ ret = ad7944_4wire_mode_conversion(adc, chan);
+ if (ret)
+ return ret;
+
+ break;
+ case AD7944_SPI_MODE_SINGLE:
+ ret = ad7944_3wire_cs_mode_conversion(adc, chan);
+ if (ret)
+ return ret;
+
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
if (chan->scan_type.storagebits > 16)
*val = adc->sample.raw.u32;
@@ -230,9 +322,23 @@ static irqreturn_t ad7944_trigger_handler(int irq, void *p)
struct ad7944_adc *adc = iio_priv(indio_dev);
int ret;
- ret = ad7944_4wire_mode_conversion(adc, &indio_dev->channels[0]);
- if (ret)
+ switch (adc->spi_mode) {
+ case AD7944_SPI_MODE_DEFAULT:
+ ret = ad7944_4wire_mode_conversion(adc, &indio_dev->channels[0]);
+ if (ret)
+ goto out;
+
+ break;
+ case AD7944_SPI_MODE_SINGLE:
+ ret = ad7944_3wire_cs_mode_conversion(adc, &indio_dev->channels[0]);
+ if (ret)
+ goto out;
+
+ break;
+ default:
+ /* not supported */
goto out;
+ }
iio_push_to_buffers_with_timestamp(indio_dev, &adc->sample.raw,
pf->timestamp);
@@ -260,16 +366,9 @@ static int ad7944_probe(struct spi_device *spi)
struct ad7944_adc *adc;
bool have_refin = false;
struct regulator *ref;
+ const char *str_val;
int ret;
- /*
- * driver currently only supports the conventional "4-wire" mode and
- * not other special wiring configurations.
- */
- if (device_property_present(dev, "adi,spi-mode"))
- return dev_err_probe(dev, -EINVAL,
- "adi,spi-mode is not currently supported\n");
-
indio_dev = devm_iio_device_alloc(dev, sizeof(*adc));
if (!indio_dev)
return -ENOMEM;
@@ -283,6 +382,22 @@ static int ad7944_probe(struct spi_device *spi)
adc->timing_spec = chip_info->timing_spec;
+ if (device_property_read_string(dev, "adi,spi-mode", &str_val) == 0) {
+ ret = sysfs_match_string(ad7944_spi_modes, str_val);
+ if (ret < 0)
+ return dev_err_probe(dev, -EINVAL,
+ "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;
+ }
+
+ if (adc->spi_mode == AD7944_SPI_MODE_CHAIN)
+ return dev_err_probe(dev, -EINVAL,
+ "chain mode is not implemented\n");
+
/*
* Some chips use unusual word sizes, so check now instead of waiting
* for the first xfer.
@@ -349,15 +464,17 @@ static int ad7944_probe(struct spi_device *spi)
adc->ref_mv = AD7944_INTERNAL_REF_MV;
}
- /*
- * CNV gpio is required in 4-wire mode which is the only currently
- * supported mode.
- */
- adc->cnv = devm_gpiod_get(dev, "cnv", GPIOD_OUT_LOW);
+ adc->cnv = devm_gpiod_get_optional(dev, "cnv", GPIOD_OUT_LOW);
if (IS_ERR(adc->cnv))
return dev_err_probe(dev, PTR_ERR(adc->cnv),
"failed to get CNV GPIO\n");
+ if (!adc->cnv && adc->spi_mode == AD7944_SPI_MODE_DEFAULT)
+ return dev_err_probe(&spi->dev, -EINVAL, "CNV GPIO is required\n");
+ if (adc->cnv && adc->spi_mode != AD7944_SPI_MODE_DEFAULT)
+ return dev_err_probe(&spi->dev, -EINVAL,
+ "CNV GPIO in single and chain mode is not currently supported\n");
+
adc->turbo = devm_gpiod_get_optional(dev, "turbo", GPIOD_OUT_LOW);
if (IS_ERR(adc->turbo))
return dev_err_probe(dev, PTR_ERR(adc->turbo),
@@ -369,6 +486,10 @@ static int ad7944_probe(struct spi_device *spi)
return dev_err_probe(dev, -EINVAL,
"cannot have both turbo-gpios and adi,always-turbo\n");
+ if (adc->spi_mode == AD7944_SPI_MODE_CHAIN && adc->always_turbo)
+ return dev_err_probe(dev, -EINVAL,
+ "cannot have both chain mode and always turbo\n");
+
indio_dev->name = chip_info->name;
indio_dev->modes = INDIO_DIRECT_MODE;
indio_dev->info = &ad7944_iio_info;
---
base-commit: bbafdb305d6b00934cc09a90ec1bb659d43e5171
change-id: 20240311-mainline-ad7944-3-wire-mode-c240fe8af979
On Thu, 14 Mar 2024 12:43:38 -0500
David Lechner <[email protected]> wrote:
> 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.
>
> Reviewed-by: Nuno Sa <[email protected]>
> Signed-off-by: David Lechner <[email protected]>
Applied to the togreg-normal branch of iio.git.
I'll rebase that on rc1 once available before exposing it to linux-next.
Thanks
Jonathan
> ---
> Changes in v2:
> - Use default: in case statements.
> - Remove redundant else.
> - Explain turbo/chain mode check in commit message.
> - Link to v1: https://lore.kernel.org/r/20240311-mainline-ad7944-3-wire-mode-v1-1-8e8199efa1f7@baylibre.com
> ---
> drivers/iio/adc/ad7944.c | 157 +++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 139 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7944.c b/drivers/iio/adc/ad7944.c
> index adb007cdd287..d5ec6b5a41c7 100644
> --- a/drivers/iio/adc/ad7944.c
> +++ b/drivers/iio/adc/ad7944.c
> @@ -32,8 +32,25 @@ struct ad7944_timing_spec {
> unsigned int turbo_conv_ns;
> };
>
> +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,
> +};
> +
> +/* maps adi,spi-mode property value to enum */
> +static const char * const ad7944_spi_modes[] = {
> + [AD7944_SPI_MODE_DEFAULT] = "",
> + [AD7944_SPI_MODE_SINGLE] = "single",
> + [AD7944_SPI_MODE_CHAIN] = "chain",
> +};
> +
> 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);
> };
>
> +/* quite time before CNV rising edge */
> +#define T_QUIET_NS 20
> +
> static const struct ad7944_timing_spec ad7944_timing_spec = {
> .conv_ns = 420,
> .turbo_conv_ns = 320,
> @@ -110,6 +130,65 @@ AD7944_DEFINE_CHIP_INFO(ad7985, ad7944, 16, 0);
> /* fully differential */
> AD7944_DEFINE_CHIP_INFO(ad7986, ad7986, 18, 1);
>
> +/*
> + * 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.
> + */
> +static int ad7944_3wire_cs_mode_conversion(struct ad7944_adc *adc,
> + const struct iio_chan_spec *chan)
> +{
> + unsigned int t_conv_ns = adc->always_turbo ? adc->timing_spec->turbo_conv_ns
> + : adc->timing_spec->conv_ns;
> + struct spi_transfer xfers[] = {
> + {
> + /*
> + * NB: can get better performance from some SPI
> + * controllers if we use the same bits_per_word
> + * in every transfer.
> + */
> + .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.
> + */
> + .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,
> + },
> + };
> +
> + return spi_sync_transfer(adc->spi, xfers, ARRAY_SIZE(xfers));
> +}
> +
> /*
> * ad7944_4wire_mode_conversion - Perform a 4-wire mode conversion and acquisition
> * @adc: The ADC device structure
> @@ -167,9 +246,22 @@ static int ad7944_single_conversion(struct ad7944_adc *adc,
> {
> int ret;
>
> - ret = ad7944_4wire_mode_conversion(adc, chan);
> - if (ret)
> - return ret;
> + switch (adc->spi_mode) {
> + case AD7944_SPI_MODE_DEFAULT:
> + ret = ad7944_4wire_mode_conversion(adc, chan);
> + if (ret)
> + return ret;
> +
> + break;
> + case AD7944_SPI_MODE_SINGLE:
> + ret = ad7944_3wire_cs_mode_conversion(adc, chan);
> + if (ret)
> + return ret;
> +
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
>
> if (chan->scan_type.storagebits > 16)
> *val = adc->sample.raw.u32;
> @@ -230,9 +322,23 @@ static irqreturn_t ad7944_trigger_handler(int irq, void *p)
> struct ad7944_adc *adc = iio_priv(indio_dev);
> int ret;
>
> - ret = ad7944_4wire_mode_conversion(adc, &indio_dev->channels[0]);
> - if (ret)
> + switch (adc->spi_mode) {
> + case AD7944_SPI_MODE_DEFAULT:
> + ret = ad7944_4wire_mode_conversion(adc, &indio_dev->channels[0]);
> + if (ret)
> + goto out;
> +
> + break;
> + case AD7944_SPI_MODE_SINGLE:
> + ret = ad7944_3wire_cs_mode_conversion(adc, &indio_dev->channels[0]);
> + if (ret)
> + goto out;
> +
> + break;
> + default:
> + /* not supported */
> goto out;
> + }
>
> iio_push_to_buffers_with_timestamp(indio_dev, &adc->sample.raw,
> pf->timestamp);
> @@ -260,16 +366,9 @@ static int ad7944_probe(struct spi_device *spi)
> struct ad7944_adc *adc;
> bool have_refin = false;
> struct regulator *ref;
> + const char *str_val;
> int ret;
>
> - /*
> - * driver currently only supports the conventional "4-wire" mode and
> - * not other special wiring configurations.
> - */
> - if (device_property_present(dev, "adi,spi-mode"))
> - return dev_err_probe(dev, -EINVAL,
> - "adi,spi-mode is not currently supported\n");
> -
> indio_dev = devm_iio_device_alloc(dev, sizeof(*adc));
> if (!indio_dev)
> return -ENOMEM;
> @@ -283,6 +382,22 @@ static int ad7944_probe(struct spi_device *spi)
>
> adc->timing_spec = chip_info->timing_spec;
>
> + if (device_property_read_string(dev, "adi,spi-mode", &str_val) == 0) {
> + ret = sysfs_match_string(ad7944_spi_modes, str_val);
> + if (ret < 0)
> + return dev_err_probe(dev, -EINVAL,
> + "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;
> + }
> +
> + if (adc->spi_mode == AD7944_SPI_MODE_CHAIN)
> + return dev_err_probe(dev, -EINVAL,
> + "chain mode is not implemented\n");
> +
> /*
> * Some chips use unusual word sizes, so check now instead of waiting
> * for the first xfer.
> @@ -349,15 +464,17 @@ static int ad7944_probe(struct spi_device *spi)
> adc->ref_mv = AD7944_INTERNAL_REF_MV;
> }
>
> - /*
> - * CNV gpio is required in 4-wire mode which is the only currently
> - * supported mode.
> - */
> - adc->cnv = devm_gpiod_get(dev, "cnv", GPIOD_OUT_LOW);
> + adc->cnv = devm_gpiod_get_optional(dev, "cnv", GPIOD_OUT_LOW);
> if (IS_ERR(adc->cnv))
> return dev_err_probe(dev, PTR_ERR(adc->cnv),
> "failed to get CNV GPIO\n");
>
> + if (!adc->cnv && adc->spi_mode == AD7944_SPI_MODE_DEFAULT)
> + return dev_err_probe(&spi->dev, -EINVAL, "CNV GPIO is required\n");
> + if (adc->cnv && adc->spi_mode != AD7944_SPI_MODE_DEFAULT)
> + return dev_err_probe(&spi->dev, -EINVAL,
> + "CNV GPIO in single and chain mode is not currently supported\n");
> +
> adc->turbo = devm_gpiod_get_optional(dev, "turbo", GPIOD_OUT_LOW);
> if (IS_ERR(adc->turbo))
> return dev_err_probe(dev, PTR_ERR(adc->turbo),
> @@ -369,6 +486,10 @@ static int ad7944_probe(struct spi_device *spi)
> return dev_err_probe(dev, -EINVAL,
> "cannot have both turbo-gpios and adi,always-turbo\n");
>
> + if (adc->spi_mode == AD7944_SPI_MODE_CHAIN && adc->always_turbo)
> + return dev_err_probe(dev, -EINVAL,
> + "cannot have both chain mode and always turbo\n");
> +
> indio_dev->name = chip_info->name;
> indio_dev->modes = INDIO_DIRECT_MODE;
> indio_dev->info = &ad7944_iio_info;
>
> ---
> base-commit: bbafdb305d6b00934cc09a90ec1bb659d43e5171
> change-id: 20240311-mainline-ad7944-3-wire-mode-c240fe8af979
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
On Sat, Mar 16, 2024 at 2:57 PM Andy Shevchenko
<[email protected]> 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
>
>
On Sun, Mar 17, 2024 at 1:10 AM David Lechner <[email protected]> wrote:
> On Sat, Mar 16, 2024 at 2:57 PM Andy Shevchenko
> <[email protected]> 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
On Mon, Mar 18, 2024 at 2:41 PM Jonathan Cameron
<[email protected]> wrote:
> > > 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?
>
> I know this matters for structures where we see lots of them, but do we actually
> care for one offs? Whilst it doesn't matter here I'd focus much more
> on readability and like parameter grouping for cases like this than wasting
> a few bytes.
This is _also_ true, but think more about cache line contamination.
Even not-so-important bytes may decrease the performance. In some
cases it's tolerable, in some it is not (high-speed ADC). In general I
assume that the developer has to understand many aspects of the
software and cache line contamination may be last but definitely not
least.
--
With Best Regards,
Andy Shevchenko
On Mon, Mar 18, 2024 at 8:10 AM Andy Shevchenko
<[email protected]> wrote:
>
> On Mon, Mar 18, 2024 at 2:41 PM Jonathan Cameron
> <[email protected]> wrote:
> > > > 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?
> >
> > I know this matters for structures where we see lots of them, but do we actually
> > care for one offs? Whilst it doesn't matter here I'd focus much more
> > on readability and like parameter grouping for cases like this than wasting
> > a few bytes.
>
> This is _also_ true, but think more about cache line contamination.
> Even not-so-important bytes may decrease the performance. In some
> cases it's tolerable, in some it is not (high-speed ADC). In general I
> assume that the developer has to understand many aspects of the
> software and cache line contamination may be last but definitely not
> least.
Where could someone who doesn't know anything about cache line
contamination learn more about it? (searching the web for that phrase
doesn't turn up much)
On Sun, Mar 17, 2024 at 3:23 AM Andy Shevchenko
<[email protected]> wrote:
>
> On Sun, Mar 17, 2024 at 1:10 AM David Lechner <[email protected]> wrote:
> > On Sat, Mar 16, 2024 at 2:57 PM Andy Shevchenko
> > <[email protected]> wrote:
> > > Thu, Mar 14, 2024 at 12:43:38PM -0500, David Lechner kirjoitti:
>
..
> > > > + 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?
>
Wouldn't this just cause the interrupt handler to trigger again
immediately resulting in very high CPU load? I don't see any other IIO
ADC drivers using the generic triggered buffer returning anything
other than IRQ_HANDLED and I always assumed this was the reason.
On Mon, Mar 18, 2024 at 4:17 PM David Lechner <[email protected]> wrote:
> On Mon, Mar 18, 2024 at 8:10 AM Andy Shevchenko
> <[email protected]> wrote:
> > On Mon, Mar 18, 2024 at 2:41 PM Jonathan Cameron
> > <[email protected]> wrote:
..
> > > > > 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?
> > >
> > > I know this matters for structures where we see lots of them, but do we actually
> > > care for one offs? Whilst it doesn't matter here I'd focus much more
> > > on readability and like parameter grouping for cases like this than wasting
> > > a few bytes.
> >
> > This is _also_ true, but think more about cache line contamination.
> > Even not-so-important bytes may decrease the performance. In some
> > cases it's tolerable, in some it is not (high-speed ADC). In general I
> > assume that the developer has to understand many aspects of the
> > software and cache line contamination may be last but definitely not
> > least.
>
> Where could someone who doesn't know anything about cache line
> contamination learn more about it? (searching the web for that phrase
> doesn't turn up much)
Agree that I have written a rarely used term.
[1]: https://en.wikipedia.org/wiki/Cache_pollution
--
With Best Regards,
Andy Shevchenko
On Mon, Mar 18, 2024 at 02:29:23PM +0000, Jonathan Cameron wrote:
> On Mon, 18 Mar 2024 15:09:32 +0200
> Andy Shevchenko <[email protected]> wrote:
>
> > On Mon, Mar 18, 2024 at 2:41 PM Jonathan Cameron
> > <[email protected]> wrote:
> > > > > 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?
> > >
> > > I know this matters for structures where we see lots of them, but do we actually
> > > care for one offs? Whilst it doesn't matter here I'd focus much more
> > > on readability and like parameter grouping for cases like this than wasting
> > > a few bytes.
> >
> > This is _also_ true, but think more about cache line contamination.
> > Even not-so-important bytes may decrease the performance. In some
> > cases it's tolerable, in some it is not (high-speed ADC). In general I
> > assume that the developer has to understand many aspects of the
> > software and cache line contamination may be last but definitely not
> > least.
> >
>
> Not totally sure what you are covering with contamination as many aspects
> around caches and that's not really a standard term for any of them (as
> far as I know).
>
> It's part of a multi cacheline allocation anyway (because it's tacked on the
> end of the iio device struct, so fairly unlikely to share with other allocations
> and definitely not on ARM because of the trailing __aligned(IIO_DMA_MINALIGN)
> elements.
>
> If it matters more locally, then pahole is more likely to push you to pack
You mean 'pahole --reorganize', right? Yeah, I need to take into account
explicit __attribute__((__aligned__)) at the start of cachelines as a
hint that the fields in a cacheline can't be moved outside of that
cacheline or plain leave that cacheline members alone, as-is.
I also need to get perf's data-type profiling as an input for 'pahole
--reorganize', with that we may take into account the existing
__aligned__ markings and combine it with what we get from data-type
profiling.
- Arnaldo
> things together in a fashion that makes false sharing and similar perf issues
> more likely if you are grouping things for packing purposes rather than
> logical groups.
>
> If you just mean cache pressure then fair enough if we squeeze everything into
> one cacheline and that doesn't cause false sharing.
> 'Maybe' this will fit on x86. On Arm64 it's not going to
> make any difference, just moving the padding around a bit within the line.
>
> So I'd argue premature optimization for a small, one off, structure.
>
> Jonathan
>
>
>
On Mon, 18 Mar 2024 15:09:32 +0200
Andy Shevchenko <[email protected]> wrote:
> On Mon, Mar 18, 2024 at 2:41 PM Jonathan Cameron
> <[email protected]> wrote:
> > > > 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?
> >
> > I know this matters for structures where we see lots of them, but do we actually
> > care for one offs? Whilst it doesn't matter here I'd focus much more
> > on readability and like parameter grouping for cases like this than wasting
> > a few bytes.
>
> This is _also_ true, but think more about cache line contamination.
> Even not-so-important bytes may decrease the performance. In some
> cases it's tolerable, in some it is not (high-speed ADC). In general I
> assume that the developer has to understand many aspects of the
> software and cache line contamination may be last but definitely not
> least.
>
Not totally sure what you are covering with contamination as many aspects
around caches and that's not really a standard term for any of them (as
far as I know).
It's part of a multi cacheline allocation anyway (because it's tacked on the
end of the iio device struct, so fairly unlikely to share with other allocations
and definitely not on ARM because of the trailing __aligned(IIO_DMA_MINALIGN)
elements.
If it matters more locally, then pahole is more likely to push you to pack
things together in a fashion that makes false sharing and similar perf issues
more likely if you are grouping things for packing purposes rather than
logical groups.
If you just mean cache pressure then fair enough if we squeeze everything into
one cacheline and that doesn't cause false sharing.
'Maybe' this will fit on x86. On Arm64 it's not going to
make any difference, just moving the padding around a bit within the line.
So I'd argue premature optimization for a small, one off, structure.
Jonathan
>
> > 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?
I know this matters for structures where we see lots of them, but do we actually
care for one offs? Whilst it doesn't matter here I'd focus much more
on readability and like parameter grouping for cases like this than wasting
a few bytes.
Jonathan
On Mon, 18 Mar 2024 09:33:44 -0500
David Lechner <[email protected]> wrote:
> On Sun, Mar 17, 2024 at 3:23 AM Andy Shevchenko
> <[email protected]> wrote:
> >
> > On Sun, Mar 17, 2024 at 1:10 AM David Lechner <[email protected]> wrote:
> > > On Sat, Mar 16, 2024 at 2:57 PM Andy Shevchenko
> > > <[email protected]> wrote:
> > > > Thu, Mar 14, 2024 at 12:43:38PM -0500, David Lechner kirjoitti:
> >
>
> ...
>
> > > > > + 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?
> >
>
> Wouldn't this just cause the interrupt handler to trigger again
> immediately resulting in very high CPU load? I don't see any other IIO
> ADC drivers using the generic triggered buffer returning anything
> other than IRQ_HANDLED and I always assumed this was the reason.
>
To me, this is a long running 'open question' of what to do on error
in an interrupt. I my mind at least there isn't a good solution so we tend
to just paper over it.
IRQ_NONE indicates it wasn't our interrupt - so if we check a status
register and none of the interrupt bits are set, then it is either
a shared interrupt in which case someone else will handle it, or it
is spurious and we want the spurious interrupt handler to deal with it.
If we get an error talking tot he device during an interrupt handler
the question is do we want to trigger again? If it's a level interrupt
and we haven't cleared it we will anyway, but for edge interrupts we've
handled it as well as we can perhaps.
Anyhow, it's all a bit unclear so given we don't expect to get these
errors I tend to more prefer IRQ_HANDLED but if someone argues
strongly for IRQ_NONE I don't push back too hard.
If anyone has clear guidance on this then please link to it!
Jonathan