On Thu, 15 Aug 2024 12:12:02 +0000
Guillaume Stols <gstols@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>");
> 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
>
