Message-ID: <20240914121442.2ed849a0@jic23-huawei>
Date: Sat, 14 Sep 2024 12:14:42 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Guillaume Stols <gstols@...libre.com>
Cc: Uwe Kleine-König <ukleinek@...nel.org>, Lars-Peter
 Clausen <lars@...afoo.de>, Michael Hennerich
 <Michael.Hennerich@...log.com>, 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>,
 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, Nuno Sa
 <nuno.sa@...log.com>, aardelean@...libre.com
Subject: Re: [PATCH 8/8] iio:adc:ad7606: Add iio-backend support


...

> >>   
> >>   	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.  
> 
> The file is provided only for the "backended" device 
> (AD7606_BI_CHANNEL), BI being Backend Interface. This mode only works 
> with PWM (and incidentally PWM is meant to be used only in conjuction 
> with backend).
> 
> When the PWM is not running because e.g sampling is not enabled, or PWM 
> failed to start, I return 0. Shall I always return the configured value 
> instead of the real one ?

Yes. Configured should be fine I think if there is no way to ask
'what will it be when I turn it on'.


> >> 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?  
> 
> Potentially, I think any of those parts can have a backend and moreover, 
> I don't see anything preventing any ADC to have a backend.
> 
> I introduced the flag as a way to differentiate the "new" way of 
> supporting parallel interface, i.e using backend, from the "old" way 
> (using port I/O).
> 
> There is a concurrency between the old implementation using port I/O and 
> the new one using iio-backend, because they are both "platform", so the 
> initial idea was that it would not make sense and be dangerous to look 
> for a backend for the parts that have no existing (i'd rather say, like 
> you pointed out,  supported) backend.
> 
> Having a second thought at it, the dt bindings already permits only 
> io-backend property to be populated for the parts that actually have a 
> backend, hence one of these is superfluous, or maybe even both are and 
> the user is responsible for setting the right value in the dts. Any advice ?

Dt binding should be enough.  The worst that happens is the driver
tries to use an unsupported backend and that will fail I hope.

So I wouldn't have this driver try to stop it.

> >> 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
> >> @@ -102,3 +195,6 @@ MODULE_AUTHOR("Michael Hennerich <michael.hennerich@...log.com>");
> >>   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.  
> OK, will remove it. According to Nuno's feedback, I could also force the 
> selection of CONFIG_IIO_BACKEND with the driver, which IMHO is not a bad 
> idea, as it would allow to remove all those ifdefs.

Hmm. I guess the questions is whether that is a bloat anyone will worry about
who is using the old way for this device.  I guess that's a problem for
Analog folk if their customers complain.  We can always relax this in future
so for now select IIO_BACKEND is fine by me.

Jonathan

Powered by blists - more mailing lists