| Message ID | 1392900697-27577-3-git-send-email-heikki.krogerus@linux.intel.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 02/20/2014 05:51 AM, Heikki Krogerus wrote: > There is no use for them in this driver. This will fix a > static checker warning.. Didn't you remove the use: - gpio = devm_gpiod_get_index(&pdev->dev, rfkill->reset_name, 0); + gpio = devm_gpiod_get_index(&pdev->dev, NULL, 0); doesn't that parameter get put into the sysfs GPIO debug file, so people can see which GPIOs are used for what? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Fri, Feb 21, 2014 at 12:38 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 02/20/2014 05:51 AM, Heikki Krogerus wrote: >> There is no use for them in this driver. This will fix a >> static checker warning.. > > Didn't you remove the use: > > - gpio = devm_gpiod_get_index(&pdev->dev, rfkill->reset_name, 0); > + gpio = devm_gpiod_get_index(&pdev->dev, NULL, 0); > > doesn't that parameter get put into the sysfs GPIO debug file, so people > can see which GPIOs are used for what? That's correct. However using con_id to pass this results in different behavior across DT and ACPI. A better way is to export the labeling function so consumers can set meaningful labels themselves. Cheers ChenYu -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/20/2014 06:55 PM, Chen-Yu Tsai wrote: > Hi, > > On Fri, Feb 21, 2014 at 12:38 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 02/20/2014 05:51 AM, Heikki Krogerus wrote: >>> There is no use for them in this driver. This will fix a >>> static checker warning.. >> >> Didn't you remove the use: >> >> - gpio = devm_gpiod_get_index(&pdev->dev, rfkill->reset_name, 0); >> + gpio = devm_gpiod_get_index(&pdev->dev, NULL, 0); >> >> doesn't that parameter get put into the sysfs GPIO debug file, so people >> can see which GPIOs are used for what? > > That's correct. However using con_id to pass this results in different > behavior across DT and ACPI. A better way is to export the labeling > function so consumers can set meaningful labels themselves. But this code is the consumer of those GPIOs. IF the parameter to devm_gpiod_get_index() isn't intended to be used, why does it exist? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 21, 2014 at 6:35 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 02/20/2014 06:55 PM, Chen-Yu Tsai wrote: >> That's correct. However using con_id to pass this results in different >> behavior across DT and ACPI. A better way is to export the labeling >> function so consumers can set meaningful labels themselves. > > But this code is the consumer of those GPIOs. IF the parameter to > devm_gpiod_get_index() isn't intended to be used, why does it exist? Kerneldoc says: /** * gpiod_get_index - obtain a GPIO from a multi-index GPIO function * @dev: GPIO consumer, can be NULL for system-global GPIOs * @con_id: function within the GPIO consumer * @idx: index of the GPIO to obtain in the consumer * Basically it is just exposing the fact that of_find_gpio() and acpi_find_gpio() both take a con_id as argument. If we drill into this, we find that it is used to conjure the arbitrary string before the gpios in the DT case, like: foo-gpios = <...>; As in tegra30-beaver.dts... sdhci@78000000 { status = "okay"; cd-gpios = <&gpio TEGRA_GPIO(I, 5) GPIO_ACTIVE_LOW>; wp-gpios = <&gpio TEGRA_GPIO(T, 3) GPIO_ACTIVE_HIGH>; power-gpios = <&gpio TEGRA_GPIO(D, 7) GPIO_ACTIVE_HIGH>; bus-width = <4>; }; Instead of passing the GPIOs as index 0,1,2 they are named and I do admit this has a nice "things are under control" aspect to it. In the ACPI case the con_id is not used for anything. So it is basically there to satisfy the habit in some device tree bindings to name gpio arrays instead of just passing gpios = <...>; (The latter should be encouraged going forward.) As DT is ABI we need to keep this forever, and driver may need to look for foo-gpios=<> and gpios=<> alike for backward compatibility if we'd change it, sweet isn't it? :-) I don't quite like this, as it is adding stupid nonsens arguments for ACPI GPIO producers (which only take an index AFAICT), but it is a first sacrifice on the altar of trying to mask the differences between DT and ACPI probe paths about which I have mixed feelings. Yours, LInus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/25/2014 02:13 AM, Linus Walleij wrote: > On Fri, Feb 21, 2014 at 6:35 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 02/20/2014 06:55 PM, Chen-Yu Tsai wrote: > >>> That's correct. However using con_id to pass this results in different >>> behavior across DT and ACPI. A better way is to export the labeling >>> function so consumers can set meaningful labels themselves. ... > As in tegra30-beaver.dts... > > sdhci@78000000 { > status = "okay"; > cd-gpios = <&gpio TEGRA_GPIO(I, 5) GPIO_ACTIVE_LOW>; > wp-gpios = <&gpio TEGRA_GPIO(T, 3) GPIO_ACTIVE_HIGH>; > power-gpios = <&gpio TEGRA_GPIO(D, 7) GPIO_ACTIVE_HIGH>; > bus-width = <4>; > }; > > Instead of passing the GPIOs as index 0,1,2 they are named > and I do admit this has a nice "things are under control" aspect > to it. > > In the ACPI case the con_id is not used for anything. > > So it is basically there to satisfy the habit in some device > tree bindings to name gpio arrays instead of just passing gpios = <...>; > (The latter should be encouraged going forward.) Do you really want to switch from named GPIO lookups to index-based GPIO lookups? Index-based lookups make it much harder to extend the DT binding in a backwards-compatible fashion, especially in the face of optional GPIOs (of which all of CD, WP, power are). If we switch to a single gpios property, I'd assert we should still do named-based lookups using a parallel gpio-names property, just like most (all?) other resource types now support. If we do that, we'll still need the name parameter. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Please know that no one should not consider me an authority on ACPI at this time. But, I have some comments / context / thoughts below. Also I apologize in advance for any email formatting issues caused by replying to this via my work exchange account / outlook client. Folks can use mgross@linux.intel.com to avoid outlook-isms from me in the future. > -----Original Message----- > From: Linus Walleij [mailto:linus.walleij@linaro.org] > Sent: Tuesday, February 25, 2014 1:14 AM > To: Stephen Warren; Alexandre Courbot; Grant Likely; > devicetree@vger.kernel.org > Cc: Chen-Yu Tsai; Heikki Krogerus; Johannes Berg; David S. Miller; Rhyland > Klein; linux-wireless; netdev; linux-kernel; Arnd Bergmann; Gross, Mark > Subject: Re: [PATCH 2/4] net: rfkill: gpio: remove gpio names > > On Fri, Feb 21, 2014 at 6:35 AM, Stephen Warren > <swarren@wwwdotorg.org> wrote: > > On 02/20/2014 06:55 PM, Chen-Yu Tsai wrote: > > >> That's correct. However using con_id to pass this results in > >> different behavior across DT and ACPI. A better way is to export the > >> labeling function so consumers can set meaningful labels themselves. > > > > But this code is the consumer of those GPIOs. IF the parameter to > > devm_gpiod_get_index() isn't intended to be used, why does it exist? > > Kerneldoc says: > > /** > * gpiod_get_index - obtain a GPIO from a multi-index GPIO function > * @dev: GPIO consumer, can be NULL for system-global GPIOs > * @con_id: function within the GPIO consumer > * @idx: index of the GPIO to obtain in the consumer > * > > Basically it is just exposing the fact that of_find_gpio() and > acpi_find_gpio() both take a con_id as argument. > > If we drill into this, we find that it is used to conjure the arbitrary string > before the gpios in the DT case, like: > > foo-gpios = <...>; > > As in tegra30-beaver.dts... > > sdhci@78000000 { > status = "okay"; > cd-gpios = <&gpio TEGRA_GPIO(I, 5) GPIO_ACTIVE_LOW>; > wp-gpios = <&gpio TEGRA_GPIO(T, 3) GPIO_ACTIVE_HIGH>; > power-gpios = <&gpio TEGRA_GPIO(D, 7) GPIO_ACTIVE_HIGH>; > bus-width = <4>; > }; > > Instead of passing the GPIOs as index 0,1,2 they are named and I do admit > this has a nice "things are under control" aspect to it. [Gross, Mark] FWIW I don't think this is as "under control" as you do. Those names in the above sdhci example are derived from a specific SDHCI tegra spec sheet or schematic. Those names likely come from the data sheet for the controller. I would like SDHCI kernel drivers to work pretty much the same for all compatible controllers. The set of compatible controllers have spec sheets that use different naming conventions for their pins and thus a name space pollution of the SDHCI enumeration ABI will be a problem. > > In the ACPI case the con_id is not used for anything. > > So it is basically there to satisfy the habit in some device tree bindings to > name gpio arrays instead of just passing gpios = <...>; (The latter should be > encouraged going forward.) [Gross, Mark] This is in fact just an idiom of the ACPI ABI inherited from writing linux drivers that work on systems with BIOS/FW developed for MS-Windows. For the devices I work on we have a number of driver teams filing bugs against the BIOS/FW to add named GPIO objects to device name spaces such that they can directly query for the GPIO their driver needs and avoid assuming the 3rd GPIO is the special one they need for whatever... So if you have the luxury of being able to influence (file bugs against or write) the platform enumeration ABI then with ACPI you can have a named gpio today. Note: there is an expectation that the _PRP feature to go into the next version of ACPI and that should enable a trivial 1-1 mapping (when _prp is used by the ACPI platform) between the different platform enumeration API's (DT and ACPI). Still, from a platform enumeration ABI point of view I see the arbitrariness of indexes not much worse than the arbitrary naming of pins off of spec sheets or schematics. Given that the authors of those specs/layouts come up with a new naming schemes every time they log into their workstations (especially for the schematics). I do admit names are a little better but still have the scaling issue WRT namespaces and arbitrary naming by HW engineers over time. > > As DT is ABI we need to keep this forever, and driver may need to look for > foo-gpios=<> and gpios=<> alike for backward compatibility if we'd change it, > sweet isn't it? :-) > > I don't quite like this, as it is adding stupid nonsens arguments for ACPI GPIO > producers (which only take an index AFAICT), but it is a first sacrifice on the > altar of trying to mask the differences between DT and ACPI probe paths > about which I have mixed feelings. [Gross, Mark] I don't have mixed feelings. I want to see converged probe paths that can handle both ACPI and DT platform ABI's seamlessly so we can reuse drivers across architectures effectively. I think the _PRP will help with that even though its use assumes you have influence over the platform FW and this will not help with supporting of legacy BIOS's will burden us with gpio index assumptions. --mark > > Yours, > LInus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/27/2014 10:38 AM, Gross, Mark wrote: > Please know that no one should not consider me an authority on ACPI at this time. But, I have some comments / context / thoughts below. > > Also I apologize in advance for any email formatting issues caused by replying to this via my work exchange account / outlook client. Folks can use mgross@linux.intel.com to avoid outlook-isms from me in the future. > >> -----Original Message----- >> From: Linus Walleij [mailto:linus.walleij@linaro.org] >> Sent: Tuesday, February 25, 2014 1:14 AM >> To: Stephen Warren; Alexandre Courbot; Grant Likely; >> devicetree@vger.kernel.org >> Cc: Chen-Yu Tsai; Heikki Krogerus; Johannes Berg; David S. Miller; Rhyland >> Klein; linux-wireless; netdev; linux-kernel; Arnd Bergmann; Gross, Mark >> Subject: Re: [PATCH 2/4] net: rfkill: gpio: remove gpio names >> >> On Fri, Feb 21, 2014 at 6:35 AM, Stephen Warren >> <swarren@wwwdotorg.org> wrote: >>> On 02/20/2014 06:55 PM, Chen-Yu Tsai wrote: >> >>>> That's correct. However using con_id to pass this results in >>>> different behavior across DT and ACPI. A better way is to export the >>>> labeling function so consumers can set meaningful labels themselves. >>> >>> But this code is the consumer of those GPIOs. IF the parameter to >>> devm_gpiod_get_index() isn't intended to be used, why does it exist? >> >> Kerneldoc says: >> >> /** >> * gpiod_get_index - obtain a GPIO from a multi-index GPIO function >> * @dev: GPIO consumer, can be NULL for system-global GPIOs >> * @con_id: function within the GPIO consumer >> * @idx: index of the GPIO to obtain in the consumer >> * >> >> Basically it is just exposing the fact that of_find_gpio() and >> acpi_find_gpio() both take a con_id as argument. >> >> If we drill into this, we find that it is used to conjure the arbitrary string >> before the gpios in the DT case, like: >> >> foo-gpios = <...>; >> >> As in tegra30-beaver.dts... >> >> sdhci@78000000 { >> status = "okay"; >> cd-gpios = <&gpio TEGRA_GPIO(I, 5) GPIO_ACTIVE_LOW>; >> wp-gpios = <&gpio TEGRA_GPIO(T, 3) GPIO_ACTIVE_HIGH>; >> power-gpios = <&gpio TEGRA_GPIO(D, 7) GPIO_ACTIVE_HIGH>; >> bus-width = <4>; >> }; >> >> Instead of passing the GPIOs as index 0,1,2 they are named and I do admit >> this has a nice "things are under control" aspect to it. > > [Gross, Mark] FWIW I don't think this is as "under control" as you do. Those > names in the above sdhci example are derived from a specific SDHCI tegra spec > sheet or schematic. Those names likely come from the data sheet for > the controller. The names of the properties are fixed and defined by the DT binding for the Tegra SDHCI controller, or even the core SDHCI bindings. Hence, they will be the same in every DT file that uses that Tegra SDHCI compatible value (the compatible property isn't show above, because the above fragment is a board.dts file, and the compatible value gets inherited from the soc.dtsi file). There won't be any variation at all, irrespective of what signal names exist in a particular board schematic. If there were ever an (upstream?) ACPI "binding"(?) for the Tegra SDHCI controller, I would hope it would use the exact same names for the GPIO signals. -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 27, 2014 at 10:47:58AM -0700, Stephen Warren wrote: > On 02/27/2014 10:38 AM, Gross, Mark wrote: > > Please know that no one should not consider me an authority on ACPI at this > > time. But, I have some comments / context / thoughts below. > > > > Also I apologize in advance for any email formatting issues caused by > > replying to this via my work exchange account / outlook client. Folks can > > use mgross@linux.intel.com to avoid outlook-isms from me in the future. > > > >> -----Original Message----- > >> From: Linus Walleij [mailto:linus.walleij@linaro.org] > >> Sent: Tuesday, February 25, 2014 1:14 AM > >> To: Stephen Warren; Alexandre Courbot; Grant Likely; > >> devicetree@vger.kernel.org > >> Cc: Chen-Yu Tsai; Heikki Krogerus; Johannes Berg; David S. Miller; Rhyland > >> Klein; linux-wireless; netdev; linux-kernel; Arnd Bergmann; Gross, Mark > >> Subject: Re: [PATCH 2/4] net: rfkill: gpio: remove gpio names > >> > >> On Fri, Feb 21, 2014 at 6:35 AM, Stephen Warren > >> <swarren@wwwdotorg.org> wrote: > >>> On 02/20/2014 06:55 PM, Chen-Yu Tsai wrote: > >> > >>>> That's correct. However using con_id to pass this results in > >>>> different behavior across DT and ACPI. A better way is to export the > >>>> labeling function so consumers can set meaningful labels themselves. > >>> > >>> But this code is the consumer of those GPIOs. IF the parameter to > >>> devm_gpiod_get_index() isn't intended to be used, why does it exist? > >> > >> Kerneldoc says: > >> > >> /** > >> * gpiod_get_index - obtain a GPIO from a multi-index GPIO function > >> * @dev: GPIO consumer, can be NULL for system-global GPIOs > >> * @con_id: function within the GPIO consumer > >> * @idx: index of the GPIO to obtain in the consumer > >> * > >> > >> Basically it is just exposing the fact that of_find_gpio() and > >> acpi_find_gpio() both take a con_id as argument. > >> > >> If we drill into this, we find that it is used to conjure the arbitrary string > >> before the gpios in the DT case, like: > >> > >> foo-gpios = <...>; > >> > >> As in tegra30-beaver.dts... > >> > >> sdhci@78000000 { > >> status = "okay"; > >> cd-gpios = <&gpio TEGRA_GPIO(I, 5) GPIO_ACTIVE_LOW>; > >> wp-gpios = <&gpio TEGRA_GPIO(T, 3) GPIO_ACTIVE_HIGH>; > >> power-gpios = <&gpio TEGRA_GPIO(D, 7) GPIO_ACTIVE_HIGH>; > >> bus-width = <4>; > >> }; > >> > >> Instead of passing the GPIOs as index 0,1,2 they are named and I do admit > >> this has a nice "things are under control" aspect to it. > > > > [Gross, Mark] FWIW I don't think this is as "under control" as you do. Those > > names in the above sdhci example are derived from a specific SDHCI > tegra spec > > sheet or schematic. Those names likely come from the data sheet for > > the controller. > > The names of the properties are fixed and defined by the DT binding for > the Tegra SDHCI controller, or even the core SDHCI bindings. Hence, they > will be the same in every DT file that uses that Tegra SDHCI compatible > value (the compatible property isn't show above, because the above > fragment is a board.dts file, and the compatible value gets inherited > from the soc.dtsi file). There won't be any variation at all, > irrespective of what signal names exist in a particular board schematic. > > If there were ever an (upstream?) ACPI "binding"(?) for the Tegra SDHCI > controller, I would hope it would use the exact same names for the GPIO > signals. me to! --mark -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 26, 2014 at 1:35 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 02/25/2014 02:13 AM, Linus Walleij wrote: >> So it is basically there to satisfy the habit in some device >> tree bindings to name gpio arrays instead of just passing gpios = <...>; >> (The latter should be encouraged going forward.) > > Do you really want to switch from named GPIO lookups to index-based GPIO > lookups? I don't know what I want. I only know that ACPI and DT people are starting to step on each others feet. > Index-based lookups make it much harder to extend the DT > binding in a backwards-compatible fashion, especially in the face of > optional GPIOs (of which all of CD, WP, power are). You're probably right. At the same time it makes the world complex for the ambition to use the same interface for DT and ACPI alike. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 28, 2014 at 1:38 AM, Gross, Mark <mark.gross@intel.com> wrote: > So if you have the luxury of being able to influence (file bugs against or write) > the platform enumeration ABI then with ACPI you can have a named gpio today. > Note: there is an expectation that the _PRP feature to go into the next version > of ACPI and that should enable a trivial 1-1 mapping (when _prp is used by the > ACPI platform) between the different platform enumeration API's (DT and ACPI). Oh I wasn't aware. This makes things better, and gives con_id a clear usecase in the ACPI probe path as well. [Stephen] >> If there were ever an (upstream?) ACPI "binding"(?) for the Tegra SDHCI >> controller, I would hope it would use the exact same names for the GPIO >> signals. > > me to! Amen to that. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/rfkill/rfkill-gpio.c b/net/rfkill/rfkill-gpio.c index 0adda44..ad5e354 100644 --- a/net/rfkill/rfkill-gpio.c +++ b/net/rfkill/rfkill-gpio.c @@ -36,8 +36,6 @@ struct rfkill_gpio_data { struct gpio_desc *shutdown_gpio; struct rfkill *rfkill_dev; - char *reset_name; - char *shutdown_name; struct clk *clk; bool clk_enabled; @@ -89,7 +87,6 @@ static int rfkill_gpio_probe(struct platform_device *pdev) struct rfkill_gpio_data *rfkill; struct gpio_desc *gpio; int ret; - int len; rfkill = devm_kzalloc(&pdev->dev, sizeof(*rfkill), GFP_KERNEL); if (!rfkill) @@ -106,21 +103,9 @@ static int rfkill_gpio_probe(struct platform_device *pdev) return -ENODEV; } - len = strlen(rfkill->name); - rfkill->reset_name = devm_kzalloc(&pdev->dev, len + 7, GFP_KERNEL); - if (!rfkill->reset_name) - return -ENOMEM; - - rfkill->shutdown_name = devm_kzalloc(&pdev->dev, len + 10, GFP_KERNEL); - if (!rfkill->shutdown_name) - return -ENOMEM; - - snprintf(rfkill->reset_name, len + 6 , "%s_reset", rfkill->name); - snprintf(rfkill->shutdown_name, len + 9, "%s_shutdown", rfkill->name); - rfkill->clk = devm_clk_get(&pdev->dev, NULL); - gpio = devm_gpiod_get_index(&pdev->dev, rfkill->reset_name, 0); + gpio = devm_gpiod_get_index(&pdev->dev, NULL, 0); if (!IS_ERR(gpio)) { ret = gpiod_direction_output(gpio, 0); if (ret) @@ -128,7 +113,7 @@ static int rfkill_gpio_probe(struct platform_device *pdev) rfkill->reset_gpio = gpio; } - gpio = devm_gpiod_get_index(&pdev->dev, rfkill->shutdown_name, 1); + gpio = devm_gpiod_get_index(&pdev->dev, NULL, 1); if (!IS_ERR(gpio)) { ret = gpiod_direction_output(gpio, 0); if (ret)
There is no use for them in this driver. This will fix a static checker warning.. net/rfkill/rfkill-gpio.c:144 rfkill_gpio_probe() warn: variable dereferenced before check 'rfkill->name' This will also make sure that when DT support is added, "gpios" property can be used as no con_id labels are provided. Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> --- net/rfkill/rfkill-gpio.c | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-)