Message-ID: <31a1dfeb-2d72-44bc-b3ca-36b4115c3010@baylibre.com>
Date: Thu, 12 Sep 2024 12:13:51 +0200
From: Guillaume Stols <gstols@...libre.com>
To: Nuno Sá <noname.nuno@...il.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 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@...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>
>> +
>> +	/* 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á
>
>

Powered by blists - more mailing lists