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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> ... > > +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. > > + 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. All the other comments will be addressed in V2. Best regards, -- Esteban "Skallwar" Blanc BayLibre