Hi, This is the fourth iteration of DT support for the TWL4030 power button. Changes since v3 [0]: * remove twl4030_pwrbutton_remove function, which is no longer needed after devm_ conversion. [0] https://lkml.org/lkml/2013/10/23/353 -- Sebastian Sebastian Reichel (3): Input: twl4030-pwrbutton - add device tree support Input: twl4030-pwrbutton: use dev_err for errors Input: twl4030-pwrbutton: simplify driver using devm_* .../bindings/input/twl4030-pwrbutton.txt | 13 ++++++ drivers/input/misc/twl4030-pwrbutton.c | 46 +++++++++------------- 2 files changed, 31 insertions(+), 28 deletions(-) create mode 100644 Documentation/devicetree/bindings/input/twl4030-pwrbutton.txt -- 1.8.4.rc3
Add device tree support for twl4030 power button driver. Signed-off-by: Sebastian Reichel <sre@debian.org> --- .../devicetree/bindings/input/twl4030-pwrbutton.txt | 13 +++++++++++++ drivers/input/misc/twl4030-pwrbutton.c | 16 ++++++++++++---- 2 files changed, 25 insertions(+), 4 deletions(-) create mode 100644 Documentation/devicetree/bindings/input/twl4030-pwrbutton.txt diff --git a/Documentation/devicetree/bindings/input/twl4030-pwrbutton.txt b/Documentation/devicetree/bindings/input/twl4030-pwrbutton.txt new file mode 100644 index 0000000..945ec74 --- /dev/null +++ b/Documentation/devicetree/bindings/input/twl4030-pwrbutton.txt @@ -0,0 +1,13 @@ +* TWL4030's pwrbutton device tree bindings + +Required SoC Specific Properties: +- compatible: should be one of the following + - "ti,twl4030-pwrbutton": For controllers compatible with twl4030 +- interrupt: should be one of the following + - <8>: For controllers compatible with twl4030 + +Example: + twl_pwrbutton: pwrbutton { + compatible = "ti,twl4030-pwrbutton"; + interrupts = <8>; + }; diff --git a/drivers/input/misc/twl4030-pwrbutton.c b/drivers/input/misc/twl4030-pwrbutton.c index b9a05fd..a3a0fe3 100644 --- a/drivers/input/misc/twl4030-pwrbutton.c +++ b/drivers/input/misc/twl4030-pwrbutton.c @@ -52,7 +52,7 @@ static irqreturn_t powerbutton_irq(int irq, void *_pwr) return IRQ_HANDLED; } -static int __init twl4030_pwrbutton_probe(struct platform_device *pdev) +static int twl4030_pwrbutton_probe(struct platform_device *pdev) { struct input_dev *pwr; int irq = platform_get_irq(pdev, 0); @@ -106,16 +106,24 @@ static int __exit twl4030_pwrbutton_remove(struct platform_device *pdev) return 0; } +#if IS_ENABLED(CONFIG_OF) +static const struct of_device_id twl4030_pwrbutton_dt_match_table[] = { + { .compatible = "ti,twl4030-pwrbutton" }, + {}, +}; +MODULE_DEVICE_TABLE(of, twl4030_pwrbutton_dt_match_table); +#endif + static struct platform_driver twl4030_pwrbutton_driver = { + .probe = twl4030_pwrbutton_probe, .remove = __exit_p(twl4030_pwrbutton_remove), .driver = { .name = "twl4030_pwrbutton", .owner = THIS_MODULE, + .of_match_table = of_match_ptr(twl4030_pwrbutton_dt_match_table), }, }; - -module_platform_driver_probe(twl4030_pwrbutton_driver, - twl4030_pwrbutton_probe); +module_platform_driver(twl4030_pwrbutton_driver); MODULE_ALIAS("platform:twl4030_pwrbutton"); MODULE_DESCRIPTION("Triton2 Power Button"); -- 1.8.4.rc3
Use dev_err() to output errors instead of dev_dbg(). Signed-off-by: Sebastian Reichel <sre@debian.org> --- drivers/input/misc/twl4030-pwrbutton.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/input/misc/twl4030-pwrbutton.c b/drivers/input/misc/twl4030-pwrbutton.c index a3a0fe3..3efbb13 100644 --- a/drivers/input/misc/twl4030-pwrbutton.c +++ b/drivers/input/misc/twl4030-pwrbutton.c @@ -60,7 +60,7 @@ static int twl4030_pwrbutton_probe(struct platform_device *pdev) pwr = input_allocate_device(); if (!pwr) { - dev_dbg(&pdev->dev, "Can't allocate power button\n"); + dev_err(&pdev->dev, "Can't allocate power button\n"); return -ENOMEM; } @@ -74,13 +74,13 @@ static int twl4030_pwrbutton_probe(struct platform_device *pdev) IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING, "twl4030_pwrbutton", pwr); if (err < 0) { - dev_dbg(&pdev->dev, "Can't get IRQ for pwrbutton: %d\n", err); + dev_err(&pdev->dev, "Can't get IRQ for pwrbutton: %d\n", err); goto free_input_dev; } err = input_register_device(pwr); if (err) { - dev_dbg(&pdev->dev, "Can't register power button: %d\n", err); + dev_err(&pdev->dev, "Can't register power button: %d\n", err); goto free_irq; } -- 1.8.4.rc3
Use managed irq resource to simplify the driver. Signed-off-by: Sebastian Reichel <sre@debian.org> --- drivers/input/misc/twl4030-pwrbutton.c | 26 ++++---------------------- 1 file changed, 4 insertions(+), 22 deletions(-) diff --git a/drivers/input/misc/twl4030-pwrbutton.c b/drivers/input/misc/twl4030-pwrbutton.c index 3efbb13..da30af0 100644 --- a/drivers/input/misc/twl4030-pwrbutton.c +++ b/drivers/input/misc/twl4030-pwrbutton.c @@ -58,7 +58,7 @@ static int twl4030_pwrbutton_probe(struct platform_device *pdev) int irq = platform_get_irq(pdev, 0); int err; - pwr = input_allocate_device(); + pwr = devm_input_allocate_device(&pdev->dev); if (!pwr) { dev_err(&pdev->dev, "Can't allocate power button\n"); return -ENOMEM; @@ -70,40 +70,23 @@ static int twl4030_pwrbutton_probe(struct platform_device *pdev) pwr->phys = "twl4030_pwrbutton/input0"; pwr->dev.parent = &pdev->dev; - err = request_threaded_irq(irq, NULL, powerbutton_irq, + err = devm_request_threaded_irq(&pwr->dev, irq, NULL, powerbutton_irq, IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING, "twl4030_pwrbutton", pwr); if (err < 0) { dev_err(&pdev->dev, "Can't get IRQ for pwrbutton: %d\n", err); - goto free_input_dev; + return err; } err = input_register_device(pwr); if (err) { dev_err(&pdev->dev, "Can't register power button: %d\n", err); - goto free_irq; + return err; } platform_set_drvdata(pdev, pwr); return 0; - -free_irq: - free_irq(irq, pwr); -free_input_dev: - input_free_device(pwr); - return err; -} - -static int __exit twl4030_pwrbutton_remove(struct platform_device *pdev) -{ - struct input_dev *pwr = platform_get_drvdata(pdev); - int irq = platform_get_irq(pdev, 0); - - free_irq(irq, pwr); - input_unregister_device(pwr); - - return 0; } #if IS_ENABLED(CONFIG_OF) @@ -116,7 +99,6 @@ MODULE_DEVICE_TABLE(of, twl4030_pwrbutton_dt_match_table); static struct platform_driver twl4030_pwrbutton_driver = { .probe = twl4030_pwrbutton_probe, - .remove = __exit_p(twl4030_pwrbutton_remove), .driver = { .name = "twl4030_pwrbutton", .owner = THIS_MODULE, -- 1.8.4.rc3
On Wed, 2013-10-23 at 19:54 +0200, Sebastian Reichel wrote: > Use dev_err() to output errors instead of dev_dbg(). [] > diff --git a/drivers/input/misc/twl4030-pwrbutton.c b/drivers/input/misc/twl4030-pwrbutton.c [] > @@ -60,7 +60,7 @@ static int twl4030_pwrbutton_probe(struct platform_device *pdev) > > pwr = input_allocate_device(); > if (!pwr) { > - dev_dbg(&pdev->dev, "Can't allocate power button\n"); > + dev_err(&pdev->dev, "Can't allocate power button\n"); > return -ENOMEM; > } input_allocate_device uses kzalloc and it will emit a standardized OOM message along with a dump_stack() so this dev_err/dev_dbg is redundant and not necessary.
[-- Attachment #1: Type: text/plain, Size: 883 bytes --] On Wed, Oct 23, 2013 at 11:17:46AM -0700, Joe Perches wrote: > On Wed, 2013-10-23 at 19:54 +0200, Sebastian Reichel wrote: > > Use dev_err() to output errors instead of dev_dbg(). > [] > > diff --git a/drivers/input/misc/twl4030-pwrbutton.c b/drivers/input/misc/twl4030-pwrbutton.c > [] > > @@ -60,7 +60,7 @@ static int twl4030_pwrbutton_probe(struct platform_device *pdev) > > > > pwr = input_allocate_device(); > > if (!pwr) { > > - dev_dbg(&pdev->dev, "Can't allocate power button\n"); > > + dev_err(&pdev->dev, "Can't allocate power button\n"); > > return -ENOMEM; > > } > > input_allocate_device uses kzalloc and it will emit a > standardized OOM message along with a dump_stack() > so this dev_err/dev_dbg is redundant and not necessary. I saw you sent a big patchset changing this for all drivers. Should I drop this patch? -- Sebastian [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --]
On Wed, 2013-10-23 at 21:31 +0200, Sebastian Reichel wrote: > On Wed, Oct 23, 2013 at 11:17:46AM -0700, Joe Perches wrote: > > On Wed, 2013-10-23 at 19:54 +0200, Sebastian Reichel wrote: > > > Use dev_err() to output errors instead of dev_dbg(). > > [] > > > diff --git a/drivers/input/misc/twl4030-pwrbutton.c b/drivers/input/misc/twl4030-pwrbutton.c > > [] > > > @@ -60,7 +60,7 @@ static int twl4030_pwrbutton_probe(struct platform_device *pdev) > > > > > > pwr = input_allocate_device(); > > > if (!pwr) { > > > - dev_dbg(&pdev->dev, "Can't allocate power button\n"); > > > + dev_err(&pdev->dev, "Can't allocate power button\n"); > > > return -ENOMEM; > > > } > > > > input_allocate_device uses kzalloc and it will emit a > > standardized OOM message along with a dump_stack() > > so this dev_err/dev_dbg is redundant and not necessary. > > I saw you sent a big patchset changing this for all drivers. > Should I drop this patch? I think the 2 other bits of this patch are fine. Maybe resend with this section dropped? -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Sebastian, On 10/23/2013 07:54 PM, Sebastian Reichel wrote: > Add device tree support for twl4030 power button driver. > > Signed-off-by: Sebastian Reichel <sre@debian.org> > --- > .../devicetree/bindings/input/twl4030-pwrbutton.txt | 13 +++++++++++++ > drivers/input/misc/twl4030-pwrbutton.c | 16 ++++++++++++---- > 2 files changed, 25 insertions(+), 4 deletions(-) > create mode 100644 Documentation/devicetree/bindings/input/twl4030-pwrbutton.txt > > diff --git a/Documentation/devicetree/bindings/input/twl4030-pwrbutton.txt b/Documentation/devicetree/bindings/input/twl4030-pwrbutton.txt > new file mode 100644 > index 0000000..945ec74 > --- /dev/null > +++ b/Documentation/devicetree/bindings/input/twl4030-pwrbutton.txt > @@ -0,0 +1,13 @@ > +* TWL4030's pwrbutton device tree bindings > + > +Required SoC Specific Properties: > +- compatible: should be one of the following > + - "ti,twl4030-pwrbutton": For controllers compatible with twl4030 > +- interrupt: should be one of the following > + - <8>: For controllers compatible with twl4030 This is <8> for your particular case, but it will depend on your SoC, won't it? Moreover, this property will be most likely inherited from the root twl node, so I do not see the need to document it here. See: Documentation/devicetree/bindings/mfd/twl-familly.txt > + > +Example: > + twl_pwrbutton: pwrbutton { > + compatible = "ti,twl4030-pwrbutton"; > + interrupts = <8>; > + }; You are missing the root twl node here, no? > diff --git a/drivers/input/misc/twl4030-pwrbutton.c b/drivers/input/misc/twl4030-pwrbutton.c > index b9a05fd..a3a0fe3 100644 > --- a/drivers/input/misc/twl4030-pwrbutton.c > +++ b/drivers/input/misc/twl4030-pwrbutton.c Missing #include <linux/of.h> ? > @@ -52,7 +52,7 @@ static irqreturn_t powerbutton_irq(int irq, void *_pwr) > return IRQ_HANDLED; > } > > -static int __init twl4030_pwrbutton_probe(struct platform_device *pdev) > +static int twl4030_pwrbutton_probe(struct platform_device *pdev) > { > struct input_dev *pwr; > int irq = platform_get_irq(pdev, 0); > @@ -106,16 +106,24 @@ static int __exit twl4030_pwrbutton_remove(struct platform_device *pdev) > return 0; > } > > +#if IS_ENABLED(CONFIG_OF) > +static const struct of_device_id twl4030_pwrbutton_dt_match_table[] = { > + { .compatible = "ti,twl4030-pwrbutton" }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, twl4030_pwrbutton_dt_match_table); > +#endif > + > static struct platform_driver twl4030_pwrbutton_driver = { > + .probe = twl4030_pwrbutton_probe, > .remove = __exit_p(twl4030_pwrbutton_remove), > .driver = { > .name = "twl4030_pwrbutton", > .owner = THIS_MODULE, > + .of_match_table = of_match_ptr(twl4030_pwrbutton_dt_match_table), > }, > }; > - > -module_platform_driver_probe(twl4030_pwrbutton_driver, > - twl4030_pwrbutton_probe); > +module_platform_driver(twl4030_pwrbutton_driver); > > MODULE_ALIAS("platform:twl4030_pwrbutton"); > MODULE_DESCRIPTION("Triton2 Power Button"); > Best regards, Florian
[-- Attachment #1: Type: text/plain, Size: 1580 bytes --] Hi Florian, On Thu, Oct 24, 2013 at 09:47:33AM +0200, Florian Vaussard wrote: > > +Required SoC Specific Properties: > > +- compatible: should be one of the following > > + - "ti,twl4030-pwrbutton": For controllers compatible with twl4030 > > +- interrupt: should be one of the following > > + - <8>: For controllers compatible with twl4030 > > This is <8> for your particular case, but it will depend on your > SoC, won't it? Moreover, this property will be most likely > inherited from the root twl node, so I do not see the need to > document it here. See: > > Documentation/devicetree/bindings/mfd/twl-familly.txt No. This is an internal twl4030 interrupt. TWL4030 functions itself as an interrupt controller. > > + > > +Example: > > + twl_pwrbutton: pwrbutton { > > + compatible = "ti,twl4030-pwrbutton"; > > + interrupts = <8>; > > + }; > > You are missing the root twl node here, no? So should I document it like this? twl4030 { compatible = "ti,twl4030"; pwrbutton { compatible = "ti,twl4030-pwrbutton"; interrupts = <8>; }; }; > > diff --git a/drivers/input/misc/twl4030-pwrbutton.c b/drivers/input/misc/twl4030-pwrbutton.c > > index b9a05fd..a3a0fe3 100644 > > --- a/drivers/input/misc/twl4030-pwrbutton.c > > +++ b/drivers/input/misc/twl4030-pwrbutton.c > > Missing #include <linux/of.h> ? It's included indirectly, but I should probably add a direct include. Thanks. > Best regards, Thanks for the comments. I will sent out an updated patchset later. -- Sebastian [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 836 bytes --]
Hello, On 10/24/2013 10:38 AM, Sebastian Reichel wrote: > Hi Florian, > > On Thu, Oct 24, 2013 at 09:47:33AM +0200, Florian Vaussard wrote: >>> +Required SoC Specific Properties: >>> +- compatible: should be one of the following >>> + - "ti,twl4030-pwrbutton": For controllers compatible with twl4030 >>> +- interrupt: should be one of the following >>> + - <8>: For controllers compatible with twl4030 >> >> This is <8> for your particular case, but it will depend on your >> SoC, won't it? Moreover, this property will be most likely >> inherited from the root twl node, so I do not see the need to >> document it here. See: >> >> Documentation/devicetree/bindings/mfd/twl-familly.txt > > No. This is an internal twl4030 interrupt. TWL4030 functions > itself as an interrupt controller. > So if it does not belong to the TWL parent, where is it used in your code? You should be parsing this property, so you can set up the IRQ properly. I am a bit confused here. If it is fixed, no need for a OF property. >>> + >>> +Example: >>> + twl_pwrbutton: pwrbutton { >>> + compatible = "ti,twl4030-pwrbutton"; >>> + interrupts = <8>; >>> + }; >> >> You are missing the root twl node here, no? > > So should I document it like this? > IMHO it is more clear for the user. > twl4030 { > compatible = "ti,twl4030"; > > pwrbutton { > compatible = "ti,twl4030-pwrbutton"; > interrupts = <8>; > }; > }; Nit, but existing documentations follow the "name@address" form for the root node, as the TWL is on an I2C bus. Either it is already defined, thus you should use "&twl4030" to reference it, or you create the TWL node and something like "twl4030@48" should be used. For an example, you can refer to existing bindings, like Documentation/devicetree/bindings/mfd/twl4030-audio.txt. Best regards, Florian