[v2,0/3] Add support for LTC2688

The ABI defined for this driver has some subtleties that were previously
discussed in this RFC [1]. This might not be the final state but,
hopefully, we are close to it:

toggle mode channels:

 * out_voltageY_toggle_en
 * out_voltageY_raw0
 * out_voltageY_raw1
 * out_voltageY_symbol

dither mode channels:

 * out_voltageY_dither_en
 * out_voltageY_dither_raw
 * out_voltageY_dither_raw_available
 * out_voltageY_dither_offset
 * out_voltageY_dither_frequency
 * out_voltageY_dither_frequency_available
 * out_voltageY_dither_phase
 * out_voltageY_dither_phase_available

Default channels won't have any of the above ABIs. A channel is toggle
capable if the devicetree 'adi,toggle-mode' flag is set. For dither, the
assumption is more silent. If 'adi,toggle-mode' is not given and a
channel is associated with a TGPx pin through 'adi,toggle-dither-input',
then the channel is assumed to be dither capable (there's no point in
having a dither capable channel without an input clock).

changes in v2:

 ltc2688:
  * Use local buffer for regmap read. Do not assume that reg is part of
larger buffer;
  * Renamed GPIO to "clr" so that is consistent with the datasheet;
  * Renamed 'mask' and 'm' to info. 'mask' is a thing from the past;
  * Removed 'LTC2688_CHAN_TOGGLE()' and defined to static ext_info arrays;
  * Use 'regmap_set_bits' to set external ref;
  * Use FIELD_{PREP|GET} for dither amplitude and channel calibbias where
only 13bits are used;
  * Use 'regmap_write()' instead of update_bits for channels settings;
  * Init 'val' at the beginning of the channel configuration loop
(and drop mask);
  * Comment 'ltc2688_reg_writable()' to account for the special condition;
  * Kmemdup default channels so that it can be safely changed per probed
device;
  * Replace extended info multiplexer functions by individual functions;
  * Use raw0 ABI for toggle channels;
  * Use dedicated offset ABI for dither channels;
  * Misc changes (spell fixes, blank lines...);
  * Have a clock property per channel. Note that we this I moved to OF
since we now have to use 'devm_get_clk_from_child()' which is using
device_node. Note that I could use 'to_of_node()' but mixing of.h and
property.h does not feel like a good idea.

 ABI:
  * Added out_voltageY_raw0 ABI for toggle mode;
  * Added out_voltageY_dither_offset.

 Bindings:
  * Use standard microvolt unit;
  * Added constrains for adi,output-range-microvolt and removed negative
values from the dts example;
  * Moved clocks to the channel object;
  * Dropped clock-names;
  * Add a dependency between 'adi,toggle-dither-input' and 'clocks'.
 
[1]: https://marc.info/?l=linux-iio&m=163662843603265&w=2

Nuno Sá (3):
  iio: dac: add support for ltc2688
  iio: ABI: add ABI file for the LTC2688 DAC
  dt-bindings: iio: Add ltc2688 documentation

 .../ABI/testing/sysfs-bus-iio-dac-ltc2688     |   80 ++
 .../bindings/iio/dac/adi,ltc2688.yaml         |  147 +++
 MAINTAINERS                                   |    9 +
 drivers/iio/dac/Kconfig                       |   11 +
 drivers/iio/dac/Makefile                      |    1 +
 drivers/iio/dac/ltc2688.c                     | 1070 +++++++++++++++++
 6 files changed, 1318 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2688
 create mode 100644 Documentation/devicetree/bindings/iio/dac/adi,ltc2688.yaml
 create mode 100644 drivers/iio/dac/ltc2688.c

On Sat, 15 Jan 2022 10:27:03 +0100
Nuno Sá <nuno.sa@analog.com> 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á <nuno.sa@analog.com>

A few minor additions inline.

In particular I think we can work around that lack of
device_for_each_available_child_node() issue and use generic fw propreties.
rather than of ones.  That way we can separate things from the question of
how to 'fix' that issue.

One thing I'm not sure on is the phase units. Right now I think you are
exposing just the raw register value whereas I think that needs converting
to radians.

Jonathan



...

> +static int ltc2688_channel_config(struct ltc2688_state *st)
> +{
> +	struct device *dev = &st->spi->dev;
> +	struct device_node *child;
> +	u32 reg, clk_input, val, tmp[2];
> +	int ret, span;
> +
> +	for_each_available_child_of_node(dev->of_node, child) {

Gah. This still going on with there not being a generic _available_
specific form.  We need to kick that again because I'm not keen to
merge another driver we'll need to tidy up later to use generic
properties.

Best bet is probably to just define
device_for_each_available_child_node() and see if anyone shoots
it down (even if it does the same as device_for_each_child_node()
in at least some cases).

Or thinking about it.. Here you could use device_for_each_child_node()
and then call fwnode_device_is_available() on the result and continue
if not true.

Will always return true (I think) but will make the intent clear.

We can tidy up to a new for_* if / when it becomes available.



> +		struct ltc2688_chan *chan;
> +
> +		ret = of_property_read_u32(child, "reg", &reg);
> +		if (ret) {
> +			of_node_put(child);
> +			return dev_err_probe(dev, ret,
> +					     "Failed to get reg property\n");
> +		}
> +
> +		if (reg >= LTC2688_DAC_CHANNELS) {
> +			of_node_put(child);
> +			return dev_err_probe(dev, -EINVAL,
> +					     "reg bigger than: %d\n",
> +					     LTC2688_DAC_CHANNELS);
> +		}
> +
> +		val = 0;
> +		chan = &st->channels[reg];
> +		if (of_property_read_bool(child, "adi,toggle-mode")) {
> +			chan->toggle_chan = true;
> +			/* assume sw toggle ABI */
> +			st->iio_chan[reg].ext_info = ltc2688_toggle_sym_ext_info;
> +			/*
> +			 * Clear IIO_CHAN_INFO_RAW bit as toggle channels expose
> +			 * out_voltage_raw{0|1} files.
> +			 */
> +			clear_bit(IIO_CHAN_INFO_RAW,
> +				  &st->iio_chan[reg].info_mask_separate);
> +		}
> +
> +		ret = of_property_read_u32_array(child, "adi,output-range-microvolt",
> +						 tmp, ARRAY_SIZE(tmp));
> +		if (!ret) {
> +			span = ltc2688_span_lookup(st, (int)tmp[0] / 1000,
> +						   tmp[1] / 1000);
> +			if (span < 0) {
> +				of_node_put(child);
> +				return dev_err_probe(dev, -EINVAL,
> +						     "output range not valid:[%d %d]\n",
> +						     tmp[0], tmp[1]);
> +			}
> +
> +			val |= FIELD_PREP(LTC2688_CH_SPAN_MSK, span);
> +		}
> +
> +		ret = of_property_read_u32(child, "adi,toggle-dither-input",
> +					   &clk_input);
> +		if (!ret) {
> +			if (clk_input >= LTC2688_CH_TGP_MAX) {
> +				of_node_put(child);
> +				return dev_err_probe(dev, -EINVAL,
> +						     "toggle-dither-input inv value(%d)\n",
> +						     clk_input);
> +			}
> +
> +			ret = ltc2688_tgp_clk_setup(st, chan, child, clk_input);
> +			if (ret) {
> +				of_node_put(child);
> +				return ret;
> +			}
> +
> +			/*
> +			 * 0 means software toggle which is the default mode.
> +			 * Hence the +1.
> +			 */
> +			val |= FIELD_PREP(LTC2688_CH_TD_SEL_MSK, clk_input + 1);
> +
> +			/*
> +			 * If a TGPx is given, we automatically assume a dither
> +			 * capable channel (unless toggle is already enabled).
> +			 * 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) {
> +				val |= FIELD_PREP(LTC2688_CH_MODE_MSK, 1);
> +				st->iio_chan[reg].ext_info = ltc2688_dither_ext_info;
> +			} else {
> +				/* wait, no sw toggle after all */
> +				st->iio_chan[reg].ext_info = ltc2688_toggle_ext_info;
> +			}
> +		}
> +
> +		if (of_property_read_bool(child, "adi,overrange")) {
> +			chan->overrange = true;
> +			val |= LTC2688_CH_OVERRANGE_MSK;
> +		}
> +
> +		if (!val)
> +			continue;
> +
> +		ret = regmap_write(st->regmap, LTC2688_CMD_CH_SETTING(reg),
> +				   val);
> +		if (ret) {
> +			of_node_put(child);
> +			return dev_err_probe(dev, -EINVAL,
> +					     "failed to set chan settings\n");
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +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, "clr", 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)
A bit of a mixture in here on whether you prefer if (ret) or if (ret < 0)
for error returns you know can't be postive.

I'd go with if (ret) for all of them.  It makes things consistent with the
cases where you directly return the value without checking it for less
than 0 like below.


> +			return ret;
> +	}
> +
> +	usleep_range(10000, 12000);
> +
> +	/*
> +	 * Duplicate the default channel configuration as it can change during
> +	 * @ltc2688_channel_config()
> +	 */
> +	st->iio_chan = devm_kmemdup(&st->spi->dev, ltc2688_channels,
> +				    sizeof(ltc2688_channels), GFP_KERNEL);
> +	if (!st->iio_chan)
> +		return -ENOMEM;
> +
> +	ret = ltc2688_channel_config(st);
> +	if (ret)
> +		return ret;
> +
> +	if (!vref)
> +		return 0;
> +
> +	return regmap_set_bits(st->regmap, LTC2688_CMD_CONFIG,
> +			       LTC2688_CONFIG_EXT_REF);
> +}
> +
> +static void ltc2688_bulk_disable(void *data)

rename to mention regulators.  Could be bulk disabling something else...

> +{
> +	struct ltc2688_state *st = data;
> +
> +	regulator_bulk_disable(ARRAY_SIZE(st->regulators), st->regulators);
> +}
> +

...

Thanks,

Jonathan

On Sun, 16 Jan 2022 16:28:08 +0000
"Sa, Nuno" <Nuno.Sa@analog.com> wrote:

> > From: Jonathan Cameron <jic23@kernel.org>
> > Sent: Sunday, January 16, 2022 1:44 PM
> > To: Sa, Nuno <Nuno.Sa@analog.com>
> > Cc: linux-iio@vger.kernel.org; devicetree@vger.kernel.org; Rob
> > Herring <robh+dt@kernel.org>; Lars-Peter Clausen
> > <lars@metafoo.de>; Hennerich, Michael
> > <Michael.Hennerich@analog.com>
> > Subject: Re: [PATCH v2 1/3] iio: dac: add support for ltc2688
> > 
> > [External]
> > 
> > On Sat, 15 Jan 2022 10:27:03 +0100
> > Nuno Sá <nuno.sa@analog.com> 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á <nuno.sa@analog.com>  
> > 
> > A few minor additions inline.
> > 
> > In particular I think we can work around that lack of
> > device_for_each_available_child_node() issue and use generic fw
> > propreties.
> > rather than of ones.  That way we can separate things from the
> > question of
> > how to 'fix' that issue.
> > 
> > One thing I'm not sure on is the phase units. Right now I think you are
> > exposing just the raw register value whereas I think that needs
> > converting
> > to radians.  
> 
> It's returning degrees which I think is fairly ok. But I know that in general
> we report radians, so I'm more than fine in changing this if you prefer it.

Radians for consistency is a must as users reading the docs may see the main
_phase descriptions and have no reason to think this one might be different.
 

> 
> > Jonathan
> > 
> > 
> > 
> > ...
> >   
> > > +static int ltc2688_channel_config(struct ltc2688_state *st)
> > > +{
> > > +	struct device *dev = &st->spi->dev;
> > > +	struct device_node *child;
> > > +	u32 reg, clk_input, val, tmp[2];
> > > +	int ret, span;
> > > +
> > > +	for_each_available_child_of_node(dev->of_node, child) {  
> > 
> > Gah. This still going on with there not being a generic _available_
> > specific form.  We need to kick that again because I'm not keen to
> > merge another driver we'll need to tidy up later to use generic
> > properties.
> > 
> > Best bet is probably to just define
> > device_for_each_available_child_node() and see if anyone shoots
> > it down (even if it does the same as device_for_each_child_node()
> > in at least some cases).
> > 
> > Or thinking about it.. Here you could use
> > device_for_each_child_node()
> > and then call fwnode_device_is_available() on the result and continue
> > if not true.
> > 
> > Will always return true (I think) but will make the intent clear.
> > 
> > We can tidy up to a new for_* if / when it becomes available.
> >   
> 
> Hmm, not sure I'm following you... I mean, I understand what you're
> saying here but there is a reason for why I changed the whole thing to
> use OF. Please take a look at the cover... I explain why I've done it.

Hohum. Reading the cover letter? :)  Next you'll be suggesting
I read manuals of new hardware!  I'll take a look.

Jonathan

>   * Have a clock property per channel. Note that we this I moved to OF
> since we now have to use 'devm_get_clk_from_child()' which is using
> device_node. Note that I could use 'to_of_node()' but mixing of.h and
> property.h does not feel like a good idea.

Ah, that's unfortunate given the clk is only needed in certain modes...

Andy/Rafael/Rob, any thoughts on how we should handle this?  Obviously
ACPI and clocks is generally a no go, but in this particular case we
aren't looking at a power management related use of clocks, but rather
using the clk framework to provide a way to control one of our inputs
used to generate the output dithered signal...  If the device just its
own clock then we'd just control it directly with no problems, but it uses
and external source.

We don't know of anyone actually looking at this device in conjunction with
ACPI so maybe just using dt specific calls for now rather than generic
firmware properties is the best we can do.

Thanks,

Jonathan

> 
>  ABI:
>   * Added out_voltageY_raw0 ABI for toggle mode;
>   * Added out_voltageY_dither_offset.
> 
>  Bindings:
>   * Use standard microvolt unit;
>   * Added constrains for adi,output-range-microvolt and removed negative
> values from the dts example;
>   * Moved clocks to the channel object;
>   * Dropped clock-names;
>   * Add a dependency between 'adi,toggle-dither-input' and 'clocks'.
>  
> [1]: https://marc.info/?l=linux-iio&m=163662843603265&w=2
> 
> Nuno Sá (3):
>   iio: dac: add support for ltc2688
>   iio: ABI: add ABI file for the LTC2688 DAC
>   dt-bindings: iio: Add ltc2688 documentation
> 
>  .../ABI/testing/sysfs-bus-iio-dac-ltc2688     |   80 ++
>  .../bindings/iio/dac/adi,ltc2688.yaml         |  147 +++
>  MAINTAINERS                                   |    9 +
>  drivers/iio/dac/Kconfig                       |   11 +
>  drivers/iio/dac/Makefile                      |    1 +
>  drivers/iio/dac/ltc2688.c                     | 1070 +++++++++++++++++
>  6 files changed, 1318 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-dac-ltc2688
>  create mode 100644 Documentation/devicetree/bindings/iio/dac/adi,ltc2688.yaml
>  create mode 100644 drivers/iio/dac/ltc2688.c
>