Re: [PATCH v2 2/6] iio: adc: ad4030: add driver for ad4030-24

["Esteban Blanc" <eblanc@baylibre.com>]

On Sun Dec 22, 2024 at 6:57 AM CET, Marcelo Schmitt wrote:
> Hello Esteban, some comments inline.
>
> On 12/19, Esteban Blanc wrote:
> > This adds a new driver for the Analog Devices INC. AD4030-24 ADC.
> > 
> > The driver implements basic support for the AD4030-24 1 channel
> > differential ADC with hardware gain and offset control.
> > 
> > Signed-off-by: Esteban Blanc <eblanc@baylibre.com>
> > ---
> [...]
> > +
> > +static int ad4030_spi_read(void *context, const void *reg, size_t reg_size,
> > +			   void *val, size_t val_size)
> > +{
> > +	int ret;
> > +	struct ad4030_state *st = context;
> > +	struct spi_transfer xfer = {
> > +		.tx_buf = st->tx_data,
> > +		.rx_buf = st->rx_data.raw,
> > +		.len = reg_size + val_size,
> > +		.speed_hz = AD4030_SPI_MAX_REG_XFER_SPEED,
> Is speed_hz really needed? What happens if the controller can't clock at 80MHz?
The goal was to reduce the speed when reading/writing to a register with
a value like 10MHz. The issue I ran into is that with my setup (Zedboard
with FPGA-based SPI controller) when I chose a speed lower than 80Mhz
the value read are wrong. The whoami check in `ad4030_detect_chip_info`
is failling. So for now, I left it at 80Mhz while I'm trying to figure
out what's going on with the controller
>
> > +	};
> > +
> > +	if (xfer.len > ARRAY_SIZE(st->tx_data) ||
> > +	    xfer.len > ARRAY_SIZE(st->rx_data.raw))
> > +		return  -EINVAL;
>
> Would it make sense to bring register configuration mode commands into the
> regmap calls?
> I mean, to do the ad4030_enter_config_mode() transfer here and the
> ad4030_exit_config_mode() at the end of this function.
> From datasheet, it looks like both enter/exit config mode are required for reg
> access so why not doing them in the regmap callbacks?
> With that, I think it won't be needed to call register config mode functions
> in ad4030_single_conversion() and in buffer enable/disable functions.
> Might need implement regmap_config read and write callbacks to properly handle
> regmap_bulk_read/write interface.
Yes, good idea.
>
>
> > +
> > +	memset(st->tx_data, 0, ARRAY_SIZE(st->tx_data));
> > +	memcpy(st->tx_data, reg, reg_size);
> > +
> > +	ret = spi_sync_transfer(st->spi, &xfer, 1);
> > +	if (ret)
> > +		return ret;
> > +
> > +	memcpy(val, &st->rx_data.raw[reg_size], val_size);
> > +
> > +	return ret;
> > +}
> > +
> [...]
> > +
> > +static int ad4030_get_chan_calibscale(struct iio_dev *indio_dev,
> > +				      struct iio_chan_spec const *chan,
> > +				      int *val,
> > +				      int *val2)
> > +{
> > +	struct ad4030_state *st = iio_priv(indio_dev);
> > +	u16 gain;
> > +	int ret;
> > +
> > +	ret = regmap_bulk_read(st->regmap, AD4030_REG_GAIN_CHAN(chan->address),
> > +			       st->rx_data.raw, AD4030_REG_GAIN_BYTES_NB);
> > +	if (ret)
> > +		return ret;
> > +
> > +	gain = get_unaligned_be16(st->rx_data.raw);
> My impression is that it is a bit odd to handle endianness after/before
> calling regmap_read/write(). Can you try set
> .val_format_endian_default = REGMAP_ENDIAN_BIG, in ad4030_regmap_bus?
> If that doesn't help, what about doing the get/set_unaligned stuff within
> the bus read/write calls?
I've addressed that in another email after Jonathan's comment.
>
> > +
> > +	/* From datasheet: multiplied output = input × gain word/0x8000 */
> > +	*val = gain / 0x8000;
> Use a define to give a name to the gain constant?
Sure.

Best regards,

-- 
Esteban Blanc
BayLibre