linux-kernel - Re: [PATCH v4 4/4] Input: atmel_mxt_ts

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZnOCDaetFnsg09if@google.com>
Date: Wed, 19 Jun 2024 18:12:45 -0700
From: Dmitry Torokhov <dmitry.torokhov@...il.com>
To: Stefan Eichenberger <eichest@...il.com>
Cc: nick@...anahar.org, robh@...nel.org, krzysztof.kozlowski+dt@...aro.org,
	conor+dt@...nel.org, nicolas.ferre@...rochip.com,
	alexandre.belloni@...tlin.com, claudiu.beznea@...on.dev,
	linus.walleij@...aro.org, linux-input@...r.kernel.org,
	devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org,
	Stefan Eichenberger <stefan.eichenberger@...adex.com>
Subject: Re: [PATCH v4 4/4] Input: atmel_mxt_ts - add support for
 poweroff-sleep

Hi Stefan,

On Wed, Apr 17, 2024 at 11:05:27AM +0200, Stefan Eichenberger wrote:
> From: Stefan Eichenberger <stefan.eichenberger@...adex.com>
> 
> Add support for poweroff-sleep to the Atmel maXTouch driver. This allows
> us to power off the input device entirely and only power it on when it
> is opened. This will also automatically power it off when we suspend the
> system.
> 
> Signed-off-by: Stefan Eichenberger <stefan.eichenberger@...adex.com>
> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c | 71 +++++++++++++++++++-----
>  1 file changed, 57 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 7c807d1f1f9b..f92808be3f5b 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -317,6 +317,7 @@ struct mxt_data {
>  	struct gpio_desc *reset_gpio;
>  	struct gpio_desc *wake_gpio;
>  	bool use_retrigen_workaround;
> +	bool poweroff_sleep;

Why is this separate from "enum mxt_suspend_mode suspend_mode"? Can we
make MXT_SUSPEND_POWEROFF and use it in mxt_start() and others? It still
can be driven by the "atmel,poweroff-sleep" device property. 

>  
>  	/* Cached parameters from object table */
>  	u16 T5_address;
> @@ -2277,6 +2278,19 @@ static void mxt_config_cb(const struct firmware *cfg, void *ctx)
>  	release_firmware(cfg);
>  }
>  
> +static int mxt_initialize_after_resume(struct mxt_data *data)
> +{
> +	const struct firmware *fw;
> +
> +	mxt_acquire_irq(data);
> +
> +	firmware_request_nowarn(&fw, MXT_CFG_NAME, &data->client->dev);
> +
> +	mxt_config_cb(fw, data);

Is this really required? As far as I know all maXTouch controllers have
NVRAM for their configs and should not lose configuration even if power
is cut off. In fact, the whole automatic request of firmware/config upon
probe I think was a mistake and I would like to get rid of it. In fact,
on Chrome OS the version of the driver in use does not do that and
instead relies on userspace to check if firmware update is needed.

If this is actually required you need to add error handling.

Thanks.

-- 
Dmitry