Hi Andy,
Thanks for the preliminary review...
> From: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> Sent: Thursday, November 11, 2021 2:49 PM
> To: Sa, Nuno <Nuno.Sa@xxxxxxxxxx>
> Cc: linux-iio <linux-iio@xxxxxxxxxxxxxxx>; Jonathan Cameron
> <jic23@xxxxxxxxxx>
> Subject: Re: [RFC PATCH 1/1] iio: dac: add support for ltc2688
>
> [External]
>
> On Thu, Nov 11, 2021 at 1:01 PM Nuno Sá <nuno.sa@xxxxxxxxxx>
> wrote:
> >
> > The LTC2688 is a 16 channel, 16 bit, +-15V DAC with an integrated
> > precission reference. It is guaranteed monotonic and has built in
>
> precision
>
> > rail-to-rail output buffers that can source or sink up to 20 mA.
>
> It's in good shape, several comments below, though.
>
> ...
>
> > +enum {
> > + LTC2688_SPAN_RANGE_0V_5V,
> > + LTC2688_SPAN_RANGE_M10V_10V,
> > + LTC2688_SPAN_RANGE_M15V_15V,
> > + /*
> > + * These 2 were deliberately re-arranged to be the last items as
> they
> > + * have the same fs and we will just return 1 in read_available.
>
> WTH "fs" mean?
full scale... will spell it out
> > + */
> > + LTC2688_SPAN_RANGE_0V_10V,
> > + LTC2688_SPAN_RANGE_M5V_5V,
> > + LTC2688_SPAN_RANGE_MAX
> > +};
>
> ...
>
> > +static const int ltc2688_off_tbl[LTC2688_SPAN_RANGE_MAX] = {
> > + [LTC2688_SPAN_RANGE_0V_5V] = 0,
> > + [LTC2688_SPAN_RANGE_M10V_10V] = -32768,
> > + [LTC2688_SPAN_RANGE_M15V_15V] = -32768,
>
> > + [LTC2688_SPAN_RANGE_0V_10V] = 0,
> > + [LTC2688_SPAN_RANGE_M5V_5V] = -32768
>
> + Comma
>
> Isn't it more natural to move them up by two lines?
There's a reason for this to be ordered like this. Anyways,
as I said in the cover letter I will probably remove all of these things
used to validate scale + offset pairs. I think is a bit overdone...
> > +};
> > +
> > +static const int ltc2688_reg_val[LTC2688_SPAN_RANGE_MAX] = {
> > + [LTC2688_SPAN_RANGE_0V_5V] = 0,
> > + [LTC2688_SPAN_RANGE_M10V_10V] = 3,
> > + [LTC2688_SPAN_RANGE_M15V_15V] = 4,
> > + [LTC2688_SPAN_RANGE_0V_10V] = 1,
> > + [LTC2688_SPAN_RANGE_M5V_5V] = 2
>
> As per above.
>
> > +};
>
> ...
>
> > +static int ltc2688_span_set(const struct ltc2688_state *st, int c, int
> span)
> > +{
> > + const struct ltc2688_chan *chan = &st->channels[c];
> > + u32 i = ARRAY_SIZE(chan->span_tbl);
> > + int off = ltc2688_offset_avail[chan->offset_idx];
>
> > + while (i--) {
>
> Can ARRAY_SIZE() give 0?
nope
> If not, do {} while (--i); looks more natural.
can do like that
> > + if (span == chan->span_tbl[i][1]) {
> > + /*
> > + * The selected scale needs to fit the current offset.
> > + * Note that for [0V 10V] and [-5V 5V], as the scale is
> > + * the same, we will always succeed here. Hence if
> > + * someone really wants [0 10V] and does not set the
> > + * offset to 0, then :man-shrugging:
> > + */
> > + if (off == ltc2688_off_tbl[i]) {
> > + break;
>
> > + } else if (i < LTC2688_SPAN_RANGE_0V_10V) {
>
> Redundant 'else'.
Why? Note that I really (where possible) want to make a distinction
between the errors... Again, I will probably remove this part and
only match 'if (span == chan->span_tbl[i][1])'
> > + /*
> > + * At this point, only one offset is valid so we
> > + * already can assume error.
> > + */
> > + dev_err(&st->spi->dev, "Offset(%d) not valid for
> current scale(0.%09d)\n",
> > + off, span);
> > + return -EPERM;
> > + }
>
> > + continue;
>
> Redundant, see below.
true, but at this point I already know that doing the next if (!i) is not really
needed. But yeah, less lines of code
>
> > + }
>
> > + if (!i) {
> > + dev_err(&st->spi->dev, "span(%d) not available\n",
> span);
> > + return -EINVAL;
> > + }
>
> This is invariant to the loop.
>
> > + }
> > +
> > + return regmap_update_bits(st->regmap,
> LTC2688_CMD_CH_SETTING(c),
> > + LTC2688_CH_SPAN_MSK,
> > + FIELD_PREP(LTC2688_CH_SPAN_MSK,
> ltc2688_reg_val[i]));
> > +};
>
> ...
>
> > + if (off != ltc2688_offset_avail[0] && off !=
> ltc2688_offset_avail[1])
> > + return -EINVAL;
>
> Extra condition, See below.
>
> > + if (off == ltc2688_offset_avail[0])
> > + chan->offset_idx = 0;
> > + else
>
> else if
>
> > + chan->offset_idx = 1;
>
> else
> return -EINVAL;
sure...
> ...
>
> > + ret = regmap_read(st->regmap, reg, ®val);
> > + if (ret < 0)
>
> Is it the first time you tested explicitly for a negative result?
should not be like this...
> > + return ret;
>
> ...
>
> > + return sprintf(buf, "%u\n", !!(regval & BIT(chan->channel)));
>
> sysfs_emit() ?
definetly...
> > +}
>
> ...
>
> > + struct ltc2688_state *st = iio_priv(indio_dev);
> > + bool en;
> > + int ret;
> > + u32 val = 0, reg;
>
> Reversed xmas tree order ?
sure
>
> > + ret = strtobool(buf, &en);
>
> kstrtobool()
sure
>
> ...
>
> > + if (ret)
> > + return ret;
> > + break;
>
> What does this mean?
means I was sleeping...
> ...
>
> > + "4",
> > + "8",
> > + "16",
> > + "32",
> > + "64",
>
> One line?
yes
>
> ...
>
> > + "0",
> > + "90",
> > + "180",
> > + "270",
>
> Ditto.
>
> ...
>
> > +enum {
> > + LTC2688_CHAN_MODE_TOGGLE,
> > + LTC2688_CHAN_MODE_DITHER
>
> + Comma.
>
> > +};
>
> ...
>
> > +static const char * const
> ltc2688_clk_names[LTC2688_CHAN_TD_MAX] = {
> > + "TGP1", "TGP2", "TGP3"
>
> In this notation, + comma.
>
> > +};
>
> ...
>
> > + for_each_set_bit(bit, &mask, ARRAY_SIZE(ltc2688_clk_names))
> {
> > + struct clk *clk;
>
> + blank line
>
> > + /*
> > + * If a TGP pin is set, then we need to provide a valid clock at
> > + * the pin.
> > + */
>
> > + }
>
> ...
>
> > + fwnode_for_each_available_child_node(fwnode, child) {
> > + struct ltc2688_chan *chan;
> > +
> > + ret = fwnode_property_read_u32(child, "reg", ®);
> > + if (ret) {
> > + fwnode_handle_put(child);
>
> > + return dev_err_probe(&st->spi->dev, ret,
> > + "Failed to get reg property\n");
>
> One line.
>
> > + } else if (reg >= LTC2688_DAC_CHANNELS) {
>
> Redundant 'else'.
Do you mean?
if (ret)
...
if (reg >= LTC2688_DAC_CHANNELS)
....
> > + fwnode_handle_put(child);
> > + return dev_err_probe(&st->spi->dev, -EINVAL,
>
> > + "reg >= %d\n", LTC2688_DAC_CHANNELS);
>
> The message is cryptic. Decrypt it for the user.
>
> > + }
> > +
> > + chan = &st->channels[reg];
> > + ret = fwnode_property_read_u32(child, "adi,mode",
> &mode);
> > + if (!ret) {
> > + if (mode > LTC2688_CHAN_MODE_DITHER) {
> > + fwnode_handle_put(child);
> > + return dev_err_probe(&st->spi->dev, -EINVAL,
> > + "chan mode inv value(%d)\n",
> > + mode);
> > + }
> > +
> > + chan->dither_toggle = true;
> > + if (mode == LTC2688_CHAN_MODE_TOGGLE) {
> > + ltc2688_channels[reg].ext_info =
> ltc2688_toggle_ext_info;
> > + } else {
> > + ltc2688_channels[reg].ext_info =
> ltc2688_dither_ext_info;
> > + /* enable dither mode */
> > + mask = LTC2688_CH_MODE_MSK;
> > + val = BIT(11);
> > + }
> > + }
> > +
> > + ret = fwnode_property_read_u32(child, "adi,toggle-dither-
> input",
> > + &clk_input);
>
> > + if (!ret) {
>
> if (ret) {
> if ...
> } else {
> ...
> }
sure
> > + if (clk_input > LTC2688_CHAN_TD_TGP3) {
> > + fwnode_handle_put(child);
> > + return dev_err_probe(&st->spi->dev, -EINVAL,
> > + "toggle-dither-input inv value(%d)\n",
> > + clk_input);
> > + }
> > +
> > + clk_msk |= BIT(clk_input);
> > + mask |= LTC2688_CH_TD_SEL_MSK;
> > + /*
> > + * 0 means software toggle which we are not supporting
> > + * for now. Hence the +1
>
> In all multi-line comments miseed (grammatical) period.
>
> > + */
> > + val |= FIELD_PREP(LTC2688_CH_TD_SEL_MSK, clk_input
> + 1);
> > + } else if (chan->dither_toggle) {
> > + /*
> > + * As sw_toggle is not supported, we need to make sure
> > + * a valid input is selected if toggle/dither mode is
> > + * requested
> > + */
> > + return dev_err_probe(&st->spi->dev, -EINVAL,
> > + "toggle-dither set but no toggle-dither-
> input\n");
> > + }
>
> > + chan->overrange = fwnode_property_read_bool(child,
> > + "adi,overrange");
>
> One line?
It will pass the 80 col limit. AFAIR, Jonathan prefers to keep it when it
does not hurt readability...
> > + if (chan->overrange) {
> > + val |= LTC2688_CH_OVERRANGE_MSK;
> > + mask |= BIT(3);
> > + }
> > +
> > + if (!mask)
> > + continue;
> > +
> > + ret = regmap_update_bits(st->regmap,
> > + LTC2688_CMD_CH_SETTING(reg), mask,
> > + val);
> > + if (ret) {
> > + fwnode_handle_put(child);
> > + return dev_err_probe(&st->spi->dev, -EINVAL,
> > + "failed to set chan settings\n");
> > + }
> > +
> > + mask = 0;
> > + val = 0;
> > + }
>
> ...
>
> > + int fs = ltc2688_span_helper[idx][1] -
> ltc2688_span_helper[idx][0];
> > +
> > + if (chan->overrange)
> > + fs = mult_frac(fs, 105, 100);
> > +
> > + return fs;
>
> if (...)
> return ...
> ?
Yes...
> ...
>
> > + u32 c, i;
>
> In one case you use int or unsigned int if I'm not mistaken, here out
> of a sudden u32. Why? Please, be consistent over the code.
I will check that...
> ...
>
> > + /* we will return IIO_VAL_INT_PLUS_NANO */
> > + s = DIV_ROUND_CLOSEST_ULL(st->vref * fs *
> 1000000000ULL,
> > + 4096 * 1 << 16);
>
> Can you make use of one of the SI prefixes here? (See unit.h)
Sure
> ...
>
> > + gpio = devm_gpiod_get_optional(&st->spi->dev, "reset",
> GPIOD_OUT_HIGH);
> > + if (IS_ERR(gpio))
> > + return dev_err_probe(&st->spi->dev, PTR_ERR(gpio),
> > + "Failed to get reset gpio");
>
> > +
>
> Redundant blank line.
>
> > + if (gpio) {
> > + usleep_range(1000, 1200);
> > + /* bring device out of reset */
> > + gpiod_set_value_cansleep(gpio, 0);
>
> > + } else {
>
> I'm wondering why 'else'? Can't it be both? Why not?
Well, if we have the reset pin, then we reset using it. If there's
no pin, we use the SW reset. It's a common pattern that we already use
for example in the ADIS devices.
> > + ret = regmap_update_bits(st->regmap,
> LTC2688_CMD_CONFIG_REG,
> > + LTC2688_CONFIG_RST,
> > + LTC2688_CONFIG_RST);
> > + if (ret < 0)
> > + return ret;
> > + }
> > +
> > + usleep_range(10000, 12000);
>
> + blank line, so the reader won't be confused about the function of
> this sleep.
>
> > + ret = ltc2688_channel_config(st);
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (vref) {
> > + ret = regmap_update_bits(st->regmap,
> LTC2688_CMD_CONFIG_REG,
> > + LTC2688_CONFIG_EXT_REF, BIT(1));
> > + if (ret < 0)
>
> These ' < 0' parts are inconsistent over the code.
Yes, I will fix that...
> > + return ret;
> > + }
> > +
> > + ltc2688_span_avail_compute(st);
> > + return 0;
> > +}
>
> ...
>
> > + /* ignoring the no op command */
> > + .max_register = U16_MAX - 1
>
> Really?! Have you ever considered what burden you bring with this to
> one who accidentally or on purpose tries to dump registers via
> debugfs?
Ups, this is completely wrong!! It should be U8_MAX...
> ...
>
> > + st->regmap = devm_regmap_init(&spi->dev, NULL, st,
> <c2688_regmap_config);
>
> I'm wondering why it's not a regmap SPI?
The problem is on the read side... In the first transfer we write the command/register
to read, then we need to release the CS pin so that the device executes the command,
and only then we read the data. AFAIK, the regmap spi implementation won't work like
this. I think CS is kept asserted the whole time...
> > + if (IS_ERR(st->regmap))
> > + return dev_err_probe(&spi->dev, PTR_ERR(st->regmap),
> > + "Failed to init regmap");
>
> ...
>
> > + ret = ltc2688_regulator_supply_get(st, "vcc");
> > + if (ret)
> > + return ret;
> > +
> > + ret = ltc2688_regulator_supply_get(st, "iovcc");
> > + if (ret)
> > + return ret;
>
> Can bulk regulators be used?
Hmm, I need to check but I think so... There's no ordering requirement here
> ...
>
> > + st->vref = ret / 1000;
>
> Make use of unit.h
>
- Nuno Sá
