On Mon, 3 Jun 2024 09:22:00 +0800 Kim Seer Paller <kimseer.paller@xxxxxxxxxx> wrote: > LTC2664 4 channel, 16 bit Voltage Output SoftSpan DAC > LTC2672 5 channel, 16 bit Current Output Softspan DAC > > Reported-by: kernel test robot <lkp@xxxxxxxxx> > Closes: https://lore.kernel.org/oe-kbuild-all/202405241141.kYcxrSem-lkp@xxxxxxxxx/ > Co-developed-by: Michael Hennerich <michael.hennerich@xxxxxxxxxx> > Signed-off-by: Michael Hennerich <michael.hennerich@xxxxxxxxxx> > Signed-off-by: Kim Seer Paller <kimseer.paller@xxxxxxxxxx> A few minor things from me to add to the more detailed comments from Nuno and David. > +static int ltc2664_dac_code_write(struct ltc2664_state *st, u32 chan, u32 input, > + u16 code) > +{ > + struct ltc2664_chan *c = &st->channels[chan]; > + int ret, reg; > + > + guard(mutex)(&st->lock); > + /* select the correct input register to write to */ > + if (c->toggle_chan) { > + ret = regmap_write(st->regmap, LTC2664_CMD_TOGGLE_SEL, > + input << chan); > + if (ret) > + return ret; > + } > + /* > + * If in toggle mode the dac should be updated by an > + * external signal (or sw toggle) and not here. > + */ > + if (st->toggle_sel & BIT(chan)) > + reg = LTC2664_CMD_WRITE_N(chan); > + else > + reg = LTC2664_CMD_WRITE_N_UPDATE_N(chan); > + > + ret = regmap_write(st->regmap, reg, code); > + if (ret) > + return ret; > + > + c->raw[input] = code; > + > + if (c->toggle_chan) { > + ret = regmap_write(st->regmap, LTC2664_CMD_TOGGLE_SEL, > + st->toggle_sel); > + if (ret) > + return ret; > + } > + > + return ret; return 0; you won't get here otherwise, and making that explicit gives more readable code. > +} > + > +static int ltc2664_dac_code_read(struct ltc2664_state *st, u32 chan, u32 input, > + u32 *code) > +{ > + guard(mutex)(&st->lock); > + *code = st->channels[chan].raw[input]; > + > + return 0; > +} > + > +static const int ltc2664_raw_range[] = {0, 1, U16_MAX}; { 0, 1, U16_MAX }; preferred (extra spaces) > + > +static const struct ltc2664_chip_info ltc2664_chip = { > + .id = LTC2664, > + .name = "ltc2664", > + .scale_get = ltc2664_scale_get, > + .offset_get = ltc2664_offset_get, > + .measurement_type = IIO_VOLTAGE, > + .num_channels = ARRAY_SIZE(ltc2664_channels), > + .iio_chan = ltc2664_channels, > + .span_helper = ltc2664_span_helper, > + .num_span = ARRAY_SIZE(ltc2664_span_helper), > + .internal_vref = 2500, > + .manual_span_support = true, > + .rfsadj_support = false, > +}; > + > +static const struct ltc2664_chip_info ltc2672_chip = { > + .id = LTC2672, As below. Seeing an id in here made me wonder what was going on given we don't have a whoami register on these. Please remove it as that model of handling chip specific stuff always bites us in complexity and lack of flexibility as we expand the parts supported by a driver. > + .name = "ltc2672", > + .scale_get = ltc2672_scale_get, > + .offset_get = ltc2672_offset_get, > + .measurement_type = IIO_CURRENT, > + .num_channels = ARRAY_SIZE(ltc2672_channels), > + .iio_chan = ltc2672_channels, > + .span_helper = ltc2672_span_helper, > + .num_span = ARRAY_SIZE(ltc2672_span_helper), > + .internal_vref = 1250, > + .manual_span_support = false, > + .rfsadj_support = true, > +}; > + > +static int ltc2664_set_span(const struct ltc2664_state *st, int min, int max, > + int chan) > +{ > + const struct ltc2664_chip_info *chip_info = st->chip_info; > + const int (*span_helper)[2] = chip_info->span_helper; > + int span, ret; > + > + st->iio_channels[chan].type = chip_info->measurement_type; > + > + for (span = 0; span < chip_info->num_span; span++) { > + if (min == span_helper[span][0] && max == span_helper[span][1]) > + break; > + } > + > + if (span == chip_info->num_span) > + return -EINVAL; > + > + ret = regmap_write(st->regmap, LTC2664_CMD_SPAN_N(chan), > + (chip_info->id == LTC2672) ? span + 1 : span); Make this specific data, not id based. The reasoning of there being a magic value (offmode) as 0 is a bit obscure so maybe a callback is best plan. Or... put a 0,0 entry in span_helper[] and check for that + ignore it or error out if anyone tries to use it. Drop that id in the chip_info structure as well as having it there will make people not consider if their 'code' should be 'data' in future cases similar to this. > + if (ret) > + return ret; > + > + return span; > +} > +
