| Message ID | 20230320011456.986321-1-jingle.wu@emc.com.tw |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Input: elan_i2c - Implement inhibit/uninhibit functions. | expand |
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
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.
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
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.
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
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 --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;
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