Message-ID: <5dedf51d8ec19b7b3bd0c6cb136048344f1c1007.camel@gmail.com>
Date: Thu, 05 Sep 2024 10:40:39 +0200
From: Nuno Sá <noname.nuno@...il.com>
To: Guillaume Stols <gstols@...libre.com>, Uwe
 Kleine-König <ukleinek@...nel.org>, Lars-Peter Clausen
 <lars@...afoo.de>, Michael Hennerich <Michael.Hennerich@...log.com>,
 Jonathan Cameron <jic23@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
 "Rafael J. Wysocki" <rafael@...nel.org>, Jonathan Corbet <corbet@....net>
Cc: linux-pwm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-fbdev@...r.kernel.org, linux-iio@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-doc@...r.kernel.org, 
	20240705211452.1157967-2-u.kleine-koenig@...libre.com, 
	20240712171821.1470833-2-u.kleine-koenig@...libre.com, 
	cover.1721040875.git.u.kleine-koenig@...libre.com, aardelean@...libre.com
Subject: Re: [PATCH 8/8] iio:adc:ad7606: Add iio-backend support

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@...libre.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á

Powered by blists - more mailing lists