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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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á





[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux