> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> Sent: Thursday, November 11, 2021 3:42 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 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.
Yeah, right... If we keep this, I will do as you suggested
> > 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?
Ah ok, I see know what you mean (and yes, it is obvious; for my shame)...
if ()
break
if ()
return -EPERM
As we do the break, the else becomes irrelevant
> Same with
>
> if ()
> return
> else
> ...
yeah yeah...
> > > > + }
>
> ...
>
> > > > + 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.
I mean, this is perfectly aligned with the open "(", so it's a pretty
normal pattern. Anyways, I'm more than happy to move this into a one
liner and just use the 100 limit. But let's see what Jonathan has to say
because I do not want to move back and forward...
> ...
>
> > > > + 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.
can do that...
> > > > + 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?
Don't think we can... The read part just calls:
https://elixir.bootlin.com/linux/latest/source/drivers/base/regmap/regmap-spi.c#L98
and has no control over the spi transfer bits...
- Nuno Sá
