Received-SPF: pass (google.com: domain of linux-kernel+bounces-104816-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Fri, 15 Mar 2024 11:51:19 -0700
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
To: Stefan Eichenberger <eichest@gmail.com>
Cc: nick@shmanahar.org, robh+dt@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
	nicolas.ferre@microchip.com, alexandre.belloni@bootlin.com,
	claudiu.beznea@tuxon.dev, linus.walleij@linaro.org,
	linux-input@vger.kernel.org, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
	francesco.dolcini@toradex.com,
	Stefan Eichenberger <stefan.eichenberger@toradex.com>
Subject: Re: [PATCH RESEND v3 2/2] Input: atmel_mxt_ts - support poweroff in
 suspend
Message-ID: <ZfSYp6aV6bRhlPUJ@google.com>
References: <20240209105012.22470-1-eichest@gmail.com>
 <20240209105012.22470-3-eichest@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240209105012.22470-3-eichest@gmail.com>

Hi Stefan,

On Fri, Feb 09, 2024 at 11:50:12AM +0100, Stefan Eichenberger wrote:
> From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> 
> Add a new device tree property to indicate that the device should be
> powered off in suspend mode. We have a shared regulator that powers the
> display, a USB hub and some other peripherals. The maXTouch controller
> doesn't normally disable the regulator in suspend mode, so our extra
> peripherals stay powered on. This is not desirable as it consumes more
> power. With this patch we add the option to disable the regulator in
> suspend mode for the maXTouch and accept the longer initialisation time.
> 
> Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c | 72 ++++++++++++++++++------
>  1 file changed, 55 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 542a31448c8f..2d5655385702 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;
>  
>  	/* Cached parameters from object table */
>  	u16 T5_address;
> @@ -2799,15 +2800,18 @@ static int mxt_configure_objects(struct mxt_data *data,
>  			dev_warn(dev, "Error %d updating config\n", error);
>  	}
>  
> -	if (data->multitouch) {
> -		error = mxt_initialize_input_device(data);
> -		if (error)
> -			return error;
> -	} else {
> -		dev_warn(dev, "No touch object detected\n");
> -	}
> +	/* If input device is not already registered */
> +	if (!data->input_dev) {
> +		if (data->multitouch) {
> +			error = mxt_initialize_input_device(data);
> +			if (error)
> +				return error;
> +		} else {
> +			dev_warn(dev, "No touch object detected\n");
> +		}
>  
> -	mxt_debug_init(data);
> +		mxt_debug_init(data);
> +	}
>  
>  	return 0;
>  }
> @@ -3325,6 +3329,8 @@ static int mxt_probe(struct i2c_client *client)
>  		msleep(MXT_RESET_INVALID_CHG);
>  	}
>  
> +	data->poweroff_sleep = device_property_read_bool(&client->dev,
> +							 "atmel,poweroff-sleep");
>  	/*
>  	 * Controllers like mXT1386 have a dedicated WAKE line that could be
>  	 * connected to a GPIO or to I2C SCL pin, or permanently asserted low.
> @@ -3387,12 +3393,21 @@ static int mxt_suspend(struct device *dev)
>  	if (!input_dev)
>  		return 0;
>  
> -	mutex_lock(&input_dev->mutex);
> +	if (!device_may_wakeup(dev) && data->poweroff_sleep) {
> +		if (data->reset_gpio)
> +			gpiod_set_value(data->reset_gpio, 1);
>  
> -	if (input_device_enabled(input_dev))
> -		mxt_stop(data);
> +		regulator_bulk_disable(ARRAY_SIZE(data->regulators),
> +				data->regulators);
> +		data->T44_address = 0;
> +	} else {
> +		mutex_lock(&input_dev->mutex);
> +
> +		if (input_device_enabled(input_dev))
> +			mxt_stop(data);
>  
> -	mutex_unlock(&input_dev->mutex);
> +		mutex_unlock(&input_dev->mutex);
> +	}

This all should go into mxt_stop(), so that if device is closed, or
inhibited, you power it down as well (if you can).

>  
>  	disable_irq(data->irq);
>  
> @@ -3408,14 +3423,37 @@ static int mxt_resume(struct device *dev)
>  	if (!input_dev)
>  		return 0;
>  
> -	enable_irq(data->irq);
> +	if (!device_may_wakeup(dev) && data->poweroff_sleep) {
> +		int ret;
>  
> -	mutex_lock(&input_dev->mutex);
> +		ret = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
> +				data->regulators);
> +		if (ret) {
> +			dev_err(dev, "failed to enable regulators: %d\n",
> +					ret);
> +			return ret;
> +		}
> +		msleep(MXT_BACKUP_TIME);
>  
> -	if (input_device_enabled(input_dev))
> -		mxt_start(data);
> +		if (data->reset_gpio) {
> +			/* Wait a while and then de-assert the RESET GPIO line */
> +			msleep(MXT_RESET_GPIO_TIME);
> +			gpiod_set_value(data->reset_gpio, 0);
> +			msleep(MXT_RESET_INVALID_CHG);
> +		}
>  
> -	mutex_unlock(&input_dev->mutex);
> +		/* This also enables the irq again */
> +		mxt_initialize(data);

And this needs to go into mxt_start(). Also, we should make sure that
once resume operation completes the device is fully operational. That
means you should not request the firmware asynchronously in
mxt_initialize() in case you are in the resume path. I think you should
also unwind mxt_initialize() and mxt_configure_objects() to make it
clear what is the part of initial initialization and what is part of
re-initializing during resume. The configuration that is exposed to
userspace (resolution, number of objects, other properties) should stay
the same, the configuration of the chip itself (power mode, etc) should
be restored.

Thanks.

-- 
Dmitry