| 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: <CAHp75Vd9WCXR_QmefqPhWO1niMnESq7LAcN=eYvSiqkWfFrNhA@mail.gmail.com>
Date: Mon, 10 Nov 2025 22:46:02 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Oleksij Rempel <o.rempel@...gutronix.de>
Cc: Jonathan Cameron <jic23@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
David Jander <david@...tonic.nl>, kernel@...gutronix.de, linux-kernel@...r.kernel.org,
linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
David Lechner <dlechner@...libre.com>, Nuno Sá <nuno.sa@...log.com>
Subject: Re: [PATCH v2 2/2] iio: adc: Add TI ADS131M0x ADC driver
On Mon, Nov 10, 2025 at 8:14 AM Oleksij Rempel <o.rempel@...gutronix.de> wrote:
>
> Add a new IIO ADC driver for Texas Instruments ADS131M0x devices
> (ADS131M02/03/04/06/08). These are 24-bit, up to 64 kSPS, simultaneous-
> sampling delta-sigma ADCs accessed via SPI.
>
> Highlights:
> - Supports 2/3/4/6/8-channel variants with per-channel RAW and SCALE.
> - Implements device-required full-duplex fixed-frame transfers.
> - Handles both input and output CRC; uses a non-reflected CCITT (0x1021)
> implementation because the generic crc_ccitt helper is incompatible.
>
> Note: Despite the almost identical name, this hardware is not
> compatible with the ADS131E0x series handled by
> drivers/iio/adc/ti-ads131e08.c.
(Note, my address for IIO reviews is andy@...nel.org)
...
+ array_size.h
+ bitops.h
> +#include <linux/cleanup.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
+ dev_printk.h
+ device/devres.h
> +#include <linux/err.h>
> +#include <linux/iio/iio.h>
+ lockdep.h
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
+ mutex.h
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
+ string.h
> +#include <linux/unaligned.h>
+ types.h
...
> +#define ADS131M_CMD_RREG(a, n) \
> + (0xa000 | ((u16)(a & 0x1f) << 7) | (u16)(n & 0x7f))
> +#define ADS131M_CMD_WREG(a, n) \
> + (0x6000 | ((u16)(a & 0x1f) << 7) | (u16)(n & 0x7f))
These two suspiciously look like a reinvention of proper unaligned
getters with the specific shift / GENMASK().
...
> +/* 1.2V internal reference, in millivolts, for IIO_VAL_FRACTIONAL_LOG2 */
> +#define ADS131M_VREF_INTERNAL_MV 1200
_mV
...
> +/* 24-bit resolution */
> +#define ADS131M_RESOLUTION_BITS 24
> +/* Divisor is 2^(Res - 1) for signed 2's complement */
> +#define ADS131M_SCALE_DIVISOR (1UL << (ADS131M_RESOLUTION_BITS - 1))
Why not BIT() here?
...
> +struct ads131m_configuration {
Running `pahole` might give some hints on optimisation of the layout.
> + const struct iio_chan_spec *channels;
> + u8 num_channels;
> + u16 reset_ack;
> + bool supports_extref;
> + bool supports_xtal;
> + const char *name;
> +};
...
> +static inline u16 ads131m_crc_ccitt_byte(u16 crc, u8 data)
> +{
> + int i;
> +
> + crc ^= ((u16)data << 8);
> + for (i = 0; i < 8; i++) {
> + if (crc & 0x8000)
> + crc = (crc << 1) ^ 0x1021;
> + else
> + crc = (crc << 1);
> + }
> +
> + return crc & 0xFFFF;
> +}
> +
> +/**
> + * ads131m_crc_calculate - Calculate CRC-16-CCITT over a buffer
> + * @buffer: The data buffer to process
> + * @len: The length of the buffer
> + *
> + * This function processes a buffer with the CCITT algorithm required
> + * by the device, using the 0xFFFF seed.
> + *
> + * Return: The final 16-bit CRC.
> + */
> +static u16 ads131m_crc_calculate(const u8 *buffer, size_t len)
> +{
> + u16 crc = 0xFFFF;
> + size_t i;
> +
> + for (i = 0; i < len; i++)
> + crc = ads131m_crc_ccitt_byte(crc, buffer[i]);
> +
> + return crc;
> +}
Why CRC16 library can't be used here (crc_ccitt() one perhaps)?
...
> +static int ads131m_write_reg_unlocked(struct ads131m_priv *priv, u8 reg,
> + u16 val)
I would go with a single line here.
...
> + dev_err_ratelimited(dev,
> + "SPI error on WREG (cycle 1)\n");
Ditto.
...
> + dev_err_ratelimited(dev,
> + "SPI error on WREG ACK (cycle 2)\n");
Ditto.
...
> + /*
> + * Cycle 3: Check STATUS for Input CRC error.
> + * This is necessary even if ACK was wrong, to clear the CRC_ERR flag.
> + */
> + ret = ads131m_check_status_crc_err(priv);
> +
> + return ret < 0 ? ret : ret_crc_err;
Use standard way of checking:
ret = ...(...);
if (ret)
return ret;
return ret_crc_err;
...
> +static int ads131m_rmw_reg(struct ads131m_priv *priv, u8 reg, u16 clear,
> + u16 set)
One line.
> +{
> + u16 old_val, new_val;
> + int ret;
> +
> + guard(mutex)(&priv->lock);
> +
> + ret = ads131m_read_reg_unlocked(priv, reg, &old_val);
> + if (ret < 0)
> + return ret;
> +
> + new_val = (old_val & ~clear) | set;
> +
Unneeded blank line.
> + if (new_val == old_val)
> + return 0;
> +
> + return ads131m_write_reg_unlocked(priv, reg, new_val);
> +}
...
> + /*
> + * The received 16-bit CRC is MSB-aligned in the last 24-bit word.
> + * We extract it from the first 2 bytes (BE) of that word.
> + */
> + received_crc = get_unaligned_be16(&priv->rx_buffer[data_len]);
> +
Unneeded blank line.
> + if (calculated_crc != received_crc) {
> + dev_err_ratelimited(dev,
> + "Output CRC error. Got %04x, expected %04x\n",
> + received_crc, calculated_crc);
> + return -EIO;
> + }
...
> + struct device *dev = &priv->spi->dev;
> + int vref_uv;
_uV
> + int ret;
...
> +#define ADS131M_VOLTAGE_CHANNEL(num) \
> + { \
> + .type = IIO_VOLTAGE, \
> + .indexed = 1, \
> + .channel = (num), \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_SCALE) \
Leave trailing commas for non-terminating entries.
> + }
...
> + /*
> + * Get the optional external reference. This schedules regulator_put()
> + * automatically.
> + */
> + priv->refin_supply = devm_regulator_get_optional(dev, "refin");
> + if (IS_ERR(priv->refin_supply)) {
> + /* -ENODEV is fine, it just means we'll use the internal ref */
> + if (PTR_ERR(priv->refin_supply) == -ENODEV)
> + priv->refin_supply = NULL;
> + else
> + return dev_err_probe(dev, PTR_ERR(priv->refin_supply),
> + "failed to get refin regulator\n");
> + }
Can be untangled to
... = devm_regulator_get_...
ret = PTR_ERR_OR_ZERO(...);
if (ret == -ENODEV)
... = NULL;
else if (ret)
return dev_err_probe(...);
...
> + if (priv->config->supports_xtal) {
> + if (!is_xtal) /* "clkin" */
> + clk_set |= ADS131M_CLOCK_XTAL_DIS;
> + }
if (foo) { if (bar) {...} } == if (foo && bar) { ... }
> + if (priv->config->supports_extref) {
> + if (!IS_ERR_OR_NULL(priv->refin_supply))
> + clk_set |= ADS131M_CLOCK_EXTREF_EN;
> + else
> + clk_clear |= ADS131M_CLOCK_EXTREF_EN;
Why not a positive check?
> + }
...
> + ret = device_property_read_string(dev, "clock-names", &clock_name);
> + if (ret < 0)
> + return dev_err_probe(dev, ret,
> + "device property 'clock-names' not found\n");
> + is_xtal = (strcmp(clock_name, "xtal") == 0);
Hmm... We have device_property_match_string().
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists