| Message ID | 20240815-ad7606_add_iio_backend_support-v1-8-cea3e11b1aa4@baylibre.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add iio backend compatibility for ad7606 | expand |
On Thu, 15 Aug 2024 12:12:02 +0000 Guillaume Stols <gstols@baylibre.com> wrote: > - Basic support for iio backend. > - Supports IIO_CHAN_INFO_SAMP_FREQ R/W. > - Only hardware mode is available, and that IIO_CHAN_INFO_RAW is not > supported if iio-backend mode is selected. > > A small correction was added to the driver's file name in the Kconfig > file's description. > I'm going to want Nuno's input on this. Given it's the summer that may take a little while, so in meantime a few comments inline. Jonathan > Signed-off-by: Guillaume Stols <gstols@baylibre.com> > --- > drivers/iio/adc/Kconfig | 3 +- > drivers/iio/adc/ad7606.c | 103 +++++++++++++++++++++++++++++++++++-------- > drivers/iio/adc/ad7606.h | 16 +++++++ > drivers/iio/adc/ad7606_par.c | 98 +++++++++++++++++++++++++++++++++++++++- > 4 files changed, 200 insertions(+), 20 deletions(-) > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > index 88e8ce2e78b3..01248b6df868 100644 > --- a/drivers/iio/adc/Kconfig > +++ b/drivers/iio/adc/Kconfig > @@ -227,9 +227,10 @@ config AD7606_IFACE_PARALLEL > help > Say yes here to build parallel interface support for Analog Devices: > ad7605-4, ad7606, ad7606-6, ad7606-4 analog to digital converters (ADC). > + It also support iio_backended devices for AD7606B. > > To compile this driver as a module, choose M here: the > - module will be called ad7606_parallel. > + module will be called ad7606_par. If we can avoid a rename that would be good. Or was this always wrong? If so spin a fix patch before this one. > > config AD7606_IFACE_SPI > tristate "Analog Devices AD7606 ADC driver with spi interface support" > diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c > index 99d5ca5c2348..a753d5caa9f8 100644 > --- a/drivers/iio/adc/ad7606.c > +++ b/drivers/iio/adc/ad7606.c > @@ -21,6 +21,7 @@ > #include <linux/util_macros.h> > #include <linux/units.h> > > +#include <linux/iio/backend.h> > #include <linux/iio/iio.h> > #include <linux/iio/buffer.h> > #include <linux/iio/sysfs.h> > @@ -148,7 +149,15 @@ static int ad7606_set_sampling_freq(struct ad7606_state *st, unsigned long freq) > > static int ad7606_read_samples(struct ad7606_state *st) > { > - unsigned int num = st->chip_info->num_channels - 1; > + unsigned int num = st->chip_info->num_channels; > + > + /* > + * Timestamp channel does not contain sample, and no timestamp channel if > + * backend is used. > + */ > + if (!st->back) > + num--; > + > u16 *data = st->data; > int ret; > > @@ -220,11 +229,15 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch) > if (!ret) > return ret; > } > - ret = wait_for_completion_timeout(&st->completion, > - msecs_to_jiffies(1000)); > - if (!ret) { > - ret = -ETIMEDOUT; > - goto error_ret; > + > + /* backend manages interruptions by itself.*/ > + if (!st->back) { > + ret = wait_for_completion_timeout(&st->completion, > + msecs_to_jiffies(1000)); > + if (!ret) { > + ret = -ETIMEDOUT; > + goto error_ret; > + } > } > > ret = ad7606_read_samples(st); > @@ -271,6 +284,12 @@ static int ad7606_read_raw(struct iio_dev *indio_dev, > case IIO_CHAN_INFO_OVERSAMPLING_RATIO: > *val = st->oversampling; > return IIO_VAL_INT; > + case IIO_CHAN_INFO_SAMP_FREQ: > + pwm_get_state_hw(st->cnvst_pwm, &cnvst_pwm_state); > + /* If the PWM is swinging, return the real frequency, otherwise 0 */ So this only exists for the pwm case. In that case can we split the channel definitions into versions with an without this and register just the right one. A sampling frequency of 0 usually means no sampling, not that we can tell what it is. If we can't tell don't provide the file. > + *val = ad7606_pwm_is_swinging(st) ? > + DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, cnvst_pwm_state.period) : 0; > + return IIO_VAL_INT; > } > return -EINVAL; > } > @@ -360,6 +379,8 @@ static int ad7606_write_raw(struct iio_dev *indio_dev, > return ret; > > return 0; > + case IIO_CHAN_INFO_SAMP_FREQ: > + return ad7606_set_sampling_freq(st, val); > default: > return -EINVAL; > } > static const struct iio_trigger_ops ad7606_trigger_ops = { > @@ -602,8 +660,6 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address, > indio_dev->channels = st->chip_info->channels; > indio_dev->num_channels = st->chip_info->num_channels; > > - init_completion(&st->completion); > - > ret = ad7606_reset(st); > if (ret) > dev_warn(st->dev, "failed to RESET: no RESET GPIO specified\n"); > @@ -635,7 +691,7 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address, > return ret; > } > > - /* If convst pin is not defined, setup PWM*/ > + /* If convst pin is not defined, setup PWM */ Push into earlier patch. Check for this sort of fix being in wrong patch before sending a series. It just adds noise to patch and to reviews. > if (!st->gpio_convst) { > st->cnvst_pwm = devm_pwm_get(dev, NULL); > if (IS_ERR(st->cnvst_pwm)) ... > diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h > index aab8fefb84be..9a098cd77812 100644 > --- a/drivers/iio/adc/ad7606.h > +++ b/drivers/iio/adc/ad7606.h > @@ -34,6 +34,12 @@ > BIT(IIO_CHAN_INFO_SCALE), \ > BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO)) > > +#define AD7606_BI_CHANNEL(num) \ > + AD760X_CHANNEL(num, 0, \ > + BIT(IIO_CHAN_INFO_SCALE), \ > + BIT(IIO_CHAN_INFO_SAMP_FREQ) | \ > + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO)) > + > #define AD7616_CHANNEL(num) \ > AD760X_CHANNEL(num, BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),\ > 0, BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO)) > @@ -61,6 +67,7 @@ enum ad7606_supported_device_ids { > * @os_req_reset some devices require a reset to update oversampling > * @init_delay_ms required delay in miliseconds for initialization > * after a restart > + * @has_backend defines if a backend is available for the given chip That seems to me more of a case of does the driver support it. Linux kernel code has no way of knowing if a backend hardware exists or not. Modify the comment to speak about if we know it works. Or is there something fundamental that stops the backend approach working with some devices? Why does the driver need this flag? > */ > struct ad7606_chip_info { > enum ad7606_supported_device_ids id; > @@ -71,6 +78,7 @@ struct ad7606_chip_info { > unsigned int oversampling_num; > bool os_req_reset; > unsigned long init_delay_ms; > + bool has_backend; > }; > > diff --git a/drivers/iio/adc/ad7606_par.c b/drivers/iio/adc/ad7606_par.c > index d83c0edc1e31..5c8a04556e25 100644 > --- a/drivers/iio/adc/ad7606_par.c > +++ b/drivers/iio/adc/ad7606_par.c > @@ -3,6 +3,8 @@ > * AD7606 Parallel Interface ADC driver > * > * Copyright 2011 Analog Devices Inc. > + * Copyright 2024 Analog Devices Inc. > + * Copyright 2024 BayLibre SAS. > */ > > #include <linux/mod_devicetable.h> > @@ -11,10 +13,86 @@ > #include <linux/types.h> > #include <linux/err.h> > #include <linux/io.h> > +#include <linux/pwm.h> > +#include <linux/gpio.h> > +#include <linux/delay.h> Not on any order currently but try to minimize a future series that might clean this up. Preference is alphabetical though fine to have a trailing block of IIO headers, then driver specific ones after that. You can either fix the current order in a separate patch, or just put your new headers in approximately the write place. > > #include <linux/iio/iio.h> > +#include <linux/iio/backend.h> > #include "ad7606.h" > +static int ad7606_bi_setup_iio_backend(struct device *dev, struct iio_dev *indio_dev) > +{ > + struct ad7606_state *st = iio_priv(indio_dev); Why 2 tabs? > + unsigned int ret, c; > + > + st->back = devm_iio_backend_get(dev, NULL); > + if (IS_ERR(st->back)) > + return PTR_ERR(st->back); > + > + /* If the device is iio_backend powered the PWM is mandatory */ > + if (!st->cnvst_pwm) > + return -EINVAL; > + > + ret = devm_iio_backend_request_buffer(dev, st->back, indio_dev); > + if (ret) > + return ret; > + > + ret = devm_iio_backend_enable(dev, st->back); > + if (ret) > + return ret; > + > + struct iio_backend_data_fmt data = { > + .sign_extend = true, > + .enable = true, > + }; > + for (c = 0; c < indio_dev->num_channels; c++) { > + ret = iio_backend_data_format_set(st->back, c, &data); > + if (ret) > + return ret; > + } > + > + indio_dev->channels = ad7606b_bi_channels; > + indio_dev->num_channels = 8; > + > + return 0; > +} > + > +static const struct ad7606_bus_ops ad7606_bi_bops = { > + .iio_backend_config = ad7606_bi_setup_iio_backend, > + .update_scan_mode = ad7606_bi_update_scan_mode, > +}; > +#endif > + > static int ad7606_par16_read_block(struct device *dev, > int count, void *buf) > { > @@ -52,7 +130,20 @@ static int ad7606_par_probe(struct platform_device *pdev) > void __iomem *addr; > resource_size_t remap_size; > int irq; > - > +#ifdef CONFIG_IIO_BACKEND > + struct iio_backend *back; > + > + /*For now, only the AD7606B is backend compatible.*/ /* For ... ble. */ > + if (chip_info->has_backend) { > + back = devm_iio_backend_get(&pdev->dev, NULL); > + if (IS_ERR(back)) > + return PTR_ERR(back); > + > + return ad7606_probe(&pdev->dev, 0, NULL, > + chip_info, > + &ad7606_bi_bops); Short wrap. Aim for 80 char limit unless it hurts readability a lot. > + } > +#endif > irq = platform_get_irq(pdev, 0); > if (irq < 0) > return irq; > @@ -74,6 +165,7 @@ static const struct platform_device_id ad7606_driver_ids[] = { > { .name = "ad7606-4", .driver_data = (kernel_ulong_t)&ad7606_4_info, }, > { .name = "ad7606-6", .driver_data = (kernel_ulong_t)&ad7606_6_info, }, > { .name = "ad7606-8", .driver_data = (kernel_ulong_t)&ad7606_8_info, }, > + { .name = "ad7606b", .driver_data = (kernel_ulong_t)&ad7606b_info, }, > { } > }; > MODULE_DEVICE_TABLE(platform, ad7606_driver_ids); > @@ -83,6 +175,7 @@ static const struct of_device_id ad7606_of_match[] = { > { .compatible = "adi,ad7606-4", .data = &ad7606_4_info }, > { .compatible = "adi,ad7606-6", .data = &ad7606_6_info }, > { .compatible = "adi,ad7606-8", .data = &ad7606_8_info }, > + { .compatible = "adi,ad7606b", .data = &ad7606b_info }, > { } > }; > MODULE_DEVICE_TABLE(of, ad7606_of_match); > @@ -102,3 +195,6 @@ MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>"); > MODULE_DESCRIPTION("Analog Devices AD7606 ADC"); > MODULE_LICENSE("GPL v2"); > MODULE_IMPORT_NS(IIO_AD7606); > +#ifdef CONFIG_IIO_BACKEND > +MODULE_IMPORT_NS(IIO_BACKEND); I'd not bother with config guards. Importing a namespace we don't use should be harmless. > +#endif >
On Thu, 2024-08-15 at 12:12 +0000, Guillaume Stols wrote: > - Basic support for iio backend. > - Supports IIO_CHAN_INFO_SAMP_FREQ R/W. > - Only hardware mode is available, and that IIO_CHAN_INFO_RAW is not > supported if iio-backend mode is selected. > > A small correction was added to the driver's file name in the Kconfig > file's description. > > Signed-off-by: Guillaume Stols <gstols@baylibre.com> > --- Hi Guillaume, Some initial feedback from me... > drivers/iio/adc/Kconfig | 3 +- > drivers/iio/adc/ad7606.c | 103 +++++++++++++++++++++++++++++++++++------- > - > drivers/iio/adc/ad7606.h | 16 +++++++ > drivers/iio/adc/ad7606_par.c | 98 +++++++++++++++++++++++++++++++++++++++- > 4 files changed, 200 insertions(+), 20 deletions(-) > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > index 88e8ce2e78b3..01248b6df868 100644 > --- a/drivers/iio/adc/Kconfig > +++ b/drivers/iio/adc/Kconfig > @@ -227,9 +227,10 @@ config AD7606_IFACE_PARALLEL > help > Say yes here to build parallel interface support for Analog > Devices: > ad7605-4, ad7606, ad7606-6, ad7606-4 analog to digital converters > (ADC). > + It also support iio_backended devices for AD7606B. > > To compile this driver as a module, choose M here: the > - module will be called ad7606_parallel. > + module will be called ad7606_par. > > config AD7606_IFACE_SPI > tristate "Analog Devices AD7606 ADC driver with spi interface > support" > diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c > index 99d5ca5c2348..a753d5caa9f8 100644 > --- a/drivers/iio/adc/ad7606.c > +++ b/drivers/iio/adc/ad7606.c > @@ -21,6 +21,7 @@ > #include <linux/util_macros.h> > #include <linux/units.h> > > +#include <linux/iio/backend.h> > #include <linux/iio/iio.h> > #include <linux/iio/buffer.h> > #include <linux/iio/sysfs.h> > @@ -148,7 +149,15 @@ static int ad7606_set_sampling_freq(struct ad7606_state > *st, unsigned long freq) > > static int ad7606_read_samples(struct ad7606_state *st) > { > - unsigned int num = st->chip_info->num_channels - 1; > + unsigned int num = st->chip_info->num_channels; > + > + /* > + * Timestamp channel does not contain sample, and no timestamp > channel if > + * backend is used. > + */ > + if (!st->back) > + num--; > + > u16 *data = st->data; > int ret; > > @@ -220,11 +229,15 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, > unsigned int ch) > if (!ret) > return ret; > } > - ret = wait_for_completion_timeout(&st->completion, > - msecs_to_jiffies(1000)); > - if (!ret) { > - ret = -ETIMEDOUT; > - goto error_ret; > + > + /* backend manages interruptions by itself.*/ missing space before closing the comment (also not sure the comments adds much) > + if (!st->back) { > + ret = wait_for_completion_timeout(&st->completion, > + msecs_to_jiffies(1000)); > + if (!ret) { > + ret = -ETIMEDOUT; > + goto error_ret; > + } > } > > ret = ad7606_read_samples(st); > @@ -271,6 +284,12 @@ static int ad7606_read_raw(struct iio_dev *indio_dev, > case IIO_CHAN_INFO_OVERSAMPLING_RATIO: > *val = st->oversampling; > return IIO_VAL_INT; > + case IIO_CHAN_INFO_SAMP_FREQ: > + pwm_get_state_hw(st->cnvst_pwm, &cnvst_pwm_state); > + /* If the PWM is swinging, return the real frequency, > otherwise 0 */ > + *val = ad7606_pwm_is_swinging(st) ? > + DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, > cnvst_pwm_state.period) : 0; > + return IIO_VAL_INT; > } > return -EINVAL; > } > @@ -360,6 +379,8 @@ static int ad7606_write_raw(struct iio_dev *indio_dev, > return ret; > > return 0; > + case IIO_CHAN_INFO_SAMP_FREQ: > + return ad7606_set_sampling_freq(st, val); > default: > return -EINVAL; > } > @@ -482,7 +503,6 @@ static int ad7606_buffer_postenable(struct iio_dev > *indio_dev) > struct ad7606_state *st = iio_priv(indio_dev); > > gpiod_set_value(st->gpio_convst, 1); > - ad7606_pwm_set_swing(st); > > return 0; > } > @@ -492,19 +512,53 @@ static int ad7606_buffer_predisable(struct iio_dev > *indio_dev) > struct ad7606_state *st = iio_priv(indio_dev); > > gpiod_set_value(st->gpio_convst, 0); > - ad7606_pwm_set_low(st); > > return 0; > } > > +static int ad7606_pwm_buffer_postenable(struct iio_dev *indio_dev) > +{ > + struct ad7606_state *st = iio_priv(indio_dev); > + > + return ad7606_pwm_set_swing(st); > +} > + > +static int ad7606_pwm_buffer_predisable(struct iio_dev *indio_dev) > +{ > + struct ad7606_state *st = iio_priv(indio_dev); > + > + return ad7606_pwm_set_low(st); > +} Maybe I'm missing something but are we removing the gpiod calls? > + > +static int ad7606_update_scan_mode(struct iio_dev *indio_dev, > + const unsigned long *scan_mask) > +{ > + struct ad7606_state *st = iio_priv(indio_dev); > + > + /* The update scan mode is only for iio backend compatible drivers. > + * If the specific update_scan_mode is not defined in the bus ops, > + * just do nothing and return 0. > + */ > + if (st->bops->update_scan_mode) > + return st->bops->update_scan_mode(indio_dev, scan_mask); > + else > + return 0; Redundant else > +} > + > static const struct iio_buffer_setup_ops ad7606_buffer_ops = { > .postenable = &ad7606_buffer_postenable, > .predisable = &ad7606_buffer_predisable, > }; > > +static const struct iio_buffer_setup_ops ad7606_pwm_buffer_ops = { > + .postenable = &ad7606_pwm_buffer_postenable, > + .predisable = &ad7606_pwm_buffer_predisable, > +}; > + > static const struct iio_info ad7606_info_no_os_or_range = { > .read_raw = &ad7606_read_raw, > .validate_trigger = &ad7606_validate_trigger, > + .update_scan_mode = &ad7606_update_scan_mode, > }; > > static const struct iio_info ad7606_info_os_and_range = { > @@ -512,6 +566,7 @@ static const struct iio_info ad7606_info_os_and_range = { > .write_raw = &ad7606_write_raw, > .attrs = &ad7606_attribute_group_os_and_range, > .validate_trigger = &ad7606_validate_trigger, > + .update_scan_mode = &ad7606_update_scan_mode, > }; > > static const struct iio_info ad7606_info_os_range_and_debug = { > @@ -520,6 +575,7 @@ static const struct iio_info > ad7606_info_os_range_and_debug = { > .debugfs_reg_access = &ad7606_reg_access, > .attrs = &ad7606_attribute_group_os_and_range, > .validate_trigger = &ad7606_validate_trigger, > + .update_scan_mode = &ad7606_update_scan_mode, > }; > > static const struct iio_info ad7606_info_os = { > @@ -527,6 +583,7 @@ static const struct iio_info ad7606_info_os = { > .write_raw = &ad7606_write_raw, > .attrs = &ad7606_attribute_group_os, > .validate_trigger = &ad7606_validate_trigger, > + .update_scan_mode = &ad7606_update_scan_mode, > }; > > static const struct iio_info ad7606_info_range = { > @@ -534,6 +591,7 @@ static const struct iio_info ad7606_info_range = { > .write_raw = &ad7606_write_raw, > .attrs = &ad7606_attribute_group_range, > .validate_trigger = &ad7606_validate_trigger, > + .update_scan_mode = &ad7606_update_scan_mode, > }; > > static const struct iio_trigger_ops ad7606_trigger_ops = { > @@ -602,8 +660,6 @@ int ad7606_probe(struct device *dev, int irq, void __iomem > *base_address, > indio_dev->channels = st->chip_info->channels; > indio_dev->num_channels = st->chip_info->num_channels; > > - init_completion(&st->completion); > - > ret = ad7606_reset(st); > if (ret) > dev_warn(st->dev, "failed to RESET: no RESET GPIO > specified\n"); > @@ -635,7 +691,7 @@ int ad7606_probe(struct device *dev, int irq, void __iomem > *base_address, > return ret; > } > > - /* If convst pin is not defined, setup PWM*/ > + /* If convst pin is not defined, setup PWM */ > if (!st->gpio_convst) { > st->cnvst_pwm = devm_pwm_get(dev, NULL); > if (IS_ERR(st->cnvst_pwm)) > @@ -671,14 +727,25 @@ int ad7606_probe(struct device *dev, int irq, void > __iomem *base_address, > if (ret) > return ret; > } > - ret = devm_request_threaded_irq(dev, irq, > - NULL, > - &ad7606_interrupt, > - IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > - chip_info->name, indio_dev); > - if (ret) > - return ret; > > + if (st->bops->iio_backend_config) { > + st->bops->iio_backend_config(dev, indio_dev); > + indio_dev->setup_ops = &ad7606_pwm_buffer_ops; Ignoring error code > + } else { > + /* Reserve the PWM use only for backend (force gpio_convst > definition)*/ > + if (!st->gpio_convst) > + return dev_err_probe(dev, -EINVAL, > + "Convst pin must be defined when > not in backend mode"); > + > + init_completion(&st->completion); > + ret = devm_request_threaded_irq(dev, irq, > + NULL, > + &ad7606_interrupt, > + IRQF_TRIGGER_FALLING | > IRQF_ONESHOT, > + chip_info->name, indio_dev); > + if (ret) > + return ret; > + } Are we still calling devm_iio_triggered_buffer_setup() in case we have a backend device? > return devm_iio_device_register(dev, indio_dev); > } ... > > +#ifdef CONFIG_IIO_BACKEND Not a fan of this #ifef... It's not that much code so I would just select IIO_BACKEND for this driver. In fact, I don't think we can separately enable IIO_BACKEND in the menuconfig menu? > +static const struct iio_chan_spec ad7606b_bi_channels[] = { > + AD7606_BI_CHANNEL(0), > + AD7606_BI_CHANNEL(1), > + AD7606_BI_CHANNEL(2), > + AD7606_BI_CHANNEL(3), > + AD7606_BI_CHANNEL(4), > + AD7606_BI_CHANNEL(5), > + AD7606_BI_CHANNEL(6), > + AD7606_BI_CHANNEL(7), > +}; > + > +static int ad7606_bi_update_scan_mode(struct iio_dev *indio_dev, const > unsigned long *scan_mask) > +{ > + struct ad7606_state *st = iio_priv(indio_dev); > + unsigned int c, ret; > + > + for (c = 0; c < indio_dev->num_channels; c++) { > + if (test_bit(c, scan_mask)) > + ret = iio_backend_chan_enable(st->back, c); > + else > + ret = iio_backend_chan_disable(st->back, c); > + if (ret) > + return ret; > + } > + > + return 0; > +} > + > +static int ad7606_bi_setup_iio_backend(struct device *dev, struct iio_dev > *indio_dev) > +{ > + struct ad7606_state *st = iio_priv(indio_dev); > + unsigned int ret, c; > + > + st->back = devm_iio_backend_get(dev, NULL); > + if (IS_ERR(st->back)) > + return PTR_ERR(st->back); > + > + /* If the device is iio_backend powered the PWM is mandatory > */ > + if (!st->cnvst_pwm) > + return -EINVAL; > + > + ret = devm_iio_backend_request_buffer(dev, st->back, > indio_dev); > + if (ret) > + return ret; > + > + ret = devm_iio_backend_enable(dev, st->back); > + if (ret) > + return ret; > + > + struct iio_backend_data_fmt data = { > + .sign_extend = true, > + .enable = true, > + }; I would follow typical kernel coding style and have this declared at the beginning of the function. > + for (c = 0; c < indio_dev->num_channels; c++) { > + ret = iio_backend_data_format_set(st->back, c, > &data); > + if (ret) > + return ret; > + } > + > + indio_dev->channels = ad7606b_bi_channels; > + indio_dev->num_channels = 8; > + > + return 0; > +} > + > +static const struct ad7606_bus_ops ad7606_bi_bops = { > + .iio_backend_config = ad7606_bi_setup_iio_backend, > + .update_scan_mode = ad7606_bi_update_scan_mode, > +}; > +#endif > + > static int ad7606_par16_read_block(struct device *dev, > int count, void *buf) > { > @@ -52,7 +130,20 @@ static int ad7606_par_probe(struct platform_device *pdev) > void __iomem *addr; > resource_size_t remap_size; > int irq; > - > +#ifdef CONFIG_IIO_BACKEND > + struct iio_backend *back; > + > + /*For now, only the AD7606B is backend compatible.*/ > + if (chip_info->has_backend) { > + back = devm_iio_backend_get(&pdev->dev, NULL); > + if (IS_ERR(back)) > + return PTR_ERR(back); > + > + return ad7606_probe(&pdev->dev, 0, NULL, > + chip_info, > + &ad7606_bi_bops); > + } > +#endif Not sure I follow the above? You also get the backend in ad7606_bi_setup_iio_backend()? So it seems to be that the has_backend flag is not really needed? - Nuno Sá
On 8/17/24 17:47, Jonathan Cameron wrote: > On Thu, 15 Aug 2024 12:12:02 +0000 > Guillaume Stols <gstols@baylibre.com> wrote: > >> - Basic support for iio backend. >> - Supports IIO_CHAN_INFO_SAMP_FREQ R/W. >> - Only hardware mode is available, and that IIO_CHAN_INFO_RAW is not >> supported if iio-backend mode is selected. >> >> A small correction was added to the driver's file name in the Kconfig >> file's description. >> > I'm going to want Nuno's input on this. Given it's the summer that may > take a little while, so in meantime a few comments inline. > > Jonathan > > >> Signed-off-by: Guillaume Stols <gstols@baylibre.com> >> --- >> drivers/iio/adc/Kconfig | 3 +- >> drivers/iio/adc/ad7606.c | 103 +++++++++++++++++++++++++++++++++++-------- >> drivers/iio/adc/ad7606.h | 16 +++++++ >> drivers/iio/adc/ad7606_par.c | 98 +++++++++++++++++++++++++++++++++++++++- >> 4 files changed, 200 insertions(+), 20 deletions(-) >> >> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig >> index 88e8ce2e78b3..01248b6df868 100644 >> --- a/drivers/iio/adc/Kconfig >> +++ b/drivers/iio/adc/Kconfig >> @@ -227,9 +227,10 @@ config AD7606_IFACE_PARALLEL >> help >> Say yes here to build parallel interface support for Analog Devices: >> ad7605-4, ad7606, ad7606-6, ad7606-4 analog to digital converters (ADC). >> + It also support iio_backended devices for AD7606B. >> >> To compile this driver as a module, choose M here: the >> - module will be called ad7606_parallel. >> + module will be called ad7606_par. > If we can avoid a rename that would be good. Or was this always wrong? > If so spin a fix patch before this one. It was always wrong, will fix it with a separate patch. > >> >> config AD7606_IFACE_SPI >> tristate "Analog Devices AD7606 ADC driver with spi interface support" >> diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c >> index 99d5ca5c2348..a753d5caa9f8 100644 >> --- a/drivers/iio/adc/ad7606.c >> +++ b/drivers/iio/adc/ad7606.c >> @@ -21,6 +21,7 @@ >> #include <linux/util_macros.h> >> #include <linux/units.h> >> >> +#include <linux/iio/backend.h> >> #include <linux/iio/iio.h> >> #include <linux/iio/buffer.h> >> #include <linux/iio/sysfs.h> >> @@ -148,7 +149,15 @@ static int ad7606_set_sampling_freq(struct ad7606_state *st, unsigned long freq) >> >> static int ad7606_read_samples(struct ad7606_state *st) >> { >> - unsigned int num = st->chip_info->num_channels - 1; >> + unsigned int num = st->chip_info->num_channels; >> + >> + /* >> + * Timestamp channel does not contain sample, and no timestamp channel if >> + * backend is used. >> + */ >> + if (!st->back) >> + num--; >> + >> u16 *data = st->data; >> int ret; >> >> @@ -220,11 +229,15 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch) >> if (!ret) >> return ret; >> } >> - ret = wait_for_completion_timeout(&st->completion, >> - msecs_to_jiffies(1000)); >> - if (!ret) { >> - ret = -ETIMEDOUT; >> - goto error_ret; >> + >> + /* backend manages interruptions by itself.*/ >> + if (!st->back) { >> + ret = wait_for_completion_timeout(&st->completion, >> + msecs_to_jiffies(1000)); >> + if (!ret) { >> + ret = -ETIMEDOUT; >> + goto error_ret; >> + } >> } >> >> ret = ad7606_read_samples(st); >> @@ -271,6 +284,12 @@ static int ad7606_read_raw(struct iio_dev *indio_dev, >> case IIO_CHAN_INFO_OVERSAMPLING_RATIO: >> *val = st->oversampling; >> return IIO_VAL_INT; >> + case IIO_CHAN_INFO_SAMP_FREQ: >> + pwm_get_state_hw(st->cnvst_pwm, &cnvst_pwm_state); >> + /* If the PWM is swinging, return the real frequency, otherwise 0 */ > So this only exists for the pwm case. In that case can we split the channel definitions > into versions with an without this and register just the right one. > > A sampling frequency of 0 usually means no sampling, not that we can tell what it > is. If we can't tell don't provide the file. The file is provided only for the "backended" device (AD7606_BI_CHANNEL), BI being Backend Interface. This mode only works with PWM (and incidentally PWM is meant to be used only in conjuction with backend). When the PWM is not running because e.g sampling is not enabled, or PWM failed to start, I return 0. Shall I always return the configured value instead of the real one ? > > >> + *val = ad7606_pwm_is_swinging(st) ? >> + DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, cnvst_pwm_state.period) : 0; >> + return IIO_VAL_INT; >> } >> return -EINVAL; >> } >> @@ -360,6 +379,8 @@ static int ad7606_write_raw(struct iio_dev *indio_dev, >> return ret; >> >> return 0; >> + case IIO_CHAN_INFO_SAMP_FREQ: >> + return ad7606_set_sampling_freq(st, val); >> default: >> return -EINVAL; >> } > >> static const struct iio_trigger_ops ad7606_trigger_ops = { >> @@ -602,8 +660,6 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address, >> indio_dev->channels = st->chip_info->channels; >> indio_dev->num_channels = st->chip_info->num_channels; >> >> - init_completion(&st->completion); >> - >> ret = ad7606_reset(st); >> if (ret) >> dev_warn(st->dev, "failed to RESET: no RESET GPIO specified\n"); >> @@ -635,7 +691,7 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address, >> return ret; >> } >> >> - /* If convst pin is not defined, setup PWM*/ >> + /* If convst pin is not defined, setup PWM */ > Push into earlier patch. Check for this sort of fix being in wrong patch > before sending a series. It just adds noise to patch and to reviews. Will do. Sorry for that. > >> if (!st->gpio_convst) { >> st->cnvst_pwm = devm_pwm_get(dev, NULL); >> if (IS_ERR(st->cnvst_pwm)) > ... > >> diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h >> index aab8fefb84be..9a098cd77812 100644 >> --- a/drivers/iio/adc/ad7606.h >> +++ b/drivers/iio/adc/ad7606.h >> @@ -34,6 +34,12 @@ >> BIT(IIO_CHAN_INFO_SCALE), \ >> BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO)) >> >> +#define AD7606_BI_CHANNEL(num) \ >> + AD760X_CHANNEL(num, 0, \ >> + BIT(IIO_CHAN_INFO_SCALE), \ >> + BIT(IIO_CHAN_INFO_SAMP_FREQ) | \ >> + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO)) >> + >> #define AD7616_CHANNEL(num) \ >> AD760X_CHANNEL(num, BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),\ >> 0, BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO)) >> @@ -61,6 +67,7 @@ enum ad7606_supported_device_ids { >> * @os_req_reset some devices require a reset to update oversampling >> * @init_delay_ms required delay in miliseconds for initialization >> * after a restart >> + * @has_backend defines if a backend is available for the given chip > That seems to me more of a case of does the driver support it. > Linux kernel code has no way of knowing if a backend hardware exists > or not. Modify the comment to speak about if we know it works. > > Or is there something fundamental that stops the backend approach > working with some devices? > > Why does the driver need this flag? Potentially, I think any of those parts can have a backend and moreover, I don't see anything preventing any ADC to have a backend. I introduced the flag as a way to differentiate the "new" way of supporting parallel interface, i.e using backend, from the "old" way (using port I/O). There is a concurrency between the old implementation using port I/O and the new one using iio-backend, because they are both "platform", so the initial idea was that it would not make sense and be dangerous to look for a backend for the parts that have no existing (i'd rather say, like you pointed out, supported) backend. Having a second thought at it, the dt bindings already permits only io-backend property to be populated for the parts that actually have a backend, hence one of these is superfluous, or maybe even both are and the user is responsible for setting the right value in the dts. Any advice ? > > >> */ >> struct ad7606_chip_info { >> enum ad7606_supported_device_ids id; >> @@ -71,6 +78,7 @@ struct ad7606_chip_info { >> unsigned int oversampling_num; >> bool os_req_reset; >> unsigned long init_delay_ms; >> + bool has_backend; >> }; >> > >> diff --git a/drivers/iio/adc/ad7606_par.c b/drivers/iio/adc/ad7606_par.c >> index d83c0edc1e31..5c8a04556e25 100644 >> --- a/drivers/iio/adc/ad7606_par.c >> +++ b/drivers/iio/adc/ad7606_par.c >> @@ -3,6 +3,8 @@ >> * AD7606 Parallel Interface ADC driver >> * >> * Copyright 2011 Analog Devices Inc. >> + * Copyright 2024 Analog Devices Inc. >> + * Copyright 2024 BayLibre SAS. >> */ >> >> #include <linux/mod_devicetable.h> >> @@ -11,10 +13,86 @@ >> #include <linux/types.h> >> #include <linux/err.h> >> #include <linux/io.h> >> +#include <linux/pwm.h> >> +#include <linux/gpio.h> >> +#include <linux/delay.h> > Not on any order currently but try to minimize a future series > that might clean this up. Preference is alphabetical though fine > to have a trailing block of IIO headers, then driver specific > ones after that. You can either fix the current order in a > separate patch, or just put your new headers in approximately the write place. np, will fix that in a separate patch. > >> >> #include <linux/iio/iio.h> >> +#include <linux/iio/backend.h> >> #include "ad7606.h" > > >> +static int ad7606_bi_setup_iio_backend(struct device *dev, struct iio_dev *indio_dev) >> +{ >> + struct ad7606_state *st = iio_priv(indio_dev); > Why 2 tabs? by mistake. >> + unsigned int ret, c; >> + >> + st->back = devm_iio_backend_get(dev, NULL); >> + if (IS_ERR(st->back)) >> + return PTR_ERR(st->back); >> + >> + /* If the device is iio_backend powered the PWM is mandatory */ >> + if (!st->cnvst_pwm) >> + return -EINVAL; >> + >> + ret = devm_iio_backend_request_buffer(dev, st->back, indio_dev); >> + if (ret) >> + return ret; >> + >> + ret = devm_iio_backend_enable(dev, st->back); >> + if (ret) >> + return ret; >> + >> + struct iio_backend_data_fmt data = { >> + .sign_extend = true, >> + .enable = true, >> + }; >> + for (c = 0; c < indio_dev->num_channels; c++) { >> + ret = iio_backend_data_format_set(st->back, c, &data); >> + if (ret) >> + return ret; >> + } >> + >> + indio_dev->channels = ad7606b_bi_channels; >> + indio_dev->num_channels = 8; >> + >> + return 0; >> +} >> + >> +static const struct ad7606_bus_ops ad7606_bi_bops = { >> + .iio_backend_config = ad7606_bi_setup_iio_backend, >> + .update_scan_mode = ad7606_bi_update_scan_mode, >> +}; >> +#endif >> + >> static int ad7606_par16_read_block(struct device *dev, >> int count, void *buf) >> { >> @@ -52,7 +130,20 @@ static int ad7606_par_probe(struct platform_device *pdev) >> void __iomem *addr; >> resource_size_t remap_size; >> int irq; >> - >> +#ifdef CONFIG_IIO_BACKEND >> + struct iio_backend *back; >> + >> + /*For now, only the AD7606B is backend compatible.*/ > /* For ... ble. */ again :-/ > >> + if (chip_info->has_backend) { >> + back = devm_iio_backend_get(&pdev->dev, NULL); >> + if (IS_ERR(back)) >> + return PTR_ERR(back); >> + >> + return ad7606_probe(&pdev->dev, 0, NULL, >> + chip_info, >> + &ad7606_bi_bops); > Short wrap. Aim for 80 char limit unless it hurts readability a lot. ok > >> + } >> +#endif >> irq = platform_get_irq(pdev, 0); >> if (irq < 0) >> return irq; >> @@ -74,6 +165,7 @@ static const struct platform_device_id ad7606_driver_ids[] = { >> { .name = "ad7606-4", .driver_data = (kernel_ulong_t)&ad7606_4_info, }, >> { .name = "ad7606-6", .driver_data = (kernel_ulong_t)&ad7606_6_info, }, >> { .name = "ad7606-8", .driver_data = (kernel_ulong_t)&ad7606_8_info, }, >> + { .name = "ad7606b", .driver_data = (kernel_ulong_t)&ad7606b_info, }, >> { } >> }; >> MODULE_DEVICE_TABLE(platform, ad7606_driver_ids); >> @@ -83,6 +175,7 @@ static const struct of_device_id ad7606_of_match[] = { >> { .compatible = "adi,ad7606-4", .data = &ad7606_4_info }, >> { .compatible = "adi,ad7606-6", .data = &ad7606_6_info }, >> { .compatible = "adi,ad7606-8", .data = &ad7606_8_info }, >> + { .compatible = "adi,ad7606b", .data = &ad7606b_info }, >> { } >> }; >> MODULE_DEVICE_TABLE(of, ad7606_of_match); >> @@ -102,3 +195,6 @@ MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>"); >> MODULE_DESCRIPTION("Analog Devices AD7606 ADC"); >> MODULE_LICENSE("GPL v2"); >> MODULE_IMPORT_NS(IIO_AD7606); >> +#ifdef CONFIG_IIO_BACKEND >> +MODULE_IMPORT_NS(IIO_BACKEND); > I'd not bother with config guards. Importing a namespace we don't > use should be harmless. OK, will remove it. According to Nuno's feedback, I could also force the selection of CONFIG_IIO_BACKEND with the driver, which IMHO is not a bad idea, as it would allow to remove all those ifdefs. > >> +#endif >>
On 9/5/24 10:40, Nuno Sá wrote: > On Thu, 2024-08-15 at 12:12 +0000, Guillaume Stols wrote: >> - Basic support for iio backend. >> - Supports IIO_CHAN_INFO_SAMP_FREQ R/W. >> - Only hardware mode is available, and that IIO_CHAN_INFO_RAW is not >> supported if iio-backend mode is selected. >> >> A small correction was added to the driver's file name in the Kconfig >> file's description. >> >> Signed-off-by: Guillaume Stols <gstols@baylibre.com> >> --- > Hi Guillaume, > > Some initial feedback from me... > >> drivers/iio/adc/Kconfig | 3 +- >> drivers/iio/adc/ad7606.c | 103 +++++++++++++++++++++++++++++++++++------- >> - >> drivers/iio/adc/ad7606.h | 16 +++++++ >> drivers/iio/adc/ad7606_par.c | 98 +++++++++++++++++++++++++++++++++++++++- >> 4 files changed, 200 insertions(+), 20 deletions(-) >> >> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig >> index 88e8ce2e78b3..01248b6df868 100644 >> --- a/drivers/iio/adc/Kconfig >> +++ b/drivers/iio/adc/Kconfig >> @@ -227,9 +227,10 @@ config AD7606_IFACE_PARALLEL >> help >> Say yes here to build parallel interface support for Analog >> Devices: >> ad7605-4, ad7606, ad7606-6, ad7606-4 analog to digital converters >> (ADC). >> + It also support iio_backended devices for AD7606B. >> >> To compile this driver as a module, choose M here: the >> - module will be called ad7606_parallel. >> + module will be called ad7606_par. >> >> config AD7606_IFACE_SPI >> tristate "Analog Devices AD7606 ADC driver with spi interface >> support" >> diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c >> index 99d5ca5c2348..a753d5caa9f8 100644 >> --- a/drivers/iio/adc/ad7606.c >> +++ b/drivers/iio/adc/ad7606.c >> @@ -21,6 +21,7 @@ >> #include <linux/util_macros.h> >> #include <linux/units.h> >> + >> + /* backend manages interruptions by itself.*/ > missing space before closing the comment (also not sure the comments adds much) thx, will check again > >> + if (!st->back) { >> + ret = wait_for_completion_timeout(&st->completion, >> + msecs_to_jiffies(1000)); >> + if (!ret) { >> + ret = -ETIMEDOUT; >> + goto error_ret; >> + } >> } >> >> ret = ad7606_read_samples(st); >> @@ -271,6 +284,12 @@ static int ad7606_read_raw(struct iio_dev *indio_dev, >> case IIO_CHAN_INFO_OVERSAMPLING_RATIO: >> *val = st->oversampling; >> return IIO_VAL_INT; >> + case IIO_CHAN_INFO_SAMP_FREQ: >> + pwm_get_state_hw(st->cnvst_pwm, &cnvst_pwm_state); >> + /* If the PWM is swinging, return the real frequency, >> otherwise 0 */ >> + *val = ad7606_pwm_is_swinging(st) ? >> + DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, >> cnvst_pwm_state.period) : 0; >> + return IIO_VAL_INT; >> } >> return -EINVAL; >> } >> @@ -360,6 +379,8 @@ static int ad7606_write_raw(struct iio_dev *indio_dev, >> return ret; >> >> return 0; >> + case IIO_CHAN_INFO_SAMP_FREQ: >> + return ad7606_set_sampling_freq(st, val); >> default: >> return -EINVAL; >> } >> @@ -482,7 +503,6 @@ static int ad7606_buffer_postenable(struct iio_dev >> *indio_dev) >> struct ad7606_state *st = iio_priv(indio_dev); >> >> gpiod_set_value(st->gpio_convst, 1); >> - ad7606_pwm_set_swing(st); >> >> return 0; >> } >> @@ -492,19 +512,53 @@ static int ad7606_buffer_predisable(struct iio_dev >> *indio_dev) >> struct ad7606_state *st = iio_priv(indio_dev); >> >> gpiod_set_value(st->gpio_convst, 0); >> - ad7606_pwm_set_low(st); >> >> return 0; >> } >> >> +static int ad7606_pwm_buffer_postenable(struct iio_dev *indio_dev) >> +{ >> + struct ad7606_state *st = iio_priv(indio_dev); >> + >> + return ad7606_pwm_set_swing(st); >> +} >> + >> +static int ad7606_pwm_buffer_predisable(struct iio_dev *indio_dev) >> +{ >> + struct ad7606_state *st = iio_priv(indio_dev); >> + >> + return ad7606_pwm_set_low(st); >> +} > Maybe I'm missing something but are we removing the gpiod calls? Well actually the pwm is meant to be used only with backend. Though it could be used without it, I dont think it is a very good idea because interrupt handling + transmission init takes quite some time, and a new rising edge could happen before the current samples are effectively transferred. However, since PWM and backend are two separate things, I wanted to show an usage for the PWM when introducing it, and one way to do it was to use it to emulate a GPIO by setting the duty cycle 100% for having a 1 (set_high) and 0% for having a 0 (set_low). Then on this patch, since we introduce iio-backend, I removed this 'mock' usage of it. But I think that I should separate the removal into an additional patch to avoid confusions. Or shall I just remove the mock usage from the PWM patch ? >> + >> +static int ad7606_update_scan_mode(struct iio_dev *indio_dev, >> + const unsigned long *scan_mask) >> +{ >> + struct ad7606_state *st = iio_priv(indio_dev); >> + >> + /* The update scan mode is only for iio backend compatible drivers. >> + * If the specific update_scan_mode is not defined in the bus ops, >> + * just do nothing and return 0. >> + */ >> + if (st->bops->update_scan_mode) >> + return st->bops->update_scan_mode(indio_dev, scan_mask); >> + else >> + return 0; > Redundant else ack >> - if (ret) >> - return ret; >> >> + if (st->bops->iio_backend_config) { >> + st->bops->iio_backend_config(dev, indio_dev); >> + indio_dev->setup_ops = &ad7606_pwm_buffer_ops; > Ignoring error code will handle > >> + } else { >> + /* Reserve the PWM use only for backend (force gpio_convst >> definition)*/ >> + if (!st->gpio_convst) >> + return dev_err_probe(dev, -EINVAL, >> + "Convst pin must be defined when >> not in backend mode"); >> + >> + init_completion(&st->completion); >> + ret = devm_request_threaded_irq(dev, irq, >> + NULL, >> + &ad7606_interrupt, >> + IRQF_TRIGGER_FALLING | >> IRQF_ONESHOT, >> + chip_info->name, indio_dev); >> + if (ret) >> + return ret; >> + } > Are we still calling devm_iio_triggered_buffer_setup() in case we have a backend > device? No, this portion of code is only executed if convst is defined (conversion trigger GPIO), which is not the case if there is a backend. > >> return devm_iio_device_register(dev, indio_dev); >> } > ... > >> +#ifdef CONFIG_IIO_BACKEND > Not a fan of this #ifef... It's not that much code so I would just select > IIO_BACKEND for this driver. In fact, I don't think we can separately enable > IIO_BACKEND in the menuconfig menu? OK I can do it that way. >> +static int ad7606_bi_setup_iio_backend(struct device *dev, struct iio_dev >> *indio_dev) >> +{ >> + struct ad7606_state *st = iio_priv(indio_dev); >> + unsigned int ret, c; >> + >> + st->back = devm_iio_backend_get(dev, NULL); >> + if (IS_ERR(st->back)) >> + return PTR_ERR(st->back); >> + >> + /* If the device is iio_backend powered the PWM is mandatory >> */ >> + if (!st->cnvst_pwm) >> + return -EINVAL; >> + >> + ret = devm_iio_backend_request_buffer(dev, st->back, >> indio_dev); >> + if (ret) >> + return ret; >> + >> + ret = devm_iio_backend_enable(dev, st->back); >> + if (ret) >> + return ret; >> + >> + struct iio_backend_data_fmt data = { >> + .sign_extend = true, >> + .enable = true, >> + }; > I would follow typical kernel coding style and have this declared at the > beginning of the function. aouch, yes ! >> - >> +#ifdef CONFIG_IIO_BACKEND >> + struct iio_backend *back; >> + >> + /*For now, only the AD7606B is backend compatible.*/ >> + if (chip_info->has_backend) { >> + back = devm_iio_backend_get(&pdev->dev, NULL); >> + if (IS_ERR(back)) >> + return PTR_ERR(back); >> + >> + return ad7606_probe(&pdev->dev, 0, NULL, >> + chip_info, >> + &ad7606_bi_bops); >> + } >> +#endif > Not sure I follow the above? You also get the backend in > ad7606_bi_setup_iio_backend()? So it seems to be that the has_backend flag is > not really needed? The first call to devm_iio_backend_get checks if there is a backend available, and if so the backend bops (ad7606_bi_bops)are passed to the generic probe function. At this stage, the backend cannot be stored in the ad7606_state structure because it is not initialized yet, it will be in the generic probe function, hence the second call. The has_backend flag is discussed in my answer to Jonathan's comment, and will probably be removed. > > - Nuno Sá > >
On Thu, 2024-09-12 at 12:13 +0200, Guillaume Stols wrote: > On 9/5/24 10:40, Nuno Sá wrote: > > On Thu, 2024-08-15 at 12:12 +0000, Guillaume Stols wrote: > > > - Basic support for iio backend. > > > - Supports IIO_CHAN_INFO_SAMP_FREQ R/W. > > > - Only hardware mode is available, and that IIO_CHAN_INFO_RAW is not > > > supported if iio-backend mode is selected. > > > > > > A small correction was added to the driver's file name in the Kconfig > > > file's description. > > > > > > Signed-off-by: Guillaume Stols <gstols@baylibre.com> > > > --- > > Hi Guillaume, > > > > Some initial feedback from me... > > > > > drivers/iio/adc/Kconfig | 3 +- > > > drivers/iio/adc/ad7606.c | 103 +++++++++++++++++++++++++++++++++++------- > > > - > > > drivers/iio/adc/ad7606.h | 16 +++++++ > > > drivers/iio/adc/ad7606_par.c | 98 +++++++++++++++++++++++++++++++++++++++- > > > 4 files changed, 200 insertions(+), 20 deletions(-) > > > > > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > > > index 88e8ce2e78b3..01248b6df868 100644 > > > --- a/drivers/iio/adc/Kconfig > > > +++ b/drivers/iio/adc/Kconfig > > > @@ -227,9 +227,10 @@ config AD7606_IFACE_PARALLEL > > > help > > > Say yes here to build parallel interface support for Analog > > > Devices: > > > ad7605-4, ad7606, ad7606-6, ad7606-4 analog to digital converters > > > (ADC). > > > + It also support iio_backended devices for AD7606B. > > > > > > To compile this driver as a module, choose M here: the > > > - module will be called ad7606_parallel. > > > + module will be called ad7606_par. > > > > > > config AD7606_IFACE_SPI > > > tristate "Analog Devices AD7606 ADC driver with spi interface > > > support" > > > diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c > > > index 99d5ca5c2348..a753d5caa9f8 100644 > > > --- a/drivers/iio/adc/ad7606.c > > > +++ b/drivers/iio/adc/ad7606.c > > > @@ -21,6 +21,7 @@ > > > #include <linux/util_macros.h> > > > #include <linux/units.h> > > > + > > > + /* backend manages interruptions by itself.*/ > > missing space before closing the comment (also not sure the comments adds much) > > > thx, will check again > > > > > > > + if (!st->back) { > > > + ret = wait_for_completion_timeout(&st->completion, > > > + msecs_to_jiffies(1000)); > > > + if (!ret) { > > > + ret = -ETIMEDOUT; > > > + goto error_ret; > > > + } > > > } > > > > > > ret = ad7606_read_samples(st); > > > @@ -271,6 +284,12 @@ static int ad7606_read_raw(struct iio_dev *indio_dev, > > > case IIO_CHAN_INFO_OVERSAMPLING_RATIO: > > > *val = st->oversampling; > > > return IIO_VAL_INT; > > > + case IIO_CHAN_INFO_SAMP_FREQ: > > > + pwm_get_state_hw(st->cnvst_pwm, &cnvst_pwm_state); > > > + /* If the PWM is swinging, return the real frequency, > > > otherwise 0 */ > > > + *val = ad7606_pwm_is_swinging(st) ? > > > + DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, > > > cnvst_pwm_state.period) : 0; > > > + return IIO_VAL_INT; > > > } > > > return -EINVAL; > > > } > > > @@ -360,6 +379,8 @@ static int ad7606_write_raw(struct iio_dev *indio_dev, > > > return ret; > > > > > > return 0; > > > + case IIO_CHAN_INFO_SAMP_FREQ: > > > + return ad7606_set_sampling_freq(st, val); > > > default: > > > return -EINVAL; > > > } > > > @@ -482,7 +503,6 @@ static int ad7606_buffer_postenable(struct iio_dev > > > *indio_dev) > > > struct ad7606_state *st = iio_priv(indio_dev); > > > > > > gpiod_set_value(st->gpio_convst, 1); > > > - ad7606_pwm_set_swing(st); > > > > > > return 0; > > > } > > > @@ -492,19 +512,53 @@ static int ad7606_buffer_predisable(struct iio_dev > > > *indio_dev) > > > struct ad7606_state *st = iio_priv(indio_dev); > > > > > > gpiod_set_value(st->gpio_convst, 0); > > > - ad7606_pwm_set_low(st); > > > > > > return 0; > > > } > > > > > > +static int ad7606_pwm_buffer_postenable(struct iio_dev *indio_dev) > > > +{ > > > + struct ad7606_state *st = iio_priv(indio_dev); > > > + > > > + return ad7606_pwm_set_swing(st); > > > +} > > > + > > > +static int ad7606_pwm_buffer_predisable(struct iio_dev *indio_dev) > > > +{ > > > + struct ad7606_state *st = iio_priv(indio_dev); > > > + > > > + return ad7606_pwm_set_low(st); > > > +} > > Maybe I'm missing something but are we removing the gpiod calls? > > > Well actually the pwm is meant to be used only with backend. Though it > could be used without it, I dont think it is a very good idea because > interrupt handling + transmission init takes quite some time, and a new > rising edge could happen before the current samples are effectively > transferred. However, since PWM and backend are two separate things, I > wanted to show an usage for the PWM when introducing it, and one way to > do it was to use it to emulate a GPIO by setting the duty cycle 100% for > having a 1 (set_high) and 0% for having a 0 (set_low). Then on this > patch, since we introduce iio-backend, I removed this 'mock' usage of it. > > But I think that I should separate the removal into an additional patch > to avoid confusions. Or shall I just remove the mock usage from the PWM > patch ? > > Yeah, probably better (with a proper commit message explaining the reasoning) > > > + > > > +static int ad7606_update_scan_mode(struct iio_dev *indio_dev, > > > + const unsigned long *scan_mask) > > > +{ > > > + struct ad7606_state *st = iio_priv(indio_dev); > > > + > > > + /* The update scan mode is only for iio backend compatible drivers. > > > + * If the specific update_scan_mode is not defined in the bus ops, > > > + * just do nothing and return 0. > > > + */ > > > + if (st->bops->update_scan_mode) > > > + return st->bops->update_scan_mode(indio_dev, scan_mask); > > > + else > > > + return 0; > > Redundant else > > > ack > > > > - if (ret) > > > - return ret; > > > > > > + if (st->bops->iio_backend_config) { > > > + st->bops->iio_backend_config(dev, indio_dev); > > > + indio_dev->setup_ops = &ad7606_pwm_buffer_ops; > > Ignoring error code > > > will handle > > > > > > > + } else { > > > + /* Reserve the PWM use only for backend (force gpio_convst > > > definition)*/ > > > + if (!st->gpio_convst) > > > + return dev_err_probe(dev, -EINVAL, > > > + "Convst pin must be defined when > > > not in backend mode"); > > > + > > > + init_completion(&st->completion); > > > + ret = devm_request_threaded_irq(dev, irq, > > > + NULL, > > > + &ad7606_interrupt, > > > + IRQF_TRIGGER_FALLING | > > > IRQF_ONESHOT, > > > + chip_info->name, indio_dev); > > > + if (ret) > > > + return ret; > > > + } > > Are we still calling devm_iio_triggered_buffer_setup() in case we have a backend > > device? > > > No, this portion of code is only executed if convst is defined > (conversion trigger GPIO), which is not the case if there is a backend. > > > > > > > return devm_iio_device_register(dev, indio_dev); > > > } > > ... > > > > > +#ifdef CONFIG_IIO_BACKEND > > Not a fan of this #ifef... It's not that much code so I would just select > > IIO_BACKEND for this driver. In fact, I don't think we can separately enable > > IIO_BACKEND in the menuconfig menu? > > > OK I can do it that way. > > > > +static int ad7606_bi_setup_iio_backend(struct device *dev, struct iio_dev > > > *indio_dev) > > > +{ > > > + struct ad7606_state *st = iio_priv(indio_dev); > > > + unsigned int ret, c; > > > + > > > + st->back = devm_iio_backend_get(dev, NULL); > > > + if (IS_ERR(st->back)) > > > + return PTR_ERR(st->back); > > > + > > > + /* If the device is iio_backend powered the PWM is mandatory > > > */ > > > + if (!st->cnvst_pwm) > > > + return -EINVAL; > > > + > > > + ret = devm_iio_backend_request_buffer(dev, st->back, > > > indio_dev); > > > + if (ret) > > > + return ret; > > > + > > > + ret = devm_iio_backend_enable(dev, st->back); > > > + if (ret) > > > + return ret; > > > + > > > + struct iio_backend_data_fmt data = { > > > + .sign_extend = true, > > > + .enable = true, > > > + }; > > I would follow typical kernel coding style and have this declared at the > > beginning of the function. > > > aouch, yes ! > > > > > - > > > +#ifdef CONFIG_IIO_BACKEND > > > + struct iio_backend *back; > > > + > > > + /*For now, only the AD7606B is backend compatible.*/ > > > + if (chip_info->has_backend) { > > > + back = devm_iio_backend_get(&pdev->dev, NULL); > > > + if (IS_ERR(back)) > > > + return PTR_ERR(back); > > > + > > > + return ad7606_probe(&pdev->dev, 0, NULL, > > > + chip_info, > > > + &ad7606_bi_bops); > > > + } > > > +#endif > > Not sure I follow the above? You also get the backend in > > ad7606_bi_setup_iio_backend()? So it seems to be that the has_backend flag is > > not really needed? > > > The first call to devm_iio_backend_get checks if there is a backend > available, and if so the backend bops (ad7606_bi_bops)are passed to the > generic probe function. > Why not checking for the presence of the DT property? We should only get the backend when ready for that. - Nuno Sá >
... > >> > >> ret = ad7606_read_samples(st); > >> @@ -271,6 +284,12 @@ static int ad7606_read_raw(struct iio_dev *indio_dev, > >> case IIO_CHAN_INFO_OVERSAMPLING_RATIO: > >> *val = st->oversampling; > >> return IIO_VAL_INT; > >> + case IIO_CHAN_INFO_SAMP_FREQ: > >> + pwm_get_state_hw(st->cnvst_pwm, &cnvst_pwm_state); > >> + /* If the PWM is swinging, return the real frequency, otherwise 0 */ > > So this only exists for the pwm case. In that case can we split the channel definitions > > into versions with an without this and register just the right one. > > > > A sampling frequency of 0 usually means no sampling, not that we can tell what it > > is. If we can't tell don't provide the file. > > The file is provided only for the "backended" device > (AD7606_BI_CHANNEL), BI being Backend Interface. This mode only works > with PWM (and incidentally PWM is meant to be used only in conjuction > with backend). > > When the PWM is not running because e.g sampling is not enabled, or PWM > failed to start, I return 0. Shall I always return the configured value > instead of the real one ? Yes. Configured should be fine I think if there is no way to ask 'what will it be when I turn it on'. > >> diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h > >> index aab8fefb84be..9a098cd77812 100644 > >> --- a/drivers/iio/adc/ad7606.h > >> +++ b/drivers/iio/adc/ad7606.h > >> @@ -34,6 +34,12 @@ > >> BIT(IIO_CHAN_INFO_SCALE), \ > >> BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO)) > >> > >> +#define AD7606_BI_CHANNEL(num) \ > >> + AD760X_CHANNEL(num, 0, \ > >> + BIT(IIO_CHAN_INFO_SCALE), \ > >> + BIT(IIO_CHAN_INFO_SAMP_FREQ) | \ > >> + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO)) > >> + > >> #define AD7616_CHANNEL(num) \ > >> AD760X_CHANNEL(num, BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),\ > >> 0, BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO)) > >> @@ -61,6 +67,7 @@ enum ad7606_supported_device_ids { > >> * @os_req_reset some devices require a reset to update oversampling > >> * @init_delay_ms required delay in miliseconds for initialization > >> * after a restart > >> + * @has_backend defines if a backend is available for the given chip > > That seems to me more of a case of does the driver support it. > > Linux kernel code has no way of knowing if a backend hardware exists > > or not. Modify the comment to speak about if we know it works. > > > > Or is there something fundamental that stops the backend approach > > working with some devices? > > > > Why does the driver need this flag? > > Potentially, I think any of those parts can have a backend and moreover, > I don't see anything preventing any ADC to have a backend. > > I introduced the flag as a way to differentiate the "new" way of > supporting parallel interface, i.e using backend, from the "old" way > (using port I/O). > > There is a concurrency between the old implementation using port I/O and > the new one using iio-backend, because they are both "platform", so the > initial idea was that it would not make sense and be dangerous to look > for a backend for the parts that have no existing (i'd rather say, like > you pointed out, supported) backend. > > Having a second thought at it, the dt bindings already permits only > io-backend property to be populated for the parts that actually have a > backend, hence one of these is superfluous, or maybe even both are and > the user is responsible for setting the right value in the dts. Any advice ? Dt binding should be enough. The worst that happens is the driver tries to use an unsupported backend and that will fail I hope. So I wouldn't have this driver try to stop it. > >> diff --git a/drivers/iio/adc/ad7606_par.c b/drivers/iio/adc/ad7606_par.c > >> index d83c0edc1e31..5c8a04556e25 100644 > >> --- a/drivers/iio/adc/ad7606_par.c > >> +++ b/drivers/iio/adc/ad7606_par.c > >> @@ -102,3 +195,6 @@ MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>"); > >> MODULE_DESCRIPTION("Analog Devices AD7606 ADC"); > >> MODULE_LICENSE("GPL v2"); > >> MODULE_IMPORT_NS(IIO_AD7606); > >> +#ifdef CONFIG_IIO_BACKEND > >> +MODULE_IMPORT_NS(IIO_BACKEND); > > I'd not bother with config guards. Importing a namespace we don't > > use should be harmless. > OK, will remove it. According to Nuno's feedback, I could also force the > selection of CONFIG_IIO_BACKEND with the driver, which IMHO is not a bad > idea, as it would allow to remove all those ifdefs. Hmm. I guess the questions is whether that is a bloat anyone will worry about who is using the old way for this device. I guess that's a problem for Analog folk if their customers complain. We can always relax this in future so for now select IIO_BACKEND is fine by me. Jonathan
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig index 88e8ce2e78b3..01248b6df868 100644 --- a/drivers/iio/adc/Kconfig +++ b/drivers/iio/adc/Kconfig @@ -227,9 +227,10 @@ config AD7606_IFACE_PARALLEL help Say yes here to build parallel interface support for Analog Devices: ad7605-4, ad7606, ad7606-6, ad7606-4 analog to digital converters (ADC). + It also support iio_backended devices for AD7606B. To compile this driver as a module, choose M here: the - module will be called ad7606_parallel. + module will be called ad7606_par. config AD7606_IFACE_SPI tristate "Analog Devices AD7606 ADC driver with spi interface support" diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c index 99d5ca5c2348..a753d5caa9f8 100644 --- a/drivers/iio/adc/ad7606.c +++ b/drivers/iio/adc/ad7606.c @@ -21,6 +21,7 @@ #include <linux/util_macros.h> #include <linux/units.h> +#include <linux/iio/backend.h> #include <linux/iio/iio.h> #include <linux/iio/buffer.h> #include <linux/iio/sysfs.h> @@ -148,7 +149,15 @@ static int ad7606_set_sampling_freq(struct ad7606_state *st, unsigned long freq) static int ad7606_read_samples(struct ad7606_state *st) { - unsigned int num = st->chip_info->num_channels - 1; + unsigned int num = st->chip_info->num_channels; + + /* + * Timestamp channel does not contain sample, and no timestamp channel if + * backend is used. + */ + if (!st->back) + num--; + u16 *data = st->data; int ret; @@ -220,11 +229,15 @@ static int ad7606_scan_direct(struct iio_dev *indio_dev, unsigned int ch) if (!ret) return ret; } - ret = wait_for_completion_timeout(&st->completion, - msecs_to_jiffies(1000)); - if (!ret) { - ret = -ETIMEDOUT; - goto error_ret; + + /* backend manages interruptions by itself.*/ + if (!st->back) { + ret = wait_for_completion_timeout(&st->completion, + msecs_to_jiffies(1000)); + if (!ret) { + ret = -ETIMEDOUT; + goto error_ret; + } } ret = ad7606_read_samples(st); @@ -271,6 +284,12 @@ static int ad7606_read_raw(struct iio_dev *indio_dev, case IIO_CHAN_INFO_OVERSAMPLING_RATIO: *val = st->oversampling; return IIO_VAL_INT; + case IIO_CHAN_INFO_SAMP_FREQ: + pwm_get_state_hw(st->cnvst_pwm, &cnvst_pwm_state); + /* If the PWM is swinging, return the real frequency, otherwise 0 */ + *val = ad7606_pwm_is_swinging(st) ? + DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, cnvst_pwm_state.period) : 0; + return IIO_VAL_INT; } return -EINVAL; } @@ -360,6 +379,8 @@ static int ad7606_write_raw(struct iio_dev *indio_dev, return ret; return 0; + case IIO_CHAN_INFO_SAMP_FREQ: + return ad7606_set_sampling_freq(st, val); default: return -EINVAL; } @@ -482,7 +503,6 @@ static int ad7606_buffer_postenable(struct iio_dev *indio_dev) struct ad7606_state *st = iio_priv(indio_dev); gpiod_set_value(st->gpio_convst, 1); - ad7606_pwm_set_swing(st); return 0; } @@ -492,19 +512,53 @@ static int ad7606_buffer_predisable(struct iio_dev *indio_dev) struct ad7606_state *st = iio_priv(indio_dev); gpiod_set_value(st->gpio_convst, 0); - ad7606_pwm_set_low(st); return 0; } +static int ad7606_pwm_buffer_postenable(struct iio_dev *indio_dev) +{ + struct ad7606_state *st = iio_priv(indio_dev); + + return ad7606_pwm_set_swing(st); +} + +static int ad7606_pwm_buffer_predisable(struct iio_dev *indio_dev) +{ + struct ad7606_state *st = iio_priv(indio_dev); + + return ad7606_pwm_set_low(st); +} + +static int ad7606_update_scan_mode(struct iio_dev *indio_dev, + const unsigned long *scan_mask) +{ + struct ad7606_state *st = iio_priv(indio_dev); + + /* The update scan mode is only for iio backend compatible drivers. + * If the specific update_scan_mode is not defined in the bus ops, + * just do nothing and return 0. + */ + if (st->bops->update_scan_mode) + return st->bops->update_scan_mode(indio_dev, scan_mask); + else + return 0; +} + static const struct iio_buffer_setup_ops ad7606_buffer_ops = { .postenable = &ad7606_buffer_postenable, .predisable = &ad7606_buffer_predisable, }; +static const struct iio_buffer_setup_ops ad7606_pwm_buffer_ops = { + .postenable = &ad7606_pwm_buffer_postenable, + .predisable = &ad7606_pwm_buffer_predisable, +}; + static const struct iio_info ad7606_info_no_os_or_range = { .read_raw = &ad7606_read_raw, .validate_trigger = &ad7606_validate_trigger, + .update_scan_mode = &ad7606_update_scan_mode, }; static const struct iio_info ad7606_info_os_and_range = { @@ -512,6 +566,7 @@ static const struct iio_info ad7606_info_os_and_range = { .write_raw = &ad7606_write_raw, .attrs = &ad7606_attribute_group_os_and_range, .validate_trigger = &ad7606_validate_trigger, + .update_scan_mode = &ad7606_update_scan_mode, }; static const struct iio_info ad7606_info_os_range_and_debug = { @@ -520,6 +575,7 @@ static const struct iio_info ad7606_info_os_range_and_debug = { .debugfs_reg_access = &ad7606_reg_access, .attrs = &ad7606_attribute_group_os_and_range, .validate_trigger = &ad7606_validate_trigger, + .update_scan_mode = &ad7606_update_scan_mode, }; static const struct iio_info ad7606_info_os = { @@ -527,6 +583,7 @@ static const struct iio_info ad7606_info_os = { .write_raw = &ad7606_write_raw, .attrs = &ad7606_attribute_group_os, .validate_trigger = &ad7606_validate_trigger, + .update_scan_mode = &ad7606_update_scan_mode, }; static const struct iio_info ad7606_info_range = { @@ -534,6 +591,7 @@ static const struct iio_info ad7606_info_range = { .write_raw = &ad7606_write_raw, .attrs = &ad7606_attribute_group_range, .validate_trigger = &ad7606_validate_trigger, + .update_scan_mode = &ad7606_update_scan_mode, }; static const struct iio_trigger_ops ad7606_trigger_ops = { @@ -602,8 +660,6 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address, indio_dev->channels = st->chip_info->channels; indio_dev->num_channels = st->chip_info->num_channels; - init_completion(&st->completion); - ret = ad7606_reset(st); if (ret) dev_warn(st->dev, "failed to RESET: no RESET GPIO specified\n"); @@ -635,7 +691,7 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address, return ret; } - /* If convst pin is not defined, setup PWM*/ + /* If convst pin is not defined, setup PWM */ if (!st->gpio_convst) { st->cnvst_pwm = devm_pwm_get(dev, NULL); if (IS_ERR(st->cnvst_pwm)) @@ -671,14 +727,25 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address, if (ret) return ret; } - ret = devm_request_threaded_irq(dev, irq, - NULL, - &ad7606_interrupt, - IRQF_TRIGGER_FALLING | IRQF_ONESHOT, - chip_info->name, indio_dev); - if (ret) - return ret; + if (st->bops->iio_backend_config) { + st->bops->iio_backend_config(dev, indio_dev); + indio_dev->setup_ops = &ad7606_pwm_buffer_ops; + } else { + /* Reserve the PWM use only for backend (force gpio_convst definition)*/ + if (!st->gpio_convst) + return dev_err_probe(dev, -EINVAL, + "Convst pin must be defined when not in backend mode"); + + init_completion(&st->completion); + ret = devm_request_threaded_irq(dev, irq, + NULL, + &ad7606_interrupt, + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, + chip_info->name, indio_dev); + if (ret) + return ret; + } return devm_iio_device_register(dev, indio_dev); } EXPORT_SYMBOL_NS_GPL(ad7606_probe, IIO_AD7606); diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h index aab8fefb84be..9a098cd77812 100644 --- a/drivers/iio/adc/ad7606.h +++ b/drivers/iio/adc/ad7606.h @@ -34,6 +34,12 @@ BIT(IIO_CHAN_INFO_SCALE), \ BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO)) +#define AD7606_BI_CHANNEL(num) \ + AD760X_CHANNEL(num, 0, \ + BIT(IIO_CHAN_INFO_SCALE), \ + BIT(IIO_CHAN_INFO_SAMP_FREQ) | \ + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO)) + #define AD7616_CHANNEL(num) \ AD760X_CHANNEL(num, BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),\ 0, BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO)) @@ -61,6 +67,7 @@ enum ad7606_supported_device_ids { * @os_req_reset some devices require a reset to update oversampling * @init_delay_ms required delay in miliseconds for initialization * after a restart + * @has_backend defines if a backend is available for the given chip */ struct ad7606_chip_info { enum ad7606_supported_device_ids id; @@ -71,6 +78,7 @@ struct ad7606_chip_info { unsigned int oversampling_num; bool os_req_reset; unsigned long init_delay_ms; + bool has_backend; }; /** @@ -116,6 +124,7 @@ struct ad7606_state { unsigned int num_scales; const unsigned int *oversampling_avail; unsigned int num_os_ratios; + struct iio_backend *back; int (*write_scale)(struct iio_dev *indio_dev, int ch, int val); int (*write_os)(struct iio_dev *indio_dev, int val); @@ -140,16 +149,21 @@ struct ad7606_state { /** * struct ad7606_bus_ops - driver bus operations + * @iio_backend_config function pointer for configuring the iio_backend for + * the compatibles that use it * @read_block function pointer for reading blocks of data * @sw_mode_config: pointer to a function which configured the device * for software mode * @reg_read function pointer for reading spi register * @reg_write function pointer for writing spi register * @write_mask function pointer for write spi register with mask + * @update_scan_mode function pointer for handling the calls to iio_info's update_scan + * mode when enabling/disabling channels. * @rd_wr_cmd pointer to the function which calculates the spi address */ struct ad7606_bus_ops { /* more methods added in future? */ + int (*iio_backend_config)(struct device *dev, struct iio_dev *indio_dev); int (*read_block)(struct device *dev, int num, void *data); int (*sw_mode_config)(struct iio_dev *indio_dev); int (*reg_read)(struct ad7606_state *st, unsigned int addr); @@ -160,6 +174,7 @@ struct ad7606_bus_ops { unsigned int addr, unsigned long mask, unsigned int val); + int (*update_scan_mode)(struct iio_dev *indio_dev, const unsigned long *scan_mask); u16 (*rd_wr_cmd)(int addr, char isWriteOp); }; @@ -264,6 +279,7 @@ static const struct ad7606_chip_info ad7606b_info = { .num_channels = 9, .oversampling_avail = ad7606_oversampling_avail, .oversampling_num = ARRAY_SIZE(ad7606_oversampling_avail), + .has_backend = true, .name = "ad7606B", .id = ID_AD7606B, }; diff --git a/drivers/iio/adc/ad7606_par.c b/drivers/iio/adc/ad7606_par.c index d83c0edc1e31..5c8a04556e25 100644 --- a/drivers/iio/adc/ad7606_par.c +++ b/drivers/iio/adc/ad7606_par.c @@ -3,6 +3,8 @@ * AD7606 Parallel Interface ADC driver * * Copyright 2011 Analog Devices Inc. + * Copyright 2024 Analog Devices Inc. + * Copyright 2024 BayLibre SAS. */ #include <linux/mod_devicetable.h> @@ -11,10 +13,86 @@ #include <linux/types.h> #include <linux/err.h> #include <linux/io.h> +#include <linux/pwm.h> +#include <linux/gpio.h> +#include <linux/delay.h> #include <linux/iio/iio.h> +#include <linux/iio/backend.h> #include "ad7606.h" +#ifdef CONFIG_IIO_BACKEND +static const struct iio_chan_spec ad7606b_bi_channels[] = { + AD7606_BI_CHANNEL(0), + AD7606_BI_CHANNEL(1), + AD7606_BI_CHANNEL(2), + AD7606_BI_CHANNEL(3), + AD7606_BI_CHANNEL(4), + AD7606_BI_CHANNEL(5), + AD7606_BI_CHANNEL(6), + AD7606_BI_CHANNEL(7), +}; + +static int ad7606_bi_update_scan_mode(struct iio_dev *indio_dev, const unsigned long *scan_mask) +{ + struct ad7606_state *st = iio_priv(indio_dev); + unsigned int c, ret; + + for (c = 0; c < indio_dev->num_channels; c++) { + if (test_bit(c, scan_mask)) + ret = iio_backend_chan_enable(st->back, c); + else + ret = iio_backend_chan_disable(st->back, c); + if (ret) + return ret; + } + + return 0; +} + +static int ad7606_bi_setup_iio_backend(struct device *dev, struct iio_dev *indio_dev) +{ + struct ad7606_state *st = iio_priv(indio_dev); + unsigned int ret, c; + + st->back = devm_iio_backend_get(dev, NULL); + if (IS_ERR(st->back)) + return PTR_ERR(st->back); + + /* If the device is iio_backend powered the PWM is mandatory */ + if (!st->cnvst_pwm) + return -EINVAL; + + ret = devm_iio_backend_request_buffer(dev, st->back, indio_dev); + if (ret) + return ret; + + ret = devm_iio_backend_enable(dev, st->back); + if (ret) + return ret; + + struct iio_backend_data_fmt data = { + .sign_extend = true, + .enable = true, + }; + for (c = 0; c < indio_dev->num_channels; c++) { + ret = iio_backend_data_format_set(st->back, c, &data); + if (ret) + return ret; + } + + indio_dev->channels = ad7606b_bi_channels; + indio_dev->num_channels = 8; + + return 0; +} + +static const struct ad7606_bus_ops ad7606_bi_bops = { + .iio_backend_config = ad7606_bi_setup_iio_backend, + .update_scan_mode = ad7606_bi_update_scan_mode, +}; +#endif + static int ad7606_par16_read_block(struct device *dev, int count, void *buf) { @@ -52,7 +130,20 @@ static int ad7606_par_probe(struct platform_device *pdev) void __iomem *addr; resource_size_t remap_size; int irq; - +#ifdef CONFIG_IIO_BACKEND + struct iio_backend *back; + + /*For now, only the AD7606B is backend compatible.*/ + if (chip_info->has_backend) { + back = devm_iio_backend_get(&pdev->dev, NULL); + if (IS_ERR(back)) + return PTR_ERR(back); + + return ad7606_probe(&pdev->dev, 0, NULL, + chip_info, + &ad7606_bi_bops); + } +#endif irq = platform_get_irq(pdev, 0); if (irq < 0) return irq; @@ -74,6 +165,7 @@ static const struct platform_device_id ad7606_driver_ids[] = { { .name = "ad7606-4", .driver_data = (kernel_ulong_t)&ad7606_4_info, }, { .name = "ad7606-6", .driver_data = (kernel_ulong_t)&ad7606_6_info, }, { .name = "ad7606-8", .driver_data = (kernel_ulong_t)&ad7606_8_info, }, + { .name = "ad7606b", .driver_data = (kernel_ulong_t)&ad7606b_info, }, { } }; MODULE_DEVICE_TABLE(platform, ad7606_driver_ids); @@ -83,6 +175,7 @@ static const struct of_device_id ad7606_of_match[] = { { .compatible = "adi,ad7606-4", .data = &ad7606_4_info }, { .compatible = "adi,ad7606-6", .data = &ad7606_6_info }, { .compatible = "adi,ad7606-8", .data = &ad7606_8_info }, + { .compatible = "adi,ad7606b", .data = &ad7606b_info }, { } }; MODULE_DEVICE_TABLE(of, ad7606_of_match); @@ -102,3 +195,6 @@ MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>"); MODULE_DESCRIPTION("Analog Devices AD7606 ADC"); MODULE_LICENSE("GPL v2"); MODULE_IMPORT_NS(IIO_AD7606); +#ifdef CONFIG_IIO_BACKEND +MODULE_IMPORT_NS(IIO_BACKEND); +#endif
- Basic support for iio backend. - Supports IIO_CHAN_INFO_SAMP_FREQ R/W. - Only hardware mode is available, and that IIO_CHAN_INFO_RAW is not supported if iio-backend mode is selected. A small correction was added to the driver's file name in the Kconfig file's description. Signed-off-by: Guillaume Stols <gstols@baylibre.com> --- drivers/iio/adc/Kconfig | 3 +- drivers/iio/adc/ad7606.c | 103 +++++++++++++++++++++++++++++++++++-------- drivers/iio/adc/ad7606.h | 16 +++++++ drivers/iio/adc/ad7606_par.c | 98 +++++++++++++++++++++++++++++++++++++++- 4 files changed, 200 insertions(+), 20 deletions(-)