On Mon, 22 Nov 2021 15:36:38 +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> > --- > changes in v4: > - move driver to DAC directory, as suggested > - use local variable `length` where FIELD_GET is used multiple times > - return `IIO_AVAIL_LIST` directly, instead of break > - reduce number of lines in the channel definitions section > drivers/iio/dac/Kconfig | 11 + > drivers/iio/dac/Makefile | 1 + > drivers/iio/dac/ad7293.c | 892 +++++++++++++++++++++++++++++++++++++++ > 3 files changed, 904 insertions(+) > create mode 100644 drivers/iio/dac/ad7293.c > New day, new things spotted, but they are all very minor so assuming the dt patch gets a tag from Rob and no one else raises any concerns I can tidy these up whilst applying. Obviously if you do happen to do a v5 then handle these as well. ... > + > +static int ad7293_set_offset(struct ad7293_state *st, unsigned int ch, u16 offset) > +{ > + if (ch < AD7293_TSENSE_MIN_OFFSET_CH) > + return ad7293_spi_write(st, AD7293_REG_VIN0_OFFSET + ch, offset); > + else if (ch < AD7293_ISENSE_MIN_OFFSET_CH) > + return ad7293_spi_write(st, AD7293_REG_TSENSE_INT_OFFSET + (ch - AD7293_TSENSE_MIN_OFFSET_CH), offset); These are 'very' long lines. I'll tweak this whilst applying if you don't end up doing a v5 for any other reason. > + else if (ch < AD7293_VOUT_MIN_OFFSET_CH) > + return ad7293_spi_write(st, AD7293_REG_ISENSE0_OFFSET + (ch - AD7293_ISENSE_MIN_OFFSET_CH), offset); > + else if (ch <= AD7293_VOUT_MAX_OFFSET_CH) > + return ad7293_spi_update_bits(st, AD7293_REG_UNI_VOUT0_OFFSET + (ch - AD7293_VOUT_MIN_OFFSET_CH), > + AD7293_REG_VOUT_OFFSET_MSK, > + FIELD_PREP(AD7293_REG_VOUT_OFFSET_MSK, offset)); > + > + return -EINVAL; > +} > + ... > + > +static int ad7293_write_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int val, int val2, long info) > +{ > + struct ad7293_state *st = iio_priv(indio_dev); > + > + switch (info) { > + case IIO_CHAN_INFO_RAW: > + switch (chan->type) { > + case IIO_VOLTAGE: > + if (!chan->output) > + return -EINVAL; > + > + return ad7293_dac_write_raw(st, chan->channel, val); > + default: > + return -EINVAL; > + } > + case IIO_CHAN_INFO_OFFSET: > + switch (chan->type) { > + case IIO_VOLTAGE: > + if (chan->output) > + return ad7293_set_offset(st, chan->channel + AD7293_VOUT_MIN_OFFSET_CH, val); More long lines. I'll add some line breaks if I end up tidying this up whilst applying. Ideally we should still stay under 80 chars where it doesn't hurt readability and under 100 in any case other than possibly a very long string that we don't want to break because it will make it hard to grep for. > + else > + return ad7293_set_offset(st, chan->channel, val); > + case IIO_CURRENT: > + return ad7293_set_offset(st, chan->channel + AD7293_ISENSE_MIN_OFFSET_CH, val); > + case IIO_TEMP: > + return ad7293_set_offset(st, chan->channel + AD7293_TSENSE_MIN_OFFSET_CH, val); > + default: > + return -EINVAL; > + } > + case IIO_CHAN_INFO_SCALE: > + switch (chan->type) { > + case IIO_VOLTAGE: > + return ad7293_adc_set_scale(st, chan->channel, val); > + case IIO_CURRENT: > + return ad7293_isense_set_scale(st, chan->channel, val); > + default: > + return -EINVAL; > + } > + default: > + return -EINVAL; > + } > +} ... > + > +static int ad7293_init(struct ad7293_state *st) > +{ > + int ret; > + u16 chip_id; > + struct spi_device *spi = st->spi; > + unsigned int supply; > + > + ret = ad7293_properties_parse(st); > + if (ret) > + return ret; > + > + ret = ad7293_reset(st); > + if (ret) > + return ret; > + > + ret = regulator_enable(st->reg_avdd); > + if (ret) { > + dev_err(&spi->dev, "Failed to enable specified AVDD Voltage!\n"); > + return ret; > + } > + > + ret = devm_add_action_or_reset(&spi->dev, ad7293_reg_disable, > + st->reg_avdd); > + if (ret) > + return ret; > + > + ret = regulator_enable(st->reg_vdrive); > + if (ret) { > + dev_err(&spi->dev, "Failed to enable specified VDRIVE Voltage!\n"); > + return ret; > + } > + > + ret = devm_add_action_or_reset(&spi->dev, ad7293_reg_disable, > + st->reg_vdrive); > + if (ret) > + return ret; > + > + supply = regulator_get_voltage(st->reg_avdd); Missed this before, but regulator_get_voltage() returns a signed int and it can be an error code. If that happens we should return that error rather than replacing it with -EINVAL. > + if (supply > 5500000 || supply < 4500000) > + return -EINVAL; > + > + supply = regulator_get_voltage(st->reg_vdrive); > + if (supply > 5500000 || supply < 1700000) > + return -EINVAL; > + > + /* Check Chip ID */ > + ret = __ad7293_spi_read(st, AD7293_REG_DEVICE_ID, &chip_id); > + if (ret) > + return ret; > + > + if (chip_id != AD7293_CHIP_ID) { > + dev_err(&spi->dev, "Invalid Chip ID.\n"); > + return -EINVAL; > + } > + > + return 0; > +} > +
