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
