[PATCH v2 0/1] Input: snvs_pwrkey - add configurable force shutdown time

[PATCH v2 0/1] Input: snvs_pwrkey - add configurable force shutdown time

[Ian Ray]

Changes since v1:
* Drop binding
* Configure LPCR only if power-off-time-sec property is set

Ian Ray (1):
  Input: snvs_pwrkey - support power-off-time-sec

 drivers/input/keyboard/snvs_pwrkey.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

-- 
2.39.5

[PATCH v2 1/1] Input: snvs_pwrkey - support power-off-time-sec

[Ian Ray]

The power-off time is configured in LPCR[17:16] BTN_PRESS_TIME:

 * b00:  5 seconds (SoC default)
 * b01: 10 seconds
 * b10: 15 seconds
 * b11: PMIC is not disabled

Signed-off-by: Ian Ray <ian.ray@gehealthcare.com>
---
 drivers/input/keyboard/snvs_pwrkey.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c
index f7b5f1e25c80..d7372ace694a 100644
--- a/drivers/input/keyboard/snvs_pwrkey.c
+++ b/drivers/input/keyboard/snvs_pwrkey.c
@@ -27,6 +27,8 @@
 #define SNVS_HPSR_BTN		BIT(6)
 #define SNVS_LPSR_SPO		BIT(18)
 #define SNVS_LPCR_DEP_EN	BIT(5)
+#define SNVS_LPCR_BPT_SHIFT	16
+#define SNVS_LPCR_BPT_MASK	(3 << SNVS_LPCR_BPT_SHIFT)
 
 #define DEBOUNCE_TIME		30
 #define REPEAT_INTERVAL		60
@@ -114,6 +116,8 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
 	struct device_node *np;
 	struct clk *clk;
 	int error;
+	unsigned int val;
+	unsigned int bpt;
 	u32 vid;
 
 	/* Get SNVS register Page */
@@ -148,6 +152,24 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
 	if (pdata->irq < 0)
 		return -EINVAL;
 
+	if (!of_property_read_u32(np, "power-off-time-sec", &val)) {
+		switch (val) {
+			case 0:
+				bpt = 0x3;
+				break;
+			case 5:
+			case 10:
+			case 15:
+				bpt = (val / 5) - 1;
+				break;
+			default:
+				dev_err(&pdev->dev, "power-off-time-sec %d out of range\n", val);
+				return -EINVAL;
+		}
+
+		regmap_update_bits(pdata->snvs, SNVS_LPCR_REG, SNVS_LPCR_BPT_MASK, bpt << SNVS_LPCR_BPT_SHIFT);
+	}
+
 	regmap_read(pdata->snvs, SNVS_HPVIDR1_REG, &vid);
 	pdata->minor_rev = vid & 0xff;
 
-- 
2.39.5

Re: [PATCH v2 1/1] Input: snvs_pwrkey - support power-off-time-sec

[Krzysztof Kozlowski]

On 14/03/2025 10:42, Ian Ray wrote:
>  
>  	/* Get SNVS register Page */
> @@ -148,6 +152,24 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
>  	if (pdata->irq < 0)
>  		return -EINVAL;
>  
> +	if (!of_property_read_u32(np, "power-off-time-sec", &val)) {

And when you test your DTS against binding what do you see? I suspect
new warning.

Best regards,
Krzysztof

Re: [PATCH v2 1/1] Input: snvs_pwrkey - support power-off-time-sec

[Ian Ray]

On Fri, Mar 14, 2025 at 01:55:47PM +0100, Krzysztof Kozlowski wrote:
> On 14/03/2025 10:42, Ian Ray wrote:
> >
> >       /* Get SNVS register Page */
> > @@ -148,6 +152,24 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
> >       if (pdata->irq < 0)
> >               return -EINVAL;
> >
> > +     if (!of_property_read_u32(np, "power-off-time-sec", &val)) {
> 
> And when you test your DTS against binding what do you see? I suspect
> new warning.

I checked the build logs (from a clean workarea), plus run-time dmesg,
both with the DTS change -- and without it.  There are no new warnings
(specifically nothing mentioning snvs-pwrkey or dts or power-off-time).

If an invalid value (such as "42") is chosen then the probe fails with
-EINVAL as expected.

Is there something else that I should have checked?

> 
> Best regards,
> Krzysztof

Blue skies,
Ian

Re: [PATCH v2 1/1] Input: snvs_pwrkey - support power-off-time-sec

[Krzysztof Kozlowski]

On 14/03/2025 14:24, Ian Ray wrote:
> On Fri, Mar 14, 2025 at 01:55:47PM +0100, Krzysztof Kozlowski wrote:
>> On 14/03/2025 10:42, Ian Ray wrote:
>>>
>>>       /* Get SNVS register Page */
>>> @@ -148,6 +152,24 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
>>>       if (pdata->irq < 0)
>>>               return -EINVAL;
>>>
>>> +     if (!of_property_read_u32(np, "power-off-time-sec", &val)) {
>>
>> And when you test your DTS against binding what do you see? I suspect
>> new warning.
> 
> I checked the build logs (from a clean workarea), plus run-time dmesg,
> both with the DTS change -- and without it.  There are no new warnings
> (specifically nothing mentioning snvs-pwrkey or dts or power-off-time).
> 
> If an invalid value (such as "42") is chosen then the probe fails with
> -EINVAL as expected.
> 
> Is there something else that I should have checked?

I don't know what your build logs process has. I meant dtbs_check
against the bindings.

Best regards,
Krzysztof

Re: [PATCH v2 1/1] Input: snvs_pwrkey - support power-off-time-sec

[Ian Ray]

On Fri, Mar 14, 2025 at 02:31:12PM +0100, Krzysztof Kozlowski wrote:
> On 14/03/2025 14:24, Ian Ray wrote:
> > On Fri, Mar 14, 2025 at 01:55:47PM +0100, Krzysztof Kozlowski wrote:
> >> On 14/03/2025 10:42, Ian Ray wrote:
> >>>
> >>>       /* Get SNVS register Page */
> >>> @@ -148,6 +152,24 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
> >>>       if (pdata->irq < 0)
> >>>               return -EINVAL;
> >>>
> >>> +     if (!of_property_read_u32(np, "power-off-time-sec", &val)) {
> >>
> >> And when you test your DTS against binding what do you see? I suspect
> >> new warning.
> >
> > I checked the build logs (from a clean workarea), plus run-time dmesg,
> > both with the DTS change -- and without it.  There are no new warnings
> > (specifically nothing mentioning snvs-pwrkey or dts or power-off-time).
> >
> > If an invalid value (such as "42") is chosen then the probe fails with
> > -EINVAL as expected.
> >
> > Is there something else that I should have checked?
> 
> I don't know what your build logs process has. I meant dtbs_check
> against the bindings.

$ dt-validate -s Documentation/devicetree/bindings/input/input.yaml \
  arch/arm64/boot/dts/freescale/imx8mp-ppdv2.dtb 

Generated no warnings.  Which (IIUC) is as expected since there is no
change to the bindings in this (v2) series.

> 
> Best regards,
> Krzysztof

Thanks,
Ian

Re: [PATCH v2 1/1] Input: snvs_pwrkey - support power-off-time-sec

[Krzysztof Kozlowski]

On 14/03/2025 15:47, Ian Ray wrote:
> On Fri, Mar 14, 2025 at 02:31:12PM +0100, Krzysztof Kozlowski wrote:
>> On 14/03/2025 14:24, Ian Ray wrote:
>>> On Fri, Mar 14, 2025 at 01:55:47PM +0100, Krzysztof Kozlowski wrote:
>>>> On 14/03/2025 10:42, Ian Ray wrote:
>>>>>
>>>>>       /* Get SNVS register Page */
>>>>> @@ -148,6 +152,24 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
>>>>>       if (pdata->irq < 0)
>>>>>               return -EINVAL;
>>>>>
>>>>> +     if (!of_property_read_u32(np, "power-off-time-sec", &val)) {
>>>>
>>>> And when you test your DTS against binding what do you see? I suspect
>>>> new warning.
>>>
>>> I checked the build logs (from a clean workarea), plus run-time dmesg,
>>> both with the DTS change -- and without it.  There are no new warnings
>>> (specifically nothing mentioning snvs-pwrkey or dts or power-off-time).
>>>
>>> If an invalid value (such as "42") is chosen then the probe fails with
>>> -EINVAL as expected.
>>>
>>> Is there something else that I should have checked?
>>
>> I don't know what your build logs process has. I meant dtbs_check
>> against the bindings.
> 
> $ dt-validate -s Documentation/devicetree/bindings/input/input.yaml \
>   arch/arm64/boot/dts/freescale/imx8mp-ppdv2.dtb 


input.yaml is not your binding, unless something changed. Did it? Is it
being applied here?

Test all bindings, so your schema will be applied.

<form letter>
It does not look like you tested the DTS against bindings. Please run
`make dtbs_check W=1` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).
Maybe you need to update your dtschema and yamllint. Don't rely on
distro packages for dtschema and be sure you are using the latest
released dtschema.
</form letter>

Best regards,
Krzysztof

Re: [PATCH v2 1/1] Input: snvs_pwrkey - support power-off-time-sec

[Ian Ray]

On Sun, Mar 16, 2025 at 02:03:52PM +0100, Krzysztof Kozlowski wrote:
> On 14/03/2025 15:47, Ian Ray wrote:
> > On Fri, Mar 14, 2025 at 02:31:12PM +0100, Krzysztof Kozlowski wrote:
> >> On 14/03/2025 14:24, Ian Ray wrote:
> >>> On Fri, Mar 14, 2025 at 01:55:47PM +0100, Krzysztof Kozlowski wrote:
> >>>> On 14/03/2025 10:42, Ian Ray wrote:
> >>>>>
> >>>>>       /* Get SNVS register Page */
> >>>>> @@ -148,6 +152,24 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev)
> >>>>>       if (pdata->irq < 0)
> >>>>>               return -EINVAL;
> >>>>>
> >>>>> +     if (!of_property_read_u32(np, "power-off-time-sec", &val)) {
> >>>>
> >>>> And when you test your DTS against binding what do you see? I suspect
> >>>> new warning.
> >>>
> >>> I checked the build logs (from a clean workarea), plus run-time dmesg,
> >>> both with the DTS change -- and without it.  There are no new warnings
> >>> (specifically nothing mentioning snvs-pwrkey or dts or power-off-time).
> >>>
> >>> If an invalid value (such as "42") is chosen then the probe fails with
> >>> -EINVAL as expected.
> >>>
> >>> Is there something else that I should have checked?
> >>
> >> I don't know what your build logs process has. I meant dtbs_check
> >> against the bindings.
> >
> > $ dt-validate -s Documentation/devicetree/bindings/input/input.yaml \
> >   arch/arm64/boot/dts/freescale/imx8mp-ppdv2.dtb
> 
> 
> input.yaml is not your binding, unless something changed. Did it? Is it
> being applied here?
> 

Ah, I see (finally); thank you for this!  Submitted v3 at [1].

[1] https://lore.kernel.org/lkml/20250315093455.1100-1-ian.ray@gehealthcare.com/


> Test all bindings, so your schema will be applied.
> 
> <form letter>
> It does not look like you tested the DTS against bindings. Please run
> `make dtbs_check W=1` (see
> Documentation/devicetree/bindings/writing-schema.rst or
> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
> for instructions).
> Maybe you need to update your dtschema and yamllint. Don't rely on
> distro packages for dtschema and be sure you are using the latest
> released dtschema.
> </form letter>
> 
> Best regards,
> Krzysztof

Best regards,
Ian