diff mbox series

Input: elan_i2c - Implement inhibit/uninhibit functions.

Message ID 20230320011456.986321-1-jingle.wu@emc.com.tw (mailing list archive)
State Superseded
Headers show
Series Input: elan_i2c - Implement inhibit/uninhibit functions. | expand

Commit Message

Jingle.Wu March 20, 2023, 1:14 a.m. UTC 
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(+)


base-commit: 38e04b3e4240a6d8fb43129ebad41608db64bc6f

Comments

Phoenix March 28, 2023, 8:54 a.m. UTC | #1 
Hi Dmitry,

No response from you yet,
Can you help review this patch, thanks

Best regards,
Phoenix Huang
-----Original Message-----
From: jingle.wu [mailto:jingle.wu@emc.com.tw] 
Sent: Monday, March 20, 2023 9:15 AM
To: linux-kernel@vger.kernel.org; linux-input@vger.kernel.org;
dmitry.torokhov@gmail.com
Cc: phoenix@emc.com.tw; josh.chen@emc.com.tw; dave.wang@emc.com.tw;
jingle.wu <jingle.wu@emc.com.tw>
Subject: [PATCH] Input: elan_i2c - Implement inhibit/uninhibit functions.

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;
+
+	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;
+}
+
+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;
+
+	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);
+
+}
+
+static int elan_uninhibit(struct input_dev *input_dev) {
+	struct elan_tp_data *data = input_get_drvdata(input_dev);
+	struct i2c_client *client = data->client;
+	int ret;
+
+	dev_dbg(&client->dev, "uninhibiting\n");
+	ret = mutex_lock_interruptible(&data->sysfs_mutex);
+	if (ret)
+		return ret;
+
+	ret = elan_reactivate(data);
+	if (ret == 0)
+		enable_irq(client->irq);
+
+	mutex_unlock(&data->sysfs_mutex);
+
+	return ret;
+}
+
+static int elan_open(struct input_dev *input_dev) {
+	if ((input_dev->users) && (input_dev->inhibited))
+		return elan_uninhibit(input_dev);
+
+	return 0;
+}
+
 static int elan_query_device_info(struct elan_tp_data *data)  {
 	int error;
@@ -1175,6 +1258,9 @@ static int elan_setup_input_device(struct elan_tp_data
*data)
 				     0, ETP_FINGER_WIDTH * min_width, 0, 0);
 	}
 
+	input->open = elan_open;
+	input->close = elan_close;
+
 	data->input = input;
 
 	return 0;

base-commit: 38e04b3e4240a6d8fb43129ebad41608db64bc6f
--
2.34.1
Dmitry Torokhov April 7, 2023, 4:57 p.m. UTC | #2 
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.
Jingle.Wu April 10, 2023, 1:26 a.m. UTC | #3 
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@gmail.com] 
Sent: Saturday, April 8, 2023 12:57 AM
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.

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
Dmitry Torokhov April 26, 2023, 10:57 p.m. UTC | #4 
Hi Jingle,

On Mon, Apr 10, 2023 at 09:26:04AM +0800, Jingle.Wu wrote:
> 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.

I do not see why this would be an issue if what you are doing is putting
the device into a low power mode.

If this issue is about need to re-calibrate after opening the lid on
certain devices, then I think we need to do the same that we did for the
I2C-HID connected devices on Redrix and hook this functionality to a LID
handler.

Thanks.
Jingle.Wu April 28, 2023, 2:21 a.m. UTC | #5 
Hi Dmitry:
	During the initial process and when the users open/close device,
having the elan uninhibit/inhibit commands (low power mode) was not what
Elan expects to happen. Due to that touchpad would do the calibration in
uninhibit moment , we don't want the calibration to be affected by fingers
on the touchpad.
	However, the LID inhibit/uninhibit functions in the Linux kernel
driver calls open/close(), so we need to separate the inhibit/uninhibit
behavior from open/close() function
	
https://elixir.bootlin.com/linux/latest/source/drivers/input/input.c#L1783
	
https://elixir.bootlin.com/linux/latest/source/drivers/input/input.c#L1813

THANKS
JINGLE

-----Original Message-----
From: 'Dmitry Torokhov' [mailto:dmitry.torokhov@gmail.com] 
Sent: Thursday, April 27, 2023 6:58 AM
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.

Hi Jingle,

On Mon, Apr 10, 2023 at 09:26:04AM +0800, Jingle.Wu wrote:
> 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.

I do not see why this would be an issue if what you are doing is putting the
device into a low power mode.

If this issue is about need to re-calibrate after opening the lid on certain
devices, then I think we need to do the same that we did for the I2C-HID
connected devices on Redrix and hook this functionality to a LID handler.

Thanks.

--
Dmitry
Dmitry Torokhov May 12, 2023, 12:07 a.m. UTC | #6 
Hi Jingle,

On Fri, Apr 28, 2023 at 10:21:53AM +0800, Jingle.Wu wrote:
> Hi Dmitry:
> 	During the initial process and when the users open/close device,
> having the elan uninhibit/inhibit commands (low power mode) was not what
> Elan expects to happen. Due to that touchpad would do the calibration in
> uninhibit moment , we don't want the calibration to be affected by fingers
> on the touchpad.
> 	However, the LID inhibit/uninhibit functions in the Linux kernel
> driver calls open/close(), so we need to separate the inhibit/uninhibit
> behavior from open/close() function
> 	
> https://elixir.bootlin.com/linux/latest/source/drivers/input/input.c#L1783
> 	
> https://elixir.bootlin.com/linux/latest/source/drivers/input/input.c#L1813

You quite intentionally can not separate inhibit/uninhibit from open and
close. As with open/close nothing stops inhibit/uninhibit to be called
rapidly multiple times in a row. You probably rely on particular
operation of ChromeOS devices during normal user interaction.

As far as I remember, you need to perform recalibration on certain
devices when LID gets open. I recommend you implement something similar
to https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/3578852
where we monitor LID events and recalibrate when needed, instead of
trying to make assumptions on when inhibit and uninhibit functions are
called.

Thanks.
diff mbox series

Patch

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;
+
+	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;
+}
+
+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;
+
+	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);
+
+}
+
+static int elan_uninhibit(struct input_dev *input_dev)
+{
+	struct elan_tp_data *data = input_get_drvdata(input_dev);
+	struct i2c_client *client = data->client;
+	int ret;
+
+	dev_dbg(&client->dev, "uninhibiting\n");
+	ret = mutex_lock_interruptible(&data->sysfs_mutex);
+	if (ret)
+		return ret;
+
+	ret = elan_reactivate(data);
+	if (ret == 0)
+		enable_irq(client->irq);
+
+	mutex_unlock(&data->sysfs_mutex);
+
+	return ret;
+}
+
+static int elan_open(struct input_dev *input_dev)
+{
+	if ((input_dev->users) && (input_dev->inhibited))
+		return elan_uninhibit(input_dev);
+
+	return 0;
+}
+
 static int elan_query_device_info(struct elan_tp_data *data)
 {
 	int error;
@@ -1175,6 +1258,9 @@  static int elan_setup_input_device(struct elan_tp_data *data)
 				     0, ETP_FINGER_WIDTH * min_width, 0, 0);
 	}
 
+	input->open = elan_open;
+	input->close = elan_close;
+
 	data->input = input;
 
 	return 0;