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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 29 May 2024 08:44:26 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: linux-kernel@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-gpio@...r.kernel.org, linux-pwm@...r.kernel.org, 
	Bartosz Golaszewski <brgl@...ev.pl>, Conor Dooley <conor+dt@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Lee Jones <lee@...nel.org>, 
	Linus Walleij <linus.walleij@...aro.org>, Rob Herring <robh@...nel.org>, 
	Uwe Kleine-König <ukleinek@...nel.org>, 
	Haibo Chen <haibo.chen@....com>
Subject: Re: [PATCH v2 2/4] mfd: adp5585: Add Analog Devices ADP5585 core support

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