On Fri, 14 Mar 2025 16:33:13 +0200 Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote: > On Fri, Mar 14, 2025 at 09:31:58AM +0200, Matti Vaittinen wrote: > > On 13/03/2025 15:19, Andy Shevchenko wrote: > > > On Thu, Mar 13, 2025 at 09:19:03AM +0200, Matti Vaittinen wrote: > > ... Picking out a few things to comment on... > > > > > +#define BD79124_MASK_HYSTERESIS GENMASK(3, 0) > > > > +#define BD79124_LOW_LIMIT_MIN 0 > > > > +#define BD79124_HIGH_LIMIT_MAX GENMASK(11, 0) > > > > > > These masks are not named after registers (or I don't see it clearly), > > > > Naming is hard. I usually try to make a balance between: > > > > 1) using names which explain the purpose when seen in the code (at call > > site) > > 2) keeping names short enough > > 3) following the naming convention in the data sheet > > 4) maintain relation to the register. > > > > I put most emphasis to the 1, while 2 is important to me as well. 3 is > > _very_ nice to have, but it is often somehow contradicting with 1 and 2. 4 > > is IMO just nice to have. The register is usually clearly visible at call > > site, and if someone adds new functionality (or checks the correctness of > > the masks/registers), then 3 is way more important. > > > > I am open to any concrete suggestions though. > > From my point of view the starting point is 3, then one may apply 2 and 4, > the 1 may mangle the name so much that register data field name becomes quite > abstract. > > > > it's > > > hard to understand which one relates to which register. Also, why not using > > > bitfield.h? > > > > I saw no need for it? > > Hmm... Okay, I think Jonathan will ask that if really needed. > I won't as I'm not a huge fan of bitfield.h. In many cases they bloat the code and increase the writes that go over the bus. Don't get me wrong, there are good usecases, but it's not a universal thing (unlike PREP_FIELD() etc which I love :) I do favour burning a few characters to make field / register relationship clear though. A few things can help I think. Structuring defines and naming: I like using whitespace in subtle ways for this. #define PREFIX_REGNAME_REG 0x00 #define PREFIX_REGNAME_FIELDNAME_MSK GENMASK(X, Y) #define PREFIX_REGNAME_FIELDNAME_FILEVALNAME 0x3 etc Makes it easy to see if we have a mismatch going on However, I don't insist on this in all cases as it is one of those "don't let perfect be the enemy of good" cases I think. So Matti, good to have one last look at the defines and see if they can be wrangled into a slightly better form. . > ... > > > > > +static void bd79124gpo_set_multiple(struct gpio_chip *gc, unsigned long *mask, > > > > + unsigned long *bits) > > > > > > Ditto, .set_multiple_rv(). > > > > As you know, this started at v6.14 cycle which is still ongoing. I don't > > think .set_rv() and .set_multiple_rv() are available? > > You mean that you are still hope to have this series to be squeezed for > v6.15-rc1? That's not me who decides, but if not, those APIs will be part of > the v6.15-rc1 (at least they are pending in GPIO subsystem). > I'd vote for a rebase on rc1 that should be really easy to for me to pick up. I'd accept a follow up series though. Ultimately won't affect when this series lands as very unlikely Linus will delay the release long enough for me to do another pull request this cycle, Jonathan
