Re: [net-next PATCH v3 09/15] device property: Introduce fwnode_get_id()

From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Saravana Kannan <saravanak@google.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>,
	Calvin Johnson <calvin.johnson@oss.nxp.com>,
	Grant Likely <grant.likely@arm.com>,
	Jeremy Linton <jeremy.linton@arm.com>,
	Andrew Lunn <andrew@lunn.ch>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Russell King - ARM Linux admin <linux@armlinux.org.uk>,
	Cristi Sovaiala <cristian.sovaiala@nxp.com>,
	Florin Laurentiu Chiculita <florinlaurentiu.chiculita@nxp.com>,
	Ioana Ciornei <ioana.ciornei@nxp.com>,
	Madalin Bucur <madalin.bucur@oss.nxp.com>,
	Heikki Krogerus <heikki.krogerus@linux.intel.com>,
	Marcin Wojtas <mw@semihalf.com>,
	Pieter Jansen Van Vuuren <pieter.jansenvv@bamboosystems.io>,
	Jon <jon@solid-run.com>,
	Diana Madalina Craciun <diana.craciun@nxp.com>,
	LKML <linux-kernel@vger.kernel.org>,
	netdev <netdev@vger.kernel.org>,
	Laurentiu Tudor <laurentiu.tudor@nxp.com>,
	ACPI Devel Maling List <linux-acpi@vger.kernel.org>,
	"linux.cj" <linux.cj@gmail.com>,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
	Bartosz Golaszewski <bgolaszewski@baylibre.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>,
	Randy Dunlap <rdunlap@infradead.org>
Subject: Re: [net-next PATCH v3 09/15] device property: Introduce fwnode_get_id()
Date: Fri, 22 Jan 2021 23:06:14 +0200	[thread overview]
Message-ID: <CAHp75VcVNj0+KZiLEsPgfz=fZoLr9g1=6ikeUo7FZ1rd4FKpWQ@mail.gmail.com> (raw)
In-Reply-To: <CAGETcx-904Cr11nVW6PC=OqWnwM-ZC-DdEUa8+7JmhsH3UOqHw@mail.gmail.com>

On Fri, Jan 22, 2021 at 10:59 PM Saravana Kannan <saravanak@google.com> wrote:
> On Fri, Jan 22, 2021 at 8:34 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Wed, Jan 20, 2021 at 9:01 PM Saravana Kannan <saravanak@google.com> wrote:
> > > On Wed, Jan 20, 2021 at 11:15 AM Rafael J. Wysocki <rafael@kernel.org> wrote:

> > > I'd rather this new function be an ops instead of a bunch of #ifdef or
> > > if (acpi) checks. Thoughts?
> >
> > Well, it looks more like a helper function than like an op and I'm not
> > even sure how many potential users of it will expect that _ADR should
> > be evaluated in the absence of the "reg" property.
> >
> > It's just that the "reg" property happens to be kind of an _ADR
> > equivalent in this particular binding AFAICS.
>
> I agree it is not clear how useful this helper function is going to be.
>
> But in general, to me, any time the wrapper/helper functions in
> drivers/base/property.c need to do something like this:
>
> if (ACPI)
>    ACPI specific code
> if (OF)
>    OF specific code
>
> I think the code should be pushed to the fwnode ops. That's one of the
> main point of fwnode. So that firmware specific stuff is done by
> firmware specific code. Also, when adding support for new firmware,
> it's pretty clear what support the firmware needs to implement.
> Instead of having to go fix up a bunch of code all over the place.

Wishful thinking.
In the very case of GPIO it's related to framework using headers local
to framework. Are you suggesting to open its guts to the entire wild
world?
I don't think it's a good idea. You see, here we have different
layering POD types, which are natural and quite low level that ops
suits best for them and quite different resource types like GPIO. And
the latter is closer to certain framework rather than to POD handling
cases.

> So fwnode_ops->get_id() would be the OP ACPI and OF would implement.
> And then we can have a wrapper in drivers/base/property.c.

-- 
With Best Regards,
Andy Shevchenko