Re: [PATCH 8/8] iio:adc:ad7606: Add iio-backend support

On 9/5/24 10:40, Nuno Sá wrote:

On Thu, 2024-08-15 at 12:12 +0000, Guillaume Stols wrote:

quoted

- 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 <redacted>
---

Hi Guillaume,

Some initial feedback from me...

quoted

  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

quoted

+	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 ?

quoted

+
+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

quoted

-	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

quoted

+	} 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.

quoted

  	return devm_iio_device_register(dev, indio_dev);
  }

...

quoted

+#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.

quoted

+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 !

quoted

-
+#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á