| 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: <20250809204820.3e0893fc@jic23-huawei>
Date: Sat, 9 Aug 2025 20:48:20 +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 v2 3/3] iio: adc: update ad7779 to use IIO backend
On Wed, 6 Aug 2025 22:25:00 +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 Ionna,
>
> +static int ad7779_set_data_lines(struct iio_dev *indio_dev,
> + unsigned int num_lanes)
> +{
> + struct ad7779_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + ret = ad7779_set_sampling_frequency(st, num_lanes * AD7779_DEFAULT_SAMPLING_1LINE);
> + if (ret)
> + return ret;
> +
> + ret = iio_backend_num_lanes_set(st->back, num_lanes);
> + if (ret)
> + return ret;
> +
> + return ad7779_spi_write_mask(st, AD7779_REG_DOUT_FORMAT,
> + AD7779_DOUT_FORMAT_MSK,
> + FIELD_PREP(AD7779_DOUT_FORMAT_MSK, 2 - ilog2(num_lanes)));
> +}
> +
> +static int ad7779_conf_channels(struct iio_dev *indio_dev, const struct ad7779_state *st)
As per other branch of thread. This needs a name that makes it more
obvious it's a probe time setup rather that anything to do with configuring
channels at runtime.
> +{
> + struct iio_chan_spec *channels;
> + struct device *dev = &st->spi->dev;
> + int num_channels = ARRAY_SIZE(ad7779_channels);
> +
> + channels = devm_kcalloc(dev, num_channels, sizeof(*channels), GFP_KERNEL);
devm_kmemdup_array()?
> + if (!channels)
> + return -ENOMEM;
> +
> + memcpy(channels, st->chip_info->channels, num_channels * sizeof(struct iio_chan_spec));
> +
> + for (int i = 0; i < 8; i++)
Andy got this, but make sure to get that 8 from somewhere. It's num_channels I think.
Though given most of this code collapses into a single call to devm_kmemdup_array()
I think that local variable may not make much sense. ARRAY_SIZE may be clearer.
> + channels[i].scan_type.endianness = IIO_CPU;
> +
> + indio_dev->channels = channels;
> +
> + return 0;
> +}
> +
> +static int setup_irq(struct ad7779_state *st, struct iio_dev *indio_dev)
As below. Do this in a precursor patch and give it a better name!
Pretty much everything in IIO driver gets namespaced with a driver
specific prefix. Makes future problems with new generic functions
much less likely and makes it clear what is local to the driver.
> +{
> + int ret;
> + struct device *dev = &st->spi->dev;
> +
> + indio_dev->info = &ad7779_info;
> + indio_dev->channels = st->chip_info->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, st->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));
return ad7779_spi_write_mask...
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int setup_back(struct ad7779_state *st, struct iio_dev *indio_dev)
> +{
> + struct device *dev = &st->spi->dev;
> + int ret = -EINVAL;
> + int num_lanes;
> +
> + indio_dev->info = &ad7779_info_data;
> +
> + ret = ad7779_conf_channels(indio_dev, st);
> + if (ret)
> + return ret;
> +
> + st->back = devm_iio_backend_get(dev, NULL);
> + if (IS_ERR(st->back)) {
> + dev_err_probe(dev, ret, "failed to get iio backend");
ret == 0 which isn't what you want.
> + return PTR_ERR(st->back);
I think Andy pointed this out as well. I'm not being too careful to not
duplicate unless I happen to remember he covered something in the earlier
review.
> + }
> +
> + 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 = device_property_read_u32(dev, "adi,num-lanes", &num_lanes);
> + if (ret < 0)
Whilst it's a bit fiddly as this is an array read underneath, the fact
num_lanes is not null is enough to ensure if (ret) is fine here.
> + return ad7779_set_data_lines(indio_dev, 4);
> +
> + return ad7779_set_data_lines(indio_dev, num_lanes);
> +}
> +
> static int ad7779_probe(struct spi_device *spi)
> {
> struct iio_dev *indio_dev;
> @@ -760,9 +917,6 @@ 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");
> -
> indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> if (!indio_dev)
> return -ENOMEM;
> @@ -804,45 +958,14 @@ 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;
> + if (device_property_present(dev, "io-backends"))
> + ret = setup_back(st, indio_dev);
> + else
> + ret = setup_irq(st, indio_dev);
Do the factoring out of the setup_irq stuff in a precursor patch.
Then this one will just make it conditional as part of adding the new backend stuff.
setup_irq isn't a good name. It does a bunch of things that aren't
directly related to the irq. Prefix the function as well
ad7779_setup_without_backend() maybe?
>
> - 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;
>
> @@ -936,3 +1059,4 @@ module_spi_driver(ad7779_driver);
> MODULE_AUTHOR("Ramona Alexandra Nechita <ramona.nechita@...log.com>");
> MODULE_DESCRIPTION("Analog Devices AD7779 ADC");
> MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS("IIO_BACKEND");
Powered by blists - more mailing lists