Re: [RFC PATCH v2 07/16] gpio: Add support for unified device properties interface

[Darren Hart <dvhart@infradead.org>]

On 9/23/14, 9:26, "Arnd Bergmann" <arnd@arndb.de> wrote:

>On Tuesday 23 September 2014 18:25:01 Rafael J. Wysocki wrote:
>> 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);

Arnd, I think you meant:

	Return gpio_leds_create_acpi(pdev) ?

This is what we did early on to prototype this concept, but the problem
with this approach we duplicate all of the creation code, which leads to
maintenance errors, and is inconsistent with the goals of the _DSD which
is to reuse the same schemas for ACPI and FDT. If we have separate pdata
creation functions anyway, we are leaving much of the advantage of the
common schema on the table. Namely the ability to reuse drivers relatively
easily across firmware implementations. We don't want driver authors to
have to care if it's ACPI or FDT.

We would have preferred to have deprecated the of property interface in
favor of the new generic device_property interface, but Grant specifically
requested that we update drivers individually rather than all at once,
which means we can't just kill the OF interface.

We agreed to that, somewhat reluctantly as it adds more work in updating
the drivers over time which will slow adoption, but I understand the
desire not to make large sweeping changes due to the risk of breaking
things inadvertently as we cannot expect to be able to test all of them.
That said, I don't want to forget that the goal is to use the common
interface over time as we convert individual drivers, and using the common
interface means we need a common iterator function and that we not have fw
implementation specific pdata create functions.

--
Darren Hart
Intel Open Source Technology Center