linux-kernel - Re: [PATCH v12 2/3] iio: adc: max14001: New driver

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMknhBHt9JVkaf1Kq76BKFM-Ff38-7ws6gaq+5fwy=pAih-fww@mail.gmail.com>
Date: Wed, 1 Oct 2025 16:03:09 +0200
From: David Lechner <dlechner@...libre.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>, 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 v12 2/3] iio: adc: max14001: New driver

On Mon, Sep 29, 2025 at 7:59 AM Marilene Andrade Garcia
<marilene.agarcia@...il.com> wrote:
>

This is looking very nice. Just one question still about the averaging
feature and a few suggestions to really polish it up.

> I have one remaining question related to the max_register attribute of the
> regmap. The register regions that can be accessed are 0x00–0x0c and
> 0x13–0x1a. As suggested, I used max_register to set the upper limit of the
> register region that can be accessed (0x1a). Beyond this address, there is
> a reserved region that should not be used (0x1b–0x1f). However, there is
> also a reserved region that should not be used between addresses 0x0d–0x12.
> Should I use something to mark this region in the regmap?

There is struct regmap_access_table that can be used with .rd_table
and .wr_table in the regmap config.

> +static int max14001_read(void *context, unsigned int reg, unsigned int *val)
> +{
> +       struct max14001_state *st = context;
> +       struct spi_transfer xfers[] = {
> +               {
> +                       .tx_buf = &st->spi_tx_buffer,
> +                       .len = sizeof(st->spi_tx_buffer),
> +                       .cs_change = 1,
> +               }, {
> +                       .rx_buf = &st->spi_rx_buffer,
> +                       .len = sizeof(st->spi_rx_buffer),
> +               },
> +       };
> +       int ret;
> +       unsigned int addr, data;
> +
> +       /*
> +        * Prepare SPI transmit buffer 16 bit-value and reverse bit order
> +        * to align with the LSB-first input on SDI port in order to meet
> +        * the device communication requirements. If the controller supports
> +        * SPI_LSB_FIRST, this step will be handled by the SPI code.

s/code/controller/ or s/code/hardware/

> +        */
> +       addr = FIELD_PREP(MAX14001_MASK_ADDR, reg);
> +

...

> +static int max14001_write_single_reg(void *context, unsigned int reg, unsigned int val)
> +{
> +       struct max14001_state *st = context;
> +       int ret;
> +
> +       /* Enable writing to the SPI register */

Always nice to put `.` at the end of the sentence in comments.

> +       ret = max14001_write(st, MAX14001_REG_WEN, MAX14001_REG_WEN_VALUE_WRITE);
> +       if (ret)
> +               return ret;
> +

...

> +static int max14001_read_raw(struct iio_dev *indio_dev,
> +                            struct iio_chan_spec const *chan,
> +                            int *val, int *val2, long mask)
> +{
> +       struct max14001_state *st = iio_priv(indio_dev);
> +       int ret;
> +
> +       switch (mask) {
> +       case IIO_CHAN_INFO_RAW:
> +               ret = regmap_read(st->regmap, MAX14001_REG_ADC, val);
> +               if (ret)
> +                       return ret;
> +
> +               return IIO_VAL_INT;
> +       case IIO_CHAN_INFO_AVERAGE_RAW:
> +               ret = regmap_read(st->regmap, MAX14001_REG_FADC, val);

I don't remember... did you give a reason why this should not be a
separate channel? Or just use REG_FADC as the raw value and forget
about REG_ADC? In any case we would want another attribute to control
the filter window size.

> +               if (ret)
> +                       return ret;
> +

...

> +static int max14001_probe(struct spi_device *spi)
> +{
> +       struct device *dev = &spi->dev;
> +       struct iio_dev *indio_dev;
> +       struct max14001_state *st;
> +       int ret = 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->spi_hw_has_lsb_first = spi->mode & SPI_LSB_FIRST;
> +       st->chip_info = spi_get_device_match_data(spi);

To be extra-safe, we should check that this does not return NULL. I
think most drivers will return -EINVAL in that case.