Re: [PATCH 1/3] iio: dac: add support for ltc2688

Lars-Peter Clausen
2021-12-15T16:01:54Z

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".
>
>>> + 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 :)

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
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().

>> There is a new API regmap_set_bits()/regmap_clear_bits() that allows
>> you
>> to write this in a more compact way. There are a few other places in
>> the
>> driver where they can be used as well.
> Hmm, will look at the new API...
>
> - Nuno Sá