| 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: <20250728151030.3f9d08b1@jic23-huawei>
Date: Mon, 28 Jul 2025 15:10:30 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Ioana Risteiu <Ioana.Risteiu@...log.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich
<Michael.Hennerich@...log.com>, "David Lechner" <dlechner@...libre.com>,
Nuno Sá <nuno.sa@...log.com>, Andy Shevchenko
<andy@...nel.org>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
<krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Ramona Nechita
<ramona.nechita@...log.com>, <linux-iio@...r.kernel.org>,
<devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 4/4] iio: adc: Update ad7779 to use IIO backend
On Mon, 28 Jul 2025 16:43:36 +0300
Ioana Risteiu <Ioana.Risteiu@...log.com> wrote:
> Add a new functionality to ad7779 driver that streams data through data
> output interface using IIO backend interface.
>
> Signed-off-by: Ioana Risteiu <Ioana.Risteiu@...log.com>
Hi Ioana,
There is some new ABI (I think) in here. I'm not clear what it is for and
ABI documentation is missing.
A few other minor comments inline.
Thanks,
Jonathan
> +static int ad7779_set_data_lines(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + unsigned int mode)
> +{
> + struct ad7779_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + ret = ad7779_spi_write_mask(st, AD7779_REG_DOUT_FORMAT,
> + AD7779_DOUT_FORMAT_MSK,
> + FIELD_PREP(AD7779_DOUT_FORMAT_MSK, mode));
> + switch (mode) {
> + case AD7779_4LINES:
> + ret = ad7779_set_sampling_frequency(st, AD7779_DEFAULT_SAMPLING_FREQ);
> + if (ret)
> + return ret;
> + ret = iio_backend_num_lanes_set(st->back, 4);
> + break;
> + case AD7779_2LINES:
> + ret = ad7779_set_sampling_frequency(st, AD7779_DEFAULT_SAMPLING_2LINE);
> + if (ret)
> + return ret;
> + ret = iio_backend_num_lanes_set(st->back, 2);
> + break;
> + case AD7779_1LINE:
> + ret = ad7779_set_sampling_frequency(st, AD7779_DEFAULT_SAMPLING_1LINE);
> + if (ret)
> + return ret;
> + ret = iio_backend_num_lanes_set(st->back, 1);
> + break;
> + default:
> + return -EINVAL;
> + }
> +
return ret;
or better yet, return in the various cases above.
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
t struct iio_chan_spec_ext_info ad7779_ext_info_filter[] = {
> + IIO_ENUM("data_lines", IIO_SHARED_BY_ALL, &ad7779_data_lines_enum),
New ABI. Needs documentation in Documentation/ABI/testing/sysfs-bus-iio*
I'm not clear why we need to expose it to userspace, and documentation
might explain that to me.
> + IIO_ENUM_AVAILABLE("data_lines", IIO_SHARED_BY_ALL,
> + &ad7779_data_lines_enum),
> + IIO_ENUM("filter_type", IIO_SHARED_BY_ALL, &ad7779_filter_enum),
> + IIO_ENUM_AVAILABLE("filter_type", IIO_SHARED_BY_ALL,
> + &ad7779_filter_enum),
> + { }
> +};
>
> +
> +static int ad7779_register_back(struct ad7779_state *st, struct iio_dev *indio_dev)
> +{
> + struct device *dev = &st->spi->dev;
> + int ret = -EINVAL;
> +
> + indio_dev->info = &ad7779_info_data;
> +
> + if (strcmp(st->chip_info->name, "ad7771") == 0) {
Don't do string matching. Much preferred to add another pointer + num_channels
to chip_info. Just add backend_channels, num_backend_channels or something like that.
> + indio_dev->channels = ad7779_channels_filter_data;
> + indio_dev->num_channels = ARRAY_SIZE(ad7779_channels_filter_data);
> + } else {
> + indio_dev->channels = ad7779_channels_data;
> + indio_dev->num_channels = ARRAY_SIZE(ad7779_channels_data);
> + }
> +
> + st->back = devm_iio_backend_get(dev, NULL);
> + if (IS_ERR(st->back)) {
> + dev_err_probe(dev, ret, "failed to get iio backend");
> + return PTR_ERR(st->back);
return dev_err_probe() key being that your message here has ret
different to what you return.
> + }
> +
> + 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;
> +
> + ret = iio_backend_num_lanes_set(st->back, 4);
> + if (ret)
> + return ret;
> +
> + return devm_iio_device_register(dev, indio_dev);
> +}
> +
> static int ad7779_probe(struct spi_device *spi)
> {
> struct iio_dev *indio_dev;
> @@ -760,8 +1009,8 @@ static int ad7779_probe(struct spi_device *spi)
> struct device *dev = &spi->dev;
> int ret = -EINVAL;
>
> - if (!spi->irq)
> - return dev_err_probe(dev, ret, "DRDY irq not present\n");
> + if (!spi->irq && !device_property_present(dev, "io-backends"))
> + return dev_err_probe(dev, ret, "Either DRDY interrupt or io-backends property required\n");
Very long line. break before the string at least.
In the binding you aren't disallowing both the irq and io-backends.
That's fine but I wonder here if we should prioritize io-backends
if both are set?
>
> indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> if (!indio_dev)
> @@ -804,49 +1053,12 @@ static int ad7779_probe(struct spi_device *spi)
> return ret;
>
> indio_dev->name = st->chip_info->name;
> - indio_dev->info = &ad7779_info;
> indio_dev->modes = INDIO_DIRECT_MODE;
> - indio_dev->channels = st->chip_info->channels;
> - indio_dev->num_channels = ARRAY_SIZE(ad7779_channels);
> -
> - st->trig = devm_iio_trigger_alloc(dev, "%s-dev%d", indio_dev->name,
> - iio_device_id(indio_dev));
> - if (!st->trig)
> - return -ENOMEM;
> -
> - st->trig->ops = &ad7779_trigger_ops;
> -
> - iio_trigger_set_drvdata(st->trig, st);
> -
> - ret = devm_request_irq(dev, spi->irq, iio_trigger_generic_data_rdy_poll,
> - IRQF_ONESHOT | IRQF_NO_AUTOEN, indio_dev->name,
> - st->trig);
> - if (ret)
> - return dev_err_probe(dev, ret, "request IRQ %d failed\n",
> - st->spi->irq);
> -
> - ret = devm_iio_trigger_register(dev, st->trig);
> - if (ret)
> - return ret;
> -
> - indio_dev->trig = iio_trigger_get(st->trig);
> -
> - init_completion(&st->completion);
> -
> - ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> - &iio_pollfunc_store_time,
> - &ad7779_trigger_handler,
> - &ad7779_buffer_setup_ops);
> - if (ret)
> - return ret;
>
> - ret = ad7779_spi_write_mask(st, AD7779_REG_DOUT_FORMAT,
> - AD7779_DCLK_CLK_DIV_MSK,
> - FIELD_PREP(AD7779_DCLK_CLK_DIV_MSK, 7));
> - if (ret)
> - return ret;
> -
> - return devm_iio_device_register(dev, indio_dev);
This call happens in bother cases. I'd be tempted to rename the functions
as setup_irq() / setup_back() and keep this call outside of either
function.
> + if (spi->irq)
> + return ad7779_register_irq(st, indio_dev);
> + else
> + return ad7779_register_back(st, indio_dev);
> }
Powered by blists - more mailing lists