On Fri, Dec 4, 2020 at 8:17 PM Cristian Pop <cristian.pop@xxxxxxxxxx> wrote: > > The AD5766/AD5767 are 16-channel, 16-bit/12-bit, voltage output dense DACs > Digital-to-Analog converters. > > This change adds support for these DACs. > Link: https://www.analog.com/media/en/technical-documentation/data-sheets/ad5766-5767.pdf Can we use Datasheet: tag instead, please? > > Signed-off-by: Cristian Pop <cristian.pop@xxxxxxxxxx> No blank lines in tag block, please. ... > + * Analog Devices AD5766, AD5767 > + * Digital to Analog Converters driver Looks like one line. ... > +#include <linux/delay.h> > +#include <linux/device.h> > +#include <linux/gpio/consumer.h> > +#include <linux/module.h> > +#include <linux/spi/spi.h> > +#include <linux/iio/iio.h> > +#include <linux/bitfield.h> Keep it sorted? ... > +#define AD5766_CMD_WR_IN_REG(x) (0x10 | ((x) & 0xF)) > +#define AD5766_CMD_WR_DAC_REG(x) (0x20 | ((x) & 0xF)) Why not GENMASK()? ... > +#define AD5766_CMD_READBACK_REG(x) (0x80 | ((x) & 0xF)) Ditto. ... > +enum ad5766_type { > + ID_AD5766, > + ID_AD5767, > +}; This may be problematic in case we switch to device_get_match_data(). ... > +enum ad5766_voltage_range { > + AD5766_VOLTAGE_RANGE_M20V_0V, > + AD5766_VOLTAGE_RANGE_M16V_to_0V, > + AD5766_VOLTAGE_RANGE_M10V_to_0V, > + AD5766_VOLTAGE_RANGE_M12V_to_14V, > + AD5766_VOLTAGE_RANGE_M16V_to_10V, > + AD5766_VOLTAGE_RANGE_M10V_to_6V, > + AD5766_VOLTAGE_RANGE_M5V_to_5V, > + AD5766_VOLTAGE_RANGE_M10V_to_10V, > + AD5766_VOLTAGE_RANGE_MAX, No comma for terminator line. > +}; ... > +enum { > + AD5766_DITHER_PWR, > + AD5766_DITHER_INVERT + comma > +}; ... > +/* > + * External dither signal can be composed with the DAC output, if activated. > + * The dither signals are applied to the N0 and N1 input pins. > + * Dither source for each of the channel can be selected by writing to > + * "dither_source",a 32 bit variable and two bits are used for each of the 16 > + * channels: 0: NO_DITHER, 1: N0, 2: N1. > + * This variable holds available dither source strings. > + */ > +static const char * const ad5766_dither_sources[] = { > + "NO_DITHER", > + "N0", > + "N1", > +}; Can't we rather be simpler, i.e. use 0,1 and -1, where the latter for NO_DITHER cas?. ... > +/* > + * Dither signal can also be scaled. > + * Available dither scale strings coresponding to "dither_scale" field in corresponding > + * "struct ad5766_state". > + * "dither_scale" is a 32 bit variable and two bits are used for each of the 16 > + * channels: 0: NO_SCALING, 1: 75%_SCALING, 2: 50%_SCALING, 3: 25%_SCALING. > + */ > +static const char * const ad5766_dither_scales[] = { > + "NO_SCALING", > + "75%_SCALING", > + "50%_SCALING", > + "25%_SCALING", > +}; Oh, no. Please, use plain numbers in percentages. ... > + * @cached_offset: Cached range coresponding to the selected offset corresponding Please, run checkpatch.pl --code-spell (or how is it called?). ... > + * @offset_avail: Offest available table Ditto. Offset (I suppose) ... > +static int _ad5766_spi_write(struct ad5766_state *st, u8 command, u16 data) > +{ > + st->data[0].b8[0] = command; > + st->data[0].b8[1] = (data & 0xFF00) >> 8; > + st->data[0].b8[2] = (data & 0x00FF) >> 0; Please, use get_unaliged_XX() / put_unaligned_XX() and other related macros / APIs. > + return spi_write(st->spi, &st->data[0].b8[0], 3); > +} ... > +static int ad5766_reset(struct ad5766_state *st) > +{ > + int ret = 0; In general it's a bad idea and in particular here it's not needed. > + return 0; > +} ... > + ret = _ad5766_spi_write(st, AD5766_CMD_DITHER_SIG_1, > + st->dither_source & 0xFFFF); Do you really need this conjunction? If so, why not GENMASK()? > + if (ret) > + return ret; ... > + ret = _ad5766_spi_write(st, AD5766_CMD_INV_DITHER, st->dither_invert); > + if (ret) > + return ret; > + > + return 0; return _ad5766_spi_write(…); ... > +static int ad5766_set_offset(struct ad5766_state *st, int val, int val2) > +{ > + int i; > + s32 (*tbl)[AD5766_VOLTAGE_RANGE_MAX][2] = &(st->offset_avail); Too many parentheses. > + > + for (i = 0; i < AD5766_VOLTAGE_RANGE_MAX; i++) { ARRAY_SIZE() ? > + if ((*tbl)[i][0] == val && (*tbl)[i][1] == val2) { > + st->cached_offset = i; > + return 0; > + } > + } Entire function hurts my eyes. Can you give some time and think over it again? > + return -EINVAL; > +} ... > +static int ad5766_set_scale(struct ad5766_state *st, int val, int val2) Ditto. ... > + *vals = (const int *)st->scale_avail; Why do you need casting? ... > + *vals = (const int *)st->offset_avail; Ditto. ... > +static int ad5766_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int *val, > + int *val2, > + long m) It may take much less LOCs. ... > +static int ad5766_write_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int val, > + int val2, > + long info) Ditto. ... > + const int max_val = (1 << chan->scan_type.realbits); > + > + if (val >= max_val || val < 0) > + return -EINVAL; > + val <<= chan->scan_type.shift; You can do better, i.e. drop unneeded temporary variable and use fls(). ... > + return (source >> chan->channel * 2); Seems parentheses in incorrect place in case you want to increase robustness. > +} ... > + st->dither_source |= (mode << (chan->channel * 2)); It's not LISP, seriously. I'm wondering if Analog has internal mailing list (and perhaps a wiki with common tips and tricks for Linux kernel programming) for review... ... > + return (scale >> chan->channel * 2); As above. ... > + st->dither_scale |= (scale << (chan->channel * 2)); As above. ... > + return sprintf(buf, "%u\n", 0x01 & > + ~(st->dither_power_ctrl >> chan->channel)); Oh là là. !(st->dither_power_ctrl & BIT(chan->channel)) ? ... > + return sprintf(buf, "%u\n", 0x01 & > + (st->dither_invert >> chan->channel)); Ditto. ... > + default: > + ret = -EINVAL; > + break; Point of this? Can't return directly? > + } > + > + return ret; ... > + switch ((u32)private) { Why casting? ... > + default: > + ret = -EINVAL; > + break; Why not to return here? > + } > + return ret ? ret : len; return len; ? ... > + offset = div_s64(offset * 1000000, denom); > + st->offset_avail[i][0] = div_s64(offset, 1000000); > + div_s64_rem(offset, 1000000, &st->offset_avail[i][1]); ... > + scale = div_u64((scale * 1000000000), (1 << realbits)); > + st->scale_avail[i][0] = (int)div_u64(scale, 1000000); > + div_s64_rem(scale, 1000000, &st->scale_avail[i][1]); Perhaps it's time to extend units.h with mV / V / uV / etc? ... > +static const struct of_device_id ad5766_dt_match[] = { > + { .compatible = "adi,ad5766" }, > + { .compatible = "adi,ad5767" }, > + {}, No comma for terninator. > +}; -- With Best Regards, Andy Shevchenko
