* [PATCH V2 0/4] input: keyboard: tegra: cleanups and DT supports @ 2013-01-05 7:45 Laxman Dewangan 2013-01-05 7:45 ` [PATCH v2 1/4] input: keyboard: tegra: fix build warning Laxman Dewangan ` (3 more replies) 0 siblings, 4 replies; 24+ messages in thread From: Laxman Dewangan @ 2013-01-05 7:45 UTC (permalink / raw) To: dmitry.torokhov Cc: grant.likely, rob.herring, swarren, devicetree-discuss, linux-doc, linux-kernel, linux-input, linux-tegra, Laxman Dewangan This patch series: - fix build warning, - use devm_* for allocation, - make column/rows configuration through DT and - remove the rarely used key mapping table. Changes from V1: - renames the rows and pins property array. - nit cleanups. Laxman Dewangan (4): input: keyboard: tegra: fix build warning input: keyboard: tegra: use devm_* for resource allocation input: keyboard: tegra: add support for rows/cols configuration from dt input: keyboard: tegra: remove default key mapping .../bindings/input/nvidia,tegra20-kbc.txt | 22 ++ drivers/input/keyboard/tegra-kbc.c | 345 ++++++-------------- 2 files changed, 118 insertions(+), 249 deletions(-) ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2 1/4] input: keyboard: tegra: fix build warning 2013-01-05 7:45 [PATCH V2 0/4] input: keyboard: tegra: cleanups and DT supports Laxman Dewangan @ 2013-01-05 7:45 ` Laxman Dewangan 2013-01-05 7:45 ` [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation Laxman Dewangan ` (2 subsequent siblings) 3 siblings, 0 replies; 24+ messages in thread From: Laxman Dewangan @ 2013-01-05 7:45 UTC (permalink / raw) To: dmitry.torokhov Cc: grant.likely, rob.herring, swarren, devicetree-discuss, linux-doc, linux-kernel, linux-input, linux-tegra, Laxman Dewangan Fix the following build warning when building driver with CONFIG_PM_SLEEP not selected. tegra-kbc.c:360:13: warning: 'tegra_kbc_set_keypress_interrupt' defined but not used [-Wunused-function] Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com> --- Changes form V1: - none drivers/input/keyboard/tegra-kbc.c | 24 ++++++++++++------------ 1 files changed, 12 insertions(+), 12 deletions(-) diff --git a/drivers/input/keyboard/tegra-kbc.c b/drivers/input/keyboard/tegra-kbc.c index c76f968..f1d3ba0 100644 --- a/drivers/input/keyboard/tegra-kbc.c +++ b/drivers/input/keyboard/tegra-kbc.c @@ -357,18 +357,6 @@ static void tegra_kbc_set_fifo_interrupt(struct tegra_kbc *kbc, bool enable) writel(val, kbc->mmio + KBC_CONTROL_0); } -static void tegra_kbc_set_keypress_interrupt(struct tegra_kbc *kbc, bool enable) -{ - u32 val; - - val = readl(kbc->mmio + KBC_CONTROL_0); - if (enable) - val |= KBC_CONTROL_KEYPRESS_INT_EN; - else - val &= ~KBC_CONTROL_KEYPRESS_INT_EN; - writel(val, kbc->mmio + KBC_CONTROL_0); -} - static void tegra_kbc_keypress_timer(unsigned long data) { struct tegra_kbc *kbc = (struct tegra_kbc *)data; @@ -866,6 +854,18 @@ static int tegra_kbc_remove(struct platform_device *pdev) } #ifdef CONFIG_PM_SLEEP +static void tegra_kbc_set_keypress_interrupt(struct tegra_kbc *kbc, bool enable) +{ + u32 val; + + val = readl(kbc->mmio + KBC_CONTROL_0); + if (enable) + val |= KBC_CONTROL_KEYPRESS_INT_EN; + else + val &= ~KBC_CONTROL_KEYPRESS_INT_EN; + writel(val, kbc->mmio + KBC_CONTROL_0); +} + static int tegra_kbc_suspend(struct device *dev) { struct platform_device *pdev = to_platform_device(dev); -- 1.7.1.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation 2013-01-05 7:45 [PATCH V2 0/4] input: keyboard: tegra: cleanups and DT supports Laxman Dewangan 2013-01-05 7:45 ` [PATCH v2 1/4] input: keyboard: tegra: fix build warning Laxman Dewangan @ 2013-01-05 7:45 ` Laxman Dewangan 2013-01-05 8:06 ` Dmitry Torokhov 2013-01-05 7:45 ` [PATCH v2] input: keyboard: tegra: add support for rows/cols configuration from dt Laxman Dewangan 2013-01-05 7:45 ` [PATCH v2 4/4] input: keyboard: tegra: remove default key mapping Laxman Dewangan 3 siblings, 1 reply; 24+ messages in thread From: Laxman Dewangan @ 2013-01-05 7:45 UTC (permalink / raw) To: dmitry.torokhov Cc: grant.likely, rob.herring, swarren, devicetree-discuss, linux-doc, linux-kernel, linux-input, linux-tegra, Laxman Dewangan Use devm_* for memory, clock, input device allocation. This reduces code for freeing these resources. Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com> --- Changes from V1: None drivers/input/keyboard/tegra-kbc.c | 93 +++++++++++------------------------- 1 files changed, 28 insertions(+), 65 deletions(-) diff --git a/drivers/input/keyboard/tegra-kbc.c b/drivers/input/keyboard/tegra-kbc.c index f1d3ba0..c036425 100644 --- a/drivers/input/keyboard/tegra-kbc.c +++ b/drivers/input/keyboard/tegra-kbc.c @@ -618,7 +618,7 @@ static struct tegra_kbc_platform_data *tegra_kbc_dt_parse_pdata( if (!np) return NULL; - pdata = kzalloc(sizeof(*pdata), GFP_KERNEL); + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL); if (!pdata) return NULL; @@ -700,33 +700,36 @@ static int tegra_kbc_probe(struct platform_device *pdev) if (!pdata) pdata = tegra_kbc_dt_parse_pdata(pdev); - if (!pdata) + if (!pdata) { + dev_err(&pdev->dev, "Platform data missing\n"); return -EINVAL; - - if (!tegra_kbc_check_pin_cfg(pdata, &pdev->dev, &num_rows)) { - err = -EINVAL; - goto err_free_pdata; } + if (!tegra_kbc_check_pin_cfg(pdata, &pdev->dev, &num_rows)) + return -EINVAL; + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); if (!res) { dev_err(&pdev->dev, "failed to get I/O memory\n"); - err = -ENXIO; - goto err_free_pdata; + return -ENXIO; } irq = platform_get_irq(pdev, 0); if (irq < 0) { dev_err(&pdev->dev, "failed to get keyboard IRQ\n"); - err = -ENXIO; - goto err_free_pdata; + return -ENXIO; + } + + kbc = devm_kzalloc(&pdev->dev, sizeof(*kbc), GFP_KERNEL); + if (!kbc) { + dev_err(&pdev->dev, "failed to alloc memory for kbc\n"); + return -ENOMEM; } - kbc = kzalloc(sizeof(*kbc), GFP_KERNEL); - input_dev = input_allocate_device(); - if (!kbc || !input_dev) { - err = -ENOMEM; - goto err_free_mem; + input_dev = devm_input_allocate_device(&pdev->dev); + if (!input_dev) { + dev_err(&pdev->dev, "failed to allocate input device\n"); + return -ENOMEM; } kbc->pdata = pdata; @@ -735,25 +738,16 @@ static int tegra_kbc_probe(struct platform_device *pdev) spin_lock_init(&kbc->lock); setup_timer(&kbc->timer, tegra_kbc_keypress_timer, (unsigned long)kbc); - res = request_mem_region(res->start, resource_size(res), pdev->name); - if (!res) { - dev_err(&pdev->dev, "failed to request I/O memory\n"); - err = -EBUSY; - goto err_free_mem; - } - - kbc->mmio = ioremap(res->start, resource_size(res)); + kbc->mmio = devm_request_and_ioremap(&pdev->dev, res); if (!kbc->mmio) { - dev_err(&pdev->dev, "failed to remap I/O memory\n"); - err = -ENXIO; - goto err_free_mem_region; + dev_err(&pdev->dev, "Cannot request memregion/iomap address\n"); + return -EADDRNOTAVAIL; } - kbc->clk = clk_get(&pdev->dev, NULL); + kbc->clk = devm_clk_get(&pdev->dev, NULL); if (IS_ERR(kbc->clk)) { dev_err(&pdev->dev, "failed to get keyboard clock\n"); - err = PTR_ERR(kbc->clk); - goto err_iounmap; + return PTR_ERR(kbc->clk); } /* @@ -778,9 +772,9 @@ static int tegra_kbc_probe(struct platform_device *pdev) input_dev->close = tegra_kbc_close; err = tegra_kbd_setup_keymap(kbc); - if (err) { + if (err < 0) { dev_err(&pdev->dev, "failed to setup keymap\n"); - goto err_put_clk; + return err; } __set_bit(EV_REP, input_dev->evbit); @@ -790,15 +784,15 @@ static int tegra_kbc_probe(struct platform_device *pdev) err = request_irq(kbc->irq, tegra_kbc_isr, IRQF_NO_SUSPEND | IRQF_TRIGGER_HIGH, pdev->name, kbc); - if (err) { + if (err < 0) { dev_err(&pdev->dev, "failed to request keyboard IRQ\n"); - goto err_put_clk; + return err; } disable_irq(kbc->irq); err = input_register_device(kbc->idev); - if (err) { + if (err < 0) { dev_err(&pdev->dev, "failed to register input device\n"); goto err_free_irq; } @@ -810,46 +804,15 @@ static int tegra_kbc_probe(struct platform_device *pdev) err_free_irq: free_irq(kbc->irq, pdev); -err_put_clk: - clk_put(kbc->clk); -err_iounmap: - iounmap(kbc->mmio); -err_free_mem_region: - release_mem_region(res->start, resource_size(res)); -err_free_mem: - input_free_device(input_dev); - kfree(kbc); -err_free_pdata: - if (!pdev->dev.platform_data) - kfree(pdata); - return err; } static int tegra_kbc_remove(struct platform_device *pdev) { struct tegra_kbc *kbc = platform_get_drvdata(pdev); - struct resource *res; - - platform_set_drvdata(pdev, NULL); free_irq(kbc->irq, pdev); - clk_put(kbc->clk); - input_unregister_device(kbc->idev); - iounmap(kbc->mmio); - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); - release_mem_region(res->start, resource_size(res)); - - /* - * If we do not have platform data attached to the device we - * allocated it ourselves and thus need to free it. - */ - if (!pdev->dev.platform_data) - kfree(kbc->pdata); - - kfree(kbc); - return 0; } -- 1.7.1.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation 2013-01-05 7:45 ` [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation Laxman Dewangan @ 2013-01-05 8:06 ` Dmitry Torokhov 2013-01-05 11:20 ` Laxman Dewangan 2013-01-06 19:27 ` Thierry Reding 0 siblings, 2 replies; 24+ messages in thread From: Dmitry Torokhov @ 2013-01-05 8:06 UTC (permalink / raw) To: Laxman Dewangan Cc: grant.likely, rob.herring, swarren, devicetree-discuss, linux-doc, linux-kernel, linux-input, linux-tegra Hi Laxman, On Sat, Jan 05, 2013 at 01:15:08PM +0530, Laxman Dewangan wrote: > Use devm_* for memory, clock, input device allocation. This reduces > code for freeing these resources. > > Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com> > --- > Changes from V1: > None > > drivers/input/keyboard/tegra-kbc.c | 93 +++++++++++------------------------- > 1 files changed, 28 insertions(+), 65 deletions(-) > > diff --git a/drivers/input/keyboard/tegra-kbc.c b/drivers/input/keyboard/tegra-kbc.c > index f1d3ba0..c036425 100644 > --- a/drivers/input/keyboard/tegra-kbc.c > +++ b/drivers/input/keyboard/tegra-kbc.c > @@ -618,7 +618,7 @@ static struct tegra_kbc_platform_data *tegra_kbc_dt_parse_pdata( > if (!np) > return NULL; > > - pdata = kzalloc(sizeof(*pdata), GFP_KERNEL); > + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL); > if (!pdata) > return NULL; > > @@ -700,33 +700,36 @@ static int tegra_kbc_probe(struct platform_device *pdev) > if (!pdata) > pdata = tegra_kbc_dt_parse_pdata(pdev); > > - if (!pdata) > + if (!pdata) { > + dev_err(&pdev->dev, "Platform data missing\n"); > return -EINVAL; > - > - if (!tegra_kbc_check_pin_cfg(pdata, &pdev->dev, &num_rows)) { > - err = -EINVAL; > - goto err_free_pdata; > } > > + if (!tegra_kbc_check_pin_cfg(pdata, &pdev->dev, &num_rows)) > + return -EINVAL; > + > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > if (!res) { > dev_err(&pdev->dev, "failed to get I/O memory\n"); > - err = -ENXIO; > - goto err_free_pdata; > + return -ENXIO; > } > > irq = platform_get_irq(pdev, 0); > if (irq < 0) { > dev_err(&pdev->dev, "failed to get keyboard IRQ\n"); > - err = -ENXIO; > - goto err_free_pdata; > + return -ENXIO; > + } > + > + kbc = devm_kzalloc(&pdev->dev, sizeof(*kbc), GFP_KERNEL); > + if (!kbc) { > + dev_err(&pdev->dev, "failed to alloc memory for kbc\n"); > + return -ENOMEM; > } > > - kbc = kzalloc(sizeof(*kbc), GFP_KERNEL); > - input_dev = input_allocate_device(); > - if (!kbc || !input_dev) { > - err = -ENOMEM; > - goto err_free_mem; > + input_dev = devm_input_allocate_device(&pdev->dev); > + if (!input_dev) { > + dev_err(&pdev->dev, "failed to allocate input device\n"); > + return -ENOMEM; > } > > kbc->pdata = pdata; > @@ -735,25 +738,16 @@ static int tegra_kbc_probe(struct platform_device *pdev) > spin_lock_init(&kbc->lock); > setup_timer(&kbc->timer, tegra_kbc_keypress_timer, (unsigned long)kbc); > > - res = request_mem_region(res->start, resource_size(res), pdev->name); > - if (!res) { > - dev_err(&pdev->dev, "failed to request I/O memory\n"); > - err = -EBUSY; > - goto err_free_mem; > - } > - > - kbc->mmio = ioremap(res->start, resource_size(res)); > + kbc->mmio = devm_request_and_ioremap(&pdev->dev, res); > if (!kbc->mmio) { > - dev_err(&pdev->dev, "failed to remap I/O memory\n"); > - err = -ENXIO; > - goto err_free_mem_region; > + dev_err(&pdev->dev, "Cannot request memregion/iomap address\n"); > + return -EADDRNOTAVAIL; Erm, no, -EBUSY please. > } > > - kbc->clk = clk_get(&pdev->dev, NULL); > + kbc->clk = devm_clk_get(&pdev->dev, NULL); > if (IS_ERR(kbc->clk)) { > dev_err(&pdev->dev, "failed to get keyboard clock\n"); > - err = PTR_ERR(kbc->clk); > - goto err_iounmap; > + return PTR_ERR(kbc->clk); > } > > /* > @@ -778,9 +772,9 @@ static int tegra_kbc_probe(struct platform_device *pdev) > input_dev->close = tegra_kbc_close; > > err = tegra_kbd_setup_keymap(kbc); > - if (err) { > + if (err < 0) { Why is this change? As far as I can see tegra_kbd_setup_keymap() never returns positive values. > dev_err(&pdev->dev, "failed to setup keymap\n"); > - goto err_put_clk; > + return err; > } > > __set_bit(EV_REP, input_dev->evbit); > @@ -790,15 +784,15 @@ static int tegra_kbc_probe(struct platform_device *pdev) > > err = request_irq(kbc->irq, tegra_kbc_isr, > IRQF_NO_SUSPEND | IRQF_TRIGGER_HIGH, pdev->name, kbc); > - if (err) { > + if (err < 0) { Neither request_irq(). BTW, why not devm_request_irq? > dev_err(&pdev->dev, "failed to request keyboard IRQ\n"); > - goto err_put_clk; > + return err; > } > > disable_irq(kbc->irq); > > err = input_register_device(kbc->idev); > - if (err) { > + if (err < 0) { Again, input_register_device() returns 0 on success. > dev_err(&pdev->dev, "failed to register input device\n"); > goto err_free_irq; > } > @@ -810,46 +804,15 @@ static int tegra_kbc_probe(struct platform_device *pdev) > > err_free_irq: > free_irq(kbc->irq, pdev); > -err_put_clk: > - clk_put(kbc->clk); > -err_iounmap: > - iounmap(kbc->mmio); > -err_free_mem_region: > - release_mem_region(res->start, resource_size(res)); > -err_free_mem: > - input_free_device(input_dev); > - kfree(kbc); > -err_free_pdata: > - if (!pdev->dev.platform_data) > - kfree(pdata); > - > return err; > } > > static int tegra_kbc_remove(struct platform_device *pdev) > { > struct tegra_kbc *kbc = platform_get_drvdata(pdev); > - struct resource *res; > - > - platform_set_drvdata(pdev, NULL); > > free_irq(kbc->irq, pdev); > - clk_put(kbc->clk); > - > input_unregister_device(kbc->idev); You do not need to call input_unregister_device for managed input devices, and if you switch request_irq to managed variant you can remove tegra_kbc_remove() altogether. > - iounmap(kbc->mmio); > - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > - release_mem_region(res->start, resource_size(res)); > - > - /* > - * If we do not have platform data attached to the device we > - * allocated it ourselves and thus need to free it. > - */ > - if (!pdev->dev.platform_data) > - kfree(kbc->pdata); > - > - kfree(kbc); > - > return 0; > } > > -- > 1.7.1.1 > Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation 2013-01-05 8:06 ` Dmitry Torokhov @ 2013-01-05 11:20 ` Laxman Dewangan 2013-01-05 23:18 ` Dmitry Torokhov 2013-01-06 19:27 ` Thierry Reding 1 sibling, 1 reply; 24+ messages in thread From: Laxman Dewangan @ 2013-01-05 11:20 UTC (permalink / raw) To: Dmitry Torokhov Cc: grant.likely, rob.herring, Stephen Warren, devicetree-discuss, linux-doc, linux-kernel, linux-input, linux-tegra HI Dmitry, Thanks for quick review. I will take care of your comment in next version. Some have my answer. On Saturday 05 January 2013 01:36 PM, Dmitry Torokhov wrote: > Hi Laxman, > > On Sat, Jan 05, 2013 at 01:15:08PM +0530, Laxman Dewangan wrote: >> Use devm_* for memory, clock, input device allocation. This reduces >> code for freeing these resources. >> err = tegra_kbd_setup_keymap(kbc); >> - if (err) { >> + if (err < 0) { > Why is this change? As far as I can see tegra_kbd_setup_keymap() never > returns positive values. Ok, mostly errors are in negative and hence this change, I will revert it and will keep original. > >> dev_err(&pdev->dev, "failed to setup keymap\n"); >> - goto err_put_clk; >> + return err; >> } >> >> __set_bit(EV_REP, input_dev->evbit); >> @@ -790,15 +784,15 @@ static int tegra_kbc_probe(struct platform_device *pdev) >> >> err = request_irq(kbc->irq, tegra_kbc_isr, >> IRQF_NO_SUSPEND | IRQF_TRIGGER_HIGH, pdev->name, kbc); >> - if (err) { >> + if (err < 0) { > Neither request_irq(). BTW, why not devm_request_irq? I understand from Mark B on different patches that using devm_request_irq() can create race condition when removing device. Interrupt can occur when device resource release is in process and so it can cause isr call which can use the freed pointer. devm_request_irq() should be avoided. Thanks, Laxman ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation 2013-01-05 11:20 ` Laxman Dewangan @ 2013-01-05 23:18 ` Dmitry Torokhov 2013-01-06 11:00 ` Laxman Dewangan 0 siblings, 1 reply; 24+ messages in thread From: Dmitry Torokhov @ 2013-01-05 23:18 UTC (permalink / raw) To: Laxman Dewangan Cc: grant.likely, rob.herring, Stephen Warren, devicetree-discuss, linux-doc, linux-kernel, linux-input, linux-tegra On Sat, Jan 05, 2013 at 04:50:58PM +0530, Laxman Dewangan wrote: > HI Dmitry, > Thanks for quick review. > > I will take care of your comment in next version. Some have my answer. > > > On Saturday 05 January 2013 01:36 PM, Dmitry Torokhov wrote: > >Hi Laxman, > > > >On Sat, Jan 05, 2013 at 01:15:08PM +0530, Laxman Dewangan wrote: > >>Use devm_* for memory, clock, input device allocation. This reduces > >>code for freeing these resources. > > >> err = tegra_kbd_setup_keymap(kbc); > >>- if (err) { > >>+ if (err < 0) { > >Why is this change? As far as I can see tegra_kbd_setup_keymap() never > >returns positive values. > > Ok, mostly errors are in negative and hence this change, I will > revert it and will keep original. > > > > >> dev_err(&pdev->dev, "failed to setup keymap\n"); > >>- goto err_put_clk; > >>+ return err; > >> } > >> __set_bit(EV_REP, input_dev->evbit); > >>@@ -790,15 +784,15 @@ static int tegra_kbc_probe(struct platform_device *pdev) > >> err = request_irq(kbc->irq, tegra_kbc_isr, > >> IRQF_NO_SUSPEND | IRQF_TRIGGER_HIGH, pdev->name, kbc); > >>- if (err) { > >>+ if (err < 0) { > >Neither request_irq(). BTW, why not devm_request_irq? > > I understand from Mark B on different patches that using > devm_request_irq() can create race condition when removing device. > Interrupt can occur when device resource release is in process and > so it can cause isr call which can use the freed pointer. > devm_request_irq() should be avoided. devm_request_irq() has a potential of creating a race condition, but it depents on the driver. In this particular case tegra driver ensures that interrupts are inhibited when input device is unregistered by providing tegra_kbc_close() method, so in this particular case it is safe to use devm_request_irq(). Also, when using managed input devices, the unregistering and final freeing is a 2-step process, so even in absence of close() method, if initialization sequence was: devm_input_allocate_device() ... devm_request_irq() ... input_unregister_device() then order of freeing resources (behind the scenes) will be devm_input_device_unregister(); /* input device is still present in memory and can * handle input_event() calls. */ free_irq(); devm_input_device_release(); So using managed request_irq() _together_ with managed input devices is OK. Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation 2013-01-05 23:18 ` Dmitry Torokhov @ 2013-01-06 11:00 ` Laxman Dewangan 0 siblings, 0 replies; 24+ messages in thread From: Laxman Dewangan @ 2013-01-06 11:00 UTC (permalink / raw) To: Dmitry Torokhov Cc: grant.likely, rob.herring, Stephen Warren, devicetree-discuss, linux-doc, linux-kernel, linux-input, linux-tegra On Sunday 06 January 2013 04:48 AM, Dmitry Torokhov wrote: > On Sat, Jan 05, 2013 at 04:50:58PM +0530, Laxman Dewangan wrote: >> HI Dmitry, >> Thanks for quick review. >> >> I will take care of your comment in next version. Some have my answer. >> >> >> On Saturday 05 January 2013 01:36 PM, Dmitry Torokhov wrote: >>> Hi Laxman, >>> >>> On Sat, Jan 05, 2013 at 01:15:08PM +0530, Laxman Dewangan wrote: >>>> Use devm_* for memory, clock, input device allocation. This reduces >>>> code for freeing these resources. >>>> err = tegra_kbd_setup_keymap(kbc); >>>> - if (err) { >>>> + if (err < 0) { >>> Why is this change? As far as I can see tegra_kbd_setup_keymap() never >>> returns positive values. >> Ok, mostly errors are in negative and hence this change, I will >> revert it and will keep original. >> >>>> dev_err(&pdev->dev, "failed to setup keymap\n"); >>>> - goto err_put_clk; >>>> + return err; >>>> } >>>> __set_bit(EV_REP, input_dev->evbit); >>>> @@ -790,15 +784,15 @@ static int tegra_kbc_probe(struct platform_device *pdev) >>>> err = request_irq(kbc->irq, tegra_kbc_isr, >>>> IRQF_NO_SUSPEND | IRQF_TRIGGER_HIGH, pdev->name, kbc); >>>> - if (err) { >>>> + if (err < 0) { >>> Neither request_irq(). BTW, why not devm_request_irq? >> I understand from Mark B on different patches that using >> devm_request_irq() can create race condition when removing device. >> Interrupt can occur when device resource release is in process and >> so it can cause isr call which can use the freed pointer. >> devm_request_irq() should be avoided. > devm_request_irq() has a potential of creating a race condition, but it > depents on the driver. In this particular case tegra driver ensures that > interrupts are inhibited when input device is unregistered by providing > tegra_kbc_close() method, so in this particular case it is safe to > use devm_request_irq(). > > Also, when using managed input devices, the unregistering and final > freeing is a 2-step process, so even in absence of close() method, if > initialization sequence was: > > devm_input_allocate_device() > ... > devm_request_irq() > ... > input_unregister_device() > > then order of freeing resources (behind the scenes) will be > > devm_input_device_unregister(); > /* input device is still present in memory and can > * handle input_event() calls. > */ > free_irq(); > devm_input_device_release(); > > So using managed request_irq() _together_ with managed input devices is > OK. Thanks for detail explanation. I will modify and sent the new version of patch. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation 2013-01-05 8:06 ` Dmitry Torokhov 2013-01-05 11:20 ` Laxman Dewangan @ 2013-01-06 19:27 ` Thierry Reding 2013-01-06 19:57 ` Dmitry Torokhov 1 sibling, 1 reply; 24+ messages in thread From: Thierry Reding @ 2013-01-06 19:27 UTC (permalink / raw) To: Dmitry Torokhov Cc: Laxman Dewangan, grant.likely, rob.herring, swarren, devicetree-discuss, linux-doc, linux-kernel, linux-input, linux-tegra [-- Attachment #1: Type: text/plain, Size: 1154 bytes --] On Sat, Jan 05, 2013 at 12:06:58AM -0800, Dmitry Torokhov wrote: > On Sat, Jan 05, 2013 at 01:15:08PM +0530, Laxman Dewangan wrote: [...] > > @@ -735,25 +738,16 @@ static int tegra_kbc_probe(struct platform_device *pdev) > > spin_lock_init(&kbc->lock); > > setup_timer(&kbc->timer, tegra_kbc_keypress_timer, (unsigned long)kbc); > > > > - res = request_mem_region(res->start, resource_size(res), pdev->name); > > - if (!res) { > > - dev_err(&pdev->dev, "failed to request I/O memory\n"); > > - err = -EBUSY; > > - goto err_free_mem; > > - } > > - > > - kbc->mmio = ioremap(res->start, resource_size(res)); > > + kbc->mmio = devm_request_and_ioremap(&pdev->dev, res); > > if (!kbc->mmio) { > > - dev_err(&pdev->dev, "failed to remap I/O memory\n"); > > - err = -ENXIO; > > - goto err_free_mem_region; > > + dev_err(&pdev->dev, "Cannot request memregion/iomap address\n"); > > + return -EADDRNOTAVAIL; > > Erm, no, -EBUSY please. EADDRNOTAVAIL is the canonical error for devm_request_and_ioremap() failure. The kerneldoc comment in lib/devres.c even gives a short example that uses this error code. Thierry [-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation 2013-01-06 19:27 ` Thierry Reding @ 2013-01-06 19:57 ` Dmitry Torokhov 2013-01-09 7:07 ` Thierry Reding 0 siblings, 1 reply; 24+ messages in thread From: Dmitry Torokhov @ 2013-01-06 19:57 UTC (permalink / raw) To: Thierry Reding Cc: Laxman Dewangan, grant.likely, rob.herring, swarren, devicetree-discuss, linux-doc, linux-kernel, linux-input, linux-tegra On Sun, Jan 06, 2013 at 08:27:39PM +0100, Thierry Reding wrote: > On Sat, Jan 05, 2013 at 12:06:58AM -0800, Dmitry Torokhov wrote: > > On Sat, Jan 05, 2013 at 01:15:08PM +0530, Laxman Dewangan wrote: > [...] > > > @@ -735,25 +738,16 @@ static int tegra_kbc_probe(struct platform_device *pdev) > > > spin_lock_init(&kbc->lock); > > > setup_timer(&kbc->timer, tegra_kbc_keypress_timer, (unsigned long)kbc); > > > > > > - res = request_mem_region(res->start, resource_size(res), pdev->name); > > > - if (!res) { > > > - dev_err(&pdev->dev, "failed to request I/O memory\n"); > > > - err = -EBUSY; > > > - goto err_free_mem; > > > - } > > > - > > > - kbc->mmio = ioremap(res->start, resource_size(res)); > > > + kbc->mmio = devm_request_and_ioremap(&pdev->dev, res); > > > if (!kbc->mmio) { > > > - dev_err(&pdev->dev, "failed to remap I/O memory\n"); > > > - err = -ENXIO; > > > - goto err_free_mem_region; > > > + dev_err(&pdev->dev, "Cannot request memregion/iomap address\n"); > > > + return -EADDRNOTAVAIL; > > > > Erm, no, -EBUSY please. > > EADDRNOTAVAIL is the canonical error for devm_request_and_ioremap() > failure. The kerneldoc comment in lib/devres.c even gives a short > example that uses this error code. I am sorry, but I do not consider a function that was added a little over a year ago as a canon. If you look at the uses of EADDRNOTAVAIL it is used predominantly in networking code to indicate that attempted _network_ address is not available. Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation 2013-01-06 19:57 ` Dmitry Torokhov @ 2013-01-09 7:07 ` Thierry Reding 2013-01-09 9:19 ` Dmitry Torokhov 0 siblings, 1 reply; 24+ messages in thread From: Thierry Reding @ 2013-01-09 7:07 UTC (permalink / raw) To: Dmitry Torokhov Cc: Laxman Dewangan, grant.likely, rob.herring, swarren, devicetree-discuss, linux-doc, linux-kernel, linux-input, linux-tegra, Wolfram Sang [-- Attachment #1: Type: text/plain, Size: 2347 bytes --] On Sun, Jan 06, 2013 at 11:57:48AM -0800, Dmitry Torokhov wrote: > On Sun, Jan 06, 2013 at 08:27:39PM +0100, Thierry Reding wrote: > > On Sat, Jan 05, 2013 at 12:06:58AM -0800, Dmitry Torokhov wrote: > > > On Sat, Jan 05, 2013 at 01:15:08PM +0530, Laxman Dewangan wrote: > > [...] > > > > @@ -735,25 +738,16 @@ static int tegra_kbc_probe(struct platform_device *pdev) > > > > spin_lock_init(&kbc->lock); > > > > setup_timer(&kbc->timer, tegra_kbc_keypress_timer, (unsigned long)kbc); > > > > > > > > - res = request_mem_region(res->start, resource_size(res), pdev->name); > > > > - if (!res) { > > > > - dev_err(&pdev->dev, "failed to request I/O memory\n"); > > > > - err = -EBUSY; > > > > - goto err_free_mem; > > > > - } > > > > - > > > > - kbc->mmio = ioremap(res->start, resource_size(res)); > > > > + kbc->mmio = devm_request_and_ioremap(&pdev->dev, res); > > > > if (!kbc->mmio) { > > > > - dev_err(&pdev->dev, "failed to remap I/O memory\n"); > > > > - err = -ENXIO; > > > > - goto err_free_mem_region; > > > > + dev_err(&pdev->dev, "Cannot request memregion/iomap address\n"); > > > > + return -EADDRNOTAVAIL; > > > > > > Erm, no, -EBUSY please. > > > > EADDRNOTAVAIL is the canonical error for devm_request_and_ioremap() > > failure. The kerneldoc comment in lib/devres.c even gives a short > > example that uses this error code. > > I am sorry, but I do not consider a function that was added a little > over a year ago as a canon. If you look at the uses of EADDRNOTAVAIL it > is used predominantly in networking code to indicate that attempted > _network_ address is not available. EBUSY might be misleading, though. devm_request_and_ioremap() can fail in both the request_mem_region() and ioremap() calls. Furthermore it'd be good to settle on a consistent error-code instead of doing it differently depending on subsystem and/or driver. Currently the various error codes used are: EBUSY, EADDRNOTAVAIL, ENXIO, ENOMEM, ENODEV, ENOENT, EINVAL, EIO, EFAULT, EADDRINUSE Also if we can settle on one error code we should follow up with a patch to make it consistent across the tree and also update that kerneldoc comment. I volunteer to do that if nobody else steps up. I'm also Cc'ing Wolfram (the original author), maybe he has some thoughts on this. Thierry [-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation 2013-01-09 7:07 ` Thierry Reding @ 2013-01-09 9:19 ` Dmitry Torokhov 2013-01-09 9:23 ` Thierry Reding 2013-02-09 9:04 ` Grant Likely 0 siblings, 2 replies; 24+ messages in thread From: Dmitry Torokhov @ 2013-01-09 9:19 UTC (permalink / raw) To: Thierry Reding Cc: Laxman Dewangan, grant.likely, rob.herring, swarren, devicetree-discuss, linux-doc, linux-kernel, linux-input, linux-tegra, Wolfram Sang On Wed, Jan 09, 2013 at 08:07:45AM +0100, Thierry Reding wrote: > On Sun, Jan 06, 2013 at 11:57:48AM -0800, Dmitry Torokhov wrote: > > On Sun, Jan 06, 2013 at 08:27:39PM +0100, Thierry Reding wrote: > > > On Sat, Jan 05, 2013 at 12:06:58AM -0800, Dmitry Torokhov wrote: > > > > On Sat, Jan 05, 2013 at 01:15:08PM +0530, Laxman Dewangan wrote: > > > [...] > > > > > @@ -735,25 +738,16 @@ static int tegra_kbc_probe(struct platform_device *pdev) > > > > > spin_lock_init(&kbc->lock); > > > > > setup_timer(&kbc->timer, tegra_kbc_keypress_timer, (unsigned long)kbc); > > > > > > > > > > - res = request_mem_region(res->start, resource_size(res), pdev->name); > > > > > - if (!res) { > > > > > - dev_err(&pdev->dev, "failed to request I/O memory\n"); > > > > > - err = -EBUSY; > > > > > - goto err_free_mem; > > > > > - } > > > > > - > > > > > - kbc->mmio = ioremap(res->start, resource_size(res)); > > > > > + kbc->mmio = devm_request_and_ioremap(&pdev->dev, res); > > > > > if (!kbc->mmio) { > > > > > - dev_err(&pdev->dev, "failed to remap I/O memory\n"); > > > > > - err = -ENXIO; > > > > > - goto err_free_mem_region; > > > > > + dev_err(&pdev->dev, "Cannot request memregion/iomap address\n"); > > > > > + return -EADDRNOTAVAIL; > > > > > > > > Erm, no, -EBUSY please. > > > > > > EADDRNOTAVAIL is the canonical error for devm_request_and_ioremap() > > > failure. The kerneldoc comment in lib/devres.c even gives a short > > > example that uses this error code. > > > > I am sorry, but I do not consider a function that was added a little > > over a year ago as a canon. If you look at the uses of EADDRNOTAVAIL it > > is used predominantly in networking code to indicate that attempted > > _network_ address is not available. > > EBUSY might be misleading, though. devm_request_and_ioremap() can fail > in both the request_mem_region() and ioremap() calls. Furthermore it'd > be good to settle on a consistent error-code instead of doing it > differently depending on subsystem and/or driver. Currently the various > error codes used are: > > EBUSY, EADDRNOTAVAIL, ENXIO, ENOMEM, ENODEV, ENOENT, EINVAL, > EIO, EFAULT, EADDRINUSE > > Also if we can settle on one error code we should follow up with a patch > to make it consistent across the tree and also update that kerneldoc > comment. I volunteer to do that if nobody else steps up. I'm also Cc'ing > Wolfram (the original author), maybe he has some thoughts on this. > If you going to change all drivers make devm_request_and_ioremap() return ERR_PTR()-encoded errors and then we can differentiate what part of it failed. Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation 2013-01-09 9:19 ` Dmitry Torokhov @ 2013-01-09 9:23 ` Thierry Reding 2013-01-14 15:49 ` Thierry Reding 2013-02-09 9:04 ` Grant Likely 1 sibling, 1 reply; 24+ messages in thread From: Thierry Reding @ 2013-01-09 9:23 UTC (permalink / raw) To: Dmitry Torokhov Cc: Laxman Dewangan, grant.likely, rob.herring, swarren, devicetree-discuss, linux-doc, linux-kernel, linux-input, linux-tegra, Wolfram Sang [-- Attachment #1: Type: text/plain, Size: 2983 bytes --] On Wed, Jan 09, 2013 at 01:19:39AM -0800, Dmitry Torokhov wrote: > On Wed, Jan 09, 2013 at 08:07:45AM +0100, Thierry Reding wrote: > > On Sun, Jan 06, 2013 at 11:57:48AM -0800, Dmitry Torokhov wrote: > > > On Sun, Jan 06, 2013 at 08:27:39PM +0100, Thierry Reding wrote: > > > > On Sat, Jan 05, 2013 at 12:06:58AM -0800, Dmitry Torokhov wrote: > > > > > On Sat, Jan 05, 2013 at 01:15:08PM +0530, Laxman Dewangan wrote: > > > > [...] > > > > > > @@ -735,25 +738,16 @@ static int tegra_kbc_probe(struct platform_device *pdev) > > > > > > spin_lock_init(&kbc->lock); > > > > > > setup_timer(&kbc->timer, tegra_kbc_keypress_timer, (unsigned long)kbc); > > > > > > > > > > > > - res = request_mem_region(res->start, resource_size(res), pdev->name); > > > > > > - if (!res) { > > > > > > - dev_err(&pdev->dev, "failed to request I/O memory\n"); > > > > > > - err = -EBUSY; > > > > > > - goto err_free_mem; > > > > > > - } > > > > > > - > > > > > > - kbc->mmio = ioremap(res->start, resource_size(res)); > > > > > > + kbc->mmio = devm_request_and_ioremap(&pdev->dev, res); > > > > > > if (!kbc->mmio) { > > > > > > - dev_err(&pdev->dev, "failed to remap I/O memory\n"); > > > > > > - err = -ENXIO; > > > > > > - goto err_free_mem_region; > > > > > > + dev_err(&pdev->dev, "Cannot request memregion/iomap address\n"); > > > > > > + return -EADDRNOTAVAIL; > > > > > > > > > > Erm, no, -EBUSY please. > > > > > > > > EADDRNOTAVAIL is the canonical error for devm_request_and_ioremap() > > > > failure. The kerneldoc comment in lib/devres.c even gives a short > > > > example that uses this error code. > > > > > > I am sorry, but I do not consider a function that was added a little > > > over a year ago as a canon. If you look at the uses of EADDRNOTAVAIL it > > > is used predominantly in networking code to indicate that attempted > > > _network_ address is not available. > > > > EBUSY might be misleading, though. devm_request_and_ioremap() can fail > > in both the request_mem_region() and ioremap() calls. Furthermore it'd > > be good to settle on a consistent error-code instead of doing it > > differently depending on subsystem and/or driver. Currently the various > > error codes used are: > > > > EBUSY, EADDRNOTAVAIL, ENXIO, ENOMEM, ENODEV, ENOENT, EINVAL, > > EIO, EFAULT, EADDRINUSE > > > > Also if we can settle on one error code we should follow up with a patch > > to make it consistent across the tree and also update that kerneldoc > > comment. I volunteer to do that if nobody else steps up. I'm also Cc'ing > > Wolfram (the original author), maybe he has some thoughts on this. > > > > If you going to change all drivers make devm_request_and_ioremap() > return ERR_PTR()-encoded errors and then we can differentiate what > part of it failed. Yeah, that thought also crossed my mind. I'll give other people some time to comment before hurling myself into preparing patches. Thierry [-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation 2013-01-09 9:23 ` Thierry Reding @ 2013-01-14 15:49 ` Thierry Reding 2013-01-14 16:16 ` Greg Kroah-Hartman 2013-01-15 13:06 ` Wolfram Sang 0 siblings, 2 replies; 24+ messages in thread From: Thierry Reding @ 2013-01-14 15:49 UTC (permalink / raw) To: Dmitry Torokhov Cc: Laxman Dewangan, grant.likely, rob.herring, swarren, devicetree-discuss, linux-doc, linux-kernel, linux-input, linux-tegra, Wolfram Sang, Arnd Bergmann, Greg Kroah-Hartman [-- Attachment #1: Type: text/plain, Size: 4207 bytes --] On Wed, Jan 09, 2013 at 10:23:52AM +0100, Thierry Reding wrote: > On Wed, Jan 09, 2013 at 01:19:39AM -0800, Dmitry Torokhov wrote: > > On Wed, Jan 09, 2013 at 08:07:45AM +0100, Thierry Reding wrote: > > > On Sun, Jan 06, 2013 at 11:57:48AM -0800, Dmitry Torokhov wrote: > > > > On Sun, Jan 06, 2013 at 08:27:39PM +0100, Thierry Reding wrote: > > > > > On Sat, Jan 05, 2013 at 12:06:58AM -0800, Dmitry Torokhov wrote: > > > > > > On Sat, Jan 05, 2013 at 01:15:08PM +0530, Laxman Dewangan wrote: > > > > > [...] > > > > > > > @@ -735,25 +738,16 @@ static int tegra_kbc_probe(struct platform_device *pdev) > > > > > > > spin_lock_init(&kbc->lock); > > > > > > > setup_timer(&kbc->timer, tegra_kbc_keypress_timer, (unsigned long)kbc); > > > > > > > > > > > > > > - res = request_mem_region(res->start, resource_size(res), pdev->name); > > > > > > > - if (!res) { > > > > > > > - dev_err(&pdev->dev, "failed to request I/O memory\n"); > > > > > > > - err = -EBUSY; > > > > > > > - goto err_free_mem; > > > > > > > - } > > > > > > > - > > > > > > > - kbc->mmio = ioremap(res->start, resource_size(res)); > > > > > > > + kbc->mmio = devm_request_and_ioremap(&pdev->dev, res); > > > > > > > if (!kbc->mmio) { > > > > > > > - dev_err(&pdev->dev, "failed to remap I/O memory\n"); > > > > > > > - err = -ENXIO; > > > > > > > - goto err_free_mem_region; > > > > > > > + dev_err(&pdev->dev, "Cannot request memregion/iomap address\n"); > > > > > > > + return -EADDRNOTAVAIL; > > > > > > > > > > > > Erm, no, -EBUSY please. > > > > > > > > > > EADDRNOTAVAIL is the canonical error for devm_request_and_ioremap() > > > > > failure. The kerneldoc comment in lib/devres.c even gives a short > > > > > example that uses this error code. > > > > > > > > I am sorry, but I do not consider a function that was added a little > > > > over a year ago as a canon. If you look at the uses of EADDRNOTAVAIL it > > > > is used predominantly in networking code to indicate that attempted > > > > _network_ address is not available. > > > > > > EBUSY might be misleading, though. devm_request_and_ioremap() can fail > > > in both the request_mem_region() and ioremap() calls. Furthermore it'd > > > be good to settle on a consistent error-code instead of doing it > > > differently depending on subsystem and/or driver. Currently the various > > > error codes used are: > > > > > > EBUSY, EADDRNOTAVAIL, ENXIO, ENOMEM, ENODEV, ENOENT, EINVAL, > > > EIO, EFAULT, EADDRINUSE > > > > > > Also if we can settle on one error code we should follow up with a patch > > > to make it consistent across the tree and also update that kerneldoc > > > comment. I volunteer to do that if nobody else steps up. I'm also Cc'ing > > > Wolfram (the original author), maybe he has some thoughts on this. > > > > > > > If you going to change all drivers make devm_request_and_ioremap() > > return ERR_PTR()-encoded errors and then we can differentiate what > > part of it failed. > > Yeah, that thought also crossed my mind. I'll give other people some > time to comment before hurling myself into preparing patches. I've prepared a patch that changes devm_request_and_ioremap() to return ERR_PTR()-encoded errors and adjusts all callers. As you can imagine it is a bit on the huge side. scripts/get_maintainer.pl lists 156 people and mailing lists. I've gone through the list, and as far as I can tell everyone on that list is actually affected by the patch, so there's not much potential for tuning it down. There is also the issue of whose tree this should go into. Unfortunately the patch can't be broken down into smaller chunks because it changes how the devm_request_and_ioremap() function's return value is handled in an incompatible way, so it won't be possible to gradually phase this in. Furthermore I can imagine that until the end of the release cycle new code may be added on which the same transformations need to be done. I have a semantic patch to do the bulk of the work, but quite a bit of coordination will be required. I'm adding Arnd and Greg on Cc, maybe they can advise on how best to handle this kind of patch. Thierry [-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation 2013-01-14 15:49 ` Thierry Reding @ 2013-01-14 16:16 ` Greg Kroah-Hartman 2013-01-14 22:15 ` Thierry Reding 2013-01-15 13:06 ` Wolfram Sang 1 sibling, 1 reply; 24+ messages in thread From: Greg Kroah-Hartman @ 2013-01-14 16:16 UTC (permalink / raw) To: Thierry Reding Cc: Dmitry Torokhov, Laxman Dewangan, grant.likely, rob.herring, swarren, devicetree-discuss, linux-doc, linux-kernel, linux-input, linux-tegra, Wolfram Sang, Arnd Bergmann On Mon, Jan 14, 2013 at 04:49:59PM +0100, Thierry Reding wrote: > On Wed, Jan 09, 2013 at 10:23:52AM +0100, Thierry Reding wrote: > > On Wed, Jan 09, 2013 at 01:19:39AM -0800, Dmitry Torokhov wrote: > > > On Wed, Jan 09, 2013 at 08:07:45AM +0100, Thierry Reding wrote: > > > > On Sun, Jan 06, 2013 at 11:57:48AM -0800, Dmitry Torokhov wrote: > > > > > On Sun, Jan 06, 2013 at 08:27:39PM +0100, Thierry Reding wrote: > > > > > > On Sat, Jan 05, 2013 at 12:06:58AM -0800, Dmitry Torokhov wrote: > > > > > > > On Sat, Jan 05, 2013 at 01:15:08PM +0530, Laxman Dewangan wrote: > > > > > > [...] > > > > > > > > @@ -735,25 +738,16 @@ static int tegra_kbc_probe(struct platform_device *pdev) > > > > > > > > spin_lock_init(&kbc->lock); > > > > > > > > setup_timer(&kbc->timer, tegra_kbc_keypress_timer, (unsigned long)kbc); > > > > > > > > > > > > > > > > - res = request_mem_region(res->start, resource_size(res), pdev->name); > > > > > > > > - if (!res) { > > > > > > > > - dev_err(&pdev->dev, "failed to request I/O memory\n"); > > > > > > > > - err = -EBUSY; > > > > > > > > - goto err_free_mem; > > > > > > > > - } > > > > > > > > - > > > > > > > > - kbc->mmio = ioremap(res->start, resource_size(res)); > > > > > > > > + kbc->mmio = devm_request_and_ioremap(&pdev->dev, res); > > > > > > > > if (!kbc->mmio) { > > > > > > > > - dev_err(&pdev->dev, "failed to remap I/O memory\n"); > > > > > > > > - err = -ENXIO; > > > > > > > > - goto err_free_mem_region; > > > > > > > > + dev_err(&pdev->dev, "Cannot request memregion/iomap address\n"); > > > > > > > > + return -EADDRNOTAVAIL; > > > > > > > > > > > > > > Erm, no, -EBUSY please. > > > > > > > > > > > > EADDRNOTAVAIL is the canonical error for devm_request_and_ioremap() > > > > > > failure. The kerneldoc comment in lib/devres.c even gives a short > > > > > > example that uses this error code. > > > > > > > > > > I am sorry, but I do not consider a function that was added a little > > > > > over a year ago as a canon. If you look at the uses of EADDRNOTAVAIL it > > > > > is used predominantly in networking code to indicate that attempted > > > > > _network_ address is not available. > > > > > > > > EBUSY might be misleading, though. devm_request_and_ioremap() can fail > > > > in both the request_mem_region() and ioremap() calls. Furthermore it'd > > > > be good to settle on a consistent error-code instead of doing it > > > > differently depending on subsystem and/or driver. Currently the various > > > > error codes used are: > > > > > > > > EBUSY, EADDRNOTAVAIL, ENXIO, ENOMEM, ENODEV, ENOENT, EINVAL, > > > > EIO, EFAULT, EADDRINUSE > > > > > > > > Also if we can settle on one error code we should follow up with a patch > > > > to make it consistent across the tree and also update that kerneldoc > > > > comment. I volunteer to do that if nobody else steps up. I'm also Cc'ing > > > > Wolfram (the original author), maybe he has some thoughts on this. > > > > > > > > > > If you going to change all drivers make devm_request_and_ioremap() > > > return ERR_PTR()-encoded errors and then we can differentiate what > > > part of it failed. > > > > Yeah, that thought also crossed my mind. I'll give other people some > > time to comment before hurling myself into preparing patches. > > I've prepared a patch that changes devm_request_and_ioremap() to return > ERR_PTR()-encoded errors and adjusts all callers. As you can imagine it > is a bit on the huge side. scripts/get_maintainer.pl lists 156 people > and mailing lists. I've gone through the list, and as far as I can tell > everyone on that list is actually affected by the patch, so there's not > much potential for tuning it down. > > There is also the issue of whose tree this should go into. Unfortunately > the patch can't be broken down into smaller chunks because it changes > how the devm_request_and_ioremap() function's return value is handled in > an incompatible way, so it won't be possible to gradually phase this in. > Furthermore I can imagine that until the end of the release cycle new > code may be added on which the same transformations need to be done. I > have a semantic patch to do the bulk of the work, but quite a bit of > coordination will be required. > > I'm adding Arnd and Greg on Cc, maybe they can advise on how best to > handle this kind of patch. You should provide a "wrapper" function that does the correct return value, convert drivers over to it, and then, when everyone is changed, drop the old call. To change 156 different drivers all at once, in a way that is not detectable by the compiler breaking the build, is not a good thing to do at all. By doing it in this manner, it will take longer, but you can push the patches through the different maintainer's trees. If they are slow, I'll be glad to take the remaining patches in my driver-core tree to do the final bits. Hope this helps, greg k-h ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation 2013-01-14 16:16 ` Greg Kroah-Hartman @ 2013-01-14 22:15 ` Thierry Reding 2013-01-14 22:24 ` Arnd Bergmann 0 siblings, 1 reply; 24+ messages in thread From: Thierry Reding @ 2013-01-14 22:15 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Dmitry Torokhov, Laxman Dewangan, grant.likely, rob.herring, swarren, devicetree-discuss, linux-doc, linux-kernel, linux-input, linux-tegra, Wolfram Sang, Arnd Bergmann [-- Attachment #1: Type: text/plain, Size: 5465 bytes --] On Mon, Jan 14, 2013 at 08:16:44AM -0800, Greg Kroah-Hartman wrote: > On Mon, Jan 14, 2013 at 04:49:59PM +0100, Thierry Reding wrote: > > On Wed, Jan 09, 2013 at 10:23:52AM +0100, Thierry Reding wrote: > > > On Wed, Jan 09, 2013 at 01:19:39AM -0800, Dmitry Torokhov wrote: > > > > On Wed, Jan 09, 2013 at 08:07:45AM +0100, Thierry Reding wrote: > > > > > On Sun, Jan 06, 2013 at 11:57:48AM -0800, Dmitry Torokhov wrote: > > > > > > On Sun, Jan 06, 2013 at 08:27:39PM +0100, Thierry Reding wrote: > > > > > > > On Sat, Jan 05, 2013 at 12:06:58AM -0800, Dmitry Torokhov wrote: > > > > > > > > On Sat, Jan 05, 2013 at 01:15:08PM +0530, Laxman Dewangan wrote: > > > > > > > [...] > > > > > > > > > @@ -735,25 +738,16 @@ static int tegra_kbc_probe(struct platform_device *pdev) > > > > > > > > > spin_lock_init(&kbc->lock); > > > > > > > > > setup_timer(&kbc->timer, tegra_kbc_keypress_timer, (unsigned long)kbc); > > > > > > > > > > > > > > > > > > - res = request_mem_region(res->start, resource_size(res), pdev->name); > > > > > > > > > - if (!res) { > > > > > > > > > - dev_err(&pdev->dev, "failed to request I/O memory\n"); > > > > > > > > > - err = -EBUSY; > > > > > > > > > - goto err_free_mem; > > > > > > > > > - } > > > > > > > > > - > > > > > > > > > - kbc->mmio = ioremap(res->start, resource_size(res)); > > > > > > > > > + kbc->mmio = devm_request_and_ioremap(&pdev->dev, res); > > > > > > > > > if (!kbc->mmio) { > > > > > > > > > - dev_err(&pdev->dev, "failed to remap I/O memory\n"); > > > > > > > > > - err = -ENXIO; > > > > > > > > > - goto err_free_mem_region; > > > > > > > > > + dev_err(&pdev->dev, "Cannot request memregion/iomap address\n"); > > > > > > > > > + return -EADDRNOTAVAIL; > > > > > > > > > > > > > > > > Erm, no, -EBUSY please. > > > > > > > > > > > > > > EADDRNOTAVAIL is the canonical error for devm_request_and_ioremap() > > > > > > > failure. The kerneldoc comment in lib/devres.c even gives a short > > > > > > > example that uses this error code. > > > > > > > > > > > > I am sorry, but I do not consider a function that was added a little > > > > > > over a year ago as a canon. If you look at the uses of EADDRNOTAVAIL it > > > > > > is used predominantly in networking code to indicate that attempted > > > > > > _network_ address is not available. > > > > > > > > > > EBUSY might be misleading, though. devm_request_and_ioremap() can fail > > > > > in both the request_mem_region() and ioremap() calls. Furthermore it'd > > > > > be good to settle on a consistent error-code instead of doing it > > > > > differently depending on subsystem and/or driver. Currently the various > > > > > error codes used are: > > > > > > > > > > EBUSY, EADDRNOTAVAIL, ENXIO, ENOMEM, ENODEV, ENOENT, EINVAL, > > > > > EIO, EFAULT, EADDRINUSE > > > > > > > > > > Also if we can settle on one error code we should follow up with a patch > > > > > to make it consistent across the tree and also update that kerneldoc > > > > > comment. I volunteer to do that if nobody else steps up. I'm also Cc'ing > > > > > Wolfram (the original author), maybe he has some thoughts on this. > > > > > > > > > > > > > If you going to change all drivers make devm_request_and_ioremap() > > > > return ERR_PTR()-encoded errors and then we can differentiate what > > > > part of it failed. > > > > > > Yeah, that thought also crossed my mind. I'll give other people some > > > time to comment before hurling myself into preparing patches. > > > > I've prepared a patch that changes devm_request_and_ioremap() to return > > ERR_PTR()-encoded errors and adjusts all callers. As you can imagine it > > is a bit on the huge side. scripts/get_maintainer.pl lists 156 people > > and mailing lists. I've gone through the list, and as far as I can tell > > everyone on that list is actually affected by the patch, so there's not > > much potential for tuning it down. > > > > There is also the issue of whose tree this should go into. Unfortunately > > the patch can't be broken down into smaller chunks because it changes > > how the devm_request_and_ioremap() function's return value is handled in > > an incompatible way, so it won't be possible to gradually phase this in. > > Furthermore I can imagine that until the end of the release cycle new > > code may be added on which the same transformations need to be done. I > > have a semantic patch to do the bulk of the work, but quite a bit of > > coordination will be required. > > > > I'm adding Arnd and Greg on Cc, maybe they can advise on how best to > > handle this kind of patch. > > You should provide a "wrapper" function that does the correct return > value, convert drivers over to it, and then, when everyone is changed, > drop the old call. To change 156 different drivers all at once, in a > way that is not detectable by the compiler breaking the build, is not a > good thing to do at all. > > By doing it in this manner, it will take longer, but you can push the > patches through the different maintainer's trees. If they are slow, > I'll be glad to take the remaining patches in my driver-core tree to do > the final bits. It certainly sounds like a less complicated way to do it. But it also involves adding a function with a made up name and drop a function with a perfectly good name instead. I wouldn't even know what name to choose for the new API. Thierry [-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation 2013-01-14 22:15 ` Thierry Reding @ 2013-01-14 22:24 ` Arnd Bergmann 2013-01-15 6:44 ` Thierry Reding 2013-01-15 12:32 ` Wolfram Sang 0 siblings, 2 replies; 24+ messages in thread From: Arnd Bergmann @ 2013-01-14 22:24 UTC (permalink / raw) To: Thierry Reding Cc: Greg Kroah-Hartman, Dmitry Torokhov, Laxman Dewangan, grant.likely, rob.herring, swarren, devicetree-discuss, linux-doc, linux-kernel, linux-input, linux-tegra, Wolfram Sang On Monday 14 January 2013, Thierry Reding wrote: > It certainly sounds like a less complicated way to do it. But it also > involves adding a function with a made up name and drop a function with > a perfectly good name instead. I wouldn't even know what name to choose > for the new API. > How about devm_ioremap_resource()? Sounds equally fitting, and is shorter. Arnd ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation 2013-01-14 22:24 ` Arnd Bergmann @ 2013-01-15 6:44 ` Thierry Reding 2013-01-15 12:32 ` Wolfram Sang 1 sibling, 0 replies; 24+ messages in thread From: Thierry Reding @ 2013-01-15 6:44 UTC (permalink / raw) To: Arnd Bergmann Cc: Greg Kroah-Hartman, Dmitry Torokhov, Laxman Dewangan, grant.likely, rob.herring, swarren, devicetree-discuss, linux-doc, linux-kernel, linux-input, linux-tegra, Wolfram Sang [-- Attachment #1: Type: text/plain, Size: 519 bytes --] On Mon, Jan 14, 2013 at 10:24:11PM +0000, Arnd Bergmann wrote: > On Monday 14 January 2013, Thierry Reding wrote: > > It certainly sounds like a less complicated way to do it. But it also > > involves adding a function with a made up name and drop a function with > > a perfectly good name instead. I wouldn't even know what name to choose > > for the new API. > > > > How about devm_ioremap_resource()? Sounds equally fitting, and is shorter. Yes, that sounds good. Thanks for the suggestion. Thierry [-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation 2013-01-14 22:24 ` Arnd Bergmann 2013-01-15 6:44 ` Thierry Reding @ 2013-01-15 12:32 ` Wolfram Sang 1 sibling, 0 replies; 24+ messages in thread From: Wolfram Sang @ 2013-01-15 12:32 UTC (permalink / raw) To: Arnd Bergmann Cc: Thierry Reding, Greg Kroah-Hartman, Dmitry Torokhov, Laxman Dewangan, grant.likely, rob.herring, swarren, devicetree-discuss, linux-doc, linux-kernel, linux-input, linux-tegra [-- Attachment #1: Type: text/plain, Size: 826 bytes --] On Mon, Jan 14, 2013 at 10:24:11PM +0000, Arnd Bergmann wrote: > On Monday 14 January 2013, Thierry Reding wrote: > > It certainly sounds like a less complicated way to do it. But it also > > involves adding a function with a made up name and drop a function with > > a perfectly good name instead. I wouldn't even know what name to choose > > for the new API. > > > > How about devm_ioremap_resource()? Sounds equally fitting, and is shorter. 2 cents, I wrote back then: * the new function is not named 'devm_ioremap_resource' because it does more than just ioremap, namely request_mem_region. I didn't want to create implicit knowledge here. -- Pengutronix e.K. | Wolfram Sang | Industrial Linux Solutions | http://www.pengutronix.de/ | [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 198 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation 2013-01-14 15:49 ` Thierry Reding 2013-01-14 16:16 ` Greg Kroah-Hartman @ 2013-01-15 13:06 ` Wolfram Sang 2013-01-15 15:44 ` Thierry Reding 1 sibling, 1 reply; 24+ messages in thread From: Wolfram Sang @ 2013-01-15 13:06 UTC (permalink / raw) To: Thierry Reding Cc: Dmitry Torokhov, Laxman Dewangan, grant.likely, rob.herring, swarren, devicetree-discuss, linux-doc, linux-kernel, linux-input, linux-tegra, Arnd Bergmann, Greg Kroah-Hartman [-- Attachment #1: Type: text/plain, Size: 3757 bytes --] Hi, > > > > > I am sorry, but I do not consider a function that was added a little > > > > > over a year ago as a canon. If you look at the uses of EADDRNOTAVAIL it > > > > > is used predominantly in networking code to indicate that attempted > > > > > _network_ address is not available. > > > > > > > > EBUSY might be misleading, though. devm_request_and_ioremap() can fail > > > > in both the request_mem_region() and ioremap() calls. Furthermore it'd > > > > be good to settle on a consistent error-code instead of doing it > > > > differently depending on subsystem and/or driver. Currently the various > > > > error codes used are: > > > > > > > > EBUSY, EADDRNOTAVAIL, ENXIO, ENOMEM, ENODEV, ENOENT, EINVAL, > > > > EIO, EFAULT, EADDRINUSE > > > > > > > > Also if we can settle on one error code we should follow up with a patch > > > > to make it consistent across the tree and also update that kerneldoc > > > > comment. I volunteer to do that if nobody else steps up. I'm also Cc'ing > > > > Wolfram (the original author), maybe he has some thoughts on this. Handling the error case was the biggest discussion back then. I initially did not want to use ERR_PTR, because I see already enough patches adding a forgotten ERR_PTR to drivers. My initial idea was to return a simple errno and have the pointer a function argument. I was convinced [1], however, that the dev_err printout is enough to make visible what actually went wrong and return a NULL pointer instead. So much for why the function does NOT return a PTR_ERR, and I still prefer that. Then, I added the example code in the documentation using EADDRNOTAVAIL. Yes, I was brave with this one. Yet, EINVAL, EBUSY, ENOENT, did not really cut it and are so heavily used in drivers that they turned into a generic "something is wrong" error. I tried here to use a not overloaded error code in order to be specific again. Since the patches were accepted, I assumed it wasn't seen as a namespace violation. (Then again, it probably would have been if that error code would go out to userspace) Naturally, I didn't have the resources to check all patches for a consistent error code. > > > If you going to change all drivers make devm_request_and_ioremap() > > > return ERR_PTR()-encoded errors and then we can differentiate what > > > part of it failed. > > > > Yeah, that thought also crossed my mind. I'll give other people some > > time to comment before hurling myself into preparing patches. As said above, that was argued away when committing the patches. But there is more to that: When working with this function, there was also the idea to abstract getting the resource away. Which then gave Sascha Hauer and me the question, if drivers really have to do this or if this couldn't be done by the kernel somehow, i.e. giving the drivers already the resources they need, completely prepared. Of course, then we would need a similar function for interrupt resources. Which has much bigger problem with return codes, since we then step into the area of the "0 is no interrupt" topic (while platform_get_irq returns an error code). As a result, I got the impression that the whole topic needs ONE concentrated, major rehaul or at least a master plan. Adding an idea here and there doesn't seem to cut it, at least not in the way devm_request_and_ioremap() was done. I would be interested in doing that but my resources don't allow me to even think about it at the moment, sadly. Regards, Wolfram [1] http://lkml.org/lkml/2011/10/24/278 -- Pengutronix e.K. | Wolfram Sang | Industrial Linux Solutions | http://www.pengutronix.de/ | [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 198 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation 2013-01-15 13:06 ` Wolfram Sang @ 2013-01-15 15:44 ` Thierry Reding 2013-01-16 6:35 ` Wolfram Sang 0 siblings, 1 reply; 24+ messages in thread From: Thierry Reding @ 2013-01-15 15:44 UTC (permalink / raw) To: Wolfram Sang Cc: Dmitry Torokhov, Laxman Dewangan, grant.likely, rob.herring, swarren, devicetree-discuss, linux-doc, linux-kernel, linux-input, linux-tegra, Arnd Bergmann, Greg Kroah-Hartman [-- Attachment #1: Type: text/plain, Size: 4527 bytes --] On Tue, Jan 15, 2013 at 02:06:23PM +0100, Wolfram Sang wrote: > Hi, > > > > > > > I am sorry, but I do not consider a function that was added a little > > > > > > over a year ago as a canon. If you look at the uses of EADDRNOTAVAIL it > > > > > > is used predominantly in networking code to indicate that attempted > > > > > > _network_ address is not available. > > > > > > > > > > EBUSY might be misleading, though. devm_request_and_ioremap() can fail > > > > > in both the request_mem_region() and ioremap() calls. Furthermore it'd > > > > > be good to settle on a consistent error-code instead of doing it > > > > > differently depending on subsystem and/or driver. Currently the various > > > > > error codes used are: > > > > > > > > > > EBUSY, EADDRNOTAVAIL, ENXIO, ENOMEM, ENODEV, ENOENT, EINVAL, > > > > > EIO, EFAULT, EADDRINUSE > > > > > > > > > > Also if we can settle on one error code we should follow up with a patch > > > > > to make it consistent across the tree and also update that kerneldoc > > > > > comment. I volunteer to do that if nobody else steps up. I'm also Cc'ing > > > > > Wolfram (the original author), maybe he has some thoughts on this. > > Handling the error case was the biggest discussion back then. I > initially did not want to use ERR_PTR, because I see already enough > patches adding a forgotten ERR_PTR to drivers. My initial idea was to > return a simple errno and have the pointer a function argument. I was > convinced [1], however, that the dev_err printout is enough to make > visible what actually went wrong and return a NULL pointer instead. So > much for why the function does NOT return a PTR_ERR, and I still prefer > that. > > Then, I added the example code in the documentation using EADDRNOTAVAIL. > Yes, I was brave with this one. Yet, EINVAL, EBUSY, ENOENT, did not > really cut it and are so heavily used in drivers that they turned into a > generic "something is wrong" error. I tried here to use a not overloaded > error code in order to be specific again. Since the patches were > accepted, I assumed it wasn't seen as a namespace violation. (Then > again, it probably would have been if that error code would go out to > userspace) Naturally, I didn't have the resources to check all patches > for a consistent error code. The problem with the current approach is that people (me included) keep telling people to use this or that error code in an attempt to achieve some kind of consistency. Also using an error message to distinguish between reasons for failure makes it impossible to handle the error other than by visual inspection. Granted, there are currently no code paths that require this. One problem with the original patch was also that it didn't actually convert any existing uses, so there was little chance of anyone noticing potential problems. More than a year later this function is used by many subsystems and a lot of drivers. It just so happened that I observed how many people just didn't know what error codes to choose and often just grabbing one randomly. By adding devm_ioremap_resource() and having it return ERR_PTR()-encoded error codes we get rid of all these problems and put the responsibility for choosing the error code where, in my opinion, it belongs: the failing function. > > > > If you going to change all drivers make devm_request_and_ioremap() > > > > return ERR_PTR()-encoded errors and then we can differentiate what > > > > part of it failed. > > > > > > Yeah, that thought also crossed my mind. I'll give other people some > > > time to comment before hurling myself into preparing patches. > > As said above, that was argued away when committing the patches. > > But there is more to that: > > When working with this function, there was also the idea to abstract > getting the resource away. Which then gave Sascha Hauer and me the > question, if drivers really have to do this or if this couldn't be done > by the kernel somehow, i.e. giving the drivers already the resources > they need, completely prepared. I'm not sure I like that very much. That could possibly lead to a new problem where drivers that need to do something special have to jump through hoops to achieve something that may otherwise be simple. Anyway, if people don't think this is a sensible conversion I should waste no more time on it. On the other hand I have the patch series ready so I might as well post it for broader review. Thierry [-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation 2013-01-15 15:44 ` Thierry Reding @ 2013-01-16 6:35 ` Wolfram Sang 0 siblings, 0 replies; 24+ messages in thread From: Wolfram Sang @ 2013-01-16 6:35 UTC (permalink / raw) To: Thierry Reding Cc: Dmitry Torokhov, Laxman Dewangan, grant.likely, rob.herring, swarren, devicetree-discuss, linux-doc, linux-kernel, linux-input, linux-tegra, Arnd Bergmann, Greg Kroah-Hartman [-- Attachment #1: Type: text/plain, Size: 3966 bytes --] Hi, > > Then, I added the example code in the documentation using EADDRNOTAVAIL. > > Yes, I was brave with this one. Yet, EINVAL, EBUSY, ENOENT, did not > > really cut it and are so heavily used in drivers that they turned into a > > generic "something is wrong" error. I tried here to use a not overloaded > > error code in order to be specific again. Since the patches were > > accepted, I assumed it wasn't seen as a namespace violation. (Then > > again, it probably would have been if that error code would go out to > > userspace) Naturally, I didn't have the resources to check all patches > > for a consistent error code. > > The problem with the current approach is that people (me included) keep > telling people to use this or that error code in an attempt to achieve > some kind of consistency. Also using an error message to distinguish > between reasons for failure makes it impossible to handle the error > other than by visual inspection. Granted, there are currently no code > paths that require this. Yes, so currently, this is rather a cosmetic change. And for that, it is quite intrusive. I think something like this is needed somewhen as part of a bigger overhaul. That's what I called "master plan" last time. That could be that resource handling gets aligned in general, also taking e.g. interrupt resources into account. But only changing error code handling for this function, doesn't seem too useful to me and might even need another change later, then. > One problem with the original patch was also that it didn't actually > convert any existing uses, so there was little chance of anyone noticing Like with the error codes now, there are so many different ways of handling resources that I did not want to mass convert all the drivers without being able to test them. I was hoping for a migration over time with patches from people who really tested what they did. > potential problems. More than a year later this function is used by many > subsystems and a lot of drivers. It just so happened that I observed how > many people just didn't know what error codes to choose and often just > grabbing one randomly. Yes, and concerning resources this needs cleaning on a bigger scale IMO. > By adding devm_ioremap_resource() and having it return ERR_PTR()-encoded > error codes we get rid of all these problems and put the responsibility > for choosing the error code where, in my opinion, it belongs: the > failing function. For completeness, it leaves the problem that people might forget to use ERR_PTR (seen that often in the clk framework). And the change is huge while I can't see any real benefit right now. > > When working with this function, there was also the idea to abstract > > getting the resource away. Which then gave Sascha Hauer and me the > > question, if drivers really have to do this or if this couldn't be done > > by the kernel somehow, i.e. giving the drivers already the resources > > they need, completely prepared. > > I'm not sure I like that very much. That could possibly lead to a new > problem where drivers that need to do something special have to jump > through hoops to achieve something that may otherwise be simple. I assume that drivers are already doing something special :) So, that would turn up in the conversion process. I can't promise that it would really work, it would need some playing around. > Anyway, if people don't think this is a sensible conversion I should > waste no more time on it. On the other hand I have the patch series > ready so I might as well post it for broader review. Working on patches is hardly a waste. Even if not accepted, you gain understanding. Please put me on CC, if you post the patches. Regards, Wolfram -- Pengutronix e.K. | Wolfram Sang | Industrial Linux Solutions | http://www.pengutronix.de/ | [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 198 bytes --] ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation 2013-01-09 9:19 ` Dmitry Torokhov 2013-01-09 9:23 ` Thierry Reding @ 2013-02-09 9:04 ` Grant Likely 1 sibling, 0 replies; 24+ messages in thread From: Grant Likely @ 2013-02-09 9:04 UTC (permalink / raw) To: Dmitry Torokhov, Thierry Reding Cc: Laxman Dewangan, rob.herring, swarren, devicetree-discuss, linux-doc, linux-kernel, linux-input, linux-tegra, Wolfram Sang On Wed, 9 Jan 2013 01:19:39 -0800, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > On Wed, Jan 09, 2013 at 08:07:45AM +0100, Thierry Reding wrote: > > On Sun, Jan 06, 2013 at 11:57:48AM -0800, Dmitry Torokhov wrote: > > > On Sun, Jan 06, 2013 at 08:27:39PM +0100, Thierry Reding wrote: > > > > On Sat, Jan 05, 2013 at 12:06:58AM -0800, Dmitry Torokhov wrote: > > > > > On Sat, Jan 05, 2013 at 01:15:08PM +0530, Laxman Dewangan wrote: > > > > [...] > > > > > > @@ -735,25 +738,16 @@ static int tegra_kbc_probe(struct platform_device *pdev) > > > > > > spin_lock_init(&kbc->lock); > > > > > > setup_timer(&kbc->timer, tegra_kbc_keypress_timer, (unsigned long)kbc); > > > > > > > > > > > > - res = request_mem_region(res->start, resource_size(res), pdev->name); > > > > > > - if (!res) { > > > > > > - dev_err(&pdev->dev, "failed to request I/O memory\n"); > > > > > > - err = -EBUSY; > > > > > > - goto err_free_mem; > > > > > > - } > > > > > > - > > > > > > - kbc->mmio = ioremap(res->start, resource_size(res)); > > > > > > + kbc->mmio = devm_request_and_ioremap(&pdev->dev, res); > > > > > > if (!kbc->mmio) { > > > > > > - dev_err(&pdev->dev, "failed to remap I/O memory\n"); > > > > > > - err = -ENXIO; > > > > > > - goto err_free_mem_region; > > > > > > + dev_err(&pdev->dev, "Cannot request memregion/iomap address\n"); > > > > > > + return -EADDRNOTAVAIL; > > > > > > > > > > Erm, no, -EBUSY please. > > > > > > > > EADDRNOTAVAIL is the canonical error for devm_request_and_ioremap() > > > > failure. The kerneldoc comment in lib/devres.c even gives a short > > > > example that uses this error code. > > > > > > I am sorry, but I do not consider a function that was added a little > > > over a year ago as a canon. If you look at the uses of EADDRNOTAVAIL it > > > is used predominantly in networking code to indicate that attempted > > > _network_ address is not available. > > > > EBUSY might be misleading, though. devm_request_and_ioremap() can fail > > in both the request_mem_region() and ioremap() calls. Furthermore it'd > > be good to settle on a consistent error-code instead of doing it > > differently depending on subsystem and/or driver. Currently the various > > error codes used are: > > > > EBUSY, EADDRNOTAVAIL, ENXIO, ENOMEM, ENODEV, ENOENT, EINVAL, > > EIO, EFAULT, EADDRINUSE > > > > Also if we can settle on one error code we should follow up with a patch > > to make it consistent across the tree and also update that kerneldoc > > comment. I volunteer to do that if nobody else steps up. I'm also Cc'ing > > Wolfram (the original author), maybe he has some thoughts on this. This really doesn't matter. As far as the driver is concerned if the memory isn't available then it is a failure. Period. Who cares what the reason was. > If you going to change all drivers make devm_request_and_ioremap() > return ERR_PTR()-encoded errors and then we can differentiate what > part of it failed. The ERR_PTR() pattern is actually worse when it comes to reading and understanding code. Us C coders have had beaten into us that the correct test for an invalid pointer is "if (!ptr)". ERR_PTR() is actively dangerous in this regard because it breaks that pattern, but you cannot tell from looking at the API that it is wrong. There are places where it makes sense, but *please* don't use the ERR_PTR pattern unless absolutely necessary. Rarely are the actual error codes important for kernel internal stuff. The error codes only really matter when passing them up to userspace where they have specific meanings. As far as in-kernel stuff goes, the policy for choosing error codes consists of "go look at the errno file and choose one that looks vaguely relevant". g. ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v2] input: keyboard: tegra: add support for rows/cols configuration from dt 2013-01-05 7:45 [PATCH V2 0/4] input: keyboard: tegra: cleanups and DT supports Laxman Dewangan 2013-01-05 7:45 ` [PATCH v2 1/4] input: keyboard: tegra: fix build warning Laxman Dewangan 2013-01-05 7:45 ` [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation Laxman Dewangan @ 2013-01-05 7:45 ` Laxman Dewangan 2013-01-05 7:45 ` [PATCH v2 4/4] input: keyboard: tegra: remove default key mapping Laxman Dewangan 3 siblings, 0 replies; 24+ messages in thread From: Laxman Dewangan @ 2013-01-05 7:45 UTC (permalink / raw) To: dmitry.torokhov Cc: grant.likely, rob.herring, swarren, devicetree-discuss, linux-doc, linux-kernel, linux-input, linux-tegra, Laxman Dewangan The NVIDIA's Tegra KBC has maximum 24 pins to make matrix keypad. Any pin can be configured as row or column. The maximum column pin can be 8 and maximum row pin can be 16. Remove the assumption that all first 16 pins will be used as row and remaining as columns and Add the property for configuring pins to either row or column from DT. Update the devicetree binding document accordingly. Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com> --- Changes from V1: - renames kbc-rows and kbc-cols to kbc-row-pins and kbc-col-pins. - cleanusp. .../bindings/input/nvidia,tegra20-kbc.txt | 22 ++++++ drivers/input/keyboard/tegra-kbc.c | 74 +++++++++++++++----- 2 files changed, 79 insertions(+), 17 deletions(-) diff --git a/Documentation/devicetree/bindings/input/nvidia,tegra20-kbc.txt b/Documentation/devicetree/bindings/input/nvidia,tegra20-kbc.txt index 72683be..2995fae 100644 --- a/Documentation/devicetree/bindings/input/nvidia,tegra20-kbc.txt +++ b/Documentation/devicetree/bindings/input/nvidia,tegra20-kbc.txt @@ -1,7 +1,18 @@ * Tegra keyboard controller +The key controller has maximum 24 pins to make matrix keypad. Any pin +can be configured as row or column. The maximum column pin can be 8 +and maximum row pins can be 16 for Tegra20/Tegra30. Required properties: - compatible: "nvidia,tegra20-kbc" +- reg: Register base address of KBC. +- interrupts: Interrupt number for the KBC. +- nvidia,kbc-row-pins: The KBC pins which are configured as row. This is an + array of pin numbers which is used as rows. +- nvidia,kbc-col-pins: The KBC pins which are configured as column. This is an + array of pin numbers which is used as column. +- linux,keymap: The keymap for keys as described in the binding document + devicetree/bindings/input/matrix-keymap.txt. Optional properties, in addition to those specified by the shared matrix-keyboard bindings: @@ -19,5 +30,16 @@ Example: keyboard: keyboard { compatible = "nvidia,tegra20-kbc"; reg = <0x7000e200 0x100>; + interrupts = <0 85 0x04>; nvidia,ghost-filter; + nvidia,debounce-delay-ms = <640>; + nvidia,kbc-row-pins = <0 1 2>; /* pin 0, 1, 2 as rows */ + nvidia,kbc-col-pins = <11 12 13>; /* pin 11, 12, 13 as columns */ + linux,keymap = <0x00000074 + 0x00010067 + 0x00020066 + 0x01010068 + 0x02000069 + 0x02010070 + 0x02020071>; }; diff --git a/drivers/input/keyboard/tegra-kbc.c b/drivers/input/keyboard/tegra-kbc.c index c036425..b65971d 100644 --- a/drivers/input/keyboard/tegra-kbc.c +++ b/drivers/input/keyboard/tegra-kbc.c @@ -614,13 +614,16 @@ static struct tegra_kbc_platform_data *tegra_kbc_dt_parse_pdata( struct device_node *np = pdev->dev.of_node; u32 prop; int i; - - if (!np) - return NULL; + u32 num_rows = 0; + u32 num_cols = 0; + u32 cols_cfg[KBC_MAX_GPIO]; + u32 rows_cfg[KBC_MAX_GPIO]; + int proplen; + int ret; pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL); if (!pdata) - return NULL; + return ERR_PTR(-ENOMEM); if (!of_property_read_u32(np, "nvidia,debounce-delay-ms", &prop)) pdata->debounce_cnt = prop; @@ -634,18 +637,55 @@ static struct tegra_kbc_platform_data *tegra_kbc_dt_parse_pdata( if (of_find_property(np, "nvidia,wakeup-source", NULL)) pdata->wakeup = true; - /* - * All currently known keymaps with device tree support use the same - * pin_cfg, so set it up here. - */ - for (i = 0; i < KBC_MAX_ROW; i++) { - pdata->pin_cfg[i].num = i; - pdata->pin_cfg[i].type = PIN_CFG_ROW; + if (!of_get_property(np, "nvidia,kbc-row-pins", &proplen)) { + dev_err(&pdev->dev, "property nvidia,kbc-row-pins not found\n"); + return ERR_PTR(-ENOENT); + } + num_rows = proplen / sizeof(u32); + + if (!of_get_property(np, "nvidia,kbc-col-pins", &proplen)) { + dev_err(&pdev->dev, "property nvidia,kbc-col-pins not found\n"); + return ERR_PTR(-ENOENT); + } + num_cols = proplen / sizeof(u32); + + if (!of_get_property(np, "linux,keymap", &proplen)) { + dev_err(&pdev->dev, "property linux,keymap not found\n"); + return ERR_PTR(-ENOENT); + } + + if (!num_rows || !num_cols || ((num_rows + num_cols) > KBC_MAX_GPIO)) { + dev_err(&pdev->dev, + "keypad rows/columns not porperly specified\n"); + return ERR_PTR(-EINVAL); } - for (i = 0; i < KBC_MAX_COL; i++) { - pdata->pin_cfg[KBC_MAX_ROW + i].num = i; - pdata->pin_cfg[KBC_MAX_ROW + i].type = PIN_CFG_COL; + /* Set all pins as non-configured */ + for (i = 0; i < KBC_MAX_GPIO; i++) + pdata->pin_cfg[i].type = PIN_CFG_IGNORE; + + ret = of_property_read_u32_array(np, "nvidia,kbc-row-pins", + rows_cfg, num_rows); + if (ret < 0) { + dev_err(&pdev->dev, "Rows configurations are not proper\n"); + return ERR_PTR(-ENOENT); + } + + ret = of_property_read_u32_array(np, "nvidia,kbc-col-pins", + cols_cfg, num_cols); + if (ret < 0) { + dev_err(&pdev->dev, "Cols configurations are not proper\n"); + return ERR_PTR(-ENOENT); + } + + for (i = 0; i < num_rows; i++) { + pdata->pin_cfg[rows_cfg[i]].type = PIN_CFG_ROW; + pdata->pin_cfg[rows_cfg[i]].num = i; + } + + for (i = 0; i < num_cols; i++) { + pdata->pin_cfg[cols_cfg[i]].type = PIN_CFG_COL; + pdata->pin_cfg[cols_cfg[i]].num = i; } return pdata; @@ -697,12 +737,12 @@ static int tegra_kbc_probe(struct platform_device *pdev) unsigned int debounce_cnt; unsigned int scan_time_rows; - if (!pdata) + if (!pdata && pdev->dev.of_node) pdata = tegra_kbc_dt_parse_pdata(pdev); - if (!pdata) { + if (IS_ERR_OR_NULL(pdata)) { dev_err(&pdev->dev, "Platform data missing\n"); - return -EINVAL; + return pdata ? PTR_ERR(pdata) : -EINVAL; } if (!tegra_kbc_check_pin_cfg(pdata, &pdev->dev, &num_rows)) -- 1.7.1.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH v2 4/4] input: keyboard: tegra: remove default key mapping 2013-01-05 7:45 [PATCH V2 0/4] input: keyboard: tegra: cleanups and DT supports Laxman Dewangan ` (2 preceding siblings ...) 2013-01-05 7:45 ` [PATCH v2] input: keyboard: tegra: add support for rows/cols configuration from dt Laxman Dewangan @ 2013-01-05 7:45 ` Laxman Dewangan 3 siblings, 0 replies; 24+ messages in thread From: Laxman Dewangan @ 2013-01-05 7:45 UTC (permalink / raw) To: dmitry.torokhov Cc: grant.likely, rob.herring, swarren, devicetree-discuss, linux-doc, linux-kernel, linux-input, linux-tegra, Laxman Dewangan Tegra KBC driver have the default key mapping for 16x8 configuration. The key mapping can be provided through platform data or through DT and the mapping varies from platform to platform, hence this default mapping is not so useful. Remove the default mapping to reduce the code lines of the driver. Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com> --- Changes from V1: - None drivers/input/keyboard/tegra-kbc.c | 156 +----------------------------------- 1 files changed, 1 insertions(+), 155 deletions(-) diff --git a/drivers/input/keyboard/tegra-kbc.c b/drivers/input/keyboard/tegra-kbc.c index ef7a0ac..c6e4985 100644 --- a/drivers/input/keyboard/tegra-kbc.c +++ b/drivers/input/keyboard/tegra-kbc.c @@ -87,147 +87,6 @@ struct tegra_kbc { struct clk *clk; }; -static const u32 tegra_kbc_default_keymap[] = { - KEY(0, 2, KEY_W), - KEY(0, 3, KEY_S), - KEY(0, 4, KEY_A), - KEY(0, 5, KEY_Z), - KEY(0, 7, KEY_FN), - - KEY(1, 7, KEY_LEFTMETA), - - KEY(2, 6, KEY_RIGHTALT), - KEY(2, 7, KEY_LEFTALT), - - KEY(3, 0, KEY_5), - KEY(3, 1, KEY_4), - KEY(3, 2, KEY_R), - KEY(3, 3, KEY_E), - KEY(3, 4, KEY_F), - KEY(3, 5, KEY_D), - KEY(3, 6, KEY_X), - - KEY(4, 0, KEY_7), - KEY(4, 1, KEY_6), - KEY(4, 2, KEY_T), - KEY(4, 3, KEY_H), - KEY(4, 4, KEY_G), - KEY(4, 5, KEY_V), - KEY(4, 6, KEY_C), - KEY(4, 7, KEY_SPACE), - - KEY(5, 0, KEY_9), - KEY(5, 1, KEY_8), - KEY(5, 2, KEY_U), - KEY(5, 3, KEY_Y), - KEY(5, 4, KEY_J), - KEY(5, 5, KEY_N), - KEY(5, 6, KEY_B), - KEY(5, 7, KEY_BACKSLASH), - - KEY(6, 0, KEY_MINUS), - KEY(6, 1, KEY_0), - KEY(6, 2, KEY_O), - KEY(6, 3, KEY_I), - KEY(6, 4, KEY_L), - KEY(6, 5, KEY_K), - KEY(6, 6, KEY_COMMA), - KEY(6, 7, KEY_M), - - KEY(7, 1, KEY_EQUAL), - KEY(7, 2, KEY_RIGHTBRACE), - KEY(7, 3, KEY_ENTER), - KEY(7, 7, KEY_MENU), - - KEY(8, 4, KEY_RIGHTSHIFT), - KEY(8, 5, KEY_LEFTSHIFT), - - KEY(9, 5, KEY_RIGHTCTRL), - KEY(9, 7, KEY_LEFTCTRL), - - KEY(11, 0, KEY_LEFTBRACE), - KEY(11, 1, KEY_P), - KEY(11, 2, KEY_APOSTROPHE), - KEY(11, 3, KEY_SEMICOLON), - KEY(11, 4, KEY_SLASH), - KEY(11, 5, KEY_DOT), - - KEY(12, 0, KEY_F10), - KEY(12, 1, KEY_F9), - KEY(12, 2, KEY_BACKSPACE), - KEY(12, 3, KEY_3), - KEY(12, 4, KEY_2), - KEY(12, 5, KEY_UP), - KEY(12, 6, KEY_PRINT), - KEY(12, 7, KEY_PAUSE), - - KEY(13, 0, KEY_INSERT), - KEY(13, 1, KEY_DELETE), - KEY(13, 3, KEY_PAGEUP), - KEY(13, 4, KEY_PAGEDOWN), - KEY(13, 5, KEY_RIGHT), - KEY(13, 6, KEY_DOWN), - KEY(13, 7, KEY_LEFT), - - KEY(14, 0, KEY_F11), - KEY(14, 1, KEY_F12), - KEY(14, 2, KEY_F8), - KEY(14, 3, KEY_Q), - KEY(14, 4, KEY_F4), - KEY(14, 5, KEY_F3), - KEY(14, 6, KEY_1), - KEY(14, 7, KEY_F7), - - KEY(15, 0, KEY_ESC), - KEY(15, 1, KEY_GRAVE), - KEY(15, 2, KEY_F5), - KEY(15, 3, KEY_TAB), - KEY(15, 4, KEY_F1), - KEY(15, 5, KEY_F2), - KEY(15, 6, KEY_CAPSLOCK), - KEY(15, 7, KEY_F6), - - /* Software Handled Function Keys */ - KEY(20, 0, KEY_KP7), - - KEY(21, 0, KEY_KP9), - KEY(21, 1, KEY_KP8), - KEY(21, 2, KEY_KP4), - KEY(21, 4, KEY_KP1), - - KEY(22, 1, KEY_KPSLASH), - KEY(22, 2, KEY_KP6), - KEY(22, 3, KEY_KP5), - KEY(22, 4, KEY_KP3), - KEY(22, 5, KEY_KP2), - KEY(22, 7, KEY_KP0), - - KEY(27, 1, KEY_KPASTERISK), - KEY(27, 3, KEY_KPMINUS), - KEY(27, 4, KEY_KPPLUS), - KEY(27, 5, KEY_KPDOT), - - KEY(28, 5, KEY_VOLUMEUP), - - KEY(29, 3, KEY_HOME), - KEY(29, 4, KEY_END), - KEY(29, 5, KEY_BRIGHTNESSDOWN), - KEY(29, 6, KEY_VOLUMEDOWN), - KEY(29, 7, KEY_BRIGHTNESSUP), - - KEY(30, 0, KEY_NUMLOCK), - KEY(30, 1, KEY_SCROLLLOCK), - KEY(30, 2, KEY_MUTE), - - KEY(31, 4, KEY_HELP), -}; - -static const -struct matrix_keymap_data tegra_kbc_default_keymap_data = { - .keymap = tegra_kbc_default_keymap, - .keymap_size = ARRAY_SIZE(tegra_kbc_default_keymap), -}; - static void tegra_kbc_report_released_keys(struct input_dev *input, unsigned short old_keycodes[], unsigned int old_num_keys, @@ -701,26 +560,13 @@ static int tegra_kbd_setup_keymap(struct tegra_kbc *kbc) const struct tegra_kbc_platform_data *pdata = kbc->pdata; const struct matrix_keymap_data *keymap_data = pdata->keymap_data; unsigned int keymap_rows = KBC_MAX_KEY; - int retval; if (keymap_data && pdata->use_fn_map) keymap_rows *= 2; - retval = matrix_keypad_build_keymap(keymap_data, NULL, + return matrix_keypad_build_keymap(keymap_data, NULL, keymap_rows, KBC_MAX_COL, kbc->keycode, kbc->idev); - if (retval == -ENOSYS || retval == -ENOENT) { - /* - * If there is no OF support in kernel or keymap - * property is missing, use default keymap. - */ - retval = matrix_keypad_build_keymap( - &tegra_kbc_default_keymap_data, NULL, - keymap_rows, KBC_MAX_COL, - kbc->keycode, kbc->idev); - } - - return retval; } static int tegra_kbc_probe(struct platform_device *pdev) -- 1.7.1.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
end of thread, other threads:[~2013-02-09 10:10 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-01-05 7:45 [PATCH V2 0/4] input: keyboard: tegra: cleanups and DT supports Laxman Dewangan 2013-01-05 7:45 ` [PATCH v2 1/4] input: keyboard: tegra: fix build warning Laxman Dewangan 2013-01-05 7:45 ` [PATCH v2 2/4] input: keyboard: tegra: use devm_* for resource allocation Laxman Dewangan 2013-01-05 8:06 ` Dmitry Torokhov 2013-01-05 11:20 ` Laxman Dewangan 2013-01-05 23:18 ` Dmitry Torokhov 2013-01-06 11:00 ` Laxman Dewangan 2013-01-06 19:27 ` Thierry Reding 2013-01-06 19:57 ` Dmitry Torokhov 2013-01-09 7:07 ` Thierry Reding 2013-01-09 9:19 ` Dmitry Torokhov 2013-01-09 9:23 ` Thierry Reding 2013-01-14 15:49 ` Thierry Reding 2013-01-14 16:16 ` Greg Kroah-Hartman 2013-01-14 22:15 ` Thierry Reding 2013-01-14 22:24 ` Arnd Bergmann 2013-01-15 6:44 ` Thierry Reding 2013-01-15 12:32 ` Wolfram Sang 2013-01-15 13:06 ` Wolfram Sang 2013-01-15 15:44 ` Thierry Reding 2013-01-16 6:35 ` Wolfram Sang 2013-02-09 9:04 ` Grant Likely 2013-01-05 7:45 ` [PATCH v2] input: keyboard: tegra: add support for rows/cols configuration from dt Laxman Dewangan 2013-01-05 7:45 ` [PATCH v2 4/4] input: keyboard: tegra: remove default key mapping Laxman Dewangan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).