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

Commit Message

Nuno Sa Dec. 14, 2021, 4:56 p.m. UTC

The LTC2688 is a 16 channel, 16 bit, +-15V DAC with an integrated
precision reference. It is guaranteed monotonic and has built in
rail-to-rail output buffers that can source or sink up to 20 mA.

Signed-off-by: Nuno Sá <[e-mail]>
---
 MAINTAINERS               |    7 +
 drivers/iio/dac/Kconfig   |   11 +
 drivers/iio/dac/Makefile  |    1 +
 drivers/iio/dac/ltc2688.c | 1081 +++++++++++++++++++++++++++++++++++++
 4 files changed, 1100 insertions(+)
 create mode 100644 drivers/iio/dac/ltc2688.c

Comments

Lars-Peter Clausen Dec. 15, 2021, 10:23 a.m. UTC | #1

On 12/14/21 5:56 PM, Nuno Sá wrote:
> The LTC2688 is a 16 channel, 16 bit, +-15V DAC with an integrated
> precision reference. It is guaranteed monotonic and has built in
> rail-to-rail output buffers that can source or sink up to 20 mA.

Looks very good!

Although I'm not sure what to make of the `raw1` API. Maybe it makes 
sense to submit an initial version of this driver without the toggle 
API. And then have a follow up discussion how to define the API for 
this. This will not be the only DAC that has this feature so it would be 
a good idea to come up with a common API.


>
> [...]
> +
> +static int ltc2688_spi_read(void *context, const void *reg, size_t reg_size,
> +			    void *val, size_t val_size)
> +{
> +	struct ltc2688_state *st = context;
> +	struct spi_transfer xfers[] = {
> +		{
> +			.tx_buf = reg,
> +			.bits_per_word = 8,
> +			/*
> +			 * This means that @val will also be part of the
> +			 * transfer as there's no pad bits. That's fine as these
> +			 * bits are don't care for the device and we fill
> +			 * @val with the proper value afterwards. Using regmap
> +			 * pad bits to get reg_size right would just make the
> +			 * write part more cumbersome than this...
> +			 */
This is making assumptions about the memory layout in the regmap core. 
This could change in the future and then this driver breaks. It is 
better to not assume that reg is part of a larger buffer.
> +			.len = reg_size + 2,
> +			.cs_change = 1,
> +		}, {
> +			.tx_buf = st->tx_data,
> +			.rx_buf = st->rx_data,
> +			.bits_per_word = 8,
> +			.len = 3,
> +		},
> +	};
> +	int ret;
> +
> +	ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> +	if (ret)
> +		return ret;
> +
> +	memcpy(val, &st->rx_data[1], val_size);
> +
> +	return 0;
> +}
> [...]
> +
> +static int ltc2688_write_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan, int val,
> +			     int val2, long mask)
Using mask for the variable name is a relic from the days when it used 
to be a mask. For new drivers it is better to use `info`. Same for the 
other functions.
> [...]
> +
> +static const char * const ltc2688_dither_phase[] = {
> +	"0", "90", "180", "270",
> +};
Usually we use radians for phase values. Although that would make for a 
bit of an awkward API in this case.
> +
> [...]
> +/*
> + * For toggle mode we only expose the symbol attr (sw_toggle) in case a TGPx is
> + * not provided in dts.
> + */
> +#define LTC2688_CHAN_TOGGLE(t, name) ({							\
> +	static const struct iio_chan_spec_ext_info t ## _ext_info[] = {			\
> +		LTC2688_CHAN_EXT_INFO("raw1", LTC2688_INPUT_B, IIO_SEPARATE),		\
> +		LTC2688_CHAN_EXT_INFO("toggle_en", LTC2688_DITHER_TOGGLE_ENABLE,	\
> +				      IIO_SEPARATE),					\
> +		LTC2688_CHAN_EXT_INFO("powerdown", LTC2688_POWERDOWN, IIO_SEPARATE),	\
> +		LTC2688_CHAN_EXT_INFO(name, LTC2688_SW_TOGGLE, IIO_SEPARATE),		\
> +		{}									\
> +	};										\
> +	t ## _ext_info;									\
> +})

This macro is a bit strange since it declares a static, but is called in 
a function. It might be better to declare the two types of ext_infos 
statically and then reference them by name from within the function.

> [...]
> +
> +static int ltc2688_tgp_setup(struct ltc2688_state *st, long clk_mask,
> +			     const struct ltc2688_dither_helper *tgp)
> +{
> +	int ret, bit;
> +
> +	for_each_set_bit(bit, &clk_mask, LTC2688_CHAN_TD_MAX) {
clk_mask should be unsigned long
> [...]
> +
> +static int ltc2688_span_lookup(const struct ltc2688_state *st, int min, int max)
> +{
> +	u32 span;
Nit: Why u32 and not unsigned int? The size doesn't seem to be important 
for the loop variable.
> +
> +	for (span = 0; span < ARRAY_SIZE(ltc2688_span_helper); span++) {
> +		if (min == ltc2688_span_helper[span][0] &&
> +		    max == ltc2688_span_helper[span][1])
> +			return span;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int ltc2688_channel_config(struct ltc2688_state *st)
> +{
> +	struct fwnode_handle *fwnode = dev_fwnode(&st->spi->dev), *child;
> +	struct ltc2688_dither_helper tgp[LTC2688_CHAN_TD_MAX] = {0};
> +	u32 reg, clk_input, val, mask, tmp[2];
> +	unsigned long clk_msk = 0;
> +	int ret, span;
> +
> +	fwnode_for_each_available_child_node(fwnode, child) {
> [...]
> +		chan = &st->channels[reg];
> +		if (fwnode_property_read_bool(child, "adi,toggle-mode")) {
> +			chan->toggle_chan = true;
> +			/* assume sw toggle ABI */
> +			ltc2688_channels[reg].ext_info = LTC2688_CHAN_TOGGLE(__s, "symbol");
Updating ltc2688_channels at runtime will break if you have multiple 
instances of the device with a different configuration. You need to 
kmemdup() the channel array.
> +		}
> +[...]
> +	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.
> +	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.
> +	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.

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.

> +}
> [...]
> +

Nuno Sa Dec. 15, 2021, 1:40 p.m. UTC | #2

Hi Lars,

Thanks for the review!

> From: Lars-Peter Clausen <[e-mail]>
> Sent: Wednesday, December 15, 2021 11:24 AM
> To: Sa, Nuno <[e-mail]>; [e-mail];
> [e-mail]
> Cc: Jonathan Cameron <[e-mail]>; Rob Herring
> <[e-mail]>; Hennerich, Michael
> <[e-mail]>
> Subject: Re: [PATCH 1/3] iio: dac: add support for ltc2688
> > 
> On 12/14/21 5:56 PM, Nuno Sá wrote:
> > The LTC2688 is a 16 channel, 16 bit, +-15V DAC with an integrated
> > precision reference. It is guaranteed monotonic and has built in
> > rail-to-rail output buffers that can source or sink up to 20 mA.
> 
> Looks very good!
> 
> Although I'm not sure what to make of the `raw1` API. Maybe it makes
> sense to submit an initial version of this driver without the toggle
> API. And then have a follow up discussion how to define the API for
> this. This will not be the only DAC that has this feature so it would be
> a good idea to come up with a common API.

This was discussed in the RFC. The raw1 refers to the second code
on the DAC output path so we can toggle between raw and raw1. The
feeling I got from HW guys internally is that this is an important feature
to support. That said, I understand that this being ABI is always sensitive
stuff but ultimately, I honestly think that raw1 fits this feature.

The thing that is making more reluctant about toggle mode is not so
much the ABI but forcing a clock to be used when a toggle channel is
mapped to a TGPx pin...
 
> 
> >
> > [...]
> > +
> > +static int ltc2688_spi_read(void *context, const void *reg, size_t
> reg_size,
> > +			    void *val, size_t val_size)
> > +{
> > +	struct ltc2688_state *st = context;
> > +	struct spi_transfer xfers[] = {
> > +		{
> > +			.tx_buf = reg,
> > +			.bits_per_word = 8,
> > +			/*
> > +			 * This means that @val will also be part of the
> > +			 * transfer as there's no pad bits. That's fine as
> these
> > +			 * bits are don't care for the device and we fill
> > +			 * @val with the proper value afterwards. Using
> regmap
> > +			 * pad bits to get reg_size right would just make
> the
> > +			 * write part more cumbersome than this...
> > +			 */
> This is making assumptions about the memory layout in the regmap
> core.
> This could change in the future and then this driver breaks. It is
> better to not assume that reg is part of a larger buffer.

True, but I think reg_size should not really change as we define it in
our regmap_config. Are you suggesting to just use plain 3 here?

> > +			.len = reg_size + 2,
> > +			.cs_change = 1,
> > +		}, {
> > +			.tx_buf = st->tx_data,
> > +			.rx_buf = st->rx_data,
> > +			.bits_per_word = 8,
> > +			.len = 3,
> > +		},
> > +	};
> > +	int ret;
> > +
> > +	ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> > +	if (ret)
> > +		return ret;
> > +
> > +	memcpy(val, &st->rx_data[1], val_size);
> > +
> > +	return 0;
> > +}
> > [...]
> > +
> > +static int ltc2688_write_raw(struct iio_dev *indio_dev,
> > +			     struct iio_chan_spec const *chan, int val,
> > +			     int val2, long mask)
> Using mask for the variable name is a relic from the days when it used
> to be a mask. For new drivers it is better to use `info`. Same for the
> other functions.

Got it. Probably copy paste...

> > [...]
> > +
> > +static const char * const ltc2688_dither_phase[] = {
> > +	"0", "90", "180", "270",
> > +};
> Usually we use radians for phase values. Although that would make for
> a
> bit of an awkward API in this case.
> > +
> > [...]
> > +/*
> > + * For toggle mode we only expose the symbol attr (sw_toggle) in
> case a TGPx is
> > + * not provided in dts.
> > + */
> > +#define LTC2688_CHAN_TOGGLE(t, name) ({
> 				\
> > +	static const struct iio_chan_spec_ext_info t ## _ext_info[] = {
> 			\
> > +		LTC2688_CHAN_EXT_INFO("raw1", LTC2688_INPUT_B,
> IIO_SEPARATE),		\
> > +		LTC2688_CHAN_EXT_INFO("toggle_en",
> LTC2688_DITHER_TOGGLE_ENABLE,	\
> > +				      IIO_SEPARATE),
> 		\
> > +		LTC2688_CHAN_EXT_INFO("powerdown",
> LTC2688_POWERDOWN, IIO_SEPARATE),	\
> > +		LTC2688_CHAN_EXT_INFO(name,
> LTC2688_SW_TOGGLE, IIO_SEPARATE),		\
> > +		{}
> 		\
> > +	};
> 		\
> > +	t ## _ext_info;
> 		\
> > +})
> 
> This macro is a bit strange since it declares a static, but is called in
> a function. It might be better to declare the two types of ext_infos
> statically and then reference them by name from within the function.

Yeah, I also though about that. In the end, the result is the same. I ended
up with this just to save some lines but I'm totally fine with your approach.
It actually makes the code easier to read.

> > [...]
> > +
> > +static int ltc2688_tgp_setup(struct ltc2688_state *st, long clk_mask,
> > +			     const struct ltc2688_dither_helper *tgp)
> > +{
> > +	int ret, bit;
> > +
> > +	for_each_set_bit(bit, &clk_mask, LTC2688_CHAN_TD_MAX) {
> clk_mask should be unsigned long
> > [...]
> > +
> > +static int ltc2688_span_lookup(const struct ltc2688_state *st, int
> min, int max)
> > +{
> > +	u32 span;
> Nit: Why u32 and not unsigned int? The size doesn't seem to be
> important
> for the loop variable.

Shorter to type :)

> > +
> > +	for (span = 0; span < ARRAY_SIZE(ltc2688_span_helper);
> span++) {
> > +		if (min == ltc2688_span_helper[span][0] &&
> > +		    max == ltc2688_span_helper[span][1])
> > +			return span;
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +
> > +static int ltc2688_channel_config(struct ltc2688_state *st)
> > +{
> > +	struct fwnode_handle *fwnode = dev_fwnode(&st->spi-
> >dev), *child;
> > +	struct ltc2688_dither_helper tgp[LTC2688_CHAN_TD_MAX] =
> {0};
> > +	u32 reg, clk_input, val, mask, tmp[2];
> > +	unsigned long clk_msk = 0;
> > +	int ret, span;
> > +
> > +	fwnode_for_each_available_child_node(fwnode, child) {
> > [...]
> > +		chan = &st->channels[reg];
> > +		if (fwnode_property_read_bool(child, "adi,toggle-
> mode")) {
> > +			chan->toggle_chan = true;
> > +			/* assume sw toggle ABI */
> > +			ltc2688_channels[reg].ext_info =
> LTC2688_CHAN_TOGGLE(__s, "symbol");
> Updating ltc2688_channels at runtime will break if you have multiple
> instances of the device with a different configuration. You need to
> kmemdup() the channel array.

Arghh, yeah (dummy mistake)! You're right! Will fix it in v2...

> > +		}
> > +[...]
> > +	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.

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

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

> 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á

Nuno Sa Dec. 15, 2021, 1:55 p.m. UTC | #3

> -----Original Message-----
> From: Sa, Nuno <[e-mail]>
> Sent: Wednesday, December 15, 2021 2:41 PM
> To: Lars-Peter Clausen <[e-mail]>; [e-mail];
> [e-mail]
> Cc: Jonathan Cameron <[e-mail]>; Rob Herring
> <[e-mail]>; Hennerich, Michael
> <[e-mail]>
> Subject: RE: [PATCH 1/3] iio: dac: add support for ltc2688
> 
> [External]
> 
> Hi Lars,
> 
> Thanks for the review!
> 
> > From: Lars-Peter Clausen <[e-mail]>
> > Sent: Wednesday, December 15, 2021 11:24 AM
> > To: Sa, Nuno <[e-mail]>; [e-mail];
> > [e-mail]
> > Cc: Jonathan Cameron <[e-mail]>; Rob Herring
> > <[e-mail]>; Hennerich, Michael
> > <[e-mail]>
> > Subject: Re: [PATCH 1/3] iio: dac: add support for ltc2688
> > >
> > On 12/14/21 5:56 PM, Nuno Sá wrote:
> > > The LTC2688 is a 16 channel, 16 bit, +-15V DAC with an integrated
> > > precision reference. It is guaranteed monotonic and has built in
> > > rail-to-rail output buffers that can source or sink up to 20 mA.
> >
> > Looks very good!
> >
> > Although I'm not sure what to make of the `raw1` API. Maybe it
> makes
> > sense to submit an initial version of this driver without the toggle
> > API. And then have a follow up discussion how to define the API for
> > this. This will not be the only DAC that has this feature so it would be
> > a good idea to come up with a common API.
> 
> This was discussed in the RFC. The raw1 refers to the second code
> on the DAC output path so we can toggle between raw and raw1. The
> feeling I got from HW guys internally is that this is an important feature
> to support. That said, I understand that this being ABI is always
> sensitive
> stuff but ultimately, I honestly think that raw1 fits this feature.
> 
> The thing that is making more reluctant about toggle mode is not so
> much the ABI but forcing a clock to be used when a toggle channel is
> mapped to a TGPx pin...
> 
> >
> > >
> > > [...]
> > > +
> > > +static int ltc2688_spi_read(void *context, const void *reg, size_t
> > reg_size,
> > > +			    void *val, size_t val_size)
> > > +{
> > > +	struct ltc2688_state *st = context;
> > > +	struct spi_transfer xfers[] = {
> > > +		{
> > > +			.tx_buf = reg,
> > > +			.bits_per_word = 8,
> > > +			/*
> > > +			 * This means that @val will also be part of the
> > > +			 * transfer as there's no pad bits. That's fine as
> > these
> > > +			 * bits are don't care for the device and we fill
> > > +			 * @val with the proper value afterwards. Using
> > regmap
> > > +			 * pad bits to get reg_size right would just make
> > the
> > > +			 * write part more cumbersome than this...
> > > +			 */
> > This is making assumptions about the memory layout in the regmap
> > core.
> > This could change in the future and then this driver breaks. It is
> > better to not assume that reg is part of a larger buffer.
> 
> True, but I think reg_size should not really change as we define it in
> our regmap_config. Are you suggesting to just use plain 3 here?

Ahhh I get what you mean. So you are saying to just use a bounce buffer
(that actually holds space for 3 bytes) in our state struct and just copy reg
onto it? That makes sense...

- Nuno Sá

Lars-Peter Clausen Dec. 15, 2021, 3:58 p.m. UTC | #4

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á

Nuno Sa Dec. 15, 2021, 5:08 p.m. UTC | #5

> From: Lars-Peter Clausen <[e-mail]>
> Sent: Wednesday, December 15, 2021 4:58 PM
> To: Sa, Nuno <[e-mail]>; [e-mail];
> [e-mail]
> Cc: Jonathan Cameron <[e-mail]>; Rob Herring
> <[e-mail]>; Hennerich, Michael
> <[e-mail]>
> 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á

Jonathan Cameron Dec. 16, 2021, 2:11 p.m. UTC | #6

On Tue, 14 Dec 2021 17:56:06 +0100
Nuno Sá <[e-mail]> wrote:

> The LTC2688 is a 16 channel, 16 bit, +-15V DAC with an integrated
> precision reference. It is guaranteed monotonic and has built in
> rail-to-rail output buffers that can source or sink up to 20 mA.
> 
> Signed-off-by: Nuno Sá <[e-mail]>

I'm not that keen on toggle having to be clock driven, but I guess we can
always change that later when usecases come along.

Some comments inline.

Jonathan

> ---

> +
> +static int ltc2688_spi_read(void *context, const void *reg, size_t reg_size,
> +			    void *val, size_t val_size)
> +{
> +	struct ltc2688_state *st = context;
> +	struct spi_transfer xfers[] = {
> +		{
> +			.tx_buf = reg,
> +			.bits_per_word = 8,
> +			/*
> +			 * This means that @val will also be part of the
> +			 * transfer as there's no pad bits. That's fine as these
> +			 * bits are don't care for the device and we fill
> +			 * @val with the proper value afterwards. Using regmap
> +			 * pad bits to get reg_size right would just make the
> +			 * write part more cumbersome than this...
> +			 */
> +			.len = reg_size + 2,

what Lars said :)

> +			.cs_change = 1,
> +		}, {
> +			.tx_buf = st->tx_data,
> +			.rx_buf = st->rx_data,
> +			.bits_per_word = 8,
> +			.len = 3,
> +		},
> +	};
> +	int ret;
> +
> +	ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> +	if (ret)
> +		return ret;
> +
> +	memcpy(val, &st->rx_data[1], val_size);
> +
> +	return 0;
> +}
> +
> +static int ltc2688_spi_write(void *context, const void *data, size_t count)
> +{
> +	struct ltc2688_state *st = context;
> +
> +	return spi_write(st->spi, data, count);
> +}
> +



> +};
> +
> +static int ltc2688_dac_code_write(struct ltc2688_state *st, u32 chan, u32 input,
> +				  u16 code)
> +{
> +	struct ltc2688_chan *c = &st->channels[chan];
> +	int ret, reg;
> +
> +	/* 2 LSBs set to 0 if writing dither amplitude */
> +	if (!c->toggle_chan && input == LT2688_INPUT_B) {
> +		if (code > GENMASK(13, 0))
> +			return -EINVAL;
> +
> +		code <<= 2;

FIELD_PREP preferred.

> +	}
> +
> +	mutex_lock(&st->lock);
> +	/* select the correct input register to read from */
> +	ret = regmap_update_bits(st->regmap, LTC2688_CMD_A_B_SELECT, BIT(chan),
> +				 input << chan);
> +	if (ret)
> +		goto unlock;
> +
> +	/*
> +	 * If in dither/toggle mode the dac should be updated by an
> +	 * external signal (or sw toggle) and not here.
> +	 */
> +	if (c->mode == LTC2688_MODE_DEFAULT)
> +		reg = LTC2688_CMD_CH_CODE_UPDATE(chan);
> +	else
> +		reg = LTC2688_CMD_CH_CODE(chan);
> +
> +	ret = regmap_write(st->regmap, reg, code);
> +unlock:
> +	mutex_unlock(&st->lock);
> +	return ret;
> +}

> +
> +static int ltc2688_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan, int *val,
> +			    int *val2, long m)
> +{
> +	struct ltc2688_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (m) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = ltc2688_dac_code_read(st, chan->channel, LT2688_INPUT_A,
> +					    val);
> +		if (ret)
> +			return ret;
> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_OFFSET:
> +		return ltc2688_offset_get(st, chan->channel, val);
> +	case IIO_CHAN_INFO_SCALE:
> +		*val2 = 16;
> +		return ltc2688_scale_get(st, chan->channel, val);

I'm not against functions returning the IIO_VAL_* like this, but if they
are I expect the function to set val2 as well.

I'd suggest return 0 on success and then do similar to what you have done for code_read above.

> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		ret = regmap_read(st->regmap,
> +				  LTC2688_CMD_CH_OFFSET(chan->channel), val);
> +		if (ret)
> +			return ret;
> +
> +		/* Just 13 bits used. 2LSB ignored */
> +		*val >>= 2;
FIELD_GET() would get rid of need for the comment.

> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_CALIBSCALE:
> +		ret = regmap_read(st->regmap,
> +				  LTC2688_CMD_CH_GAIN(chan->channel), val);
> +		if (ret)
> +			return ret;
> +
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}

> +
> +static ssize_t ltc2688_read_ext(struct iio_dev *indio_dev,
> +				uintptr_t private,
> +				const struct iio_chan_spec *chan,
> +				char *buf)
> +{
> +	struct ltc2688_state *st = iio_priv(indio_dev);
> +	u32 val;
> +	int ret;
> +
> +	switch (private) {
> +	case LTC2688_DITHER_TOGGLE_ENABLE:

As below. I'd have separate functions rather than multiplexing this
to little benefit.

> +		return ltc2688_reg_bool_get(st, chan->channel,
> +					    LTC2688_CMD_TOGGLE_DITHER_EN, buf);
> +	case LTC2688_POWERDOWN:
> +		return ltc2688_reg_bool_get(st, chan->channel,
> +					    LTC2688_CMD_POWERDOWN, buf);
> +	case LTC2688_INPUT_B:
> +		ret = ltc2688_dac_code_read(st, chan->channel, LT2688_INPUT_B,
> +					    &val);
> +		if (ret)
> +			return ret;
> +
> +		return sysfs_emit(buf, "%u\n", val);
> +	case LTC2688_INPUT_B_AVAIL:
> +		/* dither amplitude has 1/4 of the span */
> +		return sysfs_emit(buf, "[%u %u %u]\n", ltc2688_raw_range[0],
> +				  ltc2688_raw_range[1],
> +				  ltc2688_raw_range[2] / 4);
> +	case LTC2688_SW_TOGGLE:
> +		return ltc2688_reg_bool_get(st, chan->channel,
> +					    LTC2688_CMD_SW_TOGGLE, buf);
> +	case LTC2688_DITHER_FREQ:
> +		return ltc2688_dither_freq_get(st, chan->channel, buf);
> +	case LTC2688_DITHER_FREQ_AVAIL:
> +		return ltc2688_dither_freq_avail(st, chan->channel, buf);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static ssize_t ltc2688_write_ext(struct iio_dev *indio_dev,
> +				 uintptr_t private,
> +				 const struct iio_chan_spec *chan,
> +				 const char *buf, size_t len)
> +{
> +	struct ltc2688_state *st = iio_priv(indio_dev);
> +	u16 val;
> +	int ret;
> +	bool en;
> +
> +	switch (private) {
> +	case LTC2688_DITHER_TOGGLE_ENABLE:

As commented on below, I'd just have a function for each case block you have
here.

> +		ret = kstrtobool(buf, &en);
> +		if (ret)
> +			return ret;
> +
> +		ret = ltc2688_dither_toggle_set(st, chan->channel, en);
> +		if (ret)
> +			return ret;
> +
> +		return len;
> +	case LTC2688_POWERDOWN:
> +		ret = ltc2688_reg_bool_set(st, chan->channel,
> +					   LTC2688_CMD_POWERDOWN, buf);
> +		if (ret)
> +			return ret;
> +
> +		return len;
> +	case LTC2688_INPUT_B:
> +		ret = kstrtou16(buf, 10, &val);
> +		if (ret)
> +			return ret;
> +
> +		ret = ltc2688_dac_code_write(st, chan->channel, LT2688_INPUT_B,
> +					     val);
> +		if (ret)
> +			return ret;
> +
> +		return len;
> +	case LTC2688_SW_TOGGLE:
> +		ret = ltc2688_reg_bool_set(st, chan->channel,
> +					   LTC2688_CMD_SW_TOGGLE, buf);
> +		if (ret)
> +			return ret;
> +
> +		return len;
> +	case LTC2688_DITHER_FREQ:
> +		ret = ltc2688_dither_freq_set(st, chan->channel, buf);
> +		if (ret)
> +			return ret;
> +
> +		return len;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ltc2688_get_dither_phase(struct iio_dev *dev,
> +				    const struct iio_chan_spec *chan)
> +{
> +	struct ltc2688_state *st = iio_priv(dev);
> +	int ret, regval;
> +
> +	ret = regmap_read(st->regmap, LTC2688_CMD_CH_SETTING(chan->channel),
> +			  &regval);
> +	if (ret)
> +		return ret;
> +
> +	return FIELD_GET(LTC2688_CH_DIT_PH_MSK, regval);
> +}
> +
> +static int ltc2688_set_dither_phase(struct iio_dev *dev,
> +				    const struct iio_chan_spec *chan,
> +				    unsigned int phase)
> +{
> +	struct ltc2688_state *st = iio_priv(dev);
> +
> +	return regmap_update_bits(st->regmap,
> +				  LTC2688_CMD_CH_SETTING(chan->channel),
> +				  LTC2688_CH_DIT_PH_MSK,
> +				  FIELD_PREP(LTC2688_CH_DIT_PH_MSK, phase));
> +}
> +
> +static int ltc2688_reg_access(struct iio_dev *indio_dev,
> +			      unsigned int reg,
> +			      unsigned int writeval,
> +			      unsigned int *readval)
> +{
> +	struct ltc2688_state *st = iio_priv(indio_dev);
> +
> +	if (readval)
> +		return regmap_read(st->regmap, reg, readval);
> +	else
> +		return regmap_write(st->regmap, reg, writeval);
> +}
> +
> +static const char * const ltc2688_dither_phase[] = {
> +	"0", "90", "180", "270",
> +};
> +
> +static const struct iio_enum ltc2688_dither_phase_enum = {
> +	.items = ltc2688_dither_phase,
> +	.num_items = ARRAY_SIZE(ltc2688_dither_phase),
> +	.set = ltc2688_set_dither_phase,
> +	.get = ltc2688_get_dither_phase,
> +};
> +
> +#define LTC2688_CHAN_EXT_INFO(_name, _what, _shared) {	\
> +	.name = _name,					\
> +	.read = ltc2688_read_ext,			\
> +	.write = ltc2688_write_ext,			\

I'm not really convinced big multiplexer functions are a good idea here.
They seem to save little code and hurt readability a bit.

> +	.private = (_what),				\
> +	.shared = (_shared),				\
> +}
> +
> +/*
> + * For toggle mode we only expose the symbol attr (sw_toggle) in case a TGPx is
> + * not provided in dts.
> + */
> +#define LTC2688_CHAN_TOGGLE(t, name) ({							\
> +	static const struct iio_chan_spec_ext_info t ## _ext_info[] = {			\
> +		LTC2688_CHAN_EXT_INFO("raw1", LTC2688_INPUT_B, IIO_SEPARATE),		\
> +		LTC2688_CHAN_EXT_INFO("toggle_en", LTC2688_DITHER_TOGGLE_ENABLE,	\
> +				      IIO_SEPARATE),					\
> +		LTC2688_CHAN_EXT_INFO("powerdown", LTC2688_POWERDOWN, IIO_SEPARATE),	\
> +		LTC2688_CHAN_EXT_INFO(name, LTC2688_SW_TOGGLE, IIO_SEPARATE),		\
> +		{}									\
> +	};										\
> +	t ## _ext_info;									\
> +})
> +
> +static struct iio_chan_spec_ext_info ltc2688_dither_ext_info[] = {
> +	LTC2688_CHAN_EXT_INFO("dither_raw", LTC2688_INPUT_B, IIO_SEPARATE),
> +	LTC2688_CHAN_EXT_INFO("dither_raw_available", LTC2688_INPUT_B_AVAIL,
> +			      IIO_SEPARATE),
> +	/*
> +	 * Not IIO_ENUM because the available freq needs to be computed at
> +	 * probe. We could still use it, but it didn't felt much right.
> +	 *

no extra blank line.

> +	 */
> +	LTC2688_CHAN_EXT_INFO("dither_frequency", LTC2688_DITHER_FREQ,
> +			      IIO_SEPARATE),
> +	LTC2688_CHAN_EXT_INFO("dither_frequency_available",
> +			      LTC2688_DITHER_FREQ_AVAIL, IIO_SEPARATE),
> +	IIO_ENUM("dither_phase", IIO_SEPARATE, &ltc2688_dither_phase_enum),
> +	IIO_ENUM_AVAILABLE("dither_phase", IIO_SEPARATE,
> +			   &ltc2688_dither_phase_enum),
> +	LTC2688_CHAN_EXT_INFO("dither_en", LTC2688_DITHER_TOGGLE_ENABLE,
> +			      IIO_SEPARATE),
> +	LTC2688_CHAN_EXT_INFO("powerdown", LTC2688_POWERDOWN, IIO_SEPARATE),
> +	{}
> +};
> +
> +static const struct iio_chan_spec_ext_info ltc2688_ext_info[] = {
> +	LTC2688_CHAN_EXT_INFO("powerdown", LTC2688_POWERDOWN, IIO_SEPARATE),
> +	{}
> +};
> +

> +
> +enum {
> +	LTC2688_CHAN_TD_TGP1,
> +	LTC2688_CHAN_TD_TGP2,
> +	LTC2688_CHAN_TD_TGP3,
> +	LTC2688_CHAN_TD_MAX
> +};

> +/* Helper struct to deal with dither channels binded to TGPx pins */
> +struct ltc2688_dither_helper {
> +	u8 chan[LTC2688_DAC_CHANNELS];
> +	u8 n_chans;
> +};
> +
bitmap perhaps given ordering doesn't matter (I think)



...

> +
> +static int ltc2688_channel_config(struct ltc2688_state *st)
> +{
> +	struct fwnode_handle *fwnode = dev_fwnode(&st->spi->dev), *child;
> +	struct ltc2688_dither_helper tgp[LTC2688_CHAN_TD_MAX] = {0};
> +	u32 reg, clk_input, val, mask, tmp[2];
> +	unsigned long clk_msk = 0;
> +	int ret, span;
> +

I think you need to sanity check you have a fwnode

> +	fwnode_for_each_available_child_node(fwnode, child) {

I guess this is because of the whole device_for_each_available_child_node() not
existing discussion that isn't resolved. 

> +		struct ltc2688_chan *chan;
> +
> +		ret = fwnode_property_read_u32(child, "reg", &reg);
> +		if (ret) {
> +			fwnode_handle_put(child);
> +			return dev_err_probe(&st->spi->dev, ret,
> +					     "Failed to get reg property\n");
> +		}
> +
> +		if (reg >= LTC2688_DAC_CHANNELS) {
> +			fwnode_handle_put(child);
> +			return dev_err_probe(&st->spi->dev, -EINVAL,
> +					     "reg bigger than: %d\n",
> +					     LTC2688_DAC_CHANNELS);
> +		}
> +
> +		chan = &st->channels[reg];
> +		if (fwnode_property_read_bool(child, "adi,toggle-mode")) {
> +			chan->toggle_chan = true;
> +			/* assume sw toggle ABI */
> +			ltc2688_channels[reg].ext_info = LTC2688_CHAN_TOGGLE(__s, "symbol");

That's a little nasty.  Pick it up from a static const array defined outside
this function.

> +		}
> +
> +		ret = fwnode_property_read_u32_array(child, "adi,output-range-millivolt",
> +						     tmp, ARRAY_SIZE(tmp));
> +		if (!ret) {
> +			span = ltc2688_span_lookup(st, tmp[0], tmp[1]);
> +			if (span < 0)
> +				return dev_err_probe(&st->spi->dev, -EINVAL,
> +						     "output range not valid:[%d %d]\n",
> +						     tmp[0], tmp[1]);
> +
> +			mask |= LTC2688_CH_SPAN_MSK;
> +			val |= FIELD_PREP(LTC2688_CH_SPAN_MSK, span);
> +		}
> +
> +		ret = fwnode_property_read_u32(child, "adi,toggle-dither-input",
> +					       &clk_input);
> +		if (!ret) {
> +			int cur_chan = tgp[clk_input].n_chans;
> +
> +			if (clk_input > LTC2688_CHAN_TD_TGP3) {
> +				fwnode_handle_put(child);
> +				return dev_err_probe(&st->spi->dev, -EINVAL,
> +						     "toggle-dither-input inv value(%d)\n",
> +						     clk_input);
> +			}
> +
> +			mask |= LTC2688_CH_TD_SEL_MSK;
> +			/*
> +			 * 0 means software toggle which is the default mode.
> +			 * Hence the +1.
> +			 */
> +			val |= FIELD_PREP(LTC2688_CH_TD_SEL_MSK, clk_input + 1);
> +			clk_msk |= BIT(clk_input);
> +			/*
> +			 * If a TGPx is given, we automatically assume a dither
> +			 * capable channel (unless toggle is already enabled).
> +			 * Hence, we prepar our TGPx <-> channel map to make it

prepare

> +			 * easier to handle the clocks and available frequency
> +			 * calculations... On top of this we just set here the
> +			 * dither bit in the channel settings. It won't have any
> +			 * effect until the global toggle/dither bit is enabled.
> +			 */
> +			if (!chan->toggle_chan) {
> +				tgp[clk_input].chan[cur_chan] = reg;
> +				tgp[clk_input].n_chans++;
> +				mask |= LTC2688_CH_MODE_MSK;
> +				val |= FIELD_PREP(LTC2688_CH_MODE_MSK, 1);
> +				ltc2688_channels[reg].ext_info = ltc2688_dither_ext_info;
> +			} else {
> +				/* wait, no sw toggle after all */
> +				ltc2688_channels[reg].ext_info = LTC2688_CHAN_TOGGLE(__no_s, NULL);
> +			}
> +		}
> +
> +		if (fwnode_property_read_bool(child, "adi,overrange")) {
> +			chan->overrange = true;
> +			val |= LTC2688_CH_OVERRANGE_MSK;
> +			mask |= BIT(3);
> +		}
> +
> +		if (!mask)
> +			continue;
> +
> +		ret = regmap_update_bits(st->regmap,
> +					 LTC2688_CMD_CH_SETTING(reg), mask,
> +					 val);

Maybe I'm missing something, but why not just write the whole register?
Certainly most if not all of the value is controlled by this function or known
at this point to be in the reset state.


> +		if (ret) {
> +			fwnode_handle_put(child);
> +			return dev_err_probe(&st->spi->dev, -EINVAL,
> +					     "failed to set chan settings\n");
> +		}
> +
> +		mask = 0;
> +		val = 0;

Why not at the start of the loop?  Particularly as I can't see them being
initialised for the first loop.


> +	}
> +
> +	return ltc2688_tgp_setup(st, clk_msk, tgp);
> +}


...

> +static bool ltc2688_reg_writable(struct device *dev, unsigned int reg)
> +{
> +	if (reg <= LTC2688_CMD_UPDATE_ALL && reg != LTC2688_CMD_THERMAL_STAT)

Isn't UPDATE_ALL the last register?  So how do you get higher than that?
Definitely needs a comment if there is a reason that check is necessary.

> +		return true;
> +
> +	return false;
> +}
> +
> +struct regmap_bus ltc2688_regmap_bus = {
> +	.read = ltc2688_spi_read,
> +	.write = ltc2688_spi_write,
> +	.read_flag_mask = LTC2688_READ_OPERATION,
> +	.reg_format_endian_default = REGMAP_ENDIAN_BIG,
> +	.val_format_endian_default = REGMAP_ENDIAN_BIG
> +};
> +
> +static const struct regmap_config ltc2688_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 16,
> +	.readable_reg = ltc2688_reg_readable,
> +	.writeable_reg = ltc2688_reg_writable,
> +	/* ignoring the no op command */
> +	.max_register = LTC2688_CMD_UPDATE_ALL
> +};
> +
> +static const struct iio_info ltc2688_info = {
> +	.write_raw = ltc2688_write_raw,
> +	.read_raw = ltc2688_read_raw,
> +	.read_avail = ltc2688_read_avail,
> +	.debugfs_reg_access = ltc2688_reg_access,
> +};
> +
> +static int ltc2688_probe(struct spi_device *spi)
> +{
> +	struct ltc2688_state *st;
> +	struct iio_dev *indio_dev;
> +	struct regulator *vref_reg;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->spi = spi;

blank line so it's clear the comment refers to the next block.

> +	/* Just this once. No need to di it in every regmap read */

do

> +	st->tx_data[0] = LTC2688_CMD_NOOP;
> +	mutex_init(&st->lock);
> +
> +	st->regmap = devm_regmap_init(&spi->dev, &ltc2688_regmap_bus, st,

A lot of references to &spi->dev so probably worth a local
struct device *dev = &spi->dev; Might let a few things fit
on fewer lines, but either way it'll be a little tidier.


> +				      &ltc2688_regmap_config);
> +	if (IS_ERR(st->regmap))
> +		return dev_err_probe(&spi->dev, PTR_ERR(st->regmap),
> +				     "Failed to init regmap");
> +
> +	st->regulators[0].supply = "vcc";
> +	st->regulators[1].supply = "iovcc";
> +	ret = devm_regulator_bulk_get(&spi->dev, ARRAY_SIZE(st->regulators),
> +				      st->regulators);
> +	if (ret)
> +		return dev_err_probe(&spi->dev, ret, "Failed to get regulators\n");
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(st->regulators), st->regulators);
> +	if (ret)
> +		return dev_err_probe(&spi->dev, ret,
> +				     "Failed to enable regulators\n");
> +
> +	ret = devm_add_action_or_reset(&spi->dev, ltc2688_bulk_disable, st);
> +	if (ret)
> +		return ret;
> +
> +	vref_reg = devm_regulator_get_optional(&spi->dev, "vref");
> +	if (!IS_ERR(vref_reg)) {
> +		ret = regulator_enable(vref_reg);
> +		if (ret)
> +			return dev_err_probe(&spi->dev, ret,
> +					     "Failed to enable vref regulators\n");
> +
> +		ret = devm_add_action_or_reset(&spi->dev,
> +					       ltc2688_disable_regulator,
> +					       vref_reg);
> +		if (ret)
> +			return ret;
> +
> +		ret = regulator_get_voltage(vref_reg);
> +		if (ret < 0)
> +			return dev_err_probe(&spi->dev, ret,
> +					     "Failed to get vref\n");
> +
> +		st->vref = ret / 1000;
> +	} else {
> +		if (PTR_ERR(vref_reg) != -ENODEV)
> +			return dev_err_probe(&spi->dev, PTR_ERR(vref_reg),
> +					     "Failed to get vref regulator");
> +
> +		vref_reg = NULL;
> +		/* internal reference */
> +		st->vref = 4096;
> +	}
> +
> +	indio_dev->name = "ltc2688";
> +	indio_dev->info = &ltc2688_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = ltc2688_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(ltc2688_channels);
> +
> +	ret = ltc2688_setup(st, vref_reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	return devm_iio_device_register(&st->spi->dev, indio_dev);
> +}
> +

Nuno Sa Dec. 17, 2021, 12:31 p.m. UTC | #7

> From: Jonathan Cameron <[e-mail]>
> Sent: Thursday, December 16, 2021 3:11 PM
> To: Sa, Nuno <[e-mail]>
> Cc: [e-mail]; [e-mail]; Rob
> Herring <[e-mail]>; Lars-Peter Clausen
> <[e-mail]>; Hennerich, Michael
> <[e-mail]>
> Subject: Re: [PATCH 1/3] iio: dac: add support for ltc2688
> 
> [External]
> 
> On Tue, 14 Dec 2021 17:56:06 +0100
> Nuno Sá <[e-mail]> wrote:
> 
> > The LTC2688 is a 16 channel, 16 bit, +-15V DAC with an integrated
> > precision reference. It is guaranteed monotonic and has built in
> > rail-to-rail output buffers that can source or sink up to 20 mA.
> >
> > Signed-off-by: Nuno Sá <[e-mail]>
> 
> I'm not that keen on toggle having to be clock driven, but I guess we
> can
> always change that later when usecases come along.
> 

I did wrote about some concerns with toggle (among others) in the cover.
When you have the time, some feedback in there would be very welcome :).

Anyways, for toggle mode, I do agree that "has to be clock driven" is likely to harsh.
Right now if a toggle channel is associated with a TGPx pin, then a clock is 
mandatory and that's the condition that probably should be made optional.
Someone can very well want to drive the outputs with a GPIO even though
in that case we could argue to use the SW_TOGGLE.

> 
> > +
> > +static int ltc2688_spi_read(void *context, const void *reg, size_t
> reg_size,
> > +			    void *val, size_t val_size)
> > +{
> > +	struct ltc2688_state *st = context;
> > +	struct spi_transfer xfers[] = {
> > +		{
> > +			.tx_buf = reg,
> > +			.bits_per_word = 8,
> > +			/*
> > +			 * This means that @val will also be part of the
> > +			 * transfer as there's no pad bits. That's fine as
> these
> > +			 * bits are don't care for the device and we fill
> > +			 * @val with the proper value afterwards. Using
> regmap
> > +			 * pad bits to get reg_size right would just make
> the
> > +			 * write part more cumbersome than this...
> > +			 */
> > +			.len = reg_size + 2,
> 
> what Lars said :)

Agree...

> > +			.cs_change = 1,
> > +		}, {
> > +			.tx_buf = st->tx_data,
> > +			.rx_buf = st->rx_data,
> > +			.bits_per_word = 8,
> > +			.len = 3,
> > +		},
> > +	};
> > +	int ret;
> > +
> > +	ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> > +	if (ret)
> > +		return ret;
> > +
> > +	memcpy(val, &st->rx_data[1], val_size);
> > +
> > +	return 0;
> > +}
> > +
> > +static int ltc2688_spi_write(void *context, const void *data, size_t
> count)
> > +{
> > +	struct ltc2688_state *st = context;
> > +
> > +	return spi_write(st->spi, data, count);
> > +}
> > +
> 
> 
> 
> > +};
> > +
> > +static int ltc2688_dac_code_write(struct ltc2688_state *st, u32
> chan, u32 input,
> > +				  u16 code)
> > +{
> > +	struct ltc2688_chan *c = &st->channels[chan];
> > +	int ret, reg;
> > +
> > +	/* 2 LSBs set to 0 if writing dither amplitude */
> > +	if (!c->toggle_chan && input == LT2688_INPUT_B) {
> > +		if (code > GENMASK(13, 0))
> > +			return -EINVAL;
> > +
> > +		code <<= 2;
> 
> FIELD_PREP preferred.

Will do...

> > +	}
> > +
> > +	mutex_lock(&st->lock);
> > +	/* select the correct input register to read from */
> > +	ret = regmap_update_bits(st->regmap,
> LTC2688_CMD_A_B_SELECT, BIT(chan),
> > +				 input << chan);
> > +	if (ret)
> > +		goto unlock;
> > +
> > +	/*
> > +	 * If in dither/toggle mode the dac should be updated by an
> > +	 * external signal (or sw toggle) and not here.
> > +	 */
> > +	if (c->mode == LTC2688_MODE_DEFAULT)
> > +		reg = LTC2688_CMD_CH_CODE_UPDATE(chan);
> > +	else
> > +		reg = LTC2688_CMD_CH_CODE(chan);
> > +
> > +	ret = regmap_write(st->regmap, reg, code);
> > +unlock:
> > +	mutex_unlock(&st->lock);
> > +	return ret;
> > +}
> 
> > +
> > +static int ltc2688_read_raw(struct iio_dev *indio_dev,
> > +			    struct iio_chan_spec const *chan, int *val,
> > +			    int *val2, long m)
> > +{
> > +	struct ltc2688_state *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	switch (m) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		ret = ltc2688_dac_code_read(st, chan->channel,
> LT2688_INPUT_A,
> > +					    val);
> > +		if (ret)
> > +			return ret;
> > +
> > +		return IIO_VAL_INT;
> > +	case IIO_CHAN_INFO_OFFSET:
> > +		return ltc2688_offset_get(st, chan->channel, val);
> > +	case IIO_CHAN_INFO_SCALE:
> > +		*val2 = 16;
> > +		return ltc2688_scale_get(st, chan->channel, val);
> 
> I'm not against functions returning the IIO_VAL_* like this, but if they
> are I expect the function to set val2 as well.
> 
> I'd suggest return 0 on success and then do similar to what you have
> done for code_read above.

Typically I do like to save lines of code when doable and readability is
not hurt which is the case. I'm not doing the same for the code_read
because that one is also used from the extended_info interface. That
said, I don't have strong feeling about this so I can do as you suggest.

> > +	case IIO_CHAN_INFO_CALIBBIAS:
> > +		ret = regmap_read(st->regmap,
> > +				  LTC2688_CMD_CH_OFFSET(chan-
> >channel), val);
> > +		if (ret)
> > +			return ret;
> > +
> > +		/* Just 13 bits used. 2LSB ignored */
> > +		*val >>= 2;
> FIELD_GET() would get rid of need for the comment.
> 
> > +		return IIO_VAL_INT;
> > +	case IIO_CHAN_INFO_CALIBSCALE:
> > +		ret = regmap_read(st->regmap,
> > +				  LTC2688_CMD_CH_GAIN(chan-
> >channel), val);
> > +		if (ret)
> > +			return ret;
> > +
> > +		return IIO_VAL_INT;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> 
> > +
> > +static ssize_t ltc2688_read_ext(struct iio_dev *indio_dev,
> > +				uintptr_t private,
> > +				const struct iio_chan_spec *chan,
> > +				char *buf)
> > +{
> > +	struct ltc2688_state *st = iio_priv(indio_dev);
> > +	u32 val;
> > +	int ret;
> > +
> > +	switch (private) {
> > +	case LTC2688_DITHER_TOGGLE_ENABLE:
> 
> As below. I'd have separate functions rather than multiplexing this
> to little benefit.
> 
> > +		return ltc2688_reg_bool_get(st, chan->channel,
> > +
> LTC2688_CMD_TOGGLE_DITHER_EN, buf);
> > +	case LTC2688_POWERDOWN:
> > +		return ltc2688_reg_bool_get(st, chan->channel,
> > +
> LTC2688_CMD_POWERDOWN, buf);
> > +	case LTC2688_INPUT_B:
> > +		ret = ltc2688_dac_code_read(st, chan->channel,
> LT2688_INPUT_B,
> > +					    &val);
> > +		if (ret)
> > +			return ret;
> > +
> > +		return sysfs_emit(buf, "%u\n", val);
> > +	case LTC2688_INPUT_B_AVAIL:
> > +		/* dither amplitude has 1/4 of the span */
> > +		return sysfs_emit(buf, "[%u %u %u]\n",
> ltc2688_raw_range[0],
> > +				  ltc2688_raw_range[1],
> > +				  ltc2688_raw_range[2] / 4);
> > +	case LTC2688_SW_TOGGLE:
> > +		return ltc2688_reg_bool_get(st, chan->channel,
> > +					    LTC2688_CMD_SW_TOGGLE,
> buf);
> > +	case LTC2688_DITHER_FREQ:
> > +		return ltc2688_dither_freq_get(st, chan->channel,
> buf);
> > +	case LTC2688_DITHER_FREQ_AVAIL:
> > +		return ltc2688_dither_freq_avail(st, chan->channel,
> buf);
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static ssize_t ltc2688_write_ext(struct iio_dev *indio_dev,
> > +				 uintptr_t private,
> > +				 const struct iio_chan_spec *chan,
> > +				 const char *buf, size_t len)
> > +{
> > +	struct ltc2688_state *st = iio_priv(indio_dev);
> > +	u16 val;
> > +	int ret;
> > +	bool en;
> > +
> > +	switch (private) {
> > +	case LTC2688_DITHER_TOGGLE_ENABLE:
> 
> As commented on below, I'd just have a function for each case block
> you have
> here.
> 
> > +		ret = kstrtobool(buf, &en);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = ltc2688_dither_toggle_set(st, chan->channel, en);
> > +		if (ret)
> > +			return ret;
> > +
> > +		return len;
> > +	case LTC2688_POWERDOWN:
> > +		ret = ltc2688_reg_bool_set(st, chan->channel,
> > +					   LTC2688_CMD_POWERDOWN,
> buf);
> > +		if (ret)
> > +			return ret;
> > +
> > +		return len;
> > +	case LTC2688_INPUT_B:
> > +		ret = kstrtou16(buf, 10, &val);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = ltc2688_dac_code_write(st, chan->channel,
> LT2688_INPUT_B,
> > +					     val);
> > +		if (ret)
> > +			return ret;
> > +
> > +		return len;
> > +	case LTC2688_SW_TOGGLE:
> > +		ret = ltc2688_reg_bool_set(st, chan->channel,
> > +					   LTC2688_CMD_SW_TOGGLE,
> buf);
> > +		if (ret)
> > +			return ret;
> > +
> > +		return len;
> > +	case LTC2688_DITHER_FREQ:
> > +		ret = ltc2688_dither_freq_set(st, chan->channel, buf);
> > +		if (ret)
> > +			return ret;
> > +
> > +		return len;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static int ltc2688_get_dither_phase(struct iio_dev *dev,
> > +				    const struct iio_chan_spec *chan)
> > +{
> > +	struct ltc2688_state *st = iio_priv(dev);
> > +	int ret, regval;
> > +
> > +	ret = regmap_read(st->regmap,
> LTC2688_CMD_CH_SETTING(chan->channel),
> > +			  &regval);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return FIELD_GET(LTC2688_CH_DIT_PH_MSK, regval);
> > +}
> > +
> > +static int ltc2688_set_dither_phase(struct iio_dev *dev,
> > +				    const struct iio_chan_spec *chan,
> > +				    unsigned int phase)
> > +{
> > +	struct ltc2688_state *st = iio_priv(dev);
> > +
> > +	return regmap_update_bits(st->regmap,
> > +				  LTC2688_CMD_CH_SETTING(chan-
> >channel),
> > +				  LTC2688_CH_DIT_PH_MSK,
> > +
> FIELD_PREP(LTC2688_CH_DIT_PH_MSK, phase));
> > +}
> > +
> > +static int ltc2688_reg_access(struct iio_dev *indio_dev,
> > +			      unsigned int reg,
> > +			      unsigned int writeval,
> > +			      unsigned int *readval)
> > +{
> > +	struct ltc2688_state *st = iio_priv(indio_dev);
> > +
> > +	if (readval)
> > +		return regmap_read(st->regmap, reg, readval);
> > +	else
> > +		return regmap_write(st->regmap, reg, writeval);
> > +}
> > +
> > +static const char * const ltc2688_dither_phase[] = {
> > +	"0", "90", "180", "270",
> > +};
> > +
> > +static const struct iio_enum ltc2688_dither_phase_enum = {
> > +	.items = ltc2688_dither_phase,
> > +	.num_items = ARRAY_SIZE(ltc2688_dither_phase),
> > +	.set = ltc2688_set_dither_phase,
> > +	.get = ltc2688_get_dither_phase,
> > +};
> > +
> > +#define LTC2688_CHAN_EXT_INFO(_name, _what, _shared) {	\
> > +	.name = _name,					\
> > +	.read = ltc2688_read_ext,			\
> > +	.write = ltc2688_write_ext,			\
> 
> I'm not really convinced big multiplexer functions are a good idea here.
> They seem to save little code and hurt readability a bit.

I think this is a very common pattern seen in IIO and probably HWMON no?
Anyways, I'm ok with either way so I can just extend the macro to accept
the individual functions. I have to admit that in some cases (when locking is
required in some case blocks) I'm also not a big fan of these multiplexes
functions. And I think I'm calling individual functions in all the case blocks
anyways...
  
> > +	.private = (_what),				\
> > +	.shared = (_shared),				\
> > +}
> > +
> > +/*
> > + * For toggle mode we only expose the symbol attr (sw_toggle) in
> case a TGPx is
> > + * not provided in dts.
> > + */
> > +#define LTC2688_CHAN_TOGGLE(t, name) ({
> 				\
> > +	static const struct iio_chan_spec_ext_info t ## _ext_info[] = {
> 			\
> > +		LTC2688_CHAN_EXT_INFO("raw1", LTC2688_INPUT_B,
> IIO_SEPARATE),		\
> > +		LTC2688_CHAN_EXT_INFO("toggle_en",
> LTC2688_DITHER_TOGGLE_ENABLE,	\
> > +				      IIO_SEPARATE),
> 		\
> > +		LTC2688_CHAN_EXT_INFO("powerdown",
> LTC2688_POWERDOWN, IIO_SEPARATE),	\
> > +		LTC2688_CHAN_EXT_INFO(name,
> LTC2688_SW_TOGGLE, IIO_SEPARATE),		\
> > +		{}
> 		\
> > +	};
> 		\
> > +	t ## _ext_info;
> 		\
> > +})
> > +
> > +static struct iio_chan_spec_ext_info ltc2688_dither_ext_info[] = {
> > +	LTC2688_CHAN_EXT_INFO("dither_raw", LTC2688_INPUT_B,
> IIO_SEPARATE),
> > +	LTC2688_CHAN_EXT_INFO("dither_raw_available",
> LTC2688_INPUT_B_AVAIL,
> > +			      IIO_SEPARATE),
> > +	/*
> > +	 * Not IIO_ENUM because the available freq needs to be
> computed at
> > +	 * probe. We could still use it, but it didn't felt much right.
> > +	 *
> 
> no extra blank line.
> 
> > +	 */
> > +	LTC2688_CHAN_EXT_INFO("dither_frequency",
> LTC2688_DITHER_FREQ,
> > +			      IIO_SEPARATE),
> > +	LTC2688_CHAN_EXT_INFO("dither_frequency_available",
> > +			      LTC2688_DITHER_FREQ_AVAIL,
> IIO_SEPARATE),
> > +	IIO_ENUM("dither_phase", IIO_SEPARATE,
> &ltc2688_dither_phase_enum),
> > +	IIO_ENUM_AVAILABLE("dither_phase", IIO_SEPARATE,
> > +			   &ltc2688_dither_phase_enum),
> > +	LTC2688_CHAN_EXT_INFO("dither_en",
> LTC2688_DITHER_TOGGLE_ENABLE,
> > +			      IIO_SEPARATE),
> > +	LTC2688_CHAN_EXT_INFO("powerdown",
> LTC2688_POWERDOWN, IIO_SEPARATE),
> > +	{}
> > +};
> > +
> > +static const struct iio_chan_spec_ext_info ltc2688_ext_info[] = {
> > +	LTC2688_CHAN_EXT_INFO("powerdown",
> LTC2688_POWERDOWN, IIO_SEPARATE),
> > +	{}
> > +};
> > +
> 
> > +
> > +enum {
> > +	LTC2688_CHAN_TD_TGP1,
> > +	LTC2688_CHAN_TD_TGP2,
> > +	LTC2688_CHAN_TD_TGP3,
> > +	LTC2688_CHAN_TD_MAX
> > +};
> 
> > +/* Helper struct to deal with dither channels binded to TGPx pins */
> > +struct ltc2688_dither_helper {
> > +	u8 chan[LTC2688_DAC_CHANNELS];
> > +	u8 n_chans;
> > +};
> > +
> bitmap perhaps given ordering doesn't matter (I think)
> 

Yeah, did not thought about it but I think it will look better with a bitmap yes.
Although I'm not sure if I will continue with this approach or make the clocks
property a per channel one (more on this in the cover letter).
> 
> ...
> 
> > +
> > +static int ltc2688_channel_config(struct ltc2688_state *st)
> > +{
> > +	struct fwnode_handle *fwnode = dev_fwnode(&st->spi-
> >dev), *child;
> > +	struct ltc2688_dither_helper tgp[LTC2688_CHAN_TD_MAX] =
> {0};
> > +	u32 reg, clk_input, val, mask, tmp[2];
> > +	unsigned long clk_msk = 0;
> > +	int ret, span;
> > +
> 
> I think you need to sanity check you have a fwnode

AFAICT, it's done by us already :)

https://elixir.bootlin.com/linux/latest/source/drivers/base/property.c#L741

> > +	fwnode_for_each_available_child_node(fwnode, child) {
> 
> I guess this is because of the whole
> device_for_each_available_child_node() not
> existing discussion that isn't resolved.

exactly... I wanted the available option and this was the only way I
could find...

> > +		struct ltc2688_chan *chan;
> > +
> > +		ret = fwnode_property_read_u32(child, "reg", &reg);
> > +		if (ret) {
> > +			fwnode_handle_put(child);
> > +			return dev_err_probe(&st->spi->dev, ret,
> > +					     "Failed to get reg
> property\n");
> > +		}
> > +
> > +		if (reg >= LTC2688_DAC_CHANNELS) {
> > +			fwnode_handle_put(child);
> > +			return dev_err_probe(&st->spi->dev, -EINVAL,
> > +					     "reg bigger than: %d\n",
> > +					     LTC2688_DAC_CHANNELS);
> > +		}
> > +
> > +		chan = &st->channels[reg];
> > +		if (fwnode_property_read_bool(child, "adi,toggle-
> mode")) {
> > +			chan->toggle_chan = true;
> > +			/* assume sw toggle ABI */
> > +			ltc2688_channels[reg].ext_info =
> LTC2688_CHAN_TOGGLE(__s, "symbol");
> 
> That's a little nasty.  Pick it up from a static const array defined outside
> this function.

Ehehe, personally I do not think is that bad but ok, Lars was also complaining
about it so better listen to you :)

> > +		}
> > +
> > +		ret = fwnode_property_read_u32_array(child,
> "adi,output-range-millivolt",
> > +						     tmp,
> ARRAY_SIZE(tmp));
> > +		if (!ret) {
> > +			span = ltc2688_span_lookup(st, tmp[0],
> tmp[1]);
> > +			if (span < 0)
> > +				return dev_err_probe(&st->spi->dev, -
> EINVAL,
> > +						     "output range not
> valid:[%d %d]\n",
> > +						     tmp[0], tmp[1]);
> > +
> > +			mask |= LTC2688_CH_SPAN_MSK;
> > +			val |= FIELD_PREP(LTC2688_CH_SPAN_MSK,
> span);
> > +		}
> > +
> > +		ret = fwnode_property_read_u32(child, "adi,toggle-
> dither-input",
> > +					       &clk_input);
> > +		if (!ret) {
> > +			int cur_chan = tgp[clk_input].n_chans;
> > +
> > +			if (clk_input > LTC2688_CHAN_TD_TGP3) {
> > +				fwnode_handle_put(child);
> > +				return dev_err_probe(&st->spi->dev, -
> EINVAL,
> > +						     "toggle-dither-input
> inv value(%d)\n",
> > +						     clk_input);
> > +			}
> > +
> > +			mask |= LTC2688_CH_TD_SEL_MSK;
> > +			/*
> > +			 * 0 means software toggle which is the default
> mode.
> > +			 * Hence the +1.
> > +			 */
> > +			val |= FIELD_PREP(LTC2688_CH_TD_SEL_MSK,
> clk_input + 1);
> > +			clk_msk |= BIT(clk_input);
> > +			/*
> > +			 * If a TGPx is given, we automatically assume a
> dither
> > +			 * capable channel (unless toggle is already
> enabled).
> > +			 * Hence, we prepar our TGPx <-> channel map
> to make it
> 
> prepare
> 
> > +			 * easier to handle the clocks and available
> frequency
> > +			 * calculations... On top of this we just set here
> the
> > +			 * dither bit in the channel settings. It won't
> have any
> > +			 * effect until the global toggle/dither bit is
> enabled.
> > +			 */
> > +			if (!chan->toggle_chan) {
> > +				tgp[clk_input].chan[cur_chan] = reg;
> > +				tgp[clk_input].n_chans++;
> > +				mask |= LTC2688_CH_MODE_MSK;
> > +				val |=
> FIELD_PREP(LTC2688_CH_MODE_MSK, 1);
> > +				ltc2688_channels[reg].ext_info =
> ltc2688_dither_ext_info;
> > +			} else {
> > +				/* wait, no sw toggle after all */
> > +				ltc2688_channels[reg].ext_info =
> LTC2688_CHAN_TOGGLE(__no_s, NULL);
> > +			}
> > +		}
> > +
> > +		if (fwnode_property_read_bool(child,
> "adi,overrange")) {
> > +			chan->overrange = true;
> > +			val |= LTC2688_CH_OVERRANGE_MSK;
> > +			mask |= BIT(3);
> > +		}
> > +
> > +		if (!mask)
> > +			continue;
> > +
> > +		ret = regmap_update_bits(st->regmap,
> > +
> LTC2688_CMD_CH_SETTING(reg), mask,
> > +					 val);
> 
> Maybe I'm missing something, but why not just write the whole
> register?
> Certainly most if not all of the value is controlled by this function or
> known
> at this point to be in the reset state.

Yeah, that's true. The register will be 0 in reset so just writing will
be enough. Nice catch.

> 
> > +		if (ret) {
> > +			fwnode_handle_put(child);
> > +			return dev_err_probe(&st->spi->dev, -EINVAL,
> > +					     "failed to set chan
> settings\n");
> > +		}
> > +
> > +		mask = 0;
> > +		val = 0;
> 
> Why not at the start of the loop?  Particularly as I can't see them being
> initialised for the first loop.

Hmm I'm actually wondering how did this worked in my tests. I will just
move val to the beginning and mask can be dropped.
 
> 
> > +	}
> > +
> > +	return ltc2688_tgp_setup(st, clk_msk, tgp);
> > +}
> 
> 
> ...
> 
> > +static bool ltc2688_reg_writable(struct device *dev, unsigned int
> reg)
> > +{
> > +	if (reg <= LTC2688_CMD_UPDATE_ALL && reg !=
> LTC2688_CMD_THERMAL_STAT)
> 
> Isn't UPDATE_ALL the last register?  So how do you get higher than
> that?
> Definitely needs a comment if there is a reason that check is
> necessary.

If you look at the commands table you see that on the write side
we jump from 0x76 to 0x78 (UPDATE_ALL=0x7c). 0x77 refers to
reading the thermal status reg which is not writable. Actually in the
end, as it's a read the command for reading the thermal status will
be 0xf7.

> > +		return true;
> > +
> > +	return false;
> > +}
> > +
> > +struct regmap_bus ltc2688_regmap_bus = {
> > +	.read = ltc2688_spi_read,
> > +	.write = ltc2688_spi_write,
> > +	.read_flag_mask = LTC2688_READ_OPERATION,
> > +	.reg_format_endian_default = REGMAP_ENDIAN_BIG,
> > +	.val_format_endian_default = REGMAP_ENDIAN_BIG
> > +};
> > +
> > +static const struct regmap_config ltc2688_regmap_config = {
> > +	.reg_bits = 8,
> > +	.val_bits = 16,
> > +	.readable_reg = ltc2688_reg_readable,
> > +	.writeable_reg = ltc2688_reg_writable,
> > +	/* ignoring the no op command */
> > +	.max_register = LTC2688_CMD_UPDATE_ALL
> > +};
> > +
> > +static const struct iio_info ltc2688_info = {
> > +	.write_raw = ltc2688_write_raw,
> > +	.read_raw = ltc2688_read_raw,
> > +	.read_avail = ltc2688_read_avail,
> > +	.debugfs_reg_access = ltc2688_reg_access,
> > +};
> > +
> > +static int ltc2688_probe(struct spi_device *spi)
> > +{
> > +	struct ltc2688_state *st;
> > +	struct iio_dev *indio_dev;
> > +	struct regulator *vref_reg;
> > +	int ret;
> > +
> > +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	st = iio_priv(indio_dev);
> > +	st->spi = spi;
> 
> blank line so it's clear the comment refers to the next block.
> 
> > +	/* Just this once. No need to di it in every regmap read */
> 
> do
> 
> > +	st->tx_data[0] = LTC2688_CMD_NOOP;
> > +	mutex_init(&st->lock);
> > +
> > +	st->regmap = devm_regmap_init(&spi->dev,
> &ltc2688_regmap_bus, st,
> 
> A lot of references to &spi->dev so probably worth a local
> struct device *dev = &spi->dev; Might let a few things fit
> on fewer lines, but either way it'll be a little tidier.

can do that...

- Nuno Sá

Jonathan Cameron Dec. 30, 2021, 3:28 p.m. UTC | #8

On Fri, 17 Dec 2021 12:31:57 +0000
"Sa, Nuno" <[e-mail]> wrote:

> > From: Jonathan Cameron <[e-mail]>
> > Sent: Thursday, December 16, 2021 3:11 PM
> > To: Sa, Nuno <[e-mail]>
> > Cc: [e-mail]; [e-mail]; Rob
> > Herring <[e-mail]>; Lars-Peter Clausen
> > <[e-mail]>; Hennerich, Michael
> > <[e-mail]>
> > Subject: Re: [PATCH 1/3] iio: dac: add support for ltc2688
> > 
> > [External]
> > 
> > On Tue, 14 Dec 2021 17:56:06 +0100
> > Nuno Sá <[e-mail]> wrote:
> >   
> > > The LTC2688 is a 16 channel, 16 bit, +-15V DAC with an integrated
> > > precision reference. It is guaranteed monotonic and has built in
> > > rail-to-rail output buffers that can source or sink up to 20 mA.
> > >
> > > Signed-off-by: Nuno Sá <[e-mail]>  
> > 
> > I'm not that keen on toggle having to be clock driven, but I guess we
> > can
> > always change that later when usecases come along.
> >   
> 
> I did wrote about some concerns with toggle (among others) in the cover.
> When you have the time, some feedback in there would be very welcome :).

Doh.  Guess I didn't look at the cover letter. Now replied to that as well.

> 
> Anyways, for toggle mode, I do agree that "has to be clock driven" is likely to harsh.
> Right now if a toggle channel is associated with a TGPx pin, then a clock is 
> mandatory and that's the condition that probably should be made optional.
> Someone can very well want to drive the outputs with a GPIO even though
> in that case we could argue to use the SW_TOGGLE.

I wonder if we also need the case where the toggle source is invisible to us
as it's the output of some other hardware.  Obviously would be nice to model
that hardware in DT but that might not always be possible.


> > > +
> > > +static int ltc2688_read_raw(struct iio_dev *indio_dev,
> > > +			    struct iio_chan_spec const *chan, int *val,
> > > +			    int *val2, long m)
> > > +{
> > > +	struct ltc2688_state *st = iio_priv(indio_dev);
> > > +	int ret;
> > > +
> > > +	switch (m) {
> > > +	case IIO_CHAN_INFO_RAW:
> > > +		ret = ltc2688_dac_code_read(st, chan->channel,  
> > LT2688_INPUT_A,  
> > > +					    val);
> > > +		if (ret)
> > > +			return ret;
> > > +
> > > +		return IIO_VAL_INT;
> > > +	case IIO_CHAN_INFO_OFFSET:
> > > +		return ltc2688_offset_get(st, chan->channel, val);
> > > +	case IIO_CHAN_INFO_SCALE:
> > > +		*val2 = 16;
> > > +		return ltc2688_scale_get(st, chan->channel, val);  
> > 
> > I'm not against functions returning the IIO_VAL_* like this, but if they
> > are I expect the function to set val2 as well.
> > 
> > I'd suggest return 0 on success and then do similar to what you have
> > done for code_read above.  
> 
> Typically I do like to save lines of code when doable and readability is
> not hurt which is the case. I'm not doing the same for the code_read
> because that one is also used from the extended_info interface. That
> said, I don't have strong feeling about this so I can do as you suggest.

Either option is fine for me.  Set val2 inside _scale_get() or return 0
from that and then do a return IIO_VAL_INT_PLUS_MICRO here.

The particular combination at the moment is rather inconsistent as
val, val2 and the return value should all come from the same 'source'
whether it's here, or in _scale_get()

> 
> > > +	case IIO_CHAN_INFO_CALIBBIAS:
> > > +		ret = regmap_read(st->regmap,
> > > +				  LTC2688_CMD_CH_OFFSET(chan-
> > >channel), val);
> > > +		if (ret)
> > > +			return ret;
> > > +
> > > +		/* Just 13 bits used. 2LSB ignored */
> > > +		*val >>= 2;  
> > FIELD_GET() would get rid of need for the comment.
> >   
> > > +		return IIO_VAL_INT;
> > > +	case IIO_CHAN_INFO_CALIBSCALE:
> > > +		ret = regmap_read(st->regmap,
> > > +				  LTC2688_CMD_CH_GAIN(chan-
> > >channel), val);
> > > +		if (ret)
> > > +			return ret;
> > > +
> > > +		return IIO_VAL_INT;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +}  

...

> > > +
> > > +static const char * const ltc2688_dither_phase[] = {
> > > +	"0", "90", "180", "270",
> > > +};
> > > +
> > > +static const struct iio_enum ltc2688_dither_phase_enum = {
> > > +	.items = ltc2688_dither_phase,
> > > +	.num_items = ARRAY_SIZE(ltc2688_dither_phase),
> > > +	.set = ltc2688_set_dither_phase,
> > > +	.get = ltc2688_get_dither_phase,
> > > +};
> > > +
> > > +#define LTC2688_CHAN_EXT_INFO(_name, _what, _shared) {	\
> > > +	.name = _name,					\
> > > +	.read = ltc2688_read_ext,			\
> > > +	.write = ltc2688_write_ext,			\  
> > 
> > I'm not really convinced big multiplexer functions are a good idea here.
> > They seem to save little code and hurt readability a bit.  
> 
> I think this is a very common pattern seen in IIO and probably HWMON no?
> Anyways, I'm ok with either way so I can just extend the macro to accept
> the individual functions. I have to admit that in some cases (when locking is
> required in some case blocks) I'm also not a big fan of these multiplexes
> functions. And I think I'm calling individual functions in all the case blocks
> anyways...

Common pattern, but not always a good idea.  All depends on how much
common code there is.  In this case I don't think there is enough for it
to make sense.

>   
> > > +	.private = (_what),				\
> > > +	.shared = (_shared),				\
> > > +}
> > > +

...

> > > +	 */
> > > +	LTC2688_CHAN_EXT_INFO("dither_frequency",  
> > LTC2688_DITHER_FREQ,  
> > > +			      IIO_SEPARATE),
> > > +	LTC2688_CHAN_EXT_INFO("dither_frequency_available",
> > > +			      LTC2688_DITHER_FREQ_AVAIL,  
> > IIO_SEPARATE),  
> > > +	IIO_ENUM("dither_phase", IIO_SEPARATE,  
> > &ltc2688_dither_phase_enum),  
> > > +	IIO_ENUM_AVAILABLE("dither_phase", IIO_SEPARATE,
> > > +			   &ltc2688_dither_phase_enum),
> > > +	LTC2688_CHAN_EXT_INFO("dither_en",  
> > LTC2688_DITHER_TOGGLE_ENABLE,  
> > > +			      IIO_SEPARATE),
> > > +	LTC2688_CHAN_EXT_INFO("powerdown",  
> > LTC2688_POWERDOWN, IIO_SEPARATE),  
> > > +	{}
> > > +};
> > > +
> > > +static const struct iio_chan_spec_ext_info ltc2688_ext_info[] = {
> > > +	LTC2688_CHAN_EXT_INFO("powerdown",  
> > LTC2688_POWERDOWN, IIO_SEPARATE),  
> > > +	{}
> > > +};
> > > +  
> >   
> > > +
> > > +enum {
> > > +	LTC2688_CHAN_TD_TGP1,
> > > +	LTC2688_CHAN_TD_TGP2,
> > > +	LTC2688_CHAN_TD_TGP3,
> > > +	LTC2688_CHAN_TD_MAX
> > > +};  
> >   
> > > +/* Helper struct to deal with dither channels binded to TGPx pins */
> > > +struct ltc2688_dither_helper {
> > > +	u8 chan[LTC2688_DAC_CHANNELS];
> > > +	u8 n_chans;
> > > +};
> > > +  
> > bitmap perhaps given ordering doesn't matter (I think)
> >   
> 
> Yeah, did not thought about it but I think it will look better with a bitmap yes.
> Although I'm not sure if I will continue with this approach or make the clocks
> property a per channel one (more on this in the cover letter).

I'm not sure how the per channel version will look so leaving this entirely
up to you!

> > 
> > ...
> >   
> > > +
> > > +static int ltc2688_channel_config(struct ltc2688_state *st)
> > > +{
> > > +	struct fwnode_handle *fwnode = dev_fwnode(&st->spi-
> > >dev), *child;
> > > +	struct ltc2688_dither_helper tgp[LTC2688_CHAN_TD_MAX] =  
> > {0};  
> > > +	u32 reg, clk_input, val, mask, tmp[2];
> > > +	unsigned long clk_msk = 0;
> > > +	int ret, span;
> > > +  
> > 
> > I think you need to sanity check you have a fwnode  
> 
> AFAICT, it's done by us already :)
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/base/property.c#L741

Ah.  Good point. Ignore that one then.

> 
> > > +	fwnode_for_each_available_child_node(fwnode, child) {  
> > 
> > I guess this is because of the whole
> > device_for_each_available_child_node() not
> > existing discussion that isn't resolved.  
> 
> exactly... I wanted the available option and this was the only way I
> could find...
> 

Hmm. I need to revisit that discussion and see where we got to.

> >   
> > > +static bool ltc2688_reg_writable(struct device *dev, unsigned int  
> > reg)  
> > > +{
> > > +	if (reg <= LTC2688_CMD_UPDATE_ALL && reg !=  
> > LTC2688_CMD_THERMAL_STAT)
> > 
> > Isn't UPDATE_ALL the last register?  So how do you get higher than
> > that?
> > Definitely needs a comment if there is a reason that check is
> > necessary.  
> 
> If you look at the commands table you see that on the write side
> we jump from 0x76 to 0x78 (UPDATE_ALL=0x7c). 0x77 refers to
> reading the thermal status reg which is not writable. Actually in the
> end, as it's a read the command for reading the thermal status will
> be 0xf7.

I'm lost on this.   My confusion is how you get > LTC2688_CMD_UPDATE_ALL
Possibly that's what you are referring to with teh read command being 0xf...
Maybe try to distil this info down to a brief comment for next
version?

> 
> > > +		return true;
> > > +
> > > +	return false;
> > > +}

Thanks,

Jonathan

Nuno Sa Jan. 7, 2022, 3:44 p.m. UTC | #9

> From: Jonathan Cameron <[e-mail]>
> Sent: Thursday, December 30, 2021 4:28 PM
> To: Sa, Nuno <[e-mail]>
> Cc: [e-mail]; [e-mail]; Rob
> Herring <[e-mail]>; Lars-Peter Clausen
> <[e-mail]>; Hennerich, Michael
> <[e-mail]>
> Subject: Re: [PATCH 1/3] iio: dac: add support for ltc2688
> 
> [External]
> 
> On Fri, 17 Dec 2021 12:31:57 +0000
> "Sa, Nuno" <[e-mail]> wrote:
> 
> > > From: Jonathan Cameron <[e-mail]>
> > > Sent: Thursday, December 16, 2021 3:11 PM
> > > To: Sa, Nuno <[e-mail]>
> > > Cc: [e-mail]; [e-mail]; Rob
> > > Herring <[e-mail]>; Lars-Peter Clausen
> > > <[e-mail]>; Hennerich, Michael
> > > <[e-mail]>
> > > Subject: Re: [PATCH 1/3] iio: dac: add support for ltc2688
> > >
> > > [External]
> > >
> > > On Tue, 14 Dec 2021 17:56:06 +0100
> > > Nuno Sá <[e-mail]> wrote:
> > >
> > > > The LTC2688 is a 16 channel, 16 bit, +-15V DAC with an integrated
> > > > precision reference. It is guaranteed monotonic and has built in
> > > > rail-to-rail output buffers that can source or sink up to 20 mA.
> > > >
> > > > Signed-off-by: Nuno Sá <[e-mail]>
> > >
> > > I'm not that keen on toggle having to be clock driven, but I guess
> we
> > > can
> > > always change that later when usecases come along.
> > >
> >
> > I did wrote about some concerns with toggle (among others) in the
> cover.
> > When you have the time, some feedback in there would be very
> welcome :).
> 
> Doh.  Guess I didn't look at the cover letter. Now replied to that as
> well.
> 
> >
> > Anyways, for toggle mode, I do agree that "has to be clock driven" is
> likely to harsh.
> > Right now if a toggle channel is associated with a TGPx pin, then a
> clock is
> > mandatory and that's the condition that probably should be made
> optional.
> > Someone can very well want to drive the outputs with a GPIO even
> though
> > in that case we could argue to use the SW_TOGGLE.
> 
> I wonder if we also need the case where the toggle source is invisible
> to us
> as it's the output of some other hardware.  Obviously would be nice to
> model
> that hardware in DT but that might not always be possible.

Yeah, could fall into the host GPIO usecase. We don't really control it
in the driver but the user can still use it to drive the dac output. Anyways,
as stated in the cover, let's leave things as is for now...

> 
> > > > +
> > > > +static int ltc2688_read_raw(struct iio_dev *indio_dev,
> > > > +			    struct iio_chan_spec const *chan, int *val,
> > > > +			    int *val2, long m)
> > > > +{
> > > > +	struct ltc2688_state *st = iio_priv(indio_dev);
> > > > +	int ret;
> > > > +
> > > > +	switch (m) {
> > > > +	case IIO_CHAN_INFO_RAW:
> > > > +		ret = ltc2688_dac_code_read(st, chan->channel,
> > > LT2688_INPUT_A,
> > > > +					    val);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +
> > > > +		return IIO_VAL_INT;
> > > > +	case IIO_CHAN_INFO_OFFSET:
> > > > +		return ltc2688_offset_get(st, chan->channel, val);
> > > > +	case IIO_CHAN_INFO_SCALE:
> > > > +		*val2 = 16;
> > > > +		return ltc2688_scale_get(st, chan->channel, val);
> > >
> > > I'm not against functions returning the IIO_VAL_* like this, but if
> they
> > > are I expect the function to set val2 as well.
> > >
> > > I'd suggest return 0 on success and then do similar to what you
> have
> > > done for code_read above.
> >
> > Typically I do like to save lines of code when doable and readability is
> > not hurt which is the case. I'm not doing the same for the code_read
> > because that one is also used from the extended_info interface.
> That
> > said, I don't have strong feeling about this so I can do as you suggest.
> 
> Either option is fine for me.  Set val2 inside _scale_get() or return 0
> from that and then do a return IIO_VAL_INT_PLUS_MICRO here.
> 
> The particular combination at the moment is rather inconsistent as
> val, val2 and the return value should all come from the same 'source'
> whether it's here, or in _scale_get()

Already did as you first advised...

> >
> > > > +	case IIO_CHAN_INFO_CALIBBIAS:
> > > > +		ret = regmap_read(st->regmap,
> > > > +				  LTC2688_CMD_CH_OFFSET(chan-
> > > >channel), val);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +
> > > > +		/* Just 13 bits used. 2LSB ignored */
> > > > +		*val >>= 2;
> > > FIELD_GET() would get rid of need for the comment.
> > >
> > > > +		return IIO_VAL_INT;
> > > > +	case IIO_CHAN_INFO_CALIBSCALE:
> > > > +		ret = regmap_read(st->regmap,
> > > > +				  LTC2688_CMD_CH_GAIN(chan-
> > > >channel), val);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +
> > > > +		return IIO_VAL_INT;
> > > > +	default:
> > > > +		return -EINVAL;
> > > > +	}
> > > > +}
> 
> ...
> 
> > > > +
> > > > +static const char * const ltc2688_dither_phase[] = {
> > > > +	"0", "90", "180", "270",
> > > > +};
> > > > +
> > > > +static const struct iio_enum ltc2688_dither_phase_enum = {
> > > > +	.items = ltc2688_dither_phase,
> > > > +	.num_items = ARRAY_SIZE(ltc2688_dither_phase),
> > > > +	.set = ltc2688_set_dither_phase,
> > > > +	.get = ltc2688_get_dither_phase,
> > > > +};
> > > > +
> > > > +#define LTC2688_CHAN_EXT_INFO(_name, _what, _shared) {	\
> > > > +	.name = _name,					\
> > > > +	.read = ltc2688_read_ext,			\
> > > > +	.write = ltc2688_write_ext,			\
> > >
> > > I'm not really convinced big multiplexer functions are a good idea
> here.
> > > They seem to save little code and hurt readability a bit.
> >
> > I think this is a very common pattern seen in IIO and probably
> HWMON no?
> > Anyways, I'm ok with either way so I can just extend the macro to
> accept
> > the individual functions. I have to admit that in some cases (when
> locking is
> > required in some case blocks) I'm also not a big fan of these
> multiplexes
> > functions. And I think I'm calling individual functions in all the case
> blocks
> > anyways...
> 
> Common pattern, but not always a good idea.  All depends on how
> much
> common code there is.  In this case I don't think there is enough for it
> to make sense.

Agreed. 

> >
> > > > +	.private = (_what),				\
> > > > +	.shared = (_shared),				\
> > > > +}
> > > > +
> 
> ...
> 
> > > > +	 */
> > > > +	LTC2688_CHAN_EXT_INFO("dither_frequency",
> > > LTC2688_DITHER_FREQ,
> > > > +			      IIO_SEPARATE),
> > > > +	LTC2688_CHAN_EXT_INFO("dither_frequency_available",
> > > > +			      LTC2688_DITHER_FREQ_AVAIL,
> > > IIO_SEPARATE),
> > > > +	IIO_ENUM("dither_phase", IIO_SEPARATE,
> > > &ltc2688_dither_phase_enum),
> > > > +	IIO_ENUM_AVAILABLE("dither_phase", IIO_SEPARATE,
> > > > +			   &ltc2688_dither_phase_enum),
> > > > +	LTC2688_CHAN_EXT_INFO("dither_en",
> > > LTC2688_DITHER_TOGGLE_ENABLE,
> > > > +			      IIO_SEPARATE),
> > > > +	LTC2688_CHAN_EXT_INFO("powerdown",
> > > LTC2688_POWERDOWN, IIO_SEPARATE),
> > > > +	{}
> > > > +};
> > > > +
> > > > +static const struct iio_chan_spec_ext_info ltc2688_ext_info[] = {
> > > > +	LTC2688_CHAN_EXT_INFO("powerdown",
> > > LTC2688_POWERDOWN, IIO_SEPARATE),
> > > > +	{}
> > > > +};
> > > > +
> > >
> > > > +
> > > > +enum {
> > > > +	LTC2688_CHAN_TD_TGP1,
> > > > +	LTC2688_CHAN_TD_TGP2,
> > > > +	LTC2688_CHAN_TD_TGP3,
> > > > +	LTC2688_CHAN_TD_MAX
> > > > +};
> > >
> > > > +/* Helper struct to deal with dither channels binded to TGPx
> pins */
> > > > +struct ltc2688_dither_helper {
> > > > +	u8 chan[LTC2688_DAC_CHANNELS];
> > > > +	u8 n_chans;
> > > > +};
> > > > +
> > > bitmap perhaps given ordering doesn't matter (I think)
> > >
> >
> > Yeah, did not thought about it but I think it will look better with a
> bitmap yes.
> > Although I'm not sure if I will continue with this approach or make
> the clocks
> > property a per channel one (more on this in the cover letter).
> 
> I'm not sure how the per channel version will look so leaving this
> entirely
> up to you!
> 
> > >
> > > ...
> > >
> > > > +
> > > > +static int ltc2688_channel_config(struct ltc2688_state *st)
> > > > +{
> > > > +	struct fwnode_handle *fwnode = dev_fwnode(&st->spi-
> > > >dev), *child;
> > > > +	struct ltc2688_dither_helper tgp[LTC2688_CHAN_TD_MAX] =
> > > {0};
> > > > +	u32 reg, clk_input, val, mask, tmp[2];
> > > > +	unsigned long clk_msk = 0;
> > > > +	int ret, span;
> > > > +
> > >
> > > I think you need to sanity check you have a fwnode
> >
> > AFAICT, it's done by us already :)
> >
> >
> https://urldefense.com/v3/__https://elixir.bootlin.com/linux/latest/s
> ource/drivers/base/property.c*L741__;Iw!!A3Ni8CS0y2Y!sxVAp8P8XS
> 0R5sR447hKmVu7dK01fdsMfL6_c4woz7kDsbsI2fKLKfWopK4mOQ$
> 
> Ah.  Good point. Ignore that one then.
> 
> >
> > > > +	fwnode_for_each_available_child_node(fwnode, child) {
> > >
> > > I guess this is because of the whole
> > > device_for_each_available_child_node() not
> > > existing discussion that isn't resolved.
> >
> > exactly... I wanted the available option and this was the only way I
> > could find...
> >
> 
> Hmm. I need to revisit that discussion and see where we got to.
> 
> > >
> > > > +static bool ltc2688_reg_writable(struct device *dev, unsigned
> int
> > > reg)
> > > > +{
> > > > +	if (reg <= LTC2688_CMD_UPDATE_ALL && reg !=
> > > LTC2688_CMD_THERMAL_STAT)
> > >
> > > Isn't UPDATE_ALL the last register?  So how do you get higher than
> > > that?
> > > Definitely needs a comment if there is a reason that check is
> > > necessary.
> >
> > If you look at the commands table you see that on the write side
> > we jump from 0x76 to 0x78 (UPDATE_ALL=0x7c). 0x77 refers to
> > reading the thermal status reg which is not writable. Actually in the
> > end, as it's a read the command for reading the thermal status will
> > be 0xf7.
> 
> I'm lost on this.   My confusion is how you get >
> LTC2688_CMD_UPDATE_ALL
> Possibly that's what you are referring to with teh read command being
> 0xf...

Exactly... So I'm basically using the regmap read bit to get the read command
as in the datasheet. And that bit is still not set when we get into these callbacks
which means you can get 0x77 here which is < LTC2688_CMD_UPDATE_ALL but
still not writable...

I did had a comment for v2. Let's see if it's god enough :)

- Nuno Sá

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 57fb0f19ee08..90960e3d778f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11162,6 +11162,13 @@  S:	Maintained
 F:	Documentation/devicetree/bindings/iio/dac/lltc,ltc1660.yaml
 F:	drivers/iio/dac/ltc1660.c
 
+LTC2688 IIO DAC DRIVER
+M:	Nuno Sá <[e-mail]>
+L:	[e-mail]
+S:	Supported
+W:	http://ez.analog.com/community/linux-device-drivers
+F:	drivers/iio/dac/ltc2688.c
+
 LTC2947 HARDWARE MONITOR DRIVER
 M:	Nuno Sá <[e-mail]>
 L:	[e-mail]
diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
index b95619f18fa5..cf503fa93381 100644
--- a/drivers/iio/dac/Kconfig
+++ b/drivers/iio/dac/Kconfig
@@ -121,6 +121,17 @@  config AD5624R_SPI
 	  Say yes here to build support for Analog Devices AD5624R, AD5644R and
 	  AD5664R converters (DAC). This driver uses the common SPI interface.
 
+config LTC2688
+	tristate "Analog Devices LTC2688 DAC spi driver"
+	depends on SPI
+	select REGMAP
+	help
+	  Say yes here to build support for Analog Devices
+	  LTC2688 converters (DAC).
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ltc2688.
+
 config AD5686
 	tristate
 
diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
index 3c17246ee89b..5904edfb8ddc 100644
--- a/drivers/iio/dac/Makefile
+++ b/drivers/iio/dac/Makefile
@@ -34,6 +34,7 @@  obj-$(CONFIG_DS4424) += ds4424.o
 obj-$(CONFIG_LPC18XX_DAC) += lpc18xx_dac.o
 obj-$(CONFIG_LTC1660) += ltc1660.o
 obj-$(CONFIG_LTC2632) += ltc2632.o
+obj-$(CONFIG_LTC2688) += ltc2688.o
 obj-$(CONFIG_M62332) += m62332.o
 obj-$(CONFIG_MAX517) += max517.o
 obj-$(CONFIG_MAX5821) += max5821.o
diff --git a/drivers/iio/dac/ltc2688.c b/drivers/iio/dac/ltc2688.c
new file mode 100644
index 000000000000..895a0dbe35af
--- /dev/null
+++ b/drivers/iio/dac/ltc2688.c
@@ -0,0 +1,1081 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * LTC2688 16 channel, 16 bit Voltage Output SoftSpan DAC driver
+ *
+ * Copyright 2021 Analog Devices Inc.
+ */
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/limits.h>
+#include <linux/iio/iio.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/mutex.h>
+#include <linux/kernel.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+
+#define LTC2688_DAC_CHANNELS			16
+
+#define LTC2688_CMD_CH_CODE(x)			(0x00 + (x))
+#define LTC2688_CMD_CH_SETTING(x)		(0x10 + (x))
+#define LTC2688_CMD_CH_OFFSET(x)		(0X20 + (x))
+#define LTC2688_CMD_CH_GAIN(x)			(0x30 + (x))
+#define LTC2688_CMD_CH_CODE_UPDATE(x)		(0x40 + (x))
+
+#define LTC2688_CMD_CONFIG			0x70
+#define LTC2688_CMD_POWERDOWN			0x71
+#define LTC2688_CMD_A_B_SELECT			0x72
+#define LTC2688_CMD_SW_TOGGLE			0x73
+#define LTC2688_CMD_TOGGLE_DITHER_EN		0x74
+#define LTC2688_CMD_THERMAL_STAT		0x77
+#define LTC2688_CMD_UPDATE_ALL			0x7C
+#define LTC2688_CMD_NOOP			0xFF
+
+#define LTC2688_READ_OPERATION			0x80
+
+/* Channel Settings */
+#define LTC2688_CH_SPAN_MSK			GENMASK(2, 0)
+#define LTC2688_CH_OVERRANGE_MSK		BIT(3)
+#define LTC2688_CH_TD_SEL_MSK			GENMASK(5, 4)
+#define LTC2688_CH_DIT_PER_MSK			GENMASK(8, 6)
+#define LTC2688_CH_DIT_PH_MSK			GENMASK(10, 9)
+#define LTC2688_CH_MODE_MSK			BIT(11)
+
+/* Configuration register */
+#define LTC2688_CONFIG_RST			BIT(15)
+#define LTC2688_CONFIG_EXT_REF			BIT(1)
+
+#define LTC2688_DITHER_FREQ_AVAIL_N		5
+
+enum {
+	LTC2688_SPAN_RANGE_0V_5V,
+	LTC2688_SPAN_RANGE_0V_10V,
+	LTC2688_SPAN_RANGE_M5V_5V,
+	LTC2688_SPAN_RANGE_M10V_10V,
+	LTC2688_SPAN_RANGE_M15V_15V,
+	LTC2688_SPAN_RANGE_MAX
+};
+
+enum {
+	LTC2688_MODE_DEFAULT,
+	LTC2688_MODE_DITHER_TOGGLE,
+};
+
+struct ltc2688_chan {
+	long dither_frequency[LTC2688_DITHER_FREQ_AVAIL_N];
+	bool overrange;
+	bool toggle_chan;
+	u8 mode;
+};
+
+struct ltc2688_state {
+	struct spi_device *spi;
+	struct regmap *regmap;
+	struct regulator_bulk_data regulators[2];
+	struct ltc2688_chan channels[LTC2688_DAC_CHANNELS];
+	/* lock to protect against multiple access to the device and shared data */
+	struct mutex lock;
+	int vref;
+	/*
+	 * DMA (thus cache coherency maintenance) requires the
+	 * transfer buffers to live in their own cache lines.
+	 */
+	u8 tx_data[3] ____cacheline_aligned;
+	u8 rx_data[3];
+};
+
+static int ltc2688_spi_read(void *context, const void *reg, size_t reg_size,
+			    void *val, size_t val_size)
+{
+	struct ltc2688_state *st = context;
+	struct spi_transfer xfers[] = {
+		{
+			.tx_buf = reg,
+			.bits_per_word = 8,
+			/*
+			 * This means that @val will also be part of the
+			 * transfer as there's no pad bits. That's fine as these
+			 * bits are don't care for the device and we fill
+			 * @val with the proper value afterwards. Using regmap
+			 * pad bits to get reg_size right would just make the
+			 * write part more cumbersome than this...
+			 */
+			.len = reg_size + 2,
+			.cs_change = 1,
+		}, {
+			.tx_buf = st->tx_data,
+			.rx_buf = st->rx_data,
+			.bits_per_word = 8,
+			.len = 3,
+		},
+	};
+	int ret;
+
+	ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
+	if (ret)
+		return ret;
+
+	memcpy(val, &st->rx_data[1], val_size);
+
+	return 0;
+}
+
+static int ltc2688_spi_write(void *context, const void *data, size_t count)
+{
+	struct ltc2688_state *st = context;
+
+	return spi_write(st->spi, data, count);
+}
+
+static int ltc2688_span_get(const struct ltc2688_state *st, int c)
+{
+	int ret, reg, span;
+
+	ret = regmap_read(st->regmap, LTC2688_CMD_CH_SETTING(c), &reg);
+	if (ret)
+		return ret;
+
+	span = FIELD_GET(LTC2688_CH_SPAN_MSK, reg);
+	/* sanity check to make sure we don't get any weird value from the HW */
+	if (span >= LTC2688_SPAN_RANGE_MAX)
+		return -EIO;
+
+	return span;
+}
+
+static const int ltc2688_span_helper[LTC2688_SPAN_RANGE_MAX][2] = {
+	{0, 5000}, {0, 10000}, {-5000, 5000}, {-10000, 10000}, {-15000, 15000},
+};
+
+static int ltc2688_scale_get(const struct ltc2688_state *st, int c, int *val)
+{
+	const struct ltc2688_chan *chan = &st->channels[c];
+	int span, fs;
+
+	span = ltc2688_span_get(st, c);
+	if (span < 0)
+		return span;
+
+	fs = ltc2688_span_helper[span][1] - ltc2688_span_helper[span][0];
+	if (chan->overrange)
+		fs = mult_frac(fs, 105, 100);
+
+	*val = DIV_ROUND_CLOSEST(fs * st->vref, 4096);
+
+	return IIO_VAL_FRACTIONAL_LOG2;
+}
+
+static int ltc2688_offset_get(const struct ltc2688_state *st, int c, int *val)
+{
+	int span;
+
+	span = ltc2688_span_get(st, c);
+	if (span < 0)
+		return span;
+
+	if (ltc2688_span_helper[span][0] < 0)
+		*val = -32768;
+	else
+		*val = 0;
+
+	return IIO_VAL_INT;
+}
+
+enum {
+	LT2688_INPUT_A,
+	LT2688_INPUT_B,
+};
+
+static int ltc2688_dac_code_write(struct ltc2688_state *st, u32 chan, u32 input,
+				  u16 code)
+{
+	struct ltc2688_chan *c = &st->channels[chan];
+	int ret, reg;
+
+	/* 2 LSBs set to 0 if writing dither amplitude */
+	if (!c->toggle_chan && input == LT2688_INPUT_B) {
+		if (code > GENMASK(13, 0))
+			return -EINVAL;
+
+		code <<= 2;
+	}
+
+	mutex_lock(&st->lock);
+	/* select the correct input register to read from */
+	ret = regmap_update_bits(st->regmap, LTC2688_CMD_A_B_SELECT, BIT(chan),
+				 input << chan);
+	if (ret)
+		goto unlock;
+
+	/*
+	 * If in dither/toggle mode the dac should be updated by an
+	 * external signal (or sw toggle) and not here.
+	 */
+	if (c->mode == LTC2688_MODE_DEFAULT)
+		reg = LTC2688_CMD_CH_CODE_UPDATE(chan);
+	else
+		reg = LTC2688_CMD_CH_CODE(chan);
+
+	ret = regmap_write(st->regmap, reg, code);
+unlock:
+	mutex_unlock(&st->lock);
+	return ret;
+}
+
+static int ltc2688_dac_code_read(struct ltc2688_state *st, u32 chan, u32 input,
+				 u32 *code)
+{
+	struct ltc2688_chan *c = &st->channels[chan];
+	int ret;
+
+	mutex_lock(&st->lock);
+	ret = regmap_update_bits(st->regmap, LTC2688_CMD_A_B_SELECT, BIT(chan),
+				 input << chan);
+	if (ret)
+		goto unlock;
+
+	ret = regmap_read(st->regmap, LTC2688_CMD_CH_CODE(chan), code);
+unlock:
+	mutex_unlock(&st->lock);
+
+	if (!c->toggle_chan && input == LT2688_INPUT_B)
+		*code >>= 2;
+
+	return ret;
+}
+
+static const int ltc2688_raw_range[] = {0, 1, U16_MAX};
+
+static int ltc2688_read_avail(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan,
+			      const int **vals, int *type, int *length,
+			      long m)
+{
+	switch (m) {
+	case IIO_CHAN_INFO_RAW:
+		*vals = ltc2688_raw_range;
+		*type = IIO_VAL_INT;
+		return IIO_AVAIL_RANGE;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ltc2688_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan, int *val,
+			    int *val2, long m)
+{
+	struct ltc2688_state *st = iio_priv(indio_dev);
+	int ret;
+
+	switch (m) {
+	case IIO_CHAN_INFO_RAW:
+		ret = ltc2688_dac_code_read(st, chan->channel, LT2688_INPUT_A,
+					    val);
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_OFFSET:
+		return ltc2688_offset_get(st, chan->channel, val);
+	case IIO_CHAN_INFO_SCALE:
+		*val2 = 16;
+		return ltc2688_scale_get(st, chan->channel, val);
+	case IIO_CHAN_INFO_CALIBBIAS:
+		ret = regmap_read(st->regmap,
+				  LTC2688_CMD_CH_OFFSET(chan->channel), val);
+		if (ret)
+			return ret;
+
+		/* Just 13 bits used. 2LSB ignored */
+		*val >>= 2;
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_CALIBSCALE:
+		ret = regmap_read(st->regmap,
+				  LTC2688_CMD_CH_GAIN(chan->channel), val);
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ltc2688_write_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan, int val,
+			     int val2, long mask)
+{
+	struct ltc2688_state *st = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		if (val > U16_MAX || val < 0)
+			return -EINVAL;
+
+		return ltc2688_dac_code_write(st, chan->channel, LT2688_INPUT_A,
+					      val);
+	case IIO_CHAN_INFO_CALIBBIAS:
+		if (val > GENMASK(13, 0))
+			return -EINVAL;
+
+		return regmap_write(st->regmap,
+				    LTC2688_CMD_CH_OFFSET(chan->channel),
+				    val << 2);
+	case IIO_CHAN_INFO_CALIBSCALE:
+		return regmap_write(st->regmap,
+				    LTC2688_CMD_CH_GAIN(chan->channel), val);
+	default:
+		return -EINVAL;
+	}
+}
+
+enum {
+	LTC2688_DITHER_TOGGLE_ENABLE,
+	LTC2688_POWERDOWN,
+	LTC2688_INPUT_B,
+	LTC2688_INPUT_B_AVAIL,
+	LTC2688_SW_TOGGLE,
+	LTC2688_DITHER_FREQ,
+	LTC2688_DITHER_FREQ_AVAIL,
+};
+
+static int ltc2688_dither_toggle_set(struct ltc2688_state *st, u32 chan, bool en)
+{
+	struct ltc2688_chan *c = &st->channels[chan];
+	int ret;
+
+	mutex_lock(&st->lock);
+	ret = regmap_update_bits(st->regmap, LTC2688_CMD_TOGGLE_DITHER_EN,
+				 BIT(chan), en << chan);
+	if (ret)
+		goto unlock;
+
+	c->mode = en ? LTC2688_MODE_DITHER_TOGGLE : LTC2688_MODE_DEFAULT;
+unlock:
+	mutex_unlock(&st->lock);
+
+	return ret;
+}
+
+static ssize_t ltc2688_reg_bool_get(const struct ltc2688_state *st, u32 chan,
+				    u32 reg, char *buf)
+{
+	int ret;
+	u32 val;
+
+	ret = regmap_read(st->regmap, reg, &val);
+	if (ret)
+		return ret;
+
+	return sysfs_emit(buf, "%u\n", !!(val & BIT(chan)));
+}
+
+static ssize_t ltc2688_reg_bool_set(const struct ltc2688_state *st, u32 chan,
+				    u32 reg, const char *buf)
+{
+	int ret;
+	bool en;
+
+	ret = kstrtobool(buf, &en);
+	if (ret)
+		return ret;
+
+	return regmap_update_bits(st->regmap, reg, BIT(chan), en << chan);
+}
+
+static ssize_t ltc2688_dither_freq_avail(const struct ltc2688_state *st,
+					 u32 chan, char *buf)
+{
+	const struct ltc2688_chan *c = &st->channels[chan];
+	int sz = 0;
+	u32 f;
+
+	for (f = 0; f < ARRAY_SIZE(c->dither_frequency); f++)
+		sz += sysfs_emit_at(buf, sz, "%ld ", c->dither_frequency[f]);
+
+	buf[sz - 1] = '\n';
+
+	return sz;
+}
+
+static ssize_t ltc2688_dither_freq_get(const struct ltc2688_state *st, u32 chan,
+				       char *buf)
+{
+	const struct ltc2688_chan *c = &st->channels[chan];
+	u32 reg, freq;
+	int ret;
+
+	ret = regmap_read(st->regmap, LTC2688_CMD_CH_SETTING(chan), &reg);
+	if (ret)
+		return ret;
+
+	freq = FIELD_GET(LTC2688_CH_DIT_PER_MSK, reg);
+	if (freq >= ARRAY_SIZE(c->dither_frequency))
+		return -EIO;
+
+	return sysfs_emit(buf, "%ld\n", c->dither_frequency[freq]);
+}
+
+static int ltc2688_dither_freq_set(const struct ltc2688_state *st, u32 chan,
+				   const char *buf)
+{
+	const struct ltc2688_chan *c = &st->channels[chan];
+	long val;
+	u32 freq;
+	int ret;
+
+	ret = kstrtol(buf, 10, &val);
+	if (ret)
+		return ret;
+
+	for (freq = 0; freq < ARRAY_SIZE(c->dither_frequency); freq++) {
+		if (val == c->dither_frequency[freq])
+			break;
+	}
+
+	if (freq == ARRAY_SIZE(c->dither_frequency))
+		return -EINVAL;
+
+	return regmap_update_bits(st->regmap,
+				  LTC2688_CMD_CH_SETTING(chan),
+				  LTC2688_CH_DIT_PER_MSK,
+				  FIELD_PREP(LTC2688_CH_DIT_PER_MSK, freq));
+}
+
+static ssize_t ltc2688_read_ext(struct iio_dev *indio_dev,
+				uintptr_t private,
+				const struct iio_chan_spec *chan,
+				char *buf)
+{
+	struct ltc2688_state *st = iio_priv(indio_dev);
+	u32 val;
+	int ret;
+
+	switch (private) {
+	case LTC2688_DITHER_TOGGLE_ENABLE:
+		return ltc2688_reg_bool_get(st, chan->channel,
+					    LTC2688_CMD_TOGGLE_DITHER_EN, buf);
+	case LTC2688_POWERDOWN:
+		return ltc2688_reg_bool_get(st, chan->channel,
+					    LTC2688_CMD_POWERDOWN, buf);
+	case LTC2688_INPUT_B:
+		ret = ltc2688_dac_code_read(st, chan->channel, LT2688_INPUT_B,
+					    &val);
+		if (ret)
+			return ret;
+
+		return sysfs_emit(buf, "%u\n", val);
+	case LTC2688_INPUT_B_AVAIL:
+		/* dither amplitude has 1/4 of the span */
+		return sysfs_emit(buf, "[%u %u %u]\n", ltc2688_raw_range[0],
+				  ltc2688_raw_range[1],
+				  ltc2688_raw_range[2] / 4);
+	case LTC2688_SW_TOGGLE:
+		return ltc2688_reg_bool_get(st, chan->channel,
+					    LTC2688_CMD_SW_TOGGLE, buf);
+	case LTC2688_DITHER_FREQ:
+		return ltc2688_dither_freq_get(st, chan->channel, buf);
+	case LTC2688_DITHER_FREQ_AVAIL:
+		return ltc2688_dither_freq_avail(st, chan->channel, buf);
+	default:
+		return -EINVAL;
+	}
+}
+
+static ssize_t ltc2688_write_ext(struct iio_dev *indio_dev,
+				 uintptr_t private,
+				 const struct iio_chan_spec *chan,
+				 const char *buf, size_t len)
+{
+	struct ltc2688_state *st = iio_priv(indio_dev);
+	u16 val;
+	int ret;
+	bool en;
+
+	switch (private) {
+	case LTC2688_DITHER_TOGGLE_ENABLE:
+		ret = kstrtobool(buf, &en);
+		if (ret)
+			return ret;
+
+		ret = ltc2688_dither_toggle_set(st, chan->channel, en);
+		if (ret)
+			return ret;
+
+		return len;
+	case LTC2688_POWERDOWN:
+		ret = ltc2688_reg_bool_set(st, chan->channel,
+					   LTC2688_CMD_POWERDOWN, buf);
+		if (ret)
+			return ret;
+
+		return len;
+	case LTC2688_INPUT_B:
+		ret = kstrtou16(buf, 10, &val);
+		if (ret)
+			return ret;
+
+		ret = ltc2688_dac_code_write(st, chan->channel, LT2688_INPUT_B,
+					     val);
+		if (ret)
+			return ret;
+
+		return len;
+	case LTC2688_SW_TOGGLE:
+		ret = ltc2688_reg_bool_set(st, chan->channel,
+					   LTC2688_CMD_SW_TOGGLE, buf);
+		if (ret)
+			return ret;
+
+		return len;
+	case LTC2688_DITHER_FREQ:
+		ret = ltc2688_dither_freq_set(st, chan->channel, buf);
+		if (ret)
+			return ret;
+
+		return len;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ltc2688_get_dither_phase(struct iio_dev *dev,
+				    const struct iio_chan_spec *chan)
+{
+	struct ltc2688_state *st = iio_priv(dev);
+	int ret, regval;
+
+	ret = regmap_read(st->regmap, LTC2688_CMD_CH_SETTING(chan->channel),
+			  &regval);
+	if (ret)
+		return ret;
+
+	return FIELD_GET(LTC2688_CH_DIT_PH_MSK, regval);
+}
+
+static int ltc2688_set_dither_phase(struct iio_dev *dev,
+				    const struct iio_chan_spec *chan,
+				    unsigned int phase)
+{
+	struct ltc2688_state *st = iio_priv(dev);
+
+	return regmap_update_bits(st->regmap,
+				  LTC2688_CMD_CH_SETTING(chan->channel),
+				  LTC2688_CH_DIT_PH_MSK,
+				  FIELD_PREP(LTC2688_CH_DIT_PH_MSK, phase));
+}
+
+static int ltc2688_reg_access(struct iio_dev *indio_dev,
+			      unsigned int reg,
+			      unsigned int writeval,
+			      unsigned int *readval)
+{
+	struct ltc2688_state *st = iio_priv(indio_dev);
+
+	if (readval)
+		return regmap_read(st->regmap, reg, readval);
+	else
+		return regmap_write(st->regmap, reg, writeval);
+}
+
+static const char * const ltc2688_dither_phase[] = {
+	"0", "90", "180", "270",
+};
+
+static const struct iio_enum ltc2688_dither_phase_enum = {
+	.items = ltc2688_dither_phase,
+	.num_items = ARRAY_SIZE(ltc2688_dither_phase),
+	.set = ltc2688_set_dither_phase,
+	.get = ltc2688_get_dither_phase,
+};
+
+#define LTC2688_CHAN_EXT_INFO(_name, _what, _shared) {	\
+	.name = _name,					\
+	.read = ltc2688_read_ext,			\
+	.write = ltc2688_write_ext,			\
+	.private = (_what),				\
+	.shared = (_shared),				\
+}
+
+/*
+ * For toggle mode we only expose the symbol attr (sw_toggle) in case a TGPx is
+ * not provided in dts.
+ */
+#define LTC2688_CHAN_TOGGLE(t, name) ({							\
+	static const struct iio_chan_spec_ext_info t ## _ext_info[] = {			\
+		LTC2688_CHAN_EXT_INFO("raw1", LTC2688_INPUT_B, IIO_SEPARATE),		\
+		LTC2688_CHAN_EXT_INFO("toggle_en", LTC2688_DITHER_TOGGLE_ENABLE,	\
+				      IIO_SEPARATE),					\
+		LTC2688_CHAN_EXT_INFO("powerdown", LTC2688_POWERDOWN, IIO_SEPARATE),	\
+		LTC2688_CHAN_EXT_INFO(name, LTC2688_SW_TOGGLE, IIO_SEPARATE),		\
+		{}									\
+	};										\
+	t ## _ext_info;									\
+})
+
+static struct iio_chan_spec_ext_info ltc2688_dither_ext_info[] = {
+	LTC2688_CHAN_EXT_INFO("dither_raw", LTC2688_INPUT_B, IIO_SEPARATE),
+	LTC2688_CHAN_EXT_INFO("dither_raw_available", LTC2688_INPUT_B_AVAIL,
+			      IIO_SEPARATE),
+	/*
+	 * Not IIO_ENUM because the available freq needs to be computed at
+	 * probe. We could still use it, but it didn't felt much right.
+	 *
+	 */
+	LTC2688_CHAN_EXT_INFO("dither_frequency", LTC2688_DITHER_FREQ,
+			      IIO_SEPARATE),
+	LTC2688_CHAN_EXT_INFO("dither_frequency_available",
+			      LTC2688_DITHER_FREQ_AVAIL, IIO_SEPARATE),
+	IIO_ENUM("dither_phase", IIO_SEPARATE, &ltc2688_dither_phase_enum),
+	IIO_ENUM_AVAILABLE("dither_phase", IIO_SEPARATE,
+			   &ltc2688_dither_phase_enum),
+	LTC2688_CHAN_EXT_INFO("dither_en", LTC2688_DITHER_TOGGLE_ENABLE,
+			      IIO_SEPARATE),
+	LTC2688_CHAN_EXT_INFO("powerdown", LTC2688_POWERDOWN, IIO_SEPARATE),
+	{}
+};
+
+static const struct iio_chan_spec_ext_info ltc2688_ext_info[] = {
+	LTC2688_CHAN_EXT_INFO("powerdown", LTC2688_POWERDOWN, IIO_SEPARATE),
+	{}
+};
+
+#define LTC2688_CHANNEL(_chan) {					\
+	.type = IIO_VOLTAGE,						\
+	.indexed = 1,							\
+	.output = 1,							\
+	.channel = (_chan),						\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_CALIBSCALE) |		\
+		BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET) |	\
+		BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_RAW),	\
+	.info_mask_separate_available = BIT(IIO_CHAN_INFO_RAW),		\
+	.ext_info = ltc2688_ext_info					\
+}
+
+struct iio_chan_spec ltc2688_channels[] = {
+	LTC2688_CHANNEL(0),
+	LTC2688_CHANNEL(1),
+	LTC2688_CHANNEL(2),
+	LTC2688_CHANNEL(3),
+	LTC2688_CHANNEL(4),
+	LTC2688_CHANNEL(5),
+	LTC2688_CHANNEL(6),
+	LTC2688_CHANNEL(7),
+	LTC2688_CHANNEL(8),
+	LTC2688_CHANNEL(9),
+	LTC2688_CHANNEL(10),
+	LTC2688_CHANNEL(11),
+	LTC2688_CHANNEL(12),
+	LTC2688_CHANNEL(13),
+	LTC2688_CHANNEL(14),
+	LTC2688_CHANNEL(15),
+};
+
+enum {
+	LTC2688_CHAN_TD_TGP1,
+	LTC2688_CHAN_TD_TGP2,
+	LTC2688_CHAN_TD_TGP3,
+	LTC2688_CHAN_TD_MAX
+};
+
+static void ltc2688_clk_disable(void *clk)
+{
+	clk_disable_unprepare(clk);
+}
+
+/* Helper struct to deal with dither channels binded to TGPx pins */
+struct ltc2688_dither_helper {
+	u8 chan[LTC2688_DAC_CHANNELS];
+	u8 n_chans;
+};
+
+static const char * const ltc2688_clk_names[LTC2688_CHAN_TD_MAX] = {
+	"TGP1", "TGP2", "TGP3",
+};
+
+static const int ltc2688_period[LTC2688_DITHER_FREQ_AVAIL] = {
+	4, 8, 16, 32, 64,
+};
+
+static void ltc2688_dither_compute_freq_avail(struct ltc2688_state *st,
+					      const struct ltc2688_dither_helper *tgp,
+					      unsigned long rate)
+{
+	u32 e;
+
+	for (e = 0; e < tgp->n_chans; e++) {
+		int c = tgp->chan[e];
+		struct ltc2688_chan *chan = &st->channels[c];
+		u32 f;
+
+		for (f = 0; f < ARRAY_SIZE(chan->dither_frequency); f++)
+			chan->dither_frequency[f] = DIV_ROUND_CLOSEST(rate, ltc2688_period[f]);
+	}
+}
+
+static int ltc2688_tgp_setup(struct ltc2688_state *st, long clk_mask,
+			     const struct ltc2688_dither_helper *tgp)
+{
+	int ret, bit;
+
+	for_each_set_bit(bit, &clk_mask, LTC2688_CHAN_TD_MAX) {
+		struct clk *clk;
+		unsigned long rate;
+
+		clk = devm_clk_get(&st->spi->dev, ltc2688_clk_names[bit]);
+		if (IS_ERR(clk))
+			return dev_err_probe(&st->spi->dev, PTR_ERR(clk),
+					     "failed to get clk: %s\n",
+					     ltc2688_clk_names[bit]);
+
+		ret = clk_prepare_enable(clk);
+		if (ret)
+			return dev_err_probe(&st->spi->dev, ret,
+					     "failed to enable clk: %s\n",
+					     ltc2688_clk_names[bit]);
+
+		ret = devm_add_action_or_reset(&st->spi->dev,
+					       ltc2688_clk_disable, clk);
+		if (ret)
+			return ret;
+
+		/* this will only be non zero for dither channels */
+		if (!tgp[bit].n_chans)
+			continue;
+
+		rate = clk_get_rate(clk);
+		ltc2688_dither_compute_freq_avail(st, &tgp[bit], rate);
+	}
+
+	return 0;
+}
+
+static int ltc2688_span_lookup(const struct ltc2688_state *st, int min, int max)
+{
+	u32 span;
+
+	for (span = 0; span < ARRAY_SIZE(ltc2688_span_helper); span++) {
+		if (min == ltc2688_span_helper[span][0] &&
+		    max == ltc2688_span_helper[span][1])
+			return span;
+	}
+
+	return -EINVAL;
+}
+
+static int ltc2688_channel_config(struct ltc2688_state *st)
+{
+	struct fwnode_handle *fwnode = dev_fwnode(&st->spi->dev), *child;
+	struct ltc2688_dither_helper tgp[LTC2688_CHAN_TD_MAX] = {0};
+	u32 reg, clk_input, val, mask, tmp[2];
+	unsigned long clk_msk = 0;
+	int ret, span;
+
+	fwnode_for_each_available_child_node(fwnode, child) {
+		struct ltc2688_chan *chan;
+
+		ret = fwnode_property_read_u32(child, "reg", &reg);
+		if (ret) {
+			fwnode_handle_put(child);
+			return dev_err_probe(&st->spi->dev, ret,
+					     "Failed to get reg property\n");
+		}
+
+		if (reg >= LTC2688_DAC_CHANNELS) {
+			fwnode_handle_put(child);
+			return dev_err_probe(&st->spi->dev, -EINVAL,
+					     "reg bigger than: %d\n",
+					     LTC2688_DAC_CHANNELS);
+		}
+
+		chan = &st->channels[reg];
+		if (fwnode_property_read_bool(child, "adi,toggle-mode")) {
+			chan->toggle_chan = true;
+			/* assume sw toggle ABI */
+			ltc2688_channels[reg].ext_info = LTC2688_CHAN_TOGGLE(__s, "symbol");
+		}
+
+		ret = fwnode_property_read_u32_array(child, "adi,output-range-millivolt",
+						     tmp, ARRAY_SIZE(tmp));
+		if (!ret) {
+			span = ltc2688_span_lookup(st, tmp[0], tmp[1]);
+			if (span < 0)
+				return dev_err_probe(&st->spi->dev, -EINVAL,
+						     "output range not valid:[%d %d]\n",
+						     tmp[0], tmp[1]);
+
+			mask |= LTC2688_CH_SPAN_MSK;
+			val |= FIELD_PREP(LTC2688_CH_SPAN_MSK, span);
+		}
+
+		ret = fwnode_property_read_u32(child, "adi,toggle-dither-input",
+					       &clk_input);
+		if (!ret) {
+			int cur_chan = tgp[clk_input].n_chans;
+
+			if (clk_input > LTC2688_CHAN_TD_TGP3) {
+				fwnode_handle_put(child);
+				return dev_err_probe(&st->spi->dev, -EINVAL,
+						     "toggle-dither-input inv value(%d)\n",
+						     clk_input);
+			}
+
+			mask |= LTC2688_CH_TD_SEL_MSK;
+			/*
+			 * 0 means software toggle which is the default mode.
+			 * Hence the +1.
+			 */
+			val |= FIELD_PREP(LTC2688_CH_TD_SEL_MSK, clk_input + 1);
+			clk_msk |= BIT(clk_input);
+			/*
+			 * If a TGPx is given, we automatically assume a dither
+			 * capable channel (unless toggle is already enabled).
+			 * Hence, we prepar our TGPx <-> channel map to make it
+			 * easier to handle the clocks and available frequency
+			 * calculations... On top of this we just set here the
+			 * dither bit in the channel settings. It won't have any
+			 * effect until the global toggle/dither bit is enabled.
+			 */
+			if (!chan->toggle_chan) {
+				tgp[clk_input].chan[cur_chan] = reg;
+				tgp[clk_input].n_chans++;
+				mask |= LTC2688_CH_MODE_MSK;
+				val |= FIELD_PREP(LTC2688_CH_MODE_MSK, 1);
+				ltc2688_channels[reg].ext_info = ltc2688_dither_ext_info;
+			} else {
+				/* wait, no sw toggle after all */
+				ltc2688_channels[reg].ext_info = LTC2688_CHAN_TOGGLE(__no_s, NULL);
+			}
+		}
+
+		if (fwnode_property_read_bool(child, "adi,overrange")) {
+			chan->overrange = true;
+			val |= LTC2688_CH_OVERRANGE_MSK;
+			mask |= BIT(3);
+		}
+
+		if (!mask)
+			continue;
+
+		ret = regmap_update_bits(st->regmap,
+					 LTC2688_CMD_CH_SETTING(reg), mask,
+					 val);
+		if (ret) {
+			fwnode_handle_put(child);
+			return dev_err_probe(&st->spi->dev, -EINVAL,
+					     "failed to set chan settings\n");
+		}
+
+		mask = 0;
+		val = 0;
+	}
+
+	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.
+	 */
+	gpio = devm_gpiod_get_optional(&st->spi->dev, "reset", GPIOD_OUT_HIGH);
+	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));
+}
+
+static void ltc2688_bulk_disable(void *data)
+{
+	struct ltc2688_state *st = data;
+
+	regulator_bulk_disable(ARRAY_SIZE(st->regulators), st->regulators);
+}
+
+static void ltc2688_disable_regulator(void *regulator)
+{
+	regulator_disable(regulator);
+}
+
+static bool ltc2688_reg_readable(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case LTC2688_CMD_CH_CODE(0) ... LTC2688_CMD_CH_GAIN(15):
+		return true;
+	case LTC2688_CMD_CONFIG ... LTC2688_CMD_THERMAL_STAT:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool ltc2688_reg_writable(struct device *dev, unsigned int reg)
+{
+	if (reg <= LTC2688_CMD_UPDATE_ALL && reg != LTC2688_CMD_THERMAL_STAT)
+		return true;
+
+	return false;
+}
+
+struct regmap_bus ltc2688_regmap_bus = {
+	.read = ltc2688_spi_read,
+	.write = ltc2688_spi_write,
+	.read_flag_mask = LTC2688_READ_OPERATION,
+	.reg_format_endian_default = REGMAP_ENDIAN_BIG,
+	.val_format_endian_default = REGMAP_ENDIAN_BIG
+};
+
+static const struct regmap_config ltc2688_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 16,
+	.readable_reg = ltc2688_reg_readable,
+	.writeable_reg = ltc2688_reg_writable,
+	/* ignoring the no op command */
+	.max_register = LTC2688_CMD_UPDATE_ALL
+};
+
+static const struct iio_info ltc2688_info = {
+	.write_raw = ltc2688_write_raw,
+	.read_raw = ltc2688_read_raw,
+	.read_avail = ltc2688_read_avail,
+	.debugfs_reg_access = ltc2688_reg_access,
+};
+
+static int ltc2688_probe(struct spi_device *spi)
+{
+	struct ltc2688_state *st;
+	struct iio_dev *indio_dev;
+	struct regulator *vref_reg;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+	st->spi = spi;
+	/* Just this once. No need to di it in every regmap read */
+	st->tx_data[0] = LTC2688_CMD_NOOP;
+	mutex_init(&st->lock);
+
+	st->regmap = devm_regmap_init(&spi->dev, &ltc2688_regmap_bus, st,
+				      &ltc2688_regmap_config);
+	if (IS_ERR(st->regmap))
+		return dev_err_probe(&spi->dev, PTR_ERR(st->regmap),
+				     "Failed to init regmap");
+
+	st->regulators[0].supply = "vcc";
+	st->regulators[1].supply = "iovcc";
+	ret = devm_regulator_bulk_get(&spi->dev, ARRAY_SIZE(st->regulators),
+				      st->regulators);
+	if (ret)
+		return dev_err_probe(&spi->dev, ret, "Failed to get regulators\n");
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(st->regulators), st->regulators);
+	if (ret)
+		return dev_err_probe(&spi->dev, ret,
+				     "Failed to enable regulators\n");
+
+	ret = devm_add_action_or_reset(&spi->dev, ltc2688_bulk_disable, st);
+	if (ret)
+		return ret;
+
+	vref_reg = devm_regulator_get_optional(&spi->dev, "vref");
+	if (!IS_ERR(vref_reg)) {
+		ret = regulator_enable(vref_reg);
+		if (ret)
+			return dev_err_probe(&spi->dev, ret,
+					     "Failed to enable vref regulators\n");
+
+		ret = devm_add_action_or_reset(&spi->dev,
+					       ltc2688_disable_regulator,
+					       vref_reg);
+		if (ret)
+			return ret;
+
+		ret = regulator_get_voltage(vref_reg);
+		if (ret < 0)
+			return dev_err_probe(&spi->dev, ret,
+					     "Failed to get vref\n");
+
+		st->vref = ret / 1000;
+	} else {
+		if (PTR_ERR(vref_reg) != -ENODEV)
+			return dev_err_probe(&spi->dev, PTR_ERR(vref_reg),
+					     "Failed to get vref regulator");
+
+		vref_reg = NULL;
+		/* internal reference */
+		st->vref = 4096;
+	}
+
+	indio_dev->name = "ltc2688";
+	indio_dev->info = &ltc2688_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = ltc2688_channels;
+	indio_dev->num_channels = ARRAY_SIZE(ltc2688_channels);
+
+	ret = ltc2688_setup(st, vref_reg);
+	if (ret < 0)
+		return ret;
+
+	return devm_iio_device_register(&st->spi->dev, indio_dev);
+}
+
+static const struct of_device_id ltc2688_of_id[] = {
+	{ .compatible = "adi,ltc2688" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, ltc2688_of_id);
+
+static const struct spi_device_id ltc2688_id[] = {
+	{ "ltc2688" },
+	{}
+};
+MODULE_DEVICE_TABLE(spi, ltc2688_id);
+
+static struct spi_driver ltc2688_driver = {
+	.driver = {
+		.name = "ltc2688",
+		.of_match_table = ltc2688_of_id,
+	},
+	.probe = ltc2688_probe,
+	.id_table = ltc2688_id,
+};
+module_spi_driver(ltc2688_driver);
+
+MODULE_AUTHOR("Nuno Sá <[e-mail]>");
+MODULE_DESCRIPTION("Analog Devices LTC2688 DAC");
+MODULE_LICENSE("GPL");