On Sat, 5 Feb 2022 19:29:39 +0200 Andy Shevchenko <andriy.shevchenko@xxxxxxxxx> 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; > > + ...
