From: Lars-Peter Clausen <lars@metafoo.de>
Sent: Wednesday, December 15, 2021 4:58 PM
To: Sa, Nuno <Nuno.Sa@analog.com>; linux-iio@vger.kernel.org;
devicetree@vger.kernel.org
Cc: Jonathan Cameron <jic23@kernel.org>; Rob Herring
[off-list ref]; Hennerich, Michael
[off-list ref]
Subject: Re: [PATCH 1/3] iio: dac: add support for ltc2688

[External]

On 12/15/21 2:40 PM, Sa, Nuno wrote:

quoted

quoted

quoted

+		}
+[...]
+	return ltc2688_tgp_setup(st, clk_msk, tgp);
+}
+
+static int ltc2688_setup(struct ltc2688_state *st, struct regulator

*vref)

quoted

+{
+	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...

quoted

quoted

quoted

+	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

quoted

quoted

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

quoted

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

quoted

quoted

quoted

+	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,

quoted

+					 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,

quoted

+				  LTC2688_CONFIG_EXT_REF, BIT(1));

This is a bit confusing since you are using

LTC2688_CONFIG_EXT_REF

quoted

quoted

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

quoted

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,

quoted

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á