[PATCHv6 0/3] DT Support for TWL4030 power button

[PATCHv6 0/3] DT Support for TWL4030 power button

[Sebastian Reichel]

Hi,

This is the sixth iteration of DT support for the TWL4030
power button.

Changes since v5 [0]:
* Updated documentation

[0] https://lkml.org/lkml/2013/10/23/416

-- 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           | 21 +++++++++++
 drivers/input/misc/twl4030-pwrbutton.c             | 44 +++++++++-------------
 2 files changed, 38 insertions(+), 27 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/input/twl4030-pwrbutton.txt

-- 
1.8.4.rc3

[PATCHv6 1/3] Input: twl4030-pwrbutton - add device tree support

[Sebastian Reichel]

Add device tree support for twl4030 power button driver.

Signed-off-by: Sebastian Reichel <sre@debian.org>
---
 .../devicetree/bindings/input/twl4030-pwrbutton.txt | 21 +++++++++++++++++++++
 drivers/input/misc/twl4030-pwrbutton.c              | 16 ++++++++++++----
 2 files changed, 33 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..4375646
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/twl4030-pwrbutton.txt
@@ -0,0 +1,21 @@
+Texas Instruments TWL family (twl4030) pwrbutton module
+
+This module is part of the TWL4030. For more details about the whole
+chip see Documentation/devicetree/bindings/mfd/twl-familly.txt.
+
+This module provides a simple power button event via an Interrupt.
+
+Required 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 {
+	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

[PATCHv6 2/3] Input: twl4030-pwrbutton: use dev_err for errors

[Sebastian Reichel]

Use dev_err() to output errors instead of dev_dbg().

Signed-off-by: Sebastian Reichel <sre@debian.org>
---
 drivers/input/misc/twl4030-pwrbutton.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/input/misc/twl4030-pwrbutton.c b/drivers/input/misc/twl4030-pwrbutton.c
index a3a0fe3..48639ff 100644
--- a/drivers/input/misc/twl4030-pwrbutton.c
+++ b/drivers/input/misc/twl4030-pwrbutton.c
@@ -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

[PATCHv6 3/3] Input: twl4030-pwrbutton: simplify driver using devm_*

[Sebastian Reichel]

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 48639ff..be1759c 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_dbg(&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

Re: [PATCHv6 1/3] Input: twl4030-pwrbutton - add device tree support

[Florian Vaussard]

Hello,

On 10/24/2013 04:48 PM, Sebastian Reichel wrote:
> Add device tree support for twl4030 power button driver.
> 
> Signed-off-by: Sebastian Reichel <sre@debian.org>

I tested on my OMAP3 board, the button event works as expected. So for
the DT boot, feel free to add my:

Tested-by: Florian Vaussard <florian.vaussard@epfl.ch>

Best regards,

Florian

Re: [PATCHv6 1/3] Input: twl4030-pwrbutton - add device tree support

[Peter Ujfalusi]

On 10/24/2013 05:48 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 | 21 +++++++++++++++++++++
>  drivers/input/misc/twl4030-pwrbutton.c              | 16 ++++++++++++----
>  2 files changed, 33 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..4375646
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/twl4030-pwrbutton.txt
> @@ -0,0 +1,21 @@
> +Texas Instruments TWL family (twl4030) pwrbutton module
> +
> +This module is part of the TWL4030. For more details about the whole
> +chip see Documentation/devicetree/bindings/mfd/twl-familly.txt.
> +
> +This module provides a simple power button event via an Interrupt.
> +
> +Required 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 {
> +	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)

You don't need to do this.

> +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),

If you try to compile this driver with config !CONFIG_OF it will not work in
this way.

>  	},
>  };
> -
> -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");
> 


-- 
Péter

Re: [PATCHv6 1/3] Input: twl4030-pwrbutton - add device tree support

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1058 bytes --]

Hi Péter,

On Fri, Oct 25, 2013 at 03:46:02PM +0300, Peter Ujfalusi wrote:
> > [...]
> > +#if IS_ENABLED(CONFIG_OF)
> 
> You don't need to do this.

It's done like this in all the other drivers.

> > +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),
> 
> If you try to compile this driver with config !CONFIG_OF it will not work in
> this way.

For !CONFIG_OF of_match_ptr is defined as follows (in "include/linux/of.h"):

#define of_match_ptr(_ptr)  NULL

So the preprocessor will remove the undefined symbol.

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv6 1/3] Input: twl4030-pwrbutton - add device tree support

[Grant Likely]

On Thu, 24 Oct 2013 16:48:44 +0200, Sebastian Reichel <sre@debian.org> wrote:
> Add device tree support for twl4030 power button driver.

The above commit text is insufficient. There are changes in the patch
that aren't described here and have nothing to do with device tree
bindings.

> 
> Signed-off-by: Sebastian Reichel <sre@debian.org>
> ---
>  .../devicetree/bindings/input/twl4030-pwrbutton.txt | 21 +++++++++++++++++++++
>  drivers/input/misc/twl4030-pwrbutton.c              | 16 ++++++++++++----
>  2 files changed, 33 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..4375646
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/twl4030-pwrbutton.txt
> @@ -0,0 +1,21 @@
> +Texas Instruments TWL family (twl4030) pwrbutton module

Can all of the TWL or TWL4030 funciton bindings be collected into a
single file please? It is a single device after all. All of it should be
in bindings/mfd/twl-family.txt

> +
> +This module is part of the TWL4030. For more details about the whole
> +chip see Documentation/devicetree/bindings/mfd/twl-familly.txt.
> +
> +This module provides a simple power button event via an Interrupt.
> +
> +Required properties:
> +- compatible: should be one of the following
> +   - "ti,twl4030-pwrbutton": For controllers compatible with twl4030
> +- interrupt: should be one of the following

Spelling: s/interrupt/interrupts/

> +   - <8>: For controllers compatible with twl4030
> +
> +Example:
> +
> +&twl {
> +	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),

Remove the __exit_p() wrapper. __exit is for module exit functions, not
remove hooks (I know, that's not actually this patch, but the code is
definitely wrong here).

>  	.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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv6 2/3] Input: twl4030-pwrbutton: use dev_err for errors

[Aaro Koskinen]

Hi,

On Thu, Oct 24, 2013 at 04:48:45PM +0200, Sebastian Reichel wrote:
> Use dev_err() to output errors instead of dev_dbg().
> 
> Signed-off-by: Sebastian Reichel <sre@debian.org>

Reviewed-by: Aaro Koskinen <aaro.koskinen@iki.fi>

> ---
>  drivers/input/misc/twl4030-pwrbutton.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/input/misc/twl4030-pwrbutton.c b/drivers/input/misc/twl4030-pwrbutton.c
> index a3a0fe3..48639ff 100644
> --- a/drivers/input/misc/twl4030-pwrbutton.c
> +++ b/drivers/input/misc/twl4030-pwrbutton.c
> @@ -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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv6 3/3] Input: twl4030-pwrbutton: simplify driver using devm_*

[Aaro Koskinen]

On Thu, Oct 24, 2013 at 04:48:46PM +0200, Sebastian Reichel wrote:
> Use managed irq resource to simplify the driver.
> 
> Signed-off-by: Sebastian Reichel <sre@debian.org>

Reviewed-by: Aaro Koskinen <aaro.koskinen@iki.fi>

> ---
>  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 48639ff..be1759c 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_dbg(&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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv6 1/3] Input: twl4030-pwrbutton - add device tree support

[Kumar Gala]

On Oct 24, 2013, at 9:48 AM, 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 | 21 +++++++++++++++++++++
> drivers/input/misc/twl4030-pwrbutton.c              | 16 ++++++++++++----
> 2 files changed, 33 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..4375646
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/twl4030-pwrbutton.txt
> @@ -0,0 +1,21 @@
> +Texas Instruments TWL family (twl4030) pwrbutton module
> +
> +This module is part of the TWL4030. For more details about the whole
> +chip see Documentation/devicetree/bindings/mfd/twl-familly.txt.
> +
> +This module provides a simple power button event via an Interrupt.
> +
> +Required 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

Just checking, but the interrupt is always 8 for this device?

> +
> +Example:
> +
> +&twl {
> +	twl_pwrbutton: pwrbutton {
> +		compatible = "ti,twl4030-pwrbutton";
> +		interrupts = <8>;
> +	};
> +};

Otherwise Ack on binding:

Acked-by: Kumar Gala <galak@codeaurora.org>

- k

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCHv6 1/3] Input: twl4030-pwrbutton - add device tree support

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 369 bytes --]

On Fri, Oct 25, 2013 at 04:41:20PM -0500, Kumar Gala wrote:
> On Oct 24, 2013, at 9:48 AM, Sebastian Reichel wrote:
> > +- interrupt: should be one of the following
> > +   - <8>: For controllers compatible with twl4030
> 
> Just checking, but the interrupt is always 8 for this device?

Yes. It's currently hardcoded in drivers/mfd/twl-core.c.

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv6 1/3] Input: twl4030-pwrbutton - add device tree support

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1557 bytes --]

On Fri, Oct 25, 2013 at 08:09:04PM +0100, Grant Likely wrote:
> On Thu, 24 Oct 2013 16:48:44 +0200, Sebastian Reichel <sre@debian.org> wrote:
> > Add device tree support for twl4030 power button driver.
> 
> The above commit text is insufficient. There are changes in the patch
> that aren't described here and have nothing to do with device tree
> bindings.

I will update the description in PATCHv7.

> [...]
> > +++ b/Documentation/devicetree/bindings/input/twl4030-pwrbutton.txt
> 
> Can all of the TWL or TWL4030 funciton bindings be collected into a
> single file please? It is a single device after all. All of it should be
> in bindings/mfd/twl-family.txt

I guess this should be done in another patch? There's also a typo in
twl-family.txt's filename. My suggestion is to leave the patchset in
its current state. I will create another patch, which combines all
the twl4030 bindings descriptions into one file.

> [...]
> > +- interrupt: should be one of the following
> 
> Spelling: s/interrupt/interrupts/

fixed.

> [...]
> >  static struct platform_driver twl4030_pwrbutton_driver = {
> > +	.probe		= twl4030_pwrbutton_probe,
> >  	.remove		= __exit_p(twl4030_pwrbutton_remove),
> 
> Remove the __exit_p() wrapper. __exit is for module exit functions, not
> remove hooks (I know, that's not actually this patch, but the code is
> definitely wrong here).

On of the following patches converts the driver to devm, which
results in complete removal of the twl4030_pwrbutton_remove
function.

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv6 1/3] Input: twl4030-pwrbutton - add device tree support

[Kumar Gala]

On Oct 25, 2013, at 5:18 PM, Sebastian Reichel wrote:

> On Fri, Oct 25, 2013 at 04:41:20PM -0500, Kumar Gala wrote:
>> On Oct 24, 2013, at 9:48 AM, Sebastian Reichel wrote:
>>> +- interrupt: should be one of the following
>>> +   - <8>: For controllers compatible with twl4030
>> 
>> Just checking, but the interrupt is always 8 for this device?
> 
> Yes. It's currently hardcoded in drivers/mfd/twl-core.c.

The fact that is hard coded in the driver does not imply that it should be in the device tree binding.  Is there an interrupt controller as part of the TWL4030?

- k

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Re: [PATCHv6 1/3] Input: twl4030-pwrbutton - add device tree support

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1173 bytes --]

On Sat, Oct 26, 2013 at 01:37:57AM -0500, Kumar Gala wrote:
> 
> On Oct 25, 2013, at 5:18 PM, Sebastian Reichel wrote:
> 
> > On Fri, Oct 25, 2013 at 04:41:20PM -0500, Kumar Gala wrote:
> >> On Oct 24, 2013, at 9:48 AM, Sebastian Reichel wrote:
> >>> +- interrupt: should be one of the following
> >>> +   - <8>: For controllers compatible with twl4030
> >> 
> >> Just checking, but the interrupt is always 8 for this device?
> > 
> > Yes. It's currently hardcoded in drivers/mfd/twl-core.c.
> 
> The fact that is hard coded in the driver does not imply that it
> should be in the device tree binding.  Is there an interrupt
> controller as part of the TWL4030?

Hardware looks like this:

&twl4030 {
    compatible = "ti,twl4030";
	interrupt-controller;
	#interrupt-cells = <1>;

    twl_pwrbutton: pwrbutton {
		compatible = "ti,twl4030-pwrbutton";
		interrupts = <8>; /* 8th interrupt from the twl4030 */
	};
};

Simplified the initialization of twl4030 stuff works
like this for non DT boot:

twl4030_init(...) {
    init_subdev(...);
    init_subdev("twl4030-pwrbutton", ..., irq=8, ...);
    init_subdev(...);
};

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv6 1/3] Input: twl4030-pwrbutton - add device tree support

[Grant Likely]

On Sat, 26 Oct 2013 01:40:31 +0200, Sebastian Reichel <sre@debian.org> wrote:
> On Fri, Oct 25, 2013 at 08:09:04PM +0100, Grant Likely wrote:
> > On Thu, 24 Oct 2013 16:48:44 +0200, Sebastian Reichel <sre@debian.org> wrote:
> > > Add device tree support for twl4030 power button driver.
> > 
> > The above commit text is insufficient. There are changes in the patch
> > that aren't described here and have nothing to do with device tree
> > bindings.
> 
> I will update the description in PATCHv7.
> 
> > [...]
> > > +++ b/Documentation/devicetree/bindings/input/twl4030-pwrbutton.txt
> > 
> > Can all of the TWL or TWL4030 funciton bindings be collected into a
> > single file please? It is a single device after all. All of it should be
> > in bindings/mfd/twl-family.txt
> 
> I guess this should be done in another patch? There's also a typo in
> twl-family.txt's filename. My suggestion is to leave the patchset in
> its current state. I will create another patch, which combines all
> the twl4030 bindings descriptions into one file.

Yes, that is fine.

> 
> > [...]
> > > +- interrupt: should be one of the following
> > 
> > Spelling: s/interrupt/interrupts/
> 
> fixed.
> 
> > [...]
> > >  static struct platform_driver twl4030_pwrbutton_driver = {
> > > +	.probe		= twl4030_pwrbutton_probe,
> > >  	.remove		= __exit_p(twl4030_pwrbutton_remove),
> > 
> > Remove the __exit_p() wrapper. __exit is for module exit functions, not
> > remove hooks (I know, that's not actually this patch, but the code is
> > definitely wrong here).
> 
> On of the following patches converts the driver to devm, which
> results in complete removal of the twl4030_pwrbutton_remove
> function.

Okay.

g.

Re: [PATCHv6 1/3] Input: twl4030-pwrbutton - add device tree support

[Kumar Gala]

On Oct 26, 2013, at 6:31 AM, Sebastian Reichel wrote:

> On Sat, Oct 26, 2013 at 01:37:57AM -0500, Kumar Gala wrote:
>> 
>> On Oct 25, 2013, at 5:18 PM, Sebastian Reichel wrote:
>> 
>>> On Fri, Oct 25, 2013 at 04:41:20PM -0500, Kumar Gala wrote:
>>>> On Oct 24, 2013, at 9:48 AM, Sebastian Reichel wrote:
>>>>> +- interrupt: should be one of the following
>>>>> +   - <8>: For controllers compatible with twl4030
>>>> 
>>>> Just checking, but the interrupt is always 8 for this device?
>>> 
>>> Yes. It's currently hardcoded in drivers/mfd/twl-core.c.
>> 
>> The fact that is hard coded in the driver does not imply that it
>> should be in the device tree binding.  Is there an interrupt
>> controller as part of the TWL4030?
> 
> Hardware looks like this:
> 
> &twl4030 {
>    compatible = "ti,twl4030";
> 	interrupt-controller;
> 	#interrupt-cells = <1>;
> 
>    twl_pwrbutton: pwrbutton {
> 		compatible = "ti,twl4030-pwrbutton";
> 		interrupts = <8>; /* 8th interrupt from the twl4030 */
> 	};
> };
> 
> Simplified the initialization of twl4030 stuff works
> like this for non DT boot:
> 
> twl4030_init(...) {
>    init_subdev(...);
>    init_subdev("twl4030-pwrbutton", ..., irq=8, ...);
>    init_subdev(...);
> };
> 
> -- Sebastian

ok, than other than Grant's comment about merging some of this together with the other twl4030 bindings, ack.

- k

-- 
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation