On Mon, 2022-02-07 at 21:19 +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@xxxxxxxxx> > > > > Sent: Saturday, February 5, 2022 6:30 PM > > > > On Fri, Jan 21, 2022 at 03:24:59PM +0100, Nuno Sá wrote: > > > > ... > > > > > > > +#include <linux/of.h> > > > > > > > > property.h please/ > > > > > > That probably means property and of both included. See below in > > > the > > > clock_get comments... > > > > Why? OF won't be used at all. > > > see below on the clock function... > > > > ... > > > > > > > + 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... > > > > It's not bad, but I think that division between register and value > > is > > a good > > thing to have. > > > > > Can make that change though. > > > > Would be nice! > > > > ... > > > > > > 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/ > > > ... > > > > > > > + 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. > > > > ... > > 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 :). > > > > > > > > +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()'... > > Andy, Jonathan, 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 :) [1]: https://lore.kernel.org/all/CAHp75VczFs8QpsY7tuB-h4X=H54nyjABA4qDSmpQ+FRYAHZdrA@xxxxxxxxxxxxxx/ - Nuno Sá
