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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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