Hi Dimitry, On Wed, Jun 19, 2024 at 06:12:45PM -0700, Dmitry Torokhov wrote: > Hi Stefan, > > On Wed, Apr 17, 2024 at 11:05:27AM +0200, Stefan Eichenberger wrote: > > From: Stefan Eichenberger <stefan.eichenberger@xxxxxxxxxxx> > > > > 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@xxxxxxxxxxx> > > --- > > 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. I agree and will change this in the next version. > > > > /* 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. You are right, and the configuration goes to non-volatile memory, so this is not needed. I will remove the firmware loading here and improve the error handling. I will keep the firmware loading in the initial probe for now, to be consistent with the non poweroff-sleep version. Regards, Stefan