On Tuesday, September 23, 2014 06:26:07 PM Arnd Bergmann wrote:

On Tuesday 23 September 2014 18:25:01 Rafael J. Wysocki wrote:

quoted

The problem is iteration over child nodes of a given one where there
may not be struct device objects.

For example (from patch [2/16]):

+int acpi_for_each_child_node(struct acpi_device *adev,
+                             int (*fn)(struct fw_dev_node *fdn, void *data),
+                             void *data)
+{
+       struct acpi_device *child;
+       int ret = 0;
+
+       list_for_each_entry(child, &adev->children, node) {
+               struct fw_dev_node fdn = { .acpi_node = child, };
+
+               ret = fn(&fdn, data);
+               if (ret)
+                       break;
+       }
+       return ret;
+}

and then fn() can be made work for both DTs and ACPI.  Without this we'd
need to have two versions of fn(), one for DTs and one for ACPI (and possibly
more for some other FW protocols), which isn't necessary in general (and
duplicates code etc.). 

That actually is used by some patches down in the series (eg. [10/16]).

Ok, I understand what you are doing now.

Looking at the example you point to (http://www.spinics.net/lists/devicetree/msg49502.html), I still feel
that this is adding more abstraction than what is good for us, and
I'd be happier with an implementation of gpio_leds_create() that
has a bit more duplication and less abstraction.

The important part should be that the driver-side interface is
sensible, other than that an implementation like

static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
{
	if (IS_ENABLED(CONFIG_OF) && pdev->dev.of_node)
		return gpio_leds_create_of(pdev);
	else if (IS_ENABLED(CONFIG_ACPI))
		return gpio_leds_create_of(acpi);
	return ERR_PTR(-ENXIO);
}

would keep either side of it relatively simple, by leaving out the
indirect function calls and new for_each_available_child_of_node()
macro.

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.

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?

How many other users of fw_dev_node do you have at the moment?

One more, gpio_keys_polled in patch [13/16] (https://patchwork.kernel.org/patch/4917311/).

Rafael