| 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: <CAHp75Vc84_b5OdVjC+Vup9R5v=d+30vho7kwihYff-L3KO3JqQ@mail.gmail.com>
Date: Tue, 16 Sep 2025 10:57:40 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Marilene Andrade Garcia <marilene.agarcia@...il.com>
Cc: linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org, Kim Seer Paller <kimseer.paller@...log.com>,
Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich <Michael.Hennerich@...log.com>,
Jonathan Cameron <jic23@...nel.org>, 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>,
Marcelo Schmitt <marcelo.schmitt1@...il.com>, Marcelo Schmitt <Marcelo.Schmitt@...log.com>,
Ceclan Dumitru <dumitru.ceclan@...log.com>, Jonathan Santos <Jonathan.Santos@...log.com>,
Dragos Bogdan <dragos.bogdan@...log.com>
Subject: Re: [PATCH v11 2/3] iio: adc: max14001: New driver
On Tue, Sep 16, 2025 at 1:16 AM Marilene Andrade Garcia
<marilene.agarcia@...il.com> wrote:
>
> The MAX14001/MAX14002 is configurable, isolated 10-bit ADCs for multi-range
> binary inputs. In addition to ADC readings, the MAX14001/MAX14002 offers
> more features, like a binary comparator, a filtered reading that can
> provide the average of the last 2, 4, or 8 ADC readings, and an inrush
> comparator that triggers the inrush current. There is also a fault feature
> that can diagnose seven possible fault conditions. And an option to select
> an external or internal ADC voltage reference.
>
> MAX14001/MAX14002 features implemented so far:
> - Raw ADC reading.
> - Filtered ADC average reading with the default configuration.
> - MV fault disable.
> - Selection of external or internal ADC voltage reference, depending on
> whether it is declared in the device tree.
This version looks almost good to me, a few nit-picks below.
...
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -87,6 +87,7 @@ obj-$(CONFIG_MAX11100) += max11100.o
> obj-$(CONFIG_MAX1118) += max1118.o
> obj-$(CONFIG_MAX11205) += max11205.o
> obj-$(CONFIG_MAX11410) += max11410.o
> +obj-$(CONFIG_MAX14001) += max14001.o
> obj-$(CONFIG_MAX1241) += max1241.o
> obj-$(CONFIG_MAX1363) += max1363.o
> obj-$(CONFIG_MAX34408) += max34408.o
Please, keep it ordered.
...
> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/bitrev.h>
> +#include <linux/bits.h>
> +#include <linux/byteorder/generic.h>
This is wrong, should be asm/byteorder.h going after linux/* but
before linux/iio/* ones...
> +#include <linux/cleanup.h>
> +#include <linux/device.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/types.h>
> +#include <linux/units.h>
...here
asm/byteorder.h
> +#include <linux/iio/iio.h>
> +#include <linux/iio/types.h>
...
> +struct max14001_state {
> + const struct max14001_chip_info *chip_info;
> + struct spi_device *spi;
> + struct regmap *regmap;
> + int vref_mV;
> + /*
> + * lock protect against multiple concurrent accesses, RMW sequence,
Lock to protect...
> + * and SPI transfer.
> + */
> + struct mutex lock;
> + /*
> + * The following buffers will be bit-reversed during device
> + * communication, because the device transmits and receives data
> + * LSB-first.
> + * DMA (thus cache coherency maintenance) requires the transfer
> + * buffers to live in their own cache lines.
> + */
> + __be16 spi_tx_buffer __aligned(IIO_DMA_MINALIGN);
> + __be16 spi_rx_buffer;
> +};
...
> +static struct max14001_chip_info max14001_chip_info = {
> + .name = "max14001",
> +};
> +
> +static struct max14001_chip_info max14002_chip_info = {
> + .name = "max14002",
> +};
These can be moved closer to their first user (ID table?).
...
> +static int max14001_write(struct max14001_state *st, unsigned int reg, unsigned int val)
> +{
> + /*
> + * Prepare SPI transmit buffer 16 bit-value big-endian format and
> + * reverses bit order to align with the LSB-first input on SDI port
reverse
> + * in order to meet the device communication requirements.
> + */
> + st->spi_tx_buffer = cpu_to_be16(bitrev16(FIELD_PREP(MAX14001_MASK_ADDR, reg) |
> + FIELD_PREP(MAX14001_MASK_WR, 1) |
> + FIELD_PREP(MAX14001_MASK_DATA, val)));
> +
> + return spi_write(st->spi, &st->spi_tx_buffer, sizeof(st->spi_tx_buffer));
> +}
...
> +static int max14001_probe(struct spi_device *spi)
> +{
> + struct device *dev = &spi->dev;
> + struct iio_dev *indio_dev;
> + struct max14001_state *st;
> + int ret, ext_vrefin = 0;
bool use_ext_vrefin = false;
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + st = iio_priv(indio_dev);
> + st->spi = spi;
> + st->chip_info = spi_get_device_match_data(spi);
> + if (!st->chip_info)
> + return dev_err_probe(dev, -ENODEV, "Failed to get match data\n");
Remove this check, it will be almost always a dead code. The developer
implementing a new device support won't be able to test the driver
anyway with properly given chip_info.
> + indio_dev->name = st->chip_info->name;
> + indio_dev->info = &max14001_info;
> + indio_dev->channels = max14001_channel;
> + indio_dev->num_channels = ARRAY_SIZE(max14001_channel);
> + indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + st->regmap = devm_regmap_init(dev, NULL, st, &max14001_regmap_config);
> + if (IS_ERR(st->regmap))
> + return dev_err_probe(dev, PTR_ERR(st->regmap), "Failed to initialize regmap\n");
> +
> + ret = devm_mutex_init(dev, &st->lock);
> + if (ret)
> + return ret;
> +
> + ret = devm_regulator_get_enable(dev, "vdd");
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to enable Vdd supply\n");
> +
> + ret = devm_regulator_get_enable(dev, "vddl");
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to enable Vddl supply\n");
> +
> + ret = devm_regulator_get_enable_read_voltage(dev, "refin");
> + if (ret < 0 && ret != -ENODEV)
> + return dev_err_probe(dev, ret, "Failed to get REFIN voltage\n");
> +
> + if (ret < 0)
if (ret == -ENODEV) ?
It would be interesting out of curiosity to see bloat-o-meter output
for the original check and my proposal. I would choose one which gives
less code, but in case of equality, I would rather go with a more
explicit (my proposal) check.
> + ret = 1250000;
> + else
> + ext_vrefin = 1;
use_ext_vrefin = true;
> + st->vref_mV = ret / (MICRO / MILLI);
> +
> + if (ext_vrefin) {
if (use_ext_vrefin) {
> + /*
> + * Configure the MAX14001/MAX14002 to use an external voltage reference source
> + * by setting the bit 5 of the configuration register
Missing period.
> + */
> + ret = regmap_update_bits(st->regmap, MAX14001_REG_CFG, MAX14001_REG_CFG_BIT_EXRF, MAX14001_REG_CFG_BIT_EXRF);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to set External REFIN in Configuration Register\n");
> + }
> +
> + ret = max14001_disable_mv_fault(st);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to disable MV Fault\n");
> +
> + return devm_iio_device_register(dev, indio_dev);
> +}
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists