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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> From: Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>
> Sent: Saturday, February 5, 2022 6:30 PM
> To: Sa, Nuno <Nuno.Sa@xxxxxxxxxx>
> Cc: linux-iio@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; Rob
> Herring <robh+dt@xxxxxxxxxx>; Jonathan Cameron
> <jic23@xxxxxxxxxx>; Lars-Peter Clausen <lars@xxxxxxxxxx>;
> Hennerich, Michael <Michael.Hennerich@xxxxxxxxxx>
> 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á




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux