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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Andy,

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

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

That's actually not needed, I'll drop it.

> > 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?

If I don't reply to a particular comment it means I agree with it and
will address it, yes.

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

diff --git a/drivers/mfd/adp5585.c b/drivers/mfd/adp5585.c
index fa4092a5c97f..924758b8a3cd 100644
--- a/drivers/mfd/adp5585.c
+++ b/drivers/mfd/adp5585.c
@@ -125,7 +125,7 @@ static int adp5585_i2c_probe(struct i2c_client *i2c)
 		return dev_err_probe(&i2c->dev, ret,
 				     "Failed to read device ID\n");
 
-	if (ADP5585_MAN_ID(id) != ADP5585_MAN_ID_VALUE)
+	if (id & ADP5585_MAN_ID_MASK != ADP5585_MAN_ID_VALUE)
 		return dev_err_probe(&i2c->dev, -ENODEV,
 				     "Invalid device ID 0x%02x\n", id);
 
diff --git a/include/linux/mfd/adp5585.h b/include/linux/mfd/adp5585.h
index f06a574afedf..f5776ee844dc 100644
--- a/include/linux/mfd/adp5585.h
+++ b/include/linux/mfd/adp5585.h
@@ -12,8 +12,8 @@
 #include <linux/bits.h>
 
 #define ADP5585_ID			0x00
-#define		ADP5585_MAN_ID(v)		(((v) & 0xf0) >> 4)
-#define		ADP5585_MAN_ID_VALUE		0x02
+#define		ADP5585_MAN_ID_VALUE		0x20
+#define		ADP5585_MAN_ID_MASK		GENMASK(7, 4)
 #define ADP5585_INT_STATUS		0x01
 #define ADP5585_STATUS			0x02
 #define ADP5585_FIFO_1			0x03

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

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

I've tested it and it works.

-- 
Regards,

Laurent Pinchart