Received-SPF: pass (google.com: domain of linux-kernel+bounces-193475-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240528190315.3865-1-laurent.pinchart@ideasonboard.com>
 <20240528190315.3865-3-laurent.pinchart@ideasonboard.com> <ZlYwJryxeZ2LAKYG@surfacebook.localdomain>
 <20240528201326.GA8500@pendragon.ideasonboard.com>
In-Reply-To: <20240528201326.GA8500@pendragon.ideasonboard.com>
From: Andy Shevchenko <andy.shevchenko@gmail.com>
Date: Wed, 29 May 2024 08:44:26 +0300
Message-ID: <CAHp75VeHA8qH_S=KJjAMv24vGP=hmeN9wSt1_NPsRhBZfEYXXw@mail.gmail.com>
Subject: Re: [PATCH v2 2/4] mfd: adp5585: Add Analog Devices ADP5585 core support
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, 
	linux-gpio@vger.kernel.org, linux-pwm@vger.kernel.org, 
	Bartosz Golaszewski <brgl@bgdev.pl>, Conor Dooley <conor+dt@kernel.org>, 
	Krzysztof Kozlowski <krzk+dt@kernel.org>, Lee Jones <lee@kernel.org>, 
	Linus Walleij <linus.walleij@linaro.org>, Rob Herring <robh@kernel.org>, 
	=?UTF-8?Q?Uwe_Kleine=2DK=C3=B6nig?= <ukleinek@kernel.org>, 
	Haibo Chen <haibo.chen@nxp.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, May 28, 2024 at 11:13=E2=80=AFPM Laurent Pinchart
<laurent.pinchart@ideasonboard.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 t=
his.

..

> > + 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 =3D=3D l cases.

--=20
With Best Regards,
Andy Shevchenko