| 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: <aBSZWs_rWNHZbU7V@smile.fi.intel.com>
Date: Fri, 2 May 2025 13:07:22 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: David Lechner <dlechner@...libre.com>
Cc: Sayyad Abid <sayyad.abid16@...il.com>, linux-iio@...r.kernel.org,
jic23@...nel.org, lars@...afoo.de, robh@...nel.org,
krzk+dt@...nel.org, conor+dt@...nel.org, nuno.sa@...log.com,
javier.carrasco.cruz@...il.com, olivier.moysan@...s.st.com,
gstols@...libre.com, tgamblin@...libre.com, alisadariana@...il.com,
eblanc@...libre.com, antoniu.miclaus@...log.com,
stefan.popa@...log.com, ramona.gradinariu@...log.com,
herve.codina@...tlin.com, tobias.sperling@...ting.com,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 1/5] iio: adc: ti-ads1262.c: add initial driver for
TI ADS1262 ADC
On Thu, May 01, 2025 at 12:37:30PM -0500, David Lechner wrote:
> On 5/1/25 5:00 AM, Sayyad Abid wrote:
> > Add the core driver file `ti-ads1262.c` for the TI ADS1262 ADC.
> > This initial version implements basic IIO functionality for device
> > probe via SPI and reading raw voltage samples from input channels.
...
> > +#include <linux/kernel.h>
>
> This header includes too much, please use more specific headers.
>
> > +#include <linux/device.h>
Ditto for this one. Include it only if really required, otherwise we have often
used dev_printk.h, device/devres.h.
> > +#include <linux/module.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/delay.h>
>
> Alphabetical order is preferred.
> > +#include <linux/spi/spi.h>
> > +#include <linux/unaligned.h>
Also many headers are missing (probably due to inclusion of kernel.h).
...
> > +#define ADS1262_SETTLE_TIME_USECS 10000
_US is fine (no need to have longer _USECS, which is not so standard).
Also
(10 * USEC_PER_MSEC)
...
> > +/* The Read/Write commands require 4 tCLK to encode and decode, for speeds
> > + * 2x the clock rate, these commands would require extra time between the
> > + * command byte and the data. A simple way to tacke this issue is by
> > + * limiting the SPI bus transfer speed while accessing registers.
> > + */
/*
* Wrong style for multi-line comments, please use
* this as an example. Fix all comments in the file
* accordingly.
*/
...
> > +/* For reading and writing we need a buffer of size 3bytes*/
Missing space.
...
> > +/**
> > + * struct ads1262_private - ADS1262 ADC private data structure
> > + * @spi: SPI device structure
> > + * @reset_gpio: GPIO descriptor for reset pin
> > + * @prev_channel: Previously selected channel for MUX configuration
> > + * @cmd_buffer: Buffer for SPI command transfers
> > + * @rx_buffer: Buffer for SPI data reception
> > + */
> > +struct ads1262_private {
> > + struct spi_device *spi;
Is it really used? Or is struct device *dev just enough?
> > + struct gpio_desc *reset_gpio;
> > + u8 prev_channel;
> > + u8 cmd_buffer[ADS1262_SPI_CMD_BUFFER_SIZE];
> > + u8 rx_buffer[ADS1262_SPI_RDATA_BUFFER_SIZE] __aligned(IIO_DMA_MINALIGN);
>
> cmd_buffer is also used with SPI, so __aligned(IIO_DMA_MINALIGN); needs to go
> there instead.
>
> > +};
...
> > +#define ADS1262_CHAN(index) \
> > +{ \
> > + .type = IIO_VOLTAGE, \
> > + .indexed = 1, \
> > + .channel = index, \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> > + .scan_index = index, \
> > + .scan_type = { \
> > + .sign = 's', \
> > + .realbits = ADS1262_BITS_PER_SAMPLE, \
> > + .storagebits = 32, \
> > + .endianness = IIO_CPU \
Leave trailing comma here and in the similar cases (when it's not a clear
terminator entry).
> > + }, \
> > +}
...
> > +static int ads1262_write_cmd(struct ads1262_private *priv, u8 command)
> > +{
> > + struct spi_transfer xfer = {
> > + .tx_buf = priv->cmd_buffer,
> > + .rx_buf = priv->rx_buffer,
> > + .len = ADS1262_SPI_RDATA_BUFFER_SIZE,
> > + .speed_hz = ADS1262_CLK_RATE_HZ,
> > + };
> > +
> > + priv->cmd_buffer[0] = command;
> > +
> > + return spi_sync_transfer(priv->spi, &xfer, 1);
> > +}
> > +
> > +static int ads1262_reg_write(void *context, unsigned int reg, unsigned int val)
> > +{
> > + struct ads1262_private *priv = context;
> > +
> > + priv->cmd_buffer[0] = ADS1262_CMD_WREG | reg;
> > + priv->cmd_buffer[1] = 0;
> > + priv->cmd_buffer[2] = val;
> > +
> > + return spi_write(priv->spi, &priv->cmd_buffer[0], 3);
> > +}
Can't you use regmap SPI instead?
...
> > +static int ads1262_reg_read(void *context, unsigned int reg)
> > +{
> > + struct ads1262_private *priv = context;
> > + struct spi_transfer reg_read_xfer = {
> > + .tx_buf = priv->cmd_buffer,
> > + .rx_buf = priv->cmd_buffer,
> > + .len = 3,
> > + .speed_hz = ADS1262_CLK_RATE_HZ,
> > + };
> > + int ret;
> > +
> > + priv->cmd_buffer[0] = ADS1262_CMD_RREG | reg;
> > + priv->cmd_buffer[1] = 0;
> > + priv->cmd_buffer[2] = 0;
> > +
> > + ret = spi_sync_transfer(priv->spi, ®_read_xfer, 1);
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
> > +}
>
> Why not use regmap? You will still custom read/write functions similar to these
> because of needing the lower SCLK rate, but it will give you a bunch of other
> nice features for free.
Ah, same comment above :-)
...
> > +static int ads1262_reset(struct iio_dev *indio_dev)
> > +{
> > + struct ads1262_private *priv = iio_priv(indio_dev);
> > +
> > + if (priv->reset_gpio) {
> > + gpiod_set_value(priv->reset_gpio, 0);
> > + usleep_range(200, 300);
>
> Use fsleep(). Also, could make this clear that it is 4 tCLK cycles (the hard-
> coded value would have to be changed if external clock support was added).
>
> > + gpiod_set_value(priv->reset_gpio, 1);
>
> The DT bindings will take care of active low, so this looks backwards. Also
> st->reset_gpio is never assigned, so this is dead code.
>
> > + } else {
Redundant else. Just return from the conditional and have the below outside of
it.
> > + return ads1262_write_cmd(priv, ADS1262_CMD_RESET);
> > + }
Missing blank line, but see above.
> > + return 0;
> > +}
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists