| Message ID | 1393330950-7283-4-git-send-email-heikki.krogerus@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/25/2014 05:22 AM, Heikki Krogerus wrote: > 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. > diff --git a/net/rfkill/rfkill-gpio.c b/net/rfkill/rfkill-gpio.c > - gpio = devm_gpiod_get_index(&pdev->dev, rfkill->reset_name, 0); > + gpio = devm_gpiod_get_index(&pdev->dev, NULL, 0); I think the correct fix here is to look up the GPIO by name rather than by index, but simply hard-code the name rather than generating it with sprintf(). Index lookups are hard to expand compatibly, but named-based lookups scale much better. In other words, I rather specifically disagree with using a plain "gpios" property in any future DT binding, but would strongly prefer e.g. reset-gpios/shutdown-gpios or gpios/gpio-names. Still, I guess I don't object too much.
On Wed, Feb 26, 2014 at 7:04 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> - gpio = devm_gpiod_get_index(&pdev->dev, rfkill->reset_name, 0); >> + gpio = devm_gpiod_get_index(&pdev->dev, NULL, 0); > > I think the correct fix here is to look up the GPIO by name rather than > by index, but simply hard-code the name rather than generating it with > sprintf(). Index lookups are hard to expand compatibly, but named-based > lookups scale much better. > > In other words, I rather specifically disagree with using a plain > "gpios" property in any future DT binding, but would strongly prefer > e.g. reset-gpios/shutdown-gpios or gpios/gpio-names. If I understand the situation correctly it's like ACPI does not have named GPIOs so keeping specifying this in DT GPIO bindings is counter-productive to the work of abstracting the access to GPIO handlers so that drivers need not know whether ACPI or DT is used for describing the hardware. We could do worse. Like putting the GPIOs in a differently indexed order in DT vs ACPI. I have no strong opinion really, I just see that people doing DT and ACPI HW descriptions need to cooperate if they want to share infrastructure or we have to give up that pipe dream and let each HW description method have its own unique probe()-runpath. Which would be the result in this driver if we persist on using named GPIOs. Yours, Linus Walleij
On 03/04/2014 06:43 PM, Linus Walleij wrote: > On Wed, Feb 26, 2014 at 7:04 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > >>> - gpio = devm_gpiod_get_index(&pdev->dev, rfkill->reset_name, 0); >>> + gpio = devm_gpiod_get_index(&pdev->dev, NULL, 0); >> >> I think the correct fix here is to look up the GPIO by name rather than >> by index, but simply hard-code the name rather than generating it with >> sprintf(). Index lookups are hard to expand compatibly, but named-based >> lookups scale much better. >> >> In other words, I rather specifically disagree with using a plain >> "gpios" property in any future DT binding, but would strongly prefer >> e.g. reset-gpios/shutdown-gpios or gpios/gpio-names. > > If I understand the situation correctly it's like ACPI does not have named > GPIOs so keeping specifying this in DT GPIO bindings is counter-productive > to the work of abstracting the access to GPIO handlers so that drivers > need not know whether ACPI or DT is used for describing the hardware. For devices that already have both ACPI and DT bindings, we can't pretend they can be the same; they are already potentially different. We simply need to parse DT and ACPI differently, since that's the sway their bindings are defined. For any devices that don't have both ACPI and DT bindings, I agree we should certainly strive to make any new bindings aligned so we don't have to deal with this for them. However, we can't change the past.
On Wed, Mar 5, 2014 at 10:18 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 03/04/2014 06:43 PM, Linus Walleij wrote: >> If I understand the situation correctly it's like ACPI does not have named >> GPIOs so keeping specifying this in DT GPIO bindings is counter-productive >> to the work of abstracting the access to GPIO handlers so that drivers >> need not know whether ACPI or DT is used for describing the hardware. > > For devices that already have both ACPI and DT bindings, we can't > pretend they can be the same; they are already potentially different. We > simply need to parse DT and ACPI differently, since that's the sway > their bindings are defined. > > For any devices that don't have both ACPI and DT bindings, I agree we > should certainly strive to make any new bindings aligned so we don't > have to deal with this for them. > > However, we can't change the past. Yeah, right, so for this very driver there are no bindings defined (yet) and the only device tree I can find referencing it is the Tegra20-paz00 and it just use gpios = <>; So in this case I think this patch is the right way forward, but I admit I'm really uncertain in the general case. Yours, Linus Walleij
On 03/04/2014 07:37 PM, Linus Walleij wrote: > On Wed, Mar 5, 2014 at 10:18 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 03/04/2014 06:43 PM, Linus Walleij wrote: > >>> If I understand the situation correctly it's like ACPI does not have named >>> GPIOs so keeping specifying this in DT GPIO bindings is counter-productive >>> to the work of abstracting the access to GPIO handlers so that drivers >>> need not know whether ACPI or DT is used for describing the hardware. >> >> For devices that already have both ACPI and DT bindings, we can't >> pretend they can be the same; they are already potentially different. We >> simply need to parse DT and ACPI differently, since that's the sway >> their bindings are defined. >> >> For any devices that don't have both ACPI and DT bindings, I agree we >> should certainly strive to make any new bindings aligned so we don't >> have to deal with this for them. >> >> However, we can't change the past. > > Yeah, right, so for this very driver there are no bindings defined (yet) > and the only device tree I can find referencing it is the Tegra20-paz00 > and it just use gpios = <>; > > So in this case I think this patch is the right way forward, but I admit > I'm really uncertain in the general case. If there are no bindings defined at all yet, then we can define both DT and ACPI bindings to use name-based GPIOs. Index-based lookups aren't a good way forward.
On Wed, Mar 5, 2014 at 10:59 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 03/04/2014 07:37 PM, Linus Walleij wrote: >> On Wed, Mar 5, 2014 at 10:18 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: >>> On 03/04/2014 06:43 PM, Linus Walleij wrote: >> >>>> If I understand the situation correctly it's like ACPI does not have named >>>> GPIOs so keeping specifying this in DT GPIO bindings is counter-productive >>>> to the work of abstracting the access to GPIO handlers so that drivers >>>> need not know whether ACPI or DT is used for describing the hardware. >>> >>> For devices that already have both ACPI and DT bindings, we can't >>> pretend they can be the same; they are already potentially different. We >>> simply need to parse DT and ACPI differently, since that's the sway >>> their bindings are defined. >>> >>> For any devices that don't have both ACPI and DT bindings, I agree we >>> should certainly strive to make any new bindings aligned so we don't >>> have to deal with this for them. >>> >>> However, we can't change the past. >> >> Yeah, right, so for this very driver there are no bindings defined (yet) >> and the only device tree I can find referencing it is the Tegra20-paz00 >> and it just use gpios = <>; >> >> So in this case I think this patch is the right way forward, but I admit >> I'm really uncertain in the general case. > > If there are no bindings defined at all yet, then we can define both DT > and ACPI bindings to use name-based GPIOs. Index-based lookups aren't a > good way forward. After Mark clarifying that ACPI is going to have named GPIOs I'm totally aligned on this, so OK! Yours, Linus Walleij
On Fri, Mar 7, 2014 at 11:41 AM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Wed, Mar 5, 2014 at 10:59 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 03/04/2014 07:37 PM, Linus Walleij wrote: >>> On Wed, Mar 5, 2014 at 10:18 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: >>>> On 03/04/2014 06:43 PM, Linus Walleij wrote: >>> >>>>> If I understand the situation correctly it's like ACPI does not have named >>>>> GPIOs so keeping specifying this in DT GPIO bindings is counter-productive >>>>> to the work of abstracting the access to GPIO handlers so that drivers >>>>> need not know whether ACPI or DT is used for describing the hardware. >>>> >>>> For devices that already have both ACPI and DT bindings, we can't >>>> pretend they can be the same; they are already potentially different. We >>>> simply need to parse DT and ACPI differently, since that's the sway >>>> their bindings are defined. >>>> >>>> For any devices that don't have both ACPI and DT bindings, I agree we >>>> should certainly strive to make any new bindings aligned so we don't >>>> have to deal with this for them. >>>> >>>> However, we can't change the past. >>> >>> Yeah, right, so for this very driver there are no bindings defined (yet) >>> and the only device tree I can find referencing it is the Tegra20-paz00 >>> and it just use gpios = <>; >>> >>> So in this case I think this patch is the right way forward, but I admit >>> I'm really uncertain in the general case. >> >> If there are no bindings defined at all yet, then we can define both DT >> and ACPI bindings to use name-based GPIOs. Index-based lookups aren't a >> good way forward. > > After Mark clarifying that ACPI is going to have named GPIOs I'm > totally aligned on this, so OK! Glad to hear this, but is it possible to get rid of the index in current drivers? Or change the behavior to name-based OR index-based lookups. This might break any DTs that have multiple GPIOs defined under one property though. Cheers ChenYu
On 03/06/2014 08:43 PM, Chen-Yu Tsai wrote: > On Fri, Mar 7, 2014 at 11:41 AM, Linus Walleij <linus.walleij@linaro.org> wrote: >> On Wed, Mar 5, 2014 at 10:59 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: >>> On 03/04/2014 07:37 PM, Linus Walleij wrote: >>>> On Wed, Mar 5, 2014 at 10:18 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: >>>>> On 03/04/2014 06:43 PM, Linus Walleij wrote: >>>> >>>>>> If I understand the situation correctly it's like ACPI does not have named >>>>>> GPIOs so keeping specifying this in DT GPIO bindings is counter-productive >>>>>> to the work of abstracting the access to GPIO handlers so that drivers >>>>>> need not know whether ACPI or DT is used for describing the hardware. >>>>> >>>>> For devices that already have both ACPI and DT bindings, we can't >>>>> pretend they can be the same; they are already potentially different. We >>>>> simply need to parse DT and ACPI differently, since that's the sway >>>>> their bindings are defined. >>>>> >>>>> For any devices that don't have both ACPI and DT bindings, I agree we >>>>> should certainly strive to make any new bindings aligned so we don't >>>>> have to deal with this for them. >>>>> >>>>> However, we can't change the past. >>>> >>>> Yeah, right, so for this very driver there are no bindings defined (yet) >>>> and the only device tree I can find referencing it is the Tegra20-paz00 >>>> and it just use gpios = <>; >>>> >>>> So in this case I think this patch is the right way forward, but I admit >>>> I'm really uncertain in the general case. >>> >>> If there are no bindings defined at all yet, then we can define both DT >>> and ACPI bindings to use name-based GPIOs. Index-based lookups aren't a >>> good way forward. >> >> After Mark clarifying that ACPI is going to have named GPIOs I'm >> totally aligned on this, so OK! > > Glad to hear this, but is it possible to get rid of the index in current > drivers? Or change the behavior to name-based OR index-based lookups. > This might break any DTs that have multiple GPIOs defined under one > property though. For any bindings that are already defined to use index-based lookups, I think we have to continue using them, for backwards-compatibility with old DTs (and I assume old ACPI databases need the same thing).
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(-)