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á
