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

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Tue, May 28, 2024 at 11:13 PM Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> 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.

...

> > + array_size.h
> > + device.h // e.g., devm_kzalloc()
> >
> > > +#include <linux/module.h>
> > > +#include <linux/moduleparam.h>
> > > +#include <linux/init.h>
> > > +#include <linux/slab.h>
>
> I'll drop those 3 headers, there's not needed anymore.
>
> > > +#include <linux/i2c.h>
> >
> > > +#include <linux/of.h>
> > > +#include <linux/of_device.h>
> >
> > You don't need them, instead of proxying...
>
> of.h for of_device_get_match_data() and of_match_ptr(). I'll drop the
> former, but I need the latter, so I'll keep of.h

Why do you need of_match_ptr()? What for?

> of_device.h for historical reasons probably, I'll drop it.
>
> > > +#include <linux/mfd/core.h>
> > > +#include <linux/mfd/adp5585.h>
> >
> > m is earlier than 'o', but with above drop no more issue :-)
> >
> > ...just include mod_devicetable.h.
> >
> > > +#include <linux/regmap.h>
> >
> > + types.h // e.g., u8

I assume that all marked with + in my previous reply you agree on?

...

> > > +#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.

...

> > > +#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.

...

> > > +#define            ADP5585_C4_EXTEND_CFG_MASK      (1U << 6)
> >
> > > +#define            ADP5585_R4_EXTEND_CFG_MASK      (1U << 5)
> >
> > > +#define            ADP5585_R3_EXTEND_CFG_MASK      (3U << 2)
> >
> > > +#define            ADP5585_R0_EXTEND_CFG_MASK      (1U << 0)
> >
> > > +#define            ADP5585_OSC_FREQ_MASK           (3U << 5)
> >
> > BIT() / GENMASK()
>
> I'll use GENMASK for the masks.

For a single bit the BIT() is okay, and TBH I don't remember if
GENMASK() supports h == l cases.

-- 
With Best Regards,
Andy Shevchenko