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: Calvin Johnson <calvin.johnson@oss.nxp.com>,
	Grant Likely <grant.likely@arm.com>,
	"Rafael J . Wysocki" <rafael@kernel.org>,
	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@vger.kernel.org, Laurentiu Tudor <laurentiu.tudor@nxp.com>,
	ACPI Devel Maling List <linux-acpi@vger.kernel.org>,
	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: Tue, 12 Jan 2021 20:03:43 +0200	[thread overview]
Message-ID: <20210112180343.GI4077@smile.fi.intel.com> (raw)
In-Reply-To: <CAGETcx-7JVz=QLCMWicHqoagWYjeBXdFJmSv1v6MQhtPt2RS=Q@mail.gmail.com>

On Tue, Jan 12, 2021 at 09:30:31AM -0800, Saravana Kannan wrote:
> On Tue, Jan 12, 2021 at 5:42 AM Calvin Johnson
> <calvin.johnson@oss.nxp.com> wrote:
> >
> > Using fwnode_get_id(), get the reg property value for DT node
> > or get the _ADR object value for ACPI node.

...

> > +/**
> > + * fwnode_get_id - Get the id of a fwnode.
> > + * @fwnode: firmware node
> > + * @id: id of the fwnode
> > + *
> > + * This function provides the id of a fwnode which can be either
> > + * DT or ACPI node. For ACPI, "reg" property value, if present will
> > + * be provided or else _ADR value will be provided.
> > + * Returns 0 on success or a negative errno.
> > + */
> > +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id)
> > +{
> > +#ifdef CONFIG_ACPI
> > +       unsigned long long adr;
> > +       acpi_status status;
> > +#endif
> > +       int ret;
> > +
> > +       ret = fwnode_property_read_u32(fwnode, "reg", id);
> > +       if (!(ret && is_acpi_node(fwnode)))
> > +               return ret;
> > +
> > +#ifdef CONFIG_ACPI
> > +       status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode),
> > +                                      METHOD_NAME__ADR, NULL, &adr);
> > +       if (ACPI_FAILURE(status))
> > +               return -EINVAL;
> > +       *id = (u32)adr;
> > +#endif
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(fwnode_get_id);

> Please don't do it this way. The whole point of fwnode_operations is
> to avoid conditional stuff at the fwnode level.

Not fully true. We have non-POD getters that are conditional. Moreover,
we have additional layer of Primary / Secondary fwnodes on top of that.

The caller of fwnode API is indeed agnostic, but under the hood it differs by
the definition (obviously due to natural differences between ACPI and DT and
whatever else might come in the future.

> Also ACPI and DT
> aren't mutually exclusive if I'm not mistaken.

That's why we try 'reg' property for both cases first.

is_acpi_fwnode() conditional is that what I don't like though.

...

> fwnode is lower level that the device-driver framework.

Agree.

> Making
> it aware of busses like mdio, etc doesn't sound right.

Disagree. Conceptually resource providers can be quite different and fwnode API
*is* LCM for them.

-- 
With Best Regards,
Andy Shevchenko