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
