| Message ID | cover.1745605382.git.Jonathan.Santos@analog.com |
|---|---|
| Headers | show |
| Series | iio: adc: ad7768-1: Add features, improvements, and fixes | expand |
On Mon, Apr 28, 2025 at 3:14 AM Jonathan Santos <Jonathan.Santos@analog.com> wrote: > > Separate filter type and decimation rate from the sampling frequency > attribute. The new filter type attribute enables sinc3, sinc3+rej60 > and wideband filters, which were previously unavailable. > > Previously, combining decimation and MCLK divider in the sampling > frequency obscured performance trade-offs. Lower MCLK divider > settings increase power usage, while lower decimation rates reduce > precision by decreasing averaging. By creating an oversampling > attribute, which controls the decimation, users gain finer control > over performance. > > The addition of those attributes allows a wider range of sampling > frequencies and more access to the device features. Sampling frequency > table is updated after every digital filter parameter change. ... > +static int ad7768_get_filter_type_attr(struct iio_dev *dev, > + const struct iio_chan_spec *chan); > +static int ad7768_set_fil_type_attr(struct iio_dev *dev, > + const struct iio_chan_spec *chan, unsigned int filter); Would it be possible to avoid forward declarations? ... > static void ad7768_fill_samp_freq_tbl(struct ad7768_state *st) > { > - int i; > + int i, freq_filtered, len = 0; Should 'i' be signed? > + > + freq_filtered = DIV_ROUND_CLOSEST(st->mclk_freq, st->oversampling_ratio); > + for (i = 0; i < ARRAY_SIZE(ad7768_mclk_div_rates); i++) { > + st->samp_freq_avail[len] = DIV_ROUND_CLOSEST(freq_filtered, > + ad7768_mclk_div_rates[i]); > + /* Sampling frequency cannot be lower than the minimum of 50 SPS */ > + if (st->samp_freq_avail[len] >= 50) > + len++; > + } > + st->samp_freq_avail_len = len; > +} ... > + dec_rate = clamp_t(unsigned int, dec_rate, 32, max_dec_rate); Can't clamp() be used? _t variants are not very good and may play a bad joke on subtle cases. ... > + /* > + * Decimations 8 (idx 0) and 16 (idx 1) are set in the > + * FILTER[6:4] field. The other decimations are set in the > + * DEC_RATE[2:0] field, and the idx need to be offsetted by two. needs > + */ ... > + /* > + * The decimation for SINC3 filters are configured in different > + * registers Missing period at the end. > + */
On 4/27/25 7:12 PM, Jonathan Santos wrote: > In addition to GPIO synchronization, The AD7768-1 also supports > synchronization over SPI, which use is recommended when the GPIO > cannot provide a pulse synchronous with the base MCLK signal. It > consists of looping back the SYNC_OUT to the SYNC_IN pin and send > a command via SPI to trigger the synchronization. > > Introduce the 'trigger-sources' property to enable SPI-based > synchronization via SYNC_OUT pin, along with additional optional > entries for GPIO3 and DRDY pins. > > Also create #trigger-source-cells property to differentiate the trigger > sources provided by the ADC. To improve readability, create a > adi,ad7768-1.h header with the macros for the cell values. > > While at it, add description to the interrupts property. > > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com> > --- ... > --- > .../bindings/iio/adc/adi,ad7768-1.yaml | 31 ++++++++++++++++++- > include/dt-bindings/iio/adc/adi,ad7768-1.h | 10 ++++++ > 2 files changed, 40 insertions(+), 1 deletion(-) > create mode 100644 include/dt-bindings/iio/adc/adi,ad7768-1.h We should add this new file to the MAINTAINERS entry. Otherwise: Reviewed-by: David Lechner <dlechner@baylirbe.com>
On 4/27/25 7:13 PM, 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. > > Use trigger-sources property to enable device synchronization over SPI > and multi-device synchronization, as an alternative to adi,sync-in-gpios > property. > > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com> > --- ... > +static int ad7768_trigger_sources_get_sync(struct device *dev, > + struct ad7768_state *st) > +{ > + struct fwnode_reference_args args; > + struct fwnode_handle *fwnode = NULL; > + int ret; > + > + /* > + * 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 (!device_property_present(dev, "trigger-sources")) { > + /* > + * In the absence of trigger-sources property, enable self > + * synchronization over SPI (SYNC_OUT). > + */ > + st->en_spi_sync = true; > + return 0; > + } > + > + ret = fwnode_property_get_reference_args(dev_fwnode(dev), > + "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 and fallback to > + * synchronization over SPI in case of failure. > + */ > + st->gpio_sync_in = ad7768_trigger_source_get_gpio(dev, fwnode); > + if (IS_ERR(st->gpio_sync_in)) { Normally, having error be the indented path like this is preferred, but I think this case is an exception since the following is "normal" code path, not error return path. I would understand this better as: st->gpio_sync_in = ad7768_trigger_source_get_gpio(dev, fwnode); if (!IS_ERR(st->gpio_sync_in)) /* The trigger is a GPIO, our job is done here. */ goto out_put_node; /* Second, ... */ > + /* > + * For this case, it requires one argument, which indicates the > + * output pin referenced. > + */ > + if (args.nargs < 1) > + goto err_not_supp; > + > + if (args.args[0] != AD7768_TRIGGER_SOURCE_SYNC_OUT) > + goto err_not_supp; > + > + /* > + * Only self trigger is supported for now, i.e., > + * external SYNC_OUT is not allowed. > + */ > + if (fwnode->dev == dev) { As Andy pointed out, this is a bit odd. Technically, we should be allowing any trigger provider with the right capabilities. But we don't have a trigger subsystem yet to make that easy. Since we can already handle the SYNC_IN is wired to SYNC_OUT of the same chip above by omitting the trigger-sources property, I would just make a TODO here and always return the not supported error for now. > + st->en_spi_sync = true; > + goto out_put_node; > + } > + > + goto err_not_supp; > + } > + > + goto out_put_node; > + > +err_not_supp: > + ret = dev_err_probe(dev, -EOPNOTSUPP, > + "Invalid synchronization trigger source"); > +out_put_node: > + fwnode_handle_put(args.fwnode); > + return ret; > +} > +
On 04/28, Andy Shevchenko wrote: > On Mon, Apr 28, 2025 at 3:13 AM Jonathan Santos > <Jonathan.Santos@analog.com> wrote: > > > > The AD7768-1 has the ability to control other local hardware (such as gain > > stages),to power down other blocks in the signal chain, or read local > > status signals over the SPI interface. > > > > Add direct mode conditional locks in the gpio callbacks to prevent register > > access when the device is in buffered mode. > > > > This change exports the AD7768-1's four gpios and makes them accessible > > at an upper layer. > > ... > > > +#include <linux/gpio.h> > > No way. This header must not be in any of the code. (Yes, there are > leftovers in the kernel, but work is ongoing to clean that up) > Sorry about that, will fix it. > > +#include <linux/gpio/driver.h> > > #include <linux/gpio/consumer.h> > > > #include <linux/kernel.h> > > And since you are doing the big series for the driver, please drop > this header and replace it (if required) with what is used. No driver > code should use kernel.h. > Sure, noted. > > #include <linux/module.h> > > ... > > > struct ad7768_state { > > > struct regulator_dev *vcm_rdev; > > unsigned int vcm_output_sel; > > struct clk *mclk; > > + struct gpio_chip gpiochip; > > unsigned int mclk_freq; > > unsigned int samp_freq; > > struct completion completion; > > Btw, have you run `pahole`? Is this the best place for a new field in > accordance with its output? > I didn't, but I am going to use it. > ... > > > +static int ad7768_gpio_set(struct gpio_chip *chip, unsigned int offset, int value) > > +{ > > + struct iio_dev *indio_dev = gpiochip_get_data(chip); > > + struct ad7768_state *st = iio_priv(indio_dev); > > + unsigned int val; > > + int ret; > > + > > + if (!iio_device_claim_direct(indio_dev)) > > + return -EBUSY; > > + > > + ret = regmap_read(st->regmap, AD7768_REG_GPIO_CONTROL, &val); > > + if (ret) > > + goto err_release; > > + > > + if (val & BIT(offset)) > > + ret = regmap_update_bits(st->regmap, AD7768_REG_GPIO_WRITE, > > + BIT(offset), value << offset); > > And if value happens to be > 1? > Also consider the use of regmap_assign_bits(). > ok! > > +err_release: > > + iio_device_release_direct(indio_dev); > > + > > + return ret; > > +} > > ... > > > +static int ad7768_gpio_init(struct iio_dev *indio_dev) > > +{ > > + struct ad7768_state *st = iio_priv(indio_dev); > > + int ret; > > + > > + ret = regmap_write(st->regmap, AD7768_REG_GPIO_CONTROL, > > + AD7768_GPIO_UNIVERSAL_EN); > > + if (ret) > > + return ret; > > + > > + st->gpiochip = (struct gpio_chip) { > > > + .label = "ad7768_1_gpios", > > What is '_1' for? > Also, what will happen if the device has two or more such ADCs > installed? Will they all provide _the same_ label?! > the '_1' is because the part name is 'ad7768-1'. Most of similar devices only define a static name, but I could use the reg field, if you beleive it is better. > > + .base = -1, > > + .ngpio = 4, > > + .parent = &st->spi->dev, > > + .can_sleep = true, > > + .direction_input = ad7768_gpio_direction_input, > > + .direction_output = ad7768_gpio_direction_output, > > + .get = ad7768_gpio_get, > > + .set_rv = ad7768_gpio_set, > > + .owner = THIS_MODULE, > > + }; > > + > > + return devm_gpiochip_add_data(&st->spi->dev, &st->gpiochip, indio_dev); > > +} > > ... > > > + /* 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); > > + if (ret < 0) > > Why ' < 0'? > > > + return ret; > > + } > > -- > With Best Regards, > Andy Shevchenko
On 04/28, Andy Shevchenko wrote: > On Mon, Apr 28, 2025 at 3:14 AM 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 SYNC_IN pin. > > to the SYNC_IN > > > Use trigger-sources property to enable device synchronization over SPI > > and multi-device synchronization, as an alternative to adi,sync-in-gpios > > property. > > ... > > > +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); > > > + if (st->gpio_sync_in) { > > Dup check, the following have already it. > > > + gpiod_set_value_cansleep(st->gpio_sync_in, 1); > > Yes, I see the original code, but still the Q is why no delay. Perhaps > a comment explaining that the GPIO op is slow enough (?) to add. > Datasheet specifies a minimum of 1.5*Tmclk pulse width. For the recommended mclk of 16.384 MHz, it usually takes 4 times the minimum pulse width. If it can be less than that for other plataforms I can add this delay. > > + gpiod_set_value_cansleep(st->gpio_sync_in, 0); > > + } > > + > > + return 0; > > +} > > ... > > > +static struct gpio_desc *ad7768_trigger_source_get_gpio(struct device *dev, > > + struct fwnode_handle *fwnode) > > +{ > > + const char *value; > > + int ret; > > + > > + ret = fwnode_property_read_string(fwnode, "compatible", &value); > > + if (ret) > > + return ERR_PTR(ret); > > + > > + if (strcmp("gpio-trigger", value)) > > + return ERR_PTR(-EINVAL); > > Reinvention of fwnode_device_is_compatible(). > Thanks! > > + return devm_fwnode_gpiod_get_index(dev, fwnode, NULL, 0, > > + GPIOD_OUT_LOW, "sync-in"); > > +} > > ... > > > +static int ad7768_trigger_sources_get_sync(struct device *dev, > > + struct ad7768_state *st) > > +{ > > + struct fwnode_reference_args args; > > + struct fwnode_handle *fwnode = NULL; > > Redundant assignment AFAICS. > > > + int ret; > > + > > + /* > > + * 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 (!device_property_present(dev, "trigger-sources")) { > > + /* > > + * In the absence of trigger-sources property, enable self > > + * synchronization over SPI (SYNC_OUT). > > + */ > > + st->en_spi_sync = true; > > + return 0; > > + } > > + > > + ret = fwnode_property_get_reference_args(dev_fwnode(dev), > > In this case the above is better to be also fwnode_property_present(). > You save a double call to 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 and fallback to > > + * synchronization over SPI in case of failure. > > + */ > > + st->gpio_sync_in = ad7768_trigger_source_get_gpio(dev, fwnode); > > + if (IS_ERR(st->gpio_sync_in)) { > > + /* > > + * For this case, it requires one argument, which indicates the > > + * output pin referenced. > > + */ > > + if (args.nargs < 1) > > + goto err_not_supp; > > + > > + if (args.args[0] != AD7768_TRIGGER_SOURCE_SYNC_OUT) > > + goto err_not_supp; > > + > > + /* > > + * Only self trigger is supported for now, i.e., > > + * external SYNC_OUT is not allowed. > > + */ > > + if (fwnode->dev == dev) { > > ?!?! What is this?! > > For the reference: > https://elixir.bootlin.com/linux/v6.15-rc3/source/include/linux/fwnode.h#L51 > I see, my bad. I will follow David's suggestion, so we will avoid this. > > + st->en_spi_sync = true; > > + goto out_put_node; > > + } > > + > > + goto err_not_supp; > > + } > > + > > + goto out_put_node; > > + > > +err_not_supp: > > + ret = dev_err_probe(dev, -EOPNOTSUPP, > > + "Invalid synchronization trigger source"); > > Missing \n, and can be one line anyway (we don't complain about long > strings ending with string literals for ages, way before the 100 > character limit). > > > +out_put_node: > > + fwnode_handle_put(args.fwnode); > > + return ret; > > +} > > -- > With Best Regards, > Andy Shevchenko
On Sun, 27 Apr 2025 21:12:16 -0300 Jonathan Santos <Jonathan.Santos@analog.com> wrote: > In addition to GPIO synchronization, The AD7768-1 also supports > synchronization over SPI, which use is recommended when the GPIO > cannot provide a pulse synchronous with the base MCLK signal. It > consists of looping back the SYNC_OUT to the SYNC_IN pin and send > a command via SPI to trigger the synchronization. > > Introduce the 'trigger-sources' property to enable SPI-based > synchronization via SYNC_OUT pin, along with additional optional > entries for GPIO3 and DRDY pins. > > Also create #trigger-source-cells property to differentiate the trigger > sources provided by the ADC. To improve readability, create a > adi,ad7768-1.h header with the macros for the cell values. > > While at it, add description to the interrupts property. > > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com> A few things inline, but minor stuff for if you need to do a respin. > --- > v6 Changes: > * Removed references to offload. > * Changed trigger-sources-cells description. Each cell value indicates > a gpio line from the ADC. > * Added adi,ad7768-1.h header with macros for the trigger source cells. > * Removed offload trigger entry from trigger-sources. > > v5 Changes: > * Include START pin and DRDY in the trigger-sources description. > * Fixed "#trigger-source-cells" value and description. > * sync-in-gpios is represented in the trigger-sources property. > > v4 Changes: > * none > > v3 Changes: > * Fixed dt-bindings errors. > * Trigger-source is set as an alternative to sync-in-gpios, so we > don't break the previous ABI. > * increased maxItems from trigger-sources to 2. > > v2 Changes: > * Patch added as replacement for adi,sync-in-spi patch. > * addressed the request for a description to interrupts property. > --- > .../bindings/iio/adc/adi,ad7768-1.yaml | 31 ++++++++++++++++++- > include/dt-bindings/iio/adc/adi,ad7768-1.h | 10 ++++++ > 2 files changed, 40 insertions(+), 1 deletion(-) > create mode 100644 include/dt-bindings/iio/adc/adi,ad7768-1.h > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml > index 3ce59d4d065f..1f476aa15305 100644 > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml > @@ -26,7 +26,28 @@ properties: > clock-names: > const: mclk > > + trigger-sources: > + $ref: /schemas/types.yaml#/definitions/phandle-array > + minItems: 1 > + maxItems: 2 > + description: | > + A list of phandles referencing trigger source providers. Each entry > + represents a trigger source for the ADC: > + > + - First entry specifies the device responsible for driving the > + synchronization (SYNC_IN) pin, as an alternative to adi,sync-in-gpios. > + This can be a `gpio-trigger` or another `ad7768-1` device. If the > + device's own SYNC_OUT pin is internally connected to its SYNC_IN pin, > + reference the device itself or omit this property. > + - Second entry optionally defines a GPIO3 pin used as a START signal trigger. > + > + Use the accompanying trigger source cell to identify the type of each entry. > + > interrupts: > + description: > + Specifies the interrupt line associated with the ADC. This refers > + to the DRDY (Data Ready) pin, which signals when conversion results are > + available. Trivial but this seems overly wordy. The only bit that matters is the datardy bit. DRDY (Data Ready) pin, which signals conversion results are available. is probably enough. Only bother if you are respining for other reasons however as this really doesn't matter! > maxItems: 1 > > '#address-cells': > @@ -57,6 +78,15 @@ properties: > "#io-channel-cells": > const: 1 > > + "#trigger-source-cells": > + description: | > + Cell indicates the trigger output signal: 0 = SYNC_OUT, 1 = GPIO3, > + 2 = DRDY. > + > + For better readability, macros for these values are available in > + dt-bindings/iio/adc/adi,ad7768-1.h. > + const: 1 > + > required: > - compatible > - reg > @@ -65,7 +95,6 @@ required: > - vref-supply > - spi-cpol > - spi-cpha > - - adi,sync-in-gpios Maybe worth requiring oneOf adi,sync-in-gpios or trigger-sources? > > patternProperties: > "^channel@([0-9]|1[0-5])$": > diff --git a/include/dt-bindings/iio/adc/adi,ad7768-1.h b/include/dt-bindings/iio/adc/adi,ad7768-1.h > new file mode 100644 > index 000000000000..34d92856a50b > --- /dev/null > +++ b/include/dt-bindings/iio/adc/adi,ad7768-1.h > @@ -0,0 +1,10 @@ > +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */ > + > +#ifndef _DT_BINDINGS_ADI_AD7768_1_H > +#define _DT_BINDINGS_ADI_AD7768_1_H > + > +#define AD7768_TRIGGER_SOURCE_SYNC_OUT 0 > +#define AD7768_TRIGGER_SOURCE_GPIO3 1 > +#define AD7768_TRIGGER_SOURCE_DRDY 2 > + > +#endif /* _DT_BINDINGS_ADI_AD7768_1_H */
On Sun, 27 Apr 2025 21:12:02 -0300 Jonathan Santos <Jonathan.Santos@analog.com> wrote: > Inspired by pwm-trigger, create a new binding for using a GPIO > line as a trigger source. > > Link: https://lore.kernel.org/linux-iio/20250207-dlech-mainline-spi-engine-offload-2-v8-3-e48a489be48c@baylibre.com/ David, given you did the pwm one, maybe give this a quick look. Maybe it's worth generalising to cover all trigger sources in MAINTAINERS? Thanks. Otherwise I obviously need a DT review before taking this and maybe the GPIO element suggests Linus W or Bartosz might be other good reviewers? Jonathan > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com> > --- > v6 Changes: > * Changed description. > * Fixed typos and replaced GPIO pin with GPIO line. > * Added link reference for pwm-trigger. > > v5 Changes: > * New patch in v5. > --- > .../bindings/trigger-source/gpio-trigger.yaml | 40 +++++++++++++++++++ > 1 file changed, 40 insertions(+) > create mode 100644 Documentation/devicetree/bindings/trigger-source/gpio-trigger.yaml > > diff --git a/Documentation/devicetree/bindings/trigger-source/gpio-trigger.yaml b/Documentation/devicetree/bindings/trigger-source/gpio-trigger.yaml > new file mode 100644 > index 000000000000..1331d153ee82 > --- /dev/null > +++ b/Documentation/devicetree/bindings/trigger-source/gpio-trigger.yaml > @@ -0,0 +1,40 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/trigger-source/gpio-trigger.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Generic trigger source using GPIO > + > +description: A GPIO used as a trigger source. > + > +maintainers: > + - Jonathan Santos <Jonathan.Santos@analog.com> > + > +properties: > + compatible: > + const: gpio-trigger > + > + '#trigger-source-cells': > + const: 0 > + > + gpios: > + maxItems: 1 > + description: GPIO to be used as a trigger source. > + > +required: > + - compatible > + - '#trigger-source-cells' > + - gpios > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/gpio/gpio.h> > + > + trigger { > + compatible = "gpio-trigger"; > + #trigger-source-cells = <0>; > + gpios = <&gpio1 15 GPIO_ACTIVE_HIGH>; > + };
On 5/5/25 10:44 AM, Jonathan Cameron wrote: > On Sun, 27 Apr 2025 21:12:02 -0300 > Jonathan Santos <Jonathan.Santos@analog.com> wrote: > >> Inspired by pwm-trigger, create a new binding for using a GPIO >> line as a trigger source. >> >> Link: https://lore.kernel.org/linux-iio/20250207-dlech-mainline-spi-engine-offload-2-v8-3-e48a489be48c@baylibre.com/ > > David, given you did the pwm one, maybe give this a quick look. LGTM. Reviewed-by: David Lechner <dlechner@baylibre.com> > > Maybe it's worth generalising to cover all trigger sources in MAINTAINERS? Sure, I would be OK with that. > > Thanks. Otherwise I obviously need a DT review before taking this and maybe the GPIO > element suggests Linus W or Bartosz might be other good reviewers? Linus W already replied in v5: https://lore.kernel.org/linux-iio/cover.1744325346.git.Jonathan.Santos@analog.com/T/#mb263ff61984dae4a479632dbe33c94a66659fd42
On Sun, 27 Apr 2025 21:13:47 -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 SYNC_IN pin. > > Use trigger-sources property to enable device synchronization over SPI > and multi-device synchronization, as an alternative to adi,sync-in-gpios > property. > > +static int ad7768_trigger_sources_get_sync(struct device *dev, > + struct ad7768_state *st) > +{ > + struct fwnode_reference_args args; > + struct fwnode_handle *fwnode = NULL; > + int ret; > + > + /* > + * 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 (!device_property_present(dev, "trigger-sources")) { > + /* > + * In the absence of trigger-sources property, enable self > + * synchronization over SPI (SYNC_OUT). > + */ > + st->en_spi_sync = true; > + return 0; > + } > + > + ret = fwnode_property_get_reference_args(dev_fwnode(dev), > + "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 and fallback to > + * synchronization over SPI in case of failure. > + */ > + st->gpio_sync_in = ad7768_trigger_source_get_gpio(dev, fwnode); > + if (IS_ERR(st->gpio_sync_in)) { > + /* > + * For this case, it requires one argument, which indicates the > + * output pin referenced. > + */ > + if (args.nargs < 1) > + goto err_not_supp; > + > + if (args.args[0] != AD7768_TRIGGER_SOURCE_SYNC_OUT) > + goto err_not_supp; > + > + /* > + * Only self trigger is supported for now, i.e., > + * external SYNC_OUT is not allowed. > + */ > + if (fwnode->dev == dev) { > + st->en_spi_sync = true; > + goto out_put_node; > + } > + > + goto err_not_supp; > + } > + > + goto out_put_node; > + > +err_not_supp: Split the good and bad paths here so we don't have good paths jumping to mid way through the block. If the good paths need to jump then I'd suggest factoring out the stuff with the node held into a separate function. Superficially it just looks like one condition flip and you are refactoring this anyway based on other feedback so maybe this comment becomes irrelevant! > + ret = dev_err_probe(dev, -EOPNOTSUPP, > + "Invalid synchronization trigger source"); > +out_put_node: > + fwnode_handle_put(args.fwnode); > + return ret; > +} > +
On Sun, 27 Apr 2025 21:14:17 -0300 Jonathan Santos <Jonathan.Santos@analog.com> wrote: > Separate filter type and decimation rate from the sampling frequency > attribute. The new filter type attribute enables sinc3, sinc3+rej60 > and wideband filters, which were previously unavailable. > > Previously, combining decimation and MCLK divider in the sampling > frequency obscured performance trade-offs. Lower MCLK divider > settings increase power usage, while lower decimation rates reduce > precision by decreasing averaging. By creating an oversampling > attribute, which controls the decimation, users gain finer control > over performance. > > The addition of those attributes allows a wider range of sampling > frequencies and more access to the device features. Sampling frequency > table is updated after every digital filter parameter change. > > Reviewed-by: David Lechner <dlechner@baylibre.com> > Co-developed-by: Pop Paul <paul.pop@analog.com> > Signed-off-by: Pop Paul <paul.pop@analog.com> > Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com> A few trivial additions from me. > --- > v6 Changes: > * Made sinc3 decimation rate calculation clearer as requested. > * Renamed some filter functions to clarify the purpose. > * Other nits. > > v5 Changes: > * Addressed some nits. > * Use the new new iio_device_claim/release_direct() functions. > > v4 Changes: > * Sampling frequency table is dynamically updated after every > filter configuration. > > v3 Changes: > * removed unused variables. > * included sinc3+rej60 filter type. > * oversampling_ratio moved to info_mask_shared_by_type. > * reordered functions to avoid forward declaration. > * simplified regmap writes. > * Removed locking. > * replaced some helper functions for direct regmap_update_bits > calls. > * Addressed other nits. > > v2 Changes: > * Decimation_rate attribute replaced for oversampling_ratio. > --- > drivers/iio/adc/ad7768-1.c | 363 ++++++++++++++++++++++++++++++------- > 1 file changed, 293 insertions(+), 70 deletions(-) > > diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c > index 10791a85d2c5..e2b8f12260a5 100644 > --- a/drivers/iio/adc/ad7768-1.c > +++ b/drivers/iio/adc/ad7768-1.c > @@ -20,6 +20,8 @@ > #include <linux/regulator/driver.h> > #include <linux/sysfs.h> > #include <linux/spi/spi.h> > +#include <linux/unaligned.h> > +#include <linux/util_macros.h> > > #include <linux/iio/buffer.h> > #include <linux/iio/iio.h> > @@ -77,7 +79,7 @@ > #define AD7768_PWR_PWRMODE(x) FIELD_PREP(AD7768_PWR_PWRMODE_MSK, x) > > /* AD7768_REG_DIGITAL_FILTER */ > -#define AD7768_DIG_FIL_FIL_MSK GENMASK(6, 4) > +#define AD7768_DIG_FIL_FIL_MSK GENMASK(7, 4) Bug? If so does this belong in a precursor patch? > #define AD7768_DIG_FIL_FIL(x) FIELD_PREP(AD7768_DIG_FIL_FIL_MSK, x) > @@ -404,22 +473,110 @@ static int ad7768_reg_access(struct iio_dev *indio_dev, > return ret; > } > > -static int ad7768_set_dig_fil(struct ad7768_state *st, > - enum ad7768_dec_rate dec_rate) > +static int ad7768_set_sinc3_dec_rate(struct ad7768_state *st, > + unsigned int dec_rate) > { > - unsigned int mode; > + unsigned int max_dec_rate; > + u8 dec_rate_reg[2]; > + u16 regval; > int ret; > > - if (dec_rate == AD7768_DEC_RATE_8 || dec_rate == AD7768_DEC_RATE_16) > - mode = AD7768_DIG_FIL_FIL(dec_rate); > - else > - mode = AD7768_DIG_FIL_DEC_RATE(dec_rate); > + /* > + * Maximum dec_rate is limited by the MCLK_DIV value Oddly short wrap. Go nearer 80 chars. > + * and by the ODR. The edge case is for MCLK_DIV = 2 > + * ODR = 50 SPS. > + * max_dec_rate <= MCLK / (2 * 50) > + */ > +static int ad7768_get_filter_type_attr(struct iio_dev *dev, > + const struct iio_chan_spec *chan) > +{ > + struct ad7768_state *st = iio_priv(dev); > + int ret; > + unsigned int mode; > + > + ret = regmap_read(st->regmap, AD7768_REG_DIGITAL_FILTER, &mode); > + if (ret) > + return ret; > + > + mode = FIELD_GET(AD7768_DIG_FIL_FIL_MSK, mode); > + > + /* From the register value, get the corresponding filter type */ > + return ad7768_filter_regval_to_type[mode]; return ad7768_filter_regval_to_type[FIELD_GET(AD7768_DIG_FIL_FIL_MSK, mode)]; Is fine (only just over 80 chars and I'm getting more relaxed as time moves forwards). Also avoids the dual meaning of mode which I never like to see. > } > > -static int ad7768_write_raw(struct iio_dev *indio_dev, > - struct iio_chan_spec const *chan, > - int val, int val2, long info) > +static int __ad7768_write_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int val, int val2, long info) > { > struct ad7768_state *st = iio_priv(indio_dev); > + int ret; > > switch (info) { > case IIO_CHAN_INFO_SAMP_FREQ: > return ad7768_set_freq(st, val); Whilst I doubt anyone will notice this looks like a functional change that should be called out in the patch description. Previously we allowed frequency changes in buffered mode, now we don't. > + > + case IIO_CHAN_INFO_OVERSAMPLING_RATIO: > + ret = ad7768_configure_dig_fil(indio_dev, st->filter_type, val); > + if (ret) > + return ret; > + > + /* Update sampling frequency */ > + return ad7768_set_freq(st, st->samp_freq); > default: > return -EINVAL; > } > } > > +static int ad7768_write_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int val, int val2, long info) > +{ > + int ret; > + > + if (!iio_device_claim_direct(indio_dev)) > + return -EBUSY; > + > + ret = __ad7768_write_raw(indio_dev, chan, val, val2, info); > + iio_device_release_direct(indio_dev); > + > + return ret; > +} ;
On 5/6/25 2:03 PM, Jonathan Santos wrote: > On 05/05, Jonathan Cameron wrote: >> On Sun, 27 Apr 2025 21:14:17 -0300 >> Jonathan Santos <Jonathan.Santos@analog.com> wrote: >> > ... >> drivers/iio/adc/ad7768-1.c | 363 ++++++++++++++++++++++++++++++------- >>> 1 file changed, 293 insertions(+), 70 deletions(-) >>> >>> diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c >>> index 10791a85d2c5..e2b8f12260a5 100644 >>> --- a/drivers/iio/adc/ad7768-1.c >>> +++ b/drivers/iio/adc/ad7768-1.c >>> @@ -20,6 +20,8 @@ >>> #include <linux/regulator/driver.h> >>> #include <linux/sysfs.h> >>> #include <linux/spi/spi.h> >>> +#include <linux/unaligned.h> >>> +#include <linux/util_macros.h> >>> >>> #include <linux/iio/buffer.h> >>> #include <linux/iio/iio.h> >>> @@ -77,7 +79,7 @@ >>> #define AD7768_PWR_PWRMODE(x) FIELD_PREP(AD7768_PWR_PWRMODE_MSK, x) >>> >>> /* AD7768_REG_DIGITAL_FILTER */ >>> -#define AD7768_DIG_FIL_FIL_MSK GENMASK(6, 4) >>> +#define AD7768_DIG_FIL_FIL_MSK GENMASK(7, 4) >> >> Bug? If so does this belong in a precursor patch? >> > > Actually not, this extra bit is to include the 60Hz rejection enable > for sinc3 filter Seems odd to me to group those together since they are two separate fields in the register. It would make more sense to have a separate BIT(7) for the EN_60HZ_REJ bit. If we need to manipulate both at the same time in the driver, then we would use AD7768_DIG_FIL_EN_60HZ_REJ | AD7768_DIG_FIL_FIL_MSK. > >>> #define AD7768_DIG_FIL_FIL(x) FIELD_PREP(AD7768_DIG_FIL_FIL_MSK, x) >> > ... >
On 05/05, Jonathan Cameron wrote: > On Sun, 27 Apr 2025 21:12:16 -0300 > Jonathan Santos <Jonathan.Santos@analog.com> wrote: > ... > > required: > > - compatible > > - reg > > @@ -65,7 +95,6 @@ required: > > - vref-supply > > - spi-cpol > > - spi-cpha > > - - adi,sync-in-gpios > > Maybe worth requiring oneOf adi,sync-in-gpios or trigger-sources? > We cannot do that because we defined that self triggering is enabled when trigger-sources is omitted (is this case adi,sync-in-gpios is not present as well). > > > > patternProperties: > > "^channel@([0-9]|1[0-5])$": > > diff --git a/include/dt-bindings/iio/adc/adi,ad7768-1.h b/include/dt-bindings/iio/adc/adi,ad7768-1.h > > new file mode 100644 > > index 000000000000..34d92856a50b > > --- /dev/null > > +++ b/include/dt-bindings/iio/adc/adi,ad7768-1.h > > @@ -0,0 +1,10 @@ > > +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */ > > + > > +#ifndef _DT_BINDINGS_ADI_AD7768_1_H > > +#define _DT_BINDINGS_ADI_AD7768_1_H > > + > > +#define AD7768_TRIGGER_SOURCE_SYNC_OUT 0 > > +#define AD7768_TRIGGER_SOURCE_GPIO3 1 > > +#define AD7768_TRIGGER_SOURCE_DRDY 2 > > + > > +#endif /* _DT_BINDINGS_ADI_AD7768_1_H */ >