linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] power: reset: reboot-mode: Make include file global
@ 2017-05-27  6:51 Bjorn Andersson
  2017-05-27  6:51 ` [PATCH 2/2] Input: pm8941-pwrkey: Introduce reboot mode support Bjorn Andersson
  2017-06-08 16:24 ` [PATCH 1/2] power: reset: reboot-mode: Make include file global Sebastian Reichel
  0 siblings, 2 replies; 11+ messages in thread
From: Bjorn Andersson @ 2017-05-27  6:51 UTC (permalink / raw)
  To: Sebastian Reichel, Dmitry Torokhov
  Cc: linux-pm, Rob Herring, John Stultz, linux-input, linux-kernel,
	linux-arm-msm

Move the reboot-mode.h include file into include/linux to allow drivers
outside drivers/power/reset to implement reboot-mode.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/power/reset/reboot-mode.c                    | 2 +-
 drivers/power/reset/syscon-reboot-mode.c             | 2 +-
 {drivers/power/reset => include/linux}/reboot-mode.h | 0
 3 files changed, 2 insertions(+), 2 deletions(-)
 rename {drivers/power/reset => include/linux}/reboot-mode.h (100%)

diff --git a/drivers/power/reset/reboot-mode.c b/drivers/power/reset/reboot-mode.c
index fb512183ace3..8f975ca0a8c4 100644
--- a/drivers/power/reset/reboot-mode.c
+++ b/drivers/power/reset/reboot-mode.c
@@ -13,7 +13,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/reboot.h>
-#include "reboot-mode.h"
+#include <linux/reboot-mode.h>
 
 #define PREFIX "mode-"
 
diff --git a/drivers/power/reset/syscon-reboot-mode.c b/drivers/power/reset/syscon-reboot-mode.c
index c8c371b285b1..563a97d7f73e 100644
--- a/drivers/power/reset/syscon-reboot-mode.c
+++ b/drivers/power/reset/syscon-reboot-mode.c
@@ -15,7 +15,7 @@
 #include <linux/reboot.h>
 #include <linux/regmap.h>
 #include <linux/mfd/syscon.h>
-#include "reboot-mode.h"
+#include <linux/reboot-mode.h>
 
 struct syscon_reboot_mode {
 	struct regmap *map;
diff --git a/drivers/power/reset/reboot-mode.h b/include/linux/reboot-mode.h
similarity index 100%
rename from drivers/power/reset/reboot-mode.h
rename to include/linux/reboot-mode.h
-- 
2.12.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/2] Input: pm8941-pwrkey: Introduce reboot mode support
  2017-05-27  6:51 [PATCH 1/2] power: reset: reboot-mode: Make include file global Bjorn Andersson
@ 2017-05-27  6:51 ` Bjorn Andersson
  2017-05-30  2:53   ` Dmitry Torokhov
  2017-06-08 16:24 ` [PATCH 1/2] power: reset: reboot-mode: Make include file global Sebastian Reichel
  1 sibling, 1 reply; 11+ messages in thread
From: Bjorn Andersson @ 2017-05-27  6:51 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Rob Herring, John Stultz, linux-input, linux-kernel,
	linux-arm-msm, Sebastian Reichel, linux-pm

In some Qualcomm platforms the magic for informing LK which mode to
reboot into is stored in the PON_SOFT_RB_SPARE register. Register with
the reboot mode helpers to expose this to the user.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/input/misc/Kconfig         |  1 +
 drivers/input/misc/pm8941-pwrkey.c | 28 ++++++++++++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 3872488c3fd7..56552e16fab1 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -132,6 +132,7 @@ config INPUT_PCSPKR
 config INPUT_PM8941_PWRKEY
 	tristate "Qualcomm PM8941 power key support"
 	depends on MFD_SPMI_PMIC
+	select REBOOT_MODE
 	help
 	  Say Y here if you want support for the power key usually found
 	  on boards using a Qualcomm PM8941 compatible PMIC.
diff --git a/drivers/input/misc/pm8941-pwrkey.c b/drivers/input/misc/pm8941-pwrkey.c
index 18ad956454f1..c48be6dbaa78 100644
--- a/drivers/input/misc/pm8941-pwrkey.c
+++ b/drivers/input/misc/pm8941-pwrkey.c
@@ -22,6 +22,7 @@
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/reboot.h>
+#include <linux/reboot-mode.h>
 #include <linux/regmap.h>
 
 #define PON_REV2			0x01
@@ -42,6 +43,7 @@
 #define PON_DBC_CTL			0x71
 #define  PON_DBC_DELAY_MASK		0x7
 
+#define PON_SOFT_RB_SPARE		0x8f
 
 struct pm8941_pwrkey {
 	struct device *dev;
@@ -52,8 +54,26 @@ struct pm8941_pwrkey {
 
 	unsigned int revision;
 	struct notifier_block reboot_notifier;
+
+	struct reboot_mode_driver reboot_mode;
 };
 
+static int pm8941_reboot_mode_write(struct reboot_mode_driver *reboot,
+				    unsigned int magic)
+{
+	struct pm8941_pwrkey *pwrkey = container_of(reboot, struct pm8941_pwrkey,
+						    reboot_mode);
+	int ret;
+
+	ret = regmap_update_bits(pwrkey->regmap,
+				 pwrkey->baseaddr + PON_SOFT_RB_SPARE,
+				 0xfc, magic << 2);
+	if (ret < 0)
+		dev_err(pwrkey->dev, "update reboot mode bits failed\n");
+
+	return ret;
+}
+
 static int pm8941_reboot_notify(struct notifier_block *nb,
 				unsigned long code, void *unused)
 {
@@ -256,6 +276,14 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
 		return error;
 	}
 
+	pwrkey->reboot_mode.dev = &pdev->dev;
+	pwrkey->reboot_mode.write = pm8941_reboot_mode_write;
+	error = devm_reboot_mode_register(&pdev->dev, &pwrkey->reboot_mode);
+	if (error) {
+		dev_err(&pdev->dev, "can't register reboot mode\n");
+		return error;
+	}
+
 	platform_set_drvdata(pdev, pwrkey);
 	device_init_wakeup(&pdev->dev, 1);
 
-- 
2.12.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] Input: pm8941-pwrkey: Introduce reboot mode support
  2017-05-27  6:51 ` [PATCH 2/2] Input: pm8941-pwrkey: Introduce reboot mode support Bjorn Andersson
@ 2017-05-30  2:53   ` Dmitry Torokhov
  2017-05-30  4:47     ` Bjorn Andersson
  0 siblings, 1 reply; 11+ messages in thread
From: Dmitry Torokhov @ 2017-05-30  2:53 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Rob Herring, John Stultz, linux-input, linux-kernel,
	linux-arm-msm, Sebastian Reichel, linux-pm

On Fri, May 26, 2017 at 11:51:30PM -0700, Bjorn Andersson wrote:
> In some Qualcomm platforms the magic for informing LK which mode to
> reboot into is stored in the PON_SOFT_RB_SPARE register. Register with
> the reboot mode helpers to expose this to the user.

Hmm, is the power key driver the best place to have this? WHy isn't this
a driver in its own right?

> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  drivers/input/misc/Kconfig         |  1 +
>  drivers/input/misc/pm8941-pwrkey.c | 28 ++++++++++++++++++++++++++++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index 3872488c3fd7..56552e16fab1 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -132,6 +132,7 @@ config INPUT_PCSPKR
>  config INPUT_PM8941_PWRKEY
>  	tristate "Qualcomm PM8941 power key support"
>  	depends on MFD_SPMI_PMIC
> +	select REBOOT_MODE
>  	help
>  	  Say Y here if you want support for the power key usually found
>  	  on boards using a Qualcomm PM8941 compatible PMIC.
> diff --git a/drivers/input/misc/pm8941-pwrkey.c b/drivers/input/misc/pm8941-pwrkey.c
> index 18ad956454f1..c48be6dbaa78 100644
> --- a/drivers/input/misc/pm8941-pwrkey.c
> +++ b/drivers/input/misc/pm8941-pwrkey.c
> @@ -22,6 +22,7 @@
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/reboot.h>
> +#include <linux/reboot-mode.h>
>  #include <linux/regmap.h>
>  
>  #define PON_REV2			0x01
> @@ -42,6 +43,7 @@
>  #define PON_DBC_CTL			0x71
>  #define  PON_DBC_DELAY_MASK		0x7
>  
> +#define PON_SOFT_RB_SPARE		0x8f
>  
>  struct pm8941_pwrkey {
>  	struct device *dev;
> @@ -52,8 +54,26 @@ struct pm8941_pwrkey {
>  
>  	unsigned int revision;
>  	struct notifier_block reboot_notifier;
> +
> +	struct reboot_mode_driver reboot_mode;
>  };
>  
> +static int pm8941_reboot_mode_write(struct reboot_mode_driver *reboot,
> +				    unsigned int magic)
> +{
> +	struct pm8941_pwrkey *pwrkey = container_of(reboot, struct pm8941_pwrkey,
> +						    reboot_mode);
> +	int ret;
> +
> +	ret = regmap_update_bits(pwrkey->regmap,
> +				 pwrkey->baseaddr + PON_SOFT_RB_SPARE,
> +				 0xfc, magic << 2);
> +	if (ret < 0)
> +		dev_err(pwrkey->dev, "update reboot mode bits failed\n");
> +
> +	return ret;
> +}
> +
>  static int pm8941_reboot_notify(struct notifier_block *nb,
>  				unsigned long code, void *unused)
>  {
> @@ -256,6 +276,14 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
>  		return error;
>  	}
>  
> +	pwrkey->reboot_mode.dev = &pdev->dev;
> +	pwrkey->reboot_mode.write = pm8941_reboot_mode_write;
> +	error = devm_reboot_mode_register(&pdev->dev, &pwrkey->reboot_mode);
> +	if (error) {
> +		dev_err(&pdev->dev, "can't register reboot mode\n");
> +		return error;
> +	}
> +
>  	platform_set_drvdata(pdev, pwrkey);
>  	device_init_wakeup(&pdev->dev, 1);
>  
> -- 
> 2.12.0
> 

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] Input: pm8941-pwrkey: Introduce reboot mode support
  2017-05-30  2:53   ` Dmitry Torokhov
@ 2017-05-30  4:47     ` Bjorn Andersson
  2017-06-08 16:32       ` Sebastian Reichel
  0 siblings, 1 reply; 11+ messages in thread
From: Bjorn Andersson @ 2017-05-30  4:47 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Rob Herring, John Stultz, linux-input, linux-kernel,
	linux-arm-msm, Sebastian Reichel, linux-pm

On Mon 29 May 19:53 PDT 2017, Dmitry Torokhov wrote:

> On Fri, May 26, 2017 at 11:51:30PM -0700, Bjorn Andersson wrote:
> > In some Qualcomm platforms the magic for informing LK which mode to
> > reboot into is stored in the PON_SOFT_RB_SPARE register. Register with
> > the reboot mode helpers to expose this to the user.
> 
> Hmm, is the power key driver the best place to have this? WHy isn't this
> a driver in its own right?
> 

The functionality is part of the "PON" block in the Qualcomm PMICs,
other functionality from this block relates to configuration and
handling related to power-key and reset-key.

Several of these properties are intermingled, so I do believe it's best
to handle them in a single driver; that said, it might no longer be
correct to name the driver "pwrkey" or that it is a "misc input" driver.

Regards,
Bjorn

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] power: reset: reboot-mode: Make include file global
  2017-05-27  6:51 [PATCH 1/2] power: reset: reboot-mode: Make include file global Bjorn Andersson
  2017-05-27  6:51 ` [PATCH 2/2] Input: pm8941-pwrkey: Introduce reboot mode support Bjorn Andersson
@ 2017-06-08 16:24 ` Sebastian Reichel
  1 sibling, 0 replies; 11+ messages in thread
From: Sebastian Reichel @ 2017-06-08 16:24 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Dmitry Torokhov, linux-pm, Rob Herring, John Stultz, linux-input,
	linux-kernel, linux-arm-msm

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

Hi,

On Fri, May 26, 2017 at 11:51:29PM -0700, Bjorn Andersson wrote:
> Move the reboot-mode.h include file into include/linux to allow drivers
> outside drivers/power/reset to implement reboot-mode.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Thanks, queued. I also created an immutable branch:

git://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git ib-psy-reboot-mode-4.13

-- Sebastian

> ---
>  drivers/power/reset/reboot-mode.c                    | 2 +-
>  drivers/power/reset/syscon-reboot-mode.c             | 2 +-
>  {drivers/power/reset => include/linux}/reboot-mode.h | 0
>  3 files changed, 2 insertions(+), 2 deletions(-)
>  rename {drivers/power/reset => include/linux}/reboot-mode.h (100%)
> 
> diff --git a/drivers/power/reset/reboot-mode.c b/drivers/power/reset/reboot-mode.c
> index fb512183ace3..8f975ca0a8c4 100644
> --- a/drivers/power/reset/reboot-mode.c
> +++ b/drivers/power/reset/reboot-mode.c
> @@ -13,7 +13,7 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/reboot.h>
> -#include "reboot-mode.h"
> +#include <linux/reboot-mode.h>
>  
>  #define PREFIX "mode-"
>  
> diff --git a/drivers/power/reset/syscon-reboot-mode.c b/drivers/power/reset/syscon-reboot-mode.c
> index c8c371b285b1..563a97d7f73e 100644
> --- a/drivers/power/reset/syscon-reboot-mode.c
> +++ b/drivers/power/reset/syscon-reboot-mode.c
> @@ -15,7 +15,7 @@
>  #include <linux/reboot.h>
>  #include <linux/regmap.h>
>  #include <linux/mfd/syscon.h>
> -#include "reboot-mode.h"
> +#include <linux/reboot-mode.h>
>  
>  struct syscon_reboot_mode {
>  	struct regmap *map;
> diff --git a/drivers/power/reset/reboot-mode.h b/include/linux/reboot-mode.h
> similarity index 100%
> rename from drivers/power/reset/reboot-mode.h
> rename to include/linux/reboot-mode.h
> -- 
> 2.12.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] Input: pm8941-pwrkey: Introduce reboot mode support
  2017-05-30  4:47     ` Bjorn Andersson
@ 2017-06-08 16:32       ` Sebastian Reichel
  2017-06-12 23:32         ` Bjorn Andersson
  0 siblings, 1 reply; 11+ messages in thread
From: Sebastian Reichel @ 2017-06-08 16:32 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Dmitry Torokhov, Rob Herring, John Stultz, linux-input,
	linux-kernel, linux-arm-msm, linux-pm

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

Hi,

On Mon, May 29, 2017 at 09:47:11PM -0700, Bjorn Andersson wrote:
> On Mon 29 May 19:53 PDT 2017, Dmitry Torokhov wrote:
> 
> > On Fri, May 26, 2017 at 11:51:30PM -0700, Bjorn Andersson wrote:
> > > In some Qualcomm platforms the magic for informing LK which mode to
> > > reboot into is stored in the PON_SOFT_RB_SPARE register. Register with
> > > the reboot mode helpers to expose this to the user.
> > 
> > Hmm, is the power key driver the best place to have this? WHy isn't this
> > a driver in its own right?
> > 
> 
> The functionality is part of the "PON" block in the Qualcomm PMICs,
> other functionality from this block relates to configuration and
> handling related to power-key and reset-key.
> 
> Several of these properties are intermingled, so I do believe it's best
> to handle them in a single driver; that said, it might no longer be
> correct to name the driver "pwrkey" or that it is a "misc input" driver.

I merged patch 1 and provided an immutable branch, so
that this could go through the input subsystem.

To me it doesn't look that intermingled, though. I think
the reboot and reboot-mode parts could go into their own
driver in drivers/power/reset.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] Input: pm8941-pwrkey: Introduce reboot mode support
  2017-06-08 16:32       ` Sebastian Reichel
@ 2017-06-12 23:32         ` Bjorn Andersson
  2017-06-15 16:26           ` Sebastian Reichel
  0 siblings, 1 reply; 11+ messages in thread
From: Bjorn Andersson @ 2017-06-12 23:32 UTC (permalink / raw)
  To: Sebastian Reichel
  Cc: Dmitry Torokhov, Rob Herring, John Stultz, linux-input,
	linux-kernel, linux-arm-msm, linux-pm

On Thu 08 Jun 09:32 PDT 2017, Sebastian Reichel wrote:

> Hi,
> 
> On Mon, May 29, 2017 at 09:47:11PM -0700, Bjorn Andersson wrote:
> > On Mon 29 May 19:53 PDT 2017, Dmitry Torokhov wrote:
> > 
> > > On Fri, May 26, 2017 at 11:51:30PM -0700, Bjorn Andersson wrote:
> > > > In some Qualcomm platforms the magic for informing LK which mode to
> > > > reboot into is stored in the PON_SOFT_RB_SPARE register. Register with
> > > > the reboot mode helpers to expose this to the user.
> > > 
> > > Hmm, is the power key driver the best place to have this? WHy isn't this
> > > a driver in its own right?
> > > 
> > 
> > The functionality is part of the "PON" block in the Qualcomm PMICs,
> > other functionality from this block relates to configuration and
> > handling related to power-key and reset-key.
> > 
> > Several of these properties are intermingled, so I do believe it's best
> > to handle them in a single driver; that said, it might no longer be
> > correct to name the driver "pwrkey" or that it is a "misc input" driver.
> 
> I merged patch 1 and provided an immutable branch, so
> that this could go through the input subsystem.
> 

Thanks

> To me it doesn't look that intermingled, though. I think
> the reboot and reboot-mode parts could go into their own
> driver in drivers/power/reset.
> 

I did reach out to Rob regarding this and the single hardware block
should be described by a single node in DeviceTree.

As such if we split the non-input related handling into another driver
we would need to make the input driver create a subdevice during probe -
or create a new pon-driver with a new compatible that internally spawns
the pwrkey driver. Neither seems desirable to me...


The features of the PON block not yet shown on LKML are status registers
to indicate the reason for powering up the PMIC and a watchdog (which I
don't believe is used or exposed today).

Regards,
Bjorn

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] Input: pm8941-pwrkey: Introduce reboot mode support
  2017-06-12 23:32         ` Bjorn Andersson
@ 2017-06-15 16:26           ` Sebastian Reichel
  2017-06-15 18:33             ` Bjorn Andersson
  0 siblings, 1 reply; 11+ messages in thread
From: Sebastian Reichel @ 2017-06-15 16:26 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Dmitry Torokhov, Rob Herring, John Stultz, linux-input,
	linux-kernel, linux-arm-msm, linux-pm

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

Hi,

On Mon, Jun 12, 2017 at 04:32:03PM -0700, Bjorn Andersson wrote:
> On Thu 08 Jun 09:32 PDT 2017, Sebastian Reichel wrote:
> 
> > Hi,
> > 
> > On Mon, May 29, 2017 at 09:47:11PM -0700, Bjorn Andersson wrote:
> > > On Mon 29 May 19:53 PDT 2017, Dmitry Torokhov wrote:
> > > 
> > > > On Fri, May 26, 2017 at 11:51:30PM -0700, Bjorn Andersson wrote:
> > > > > In some Qualcomm platforms the magic for informing LK which mode to
> > > > > reboot into is stored in the PON_SOFT_RB_SPARE register. Register with
> > > > > the reboot mode helpers to expose this to the user.
> > > > 
> > > > Hmm, is the power key driver the best place to have this? WHy isn't this
> > > > a driver in its own right?
> > > > 
> > > 
> > > The functionality is part of the "PON" block in the Qualcomm PMICs,
> > > other functionality from this block relates to configuration and
> > > handling related to power-key and reset-key.
> > > 
> > > Several of these properties are intermingled, so I do believe it's best
> > > to handle them in a single driver; that said, it might no longer be
> > > correct to name the driver "pwrkey" or that it is a "misc input" driver.
> > 
> > I merged patch 1 and provided an immutable branch, so
> > that this could go through the input subsystem.
> > 
> 
> Thanks
> 
> > To me it doesn't look that intermingled, though. I think
> > the reboot and reboot-mode parts could go into their own
> > driver in drivers/power/reset.
> > 
> 
> I did reach out to Rob regarding this and the single hardware block
> should be described by a single node in DeviceTree.
> 
> As such if we split the non-input related handling into another driver
> we would need to make the input driver create a subdevice during probe -
> or create a new pon-driver with a new compatible that internally spawns
> the pwrkey driver. Neither seems desirable to me...

The pon-driver would have been the proper solution, but with the
binding already being defined that's no longer a nice option :(

> The features of the PON block not yet shown on LKML are status registers
> to indicate the reason for powering up the PMIC and a watchdog (which I
> don't believe is used or exposed today).

So we have a block, which has watchdog, powerdown, reboot, boot-reason,
reboot-mode and power key. To me that does not look like it should be
one driver.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] Input: pm8941-pwrkey: Introduce reboot mode support
  2017-06-15 16:26           ` Sebastian Reichel
@ 2017-06-15 18:33             ` Bjorn Andersson
  2017-06-15 21:38               ` Rob Herring
  0 siblings, 1 reply; 11+ messages in thread
From: Bjorn Andersson @ 2017-06-15 18:33 UTC (permalink / raw)
  To: Sebastian Reichel
  Cc: Dmitry Torokhov, Rob Herring, John Stultz, linux-input,
	linux-kernel, linux-arm-msm, linux-pm

On Thu 15 Jun 09:26 PDT 2017, Sebastian Reichel wrote:

> Hi,
> 
> On Mon, Jun 12, 2017 at 04:32:03PM -0700, Bjorn Andersson wrote:
[..]
> > As such if we split the non-input related handling into another driver
> > we would need to make the input driver create a subdevice during probe -
> > or create a new pon-driver with a new compatible that internally spawns
> > the pwrkey driver. Neither seems desirable to me...
> 
> The pon-driver would have been the proper solution, but with the
> binding already being defined that's no longer a nice option :(
> 

We have a binding for the "qcom,pm8941-pwrkey", but as long as we
maintain the compatibility in the input driver with this we could come
up with a new binding for the "pon" block.

> > The features of the PON block not yet shown on LKML are status registers
> > to indicate the reason for powering up the PMIC and a watchdog (which I
> > don't believe is used or exposed today).
> 
> So we have a block, which has watchdog, powerdown, reboot, boot-reason,
> reboot-mode and power key. To me that does not look like it should be
> one driver.
> 

Unfortunately I do agree with this.

It would make sense to describe the pon in a single DT-node and have a
pon-driver spawning off individual driver for each functionality. That
way we get a clean representation in DT and we get clean implementation
of each component...

Regards,
Bjorn

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] Input: pm8941-pwrkey: Introduce reboot mode support
  2017-06-15 18:33             ` Bjorn Andersson
@ 2017-06-15 21:38               ` Rob Herring
  2017-06-20 14:27                 ` Sebastian Reichel
  0 siblings, 1 reply; 11+ messages in thread
From: Rob Herring @ 2017-06-15 21:38 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Sebastian Reichel, Dmitry Torokhov, John Stultz, linux-input,
	linux-kernel, linux-arm-msm, linux-pm

On Thu, Jun 15, 2017 at 1:33 PM, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
> On Thu 15 Jun 09:26 PDT 2017, Sebastian Reichel wrote:
>
>> Hi,
>>
>> On Mon, Jun 12, 2017 at 04:32:03PM -0700, Bjorn Andersson wrote:
> [..]
>> > As such if we split the non-input related handling into another driver
>> > we would need to make the input driver create a subdevice during probe -
>> > or create a new pon-driver with a new compatible that internally spawns
>> > the pwrkey driver. Neither seems desirable to me...
>>
>> The pon-driver would have been the proper solution, but with the
>> binding already being defined that's no longer a nice option :(
>>
>
> We have a binding for the "qcom,pm8941-pwrkey", but as long as we
> maintain the compatibility in the input driver with this we could come
> up with a new binding for the "pon" block.

Yes. My only objection was having the pon node with child devices
purely because that is how Linux splits the driver.

>> > The features of the PON block not yet shown on LKML are status registers
>> > to indicate the reason for powering up the PMIC and a watchdog (which I
>> > don't believe is used or exposed today).
>>
>> So we have a block, which has watchdog, powerdown, reboot, boot-reason,
>> reboot-mode and power key. To me that does not look like it should be
>> one driver.
>>
>
> Unfortunately I do agree with this.

As do I. But that is a separate decision from DT bindings.

> It would make sense to describe the pon in a single DT-node and have a
> pon-driver spawning off individual driver for each functionality. That
> way we get a clean representation in DT and we get clean implementation
> of each component...

My objection here is we should not just create child nodes to align
with current Linux driver needs. That said, child nodes do sometimes
make sense. If, for example, the key function was not fixed and you
needed to configure what key function is used, then probably a child
node makes sense. It's always possible to add child nodes later
without breaking compatibility. It's hard to remove them.

Rob

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] Input: pm8941-pwrkey: Introduce reboot mode support
  2017-06-15 21:38               ` Rob Herring
@ 2017-06-20 14:27                 ` Sebastian Reichel
  0 siblings, 0 replies; 11+ messages in thread
From: Sebastian Reichel @ 2017-06-20 14:27 UTC (permalink / raw)
  To: Rob Herring
  Cc: Bjorn Andersson, Dmitry Torokhov, John Stultz, linux-input,
	linux-kernel, linux-arm-msm, linux-pm

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

Hi,

On Thu, Jun 15, 2017 at 04:38:34PM -0500, Rob Herring wrote:
> On Thu, Jun 15, 2017 at 1:33 PM, Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> > On Thu 15 Jun 09:26 PDT 2017, Sebastian Reichel wrote:
> >
> >> Hi,
> >>
> >> On Mon, Jun 12, 2017 at 04:32:03PM -0700, Bjorn Andersson wrote:
> > [..]
> >> > As such if we split the non-input related handling into another driver
> >> > we would need to make the input driver create a subdevice during probe -
> >> > or create a new pon-driver with a new compatible that internally spawns
> >> > the pwrkey driver. Neither seems desirable to me...
> >>
> >> The pon-driver would have been the proper solution, but with the
> >> binding already being defined that's no longer a nice option :(
> >>
> >
> > We have a binding for the "qcom,pm8941-pwrkey", but as long as we
> > maintain the compatibility in the input driver with this we could come
> > up with a new binding for the "pon" block.
> 
> Yes. My only objection was having the pon node with child devices
> purely because that is how Linux splits the driver.


So maybe 'compatible = "qcom,pm8041-pon", "qcom,pm8941-pwrkey";'
with pm8941-pwrkey being marked as deprecated in the binding?

> >> > The features of the PON block not yet shown on LKML are status registers
> >> > to indicate the reason for powering up the PMIC and a watchdog (which I
> >> > don't believe is used or exposed today).
> >>
> >> So we have a block, which has watchdog, powerdown, reboot, boot-reason,
> >> reboot-mode and power key. To me that does not look like it should be
> >> one driver.
> >>
> >
> > Unfortunately I do agree with this.
> 
> As do I. But that is a separate decision from DT bindings.

not much of a discussion if everyone agrees? :)

> > It would make sense to describe the pon in a single DT-node and have a
> > pon-driver spawning off individual driver for each functionality. That
> > way we get a clean representation in DT and we get clean implementation
> > of each component...
> 
> My objection here is we should not just create child nodes to align
> with current Linux driver needs.

Ack.

> That said, child nodes do sometimes make sense. If, for example,
> the key function was not fixed and you needed to configure what
> key function is used, then probably a child node makes sense.

Ack.

> It's always possible to add child nodes later without breaking
> compatibility. It's hard to remove them.

I don't think so. Adding a child node means, that we want to do
something based on the existance of this child node. So something,
that worked without the child node before no longer works
afterwards.

OTOH removing a child node means, that the child node is not needed
and stuff will work without it => system still works.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2017-06-20 14:27 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-27  6:51 [PATCH 1/2] power: reset: reboot-mode: Make include file global Bjorn Andersson
2017-05-27  6:51 ` [PATCH 2/2] Input: pm8941-pwrkey: Introduce reboot mode support Bjorn Andersson
2017-05-30  2:53   ` Dmitry Torokhov
2017-05-30  4:47     ` Bjorn Andersson
2017-06-08 16:32       ` Sebastian Reichel
2017-06-12 23:32         ` Bjorn Andersson
2017-06-15 16:26           ` Sebastian Reichel
2017-06-15 18:33             ` Bjorn Andersson
2017-06-15 21:38               ` Rob Herring
2017-06-20 14:27                 ` Sebastian Reichel
2017-06-08 16:24 ` [PATCH 1/2] power: reset: reboot-mode: Make include file global Sebastian Reichel

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).