RE: [PATCH] Input: elan_i2c - Implement inhibit/uninhibit functions.

[Jingle.Wu]

HI Dmitry:

> +static void elan_close(struct input_dev *input_dev) {
> +	if ((input_dev->users) && (!input_dev->inhibited))
> +		elan_inhibit(input_dev);

This check is for "only inhibit request", and elan_open() its check is for
"only uninhibit request".
Because input_dev-> open() close() will be executed 2-3 times when initial.

Thanks
Jingle

-----Original Message-----
From: Dmitry Torokhov [mailto:dmitry.torokhov@xxxxxxxxx] 
Sent: Saturday, April 8, 2023 12:57 AM
To: jingle.wu <jingle.wu@xxxxxxxxxx>
Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-input@xxxxxxxxxxxxxxx;
phoenix@xxxxxxxxxx; josh.chen@xxxxxxxxxx; dave.wang@xxxxxxxxxx
Subject: Re: [PATCH] Input: elan_i2c - Implement inhibit/uninhibit
functions.

Hi Jingle,

On Mon, Mar 20, 2023 at 09:14:56AM +0800, jingle.wu wrote:
> Add inhibit/uninhibit functions.
> 
> Signed-off-by: Jingle.wu <jingle.wu@xxxxxxxxxx>
> ---
>  drivers/input/mouse/elan_i2c_core.c | 86 
> +++++++++++++++++++++++++++++
>  1 file changed, 86 insertions(+)
> 
> diff --git a/drivers/input/mouse/elan_i2c_core.c 
> b/drivers/input/mouse/elan_i2c_core.c
> index 5f0d75a45c80..b7100945c9cc 100644
> --- a/drivers/input/mouse/elan_i2c_core.c
> +++ b/drivers/input/mouse/elan_i2c_core.c
> @@ -329,6 +329,89 @@ static int elan_initialize(struct elan_tp_data *data,
bool skip_reset)
>  	return error;
>  }
>  
> +static int elan_reactivate(struct elan_tp_data *data) {
> +	struct device *dev = &data->client->dev;
> +	int ret;

Please call this variable and other similar ones "error".

> +
> +	ret = elan_set_power(data, true);
> +	if (ret)
> +		dev_err(dev, "failed to restore power: %d\n", ret);
> +
> +	ret = data->ops->sleep_control(data->client, false);
> +	if (ret) {
> +		dev_err(dev,
> +			"failed to wake device up: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return ret;

return 0;

> +}
> +
> +static void elan_inhibit(struct input_dev *input_dev) {
> +	struct elan_tp_data *data = input_get_drvdata(input_dev);
> +	struct i2c_client *client = data->client;
> +	int ret;
> +
> +	if (data->in_fw_update)
> +		return;

Simply and silently ignoring inhibit request is not great. Can we wait for
firmware update to complete?

> +
> +	dev_dbg(&client->dev, "inhibiting\n");
> +	/*
> +	 * We are taking the mutex to make sure sysfs operations are
> +	 * complete before we attempt to bring the device into low[er]
> +	 * power mode.
> +	 */
> +	ret = mutex_lock_interruptible(&data->sysfs_mutex);
> +	if (ret)
> +		return;
> +
> +	disable_irq(client->irq);
> +
> +	ret = elan_set_power(data, false);
> +	if (ret)
> +		enable_irq(client->irq);
> +
> +	mutex_unlock(&data->sysfs_mutex);
> +
> +}
> +
> +static void elan_close(struct input_dev *input_dev) {
> +	if ((input_dev->users) && (!input_dev->inhibited))
> +		elan_inhibit(input_dev);

I am not sure why you need these checks. Input core will only call
input_dev->close() when device is powered up st (i.e. it is not inhibited
and there are users of it) and when either:

- there is inhibit request or
- the last user is letting go of the device

Similarly elan_open() will be called when first user opens device if device
is not inhibited, or when request to uninhibit comes for inhibited device
that has users.

But you need to make sure you start in a low power state.

Thanks.

--
Dmitry