| Message ID | 27cccb51cc56f1bb57cb06d279854a503d779e25.1748447035.git.Jonathan.Santos@analog.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v9,01/12] device property: add fwnode_find_reference_args() | expand |
On Thu, 29 May 2025 19:50:29 -0300 Jonathan Santos <Jonathan.Santos@analog.com> 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 the SYNC_IN pin. > > Use trigger-sources property to enable device synchronization over SPI > and multi-device synchronization while replacing sync-in-gpios property. > > Reviewed-by: David Lechner <dlechner@baylibre.com> > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com> A couple of trivial comments. Not enough to respin unless something else comes up. > @@ -296,6 +301,27 @@ static const struct regmap_config ad7768_regmap24_config = { > .max_register = AD7768_REG24_COEFF_DATA, > }; > > +static int ad7768_send_sync_pulse(struct ad7768_state *st) > +{ > + if (st->en_spi_sync) > + return regmap_write(st->regmap, AD7768_REG_SYNC_RESET, 0x00); > + > + /* > + * The datasheet specifies a minimum SYNC_IN pulse width of 1.5 × Tmclk, > + * where Tmclk is the MCLK period. The supported MCLK frequencies range > + * from 0.6 MHz to 17 MHz, which corresponds to a minimum SYNC_IN pulse > + * width of approximately 2.5 µs in the worst-case scenario (0.6 MHz). > + * > + * Add a delay to ensure the pulse width is always sufficient to > + * trigger synchronization. > + */ > + gpiod_set_value_cansleep(st->gpio_sync_in, 1); > + fsleep(3); > + gpiod_set_value_cansleep(st->gpio_sync_in, 0); This change + comment should really have been in a separate patch as there is always the potential someone might want to backport it. > + > + return 0; > +} > + > static int ad7768_set_mode(struct ad7768_state *st, > enum ad7768_conv_mode mode) > { > @@ -392,10 +418,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_trigger_sources_get_sync(struct device *dev, > + struct ad7768_state *st) > +{ > + struct fwnode_handle *dev_fwnode = dev_fwnode(dev); > + > + /* > + * The AD7768-1 allows two primary methods for driving the SYNC_IN pin > + * to synchronize one or more devices: > + * 1. Using an external GPIO. > + * 2. Using a SPI command, where the SYNC_OUT pin generates a > + * synchronization pulse that drives the SYNC_IN pin. > + */ > + if (fwnode_property_present(dev_fwnode, "trigger-sources")) > + return ad7768_trigger_sources_sync_setup(dev, dev_fwnode, st); > + > + /* > + * In the absence of trigger-sources property, enable self > + * synchronization over SPI (SYNC_OUT). > + */ > + st->en_spi_sync = true; Really trivial but if you respin for some reason blank line here. > + return 0; > +}
On Fri, 30 May 2025 20:45:32 +0300 Andy Shevchenko <andy@kernel.org> wrote: > On Thu, May 29, 2025 at 07:50:29PM -0300, 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 the SYNC_IN pin. > > > > Use trigger-sources property to enable device synchronization over SPI > > and multi-device synchronization while replacing sync-in-gpios property. > > ... > > > struct ad7768_state { > > > struct iio_trigger *trig; > > struct gpio_desc *gpio_sync_in; > > struct gpio_desc *gpio_reset; > > > + bool en_spi_sync; > > I'm wondering if moving this... > > > const char *labels[ARRAY_SIZE(ad7768_channels)]; > > struct gpio_chip gpiochip; > > ...to here saves a few bytes in accordance to `pahole`. > > > }; > > ... > > > +static int ad7768_trigger_sources_sync_setup(struct device *dev, > > + struct fwnode_handle *dev_fwnode, > > + struct ad7768_state *st) > > +{ > > + struct fwnode_reference_args args; > > + > > + struct fwnode_handle *fwnode __free(fwnode_handle) = > > + fwnode_find_reference_args(dev_fwnode, "trigger-sources", > > + "#trigger-source-cells", 0, > > + AD7768_TRIGGER_SOURCE_SYNC_IDX, &args); > > I don't see how args are being used. This puts in doubt the need of the first > patch. That did get discussed (more context needed in the commit message for patch 1). I wasn't happy with ignoring #trigger-source-cells which is required in the binding but here is known to be 0. If it was larger than 0 but we didn't care about the arguments I believe we'd still need to use this call to take the right stride through the data array that this is coming from. Ultimately I think that is this bit of code establishing the end of the phandle. https://elixir.bootlin.com/linux/v6.15/source/drivers/of/base.c#L1300 I might have gotten it wrong how this all works though! J
diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c index 6a409a46773c..01da1c2a138c 100644 --- a/drivers/iio/adc/ad7768-1.c +++ b/drivers/iio/adc/ad7768-1.c @@ -29,6 +29,8 @@ #include <linux/iio/triggered_buffer.h> #include <linux/iio/trigger_consumer.h> +#include <dt-bindings/iio/adc/adi,ad7768-1.h> + /* AD7768 registers definition */ #define AD7768_REG_CHIP_TYPE 0x3 #define AD7768_REG_PROD_ID_L 0x4 @@ -101,6 +103,8 @@ #define AD7768_VCM_OFF 0x07 +#define AD7768_TRIGGER_SOURCE_SYNC_IDX 0 + enum ad7768_conv_mode { AD7768_CONTINUOUS, AD7768_ONE_SHOT, @@ -209,6 +213,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)]; struct gpio_chip gpiochip; /* @@ -296,6 +301,27 @@ static const struct regmap_config ad7768_regmap24_config = { .max_register = AD7768_REG24_COEFF_DATA, }; +static int ad7768_send_sync_pulse(struct ad7768_state *st) +{ + if (st->en_spi_sync) + return regmap_write(st->regmap, AD7768_REG_SYNC_RESET, 0x00); + + /* + * The datasheet specifies a minimum SYNC_IN pulse width of 1.5 × Tmclk, + * where Tmclk is the MCLK period. The supported MCLK frequencies range + * from 0.6 MHz to 17 MHz, which corresponds to a minimum SYNC_IN pulse + * width of approximately 2.5 µs in the worst-case scenario (0.6 MHz). + * + * Add a delay to ensure the pulse width is always sufficient to + * trigger synchronization. + */ + gpiod_set_value_cansleep(st->gpio_sync_in, 1); + fsleep(3); + 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) { @@ -392,10 +418,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) @@ -672,6 +695,62 @@ static const struct iio_info ad7768_info = { .debugfs_reg_access = &ad7768_reg_access, }; +static int ad7768_trigger_sources_sync_setup(struct device *dev, + struct fwnode_handle *dev_fwnode, + struct ad7768_state *st) +{ + struct fwnode_reference_args args; + + struct fwnode_handle *fwnode __free(fwnode_handle) = + fwnode_find_reference_args(dev_fwnode, "trigger-sources", + "#trigger-source-cells", 0, + AD7768_TRIGGER_SOURCE_SYNC_IDX, &args); + if (IS_ERR(fwnode)) + return PTR_ERR(fwnode); + + /* First, try getting the GPIO trigger source */ + if (fwnode_device_is_compatible(fwnode, "gpio-trigger")) { + st->gpio_sync_in = devm_fwnode_gpiod_get_index(dev, fwnode, + NULL, + 0, + GPIOD_OUT_LOW, + "sync-in"); + return PTR_ERR_OR_ZERO(st->gpio_sync_in); + } + + /* + * TODO: Support the other cases when we have a trigger subsystem + * to reliably handle other types of devices as trigger sources. + * + * For now, return an error message. For self triggering, omit the + * trigger-sources property. + */ + return dev_err_probe(dev, -EOPNOTSUPP, "Invalid synchronization trigger source\n"); +} + +static int ad7768_trigger_sources_get_sync(struct device *dev, + struct ad7768_state *st) +{ + struct fwnode_handle *dev_fwnode = dev_fwnode(dev); + + /* + * The AD7768-1 allows two primary methods for driving the SYNC_IN pin + * to synchronize one or more devices: + * 1. Using an external GPIO. + * 2. Using a SPI command, where the SYNC_OUT pin generates a + * synchronization pulse that drives the SYNC_IN pin. + */ + if (fwnode_property_present(dev_fwnode, "trigger-sources")) + return ad7768_trigger_sources_sync_setup(dev, dev_fwnode, st); + + /* + * In the absence of trigger-sources property, enable self + * synchronization over SPI (SYNC_OUT). + */ + st->en_spi_sync = true; + return 0; +} + static int ad7768_setup(struct iio_dev *indio_dev) { struct ad7768_state *st = iio_priv(indio_dev); @@ -702,11 +781,22 @@ 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); + /* For backwards compatibility, try the adi,sync-in-gpios property */ + st->gpio_sync_in = devm_gpiod_get_optional(&st->spi->dev, "adi,sync-in", + GPIOD_OUT_LOW); if (IS_ERR(st->gpio_sync_in)) return PTR_ERR(st->gpio_sync_in); + /* + * If the synchronization is not defined by adi,sync-in-gpios, try the + * trigger-sources. + */ + if (!st->gpio_sync_in) { + ret = ad7768_trigger_sources_get_sync(&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")) { ret = ad7768_gpio_init(indio_dev);