* Re: [PATCH v3] Add rumble support to latest xbox controllers
From: Siarhei Vishniakou @ 2023-04-25 16:40 UTC (permalink / raw)
To: Bastien Nocera
Cc: Edward Matijevic, benjamin.tissoires, jikos, linux-input,
linux-kernel
In-Reply-To: <f82bdf01743b11e92873c5c9937ae253bd1c8bb3.camel@hadess.net>
Right, confirmed on the BLE support.
I uploaded a new patch with the C-style comments and added the
xbox.com link to the commit message.
On Tue, Apr 25, 2023 at 9:07 AM Bastien Nocera <hadess@hadess.net> wrote:
>
> On Tue, 2023-04-25 at 10:46 -0500, Edward Matijevic wrote:
> > > > +#define USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1708 0x02fd
> > > > +#define
> > > > USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1708_BLE 0x0b20
> > > The 1708 model uses Bluetooth Classic, not Bluetooth LE.
> >
> > The new firmware adds Bluetooth LE support to the 1708
> > and prioritizes BLE over Classic which necessitates the change
> > The controllers are broken without the "new firmware" IDs which are
> > for BLE
>
> Oh! I completely missed that. So both the 1708 and 1797 used Bluetooth
> Classic with the old firmware, and support Bluetooth LE with the new
> one. Am I understanding this correctly?
>
> If that's right, looks like I might need to update Wikipedia ;)
>
> Seeing as you will be updating the patch for that comment style
> problem, you could probably add a reference to this article in the
> commit message, it seems authoritative enough:
> https://news.xbox.com/en-us/2021/09/08/xbox-controller-firmware-update-rolling-out-to-insiders-starting-today/
^ permalink raw reply
* [PATCH v2] Fix freeze in lm8333 i2c keyboard driver
From: Tomas Mudrunka @ 2023-04-25 16:49 UTC (permalink / raw)
To: jeff; +Cc: dmitry.torokhov, linux-input, linux-kernel, tomas.mudrunka
In-Reply-To: <ZEf0RYdD5jhE9JEk@nixie71>
LM8333 uses gpio interrupt line which is activated by falling edge.
When button is pressed before driver is loaded,
driver will miss the edge and never respond again.
To fix this we clear the interrupt via i2c after registering IRQ.
Signed-off-by: Tomas Mudrunka <tomas.mudrunka@gmail.com>
---
drivers/input/keyboard/lm8333.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/input/keyboard/lm8333.c b/drivers/input/keyboard/lm8333.c
index 7457c3220..9a810ca00 100644
--- a/drivers/input/keyboard/lm8333.c
+++ b/drivers/input/keyboard/lm8333.c
@@ -184,6 +184,8 @@ static int lm8333_probe(struct i2c_client *client)
if (err)
goto free_mem;
+ lm8333_read8(lm8333, LM8333_READ_INT);
+
err = input_register_device(input);
if (err)
goto free_irq;
--
2.40.0
^ permalink raw reply related
* Re: [PATCH] HID: intel-ish-hid: pci-ish: Fix use after free bug in ish_remove due to race condition
From: Zheng Hacker @ 2023-04-25 17:18 UTC (permalink / raw)
To: srinivas pandruvada
Cc: Zheng Wang, jikos, benjamin.tissoires, rafael, hdegoede, gregkh,
linux-input, linux-kernel, 1395428693sheep, alex000young
In-Reply-To: <2bd0143cf0e638c88f57409f854c0529a1b12b6b.camel@linux.intel.com>
srinivas pandruvada <srinivas.pandruvada@linux.intel.com> 于2023年4月25日周二 23:55写道：
>
> On Thu, 2023-04-13 at 01:14 +0800, Zheng Wang wrote:
> > In ish_probe, it calls ish_dev_init to init the device. In this
> > function,
> > ishtp_device_init is called and &dev->bh_hbm_work is bound with
> > bh_hbm_work_fn. recv_hbm may be called to start the timer work.
> >
> > If we remove the module which will call ish_remove to make cleanup,
> > there may be an unfinished work. The possible sequence is as follows:
> >
> > Fix it by canceling the work before cleanup in
> > ishtp_bus_remove_all_clients
> >
> > CPU0 CPUc1
> >
> > |bh_hbm_work_fn
> > ish_remove |
> > ishtp_bus_remove_all_clients |
> > kfree(ishtp_dev->fw_clients); |
> > |
> > |&dev->fw_clients[...]
> > | //use
> >
> > Fixes: 3703f53b99e4 ("HID: intel_ish-hid: ISH Transport layer")
> > Signed-off-by: Zheng Wang <zyytlz.wz@163.com>
> Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
>
> It is a extremely rare case, so it is OK to not mark for stable.
Got it, thanks!
Best regards,
Zheng
>
> > ---
> > drivers/hid/intel-ish-hid/ishtp/bus.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/hid/intel-ish-hid/ishtp/bus.c
> > b/drivers/hid/intel-ish-hid/ishtp/bus.c
> > index 81385ab37fa9..ada7cd08dbeb 100644
> > --- a/drivers/hid/intel-ish-hid/ishtp/bus.c
> > +++ b/drivers/hid/intel-ish-hid/ishtp/bus.c
> > @@ -744,6 +744,7 @@ void ishtp_bus_remove_all_clients(struct
> > ishtp_device *ishtp_dev,
> > */
> > }
> > spin_unlock_irqrestore(&ishtp_dev->cl_list_lock, flags);
> > + cancel_work_sync(&ishtp_dev->bh_hbm_work);
> >
> > /* Release DMA buffers for client messages */
> > ishtp_cl_free_dma_buf(ishtp_dev);
>
^ permalink raw reply
* 8bitdo usb adapter 2 in the NintendoMode (hid-nintendo.ko)
From: Robert Swiecki @ 2023-04-25 23:51 UTC (permalink / raw)
To: Daniel J. Ogorchock; +Cc: linux-input
Hi Daniel and linux-input@vger.kernel.org,
I've recently bought a 8bitdo usb adapter 2 -
https://www.8bitdo.com/usb-wireless-adapter-2/ - and it didn't work
with Linux in its Nintendo Switch Pro mode (emulating the Nintendo Pro
Controller over the USB).
It turned out that the following func sends 2 bytes in the
JC_OUTPUT_USB_CMD, while the adapter expects 64 bytes. Maybe it
expects fewer, but that's what the yuzu emulator sends to it over
hidraw (bypassing the hid-nintendo), and it works.
static int joycon_send_usb(struct joycon_ctlr *ctlr, u8 cmd, u32 timeout)
{
int ret;
u8 buf[2] = {JC_OUTPUT_USB_CMD};
buf[1] = cmd;
ctlr->usb_ack_match = cmd;
ctlr->msg_type = JOYCON_MSG_TYPE_USB;
ret = joycon_hid_send_sync(ctlr, buf, sizeof(buf), timeout);
if (ret)
hid_dbg(ctlr->hdev, "send usb command failed; ret=%d\n", ret);
return ret;
}
If only 2 bytes are sent, then it causes the adapter to disconnect,
probably due to some desync on the protocol level on the adapter side
of things.
[135278.921421] nintendo 0003:057E:2009.00DD: hidraw16: USB HID v81.11
Joystick [Nintendo Co., Ltd. Pro Controller] on
usb-0000:07:00.3-3.1/input0
[135280.000469] nintendo 0003:057E:2009.00DD: failed reading SPI flash; ret=-71
[135280.000473] nintendo 0003:057E:2009.00DD: using factory cal for left stick
[135280.008470] nintendo 0003:057E:2009.00DD: failed reading SPI flash; ret=-71
[135280.008473] nintendo 0003:057E:2009.00DD: using factory cal for right stick
[135280.016469] nintendo 0003:057E:2009.00DD: failed reading SPI flash; ret=-71
[135280.016472] nintendo 0003:057E:2009.00DD: Failed to read left
stick cal, using defaults; e=-71
[135280.024468] nintendo 0003:057E:2009.00DD: failed reading SPI flash; ret=-71
[135280.024471] nintendo 0003:057E:2009.00DD: Failed to read right
stick cal, using defaults; e=-71
[135280.032470] nintendo 0003:057E:2009.00DD: failed reading SPI flash; ret=-71
[135280.032473] nintendo 0003:057E:2009.00DD: using factory cal for IMU
[135280.040468] nintendo 0003:057E:2009.00DD: failed reading SPI flash; ret=-71
[135280.040471] nintendo 0003:057E:2009.00DD: Failed to read IMU cal,
using defaults; ret=-71
[135280.040472] nintendo 0003:057E:2009.00DD: Unable to read IMU
calibration data
[135280.048470] nintendo 0003:057E:2009.00DD: Failed to set report mode; ret=-71
[135280.048743] nintendo 0003:057E:2009.00DD: probe - fail = -71
[135280.048749] nintendo: probe of 0003:057E:2009.00DD failed with error -71
[135280.050014] usb 3-3.1: USB disconnect, device number 53
In case it didn't create any problems with the actual Nintendo Pro
Controller, nor with the JoyCons, could we change this code from
u8 buf[2] = {JC_OUTPUT_USB_CMD};
to
u8 buf[64] = {JC_OUTPUT_USB_CMD};
?
I know this change is a bit "out of the blue", but given that the
protocol seems to be generally undocumented and reverse-engineered,
maybe that's fine here.
Thanks in advance.
--
Robert Święcki
^ permalink raw reply
* [PATCH v2 0/2] Fix Goodix touchscreen power leakage for MT8186 boards
From: Fei Shao @ 2023-04-26 9:32 UTC (permalink / raw)
To: Jeff LaBundy, Douglas Anderson, Benjamin Tissoires, Rob Herring
Cc: linux-mediatek, Fei Shao, Dmitry Torokhov, Jiri Kosina,
Krzysztof Kozlowski, Matthias Kaehlcke, Stephen Kitt, devicetree,
linux-input, linux-kernel
These changes are based on the series in [1], which modified the
i2c-hid-of-goodix driver and removed the workaround for a power leakage
issue, so the issue revisits on Mediatek MT8186 boards (Steelix).
The root cause is that the touchscreen can be powered in different ways
depending on the hardware designs, and it's not as easy to come up with
a solution that is both simple and elegant for all the known designs.
To address the issue, I ended up adding a new boolean property for the
driver so that we can control the power up/down sequence depending on
that.
Adding a new property might not be the cleanest approach for this, but
at least the intention would be easy enough to understand, and it
introduces relatively small change to the code and fully preserves the
original control flow.
I hope this is something acceptable, and I'm open to any better
approaches.
[1] https://lore.kernel.org/all/20230207024816.525938-1-dianders@chromium.org/
Changes in v2:
- Use a more accurate property name and with "goodix," prefix.
- Do not change the regulator_enable logic during power-up.
Fei Shao (2):
dt-bindings: input: goodix: Add "goodix,no-reset-during-suspend"
property
HID: i2c-hid: goodix: Add support for "goodix,no-reset-during-suspend"
property
.../bindings/input/goodix,gt7375p.yaml | 9 +++++++
drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 26 +++++++++++++++++++
2 files changed, 35 insertions(+)
--
2.40.1.495.gc816e09b53d-goog
^ permalink raw reply
* [PATCH v2 1/2] dt-bindings: input: goodix: Add "goodix,no-reset-during-suspend" property
From: Fei Shao @ 2023-04-26 9:32 UTC (permalink / raw)
To: Jeff LaBundy, Douglas Anderson, Benjamin Tissoires, Rob Herring
Cc: linux-mediatek, Fei Shao, Matthias Brugger, Dmitry Torokhov,
Krzysztof Kozlowski, devicetree, linux-input, linux-kernel
In-Reply-To: <20230426093231.1466984-1-fshao@chromium.org>
We observed that on Chromebook device Steelix, if Goodix GT7375P
touchscreen is powered in suspend (because, for example, it connects to
an always-on regulator) and with the reset GPIO asserted, it will
introduce about 14mW power leakage.
To address that, we add this property to skip reset during suspend.
If it's set, the driver will stop asserting the reset GPIO during
power-down. Refer to the comments in the driver for details.
Signed-off-by: Fei Shao <fshao@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
---
Changes in v2:
- Use a more accurate property name and with "goodix," prefix.
.../devicetree/bindings/input/goodix,gt7375p.yaml | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
index ce18d7dadae2..1edad1da1196 100644
--- a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
+++ b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
@@ -43,6 +43,15 @@ properties:
itself as long as it allows the main board to make signals compatible
with what the touchscreen is expecting for its IO rails.
+ goodix,no-reset-during-suspend:
+ description:
+ Set this to true to enforce the driver to not assert the reset GPIO
+ during suspend.
+ Due to potential touchscreen hardware flaw, back-powering could happen in
+ suspend if the power supply is on and with active-low reset GPIO asserted.
+ This property is used to avoid the back-powering issue.
+ type: boolean
+
required:
- compatible
- reg
--
2.40.1.495.gc816e09b53d-goog
^ permalink raw reply related
* [PATCH v2 2/2] HID: i2c-hid: goodix: Add support for "goodix,no-reset-during-suspend" property
From: Fei Shao @ 2023-04-26 9:32 UTC (permalink / raw)
To: Jeff LaBundy, Douglas Anderson, Benjamin Tissoires, Rob Herring
Cc: linux-mediatek, Fei Shao, Dmitry Torokhov, Jiri Kosina,
Matthias Kaehlcke, Stephen Kitt, linux-input, linux-kernel
In-Reply-To: <20230426093231.1466984-1-fshao@chromium.org>
In the beginning, commit 18eeef46d359 ("HID: i2c-hid: goodix: Tie the
reset line to true state of the regulator") introduced a change to tie
the reset line of the Goodix touchscreen to the state of the regulator
to fix a power leakage issue in suspend.
After some time, the change was deemed unnecessary and was reverted in
commit 557e05fa9fdd ("HID: i2c-hid: goodix: Stop tying the reset line to
the regulator") due to difficulties in managing regulator notifiers for
designs like Evoker, which provides a second power rail to touchscreen.
However, the revert caused a power regression on another Chromebook
device Steelix in the field, which has a dedicated always-on regulator
for touchscreen and was covered by the workaround in the first commit.
To address both cases, this patch adds the support for the new
"goodix,no-reset-during-suspend" property in the driver:
- When set to true, the driver does not assert the reset GPIO during
power-down.
Instead, the GPIO will be asserted during power-up to ensure the
touchscreen always has a clean start and consistent behavior after
resuming.
This is for designs with a dedicated always-on regulator.
- When set to false or unset, the driver uses the original control flow
and asserts GPIO and disable regulators normally.
This is for the two-regulator and shared-regulator designs.
Signed-off-by: Fei Shao <fshao@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
Changes in v2:
- Do not change the regulator_enable logic during power-up.
drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 26 +++++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
index 0060e3dcd775..fc4532fcadcc 100644
--- a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
+++ b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
@@ -28,6 +28,7 @@ struct i2c_hid_of_goodix {
struct regulator *vdd;
struct regulator *vddio;
struct gpio_desc *reset_gpio;
+ bool no_reset_during_suspend;
const struct goodix_i2c_hid_timing_data *timings;
};
@@ -37,6 +38,20 @@ static int goodix_i2c_hid_power_up(struct i2chid_ops *ops)
container_of(ops, struct i2c_hid_of_goodix, ops);
int ret;
+ if (ihid_goodix->no_reset_during_suspend) {
+ /*
+ * This is not mandatory, but we assert reset here (instead of
+ * during power-down) to ensure the device will have a clean
+ * state after powering up, just like the normal scenarios will
+ * have.
+ *
+ * Note that in this case we assume the regulators should be
+ * (marked as) always-on, so the regulator core knows what to
+ * do with them in the following regulator_enable() calls
+ * despite regulator_disable() was not called previously.
+ */
+ gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
+ }
ret = regulator_enable(ihid_goodix->vdd);
if (ret)
return ret;
@@ -60,6 +75,14 @@ static void goodix_i2c_hid_power_down(struct i2chid_ops *ops)
struct i2c_hid_of_goodix *ihid_goodix =
container_of(ops, struct i2c_hid_of_goodix, ops);
+ /*
+ * Don't assert reset GPIO if it's set.
+ * Also, it's okay to skip the following regulator_disable() calls
+ * because the regulators should be always-on in this case.
+ */
+ if (ihid_goodix->no_reset_during_suspend)
+ return;
+
gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
regulator_disable(ihid_goodix->vddio);
regulator_disable(ihid_goodix->vdd);
@@ -91,6 +114,9 @@ static int i2c_hid_of_goodix_probe(struct i2c_client *client)
if (IS_ERR(ihid_goodix->vddio))
return PTR_ERR(ihid_goodix->vddio);
+ ihid_goodix->no_reset_during_suspend =
+ of_property_read_bool(client->dev.of_node, "goodix,no-reset-during-suspend");
+
ihid_goodix->timings = device_get_match_data(&client->dev);
return i2c_hid_core_probe(client, &ihid_goodix->ops, 0x0001, 0);
--
2.40.1.495.gc816e09b53d-goog
^ permalink raw reply related
* [PATCH v2 0/2] STMPE device tree bindings
From: Linus Walleij @ 2023-04-26 11:21 UTC (permalink / raw)
To: Bartosz Golaszewski, Rob Herring, Krzysztof Kozlowski,
Maxime Coquelin, Alexandre Torgue, Dmitry Torokhov, Lee Jones,
Philippe Schenker, Stefan Agner, Marek Vasut, Steffen Trumtrar
Cc: linux-gpio, devicetree, linux-stm32, linux-arm-kernel,
linux-kernel, linux-input, Linus Walleij
This adds the missing GPIO bindings for the STMPE port expander
and converts the existing MFD binding to YAML.
ChangeLog v1->v2:
- Split off a separate GPIO binding
- Updated the MFD binding according to feedback
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
Linus Walleij (2):
dt-bindings: gpio: Add STMPE YAML DT schema
dt-bindings: MFD: Convert STMPE to YAML schema
.../devicetree/bindings/gpio/st,stmpe-gpio.yaml | 60 +++++
.../devicetree/bindings/input/stmpe-keypad.txt | 41 ---
.../bindings/input/touchscreen/stmpe.txt | 108 --------
.../devicetree/bindings/mfd/st,stmpe.yaml | 298 +++++++++++++++++++++
Documentation/devicetree/bindings/mfd/stmpe.txt | 42 ---
5 files changed, 358 insertions(+), 191 deletions(-)
---
base-commit: 457391b0380335d5e9a5babdec90ac53928b23b4
change-id: 20230426-stmpe-dt-bindings-c3479dd71a28
Best regards,
--
Linus Walleij <linus.walleij@linaro.org>
^ permalink raw reply
* [PATCH v2 1/2] dt-bindings: gpio: Add STMPE YAML DT schema
From: Linus Walleij @ 2023-04-26 11:21 UTC (permalink / raw)
To: Bartosz Golaszewski, Rob Herring, Krzysztof Kozlowski,
Maxime Coquelin, Alexandre Torgue, Dmitry Torokhov, Lee Jones,
Philippe Schenker, Stefan Agner, Marek Vasut, Steffen Trumtrar
Cc: linux-gpio, devicetree, linux-stm32, linux-arm-kernel,
linux-kernel, linux-input, Linus Walleij
In-Reply-To: <20230426-stmpe-dt-bindings-v2-0-2f85a1fffcda@linaro.org>
This adds a schema for the STMPE GPIO that while it is used a
lot in the kernel tree is anyway missing its bindings.
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- New patch split off from the MFD patch.
---
.../devicetree/bindings/gpio/st,stmpe-gpio.yaml | 60 ++++++++++++++++++++++
1 file changed, 60 insertions(+)
diff --git a/Documentation/devicetree/bindings/gpio/st,stmpe-gpio.yaml b/Documentation/devicetree/bindings/gpio/st,stmpe-gpio.yaml
new file mode 100644
index 000000000000..6e991ebbdf77
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/st,stmpe-gpio.yaml
@@ -0,0 +1,60 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/st,stmpe-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: STMicroelectonics Port Expander (STMPE) GPIO Block
+
+description: STMicroelectronics Port Expander (STMPE) is a series of slow
+ bus controllers for various expanded peripherals such as GPIO, keypad,
+ touchscreen, ADC, PWM or rotator. It can contain one or several different
+ peripherals connected to SPI or I2C. These bindings pertain to the
+ GPIO portions of these expanders.
+
+maintainers:
+ - Linus Walleij <linus.walleij@linaro.org>
+
+properties:
+ compatible:
+ const: st,stmpe-gpio
+
+ "#gpio-cells":
+ const: 2
+
+ "#interrupt-cells":
+ const: 2
+
+ gpio-controller: true
+
+ interrupt-controller: true
+
+ st,norequest-mask:
+ description: A bitmask of GPIO lines that cannot be requested because for
+ for example not being connected to anything on the system
+ $ref: /schemas/types.yaml#/definitions/uint32
+
+patternProperties:
+ "^.+-hog(-[0-9]+)?$":
+ type: object
+
+ properties:
+ gpio-hog: true
+ gpios: true
+ input: true
+ output-high: true
+ output-low: true
+ line-name: true
+
+ required:
+ - gpio-hog
+ - gpios
+
+additionalProperties: false
+
+required:
+ - compatible
+ - "#gpio-cells"
+ - "#interrupt-cells"
+ - gpio-controller
+ - interrupt-controller
--
2.34.1
^ permalink raw reply related
* [PATCH v2 2/2] dt-bindings: MFD: Convert STMPE to YAML schema
From: Linus Walleij @ 2023-04-26 11:21 UTC (permalink / raw)
To: Bartosz Golaszewski, Rob Herring, Krzysztof Kozlowski,
Maxime Coquelin, Alexandre Torgue, Dmitry Torokhov, Lee Jones,
Philippe Schenker, Stefan Agner, Marek Vasut, Steffen Trumtrar
Cc: linux-gpio, devicetree, linux-stm32, linux-arm-kernel,
linux-kernel, linux-input, Linus Walleij
In-Reply-To: <20230426-stmpe-dt-bindings-v2-0-2f85a1fffcda@linaro.org>
This converts the STMPE MFD device tree bindings to the YAML
schema.
Reference the existing schema for the ADC, just define the
other subnode schemas directly in the MFD schema.
Add two examples so we have examples covering both the simple
GPIO expander and the more complex with ADC and touchscreen.
Some in-tree users do not follow the naming conventions for nodes
so these DTS files need to be augmented to use proper node names
like "adc", "pwm", "gpio", "keyboard-controller" etc before the
bindings take effect on them.
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Split off the GPIO bindings to their own schema, as the old
bindings didn't even have any GPIO bindings. Put the GPIO
schema before this schema so we can use GPIO in the examples.
- Drop nodename and pattern as STMPE is not a generic name.
- Add maxItems to the resets.
- Make wakeup-source just :true, as it is a generic property.
- Move unevaluatedProperties for subnodes right before properties
as requested.
- Name devices "port-expander" in the examples.
- Use lowercase hex in line init.
---
.../devicetree/bindings/input/stmpe-keypad.txt | 41 ---
.../bindings/input/touchscreen/stmpe.txt | 108 --------
.../devicetree/bindings/mfd/st,stmpe.yaml | 298 +++++++++++++++++++++
Documentation/devicetree/bindings/mfd/stmpe.txt | 42 ---
4 files changed, 298 insertions(+), 191 deletions(-)
diff --git a/Documentation/devicetree/bindings/input/stmpe-keypad.txt b/Documentation/devicetree/bindings/input/stmpe-keypad.txt
deleted file mode 100644
index 12bb771d66d4..000000000000
--- a/Documentation/devicetree/bindings/input/stmpe-keypad.txt
+++ /dev/null
@@ -1,41 +0,0 @@
-* STMPE Keypad
-
-Required properties:
- - compatible : "st,stmpe-keypad"
- - linux,keymap : See ./matrix-keymap.txt
-
-Optional properties:
- - debounce-interval : Debouncing interval time in milliseconds
- - st,scan-count : Scanning cycles elapsed before key data is updated
- - st,no-autorepeat : If specified device will not autorepeat
- - keypad,num-rows : See ./matrix-keymap.txt
- - keypad,num-columns : See ./matrix-keymap.txt
-
-Example:
-
- stmpe_keypad {
- compatible = "st,stmpe-keypad";
-
- debounce-interval = <64>;
- st,scan-count = <8>;
- st,no-autorepeat;
-
- linux,keymap = <0x205006b
- 0x4010074
- 0x3050072
- 0x1030004
- 0x502006a
- 0x500000a
- 0x5008b
- 0x706001c
- 0x405000b
- 0x6070003
- 0x3040067
- 0x303006c
- 0x60400e7
- 0x602009e
- 0x4020073
- 0x5050002
- 0x4030069
- 0x3020008>;
- };
diff --git a/Documentation/devicetree/bindings/input/touchscreen/stmpe.txt b/Documentation/devicetree/bindings/input/touchscreen/stmpe.txt
deleted file mode 100644
index 238b51555c04..000000000000
--- a/Documentation/devicetree/bindings/input/touchscreen/stmpe.txt
+++ /dev/null
@@ -1,108 +0,0 @@
-STMPE Touchscreen
-----------------
-
-Required properties:
- - compatible: "st,stmpe-ts"
-
-Optional properties:
-- st,ave-ctrl : Sample average control
- 0 -> 1 sample
- 1 -> 2 samples
- 2 -> 4 samples
- 3 -> 8 samples
-- st,touch-det-delay : Touch detect interrupt delay (recommended is 3)
- 0 -> 10 us
- 1 -> 50 us
- 2 -> 100 us
- 3 -> 500 us
- 4 -> 1 ms
- 5 -> 5 ms
- 6 -> 10 ms
- 7 -> 50 ms
-- st,settling : Panel driver settling time (recommended is 2)
- 0 -> 10 us
- 1 -> 100 us
- 2 -> 500 us
- 3 -> 1 ms
- 4 -> 5 ms
- 5 -> 10 ms
- 6 -> 50 ms
- 7 -> 100 ms
-- st,fraction-z : Length of the fractional part in z (recommended is 7)
- (fraction-z ([0..7]) = Count of the fractional part)
-- st,i-drive : current limit value of the touchscreen drivers
- 0 -> 20 mA (typical 35mA max)
- 1 -> 50 mA (typical 80 mA max)
-
-Optional properties common with MFD (deprecated):
- - st,sample-time : ADC conversion time in number of clock.
- 0 -> 36 clocks
- 1 -> 44 clocks
- 2 -> 56 clocks
- 3 -> 64 clocks
- 4 -> 80 clocks (recommended)
- 5 -> 96 clocks
- 6 -> 124 clocks
- - st,mod-12b : ADC Bit mode
- 0 -> 10bit ADC
- 1 -> 12bit ADC
- - st,ref-sel : ADC reference source
- 0 -> internal
- 1 -> external
- - st,adc-freq : ADC Clock speed
- 0 -> 1.625 MHz
- 1 -> 3.25 MHz
- 2 || 3 -> 6.5 MHz
-
-Node should be child node of stmpe node to which it belongs.
-
-Note that common ADC settings of stmpe_touchscreen (child) will take precedence
-over the settings done in MFD.
-
-Example:
-
-stmpe811@41 {
- compatible = "st,stmpe811";
- pinctrl-names = "default";
- pinctrl-0 = <&pinctrl_touch_int>;
- #address-cells = <1>;
- #size-cells = <0>;
- reg = <0x41>;
- interrupts = <10 IRQ_TYPE_LEVEL_LOW>;
- interrupt-parent = <&gpio4>;
- interrupt-controller;
- id = <0>;
- blocks = <0x5>;
- irq-trigger = <0x1>;
- /* Common ADC settings */
- /* 3.25 MHz ADC clock speed */
- st,adc-freq = <1>;
- /* 12-bit ADC */
- st,mod-12b = <1>;
- /* internal ADC reference */
- st,ref-sel = <0>;
- /* ADC converstion time: 80 clocks */
- st,sample-time = <4>;
-
- stmpe_touchscreen {
- compatible = "st,stmpe-ts";
- reg = <0>;
- /* 8 sample average control */
- st,ave-ctrl = <3>;
- /* 5 ms touch detect interrupt delay */
- st,touch-det-delay = <5>;
- /* 1 ms panel driver settling time */
- st,settling = <3>;
- /* 7 length fractional part in z */
- st,fraction-z = <7>;
- /*
- * 50 mA typical 80 mA max touchscreen drivers
- * current limit value
- */
- st,i-drive = <1>;
- };
- stmpe_adc {
- compatible = "st,stmpe-adc";
- st,norequest-mask = <0x0F>;
- };
-};
diff --git a/Documentation/devicetree/bindings/mfd/st,stmpe.yaml b/Documentation/devicetree/bindings/mfd/st,stmpe.yaml
new file mode 100644
index 000000000000..dd24ae2d5fb4
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/st,stmpe.yaml
@@ -0,0 +1,298 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/st,stmpe.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: STMicroelectonics Port Expander (STMPE)
+
+description: STMicroelectronics Port Expander (STMPE) is a series of slow
+ bus controllers for various expanded peripherals such as GPIO, keypad,
+ touchscreen, ADC, PWM or rotator. It can contain one or several different
+ peripherals connected to SPI or I2C.
+
+maintainers:
+ - Linus Walleij <linus.walleij@linaro.org>
+
+allOf:
+ - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+ compatible:
+ enum:
+ - st,stmpe601
+ - st,stmpe801
+ - st,stmpe811
+ - st,stmpe1600
+ - st,stmpe1601
+ - st,stmpe2401
+ - st,stmpe2403
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ vcc-supply: true
+
+ vio-supply: true
+
+ reset-gpios:
+ maxItems: 1
+
+ wakeup-source: true
+
+ st,autosleep-timeout:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ enum: [ 4, 16, 32, 64, 128, 256, 512, 1024 ]
+ description: Time idle before going to automatic sleep to save power
+
+ st,sample-time:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ enum: [ 0, 1, 2, 3, 4, 5, 6 ]
+ description: |
+ Sample time per iteration
+ 0 = 36 clock ticks
+ 1 = 44 clock ticks
+ 2 = 56 clock ticks
+ 3 = 64 clock ticks
+ 4 = 80 clock ticks - recommended
+ 5 = 96 clock ticks
+ 6 = 124 clock ticks
+
+ st,mod-12b:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ enum: [ 0, 1 ]
+ description: ADC bit mode 0 = 10bit ADC, 1 = 12bit ADC
+
+ st,ref-sel:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ enum: [ 0, 1 ]
+ description: ADC reference source 0 = internal, 1 = external
+
+ st,adc-freq:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ enum: [ 0, 1, 2, 3 ]
+ description: |
+ ADC clock speed
+ 0 = 1.625 MHz
+ 1 = 3.25 MHz
+ 2, 3 = 6.5 MHz
+
+ adc:
+ type: object
+ $ref: /schemas/iio/adc/st,stmpe-adc.yaml#
+
+ gpio:
+ type: object
+ $ref: /schemas/gpio/st,stmpe-gpio.yaml#
+
+ keyboard-controller:
+ type: object
+ $ref: /schemas/input/matrix-keymap.yaml#
+
+ unevaluatedProperties: false
+
+ properties:
+ compatible:
+ const: st,stmpe-keypad
+
+ debounce-interval:
+ description: Debouncing interval in milliseconds
+ $ref: /schemas/types.yaml#/definitions/uint32
+
+ st,no-autorepeat:
+ description: If present, the keys will not autorepeat when pressed
+ $ref: /schemas/types.yaml#/definitions/flag
+
+ st,scan-count:
+ description: Scanning cycles elapsed before key data is updated
+ $ref: /schemas/types.yaml#/definitions/uint32
+
+ required:
+ - compatible
+ - linux,keymap
+
+ pwm:
+ type: object
+ $ref: /schemas/pwm/pwm.yaml#
+
+ unevaluatedProperties: false
+
+ properties:
+ compatible:
+ const: st,stmpe-pwm
+
+ required:
+ - compatible
+ - "#pwm-cells"
+
+ touchscreen:
+ type: object
+ $ref: /schemas/input/touchscreen/touchscreen.yaml#
+
+ unevaluatedProperties: false
+
+ properties:
+ compatible:
+ const: st,stmpe-ts
+
+ st,ave-ctrl:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ enum: [ 0, 1, 2, 3 ]
+ description: |
+ Sample average control
+ 0 = 1 sample
+ 1 = 2 samples
+ 2 = 4 samples
+ 3 = 8 samples
+
+ st,touch-det-delay:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ enum: [ 0, 1, 2, 3, 4, 5, 6, 7 ]
+ description: |
+ Touch detection delay
+ 0 = 10 us
+ 1 = 50 us
+ 2 = 100 us
+ 3 = 500 us - recommended
+ 4 = 1 ms
+ 5 = 5 ms
+ 6 = 10 ms
+ 7 = 50 ms
+
+ st,settling:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ enum: [ 0, 1, 2, 3, 4, 5, 6, 7 ]
+ description: |
+ Panel driver settling time
+ 0 = 10 us
+ 1 = 100 us
+ 2 = 500 us - recommended
+ 3 = 1 ms
+ 4 = 5 ms
+ 5 = 10 ms
+ 6 = 50 ms
+ 7 = 100 ms
+
+ st,fraction-z:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ enum: [ 0, 1, 2, 3, 4, 5, 6, 7 ]
+ description: Length of the fractional part in z, recommended is 7
+ (fraction-z ([0..7]) = Count of the fractional part)
+
+ st,i-drive:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ enum: [ 0, 1 ]
+ description: |
+ current limit value of the touchscreen drivers
+ 0 = 20 mA (typical 35 mA max)
+ 1 = 50 mA (typical 80 mA max)
+
+ required:
+ - compatible
+
+additionalProperties: false
+
+required:
+ - compatible
+ - reg
+ - interrupts
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+ #include <dt-bindings/interrupt-controller/irq.h>
+ #include <dt-bindings/input/input.h>
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port-expander@43 {
+ compatible = "st,stmpe2401";
+ reg = <0x43>;
+ reset-gpios = <&gpio 13 GPIO_ACTIVE_LOW>;
+ interrupts = <26 IRQ_TYPE_EDGE_FALLING>;
+ interrupt-parent = <&gpio>;
+ vcc-supply = <&db8500_vsmps2_reg>;
+ vio-supply = <&db8500_vsmps2_reg>;
+ wakeup-source;
+ st,autosleep-timeout = <1024>;
+
+ gpio {
+ compatible = "st,stmpe-gpio";
+ gpio-controller;
+ #gpio-cells = <2>;
+ interrupt-controller;
+ #interrupt-cells = <2>;
+ st,norequest-mask = <0xf0f002>;
+ };
+
+ keyboard-controller {
+ compatible = "st,stmpe-keypad";
+ debounce-interval = <64>;
+ st,scan-count = <8>;
+ st,no-autorepeat;
+ keypad,num-rows = <8>;
+ keypad,num-columns = <8>;
+ linux,keymap = <
+ MATRIX_KEY(0x00, 0x00, KEY_1)
+ MATRIX_KEY(0x00, 0x01, KEY_2)
+ MATRIX_KEY(0x00, 0x02, KEY_3)
+ MATRIX_KEY(0x00, 0x03, KEY_4)
+ MATRIX_KEY(0x00, 0x04, KEY_5)
+ MATRIX_KEY(0x00, 0x05, KEY_6)
+ MATRIX_KEY(0x00, 0x06, KEY_7)
+ MATRIX_KEY(0x00, 0x07, KEY_8)
+ MATRIX_KEY(0x00, 0x08, KEY_9)
+ MATRIX_KEY(0x00, 0x09, KEY_0)
+ >;
+ };
+
+ pwm {
+ compatible = "st,stmpe-pwm";
+ #pwm-cells = <2>;
+ };
+ };
+
+ port-expander@41 {
+ compatible = "st,stmpe811";
+ reg = <0x41>;
+ interrupts = <10 IRQ_TYPE_LEVEL_LOW>;
+ interrupt-parent = <&gpio>;
+ st,adc-freq = <1>;
+ st,mod-12b = <1>;
+ st,ref-sel = <0>;
+ st,sample-time = <4>;
+
+ adc {
+ compatible = "st,stmpe-adc";
+ st,norequest-mask = <0x0f>;
+ #io-channel-cells = <1>;
+ };
+
+ gpio {
+ compatible = "st,stmpe-gpio";
+ gpio-controller;
+ #gpio-cells = <2>;
+ interrupt-controller;
+ #interrupt-cells = <2>;
+ };
+
+ pwm {
+ compatible = "st,stmpe-pwm";
+ #pwm-cells = <2>;
+ };
+
+ touchscreen {
+ compatible = "st,stmpe-ts";
+ st,ave-ctrl = <3>;
+ st,touch-det-delay = <5>;
+ st,settling = <3>;
+ st,fraction-z = <7>;
+ st,i-drive = <1>;
+ };
+ };
+ };
+...
diff --git a/Documentation/devicetree/bindings/mfd/stmpe.txt b/Documentation/devicetree/bindings/mfd/stmpe.txt
deleted file mode 100644
index d4408a417193..000000000000
--- a/Documentation/devicetree/bindings/mfd/stmpe.txt
+++ /dev/null
@@ -1,42 +0,0 @@
-* ST Microelectronics STMPE Multi-Functional Device
-
-STMPE is an MFD device which may expose the following inbuilt devices: gpio,
-keypad, touchscreen, adc, pwm, rotator.
-
-Required properties:
- - compatible : "st,stmpe[610|801|811|1600|1601|2401|2403]"
- - reg : I2C/SPI address of the device
-
-Optional properties:
- - interrupts : The interrupt outputs from the controller
- - interrupt-controller : Marks the device node as an interrupt controller
- - wakeup-source : Marks the input device as wakable
- - st,autosleep-timeout : Valid entries (ms); 4, 16, 32, 64, 128, 256, 512 and 1024
- - irq-gpio : If present, which GPIO to use for event IRQ
-
-Optional properties for devices with touch and ADC (STMPE811|STMPE610):
- - st,sample-time : ADC conversion time in number of clock.
- 0 -> 36 clocks 4 -> 80 clocks (recommended)
- 1 -> 44 clocks 5 -> 96 clocks
- 2 -> 56 clocks 6 -> 124 clocks
- 3 -> 64 clocks
- - st,mod-12b : ADC Bit mode
- 0 -> 10bit ADC 1 -> 12bit ADC
- - st,ref-sel : ADC reference source
- 0 -> internal 1 -> external
- - st,adc-freq : ADC Clock speed
- 0 -> 1.625 MHz 2 || 3 -> 6.5 MHz
- 1 -> 3.25 MHz
-
-Example:
-
- stmpe1601: stmpe1601@40 {
- compatible = "st,stmpe1601";
- reg = <0x40>;
- interrupts = <26 0x4>;
- interrupt-parent = <&gpio6>;
- interrupt-controller;
-
- wakeup-source;
- st,autosleep-timeout = <1024>;
- };
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v2 2/2] HID: i2c-hid: goodix: Add support for "goodix,no-reset-during-suspend" property
From: Doug Anderson @ 2023-04-26 14:05 UTC (permalink / raw)
To: Fei Shao
Cc: Jeff LaBundy, Benjamin Tissoires, Rob Herring, linux-mediatek,
Dmitry Torokhov, Jiri Kosina, Matthias Kaehlcke, Stephen Kitt,
linux-input, linux-kernel
In-Reply-To: <20230426093231.1466984-3-fshao@chromium.org>
Hi,
On Wed, Apr 26, 2023 at 2:33 AM Fei Shao <fshao@chromium.org> wrote:
>
> In the beginning, commit 18eeef46d359 ("HID: i2c-hid: goodix: Tie the
> reset line to true state of the regulator") introduced a change to tie
> the reset line of the Goodix touchscreen to the state of the regulator
> to fix a power leakage issue in suspend.
>
> After some time, the change was deemed unnecessary and was reverted in
> commit 557e05fa9fdd ("HID: i2c-hid: goodix: Stop tying the reset line to
> the regulator") due to difficulties in managing regulator notifiers for
> designs like Evoker, which provides a second power rail to touchscreen.
>
> However, the revert caused a power regression on another Chromebook
> device Steelix in the field, which has a dedicated always-on regulator
> for touchscreen and was covered by the workaround in the first commit.
>
> To address both cases, this patch adds the support for the new
> "goodix,no-reset-during-suspend" property in the driver:
> - When set to true, the driver does not assert the reset GPIO during
> power-down.
> Instead, the GPIO will be asserted during power-up to ensure the
> touchscreen always has a clean start and consistent behavior after
> resuming.
> This is for designs with a dedicated always-on regulator.
> - When set to false or unset, the driver uses the original control flow
> and asserts GPIO and disable regulators normally.
> This is for the two-regulator and shared-regulator designs.
>
> Signed-off-by: Fei Shao <fshao@chromium.org>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
>
> ---
>
> Changes in v2:
> - Do not change the regulator_enable logic during power-up.
>
> drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 26 +++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> index 0060e3dcd775..fc4532fcadcc 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> @@ -28,6 +28,7 @@ struct i2c_hid_of_goodix {
> struct regulator *vdd;
> struct regulator *vddio;
> struct gpio_desc *reset_gpio;
> + bool no_reset_during_suspend;
> const struct goodix_i2c_hid_timing_data *timings;
> };
>
> @@ -37,6 +38,20 @@ static int goodix_i2c_hid_power_up(struct i2chid_ops *ops)
> container_of(ops, struct i2c_hid_of_goodix, ops);
> int ret;
>
> + if (ihid_goodix->no_reset_during_suspend) {
> + /*
> + * This is not mandatory, but we assert reset here (instead of
> + * during power-down) to ensure the device will have a clean
> + * state after powering up, just like the normal scenarios will
> + * have.
> + *
> + * Note that in this case we assume the regulators should be
> + * (marked as) always-on, so the regulator core knows what to
> + * do with them in the following regulator_enable() calls
> + * despite regulator_disable() was not called previously.
> + */
> + gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
> + }
> ret = regulator_enable(ihid_goodix->vdd);
> if (ret)
> return ret;
> @@ -60,6 +75,14 @@ static void goodix_i2c_hid_power_down(struct i2chid_ops *ops)
> struct i2c_hid_of_goodix *ihid_goodix =
> container_of(ops, struct i2c_hid_of_goodix, ops);
>
> + /*
> + * Don't assert reset GPIO if it's set.
> + * Also, it's okay to skip the following regulator_disable() calls
> + * because the regulators should be always-on in this case.
> + */
> + if (ihid_goodix->no_reset_during_suspend)
> + return;
> +
> gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
> regulator_disable(ihid_goodix->vddio);
> regulator_disable(ihid_goodix->vdd);
I think the above is wrong. You should just skip the GPIO call when
"no_reset_during_suspend", not the regulator calls. As your code is
written, you'll enable the regulators over and over again in
"power_up" and never in "power_down".
-Doug
^ permalink raw reply
* Re: [PATCH v2 2/2] HID: i2c-hid: goodix: Add support for "goodix,no-reset-during-suspend" property
From: Fei Shao @ 2023-04-26 14:24 UTC (permalink / raw)
To: Doug Anderson
Cc: Jeff LaBundy, Benjamin Tissoires, Rob Herring, linux-mediatek,
Dmitry Torokhov, Jiri Kosina, Matthias Kaehlcke, Stephen Kitt,
linux-input, linux-kernel
In-Reply-To: <CAD=FV=VY+01V7hBfUeZKR3OnnTcNuuYb-vhLWGgdS5jb4QTLBQ@mail.gmail.com>
On Wed, Apr 26, 2023 at 10:05 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Wed, Apr 26, 2023 at 2:33 AM Fei Shao <fshao@chromium.org> wrote:
> >
> > In the beginning, commit 18eeef46d359 ("HID: i2c-hid: goodix: Tie the
> > reset line to true state of the regulator") introduced a change to tie
> > the reset line of the Goodix touchscreen to the state of the regulator
> > to fix a power leakage issue in suspend.
> >
> > After some time, the change was deemed unnecessary and was reverted in
> > commit 557e05fa9fdd ("HID: i2c-hid: goodix: Stop tying the reset line to
> > the regulator") due to difficulties in managing regulator notifiers for
> > designs like Evoker, which provides a second power rail to touchscreen.
> >
> > However, the revert caused a power regression on another Chromebook
> > device Steelix in the field, which has a dedicated always-on regulator
> > for touchscreen and was covered by the workaround in the first commit.
> >
> > To address both cases, this patch adds the support for the new
> > "goodix,no-reset-during-suspend" property in the driver:
> > - When set to true, the driver does not assert the reset GPIO during
> > power-down.
> > Instead, the GPIO will be asserted during power-up to ensure the
> > touchscreen always has a clean start and consistent behavior after
> > resuming.
> > This is for designs with a dedicated always-on regulator.
> > - When set to false or unset, the driver uses the original control flow
> > and asserts GPIO and disable regulators normally.
> > This is for the two-regulator and shared-regulator designs.
> >
> > Signed-off-by: Fei Shao <fshao@chromium.org>
> > Reviewed-by: Douglas Anderson <dianders@chromium.org>
> >
> > ---
> >
> > Changes in v2:
> > - Do not change the regulator_enable logic during power-up.
> >
> > drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 26 +++++++++++++++++++++++++
> > 1 file changed, 26 insertions(+)
> >
> > diff --git a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> > index 0060e3dcd775..fc4532fcadcc 100644
> > --- a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> > +++ b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> > @@ -28,6 +28,7 @@ struct i2c_hid_of_goodix {
> > struct regulator *vdd;
> > struct regulator *vddio;
> > struct gpio_desc *reset_gpio;
> > + bool no_reset_during_suspend;
> > const struct goodix_i2c_hid_timing_data *timings;
> > };
> >
> > @@ -37,6 +38,20 @@ static int goodix_i2c_hid_power_up(struct i2chid_ops *ops)
> > container_of(ops, struct i2c_hid_of_goodix, ops);
> > int ret;
> >
> > + if (ihid_goodix->no_reset_during_suspend) {
> > + /*
> > + * This is not mandatory, but we assert reset here (instead of
> > + * during power-down) to ensure the device will have a clean
> > + * state after powering up, just like the normal scenarios will
> > + * have.
> > + *
> > + * Note that in this case we assume the regulators should be
> > + * (marked as) always-on, so the regulator core knows what to
> > + * do with them in the following regulator_enable() calls
> > + * despite regulator_disable() was not called previously.
> > + */
> > + gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
> > + }
> > ret = regulator_enable(ihid_goodix->vdd);
> > if (ret)
> > return ret;
> > @@ -60,6 +75,14 @@ static void goodix_i2c_hid_power_down(struct i2chid_ops *ops)
> > struct i2c_hid_of_goodix *ihid_goodix =
> > container_of(ops, struct i2c_hid_of_goodix, ops);
> >
> > + /*
> > + * Don't assert reset GPIO if it's set.
> > + * Also, it's okay to skip the following regulator_disable() calls
> > + * because the regulators should be always-on in this case.
> > + */
> > + if (ihid_goodix->no_reset_during_suspend)
> > + return;
> > +
> > gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
> > regulator_disable(ihid_goodix->vddio);
> > regulator_disable(ihid_goodix->vdd);
>
> I think the above is wrong. You should just skip the GPIO call when
> "no_reset_during_suspend", not the regulator calls. As your code is
> written, you'll enable the regulators over and over again in
> "power_up" and never in "power_down".
Agree, I'll resend v3. Thanks for the feedback!
Regards,
Fei
>
> -Doug
^ permalink raw reply
* [PATCH v3 0/2] Fix Goodix touchscreen power leakage for MT8186 boards
From: Fei Shao @ 2023-04-26 14:44 UTC (permalink / raw)
To: Jeff LaBundy, Douglas Anderson, Benjamin Tissoires, Rob Herring
Cc: linux-mediatek, Fei Shao, Dmitry Torokhov, Jiri Kosina,
Krzysztof Kozlowski, Matthias Kaehlcke, Stephen Kitt, devicetree,
linux-input, linux-kernel
These changes are based on the series in [1], which modified the
i2c-hid-of-goodix driver and removed the workaround for a power leakage
issue, so the issue revisits on Mediatek MT8186 boards (Steelix).
The root cause is that the touchscreen can be powered in different ways
depending on the hardware designs, and it's not as easy to come up with
a solution that is both simple and elegant for all the known designs.
To address the issue, I ended up adding a new boolean property for the
driver so that we can control the power up/down sequence depending on
that.
Adding a new property might not be the cleanest approach for this, but
at least the intention would be easy enough to understand, and it
introduces relatively small change to the code and fully preserves the
original control flow.
I hope this is something acceptable, and I'm open to any better
approaches.
[1] https://lore.kernel.org/all/20230207024816.525938-1-dianders@chromium.org/
Changes in v3:
- In power-down, only skip the GPIO but not the regulator calls if the
flag is set
Changes in v2:
- Use a more accurate property name and with "goodix," prefix.
- Do not change the regulator_enable logic during power-up.
Fei Shao (2):
dt-bindings: input: goodix: Add "goodix,no-reset-during-suspend"
property
HID: i2c-hid: goodix: Add support for "goodix,no-reset-during-suspend"
property
.../bindings/input/goodix,gt7375p.yaml | 9 +++++++++
drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 16 +++++++++++++++-
2 files changed, 24 insertions(+), 1 deletion(-)
--
2.40.1.495.gc816e09b53d-goog
^ permalink raw reply
* [PATCH v3 1/2] dt-bindings: input: goodix: Add "goodix,no-reset-during-suspend" property
From: Fei Shao @ 2023-04-26 14:44 UTC (permalink / raw)
To: Jeff LaBundy, Douglas Anderson, Benjamin Tissoires, Rob Herring
Cc: linux-mediatek, Fei Shao, Matthias Brugger, Dmitry Torokhov,
Krzysztof Kozlowski, devicetree, linux-input, linux-kernel
In-Reply-To: <20230426144423.2820826-1-fshao@chromium.org>
We observed that on Chromebook device Steelix, if Goodix GT7375P
touchscreen is powered in suspend (because, for example, it connects to
an always-on regulator) and with the reset GPIO asserted, it will
introduce about 14mW power leakage.
To address that, we add this property to skip reset during suspend.
If it's set, the driver will stop asserting the reset GPIO during
power-down. Refer to the comments in the driver for details.
Signed-off-by: Fei Shao <fshao@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
---
(no changes since v2)
Changes in v2:
- Use a more accurate property name and with "goodix," prefix.
.../devicetree/bindings/input/goodix,gt7375p.yaml | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
index ce18d7dadae2..1edad1da1196 100644
--- a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
+++ b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
@@ -43,6 +43,15 @@ properties:
itself as long as it allows the main board to make signals compatible
with what the touchscreen is expecting for its IO rails.
+ goodix,no-reset-during-suspend:
+ description:
+ Set this to true to enforce the driver to not assert the reset GPIO
+ during suspend.
+ Due to potential touchscreen hardware flaw, back-powering could happen in
+ suspend if the power supply is on and with active-low reset GPIO asserted.
+ This property is used to avoid the back-powering issue.
+ type: boolean
+
required:
- compatible
- reg
--
2.40.1.495.gc816e09b53d-goog
^ permalink raw reply related
* [PATCH v3 2/2] HID: i2c-hid: goodix: Add support for "goodix,no-reset-during-suspend" property
From: Fei Shao @ 2023-04-26 14:44 UTC (permalink / raw)
To: Jeff LaBundy, Douglas Anderson, Benjamin Tissoires, Rob Herring
Cc: linux-mediatek, Fei Shao, Dmitry Torokhov, Jiri Kosina,
Matthias Kaehlcke, Stephen Kitt, linux-input, linux-kernel
In-Reply-To: <20230426144423.2820826-1-fshao@chromium.org>
In the beginning, commit 18eeef46d359 ("HID: i2c-hid: goodix: Tie the
reset line to true state of the regulator") introduced a change to tie
the reset line of the Goodix touchscreen to the state of the regulator
to fix a power leakage issue in suspend.
After some time, the change was deemed unnecessary and was reverted in
commit 557e05fa9fdd ("HID: i2c-hid: goodix: Stop tying the reset line to
the regulator") due to difficulties in managing regulator notifiers for
designs like Evoker, which provides a second power rail to touchscreen.
However, the revert caused a power regression on another Chromebook
device Steelix in the field, which has a dedicated always-on regulator
for touchscreen and was covered by the workaround in the first commit.
To address both cases, this patch adds the support for the new
"goodix,no-reset-during-suspend" property in the driver:
- When set to true, the driver does not assert the reset GPIO during
power-down.
Instead, the GPIO will be asserted during power-up to ensure the
touchscreen always has a clean start and consistent behavior after
resuming.
This is for designs with a dedicated always-on regulator.
- When set to false or unset, the driver uses the original control flow
and asserts GPIO and disables regulators normally.
This is for the two-regulator and shared-regulator designs.
Signed-off-by: Fei Shao <fshao@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
Changes in v3:
- In power-down, only skip the GPIO but not the regulator calls if the
flag is set
Changes in v2:
- Do not change the regulator_enable logic during power-up.
drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
index 0060e3dcd775..3ed365b50432 100644
--- a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
+++ b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
@@ -28,6 +28,7 @@ struct i2c_hid_of_goodix {
struct regulator *vdd;
struct regulator *vddio;
struct gpio_desc *reset_gpio;
+ bool no_reset_during_suspend;
const struct goodix_i2c_hid_timing_data *timings;
};
@@ -37,6 +38,14 @@ static int goodix_i2c_hid_power_up(struct i2chid_ops *ops)
container_of(ops, struct i2c_hid_of_goodix, ops);
int ret;
+ if (ihid_goodix->no_reset_during_suspend) {
+ /*
+ * We assert reset GPIO here (instead of during power-down) to
+ * ensure the device will have a clean state after powering up,
+ * just like the normal scenarios will have.
+ */
+ gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
+ }
ret = regulator_enable(ihid_goodix->vdd);
if (ret)
return ret;
@@ -60,7 +69,9 @@ static void goodix_i2c_hid_power_down(struct i2chid_ops *ops)
struct i2c_hid_of_goodix *ihid_goodix =
container_of(ops, struct i2c_hid_of_goodix, ops);
- gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
+ if (!ihid_goodix->no_reset_during_suspend)
+ gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
+
regulator_disable(ihid_goodix->vddio);
regulator_disable(ihid_goodix->vdd);
}
@@ -91,6 +102,9 @@ static int i2c_hid_of_goodix_probe(struct i2c_client *client)
if (IS_ERR(ihid_goodix->vddio))
return PTR_ERR(ihid_goodix->vddio);
+ ihid_goodix->no_reset_during_suspend =
+ of_property_read_bool(client->dev.of_node, "goodix,no-reset-during-suspend");
+
ihid_goodix->timings = device_get_match_data(&client->dev);
return i2c_hid_core_probe(client, &ihid_goodix->ops, 0x0001, 0);
--
2.40.1.495.gc816e09b53d-goog
^ permalink raw reply related
* Re: [PATCH v3 2/2] HID: i2c-hid: goodix: Add support for "goodix,no-reset-during-suspend" property
From: Doug Anderson @ 2023-04-26 16:39 UTC (permalink / raw)
To: Fei Shao
Cc: Jeff LaBundy, Benjamin Tissoires, Rob Herring, linux-mediatek,
Dmitry Torokhov, Jiri Kosina, Matthias Kaehlcke, Stephen Kitt,
linux-input, linux-kernel
In-Reply-To: <20230426144423.2820826-3-fshao@chromium.org>
Hi,
On Wed, Apr 26, 2023 at 7:44 AM Fei Shao <fshao@chromium.org> wrote:
>
> In the beginning, commit 18eeef46d359 ("HID: i2c-hid: goodix: Tie the
> reset line to true state of the regulator") introduced a change to tie
> the reset line of the Goodix touchscreen to the state of the regulator
> to fix a power leakage issue in suspend.
>
> After some time, the change was deemed unnecessary and was reverted in
> commit 557e05fa9fdd ("HID: i2c-hid: goodix: Stop tying the reset line to
> the regulator") due to difficulties in managing regulator notifiers for
> designs like Evoker, which provides a second power rail to touchscreen.
>
> However, the revert caused a power regression on another Chromebook
> device Steelix in the field, which has a dedicated always-on regulator
> for touchscreen and was covered by the workaround in the first commit.
>
> To address both cases, this patch adds the support for the new
> "goodix,no-reset-during-suspend" property in the driver:
> - When set to true, the driver does not assert the reset GPIO during
> power-down.
> Instead, the GPIO will be asserted during power-up to ensure the
> touchscreen always has a clean start and consistent behavior after
> resuming.
> This is for designs with a dedicated always-on regulator.
> - When set to false or unset, the driver uses the original control flow
> and asserts GPIO and disables regulators normally.
> This is for the two-regulator and shared-regulator designs.
>
> Signed-off-by: Fei Shao <fshao@chromium.org>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
>
> ---
>
> Changes in v3:
> - In power-down, only skip the GPIO but not the regulator calls if the
> flag is set
>
> Changes in v2:
> - Do not change the regulator_enable logic during power-up.
>
> drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
You already carried over my Reviewed-by tag, which is fine. ...but
just sending a quick confirmation that v3 looks good to me. Thanks!
-Doug
^ permalink raw reply
* Re: [PATCH 00/43] ep93xx device tree conversion
From: Linus Walleij @ 2023-04-26 20:56 UTC (permalink / raw)
To: Nikita Shubin
Cc: Arnd Bergmann, Linus Walleij, Alexander Sverdlin, David S. Miller,
Jonathan Neuschäfer, Russell King (Oracle),
Uwe Kleine-König, Alessandro Zummo, Alexander Gordeev,
Alexandre Belloni, Andy Shevchenko, Arnd Bergmann,
Bartosz Golaszewski, Brian Norris, Chuanhong Guo, Conor Dooley,
Damien Le Moal, Daniel Lezcano, Dmitry Torokhov,
Emil Renner Berthing, Eric Dumazet, Florian Fainelli,
Guenter Roeck, Hartley Sweeten, Heiko Stuebner, Hitomi Hasegawa,
Jakub Kicinski, Jaroslav Kysela, Jean Delvare, Joel Stanley,
Jonathan Cameron, Krzysztof Kozlowski, Le Moal, Liam Girdwood,
Liang Yang, Lukasz Majewski, Lv Ruyi, Mark Brown, Masahiro Yamada,
Michael Turquette, Miquel Raynal, Nathan Chancellor,
Nick Desaulniers, Nicolas Saenz Julienne, Olof Johansson,
Paolo Abeni, Qin Jian, Richard Weinberger, Rob Herring,
Robert Jarzmik, Russell King, Sebastian Reichel, Sergey Shtylyov,
Stephen Boyd, Sumanth Korikkar, Sven Peter, Takashi Iwai,
Thierry Reding, Thomas Gleixner, Ulf Hansson, Vasily Gorbik,
Vignesh Raghavendra, Vinod Koul, Walker Chen, Wim Van Sebroeck,
Yinbo Zhu, alsa-devel, devicetree, dmaengine, linux-arm-kernel,
linux-clk, linux-gpio, linux-ide, linux-input, linux-kernel,
linux-mtd, linux-pm, linux-pwm, linux-rtc, linux-spi,
linux-watchdog, netdev, soc
In-Reply-To: <20230424123522.18302-1-nikita.shubin@maquefel.me>
On Mon, Apr 24, 2023 at 11:35 AM Nikita Shubin
<nikita.shubin@maquefel.me> wrote:
> This series aims to convert ep93xx from platform to full device tree support.
>
> Tested on ts7250 64 RAM/128 MiB Nand flash, edb9302.
Neat, I'd say let's merge this for 6.5 once the final rough edges are
off. The DT bindings should be easy to fix.
This is a big patch set and the improvement to the ARM kernel it
brings is great, so I am a bit worried about over-review stalling the
merged. If there start to be nitpicky comments I would prefer that
we merge it and let minor comments and "nice-to-haves" be
addressed in-tree during the development cycle.
I encourage you to use b4 to manage the patch series if you
have time to learn it, it could help you:
https://people.kernel.org/monsieuricon/sending-a-kernel-patch-with-b4-part-1
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 00/43] ep93xx device tree conversion
From: Linus Walleij @ 2023-04-26 21:06 UTC (permalink / raw)
To: Mark Brown
Cc: Nikita Shubin, Arnd Bergmann, Linus Walleij, Alexander Sverdlin,
David S. Miller, Jonathan Neuschäfer, Russell King (Oracle),
Uwe Kleine-König, Alessandro Zummo, Alexander Gordeev,
Alexandre Belloni, Andy Shevchenko, Arnd Bergmann,
Bartosz Golaszewski, Brian Norris, Chuanhong Guo, Conor Dooley,
Damien Le Moal, Daniel Lezcano, Dmitry Torokhov,
Emil Renner Berthing, Eric Dumazet, Florian Fainelli,
Guenter Roeck, Hartley Sweeten, Heiko Stuebner, Hitomi Hasegawa,
Jakub Kicinski, Jaroslav Kysela, Jean Delvare, Joel Stanley,
Jonathan Cameron, Krzysztof Kozlowski, Le Moal, Liam Girdwood,
Liang Yang, Lukasz Majewski, Lv Ruyi, Masahiro Yamada,
Michael Turquette, Miquel Raynal, Nathan Chancellor,
Nick Desaulniers, Nicolas Saenz Julienne, Olof Johansson,
Paolo Abeni, Qin Jian, Richard Weinberger, Rob Herring,
Robert Jarzmik, Russell King, Sebastian Reichel, Sergey Shtylyov,
Stephen Boyd, Sumanth Korikkar, Sven Peter, Takashi Iwai,
Thierry Reding, Thomas Gleixner, Ulf Hansson, Vasily Gorbik,
Vignesh Raghavendra, Vinod Koul, Walker Chen, Wim Van Sebroeck,
Yinbo Zhu, alsa-devel, devicetree, dmaengine, linux-arm-kernel,
linux-clk, linux-gpio, linux-ide, linux-input, linux-kernel,
linux-mtd, linux-pm, linux-pwm, linux-rtc, linux-spi,
linux-watchdog, netdev, soc
In-Reply-To: <b5396ef5-3fed-4e98-8f37-a9cd4473bddc@sirena.org.uk>
On Wed, Apr 26, 2023 at 11:02 PM Mark Brown <broonie@kernel.org> wrote:
> On Wed, Apr 26, 2023 at 10:56:53PM +0200, Linus Walleij wrote:
>
> > This is a big patch set and the improvement to the ARM kernel it
> > brings is great, so I am a bit worried about over-review stalling the
> > merged. If there start to be nitpicky comments I would prefer that
> > we merge it and let minor comments and "nice-to-haves" be
> > addressed in-tree during the development cycle.
>
> I'm really not enthusiastic about the SPI bindings being merged as-is.
Agree, the bindings are more important than the code IMO,
they tend to get written in stone.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH] Input: elan_i2c - Implement inhibit/uninhibit functions.
From: 'Dmitry Torokhov' @ 2023-04-26 22:57 UTC (permalink / raw)
To: Jingle.Wu; +Cc: linux-kernel, linux-input, phoenix, josh.chen, dave.wang
In-Reply-To: <000001d96b4b$6b30cda0$419268e0$@emc.com.tw>
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
^ permalink raw reply
* Re: [PATCH] Fix freeze in lm8333 i2c keyboard driver
From: Dmitry Torokhov @ 2023-04-26 23:16 UTC (permalink / raw)
To: Jeff LaBundy; +Cc: Tomas Mudrunka, linux-input, linux-kernel
In-Reply-To: <ZEf0RYdD5jhE9JEk@nixie71>
Hi Jeff, Tomas,
On Tue, Apr 25, 2023 at 10:39:49AM -0500, Jeff LaBundy wrote:
> Hi Tomas,
>
> On Tue, Apr 25, 2023 at 03:00:53PM +0200, Tomas Mudrunka wrote:
> > LM8333 uses gpio interrupt line which is active-low.
> > When interrupt is set to FALLING edge and button is pressed
> > before driver loads, driver will miss the edge and never respond.
> > To fix this we handle ONESHOT LOW interrupt rather than edge.
> >
> > Signed-off-by: Tomas Mudrunka <tomas.mudrunka@gmail.com>
> > ---
> > drivers/input/keyboard/lm8333.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/input/keyboard/lm8333.c b/drivers/input/keyboard/lm8333.c
> > index 7457c3220..c5770ebb2 100644
> > --- a/drivers/input/keyboard/lm8333.c
> > +++ b/drivers/input/keyboard/lm8333.c
> > @@ -179,7 +179,7 @@ static int lm8333_probe(struct i2c_client *client)
> > }
> >
> > err = request_threaded_irq(client->irq, NULL, lm8333_irq_thread,
> > - IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> > + IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> > "lm8333", lm8333);
> > if (err)
> > goto free_mem;
>
> Thanks for the patch, but this is a NAK in my opinion.
>
> First of all, we should not be hard-coding interrupt polarity in the
> first place; that is an existing piece of technical debt in this driver.
Yes, I wonder if the original hardware was limited to the edge
interrupts.
>
> Second, changing from edge-triggered to level-triggered interrupts runs
> the risk of creating an interrupt storm depending on the time it takes
> the device to deassert the irq following the I2C read and the point at
> which the threaded handler returns. Have you measured this?
IRQF_ONESHOT ensures that the level interrupt is unmasked only when the
threaded handler returns.
>
> Can we not simply read the interrupt status registers once at start-up
> to clear any pending status? This is essentially what your change does
> anyway, albeit indirectly.
>
Thanks.
--
Dmitry
^ permalink raw reply
* [PATCH] Call input_set_timestamp for events injected using uinput
From: Biswarup Pal @ 2023-04-27 0:01 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Biswarup Pal, kernel-team, linux-input, linux-kernel
Currently, uinput doesn't use the input_set_timestamp API, so any
event injected using uinput is not accurately timestamped in terms of
measuring when the actual event happened. Hence, call the
input_set_timestamp API from uinput in order to provide a more
accurate sense of time for the event. Propagate only the timestamps
which are a) positive, b) within a pre-defined offset (10 secs) from
the current time, and c) not in the future.
Signed-off-by: Biswarup Pal <biswarupp@google.com>
---
drivers/input/misc/uinput.c | 34 ++++++++++++++++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index f2593133e524..d98212d55108 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -33,6 +33,7 @@
#define UINPUT_NAME "uinput"
#define UINPUT_BUFFER_SIZE 16
#define UINPUT_NUM_REQUESTS 16
+#define UINPUT_TIMESTAMP_ALLOWED_OFFSET_SECS 10
enum uinput_state { UIST_NEW_DEVICE, UIST_SETUP_COMPLETE, UIST_CREATED };
@@ -569,11 +570,40 @@ static int uinput_setup_device_legacy(struct uinput_device *udev,
return retval;
}
+/*
+ * Returns true if the given timestamp is valid (i.e., if all the following
+ * conditions are satisfied), false otherwise.
+ * 1) given timestamp is positive
+ * 2) it's within the allowed offset before the current time
+ * 3) it's not in the future
+ */
+static bool is_valid_timestamp(const ktime_t timestamp)
+{
+ ktime_t zero_time;
+ ktime_t current_time;
+ ktime_t min_time;
+ ktime_t offset;
+
+ zero_time = ktime_set(0, 0);
+ if (ktime_compare(zero_time, timestamp) >= 0)
+ return false;
+
+ current_time = ktime_get();
+ offset = ktime_set(UINPUT_TIMESTAMP_ALLOWED_OFFSET_SECS, 0);
+ min_time = ktime_sub(current_time, offset);
+
+ if (ktime_after(min_time, timestamp) || ktime_after(timestamp, current_time))
+ return false;
+
+ return true;
+}
+
static ssize_t uinput_inject_events(struct uinput_device *udev,
const char __user *buffer, size_t count)
{
struct input_event ev;
size_t bytes = 0;
+ ktime_t timestamp;
if (count != 0 && count < input_event_size())
return -EINVAL;
@@ -588,6 +618,10 @@ static ssize_t uinput_inject_events(struct uinput_device *udev,
if (input_event_from_user(buffer + bytes, &ev))
return -EFAULT;
+ timestamp = ktime_set(ev.input_event_sec, ev.input_event_usec * NSEC_PER_USEC);
+ if (is_valid_timestamp(timestamp))
+ input_set_timestamp(udev->dev, timestamp);
+
input_event(udev->dev, ev.type, ev.code, ev.value);
bytes += input_event_size();
cond_resched();
--
2.40.1.495.gc816e09b53d-goog
^ permalink raw reply related
* Re: [PATCH] Fix freeze in lm8333 i2c keyboard driver
From: Jeff LaBundy @ 2023-04-27 0:27 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Tomas Mudrunka, linux-input, linux-kernel
In-Reply-To: <ZEmwsViIjUVPZ4Cd@google.com>
Hi Dmitry,
On Wed, Apr 26, 2023 at 04:16:01PM -0700, Dmitry Torokhov wrote:
> Hi Jeff, Tomas,
>
> On Tue, Apr 25, 2023 at 10:39:49AM -0500, Jeff LaBundy wrote:
> > Hi Tomas,
> >
> > On Tue, Apr 25, 2023 at 03:00:53PM +0200, Tomas Mudrunka wrote:
> > > LM8333 uses gpio interrupt line which is active-low.
> > > When interrupt is set to FALLING edge and button is pressed
> > > before driver loads, driver will miss the edge and never respond.
> > > To fix this we handle ONESHOT LOW interrupt rather than edge.
> > >
> > > Signed-off-by: Tomas Mudrunka <tomas.mudrunka@gmail.com>
> > > ---
> > > drivers/input/keyboard/lm8333.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/input/keyboard/lm8333.c b/drivers/input/keyboard/lm8333.c
> > > index 7457c3220..c5770ebb2 100644
> > > --- a/drivers/input/keyboard/lm8333.c
> > > +++ b/drivers/input/keyboard/lm8333.c
> > > @@ -179,7 +179,7 @@ static int lm8333_probe(struct i2c_client *client)
> > > }
> > >
> > > err = request_threaded_irq(client->irq, NULL, lm8333_irq_thread,
> > > - IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> > > + IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> > > "lm8333", lm8333);
> > > if (err)
> > > goto free_mem;
> >
> > Thanks for the patch, but this is a NAK in my opinion.
> >
> > First of all, we should not be hard-coding interrupt polarity in the
> > first place; that is an existing piece of technical debt in this driver.
>
> Yes, I wonder if the original hardware was limited to the edge
> interrupts.
>
> >
> > Second, changing from edge-triggered to level-triggered interrupts runs
> > the risk of creating an interrupt storm depending on the time it takes
> > the device to deassert the irq following the I2C read and the point at
> > which the threaded handler returns. Have you measured this?
>
> IRQF_ONESHOT ensures that the level interrupt is unmasked only when the
> threaded handler returns.
Yes that's correct; what I mean to say is that depending on the nature of
the read-to-clear mechanism in the part, there is a chance that the IRQ
has not been deasserted by the time the threaded handler returns. On some
devices for example, the IRQ is not deasserted until some time after the
read's stop condition.
For these cases, I consider it best practice to measure the I2C and IRQ
lines on a scope and if necessary, add a small delay before the interrupt
handler returns. This is especially true for open-drain interrupts that
may need a few hundred extra us for the pin to rise.
>
> >
> > Can we not simply read the interrupt status registers once at start-up
> > to clear any pending status? This is essentially what your change does
> > anyway, albeit indirectly.
> >
>
> Thanks.
>
> --
> Dmitry
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH v2] Fix freeze in lm8333 i2c keyboard driver
From: Jeff LaBundy @ 2023-04-27 0:41 UTC (permalink / raw)
To: Tomas Mudrunka; +Cc: dmitry.torokhov, linux-input, linux-kernel
In-Reply-To: <20230425164903.610455-1-tomas.mudrunka@gmail.com>
Hi Tomas,
On Tue, Apr 25, 2023 at 06:49:03PM +0200, Tomas Mudrunka wrote:
> LM8333 uses gpio interrupt line which is activated by falling edge.
> When button is pressed before driver is loaded,
> driver will miss the edge and never respond again.
> To fix this we clear the interrupt via i2c after registering IRQ.
>
> Signed-off-by: Tomas Mudrunka <tomas.mudrunka@gmail.com>
> ---
> drivers/input/keyboard/lm8333.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/input/keyboard/lm8333.c b/drivers/input/keyboard/lm8333.c
> index 7457c3220..9a810ca00 100644
> --- a/drivers/input/keyboard/lm8333.c
> +++ b/drivers/input/keyboard/lm8333.c
> @@ -184,6 +184,8 @@ static int lm8333_probe(struct i2c_client *client)
> if (err)
> goto free_mem;
>
> + lm8333_read8(lm8333, LM8333_READ_INT);
> +
This is the right idea. I am sort of splitting hairs here, however I
think it makes sense to place this read before the IRQ is requested
and not after.
As written, there is room for an ever-so-tiny race condition wherein
the IRQ is asserted just after it is requested. Before the threaded
handler has run however, the new read in probe swallows the IRQ status
before the threaded handler can read it and react to errors.
Also, I think you should at least capture and evaluate lm8333_read8()'s
return value as is already done for the calls to lm8333_write8().
> err = input_register_device(input);
> if (err)
> goto free_irq;
> --
> 2.40.0
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH v3 1/2] dt-bindings: input: goodix: Add "goodix,no-reset-during-suspend" property
From: Jeff LaBundy @ 2023-04-27 1:01 UTC (permalink / raw)
To: Fei Shao
Cc: Douglas Anderson, Benjamin Tissoires, Rob Herring, linux-mediatek,
Matthias Brugger, Dmitry Torokhov, Krzysztof Kozlowski,
devicetree, linux-input, linux-kernel
In-Reply-To: <20230426144423.2820826-2-fshao@chromium.org>
Hi Fei,
On Wed, Apr 26, 2023 at 10:44:21PM +0800, Fei Shao wrote:
> We observed that on Chromebook device Steelix, if Goodix GT7375P
> touchscreen is powered in suspend (because, for example, it connects to
> an always-on regulator) and with the reset GPIO asserted, it will
> introduce about 14mW power leakage.
>
> To address that, we add this property to skip reset during suspend.
> If it's set, the driver will stop asserting the reset GPIO during
> power-down. Refer to the comments in the driver for details.
>
> Signed-off-by: Fei Shao <fshao@chromium.org>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
> ---
Reviewed-by: Jeff LaBundy <jeff@labundy.com>
>
> (no changes since v2)
>
> Changes in v2:
> - Use a more accurate property name and with "goodix," prefix.
>
> .../devicetree/bindings/input/goodix,gt7375p.yaml | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
> index ce18d7dadae2..1edad1da1196 100644
> --- a/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
> +++ b/Documentation/devicetree/bindings/input/goodix,gt7375p.yaml
> @@ -43,6 +43,15 @@ properties:
> itself as long as it allows the main board to make signals compatible
> with what the touchscreen is expecting for its IO rails.
>
> + goodix,no-reset-during-suspend:
> + description:
> + Set this to true to enforce the driver to not assert the reset GPIO
> + during suspend.
> + Due to potential touchscreen hardware flaw, back-powering could happen in
> + suspend if the power supply is on and with active-low reset GPIO asserted.
> + This property is used to avoid the back-powering issue.
> + type: boolean
> +
> required:
> - compatible
> - reg
> --
> 2.40.1.495.gc816e09b53d-goog
>
Many thanks to you and Doug for the informative discussion.
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH v3 2/2] HID: i2c-hid: goodix: Add support for "goodix,no-reset-during-suspend" property
From: Jeff LaBundy @ 2023-04-27 1:07 UTC (permalink / raw)
To: Fei Shao
Cc: Douglas Anderson, Benjamin Tissoires, Rob Herring, linux-mediatek,
Dmitry Torokhov, Jiri Kosina, Matthias Kaehlcke, Stephen Kitt,
linux-input, linux-kernel
In-Reply-To: <20230426144423.2820826-3-fshao@chromium.org>
Hi Fei,
On Wed, Apr 26, 2023 at 10:44:22PM +0800, Fei Shao wrote:
> In the beginning, commit 18eeef46d359 ("HID: i2c-hid: goodix: Tie the
> reset line to true state of the regulator") introduced a change to tie
> the reset line of the Goodix touchscreen to the state of the regulator
> to fix a power leakage issue in suspend.
>
> After some time, the change was deemed unnecessary and was reverted in
> commit 557e05fa9fdd ("HID: i2c-hid: goodix: Stop tying the reset line to
> the regulator") due to difficulties in managing regulator notifiers for
> designs like Evoker, which provides a second power rail to touchscreen.
>
> However, the revert caused a power regression on another Chromebook
> device Steelix in the field, which has a dedicated always-on regulator
> for touchscreen and was covered by the workaround in the first commit.
>
> To address both cases, this patch adds the support for the new
> "goodix,no-reset-during-suspend" property in the driver:
> - When set to true, the driver does not assert the reset GPIO during
> power-down.
> Instead, the GPIO will be asserted during power-up to ensure the
> touchscreen always has a clean start and consistent behavior after
> resuming.
> This is for designs with a dedicated always-on regulator.
> - When set to false or unset, the driver uses the original control flow
> and asserts GPIO and disables regulators normally.
> This is for the two-regulator and shared-regulator designs.
>
> Signed-off-by: Fei Shao <fshao@chromium.org>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
Great work; one tiny nit below. If you do not agree with it or have found
precedent that suggests it is OK, feel free to ignore it. Either way:
Reviewed-by: Jeff LaBundy <jeff@labundy.com>
>
> ---
>
> Changes in v3:
> - In power-down, only skip the GPIO but not the regulator calls if the
> flag is set
>
> Changes in v2:
> - Do not change the regulator_enable logic during power-up.
>
> drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> index 0060e3dcd775..3ed365b50432 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> @@ -28,6 +28,7 @@ struct i2c_hid_of_goodix {
> struct regulator *vdd;
> struct regulator *vddio;
> struct gpio_desc *reset_gpio;
> + bool no_reset_during_suspend;
> const struct goodix_i2c_hid_timing_data *timings;
> };
>
> @@ -37,6 +38,14 @@ static int goodix_i2c_hid_power_up(struct i2chid_ops *ops)
> container_of(ops, struct i2c_hid_of_goodix, ops);
> int ret;
>
> + if (ihid_goodix->no_reset_during_suspend) {
> + /*
> + * We assert reset GPIO here (instead of during power-down) to
> + * ensure the device will have a clean state after powering up,
> + * just like the normal scenarios will have.
> + */
> + gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
> + }
I don't think curly braces are technically required here, as there is only
one statement within the conditional despite the comments making it appear
otherwise. Maybe this would work too:
/* ... */
if (...)
gpiod_set_value_cansleep(...);
Again however, I do not feel strongly about this.
> ret = regulator_enable(ihid_goodix->vdd);
> if (ret)
> return ret;
> @@ -60,7 +69,9 @@ static void goodix_i2c_hid_power_down(struct i2chid_ops *ops)
> struct i2c_hid_of_goodix *ihid_goodix =
> container_of(ops, struct i2c_hid_of_goodix, ops);
>
> - gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
> + if (!ihid_goodix->no_reset_during_suspend)
> + gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
> +
> regulator_disable(ihid_goodix->vddio);
> regulator_disable(ihid_goodix->vdd);
> }
> @@ -91,6 +102,9 @@ static int i2c_hid_of_goodix_probe(struct i2c_client *client)
> if (IS_ERR(ihid_goodix->vddio))
> return PTR_ERR(ihid_goodix->vddio);
>
> + ihid_goodix->no_reset_during_suspend =
> + of_property_read_bool(client->dev.of_node, "goodix,no-reset-during-suspend");
> +
> ihid_goodix->timings = device_get_match_data(&client->dev);
>
> return i2c_hid_core_probe(client, &ihid_goodix->ops, 0x0001, 0);
> --
> 2.40.1.495.gc816e09b53d-goog
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox