| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ac765343-7804-4bd5-8057-d67fec2f17b1@baylibre.com>
Date: Tue, 8 Oct 2024 16:15:19 +0200
From: Guillaume Stols <gstols@...libre.com>
To: Jonathan Cameron <jic23@...nel.org>
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,
aardelean@...libre.com, dlechner@...libre.com,
Jonathan Cameron <Jonathan.Cameron@...wei.com>
Subject: Re: [PATCH v3 09/10] iio: adc: ad7606: Add iio-backend support
On 10/5/24 13:53, Jonathan Cameron wrote:
> On Fri, 04 Oct 2024 21:48:43 +0000
> Guillaume Stols <gstols@...libre.com> 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.
> I don't much like the trivial window between this patch and the next
> where the emulated mode is still there but the sleeps aren't adapting with sampling frequency.
>
> Maybe it's worth a dance of leaving the write_raw support
> until after this one so the frequency remains fixed until after
> the fsleep(2) calls are gone?
>
> There is another bit that I'm unsure is technically correct until after
> the next patch. Maybe I'm reading the diff wrong though!
>
> Thanks,
>
> J
>
>> Signed-off-by: Guillaume Stols <gstols@...libre.com>
>> ---
>> drivers/iio/adc/Kconfig | 2 +
>> drivers/iio/adc/ad7606.c | 124 +++++++++++++++++++++++++++++++++++++------
>> drivers/iio/adc/ad7606.h | 15 ++++++
>> drivers/iio/adc/ad7606_par.c | 94 +++++++++++++++++++++++++++++++-
>> 4 files changed, 219 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 4ab1a3092d88..9b52d5b2c592 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -224,9 +224,11 @@ config AD7606_IFACE_PARALLEL
>> tristate "Analog Devices AD7606 ADC driver with parallel interface support"
>> depends on HAS_IOPORT
>> select AD7606
>> + select IIO_BACKEND
>> 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_par.
>> diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
>> index 3666a58f8a6f..d86eb7c3e4f7 100644
>> --- a/drivers/iio/adc/ad7606.c
>> +++ b/drivers/iio/adc/ad7606.c
>> @@ -21,6 +21,7 @@
>> @@ -737,6 +773,10 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
>> return ret;
>>
>> return 0;
>> + case IIO_CHAN_INFO_SAMP_FREQ:
>> + if (val < 0 && val2 != 0)
>> + return -EINVAL;
>> + return ad7606_set_sampling_freq(st, val);
> Currently I think for the !backend + pwm case this can go out of
> range for which that code works (fsleep removed in next patch).
> Perhaps delay adding this until after that patch.
Hi Jonathan,
The sampling frequency can be adjusted only for the backend version,
otherwise (including pwm+interrupt), there is no sysfs access to the
sampling frequency (only available for AD7606_BI_CHANNEL).
>> default:
>> return -EINVAL;
>> }
>> @@ -1108,7 +1186,24 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
>> st->cnvst_pwm);
>> if (ret)
>> return ret;
>> + }
>> +
>> + if (st->bops->iio_backend_config) {
>> + /*
>> + * If there is a backend, the PWM should not overpass the maximum sampling
>> + * frequency the chip supports.
>> + */
>> + ret = ad7606_set_sampling_freq(st,
>> + chip_info->max_samplerate ? : 2 * KILO);
>> + if (ret)
>> + return ret;
>> +
>> + ret = st->bops->iio_backend_config(dev, indio_dev);
>> + if (ret)
>> + return ret;
>> + indio_dev->setup_ops = &ad7606_pwm_buffer_ops;
>> } else {
>> + init_completion(&st->completion);
>> st->trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
>> indio_dev->name,
>> iio_device_id(indio_dev));
> It's a little hard to unwind the patches, but this was previously in the !pwm case.
> At this point in the series we still allow the pwm case to work with ! backend.
> So is this now running in that case? Do we need a temporary additional check
> on !pwm
mmm actually this should not be in a condition in the PWM patch. Will
fix this directly there.
>
>
>> @@ -1126,15 +1221,14 @@ int ad7606_probe(struct device *dev, int irq, void __iomem *base_address,
>> &ad7606_buffer_ops);
>> 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;
>> }
>> - ret = devm_request_threaded_irq(dev, irq,
>> - NULL,
>> - &ad7606_interrupt,
>> - IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>> - chip_info->name, indio_dev);
>> - if (ret)
>> - return ret;
>> -
>> return devm_iio_device_register(dev, indio_dev);
>> }
>> EXPORT_SYMBOL_NS_GPL(ad7606_probe, IIO_AD7606);
>> diff --git a/drivers/iio/adc/ad7606_par.c b/drivers/iio/adc/ad7606_par.c
>> index b87be2f1ca04..6042f6799272 100644
>> --- a/drivers/iio/adc/ad7606_par.c
>> +++ b/drivers/iio/adc/ad7606_par.c
>> @@ -2,7 +2,8 @@
>> +
>> +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;
>> + struct iio_backend_data_fmt data = {
>> + .sign_extend = true,
>> + .enable = true,
>> + };
>> +
>> + 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 dev_err_probe(st->dev, -EINVAL,
>> + "A PWM is mandatory when using backend.\n");
>> +
>> + 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;
>> +
>> + 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;
> Ultimately this may want to move into the chip_info structures as more devices are added
> but this is fine for now I suppose.
Will do this in a next series where support is added for the other chips.
>
>> + indio_dev->num_channels = 8;
>> +
>> + return 0;
>> +}
Powered by blists - more mailing lists