Changes from v4:
- Added new dt-binding patch as 1/5
Changes from v3:
- Fixed dereference issue in 2/4
- Added Stephen's reviewed by tag for 2/4
Changes from v2:
- Addressed Stephen's comments
- Add Stephen's reviewed by tag for 1/4
- Fixed style for 2/4
- Corrected function call to use correct function for 3/4
Changes from v1:
- Removed Change-Id from all patches
- Updated subject line of cover letter
- Addressed Stephen's comments for v1 1/3
- Separated error message fix to own patch (v2 1/4)
- Separated PON GEN3 base address changes to own patch (v2 2/4)
- Added new variables and functions to make code more readable
- Removed v1 3/3 as per Bjorn's comments
Anjelique Melendez (2):
input: misc: pm8941-pwrkey: fix error message
input: misc: pm8941-pwrkey: add support for PON GEN3 base addresses
David Collins (3):
dt-bindings: power: reset: qcom-pon: update "reg" property details
input: misc: pm8941-pwrkey: add software key press debouncing support
input: misc: pm8941-pwrkey: simulate missed key press events
Documentation/devicetree/bindings/power/reset/qcom,pon.yaml | 20 +++++++++++++++++++-
drivers/input/misc/pm8941-pwrkey.c | 124 +++++++++++++++++++++++++----
2 files changed, 129 insertions(+), 15 deletions(-)
--
2.35.1
Currently, error message reads "failed to set debounce". However,
code is attempting to read revision not set debounce. Fix this.
Signed-off-by: Anjelique Melendez <[email protected]>
Reviewed-by: Stephen Boyd <[email protected]>
---
drivers/input/misc/pm8941-pwrkey.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/misc/pm8941-pwrkey.c b/drivers/input/misc/pm8941-pwrkey.c
index 33609603245d..e0240db12d4f 100644
--- a/drivers/input/misc/pm8941-pwrkey.c
+++ b/drivers/input/misc/pm8941-pwrkey.c
@@ -217,7 +217,7 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
error = regmap_read(pwrkey->regmap, pwrkey->baseaddr + PON_REV2,
&pwrkey->revision);
if (error) {
- dev_err(&pdev->dev, "failed to set debounce: %d\n", error);
+ dev_err(&pdev->dev, "failed to read revision: %d\n", error);
return error;
}
--
2.35.1
Currently, PON address is read from the "reg" property. For PON GEN3,
which starts with PMK8350, the "reg" property will have both the PON
HLOS and PON PBS addesses defined. Add support so that all PON
generations can be configured.
Signed-off-by: Anjelique Melendez <[email protected]>
Reviewed-by: Stephen Boyd <[email protected]>
---
drivers/input/misc/pm8941-pwrkey.c | 31 +++++++++++++++++++++++-------
1 file changed, 24 insertions(+), 7 deletions(-)
diff --git a/drivers/input/misc/pm8941-pwrkey.c b/drivers/input/misc/pm8941-pwrkey.c
index e0240db12d4f..43106e4cfd23 100644
--- a/drivers/input/misc/pm8941-pwrkey.c
+++ b/drivers/input/misc/pm8941-pwrkey.c
@@ -12,6 +12,7 @@
#include <linux/log2.h>
#include <linux/module.h>
#include <linux/of.h>
+#include <linux/of_address.h>
#include <linux/of_device.h>
#include <linux/platform_device.h>
#include <linux/reboot.h>
@@ -44,6 +45,7 @@ struct pm8941_data {
unsigned int status_bit;
bool supports_ps_hold_poff_config;
bool supports_debounce_config;
+ bool has_pon_pbs;
const char *name;
const char *phys;
};
@@ -52,6 +54,7 @@ struct pm8941_pwrkey {
struct device *dev;
int irq;
u32 baseaddr;
+ u32 pon_pbs_baseaddr;
struct regmap *regmap;
struct input_dev *input;
@@ -167,6 +170,8 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
struct pm8941_pwrkey *pwrkey;
bool pull_up;
struct device *parent;
+ struct device_node *regmap_node;
+ const __be32 *addr;
u32 req_delay;
int error;
@@ -188,8 +193,10 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
pwrkey->data = of_device_get_match_data(&pdev->dev);
parent = pdev->dev.parent;
+ regmap_node = pdev->dev.of_node;
pwrkey->regmap = dev_get_regmap(parent, NULL);
if (!pwrkey->regmap) {
+ regmap_node = parent->of_node;
/*
* We failed to get regmap for parent. Let's see if we are
* a child of pon node and read regmap and reg from its
@@ -200,15 +207,21 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
dev_err(&pdev->dev, "failed to locate regmap\n");
return -ENODEV;
}
+ }
- error = of_property_read_u32(parent->of_node,
- "reg", &pwrkey->baseaddr);
- } else {
- error = of_property_read_u32(pdev->dev.of_node, "reg",
- &pwrkey->baseaddr);
+ addr = of_get_address(regmap_node, 0, NULL, NULL);
+ if (!addr) {
+ dev_err(&pdev->dev, "reg property missing\n");
+ return -EINVAL;
+ }
+ pwrkey->baseaddr = be32_to_cpup(addr);
+
+ if (pwrkey->data->has_pon_pbs) {
+ /* PON_PBS base address is optional */
+ addr = of_get_address(regmap_node, 1, NULL, NULL);
+ if (addr)
+ pwrkey->pon_pbs_baseaddr = be32_to_cpup(addr);
}
- if (error)
- return error;
pwrkey->irq = platform_get_irq(pdev, 0);
if (pwrkey->irq < 0)
@@ -316,6 +329,7 @@ static const struct pm8941_data pwrkey_data = {
.phys = "pm8941_pwrkey/input0",
.supports_ps_hold_poff_config = true,
.supports_debounce_config = true,
+ .has_pon_pbs = false,
};
static const struct pm8941_data resin_data = {
@@ -325,6 +339,7 @@ static const struct pm8941_data resin_data = {
.phys = "pm8941_resin/input0",
.supports_ps_hold_poff_config = true,
.supports_debounce_config = true,
+ .has_pon_pbs = false,
};
static const struct pm8941_data pon_gen3_pwrkey_data = {
@@ -333,6 +348,7 @@ static const struct pm8941_data pon_gen3_pwrkey_data = {
.phys = "pmic_pwrkey/input0",
.supports_ps_hold_poff_config = false,
.supports_debounce_config = false,
+ .has_pon_pbs = true,
};
static const struct pm8941_data pon_gen3_resin_data = {
@@ -341,6 +357,7 @@ static const struct pm8941_data pon_gen3_resin_data = {
.phys = "pmic_resin/input0",
.supports_ps_hold_poff_config = false,
.supports_debounce_config = false,
+ .has_pon_pbs = true,
};
static const struct of_device_id pm8941_pwr_key_id_table[] = {
--
2.35.1
From: David Collins <[email protected]>
Update the description of "reg" property to add the PON_PBS base
address along with PON_HLOS base address. Also add "reg-names"
property description.
Signed-off-by: David Collins <[email protected]>
Signed-off-by: Anjelique Melendez <[email protected]>
---
.../bindings/power/reset/qcom,pon.yaml | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml b/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml
index 353f155d..542200b2 100644
--- a/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml
+++ b/Documentation/bindings/power/reset/qcom,pon.yaml
@@ -26,7 +26,25 @@ properties:
- qcom,pm8998-pon
reg:
- maxItems: 1
+ description: |
+ Specifies the SPMI base address for the PON (power-on) peripheral. For
+ PMICs that have the PON peripheral (GEN3) split into PON_HLOS and PON_PBS
+ (e.g. PMK8350), this can hold addresses of both PON_HLOS and PON_PBS
+ peripherals. In that case, the PON_PBS address needs to be specified to
+ facilitate software debouncing on some PMICs.
+ minItems: 1
+ maxItems: 2
+
+ reg-names:
+ description: |
+ For PON GEN1 and GEN2, it should be "pon". For PON GEN3 it should include
+ "pon_hlos" and optionally "pon_pbs".
+ minItems: 1
+ maxItems: 2
+ items:
+ - const: pon_hlos
+ - const: pon_pbs
+ - const: pon
pwrkey:
type: object
--
2.35.1
On Mon, Apr 11, 2022 at 01:05:03PM -0700, Anjelique Melendez wrote:
> From: David Collins <[email protected]>
>
> Update the description of "reg" property to add the PON_PBS base
> address along with PON_HLOS base address. Also add "reg-names"
> property description.
>
> Signed-off-by: David Collins <[email protected]>
> Signed-off-by: Anjelique Melendez <[email protected]>
> ---
> .../bindings/power/reset/qcom,pon.yaml | 20 +++++++++++++++++++-
> 1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml b/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml
> index 353f155d..542200b2 100644
> --- a/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml
> +++ b/Documentation/bindings/power/reset/qcom,pon.yaml
> @@ -26,7 +26,25 @@ properties:
> - qcom,pm8998-pon
>
> reg:
> - maxItems: 1
> + description: |
> + Specifies the SPMI base address for the PON (power-on) peripheral. For
> + PMICs that have the PON peripheral (GEN3) split into PON_HLOS and PON_PBS
> + (e.g. PMK8350), this can hold addresses of both PON_HLOS and PON_PBS
> + peripherals. In that case, the PON_PBS address needs to be specified to
> + facilitate software debouncing on some PMICs.
> + minItems: 1
> + maxItems: 2
> +
> + reg-names:
> + description: |
> + For PON GEN1 and GEN2, it should be "pon". For PON GEN3 it should include
> + "pon_hlos" and optionally "pon_pbs".
> + minItems: 1
> + maxItems: 2
> + items:
> + - const: pon_hlos
> + - const: pon_pbs
> + - const: pon
This says there are 3 entries, but you limited to 2. The schema also
doesn't match what the description says. Entries should be extended by
adding new entries to the end and keeping optional entries last. So like
this:
minItems: 1
items:
- const: pon
- const: pon_hlos
- const: pon_pbs
Rob
On 4/14/2022 8:34 AM, Rob Herring wrote:
> On Mon, Apr 11, 2022 at 01:05:03PM -0700, Anjelique Melendez wrote:
>> From: David Collins <[email protected]>
>>
>> Update the description of "reg" property to add the PON_PBS base
>> address along with PON_HLOS base address. Also add "reg-names"
>> property description.
>>
>> Signed-off-by: David Collins <[email protected]>
>> Signed-off-by: Anjelique Melendez <[email protected]>
>> ---
>> .../bindings/power/reset/qcom,pon.yaml | 20 +++++++++++++++++++-
>> 1 file changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml b/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml
>> index 353f155d..542200b2 100644
>> --- a/Documentation/devicetree/bindings/power/reset/qcom,pon.yaml
>> +++ b/Documentation/bindings/power/reset/qcom,pon.yaml
>> @@ -26,7 +26,25 @@ properties:
>> - qcom,pm8998-pon
>>
>> reg:
>> - maxItems: 1
>> + description: |
>> + Specifies the SPMI base address for the PON (power-on) peripheral. For
>> + PMICs that have the PON peripheral (GEN3) split into PON_HLOS and PON_PBS
>> + (e.g. PMK8350), this can hold addresses of both PON_HLOS and PON_PBS
>> + peripherals. In that case, the PON_PBS address needs to be specified to
>> + facilitate software debouncing on some PMICs.
>> + minItems: 1
>> + maxItems: 2
>> +
>> + reg-names:
>> + description: |
>> + For PON GEN1 and GEN2, it should be "pon". For PON GEN3 it should include
>> + "pon_hlos" and optionally "pon_pbs".
>> + minItems: 1
>> + maxItems: 2
>> + items:
>> + - const: pon_hlos
>> + - const: pon_pbs
>> + - const: pon
>
> This says there are 3 entries, but you limited to 2. The schema also
> doesn't match what the description says. Entries should be extended by
> adding new entries to the end and keeping optional entries last. So like
> this:
>
> minItems: 1
> items:
> - const: pon
> - const: pon_hlos
> - const: pon_pbs
>
> RobWill update in next patch. Thanks
On 4/14/2022 2:18 PM, Dmitry Baryshkov wrote:
> On 11/04/2022 23:05, Anjelique Melendez wrote:
>> Currently, PON address is read from the "reg" property. For PON GEN3,
>> which starts with PMK8350, the "reg" property will have both the PON
>> HLOS and PON PBS addesses defined. Add support so that all PON
>> generations can be configured.
>>
>> Signed-off-by: Anjelique Melendez <[email protected]>
>> Reviewed-by: Stephen Boyd <[email protected]>
>> ---
>> drivers/input/misc/pm8941-pwrkey.c | 31 +++++++++++++++++++++++-------
>> 1 file changed, 24 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/input/misc/pm8941-pwrkey.c b/drivers/input/misc/pm8941-pwrkey.c
>> index e0240db12d4f..43106e4cfd23 100644
>> --- a/drivers/input/misc/pm8941-pwrkey.c
>> +++ b/drivers/input/misc/pm8941-pwrkey.c
>> @@ -12,6 +12,7 @@
>> #include <linux/log2.h>
>> #include <linux/module.h>
>> #include <linux/of.h>
>> +#include <linux/of_address.h>
>> #include <linux/of_device.h>
>> #include <linux/platform_device.h>
>> #include <linux/reboot.h>
>> @@ -44,6 +45,7 @@ struct pm8941_data {
>> unsigned int status_bit;
>> bool supports_ps_hold_poff_config;
>> bool supports_debounce_config;
>> + bool has_pon_pbs;
>> const char *name;
>> const char *phys;
>> };
>> @@ -52,6 +54,7 @@ struct pm8941_pwrkey {
>> struct device *dev;
>> int irq;
>> u32 baseaddr;
>> + u32 pon_pbs_baseaddr;
>> struct regmap *regmap;
>> struct input_dev *input;
>> @@ -167,6 +170,8 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
>> struct pm8941_pwrkey *pwrkey;
>> bool pull_up;
>> struct device *parent;
>> + struct device_node *regmap_node;
>> + const __be32 *addr;
>> u32 req_delay;
>> int error;
>> @@ -188,8 +193,10 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
>> pwrkey->data = of_device_get_match_data(&pdev->dev);
>> parent = pdev->dev.parent;
>> + regmap_node = pdev->dev.of_node;
>> pwrkey->regmap = dev_get_regmap(parent, NULL);
>> if (!pwrkey->regmap) {
>> + regmap_node = parent->of_node;
>> /*
>> * We failed to get regmap for parent. Let's see if we are
>> * a child of pon node and read regmap and reg from its
>> @@ -200,15 +207,21 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
>> dev_err(&pdev->dev, "failed to locate regmap\n");
>> return -ENODEV;
>> }
>> + }
>> - error = of_property_read_u32(parent->of_node,
>> - "reg", &pwrkey->baseaddr);
>> - } else {
>> - error = of_property_read_u32(pdev->dev.of_node, "reg",
>> - &pwrkey->baseaddr);
>> + addr = of_get_address(regmap_node, 0, NULL, NULL);
>> + if (!addr) {
>> + dev_err(&pdev->dev, "reg property missing\n");
>> + return -EINVAL;
>> + }
>> + pwrkey->baseaddr = be32_to_cpup(addr);
>> +
>> + if (pwrkey->data->has_pon_pbs) {
>> + /* PON_PBS base address is optional */
>> + addr = of_get_address(regmap_node, 1, NULL, NULL);
>> + if (addr)
>> + pwrkey->pon_pbs_baseaddr = be32_to_cpup(addr);
>> }
>> - if (error)
>> - return error;
>> pwrkey->irq = platform_get_irq(pdev, 0);
>> if (pwrkey->irq < 0)
>> @@ -316,6 +329,7 @@ static const struct pm8941_data pwrkey_data = {
>> .phys = "pm8941_pwrkey/input0",
>> .supports_ps_hold_poff_config = true,
>> .supports_debounce_config = true,
>> + .has_pon_pbs = false,
>> };
>> static const struct pm8941_data resin_data = {
>> @@ -325,6 +339,7 @@ static const struct pm8941_data resin_data = {
>> .phys = "pm8941_resin/input0",
>> .supports_ps_hold_poff_config = true,
>> .supports_debounce_config = true,
>> + .has_pon_pbs = false,
>
> No need to declare that a field is false. Just skip this completely.
ACK. Will get rid in next version.
>
>> };
>> static const struct pm8941_data pon_gen3_pwrkey_data = {
>> @@ -333,6 +348,7 @@ static const struct pm8941_data pon_gen3_pwrkey_data = {
>> .phys = "pmic_pwrkey/input0",
>> .supports_ps_hold_poff_config = false,
>> .supports_debounce_config = false,
>> + .has_pon_pbs = true,
>> };
>> static const struct pm8941_data pon_gen3_resin_data = {
>> @@ -341,6 +357,7 @@ static const struct pm8941_data pon_gen3_resin_data = {
>> .phys = "pmic_resin/input0",
>> .supports_ps_hold_poff_config = false,
>> .supports_debounce_config = false,
>> + .has_pon_pbs = true,
>> };
>> static const struct of_device_id pm8941_pwr_key_id_table[] = {
>
>
On 11/04/2022 23:05, Anjelique Melendez wrote:
> Currently, PON address is read from the "reg" property. For PON GEN3,
> which starts with PMK8350, the "reg" property will have both the PON
> HLOS and PON PBS addesses defined. Add support so that all PON
> generations can be configured.
>
> Signed-off-by: Anjelique Melendez <[email protected]>
> Reviewed-by: Stephen Boyd <[email protected]>
> ---
> drivers/input/misc/pm8941-pwrkey.c | 31 +++++++++++++++++++++++-------
> 1 file changed, 24 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/input/misc/pm8941-pwrkey.c b/drivers/input/misc/pm8941-pwrkey.c
> index e0240db12d4f..43106e4cfd23 100644
> --- a/drivers/input/misc/pm8941-pwrkey.c
> +++ b/drivers/input/misc/pm8941-pwrkey.c
> @@ -12,6 +12,7 @@
> #include <linux/log2.h>
> #include <linux/module.h>
> #include <linux/of.h>
> +#include <linux/of_address.h>
> #include <linux/of_device.h>
> #include <linux/platform_device.h>
> #include <linux/reboot.h>
> @@ -44,6 +45,7 @@ struct pm8941_data {
> unsigned int status_bit;
> bool supports_ps_hold_poff_config;
> bool supports_debounce_config;
> + bool has_pon_pbs;
> const char *name;
> const char *phys;
> };
> @@ -52,6 +54,7 @@ struct pm8941_pwrkey {
> struct device *dev;
> int irq;
> u32 baseaddr;
> + u32 pon_pbs_baseaddr;
> struct regmap *regmap;
> struct input_dev *input;
>
> @@ -167,6 +170,8 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
> struct pm8941_pwrkey *pwrkey;
> bool pull_up;
> struct device *parent;
> + struct device_node *regmap_node;
> + const __be32 *addr;
> u32 req_delay;
> int error;
>
> @@ -188,8 +193,10 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
> pwrkey->data = of_device_get_match_data(&pdev->dev);
>
> parent = pdev->dev.parent;
> + regmap_node = pdev->dev.of_node;
> pwrkey->regmap = dev_get_regmap(parent, NULL);
> if (!pwrkey->regmap) {
> + regmap_node = parent->of_node;
> /*
> * We failed to get regmap for parent. Let's see if we are
> * a child of pon node and read regmap and reg from its
> @@ -200,15 +207,21 @@ static int pm8941_pwrkey_probe(struct platform_device *pdev)
> dev_err(&pdev->dev, "failed to locate regmap\n");
> return -ENODEV;
> }
> + }
>
> - error = of_property_read_u32(parent->of_node,
> - "reg", &pwrkey->baseaddr);
> - } else {
> - error = of_property_read_u32(pdev->dev.of_node, "reg",
> - &pwrkey->baseaddr);
> + addr = of_get_address(regmap_node, 0, NULL, NULL);
> + if (!addr) {
> + dev_err(&pdev->dev, "reg property missing\n");
> + return -EINVAL;
> + }
> + pwrkey->baseaddr = be32_to_cpup(addr);
> +
> + if (pwrkey->data->has_pon_pbs) {
> + /* PON_PBS base address is optional */
> + addr = of_get_address(regmap_node, 1, NULL, NULL);
> + if (addr)
> + pwrkey->pon_pbs_baseaddr = be32_to_cpup(addr);
> }
> - if (error)
> - return error;
>
> pwrkey->irq = platform_get_irq(pdev, 0);
> if (pwrkey->irq < 0)
> @@ -316,6 +329,7 @@ static const struct pm8941_data pwrkey_data = {
> .phys = "pm8941_pwrkey/input0",
> .supports_ps_hold_poff_config = true,
> .supports_debounce_config = true,
> + .has_pon_pbs = false,
> };
>
> static const struct pm8941_data resin_data = {
> @@ -325,6 +339,7 @@ static const struct pm8941_data resin_data = {
> .phys = "pm8941_resin/input0",
> .supports_ps_hold_poff_config = true,
> .supports_debounce_config = true,
> + .has_pon_pbs = false,
No need to declare that a field is false. Just skip this completely.
> };
>
> static const struct pm8941_data pon_gen3_pwrkey_data = {
> @@ -333,6 +348,7 @@ static const struct pm8941_data pon_gen3_pwrkey_data = {
> .phys = "pmic_pwrkey/input0",
> .supports_ps_hold_poff_config = false,
> .supports_debounce_config = false,
> + .has_pon_pbs = true,
> };
>
> static const struct pm8941_data pon_gen3_resin_data = {
> @@ -341,6 +357,7 @@ static const struct pm8941_data pon_gen3_resin_data = {
> .phys = "pmic_resin/input0",
> .supports_ps_hold_poff_config = false,
> .supports_debounce_config = false,
> + .has_pon_pbs = true,
> };
>
> static const struct of_device_id pm8941_pwr_key_id_table[] = {
--
With best wishes
Dmitry