* [PATCH 1/4] Input: ep93xx_keypad - annotate suspend/resume as __maybe_unused @ 2021-10-12 1:37 Dmitry Torokhov 2021-10-12 1:37 ` [PATCH 2/4] Input: ep93xx_keypad - use BIT() and GENMASK() macros Dmitry Torokhov ` (3 more replies) 0 siblings, 4 replies; 11+ messages in thread From: Dmitry Torokhov @ 2021-10-12 1:37 UTC (permalink / raw) To: linux-input; +Cc: Alexander Sverdlin, linux-kernel Instead of guarding suspend/resume methods with #ifdef CONFIG_PM let's mark them as __maybe_unused as this allows better compile coverage. Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/input/keyboard/ep93xx_keypad.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/input/keyboard/ep93xx_keypad.c b/drivers/input/keyboard/ep93xx_keypad.c index e0e931e796fa..a0c6cdf8e0d3 100644 --- a/drivers/input/keyboard/ep93xx_keypad.c +++ b/drivers/input/keyboard/ep93xx_keypad.c @@ -175,8 +175,7 @@ static void ep93xx_keypad_close(struct input_dev *pdev) } -#ifdef CONFIG_PM_SLEEP -static int ep93xx_keypad_suspend(struct device *dev) +static int __maybe_unused ep93xx_keypad_suspend(struct device *dev) { struct platform_device *pdev = to_platform_device(dev); struct ep93xx_keypad *keypad = platform_get_drvdata(pdev); @@ -197,7 +196,7 @@ static int ep93xx_keypad_suspend(struct device *dev) return 0; } -static int ep93xx_keypad_resume(struct device *dev) +static int __maybe_unused ep93xx_keypad_resume(struct device *dev) { struct platform_device *pdev = to_platform_device(dev); struct ep93xx_keypad *keypad = platform_get_drvdata(pdev); @@ -220,7 +219,6 @@ static int ep93xx_keypad_resume(struct device *dev) return 0; } -#endif static SIMPLE_DEV_PM_OPS(ep93xx_keypad_pm_ops, ep93xx_keypad_suspend, ep93xx_keypad_resume); -- 2.33.0.882.g93a45727a2-goog ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/4] Input: ep93xx_keypad - use BIT() and GENMASK() macros 2021-10-12 1:37 [PATCH 1/4] Input: ep93xx_keypad - annotate suspend/resume as __maybe_unused Dmitry Torokhov @ 2021-10-12 1:37 ` Dmitry Torokhov 2021-10-12 6:58 ` Alexander Sverdlin 2021-10-12 1:37 ` [PATCH 3/4] Input: ep93xx_keypad - use dev_pm_set_wake_irq() Dmitry Torokhov ` (2 subsequent siblings) 3 siblings, 1 reply; 11+ messages in thread From: Dmitry Torokhov @ 2021-10-12 1:37 UTC (permalink / raw) To: linux-input; +Cc: Alexander Sverdlin, linux-kernel Also drop parenthesis around macros that do not use expressions as they are not needed. Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/input/keyboard/ep93xx_keypad.c | 37 +++++++++++++------------- 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/drivers/input/keyboard/ep93xx_keypad.c b/drivers/input/keyboard/ep93xx_keypad.c index a0c6cdf8e0d3..6be5474ba2f2 100644 --- a/drivers/input/keyboard/ep93xx_keypad.c +++ b/drivers/input/keyboard/ep93xx_keypad.c @@ -17,6 +17,7 @@ * flag. */ +#include <linux/bits.h> #include <linux/module.h> #include <linux/platform_device.h> #include <linux/interrupt.h> @@ -35,28 +36,28 @@ #define KEY_REG 0x08 /* Key Value Capture register */ /* Key Scan Initialization Register bit defines */ -#define KEY_INIT_DBNC_MASK (0x00ff0000) -#define KEY_INIT_DBNC_SHIFT (16) -#define KEY_INIT_DIS3KY (1<<15) -#define KEY_INIT_DIAG (1<<14) -#define KEY_INIT_BACK (1<<13) -#define KEY_INIT_T2 (1<<12) -#define KEY_INIT_PRSCL_MASK (0x000003ff) -#define KEY_INIT_PRSCL_SHIFT (0) +#define KEY_INIT_DBNC_MASK GENMASK(23, 16) +#define KEY_INIT_DBNC_SHIFT 16 +#define KEY_INIT_DIS3KY BIT(15) +#define KEY_INIT_DIAG BIT(14) +#define KEY_INIT_BACK BIT(13) +#define KEY_INIT_T2 BIT(12) +#define KEY_INIT_PRSCL_MASK GENMASK(9, 0) +#define KEY_INIT_PRSCL_SHIFT 0 /* Key Scan Diagnostic Register bit defines */ -#define KEY_DIAG_MASK (0x0000003f) -#define KEY_DIAG_SHIFT (0) +#define KEY_DIAG_MASK GENMASK(5, 0) +#define KEY_DIAG_SHIFT 0 /* Key Value Capture Register bit defines */ -#define KEY_REG_K (1<<15) -#define KEY_REG_INT (1<<14) -#define KEY_REG_2KEYS (1<<13) -#define KEY_REG_1KEY (1<<12) -#define KEY_REG_KEY2_MASK (0x00000fc0) -#define KEY_REG_KEY2_SHIFT (6) -#define KEY_REG_KEY1_MASK (0x0000003f) -#define KEY_REG_KEY1_SHIFT (0) +#define KEY_REG_K BIT(15) +#define KEY_REG_INT BIT(14) +#define KEY_REG_2KEYS BIT(13) +#define KEY_REG_1KEY BIT(12) +#define KEY_REG_KEY2_MASK GENMASK(11, 6) +#define KEY_REG_KEY2_SHIFT 6 +#define KEY_REG_KEY1_MASK GENMASK(5, 0) +#define KEY_REG_KEY1_SHIFT 0 #define EP93XX_MATRIX_SIZE (EP93XX_MATRIX_ROWS * EP93XX_MATRIX_COLS) -- 2.33.0.882.g93a45727a2-goog ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/4] Input: ep93xx_keypad - use BIT() and GENMASK() macros 2021-10-12 1:37 ` [PATCH 2/4] Input: ep93xx_keypad - use BIT() and GENMASK() macros Dmitry Torokhov @ 2021-10-12 6:58 ` Alexander Sverdlin 0 siblings, 0 replies; 11+ messages in thread From: Alexander Sverdlin @ 2021-10-12 6:58 UTC (permalink / raw) To: Dmitry Torokhov, linux-input; +Cc: linux-kernel Hi! On Mon, 2021-10-11 at 18:37 -0700, Dmitry Torokhov wrote: > Also drop parenthesis around macros that do not use expressions as they are > not needed. > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> Acked-by: Alexander Sverdlin <alexander.sverdlin@gmail.com> > --- > drivers/input/keyboard/ep93xx_keypad.c | 37 +++++++++++++------------- > 1 file changed, 19 insertions(+), 18 deletions(-) > > diff --git a/drivers/input/keyboard/ep93xx_keypad.c b/drivers/input/keyboard/ep93xx_keypad.c > index a0c6cdf8e0d3..6be5474ba2f2 100644 > --- a/drivers/input/keyboard/ep93xx_keypad.c > +++ b/drivers/input/keyboard/ep93xx_keypad.c > @@ -17,6 +17,7 @@ > * flag. > */ > > +#include <linux/bits.h> > #include <linux/module.h> > #include <linux/platform_device.h> > #include <linux/interrupt.h> > @@ -35,28 +36,28 @@ > #define KEY_REG 0x08 /* Key Value Capture register */ > > /* Key Scan Initialization Register bit defines */ > -#define KEY_INIT_DBNC_MASK (0x00ff0000) > -#define KEY_INIT_DBNC_SHIFT (16) > -#define KEY_INIT_DIS3KY (1<<15) > -#define KEY_INIT_DIAG (1<<14) > -#define KEY_INIT_BACK (1<<13) > -#define KEY_INIT_T2 (1<<12) > -#define KEY_INIT_PRSCL_MASK (0x000003ff) > -#define KEY_INIT_PRSCL_SHIFT (0) > +#define KEY_INIT_DBNC_MASK GENMASK(23, 16) > +#define KEY_INIT_DBNC_SHIFT 16 > +#define KEY_INIT_DIS3KY BIT(15) > +#define KEY_INIT_DIAG BIT(14) > +#define KEY_INIT_BACK BIT(13) > +#define KEY_INIT_T2 BIT(12) > +#define KEY_INIT_PRSCL_MASK GENMASK(9, 0) > +#define KEY_INIT_PRSCL_SHIFT 0 > > /* Key Scan Diagnostic Register bit defines */ > -#define KEY_DIAG_MASK (0x0000003f) > -#define KEY_DIAG_SHIFT (0) > +#define KEY_DIAG_MASK GENMASK(5, 0) > +#define KEY_DIAG_SHIFT 0 > > /* Key Value Capture Register bit defines */ > -#define KEY_REG_K (1<<15) > -#define KEY_REG_INT (1<<14) > -#define KEY_REG_2KEYS (1<<13) > -#define KEY_REG_1KEY (1<<12) > -#define KEY_REG_KEY2_MASK (0x00000fc0) > -#define KEY_REG_KEY2_SHIFT (6) > -#define KEY_REG_KEY1_MASK (0x0000003f) > -#define KEY_REG_KEY1_SHIFT (0) > +#define KEY_REG_K BIT(15) > +#define KEY_REG_INT BIT(14) > +#define KEY_REG_2KEYS BIT(13) > +#define KEY_REG_1KEY BIT(12) > +#define KEY_REG_KEY2_MASK GENMASK(11, 6) > +#define KEY_REG_KEY2_SHIFT 6 > +#define KEY_REG_KEY1_MASK GENMASK(5, 0) > +#define KEY_REG_KEY1_SHIFT 0 > > #define EP93XX_MATRIX_SIZE (EP93XX_MATRIX_ROWS * EP93XX_MATRIX_COLS) > -- Alexander Sverdlin. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/4] Input: ep93xx_keypad - use dev_pm_set_wake_irq() 2021-10-12 1:37 [PATCH 1/4] Input: ep93xx_keypad - annotate suspend/resume as __maybe_unused Dmitry Torokhov 2021-10-12 1:37 ` [PATCH 2/4] Input: ep93xx_keypad - use BIT() and GENMASK() macros Dmitry Torokhov @ 2021-10-12 1:37 ` Dmitry Torokhov 2021-10-12 7:30 ` Alexander Sverdlin 2021-10-12 1:37 ` [PATCH 4/4] Input: ep93xx_keypad - switch to using managed resources Dmitry Torokhov 2021-10-12 6:31 ` [PATCH 1/4] Input: ep93xx_keypad - annotate suspend/resume as __maybe_unused Alexander Sverdlin 3 siblings, 1 reply; 11+ messages in thread From: Dmitry Torokhov @ 2021-10-12 1:37 UTC (permalink / raw) To: linux-input; +Cc: Alexander Sverdlin, linux-kernel Instead of manually toggling interrupt as wakeup source in suspend/resume methods, let's declare keypad interrupt and wakeup interrupt and leave the rest to the PM core. Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/input/keyboard/ep93xx_keypad.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/drivers/input/keyboard/ep93xx_keypad.c b/drivers/input/keyboard/ep93xx_keypad.c index 6be5474ba2f2..a66cfeaf5b21 100644 --- a/drivers/input/keyboard/ep93xx_keypad.c +++ b/drivers/input/keyboard/ep93xx_keypad.c @@ -27,6 +27,7 @@ #include <linux/slab.h> #include <linux/soc/cirrus/ep93xx.h> #include <linux/platform_data/keypad-ep93xx.h> +#include <linux/pm_wakeirq.h> /* * Keypad Interface Register offsets @@ -191,9 +192,6 @@ static int __maybe_unused ep93xx_keypad_suspend(struct device *dev) mutex_unlock(&input_dev->mutex); - if (device_may_wakeup(&pdev->dev)) - enable_irq_wake(keypad->irq); - return 0; } @@ -203,9 +201,6 @@ static int __maybe_unused ep93xx_keypad_resume(struct device *dev) struct ep93xx_keypad *keypad = platform_get_drvdata(pdev); struct input_dev *input_dev = keypad->input_dev; - if (device_may_wakeup(&pdev->dev)) - disable_irq_wake(keypad->irq); - mutex_lock(&input_dev->mutex); if (input_device_enabled(input_dev)) { @@ -316,7 +311,11 @@ static int ep93xx_keypad_probe(struct platform_device *pdev) goto failed_free_irq; platform_set_drvdata(pdev, keypad); + device_init_wakeup(&pdev->dev, 1); + err = dev_pm_set_wake_irq(&pdev->dev, keypad->irq); + if (err) + dev_warn(&pdev->dev, "failed to set up wakeup irq: %d\n", err); return 0; @@ -342,6 +341,8 @@ static int ep93xx_keypad_remove(struct platform_device *pdev) struct ep93xx_keypad *keypad = platform_get_drvdata(pdev); struct resource *res; + dev_pm_clear_wake_irq(&pdev->dev); + free_irq(keypad->irq, keypad); if (keypad->enabled) -- 2.33.0.882.g93a45727a2-goog ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 3/4] Input: ep93xx_keypad - use dev_pm_set_wake_irq() 2021-10-12 1:37 ` [PATCH 3/4] Input: ep93xx_keypad - use dev_pm_set_wake_irq() Dmitry Torokhov @ 2021-10-12 7:30 ` Alexander Sverdlin 0 siblings, 0 replies; 11+ messages in thread From: Alexander Sverdlin @ 2021-10-12 7:30 UTC (permalink / raw) To: Dmitry Torokhov, linux-input; +Cc: linux-kernel Hi! On Mon, 2021-10-11 at 18:37 -0700, Dmitry Torokhov wrote: > Instead of manually toggling interrupt as wakeup source in suspend/resume > methods, let's declare keypad interrupt and wakeup interrupt and leave the > rest to the PM core. > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> Acked-by: Alexander Sverdlin <alexander.sverdlin@gmail.com> > --- > drivers/input/keyboard/ep93xx_keypad.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/input/keyboard/ep93xx_keypad.c b/drivers/input/keyboard/ep93xx_keypad.c > index 6be5474ba2f2..a66cfeaf5b21 100644 > --- a/drivers/input/keyboard/ep93xx_keypad.c > +++ b/drivers/input/keyboard/ep93xx_keypad.c > @@ -27,6 +27,7 @@ > #include <linux/slab.h> > #include <linux/soc/cirrus/ep93xx.h> > #include <linux/platform_data/keypad-ep93xx.h> > +#include <linux/pm_wakeirq.h> > > /* > * Keypad Interface Register offsets > @@ -191,9 +192,6 @@ static int __maybe_unused ep93xx_keypad_suspend(struct device *dev) > > mutex_unlock(&input_dev->mutex); > > - if (device_may_wakeup(&pdev->dev)) > - enable_irq_wake(keypad->irq); > - > return 0; > } > > @@ -203,9 +201,6 @@ static int __maybe_unused ep93xx_keypad_resume(struct device *dev) > struct ep93xx_keypad *keypad = platform_get_drvdata(pdev); > struct input_dev *input_dev = keypad->input_dev; > > - if (device_may_wakeup(&pdev->dev)) > - disable_irq_wake(keypad->irq); > - > mutex_lock(&input_dev->mutex); > > if (input_device_enabled(input_dev)) { > @@ -316,7 +311,11 @@ static int ep93xx_keypad_probe(struct platform_device *pdev) > goto failed_free_irq; > > platform_set_drvdata(pdev, keypad); > + > device_init_wakeup(&pdev->dev, 1); > + err = dev_pm_set_wake_irq(&pdev->dev, keypad->irq); > + if (err) > + dev_warn(&pdev->dev, "failed to set up wakeup irq: %d\n", err); > > return 0; > > @@ -342,6 +341,8 @@ static int ep93xx_keypad_remove(struct platform_device *pdev) > struct ep93xx_keypad *keypad = platform_get_drvdata(pdev); > struct resource *res; > > + dev_pm_clear_wake_irq(&pdev->dev); > + > free_irq(keypad->irq, keypad); > > if (keypad->enabled) -- Alexander Sverdlin. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 4/4] Input: ep93xx_keypad - switch to using managed resources 2021-10-12 1:37 [PATCH 1/4] Input: ep93xx_keypad - annotate suspend/resume as __maybe_unused Dmitry Torokhov 2021-10-12 1:37 ` [PATCH 2/4] Input: ep93xx_keypad - use BIT() and GENMASK() macros Dmitry Torokhov 2021-10-12 1:37 ` [PATCH 3/4] Input: ep93xx_keypad - use dev_pm_set_wake_irq() Dmitry Torokhov @ 2021-10-12 1:37 ` Dmitry Torokhov 2021-10-12 7:54 ` Alexander Sverdlin 2021-10-13 2:36 ` [PATCH v2 " Dmitry Torokhov 2021-10-12 6:31 ` [PATCH 1/4] Input: ep93xx_keypad - annotate suspend/resume as __maybe_unused Alexander Sverdlin 3 siblings, 2 replies; 11+ messages in thread From: Dmitry Torokhov @ 2021-10-12 1:37 UTC (permalink / raw) To: linux-input; +Cc: Alexander Sverdlin, linux-kernel By using managed resources (devm) we are able to streamline error handling in probe and remove most of the custom remove method. Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/input/keyboard/ep93xx_keypad.c | 116 ++++++++----------------- 1 file changed, 36 insertions(+), 80 deletions(-) diff --git a/drivers/input/keyboard/ep93xx_keypad.c b/drivers/input/keyboard/ep93xx_keypad.c index a66cfeaf5b21..90157707dc05 100644 --- a/drivers/input/keyboard/ep93xx_keypad.c +++ b/drivers/input/keyboard/ep93xx_keypad.c @@ -219,6 +219,13 @@ static int __maybe_unused ep93xx_keypad_resume(struct device *dev) static SIMPLE_DEV_PM_OPS(ep93xx_keypad_pm_ops, ep93xx_keypad_suspend, ep93xx_keypad_resume); +static void ep93xx_keypad_release_gpio_action(void *_pdev) +{ + struct platform_device *pdev = _pdev; + + ep93xx_keypad_release_gpio(pdev); +} + static int ep93xx_keypad_probe(struct platform_device *pdev) { struct ep93xx_keypad *keypad; @@ -227,61 +234,46 @@ static int ep93xx_keypad_probe(struct platform_device *pdev) struct resource *res; int err; - keypad = kzalloc(sizeof(struct ep93xx_keypad), GFP_KERNEL); + keypad = devm_kzalloc(&pdev->dev, sizeof(*keypad), GFP_KERNEL); if (!keypad) return -ENOMEM; keypad->pdata = dev_get_platdata(&pdev->dev); - if (!keypad->pdata) { - err = -EINVAL; - goto failed_free; - } + if (!keypad->pdata) + return -EINVAL; keymap_data = keypad->pdata->keymap_data; - if (!keymap_data) { - err = -EINVAL; - goto failed_free; - } + if (!keymap_data) + return -EINVAL; keypad->irq = platform_get_irq(pdev, 0); - if (keypad->irq < 0) { - err = keypad->irq; - goto failed_free; - } + if (keypad->irq < 0) + return keypad->irq; res = platform_get_resource(pdev, IORESOURCE_MEM, 0); - if (!res) { - err = -ENXIO; - goto failed_free; - } - - res = request_mem_region(res->start, resource_size(res), pdev->name); - if (!res) { - err = -EBUSY; - goto failed_free; - } + if (!res) + return -ENXIO; - keypad->mmio_base = ioremap(res->start, resource_size(res)); - if (keypad->mmio_base == NULL) { - err = -ENXIO; - goto failed_free_mem; - } + keypad->mmio_base = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(keypad->mmio_base)) + return PTR_ERR(keypad->mmio_base); err = ep93xx_keypad_acquire_gpio(pdev); if (err) - goto failed_free_io; + return err; + + err = devm_add_action_or_reset(&pdev->dev, + ep93xx_keypad_release_gpio_action, pdev); + if (err) + return err; keypad->clk = clk_get(&pdev->dev, NULL); - if (IS_ERR(keypad->clk)) { - err = PTR_ERR(keypad->clk); - goto failed_free_gpio; - } + if (IS_ERR(keypad->clk)) + return PTR_ERR(keypad->clk); - input_dev = input_allocate_device(); - if (!input_dev) { - err = -ENOMEM; - goto failed_put_clk; - } + input_dev = devm_input_allocate_device(&pdev->dev); + if (!input_dev) + return -ENOMEM; keypad->input_dev = input_dev; @@ -289,26 +281,26 @@ static int ep93xx_keypad_probe(struct platform_device *pdev) input_dev->id.bustype = BUS_HOST; input_dev->open = ep93xx_keypad_open; input_dev->close = ep93xx_keypad_close; - input_dev->dev.parent = &pdev->dev; err = matrix_keypad_build_keymap(keymap_data, NULL, EP93XX_MATRIX_ROWS, EP93XX_MATRIX_COLS, keypad->keycodes, input_dev); if (err) - goto failed_free_dev; + return err; if (keypad->pdata->flags & EP93XX_KEYPAD_AUTOREPEAT) __set_bit(EV_REP, input_dev->evbit); input_set_drvdata(input_dev, keypad); - err = request_irq(keypad->irq, ep93xx_keypad_irq_handler, - 0, pdev->name, keypad); + err = devm_request_irq(&pdev->dev, keypad->irq, + ep93xx_keypad_irq_handler, + 0, pdev->name, keypad); if (err) - goto failed_free_dev; + return err; err = input_register_device(input_dev); if (err) - goto failed_free_irq; + return err; platform_set_drvdata(pdev, keypad); @@ -318,48 +310,12 @@ static int ep93xx_keypad_probe(struct platform_device *pdev) dev_warn(&pdev->dev, "failed to set up wakeup irq: %d\n", err); return 0; - -failed_free_irq: - free_irq(keypad->irq, keypad); -failed_free_dev: - input_free_device(input_dev); -failed_put_clk: - clk_put(keypad->clk); -failed_free_gpio: - ep93xx_keypad_release_gpio(pdev); -failed_free_io: - iounmap(keypad->mmio_base); -failed_free_mem: - release_mem_region(res->start, resource_size(res)); -failed_free: - kfree(keypad); - return err; } static int ep93xx_keypad_remove(struct platform_device *pdev) { - struct ep93xx_keypad *keypad = platform_get_drvdata(pdev); - struct resource *res; - dev_pm_clear_wake_irq(&pdev->dev); - free_irq(keypad->irq, keypad); - - if (keypad->enabled) - clk_disable(keypad->clk); - clk_put(keypad->clk); - - input_unregister_device(keypad->input_dev); - - ep93xx_keypad_release_gpio(pdev); - - iounmap(keypad->mmio_base); - - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); - release_mem_region(res->start, resource_size(res)); - - kfree(keypad); - return 0; } -- 2.33.0.882.g93a45727a2-goog ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 4/4] Input: ep93xx_keypad - switch to using managed resources 2021-10-12 1:37 ` [PATCH 4/4] Input: ep93xx_keypad - switch to using managed resources Dmitry Torokhov @ 2021-10-12 7:54 ` Alexander Sverdlin 2021-10-13 2:34 ` Dmitry Torokhov 2021-10-13 2:36 ` [PATCH v2 " Dmitry Torokhov 1 sibling, 1 reply; 11+ messages in thread From: Alexander Sverdlin @ 2021-10-12 7:54 UTC (permalink / raw) To: Dmitry Torokhov, linux-input; +Cc: linux-kernel Hello Dmitry, just one question below: On Mon, 2021-10-11 at 18:37 -0700, Dmitry Torokhov wrote: > By using managed resources (devm) we are able to streamline error handling > in probe and remove most of the custom remove method. > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > drivers/input/keyboard/ep93xx_keypad.c | 116 ++++++++----------------- > 1 file changed, 36 insertions(+), 80 deletions(-) > > diff --git a/drivers/input/keyboard/ep93xx_keypad.c b/drivers/input/keyboard/ep93xx_keypad.c > index a66cfeaf5b21..90157707dc05 100644 > --- a/drivers/input/keyboard/ep93xx_keypad.c > +++ b/drivers/input/keyboard/ep93xx_keypad.c ... > @@ -227,61 +234,46 @@ static int ep93xx_keypad_probe(struct platform_device *pdev) > struct resource *res; > int err; > > - keypad = kzalloc(sizeof(struct ep93xx_keypad), GFP_KERNEL); > + keypad = devm_kzalloc(&pdev->dev, sizeof(*keypad), GFP_KERNEL); > if (!keypad) > return -ENOMEM; > > keypad->pdata = dev_get_platdata(&pdev->dev); > - if (!keypad->pdata) { > - err = -EINVAL; > - goto failed_free; > - } > + if (!keypad->pdata) > + return -EINVAL; > > keymap_data = keypad->pdata->keymap_data; > - if (!keymap_data) { > - err = -EINVAL; > - goto failed_free; > - } > + if (!keymap_data) > + return -EINVAL; > > keypad->irq = platform_get_irq(pdev, 0); > - if (keypad->irq < 0) { > - err = keypad->irq; > - goto failed_free; > - } > + if (keypad->irq < 0) > + return keypad->irq; > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > - if (!res) { > - err = -ENXIO; > - goto failed_free; > - } > - > - res = request_mem_region(res->start, resource_size(res), pdev->name); > - if (!res) { > - err = -EBUSY; > - goto failed_free; > - } > + if (!res) > + return -ENXIO; > > - keypad->mmio_base = ioremap(res->start, resource_size(res)); > - if (keypad->mmio_base == NULL) { > - err = -ENXIO; > - goto failed_free_mem; > - } > + keypad->mmio_base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(keypad->mmio_base)) > + return PTR_ERR(keypad->mmio_base); > > err = ep93xx_keypad_acquire_gpio(pdev); > if (err) > - goto failed_free_io; > + return err; > + > + err = devm_add_action_or_reset(&pdev->dev, > + ep93xx_keypad_release_gpio_action, pdev); > + if (err) > + return err; > > keypad->clk = clk_get(&pdev->dev, NULL); Isn't the conversion to devm_clk_get() missing here? > - if (IS_ERR(keypad->clk)) { > - err = PTR_ERR(keypad->clk); > - goto failed_free_gpio; > - } > + if (IS_ERR(keypad->clk)) > + return PTR_ERR(keypad->clk); > > - input_dev = input_allocate_device(); > - if (!input_dev) { > - err = -ENOMEM; > - goto failed_put_clk; > - } > + input_dev = devm_input_allocate_device(&pdev->dev); > + if (!input_dev) > + return -ENOMEM; > > keypad->input_dev = input_dev; > > @@ -289,26 +281,26 @@ static int ep93xx_keypad_probe(struct platform_device *pdev) > input_dev->id.bustype = BUS_HOST; > input_dev->open = ep93xx_keypad_open; > input_dev->close = ep93xx_keypad_close; > - input_dev->dev.parent = &pdev->dev; > > err = matrix_keypad_build_keymap(keymap_data, NULL, > EP93XX_MATRIX_ROWS, EP93XX_MATRIX_COLS, > keypad->keycodes, input_dev); > if (err) > - goto failed_free_dev; > + return err; > > if (keypad->pdata->flags & EP93XX_KEYPAD_AUTOREPEAT) > __set_bit(EV_REP, input_dev->evbit); > input_set_drvdata(input_dev, keypad); > > - err = request_irq(keypad->irq, ep93xx_keypad_irq_handler, > - 0, pdev->name, keypad); > + err = devm_request_irq(&pdev->dev, keypad->irq, > + ep93xx_keypad_irq_handler, > + 0, pdev->name, keypad); > if (err) > - goto failed_free_dev; > + return err; > > err = input_register_device(input_dev); > if (err) > - goto failed_free_irq; > + return err; > > platform_set_drvdata(pdev, keypad); > -- Alexander Sverdlin. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 4/4] Input: ep93xx_keypad - switch to using managed resources 2021-10-12 7:54 ` Alexander Sverdlin @ 2021-10-13 2:34 ` Dmitry Torokhov 0 siblings, 0 replies; 11+ messages in thread From: Dmitry Torokhov @ 2021-10-13 2:34 UTC (permalink / raw) To: Alexander Sverdlin; +Cc: linux-input, linux-kernel On Tue, Oct 12, 2021 at 09:54:32AM +0200, Alexander Sverdlin wrote: > Hello Dmitry, > > just one question below: > > On Mon, 2021-10-11 at 18:37 -0700, Dmitry Torokhov wrote: > > + return err; > > + > > + err = devm_add_action_or_reset(&pdev->dev, > > + ep93xx_keypad_release_gpio_action, pdev); > > + if (err) > > + return err; > > > > keypad->clk = clk_get(&pdev->dev, NULL); > > Isn't the conversion to devm_clk_get() missing here? Indeed it is. I'll post an updated patch in a sec. > > > - if (IS_ERR(keypad->clk)) { > > - err = PTR_ERR(keypad->clk); > > - goto failed_free_gpio; > > - } > > + if (IS_ERR(keypad->clk)) > > + return PTR_ERR(keypad->clk); Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 4/4] Input: ep93xx_keypad - switch to using managed resources 2021-10-12 1:37 ` [PATCH 4/4] Input: ep93xx_keypad - switch to using managed resources Dmitry Torokhov 2021-10-12 7:54 ` Alexander Sverdlin @ 2021-10-13 2:36 ` Dmitry Torokhov 2021-10-13 5:42 ` Alexander Sverdlin 1 sibling, 1 reply; 11+ messages in thread From: Dmitry Torokhov @ 2021-10-13 2:36 UTC (permalink / raw) To: linux-input; +Cc: Alexander Sverdlin, linux-kernel By using managed resources (devm) we are able to streamline error handling in probe and remove most of the custom remove method. Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/input/keyboard/ep93xx_keypad.c | 118 ++++++++----------------- 1 file changed, 37 insertions(+), 81 deletions(-) diff --git a/drivers/input/keyboard/ep93xx_keypad.c b/drivers/input/keyboard/ep93xx_keypad.c index a66cfeaf5b21..272a4f1c6e81 100644 --- a/drivers/input/keyboard/ep93xx_keypad.c +++ b/drivers/input/keyboard/ep93xx_keypad.c @@ -219,6 +219,13 @@ static int __maybe_unused ep93xx_keypad_resume(struct device *dev) static SIMPLE_DEV_PM_OPS(ep93xx_keypad_pm_ops, ep93xx_keypad_suspend, ep93xx_keypad_resume); +static void ep93xx_keypad_release_gpio_action(void *_pdev) +{ + struct platform_device *pdev = _pdev; + + ep93xx_keypad_release_gpio(pdev); +} + static int ep93xx_keypad_probe(struct platform_device *pdev) { struct ep93xx_keypad *keypad; @@ -227,61 +234,46 @@ static int ep93xx_keypad_probe(struct platform_device *pdev) struct resource *res; int err; - keypad = kzalloc(sizeof(struct ep93xx_keypad), GFP_KERNEL); + keypad = devm_kzalloc(&pdev->dev, sizeof(*keypad), GFP_KERNEL); if (!keypad) return -ENOMEM; keypad->pdata = dev_get_platdata(&pdev->dev); - if (!keypad->pdata) { - err = -EINVAL; - goto failed_free; - } + if (!keypad->pdata) + return -EINVAL; keymap_data = keypad->pdata->keymap_data; - if (!keymap_data) { - err = -EINVAL; - goto failed_free; - } + if (!keymap_data) + return -EINVAL; keypad->irq = platform_get_irq(pdev, 0); - if (keypad->irq < 0) { - err = keypad->irq; - goto failed_free; - } + if (keypad->irq < 0) + return keypad->irq; res = platform_get_resource(pdev, IORESOURCE_MEM, 0); - if (!res) { - err = -ENXIO; - goto failed_free; - } - - res = request_mem_region(res->start, resource_size(res), pdev->name); - if (!res) { - err = -EBUSY; - goto failed_free; - } + if (!res) + return -ENXIO; - keypad->mmio_base = ioremap(res->start, resource_size(res)); - if (keypad->mmio_base == NULL) { - err = -ENXIO; - goto failed_free_mem; - } + keypad->mmio_base = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(keypad->mmio_base)) + return PTR_ERR(keypad->mmio_base); err = ep93xx_keypad_acquire_gpio(pdev); if (err) - goto failed_free_io; + return err; - keypad->clk = clk_get(&pdev->dev, NULL); - if (IS_ERR(keypad->clk)) { - err = PTR_ERR(keypad->clk); - goto failed_free_gpio; - } + err = devm_add_action_or_reset(&pdev->dev, + ep93xx_keypad_release_gpio_action, pdev); + if (err) + return err; - input_dev = input_allocate_device(); - if (!input_dev) { - err = -ENOMEM; - goto failed_put_clk; - } + keypad->clk = devm_clk_get(&pdev->dev, NULL); + if (IS_ERR(keypad->clk)) + return PTR_ERR(keypad->clk); + + input_dev = devm_input_allocate_device(&pdev->dev); + if (!input_dev) + return -ENOMEM; keypad->input_dev = input_dev; @@ -289,26 +281,26 @@ static int ep93xx_keypad_probe(struct platform_device *pdev) input_dev->id.bustype = BUS_HOST; input_dev->open = ep93xx_keypad_open; input_dev->close = ep93xx_keypad_close; - input_dev->dev.parent = &pdev->dev; err = matrix_keypad_build_keymap(keymap_data, NULL, EP93XX_MATRIX_ROWS, EP93XX_MATRIX_COLS, keypad->keycodes, input_dev); if (err) - goto failed_free_dev; + return err; if (keypad->pdata->flags & EP93XX_KEYPAD_AUTOREPEAT) __set_bit(EV_REP, input_dev->evbit); input_set_drvdata(input_dev, keypad); - err = request_irq(keypad->irq, ep93xx_keypad_irq_handler, - 0, pdev->name, keypad); + err = devm_request_irq(&pdev->dev, keypad->irq, + ep93xx_keypad_irq_handler, + 0, pdev->name, keypad); if (err) - goto failed_free_dev; + return err; err = input_register_device(input_dev); if (err) - goto failed_free_irq; + return err; platform_set_drvdata(pdev, keypad); @@ -318,48 +310,12 @@ static int ep93xx_keypad_probe(struct platform_device *pdev) dev_warn(&pdev->dev, "failed to set up wakeup irq: %d\n", err); return 0; - -failed_free_irq: - free_irq(keypad->irq, keypad); -failed_free_dev: - input_free_device(input_dev); -failed_put_clk: - clk_put(keypad->clk); -failed_free_gpio: - ep93xx_keypad_release_gpio(pdev); -failed_free_io: - iounmap(keypad->mmio_base); -failed_free_mem: - release_mem_region(res->start, resource_size(res)); -failed_free: - kfree(keypad); - return err; } static int ep93xx_keypad_remove(struct platform_device *pdev) { - struct ep93xx_keypad *keypad = platform_get_drvdata(pdev); - struct resource *res; - dev_pm_clear_wake_irq(&pdev->dev); - free_irq(keypad->irq, keypad); - - if (keypad->enabled) - clk_disable(keypad->clk); - clk_put(keypad->clk); - - input_unregister_device(keypad->input_dev); - - ep93xx_keypad_release_gpio(pdev); - - iounmap(keypad->mmio_base); - - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); - release_mem_region(res->start, resource_size(res)); - - kfree(keypad); - return 0; } -- 2.33.0.882.g93a45727a2-goog ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 4/4] Input: ep93xx_keypad - switch to using managed resources 2021-10-13 2:36 ` [PATCH v2 " Dmitry Torokhov @ 2021-10-13 5:42 ` Alexander Sverdlin 0 siblings, 0 replies; 11+ messages in thread From: Alexander Sverdlin @ 2021-10-13 5:42 UTC (permalink / raw) To: Dmitry Torokhov, linux-input; +Cc: linux-kernel On Tue, 2021-10-12 at 19:36 -0700, Dmitry Torokhov wrote: > By using managed resources (devm) we are able to streamline error handling > in probe and remove most of the custom remove method. > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> Acked-by: Alexander Sverdlin <alexander.sverdlin@gmail.com> > --- > drivers/input/keyboard/ep93xx_keypad.c | 118 ++++++++----------------- > 1 file changed, 37 insertions(+), 81 deletions(-) > > diff --git a/drivers/input/keyboard/ep93xx_keypad.c b/drivers/input/keyboard/ep93xx_keypad.c > index a66cfeaf5b21..272a4f1c6e81 100644 > --- a/drivers/input/keyboard/ep93xx_keypad.c > +++ b/drivers/input/keyboard/ep93xx_keypad.c > @@ -219,6 +219,13 @@ static int __maybe_unused ep93xx_keypad_resume(struct device *dev) > static SIMPLE_DEV_PM_OPS(ep93xx_keypad_pm_ops, > ep93xx_keypad_suspend, ep93xx_keypad_resume); > > +static void ep93xx_keypad_release_gpio_action(void *_pdev) > +{ > + struct platform_device *pdev = _pdev; > + > + ep93xx_keypad_release_gpio(pdev); > +} > + > static int ep93xx_keypad_probe(struct platform_device *pdev) > { > struct ep93xx_keypad *keypad; > @@ -227,61 +234,46 @@ static int ep93xx_keypad_probe(struct platform_device *pdev) > struct resource *res; > int err; > > - keypad = kzalloc(sizeof(struct ep93xx_keypad), GFP_KERNEL); > + keypad = devm_kzalloc(&pdev->dev, sizeof(*keypad), GFP_KERNEL); > if (!keypad) > return -ENOMEM; > > keypad->pdata = dev_get_platdata(&pdev->dev); > - if (!keypad->pdata) { > - err = -EINVAL; > - goto failed_free; > - } > + if (!keypad->pdata) > + return -EINVAL; > > keymap_data = keypad->pdata->keymap_data; > - if (!keymap_data) { > - err = -EINVAL; > - goto failed_free; > - } > + if (!keymap_data) > + return -EINVAL; > > keypad->irq = platform_get_irq(pdev, 0); > - if (keypad->irq < 0) { > - err = keypad->irq; > - goto failed_free; > - } > + if (keypad->irq < 0) > + return keypad->irq; > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > - if (!res) { > - err = -ENXIO; > - goto failed_free; > - } > - > - res = request_mem_region(res->start, resource_size(res), pdev->name); > - if (!res) { > - err = -EBUSY; > - goto failed_free; > - } > + if (!res) > + return -ENXIO; > > - keypad->mmio_base = ioremap(res->start, resource_size(res)); > - if (keypad->mmio_base == NULL) { > - err = -ENXIO; > - goto failed_free_mem; > - } > + keypad->mmio_base = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(keypad->mmio_base)) > + return PTR_ERR(keypad->mmio_base); > > err = ep93xx_keypad_acquire_gpio(pdev); > if (err) > - goto failed_free_io; > + return err; > > - keypad->clk = clk_get(&pdev->dev, NULL); > - if (IS_ERR(keypad->clk)) { > - err = PTR_ERR(keypad->clk); > - goto failed_free_gpio; > - } > + err = devm_add_action_or_reset(&pdev->dev, > + ep93xx_keypad_release_gpio_action, pdev); > + if (err) > + return err; > > - input_dev = input_allocate_device(); > - if (!input_dev) { > - err = -ENOMEM; > - goto failed_put_clk; > - } > + keypad->clk = devm_clk_get(&pdev->dev, NULL); > + if (IS_ERR(keypad->clk)) > + return PTR_ERR(keypad->clk); > + > + input_dev = devm_input_allocate_device(&pdev->dev); > + if (!input_dev) > + return -ENOMEM; > > keypad->input_dev = input_dev; > > @@ -289,26 +281,26 @@ static int ep93xx_keypad_probe(struct platform_device *pdev) > input_dev->id.bustype = BUS_HOST; > input_dev->open = ep93xx_keypad_open; > input_dev->close = ep93xx_keypad_close; > - input_dev->dev.parent = &pdev->dev; > > err = matrix_keypad_build_keymap(keymap_data, NULL, > EP93XX_MATRIX_ROWS, EP93XX_MATRIX_COLS, > keypad->keycodes, input_dev); > if (err) > - goto failed_free_dev; > + return err; > > if (keypad->pdata->flags & EP93XX_KEYPAD_AUTOREPEAT) > __set_bit(EV_REP, input_dev->evbit); > input_set_drvdata(input_dev, keypad); > > - err = request_irq(keypad->irq, ep93xx_keypad_irq_handler, > - 0, pdev->name, keypad); > + err = devm_request_irq(&pdev->dev, keypad->irq, > + ep93xx_keypad_irq_handler, > + 0, pdev->name, keypad); > if (err) > - goto failed_free_dev; > + return err; > > err = input_register_device(input_dev); > if (err) > - goto failed_free_irq; > + return err; > > platform_set_drvdata(pdev, keypad); > > @@ -318,48 +310,12 @@ static int ep93xx_keypad_probe(struct platform_device *pdev) > dev_warn(&pdev->dev, "failed to set up wakeup irq: %d\n", err); > > return 0; > - > -failed_free_irq: > - free_irq(keypad->irq, keypad); > -failed_free_dev: > - input_free_device(input_dev); > -failed_put_clk: > - clk_put(keypad->clk); > -failed_free_gpio: > - ep93xx_keypad_release_gpio(pdev); > -failed_free_io: > - iounmap(keypad->mmio_base); > -failed_free_mem: > - release_mem_region(res->start, resource_size(res)); > -failed_free: > - kfree(keypad); > - return err; > } > > static int ep93xx_keypad_remove(struct platform_device *pdev) > { > - struct ep93xx_keypad *keypad = platform_get_drvdata(pdev); > - struct resource *res; > - > dev_pm_clear_wake_irq(&pdev->dev); > > - free_irq(keypad->irq, keypad); > - > - if (keypad->enabled) > - clk_disable(keypad->clk); > - clk_put(keypad->clk); > - > - input_unregister_device(keypad->input_dev); > - > - ep93xx_keypad_release_gpio(pdev); > - > - iounmap(keypad->mmio_base); > - > - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > - release_mem_region(res->start, resource_size(res)); > - > - kfree(keypad); > - > return 0; > } > -- Alexander Sverdlin. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/4] Input: ep93xx_keypad - annotate suspend/resume as __maybe_unused 2021-10-12 1:37 [PATCH 1/4] Input: ep93xx_keypad - annotate suspend/resume as __maybe_unused Dmitry Torokhov ` (2 preceding siblings ...) 2021-10-12 1:37 ` [PATCH 4/4] Input: ep93xx_keypad - switch to using managed resources Dmitry Torokhov @ 2021-10-12 6:31 ` Alexander Sverdlin 3 siblings, 0 replies; 11+ messages in thread From: Alexander Sverdlin @ 2021-10-12 6:31 UTC (permalink / raw) To: Dmitry Torokhov, linux-input; +Cc: linux-kernel Hi! On Mon, 2021-10-11 at 18:37 -0700, Dmitry Torokhov wrote: > Instead of guarding suspend/resume methods with #ifdef CONFIG_PM > let's mark them as __maybe_unused as this allows better compile > coverage. > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> Acked-by: Alexander Sverdlin <alexander.sverdlin@gmail.com> > --- > drivers/input/keyboard/ep93xx_keypad.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/input/keyboard/ep93xx_keypad.c b/drivers/input/keyboard/ep93xx_keypad.c > index e0e931e796fa..a0c6cdf8e0d3 100644 > --- a/drivers/input/keyboard/ep93xx_keypad.c > +++ b/drivers/input/keyboard/ep93xx_keypad.c > @@ -175,8 +175,7 @@ static void ep93xx_keypad_close(struct input_dev *pdev) > } > > > -#ifdef CONFIG_PM_SLEEP > -static int ep93xx_keypad_suspend(struct device *dev) > +static int __maybe_unused ep93xx_keypad_suspend(struct device *dev) > { > struct platform_device *pdev = to_platform_device(dev); > struct ep93xx_keypad *keypad = platform_get_drvdata(pdev); > @@ -197,7 +196,7 @@ static int ep93xx_keypad_suspend(struct device *dev) > return 0; > } > > -static int ep93xx_keypad_resume(struct device *dev) > +static int __maybe_unused ep93xx_keypad_resume(struct device *dev) > { > struct platform_device *pdev = to_platform_device(dev); > struct ep93xx_keypad *keypad = platform_get_drvdata(pdev); > @@ -220,7 +219,6 @@ static int ep93xx_keypad_resume(struct device *dev) > > return 0; > } > -#endif > > static SIMPLE_DEV_PM_OPS(ep93xx_keypad_pm_ops, > ep93xx_keypad_suspend, ep93xx_keypad_resume); -- Alexander Sverdlin. ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-10-13 5:42 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-12 1:37 [PATCH 1/4] Input: ep93xx_keypad - annotate suspend/resume as __maybe_unused Dmitry Torokhov 2021-10-12 1:37 ` [PATCH 2/4] Input: ep93xx_keypad - use BIT() and GENMASK() macros Dmitry Torokhov 2021-10-12 6:58 ` Alexander Sverdlin 2021-10-12 1:37 ` [PATCH 3/4] Input: ep93xx_keypad - use dev_pm_set_wake_irq() Dmitry Torokhov 2021-10-12 7:30 ` Alexander Sverdlin 2021-10-12 1:37 ` [PATCH 4/4] Input: ep93xx_keypad - switch to using managed resources Dmitry Torokhov 2021-10-12 7:54 ` Alexander Sverdlin 2021-10-13 2:34 ` Dmitry Torokhov 2021-10-13 2:36 ` [PATCH v2 " Dmitry Torokhov 2021-10-13 5:42 ` Alexander Sverdlin 2021-10-12 6:31 ` [PATCH 1/4] Input: ep93xx_keypad - annotate suspend/resume as __maybe_unused Alexander Sverdlin
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).