Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 7 Apr 2023 09:57:12 -0700
From:   Dmitry Torokhov <dmitry.torokhov@gmail.com>
To:     "jingle.wu" <jingle.wu@emc.com.tw>
Cc:     linux-kernel@vger.kernel.org, linux-input@vger.kernel.org,
        phoenix@emc.com.tw, josh.chen@emc.com.tw, dave.wang@emc.com.tw
Subject: Re: [PATCH] Input: elan_i2c - Implement inhibit/uninhibit functions.
Message-ID: <ZDBKwo4UMUm+TSnj@penguin>
References: <20230320011456.986321-1-jingle.wu@emc.com.tw>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230320011456.986321-1-jingle.wu@emc.com.tw>
Precedence: bulk

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@emc.com.tw>
> ---
>  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