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

[Jonathan Cameron <jic23@kernel.org>]

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

...