| Message ID | cover.1748447035.git.Jonathan.Santos@analog.com |
|---|---|
| Headers | show |
| Series | iio: adc: ad7768-1: Add features, improvements, and fixes | 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 Thu, 29 May 2025 19:47:57 -0300 Jonathan Santos <Jonathan.Santos@analog.com> wrote: > This patch series introduces some new features, improvements, > and fixes for the AD7768-1 ADC driver. > > The goal is to support all key functionalities listed in the device > datasheet, including filter mode selection, common mode voltage output > configuration and GPIO support. Additionally, this includes fixes > for SPI communication and for IIO interface, and also code improvements > to enhance maintainability and readability. Looks good to me. I'll be waiting on at least some tags on patch 1 though before applying this. Thanks, Jonathan
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. > + 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); Call it just fwnode. > + /* > + * 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; > +}
On Thu, May 29, 2025 at 07:48:17PM -0300, Jonathan Santos wrote: > Similarly to fwnode_find_reference(), find and return a named reference > to a fwnode handle, including its arguments. This wrapper facilitates > cleanup handling and improves readability. In principle I'm okay with the code, but I probably missed how this new API is being used (in terms of a need of those arguments to be retrieved).
On Thu, May 29, 2025 at 07:47:57PM -0300, Jonathan Santos wrote: > This patch series introduces some new features, improvements, > and fixes for the AD7768-1 ADC driver. > > The goal is to support all key functionalities listed in the device > datasheet, including filter mode selection, common mode voltage output > configuration and GPIO support. Additionally, this includes fixes > for SPI communication and for IIO interface, and also code improvements > to enhance maintainability and readability. If you haven't run `make W=1 ...` build against this driver, do it with GCC and clang and fix all respective compilation warnings / errors, if any.
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
On Fri, 30 May 2025 20:48:14 +0300 Andy Shevchenko <andy@kernel.org> wrote: > On Thu, May 29, 2025 at 07:48:17PM -0300, Jonathan Santos wrote: > > Similarly to fwnode_find_reference(), find and return a named reference > > to a fwnode handle, including its arguments. This wrapper facilitates > > cleanup handling and improves readability. > > In principle I'm okay with the code, but I probably missed how this new API is > being used (in terms of a need of those arguments to be retrieved). > Good point - this needs a little more justification, particularly with respect to why we are using it in this driver. J
On 05/30, Andy Shevchenko wrote: > On Thu, May 29, 2025 at 07:50:29PM -0300, Jonathan Santos wrote: > > +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. > If the wrapper is the issue, maybe it is better doing this instead? 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) = NULL; int ret; ret = fwnode_property_get_reference_args(dev_fwnode, "trigger-sources", "#trigger-source-cells", 0, AD7768_TRIGGER_SOURCE_SYNC_IDX, &args); if (ret) return ret; fwnode = args.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"); > > +} Then we can get rid of the first patch. Regards, Jonathan S.
Mon, Jun 02, 2025 at 02:15:23PM -0300, Jonathan Santos kirjoitti: > On 05/30, Andy Shevchenko wrote: > > On Thu, May 29, 2025 at 07:50:29PM -0300, Jonathan Santos wrote: ... > > > +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. > > If the wrapper is the issue, maybe it is better doing this instead? > > static int ad7768_trigger_sources_sync_setup(struct device *dev, > struct fwnode_handle *dev_fwnode, Name it fwnode... > struct ad7768_state *st) > { > struct fwnode_reference_args args; > struct fwnode_handle *fwnode __free(fwnode_handle) = NULL; ...and this one something like 'ref'. Also I'm not sure the __free() will do what you expect from it. > int ret; > > ret = fwnode_property_get_reference_args(dev_fwnode, "trigger-sources", > "#trigger-source-cells", 0, > AD7768_TRIGGER_SOURCE_SYNC_IDX, &args); > if (ret) > return ret; > > fwnode = args.fwnode; Yes, please. This looks better. > > > + /* 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"); > > > +} > > Then we can get rid of the first patch.
On Tue, 3 Jun 2025 00:25:40 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > Mon, Jun 02, 2025 at 02:15:23PM -0300, Jonathan Santos kirjoitti: > > On 05/30, Andy Shevchenko wrote: > > > On Thu, May 29, 2025 at 07:50:29PM -0300, Jonathan Santos wrote: > > ... > > > > > +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. > > > > If the wrapper is the issue, maybe it is better doing this instead? > > > > static int ad7768_trigger_sources_sync_setup(struct device *dev, > > struct fwnode_handle *dev_fwnode, > > Name it fwnode... > > > struct ad7768_state *st) > > { > > struct fwnode_reference_args args; > > struct fwnode_handle *fwnode __free(fwnode_handle) = NULL; > > ...and this one something like 'ref'. Also I'm not sure the __free() will do > what you expect from it. > > > int ret; > > > > ret = fwnode_property_get_reference_args(dev_fwnode, "trigger-sources", > > "#trigger-source-cells", 0, > > AD7768_TRIGGER_SOURCE_SYNC_IDX, &args); > > if (ret) > > return ret; > > > > fwnode = args.fwnode; > > Yes, please. This looks better. Take a look at the guidance in cleanup.h first. Linus made it very clear that the = NULL then set it later pattern was not the way to go. That was one of the things buried deep in a discussion thread soon after the cleanup.h stuff was proposed and the lack of general knowledge of that preference lead to Dan adding that documentation to cleanup.h. If __free is used Linus always wants to see the constructor and destructor in the same line. That is why I suggested the approach in patch 1. If that isn't acceptable (yet), wrap this up in a local function doing the equivalent of patch 1 and we can wait and see how common this ends up being. Jonathan > > > > > + /* 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"); > > > > +} > > > > Then we can get rid of the first patch. >