Re: [PATCH RFC 2/5] iio: adc: ad4030: add driver for ad4030-24

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 29 Jul 2024 16:42:16 +0200
"Esteban Blanc" <eblanc@baylibre.com> wrote:

> On Sat Jun 29, 2024 at 6:39 PM CEST, Jonathan Cameron wrote:
> > On Thu, 27 Jun 2024 13:59:13 +0200
> > Esteban Blanc <eblanc@baylibre.com> 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)
> > > +{
> > > +	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,
> > > +	};
> > > +	int ret;
> > > +
> > > +	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);  
> >
> > Can you just use spi_write_then_read() here?
> >  
> 
> I was planning on doing that. But I'm getting a timeout issue when
> using `spi_write_then_read`. I can see the tx_data going out, rx_data
> is recived but CS is kept asserted. I need to investigate more but in
> the meantime I'm using this as it is working. I will remove this
> workaround if I can find a fix and add a comment for now.
Fair enough. We've had a few drivers where the timing doesn't work
recently. Definitely good to leave a comment to avoid a 'fix' :)

> 
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	if (st->chip->precision_bits == 16)
> > > +		offset <<= 16;
> > > +	else
> > > +		offset <<= 8;  
> >
> > As below. This is hard tor read. Just use appropriate unaligned gets for the
> > two cases to extract the write bytes directly.
> >  
> > > +	*val = be32_to_cpu(offset);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int ad4030_set_chan_gain(struct iio_dev *indio_dev, int ch, int gain_int,
> > > +				int gain_frac)
> > > +{
> > > +	struct ad4030_state *st = iio_priv(indio_dev);
> > > +	__be16 val;
> > > +	u64 gain;
> > > +
> > > +	gain = mul_u32_u32(gain_int, MICRO) + gain_frac;
> > > +
> > > +	if (gain > AD4030_REG_GAIN_MAX_GAIN)
> > > +		return -EINVAL;
> > > +
> > > +	val = cpu_to_be16(DIV_ROUND_CLOSEST_ULL(gain * 0x8000, MICRO));
> > > +
> > > +	return regmap_bulk_write(st->regmap, AD4030_REG_GAIN_CHAN(ch), &val,
> > > +			  AD4030_REG_GAIN_BYTES_NB);
> > > +}
> > > +
> > > +static int ad4030_set_chan_offset(struct iio_dev *indio_dev, int ch, int offset)
> > > +{
> > > +	struct ad4030_state *st = iio_priv(indio_dev);
> > > +	__be32 val;
> > > +
> > > +	if (offset < st->min_offset || offset > st->max_offset)
> > > +		return -EINVAL;
> > > +
> > > +	val = cpu_to_be32(offset);
> > > +	if (st->chip->precision_bits == 16)
> > > +		val >>= 16;
> > > +	else
> > > +		val >>= 8;  
> >
> > I 'think' I get what this is doing but not 100% sure as it's a bit too unusual
> > and I'm not even sure what happens if we shift a __be32 value on a little endian
> > system. I would instead split this into appropriate cpu_to_be24() and cpu_to_be16()
> > to put the value directly in the right place rather than shifting in place.  
> 
> cpu_to_be24 does not exist yet. I will have a look on how to add them.
Ah. Almost by definition be24 isn't aligned in some cases.
So put_unaligned_be24() is what you are looking for.
My mistake!

Jonathan

> 
> 
> All the other comments will be addressed in V2.
> 
> Best regards,
>