On Sun, 16 Mar 2025 09:52:33 +0000 Jonathan Cameron <jic23@xxxxxxxxxx> wrote: > 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 Oops. You weren't talking about the regmap bitfields. Ignore this. This driver is using FIELD_PREP / FIELD_GET. Maybe should be more extensive? > 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
