| Message ID | 1410868367-11056-8-git-send-email-mika.westerberg@linux.intel.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
On Tue, Sep 16, 2014 at 1:52 PM, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > Some drivers need to deal with only firmware representation of its > GPIOs. An example would be a GPIO button array driver where each button > is described as a separate firmware node in device tree. Typically these > child nodes do not have physical representation in the Linux device > model. > > In order to help device drivers to handle such firmware child nodes we > add dev[m]_node_get_named_gpiod() that takes a firmware node pointer as > parameter, finds the GPIO using whatever is the underlying firmware > method, and requests the GPIO properly. > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> I have a hard time figuring out if this is what we want for common accessors between DT and ACPI. Can I get some input from Grant, Arnd, Mark, Darren...? Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 23 September 2014 17:25:50 Linus Walleij wrote: > On Tue, Sep 16, 2014 at 1:52 PM, Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > > Some drivers need to deal with only firmware representation of its > > GPIOs. An example would be a GPIO button array driver where each button > > is described as a separate firmware node in device tree. Typically these > > child nodes do not have physical representation in the Linux device > > model. > > > > In order to help device drivers to handle such firmware child nodes we > > add dev[m]_node_get_named_gpiod() that takes a firmware node pointer as > > parameter, finds the GPIO using whatever is the underlying firmware > > method, and requests the GPIO properly. > > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > I have a hard time figuring out if this is what we want for common > accessors between DT and ACPI. > > Can I get some input from Grant, Arnd, Mark, Darren...? I just took a brief look at this. My first impression is that the fw_dev_node structure is weird when all callers just do (in patch 2) + struct fw_dev_node fdn = { + .of_node = dev->of_node, + .acpi_node = ACPI_COMPANION(dev), + }; I'd much rather see an interface that passes the 'struct device' pointer down to dev_get_named_gpiod() and all other exported functions, and then internally does the conversion at the point where the access is done. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 23, 2014 at 05:45:57PM +0200, Arnd Bergmann wrote: > On Tuesday 23 September 2014 17:25:50 Linus Walleij wrote: > > On Tue, Sep 16, 2014 at 1:52 PM, Mika Westerberg > > <mika.westerberg@linux.intel.com> wrote: > > > > > Some drivers need to deal with only firmware representation of its > > > GPIOs. An example would be a GPIO button array driver where each button > > > is described as a separate firmware node in device tree. Typically these > > > child nodes do not have physical representation in the Linux device > > > model. > > > > > > In order to help device drivers to handle such firmware child nodes we > > > add dev[m]_node_get_named_gpiod() that takes a firmware node pointer as > > > parameter, finds the GPIO using whatever is the underlying firmware > > > method, and requests the GPIO properly. > > > > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > > > I have a hard time figuring out if this is what we want for common > > accessors between DT and ACPI. > > > > Can I get some input from Grant, Arnd, Mark, Darren...? > > I just took a brief look at this. My first impression is that the > fw_dev_node structure is weird when all callers just do (in patch 2) > > + struct fw_dev_node fdn = { > + .of_node = dev->of_node, > + .acpi_node = ACPI_COMPANION(dev), > + }; > > I'd much rather see an interface that passes the 'struct device' > pointer down to dev_get_named_gpiod() and all other exported > functions, and then internally does the conversion at the point > where the access is done. Problem is that if you don't have the dev pointer in the first place. Please look how leds-gpio.c or gpio_keys_polled.c are using this. Of course you have the first level device but when you need to iterate "leds" or "buttons" below where there is no Linux device available we need something like this. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 23, 2014 at 06:52:02PM +0300, Mika Westerberg wrote: > On Tue, Sep 23, 2014 at 05:45:57PM +0200, Arnd Bergmann wrote: > > On Tuesday 23 September 2014 17:25:50 Linus Walleij wrote: > > > On Tue, Sep 16, 2014 at 1:52 PM, Mika Westerberg > > > <mika.westerberg@linux.intel.com> wrote: > > > > > > > Some drivers need to deal with only firmware representation of its > > > > GPIOs. An example would be a GPIO button array driver where each button > > > > is described as a separate firmware node in device tree. Typically these > > > > child nodes do not have physical representation in the Linux device > > > > model. > > > > > > > > In order to help device drivers to handle such firmware child nodes we > > > > add dev[m]_node_get_named_gpiod() that takes a firmware node pointer as > > > > parameter, finds the GPIO using whatever is the underlying firmware > > > > method, and requests the GPIO properly. > > > > > > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > > > > > I have a hard time figuring out if this is what we want for common > > > accessors between DT and ACPI. > > > > > > Can I get some input from Grant, Arnd, Mark, Darren...? > > > > I just took a brief look at this. My first impression is that the > > fw_dev_node structure is weird when all callers just do (in patch 2) > > > > + struct fw_dev_node fdn = { > > + .of_node = dev->of_node, > > + .acpi_node = ACPI_COMPANION(dev), > > + }; > > > > I'd much rather see an interface that passes the 'struct device' > > pointer down to dev_get_named_gpiod() and all other exported > > functions, and then internally does the conversion at the point > > where the access is done. > > Problem is that if you don't have the dev pointer in the first place. > Please look how leds-gpio.c or gpio_keys_polled.c are using this. > > Of course you have the first level device but when you need to iterate > "leds" or "buttons" below where there is no Linux device available we > need something like this. Maybe we should be passing the parent/owner device to the iterator functions? Thanks.
On Tuesday, September 23, 2014 05:45:57 PM Arnd Bergmann wrote: > On Tuesday 23 September 2014 17:25:50 Linus Walleij wrote: > > On Tue, Sep 16, 2014 at 1:52 PM, Mika Westerberg > > <mika.westerberg@linux.intel.com> wrote: > > > > > Some drivers need to deal with only firmware representation of its > > > GPIOs. An example would be a GPIO button array driver where each button > > > is described as a separate firmware node in device tree. Typically these > > > child nodes do not have physical representation in the Linux device > > > model. > > > > > > In order to help device drivers to handle such firmware child nodes we > > > add dev[m]_node_get_named_gpiod() that takes a firmware node pointer as > > > parameter, finds the GPIO using whatever is the underlying firmware > > > method, and requests the GPIO properly. > > > > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > > > I have a hard time figuring out if this is what we want for common > > accessors between DT and ACPI. > > > > Can I get some input from Grant, Arnd, Mark, Darren...? > > I just took a brief look at this. My first impression is that the > fw_dev_node structure is weird when all callers just do (in patch 2) > > + struct fw_dev_node fdn = { > + .of_node = dev->of_node, > + .acpi_node = ACPI_COMPANION(dev), > + }; > > I'd much rather see an interface that passes the 'struct device' > pointer down to dev_get_named_gpiod() and all other exported > functions, and then internally does the conversion at the point > where the access is done. 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]). Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); 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. How many other users of fw_dev_node do you have at the moment? Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, September 23, 2014 09:17:24 AM Dmitry Torokhov wrote: > On Tue, Sep 23, 2014 at 06:52:02PM +0300, Mika Westerberg wrote: > > On Tue, Sep 23, 2014 at 05:45:57PM +0200, Arnd Bergmann wrote: > > > On Tuesday 23 September 2014 17:25:50 Linus Walleij wrote: > > > > On Tue, Sep 16, 2014 at 1:52 PM, Mika Westerberg > > > > <mika.westerberg@linux.intel.com> wrote: > > > > > > > > > Some drivers need to deal with only firmware representation of its > > > > > GPIOs. An example would be a GPIO button array driver where each button > > > > > is described as a separate firmware node in device tree. Typically these > > > > > child nodes do not have physical representation in the Linux device > > > > > model. > > > > > > > > > > In order to help device drivers to handle such firmware child nodes we > > > > > add dev[m]_node_get_named_gpiod() that takes a firmware node pointer as > > > > > parameter, finds the GPIO using whatever is the underlying firmware > > > > > method, and requests the GPIO properly. > > > > > > > > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > > > > > > > I have a hard time figuring out if this is what we want for common > > > > accessors between DT and ACPI. > > > > > > > > Can I get some input from Grant, Arnd, Mark, Darren...? > > > > > > I just took a brief look at this. My first impression is that the > > > fw_dev_node structure is weird when all callers just do (in patch 2) > > > > > > + struct fw_dev_node fdn = { > > > + .of_node = dev->of_node, > > > + .acpi_node = ACPI_COMPANION(dev), > > > + }; > > > > > > I'd much rather see an interface that passes the 'struct device' > > > pointer down to dev_get_named_gpiod() and all other exported > > > functions, and then internally does the conversion at the point > > > where the access is done. > > > > Problem is that if you don't have the dev pointer in the first place. > > Please look how leds-gpio.c or gpio_keys_polled.c are using this. > > > > Of course you have the first level device but when you need to iterate > > "leds" or "buttons" below where there is no Linux device available we > > need something like this. > > Maybe we should be passing the parent/owner device to the iterator > functions? Yes, we can do that. That's one alternative for what we have in the current set. Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, September 23, 2014 06:26:07 PM Arnd Bergmann 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); > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 23 September 2014 22:47:36 Rafael J. Wysocki wrote: > 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. > 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. 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. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 23 September 2014 14:15:47 Darren Hart wrote: > Arnd, I think you meant: > > Return gpio_leds_create_acpi(pdev) ? Yes, sorry for the confusion on my part. > 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. I think we are absolutely in agreement about the basic principle here, but disagree on how far we'd want to take the abstraction. I got a little confused by the leds-gpio example, as I initially saw that as generic infrastructure rather than a specific driver. As I just wrote in my reply to Rafael, I generally believe we should strive to have generic driver-side interfaces so drivers don't have to care, but keep the differences in subsystem specific code. > 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. I don't actually have a strong opinion on that matter, having only the device property interface does have some advantages as well, but I also agree that we are somewhat better off not having to change all the drivers. > 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. I've looked a bit closer at how the LED subsystem handles sub-nodes in DT at the moment. It seems that there is some duplication between the drivers already, as they all implement a variation of the sub-node parsing code. There are other non-LED drivers with similar loops, but they seem to be either very specialized, or explicitly for DT abstractions, so I'm still not convinced we need a generic loop-through-child-nodes-and-parse-properties interface. How would you feel about a more general way of probing LED, using a new helper in the leds-core that iterates over the child nodes and parses the standard properties but calls into a driver specific callback to parse the specific properties? It's probably much more work than your current approach, but it seems to me that there is more to gain by solving the problem for LED drivers in particular to cut down the per-driver duplication at the same time as the per-firmware-interface duplication. As a start, we could probably take the proposed device_for_each_child_node and move that into the leds-core, changing the fw_dev_node argument for an led_classdev with the addition of the of_node and acpi_object members. It would still leave it up to the gpio-leds driver to do if (led_cdev->of_node) gpiod = devm_of_get_gpiod(led_cdev->of_node, ...); else gpiod = devm_acpi_get_gpiod(led_cdev->acpi_object, ...); but there seems little benefit in abstracting this because there is only one driver that needs it. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Sep 24, 2014 at 11:12:36AM +0200, Arnd Bergmann wrote: > As a start, we could probably take the proposed device_for_each_child_node > and move that into the leds-core, changing the fw_dev_node argument > for an led_classdev with the addition of the of_node and acpi_object > members. It would still leave it up to the gpio-leds driver to do > > if (led_cdev->of_node) > gpiod = devm_of_get_gpiod(led_cdev->of_node, ...); > else > gpiod = devm_acpi_get_gpiod(led_cdev->acpi_object, ...); > > but there seems little benefit in abstracting this because there is > only one driver that needs it. The same interface is used also in gpio_keys_polled.c driver so if we want to avoid duplicating code this needs to be abstracted away from the drivers. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, September 24, 2014 09:55:12 AM Arnd Bergmann wrote: > On Tuesday 23 September 2014 22:47:36 Rafael J. Wysocki wrote: > > 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). > > 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.
On Wednesday, September 24, 2014 12:38:23 PM Mika Westerberg wrote: > On Wed, Sep 24, 2014 at 11:12:36AM +0200, Arnd Bergmann wrote: > > As a start, we could probably take the proposed device_for_each_child_node > > and move that into the leds-core, changing the fw_dev_node argument > > for an led_classdev with the addition of the of_node and acpi_object > > members. It would still leave it up to the gpio-leds driver to do > > > > if (led_cdev->of_node) > > gpiod = devm_of_get_gpiod(led_cdev->of_node, ...); > > else > > gpiod = devm_acpi_get_gpiod(led_cdev->acpi_object, ...); > > > > but there seems little benefit in abstracting this because there is > > only one driver that needs it. > > The same interface is used also in gpio_keys_polled.c driver so if we > want to avoid duplicating code this needs to be abstracted away from the > drivers. Well, precisely. Moving it to the leds-core doesn't buy us anything.
On Wed, Sep 24, 2014 at 11:12:36AM +0200, Arnd Bergmann wrote: > On Tuesday 23 September 2014 14:15:47 Darren Hart wrote: > > Arnd, I think you meant: > > > > Return gpio_leds_create_acpi(pdev) ? > > Yes, sorry for the confusion on my part. Hi Arnd, No problem, just wanted to make sure I knew what you meant. > > > 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. > > I think we are absolutely in agreement about the basic principle here, > but disagree on how far we'd want to take the abstraction. > > I got a little confused by the leds-gpio example, as I initially saw > that as generic infrastructure rather than a specific driver. As I just > wrote in my reply to Rafael, I generally believe we should strive to > have generic driver-side interfaces so drivers don't have to care, > but keep the differences in subsystem specific code. > > > 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. > > I don't actually have a strong opinion on that matter, having only the > device property interface does have some advantages as well, but I also > agree that we are somewhat better off not having to change all the drivers. > > > 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. > > I've looked a bit closer at how the LED subsystem handles sub-nodes > in DT at the moment. It seems that there is some duplication between > the drivers already, as they all implement a variation of the sub-node > parsing code. > > There are other non-LED drivers with similar loops, but they seem to be > either very specialized, or explicitly for DT abstractions, so I'm still > not convinced we need a generic loop-through-child-nodes-and-parse-properties > interface. > > How would you feel about a more general way of probing LED, using > a new helper in the leds-core that iterates over the child nodes > and parses the standard properties but calls into a driver specific > callback to parse the specific properties? > It's probably much more work than your current approach, but it seems > to me that there is more to gain by solving the problem for LED > drivers in particular to cut down the per-driver duplication > at the same time as the per-firmware-interface duplication. > > As a start, we could probably take the proposed device_for_each_child_node > and move that into the leds-core, changing the fw_dev_node argument > for an led_classdev with the addition of the of_node and acpi_object > members. It would still leave it up to the gpio-leds driver to do > > if (led_cdev->of_node) > gpiod = devm_of_get_gpiod(led_cdev->of_node, ...); > else > gpiod = devm_acpi_get_gpiod(led_cdev->acpi_object, ...); So as Mika has pointed out, LEDs aren't the only ones affected. Several drivers will need to walk through non-device child nodes, and it seems to me that having a firmware-independent mechanism to do so benefits the drivers by both making them smaller and by increasing the reusability of new drivers and drivers updated to use the new API across platforms. I fear we might be entering bike shed territory as we seem to be repeating points now. Can you restate your concern with the interface and why this level of abstraction is worse for the kernel? I'm not seeing this point, so I'm not sure what to address in my response. Grant, Linus W? Thoughts? > > but there seems little benefit in abstracting this because there is > only one driver that needs it. > > Arnd >
On Thursday 25 September 2014 20:21:32 Darren Hart wrote: > On Wed, Sep 24, 2014 at 11:12:36AM +0200, Arnd Bergmann wrote: > > How would you feel about a more general way of probing LED, using > > a new helper in the leds-core that iterates over the child nodes > > and parses the standard properties but calls into a driver specific > > callback to parse the specific properties? > > It's probably much more work than your current approach, but it seems > > to me that there is more to gain by solving the problem for LED > > drivers in particular to cut down the per-driver duplication > > at the same time as the per-firmware-interface duplication. > > > > As a start, we could probably take the proposed device_for_each_child_node > > and move that into the leds-core, changing the fw_dev_node argument > > for an led_classdev with the addition of the of_node and acpi_object > > members. It would still leave it up to the gpio-leds driver to do > > > > if (led_cdev->of_node) > > gpiod = devm_of_get_gpiod(led_cdev->of_node, ...); > > else > > gpiod = devm_acpi_get_gpiod(led_cdev->acpi_object, ...); > > So as Mika has pointed out, LEDs aren't the only ones affected. Several drivers > will need to walk through non-device child nodes, and it seems to me that having > a firmware-independent mechanism to do so benefits the drivers by both making > them smaller and by increasing the reusability of new drivers and drivers > updated to use the new API across platforms. > > I fear we might be entering bike shed territory as we seem to be repeating > points now. Can you restate your concern with the interface and why this level > of abstraction is worse for the kernel? I'm not seeing this point, so I'm not > sure what to address in my response. I think we should have abstractions for all common interfaces but make them as simple as possible. In the discussions at the kernel summit, everyone agreed that we should have common accessors for simple properties (bool, int, string, ...) based on device pointers, as well as subsystem specific accessors to handle the high-level abstractions (registers, interrupts, gpio, regulator, pinctrl, dma, reset, pwm, ...). Having generalized accessors for the same properties in child nodes of the device goes beyond that, and I think this is the wrong trade-off between interface simplicity and generality since only few drivers will be able to use those. I think we will always have to live with a leaky abstraction because some drivers need to do things beyond what we can do with a common API. > Grant, Linus W? Thoughts? I definitely want to hear other voices on this too. This is really not a fundamental debate I think, but more a question of how far the abstraction should go. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday, September 26, 2014 10:36:06 AM Arnd Bergmann wrote: > On Thursday 25 September 2014 20:21:32 Darren Hart wrote: > > On Wed, Sep 24, 2014 at 11:12:36AM +0200, Arnd Bergmann wrote: > > > How would you feel about a more general way of probing LED, using > > > a new helper in the leds-core that iterates over the child nodes > > > and parses the standard properties but calls into a driver specific > > > callback to parse the specific properties? > > > It's probably much more work than your current approach, but it seems > > > to me that there is more to gain by solving the problem for LED > > > drivers in particular to cut down the per-driver duplication > > > at the same time as the per-firmware-interface duplication. > > > > > > As a start, we could probably take the proposed device_for_each_child_node > > > and move that into the leds-core, changing the fw_dev_node argument > > > for an led_classdev with the addition of the of_node and acpi_object > > > members. It would still leave it up to the gpio-leds driver to do > > > > > > if (led_cdev->of_node) > > > gpiod = devm_of_get_gpiod(led_cdev->of_node, ...); > > > else > > > gpiod = devm_acpi_get_gpiod(led_cdev->acpi_object, ...); > > > > So as Mika has pointed out, LEDs aren't the only ones affected. Several drivers > > will need to walk through non-device child nodes, and it seems to me that having > > a firmware-independent mechanism to do so benefits the drivers by both making > > them smaller and by increasing the reusability of new drivers and drivers > > updated to use the new API across platforms. > > > > I fear we might be entering bike shed territory as we seem to be repeating > > points now. Can you restate your concern with the interface and why this level > > of abstraction is worse for the kernel? I'm not seeing this point, so I'm not > > sure what to address in my response. > > I think we should have abstractions for all common interfaces but make > them as simple as possible. In the discussions at the kernel summit, > everyone agreed that we should have common accessors for simple properties > (bool, int, string, ...) based on device pointers, as well as subsystem > specific accessors to handle the high-level abstractions (registers, > interrupts, gpio, regulator, pinctrl, dma, reset, pwm, ...). > > Having generalized accessors for the same properties in child nodes of > the device goes beyond that, and I think this is the wrong trade-off > between interface simplicity and generality since only few drivers will > be able to use those. I think we will always have to live with a leaky > abstraction because some drivers need to do things beyond what we can > do with a common API. Some drivers do, but then we can avoid adding DT/ACPI knowledge to some drivers by adding general accessors for properties in child nodes. In my opinion, drivers should not do things specific to DT/ACPI unless that is unavoidable. > > > Grant, Linus W? Thoughts? > > I definitely want to hear other voices on this too. This is really not > a fundamental debate I think, but more a question of how far the abstraction > should go. Right.
On Fri, Sep 26, 2014 at 5:21 AM, Darren Hart <dvhart@infradead.org> wrote: > On Wed, Sep 24, 2014 at 11:12:36AM +0200, Arnd Bergmann wrote: > So as Mika has pointed out, LEDs aren't the only ones affected. Several drivers > will need to walk through non-device child nodes, and it seems to me that having > a firmware-independent mechanism to do so benefits the drivers by both making > them smaller and by increasing the reusability of new drivers and drivers > updated to use the new API across platforms. In a recent round of reviews, for the OF case, that led to drivers which used device_initcall() without being a module, getting a match and handle to the parent device, and then walking over the nodes and instantiating child objects (Linux devices usually) in the process. It was done as a response to the remark from Rob Herring that we were modeling things in the device tree as devices when they really weren't, we were just doing it that way because it fits the Linux device model and it's easier. So we have that case too. The question is if it's anything close to generalizable. > Grant, Linus W? Thoughts? I'm uncertain on the whole subject, I called on the others because of that... For a while I had Andy Schevenko patch the GPIO and SFI core too, but it timed out due to no response from Len Brown. (Maybe I should just merge that stuff!) Do you (Intel) also want to unify the Medfield SFI thing into this or have you given up on it? Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2014-10-07 at 15:37 +0200, Linus Walleij wrote: > On Fri, Sep 26, 2014 at 5:21 AM, Darren Hart <dvhart@infradead.org> wrote: > > On Wed, Sep 24, 2014 at 11:12:36AM +0200, Arnd Bergmann wrote: > > > So as Mika has pointed out, LEDs aren't the only ones affected. Several drivers > > will need to walk through non-device child nodes, and it seems to me that having > > a firmware-independent mechanism to do so benefits the drivers by both making > > them smaller and by increasing the reusability of new drivers and drivers > > updated to use the new API across platforms. > > In a recent round of reviews, for the OF case, that led to drivers > which used device_initcall() without being a module, getting a match > and handle to the parent device, and then walking over the nodes > and instantiating child objects (Linux devices usually) in the process. > > It was done as a response to the remark from Rob Herring that > we were modeling things in the device tree as devices when they > really weren't, we were just doing it that way because it fits the > Linux device model and it's easier. > > So we have that case too. > > The question is if it's anything close to generalizable. > > > Grant, Linus W? Thoughts? > > I'm uncertain on the whole subject, I called on the others > because of that... > > For a while I had Andy Schevenko patch the GPIO and > SFI core too, but it timed out due to no response from Len > Brown. (Maybe I should just merge that stuff!) Do you (Intel) also > want to unify the Medfield SFI thing into this or have you > given up on it? I think SFI is quite outdated stuff, though I have still Medfield device close to me. I don't think there will be any new platform with SFI (on the other hand we never know :-) ). Thus, my opinion you may go ahead without worrying about SFI.
On Tuesday, October 07, 2014 03:37:04 PM Linus Walleij wrote: > On Fri, Sep 26, 2014 at 5:21 AM, Darren Hart <dvhart@infradead.org> wrote: > > On Wed, Sep 24, 2014 at 11:12:36AM +0200, Arnd Bergmann wrote: > > > So as Mika has pointed out, LEDs aren't the only ones affected. Several drivers > > will need to walk through non-device child nodes, and it seems to me that having > > a firmware-independent mechanism to do so benefits the drivers by both making > > them smaller and by increasing the reusability of new drivers and drivers > > updated to use the new API across platforms. > > In a recent round of reviews, for the OF case, that led to drivers > which used device_initcall() without being a module, getting a match > and handle to the parent device, and then walking over the nodes > and instantiating child objects (Linux devices usually) in the process. > > It was done as a response to the remark from Rob Herring that > we were modeling things in the device tree as devices when they > really weren't, we were just doing it that way because it fits the > Linux device model and it's easier. > > So we have that case too. > > The question is if it's anything close to generalizable. Well, OK. Can you please have a look at these patchse in v4: https://patchwork.kernel.org/patch/5040161/ https://patchwork.kernel.org/patch/5040081/ https://patchwork.kernel.org/patch/5040061/ We now have an ACK from Dmitry on the gpio_keys_polled thing and Bryan said he was OK with the leds changes already in v2 (IIRC). Also Alexandre is saying that he's not opposed to the changes in https://patchwork.kernel.org/patch/5040161/, although there may be a better way. And we have AKCs from Greg on the driver core changes, so it looks like GPIO really is the only missing thing and we need that code to support our hardware.
diff --git a/drivers/gpio/devres.c b/drivers/gpio/devres.c index 954b9f6b0ef8..1556a251fc8e 100644 --- a/drivers/gpio/devres.c +++ b/drivers/gpio/devres.c @@ -109,6 +109,41 @@ struct gpio_desc *__must_check __devm_gpiod_get_index(struct device *dev, EXPORT_SYMBOL(__devm_gpiod_get_index); /** + * devm_node_get_named_gpiod - resource-managed dev_node_get_named_gpiod() + * @dev: GPIO consumer + * @fdn: firmware device node + * @propname: name of the firmware property + * @idx: index of the GPIO in the property value in case of many + * + * Managed dev_node_get_named_gpiod(). GPIO descriptors returned from + * this function are automatically disposed on driver detach. + */ +struct gpio_desc *devm_node_get_named_gpiod(struct device *dev, + struct fw_dev_node *fdn, + const char *propname, int index) +{ + struct gpio_desc **dr; + struct gpio_desc *desc; + + dr = devres_alloc(devm_gpiod_release, sizeof(struct gpio_desc *), + GFP_KERNEL); + if (!dr) + return ERR_PTR(-ENOMEM); + + desc = dev_node_get_named_gpiod(fdn, propname, index); + if (IS_ERR(desc)) { + devres_free(dr); + return desc; + } + + *dr = desc; + devres_add(dev, dr); + + return desc; +} +EXPORT_SYMBOL(devm_node_get_named_gpiod); + +/** * devm_gpiod_get_index_optional - Resource-managed gpiod_get_index_optional() * @dev: GPIO consumer * @con_id: function within the GPIO consumer diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 444d43c9fd3e..d364214d2946 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1717,6 +1717,61 @@ struct gpio_desc *__must_check __gpiod_get_index(struct device *dev, EXPORT_SYMBOL_GPL(__gpiod_get_index); /** + * dev_node_get_named_gpiod - obtain a GPIO from firmware device node + * @fdn: firmware device node + * @propname: name of the firmware property + * @idx: index of the GPIO in the property value in case of many + * + * This function can be used for drivers that get their configuration + * from firmware in such way that there is not always corresponding + * physical device pointer available. For example some properties are + * described as a child nodes for the parent device in DT or ACPI. + * + * Function properly finds the corresponding GPIO using whatever is the + * underlying firmware interface and then makes sure that the GPIO + * descriptor is requested before it is returned to the caller. + * + * In case of error an ERR_PTR() is returned. + */ +struct gpio_desc *dev_node_get_named_gpiod(struct fw_dev_node *fdn, + const char *propname, int index) +{ + struct gpio_desc *desc = ERR_PTR(-ENODEV); + struct acpi_device *adev = fdn->acpi_node; + struct device_node *np = fdn->of_node; + bool active_low = false; + int ret; + + if (IS_ENABLED(CONFIG_OF) && np) { + enum of_gpio_flags flags; + + desc = of_get_named_gpiod_flags(np, propname, index, &flags); + if (!IS_ERR(desc)) + active_low = flags & OF_GPIO_ACTIVE_LOW; + } else if (IS_ENABLED(CONFIG_ACPI) && adev) { + struct acpi_gpio_info info; + + desc = acpi_get_gpiod_by_index(adev, propname, index, &info); + if (!IS_ERR(desc)) + active_low = info.active_low; + } + + if (IS_ERR(desc)) + return desc; + + ret = gpiod_request(desc, NULL); + if (ret) + return ERR_PTR(ret); + + /* Only value flag can be set from both DT and ACPI is active_low */ + if (active_low) + set_bit(FLAG_ACTIVE_LOW, &desc->flags); + + return desc; +} +EXPORT_SYMBOL_GPL(dev_node_get_named_gpiod); + +/** * gpiod_get_index_optional - obtain an optional GPIO from a multi-index GPIO * function * @dev: GPIO consumer, can be NULL for system-global GPIOs diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h index 12f146fa6604..aa1b273f0e38 100644 --- a/include/linux/gpio/consumer.h +++ b/include/linux/gpio/consumer.h @@ -6,6 +6,7 @@ #include <linux/kernel.h> struct device; +struct fw_dev_node; /** * Opaque descriptor for a GPIO. These are obtained using gpiod_get() and are @@ -94,6 +95,12 @@ int gpiod_to_irq(const struct gpio_desc *desc); struct gpio_desc *gpio_to_desc(unsigned gpio); int desc_to_gpio(const struct gpio_desc *desc); +/* Firmware node interface */ +struct gpio_desc *dev_node_get_named_gpiod(struct fw_dev_node *fdn, + const char *propname, int index); +struct gpio_desc *devm_node_get_named_gpiod(struct device *dev, + struct fw_dev_node *fdn, + const char *propname, int index); #else /* CONFIG_GPIOLIB */ static inline struct gpio_desc *__must_check __gpiod_get(struct device *dev,
Some drivers need to deal with only firmware representation of its GPIOs. An example would be a GPIO button array driver where each button is described as a separate firmware node in device tree. Typically these child nodes do not have physical representation in the Linux device model. In order to help device drivers to handle such firmware child nodes we add dev[m]_node_get_named_gpiod() that takes a firmware node pointer as parameter, finds the GPIO using whatever is the underlying firmware method, and requests the GPIO properly. Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> --- drivers/gpio/devres.c | 35 +++++++++++++++++++++++++++ drivers/gpio/gpiolib.c | 55 +++++++++++++++++++++++++++++++++++++++++++ include/linux/gpio/consumer.h | 7 ++++++ 3 files changed, 97 insertions(+)