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

[David Lechner]

On Mon, Sep 29, 2025 at 7:59 AM Marilene Andrade Garcia
<marilene.agarcia@xxxxxxxxx> 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.