diff mbox series

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

Message ID 20230312183106.551840-1-jajadekroon@gmail.com (mailing list archive)
State New, archived
Headers show
Series [v2,1/2] dt-bindings: input: touchscreen: Add 'hold-in-reset-in-suspend' property to goodix | expand

Commit Message

Jan Jasper de Kroon March 12, 2023, 6:31 p.m. UTC
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(+)

Comments

Krzysztof Kozlowski March 14, 2023, 5:47 p.m. UTC | #1
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
Jan Jasper de Kroon March 16, 2023, 3:41 p.m. UTC | #2
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
Krzysztof Kozlowski March 16, 2023, 7:24 p.m. UTC | #3
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
diff mbox series

Patch

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;
       };
     };