> From: Lars-Peter Clausen <lars@xxxxxxxxxx>
> Sent: Wednesday, December 15, 2021 4:58 PM
> To: Sa, Nuno <Nuno.Sa@xxxxxxxxxx>; linux-iio@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx
> Cc: Jonathan Cameron <jic23@xxxxxxxxxx>; Rob Herring
> <robh+dt@xxxxxxxxxx>; Hennerich, Michael
> <Michael.Hennerich@xxxxxxxxxx>
> Subject: Re: [PATCH 1/3] iio: dac: add support for ltc2688
>
> [External]
>
> On 12/15/21 2:40 PM, Sa, Nuno wrote:
> >
> >>> + }
> >>> +[...]
> >>> + return ltc2688_tgp_setup(st, clk_msk, tgp);
> >>> +}
> >>> +
> >>> +static int ltc2688_setup(struct ltc2688_state *st, struct regulator
> >> *vref)
> >>> +{
> >>> + struct gpio_desc *gpio;
> >>> + int ret;
> >>> +
> >>> + /*
> >>> + * If we have a reset pin, use that to reset the board, If not, use
> >>> + * the reset bit.
> >>> + */
> >> Looking at the datasheet I do not see a reset pin on the chip.
> > IIRC, it's called CLR... But looking at it again if feels like a reset pin but
> > without directly saying so in the datasheet.
> ok, but then the gpio should be called "clr" and not "reset".
ok...
> >
> >>> + gpio = devm_gpiod_get_optional(&st->spi->dev, "reset",
> >> GPIOD_OUT_HIGH);
> >> Usually when we have a reset which is active low we define it in the
> DT
> >> as active low rather than doing the inversion in the driver.
> > And that's how I tested it in dts. The ' GPIOD_OUT_HIGH' is to
> request
> > it in the asserted state and then we just have to de-assert it to take it
> > out of reset. It's actually the same pattern used in the adis lib. IIRC,
> > you were actually the one to suggest this :)
> I'm stupid... just read it the wrong way, code is correct the way it is
> >>> + if (IS_ERR(gpio))
> >>> + return dev_err_probe(&st->spi->dev, PTR_ERR(gpio),
> >>> + "Failed to get reset gpio");
> >>> + if (gpio) {
> >>> + usleep_range(1000, 1200);
> >>> + /* bring device out of reset */
> >>> + gpiod_set_value_cansleep(gpio, 0);
> >>> + } else {
> >>> + ret = regmap_update_bits(st->regmap,
> >> LTC2688_CMD_CONFIG,
> >>> + LTC2688_CONFIG_RST,
> >>> + LTC2688_CONFIG_RST);
> >>> + if (ret < 0)
> >>> + return ret;
> >>> + }
> >>> +
> >>> + usleep_range(10000, 12000);
> >>> +
> >>> + ret = ltc2688_channel_config(st);
> >>> + if (ret)
> >>> + return ret;
> >>> +
> >>> + if (!vref)
> >>> + return 0;
> >>> +
> >>> + return regmap_update_bits(st->regmap,
> >> LTC2688_CMD_CONFIG,
> >>> + LTC2688_CONFIG_EXT_REF, BIT(1));
> >> This is a bit confusing since you are using
> LTC2688_CONFIG_EXT_REF
> >> for
> >> the mask and BIT(1) for the value, even though both are the same.
> > I tried to be more or less consistent. So, for masks I used a define
> and
> > for the actually value I used the "raw" BIT, FIELD_PREP, FIELD_GET as
> > I think Jonathan prefers that way. If that's also the preferred way for
> masks,
> > I'm happy to update it.
>
> Just 5 lines above you use the define for both the mask and the value
> :)
:facepalm:! At least in my mind I tried to do it...
> I don't think it is a good idea to use raw BIT(x) in the code. They are
> just as magic of a value as writing 0x8. There is no way for a reviewer
Well BIT(3) is still better than 0x8 but I get your point.
> to quickly see whether that BIT(x) actually is the right value for the
> mask.
>
> If you wanted to go the FIELD_PREP route you could write this as
>
> ..., LTC2688_CONFIG_EXT_REF,
> FIELD_PREP(LTC2688_CONFIG_EXT_REF, 1)
>
> But my personal preference is just to pass the mask as the value when
> changing a single bit value. Makes it clear that it is a single bit
> field and you are setting it. Or just use regmap_set_bits().
I think 'regmap_set_bits()' is ideal for these cases and probably introduced
for things like this. And I suspect you prefer that I use
'LTC2688_CONFIG_EXT_REF' as the bit argument :)
- Nuno Sá
