| Message ID | 1389941251-32692-5-git-send-email-wens@csie.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Friday 17 January 2014, Chen-Yu Tsai wrote: > diff --git a/Documentation/devicetree/bindings/rfkill/rfkill-gpio.txt b/Documentation/devicetree/bindings/rfkill/rfkill-gpio.txt > new file mode 100644 > index 0000000..8a07ea4 > --- /dev/null > +++ b/Documentation/devicetree/bindings/rfkill/rfkill-gpio.txt > @@ -0,0 +1,26 @@ > +GPIO controlled RFKILL devices > + > +Required properties: > +- compatible : Must be "rfkill-gpio". > +- rfkill-name : Name of RFKILL device > +- rfkill-type : Type of RFKILL device: 1 for WiFi, 2 for BlueTooth > +- NAME_shutdown-gpios : GPIO phandle to shutdown control > + (phandle must be the second) > +- NAME_reset-gpios : GPIO phandle to reset control > + > +NAME must match the rfkill-name property. NAME_shutdown-gpios or > +NAME_reset-gpios, or both, must be defined. > + I don't understand this part. Why do you include the name in the gpios property, rather than just hardcoding the property strings to "shutdown-gpios" and "reset-gpios"? The description of hte "rfkill-name" property seems to suggest that you can only have one logical RFKILL device per device node, so he names would not be ambiguous. Arnd
On Sat, Jan 18, 2014 at 12:47 AM, Arnd Bergmann <arnd@arndb.de> wrote: > On Friday 17 January 2014, Chen-Yu Tsai wrote: >> diff --git a/Documentation/devicetree/bindings/rfkill/rfkill-gpio.txt b/Documentation/devicetree/bindings/rfkill/rfkill-gpio.txt >> new file mode 100644 >> index 0000000..8a07ea4 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/rfkill/rfkill-gpio.txt >> @@ -0,0 +1,26 @@ >> +GPIO controlled RFKILL devices >> + >> +Required properties: >> +- compatible : Must be "rfkill-gpio". >> +- rfkill-name : Name of RFKILL device >> +- rfkill-type : Type of RFKILL device: 1 for WiFi, 2 for BlueTooth >> +- NAME_shutdown-gpios : GPIO phandle to shutdown control >> + (phandle must be the second) >> +- NAME_reset-gpios : GPIO phandle to reset control >> + >> +NAME must match the rfkill-name property. NAME_shutdown-gpios or >> +NAME_reset-gpios, or both, must be defined. >> + > > I don't understand this part. Why do you include the name in the > gpios property, rather than just hardcoding the property strings > to "shutdown-gpios" and "reset-gpios"? This quirk is a result of how gpiod_get_index implements device tree lookup. You'll also notice that the shutdown GPIO must be the second phandle, as the driver uses indexed lookup to support ACPI cases. If con_id is given, it is prepended to "gpios" as the property string. con_id is also used as the label passed to gpiod_request, which is then shown in /sys/kernel/debug/gpio. I can do a seperate lookup for the device tree case, with or without fallback to platform lookup tables. Then the names can be "reset-gpio" and "shutdown-gpio". Need to promote gpiod_request to non-static so we can register usage of the gpio, to match non-dt code path. Personally I prefer adding a "label" parameter to gpiod_get_index, so we can use a different name than con_id. con_id isn't used in the ACPI case, and is optional in platform lookup case. However device tree lookup is dependent on this. What I see is non-uniform behavior between the three. In my opinion this is undesirable, but I haven't come up with a good solution yet. About the property string, is the plural form required, even though we only want one? Thanks! ChenYu > The description of hte "rfkill-name" property seems to suggest > that you can only have one logical RFKILL device per device node, > so he names would not be ambiguous. > > Arnd
On Friday 17 January 2014, Chen-Yu Tsai wrote: > On Sat, Jan 18, 2014 at 12:47 AM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Friday 17 January 2014, Chen-Yu Tsai wrote: > >> diff --git a/Documentation/devicetree/bindings/rfkill/rfkill-gpio.txt b/Documentation/devicetree/bindings/rfkill/rfkill-gpio.txt > >> new file mode 100644 > >> index 0000000..8a07ea4 > >> --- /dev/null > >> +++ b/Documentation/devicetree/bindings/rfkill/rfkill-gpio.txt > >> @@ -0,0 +1,26 @@ > >> +GPIO controlled RFKILL devices > >> + > >> +Required properties: > >> +- compatible : Must be "rfkill-gpio". > >> +- rfkill-name : Name of RFKILL device > >> +- rfkill-type : Type of RFKILL device: 1 for WiFi, 2 for BlueTooth > >> +- NAME_shutdown-gpios : GPIO phandle to shutdown control > >> + (phandle must be the second) > >> +- NAME_reset-gpios : GPIO phandle to reset control > >> + > >> +NAME must match the rfkill-name property. NAME_shutdown-gpios or > >> +NAME_reset-gpios, or both, must be defined. > >> + > > > > I don't understand this part. Why do you include the name in the > > gpios property, rather than just hardcoding the property strings > > to "shutdown-gpios" and "reset-gpios"? > > This quirk is a result of how gpiod_get_index implements device tree > lookup. You'll also notice that the shutdown GPIO must be the second > phandle, as the driver uses indexed lookup to support ACPI cases. > If con_id is given, it is prepended to "gpios" as the property string. > con_id is also used as the label passed to gpiod_request, which is > then shown in /sys/kernel/debug/gpio. The Linux implementation should not enforce an inferior DT binding. I think it would be better to change the code instead and make this work with a more sensible representation. > I can do a seperate lookup for the device tree case, with or without > fallback to platform lookup tables. Then the names can be "reset-gpio" > and "shutdown-gpio". Need to promote gpiod_request to non-static so > we can register usage of the gpio, to match non-dt code path. > > Personally I prefer adding a "label" parameter to gpiod_get_index, so > we can use a different name than con_id. con_id isn't used in the ACPI > case, and is optional in platform lookup case. However device tree > lookup is dependent on this. What I see is non-uniform behavior > between the three. In my opinion this is undesirable, but I haven't > come up with a good solution yet. (adding the gpio people here). I don't understand enough of the current API to make a good call here, but I agree that we should try to make it more uniform and do it in a way that allows simpler DT bindings for devices using it. > About the property string, is the plural form required, even though we > only want one? I would keep the plural form for consistency. Arnd
On Fri, Jan 17, 2014 at 6:43 PM, Chen-Yu Tsai <wens@csie.org> wrote: > On Sat, Jan 18, 2014 at 12:47 AM, Arnd Bergmann <arnd@arndb.de> wrote: >>> +- NAME_shutdown-gpios : GPIO phandle to shutdown control >>> + (phandle must be the second) >>> +- NAME_reset-gpios : GPIO phandle to reset control >>> + >>> +NAME must match the rfkill-name property. NAME_shutdown-gpios or >>> +NAME_reset-gpios, or both, must be defined. >>> + >> >> I don't understand this part. Why do you include the name in the >> gpios property, rather than just hardcoding the property strings >> to "shutdown-gpios" and "reset-gpios"? > > This quirk is a result of how gpiod_get_index implements device tree > lookup. Why can't it just have a single property "gpios", where the first element is the reset GPIO and the second is the shutdown GPIO? rfkill-gpio does this: gpio = devm_gpiod_get_index(&pdev->dev, rfkill->reset_name, 0); gpio = devm_gpiod_get_index(&pdev->dev, rfkill->shutdown_name, 1); The passed con ID name parameter is only there for the device tree case it seems. (ACPI ignores it.) So what about you just don't pass it at all and patch it to do like this instead: gpio = devm_gpiod_get_index(&pdev->dev, NULL, 0); gpio = devm_gpiod_get_index(&pdev->dev, NULL, 1); Heikki, are you OK with this change? I think this is actually necessary if the ACPI and DT unification pipe dream shall limp forward, we cannot have arguments passed that have a semantic effect on DT but not on ACPI... Drivers that are supposed to use both ACPI and DT will always have to pass NULL as con ID. > If con_id is given, it is prepended to "gpios" as the property string. > con_id is also used as the label passed to gpiod_request, which is > then shown in /sys/kernel/debug/gpio. If your problem is really what turns up in debugfs, then we need to figure out a way to label gpios outside of the *gpiod_get* calls. The string passed in *gpiod_get* is a "connection ID" not a proper name for the GPIO. Yours, Linus Walleij
On Sat, Jan 18, 2014 at 7:11 AM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Fri, Jan 17, 2014 at 6:43 PM, Chen-Yu Tsai <wens@csie.org> wrote: >> On Sat, Jan 18, 2014 at 12:47 AM, Arnd Bergmann <arnd@arndb.de> wrote: > >>>> +- NAME_shutdown-gpios : GPIO phandle to shutdown control >>>> + (phandle must be the second) >>>> +- NAME_reset-gpios : GPIO phandle to reset control >>>> + >>>> +NAME must match the rfkill-name property. NAME_shutdown-gpios or >>>> +NAME_reset-gpios, or both, must be defined. >>>> + >>> >>> I don't understand this part. Why do you include the name in the >>> gpios property, rather than just hardcoding the property strings >>> to "shutdown-gpios" and "reset-gpios"? >> >> This quirk is a result of how gpiod_get_index implements device tree >> lookup. > > Why can't it just have a single property "gpios", where the first > element is the reset GPIO and the second is the shutdown GPIO? > > rfkill-gpio does this: > > gpio = devm_gpiod_get_index(&pdev->dev, rfkill->reset_name, 0); > gpio = devm_gpiod_get_index(&pdev->dev, rfkill->shutdown_name, 1); > > The passed con ID name parameter is only there for the device > tree case it seems. (ACPI ignores it.) So what about you just > don't pass it at all and patch it to do like this instead: > > gpio = devm_gpiod_get_index(&pdev->dev, NULL, 0); > gpio = devm_gpiod_get_index(&pdev->dev, NULL, 1); I'd like that. It's much cleaner. > Heikki, are you OK with this change? > > I think this is actually necessary if the ACPI and DT unification > pipe dream shall limp forward, we cannot have arguments passed > that have a semantic effect on DT but not on ACPI... Drivers > that are supposed to use both ACPI and DT will always > have to pass NULL as con ID. > >> If con_id is given, it is prepended to "gpios" as the property string. >> con_id is also used as the label passed to gpiod_request, which is >> then shown in /sys/kernel/debug/gpio. > > If your problem is really what turns up in debugfs, then we need > to figure out a way to label gpios outside of the *gpiod_get* calls. Let's add a gpiod_set_label call. Currently there's a desc_set_label in gpiolib, which is static inlined. We can either rename and promote it to non-static, or add a new wrapping function. > The string passed in *gpiod_get* is a "connection ID" not a proper > name for the GPIO. I see. Perhaps we should not pass this to gpiod_request as the label, or add a comment stating consumers can use the new gpiod_set_label call to change it. Cheers, ChenYu
Hi, On Sat, Jan 18, 2014 at 12:11:56AM +0100, Linus Walleij wrote: > On Fri, Jan 17, 2014 at 6:43 PM, Chen-Yu Tsai <wens@csie.org> wrote: > > On Sat, Jan 18, 2014 at 12:47 AM, Arnd Bergmann <arnd@arndb.de> wrote: > > >>> +- NAME_shutdown-gpios : GPIO phandle to shutdown control > >>> + (phandle must be the second) > >>> +- NAME_reset-gpios : GPIO phandle to reset control > >>> + > >>> +NAME must match the rfkill-name property. NAME_shutdown-gpios or > >>> +NAME_reset-gpios, or both, must be defined. > >>> + > >> > >> I don't understand this part. Why do you include the name in the > >> gpios property, rather than just hardcoding the property strings > >> to "shutdown-gpios" and "reset-gpios"? > > > > This quirk is a result of how gpiod_get_index implements device tree > > lookup. > > Why can't it just have a single property "gpios", where the first > element is the reset GPIO and the second is the shutdown GPIO? > > rfkill-gpio does this: > > gpio = devm_gpiod_get_index(&pdev->dev, rfkill->reset_name, 0); > gpio = devm_gpiod_get_index(&pdev->dev, rfkill->shutdown_name, 1); > > The passed con ID name parameter is only there for the device > tree case it seems. (ACPI ignores it.) So what about you just > don't pass it at all and patch it to do like this instead: > > gpio = devm_gpiod_get_index(&pdev->dev, NULL, 0); > gpio = devm_gpiod_get_index(&pdev->dev, NULL, 1); > > Heikki, are you OK with this change? Yes, definitely. That is much cleaner. Thanks,
On Sat, Jan 18, 2014 at 8:11 AM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Fri, Jan 17, 2014 at 6:43 PM, Chen-Yu Tsai <wens@csie.org> wrote: >> On Sat, Jan 18, 2014 at 12:47 AM, Arnd Bergmann <arnd@arndb.de> wrote: > >>>> +- NAME_shutdown-gpios : GPIO phandle to shutdown control >>>> + (phandle must be the second) >>>> +- NAME_reset-gpios : GPIO phandle to reset control >>>> + >>>> +NAME must match the rfkill-name property. NAME_shutdown-gpios or >>>> +NAME_reset-gpios, or both, must be defined. >>>> + >>> >>> I don't understand this part. Why do you include the name in the >>> gpios property, rather than just hardcoding the property strings >>> to "shutdown-gpios" and "reset-gpios"? >> >> This quirk is a result of how gpiod_get_index implements device tree >> lookup. > > Why can't it just have a single property "gpios", where the first > element is the reset GPIO and the second is the shutdown GPIO? > > rfkill-gpio does this: > > gpio = devm_gpiod_get_index(&pdev->dev, rfkill->reset_name, 0); > gpio = devm_gpiod_get_index(&pdev->dev, rfkill->shutdown_name, 1); > > The passed con ID name parameter is only there for the device > tree case it seems. (ACPI ignores it.) So what about you just > don't pass it at all and patch it to do like this instead: > > gpio = devm_gpiod_get_index(&pdev->dev, NULL, 0); > gpio = devm_gpiod_get_index(&pdev->dev, NULL, 1); > > Heikki, are you OK with this change? > > I think this is actually necessary if the ACPI and DT unification > pipe dream shall limp forward, we cannot have arguments passed > that have a semantic effect on DT but not on ACPI... Drivers > that are supposed to use both ACPI and DT will always > have to pass NULL as con ID. I agree that's how it should be be done with the current API if your driver can obtain GPIOs from both ACPI and DT. This is a potential issue, as drivers are not supposed to make assumptions about who is going to be their GPIO provider. Let's say you started a driver with only DT in mind, and used gpio_get(dev, con_id) to get your GPIOs. DT bindings are thus of the form "con_id-gpio = <phandle>", and set in stone. Then later, someone wants to use your driver with ACPI. How do you handle that gracefully? I'm starting to wonder, now that ACPI is a first-class GPIO provider, whether we should not start to encourage the deprecation of the "con_id-gpio = <phandle>" binding form in DT and only use a single indexed GPIO property per device. The con_id parameter would then only be used as a label, which would also have the nice side-effect that all GPIOs used for a given function will be reported under the same name no matter what the GPIO provider is. From an aesthetic point of view, I definitely prefer using con_id to identify GPIOs instead of indexes, but I don't see how we can make it play nice with ACPI. Thoughts? Alex.
On Tue, Jan 21, 2014 at 4:11 AM, Alexandre Courbot <gnurou@gmail.com> wrote: > On Sat, Jan 18, 2014 at 8:11 AM, Linus Walleij <linus.walleij@linaro.org> wrote: >> gpio = devm_gpiod_get_index(&pdev->dev, NULL, 0); >> gpio = devm_gpiod_get_index(&pdev->dev, NULL, 1); >> >> Heikki, are you OK with this change? >> >> I think this is actually necessary if the ACPI and DT unification >> pipe dream shall limp forward, we cannot have arguments passed >> that have a semantic effect on DT but not on ACPI... Drivers >> that are supposed to use both ACPI and DT will always >> have to pass NULL as con ID. > > I agree that's how it should be be done with the current API if your > driver can obtain GPIOs from both ACPI and DT. This is a potential > issue, as drivers are not supposed to make assumptions about who is > going to be their GPIO provider. Let's say you started a driver with > only DT in mind, and used gpio_get(dev, con_id) to get your GPIOs. DT > bindings are thus of the form "con_id-gpio = <phandle>", and set in > stone. Then later, someone wants to use your driver with ACPI. How do > you handle that gracefully? Short answer is you can't. You have to pour backward-compatibility code into the driver first checking for that property and then falling back to the new binding if it doesn't exist. > I'm starting to wonder, now that ACPI is a first-class GPIO provider, > whether we should not start to encourage the deprecation of the > "con_id-gpio = <phandle>" binding form in DT and only use a single > indexed GPIO property per device. You have a valid point. > The con_id parameter would then only > be used as a label, which would also have the nice side-effect that > all GPIOs used for a given function will be reported under the same > name no matter what the GPIO provider is. As discussed earlier in this thread I'm not sure the con_id is suitable for labelling GPIOs. It'd be better to have a proper name specified in DT/ACPI instead. > From an aesthetic point of view, I definitely prefer using con_id to > identify GPIOs instead of indexes, but I don't see how we can make it > play nice with ACPI. Thoughts? Let's ask the DT maintainers... I'm a bit sceptic to the whole ACPI-DT-API-should-be-unified just-one-function-call business, as this was just a very simple example of what can happen to something as simple as devm_gpiod_get[_index](). Yours, Linus Walleij
On Tuesday 21 January 2014, Linus Walleij wrote: > On Tue, Jan 21, 2014 at 4:11 AM, Alexandre Courbot <gnurou@gmail.com> wrote: > > On Sat, Jan 18, 2014 at 8:11 AM, Linus Walleij <linus.walleij@linaro.org> wrote: > > > > I agree that's how it should be be done with the current API if your > > driver can obtain GPIOs from both ACPI and DT. This is a potential > > issue, as drivers are not supposed to make assumptions about who is > > going to be their GPIO provider. Let's say you started a driver with > > only DT in mind, and used gpio_get(dev, con_id) to get your GPIOs. DT > > bindings are thus of the form "con_id-gpio = <phandle>", and set in > > stone. Then later, someone wants to use your driver with ACPI. How do > > you handle that gracefully? > > Short answer is you can't. You have to pour backward-compatibility > code into the driver first checking for that property and then falling > back to the new binding if it doesn't exist. With the ACPI named properties extension, it should be possible to have something akin to a "gpio-names" list that can be attached to an indexed array of gpio descriptors. I assume that Intel is going to need this for named irqs, clocks, regulators, dmas as well, so I think it will eventually get there. It's not something that can be done today though, or that is standardized in APCI-5.0. My guess is that named GPIOs are going to make more sense on x86 embedded than on arm64 server. > > I'm starting to wonder, now that ACPI is a first-class GPIO provider, > > whether we should not start to encourage the deprecation of the > > "con_id-gpio = <phandle>" binding form in DT and only use a single > > indexed GPIO property per device. > > You have a valid point. Independent of ACPI, I prefer indexed "gpios" properties over "con_id-gpio" properties anyway, because it's more consistent with some of the other subsystems. I don't have an opinion though on whether we should also allow a "gpios"/"gpio-names" pair, or whether we should keep the indexed "gpios" list for the anonymous case. > > The con_id parameter would then only > > be used as a label, which would also have the nice side-effect that > > all GPIOs used for a given function will be reported under the same > > name no matter what the GPIO provider is. > > As discussed earlier in this thread I'm not sure the con_id is > suitable for labelling GPIOs. It'd be better to have a proper name > specified in DT/ACPI instead. +1 > > From an aesthetic point of view, I definitely prefer using con_id to > > identify GPIOs instead of indexes, but I don't see how we can make it > > play nice with ACPI. Thoughts? > > Let's ask the DT maintainers... > > I'm a bit sceptic to the whole ACPI-DT-API-should-be-unified > just-one-function-call business, as this was just a very simple example > of what can happen to something as simple as > devm_gpiod_get[_index](). I think a unified kernel API makes more sense for some subsystems than others, and it depends a bit on the rate of adoption of APCI for drivers that already have a DT binding (or vice versa, if that happens). GPIO might actually be in the first category since it's commonly used for off-chip components that will get shared across ARM and x86 (as well as everything else), while a common kernel API would be less important for things that are internal to an SoC where Intel is the only company needing ACPI support. Arnd
On Tue, Jan 21, 2014 at 9:35 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Tuesday 21 January 2014, Linus Walleij wrote: >> On Tue, Jan 21, 2014 at 4:11 AM, Alexandre Courbot <gnurou@gmail.com> wrote: >> > On Sat, Jan 18, 2014 at 8:11 AM, Linus Walleij <linus.walleij@linaro.org> wrote: >> > >> > I agree that's how it should be be done with the current API if your >> > driver can obtain GPIOs from both ACPI and DT. This is a potential >> > issue, as drivers are not supposed to make assumptions about who is >> > going to be their GPIO provider. Let's say you started a driver with >> > only DT in mind, and used gpio_get(dev, con_id) to get your GPIOs. DT >> > bindings are thus of the form "con_id-gpio = <phandle>", and set in >> > stone. Then later, someone wants to use your driver with ACPI. How do >> > you handle that gracefully? >> >> Short answer is you can't. You have to pour backward-compatibility >> code into the driver first checking for that property and then falling >> back to the new binding if it doesn't exist. > > With the ACPI named properties extension, it should be possible to have > something akin to a "gpio-names" list that can be attached to an indexed > array of gpio descriptors. I assume that Intel is going to need this > for named irqs, clocks, regulators, dmas as well, so I think it will > eventually get there. It's not something that can be done today though, > or that is standardized in APCI-5.0. Good to know. I'm not sure this would help with the named GPIO resolution issue however. I assume that contrary to DT we will have no control over the naming of ACPI-defined GPIOs, and thus there is little chance that a GPIO serving a function will end up having the same name as the one we chose for the DT binding... > > My guess is that named GPIOs are going to make more sense on x86 embedded > than on arm64 server. > >> > I'm starting to wonder, now that ACPI is a first-class GPIO provider, >> > whether we should not start to encourage the deprecation of the >> > "con_id-gpio = <phandle>" binding form in DT and only use a single >> > indexed GPIO property per device. >> >> You have a valid point. > > Independent of ACPI, I prefer indexed "gpios" properties over "con_id-gpio" > properties anyway, because it's more consistent with some of the other > subsystems. I don't have an opinion though on whether we should also > allow a "gpios"/"gpio-names" pair, or whether we should keep the indexed > "gpios" list for the anonymous case. > >> > The con_id parameter would then only >> > be used as a label, which would also have the nice side-effect that >> > all GPIOs used for a given function will be reported under the same >> > name no matter what the GPIO provider is. >> >> As discussed earlier in this thread I'm not sure the con_id is >> suitable for labelling GPIOs. It'd be better to have a proper name >> specified in DT/ACPI instead. > > +1 I wonder why you guys prefer to have the name defined in the GPIO mapping. Having the driver decide the label makes it easier to look up which GPIO does what in debugfs, whereas nothing prevents people to name GPIOs whatever inadequate name they want in the device DT node. What am I overlooking here? > >> > From an aesthetic point of view, I definitely prefer using con_id to >> > identify GPIOs instead of indexes, but I don't see how we can make it >> > play nice with ACPI. Thoughts? >> >> Let's ask the DT maintainers... >> >> I'm a bit sceptic to the whole ACPI-DT-API-should-be-unified >> just-one-function-call business, as this was just a very simple example >> of what can happen to something as simple as >> devm_gpiod_get[_index](). > > I think a unified kernel API makes more sense for some subsystems than > others, and it depends a bit on the rate of adoption of APCI for drivers > that already have a DT binding (or vice versa, if that happens). > > GPIO might actually be in the first category since it's commonly used > for off-chip components that will get shared across ARM and x86 (as > well as everything else), while a common kernel API would be less > important for things that are internal to an SoC where Intel is the > only company needing ACPI support. I am afraid I don't have a good enough view of the ACPI landscape to understand how often drivers might be reused on both ACPI and DT. But I suppose nothing speaks against that, technically speaking. Maybe Mika would have comments to make here? The good (or bad, rather) thing about DT is that we can do whatever we please with the new bindings: decide which name or which index (doesn't matter here) a GPIO should have. However we don't have this control over ACPI, where nothing guarantees that the same index will be used for the same GPIO function. And there goes our unified GPIO mapping. Workarounds would imply additional layers of mapping, or using different probe functions depending on whether we rely on DT or ACPI (I don't want to imagine there will be systems that use *both*). Considering that we already have drivers using that trick (e.g. to choose between SPI or I2C), the latter might be acceptable. Alex.
On Tue, Jan 21, 2014 at 11:53:13PM +0900, Alexandre Courbot wrote: > > I think a unified kernel API makes more sense for some subsystems than > > others, and it depends a bit on the rate of adoption of APCI for drivers > > that already have a DT binding (or vice versa, if that happens). > > > > GPIO might actually be in the first category since it's commonly used > > for off-chip components that will get shared across ARM and x86 (as > > well as everything else), while a common kernel API would be less > > important for things that are internal to an SoC where Intel is the > > only company needing ACPI support. > > I am afraid I don't have a good enough view of the ACPI landscape to > understand how often drivers might be reused on both ACPI and DT. But > I suppose nothing speaks against that, technically speaking. Maybe > Mika would have comments to make here? Well, we try to reuse existing drivers whenever possible. As an example Intel LPSS devices (that exists on Haswell and Baytrail) are mostly existing drivers from ARM world. I would say that GPIO is one of such things where we would like to have an unified interface definitely.
On Tuesday 21 January 2014, Alexandre Courbot wrote: > >> As discussed earlier in this thread I'm not sure the con_id is > >> suitable for labelling GPIOs. It'd be better to have a proper name > >> specified in DT/ACPI instead. > > > > +1 > > I wonder why you guys prefer to have the name defined in the GPIO > mapping. Having the driver decide the label makes it easier to look up > which GPIO does what in debugfs, whereas nothing prevents people to > name GPIOs whatever inadequate name they want in the device DT node. > What am I overlooking here? I should have another look at the debugfs representation, but isn't there a global namespace that gets used for all gpios? Neither the con_id nor the name that the driver picks would be globally unique and stable across kernel versions, so they don't make a good user interface. I think what we want here is a system-wide unique identifier for each gpio line that may get represented in debugfs, and use a new DT mechanism to communicate that. Arnd
On Tue, Jan 21, 2014 at 3:53 PM, Alexandre Courbot <gnurou@gmail.com> wrote: > On Tue, Jan 21, 2014 at 9:35 PM, Arnd Bergmann <arnd@arndb.de> wrote: >> On Tuesday 21 January 2014, Linus Walleij wrote: >>> As discussed earlier in this thread I'm not sure the con_id is >>> suitable for labelling GPIOs. It'd be better to have a proper name >>> specified in DT/ACPI instead. >> >> +1 > > I wonder why you guys prefer to have the name defined in the GPIO > mapping. Having the driver decide the label makes it easier to look up > which GPIO does what in debugfs, whereas nothing prevents people to > name GPIOs whatever inadequate name they want in the device DT node. > What am I overlooking here? The proper name of a GPIO does not come from the driver but from the usecase, i.e. often the name of the rail on the board where it is used. Remember GPIO are per definition general purpose, they cannot get any clever names from the driver. They would just be named "chip-foo-gpio0" thru "chip-foo-gpioN" if the driver was to name them. Using the rail name on the board is way more useful. A GPIO line named "HAL_SENSOR" or "MMC_CD" is actually telling us what that line is used for. But such names can only come from a DT or ACPI table that has knowledge of how the GPIO is used on a certain system/board and not just from the driver. Yours, Linus Walleij
On Tue, Jan 21, 2014 at 3:53 PM, Alexandre Courbot <gnurou@gmail.com> wrote: > The good (or bad, rather) thing about DT is that we can do whatever we > please with the new bindings: decide which name or which index > (doesn't matter here) a GPIO should have. However we don't have this > control over ACPI, where nothing guarantees that the same index will > be used for the same GPIO function. It's not like ACPI will impose some tables on us and expect us to use them as-is no matter how crazy they are. Then indeed the whole idea of unifying ACPI and DT accessors would be moot and it would only work by mistake or as a good joke. The situation with ACPI is just like with DT, it is assumed that the author of these tables also look at the Linux kernel drivers to figure out what argument goes where and we can influence them. Yours, Linus Walleij
On Wed, Jan 22, 2014 at 10:58:38AM +0100, Linus Walleij wrote: > On Tue, Jan 21, 2014 at 3:53 PM, Alexandre Courbot <gnurou@gmail.com> wrote: > > > The good (or bad, rather) thing about DT is that we can do whatever we > > please with the new bindings: decide which name or which index > > (doesn't matter here) a GPIO should have. However we don't have this > > control over ACPI, where nothing guarantees that the same index will > > be used for the same GPIO function. > > It's not like ACPI will impose some tables on us and expect us to > use them as-is no matter how crazy they are. Then indeed the whole > idea of unifying ACPI and DT accessors would be moot and it would > only work by mistake or as a good joke. With the current ACPI version we really don't have much of a choice here. Only way we can distinguish between a set of GPIOs is to use index and hope that the vendor puts the GPIOs in the table in same order that the driver expects :-( This is going to get a bit better when ACPI gets properties (like Arnd commented already) as then we can get the correct index based on a name in a table. However, it still requires the vendor to use naming that is suitable for Linux driver in question. > The situation with ACPI is just like with DT, it is assumed that the > author of these tables also look at the Linux kernel drivers to figure > out what argument goes where and we can influence them. I certainly hope, now that Android is becoming more and more important, that the vendors will actually check what the Linux drivers expect and populate the tables with such information that is suitable for both ACPI and DT enabled driver.
On Tue, Jan 21, 2014 at 07:50:06PM +0100, Arnd Bergmann wrote: > I should have another look at the debugfs representation, but isn't > there a global namespace that gets used for all gpios? Neither the > con_id nor the name that the driver picks would be globally unique > and stable across kernel versions, so they don't make a good user > interface. Currently the globally unique name is the GPIO number but that's not stable since it gets dynamically assigned. > I think what we want here is a system-wide unique identifier for > each gpio line that may get represented in debugfs, and use a new > DT mechanism to communicate that. We've mostly been using things based off dev_name() for stability.
Hi, On Fri, Jan 17, 2014 at 02:47:29PM +0800, Chen-Yu Tsai wrote: > Signed-off-by: Chen-Yu Tsai <wens@csie.org> > --- > .../devicetree/bindings/rfkill/rfkill-gpio.txt | 26 ++++++++++++++++++++++ > net/rfkill/rfkill-gpio.c | 23 +++++++++++++++++++ > 2 files changed, 49 insertions(+) > create mode 100644 Documentation/devicetree/bindings/rfkill/rfkill-gpio.txt > > diff --git a/Documentation/devicetree/bindings/rfkill/rfkill-gpio.txt b/Documentation/devicetree/bindings/rfkill/rfkill-gpio.txt > new file mode 100644 > index 0000000..8a07ea4 > --- /dev/null > +++ b/Documentation/devicetree/bindings/rfkill/rfkill-gpio.txt > @@ -0,0 +1,26 @@ > +GPIO controlled RFKILL devices > + > +Required properties: > +- compatible : Must be "rfkill-gpio". > +- rfkill-name : Name of RFKILL device > +- rfkill-type : Type of RFKILL device: 1 for WiFi, 2 for BlueTooth > +- NAME_shutdown-gpios : GPIO phandle to shutdown control > + (phandle must be the second) Can't it be handled by a regulator? > +- NAME_reset-gpios : GPIO phandle to reset control And this one using the reset framework? Maxime
Hi, On Mon, Jan 27, 2014 at 10:24 PM, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > Hi, > > On Fri, Jan 17, 2014 at 02:47:29PM +0800, Chen-Yu Tsai wrote: >> Signed-off-by: Chen-Yu Tsai <wens@csie.org> >> --- >> .../devicetree/bindings/rfkill/rfkill-gpio.txt | 26 ++++++++++++++++++++++ >> net/rfkill/rfkill-gpio.c | 23 +++++++++++++++++++ >> 2 files changed, 49 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/rfkill/rfkill-gpio.txt >> >> diff --git a/Documentation/devicetree/bindings/rfkill/rfkill-gpio.txt b/Documentation/devicetree/bindings/rfkill/rfkill-gpio.txt >> new file mode 100644 >> index 0000000..8a07ea4 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/rfkill/rfkill-gpio.txt >> @@ -0,0 +1,26 @@ >> +GPIO controlled RFKILL devices >> + >> +Required properties: >> +- compatible : Must be "rfkill-gpio". >> +- rfkill-name : Name of RFKILL device >> +- rfkill-type : Type of RFKILL device: 1 for WiFi, 2 for BlueTooth >> +- NAME_shutdown-gpios : GPIO phandle to shutdown control >> + (phandle must be the second) > > Can't it be handled by a regulator? > >> +- NAME_reset-gpios : GPIO phandle to reset control > > And this one using the reset framework? The driver is already used in platform device and ACPI fashions. AFAIK, ACPI only passes the GPIO lines. Preferably the behavior and requirements between the different usages remain the same. Cheers ChenYu
diff --git a/Documentation/devicetree/bindings/rfkill/rfkill-gpio.txt b/Documentation/devicetree/bindings/rfkill/rfkill-gpio.txt new file mode 100644 index 0000000..8a07ea4 --- /dev/null +++ b/Documentation/devicetree/bindings/rfkill/rfkill-gpio.txt @@ -0,0 +1,26 @@ +GPIO controlled RFKILL devices + +Required properties: +- compatible : Must be "rfkill-gpio". +- rfkill-name : Name of RFKILL device +- rfkill-type : Type of RFKILL device: 1 for WiFi, 2 for BlueTooth +- NAME_shutdown-gpios : GPIO phandle to shutdown control + (phandle must be the second) +- NAME_reset-gpios : GPIO phandle to reset control + +NAME must match the rfkill-name property. NAME_shutdown-gpios or +NAME_reset-gpios, or both, must be defined. + +Optional properties: +- clocks : phandle to clock to enable/disable + +Example: + + rfkill_bt: rfkill@0 { + compatible = "rfkill-gpio"; + rfkill-name = "bluetooth"; + rfkill-type = <2>; + bluetooth_shutdown-gpios = <0>, <&pio 7 18 0>; + bluetooth_reset-gpios = <&pio 7 24 0>; + clocks = <&clk_out_a>; + }; diff --git a/net/rfkill/rfkill-gpio.c b/net/rfkill/rfkill-gpio.c index 3084fa3..48381a8 100644 --- a/net/rfkill/rfkill-gpio.c +++ b/net/rfkill/rfkill-gpio.c @@ -26,6 +26,7 @@ #include <linux/slab.h> #include <linux/acpi.h> #include <linux/gpio/consumer.h> +#include <linux/of_gpio.h> #include <linux/rfkill-gpio.h> @@ -83,6 +84,18 @@ static int rfkill_gpio_acpi_probe(struct device *dev, return 0; } +static int rfkill_gpio_dt_probe(struct device *dev, + struct rfkill_gpio_data *rfkill) +{ + struct device_node * np = dev->of_node; + + rfkill->name = np->name; + of_property_read_string(np, "rfkill-name", &rfkill->name); + of_property_read_u32(np, "rfkill-type", &rfkill->type); + + return 0; +} + static int rfkill_gpio_probe(struct platform_device *pdev) { struct rfkill_gpio_platform_data *pdata = pdev->dev.platform_data; @@ -100,6 +113,10 @@ static int rfkill_gpio_probe(struct platform_device *pdev) ret = rfkill_gpio_acpi_probe(&pdev->dev, rfkill); if (ret) return ret; + } else if (&pdev->dev.of_node) { + ret = rfkill_gpio_dt_probe(&pdev->dev, rfkill); + if (ret) + return ret; } else if (pdata) { clk_name = pdata->power_clk_name; rfkill->name = pdata->name; @@ -189,6 +206,11 @@ static const struct acpi_device_id rfkill_acpi_match[] = { { }, }; +static const struct of_device_id rfkill_of_match[] = { + { .compatible = "rfkill-gpio", }, + {}, +}; + static struct platform_driver rfkill_gpio_driver = { .probe = rfkill_gpio_probe, .remove = rfkill_gpio_remove, @@ -196,6 +218,7 @@ static struct platform_driver rfkill_gpio_driver = { .name = "rfkill_gpio", .owner = THIS_MODULE, .acpi_match_table = ACPI_PTR(rfkill_acpi_match), + .of_match_table = of_match_ptr(rfkill_of_match), }, };
Signed-off-by: Chen-Yu Tsai <wens@csie.org> --- .../devicetree/bindings/rfkill/rfkill-gpio.txt | 26 ++++++++++++++++++++++ net/rfkill/rfkill-gpio.c | 23 +++++++++++++++++++ 2 files changed, 49 insertions(+) create mode 100644 Documentation/devicetree/bindings/rfkill/rfkill-gpio.txt