Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220620162059.1097264-1-cosmin.tanislav@analog.com> <20220620162059.1097264-3-cosmin.tanislav@analog.com>
In-Reply-To: <20220620162059.1097264-3-cosmin.tanislav@analog.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Mon, 20 Jun 2022 20:29:43 +0200
Message-ID: <CAHp75VcBJkQ+CwyoDaTJ_AD+mv9d0tEd_txqHwkPRy4-xvnyKg@mail.gmail.com>
Subject: Re: [PATCH v5 2/2] iio: adc: ad4130: add AD4130 driver
To:     Cosmin Tanislav <demonsingur@gmail.com>
Cc:     Jonathan Cameron <jic23@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Linus Walleij <linus.walleij@linaro.org>,
        linux-iio <linux-iio@vger.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        devicetree <devicetree@vger.kernel.org>,
        Cosmin Tanislav <cosmin.tanislav@analog.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Jun 20, 2022 at 6:27 PM Cosmin Tanislav <demonsingur@gmail.com> wrote:
>
> AD4130-8 is an ultra-low power, high precision, measurement solution for
> low bandwidth battery operated applications.
>
> The fully integrated AFE (Analog Front-End) includes a multiplexer for up
> to 16 single-ended or 8 differential inputs, PGA (Programmable Gain
> Amplifier), 24-bit Sigma-Delta ADC, on-chip reference and oscillator,
> selectable filter options, smart sequencer, sensor biasing and excitation
> options, diagnostics, and a FIFO buffer.

...

> +KernelVersion: 5.18

Are you sure?

...

> +struct ad4130_state {
> +       struct regmap                   *regmap;
> +       struct spi_device               *spi;
> +       struct clk                      *mclk;
> +       struct regulator_bulk_data      regulators[4];
> +       u32                             irq_trigger;
> +       u32                             inv_irq_trigger;
> +
> +       /*
> +        * Synchronize access to members of driver state, and ensure atomicity

the driver

> +        * of consecutive regmap operations.
> +        */
> +       struct mutex                    lock;
> +       struct completion               completion;
> +
> +       struct iio_chan_spec            chans[AD4130_MAX_CHANNELS];
> +       struct ad4130_chan_info         chans_info[AD4130_MAX_CHANNELS];
> +       struct ad4130_slot_info         slots_info[AD4130_MAX_SETUPS];
> +       enum ad4130_pin_function        pins_fn[AD4130_MAX_ANALOG_PINS];
> +       u32                             vbias_pins[AD4130_MAX_ANALOG_PINS];
> +       u32                             num_vbias_pins;
> +       int                             scale_tbls[AD4130_REF_SEL_MAX][AD4130_MAX_PGA][2];
> +       struct gpio_chip                gc;
> +       unsigned int                    gpio_offsets[AD4130_MAX_GPIOS];
> +       unsigned int                    num_gpios;
> +
> +       u32                     int_pin_sel;
> +       u32                     int_ref_uv;
> +       u32                     mclk_sel;
> +       bool                    int_ref_en;
> +       bool                    bipolar;
> +
> +       unsigned int            num_enabled_channels;
> +       unsigned int            effective_watermark;
> +       unsigned int            watermark;
> +
> +       struct spi_message      fifo_msg;
> +       struct spi_transfer     fifo_xfer[2];
> +
> +       /*
> +        * DMA (thus cache coherency maintenance) requires the
> +        * transfer buffers to live in their own cache lines.
> +        */

This is a good comment, but what fields does it apply to?

> +       u8                      reset_buf[AD4130_RESET_BUF_SIZE] __aligned(IIO_DMA_MINALIGN);
> +       u8                      reg_write_tx_buf[4];
> +       u8                      reg_read_tx_buf[1];
> +       u8                      reg_read_rx_buf[3];
> +       u8                      fifo_tx_buf[2];
> +       u8                      fifo_rx_buf[AD4130_FIFO_SIZE *
> +                                           AD4130_FIFO_MAX_SAMPLE_SIZE];
> +};

...

> +static IIO_DEVICE_ATTR(hwfifo_watermark, 0444, ad4130_get_fifo_watermark, NULL, 0);
> +static IIO_DEVICE_ATTR(hwfifo_enabled, 0444, ad4130_get_fifo_enabled, NULL, 0);

IIO_DEVICE_ATTR_RO()

...

> +       for (i = 0; i < st->num_gpios; i++)
> +               val |= BIT(st->gpio_offsets[i]);

This might overflow.

...

> +       for (i = 0; i < st->num_vbias_pins; i++)
> +               val |= BIT(st->vbias_pins[i]);

Ditto.

...

> +       st->regmap = devm_regmap_init(dev, NULL, st, &ad4130_regmap_config);
> +       if (IS_ERR(st->regmap))
> +               return PTR_ERR(st->regmap);

Can it use regular regmap SPI?

-- 
With Best Regards,
Andy Shevchenko