Re: [PATCH v2 9/9] iio: adc: ad7606: Add support for writing registers when using backend

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 10 Dec 2024 10:46:49 +0000
Guillaume Stols <gstols@baylibre.com> wrote:

> Adds the logic for effectively enabling the software mode for the
> iio-backend, i.e enabling the software mode channel configuration and
> implementing the register writing functions.
> 
> Signed-off-by: Guillaume Stols <gstols@baylibre.com>
Hi Guillaume,

Main thing in here is really about not adding more cases of
iio_device_claim_direct_scoped() given the problems we are having with it
and lack of a clean replacement.

My current thinking is that it's just not worth the pain of weird
compiler quirks etc and readability issues (even though I for one have
gotten used to that bit!)

Jonathan

> ---
>  drivers/iio/adc/ad7606.h     | 15 ++++++++++++
>  drivers/iio/adc/ad7606_par.c | 56 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 71 insertions(+)
> 
> diff --git a/drivers/iio/adc/ad7606.h b/drivers/iio/adc/ad7606.h
> index ada8065fba4e..9da39c2d5d53 100644
> --- a/drivers/iio/adc/ad7606.h
> +++ b/drivers/iio/adc/ad7606.h
> @@ -96,6 +96,21 @@
>  		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),  \
>  		0, 0, 16)
>  
> +#define AD7606_BI_SW_CHANNEL(num)			\
> +	AD760X_CHANNEL(num,				\
> +		/* mask separate */			\
> +		BIT(IIO_CHAN_INFO_SCALE),		\
> +		/* mask type */				\
> +		0,					\
> +		/* mask all */				\
> +		BIT(IIO_CHAN_INFO_SAMP_FREQ) |		\
> +		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),	\
> +		/* mask separate available */		\
> +		BIT(IIO_CHAN_INFO_SCALE),		\
> +		/* mask all available */		\
> +		BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),	\
> +		16)
> +
>  struct ad7606_state;
>  
>  typedef int (*ad7606_scale_setup_cb_t)(struct iio_dev *indio_dev,
> diff --git a/drivers/iio/adc/ad7606_par.c b/drivers/iio/adc/ad7606_par.c
> index 64733b607aa8..c159cd4f7802 100644
> --- a/drivers/iio/adc/ad7606_par.c
> +++ b/drivers/iio/adc/ad7606_par.c
> @@ -13,12 +13,14 @@
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/property.h>
> +#include <linux/pwm.h>

Why the addition of this?  If it was simply missing previously spin a
separate patch.

>  #include <linux/types.h>
>  
>  #include <linux/iio/backend.h>
>  #include <linux/iio/iio.h>
>  
>  #include "ad7606.h"
> +#include "ad7606_bi.h"
>  
>  static const struct iio_chan_spec ad7606b_bi_channels[] = {
>  	AD7606_BI_CHANNEL(0),
> @@ -31,6 +33,17 @@ static const struct iio_chan_spec ad7606b_bi_channels[] = {
>  	AD7606_BI_CHANNEL(7),
>  };
>  
> +static const struct iio_chan_spec ad7606b_bi_sw_channels[] = {
> +	AD7606_BI_SW_CHANNEL(0),
> +	AD7606_BI_SW_CHANNEL(1),
> +	AD7606_BI_SW_CHANNEL(2),
> +	AD7606_BI_SW_CHANNEL(3),
> +	AD7606_BI_SW_CHANNEL(4),
> +	AD7606_BI_SW_CHANNEL(5),
> +	AD7606_BI_SW_CHANNEL(6),
> +	AD7606_BI_SW_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);
> @@ -86,9 +99,52 @@ static int ad7606_bi_setup_iio_backend(struct device *dev, struct iio_dev *indio
>  	return 0;
>  }
>  
> +static int ad7606_bi_reg_read(struct iio_dev *indio_dev, unsigned int addr)
> +{
> +	struct ad7606_state *st = iio_priv(indio_dev);
> +	int val, ret;
> +	struct ad7606_platform_data *pdata =  st->dev->platform_data;
> +
> +	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
As below. Let's go back to
	ret = iio_device_claim_direct_mode(indio-dev)
	if (ret)
		return ret;

	ret = ....

	iio_device_release_direct_mode()
	if (ret < 0)
		return ret;

	return val;
> +		ret = pdata->bus_reg_read(st->back,
> +					addr,
> +					&val);
> +	}
> +	if (ret < 0)
> +		return ret;
> +
> +	return val;
> +}
> +
> +static int ad7606_bi_reg_write(struct iio_dev *indio_dev,
> +			       unsigned int addr,
> +			       unsigned int val)
> +{
> +	struct ad7606_state *st = iio_priv(indio_dev);
> +	struct ad7606_platform_data *pdata =  st->dev->platform_data;
> +	int ret;
> +
> +	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> +	ret = pdata->bus_reg_write(st->back,
Quite a few things wrong with indentation here.

Also a small request.   Don't use iio_device_claim_direct_scoped.
It's proved a PITA for all sorts of reasons so one of my possible projects
for when I get bored is to look at just how bad it would be to rip it out.

One of those things that seems cool and useful but turns out it's not worth it.

Jonathan


> +					addr,
> +					val);
> +	}
> +	return ret;
> +}