Re: [RFC PATCH 0/2] Add software node support to regulator framework

From: Mark Brown <broonie@kernel.org>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Daniel Scally <djrscally@gmail.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Platform Driver <platform-driver-x86@vger.kernel.org>,
	Hans de Goede <hdegoede@redhat.com>,
	Mark Gross <mgross@linux.intel.com>,
	Maximilian Luz <luzmaximilian@gmail.com>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	kieran.bingham@ideasonboard.com
Subject: Re: [RFC PATCH 0/2] Add software node support to regulator framework
Date: Mon, 12 Jul 2021 18:01:20 +0100	[thread overview]
Message-ID: <20210712170120.GG4435@sirena.org.uk> (raw)
In-Reply-To: <CAHp75VcQUUDdLYbpvTXSMPvjBzbHtBxywVBPS_xfY5JXyo9XxA@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 2316 bytes --]

On Mon, Jul 12, 2021 at 07:08:23PM +0300, Andy Shevchenko wrote:
> On Mon, Jul 12, 2021 at 4:35 PM Mark Brown <broonie@kernel.org> wrote:

> > But why?  I'm not seeing the advantage over providing platform data
> > based on DMI quirks here, it seems like a bunch of work for no reason.

> What do you mean by additional work? It's exactly opposite since most
> of the drivers in the kernel are using the fwnode interface rather
> than platform data. Why should we _add_ the specific platform data
> handling code in the certain drivers instead of not touching them at
> all?

It's adding an entirely new representation of standard data with less
validation support than either DT or platform data which seems like a
needlessly roundabout and error prone way of moving the data about with
less tooling support.  As far as I can tell the only advantage is that
it lets you write the quirk in a different source file but I'm finding
it hard to get excited about that.  If we were fixing up an existing
ACPI binding or something this approach would make sense to me but it's
making something entirely new out of whole cloth here.

We already have generic platform data support for the regulator API so
driver modifications would just be the DMI matching which is going to
have to happen somewhere anyway, I don't see a huge win from putting
them in one file rather than another.  It should basically just boil
down to being another data table, I imagine you could write a helper for
it, or probably even come up with some generic thing that let you
register a platform data/DMI combo independently of the driver to get it
out of the driver code (looking more like the existing GPIO code which
is already being used in another bit of this quirking).

It feels like this should be making more use of existing stuff than it
is.  If you look at what the core part of the code does it's taking data
which was provided by one part of the kernel in one set of C structs and
parsing those into a struct init_data which is what the core actually
wants to consume.  This seems like an entirely redundant process, we
should be able to just write the machine configuration into some struct
init_datas and get that associated with the regulators without creating
an otherwise entirely unused intermediate representation of the data.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]