On Mon, 15 Nov 2021 12:23:39 +0200 Antoniu Miclaus <antoniu.miclaus@xxxxxxxxxx> wrote: > The AD7293 is a Power Amplifier drain current controller > containing functionality for general-purpose monitoring > and control of current, voltage, and temperature, integrated > into a single chip solution with an SPI-compatible interface. > > Datasheet: > https://www.analog.com/media/en/technical-documentation/data-sheets/AD7293.pdf > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@xxxxxxxxxx> Hi Antoniu A few minor / trivial things noticed whilst having a read through. They are all things I'd have either ignored or fixed up if you weren't likely to be doing a v4 anyway because of the dt-bindings... Seeing as you probably will be, please tidy these up as well. This is looking like a nice driver to me. I'm a bit unsure if we should have this in a directory called amplifiers though. The description is (I think) referring to closed loop control of current as shown in figure 46. The circuit with external transistor + sense resistor operates as a current DAC. As such I'd move this to the DAC directory as I think that's the primary purpose intended for this device. I guess a follow up set would supply explicit support for closed loop operation? That can be enabled when someone has a use case for it. Thanks, Jonathan > --- ... > + > +static int __ad7293_spi_read(struct ad7293_state *st, unsigned int reg, > + u16 *val) > +{ > + int ret; > + struct spi_transfer t = {0}; > + > + ret = ad7293_page_select(st, reg); > + if (ret) > + return ret; > + > + st->data[0] = AD7293_READ | FIELD_GET(AD7293_REG_ADDR_MSK, reg); > + st->data[1] = 0x0; > + st->data[2] = 0x0; > + > + t.tx_buf = &st->data[0]; > + t.rx_buf = &st->data[0]; > + t.len = 1 + FIELD_GET(AD7293_TRANSF_LEN_MSK, reg); Given you are going to use this field multiple times, I would use a local variable. > + > + ret = spi_sync_transfer(st->spi, &t, 1); > + if (ret) > + return ret; > + > + if (FIELD_GET(AD7293_TRANSF_LEN_MSK, reg) == 1) > + *val = st->data[1]; > + else > + *val = get_unaligned_be16(&st->data[1]); > + > + return 0; > +} ... > + > +static int __ad7293_spi_write(struct ad7293_state *st, unsigned int reg, > + u16 val) > +{ > + int ret; > + unsigned int length = 1 + FIELD_GET(AD7293_TRANSF_LEN_MSK, reg); I suggest not having the 1 + here > + > + ret = ad7293_page_select(st, reg); > + if (ret) > + return ret; > + > + st->data[0] = FIELD_GET(AD7293_REG_ADDR_MSK, reg); > + > + if (FIELD_GET(AD7293_TRANSF_LEN_MSK, reg) == 1) then this becomes if (length == 1) > + st->data[1] = val; > + else > + put_unaligned_be16(val, &st->data[1]); > + > + return spi_write(st->spi, &st->data[0], length); and you can use length + 1 here. I think that ends up a little bit clearer. > +} > + ... > + > +static int ad7293_read_avail(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + const int **vals, int *type, int *length, > + long info) > +{ > + switch (info) { > + case IIO_CHAN_INFO_OFFSET: > + *vals = dac_offset_table; > + *type = IIO_VAL_INT; > + *length = ARRAY_SIZE(dac_offset_table); > + > + return IIO_AVAIL_LIST; > + case IIO_CHAN_INFO_SCALE: > + *type = IIO_VAL_INT; > + > + switch (chan->type) { > + case IIO_VOLTAGE: > + *vals = adc_range_table; > + *length = ARRAY_SIZE(adc_range_table); > + break; > + case IIO_CURRENT: > + *vals = isense_gain_table; > + *length = ARRAY_SIZE(isense_gain_table); > + break; > + default: > + return -EINVAL; > + } > + > + return IIO_AVAIL_LIST; Trivial: I'd prefer to see this return where the breaks are above. Nice to pair the values and type in a couple of lines rather than having to look down here. > + default: > + return -EINVAL; > + } > +} > + > +#define AD7293_CHAN_ADC(_channel) { \ > + .type = IIO_VOLTAGE, \ > + .output = 0, \ > + .indexed = 1, \ > + .channel = _channel, \ > + .info_mask_separate = \ > + BIT(IIO_CHAN_INFO_RAW) | \ > + BIT(IIO_CHAN_INFO_SCALE) | \ > + BIT(IIO_CHAN_INFO_OFFSET), \ > + .info_mask_shared_by_type_available = \ > + BIT(IIO_CHAN_INFO_SCALE) \ > +} Trivial, but I think you could sensibly reduce how many lines these are spread over without loss of readability. #define AD7293_CHAN_ADC(_channel) { \ .type = IIO_VOLTAGE, \ .output = 0, \ .indexed = 1, \ .channel = _channel, \ .info_mask_separate = \ BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE) | \ BIT(IIO_CHAN_INFO_OFFSET), \ .info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SCALE) \ } etc > + > +#define AD7293_CHAN_DAC(_channel) { \ > + .type = IIO_VOLTAGE, \ > + .output = 1, \ > + .indexed = 1, \ > + .channel = _channel, \ > + .info_mask_separate = \ > + BIT(IIO_CHAN_INFO_RAW) | \ > + BIT(IIO_CHAN_INFO_OFFSET), \ > + .info_mask_shared_by_type_available = \ > + BIT(IIO_CHAN_INFO_OFFSET), \ > +} > + > +#define AD7293_CHAN_ISENSE(_channel) { \ > + .type = IIO_CURRENT, \ > + .output = 0, \ > + .indexed = 1, \ > + .channel = _channel, \ > + .info_mask_separate = \ > + BIT(IIO_CHAN_INFO_RAW) | \ > + BIT(IIO_CHAN_INFO_OFFSET) | \ > + BIT(IIO_CHAN_INFO_SCALE), \ > + .info_mask_shared_by_type_available = \ > + BIT(IIO_CHAN_INFO_SCALE) \ > +} > + > +#define AD7293_CHAN_TEMP(_channel) { \ > + .type = IIO_TEMP, \ > + .output = 0, \ > + .indexed = 1, \ > + .channel = _channel, \ > + .info_mask_separate = \ > + BIT(IIO_CHAN_INFO_RAW) | \ > + BIT(IIO_CHAN_INFO_OFFSET), \ > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) \ > +} > +
