Re: [PATCH v2 2/4] mfd: Add RZ/V2M PWC core driver

[Lee Jones <lee@xxxxxxxxxx>]

On Tue, 03 Jan 2023, Fabrizio Castro wrote:

> Hi Lees,
> 
> Thanks for your feedback!
> 
> > From: Lee Jones <lee@xxxxxxxxxx>
> > Sent: 03 January 2023 12:52
> > Subject: Re: [PATCH v2 2/4] mfd: Add RZ/V2M PWC core driver
> > 
> > On Tue, 03 Jan 2023, Fabrizio Castro wrote:
> > 
> > > Hi Geert,
> > >
> > > Thanks for your feedback!
> > >
> > > > -----Original Message-----
> > > > From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> > > > Sent: 03 January 2023 08:37
> > > > To: Fabrizio Castro <fabrizio.castro.jz@xxxxxxxxxxx>
> > > > Cc: Linus Walleij <linus.walleij@xxxxxxxxxx>; Bartosz Golaszewski
> > > > <brgl@xxxxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx>; Krzysztof Kozlowski
> > > > <krzysztof.kozlowski+dt@xxxxxxxxxx>; Sebastian Reichel
> > <sre@xxxxxxxxxx>;
> > > > Geert Uytterhoeven <geert+renesas@xxxxxxxxx>; Lee Jones
> > <lee@xxxxxxxxxx>;
> > > > linux-gpio@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
> > > > kernel@xxxxxxxxxxxxxxx; linux-pm@xxxxxxxxxxxxxxx; Chris Paterson
> > > > <Chris.Paterson2@xxxxxxxxxxx>; Biju Das <biju.das@xxxxxxxxxxxxxx>;
> > linux-
> > > > renesas-soc@xxxxxxxxxxxxxxx; Laurent Pinchart
> > > > <laurent.pinchart@xxxxxxxxxxxxxxxx>; Jacopo Mondi <jacopo@xxxxxxxxxx>
> > > > Subject: Re: [PATCH v2 2/4] mfd: Add RZ/V2M PWC core driver

Could you please tell your mailer to remove mail headers from the body
please.

> > > > > The External Power Sequence Controller (PWC) IP (found in the
> > > > > RZ/V2M SoC) is a controller for external power supplies (regulators
> > > > > and power switches), and it supports the following features: it
> > > > > generates a power on/off sequence for external power supplies,
> > > > > it generates an on/off sequence for the LPDDR4 core power supply
> > > > > (LPVDD), it comes with General-Purpose Outputs, and it processes
> > > > > key input signals.
> > > >
> > > > Thanks for your patch!
> > > >
> > > > > The PWC is basically a Multi-Function Device (MFD), its software
> > > > > support comes with a core driver, and specialized drivers for
> > > > > its specific features.
> > > >
> > > > I have to admit I'm not such a big fan of MFD.  In this driver,
> > > > you are not even sharing resources in the MFD cells, just the mapped
> > > > register base.  So I think you can easily save +100 LoC and reduce
> > > > maintenance synchronization overhead across subsystems by just having
> > > > a single non-MFD driver instead.
> > > >
> > > > Did you pick MFD because the PWC poweroff feature depends on board
> > > > wiring, and thus is optional?
> > >
> > > I am not a big fan of MFD, either.
> > 
> > Interesting.
> > 
> > Could you both elaborate further please?
> 
> I have nothing against MFD

Okay, just checking.  I'll withdraw my next command then. :)

$ rm -rf drivers/mfd

> > If you do not have any resources to share, you can simply register each
> > of the devices via Device Tree.  I do not see a valid reason to force a
> > parent / child relationship for your use-case.
> 
> There would probably be overlapping on the same memory region, which would
> lead to ioremapping the same region multiple times, which is something
> I would prefer to avoid if possible.

Okay, so you *do* have shared resources.

In which case, why is simple-mfd not working for you?

> > Many people attempt to use MFD as a dumping ground / workaround for a
> > bunch of reasons.  Some valid, others not so much.
> 
> As it turns out, it looks like I don't have valid reasons to use MFD,
> therefore I'll switch to a single, non MFD, driver.
> 
> Thank you for taking the time to look into this though! Really
> appreciated.

Although it is considered okay to have a multi-purpose driver in any one
of the subsystems, it's sometimes nicer to split the various
functionality to be looked after (maintained) by their respective
subject matter experts.  You have to do what's right in any given
situation.

Ultimately it's a call you need to make with the maintainer(s).

-- 
Lee Jones [李琼斯]