| Message ID | 20220121142501.151-1-nuno.sa@analog.com |
|---|---|
| Headers | show |
| Series | Add support for LTC2688 | expand |
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 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.
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 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á >
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