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

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

[jingle.wu]

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

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

[phoenix]

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

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

[Dmitry Torokhov]

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

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@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

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

['Dmitry Torokhov']

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

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

[Jingle.Wu]

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

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

['Dmitry Torokhov']

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.

-- 
Dmitry

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

['Dmitry Torokhov']

Hi Jingle,

On Wed, Jul 05, 2023 at 02:09:18PM +0800, Jingle.Wu wrote:
> HI Dmitry:
> 
> 1.
> > +static void elan_input_lid_event(struct input_handle *handle, 
> > +unsigned
> int type,
> > +			     unsigned int code, int value) {
> > +	struct elan_tp_data *data, *n;
> > +
> > +	if (type == EV_SW && code == SW_LID) {
> > +		list_for_each_entry_safe(data, n,
> &elan_devices_with_lid_handler, 
> > +list) {
> 
> Why do you need the "_safe()" variant here?
> -> Because I took the above link as reference.  
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/3
> 578852/7/drivers/hid/hid-multitouch.c#394

It is wrong there too. The _safe() variant protects list traversal when
the list is being modified by the same thread, here we do not do that.

> 
> 2. 
> > +	data->lid_switch = true;
> > +	INIT_LIST_HEAD(&data->list);
> > +	INIT_WORK(&data->lid_work, lid_work_handler);
> > +	list_add_tail(&data->list, &elan_devices_with_lid_handler);
> 
> It looks like you call elan_create_lid_handler() from elan_probe() which
> means it can be called several times (we should not assume there is only one
> controller), I do not see it being destroyed in remove() either, so it will
> break if you bind/unbind the driver.
> 
> I also not sure why you need the list of you have a handler per device.
> 
> -> If the elan_create_lid_handler() function not be put into probe(), the
> lid
> handler would be invalid in elan private data("struct elan_tp_data *data").
> Or do you have any suggestions for it? Thanks.

The handler's connect() is called for each matching device so you can
tie it up at that time.

> 
> 3.
> > +	error = elan_create_lid_handler(data);
> > +	if (error)
> > +		dev_err(dev, "failed to create lid handler: %d\n", error);
> 
> Do we need this on _ALL_ devices with ELan controllers, or just certain
> ones? If we need this on all devices how did it work before?
> 
> -> Yes, we need this on all device.
> In Chromeos kernel v5.4 before, the elan_inhibit()/uninhibit function was
> built and worked
> successfully in elan_i2c_driver.
> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/refs/heads
> /chromeos-5.4/drivers/input/mouse/elan_i2c_core.c#353

We have the Elan touchpad driver without this functionality for many
years. I am aware that certain devices need this, but the fact that
Chrome OS kernel 5.4 (which is only used on a subset of Chromebooks) has
it does not necessarily mean that this functionality is needed on _ALL_
devices.

Thanks.

-- 
Dmitry

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

[Jingle.Wu]

HI Dmitry:

...
module_i2c_driver(elan_driver);

static int __init elan_init(void)
{
	return elan_create_lid_handler();
}

static void __exit elan_exit(void)
{
	if (elan_mode_wq)
		elan_destroy_lid_handler();
}

module_init(elan_init);
module_exit(elan_exit);
...

If I change to elan_create_lid_handler() at module_init, complie error as
follow.

<pre><b>./include/linux/module.h:130:49:</b> <font color="#C01C28"><b>error:
</b></font>redefinition of '<b>__inittest</b>'
  130 |         static inline initcall_t __maybe_unused <font
color="#C01C28"><b>__inittest</b></font>(void)                \
      |                                                 <font
color="#C01C28"><b>^~~~~~~~~~</b></font>
<b>drivers/input/mouse/elan_i2c_core.c:1666:1:</b> <font
color="#2AA1B3"><b>note: </b></font>in expansion of macro
'<b>module_init</b>'
 1666 | <font color="#2AA1B3"><b>module_init</b></font>(elan_init);
      | <font color="#2AA1B3"><b>^~~~~~~~~~~</b></font>
<b>./include/linux/module.h:130:49:</b> <font color="#2AA1B3"><b>note:
</b></font>previous definition of '<b>__inittest</b>' with type '<b>int
(*(void))(void)</b>'
  130 |         static inline initcall_t __maybe_unused <font
color="#2AA1B3"><b>__inittest</b></font>(void)                \
      |                                                 <font
color="#2AA1B3"><b>^~~~~~~~~~</b></font>
<b>./include/linux/device/driver.h:266:1:</b> <font color="#2AA1B3"><b>note:
</b></font>in expansion of macro '<b>module_init</b>'
  266 | <font color="#2AA1B3"><b>module_init</b></font>(__driver##_init); \
      | <font color="#2AA1B3"><b>^~~~~~~~~~~</b></font>
<b>./include/linux/i2c.h:956:9:</b> <font color="#2AA1B3"><b>note:
</b></font>in expansion of macro '<b>module_driver</b>'
  956 |         <font
color="#2AA1B3"><b>module_driver</b></font>(__i2c_driver, i2c_add_driver, \
      |         <font color="#2AA1B3"><b>^~~~~~~~~~~~~</b></font>
<b>drivers/input/mouse/elan_i2c_core.c:1653:1:</b> <font
color="#2AA1B3"><b>note: </b></font>in expansion of macro
'<b>module_i2c_driver</b>'
 1653 | <font color="#2AA1B3"><b>module_i2c_driver</b></font>(elan_driver);
      | <font color="#2AA1B3"><b>^~~~~~~~~~~~~~~~~</b></font>
<b>./include/linux/module.h:132:13:</b> <font color="#C01C28"><b>error:
</b></font>redefinition of '<b>init_module</b>'
  132 |         int <font color="#C01C28"><b>init_module</b></font>(void)
__copy(initfn)                    \
      |             <font color="#C01C28"><b>^~~~~~~~~~~</b></font>
<b>drivers/input/mouse/elan_i2c_core.c:1666:1:</b> <font
color="#2AA1B3"><b>note: </b></font>in expansion of macro
'<b>module_init</b>'
 1666 | <font color="#2AA1B3"><b>module_init</b></font>(elan_init);
      | <font color="#2AA1B3"><b>^~~~~~~~~~~</b></font>
<b>./include/linux/module.h:132:13:</b> <font color="#2AA1B3"><b>note:
</b></font>previous definition of '<b>init_module</b>' with type
'<b>int(void)</b>'
  132 |         int <font color="#2AA1B3"><b>init_module</b></font>(void)
__copy(initfn)                    \
      |             <font color="#2AA1B3"><b>^~~~~~~~~~~</b></font>
<b>./include/linux/device/driver.h:266:1:</b> <font color="#2AA1B3"><b>note:
</b></font>in expansion of macro '<b>module_init</b>'
  266 | <font color="#2AA1B3"><b>module_init</b></font>(__driver##_init); \
      | <font color="#2AA1B3"><b>^~~~~~~~~~~</b></font>
<b>./include/linux/i2c.h:956:9:</b> <font color="#2AA1B3"><b>note:
</b></font>in expansion of macro '<b>module_driver</b>'
  956 |         <font
color="#2AA1B3"><b>module_driver</b></font>(__i2c_driver, i2c_add_driver, \
      |         <font color="#2AA1B3"><b>^~~~~~~~~~~~~</b></font>
<b>drivers/input/mouse/elan_i2c_core.c:1653:1:</b> <font
color="#2AA1B3"><b>note: </b></font>in expansion of macro
'<b>module_i2c_driver</b>'
 1653 | <font color="#2AA1B3"><b>module_i2c_driver</b></font>(elan_driver);
      | <font color="#2AA1B3"><b>^~~~~~~~~~~~~~~~~</b></font>
<b>./include/linux/module.h:138:49:</b> <font color="#C01C28"><b>error:
</b></font>redefinition of '<b>__exittest</b>'
  138 |         static inline exitcall_t __maybe_unused <font
color="#C01C28"><b>__exittest</b></font>(void)                \
      |                                                 <font
color="#C01C28"><b>^~~~~~~~~~</b></font>
<b>drivers/input/mouse/elan_i2c_core.c:1667:1:</b> <font
color="#2AA1B3"><b>note: </b></font>in expansion of macro
'<b>module_exit</b>'
 1667 | <font color="#2AA1B3"><b>module_exit</b></font>(elan_exit);
      | <font color="#2AA1B3"><b>^~~~~~~~~~~</b></font>
<b>./include/linux/module.h:138:49:</b> <font color="#2AA1B3"><b>note:
</b></font>previous definition of '<b>__exittest</b>' with type '<b>void
(*(void))(void)</b>'
  138 |         static inline exitcall_t __maybe_unused <font
color="#2AA1B3"><b>__exittest</b></font>(void)                \
      |                                                 <font
color="#2AA1B3"><b>^~~~~~~~~~</b></font>
<b>./include/linux/device/driver.h:271:1:</b> <font color="#2AA1B3"><b>note:
</b></font>in expansion of macro '<b>module_exit</b>'
  271 | <font color="#2AA1B3"><b>module_exit</b></font>(__driver##_exit);
      | <font color="#2AA1B3"><b>^~~~~~~~~~~</b></font>
<b>./include/linux/i2c.h:956:9:</b> <font color="#2AA1B3"><b>note:
</b></font>in expansion of macro '<b>module_driver</b>'
  956 |         <font
color="#2AA1B3"><b>module_driver</b></font>(__i2c_driver, i2c_add_driver, \
      |         <font color="#2AA1B3"><b>^~~~~~~~~~~~~</b></font>
<b>drivers/input/mouse/elan_i2c_core.c:1653:1:</b> <font
color="#2AA1B3"><b>note: </b></font>in expansion of macro
'<b>module_i2c_driver</b>'
 1653 | <font color="#2AA1B3"><b>module_i2c_driver</b></font>(elan_driver);
      | <font color="#2AA1B3"><b>^~~~~~~~~~~~~~~~~</b></font>
<b>./include/linux/module.h:140:14:</b> <font color="#C01C28"><b>error:
</b></font>redefinition of '<b>cleanup_module</b>'
  140 |         void <font
color="#C01C28"><b>cleanup_module</b></font>(void) __copy(exitfn)
\
      |              <font color="#C01C28"><b>^~~~~~~~~~~~~~</b></font>
<b>drivers/input/mouse/elan_i2c_core.c:1667:1:</b> <font
color="#2AA1B3"><b>note: </b></font>in expansion of macro
'<b>module_exit</b>'
 1667 | <font color="#2AA1B3"><b>module_exit</b></font>(elan_exit);
      | <font color="#2AA1B3"><b>^~~~~~~~~~~</b></font>
<b>./include/linux/module.h:140:14:</b> <font color="#2AA1B3"><b>note:
</b></font>previous definition of '<b>cleanup_module</b>' with type
'<b>void(void)</b>'
  140 |         void <font
color="#2AA1B3"><b>cleanup_module</b></font>(void) __copy(exitfn)
\
      |              <font color="#2AA1B3"><b>^~~~~~~~~~~~~~</b></font>
<b>./include/linux/device/driver.h:271:1:</b> <font color="#2AA1B3"><b>note:
</b></font>in expansion of macro '<b>module_exit</b>'
  271 | <font color="#2AA1B3"><b>module_exit</b></font>(__driver##_exit);
      | <font color="#2AA1B3"><b>^~~~~~~~~~~</b></font>
<b>./include/linux/i2c.h:956:9:</b> <font color="#2AA1B3"><b>note:
</b></font>in expansion of macro '<b>module_driver</b>'
  956 |         <font
color="#2AA1B3"><b>module_driver</b></font>(__i2c_driver, i2c_add_driver, \
      |         <font color="#2AA1B3"><b>^~~~~~~~~~~~~</b></font>
<b>drivers/input/mouse/elan_i2c_core.c:1653:1:</b> <font
color="#2AA1B3"><b>note: </b></font>in expansion of macro
'<b>module_i2c_driver</b>'
 1653 | <font color="#2AA1B3"><b>module_i2c_driver</b></font>(elan_driver);
      | <font color="#2AA1B3"><b>^~~~~~~~~~~~~~~~~</b></font>
</pre>

THANJS
JINGLE
-----Original Message-----
From: Jingle.Wu [mailto:jingle.wu@emc.com.tw] 
Sent: Wednesday, July 5, 2023 2:09 PM
To: 'Dmitry Torokhov' <dmitry.torokhov@gmail.com>
Cc: 'linux-kernel@vger.kernel.org' <linux-kernel@vger.kernel.org>;
'linux-input@vger.kernel.org' <linux-input@vger.kernel.org>;
'phoenix@emc.com.tw' <phoenix@emc.com.tw>; 'josh.chen@emc.com.tw'
<josh.chen@emc.com.tw>; 'dave.wang@emc.com.tw' <dave.wang@emc.com.tw>
Subject: RE: [PATCH] Input: elan_i2c - Implement inhibit/uninhibit
functions.

HI Dmitry:

1.
> +static void elan_input_lid_event(struct input_handle *handle, 
> +unsigned
int type,
> +			     unsigned int code, int value) {
> +	struct elan_tp_data *data, *n;
> +
> +	if (type == EV_SW && code == SW_LID) {
> +		list_for_each_entry_safe(data, n,
&elan_devices_with_lid_handler, 
> +list) {

Why do you need the "_safe()" variant here?
-> Because I took the above link as reference.  
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/3
578852/7/drivers/hid/hid-multitouch.c#394

2. 
> +	data->lid_switch = true;
> +	INIT_LIST_HEAD(&data->list);
> +	INIT_WORK(&data->lid_work, lid_work_handler);
> +	list_add_tail(&data->list, &elan_devices_with_lid_handler);

It looks like you call elan_create_lid_handler() from elan_probe() which
means it can be called several times (we should not assume there is only one
controller), I do not see it being destroyed in remove() either, so it will
break if you bind/unbind the driver.

I also not sure why you need the list of you have a handler per device.

-> If the elan_create_lid_handler() function not be put into probe(), 
-> the lid
handler would be invalid in elan private data("struct elan_tp_data *data").
Or do you have any suggestions for it? Thanks.

3.
> +	error = elan_create_lid_handler(data);
> +	if (error)
> +		dev_err(dev, "failed to create lid handler: %d\n", error);

Do we need this on _ALL_ devices with ELan controllers, or just certain
ones? If we need this on all devices how did it work before?

-> Yes, we need this on all device.
In Chromeos kernel v5.4 before, the elan_inhibit()/uninhibit function was
built and worked successfully in elan_i2c_driver.
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/refs/heads
/chromeos-5.4/drivers/input/mouse/elan_i2c_core.c#353


Thanks
JINGLE

-----Original Message-----
From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
Sent: Friday, June 30, 2023 6: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 Wed, May 31, 2023 at 05:03:40PM +0800, jingle.wu wrote:
>  Add inhibit/uninhibit functions.

You need to provide justification for this change, i.e. explain why you need
this/what is currently not working or working sub-optimally.

> 
>  Signed-off-by: Jingle.wu <jingle.wu@emc.com.tw>
> ---
>  drivers/input/mouse/elan_i2c_core.c | 207
> ++++++++++++++++++++++++++++
>  1 file changed, 207 insertions(+)
> 
> diff --git a/drivers/input/mouse/elan_i2c_core.c
> b/drivers/input/mouse/elan_i2c_core.c
> index 5f0d75a45c80..4ea57f4c7bd4 100644
> --- a/drivers/input/mouse/elan_i2c_core.c
> +++ b/drivers/input/mouse/elan_i2c_core.c
> @@ -56,6 +56,7 @@ struct elan_tp_data {
>  	struct input_dev	*input;
>  	struct input_dev	*tp_input; /* trackpoint input node */
>  	struct regulator	*vcc;
> +	struct list_head list;	/* for list of devices needing input handler
*/
>  
>  	const struct elan_transport_ops *ops;
>  
> @@ -63,6 +64,11 @@ struct elan_tp_data {
>  	struct completion	fw_completion;
>  	bool			in_fw_update;
>  
> +	struct work_struct	lid_work;
> +	bool			lid_switch;
> +	int			lid_value;
> +	bool			in_inhibit;
> +
>  	struct mutex		sysfs_mutex;
>  
>  	unsigned int		max_x;
> @@ -96,6 +102,9 @@ struct elan_tp_data {
>  	u32			quirks;		/* Various quirks */
>  };
>  
> +static struct workqueue_struct *elan_mode_wq; static 
> +LIST_HEAD(elan_devices_with_lid_handler);
> +
>  static u32 elan_i2c_lookup_quirks(u16 ic_type, u16 product_id)  {
>  	static const struct {
> @@ -329,6 +338,74 @@ 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 error;
> +
> +	error = elan_set_power(data, true);
> +	if (error)
> +		dev_err(dev, "failed to restore power: %d\n", error);
> +
> +	error = data->ops->sleep_control(data->client, false);
> +	if (error) {
> +		dev_err(dev,
> +			"failed to wake device up: %d\n", error);
> +		return error;
> +	}
> +
> +	return error;
> +}
> +
> +static int elan_inhibit(struct input_dev *input_dev)

I would rather you did not call it inhibit/uninhibit because this is not
what it does.

Please split the logic for recalibration from logic of powering on and off
the device. I also hope in the future firmware revisions the distinction
will be more clear (i.e. have a separate method/command to recalibrate
baseline on demand).

> +{
> +	struct elan_tp_data *data = input_get_drvdata(input_dev);
> +	struct i2c_client *client = data->client;
> +	int error;
> +
> +	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.
> +	 */
> +	error = mutex_lock_interruptible(&data->sysfs_mutex);
> +	if (error)
> +		return error;
> +
> +	disable_irq(client->irq);
> +
> +	error = elan_set_power(data, false);
> +	if (error)
> +		enable_irq(client->irq);
> +
> +	data->in_inhibit = true;
> +	mutex_unlock(&data->sysfs_mutex);
> +
> +	return error;
> +}
> +
> +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 error;
> +
> +	dev_dbg(&client->dev, "uninhibiting\n");
> +	error = mutex_lock_interruptible(&data->sysfs_mutex);
> +	if (error)
> +		return error;
> +
> +	error = elan_reactivate(data);
> +	if (error == 0)
> +		enable_irq(client->irq);
> +
> +	data->in_inhibit = false;
> +	mutex_unlock(&data->sysfs_mutex);
> +
> +	return error;
> +}
> +
>  static int elan_query_device_info(struct elan_tp_data *data)  {
>  	int error;
> @@ -1187,6 +1264,124 @@ static void elan_disable_regulator(void *_data)
>  	regulator_disable(data->vcc);
>  }
>  
> +static void lid_work_handler(struct work_struct *work) {
> +	struct elan_tp_data *data = container_of(work, struct elan_tp_data,
> +					    lid_work);
> +
> +	if (data->lid_value)
> +		elan_inhibit(data->input);
> +	else
> +		elan_uninhibit(data->input);
> +
> +}
> +
> +static void elan_input_lid_event(struct input_handle *handle, unsigned
int type,
> +			     unsigned int code, int value) {
> +	struct elan_tp_data *data, *n;
> +
> +	if (type == EV_SW && code == SW_LID) {
> +		list_for_each_entry_safe(data, n,
&elan_devices_with_lid_handler,
> +list) {

Why do you need the "_safe()" variant here?

> +			data->lid_value = value;
> +			queue_work(elan_mode_wq, &data->lid_work);
> +		}
> +	}
> +
> +}
> +
> +struct elan_input_lid {
> +	struct input_handle handle;
> +};
> +
> +static int elan_input_lid_connect(struct input_handler *handler,
> +				struct input_dev *dev,
> +				const struct input_device_id *id) {
> +	struct elan_input_lid *lid;
> +	char *name;
> +	int error;
> +
> +	lid = kzalloc(sizeof(*lid), GFP_KERNEL);
> +	if (!lid)
> +		return -ENOMEM;
> +	name = kasprintf(GFP_KERNEL, "elan-i2c-lid-%s",
dev_name(&dev->dev));
> +	if (!name) {
> +		error = -ENOMEM;
> +		goto err_free_lid;
> +	}
> +	lid->handle.dev = dev;
> +	lid->handle.handler = handler;
> +	lid->handle.name = name;
> +	lid->handle.private = lid;
> +	error = input_register_handle(&lid->handle);
> +	if (error)
> +		goto err_free_name;
> +	error = input_open_device(&lid->handle);
> +	if (error)
> +		goto err_unregister_handle;
> +	return 0;
> +err_unregister_handle:
> +	input_unregister_handle(&lid->handle);
> +err_free_name:
> +	kfree(name);
> +err_free_lid:
> +	kfree(lid);
> +	return error;
> +}
> +
> +static void elan_input_lid_disconnect(struct input_handle *handle) {
> +	struct elan_input_lid *lid = handle->private;
> +
> +	input_close_device(handle);
> +	input_unregister_handle(handle);
> +	kfree(handle->name);
> +	kfree(lid);
> +}
> +
> +static const struct input_device_id elan_input_lid_ids[] = {
> +	{
> +		.flags = INPUT_DEVICE_ID_MATCH_EVBIT |
INPUT_DEVICE_ID_MATCH_SWBIT,
> +		.evbit = { BIT_MASK(EV_SW) },
> +		.swbit = { [BIT_WORD(SW_LID)] = BIT_MASK(SW_LID) },
> +	},
> +	{ },
> +};
> +
> +static struct input_handler elan_input_lid_handler = {
> +	.event =	elan_input_lid_event,
> +	.connect =	elan_input_lid_connect,
> +	.disconnect =	elan_input_lid_disconnect,
> +	.name =		"elan-i2c-lid",
> +	.id_table =	elan_input_lid_ids,
> +};
> +
> +static int elan_create_lid_handler(struct elan_tp_data *data) {
> +	int error = 0;
> +
> +	elan_mode_wq = create_singlethread_workqueue("elan-i2c-lid");
> +	if (elan_mode_wq == NULL)
> +		return -ENOMEM;
> +	error = input_register_handler(&elan_input_lid_handler);
> +	if (error)
> +		goto remove_wq;
> +
> +	data->lid_switch = true;
> +	INIT_LIST_HEAD(&data->list);
> +	INIT_WORK(&data->lid_work, lid_work_handler);
> +	list_add_tail(&data->list, &elan_devices_with_lid_handler);

It looks like you call elan_create_lid_handler() from elan_probe() which
means it can be called several times (we should not assume there is only one
controller), I do not see it being destroyed in remove() either, so it will
break if you bind/unbind the driver.

I also not sure why you need the list of you have a handler per device.

> +
> +	return 0;
> +
> +remove_wq:
> +	data->lid_switch = false;
> +	destroy_workqueue(elan_mode_wq);
> +	elan_mode_wq = NULL;
> +	return error;
> +}
> +
>  static int elan_probe(struct i2c_client *client)  {
>  	const struct elan_transport_ops *transport_ops; @@ -1325,6 +1520,10 
> @@ static int elan_probe(struct i2c_client *client)
>  		}
>  	}
>  
> +	error = elan_create_lid_handler(data);
> +	if (error)
> +		dev_err(dev, "failed to create lid handler: %d\n", error);

Do we need this on _ALL_ devices with ELan controllers, or just certain
ones? If we need this on all devices how did it work before?

> +
>  	return 0;
>  }
>  
> @@ -1334,6 +1533,10 @@ static int elan_suspend(struct device *dev)
>  	struct elan_tp_data *data = i2c_get_clientdata(client);
>  	int ret;
>  
> +	/* Wait for switch on completion */
> +	if (data->lid_switch)
> +		flush_workqueue(elan_mode_wq);
> +
>  	/*
>  	 * We are taking the mutex to make sure sysfs operations are
>  	 * complete before we attempt to bring the device into low[er] @@
> -1371,6 +1574,10 @@ static int elan_resume(struct device *dev)
>  	struct elan_tp_data *data = i2c_get_clientdata(client);
>  	int error;
>  
> +	/* Wait for switch on completion */
> +	if (data->lid_switch)
> +		flush_workqueue(elan_mode_wq);
> +
>  	if (!device_may_wakeup(dev)) {
>  		error = regulator_enable(data->vcc);
>  		if (error) {
> --
> 2.34.1
> 

Thanks.

--
Dmitry

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

[Jingle.Wu]

HI Dmitry:

1.
> +static void elan_input_lid_event(struct input_handle *handle, 
> +unsigned
int type,
> +			     unsigned int code, int value) {
> +	struct elan_tp_data *data, *n;
> +
> +	if (type == EV_SW && code == SW_LID) {
> +		list_for_each_entry_safe(data, n,
&elan_devices_with_lid_handler, 
> +list) {

Why do you need the "_safe()" variant here?
-> Because I took the above link as reference.  
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/3
578852/7/drivers/hid/hid-multitouch.c#394

2. 
> +	data->lid_switch = true;
> +	INIT_LIST_HEAD(&data->list);
> +	INIT_WORK(&data->lid_work, lid_work_handler);
> +	list_add_tail(&data->list, &elan_devices_with_lid_handler);

It looks like you call elan_create_lid_handler() from elan_probe() which
means it can be called several times (we should not assume there is only one
controller), I do not see it being destroyed in remove() either, so it will
break if you bind/unbind the driver.

I also not sure why you need the list of you have a handler per device.

-> If the elan_create_lid_handler() function not be put into probe(), the
lid
handler would be invalid in elan private data("struct elan_tp_data *data").
Or do you have any suggestions for it? Thanks.

3.
> +	error = elan_create_lid_handler(data);
> +	if (error)
> +		dev_err(dev, "failed to create lid handler: %d\n", error);

Do we need this on _ALL_ devices with ELan controllers, or just certain
ones? If we need this on all devices how did it work before?

-> Yes, we need this on all device.
In Chromeos kernel v5.4 before, the elan_inhibit()/uninhibit function was
built and worked
successfully in elan_i2c_driver.
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/refs/heads
/chromeos-5.4/drivers/input/mouse/elan_i2c_core.c#353


Thanks
JINGLE

-----Original Message-----
From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com] 
Sent: Friday, June 30, 2023 6: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 Wed, May 31, 2023 at 05:03:40PM +0800, jingle.wu wrote:
>  Add inhibit/uninhibit functions.

You need to provide justification for this change, i.e. explain why you need
this/what is currently not working or working sub-optimally.

> 
>  Signed-off-by: Jingle.wu <jingle.wu@emc.com.tw>
> ---
>  drivers/input/mouse/elan_i2c_core.c | 207 
> ++++++++++++++++++++++++++++
>  1 file changed, 207 insertions(+)
> 
> diff --git a/drivers/input/mouse/elan_i2c_core.c 
> b/drivers/input/mouse/elan_i2c_core.c
> index 5f0d75a45c80..4ea57f4c7bd4 100644
> --- a/drivers/input/mouse/elan_i2c_core.c
> +++ b/drivers/input/mouse/elan_i2c_core.c
> @@ -56,6 +56,7 @@ struct elan_tp_data {
>  	struct input_dev	*input;
>  	struct input_dev	*tp_input; /* trackpoint input node */
>  	struct regulator	*vcc;
> +	struct list_head list;	/* for list of devices needing input handler
*/
>  
>  	const struct elan_transport_ops *ops;
>  
> @@ -63,6 +64,11 @@ struct elan_tp_data {
>  	struct completion	fw_completion;
>  	bool			in_fw_update;
>  
> +	struct work_struct	lid_work;
> +	bool			lid_switch;
> +	int			lid_value;
> +	bool			in_inhibit;
> +
>  	struct mutex		sysfs_mutex;
>  
>  	unsigned int		max_x;
> @@ -96,6 +102,9 @@ struct elan_tp_data {
>  	u32			quirks;		/* Various quirks */
>  };
>  
> +static struct workqueue_struct *elan_mode_wq; static 
> +LIST_HEAD(elan_devices_with_lid_handler);
> +
>  static u32 elan_i2c_lookup_quirks(u16 ic_type, u16 product_id)  {
>  	static const struct {
> @@ -329,6 +338,74 @@ 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 error;
> +
> +	error = elan_set_power(data, true);
> +	if (error)
> +		dev_err(dev, "failed to restore power: %d\n", error);
> +
> +	error = data->ops->sleep_control(data->client, false);
> +	if (error) {
> +		dev_err(dev,
> +			"failed to wake device up: %d\n", error);
> +		return error;
> +	}
> +
> +	return error;
> +}
> +
> +static int elan_inhibit(struct input_dev *input_dev)

I would rather you did not call it inhibit/uninhibit because this is not
what it does.

Please split the logic for recalibration from logic of powering on and off
the device. I also hope in the future firmware revisions the distinction
will be more clear (i.e. have a separate method/command to recalibrate
baseline on demand).

> +{
> +	struct elan_tp_data *data = input_get_drvdata(input_dev);
> +	struct i2c_client *client = data->client;
> +	int error;
> +
> +	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.
> +	 */
> +	error = mutex_lock_interruptible(&data->sysfs_mutex);
> +	if (error)
> +		return error;
> +
> +	disable_irq(client->irq);
> +
> +	error = elan_set_power(data, false);
> +	if (error)
> +		enable_irq(client->irq);
> +
> +	data->in_inhibit = true;
> +	mutex_unlock(&data->sysfs_mutex);
> +
> +	return error;
> +}
> +
> +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 error;
> +
> +	dev_dbg(&client->dev, "uninhibiting\n");
> +	error = mutex_lock_interruptible(&data->sysfs_mutex);
> +	if (error)
> +		return error;
> +
> +	error = elan_reactivate(data);
> +	if (error == 0)
> +		enable_irq(client->irq);
> +
> +	data->in_inhibit = false;
> +	mutex_unlock(&data->sysfs_mutex);
> +
> +	return error;
> +}
> +
>  static int elan_query_device_info(struct elan_tp_data *data)  {
>  	int error;
> @@ -1187,6 +1264,124 @@ static void elan_disable_regulator(void *_data)
>  	regulator_disable(data->vcc);
>  }
>  
> +static void lid_work_handler(struct work_struct *work) {
> +	struct elan_tp_data *data = container_of(work, struct elan_tp_data,
> +					    lid_work);
> +
> +	if (data->lid_value)
> +		elan_inhibit(data->input);
> +	else
> +		elan_uninhibit(data->input);
> +
> +}
> +
> +static void elan_input_lid_event(struct input_handle *handle, unsigned
int type,
> +			     unsigned int code, int value) {
> +	struct elan_tp_data *data, *n;
> +
> +	if (type == EV_SW && code == SW_LID) {
> +		list_for_each_entry_safe(data, n,
&elan_devices_with_lid_handler, 
> +list) {

Why do you need the "_safe()" variant here?

> +			data->lid_value = value;
> +			queue_work(elan_mode_wq, &data->lid_work);
> +		}
> +	}
> +
> +}
> +
> +struct elan_input_lid {
> +	struct input_handle handle;
> +};
> +
> +static int elan_input_lid_connect(struct input_handler *handler,
> +				struct input_dev *dev,
> +				const struct input_device_id *id) {
> +	struct elan_input_lid *lid;
> +	char *name;
> +	int error;
> +
> +	lid = kzalloc(sizeof(*lid), GFP_KERNEL);
> +	if (!lid)
> +		return -ENOMEM;
> +	name = kasprintf(GFP_KERNEL, "elan-i2c-lid-%s",
dev_name(&dev->dev));
> +	if (!name) {
> +		error = -ENOMEM;
> +		goto err_free_lid;
> +	}
> +	lid->handle.dev = dev;
> +	lid->handle.handler = handler;
> +	lid->handle.name = name;
> +	lid->handle.private = lid;
> +	error = input_register_handle(&lid->handle);
> +	if (error)
> +		goto err_free_name;
> +	error = input_open_device(&lid->handle);
> +	if (error)
> +		goto err_unregister_handle;
> +	return 0;
> +err_unregister_handle:
> +	input_unregister_handle(&lid->handle);
> +err_free_name:
> +	kfree(name);
> +err_free_lid:
> +	kfree(lid);
> +	return error;
> +}
> +
> +static void elan_input_lid_disconnect(struct input_handle *handle) {
> +	struct elan_input_lid *lid = handle->private;
> +
> +	input_close_device(handle);
> +	input_unregister_handle(handle);
> +	kfree(handle->name);
> +	kfree(lid);
> +}
> +
> +static const struct input_device_id elan_input_lid_ids[] = {
> +	{
> +		.flags = INPUT_DEVICE_ID_MATCH_EVBIT |
INPUT_DEVICE_ID_MATCH_SWBIT,
> +		.evbit = { BIT_MASK(EV_SW) },
> +		.swbit = { [BIT_WORD(SW_LID)] = BIT_MASK(SW_LID) },
> +	},
> +	{ },
> +};
> +
> +static struct input_handler elan_input_lid_handler = {
> +	.event =	elan_input_lid_event,
> +	.connect =	elan_input_lid_connect,
> +	.disconnect =	elan_input_lid_disconnect,
> +	.name =		"elan-i2c-lid",
> +	.id_table =	elan_input_lid_ids,
> +};
> +
> +static int elan_create_lid_handler(struct elan_tp_data *data) {
> +	int error = 0;
> +
> +	elan_mode_wq = create_singlethread_workqueue("elan-i2c-lid");
> +	if (elan_mode_wq == NULL)
> +		return -ENOMEM;
> +	error = input_register_handler(&elan_input_lid_handler);
> +	if (error)
> +		goto remove_wq;
> +
> +	data->lid_switch = true;
> +	INIT_LIST_HEAD(&data->list);
> +	INIT_WORK(&data->lid_work, lid_work_handler);
> +	list_add_tail(&data->list, &elan_devices_with_lid_handler);

It looks like you call elan_create_lid_handler() from elan_probe() which
means it can be called several times (we should not assume there is only one
controller), I do not see it being destroyed in remove() either, so it will
break if you bind/unbind the driver.

I also not sure why you need the list of you have a handler per device.

> +
> +	return 0;
> +
> +remove_wq:
> +	data->lid_switch = false;
> +	destroy_workqueue(elan_mode_wq);
> +	elan_mode_wq = NULL;
> +	return error;
> +}
> +
>  static int elan_probe(struct i2c_client *client)  {
>  	const struct elan_transport_ops *transport_ops; @@ -1325,6 +1520,10 
> @@ static int elan_probe(struct i2c_client *client)
>  		}
>  	}
>  
> +	error = elan_create_lid_handler(data);
> +	if (error)
> +		dev_err(dev, "failed to create lid handler: %d\n", error);

Do we need this on _ALL_ devices with ELan controllers, or just certain
ones? If we need this on all devices how did it work before?

> +
>  	return 0;
>  }
>  
> @@ -1334,6 +1533,10 @@ static int elan_suspend(struct device *dev)
>  	struct elan_tp_data *data = i2c_get_clientdata(client);
>  	int ret;
>  
> +	/* Wait for switch on completion */
> +	if (data->lid_switch)
> +		flush_workqueue(elan_mode_wq);
> +
>  	/*
>  	 * We are taking the mutex to make sure sysfs operations are
>  	 * complete before we attempt to bring the device into low[er] @@ 
> -1371,6 +1574,10 @@ static int elan_resume(struct device *dev)
>  	struct elan_tp_data *data = i2c_get_clientdata(client);
>  	int error;
>  
> +	/* Wait for switch on completion */
> +	if (data->lid_switch)
> +		flush_workqueue(elan_mode_wq);
> +
>  	if (!device_may_wakeup(dev)) {
>  		error = regulator_enable(data->vcc);
>  		if (error) {
> --
> 2.34.1
> 

Thanks.

-- 
Dmitry

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

[jingle.wu]

Add inhibit/uninhibit functions.

Due to some devices, the touchpad will be interfered with by
the screen noise when lid close.
---
 drivers/input/mouse/elan_i2c_core.c | 207 ++++++++++++++++++++++++++++
 1 file changed, 207 insertions(+)

diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
index 5f0d75a45c80..4ea57f4c7bd4 100644
--- a/drivers/input/mouse/elan_i2c_core.c
+++ b/drivers/input/mouse/elan_i2c_core.c
@@ -56,6 +56,7 @@ struct elan_tp_data {
 	struct input_dev	*input;
 	struct input_dev	*tp_input; /* trackpoint input node */
 	struct regulator	*vcc;
+	struct list_head list;	/* for list of devices needing input handler */
 
 	const struct elan_transport_ops *ops;
 
@@ -63,6 +64,11 @@ struct elan_tp_data {
 	struct completion	fw_completion;
 	bool			in_fw_update;
 
+	struct work_struct	lid_work;
+	bool			lid_switch;
+	int			lid_value;
+	bool			in_inhibit;
+
 	struct mutex		sysfs_mutex;
 
 	unsigned int		max_x;
@@ -96,6 +102,9 @@ struct elan_tp_data {
 	u32			quirks;		/* Various quirks */
 };
 
+static struct workqueue_struct *elan_mode_wq;
+static LIST_HEAD(elan_devices_with_lid_handler);
+
 static u32 elan_i2c_lookup_quirks(u16 ic_type, u16 product_id)
 {
 	static const struct {
@@ -329,6 +338,74 @@ 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 error;
+
+	error = elan_set_power(data, true);
+	if (error)
+		dev_err(dev, "failed to restore power: %d\n", error);
+
+	error = data->ops->sleep_control(data->client, false);
+	if (error) {
+		dev_err(dev,
+			"failed to wake device up: %d\n", error);
+		return error;
+	}
+
+	return error;
+}
+
+static int elan_inhibit(struct input_dev *input_dev)
+{
+	struct elan_tp_data *data = input_get_drvdata(input_dev);
+	struct i2c_client *client = data->client;
+	int error;
+
+	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.
+	 */
+	error = mutex_lock_interruptible(&data->sysfs_mutex);
+	if (error)
+		return error;
+
+	disable_irq(client->irq);
+
+	error = elan_set_power(data, false);
+	if (error)
+		enable_irq(client->irq);
+
+	data->in_inhibit = true;
+	mutex_unlock(&data->sysfs_mutex);
+
+	return error;
+}
+
+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 error;
+
+	dev_dbg(&client->dev, "uninhibiting\n");
+	error = mutex_lock_interruptible(&data->sysfs_mutex);
+	if (error)
+		return error;
+
+	error = elan_reactivate(data);
+	if (error == 0)
+		enable_irq(client->irq);
+
+	data->in_inhibit = false;
+	mutex_unlock(&data->sysfs_mutex);
+
+	return error;
+}
+
 static int elan_query_device_info(struct elan_tp_data *data)
 {
 	int error;
@@ -1187,6 +1264,124 @@ static void elan_disable_regulator(void *_data)
 	regulator_disable(data->vcc);
 }
 
+static void lid_work_handler(struct work_struct *work)
+{
+	struct elan_tp_data *data = container_of(work, struct elan_tp_data,
+					    lid_work);
+
+	if (data->lid_value)
+		elan_inhibit(data->input);
+	else
+		elan_uninhibit(data->input);
+
+}
+
+static void elan_input_lid_event(struct input_handle *handle, unsigned int type,
+			     unsigned int code, int value)
+{
+	struct elan_tp_data *data, *n;
+
+	if (type == EV_SW && code == SW_LID) {
+		list_for_each_entry_safe(data, n, &elan_devices_with_lid_handler, list) {
+			data->lid_value = value;
+			queue_work(elan_mode_wq, &data->lid_work);
+		}
+	}
+
+}
+
+struct elan_input_lid {
+	struct input_handle handle;
+};
+
+static int elan_input_lid_connect(struct input_handler *handler,
+				struct input_dev *dev,
+				const struct input_device_id *id)
+{
+	struct elan_input_lid *lid;
+	char *name;
+	int error;
+
+	lid = kzalloc(sizeof(*lid), GFP_KERNEL);
+	if (!lid)
+		return -ENOMEM;
+	name = kasprintf(GFP_KERNEL, "elan-i2c-lid-%s", dev_name(&dev->dev));
+	if (!name) {
+		error = -ENOMEM;
+		goto err_free_lid;
+	}
+	lid->handle.dev = dev;
+	lid->handle.handler = handler;
+	lid->handle.name = name;
+	lid->handle.private = lid;
+	error = input_register_handle(&lid->handle);
+	if (error)
+		goto err_free_name;
+	error = input_open_device(&lid->handle);
+	if (error)
+		goto err_unregister_handle;
+	return 0;
+err_unregister_handle:
+	input_unregister_handle(&lid->handle);
+err_free_name:
+	kfree(name);
+err_free_lid:
+	kfree(lid);
+	return error;
+}
+
+static void elan_input_lid_disconnect(struct input_handle *handle)
+{
+	struct elan_input_lid *lid = handle->private;
+
+	input_close_device(handle);
+	input_unregister_handle(handle);
+	kfree(handle->name);
+	kfree(lid);
+}
+
+static const struct input_device_id elan_input_lid_ids[] = {
+	{
+		.flags = INPUT_DEVICE_ID_MATCH_EVBIT | INPUT_DEVICE_ID_MATCH_SWBIT,
+		.evbit = { BIT_MASK(EV_SW) },
+		.swbit = { [BIT_WORD(SW_LID)] = BIT_MASK(SW_LID) },
+	},
+	{ },
+};
+
+static struct input_handler elan_input_lid_handler = {
+	.event =	elan_input_lid_event,
+	.connect =	elan_input_lid_connect,
+	.disconnect =	elan_input_lid_disconnect,
+	.name =		"elan-i2c-lid",
+	.id_table =	elan_input_lid_ids,
+};
+
+static int elan_create_lid_handler(struct elan_tp_data *data)
+{
+	int error = 0;
+
+	elan_mode_wq = create_singlethread_workqueue("elan-i2c-lid");
+	if (elan_mode_wq == NULL)
+		return -ENOMEM;
+	error = input_register_handler(&elan_input_lid_handler);
+	if (error)
+		goto remove_wq;
+
+	data->lid_switch = true;
+	INIT_LIST_HEAD(&data->list);
+	INIT_WORK(&data->lid_work, lid_work_handler);
+	list_add_tail(&data->list, &elan_devices_with_lid_handler);
+
+	return 0;
+
+remove_wq:
+	data->lid_switch = false;
+	destroy_workqueue(elan_mode_wq);
+	elan_mode_wq = NULL;
+	return error;
+}
+
 static int elan_probe(struct i2c_client *client)
 {
 	const struct elan_transport_ops *transport_ops;
@@ -1325,6 +1520,10 @@ static int elan_probe(struct i2c_client *client)
 		}
 	}
 
+	error = elan_create_lid_handler(data);
+	if (error)
+		dev_err(dev, "failed to create lid handler: %d\n", error);
+
 	return 0;
 }
 
@@ -1334,6 +1533,10 @@ static int elan_suspend(struct device *dev)
 	struct elan_tp_data *data = i2c_get_clientdata(client);
 	int ret;
 
+	/* Wait for switch on completion */
+	if (data->lid_switch)
+		flush_workqueue(elan_mode_wq);
+
 	/*
 	 * We are taking the mutex to make sure sysfs operations are
 	 * complete before we attempt to bring the device into low[er]
@@ -1371,6 +1574,10 @@ static int elan_resume(struct device *dev)
 	struct elan_tp_data *data = i2c_get_clientdata(client);
 	int error;
 
+	/* Wait for switch on completion */
+	if (data->lid_switch)
+		flush_workqueue(elan_mode_wq);
+
 	if (!device_may_wakeup(dev)) {
 		error = regulator_enable(data->vcc);
 		if (error) {
-- 
2.34.1

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

[Dmitry Torokhov]

Hi Jingle,

On Wed, May 31, 2023 at 05:03:40PM +0800, jingle.wu wrote:
>  Add inhibit/uninhibit functions.

You need to provide justification for this change, i.e. explain why you
need this/what is currently not working or working sub-optimally.

> 
>  Signed-off-by: Jingle.wu <jingle.wu@emc.com.tw>
> ---
>  drivers/input/mouse/elan_i2c_core.c | 207 ++++++++++++++++++++++++++++
>  1 file changed, 207 insertions(+)
> 
> diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
> index 5f0d75a45c80..4ea57f4c7bd4 100644
> --- a/drivers/input/mouse/elan_i2c_core.c
> +++ b/drivers/input/mouse/elan_i2c_core.c
> @@ -56,6 +56,7 @@ struct elan_tp_data {
>  	struct input_dev	*input;
>  	struct input_dev	*tp_input; /* trackpoint input node */
>  	struct regulator	*vcc;
> +	struct list_head list;	/* for list of devices needing input handler */
>  
>  	const struct elan_transport_ops *ops;
>  
> @@ -63,6 +64,11 @@ struct elan_tp_data {
>  	struct completion	fw_completion;
>  	bool			in_fw_update;
>  
> +	struct work_struct	lid_work;
> +	bool			lid_switch;
> +	int			lid_value;
> +	bool			in_inhibit;
> +
>  	struct mutex		sysfs_mutex;
>  
>  	unsigned int		max_x;
> @@ -96,6 +102,9 @@ struct elan_tp_data {
>  	u32			quirks;		/* Various quirks */
>  };
>  
> +static struct workqueue_struct *elan_mode_wq;
> +static LIST_HEAD(elan_devices_with_lid_handler);
> +
>  static u32 elan_i2c_lookup_quirks(u16 ic_type, u16 product_id)
>  {
>  	static const struct {
> @@ -329,6 +338,74 @@ 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 error;
> +
> +	error = elan_set_power(data, true);
> +	if (error)
> +		dev_err(dev, "failed to restore power: %d\n", error);
> +
> +	error = data->ops->sleep_control(data->client, false);
> +	if (error) {
> +		dev_err(dev,
> +			"failed to wake device up: %d\n", error);
> +		return error;
> +	}
> +
> +	return error;
> +}
> +
> +static int elan_inhibit(struct input_dev *input_dev)

I would rather you did not call it inhibit/uninhibit because this is not
what it does.

Please split the logic for recalibration from logic of powering on and
off the device. I also hope in the future firmware revisions the
distinction will be more clear (i.e. have a separate method/command to
recalibrate baseline on demand).

> +{
> +	struct elan_tp_data *data = input_get_drvdata(input_dev);
> +	struct i2c_client *client = data->client;
> +	int error;
> +
> +	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.
> +	 */
> +	error = mutex_lock_interruptible(&data->sysfs_mutex);
> +	if (error)
> +		return error;
> +
> +	disable_irq(client->irq);
> +
> +	error = elan_set_power(data, false);
> +	if (error)
> +		enable_irq(client->irq);
> +
> +	data->in_inhibit = true;
> +	mutex_unlock(&data->sysfs_mutex);
> +
> +	return error;
> +}
> +
> +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 error;
> +
> +	dev_dbg(&client->dev, "uninhibiting\n");
> +	error = mutex_lock_interruptible(&data->sysfs_mutex);
> +	if (error)
> +		return error;
> +
> +	error = elan_reactivate(data);
> +	if (error == 0)
> +		enable_irq(client->irq);
> +
> +	data->in_inhibit = false;
> +	mutex_unlock(&data->sysfs_mutex);
> +
> +	return error;
> +}
> +
>  static int elan_query_device_info(struct elan_tp_data *data)
>  {
>  	int error;
> @@ -1187,6 +1264,124 @@ static void elan_disable_regulator(void *_data)
>  	regulator_disable(data->vcc);
>  }
>  
> +static void lid_work_handler(struct work_struct *work)
> +{
> +	struct elan_tp_data *data = container_of(work, struct elan_tp_data,
> +					    lid_work);
> +
> +	if (data->lid_value)
> +		elan_inhibit(data->input);
> +	else
> +		elan_uninhibit(data->input);
> +
> +}
> +
> +static void elan_input_lid_event(struct input_handle *handle, unsigned int type,
> +			     unsigned int code, int value)
> +{
> +	struct elan_tp_data *data, *n;
> +
> +	if (type == EV_SW && code == SW_LID) {
> +		list_for_each_entry_safe(data, n, &elan_devices_with_lid_handler, list) {

Why do you need the "_safe()" variant here?

> +			data->lid_value = value;
> +			queue_work(elan_mode_wq, &data->lid_work);
> +		}
> +	}
> +
> +}
> +
> +struct elan_input_lid {
> +	struct input_handle handle;
> +};
> +
> +static int elan_input_lid_connect(struct input_handler *handler,
> +				struct input_dev *dev,
> +				const struct input_device_id *id)
> +{
> +	struct elan_input_lid *lid;
> +	char *name;
> +	int error;
> +
> +	lid = kzalloc(sizeof(*lid), GFP_KERNEL);
> +	if (!lid)
> +		return -ENOMEM;
> +	name = kasprintf(GFP_KERNEL, "elan-i2c-lid-%s", dev_name(&dev->dev));
> +	if (!name) {
> +		error = -ENOMEM;
> +		goto err_free_lid;
> +	}
> +	lid->handle.dev = dev;
> +	lid->handle.handler = handler;
> +	lid->handle.name = name;
> +	lid->handle.private = lid;
> +	error = input_register_handle(&lid->handle);
> +	if (error)
> +		goto err_free_name;
> +	error = input_open_device(&lid->handle);
> +	if (error)
> +		goto err_unregister_handle;
> +	return 0;
> +err_unregister_handle:
> +	input_unregister_handle(&lid->handle);
> +err_free_name:
> +	kfree(name);
> +err_free_lid:
> +	kfree(lid);
> +	return error;
> +}
> +
> +static void elan_input_lid_disconnect(struct input_handle *handle)
> +{
> +	struct elan_input_lid *lid = handle->private;
> +
> +	input_close_device(handle);
> +	input_unregister_handle(handle);
> +	kfree(handle->name);
> +	kfree(lid);
> +}
> +
> +static const struct input_device_id elan_input_lid_ids[] = {
> +	{
> +		.flags = INPUT_DEVICE_ID_MATCH_EVBIT | INPUT_DEVICE_ID_MATCH_SWBIT,
> +		.evbit = { BIT_MASK(EV_SW) },
> +		.swbit = { [BIT_WORD(SW_LID)] = BIT_MASK(SW_LID) },
> +	},
> +	{ },
> +};
> +
> +static struct input_handler elan_input_lid_handler = {
> +	.event =	elan_input_lid_event,
> +	.connect =	elan_input_lid_connect,
> +	.disconnect =	elan_input_lid_disconnect,
> +	.name =		"elan-i2c-lid",
> +	.id_table =	elan_input_lid_ids,
> +};
> +
> +static int elan_create_lid_handler(struct elan_tp_data *data)
> +{
> +	int error = 0;
> +
> +	elan_mode_wq = create_singlethread_workqueue("elan-i2c-lid");
> +	if (elan_mode_wq == NULL)
> +		return -ENOMEM;
> +	error = input_register_handler(&elan_input_lid_handler);
> +	if (error)
> +		goto remove_wq;
> +
> +	data->lid_switch = true;
> +	INIT_LIST_HEAD(&data->list);
> +	INIT_WORK(&data->lid_work, lid_work_handler);
> +	list_add_tail(&data->list, &elan_devices_with_lid_handler);

It looks like you call elan_create_lid_handler() from elan_probe() which
means it can be called several times (we should not assume there is only
one controller), I do not see it being destroyed in remove() either, so
it will break if you bind/unbind the driver.

I also not sure why you need the list of you have a handler per device.

> +
> +	return 0;
> +
> +remove_wq:
> +	data->lid_switch = false;
> +	destroy_workqueue(elan_mode_wq);
> +	elan_mode_wq = NULL;
> +	return error;
> +}
> +
>  static int elan_probe(struct i2c_client *client)
>  {
>  	const struct elan_transport_ops *transport_ops;
> @@ -1325,6 +1520,10 @@ static int elan_probe(struct i2c_client *client)
>  		}
>  	}
>  
> +	error = elan_create_lid_handler(data);
> +	if (error)
> +		dev_err(dev, "failed to create lid handler: %d\n", error);

Do we need this on _ALL_ devices with ELan controllers, or just certain
ones? If we need this on all devices how did it work before?

> +
>  	return 0;
>  }
>  
> @@ -1334,6 +1533,10 @@ static int elan_suspend(struct device *dev)
>  	struct elan_tp_data *data = i2c_get_clientdata(client);
>  	int ret;
>  
> +	/* Wait for switch on completion */
> +	if (data->lid_switch)
> +		flush_workqueue(elan_mode_wq);
> +
>  	/*
>  	 * We are taking the mutex to make sure sysfs operations are
>  	 * complete before we attempt to bring the device into low[er]
> @@ -1371,6 +1574,10 @@ static int elan_resume(struct device *dev)
>  	struct elan_tp_data *data = i2c_get_clientdata(client);
>  	int error;
>  
> +	/* Wait for switch on completion */
> +	if (data->lid_switch)
> +		flush_workqueue(elan_mode_wq);
> +
>  	if (!device_may_wakeup(dev)) {
>  		error = regulator_enable(data->vcc);
>  		if (error) {
> -- 
> 2.34.1
> 

Thanks.

-- 
Dmitry

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

[jingle.wu]

 Add inhibit/uninhibit functions.

 Signed-off-by: Jingle.wu <jingle.wu@emc.com.tw>
---
 drivers/input/mouse/elan_i2c_core.c | 207 ++++++++++++++++++++++++++++
 1 file changed, 207 insertions(+)

diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
index 5f0d75a45c80..4ea57f4c7bd4 100644
--- a/drivers/input/mouse/elan_i2c_core.c
+++ b/drivers/input/mouse/elan_i2c_core.c
@@ -56,6 +56,7 @@ struct elan_tp_data {
 	struct input_dev	*input;
 	struct input_dev	*tp_input; /* trackpoint input node */
 	struct regulator	*vcc;
+	struct list_head list;	/* for list of devices needing input handler */
 
 	const struct elan_transport_ops *ops;
 
@@ -63,6 +64,11 @@ struct elan_tp_data {
 	struct completion	fw_completion;
 	bool			in_fw_update;
 
+	struct work_struct	lid_work;
+	bool			lid_switch;
+	int			lid_value;
+	bool			in_inhibit;
+
 	struct mutex		sysfs_mutex;
 
 	unsigned int		max_x;
@@ -96,6 +102,9 @@ struct elan_tp_data {
 	u32			quirks;		/* Various quirks */
 };
 
+static struct workqueue_struct *elan_mode_wq;
+static LIST_HEAD(elan_devices_with_lid_handler);
+
 static u32 elan_i2c_lookup_quirks(u16 ic_type, u16 product_id)
 {
 	static const struct {
@@ -329,6 +338,74 @@ 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 error;
+
+	error = elan_set_power(data, true);
+	if (error)
+		dev_err(dev, "failed to restore power: %d\n", error);
+
+	error = data->ops->sleep_control(data->client, false);
+	if (error) {
+		dev_err(dev,
+			"failed to wake device up: %d\n", error);
+		return error;
+	}
+
+	return error;
+}
+
+static int elan_inhibit(struct input_dev *input_dev)
+{
+	struct elan_tp_data *data = input_get_drvdata(input_dev);
+	struct i2c_client *client = data->client;
+	int error;
+
+	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.
+	 */
+	error = mutex_lock_interruptible(&data->sysfs_mutex);
+	if (error)
+		return error;
+
+	disable_irq(client->irq);
+
+	error = elan_set_power(data, false);
+	if (error)
+		enable_irq(client->irq);
+
+	data->in_inhibit = true;
+	mutex_unlock(&data->sysfs_mutex);
+
+	return error;
+}
+
+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 error;
+
+	dev_dbg(&client->dev, "uninhibiting\n");
+	error = mutex_lock_interruptible(&data->sysfs_mutex);
+	if (error)
+		return error;
+
+	error = elan_reactivate(data);
+	if (error == 0)
+		enable_irq(client->irq);
+
+	data->in_inhibit = false;
+	mutex_unlock(&data->sysfs_mutex);
+
+	return error;
+}
+
 static int elan_query_device_info(struct elan_tp_data *data)
 {
 	int error;
@@ -1187,6 +1264,124 @@ static void elan_disable_regulator(void *_data)
 	regulator_disable(data->vcc);
 }
 
+static void lid_work_handler(struct work_struct *work)
+{
+	struct elan_tp_data *data = container_of(work, struct elan_tp_data,
+					    lid_work);
+
+	if (data->lid_value)
+		elan_inhibit(data->input);
+	else
+		elan_uninhibit(data->input);
+
+}
+
+static void elan_input_lid_event(struct input_handle *handle, unsigned int type,
+			     unsigned int code, int value)
+{
+	struct elan_tp_data *data, *n;
+
+	if (type == EV_SW && code == SW_LID) {
+		list_for_each_entry_safe(data, n, &elan_devices_with_lid_handler, list) {
+			data->lid_value = value;
+			queue_work(elan_mode_wq, &data->lid_work);
+		}
+	}
+
+}
+
+struct elan_input_lid {
+	struct input_handle handle;
+};
+
+static int elan_input_lid_connect(struct input_handler *handler,
+				struct input_dev *dev,
+				const struct input_device_id *id)
+{
+	struct elan_input_lid *lid;
+	char *name;
+	int error;
+
+	lid = kzalloc(sizeof(*lid), GFP_KERNEL);
+	if (!lid)
+		return -ENOMEM;
+	name = kasprintf(GFP_KERNEL, "elan-i2c-lid-%s", dev_name(&dev->dev));
+	if (!name) {
+		error = -ENOMEM;
+		goto err_free_lid;
+	}
+	lid->handle.dev = dev;
+	lid->handle.handler = handler;
+	lid->handle.name = name;
+	lid->handle.private = lid;
+	error = input_register_handle(&lid->handle);
+	if (error)
+		goto err_free_name;
+	error = input_open_device(&lid->handle);
+	if (error)
+		goto err_unregister_handle;
+	return 0;
+err_unregister_handle:
+	input_unregister_handle(&lid->handle);
+err_free_name:
+	kfree(name);
+err_free_lid:
+	kfree(lid);
+	return error;
+}
+
+static void elan_input_lid_disconnect(struct input_handle *handle)
+{
+	struct elan_input_lid *lid = handle->private;
+
+	input_close_device(handle);
+	input_unregister_handle(handle);
+	kfree(handle->name);
+	kfree(lid);
+}
+
+static const struct input_device_id elan_input_lid_ids[] = {
+	{
+		.flags = INPUT_DEVICE_ID_MATCH_EVBIT | INPUT_DEVICE_ID_MATCH_SWBIT,
+		.evbit = { BIT_MASK(EV_SW) },
+		.swbit = { [BIT_WORD(SW_LID)] = BIT_MASK(SW_LID) },
+	},
+	{ },
+};
+
+static struct input_handler elan_input_lid_handler = {
+	.event =	elan_input_lid_event,
+	.connect =	elan_input_lid_connect,
+	.disconnect =	elan_input_lid_disconnect,
+	.name =		"elan-i2c-lid",
+	.id_table =	elan_input_lid_ids,
+};
+
+static int elan_create_lid_handler(struct elan_tp_data *data)
+{
+	int error = 0;
+
+	elan_mode_wq = create_singlethread_workqueue("elan-i2c-lid");
+	if (elan_mode_wq == NULL)
+		return -ENOMEM;
+	error = input_register_handler(&elan_input_lid_handler);
+	if (error)
+		goto remove_wq;
+
+	data->lid_switch = true;
+	INIT_LIST_HEAD(&data->list);
+	INIT_WORK(&data->lid_work, lid_work_handler);
+	list_add_tail(&data->list, &elan_devices_with_lid_handler);
+
+	return 0;
+
+remove_wq:
+	data->lid_switch = false;
+	destroy_workqueue(elan_mode_wq);
+	elan_mode_wq = NULL;
+	return error;
+}
+
 static int elan_probe(struct i2c_client *client)
 {
 	const struct elan_transport_ops *transport_ops;
@@ -1325,6 +1520,10 @@ static int elan_probe(struct i2c_client *client)
 		}
 	}
 
+	error = elan_create_lid_handler(data);
+	if (error)
+		dev_err(dev, "failed to create lid handler: %d\n", error);
+
 	return 0;
 }
 
@@ -1334,6 +1533,10 @@ static int elan_suspend(struct device *dev)
 	struct elan_tp_data *data = i2c_get_clientdata(client);
 	int ret;
 
+	/* Wait for switch on completion */
+	if (data->lid_switch)
+		flush_workqueue(elan_mode_wq);
+
 	/*
 	 * We are taking the mutex to make sure sysfs operations are
 	 * complete before we attempt to bring the device into low[er]
@@ -1371,6 +1574,10 @@ static int elan_resume(struct device *dev)
 	struct elan_tp_data *data = i2c_get_clientdata(client);
 	int error;
 
+	/* Wait for switch on completion */
+	if (data->lid_switch)
+		flush_workqueue(elan_mode_wq);
+
 	if (!device_may_wakeup(dev)) {
 		error = regulator_enable(data->vcc);
 		if (error) {
-- 
2.34.1

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

[jingle.wu]

Add inhibit/uninhibit functions.

Signed-off-by: Jingle.wu <jingle.wu@emc.com.tw>
---
 drivers/input/mouse/elan_i2c_core.c | 83 +++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)

diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
index 5f0d75a45c80..423f22fded59 100644
--- a/drivers/input/mouse/elan_i2c_core.c
+++ b/drivers/input/mouse/elan_i2c_core.c
@@ -329,6 +329,86 @@ 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 error;
+
+	error = elan_set_power(data, true);
+	if (error)
+		dev_err(dev, "failed to restore power: %d\n", error);
+
+	error = data->ops->sleep_control(data->client, false);
+	if (error) {
+		dev_err(dev,
+			"failed to wake device up: %d\n", error);
+		return error;
+	}
+
+	return error;
+}
+
+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 error;
+
+	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.
+	 */
+	error = mutex_lock_interruptible(&data->sysfs_mutex);
+	if (error)
+		return;
+
+	disable_irq(client->irq);
+
+	error = elan_set_power(data, false);
+	if (error)
+		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 error;
+
+	dev_dbg(&client->dev, "uninhibiting\n");
+	error = mutex_lock_interruptible(&data->sysfs_mutex);
+	if (error)
+		return error;
+
+	error = elan_reactivate(data);
+	if (error == 0)
+		enable_irq(client->irq);
+
+	mutex_unlock(&data->sysfs_mutex);
+
+	return error;
+}
+
+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 +1255,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;
-- 
2.34.1

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

[kernel test robot]

Hi jingle.wu,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on dtor-input/next]
[also build test WARNING on dtor-input/for-linus hid/for-next linus/master v6.3-rc2 next-20230317]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/jingle-wu/Input-elan_i2c-Implement-inhibit-uninhibit-functions/20230317-152004
base:   https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
patch link:    https://lore.kernel.org/r/20230317071646.977357-1-jingle.wu%40emc.com.tw
patch subject: [PATCH] Input: elan_i2c - Implement inhibit/uninhibit functions.
config: nios2-randconfig-m031-20230319 (https://download.01.org/0day-ci/archive/20230319/202303191024.mbwWRV3A-lkp@intel.com/config)
compiler: nios2-linux-gcc (GCC) 12.1.0

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303191024.mbwWRV3A-lkp@intel.com/

smatch warnings:
drivers/input/mouse/elan_i2c_core.c:342 elan_reactivate() warn: inconsistent indenting

vim +342 drivers/input/mouse/elan_i2c_core.c

   331	
   332	static int elan_reactivate(struct elan_tp_data *data)
   333	{
   334		struct device *dev = &data->client->dev;
   335		int ret;
   336	
   337		ret = elan_set_power(data, true);
   338		if (ret)
   339			dev_err(dev, "failed to restore power: %d\n", ret);
   340	
   341		ret = data->ops->sleep_control(data->client, false);
 > 342			if (ret) {
   343				dev_err(dev,
   344					"failed to wake device up: %d\n", ret);
   345				return ret;
   346			}
   347	
   348		return ret;
   349	}
   350	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

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

[jingle.wu]

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..cc0375265659 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;
-- 
2.34.1