| 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: <f90874bb-f08f-437e-bc8b-d549b888ff76@baylibre.com>
Date: Mon, 27 Jan 2025 18:08:34 -0600
From: David Lechner <dlechner@...libre.com>
To: Jonathan Santos <Jonathan.Santos@...log.com>, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Cc: lars@...afoo.de, Michael.Hennerich@...log.com,
marcelo.schmitt@...log.com, jic23@...nel.org, robh@...nel.org,
krzk+dt@...nel.org, conor+dt@...nel.org, jonath4nns@...il.com,
marcelo.schmitt1@...il.com
Subject: Re: [PATCH v2 14/16] iio: adc: ad7768-1: add support for
Synchronization over SPI
On 1/27/25 9:14 AM, Jonathan Santos wrote:
> The synchronization method using GPIO requires the generated pulse to be
> truly synchronous with the base MCLK signal. When it is not possible to
> do that in hardware, the datasheet recommends using synchronization over
> SPI, where the generated pulse is already synchronous with MCLK. This
> requires the SYNC_OUT pin to be connected to SYNC_IN pin. In
> multidevices setup, the SYNC_OUT from other devices can be used as
> synchronization source.
>
> Use trigger-sources property to enable device synchronization over SPI
> for single and multiple devices.
>
> Signed-off-by: Jonathan Santos <Jonathan.Santos@...log.com>
> ---
> v2 Changes:
> * Synchronization via SPI is enabled when the Sync GPIO is not defined.
> * now trigger-sources property indicates the synchronization provider or
> main device. The main device will be used to drive the SYNC_IN when
> requested (via GPIO or SPI).
> ---
> drivers/iio/adc/ad7768-1.c | 81 ++++++++++++++++++++++++++++++++++----
> 1 file changed, 73 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> index 7686556c7808..01ccbe0aa708 100644
> --- a/drivers/iio/adc/ad7768-1.c
> +++ b/drivers/iio/adc/ad7768-1.c
> @@ -193,6 +193,7 @@ static const struct iio_chan_spec ad7768_channels[] = {
>
> struct ad7768_state {
> struct spi_device *spi;
> + struct spi_device *sync_source_spi;
> struct regmap *regmap;
> struct regulator *vref;
> struct mutex lock;
> @@ -206,6 +207,7 @@ struct ad7768_state {
> struct iio_trigger *trig;
> struct gpio_desc *gpio_sync_in;
> struct gpio_desc *gpio_reset;
> + bool en_spi_sync;
> const char *labels[ARRAY_SIZE(ad7768_channels)];
> /*
> * DMA (thus cache coherency maintenance) may require the
> @@ -264,6 +266,21 @@ static int ad7768_spi_reg_write(void *context,
> return spi_write(st->spi, st->data.d8, 2);
> }
>
> +static int ad7768_send_sync_pulse(struct ad7768_state *st)
> +{
> + if (st->en_spi_sync) {
> + st->data.d8[0] = AD7768_WR_FLAG_MSK(AD7768_REG_SYNC_RESET);
> + st->data.d8[1] = 0x00;
> +
> + return spi_write(st->sync_source_spi, st->data.d8, 2);
> + } else if (st->gpio_sync_in) {
Redundant else after return.
> + gpiod_set_value_cansleep(st->gpio_sync_in, 1);
> + gpiod_set_value_cansleep(st->gpio_sync_in, 0);
> + }
> +
> + return 0;
> +}
> +
> static int ad7768_set_mode(struct ad7768_state *st,
> enum ad7768_conv_mode mode)
> {
> @@ -348,10 +365,7 @@ static int ad7768_set_dig_fil(struct ad7768_state *st,
> return ret;
>
> /* A sync-in pulse is required every time the filter dec rate changes */
> - gpiod_set_value(st->gpio_sync_in, 1);
> - gpiod_set_value(st->gpio_sync_in, 0);
> -
> - return 0;
> + return ad7768_send_sync_pulse(st);
> }
>
> static int ad7768_gpio_direction_input(struct gpio_chip *chip, unsigned int offset)
> @@ -638,6 +652,58 @@ static const struct iio_info ad7768_info = {
> .debugfs_reg_access = &ad7768_reg_access,
> };
>
> +static int ad7768_parse_trigger_sources(struct device *dev, struct ad7768_state *st)
> +{
> + struct fwnode_reference_args args;
> + int ret;
> +
> + /*
> + * The AD7768-1 allows two primary methods for driving the SYNC_IN pin to synchronize
> + * one or more devices:
> + * 1. Using a GPIO to directly drive the SYNC_IN pin.
> + * 2. Using a SPI command, where the SYNC_OUT pin generates a synchronization pulse
> + * that loops back to the SYNC_IN pin.
> + *
> + * In multi-device setups, the SYNC_IN pin can also be driven by the SYNC_OUT pin of
> + * another device. To support this, we use the "trigger-source" property to get a
> + * reference to the "main" device responsible for generating the synchronization pulse.
> + * In a single-device setup, the "trigger-source" property should reference the device's
> + * own node.
> + */
> + ret = fwnode_property_get_reference_args(dev_fwnode(dev), "trigger-sources",
> + "#trigger-source-cells", 0, 0, &args);
The trigger-sources property is optional, so we should have a special case for
ret == -EINVAL here. Otherwise existing users that use the gpio binding will
fail here.
> + if (ret) {
> + dev_err(dev, "Failed to get trigger-sources reference: %d\n", ret);
> + return ret;
return dev_err_probe(...);
> + }
> +
> + st->gpio_sync_in = devm_gpiod_get_optional(args.fwnode->dev, "adi,sync-in",
> + GPIOD_OUT_LOW);
I would put this in the -EINVAL special case mentioned above, then we don't
nee to use the _optional variant.
> + if (IS_ERR(st->gpio_sync_in))
> + return PTR_ERR(st->gpio_sync_in);
This leaks args.fwnode reference on return (but would be fixed if making the
above suggested change).
> +
> + /*
> + * If the SYNC_IN GPIO is not defined, fall back to synchronization over SPI.
> + * Use the trigger-source reference to identify the main SPI device for generating
> + * the synchronization pulse.
> + */
> + if (!st->gpio_sync_in) {
> + st->sync_source_spi = to_spi_device(args.fwnode->dev);
This seems unsafe. The DT could be pointing to something other than a SPI device.
to_spi_device() calls container_of() so would cause undefined behavior if dev
was something else. Also, even if it is a spi device, we don't know what state
it is in and if it is safe to use it.
I think it would be sufficient for now to just check that args.fw_node is the
the same one as this SPI device since that is the supported case.
> + if (!st->sync_source_spi) {
> + dev_err(dev,
> + "Synchronization setup failed. GPIO is undefined and trigger-source reference is invalid\n");
> + return -EINVAL;
Leaks args.fwnode on return. Also, dev_err_probe().
> + }
> +
> + st->en_spi_sync = true;
> + }
> +
> + /* Release the fwnode reference after use */
> + fwnode_handle_put(args.fwnode);
> +
> + return 0;
> +}
> +
> static int ad7768_setup(struct iio_dev *indio_dev)
> {
> struct ad7768_state *st = iio_priv(indio_dev);
> @@ -668,10 +734,9 @@ static int ad7768_setup(struct iio_dev *indio_dev)
> return ret;
> }
>
> - st->gpio_sync_in = devm_gpiod_get(&st->spi->dev, "adi,sync-in",
> - GPIOD_OUT_LOW);
> - if (IS_ERR(st->gpio_sync_in))
> - return PTR_ERR(st->gpio_sync_in);
> + ret = ad7768_parse_trigger_sources(&st->spi->dev, st);
> + if (ret)
> + return ret;
>
> /* Only create a Chip GPIO if flagged for it */
> if (device_property_read_bool(&st->spi->dev, "gpio-controller")) {
Powered by blists - more mailing lists