On Wed, May 29, 2024 at 12:35 PM Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote: > On Wed, May 29, 2024 at 08:44:26AM +0300, Andy Shevchenko wrote: > > On Tue, May 28, 2024 at 11:13 PM Laurent Pinchart wrote: > > > On Tue, May 28, 2024 at 10:27:34PM +0300, Andy Shevchenko wrote: > > > > Tue, May 28, 2024 at 10:03:12PM +0300, Laurent Pinchart kirjoitti: ... > > > > > + depends on I2C && OF > > > > > > > > Why OF? > > > > > > Because the driver works on OF systems only. > > > > > > > No COMPILE_TEST? > > > > > > The driver won't compile without CONFIG_I2C, so I can use > > > > > > depends on I2C > > > depends on OF || COMPILE_TEST > > > > > > Do you think that's better ? > > > > I think that dropping OF completely is the best. > > OF || COMPILE_TEST would work as well, but I still don't know why we need this. > > For the same reason that many drivers depend on specific CONFIG_$ARCH. It's different. You may not do in many cases the $ARCH || COMPILE_TEST, while OF || COMPILE_TEST should just work in 100% cases. > They can't run on other platforms, the dependency hides the symbol for > users who can't use the driver. This driver works on OF platforms only. What you are talking about is functional dependency, what I'm talking about is the compilation one. So, it's a pity that Kbuild doesn't distinguish these two basic concepts in dependencies and FOO || COMPILE_TEST is basically an artificial hack to tell "hey, FOO is _functional_ dependency, I do not care if I can compile without it". ... > > > > > +#define ADP5585_MAN_ID(v) (((v) & 0xf0) >> 4) > > > > > > > > GENMASK() > > > > > > This is not a mask. Or do you mean > > > > > > (((v) & GENMASK(7, 4)) >> 4) > > > > > > ? > > > > Yes. > > > > > I think that's overkill. > > > > Why? You have a mask, use it for less error prone code. > > I'll change this to ... > - if (ADP5585_MAN_ID(id) != ADP5585_MAN_ID_VALUE) > + if (id & ADP5585_MAN_ID_MASK != ADP5585_MAN_ID_VALUE) (Don't forget inner parentheses) ... > > > > > +#define ADP5585_Rx_PULL_CFG_MASK (3) > > > > > > > > GENMASK() > > > > > > Not here, as this value is meant to be passed to ADP5585_Rx_PULL_CFG(). > > > > Why is it marked as a mask? Rename it to _ALL or alike. > > It's a mask, but used as > > ADP5585_Rx_PULL_CFG(ADP5585_Rx_PULL_CFG_MASK) > > We're reaching a level of bike-shedding that even I find impressive :-) > As with a few other of your review comments that I think are related to > personal taste more than anything else, I'll defer to the subsystem > maintainer and follow their preference on this one. I would name it _ALL and use (BIT(2) - 1) notation to show that you want all of them to be set. But okay, let's leave this to the maintainer. -- With Best Regards, Andy Shevchenko
