[PATCH v2 1/2] dt-bindings: input: touchscreen: Add 'hold-in-reset-in-suspend' property to goodix

[PATCH v2 1/2] dt-bindings: input: touchscreen: Add 'hold-in-reset-in-suspend' property to goodix

[Jan Jasper de Kroon]

This patch adds a new optional property, 'hold-in-reset-in-suspend', to the
Goodix touchscreen device tree binding. When set to true, the touchscreen
controller will be held in reset mode during system suspend, reducing
power consumption. If not present, the property defaults to false.

I am submitting this patch to the Device Tree mailing list to add a new
property to the Goodix touchscreen device tree binding. This patch
supplements a related patch sent to the linux-input mailing list, which
updates the Goodix touchscreen driver to use this new property.The
linux-input patch can be found at:
https://lore.kernel.org/all/20230311131534.484700-1-jajadekroon@gmail.com/

Signed-off-by: Jan Jasper de Kroon <jajadekroon@gmail.com>
---
V1 -> V2:
- Updated subject prefix to match subsystem
- Added more detailed description of the change
- Fixed formatting issues in commit message
 .../devicetree/bindings/input/touchscreen/goodix.yaml  | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
index 3d016b87c8df..cd4dd3d25364 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
+++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
@@ -56,6 +56,15 @@ properties:
   touchscreen-size-y: true
   touchscreen-swapped-x-y: true
 
+  hold-in-reset-in-suspend:
+    type: boolean
+    default: false
+    description: |
+      When set to true, the touchscreen controller will be held in reset mode
+      during system suspend. This can help reduce power consumption, but may
+      cause the touchscreen to take longer to resume when the system is woken
+      up from suspend. Defaults to false if not present.
+
 additionalProperties: false
 
 required:
@@ -75,6 +84,7 @@ examples:
         interrupts = <0 0>;
         irq-gpios = <&gpio1 0 0>;
         reset-gpios = <&gpio1 1 0>;
+        hold-in-reset-in-suspend;
       };
     };
 
-- 
2.34.3

Re: [PATCH v2 1/2] dt-bindings: input: touchscreen: Add 'hold-in-reset-in-suspend' property to goodix

[Krzysztof Kozlowski]

On 12/03/2023 19:31, Jan Jasper de Kroon wrote:
> This patch adds a new optional property, 'hold-in-reset-in-suspend', to the

Do not use "This commit/patch", but imperative mood. See:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

> Goodix touchscreen device tree binding. When set to true, the touchscreen
> controller will be held in reset mode during system suspend, reducing
> power consumption. If not present, the property defaults to false.
> 
> I am submitting this patch to the Device Tree mailing list to add a new

Drop the "I am ..." Same comment as above.

> property to the Goodix touchscreen device tree binding. This patch
> supplements a related patch sent to the linux-input mailing list, which
> updates the Goodix touchscreen driver to use this new property.

Anyway entire paragraph does not look related to commit msg. Drop it.

> The
> linux-input patch can be found at:
> https://lore.kernel.org/all/20230311131534.484700-1-jajadekroon@gmail.com/

Also this. It should be rather places under ---.

> 
> Signed-off-by: Jan Jasper de Kroon <jajadekroon@gmail.com>
> ---
> V1 -> V2:
> - Updated subject prefix to match subsystem
> - Added more detailed description of the change
> - Fixed formatting issues in commit message
>  .../devicetree/bindings/input/touchscreen/goodix.yaml  | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
> index 3d016b87c8df..cd4dd3d25364 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
> +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
> @@ -56,6 +56,15 @@ properties:
>    touchscreen-size-y: true
>    touchscreen-swapped-x-y: true
>  
> +  hold-in-reset-in-suspend:

Missing vendor prefix.

> +    type: boolean
> +    default: false

Drop default.

> +    description: |
> +      When set to true, the touchscreen controller will be held in reset mode
> +      during system suspend. This can help reduce power consumption, but may
> +      cause the touchscreen to take longer to resume when the system is woken
> +      up from suspend. Defaults to false if not present.

Drop last sentence.

Anyway, the property does not look suitable for Devicetree. You describe
system policy - trade off between energy saving and responsivness to the
user. DT is not for policies. Use other interfaces for configuring it,
e.g. some user-space, existing PM interfaces or /sysfs (which is ABI and
needs its Documentation).


> +
>  additionalProperties: false
>  
>  required:
> @@ -75,6 +84,7 @@ examples:
>          interrupts = <0 0>;
>          irq-gpios = <&gpio1 0 0>;
>          reset-gpios = <&gpio1 1 0>;
> +        hold-in-reset-in-suspend;
>        };
>      };
>  

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: input: touchscreen: Add 'hold-in-reset-in-suspend' property to goodix

[Jan Jasper de Kroon]

Op 14-03-2023 om 18:47 schreef Krzysztof Kozlowski:
> On 12/03/2023 19:31, Jan Jasper de Kroon wrote:
>> This patch adds a new optional property, 'hold-in-reset-in-suspend', to the
> Do not use "This commit/patch", but imperative mood. See:
> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
>
>> Goodix touchscreen device tree binding. When set to true, the touchscreen
>> controller will be held in reset mode during system suspend, reducing
>> power consumption. If not present, the property defaults to false.
>>
>> I am submitting this patch to the Device Tree mailing list to add a new
> Drop the "I am ..." Same comment as above.
>
>> property to the Goodix touchscreen device tree binding. This patch
>> supplements a related patch sent to the linux-input mailing list, which
>> updates the Goodix touchscreen driver to use this new property.
> Anyway entire paragraph does not look related to commit msg. Drop it.
>
>> The
>> linux-input patch can be found at:
>> https://lore.kernel.org/all/20230311131534.484700-1-jajadekroon@gmail.com/
> Also this. It should be rather places under ---.
>
>> Signed-off-by: Jan Jasper de Kroon <jajadekroon@gmail.com>
>> ---
>> V1 -> V2:
>> - Updated subject prefix to match subsystem
>> - Added more detailed description of the change
>> - Fixed formatting issues in commit message
>>   .../devicetree/bindings/input/touchscreen/goodix.yaml  | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
>> index 3d016b87c8df..cd4dd3d25364 100644
>> --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
>> +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
>> @@ -56,6 +56,15 @@ properties:
>>     touchscreen-size-y: true
>>     touchscreen-swapped-x-y: true
>>   
>> +  hold-in-reset-in-suspend:
> Missing vendor prefix.
>
>> +    type: boolean
>> +    default: false
> Drop default.
>
>> +    description: |
>> +      When set to true, the touchscreen controller will be held in reset mode
>> +      during system suspend. This can help reduce power consumption, but may
>> +      cause the touchscreen to take longer to resume when the system is woken
>> +      up from suspend. Defaults to false if not present.
> Drop last sentence.
>
> Anyway, the property does not look suitable for Devicetree. You describe
> system policy - trade off between energy saving and responsivness to the
> user. DT is not for policies. Use other interfaces for configuring it,
> e.g. some user-space, existing PM interfaces or /sysfs (which is ABI and
> needs its Documentation).
>
>
>> +
>>   additionalProperties: false
>>   
>>   required:
>> @@ -75,6 +84,7 @@ examples:
>>           interrupts = <0 0>;
>>           irq-gpios = <&gpio1 0 0>;
>>           reset-gpios = <&gpio1 1 0>;
>> +        hold-in-reset-in-suspend;
>>         };
>>       };
>>   
> Best regards,
> Krzysztof
>
I think the latest patch covers most of the feedback you provided.
Regarding the addition of this feature to the DeviceTree. Currently this
is only used on the Linux powered PinePhone Original and PinePhone Pro. It
also isn't really a policy change, but rather an attempt to minimize
battery consumption on these power hungry devices. Developers made a lot
of tweaks here and there, to make the PinePhone get through a day of light
use. This is one of these tweaks.

Best regards,
Jan Jasper de Kroon

Re: [PATCH v2 1/2] dt-bindings: input: touchscreen: Add 'hold-in-reset-in-suspend' property to goodix

[Krzysztof Kozlowski]

On 16/03/2023 16:41, Jan Jasper de Kroon wrote:
>> Anyway, the property does not look suitable for Devicetree. You describe
>> system policy - trade off between energy saving and responsivness to the
>> user. DT is not for policies. Use other interfaces for configuring it,
>> e.g. some user-space, existing PM interfaces or /sysfs (which is ABI and
>> needs its Documentation).
>>
>>
>>> +
>>>   additionalProperties: false
>>>   
>>>   required:
>>> @@ -75,6 +84,7 @@ examples:
>>>           interrupts = <0 0>;
>>>           irq-gpios = <&gpio1 0 0>;
>>>           reset-gpios = <&gpio1 1 0>;
>>> +        hold-in-reset-in-suspend;
>>>         };
>>>       };
>>>   
>> Best regards,
>> Krzysztof
>>
> I think the latest patch covers most of the feedback you provided.
> Regarding the addition of this feature to the DeviceTree. Currently this
> is only used on the Linux powered PinePhone Original and PinePhone Pro. It
> also isn't really a policy change, 

What is "policy change"? I said it is system policy.

> but rather an attempt to minimize
> battery consumption on these power hungry devices.

We do not talk about the goal, but whether this is property of
Devicetree or not.

>  Developers made a lot
> of tweaks here and there, to make the PinePhone get through a day of light
> use. This is one of these tweaks.

Awesome, how is this related to my concerns that it is not suitable for
Devicetree? Developers can invent thousands of things, shall we put them
all to Devicetree?

Bring specific arguments to my questions. Arguing that it is not a
"policy change" is not related to my question. Just like calling
something tweaks.


Best regards,
Krzysztof

[PATCH v3 1/2] dt-bindings: input: touchscreen: Add 'goodix-hold-in-reset' property to Goodix

[Jan Jasper de Kroon]

Add an optional 'goodix-hold-in-reset', to the Goodix touchscreen
device tree binding. When set to true, the touchscreen controller will
be held in reset mode during system suspend, reducing power consumption.
If not present, the property defaults to false.

Signed-off-by: Jan Jasper de Kroon <jajadekroon@gmail.com>
---
Changes from v2 to v3:
- Used imperative mood instead of "This patch adds".
- Dropped "I am submitting this patch to..." as it is redundant.
- Removed the paragraph related to the related patch sent to the 
  linux-input mailing list as it is not necessary.
- Renamed the hold-in-reset-in-suspend function to 
  goodix-hold-in-reset to prevent potential naming conflicts with other 
  functions in the codebase. No functional changes were made.

Changes from v1 to v2:
- Updated subject prefix to match subsystem.
- Added more detailed description of the change.
- Fixed formatting issues in commit message.
 .../devicetree/bindings/input/touchscreen/goodix.yaml     | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
index 3d016b87c8df..197f8db9acc2 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
+++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
@@ -56,6 +56,13 @@ properties:
   touchscreen-size-y: true
   touchscreen-swapped-x-y: true
 
+  goodix-hold-in-reset:
+    description: |
+      When set to true, the touchscreen controller will be held in reset mode
+      during system suspend. This can help reduce power consumption, but may
+      cause the touchscreen to take longer to resume when the system is woken
+      up from suspend.
+
 additionalProperties: false
 
 required:
@@ -75,6 +82,7 @@ examples:
         interrupts = <0 0>;
         irq-gpios = <&gpio1 0 0>;
         reset-gpios = <&gpio1 1 0>;
+        goodix-hold-in-reset;
       };
     };
 
-- 
2.34.3

Re: [PATCH v3 1/2] dt-bindings: input: touchscreen: Add 'goodix-hold-in-reset' property to Goodix

[Krzysztof Kozlowski]

On 16/03/2023 16:29, Jan Jasper de Kroon wrote:
> Add an optional 'goodix-hold-in-reset', to the Goodix touchscreen
> device tree binding. When set to true, the touchscreen controller will
> be held in reset mode during system suspend, reducing power consumption.
> If not present, the property defaults to false.
> 
> Signed-off-by: Jan Jasper de Kroon <jajadekroon@gmail.com>

Don't attach new patchsets to some other threads. It messes with our
tools and reading/reviewing process.

> ---
> Changes from v2 to v3:
> - Used imperative mood instead of "This patch adds".
> - Dropped "I am submitting this patch to..." as it is redundant.
> - Removed the paragraph related to the related patch sent to the 
>   linux-input mailing list as it is not necessary.
> - Renamed the hold-in-reset-in-suspend function to 
>   goodix-hold-in-reset to prevent potential naming conflicts with other 
>   functions in the codebase. No functional changes were made.
> 
> Changes from v1 to v2:
> - Updated subject prefix to match subsystem.
> - Added more detailed description of the change.
> - Fixed formatting issues in commit message.
>  .../devicetree/bindings/input/touchscreen/goodix.yaml     | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
> index 3d016b87c8df..197f8db9acc2 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
> +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
> @@ -56,6 +56,13 @@ properties:
>    touchscreen-size-y: true
>    touchscreen-swapped-x-y: true
>  
> +  goodix-hold-in-reset:

That's not a vendor prefix... missing coma.


> +    description: |
> +      When set to true, the touchscreen controller will be held in reset mode
> +      during system suspend. This can help reduce power consumption, but may
> +      cause the touchscreen to take longer to resume when the system is woken
> +      up from suspend.

Anyway, my concerns were not answered, so to be clear:

NAK till you answer them. Do not send new versions without answering
existing concerns and discussion.


Best regards,
Krzysztof

Re: [PATCH v3 1/2] dt-bindings: input: touchscreen: Add 'goodix-hold-in-reset' property to Goodix

[Jan Jasper de Kroon]

Op 16-03-2023 om 20:25 schreef Krzysztof Kozlowski:
> On 16/03/2023 16:29, Jan Jasper de Kroon wrote:
>> Add an optional 'goodix-hold-in-reset', to the Goodix touchscreen
>> device tree binding. When set to true, the touchscreen controller will
>> be held in reset mode during system suspend, reducing power consumption.
>> If not present, the property defaults to false.
>>
>> Signed-off-by: Jan Jasper de Kroon <jajadekroon@gmail.com>
> Don't attach new patchsets to some other threads. It messes with our
> tools and reading/reviewing process.
Thank you for bringing this to my attention. I apologize for any
inconvenience caused by attaching the patchset to the wrong threads. As a
new user of LKML, I'm still learning the appropriate protocol for
submitting patches. Going forward, I will ensure to attach patchsets to
the correct threads.
>> ---
>> Changes from v2 to v3:
>> - Used imperative mood instead of "This patch adds".
>> - Dropped "I am submitting this patch to..." as it is redundant.
>> - Removed the paragraph related to the related patch sent to the
>>    linux-input mailing list as it is not necessary.
>> - Renamed the hold-in-reset-in-suspend function to
>>    goodix-hold-in-reset to prevent potential naming conflicts with other
>>    functions in the codebase. No functional changes were made.
>>
>> Changes from v1 to v2:
>> - Updated subject prefix to match subsystem.
>> - Added more detailed description of the change.
>> - Fixed formatting issues in commit message.
>>   .../devicetree/bindings/input/touchscreen/goodix.yaml     | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
>> index 3d016b87c8df..197f8db9acc2 100644
>> --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
>> +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
>> @@ -56,6 +56,13 @@ properties:
>>     touchscreen-size-y: true
>>     touchscreen-swapped-x-y: true
>>   
>> +  goodix-hold-in-reset:
> That's not a vendor prefix... missing coma.
Thank you for pointing out the mistake in the vendor prefix. I appreciate
your feedback and apologize for any inconvenience caused. I wasn't aware
of the correct vendor prefix style, but I've learned from developer Hans
de Goede that it should be "goodix,hold-in-reset." I will make sure to
correct this in my local branch and ensure that it is applied correctly in
the future. Thanks again for bringing this to my attention.
>> +    description: |
>> +      When set to true, the touchscreen controller will be held in reset mode
>> +      during system suspend. This can help reduce power consumption, but may
>> +      cause the touchscreen to take longer to resume when the system is woken
>> +      up from suspend.
> Anyway, my concerns were not answered, so to be clear:
>
> NAK till you answer them. Do not send new versions without answering
> existing concerns and discussion.
Thank you again for reviewing my patchset and providing feedback. I
appreciate your time and effort in ensuring the quality and suitability
of the DeviceTree.

Regarding the concerns you raised about the proposed feature, I would
like to address them directly. You mentioned that the property does not
look suitable for Devicetree because it describes system policies that are
not within the scope of Devicetree. While I understand your point, I
believe this property is appropriate for Devicetree for the following
reasons:

- The property directly relates to the hardware configuration of the
   device, specifically the touchscreen controller, and is not a software
   policy.

- The property is required for proper system operation and is not optional
   in specific device use cases. To be more specific in the case of the
   PinePhone Original and Pro. The original commit message of the driver
   implementation in driver/input/touchscreen contained the following:
   "It consumes quite a bit of power (~40mW) during system sleep, and more
   when the screen is touched."
   Because the phone is usually kept in your pocket, so prone to a lot of
   screen touches, this is highly undesired behavior for the touchscreen in
   this case. This in my opinion makes it a mandatory property in this
   situation.

- The property is not a user-facing configuration option and is not meant
   to be changed by the end-user.

- The property, although in separate device specific kernel, and still
   called 'poweroff-in-suspend' is already in use on specific devices,
   including the PinePhone Original and PinePhone Pro.

However, I understand your concern that Devicetree should not be used for
policies. To address this concern, I would like to propose the following
changes to the property description:
1. Remove the sentence about reducing power consumption, as this could be
    considered a policy.
2. Emphasize that the property is a required hardware configuration and
    not an optional feature on certain devices.
3. Recommend that any changes to the property value should only be made by
    experienced system administrators and not end-users.

I hope these changes address your concerns and make the property more
suitable for inclusion in Devicetree. If you have any further suggestions
or feedback, please let me know. Thank you again for your time and
guidance.

Best regards,

Jan Jasper de Kroon
>
>
> Best regards,
> Krzysztof
>

Re: [PATCH v3 1/2] dt-bindings: input: touchscreen: Add 'goodix-hold-in-reset' property to Goodix

[Krzysztof Kozlowski]

On 17/03/2023 11:39, Jan Jasper de Kroon wrote:
> 
> Op 16-03-2023 om 20:25 schreef Krzysztof Kozlowski:
>> On 16/03/2023 16:29, Jan Jasper de Kroon wrote:
>>> Add an optional 'goodix-hold-in-reset', to the Goodix touchscreen
>>> device tree binding. When set to true, the touchscreen controller will
>>> be held in reset mode during system suspend, reducing power consumption.
>>> If not present, the property defaults to false.
>>>
>>> Signed-off-by: Jan Jasper de Kroon <jajadekroon@gmail.com>
>> Don't attach new patchsets to some other threads. It messes with our
>> tools and reading/reviewing process.
> Thank you for bringing this to my attention. I apologize for any
> inconvenience caused by attaching the patchset to the wrong threads. As a
> new user of LKML, I'm still learning the appropriate protocol for
> submitting patches. Going forward, I will ensure to attach patchsets to
> the correct threads.
>>> ---
>>> Changes from v2 to v3:
>>> - Used imperative mood instead of "This patch adds".
>>> - Dropped "I am submitting this patch to..." as it is redundant.
>>> - Removed the paragraph related to the related patch sent to the
>>>    linux-input mailing list as it is not necessary.
>>> - Renamed the hold-in-reset-in-suspend function to
>>>    goodix-hold-in-reset to prevent potential naming conflicts with other
>>>    functions in the codebase. No functional changes were made.
>>>
>>> Changes from v1 to v2:
>>> - Updated subject prefix to match subsystem.
>>> - Added more detailed description of the change.
>>> - Fixed formatting issues in commit message.
>>>   .../devicetree/bindings/input/touchscreen/goodix.yaml     | 8 ++++++++
>>>   1 file changed, 8 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
>>> index 3d016b87c8df..197f8db9acc2 100644
>>> --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
>>> +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
>>> @@ -56,6 +56,13 @@ properties:
>>>     touchscreen-size-y: true
>>>     touchscreen-swapped-x-y: true
>>>   
>>> +  goodix-hold-in-reset:
>> That's not a vendor prefix... missing coma.
> Thank you for pointing out the mistake in the vendor prefix. I appreciate
> your feedback and apologize for any inconvenience caused. I wasn't aware
> of the correct vendor prefix style, but I've learned from developer Hans
> de Goede that it should be "goodix,hold-in-reset." I will make sure to
> correct this in my local branch and ensure that it is applied correctly in
> the future. Thanks again for bringing this to my attention.
>>> +    description: |
>>> +      When set to true, the touchscreen controller will be held in reset mode
>>> +      during system suspend. This can help reduce power consumption, but may
>>> +      cause the touchscreen to take longer to resume when the system is woken
>>> +      up from suspend.
>> Anyway, my concerns were not answered, so to be clear:
>>
>> NAK till you answer them. Do not send new versions without answering
>> existing concerns and discussion.
> Thank you again for reviewing my patchset and providing feedback. I
> appreciate your time and effort in ensuring the quality and suitability
> of the DeviceTree.
> 
> Regarding the concerns you raised about the proposed feature, I would
> like to address them directly. You mentioned that the property does not
> look suitable for Devicetree because it describes system policies that are
> not within the scope of Devicetree. While I understand your point, I
> believe this property is appropriate for Devicetree for the following
> reasons:
> 
> - The property directly relates to the hardware configuration of the
>    device, specifically the touchscreen controller, and is not a software
>    policy.

Keeping device in reset state is not hardware configuration but driver
behavior. You did not Cc us on all patches for some reason, so it's
difficult to judge what exactly your driver is doing.

> 
> - The property is required for proper system operation and is not optional
>    in specific device use cases. To be more specific in the case of the
>    PinePhone Original and Pro. The original commit message of the driver
>    implementation in driver/input/touchscreen contained the following:
>    "It consumes quite a bit of power (~40mW) during system sleep, and more
>    when the screen is touched."
>    Because the phone is usually kept in your pocket, so prone to a lot of
>    screen touches, this is highly undesired behavior for the touchscreen in
>    this case. This in my opinion makes it a mandatory property in this
>    situation.

Why then the touchscree should not be kept in reset for other devices?
IOW, this should be always used. If you now say "I prefer to keep or not
keep it in reset for my device" - it's a policy.


> 
> - The property is not a user-facing configuration option and is not meant
>    to be changed by the end-user.

Does not matter.

> 
> - The property, although in separate device specific kernel, and still
>    called 'poweroff-in-suspend' is already in use on specific devices,
>    including the PinePhone Original and PinePhone Pro.

I could not find such property in the kernel.

> 
> However, I understand your concern that Devicetree should not be used for
> policies. To address this concern, I would like to propose the following
> changes to the property description:
> 1. Remove the sentence about reducing power consumption, as this could be
>     considered a policy.
> 2. Emphasize that the property is a required hardware configuration and
>     not an optional feature on certain devices.
> 3. Recommend that any changes to the property value should only be made by
>     experienced system administrators and not end-users.

Please answer - why this should not be enabled always.

Best regards,
Krzysztof

Re: [PATCH v3 1/2] dt-bindings: input: touchscreen: Add 'goodix-hold-in-reset' property to Goodix

[Jan Jasper de Kroon]

Op 19-03-2023 om 15:09 schreef Krzysztof Kozlowski:
> On 17/03/2023 11:39, Jan Jasper de Kroon wrote:
>> Op 16-03-2023 om 20:25 schreef Krzysztof Kozlowski:
>>> On 16/03/2023 16:29, Jan Jasper de Kroon wrote:
>>>> Add an optional 'goodix-hold-in-reset', to the Goodix touchscreen
>>>> device tree binding. When set to true, the touchscreen controller will
>>>> be held in reset mode during system suspend, reducing power consumption.
>>>> If not present, the property defaults to false.
>>>>
>>>> Signed-off-by: Jan Jasper de Kroon <jajadekroon@gmail.com>
>>> Don't attach new patchsets to some other threads. It messes with our
>>> tools and reading/reviewing process.
>> Thank you for bringing this to my attention. I apologize for any
>> inconvenience caused by attaching the patchset to the wrong threads. As a
>> new user of LKML, I'm still learning the appropriate protocol for
>> submitting patches. Going forward, I will ensure to attach patchsets to
>> the correct threads.
>>>> ---
>>>> Changes from v2 to v3:
>>>> - Used imperative mood instead of "This patch adds".
>>>> - Dropped "I am submitting this patch to..." as it is redundant.
>>>> - Removed the paragraph related to the related patch sent to the
>>>>     linux-input mailing list as it is not necessary.
>>>> - Renamed the hold-in-reset-in-suspend function to
>>>>     goodix-hold-in-reset to prevent potential naming conflicts with other
>>>>     functions in the codebase. No functional changes were made.
>>>>
>>>> Changes from v1 to v2:
>>>> - Updated subject prefix to match subsystem.
>>>> - Added more detailed description of the change.
>>>> - Fixed formatting issues in commit message.
>>>>    .../devicetree/bindings/input/touchscreen/goodix.yaml     | 8 ++++++++
>>>>    1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
>>>> index 3d016b87c8df..197f8db9acc2 100644
>>>> --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
>>>> +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
>>>> @@ -56,6 +56,13 @@ properties:
>>>>      touchscreen-size-y: true
>>>>      touchscreen-swapped-x-y: true
>>>>    
>>>> +  goodix-hold-in-reset:
>>> That's not a vendor prefix... missing coma.
>> Thank you for pointing out the mistake in the vendor prefix. I appreciate
>> your feedback and apologize for any inconvenience caused. I wasn't aware
>> of the correct vendor prefix style, but I've learned from developer Hans
>> de Goede that it should be "goodix,hold-in-reset." I will make sure to
>> correct this in my local branch and ensure that it is applied correctly in
>> the future. Thanks again for bringing this to my attention.
>>>> +    description: |
>>>> +      When set to true, the touchscreen controller will be held in reset mode
>>>> +      during system suspend. This can help reduce power consumption, but may
>>>> +      cause the touchscreen to take longer to resume when the system is woken
>>>> +      up from suspend.
>>> Anyway, my concerns were not answered, so to be clear:
>>>
>>> NAK till you answer them. Do not send new versions without answering
>>> existing concerns and discussion.
>> Thank you again for reviewing my patchset and providing feedback. I
>> appreciate your time and effort in ensuring the quality and suitability
>> of the DeviceTree.
>>
>> Regarding the concerns you raised about the proposed feature, I would
>> like to address them directly. You mentioned that the property does not
>> look suitable for Devicetree because it describes system policies that are
>> not within the scope of Devicetree. While I understand your point, I
>> believe this property is appropriate for Devicetree for the following
>> reasons:
>>
>> - The property directly relates to the hardware configuration of the
>>     device, specifically the touchscreen controller, and is not a software
>>     policy.
> Keeping device in reset state is not hardware configuration but driver
> behavior. You did not Cc us on all patches for some reason, so it's
> difficult to judge what exactly your driver is doing.
Thank you for your response. I apologize for not including all the
necessary information in my previous messages. Like you are already aware,
the DT patch is only one part of the solution, and the driver part has
been submitted to the linux-input mailing list. Here is the link to the
latest patch submission:
https://lore.kernel.org/all/20230316152159.66922-1-jajadekroon@gmail.com/

I understand that it may have been difficult to judge what the driver is
doing without the complete context. The original patch consists of two
'out-of-tree' commits, one that attempts to power off the touchscreen device
controller completely, including the regulators:
https://github.com/megous/linux/commit/a38e3e2900c69f5b9961aca8e003c21950453857
and another that reverts disabling the regulators:
https://github.com/megous/linux/commit/cafc7adf456c03eb4564c2ba750a5345b9c6ceed
The reason for this is that different peripherals are attached to the same
power supply in the case of the PinePhone Original and PinePhone Pro.

I hope this clarifies part of the situation. If you have any further
questions or concerns, please do not hesitate to let me know.
>
>> - The property is required for proper system operation and is not optional
>>     in specific device use cases. To be more specific in the case of the
>>     PinePhone Original and Pro. The original commit message of the driver
>>     implementation in driver/input/touchscreen contained the following:
>>     "It consumes quite a bit of power (~40mW) during system sleep, and more
>>     when the screen is touched."
>>     Because the phone is usually kept in your pocket, so prone to a lot of
>>     screen touches, this is highly undesired behavior for the touchscreen in
>>     this case. This in my opinion makes it a mandatory property in this
>>     situation.
> Why then the touchscree should not be kept in reset for other devices?
> IOW, this should be always used. If you now say "I prefer to keep or not
> keep it in reset for my device" - it's a policy.
Thank you for your question. While it's true that keeping the touchscreen
in reset state during system sleep can reduce power consumption for other
devices, the decision to use this property should be based on the specific
use case and hardware configuration of each device. In the case of the
PinePhone Original and Pro, the touchscreen's power consumption during
system sleep is significant, and the device is often kept in a pocket, so
accidental screen touches can occur frequently, leading to further power
drain. As such, keeping the touchscreen in reset state is necessary for
proper system operation in these specific devices. However, for other
devices with different hardware configurations and use cases, the decision
to use this property should be based on a thorough assessment of the power
consumption and potential impact on system behavior.
>
>
>> - The property is not a user-facing configuration option and is not meant
>>     to be changed by the end-user.
> Does not matter.
>
>> - The property, although in separate device specific kernel, and still
>>     called 'poweroff-in-suspend' is already in use on specific devices,
>>     including the PinePhone Original and PinePhone Pro.
> I could not find such property in the kernel.
I apologize for the confusion, but the current mainline kernel doesn't
include this property. As I stated to support the PinePhone Original and
PinePhone Pro, the community makes use of a forked mainline kernel, with
a lot of out-of-tree patches/commits, mainly maintained by developer
Ondrej Jirman. For the PinePhone Original, the DeviceTree configuration
in the PinePhone DTS gets set in the following commit:
https://github.com/megous/linux/commit/74fc0a5f0527afdccb67ce3be690f0ae18c8eca6
For the PinePhone Pro it is set in the following commit, at line 466:
https://github.com/megous/linux/commit/471c5f33ba74c3d498ccc1eb69c098623b193926#
The property here is still called "poweroff-in-suspend".
>
>> However, I understand your concern that Devicetree should not be used for
>> policies. To address this concern, I would like to propose the following
>> changes to the property description:
>> 1. Remove the sentence about reducing power consumption, as this could be
>>      considered a policy.
>> 2. Emphasize that the property is a required hardware configuration and
>>      not an optional feature on certain devices.
>> 3. Recommend that any changes to the property value should only be made by
>>      experienced system administrators and not end-users.
> Please answer - why this should not be enabled always.
One reason why the Touchscreen Controller should not be kept in reset
always is that some devices may have a use case where the touchscreen
needs to remain active even during system sleep, and keeping it in reset
would cause issues with that case. However, in the case of the
battery-powered PinePhone Original and Pro, keeping the touchscreen
controller in reset during system sleep is required for proper system
operation. Having the device in your pocket makes unintentional screen
touches almost unavoidable, and this property enabled is necessary for
these devices. In the case of the PinePhone Original and Pro, enabling
this feature we do consider its impact on battery life or in other words
power consumption.
But bottomlined, enabling this feature should be evaluated on a
case-by-case basis, taking into consideration the specific device use case
and hardware configurations. Thank you for your feedback.
>
> Best regards,
> Krzysztof
>

Re: [PATCH v3 1/2] dt-bindings: input: touchscreen: Add 'goodix-hold-in-reset' property to Goodix

[Krzysztof Kozlowski]

On 19/03/2023 17:38, Jan Jasper de Kroon wrote:
>>
>>> - The property is required for proper system operation and is not optional
>>>     in specific device use cases. To be more specific in the case of the
>>>     PinePhone Original and Pro. The original commit message of the driver
>>>     implementation in driver/input/touchscreen contained the following:
>>>     "It consumes quite a bit of power (~40mW) during system sleep, and more
>>>     when the screen is touched."
>>>     Because the phone is usually kept in your pocket, so prone to a lot of
>>>     screen touches, this is highly undesired behavior for the touchscreen in
>>>     this case. This in my opinion makes it a mandatory property in this
>>>     situation.
>> Why then the touchscree should not be kept in reset for other devices?
>> IOW, this should be always used. If you now say "I prefer to keep or not
>> keep it in reset for my device" - it's a policy.
> Thank you for your question. While it's true that keeping the touchscreen
> in reset state during system sleep can reduce power consumption for other
> devices, the decision to use this property should be based on the specific
> use case and hardware configuration of each device. In the case of the
> PinePhone Original and Pro, the touchscreen's power consumption during
> system sleep is significant, and the device is often kept in a pocket, so
> accidental screen touches can occur frequently, leading to further power
> drain. As such, keeping the touchscreen in reset state is necessary for
> proper system operation in these specific devices. However, for other
> devices with different hardware configurations and use cases, the decision
> to use this property should be based on a thorough assessment of the power
> consumption and potential impact on system behavior.

Why? Even if energy consumption for these other devices is very low, it
is still reasonable to keep the touchscreen off during suspend. Why
anyone would like otherwise?

Wakeup could be the reason, but for this we have property.

>>
>>
>>> - The property is not a user-facing configuration option and is not meant
>>>     to be changed by the end-user.
>> Does not matter.
>>
>>> - The property, although in separate device specific kernel, and still
>>>     called 'poweroff-in-suspend' is already in use on specific devices,
>>>     including the PinePhone Original and PinePhone Pro.
>> I could not find such property in the kernel.
> I apologize for the confusion, but the current mainline kernel doesn't
> include this property. As I stated to support the PinePhone Original and
> PinePhone Pro, the community makes use of a forked mainline kernel, with
> a lot of out-of-tree patches/commits, mainly maintained by developer
> Ondrej Jirman. For the PinePhone Original, the DeviceTree configuration
> in the PinePhone DTS gets set in the following commit:
> https://github.com/megous/linux/commit/74fc0a5f0527afdccb67ce3be690f0ae18c8eca6
> For the PinePhone Pro it is set in the following commit, at line 466:
> https://github.com/megous/linux/commit/471c5f33ba74c3d498ccc1eb69c098623b193926#
> The property here is still called "poweroff-in-suspend".

Whatever forks are doing is rarely argument for us. Did that property
pass DT maintainers review? No.

>>
>>> However, I understand your concern that Devicetree should not be used for
>>> policies. To address this concern, I would like to propose the following
>>> changes to the property description:
>>> 1. Remove the sentence about reducing power consumption, as this could be
>>>      considered a policy.
>>> 2. Emphasize that the property is a required hardware configuration and
>>>      not an optional feature on certain devices.
>>> 3. Recommend that any changes to the property value should only be made by
>>>      experienced system administrators and not end-users.
>> Please answer - why this should not be enabled always.
> One reason why the Touchscreen Controller should not be kept in reset
> always is that some devices may have a use case where the touchscreen
> needs to remain active even during system sleep, and keeping it in reset
> would cause issues with that case.

Use case is rather policy... Except wakeup, but for this we have property.

>  However, in the case of the
> battery-powered PinePhone Original and Pro, keeping the touchscreen
> controller in reset during system sleep is required for proper system
> operation.

The question was "why not enabled always". How this is related?

>  Having the device in your pocket makes unintentional screen
> touches almost unavoidable, and this property enabled is necessary for
> these devices. In the case of the PinePhone Original and Pro, enabling
> this feature we do consider its impact on battery life or in other words
> power consumption.

You keep repeating the same and email is very long.

> But bottomlined, enabling this feature should be evaluated on a
> case-by-case basis, taking into consideration the specific device use case
> and hardware configurations. Thank you for your feedback.

Best regards,
Krzysztof

[PATCH v3 1/2] dt-bindings: input: touchscreen: Add 'goodix-hold-in-reset' property to Goodix

[Jan Jasper de Kroon]

Add an optional 'goodix-hold-in-reset', to the Goodix touchscreen
device tree binding. When set to true, the touchscreen controller will
be held in reset mode during system suspend, reducing power consumption.
If not present, the property defaults to false.

Signed-off-by: Jan Jasper de Kroon <jajadekroon@gmail.com>
---
Changes from v2 to v3:
- Used imperative mood instead of "This patch adds".
- Dropped "I am submitting this patch to..." as it is redundant.
- Removed the paragraph related to the related patch sent to the 
  linux-input mailing list as it is not necessary.
- Renamed the hold-in-reset-in-suspend function to 
  goodix-hold-in-reset to prevent potential naming conflicts with other 
  functions in the codebase. No functional changes were made.

Changes from v1 to v2:
- Updated subject prefix to match subsystem.
- Added more detailed description of the change.
- Fixed formatting issues in commit message.
 .../devicetree/bindings/input/touchscreen/goodix.yaml     | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
index 3d016b87c8df..197f8db9acc2 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
+++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
@@ -56,6 +56,13 @@ properties:
   touchscreen-size-y: true
   touchscreen-swapped-x-y: true
 
+  goodix-hold-in-reset:
+    description: |
+      When set to true, the touchscreen controller will be held in reset mode
+      during system suspend. This can help reduce power consumption, but may
+      cause the touchscreen to take longer to resume when the system is woken
+      up from suspend.
+
 additionalProperties: false
 
 required:
@@ -75,6 +82,7 @@ examples:
         interrupts = <0 0>;
         irq-gpios = <&gpio1 0 0>;
         reset-gpios = <&gpio1 1 0>;
+        goodix-hold-in-reset;
       };
     };
 
-- 
2.34.3

Re: [PATCH v3 1/2] dt-bindings: input: touchscreen: Add 'goodix-hold-in-reset' property to Goodix

[Krzysztof Kozlowski]

On 16/03/2023 16:47, Jan Jasper de Kroon wrote:
> Add an optional 'goodix-hold-in-reset', to the Goodix touchscreen
> device tree binding. When set to true, the touchscreen controller will
> be held in reset mode during system suspend, reducing power consumption.
> If not present, the property defaults to false.
> 
> Signed-off-by: Jan Jasper de Kroon <jajadekroon@gmail.com>

Duplicated message, so just for formality: discussion did not finish.

Best regards,
Krzysztof