On Thu, Nov 11, 2021 at 4:30 PM Sa, Nuno <Nuno.Sa@xxxxxxxxxx> wrote:
> > From: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> > Sent: Thursday, November 11, 2021 2:49 PM
> > On Thu, Nov 11, 2021 at 1:01 PM Nuno Sá <nuno.sa@xxxxxxxxxx>
> > wrote:
...
> > > +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.
I understand that for enum, but here it doesn't make any sense to me
since you are addressing them by indices.
> 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...
> > > +};
...
> > > + 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...
while () {
if ()
...
break;
} else {
...
}
}
Isn't it obvious?
Same with
if ()
return
else
...
> > > + }
...
> > > + 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)
> ....
Yes.
> > > + }
...
> > > + 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...
I believe it will increase readability being located on one 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.
Perhaps a comment in the code is needed.
> > > + ret = regmap_update_bits(st->regmap,
> > LTC2688_CMD_CONFIG_REG,
> > > + LTC2688_CONFIG_RST,
> > > + LTC2688_CONFIG_RST);
> > > + if (ret < 0)
> > > + return ret;
> > > + }
...
> > > + /* 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...
Ah, that explains!
...
> > > + 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...
I believe it's configurable, no? Like the cs_change flag somewhere.
Can you double check?
--
With Best Regards,
Andy Shevchenko
