[v3,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'.

Changes in v3:

 ltc2688:
  * Fix mismatch between functions and function pointers detected by kernel
test bot; 
  * Always use if (ret) when ret > 0 has no meaning;
  * Rename ltc2688_bulk_disable -> ltc2688_disable_regulators;
  * Report dither phase in radians rather than degrees.

 ABI:
  * Specify units for dither_phase and dither_freqency; 
  * Say why its useful to have dither_en and toggle_en;
  * Combine out_voltageY_raw0 and out_voltageY_raw1;
  * Fix some description issues in out_voltageY_raw{0|1} and
out_voltageY_symbol.

 Bindings:
  * Remove mentions to ABI (linux specifix);
  * Slightly rephrased VREF and adi,toggle-dither-input properties and
suggested.
   
[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     |   86 ++
 .../bindings/iio/dac/adi,ltc2688.yaml         |  146 +++
 MAINTAINERS                                   |    9 +
 drivers/iio/dac/Kconfig                       |   11 +
 drivers/iio/dac/Makefile                      |    1 +
 drivers/iio/dac/ltc2688.c                     | 1070 +++++++++++++++++
 6 files changed, 1323 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 Fri, 21 Jan 2022 15:24:58 +0100
Nuno Sá <nuno.sa@analog.com> wrote:

> 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'.
> 
> Changes in v3:
> 
>  ltc2688:
>   * Fix mismatch between functions and function pointers detected by kernel
> test bot; 
>   * Always use if (ret) when ret > 0 has no meaning;
>   * Rename ltc2688_bulk_disable -> ltc2688_disable_regulators;
>   * Report dither phase in radians rather than degrees.
> 
>  ABI:
>   * Specify units for dither_phase and dither_freqency; 
>   * Say why its useful to have dither_en and toggle_en;
>   * Combine out_voltageY_raw0 and out_voltageY_raw1;
>   * Fix some description issues in out_voltageY_raw{0|1} and
> out_voltageY_symbol.
> 
>  Bindings:
>   * Remove mentions to ABI (linux specifix);
>   * Slightly rephrased VREF and adi,toggle-dither-input properties and
> suggested.
>    
> [1]: https://marc.info/?l=linux-iio&m=163662843603265&w=2

Series looks good to me, but will have to wait a little longer for DT and
any other review before I apply it.

Thanks,

Jonathan

> 
> 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     |   86 ++
>  .../bindings/iio/dac/adi,ltc2688.yaml         |  146 +++
>  MAINTAINERS                                   |    9 +
>  drivers/iio/dac/Kconfig                       |   11 +
>  drivers/iio/dac/Makefile                      |    1 +
>  drivers/iio/dac/ltc2688.c                     | 1070 +++++++++++++++++
>  6 files changed, 1323 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, Jan 22, 2022 at 05:27:06PM +0000, Jonathan Cameron wrote:
> On Fri, 21 Jan 2022 15:24:58 +0100
> Nuno Sá <nuno.sa@analog.com> wrote:

...

> Series looks good to me, but will have to wait a little longer for DT and
> any other review before I apply it.

Thanks! I have comments.

On Fri, Jan 21, 2022 at 03:25:00PM +0100, Nuno Sá wrote:
> Define the sysfs interface for toggle or dither capable channels. Dither
> capable channels will have the extended interface:
> 
>  * out_voltageY_dither_en
>  * out_voltageY_dither_raw
>  * out_voltageY_dither_offset
>  * out_voltageY_dither_raw_available
>  * out_voltageY_dither_frequency
>  * out_voltageY_dither_frequency_available
>  * out_voltageY_dither_phase
>  * out_voltageY_dither_phase_available
> 
> Toggle enabled channels will have:
> 
>  * out_voltageY_toggle_en
>  * out_voltageY_raw0
>  * out_voltageY_raw1
>  * out_voltageY_symbol
> 
> The common interface present in all channels is:
> 
>  * out_voltageY_raw (not present in toggle enabled channels)
>  * out_voltageY_raw_available
>  * out_voltageY_powerdown
>  * out_voltageY_scale
>  * out_voltageY_offset
>  * out_voltageY_calibbias
>  * out_voltageY_calibscale

...

> +KernelVersion:	5.17

v5.17 alredy gone for new features.

On Fri, Jan 21, 2022 at 03:24:59PM +0100, 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.

...

> +#include <linux/of.h>

property.h please/

...

> +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 = st->tx_data,
> +			.bits_per_word = 8,
> +			.len = 3,
> +			.cs_change = 1,
> +		}, {
> +			.tx_buf = st->tx_data + 3,
> +			.rx_buf = st->rx_data,
> +			.bits_per_word = 8,
> +			.len = 3,
> +		},
> +	};
> +	int ret;

> +	memcpy(st->tx_data, reg, reg_size);
> +
> +	ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> +	if (ret)
> +		return ret;
> +
> +	memcpy(val, &st->rx_data[1], val_size);
> +
> +	return 0;
> +}

First of all, yuo have fixed len in transfer sizes, so what the purpose of the reg_size / val_size?
Second, why do you need this specific function instead of regmap bulk ops against be24/le24?

...

> +unlock:

out_unlock: ?
(And in similar cases)

...

> +	if (ret)
> +		return ret;
> +
> +	return len;

In some cases the return ret ?: len; is used, in some like above. Maybe a bit
of consistency?

...

> +	if (private == LTC2688_INPUT_B_AVAIL)
> +		return sysfs_emit(buf, "[%u %u %u]\n", ltc2688_raw_range[0],
> +				  ltc2688_raw_range[1],
> +				  ltc2688_raw_range[2] / 4);

Is it standard form "[A B C]" for ranges in IIO? I haven't looked into the code
deeply (and datasheet at all) to understand meaning. To me range is usually out
of two numbers.

> +	if (private == LTC2688_DITHER_OFF)
> +		return sysfs_emit(buf, "0\n");

> +	ret = ltc2688_dac_code_read(st, chan->channel, private, &val);
> +	if (ret)
> +		return ret;
> +
> +	return sysfs_emit(buf, "%u\n", val);

These three types of output for one sysfs node? Seems it's not align with the
idea behind sysfs. It maybe that I missed something.

...

> +	ret = kstrtou16(buf, 10, &val);

In other function you have long, here u16. I would expect that the types are of
the same class, e.g. if here you have u16, then there something like s32 / s64.
Or here something like unsigned short.

A bit of elaboration why u16 is chosen here?

...

> +	.info_mask_separate_available = BIT(IIO_CHAN_INFO_RAW),		\
> +	.ext_info = ltc2688_ext_info					\

+ Comma

...

> +static int ltc2688_tgp_clk_setup(struct ltc2688_state *st,
> +				 struct ltc2688_chan *chan,
> +				 struct device_node *np, int tgp)
> +{
> +	unsigned long rate;
> +	struct clk *clk;
> +	int ret, f;
> +
> +	clk = devm_get_clk_from_child(&st->spi->dev, np, NULL);
> +	if (IS_ERR(clk))

Make it optional for non-OF, can be done as easy as

	if (IS_ERR(clk)) {
		if (PTR_ERR(clk) == -ENOENT)
			clk = NULL;
		else
			return dev_err_probe(...);
	}

> +		return dev_err_probe(&st->spi->dev, PTR_ERR(clk),
> +				     "failed to get tgp clk.\n");
> +
> +	ret = clk_prepare_enable(clk);
> +	if (ret)
> +		return dev_err_probe(&st->spi->dev, ret,
> +				     "failed to enable tgp clk.\n");
> +
> +	ret = devm_add_action_or_reset(&st->spi->dev, ltc2688_clk_disable, clk);
> +	if (ret)
> +		return ret;
> +
> +	if (chan->toggle_chan)
> +		return 0;
> +
> +	/* calculate available dither frequencies */
> +	rate = clk_get_rate(clk);
> +	for (f = 0; f < ARRAY_SIZE(chan->dither_frequency); f++)
> +		chan->dither_frequency[f] = DIV_ROUND_CLOSEST(rate, ltc2688_period[f]);
> +
> +	return 0;
> +}

...

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

device_for_each_child_node()

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

Do you need atomic operation here?

> +		}
> +
> +		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;
> +}

...

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

+ Comma.

> +};
> +
> +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

Ditto.

> +};

...

> +	vref_reg = devm_regulator_get_optional(dev, "vref");

> +	if (!IS_ERR(vref_reg)) {

Why not positive conditional check (and hence standard pattern -- error
handling first)?

> +		ret = regulator_enable(vref_reg);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to enable vref regulators\n");
> +
> +		ret = devm_add_action_or_reset(dev, ltc2688_disable_regulator,
> +					       vref_reg);
> +		if (ret)
> +			return ret;
> +
> +		ret = regulator_get_voltage(vref_reg);
> +		if (ret < 0)
> +			return dev_err_probe(dev, ret, "Failed to get vref\n");
> +
> +		st->vref = ret / 1000;
> +	} else {
> +		if (PTR_ERR(vref_reg) != -ENODEV)
> +			return dev_err_probe(dev, PTR_ERR(vref_reg),
> +					     "Failed to get vref regulator");
> +
> +		vref_reg = NULL;
> +		/* internal reference */
> +		st->vref = 4096;
> +	}

On Sat, 5 Feb 2022 19:29:39 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

A few comments from me, mostly because I couldn't resist jumping in ;)
Note this is only some of the things Andy raised....

Jonathan

> 
> > +	if (private == LTC2688_INPUT_B_AVAIL)
> > +		return sysfs_emit(buf, "[%u %u %u]\n", ltc2688_raw_range[0],
> > +				  ltc2688_raw_range[1],
> > +				  ltc2688_raw_range[2] / 4);  
> 
> Is it standard form "[A B C]" for ranges in IIO? I haven't looked into the code
> deeply (and datasheet at all) to understand meaning. To me range is usually out
> of two numbers.

https://elixir.bootlin.com/linux/latest/source/Documentation/ABI/testing/sysfs-bus-iio#L105
Yes, [Min Step Max]

> 
> > +	if (private == LTC2688_DITHER_OFF)
> > +		return sysfs_emit(buf, "0\n");  
> 
> > +	ret = ltc2688_dac_code_read(st, chan->channel, private, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return sysfs_emit(buf, "%u\n", val);  
> 
> These three types of output for one sysfs node? Seems it's not align with the
> idea behind sysfs. It maybe that I missed something.

Different sysfs nodes.  Though it's a fair question on whether this would be
better done as a bunch of separate callbacks, rather than one with a lookup
on the private parameter.


> 
> > +static int ltc2688_tgp_clk_setup(struct ltc2688_state *st,
> > +				 struct ltc2688_chan *chan,
> > +				 struct device_node *np, int tgp)
> > +{
> > +	unsigned long rate;
> > +	struct clk *clk;
> > +	int ret, f;
> > +
> > +	clk = devm_get_clk_from_child(&st->spi->dev, np, NULL);
> > +	if (IS_ERR(clk))  
> 
> Make it optional for non-OF, can be done as easy as
> 
> 	if (IS_ERR(clk)) {
> 		if (PTR_ERR(clk) == -ENOENT)
> 			clk = NULL;
> 		else
> 			return dev_err_probe(...);
> 	}

Interesting.  We should probably look at where else this
is appropriate.

> 
> > +		return dev_err_probe(&st->spi->dev, PTR_ERR(clk),
> > +				     "failed to get tgp clk.\n");
> > +
> > +	ret = clk_prepare_enable(clk);
> > +	if (ret)
> > +		return dev_err_probe(&st->spi->dev, ret,
> > +				     "failed to enable tgp clk.\n");
> > +
> > +	ret = devm_add_action_or_reset(&st->spi->dev, ltc2688_clk_disable, clk);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (chan->toggle_chan)
> > +		return 0;
> > +
> > +	/* calculate available dither frequencies */
> > +	rate = clk_get_rate(clk);
> > +	for (f = 0; f < ARRAY_SIZE(chan->dither_frequency); f++)
> > +		chan->dither_frequency[f] = DIV_ROUND_CLOSEST(rate, ltc2688_period[f]);
> > +
> > +	return 0;
> > +}  
> 
> ...
> 
> > +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) {  
> 
> device_for_each_child_node()

This is the old issue with missing
device_for_each_available_child_node()
though can just add a check on whether it's available inside the loop.
> 
> > +		struct ltc2688_chan *chan;
> > +

...

On Sat, Feb 05, 2022 at 06:44:59PM +0000, Jonathan Cameron wrote:
> On Sat, 5 Feb 2022 19:29:39 +0200
> Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> 
> A few comments from me, mostly because I couldn't resist jumping in ;)
> Note this is only some of the things Andy raised....

...

> > > +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) {  
> > 
> > device_for_each_child_node()
> 
> This is the old issue with missing
> device_for_each_available_child_node()
> though can just add a check on whether it's available inside the loop.

Didn't we discuss this with Rob and he told that device_for_each_child_node()
is already for available only?

> > > +		struct ltc2688_chan *chan;
> > > +

On Sat, Feb 05, 2022 at 08:50:31PM +0200, Andy Shevchenko wrote:
> On Sat, Feb 05, 2022 at 06:44:59PM +0000, Jonathan Cameron wrote:
> > On Sat, 5 Feb 2022 19:29:39 +0200
> > Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> > 
> > A few comments from me, mostly because I couldn't resist jumping in ;)
> > Note this is only some of the things Andy raised....
> 
> ...
> 
> > > > +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) {  
> > > 
> > > device_for_each_child_node()
> > 
> > This is the old issue with missing
> > device_for_each_available_child_node()
> > though can just add a check on whether it's available inside the loop.
> 
> Didn't we discuss this with Rob and he told that device_for_each_child_node()
> is already for available only?

https://lore.kernel.org/lkml/20211205190101.26de4a57@jic23-huawei/T/#u

So, the fwnode has a correct implementation, and we may use it here.

> From: Andy Shevchenko <andriy.shevchenko@intel.com>
> Sent: Saturday, February 5, 2022 6:30 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-iio@vger.kernel.org; devicetree@vger.kernel.org; Rob
> Herring <robh+dt@kernel.org>; Jonathan Cameron
> <jic23@kernel.org>; Lars-Peter Clausen <lars@metafoo.de>;
> Hennerich, Michael <Michael.Hennerich@analog.com>
> Subject: Re: [PATCH v3 1/3] iio: dac: add support for ltc2688
> 
> [External]
> 
> On Fri, Jan 21, 2022 at 03:24:59PM +0100, 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.
> 
> ...
> 
> > +#include <linux/of.h>
> 
> property.h please/

That probably means property and of both included. See below in the
clock_get comments...

> ...
> 
> > +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 = st->tx_data,
> > +			.bits_per_word = 8,
> > +			.len = 3,
> > +			.cs_change = 1,
> > +		}, {
> > +			.tx_buf = st->tx_data + 3,
> > +			.rx_buf = st->rx_data,
> > +			.bits_per_word = 8,
> > +			.len = 3,
> > +		},
> > +	};
> > +	int ret;
> 
> > +	memcpy(st->tx_data, reg, reg_size);
> > +
> > +	ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> > +	if (ret)
> > +		return ret;
> > +
> > +	memcpy(val, &st->rx_data[1], val_size);
> > +
> > +	return 0;
> > +}
> 
> First of all, yuo have fixed len in transfer sizes, so what the purpose of
> the reg_size / val_size?

Well, reg_size is 1 byte and val_size is 2 as defined in the regmap_bus
struct. And that is what it must be used for the transfer to work. I 
could also hardcode 1 and 2 but I preferred to use the parameters. I guess
you can argue (and probably this is why you are complaining about this)
for me to use reg_size + val_size in the transfer length for consistency.
That's fair but I do not think this is __that__ bad... Can make that change
though.

> Second, why do you need this specific function instead of regmap bulk
> ops against be24/le24?
>

Not sure I'm following this one... If you mean why am I using a custom 
regmap_bus implementation, that was already explained in the RFC patch.
And IIRC, you were the one already asking 😉.
 
> ...
> 
> > +unlock:
> 
> out_unlock: ?
> (And in similar cases)

Well, why not? can do that...

> ...
> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	return len;
> 
> In some cases the return ret ?: len; is used, in some like above. Maybe
> a bit
> of consistency?

That's fair... At this point, I will go with the one that evolves less changes
unless noted otherwise.
 
> ...
> 
> > +	if (private == LTC2688_INPUT_B_AVAIL)
> > +		return sysfs_emit(buf, "[%u %u %u]\n",
> ltc2688_raw_range[0],
> > +				  ltc2688_raw_range[1],
> > +				  ltc2688_raw_range[2] / 4);
> 
> Is it standard form "[A B C]" for ranges in IIO? I haven't looked into the
> code
> deeply (and datasheet at all) to understand meaning. To me range is
> usually out
> of two numbers.
> 
> > +	if (private == LTC2688_DITHER_OFF)
> > +		return sysfs_emit(buf, "0\n");
> 
> > +	ret = ltc2688_dac_code_read(st, chan->channel, private, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return sysfs_emit(buf, "%u\n", val);
> 
> These three types of output for one sysfs node? Seems it's not align
> with the
> idea behind sysfs. It maybe that I missed something.
> 
> ...
> 
> > +	ret = kstrtou16(buf, 10, &val);
> 
> In other function you have long, here u16. I would expect that the
> types are of
> the same class, e.g. if here you have u16, then there something like
> s32 / s64.
> Or here something like unsigned short.
> 
> A bit of elaboration why u16 is chosen here?

Well, I never really saw any enforcement here to be honest (rather than using
stdint types...). So I pretty much just use these in unsigned types because
I'm lazy and u16 is faster to type than unsigned short... In this case, unless Jonathan
really asks for it, I prefer not to go all over the driver and change this...

> ...
> 
> > +	.info_mask_separate_available = BIT(IIO_CHAN_INFO_RAW),
> 		\
> > +	.ext_info = ltc2688_ext_info					\
> 
> + Comma
> 
> ...
> 
> > +static int ltc2688_tgp_clk_setup(struct ltc2688_state *st,
> > +				 struct ltc2688_chan *chan,
> > +				 struct device_node *np, int tgp)
> > +{
> > +	unsigned long rate;
> > +	struct clk *clk;
> > +	int ret, f;
> > +
> > +	clk = devm_get_clk_from_child(&st->spi->dev, np, NULL);
> > +	if (IS_ERR(clk))
> 
> Make it optional for non-OF, can be done as easy as
> 
> 	if (IS_ERR(clk)) {
> 		if (PTR_ERR(clk) == -ENOENT)
> 			clk = NULL;
> 		else
> 			return dev_err_probe(...);
> 	}
> 
> > +		return dev_err_probe(&st->spi->dev, PTR_ERR(clk),
> > +				     "failed to get tgp clk.\n");

Well, I might be missing the point but I think this is not so straight....
We will only get here if the property " adi,toggle-dither-input" is given
in which case having the associated clocks is __mandatory__. Hence,
once we are here, this can never be optional. That said, we need
device_node and hence of.h to be included and this was the main reason
why I changed from property.h to of.h (once I started to use
'devm_get_clk_from_child()'. I don’t really think that using both of and
property is a good idea and I raised this in the previous version of this series
and no one made it clear that using both of and property would be acceptable
so I kept my move to of in the current version. 

> > +	ret = clk_prepare_enable(clk);
> > +	if (ret)
> > +		return dev_err_probe(&st->spi->dev, ret,
> > +				     "failed to enable tgp clk.\n");
> > +
> > +	ret = devm_add_action_or_reset(&st->spi->dev,
> ltc2688_clk_disable, clk);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (chan->toggle_chan)
> > +		return 0;
> > +
> > +	/* calculate available dither frequencies */
> > +	rate = clk_get_rate(clk);
> > +	for (f = 0; f < ARRAY_SIZE(chan->dither_frequency); f++)
> > +		chan->dither_frequency[f] =
> DIV_ROUND_CLOSEST(rate, ltc2688_period[f]);
> > +
> > +	return 0;
> > +}
> 
> ...
> 
> > +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) {
> 
> device_for_each_child_node()
> 
> > +		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);
> 
> Do you need atomic operation here?

Not really, but I still prefer to use 'clear_bit()' rather than doing it
by hand... Is there another utility for this?

> > +		}
> > +
> > +		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;
> > +}
> 
> ...
> 
> > +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
> 
> + Comma.
> 
> > +};
> > +
> > +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
> 
> Ditto.
> 
> > +};
> 
> ...
> 
> > +	vref_reg = devm_regulator_get_optional(dev, "vref");
> 
> > +	if (!IS_ERR(vref_reg)) {
> 
> Why not positive conditional check (and hence standard pattern --
> error
> handling first)?
> 

No reason.. I can flip the logic...

- Nuno Sá

On Sat, 5 Feb 2022 20:58:12 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Sat, Feb 05, 2022 at 08:50:31PM +0200, Andy Shevchenko wrote:
> > On Sat, Feb 05, 2022 at 06:44:59PM +0000, Jonathan Cameron wrote:  
> > > On Sat, 5 Feb 2022 19:29:39 +0200
> > > Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> > > 
> > > A few comments from me, mostly because I couldn't resist jumping in ;)
> > > Note this is only some of the things Andy raised....  
> > 
> > ...
> >   
> > > > > +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) {    
> > > > 
> > > > device_for_each_child_node()  
> > > 
> > > This is the old issue with missing
> > > device_for_each_available_child_node()
> > > though can just add a check on whether it's available inside the loop.  
> > 
> > Didn't we discuss this with Rob and he told that device_for_each_child_node()
> > is already for available only?  
> 
> https://lore.kernel.org/lkml/20211205190101.26de4a57@jic23-huawei/T/#u
> 
> So, the fwnode has a correct implementation, and we may use it here.
> 
I wasn't totally sure of the conclusion of that discussion.
a) Fine to just use device_for_each_child_node() for this case and not worry about it.
b) Worth adding device_for_each_available_child_node() with the same implementation
c) (possibly workaround / avoid the issue) Use device_for_each_child_node() but also
check validity (hopefully compiler would remove the check) in order to act
as documentation.

I'm fine with any of the above.

J

On Sun, Feb 06, 2022 at 03:16:24PM +0000, Jonathan Cameron wrote:
> On Sat, 5 Feb 2022 20:58:12 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > On Sat, Feb 05, 2022 at 08:50:31PM +0200, Andy Shevchenko wrote:

...

> > https://lore.kernel.org/lkml/20211205190101.26de4a57@jic23-huawei/T/#u
> > 
> > So, the fwnode has a correct implementation, and we may use it here.
> > 
> I wasn't totally sure of the conclusion of that discussion.
> a) Fine to just use device_for_each_child_node() for this case and not worry
> about it.

Yes. As he mentioned the device_for_each_child_node() is implemented correctly
from day 1.

> b) Worth adding device_for_each_available_child_node() with the same
> implementation

I believe it's an opposite prospective, i.e. drop
of_for_each_available_child_node() and use the of_for_each_child_node()
everywhere.

> c) (possibly workaround / avoid the issue) Use device_for_each_child_node()
> but also check validity (hopefully compiler would remove the check)
> in order to act as documentation.

Makes no sense because implementation does it already.

> I'm fine with any of the above.

On Sun, Feb 06, 2022 at 01:19:59PM +0000, Sa, Nuno wrote:
> > From: Andy Shevchenko <andriy.shevchenko@intel.com>
> > Sent: Saturday, February 5, 2022 6:30 PM
> > On Fri, Jan 21, 2022 at 03:24:59PM +0100, Nuno Sá wrote:

...

> > > +#include <linux/of.h>
> > 
> > property.h please/
> 
> That probably means property and of both included. See below in the
> clock_get comments...

Why? OF won't be used at all.

...

> > > +	memcpy(st->tx_data, reg, reg_size);
> > > +
> > > +	ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	memcpy(val, &st->rx_data[1], val_size);
> > > +
> > > +	return 0;
> > > +}
> > 
> > First of all, yuo have fixed len in transfer sizes, so what the purpose of
> > the reg_size / val_size?
> 
> Well, reg_size is 1 byte and val_size is 2 as defined in the regmap_bus
> struct. And that is what it must be used for the transfer to work. I 
> could also hardcode 1 and 2 but I preferred to use the parameters. I guess
> you can argue (and probably this is why you are complaining about this)
> for me to use reg_size + val_size in the transfer length for consistency.
> That's fair but I do not think this is __that__ bad...

It's not bad, but I think that division between register and value is a good
thing to have.

> Can make that change though.

Would be nice!

...

> > Second, why do you need this specific function instead of regmap bulk
> > ops against be24/le24?
> 
> Not sure I'm following this one... If you mean why am I using a custom 
> regmap_bus implementation, that was already explained in the RFC patch.
> And IIRC, you were the one already asking 😉.

Hmm... It was some time I have looked there. Any message ID to share, so
I can find it quickly?

...

> > > +	ret = kstrtou16(buf, 10, &val);
> > 
> > In other function you have long, here u16. I would expect that the
> > types are of
> > the same class, e.g. if here you have u16, then there something like
> > s32 / s64.
> > Or here something like unsigned short.
> > 
> > A bit of elaboration why u16 is chosen here?
> 
> Well, I never really saw any enforcement here to be honest (rather than using
> stdint types...). So I pretty much just use these in unsigned types because
> I'm lazy and u16 is faster to type than unsigned short... In this case, unless Jonathan
> really asks for it, I prefer not to go all over the driver and change this...

This is about consistency. It may work as is, but it feels not good when for
int (or unsigned int) one uses fixed-width types. Also it's non-written advice
to use fixed-width variables when it's about programming registers or so, for
the rest, use POD types.

...

> > > +static int ltc2688_tgp_clk_setup(struct ltc2688_state *st,
> > > +				 struct ltc2688_chan *chan,
> > > +				 struct device_node *np, int tgp)
> > > +{
> > > +	unsigned long rate;
> > > +	struct clk *clk;
> > > +	int ret, f;
> > > +
> > > +	clk = devm_get_clk_from_child(&st->spi->dev, np, NULL);
> > > +	if (IS_ERR(clk))
> > 
> > Make it optional for non-OF, can be done as easy as
> > 
> > 	if (IS_ERR(clk)) {
> > 		if (PTR_ERR(clk) == -ENOENT)
> > 			clk = NULL;
> > 		else
> > 			return dev_err_probe(...);
> > 	}
> > 
> > > +		return dev_err_probe(&st->spi->dev, PTR_ERR(clk),
> > > +				     "failed to get tgp clk.\n");
> 
> Well, I might be missing the point but I think this is not so straight....
> We will only get here if the property " adi,toggle-dither-input" is given
> in which case having the associated clocks is __mandatory__.

Ah, okay, would be a limitation for non-OF platforms.

> Hence,
> once we are here, this can never be optional. That said, we need
> device_node 

That's fine, since CCF is OF-centric API.

> and hence of.h

Why? This header doesn't bring anything you will use here.

> to be included and this was the main reason
> why I changed from property.h to of.h (once I started to use
> 'devm_get_clk_from_child()'. I don’t really think that using both of and
> property is a good idea and I raised this in the previous version of this series
> and no one made it clear that using both of and property would be acceptable
> so I kept my move to of in the current version.

It's a good idea for sensors to be able to use them outside of OF platforms.
CCF is PITA, but at least with the conversion to device property API, this
become the only one (and it has a few possible workarounds).

> > > +	ret = clk_prepare_enable(clk);
> > > +	if (ret)
> > > +		return dev_err_probe(&st->spi->dev, ret,
> > > +				     "failed to enable tgp clk.\n");

...

> > > +			clear_bit(IIO_CHAN_INFO_RAW,
> > > +				  &st-
> > >iio_chan[reg].info_mask_separate);
> > 
> > Do you need atomic operation here?
> 
> Not really, but I still prefer to use 'clear_bit()' rather than doing it
> by hand... Is there another utility for this?

__clear_bit().

On Mon, 2022-02-07 at 13:09 +0200, Andy Shevchenko wrote:
> On Sun, Feb 06, 2022 at 01:19:59PM +0000, Sa, Nuno wrote:
> > > From: Andy Shevchenko <andriy.shevchenko@intel.com>
> > > Sent: Saturday, February 5, 2022 6:30 PM
> > > On Fri, Jan 21, 2022 at 03:24:59PM +0100, Nuno Sá wrote:
> 
> ...
> 
> > > > +#include <linux/of.h>
> > > 
> > > property.h please/
> > 
> > That probably means property and of both included. See below in the
> > clock_get comments...
> 
> Why? OF won't be used at all.
> 
see below on the clock function...
> 
> ...
> 
> > > > +       memcpy(st->tx_data, reg, reg_size);
> > > > +
> > > > +       ret = spi_sync_transfer(st->spi, xfers,
> > > > ARRAY_SIZE(xfers));
> > > > +       if (ret)
> > > > +               return ret;
> > > > +
> > > > +       memcpy(val, &st->rx_data[1], val_size);
> > > > +
> > > > +       return 0;
> > > > +}
> > > 
> > > First of all, yuo have fixed len in transfer sizes, so what the
> > > purpose of
> > > the reg_size / val_size?
> > 
> > Well, reg_size is 1 byte and val_size is 2 as defined in the
> > regmap_bus
> > struct. And that is what it must be used for the transfer to work.
> > I 
> > could also hardcode 1 and 2 but I preferred to use the parameters.
> > I guess
> > you can argue (and probably this is why you are complaining about
> > this)
> > for me to use reg_size + val_size in the transfer length for
> > consistency.
> > That's fair but I do not think this is __that__ bad...
> 
> It's not bad, but I think that division between register and value is
> a good
> thing to have.
> 
> > Can make that change though.
> 
> Would be nice!
> 
> ...
> 
> > > Second, why do you need this specific function instead of regmap
> > > bulk
> > > ops against be24/le24?
> > 
> > Not sure I'm following this one... If you mean why am I using a
> > custom 
> > regmap_bus implementation, that was already explained in the RFC
> > patch.
> > And IIRC, you were the one already asking 😉.
> 
> Hmm... It was some time I have looked there. Any message ID to share,
> so
> I can find it quickly?
> 

https://lore.kernel.org/all/20211112152235.12fdcc49@jic23-huawei/

> ...
> 
> > > > +       ret = kstrtou16(buf, 10, &val);
> > > 
> > > In other function you have long, here u16. I would expect that
> > > the
> > > types are of
> > > the same class, e.g. if here you have u16, then there something
> > > like
> > > s32 / s64.
> > > Or here something like unsigned short.
> > > 
> > > A bit of elaboration why u16 is chosen here?
> > 
> > Well, I never really saw any enforcement here to be honest (rather
> > than using
> > stdint types...). So I pretty much just use these in unsigned types
> > because
> > I'm lazy and u16 is faster to type than unsigned short... In this
> > case, unless Jonathan
> > really asks for it, I prefer not to go all over the driver and
> > change this...
> 
> This is about consistency. It may work as is, but it feels not good
> when for
> int (or unsigned int) one uses fixed-width types. Also it's non-
> written advice
> to use fixed-width variables when it's about programming registers or
> so, for
> the rest, use POD types.
> 
> ...

I can understand your reasoning but again this is something that
I never really saw being enforced. So, I'm more than ok to change it
if it really becomes something that we will try to "enforce" in IIO.
Otherwise it just feels as a random nitpick :).

> 
> > > > +static int ltc2688_tgp_clk_setup(struct ltc2688_state *st,
> > > > +                                struct ltc2688_chan *chan,
> > > > +                                struct device_node *np, int
> > > > tgp)
> > > > +{
> > > > +       unsigned long rate;
> > > > +       struct clk *clk;
> > > > +       int ret, f;
> > > > +
> > > > +       clk = devm_get_clk_from_child(&st->spi->dev, np, NULL);
> > > > +       if (IS_ERR(clk))
> > > 
> > > Make it optional for non-OF, can be done as easy as
> > > 
> > >         if (IS_ERR(clk)) {
> > >                 if (PTR_ERR(clk) == -ENOENT)
> > >                         clk = NULL;
> > >                 else
> > >                         return dev_err_probe(...);
> > >         }
> > > 
> > > > +               return dev_err_probe(&st->spi->dev,
> > > > PTR_ERR(clk),
> > > > +                                    "failed to get tgp
> > > > clk.\n");
> > 
> > Well, I might be missing the point but I think this is not so
> > straight....
> > We will only get here if the property " adi,toggle-dither-input" is
> > given
> > in which case having the associated clocks is __mandatory__.
> 
> Ah, okay, would be a limitation for non-OF platforms.
> 
> > Hence,
> > once we are here, this can never be optional. That said, we need
> > device_node 
> 
> That's fine, since CCF is OF-centric API.
> 
> > and hence of.h
> 
> Why? This header doesn't bring anything you will use here.

Correct me if Im missing something. AFAIU, the idea is to use
'device_for_each_child_node()' which returns a fwnode_handle. That
means, that we will have to pass that to this function and use
'to_of_node()' to pass a device_node to 'devm_get_clk_from_child()'.

This means, we need of.h for 'to_of_node()'...


- Nuno Sá

On Mon, 2022-02-07 at 21:19 +0100, Nuno Sá wrote:
> On Mon, 2022-02-07 at 13:09 +0200, Andy Shevchenko wrote:
> > On Sun, Feb 06, 2022 at 01:19:59PM +0000, Sa, Nuno wrote:
> > > > From: Andy Shevchenko <andriy.shevchenko@intel.com>
> > > > Sent: Saturday, February 5, 2022 6:30 PM
> > > > On Fri, Jan 21, 2022 at 03:24:59PM +0100, Nuno Sá wrote:
> > 
> > ...
> > 
> > > > > +#include <linux/of.h>
> > > > 
> > > > property.h please/
> > > 
> > > That probably means property and of both included. See below in
> > > the
> > > clock_get comments...
> > 
> > Why? OF won't be used at all.
> > 
> see below on the clock function...
> > 
> > ...
> > 
> > > > > +       memcpy(st->tx_data, reg, reg_size);
> > > > > +
> > > > > +       ret = spi_sync_transfer(st->spi, xfers,
> > > > > ARRAY_SIZE(xfers));
> > > > > +       if (ret)
> > > > > +               return ret;
> > > > > +
> > > > > +       memcpy(val, &st->rx_data[1], val_size);
> > > > > +
> > > > > +       return 0;
> > > > > +}
> > > > 
> > > > First of all, yuo have fixed len in transfer sizes, so what the
> > > > purpose of
> > > > the reg_size / val_size?
> > > 
> > > Well, reg_size is 1 byte and val_size is 2 as defined in the
> > > regmap_bus
> > > struct. And that is what it must be used for the transfer to
> > > work.
> > > I 
> > > could also hardcode 1 and 2 but I preferred to use the
> > > parameters.
> > > I guess
> > > you can argue (and probably this is why you are complaining about
> > > this)
> > > for me to use reg_size + val_size in the transfer length for
> > > consistency.
> > > That's fair but I do not think this is __that__ bad...
> > 
> > It's not bad, but I think that division between register and value
> > is
> > a good
> > thing to have.
> > 
> > > Can make that change though.
> > 
> > Would be nice!
> > 
> > ...
> > 
> > > > Second, why do you need this specific function instead of
> > > > regmap
> > > > bulk
> > > > ops against be24/le24?
> > > 
> > > Not sure I'm following this one... If you mean why am I using a
> > > custom 
> > > regmap_bus implementation, that was already explained in the RFC
> > > patch.
> > > And IIRC, you were the one already asking 😉.
> > 
> > Hmm... It was some time I have looked there. Any message ID to
> > share,
> > so
> > I can find it quickly?
> > 
> 
> https://lore.kernel.org/all/20211112152235.12fdcc49@jic23-huawei/
> 
> > ...
> > 
> > > > > +       ret = kstrtou16(buf, 10, &val);
> > > > 
> > > > In other function you have long, here u16. I would expect that
> > > > the
> > > > types are of
> > > > the same class, e.g. if here you have u16, then there something
> > > > like
> > > > s32 / s64.
> > > > Or here something like unsigned short.
> > > > 
> > > > A bit of elaboration why u16 is chosen here?
> > > 
> > > Well, I never really saw any enforcement here to be honest
> > > (rather
> > > than using
> > > stdint types...). So I pretty much just use these in unsigned
> > > types
> > > because
> > > I'm lazy and u16 is faster to type than unsigned short... In this
> > > case, unless Jonathan
> > > really asks for it, I prefer not to go all over the driver and
> > > change this...
> > 
> > This is about consistency. It may work as is, but it feels not good
> > when for
> > int (or unsigned int) one uses fixed-width types. Also it's non-
> > written advice
> > to use fixed-width variables when it's about programming registers
> > or
> > so, for
> > the rest, use POD types.
> > 
> > ...
> 
> I can understand your reasoning but again this is something that
> I never really saw being enforced. So, I'm more than ok to change it
> if it really becomes something that we will try to "enforce" in IIO.
> Otherwise it just feels as a random nitpick :).
> 
> > 
> > > > > +static int ltc2688_tgp_clk_setup(struct ltc2688_state *st,
> > > > > +                                struct ltc2688_chan *chan,
> > > > > +                                struct device_node *np, int
> > > > > tgp)
> > > > > +{
> > > > > +       unsigned long rate;
> > > > > +       struct clk *clk;
> > > > > +       int ret, f;
> > > > > +
> > > > > +       clk = devm_get_clk_from_child(&st->spi->dev, np,
> > > > > NULL);
> > > > > +       if (IS_ERR(clk))
> > > > 
> > > > Make it optional for non-OF, can be done as easy as
> > > > 
> > > >         if (IS_ERR(clk)) {
> > > >                 if (PTR_ERR(clk) == -ENOENT)
> > > >                         clk = NULL;
> > > >                 else
> > > >                         return dev_err_probe(...);
> > > >         }
> > > > 
> > > > > +               return dev_err_probe(&st->spi->dev,
> > > > > PTR_ERR(clk),
> > > > > +                                    "failed to get tgp
> > > > > clk.\n");
> > > 
> > > Well, I might be missing the point but I think this is not so
> > > straight....
> > > We will only get here if the property " adi,toggle-dither-input"
> > > is
> > > given
> > > in which case having the associated clocks is __mandatory__.
> > 
> > Ah, okay, would be a limitation for non-OF platforms.
> > 
> > > Hence,
> > > once we are here, this can never be optional. That said, we need
> > > device_node 
> > 
> > That's fine, since CCF is OF-centric API.
> > 
> > > and hence of.h
> > 
> > Why? This header doesn't bring anything you will use here.
> 
> Correct me if Im missing something. AFAIU, the idea is to use
> 'device_for_each_child_node()' which returns a fwnode_handle. That
> means, that we will have to pass that to this function and use
> 'to_of_node()' to pass a device_node to 'devm_get_clk_from_child()'.
> 
> This means, we need of.h for 'to_of_node()'...
> 
> 

Andy, Jonathan,

I would definetly like to have some settlement on the above (before
sending a v4). It kind of was discussed a bit already in [1] where I
felt we had to live with OF for this driver (that is why I kept OF in
v3. With the above, I cannot
really see how we can have device API with also including OF... If I
missing something please let me know :)


[1]: https://lore.kernel.org/all/CAHp75VczFs8QpsY7tuB-h4X=H54nyjABA4qDSmpQ+FRYAHZdrA@mail.gmail.com/
- Nuno Sá

On Mon, Feb 07, 2022 at 09:19:46PM +0100, Nuno Sá wrote:
> On Mon, 2022-02-07 at 13:09 +0200, Andy Shevchenko wrote:
> > On Sun, Feb 06, 2022 at 01:19:59PM +0000, Sa, Nuno wrote:
> > > > From: Andy Shevchenko <andriy.shevchenko@intel.com>
> > > > Sent: Saturday, February 5, 2022 6:30 PM
> > > > On Fri, Jan 21, 2022 at 03:24:59PM +0100, Nuno Sá wrote:

...

> > > > Second, why do you need this specific function instead of regmap
> > > > bulk
> > > > ops against be24/le24?
> > > 
> > > Not sure I'm following this one... If you mean why am I using a
> > > custom 
> > > regmap_bus implementation, that was already explained in the RFC
> > > patch.
> > > And IIRC, you were the one already asking 😉.
> > 
> > Hmm... It was some time I have looked there. Any message ID to share,
> > so
> > I can find it quickly?

> https://lore.kernel.org/all/20211112152235.12fdcc49@jic23-huawei/

Thanks!

So, it's all about cs_change, right?
But doesn't bulk operation work exactly as we need here?

Looking again to the RFC code, it seems like we can still do it

First, you call _gather_write() followed by _read(). It will show exactly what
you do, i.e. you send command first with the value 0x0000, followed by sending
command and reading back the value at the same time.

Would it work?

...

> > > > > +       ret = kstrtou16(buf, 10, &val);
> > > > 
> > > > In other function you have long, here u16. I would expect that
> > > > the
> > > > types are of
> > > > the same class, e.g. if here you have u16, then there something
> > > > like
> > > > s32 / s64.
> > > > Or here something like unsigned short.
> > > > 
> > > > A bit of elaboration why u16 is chosen here?
> > > 
> > > Well, I never really saw any enforcement here to be honest (rather
> > > than using
> > > stdint types...). So I pretty much just use these in unsigned types
> > > because
> > > I'm lazy and u16 is faster to type than unsigned short... In this
> > > case, unless Jonathan
> > > really asks for it, I prefer not to go all over the driver and
> > > change this...
> > 
> > This is about consistency. It may work as is, but it feels not good
> > when for
> > int (or unsigned int) one uses fixed-width types. Also it's non-
> > written advice
> > to use fixed-width variables when it's about programming registers or
> > so, for
> > the rest, use POD types.

> I can understand your reasoning but again this is something that
> I never really saw being enforced. So, I'm more than ok to change it
> if it really becomes something that we will try to "enforce" in IIO.
> Otherwise it just feels as a random nitpick :).

No, this is about consistency and common sense. If you define type uXX,
we have an API for that exact type. It's confusing why POD type APIs
are used with fixed-width types or vise versa.

Moreover (which is pure theoretical, though) some architectures might
have no (mutual) equivalency between these types.

...

> > > > > +static int ltc2688_tgp_clk_setup(struct ltc2688_state *st,
> > > > > +                                struct ltc2688_chan *chan,
> > > > > +                                struct device_node *np, int
> > > > > tgp)
> > > > > +{
> > > > > +       unsigned long rate;
> > > > > +       struct clk *clk;
> > > > > +       int ret, f;
> > > > > +
> > > > > +       clk = devm_get_clk_from_child(&st->spi->dev, np, NULL);
> > > > > +       if (IS_ERR(clk))
> > > > 
> > > > Make it optional for non-OF, can be done as easy as
> > > > 
> > > >         if (IS_ERR(clk)) {
> > > >                 if (PTR_ERR(clk) == -ENOENT)
> > > >                         clk = NULL;
> > > >                 else
> > > >                         return dev_err_probe(...);
> > > >         }
> > > > 
> > > > > +               return dev_err_probe(&st->spi->dev,
> > > > > PTR_ERR(clk),
> > > > > +                                    "failed to get tgp
> > > > > clk.\n");
> > > 
> > > Well, I might be missing the point but I think this is not so
> > > straight....
> > > We will only get here if the property " adi,toggle-dither-input" is
> > > given
> > > in which case having the associated clocks is __mandatory__.
> > 
> > Ah, okay, would be a limitation for non-OF platforms.
> > 
> > > Hence,
> > > once we are here, this can never be optional. That said, we need
> > > device_node 
> > 
> > That's fine, since CCF is OF-centric API.
> > 
> > > and hence of.h
> > 
> > Why? This header doesn't bring anything you will use here.
> 
> Correct me if Im missing something. AFAIU, the idea is to use
> 'device_for_each_child_node()' which returns a fwnode_handle. That
> means, that we will have to pass that to this function and use
> 'to_of_node()' to pass a device_node to 'devm_get_clk_from_child()'.
> 
> This means, we need of.h for 'to_of_node()'...

Yeah, you are right, but it would be still better since it narrows
the problem to the CCF calls only.

On Mon, Feb 14, 2022 at 02:23:01PM +0100, Nuno Sá wrote:
> On Mon, 2022-02-07 at 21:19 +0100, Nuno Sá wrote:

> I would definetly like to have some settlement on the above (before
> sending a v4). It kind of was discussed a bit already in [1] where I
> felt we had to live with OF for this driver (that is why I kept OF in
> v3. With the above, I cannot
> really see how we can have device API with also including OF... If I
> missing something please let me know :)

Sorry for the delay, answered to your previous message.

> [1]: https://lore.kernel.org/all/CAHp75VczFs8QpsY7tuB-h4X=H54nyjABA4qDSmpQ+FRYAHZdrA@mail.gmail.com/

On Mon, 2022-02-14 at 15:50 +0200, Andy Shevchenko wrote:
> On Mon, Feb 14, 2022 at 02:23:01PM +0100, Nuno Sá wrote:
> > On Mon, 2022-02-07 at 21:19 +0100, Nuno Sá wrote:
> 
> > I would definetly like to have some settlement on the above (before
> > sending a v4). It kind of was discussed a bit already in [1] where
> > I
> > felt we had to live with OF for this driver (that is why I kept OF
> > in
> > v3. With the above, I cannot
> > really see how we can have device API with also including OF... If
> > I
> > missing something please let me know :)
> 
> Sorry for the delay, answered to your previous message.

No worries. As I already said, I'm not doing much till the end of the
month but I definetly wanted the device vs OF question settled before
starting v4.

- Nuno Sá
>

On Mon, 2022-02-14 at 15:49 +0200, Andy Shevchenko wrote:
> On Mon, Feb 07, 2022 at 09:19:46PM +0100, Nuno Sá wrote:
> > On Mon, 2022-02-07 at 13:09 +0200, Andy Shevchenko wrote:
> > > On Sun, Feb 06, 2022 at 01:19:59PM +0000, Sa, Nuno wrote:
> > > > > From: Andy Shevchenko <andriy.shevchenko@intel.com>
> > > > > Sent: Saturday, February 5, 2022 6:30 PM
> > > > > On Fri, Jan 21, 2022 at 03:24:59PM +0100, Nuno Sá wrote:
> 
> ...
> 
> > > > > Second, why do you need this specific function instead of
> > > > > regmap
> > > > > bulk
> > > > > ops against be24/le24?
> > > > 
> > > > Not sure I'm following this one... If you mean why am I using a
> > > > custom 
> > > > regmap_bus implementation, that was already explained in the
> > > > RFC
> > > > patch.
> > > > And IIRC, you were the one already asking 😉.
> > > 
> > > Hmm... It was some time I have looked there. Any message ID to
> > > share,
> > > so
> > > I can find it quickly?
> 
> > https://lore.kernel.org/all/20211112152235.12fdcc49@jic23-huawei/
> 
> Thanks!
> 
> So, it's all about cs_change, right?
> But doesn't bulk operation work exactly as we need here?
> 

Yes... that and we need to send the NOOP command in the second TX
transfer.

> Looking again to the RFC code, it seems like we can still do it
> 
> First, you call _gather_write() followed by _read(). It will show
> exactly what
> you do, i.e. you send command first with the value 0x0000, followed
> by sending
> command and reading back the value at the same time.
> 
> Would it work?

Well, _gather_write() are 2 spi transfers only with TX set. That means
that only on the _read() (which will be another spi_message) we will
ask for the data. Im not really sure this would work being it on a
different message. This would also mean, one extra dummy transfer. To
me that already feels that a custom bus implementation is not a bad
idea...
> 
> ...
> 
> > > > > > +       ret = kstrtou16(buf, 10, &val);
> > > > > 
> > > > > In other function you have long, here u16. I would expect
> > > > > that
> > > > > the
> > > > > types are of
> > > > > the same class, e.g. if here you have u16, then there
> > > > > something
> > > > > like
> > > > > s32 / s64.
> > > > > Or here something like unsigned short.
> > > > > 
> > > > > A bit of elaboration why u16 is chosen here?
> > > > 
> > > > Well, I never really saw any enforcement here to be honest
> > > > (rather
> > > > than using
> > > > stdint types...). So I pretty much just use these in unsigned
> > > > types
> > > > because
> > > > I'm lazy and u16 is faster to type than unsigned short... In
> > > > this
> > > > case, unless Jonathan
> > > > really asks for it, I prefer not to go all over the driver and
> > > > change this...
> > > 
> > > This is about consistency. It may work as is, but it feels not
> > > good
> > > when for
> > > int (or unsigned int) one uses fixed-width types. Also it's non-
> > > written advice
> > > to use fixed-width variables when it's about programming
> > > registers or
> > > so, for
> > > the rest, use POD types.
> 

Ok, going a bit back in the discussion, you argued that in one place I
was using long while here u16. Well, in the place I'm using long, that
was on purpose because that value is to be compared against an array of
longs (which has to be long because it depends on CCF rates). I guess I
can als0 use s64, but there is also a reason why long was used.

In the u16 case, we really want to have 2 bytes because I'm going to
use that value to write the dac code which is 2 bytes.

> > I can understand your reasoning but again this is something that
> > I never really saw being enforced. So, I'm more than ok to change
> > it
> > if it really becomes something that we will try to "enforce" in
> > IIO.
> > Otherwise it just feels as a random nitpick :).
> 
> No, this is about consistency and common sense. If you define type
> uXX,
> we have an API for that exact type. It's confusing why POD type APIs
> are used with fixed-width types or vise versa.
> 
> Moreover (which is pure theoretical, though) some architectures might
> have no (mutual) equivalency between these types.
> 
> ...
> 
> > > > > > +static int ltc2688_tgp_clk_setup(struct ltc2688_state *st,
> > > > > > +                                struct ltc2688_chan *chan,
> > > > > > +                                struct device_node *np,
> > > > > > int
> > > > > > tgp)
> > > > > > +{
> > > > > > +       unsigned long rate;
> > > > > > +       struct clk *clk;
> > > > > > +       int ret, f;
> > > > > > +
> > > > > > +       clk = devm_get_clk_from_child(&st->spi->dev, np,
> > > > > > NULL);
> > > > > > +       if (IS_ERR(clk))
> > > > > 
> > > > > Make it optional for non-OF, can be done as easy as
> > > > > 
> > > > >         if (IS_ERR(clk)) {
> > > > >                 if (PTR_ERR(clk) == -ENOENT)
> > > > >                         clk = NULL;
> > > > >                 else
> > > > >                         return dev_err_probe(...);
> > > > >         }
> > > > > 
> > > > > > +               return dev_err_probe(&st->spi->dev,
> > > > > > PTR_ERR(clk),
> > > > > > +                                    "failed to get tgp
> > > > > > clk.\n");
> > > > 
> > > > Well, I might be missing the point but I think this is not so
> > > > straight....
> > > > We will only get here if the property " adi,toggle-dither-
> > > > input" is
> > > > given
> > > > in which case having the associated clocks is __mandatory__.
> > > 
> > > Ah, okay, would be a limitation for non-OF platforms.
> > > 
> > > > Hence,
> > > > once we are here, this can never be optional. That said, we
> > > > need
> > > > device_node 
> > > 
> > > That's fine, since CCF is OF-centric API.
> > > 
> > > > and hence of.h
> > > 
> > > Why? This header doesn't bring anything you will use here.
> > 
> > Correct me if Im missing something. AFAIU, the idea is to use
> > 'device_for_each_child_node()' which returns a fwnode_handle. That
> > means, that we will have to pass that to this function and use
> > 'to_of_node()' to pass a device_node to
> > 'devm_get_clk_from_child()'.
> > 
> > This means, we need of.h for 'to_of_node()'...
> 
> Yeah, you are right, but it would be still better since it narrows
> the problem to the CCF calls only.
> 

So, to clear....

In your opinion, you are fine whith using device properties and just
have 'to_of_node()' in this CCF call? I'm fine with it, so if Jonathan
does not have any complain about it, will do like this in v4,

Jonathan, any comment on this one?

- Nuno Sá

On Fri, 18 Feb 2022 14:51:28 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Mon, 2022-02-14 at 15:49 +0200, Andy Shevchenko wrote:
> > On Mon, Feb 07, 2022 at 09:19:46PM +0100, Nuno Sá wrote:  
> > > On Mon, 2022-02-07 at 13:09 +0200, Andy Shevchenko wrote:  
> > > > On Sun, Feb 06, 2022 at 01:19:59PM +0000, Sa, Nuno wrote:  
> > > > > > From: Andy Shevchenko <andriy.shevchenko@intel.com>
> > > > > > Sent: Saturday, February 5, 2022 6:30 PM
> > > > > > On Fri, Jan 21, 2022 at 03:24:59PM +0100, Nuno Sá wrote:  
> > 
> > ...
> >   
> > > > > > Second, why do you need this specific function instead of
> > > > > > regmap
> > > > > > bulk
> > > > > > ops against be24/le24?  
> > > > > 
> > > > > Not sure I'm following this one... If you mean why am I using a
> > > > > custom 
> > > > > regmap_bus implementation, that was already explained in the
> > > > > RFC
> > > > > patch.
> > > > > And IIRC, you were the one already asking 😉.  
> > > > 
> > > > Hmm... It was some time I have looked there. Any message ID to
> > > > share,
> > > > so
> > > > I can find it quickly?  
> >   
> > > https://lore.kernel.org/all/20211112152235.12fdcc49@jic23-huawei/  
> > 
> > Thanks!
> > 
> > So, it's all about cs_change, right?
> > But doesn't bulk operation work exactly as we need here?
> >   
> 
> Yes... that and we need to send the NOOP command in the second TX
> transfer.
> 
> > Looking again to the RFC code, it seems like we can still do it
> > 
> > First, you call _gather_write() followed by _read(). It will show
> > exactly what
> > you do, i.e. you send command first with the value 0x0000, followed
> > by sending
> > command and reading back the value at the same time.
> > 
> > Would it work?  
> 
> Well, _gather_write() are 2 spi transfers only with TX set. That means
> that only on the _read() (which will be another spi_message) we will
> ask for the data. Im not really sure this would work being it on a
> different message. This would also mean, one extra dummy transfer. To
> me that already feels that a custom bus implementation is not a bad
> idea...
> > 
> > ...
> >   
> > > > > > > +       ret = kstrtou16(buf, 10, &val);  
> > > > > > 
> > > > > > In other function you have long, here u16. I would expect
> > > > > > that
> > > > > > the
> > > > > > types are of
> > > > > > the same class, e.g. if here you have u16, then there
> > > > > > something
> > > > > > like
> > > > > > s32 / s64.
> > > > > > Or here something like unsigned short.
> > > > > > 
> > > > > > A bit of elaboration why u16 is chosen here?  
> > > > > 
> > > > > Well, I never really saw any enforcement here to be honest
> > > > > (rather
> > > > > than using
> > > > > stdint types...). So I pretty much just use these in unsigned
> > > > > types
> > > > > because
> > > > > I'm lazy and u16 is faster to type than unsigned short... In
> > > > > this
> > > > > case, unless Jonathan
> > > > > really asks for it, I prefer not to go all over the driver and
> > > > > change this...  
> > > > 
> > > > This is about consistency. It may work as is, but it feels not
> > > > good
> > > > when for
> > > > int (or unsigned int) one uses fixed-width types. Also it's non-
> > > > written advice
> > > > to use fixed-width variables when it's about programming
> > > > registers or
> > > > so, for
> > > > the rest, use POD types.  
> >   
> 
> Ok, going a bit back in the discussion, you argued that in one place I
> was using long while here u16. Well, in the place I'm using long, that
> was on purpose because that value is to be compared against an array of
> longs (which has to be long because it depends on CCF rates). I guess I
> can als0 use s64, but there is also a reason why long was used.
> 
> In the u16 case, we really want to have 2 bytes because I'm going to
> use that value to write the dac code which is 2 bytes.
> 
> > > I can understand your reasoning but again this is something that
> > > I never really saw being enforced. So, I'm more than ok to change
> > > it
> > > if it really becomes something that we will try to "enforce" in
> > > IIO.
> > > Otherwise it just feels as a random nitpick :).  
> > 
> > No, this is about consistency and common sense. If you define type
> > uXX,
> > we have an API for that exact type. It's confusing why POD type APIs
> > are used with fixed-width types or vise versa.
> > 
> > Moreover (which is pure theoretical, though) some architectures might
> > have no (mutual) equivalency between these types.
> > 
> > ...
> >   
> > > > > > > +static int ltc2688_tgp_clk_setup(struct ltc2688_state *st,
> > > > > > > +                                struct ltc2688_chan *chan,
> > > > > > > +                                struct device_node *np,
> > > > > > > int
> > > > > > > tgp)
> > > > > > > +{
> > > > > > > +       unsigned long rate;
> > > > > > > +       struct clk *clk;
> > > > > > > +       int ret, f;
> > > > > > > +
> > > > > > > +       clk = devm_get_clk_from_child(&st->spi->dev, np,
> > > > > > > NULL);
> > > > > > > +       if (IS_ERR(clk))  
> > > > > > 
> > > > > > Make it optional for non-OF, can be done as easy as
> > > > > > 
> > > > > >         if (IS_ERR(clk)) {
> > > > > >                 if (PTR_ERR(clk) == -ENOENT)
> > > > > >                         clk = NULL;
> > > > > >                 else
> > > > > >                         return dev_err_probe(...);
> > > > > >         }
> > > > > >   
> > > > > > > +               return dev_err_probe(&st->spi->dev,
> > > > > > > PTR_ERR(clk),
> > > > > > > +                                    "failed to get tgp
> > > > > > > clk.\n");  
> > > > > 
> > > > > Well, I might be missing the point but I think this is not so
> > > > > straight....
> > > > > We will only get here if the property " adi,toggle-dither-
> > > > > input" is
> > > > > given
> > > > > in which case having the associated clocks is __mandatory__.  
> > > > 
> > > > Ah, okay, would be a limitation for non-OF platforms.
> > > >   
> > > > > Hence,
> > > > > once we are here, this can never be optional. That said, we
> > > > > need
> > > > > device_node   
> > > > 
> > > > That's fine, since CCF is OF-centric API.
> > > >   
> > > > > and hence of.h  
> > > > 
> > > > Why? This header doesn't bring anything you will use here.  
> > > 
> > > Correct me if Im missing something. AFAIU, the idea is to use
> > > 'device_for_each_child_node()' which returns a fwnode_handle. That
> > > means, that we will have to pass that to this function and use
> > > 'to_of_node()' to pass a device_node to
> > > 'devm_get_clk_from_child()'.
> > > 
> > > This means, we need of.h for 'to_of_node()'...  
> > 
> > Yeah, you are right, but it would be still better since it narrows
> > the problem to the CCF calls only.
> >   
> 
> So, to clear....
> 
> In your opinion, you are fine whith using device properties and just
> have 'to_of_node()' in this CCF call? I'm fine with it, so if Jonathan
> does not have any complain about it, will do like this in v4,
> 
> Jonathan, any comment on this one?

Whilst it's less than ideal, I'm fine with it being all generic except
for the clock part and using to_of_node() which I think is what Andy
is suggesting.

Thanks,

Jonathan


> 
> - Nuno Sá
>

On Sat, 2022-02-05 at 19:29 +0200, Andy Shevchenko wrote:
> On Fri, Jan 21, 2022 at 03:24:59PM +0100, 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.
> 
> ...
> 
> > +#include <linux/of.h>
> 
> property.h please/
> 
> ...
> 
> > +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 = st->tx_data,
> > +                       .bits_per_word = 8,
> > +                       .len = 3,
> > +                       .cs_change = 1,
> > +               }, {
> > +                       .tx_buf = st->tx_data + 3,
> > +                       .rx_buf = st->rx_data,
> > +                       .bits_per_word = 8,
> > +                       .len = 3,
> > +               },
> > +       };
> > +       int ret;
> 
> > +       memcpy(st->tx_data, reg, reg_size);
> > +
> > +       ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> > +       if (ret)
> > +               return ret;
> > +
> > +       memcpy(val, &st->rx_data[1], val_size);
> > +
> > +       return 0;
> > +}
> 
> First of all, yuo have fixed len in transfer sizes, so what the
> purpose of the reg_size / val_size?
> Second, why do you need this specific function instead of regmap bulk
> ops against be24/le24?
> 
> ...
> 
> > +unlock:
> 
> out_unlock: ?
> (And in similar cases)
> 
> ...
> 
> > +       if (ret)
> > +               return ret;
> > +
> > +       return len;
> 
> In some cases the return ret ?: len; is used, in some like above.
> Maybe a bit
> of consistency?
> 

Hmm, when doing some changes for v4 I realized why I used the ternary
operator here (typically I'm not a fan). The thing is that we already
check the error condition after calling regmap_update_bits() which is
not the last code executed. Hence, I didn't want to do again

if (ret)
	return ret

after unlocking the mutex.

In the other places the error check is always on the last lines where
nothing else will happen (either return error or len). 

Alternatively, to remove the ternary operator, I would prefer to
actually remove the label and goto and after regmap_update_bits(), do:

if (ret) {
	mutex_unlock();
	return ret;
}

It might be not consistent with other places were goto is used but this
function also has it's differencies...

- Nuno Sá

> ...
> 
> > +       if (private == LTC2688_INPUT_B_AVAIL)
> > +               return sysfs_emit(buf, "[%u %u %u]\n",
> > ltc2688_raw_range[0],
> > +                                 ltc2688_raw_range[1],
> > +                                 ltc2688_raw_range[2] / 4);
> 
> Is it standard form "[A B C]" for ranges in IIO? I haven't looked
> into the code
> deeply (and datasheet at all) to understand meaning. To me range is
> usually out
> of two numbers.
> 
> > +       if (private == LTC2688_DITHER_OFF)
> > +               return sysfs_emit(buf, "0\n");
> 
> > +       ret = ltc2688_dac_code_read(st, chan->channel, private,
> > &val);
> > +       if (ret)
> > +               return ret;
> > +
> > +       return sysfs_emit(buf, "%u\n", val);
> 
> These three types of output for one sysfs node? Seems it's not align
> with the
> idea behind sysfs. It maybe that I missed something.
> 
> ...
> 
> > +       ret = kstrtou16(buf, 10, &val);
> 
> In other function you have long, here u16. I would expect that the
> types are of
> the same class, e.g. if here you have u16, then there something like
> s32 / s64.
> Or here something like unsigned short.
> 
> A bit of elaboration why u16 is chosen here?
> 
> ...
> 
> > +       .info_mask_separate_available =
> > BIT(IIO_CHAN_INFO_RAW),         \
> > +       .ext_info =
> > ltc2688_ext_info                                    \
> 
> + Comma
> 
> ...
> 
> > +static int ltc2688_tgp_clk_setup(struct ltc2688_state *st,
> > +                                struct ltc2688_chan *chan,
> > +                                struct device_node *np, int tgp)
> > +{
> > +       unsigned long rate;
> > +       struct clk *clk;
> > +       int ret, f;
> > +
> > +       clk = devm_get_clk_from_child(&st->spi->dev, np, NULL);
> > +       if (IS_ERR(clk))
> 
> Make it optional for non-OF, can be done as easy as
> 
>         if (IS_ERR(clk)) {
>                 if (PTR_ERR(clk) == -ENOENT)
>                         clk = NULL;
>                 else
>                         return dev_err_probe(...);
>         }
> 
> > +               return dev_err_probe(&st->spi->dev, PTR_ERR(clk),
> > +                                    "failed to get tgp clk.\n");
> > +
> > +       ret = clk_prepare_enable(clk);
> > +       if (ret)
> > +               return dev_err_probe(&st->spi->dev, ret,
> > +                                    "failed to enable tgp
> > clk.\n");
> > +
> > +       ret = devm_add_action_or_reset(&st->spi->dev,
> > ltc2688_clk_disable, clk);
> > +       if (ret)
> > +               return ret;
> > +
> > +       if (chan->toggle_chan)
> > +               return 0;
> > +
> > +       /* calculate available dither frequencies */
> > +       rate = clk_get_rate(clk);
> > +       for (f = 0; f < ARRAY_SIZE(chan->dither_frequency); f++)
> > +               chan->dither_frequency[f] = DIV_ROUND_CLOSEST(rate,
> > ltc2688_period[f]);
> > +
> > +       return 0;
> > +}
> 
> ...
> 
> > +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) {
> 
> device_for_each_child_node()
> 
> > +               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);
> 
> Do you need atomic operation here?
> 
> > +               }
> > +
> > +               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;
> > +}
> 
> ...
> 
> > +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
> 
> + Comma.
> 
> > +};
> > +
> > +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
> 
> Ditto.
> 
> > +};
> 
> ...
> 
> > +       vref_reg = devm_regulator_get_optional(dev, "vref");
> 
> > +       if (!IS_ERR(vref_reg)) {
> 
> Why not positive conditional check (and hence standard pattern --
> error
> handling first)?
> 
> > +               ret = regulator_enable(vref_reg);
> > +               if (ret)
> > +                       return dev_err_probe(dev, ret,
> > +                                            "Failed to enable vref
> > regulators\n");
> > +
> > +               ret = devm_add_action_or_reset(dev,
> > ltc2688_disable_regulator,
> > +                                              vref_reg);
> > +               if (ret)
> > +                       return ret;
> > +
> > +               ret = regulator_get_voltage(vref_reg);
> > +               if (ret < 0)
> > +                       return dev_err_probe(dev, ret, "Failed to
> > get vref\n");
> > +
> > +               st->vref = ret / 1000;
> > +       } else {
> > +               if (PTR_ERR(vref_reg) != -ENODEV)
> > +                       return dev_err_probe(dev,
> > PTR_ERR(vref_reg),
> > +                                            "Failed to get vref
> > regulator");
> > +
> > +               vref_reg = NULL;
> > +               /* internal reference */
> > +               st->vref = 4096;
> > +       }
>

On Fri, Feb 18, 2022 at 02:51:28PM +0100, Nuno Sá wrote:
> On Mon, 2022-02-14 at 15:49 +0200, Andy Shevchenko wrote:
> > On Mon, Feb 07, 2022 at 09:19:46PM +0100, Nuno Sá wrote:
> > > On Mon, 2022-02-07 at 13:09 +0200, Andy Shevchenko wrote:
> > > > On Sun, Feb 06, 2022 at 01:19:59PM +0000, Sa, Nuno wrote:
> > > > > > From: Andy Shevchenko <andriy.shevchenko@intel.com>
> > > > > > Sent: Saturday, February 5, 2022 6:30 PM
> > > > > > On Fri, Jan 21, 2022 at 03:24:59PM +0100, Nuno Sá wrote:

...

> > > > > > Second, why do you need this specific function instead of
> > > > > > regmap
> > > > > > bulk
> > > > > > ops against be24/le24?
> > > > > 
> > > > > Not sure I'm following this one... If you mean why am I using a
> > > > > custom 
> > > > > regmap_bus implementation, that was already explained in the
> > > > > RFC
> > > > > patch.
> > > > > And IIRC, you were the one already asking 😉.
> > > > 
> > > > Hmm... It was some time I have looked there. Any message ID to
> > > > share,
> > > > so
> > > > I can find it quickly?
> > 
> > > https://lore.kernel.org/all/20211112152235.12fdcc49@jic23-huawei/
> > 
> > Thanks!
> > 
> > So, it's all about cs_change, right?
> > But doesn't bulk operation work exactly as we need here?
> > 
> 
> Yes... that and we need to send the NOOP command in the second TX
> transfer.
> 
> > Looking again to the RFC code, it seems like we can still do it
> > 
> > First, you call _gather_write() followed by _read(). It will show
> > exactly what
> > you do, i.e. you send command first with the value 0x0000, followed
> > by sending
> > command and reading back the value at the same time.
> > 
> > Would it work?
> 
> Well, _gather_write() are 2 spi transfers only with TX set. That means
> that only on the _read() (which will be another spi_message) we will
> ask for the data. Im not really sure this would work being it on a
> different message. This would also mean, one extra dummy transfer. To
> me that already feels that a custom bus implementation is not a bad
> idea...

I see, okay, what Jonothan decides then. Still I'm not convinced.

...

> > > > > > > +       ret = kstrtou16(buf, 10, &val);
> > > > > > 
> > > > > > In other function you have long, here u16. I would expect
> > > > > > that
> > > > > > the
> > > > > > types are of
> > > > > > the same class, e.g. if here you have u16, then there
> > > > > > something
> > > > > > like
> > > > > > s32 / s64.
> > > > > > Or here something like unsigned short.
> > > > > > 
> > > > > > A bit of elaboration why u16 is chosen here?
> > > > > 
> > > > > Well, I never really saw any enforcement here to be honest
> > > > > (rather
> > > > > than using
> > > > > stdint types...). So I pretty much just use these in unsigned
> > > > > types
> > > > > because
> > > > > I'm lazy and u16 is faster to type than unsigned short... In
> > > > > this
> > > > > case, unless Jonathan
> > > > > really asks for it, I prefer not to go all over the driver and
> > > > > change this...
> > > > 
> > > > This is about consistency. It may work as is, but it feels not
> > > > good
> > > > when for
> > > > int (or unsigned int) one uses fixed-width types. Also it's non-
> > > > written advice
> > > > to use fixed-width variables when it's about programming
> > > > registers or
> > > > so, for
> > > > the rest, use POD types.
> 
> Ok, going a bit back in the discussion, you argued that in one place I
> was using long while here u16. Well, in the place I'm using long, that
> was on purpose because that value is to be compared against an array of
> longs (which has to be long because it depends on CCF rates). I guess I
> can als0 use s64, but there is also a reason why long was used.
> 
> In the u16 case, we really want to have 2 bytes because I'm going to
> use that value to write the dac code which is 2 bytes.

Okay, that's what I want to hear. If it's indeed goes to be a value to the
register, then it's fine.

Perhaps a comment?

> > > I can understand your reasoning but again this is something that
> > > I never really saw being enforced. So, I'm more than ok to change
> > > it
> > > if it really becomes something that we will try to "enforce" in
> > > IIO.
> > > Otherwise it just feels as a random nitpick :).
> > 
> > No, this is about consistency and common sense. If you define type
> > uXX,
> > we have an API for that exact type. It's confusing why POD type APIs
> > are used with fixed-width types or vise versa.
> > 
> > Moreover (which is pure theoretical, though) some architectures might
> > have no (mutual) equivalency between these types.

...

> > > > > > > +static int ltc2688_tgp_clk_setup(struct ltc2688_state *st,
> > > > > > > +                                struct ltc2688_chan *chan,
> > > > > > > +                                struct device_node *np,
> > > > > > > int
> > > > > > > tgp)
> > > > > > > +{
> > > > > > > +       unsigned long rate;
> > > > > > > +       struct clk *clk;
> > > > > > > +       int ret, f;
> > > > > > > +
> > > > > > > +       clk = devm_get_clk_from_child(&st->spi->dev, np,
> > > > > > > NULL);
> > > > > > > +       if (IS_ERR(clk))
> > > > > > 
> > > > > > Make it optional for non-OF, can be done as easy as
> > > > > > 
> > > > > >         if (IS_ERR(clk)) {
> > > > > >                 if (PTR_ERR(clk) == -ENOENT)
> > > > > >                         clk = NULL;
> > > > > >                 else
> > > > > >                         return dev_err_probe(...);
> > > > > >         }
> > > > > > 
> > > > > > > +               return dev_err_probe(&st->spi->dev,
> > > > > > > PTR_ERR(clk),
> > > > > > > +                                    "failed to get tgp
> > > > > > > clk.\n");
> > > > > 
> > > > > Well, I might be missing the point but I think this is not so
> > > > > straight....
> > > > > We will only get here if the property " adi,toggle-dither-
> > > > > input" is
> > > > > given
> > > > > in which case having the associated clocks is __mandatory__.
> > > > 
> > > > Ah, okay, would be a limitation for non-OF platforms.
> > > > 
> > > > > Hence,
> > > > > once we are here, this can never be optional. That said, we
> > > > > need
> > > > > device_node 
> > > > 
> > > > That's fine, since CCF is OF-centric API.
> > > > 
> > > > > and hence of.h
> > > > 
> > > > Why? This header doesn't bring anything you will use here.
> > > 
> > > Correct me if Im missing something. AFAIU, the idea is to use
> > > 'device_for_each_child_node()' which returns a fwnode_handle. That
> > > means, that we will have to pass that to this function and use
> > > 'to_of_node()' to pass a device_node to
> > > 'devm_get_clk_from_child()'.
> > > 
> > > This means, we need of.h for 'to_of_node()'...
> > 
> > Yeah, you are right, but it would be still better since it narrows
> > the problem to the CCF calls only.
> 
> So, to clear....
> 
> In your opinion, you are fine whith using device properties and just
> have 'to_of_node()' in this CCF call? I'm fine with it, so if Jonathan
> does not have any complain about it, will do like this in v4,

Yes, that will show that only CCF is missing the fwnode APIs.

On Sun, 2022-02-20 at 13:32 +0200, Andy Shevchenko wrote:
> On Fri, Feb 18, 2022 at 02:51:28PM +0100, Nuno Sá wrote:
> > On Mon, 2022-02-14 at 15:49 +0200, Andy Shevchenko wrote:
> > > On Mon, Feb 07, 2022 at 09:19:46PM +0100, Nuno Sá wrote:
> > > > On Mon, 2022-02-07 at 13:09 +0200, Andy Shevchenko wrote:
> > > > > On Sun, Feb 06, 2022 at 01:19:59PM +0000, Sa, Nuno wrote:
> > > > > > > From: Andy Shevchenko <andriy.shevchenko@intel.com>
> > > > > > > Sent: Saturday, February 5, 2022 6:30 PM
> > > > > > > On Fri, Jan 21, 2022 at 03:24:59PM +0100, Nuno Sá wrote:
> 
> ...
> 
> > > > > > > Second, why do you need this specific function instead of
> > > > > > > regmap
> > > > > > > bulk
> > > > > > > ops against be24/le24?
> > > > > > 
> > > > > > Not sure I'm following this one... If you mean why am I
> > > > > > using a
> > > > > > custom 
> > > > > > regmap_bus implementation, that was already explained in
> > > > > > the
> > > > > > RFC
> > > > > > patch.
> > > > > > And IIRC, you were the one already asking 😉.
> > > > > 
> > > > > Hmm... It was some time I have looked there. Any message ID
> > > > > to
> > > > > share,
> > > > > so
> > > > > I can find it quickly?
> > > 
> > > > https://lore.kernel.org/all/20211112152235.12fdcc49@jic23-huawei/
> > > 
> > > Thanks!
> > > 
> > > So, it's all about cs_change, right?
> > > But doesn't bulk operation work exactly as we need here?
> > > 
> > 
> > Yes... that and we need to send the NOOP command in the second TX
> > transfer.
> > 
> > > Looking again to the RFC code, it seems like we can still do it
> > > 
> > > First, you call _gather_write() followed by _read(). It will show
> > > exactly what
> > > you do, i.e. you send command first with the value 0x0000,
> > > followed
> > > by sending
> > > command and reading back the value at the same time.
> > > 
> > > Would it work?
> > 
> > Well, _gather_write() are 2 spi transfers only with TX set. That
> > means
> > that only on the _read() (which will be another spi_message) we
> > will
> > ask for the data. Im not really sure this would work being it on a
> > different message. This would also mean, one extra dummy transfer.
> > To
> > me that already feels that a custom bus implementation is not a bad
> > idea...
> 
> I see, okay, what Jonothan decides then. Still I'm not convinced.
> 
> ...
> 
> > > > > > > > +       ret = kstrtou16(buf, 10, &val);
> > > > > > > 
> > > > > > > In other function you have long, here u16. I would expect
> > > > > > > that
> > > > > > > the
> > > > > > > types are of
> > > > > > > the same class, e.g. if here you have u16, then there
> > > > > > > something
> > > > > > > like
> > > > > > > s32 / s64.
> > > > > > > Or here something like unsigned short.
> > > > > > > 
> > > > > > > A bit of elaboration why u16 is chosen here?
> > > > > > 
> > > > > > Well, I never really saw any enforcement here to be honest
> > > > > > (rather
> > > > > > than using
> > > > > > stdint types...). So I pretty much just use these in
> > > > > > unsigned
> > > > > > types
> > > > > > because
> > > > > > I'm lazy and u16 is faster to type than unsigned short...
> > > > > > In
> > > > > > this
> > > > > > case, unless Jonathan
> > > > > > really asks for it, I prefer not to go all over the driver
> > > > > > and
> > > > > > change this...
> > > > > 
> > > > > This is about consistency. It may work as is, but it feels
> > > > > not
> > > > > good
> > > > > when for
> > > > > int (or unsigned int) one uses fixed-width types. Also it's
> > > > > non-
> > > > > written advice
> > > > > to use fixed-width variables when it's about programming
> > > > > registers or
> > > > > so, for
> > > > > the rest, use POD types.
> > 
> > Ok, going a bit back in the discussion, you argued that in one
> > place I
> > was using long while here u16. Well, in the place I'm using long,
> > that
> > was on purpose because that value is to be compared against an
> > array of
> > longs (which has to be long because it depends on CCF rates). I
> > guess I
> > can als0 use s64, but there is also a reason why long was used.
> > 
> > In the u16 case, we really want to have 2 bytes because I'm going
> > to
> > use that value to write the dac code which is 2 bytes.
> 
> Okay, that's what I want to hear. If it's indeed goes to be a value
> to the
> register, then it's fine.
> 
> Perhaps a comment?
> 

I guess you mean to have a comment to state that here we have fixed
size type (as opposed to long, used in another place), because we
directly use the value on a register write?

Asking it because I'm not planning to add comments in all the places
where I have fixed size types for register read/writes...

> > > > I can understand your reasoning but again this is something
> > > > that
> > > > I never really saw being enforced. So, I'm more than ok to
> > > > change
> > > > it
> > > > if it really becomes something that we will try to "enforce" in
> > > > IIO.
> > > > Otherwise it just feels as a random nitpick :).
> > > 
> > > No, this is about consistency and common sense. If you define
> > > type
> > > uXX,
> > > we have an API for that exact type. It's confusing why POD type
> > > APIs
> > > are used with fixed-width types or vise versa.
> > > 
> > > Moreover (which is pure theoretical, though) some architectures
> > > might
> > > have no (mutual) equivalency between these types.
> 
> ...
> 
> > > > > > > > +static int ltc2688_tgp_clk_setup(struct ltc2688_state
> > > > > > > > *st,
> > > > > > > > +                                struct ltc2688_chan
> > > > > > > > *chan,
> > > > > > > > +                                struct device_node
> > > > > > > > *np,
> > > > > > > > int
> > > > > > > > tgp)
> > > > > > > > +{
> > > > > > > > +       unsigned long rate;
> > > > > > > > +       struct clk *clk;
> > > > > > > > +       int ret, f;
> > > > > > > > +
> > > > > > > > +       clk = devm_get_clk_from_child(&st->spi->dev,
> > > > > > > > np,
> > > > > > > > NULL);
> > > > > > > > +       if (IS_ERR(clk))
> > > > > > > 
> > > > > > > Make it optional for non-OF, can be done as easy as
> > > > > > > 
> > > > > > >         if (IS_ERR(clk)) {
> > > > > > >                 if (PTR_ERR(clk) == -ENOENT)
> > > > > > >                         clk = NULL;
> > > > > > >                 else
> > > > > > >                         return dev_err_probe(...);
> > > > > > >         }
> > > > > > > 
> > > > > > > > +               return dev_err_probe(&st->spi->dev,
> > > > > > > > PTR_ERR(clk),
> > > > > > > > +                                    "failed to get tgp
> > > > > > > > clk.\n");
> > > > > > 
> > > > > > Well, I might be missing the point but I think this is not
> > > > > > so
> > > > > > straight....
> > > > > > We will only get here if the property " adi,toggle-dither-
> > > > > > input" is
> > > > > > given
> > > > > > in which case having the associated clocks is
> > > > > > __mandatory__.
> > > > > 
> > > > > Ah, okay, would be a limitation for non-OF platforms.
> > > > > 
> > > > > > Hence,
> > > > > > once we are here, this can never be optional. That said, we
> > > > > > need
> > > > > > device_node 
> > > > > 
> > > > > That's fine, since CCF is OF-centric API.
> > > > > 
> > > > > > and hence of.h
> > > > > 
> > > > > Why? This header doesn't bring anything you will use here.
> > > > 
> > > > Correct me if Im missing something. AFAIU, the idea is to use
> > > > 'device_for_each_child_node()' which returns a fwnode_handle.
> > > > That
> > > > means, that we will have to pass that to this function and use
> > > > 'to_of_node()' to pass a device_node to
> > > > 'devm_get_clk_from_child()'.
> > > > 
> > > > This means, we need of.h for 'to_of_node()'...
> > > 
> > > Yeah, you are right, but it would be still better since it
> > > narrows
> > > the problem to the CCF calls only.
> > 
> > So, to clear....
> > 
> > In your opinion, you are fine whith using device properties and
> > just
> > have 'to_of_node()' in this CCF call? I'm fine with it, so if
> > Jonathan
> > does not have any complain about it, will do like this in v4,
> 
> Yes, that will show that only CCF is missing the fwnode APIs.
> 

Ok, it's settled then...

- Nuno Sá

On Mon, Feb 21, 2022 at 01:48:12PM +0100, Nuno Sá wrote:
> On Sun, 2022-02-20 at 13:32 +0200, Andy Shevchenko wrote:
> > On Fri, Feb 18, 2022 at 02:51:28PM +0100, Nuno Sá wrote:
> > > On Mon, 2022-02-14 at 15:49 +0200, Andy Shevchenko wrote:
> > > > On Mon, Feb 07, 2022 at 09:19:46PM +0100, Nuno Sá wrote:
> > > > > On Mon, 2022-02-07 at 13:09 +0200, Andy Shevchenko wrote:
> > > > > > On Sun, Feb 06, 2022 at 01:19:59PM +0000, Sa, Nuno wrote:
> > > > > > > > From: Andy Shevchenko <andriy.shevchenko@intel.com>
> > > > > > > > Sent: Saturday, February 5, 2022 6:30 PM
> > > > > > > > On Fri, Jan 21, 2022 at 03:24:59PM +0100, Nuno Sá wrote:

...

> > > > > > > > > +       ret = kstrtou16(buf, 10, &val);
> > > > > > > > 
> > > > > > > > In other function you have long, here u16. I would expect that
> > > > > > > > the types are of the same class, e.g. if here you have u16,
> > > > > > > > then there something like s32 / s64.  Or here something like
> > > > > > > > unsigned short.
> > > > > > > > 
> > > > > > > > A bit of elaboration why u16 is chosen here?
> > > > > > > 
> > > > > > > Well, I never really saw any enforcement here to be honest
> > > > > > > (rather than using stdint types...). So I pretty much just use
> > > > > > > these in unsigned types because I'm lazy and u16 is faster to
> > > > > > > type than unsigned short...  In this case, unless Jonathan really
> > > > > > > asks for it, I prefer not to go all over the driver and change
> > > > > > > this...
> > > > > > 
> > > > > > This is about consistency. It may work as is, but it feels not good
> > > > > > when for int (or unsigned int) one uses fixed-width types. Also
> > > > > > it's non- written advice to use fixed-width variables when it's
> > > > > > about programming registers or so, for the rest, use POD types.
> > > 
> > > Ok, going a bit back in the discussion, you argued that in one place I
> > > was using long while here u16. Well, in the place I'm using long, that
> > > was on purpose because that value is to be compared against an array of
> > > longs (which has to be long because it depends on CCF rates). I guess I
> > > can als0 use s64, but there is also a reason why long was used.
> > > 
> > > In the u16 case, we really want to have 2 bytes because I'm going to use
> > > that value to write the dac code which is 2 bytes.
> > 
> > Okay, that's what I want to hear. If it's indeed goes to be a value to the
> > register, then it's fine.
> > 
> > Perhaps a comment?
> 
> I guess you mean to have a comment to state that here we have fixed
> size type (as opposed to long, used in another place), because we
> directly use the value on a register write?
> 
> Asking it because I'm not planning to add comments in all the places
> where I have fixed size types for register read/writes...

Thinking more about it and now I'm convinced that using the value that goes to
the register in ABI is bad idea (means that user space must not care about the
size or contents of the hardware register and should be abstract representation
of the HW).

OTOH this seems to be "raw" value of something. So, I maybe missed the convention
in IIO about this kind of values WRT the variable types used on ABI side.

That said, I leave it to Jonathan since I'm not convinced that u16 is a proper
choice here.

> > > > > I can understand your reasoning but again this is something that I
> > > > > never really saw being enforced. So, I'm more than ok to change it if
> > > > > it really becomes something that we will try to "enforce" in IIO.
> > > > > Otherwise it just feels as a random nitpick :).
> > > > 
> > > > No, this is about consistency and common sense. If you define type uXX,
> > > > we have an API for that exact type. It's confusing why POD type APIs
> > > > are used with fixed-width types or vise versa.
> > > > 
> > > > Moreover (which is pure theoretical, though) some architectures might
> > > > have no (mutual) equivalency between these types.

On Mon, 21 Feb 2022 19:04:38 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Mon, Feb 21, 2022 at 01:48:12PM +0100, Nuno Sá wrote:
> > On Sun, 2022-02-20 at 13:32 +0200, Andy Shevchenko wrote:  
> > > On Fri, Feb 18, 2022 at 02:51:28PM +0100, Nuno Sá wrote:  
> > > > On Mon, 2022-02-14 at 15:49 +0200, Andy Shevchenko wrote:  
> > > > > On Mon, Feb 07, 2022 at 09:19:46PM +0100, Nuno Sá wrote:  
> > > > > > On Mon, 2022-02-07 at 13:09 +0200, Andy Shevchenko wrote:  
> > > > > > > On Sun, Feb 06, 2022 at 01:19:59PM +0000, Sa, Nuno wrote:  
> > > > > > > > > From: Andy Shevchenko <andriy.shevchenko@intel.com>
> > > > > > > > > Sent: Saturday, February 5, 2022 6:30 PM
> > > > > > > > > On Fri, Jan 21, 2022 at 03:24:59PM +0100, Nuno Sá wrote:  
> 
> ...
> 
> > > > > > > > > > +       ret = kstrtou16(buf, 10, &val);  
> > > > > > > > > 
> > > > > > > > > In other function you have long, here u16. I would expect that
> > > > > > > > > the types are of the same class, e.g. if here you have u16,
> > > > > > > > > then there something like s32 / s64.  Or here something like
> > > > > > > > > unsigned short.
> > > > > > > > > 
> > > > > > > > > A bit of elaboration why u16 is chosen here?  
> > > > > > > > 
> > > > > > > > Well, I never really saw any enforcement here to be honest
> > > > > > > > (rather than using stdint types...). So I pretty much just use
> > > > > > > > these in unsigned types because I'm lazy and u16 is faster to
> > > > > > > > type than unsigned short...  In this case, unless Jonathan really
> > > > > > > > asks for it, I prefer not to go all over the driver and change
> > > > > > > > this...  
> > > > > > > 
> > > > > > > This is about consistency. It may work as is, but it feels not good
> > > > > > > when for int (or unsigned int) one uses fixed-width types. Also
> > > > > > > it's non- written advice to use fixed-width variables when it's
> > > > > > > about programming registers or so, for the rest, use POD types.  
> > > > 
> > > > Ok, going a bit back in the discussion, you argued that in one place I
> > > > was using long while here u16. Well, in the place I'm using long, that
> > > > was on purpose because that value is to be compared against an array of
> > > > longs (which has to be long because it depends on CCF rates). I guess I
> > > > can als0 use s64, but there is also a reason why long was used.
> > > > 
> > > > In the u16 case, we really want to have 2 bytes because I'm going to use
> > > > that value to write the dac code which is 2 bytes.  
> > > 
> > > Okay, that's what I want to hear. If it's indeed goes to be a value to the
> > > register, then it's fine.
> > > 
> > > Perhaps a comment?  
> > 
> > I guess you mean to have a comment to state that here we have fixed
> > size type (as opposed to long, used in another place), because we
> > directly use the value on a register write?
> > 
> > Asking it because I'm not planning to add comments in all the places
> > where I have fixed size types for register read/writes...  
> 
> Thinking more about it and now I'm convinced that using the value that goes to
> the register in ABI is bad idea (means that user space must not care about the
> size or contents of the hardware register and should be abstract representation
> of the HW).
> 
> OTOH this seems to be "raw" value of something. So, I maybe missed the convention
> in IIO about this kind of values WRT the variable types used on ABI side.
> 
> That said, I leave it to Jonathan since I'm not convinced that u16 is a proper
> choice here.

From a userspace point of view it doesn't care as it's writing a string.
In this particular case the string only has valid values that from 0-(2^16-1)
(i.e. 16 bits).  So if it writes outside of that range it is an error.
You could read it into an unsigned long and then check against the range,
but there is little point given you'd still return an error if it was out of
range.  The fact that kstrto16() does that for you really just a shortcut
though it will return -ERANGE rather than perhaps -EINVAL which might be used
for a more generic "not this value".

Userspace can also read the range that is acceptable from
out_voltage0_raw_available [0 1 2^16-1] and hence not write an invalid value
in the first place - which is obviously preferred to getting an error.
Scaling etc is also expressed to userspace so it it wants to write a particular
voltage it can perform the appropriate scaling. Note that moving linear scaling
like this to userspace allows easy use of floating point + may be a significant
performance advantage if using the chrdev interface which uses the same
approach (and values) as the sysfs interface.

Jonathan


 
> 
> > > > > > I can understand your reasoning but again this is something that I
> > > > > > never really saw being enforced. So, I'm more than ok to change it if
> > > > > > it really becomes something that we will try to "enforce" in IIO.
> > > > > > Otherwise it just feels as a random nitpick :).  
> > > > > 
> > > > > No, this is about consistency and common sense. If you define type uXX,
> > > > > we have an API for that exact type. It's confusing why POD type APIs
> > > > > are used with fixed-width types or vise versa.
> > > > > 
> > > > > Moreover (which is pure theoretical, though) some architectures might
> > > > > have no (mutual) equivalency between these types.  
>

On Mon, Feb 21, 2022 at 05:30:45PM +0000, Jonathan Cameron wrote:
> On Mon, 21 Feb 2022 19:04:38 +0200
> Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> 
> > On Mon, Feb 21, 2022 at 01:48:12PM +0100, Nuno Sá wrote:
> > > On Sun, 2022-02-20 at 13:32 +0200, Andy Shevchenko wrote:  
> > > > On Fri, Feb 18, 2022 at 02:51:28PM +0100, Nuno Sá wrote:  
> > > > > On Mon, 2022-02-14 at 15:49 +0200, Andy Shevchenko wrote:  
> > > > > > On Mon, Feb 07, 2022 at 09:19:46PM +0100, Nuno Sá wrote:  
> > > > > > > On Mon, 2022-02-07 at 13:09 +0200, Andy Shevchenko wrote:  
> > > > > > > > On Sun, Feb 06, 2022 at 01:19:59PM +0000, Sa, Nuno wrote:  
> > > > > > > > > > From: Andy Shevchenko <andriy.shevchenko@intel.com>
> > > > > > > > > > Sent: Saturday, February 5, 2022 6:30 PM
> > > > > > > > > > On Fri, Jan 21, 2022 at 03:24:59PM +0100, Nuno Sá wrote:  
> > 
> > ...
> > 
> > > > > > > > > > > +       ret = kstrtou16(buf, 10, &val);  
> > > > > > > > > > 
> > > > > > > > > > In other function you have long, here u16. I would expect that
> > > > > > > > > > the types are of the same class, e.g. if here you have u16,
> > > > > > > > > > then there something like s32 / s64.  Or here something like
> > > > > > > > > > unsigned short.
> > > > > > > > > > 
> > > > > > > > > > A bit of elaboration why u16 is chosen here?  
> > > > > > > > > 
> > > > > > > > > Well, I never really saw any enforcement here to be honest
> > > > > > > > > (rather than using stdint types...). So I pretty much just use
> > > > > > > > > these in unsigned types because I'm lazy and u16 is faster to
> > > > > > > > > type than unsigned short...  In this case, unless Jonathan really
> > > > > > > > > asks for it, I prefer not to go all over the driver and change
> > > > > > > > > this...  
> > > > > > > > 
> > > > > > > > This is about consistency. It may work as is, but it feels not good
> > > > > > > > when for int (or unsigned int) one uses fixed-width types. Also
> > > > > > > > it's non- written advice to use fixed-width variables when it's
> > > > > > > > about programming registers or so, for the rest, use POD types.  
> > > > > 
> > > > > Ok, going a bit back in the discussion, you argued that in one place I
> > > > > was using long while here u16. Well, in the place I'm using long, that
> > > > > was on purpose because that value is to be compared against an array of
> > > > > longs (which has to be long because it depends on CCF rates). I guess I
> > > > > can als0 use s64, but there is also a reason why long was used.
> > > > > 
> > > > > In the u16 case, we really want to have 2 bytes because I'm going to use
> > > > > that value to write the dac code which is 2 bytes.  
> > > > 
> > > > Okay, that's what I want to hear. If it's indeed goes to be a value to the
> > > > register, then it's fine.
> > > > 
> > > > Perhaps a comment?  
> > > 
> > > I guess you mean to have a comment to state that here we have fixed
> > > size type (as opposed to long, used in another place), because we
> > > directly use the value on a register write?
> > > 
> > > Asking it because I'm not planning to add comments in all the places
> > > where I have fixed size types for register read/writes...  
> > 
> > Thinking more about it and now I'm convinced that using the value that goes to
> > the register in ABI is bad idea (means that user space must not care about the
> > size or contents of the hardware register and should be abstract representation
> > of the HW).
> > 
> > OTOH this seems to be "raw" value of something. So, I maybe missed the convention
> > in IIO about this kind of values WRT the variable types used on ABI side.
> > 
> > That said, I leave it to Jonathan since I'm not convinced that u16 is a proper
> > choice here.
> 
> From a userspace point of view it doesn't care as it's writing a string.
> In this particular case the string only has valid values that from 0-(2^16-1)
> (i.e. 16 bits).  So if it writes outside of that range it is an error.
> You could read it into an unsigned long and then check against the range,
> but there is little point given you'd still return an error if it was out of
> range.  The fact that kstrto16() does that for you really just a shortcut
> though it will return -ERANGE rather than perhaps -EINVAL which might be used
> for a more generic "not this value".
> 
> Userspace can also read the range that is acceptable from
> out_voltage0_raw_available [0 1 2^16-1] and hence not write an invalid value
> in the first place - which is obviously preferred to getting an error.
> Scaling etc is also expressed to userspace so it it wants to write a particular
> voltage it can perform the appropriate scaling. Note that moving linear scaling
> like this to userspace allows easy use of floating point + may be a significant
> performance advantage if using the chrdev interface which uses the same
> approach (and values) as the sysfs interface.

With the same logic it can be unsigned short, no?

The point is to use u16 when it's indeed fixed-width value that goes to
hardware or being used as part of a protocol. And thus mentioning of the
IOCTL protocols may justify the choice. Then the question to the other
values, shouldn't they be also fixed-width ones?

> > > > > > > I can understand your reasoning but again this is something that I
> > > > > > > never really saw being enforced. So, I'm more than ok to change it if
> > > > > > > it really becomes something that we will try to "enforce" in IIO.
> > > > > > > Otherwise it just feels as a random nitpick :).  
> > > > > > 
> > > > > > No, this is about consistency and common sense. If you define type uXX,
> > > > > > we have an API for that exact type. It's confusing why POD type APIs
> > > > > > are used with fixed-width types or vise versa.
> > > > > > 
> > > > > > Moreover (which is pure theoretical, though) some architectures might
> > > > > > have no (mutual) equivalency between these types.

On Mon, 21 Feb 2022 20:49:48 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Mon, Feb 21, 2022 at 05:30:45PM +0000, Jonathan Cameron wrote:
> > On Mon, 21 Feb 2022 19:04:38 +0200
> > Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> >   
> > > On Mon, Feb 21, 2022 at 01:48:12PM +0100, Nuno Sá wrote:  
> > > > On Sun, 2022-02-20 at 13:32 +0200, Andy Shevchenko wrote:    
> > > > > On Fri, Feb 18, 2022 at 02:51:28PM +0100, Nuno Sá wrote:    
> > > > > > On Mon, 2022-02-14 at 15:49 +0200, Andy Shevchenko wrote:    
> > > > > > > On Mon, Feb 07, 2022 at 09:19:46PM +0100, Nuno Sá wrote:    
> > > > > > > > On Mon, 2022-02-07 at 13:09 +0200, Andy Shevchenko wrote:    
> > > > > > > > > On Sun, Feb 06, 2022 at 01:19:59PM +0000, Sa, Nuno wrote:    
> > > > > > > > > > > From: Andy Shevchenko <andriy.shevchenko@intel.com>
> > > > > > > > > > > Sent: Saturday, February 5, 2022 6:30 PM
> > > > > > > > > > > On Fri, Jan 21, 2022 at 03:24:59PM +0100, Nuno Sá wrote:    
> > > 
> > > ...
> > >   
> > > > > > > > > > > > +       ret = kstrtou16(buf, 10, &val);    
> > > > > > > > > > > 
> > > > > > > > > > > In other function you have long, here u16. I would expect that
> > > > > > > > > > > the types are of the same class, e.g. if here you have u16,
> > > > > > > > > > > then there something like s32 / s64.  Or here something like
> > > > > > > > > > > unsigned short.
> > > > > > > > > > > 
> > > > > > > > > > > A bit of elaboration why u16 is chosen here?    
> > > > > > > > > > 
> > > > > > > > > > Well, I never really saw any enforcement here to be honest
> > > > > > > > > > (rather than using stdint types...). So I pretty much just use
> > > > > > > > > > these in unsigned types because I'm lazy and u16 is faster to
> > > > > > > > > > type than unsigned short...  In this case, unless Jonathan really
> > > > > > > > > > asks for it, I prefer not to go all over the driver and change
> > > > > > > > > > this...    
> > > > > > > > > 
> > > > > > > > > This is about consistency. It may work as is, but it feels not good
> > > > > > > > > when for int (or unsigned int) one uses fixed-width types. Also
> > > > > > > > > it's non- written advice to use fixed-width variables when it's
> > > > > > > > > about programming registers or so, for the rest, use POD types.    
> > > > > > 
> > > > > > Ok, going a bit back in the discussion, you argued that in one place I
> > > > > > was using long while here u16. Well, in the place I'm using long, that
> > > > > > was on purpose because that value is to be compared against an array of
> > > > > > longs (which has to be long because it depends on CCF rates). I guess I
> > > > > > can als0 use s64, but there is also a reason why long was used.
> > > > > > 
> > > > > > In the u16 case, we really want to have 2 bytes because I'm going to use
> > > > > > that value to write the dac code which is 2 bytes.    
> > > > > 
> > > > > Okay, that's what I want to hear. If it's indeed goes to be a value to the
> > > > > register, then it's fine.
> > > > > 
> > > > > Perhaps a comment?    
> > > > 
> > > > I guess you mean to have a comment to state that here we have fixed
> > > > size type (as opposed to long, used in another place), because we
> > > > directly use the value on a register write?
> > > > 
> > > > Asking it because I'm not planning to add comments in all the places
> > > > where I have fixed size types for register read/writes...    
> > > 
> > > Thinking more about it and now I'm convinced that using the value that goes to
> > > the register in ABI is bad idea (means that user space must not care about the
> > > size or contents of the hardware register and should be abstract representation
> > > of the HW).
> > > 
> > > OTOH this seems to be "raw" value of something. So, I maybe missed the convention
> > > in IIO about this kind of values WRT the variable types used on ABI side.
> > > 
> > > That said, I leave it to Jonathan since I'm not convinced that u16 is a proper
> > > choice here.  
> > 
> > From a userspace point of view it doesn't care as it's writing a string.
> > In this particular case the string only has valid values that from 0-(2^16-1)
> > (i.e. 16 bits).  So if it writes outside of that range it is an error.
> > You could read it into an unsigned long and then check against the range,
> > but there is little point given you'd still return an error if it was out of
> > range.  The fact that kstrto16() does that for you really just a shortcut
> > though it will return -ERANGE rather than perhaps -EINVAL which might be used
> > for a more generic "not this value".
> > 
> > Userspace can also read the range that is acceptable from
> > out_voltage0_raw_available [0 1 2^16-1] and hence not write an invalid value
> > in the first place - which is obviously preferred to getting an error.
> > Scaling etc is also expressed to userspace so it it wants to write a particular
> > voltage it can perform the appropriate scaling. Note that moving linear scaling
> > like this to userspace allows easy use of floating point + may be a significant
> > performance advantage if using the chrdev interface which uses the same
> > approach (and values) as the sysfs interface.  
> 
> With the same logic it can be unsigned short, no?

It could be any integer as long as it is at least as large as a u16.
But it it is larger than a u16 you'll need an additional check on the
maximum.

> 
> The point is to use u16 when it's indeed fixed-width value that goes to
> hardware or being used as part of a protocol. And thus mentioning of the
> IOCTL protocols may justify the choice. Then the question to the other
> values, shouldn't they be also fixed-width ones?

If we had a fixed width type that took the values 0-4 sure using such
a magic type would make sense, but we don't.

Note that internally kstrtou16 is just strtoull and a range check.
The one other case we have here does pretty much the same thing.

Jonathan

> 
> > > > > > > > I can understand your reasoning but again this is something that I
> > > > > > > > never really saw being enforced. So, I'm more than ok to change it if
> > > > > > > > it really becomes something that we will try to "enforce" in IIO.
> > > > > > > > Otherwise it just feels as a random nitpick :).    
> > > > > > > 
> > > > > > > No, this is about consistency and common sense. If you define type uXX,
> > > > > > > we have an API for that exact type. It's confusing why POD type APIs
> > > > > > > are used with fixed-width types or vise versa.
> > > > > > > 
> > > > > > > Moreover (which is pure theoretical, though) some architectures might
> > > > > > > have no (mutual) equivalency between these types.    
>