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

["Sa, Nuno" <Nuno.Sa@analog.com>]

> 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
> <robh+dt@kernel.org>; Hennerich, Michael
> <Michael.Hennerich@analog.com>
> 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á