On Fri, 18 Feb 2022 14:51:28 +0100 Nuno Sá <noname.nuno@xxxxxxxxx> 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@xxxxxxxxx> > > > > > > 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á >
