Re: [PATCH v2 2/4] mfd: adp5585: Add Analog Devices ADP5585 core support

[Andy Shevchenko]

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