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?
> + */
> + 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?
> +};
> +
> +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?
If not, do {} while (--i); looks more natural.
> + 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'.
> + /*
> + * 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.
> + }
> + 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;
...
> + ret = regmap_read(st->regmap, reg, ®val);
> + if (ret < 0)
Is it the first time you tested explicitly for a negative result?
> + return ret;
...
> + return sprintf(buf, "%u\n", !!(regval & BIT(chan->channel)));
sysfs_emit() ?
> +}
...
> + struct ltc2688_state *st = iio_priv(indio_dev);
> + bool en;
> + int ret;
> + u32 val = 0, reg;
Reversed xmas tree order ?
> + ret = strtobool(buf, &en);
kstrtobool()
...
> + if (ret)
> + return ret;
> + break;
What does this mean?
...
> + "4",
> + "8",
> + "16",
> + "32",
> + "64",
One line?
...
> + "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'.
> + 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 {
...
}
> + 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?
> + 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 ...
?
...
> + 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.
...
> + /* 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)
...
> + 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?
> + 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.
> + 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?
...
> + st->regmap = devm_regmap_init(&spi->dev, NULL, st, <c2688_regmap_config);
I'm wondering why it's not a regmap SPI?
> + 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?
...
> + st->vref = ret / 1000;
Make use of unit.h
--
With Best Regards,
Andy Shevchenko
