Messages in this thread | | | Date | Thu, 23 Jun 2022 18:27:12 +0300 | Subject | Re: [PATCH v5 2/2] iio: adc: ad4130: add AD4130 driver | From | Cosmin Tanislav <> |
| |
On 6/20/22 21:29, Andy Shevchenko wrote: > 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?
Whatever is below it, grouped together. This is not hard to understand.
> >> + 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.
No it might not. num_gpios is at max AD4130_MAX_GPIOS, which is 4.
> > ... > >> + for (i = 0; i < st->num_vbias_pins; i++) >> + val |= BIT(st->vbias_pins[i]); > > Ditto.
No it might not. num_vbias_pins is at max AD4130_MAX_ANALOG_PINS, which is 16.
> > ... > >> + 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?
No. I'm pretty sure I've already talked about this.
>
| |