On Wednesday, September 24, 2014 09:55:12 AM Arnd Bergmann wrote:

On Tuesday 23 September 2014 22:47:36 Rafael J. Wysocki wrote:

quoted

Quite frankly, I'm not sure what you're asking for.

It seems to mean "I kind of don't like the current implementation", but
then the last part is quite unclear to me.  Are you suggesting to add more
"if (IS_ENABLED(CONFIG_OF) && pdev->dev.of_node) etc" type of checks to
device drivers?  That I'd like to avoid to be honest.

No, that is not what I want. Device drivers should ideally call interfaces
that just take a 'struct device' or 'struct platform_device' pointer,
and those should be implemented in an appropriate way.

But then you have drivers that access properties of their child nodes which
have no corresponding struct device(s).

quoted

Instead of the current proposal we can introduce something like

int device_get_child_property(struct device *dev, void *child_node,
                              const char *propname, void **valptr);

(and analogously for device_read_property*) and use that in the drivers that
need to iterate over child nodes of a device.  Quite along the lines of what
Dmitry is suggesting.

Then, fn() in acpi_for_each_child_node() (and the of_ counterpart of it)
would become

int (*fn)(struct device *dev, void *child_node, void *data)

and so on.

Would you prefer that?

I must still be missing part of what you are trying to achieve above.
We definitely need an interface to get properties from the device itself,
like

int device_get_property(struct device *dev, const char *propname, void **valptr);

(whatever valptr ends up being, that would be a separate discussion).

As soon as it comes to devices that have child nodes, I don't see a
necessity to have a generic abstraction for them, as this is typically
only done for some of the more obscure bindings, or for child nodes that
are defined in a subsystem-wide binding rather than a device private
binding.

Well, I don't really agree here.

We can demonstrably reduce code duplication (and therefore complexity too) in
at least two drivers by doing that at a reasonably low cost, which is
simply exposing the API for child nodes and adding helpers for iterating
over them.

We don't have to use struct fw_dev_node (or similar) for that if that's what
bothers you, but in my opinion something like "get me a property of this thing
which may be either a device tree node or an ACPI object" (ie. what the current
dev_node_get_property() does) is more straightforwad than "get me a property
of that child of this device which may be either a device tree node or an ACPI
object" (ie. what device_get_child_property() as described above would do).

For the former case, I think they are indeed better left in drivers that
actively know the difference between DT and ACPI, and that don't necessarily
use the same binding for both. In the latter case, I'd leave the
implementation up to subsystem code, which again would know what
interface it is using.

The case in question is drivers that need not know the difference between DT and
ACPI and do use the same binding for both.  It seems quite clear what needs to
be done in the other cases.

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.