| 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: <aRzUsMb4i3M7HoN7@smile.fi.intel.com>
Date: Tue, 18 Nov 2025 22:18:56 +0200
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Marcelo Schmitt <marcelo.schmitt@...log.com>
Cc: linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
jic23@...nel.org, nuno.sa@...log.com, dlechner@...libre.com,
andy@...nel.org, Michael.Hennerich@...log.com, robh@...nel.org,
krzk+dt@...nel.org, conor+dt@...nel.org, corbet@....net,
cosmin.tanislav@...log.com, marcelo.schmitt1@...il.com
Subject: Re: [PATCH v2 2/3] iio: adc: Initial support for AD4134
On Tue, Nov 18, 2025 at 02:32:43PM -0300, Marcelo Schmitt wrote:
> AD4134 is a 24-bit, 4-channel, simultaneous sampling, precision
> analog-to-digital converter (ADC). The device can be managed through SPI or
> direct control of pin logical levels (pin control mode). The AD4134 design
> also features a dedicated bus for ADC sample data output. Though, this
> initial driver for AD4134 only supports usual SPI connections.
>
> Add basic support for AD4134 that enables single-shot ADC sample read.
Thanks for an update! My comments below.
...
> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/bits.h>
bitops.h implies bits.h and the latter can be omitted. But it's up to you, I'm
fine with the current way.
> +#include <linux/clk.h>
> +#include <linux/crc8.h>
> +#include <linux/delay.h>
> +#include <linux/dev_printk.h>
> +#include <linux/err.h>
> +#include <linux/export.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/types.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/time.h>
> +#include <linux/types.h>
> +#include <linux/unaligned.h>
> +#include <linux/units.h>
...
> +struct ad4134_state {
> + struct spi_device *spi;
> + struct device *dev;
> + struct regmap *regmap;
We have a duplication here, I believe. Either struct device or struct regmap
may be dropped. If it's not the case, the fields needs a good description /
justification.
> + unsigned long sys_clk_hz;
> + struct gpio_desc *odr_gpio;
> + int refin_mv;
> + /*
> + * DMA (thus cache coherency maintenance) requires the transfer buffers
> + * to live in their own cache lines.
> + */
> + u8 rx_buf[AD4134_SPI_MAX_XFER_LEN] __aligned(IIO_DMA_MINALIGN);
> + u8 tx_buf[AD4134_SPI_MAX_XFER_LEN];
> +};
...
> +static int ad4134_regulator_setup(struct ad4134_state *st)
> +{
> + struct device *dev = &st->spi->dev;
> + bool use_internal_ldo_regulator;
> + int ret;
> +
> + /* Required regulators */
> + ret = devm_regulator_bulk_get_enable(dev, 3, ad4143_regulator_names);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to enable power supplies\n");
> +
> + /* Required regulator that we need to read the voltage */
> + ret = devm_regulator_get_enable_read_voltage(dev, "refin");
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "failed to get REFIN voltage.\n");
> + st->refin_mv = ret / MILLI;
I think the divisor should be (MICRO / MILLI). This what Jonathan suggested
last time I remember (in some other reviews).
> + /*
> + * If ldoin is not provided, then avdd1v8, dvdd1v8, and clkvdd are
> + * required.
> + */
> + ret = devm_regulator_get_enable_optional(dev, "ldoin");
> + if (ret < 0 && ret != -ENODEV)
> + return dev_err_probe(dev, ret, "failed to enable ldoin supply\n");
> +
> + use_internal_ldo_regulator = ret == 0;
> + if (!use_internal_ldo_regulator) {
Can it be
if (use_internal_ldo_regulator)
return 0;
with the dropped indentation level for the below?
> + ret = devm_regulator_get_enable(dev, "avdd1v8");
> + if (ret < 0)
> + return dev_err_probe(dev, ret,
> + "failed to enable avdd1v8 supply\n");
> +
> + ret = devm_regulator_get_enable(dev, "dvdd1v8");
> + if (ret < 0)
> + return dev_err_probe(dev, ret,
> + "failed to enable dvdd1v8 supply\n");
> +
> + ret = devm_regulator_get_enable(dev, "clkvdd");
> + if (ret < 0)
> + return dev_err_probe(dev, ret,
> + "failed to enable clkvdd supply\n");
> + }
> +
> + return 0;
> +}
...
> +static int ad4134_clock_select(struct ad4134_state *st)
> +{
> + struct device *dev = &st->spi->dev;
> + struct clk *sys_clk;
> + int ret;
> + sys_clk = devm_clk_get_optional_enabled(dev, "xtal1-xtal2");
> + if (IS_ERR_OR_NULL(sys_clk)) {
I don't understand the choice of _optional() with the _NULL here.
Also note, we missed the deferred probe case here.
Was it me who suggested this? :-)
One solution may be to check the clock name presence in the clock-names or
whatever the name of that DT property before trying to get this clock.
> + ret = PTR_ERR_OR_ZERO(sys_clk);
> + sys_clk = devm_clk_get_enabled(dev, "clkin");
> + if (IS_ERR(sys_clk))
Yeah, v1 might look better, but I don't remember by heart.
> + return dev_err_probe(dev, PTR_ERR(sys_clk),
> + "failed to get xtal1-xtal2: %d, clkin: %ld\n",
> + ret, PTR_ERR(sys_clk));
No need to have a duplicated error code being printed.
> + }
> +
> + st->sys_clk_hz = clk_get_rate(sys_clk);
> + if (st->sys_clk_hz != AD4134_EXT_CLOCK_MHZ)
> + dev_warn(dev, "invalid external clock frequency %lu\n",
> + st->sys_clk_hz);
> +
> + return 0;
> +}
...
> + reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(reset_gpio))
> + return dev_err_probe(dev, PTR_ERR(reset_gpio),
> + "failed to find reset GPIO\n");
> +
> + if (reset_gpio) {
> + fsleep(AD4134_RESET_TIME_US);
> + gpiod_set_value_cansleep(reset_gpio, 0);
> + }
Can we use reset-gpio driver instead of custom implementation?
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists