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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

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

-- 
With Best Regards,
Andy Shevchenko