* Re: [PATCH v3 2/2] arm64: dts: qcom: sc8280xp-x13s: Fix/enable touchscreen
From: Johan Hovold @ 2024-01-29 16:59 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Benjamin Tissoires, Jiri Kosina, Bjorn Andersson, Konrad Dybcio,
Johan Hovold, linux-arm-msm, linux-input, devicetree,
linux-kernel, Konrad Dybcio, Krzysztof Kozlowski, Daniel Thompson
In-Reply-To: <20240129-x13s-touchscreen-v3-2-c4a933034145@quicinc.com>
On Mon, Jan 29, 2024 at 08:47:48AM -0800, Bjorn Andersson wrote:
> The touchscreen present on some SKUs of Lenovo Thinkpad X13s is never
> detected by Linux. Power is applied and the device is brought out of
> reset using the pinconfig in DeviceTree, but the read-test in
> __i2c_hid_core_probe() fails to access the device, which result in probe
> being aborted.
>
> Some users have reported success after rebinding the device.
>
> Looking to the ACPI tables, there's a 5ms after-power and a 200ms
> after-reset delay. The power-supply is shared with other components, so
> this is active all the way through boot. The reset GPIO, on the other
> hand, is low (reset asserted) at boot, so this is first deasserted by
> the implicit application of the pinconf state.
>
> This means the time between reset deassert and __i2c_hid_core_probe() is
> significantly below the value documented in the ACPI tables.
>
> As the I2C HID binding and driver support specifying a reset gpio,
> replace the pinconf-based scheme to pull the device out of reset. Then
> specify the after-reset time.
>
> The shared power rail is currently always on, but in case this ever
> change, the after-power delay is added as well, to not violate the
> power-on to reset-deassert timing requirement.
>
> Fixes: 32c231385ed4 ("arm64: dts: qcom: sc8280xp: add Lenovo Thinkpad X13s devicetree")
> Tested-by: Daniel Thompson <daniel.thompson@linaro.org>
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
Thanks for the update.
Reviewed-by: Johan Hovold <johan+linaro@kernel.org>
^ permalink raw reply
* Re: [PATCH v3 2/2] arm64: dts: qcom: sc8280xp-x13s: Fix/enable touchscreen
From: Daniel Thompson @ 2024-01-29 16:57 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Benjamin Tissoires, Jiri Kosina, Bjorn Andersson, Konrad Dybcio,
Johan Hovold, linux-arm-msm, linux-input, devicetree,
linux-kernel, Konrad Dybcio, Krzysztof Kozlowski
In-Reply-To: <20240129-x13s-touchscreen-v3-2-c4a933034145@quicinc.com>
On Mon, Jan 29, 2024 at 08:47:48AM -0800, Bjorn Andersson wrote:
> The touchscreen present on some SKUs of Lenovo Thinkpad X13s is never
> detected by Linux. Power is applied and the device is brought out of
> reset using the pinconfig in DeviceTree, but the read-test in
> __i2c_hid_core_probe() fails to access the device, which result in probe
> being aborted.
>
> Some users have reported success after rebinding the device.
>
> Looking to the ACPI tables, there's a 5ms after-power and a 200ms
> after-reset delay. The power-supply is shared with other components, so
> this is active all the way through boot. The reset GPIO, on the other
> hand, is low (reset asserted) at boot, so this is first deasserted by
> the implicit application of the pinconf state.
>
> This means the time between reset deassert and __i2c_hid_core_probe() is
> significantly below the value documented in the ACPI tables.
>
> As the I2C HID binding and driver support specifying a reset gpio,
> replace the pinconf-based scheme to pull the device out of reset. Then
> specify the after-reset time.
>
> The shared power rail is currently always on, but in case this ever
> change, the after-power delay is added as well, to not violate the
> power-on to reset-deassert timing requirement.
>
> Fixes: 32c231385ed4 ("arm64: dts: qcom: sc8280xp: add Lenovo Thinkpad X13s devicetree")
> Tested-by: Daniel Thompson <daniel.thompson@linaro.org>
For the sake of completeness (and to make sure I don't forget if anyone
asks me in the future) I also (successfully) tested with the output
high removed from the touchscreen pinctrl.
Daniel.
PS Strictly speaking I tested v2 and am replying to v3... but the DT
should be the same for both versions ;-).
^ permalink raw reply
* Re: [PATCH v3 1/2] dt-bindings: HID: i2c-hid: Document reset-related properties
From: Johan Hovold @ 2024-01-29 16:56 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Benjamin Tissoires, Jiri Kosina, Bjorn Andersson, Konrad Dybcio,
Johan Hovold, linux-arm-msm, linux-input, devicetree,
linux-kernel, Konrad Dybcio, Krzysztof Kozlowski
In-Reply-To: <20240129-x13s-touchscreen-v3-1-c4a933034145@quicinc.com>
On Mon, Jan 29, 2024 at 08:47:47AM -0800, Bjorn Andersson wrote:
> Some I2C HID devices has a reset pin and requires that some specified
> time elapses after this reset pin is deasserted, before communication
> with the device is attempted.
>
> The Linux implementation is looking for these in the "reset-gpios" and
> "post-reset-deassert-delay-ms" properties already, so use these property
> names.
> + post-reset-deassert-delay-ms:
> + description: Time required by the device after reset has been deasserted,
> + before it is ready for communication.
> +
> + reset-gpios: true
Hmm, for the third time, it seems you ignored my comment that you need
to remove the comment about these properties from the driver as part of
this series.
/*
* Note this is a kernel internal device-property set by x86 platform code,
* this MUST not be used in devicetree files without first adding it to
* the DT bindings.
*/
if (!device_property_read_u32(dev, "post-reset-deassert-delay-ms", &val))
ihid_of->post_reset_delay_ms = val;
> +
> touchscreen-inverted-x: true
>
> touchscreen-inverted-y: true
Johan
^ permalink raw reply
* Re: [PATCH] HID: logitech-hidpp: Do not flood kernel log
From: Hans de Goede @ 2024-01-29 16:54 UTC (permalink / raw)
To: Oleksandr Natalenko, linux-kernel
Cc: linux-input, Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires
In-Reply-To: <20240129164931.38477-1-oleksandr@natalenko.name>
Hi,
On 1/29/24 17:49, Oleksandr Natalenko wrote:
> Since commit 680ee411a98e ("HID: logitech-hidpp: Fix connect event race")
> the following messages appear in the kernel log from time to time:
>
> logitech-hidpp-device 0003:046D:408A.0005: HID++ 4.5 device connected.
> logitech-hidpp-device 0003:046D:408A.0005: HID++ 4.5 device connected.
> logitech-hidpp-device 0003:046D:4051.0006: Disconnected
> logitech-hidpp-device 0003:046D:408A.0005: Disconnected
>
> As discussed, print the first per-device "device connected" message
> at info level, demoting subsequent messages to debug level. Also,
> demote the "Disconnected message" to debug level unconditionally.
>
> Link: https://lore.kernel.org/lkml/3277085.44csPzL39Z@natalenko.name/
> Signed-off-by: Oleksandr Natalenko <oleksandr@natalenko.name>
Thanks, patch looks good to me:
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Regards,
Hans
> ---
> drivers/hid/hid-logitech-hidpp.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 6ef0c88e3e60a..d2f3f234f29de 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -203,6 +203,8 @@ struct hidpp_device {
> struct hidpp_scroll_counter vertical_wheel_counter;
>
> u8 wireless_feature_index;
> +
> + bool connected_once;
> };
>
> /* HID++ 1.0 error codes */
> @@ -988,8 +990,13 @@ static int hidpp_root_get_protocol_version(struct hidpp_device *hidpp)
> hidpp->protocol_minor = response.rap.params[1];
>
> print_version:
> - hid_info(hidpp->hid_dev, "HID++ %u.%u device connected.\n",
> - hidpp->protocol_major, hidpp->protocol_minor);
> + if (!hidpp->connected_once) {
> + hid_info(hidpp->hid_dev, "HID++ %u.%u device connected.\n",
> + hidpp->protocol_major, hidpp->protocol_minor);
> + hidpp->connected_once = true;
> + } else
> + hid_dbg(hidpp->hid_dev, "HID++ %u.%u device connected.\n",
> + hidpp->protocol_major, hidpp->protocol_minor);
> return 0;
> }
>
> @@ -4184,7 +4191,7 @@ static void hidpp_connect_event(struct work_struct *work)
> /* Get device version to check if it is connected */
> ret = hidpp_root_get_protocol_version(hidpp);
> if (ret) {
> - hid_info(hidpp->hid_dev, "Disconnected\n");
> + hid_dbg(hidpp->hid_dev, "Disconnected\n");
> if (hidpp->battery.ps) {
> hidpp->battery.online = false;
> hidpp->battery.status = POWER_SUPPLY_STATUS_UNKNOWN;
^ permalink raw reply
* [PATCH] HID: logitech-hidpp: Do not flood kernel log
From: Oleksandr Natalenko @ 2024-01-29 16:49 UTC (permalink / raw)
To: linux-kernel
Cc: linux-input, Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires, Hans de Goede
Since commit 680ee411a98e ("HID: logitech-hidpp: Fix connect event race")
the following messages appear in the kernel log from time to time:
logitech-hidpp-device 0003:046D:408A.0005: HID++ 4.5 device connected.
logitech-hidpp-device 0003:046D:408A.0005: HID++ 4.5 device connected.
logitech-hidpp-device 0003:046D:4051.0006: Disconnected
logitech-hidpp-device 0003:046D:408A.0005: Disconnected
As discussed, print the first per-device "device connected" message
at info level, demoting subsequent messages to debug level. Also,
demote the "Disconnected message" to debug level unconditionally.
Link: https://lore.kernel.org/lkml/3277085.44csPzL39Z@natalenko.name/
Signed-off-by: Oleksandr Natalenko <oleksandr@natalenko.name>
---
drivers/hid/hid-logitech-hidpp.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 6ef0c88e3e60a..d2f3f234f29de 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -203,6 +203,8 @@ struct hidpp_device {
struct hidpp_scroll_counter vertical_wheel_counter;
u8 wireless_feature_index;
+
+ bool connected_once;
};
/* HID++ 1.0 error codes */
@@ -988,8 +990,13 @@ static int hidpp_root_get_protocol_version(struct hidpp_device *hidpp)
hidpp->protocol_minor = response.rap.params[1];
print_version:
- hid_info(hidpp->hid_dev, "HID++ %u.%u device connected.\n",
- hidpp->protocol_major, hidpp->protocol_minor);
+ if (!hidpp->connected_once) {
+ hid_info(hidpp->hid_dev, "HID++ %u.%u device connected.\n",
+ hidpp->protocol_major, hidpp->protocol_minor);
+ hidpp->connected_once = true;
+ } else
+ hid_dbg(hidpp->hid_dev, "HID++ %u.%u device connected.\n",
+ hidpp->protocol_major, hidpp->protocol_minor);
return 0;
}
@@ -4184,7 +4191,7 @@ static void hidpp_connect_event(struct work_struct *work)
/* Get device version to check if it is connected */
ret = hidpp_root_get_protocol_version(hidpp);
if (ret) {
- hid_info(hidpp->hid_dev, "Disconnected\n");
+ hid_dbg(hidpp->hid_dev, "Disconnected\n");
if (hidpp->battery.ps) {
hidpp->battery.online = false;
hidpp->battery.status = POWER_SUPPLY_STATUS_UNKNOWN;
--
2.43.0
^ permalink raw reply related
* [PATCH v3 0/2] arm64: dts: qcom: sc8280xp-x13s: Enable touchscreen
From: Bjorn Andersson @ 2024-01-29 16:47 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Benjamin Tissoires, Jiri Kosina, Bjorn Andersson, Konrad Dybcio,
Johan Hovold
Cc: linux-arm-msm, linux-input, devicetree, linux-kernel,
Konrad Dybcio, Krzysztof Kozlowski, Bjorn Andersson,
Daniel Thompson
This documents and defines the necessary properties for the I2C
HID-based touchscreen found in some SKUs of the Lenovo Thinkpad X13s to
work.
Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
Changes in v3:
- Rewrote the commit message, to properly describe the problem being
resolved.
- Link to v2: https://lore.kernel.org/r/20240126-x13s-touchscreen-v2-0-5374ccc9e10d@quicinc.com
Changes in v2:
- Dropped output-high from &ts0_default, to avoid bouncing the reset
line unnecessarily
- Link to v1: https://lore.kernel.org/r/20240125-x13s-touchscreen-v1-0-ab8c882def9c@quicinc.com
---
Bjorn Andersson (2):
dt-bindings: HID: i2c-hid: Document reset-related properties
arm64: dts: qcom: sc8280xp-x13s: Fix/enable touchscreen
Documentation/devicetree/bindings/input/hid-over-i2c.yaml | 6 ++++++
arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 7 +++++--
2 files changed, 11 insertions(+), 2 deletions(-)
---
base-commit: 8bf1262c53f50fa91fe15d01e5ef5629db55313c
change-id: 20240125-x13s-touchscreen-48012ff3c24e
Best regards,
--
Bjorn Andersson <quic_bjorande@quicinc.com>
^ permalink raw reply
* [PATCH v3 2/2] arm64: dts: qcom: sc8280xp-x13s: Fix/enable touchscreen
From: Bjorn Andersson @ 2024-01-29 16:47 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Benjamin Tissoires, Jiri Kosina, Bjorn Andersson, Konrad Dybcio,
Johan Hovold
Cc: linux-arm-msm, linux-input, devicetree, linux-kernel,
Konrad Dybcio, Krzysztof Kozlowski, Bjorn Andersson,
Daniel Thompson
In-Reply-To: <20240129-x13s-touchscreen-v3-0-c4a933034145@quicinc.com>
The touchscreen present on some SKUs of Lenovo Thinkpad X13s is never
detected by Linux. Power is applied and the device is brought out of
reset using the pinconfig in DeviceTree, but the read-test in
__i2c_hid_core_probe() fails to access the device, which result in probe
being aborted.
Some users have reported success after rebinding the device.
Looking to the ACPI tables, there's a 5ms after-power and a 200ms
after-reset delay. The power-supply is shared with other components, so
this is active all the way through boot. The reset GPIO, on the other
hand, is low (reset asserted) at boot, so this is first deasserted by
the implicit application of the pinconf state.
This means the time between reset deassert and __i2c_hid_core_probe() is
significantly below the value documented in the ACPI tables.
As the I2C HID binding and driver support specifying a reset gpio,
replace the pinconf-based scheme to pull the device out of reset. Then
specify the after-reset time.
The shared power rail is currently always on, but in case this ever
change, the after-power delay is added as well, to not violate the
power-on to reset-deassert timing requirement.
Fixes: 32c231385ed4 ("arm64: dts: qcom: sc8280xp: add Lenovo Thinkpad X13s devicetree")
Tested-by: Daniel Thompson <daniel.thompson@linaro.org>
Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
index def3976bd5bb..33731b95ad51 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
+++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
@@ -620,7 +620,6 @@ &i2c4 {
status = "okay";
- /* FIXME: verify */
touchscreen@10 {
compatible = "hid-over-i2c";
reg = <0x10>;
@@ -630,6 +629,11 @@ touchscreen@10 {
vdd-supply = <&vreg_misc_3p3>;
vddl-supply = <&vreg_s10b>;
+ reset-gpios = <&tlmm 99 GPIO_ACTIVE_LOW>;
+
+ post-power-on-delay-ms = <5>;
+ post-reset-deassert-delay-ms = <200>;
+
pinctrl-names = "default";
pinctrl-0 = <&ts0_default>;
};
@@ -1450,7 +1454,6 @@ int-n-pins {
reset-n-pins {
pins = "gpio99";
function = "gpio";
- output-high;
drive-strength = <16>;
};
};
--
2.25.1
^ permalink raw reply related
* [PATCH v3 1/2] dt-bindings: HID: i2c-hid: Document reset-related properties
From: Bjorn Andersson @ 2024-01-29 16:47 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Benjamin Tissoires, Jiri Kosina, Bjorn Andersson, Konrad Dybcio,
Johan Hovold
Cc: linux-arm-msm, linux-input, devicetree, linux-kernel,
Konrad Dybcio, Krzysztof Kozlowski, Bjorn Andersson
In-Reply-To: <20240129-x13s-touchscreen-v3-0-c4a933034145@quicinc.com>
Some I2C HID devices has a reset pin and requires that some specified
time elapses after this reset pin is deasserted, before communication
with the device is attempted.
The Linux implementation is looking for these in the "reset-gpios" and
"post-reset-deassert-delay-ms" properties already, so use these property
names.
Reviewed-by: Johan Hovold <johan+linaro@kernel.org>
Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
Documentation/devicetree/bindings/input/hid-over-i2c.yaml | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/hid-over-i2c.yaml b/Documentation/devicetree/bindings/input/hid-over-i2c.yaml
index 138caad96a29..f07ff4cb3d26 100644
--- a/Documentation/devicetree/bindings/input/hid-over-i2c.yaml
+++ b/Documentation/devicetree/bindings/input/hid-over-i2c.yaml
@@ -50,6 +50,12 @@ properties:
description: Time required by the device after enabling its regulators
or powering it on, before it is ready for communication.
+ post-reset-deassert-delay-ms:
+ description: Time required by the device after reset has been deasserted,
+ before it is ready for communication.
+
+ reset-gpios: true
+
touchscreen-inverted-x: true
touchscreen-inverted-y: true
--
2.25.1
^ permalink raw reply related
* Re: Flood of logitech-hidpp-device messages in v6.7
From: Hans de Goede @ 2024-01-29 16:31 UTC (permalink / raw)
To: Oleksandr Natalenko, linux-kernel
Cc: linux-input, Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires
In-Reply-To: <4894984.31r3eYUQgx@natalenko.name>
Hi Oleksandr,
On 1/29/24 17:19, Oleksandr Natalenko wrote:
> On pondělí 29. ledna 2024 17:08:56 CET Hans de Goede wrote:
>> Hi,
>>
>> On 1/29/24 16:58, Oleksandr Natalenko wrote:
>>> Hello.
>>>
>>> On úterý 9. ledna 2024 12:58:10 CET Hans de Goede wrote:
>>>> Hi Oleksandr,
>>>>
>>>> On 1/9/24 12:45, Oleksandr Natalenko wrote:
>>>>> Hello Hans et al.
>>>>>
>>>>> Starting from v6.7 release I get the following messages repeating in `dmesg` regularly:
>>>>>
>>>>> ```
>>>>> Jan 09 10:05:06 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: Disconnected
>>>>> Jan 09 10:07:15 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: Disconnected
>>>>> Jan 09 10:16:51 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: HID++ 4.5 device connected.
>>>>> Jan 09 10:16:55 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: HID++ 4.5 device connected.
>>>>> Jan 09 10:16:55 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: HID++ 4.5 device connected.
>>>>> Jan 09 10:36:31 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: Disconnected
>>>>> Jan 09 10:37:07 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: HID++ 4.5 device connected.
>>>>> Jan 09 10:46:21 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: Disconnected
>>>>> Jan 09 10:48:23 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: Disconnected
>>>>> Jan 09 11:12:27 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: HID++ 4.5 device connected.
>>>>> Jan 09 11:12:47 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: HID++ 4.5 device connected.
>>>>> Jan 09 11:12:47 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: HID++ 4.5 device connected.
>>>>> Jan 09 11:38:32 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: Disconnected
>>>>> Jan 09 11:43:32 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: Disconnected
>>>>> Jan 09 11:45:10 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: HID++ 4.5 device connected.
>>>>> Jan 09 11:45:11 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: HID++ 4.5 device connected.
>>>>> Jan 09 11:45:11 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: HID++ 4.5 device connected.
>>>>> Jan 09 12:31:48 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: Disconnected
>>>>> Jan 09 12:33:21 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: HID++ 4.5 device connected.
>>>>> ```
>>>>>
>>>>> I've got the following hardware:
>>>>>
>>>>> * Bus 006 Device 004: ID 046d:c52b Logitech, Inc. Unifying Receiver
>>>>> * Logitech MX Keys
>>>>> * Logitech M510v2
>>>>>
>>>>> With v6.6 I do not get those messages.
>>>>>
>>>>> I think this is related to 680ee411a98e ("HID: logitech-hidpp: Fix connect event race").
>>>>>
>>>>> My speculation is that some of the devices enter powersaving state after being idle for some time (5 mins?), and then wake up and reconnect once I touch either keyboard or mouse. I should highlight that everything works just fine, it is the flood of messages that worries me.
>>>>>
>>>>> Is it expected?
>>>>
>>>> Yes this is expected, looking at your logs I see about 10 messages per
>>>> hour which IMHO is not that bad.
>>>>
>>>> I guess we could change things to track we have logged the connect
>>>> message once and if yes then log future connect messages (and all
>>>> disconnect messages) at debug level.
>>>
>>> How granular such a tracking should be? Per-`struct hidpp_device`?
>>
>> Yes per struct hidpp_device we want to log the connect message once
>> per device since it gives info which might be useful for troubleshooting.
>>
>>> Should there be something like `hid_info_once_then_dbg()` macro, or open-code it in each place instead?
>>
>> Since we want something like e.g. a "first_connect" (initialized
>> to true if you use that name) flag per struct hidpp_device this needs
>> to be open coded.
>
> OK, would something like this make sense (not tested)?
Yes, thank you. I would call once_connected connected_once and
you can drop the disconnected flag and just always log
disconnect messages at the dbg level.
Regards,
Hans
>
> ```
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 6ef0c88e3e60a..a9899709d6b74 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -203,6 +203,9 @@ struct hidpp_device {
> struct hidpp_scroll_counter vertical_wheel_counter;
>
> u8 wireless_feature_index;
> +
> + bool once_connected;
> + bool once_disconnected;
> };
>
> /* HID++ 1.0 error codes */
> @@ -988,8 +991,13 @@ static int hidpp_root_get_protocol_version(struct hidpp_device *hidpp)
> hidpp->protocol_minor = response.rap.params[1];
>
> print_version:
> - hid_info(hidpp->hid_dev, "HID++ %u.%u device connected.\n",
> - hidpp->protocol_major, hidpp->protocol_minor);
> + if (!hidpp->once_connected) {
> + hid_info(hidpp->hid_dev, "HID++ %u.%u device connected.\n",
> + hidpp->protocol_major, hidpp->protocol_minor);
> + hidpp->once_connected = true;
> + } else
> + hid_dbg(hidpp->hid_dev, "HID++ %u.%u device connected.\n",
> + hidpp->protocol_major, hidpp->protocol_minor);
> return 0;
> }
>
> @@ -4184,7 +4192,11 @@ static void hidpp_connect_event(struct work_struct *work)
> /* Get device version to check if it is connected */
> ret = hidpp_root_get_protocol_version(hidpp);
> if (ret) {
> - hid_info(hidpp->hid_dev, "Disconnected\n");
> + if (!hidpp->once_disconnected) {
> + hid_info(hidpp->hid_dev, "Disconnected\n");
> + hidpp->once_disconnected = true;
> + } else
> + hid_dbg(hidpp->hid_dev, "Disconnected\n");
> if (hidpp->battery.ps) {
> hidpp->battery.online = false;
> hidpp->battery.status = POWER_SUPPLY_STATUS_UNKNOWN;
> ```
>
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>
>>
>
>
^ permalink raw reply
* Re: Flood of logitech-hidpp-device messages in v6.7
From: Oleksandr Natalenko @ 2024-01-29 16:19 UTC (permalink / raw)
To: linux-kernel, Hans de Goede
Cc: linux-input, Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires
In-Reply-To: <489d6c71-73eb-4605-8293-5cfea385cf08@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 5603 bytes --]
On pondělí 29. ledna 2024 17:08:56 CET Hans de Goede wrote:
> Hi,
>
> On 1/29/24 16:58, Oleksandr Natalenko wrote:
> > Hello.
> >
> > On úterý 9. ledna 2024 12:58:10 CET Hans de Goede wrote:
> >> Hi Oleksandr,
> >>
> >> On 1/9/24 12:45, Oleksandr Natalenko wrote:
> >>> Hello Hans et al.
> >>>
> >>> Starting from v6.7 release I get the following messages repeating in `dmesg` regularly:
> >>>
> >>> ```
> >>> Jan 09 10:05:06 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: Disconnected
> >>> Jan 09 10:07:15 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: Disconnected
> >>> Jan 09 10:16:51 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: HID++ 4.5 device connected.
> >>> Jan 09 10:16:55 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: HID++ 4.5 device connected.
> >>> Jan 09 10:16:55 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: HID++ 4.5 device connected.
> >>> Jan 09 10:36:31 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: Disconnected
> >>> Jan 09 10:37:07 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: HID++ 4.5 device connected.
> >>> Jan 09 10:46:21 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: Disconnected
> >>> Jan 09 10:48:23 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: Disconnected
> >>> Jan 09 11:12:27 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: HID++ 4.5 device connected.
> >>> Jan 09 11:12:47 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: HID++ 4.5 device connected.
> >>> Jan 09 11:12:47 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: HID++ 4.5 device connected.
> >>> Jan 09 11:38:32 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: Disconnected
> >>> Jan 09 11:43:32 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: Disconnected
> >>> Jan 09 11:45:10 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: HID++ 4.5 device connected.
> >>> Jan 09 11:45:11 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: HID++ 4.5 device connected.
> >>> Jan 09 11:45:11 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: HID++ 4.5 device connected.
> >>> Jan 09 12:31:48 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: Disconnected
> >>> Jan 09 12:33:21 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: HID++ 4.5 device connected.
> >>> ```
> >>>
> >>> I've got the following hardware:
> >>>
> >>> * Bus 006 Device 004: ID 046d:c52b Logitech, Inc. Unifying Receiver
> >>> * Logitech MX Keys
> >>> * Logitech M510v2
> >>>
> >>> With v6.6 I do not get those messages.
> >>>
> >>> I think this is related to 680ee411a98e ("HID: logitech-hidpp: Fix connect event race").
> >>>
> >>> My speculation is that some of the devices enter powersaving state after being idle for some time (5 mins?), and then wake up and reconnect once I touch either keyboard or mouse. I should highlight that everything works just fine, it is the flood of messages that worries me.
> >>>
> >>> Is it expected?
> >>
> >> Yes this is expected, looking at your logs I see about 10 messages per
> >> hour which IMHO is not that bad.
> >>
> >> I guess we could change things to track we have logged the connect
> >> message once and if yes then log future connect messages (and all
> >> disconnect messages) at debug level.
> >
> > How granular such a tracking should be? Per-`struct hidpp_device`?
>
> Yes per struct hidpp_device we want to log the connect message once
> per device since it gives info which might be useful for troubleshooting.
>
> > Should there be something like `hid_info_once_then_dbg()` macro, or open-code it in each place instead?
>
> Since we want something like e.g. a "first_connect" (initialized
> to true if you use that name) flag per struct hidpp_device this needs
> to be open coded.
OK, would something like this make sense (not tested)?
```
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 6ef0c88e3e60a..a9899709d6b74 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -203,6 +203,9 @@ struct hidpp_device {
struct hidpp_scroll_counter vertical_wheel_counter;
u8 wireless_feature_index;
+
+ bool once_connected;
+ bool once_disconnected;
};
/* HID++ 1.0 error codes */
@@ -988,8 +991,13 @@ static int hidpp_root_get_protocol_version(struct hidpp_device *hidpp)
hidpp->protocol_minor = response.rap.params[1];
print_version:
- hid_info(hidpp->hid_dev, "HID++ %u.%u device connected.\n",
- hidpp->protocol_major, hidpp->protocol_minor);
+ if (!hidpp->once_connected) {
+ hid_info(hidpp->hid_dev, "HID++ %u.%u device connected.\n",
+ hidpp->protocol_major, hidpp->protocol_minor);
+ hidpp->once_connected = true;
+ } else
+ hid_dbg(hidpp->hid_dev, "HID++ %u.%u device connected.\n",
+ hidpp->protocol_major, hidpp->protocol_minor);
return 0;
}
@@ -4184,7 +4192,11 @@ static void hidpp_connect_event(struct work_struct *work)
/* Get device version to check if it is connected */
ret = hidpp_root_get_protocol_version(hidpp);
if (ret) {
- hid_info(hidpp->hid_dev, "Disconnected\n");
+ if (!hidpp->once_disconnected) {
+ hid_info(hidpp->hid_dev, "Disconnected\n");
+ hidpp->once_disconnected = true;
+ } else
+ hid_dbg(hidpp->hid_dev, "Disconnected\n");
if (hidpp->battery.ps) {
hidpp->battery.online = false;
hidpp->battery.status = POWER_SUPPLY_STATUS_UNKNOWN;
```
>
> Regards,
>
> Hans
>
>
>
>
>
--
Oleksandr Natalenko (post-factum)
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: Flood of logitech-hidpp-device messages in v6.7
From: Hans de Goede @ 2024-01-29 16:08 UTC (permalink / raw)
To: Oleksandr Natalenko, linux-kernel
Cc: linux-input, Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires
In-Reply-To: <12371430.O9o76ZdvQC@natalenko.name>
Hi,
On 1/29/24 16:58, Oleksandr Natalenko wrote:
> Hello.
>
> On úterý 9. ledna 2024 12:58:10 CET Hans de Goede wrote:
>> Hi Oleksandr,
>>
>> On 1/9/24 12:45, Oleksandr Natalenko wrote:
>>> Hello Hans et al.
>>>
>>> Starting from v6.7 release I get the following messages repeating in `dmesg` regularly:
>>>
>>> ```
>>> Jan 09 10:05:06 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: Disconnected
>>> Jan 09 10:07:15 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: Disconnected
>>> Jan 09 10:16:51 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: HID++ 4.5 device connected.
>>> Jan 09 10:16:55 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: HID++ 4.5 device connected.
>>> Jan 09 10:16:55 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: HID++ 4.5 device connected.
>>> Jan 09 10:36:31 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: Disconnected
>>> Jan 09 10:37:07 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: HID++ 4.5 device connected.
>>> Jan 09 10:46:21 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: Disconnected
>>> Jan 09 10:48:23 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: Disconnected
>>> Jan 09 11:12:27 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: HID++ 4.5 device connected.
>>> Jan 09 11:12:47 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: HID++ 4.5 device connected.
>>> Jan 09 11:12:47 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: HID++ 4.5 device connected.
>>> Jan 09 11:38:32 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: Disconnected
>>> Jan 09 11:43:32 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: Disconnected
>>> Jan 09 11:45:10 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: HID++ 4.5 device connected.
>>> Jan 09 11:45:11 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: HID++ 4.5 device connected.
>>> Jan 09 11:45:11 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: HID++ 4.5 device connected.
>>> Jan 09 12:31:48 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: Disconnected
>>> Jan 09 12:33:21 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: HID++ 4.5 device connected.
>>> ```
>>>
>>> I've got the following hardware:
>>>
>>> * Bus 006 Device 004: ID 046d:c52b Logitech, Inc. Unifying Receiver
>>> * Logitech MX Keys
>>> * Logitech M510v2
>>>
>>> With v6.6 I do not get those messages.
>>>
>>> I think this is related to 680ee411a98e ("HID: logitech-hidpp: Fix connect event race").
>>>
>>> My speculation is that some of the devices enter powersaving state after being idle for some time (5 mins?), and then wake up and reconnect once I touch either keyboard or mouse. I should highlight that everything works just fine, it is the flood of messages that worries me.
>>>
>>> Is it expected?
>>
>> Yes this is expected, looking at your logs I see about 10 messages per
>> hour which IMHO is not that bad.
>>
>> I guess we could change things to track we have logged the connect
>> message once and if yes then log future connect messages (and all
>> disconnect messages) at debug level.
>
> How granular such a tracking should be? Per-`struct hidpp_device`?
Yes per struct hidpp_device we want to log the connect message once
per device since it gives info which might be useful for troubleshooting.
> Should there be something like `hid_info_once_then_dbg()` macro, or open-code it in each place instead?
Since we want something like e.g. a "first_connect" (initialized
to true if you use that name) flag per struct hidpp_device this needs
to be open coded.
Regards,
Hans
^ permalink raw reply
* Re: Flood of logitech-hidpp-device messages in v6.7
From: Oleksandr Natalenko @ 2024-01-29 15:58 UTC (permalink / raw)
To: linux-kernel, Hans de Goede
Cc: linux-input, Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires
In-Reply-To: <824573bb-ae01-41b9-8f97-a760ae8f3f18@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 3436 bytes --]
Hello.
On úterý 9. ledna 2024 12:58:10 CET Hans de Goede wrote:
> Hi Oleksandr,
>
> On 1/9/24 12:45, Oleksandr Natalenko wrote:
> > Hello Hans et al.
> >
> > Starting from v6.7 release I get the following messages repeating in `dmesg` regularly:
> >
> > ```
> > Jan 09 10:05:06 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: Disconnected
> > Jan 09 10:07:15 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: Disconnected
> > Jan 09 10:16:51 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: HID++ 4.5 device connected.
> > Jan 09 10:16:55 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: HID++ 4.5 device connected.
> > Jan 09 10:16:55 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: HID++ 4.5 device connected.
> > Jan 09 10:36:31 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: Disconnected
> > Jan 09 10:37:07 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: HID++ 4.5 device connected.
> > Jan 09 10:46:21 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: Disconnected
> > Jan 09 10:48:23 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: Disconnected
> > Jan 09 11:12:27 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: HID++ 4.5 device connected.
> > Jan 09 11:12:47 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: HID++ 4.5 device connected.
> > Jan 09 11:12:47 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: HID++ 4.5 device connected.
> > Jan 09 11:38:32 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: Disconnected
> > Jan 09 11:43:32 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: Disconnected
> > Jan 09 11:45:10 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: HID++ 4.5 device connected.
> > Jan 09 11:45:11 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: HID++ 4.5 device connected.
> > Jan 09 11:45:11 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: HID++ 4.5 device connected.
> > Jan 09 12:31:48 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: Disconnected
> > Jan 09 12:33:21 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: HID++ 4.5 device connected.
> > ```
> >
> > I've got the following hardware:
> >
> > * Bus 006 Device 004: ID 046d:c52b Logitech, Inc. Unifying Receiver
> > * Logitech MX Keys
> > * Logitech M510v2
> >
> > With v6.6 I do not get those messages.
> >
> > I think this is related to 680ee411a98e ("HID: logitech-hidpp: Fix connect event race").
> >
> > My speculation is that some of the devices enter powersaving state after being idle for some time (5 mins?), and then wake up and reconnect once I touch either keyboard or mouse. I should highlight that everything works just fine, it is the flood of messages that worries me.
> >
> > Is it expected?
>
> Yes this is expected, looking at your logs I see about 10 messages per
> hour which IMHO is not that bad.
>
> I guess we could change things to track we have logged the connect
> message once and if yes then log future connect messages (and all
> disconnect messages) at debug level.
How granular such a tracking should be? Per-`struct hidpp_device`?
Should there be something like `hid_info_once_then_dbg()` macro, or open-code it in each place instead?
Thanks.
> Jiri, Benjamin, do you have any opinion on this ?
>
> Regards,
>
> Hans
>
>
>
>
--
Oleksandr Natalenko (post-factum)
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: Implement per-key keyboard backlight as auxdisplay?
From: Hans de Goede @ 2024-01-29 13:24 UTC (permalink / raw)
To: Werner Sembach, Pavel Machek
Cc: Jani Nikula, jikos, Jelle van der Waa, Miguel Ojeda, Lee Jones,
linux-kernel, dri-devel@lists.freedesktop.org, linux-input, ojeda,
linux-leds
In-Reply-To: <0cdb78b1-7763-4bb6-9582-d70577781e61@tuxedocomputers.com>
Hi Werner,
On 1/19/24 17:04, Werner Sembach wrote:
> Am 19.01.24 um 09:44 schrieb Hans de Goede:
<snip>
>> So my proposal would be an ioctl interface (ioctl only no r/w)
>> using /dev/rgbkbd0 /dev/rgbkdb1, etc. registered as a misc chardev.
>>
>> For per key controllable rgb LEDs we need to discuss a coordinate
>> system. I propose using a fixed size of 16 rows of 64 keys,
>> so 64x16 in standard WxH notation.
>>
>> And then storing RGB in separate bytes, so userspace will then
>> always send a buffer of 192 bytes per line (64x3) x 14 rows
>> = 3072 bytes. With the kernel driver ignoring parts of
>> the buffer where there are no actual keys.
> The be sure the "14 rows" is a typo? And should be 16 rows?
Yes that should be 16.
<snip>
>> This way we can address all the possible keys in the various
>> standard layouts in one standard wat and then the drivers can
>> just skip keys which are not there when preparing the buffer
>> to send to the hw / fw.
>
> Some remarks here:
>
> - Some keyboards might have two or more leds for big keys like (iso-)enter, shift, capslock, num+, etc. that in theory are individually controllable by the firmware. In windows drivers this is usually abstracted away, but could be interesting for effects (e.g. if the top of iso-enter is separate from the bottom of iso-enter like with one of our devices).
>
> - In combination with this: The driver might not be able to tell if the actual physical keyboard is ISO or ANSI, so it might not be able the correctly assign the leds around enter correctly as being an own key or being part of ANSI- or ISO-enter.
>
> - Should the interface have different addresses for the different enter and num+ styles (or even the different length shifts and spacebars)?
>
> One idea for this: Actually assign 1 value per line for tall keys per line, 3 (or maybe even 4, to have one spare) values per line for wide keys and 6 (or 8) values for space. e.g.:
That sounds workable OTOH combined with your remarks about also supporting
lightbars. I'm starting to think that we need to just punt this to userspace.
So basically change things from trying to present a standardized address
space where say the 'Q' key is always in the same place just model
a keyboard as a string of LEDs (1 dimensional / so an array) and leave
mapping which address in the array is which key to userspace, then userspace
can have json or whatever files for this per keyboard.
This keeps the kernel interface much more KISS which I think is what
we need to strive for.
So instead of having /dev/rgbkbd we get a /dev/rgbledstring and then
that can be used for rbb-kbds and also your lightbar example as well
as actual RGB LED strings, which depending on the controller may
also have zones / effects, etc. just like the keyboards.
> - Right shift would have 3 values in row 10. The first value might be the left side of shift or the additional ABNT/JIS key. The 2nd Key might be the left side or middle of shift and the third key might be the right side of shift or the only value for the whole key. The additional ABNT/JIS key still also has a dedicated value which is used by drivers which can differentiate between physical layouts.
>
> - Enter would have 3 values in row 8 and 3 values in row 9. With the same disambiguation as the additional ABNT/JIS but this time for ansii-/ and iso-#
>
> - Num+ would have 2 values, one row 8 and one in row 9. The one in row 9 might control the whole key or might just control the lower half. The one in row 8 might be another key or the upper half
>
> For the left half if the main block the leftmost value should be the "might be the only relevant"-value while the right most value should be the "might be another key"-value. For the right side of the main block this should be swapped. Unused values should be adjacent to the "might be another key"-value, e.g.:
>
> | Left shift value 1 | Left shift value 2 | Left shift value 3 | Left shift value 4 | 102nd key value
> ISO/ANSI aware | Left shift color | Unused | Unused | Unused | 102nd key color
> ISO non aware 1 led under shift | Left shift color | Unused | Unused | 102nd key color | Unused
> ANSI non aware 1 led under shift | Left shift color | Unused | Unused | Unused | Unused
> ISO non aware 2 leds under shift | Left shift left color | Left shift right color | Unused | 102nd key color | Unused
> ANSI non aware 2 leds under shift | Left shift left color | Left shift right color | Unused | Unused | Unused
> ISO non aware 3 leds under shift | Left shift left color | Left shift middle color | Left shift right color | 102nd key color | Unused
> ANSI non aware 3 leds under shift | Left shift left color | Left shift middle color | Unused | Left shift right color | Unused
> ANSI non aware 4 leds under shift | Left shift left color | Left shift middle left color | Left shift middle right color | Left shift right color | Unused
>
> Like this with no information you can still reliable target the ANSI-shift space, if you know it's an ISO keyboard from user space you can target shift and 102nd key, and if you have even more information you can have multi color shift if the firmware supports it.
Right, so see above I think we need to push all these complications
into userspace. And simple come up for a kernel interface
for RGB LED strings with zones / effects / possibly individual
addressable LEDs.
Also we should really only use whatever kernel interface we come up
with for devices which cannot be supported directly from userspace
through e.g. hidraw access. Looking at keyboards then the openrgb project:
https://openrgb.org/devices_0.9.html
Currently already supports 398 keyboard modes, we really do not want
to be adding support for all those to the kernel.
Further down in the thread you mention Mice with RGB LEDs,
Mice are almost always HID devices and already have extensive support,
including for their LEDs in userspace through libratbag and the piper UI,
see the screenshots here (click on the camera icon):
https://linux.softpedia.com/get/Utilities/Piper-libratbag-104168.shtml
Again we really don't want to be re-doing all this work in the kernel
only to end up conflicting with the existing userspace support.
<snip>
>> 5. A set_effect ioctl which takes a struct with the following members:
>>
>> {
>> long size; /* Size of passed in struct including the size member itself */
>> int effect_nr; /* enum value of the effect to enable, 0 for disable effect */
>> int zone; /* zone to apply the effect to */
> Don't know if this is necessary, the keyboards I have seen so far apply firmware effects globally.
>> int speed; /* cycle speed of the effect in milli-hz */
>
> I would split this into speed and speed_max and don't specify an actual unit. The firmwares effects I have seen so far: If they have a speed value, it's some low number interpreted as a proportional x/n * the max speed of this effect, with n being some low number like 8 or 10.
>
> But i don't know if such clearly named properties are even sensefull, see below.
>
>> char color1[3]; /* effect dependend may be unused. */
>> char color2[3]; /* effect dependend may be unused. */
>> }
>
> We can not predetermine how many colors we might need in the future.
>
> Firmware effects can vary vastly in complexity, e.g. breathing can be a single bit switch that just varies the brightness of whatever color setting is currently applied. It could have an optional speed argument. It could have nth additional color arguments to cycle through, it could have an optional randomize bit that either randomizes the order of the defined colors or means that it is picking completely random color ignoring the color settings if set.
>
> Like this we could have a very fast explosion of the effects enum e.g.: breathing, breathing_2_colors, breathing_3_colors, ... breathing_n_colors, breathing_speed_controlled, breathing_speed_controlled_2_colors, ... breathing_speed_controlled_n_colors_random_bit, etc.
>
> Or we give up on generic names and just make something like: tongfang_breathing_1, tongfang_scan_1, tongfang_breathing_2, clevo_breathing_1
>
> Each with an own struct defined in a big .h file.
>
> Otherwise I think the config struct needs to be dynamically created out of information the driver gives to userspace.
Given that as mentioned above I believe that we should only use a kernel
driver where direct userspace access is impossible I believe that having
things like tongfang_breathing_1, tongfang_scan_1, tongfang_breathing_2,
clevo_breathing_1, etc. for the hopefully small set of devices which
needs an actual kernel driver to be reasonable.
Talking about existing RGB LED support I believe that we should also
reach out to and get feedback on (or even an ack for) the new rgbledstring
API from the OpenRGB folks: https://openrgb.org
Maybe they already have a nice abstraction to deal with different
kind of effects which we can copy for the kernel API ?
Regards,
Hans
^ permalink raw reply
* Re: Implement per-key keyboard backlight as auxdisplay?
From: Hans de Goede @ 2024-01-29 13:24 UTC (permalink / raw)
To: Pavel Machek
Cc: Werner Sembach, Jani Nikula, jikos, Jelle van der Waa,
Miguel Ojeda, Lee Jones, linux-kernel,
dri-devel@lists.freedesktop.org, linux-input, ojeda, linux-leds
In-Reply-To: <ZarYSkzISyS+wuYR@duo.ucw.cz>
Hi,
On 1/19/24 21:15, Pavel Machek wrote:
> Hi!
>
>>>> 2. Implement per-key keyboards as auxdisplay
>>>>
>>>> - Pro:
>>>>
>>>> - Already has a concept for led positions
>>>>
>>>> - Is conceptually closer to "multiple leds forming a singular entity"
>>>>
>>>> - Con:
>>>>
>>>> - No preexisting UPower support
>>>>
>>>> - No concept for special hardware lightning modes
>>>>
>>>> - No support for arbitrary led outlines yet (e.g. ISO style enter-key)
>>>
>>> Please do this one.
>>
>> Ok, so based on the discussion so far and Pavel's feedback lets try to
>> design a custom userspace API for this. I do not believe that auxdisplay
>> is a good fit because:
>
> Ok, so lets call this a "display". These days, framebuffers and drm
> handles displays. My proposal is to use similar API as other displays.
>
>> So my proposal would be an ioctl interface (ioctl only no r/w)
>> using /dev/rgbkbd0 /dev/rgbkdb1, etc. registered as a misc chardev.
>>
>> For per key controllable rgb LEDs we need to discuss a coordinate
>> system. I propose using a fixed size of 16 rows of 64 keys,
>> so 64x16 in standard WxH notation.
>>
>> And then storing RGB in separate bytes, so userspace will then
>> always send a buffer of 192 bytes per line (64x3) x 14 rows
>> = 3072 bytes. With the kernel driver ignoring parts of
>> the buffer where there are no actual keys.
>
> That's really really weird interface. If you are doing RGB888 64x14,
> lets make it a ... display? :-).
>
> ioctl always sending 3072 bytes is really a hack.
>
> Small displays exist and are quite common, surely we'd handle this as
> a display:
> https://pajenicko.cz/displeje/graficky-oled-displej-0-66-64x48-i2c-bily-wemos-d1-mini
> It is 64x48.
This is indeed a display and should use display APIs
> And then there's this:
> https://pajenicko.cz/displeje/maticovy-8x8-led-displej-s-radicem-max7219
> and this:
> https://pajenicko.cz/displeje/maticovy-8x32-led-displej-s-radicem-max7219
>
> One of them is 8x8.
>
> Surely those should be displays, too?
The 8x8 one not really, the other one could be used to scroll
some text one but cannot display images, so not really displays
IMHO.
Anyways we are talking about keyboards here and those do not have
a regular x-y grid like your example above, so they certainly do
not count as displays. See the long discussion earlier in the thread.
Regards,
Hans
^ permalink raw reply
* Re: Flood of logitech-hidpp-device messages in v6.7
From: Hans de Goede @ 2024-01-29 11:10 UTC (permalink / raw)
To: Jiri Kosina
Cc: Oleksandr Natalenko, linux-kernel, linux-input, Filipe Laíns,
Bastien Nocera, Benjamin Tissoires
In-Reply-To: <nycvar.YFH.7.76.2401172001530.29548@cbobk.fhfr.pm>
Hi Jiri,
On 1/17/24 20:01, Jiri Kosina wrote:
> On Tue, 9 Jan 2024, Hans de Goede wrote:
>
>>> Jan 09 10:05:06 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: Disconnected
>>> Jan 09 10:07:15 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: Disconnected
>>> Jan 09 10:16:51 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: HID++ 4.5 device connected.
>>> Jan 09 10:16:55 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: HID++ 4.5 device connected.
>>> Jan 09 10:16:55 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: HID++ 4.5 device connected.
>>> Jan 09 10:36:31 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: Disconnected
>>> Jan 09 10:37:07 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: HID++ 4.5 device connected.
>>> Jan 09 10:46:21 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: Disconnected
>>> Jan 09 10:48:23 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: Disconnected
>>> Jan 09 11:12:27 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: HID++ 4.5 device connected.
>>> Jan 09 11:12:47 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: HID++ 4.5 device connected.
>>> Jan 09 11:12:47 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: HID++ 4.5 device connected.
>>> Jan 09 11:38:32 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: Disconnected
>>> Jan 09 11:43:32 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: Disconnected
>>> Jan 09 11:45:10 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: HID++ 4.5 device connected.
>>> Jan 09 11:45:11 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: HID++ 4.5 device connected.
>>> Jan 09 11:45:11 spock kernel: logitech-hidpp-device 0003:046D:408A.0005: HID++ 4.5 device connected.
>>> Jan 09 12:31:48 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: Disconnected
>>> Jan 09 12:33:21 spock kernel: logitech-hidpp-device 0003:046D:4051.0006: HID++ 4.5 device connected.
>>> ```
>>>
>>> I've got the following hardware:
>>>
>>> * Bus 006 Device 004: ID 046d:c52b Logitech, Inc. Unifying Receiver
>>> * Logitech MX Keys
>>> * Logitech M510v2
>>>
>>> With v6.6 I do not get those messages.
>>>
>>> I think this is related to 680ee411a98e ("HID: logitech-hidpp: Fix connect event race").
>>>
>>> My speculation is that some of the devices enter powersaving state after being idle for some time (5 mins?), and then wake up and reconnect once I touch either keyboard or mouse. I should highlight that everything works just fine, it is the flood of messages that worries me.
>>>
>>> Is it expected?
>>
>> Yes this is expected, looking at your logs I see about 10 messages per
>> hour which IMHO is not that bad.
>>
>> I guess we could change things to track we have logged the connect
>> message once and if yes then log future connect messages (and all
>> disconnect messages) at debug level.
>>
>> Jiri, Benjamin, do you have any opinion on this ?
>
> Works for me, thanks. Do you plan to submit the patch implementing this?
I was planning too, but I'm a bit swamped atm. I would not mind
someone else implementing this suggestion and I would be happy
to review and test.
Regards,
Hans
^ permalink raw reply
* [PATCH v15 4/4] Input: goodix-berlin - add SPI support for Goodix Berlin Touchscreen IC
From: Neil Armstrong @ 2024-01-29 9:16 UTC (permalink / raw)
To: Dmitry Torokhov, linux-input
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bastien Nocera,
Hans de Goede, Henrik Rydberg, Jeff LaBundy, linux-arm-msm,
devicetree, linux-kernel, Neil Armstrong
In-Reply-To: <20240129-topic-goodix-berlin-upstream-initial-v15-0-6f7d096c0a0a@linaro.org>
Add initial support for the new Goodix "Berlin" touchscreen ICs
over the SPI interface.
The driver doesn't use the regmap_spi code since the SPI messages
needs to be prefixed, thus this custom regmap code.
This initial driver is derived from the Goodix goodix_ts_berlin
available at [1] and [2] and only supports the GT9916 IC
present on the Qualcomm SM8550 MTP & QRD touch panel.
The current implementation only supports BerlinD, aka GT9916.
[1] https://github.com/goodix/goodix_ts_berlin
[2] https://git.codelinaro.org/clo/la/platform/vendor/opensource/touch-drivers
Reviewed-by: Jeff LaBundy <jeff@labundy.com>
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
drivers/input/touchscreen/Kconfig | 14 ++
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/goodix_berlin_spi.c | 178 ++++++++++++++++++++++++++
3 files changed, 193 insertions(+)
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index cc7b88118158..c821fe3ee794 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -433,6 +433,20 @@ config TOUCHSCREEN_GOODIX_BERLIN_I2C
To compile this driver as a module, choose M here: the
module will be called goodix_berlin_i2c.
+config TOUCHSCREEN_GOODIX_BERLIN_SPI
+ tristate "Goodix Berlin SPI touchscreen"
+ depends on SPI_MASTER
+ select REGMAP
+ select TOUCHSCREEN_GOODIX_BERLIN_CORE
+ help
+ Say Y here if you have a Goodix Berlin IC connected to
+ your system via SPI.
+
+ If unsure, say N.
+
+ To compile this driver as a module, choose M here: the
+ module will be called goodix_berlin_spi.
+
config TOUCHSCREEN_HIDEEP
tristate "HiDeep Touch IC"
depends on I2C
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 7ef677cf7a10..a81cb5aa21a5 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -49,6 +49,7 @@ obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_CORE) += goodix_berlin_core.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_I2C) += goodix_berlin_i2c.o
+obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_SPI) += goodix_berlin_spi.o
obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
diff --git a/drivers/input/touchscreen/goodix_berlin_spi.c b/drivers/input/touchscreen/goodix_berlin_spi.c
new file mode 100644
index 000000000000..4cc557da048a
--- /dev/null
+++ b/drivers/input/touchscreen/goodix_berlin_spi.c
@@ -0,0 +1,178 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Goodix Berlin Touchscreen Driver
+ *
+ * Copyright (C) 2020 - 2021 Goodix, Inc.
+ * Copyright (C) 2023 Linaro Ltd.
+ *
+ * Based on goodix_ts_berlin driver.
+ */
+#include <asm/unaligned.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/spi/spi.h>
+#include <linux/input.h>
+
+#include "goodix_berlin.h"
+
+#define GOODIX_BERLIN_SPI_TRANS_PREFIX_LEN 1
+#define GOODIX_BERLIN_REGISTER_WIDTH 4
+#define GOODIX_BERLIN_SPI_READ_DUMMY_LEN 3
+#define GOODIX_BERLIN_SPI_READ_PREFIX_LEN (GOODIX_BERLIN_SPI_TRANS_PREFIX_LEN + \
+ GOODIX_BERLIN_REGISTER_WIDTH + \
+ GOODIX_BERLIN_SPI_READ_DUMMY_LEN)
+#define GOODIX_BERLIN_SPI_WRITE_PREFIX_LEN (GOODIX_BERLIN_SPI_TRANS_PREFIX_LEN + \
+ GOODIX_BERLIN_REGISTER_WIDTH)
+
+#define GOODIX_BERLIN_SPI_WRITE_FLAG 0xF0
+#define GOODIX_BERLIN_SPI_READ_FLAG 0xF1
+
+static int goodix_berlin_spi_read(void *context, const void *reg_buf,
+ size_t reg_size, void *val_buf,
+ size_t val_size)
+{
+ struct spi_device *spi = context;
+ struct spi_transfer xfers;
+ struct spi_message spi_msg;
+ const u32 *reg = reg_buf; /* reg is stored as native u32 at start of buffer */
+ u8 *buf;
+ int error;
+
+ if (reg_size != GOODIX_BERLIN_REGISTER_WIDTH)
+ return -EINVAL;
+
+ buf = kzalloc(GOODIX_BERLIN_SPI_READ_PREFIX_LEN + val_size, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ spi_message_init(&spi_msg);
+ memset(&xfers, 0, sizeof(xfers));
+
+ /* buffer format: 0xF1 + addr(4bytes) + dummy(3bytes) + data */
+ buf[0] = GOODIX_BERLIN_SPI_READ_FLAG;
+ put_unaligned_be32(*reg, buf + GOODIX_BERLIN_SPI_TRANS_PREFIX_LEN);
+ memset(buf + GOODIX_BERLIN_SPI_TRANS_PREFIX_LEN + GOODIX_BERLIN_REGISTER_WIDTH,
+ 0xff, GOODIX_BERLIN_SPI_READ_DUMMY_LEN);
+
+ xfers.tx_buf = buf;
+ xfers.rx_buf = buf;
+ xfers.len = GOODIX_BERLIN_SPI_READ_PREFIX_LEN + val_size;
+ xfers.cs_change = 0;
+ spi_message_add_tail(&xfers, &spi_msg);
+
+ error = spi_sync(spi, &spi_msg);
+ if (error < 0)
+ dev_err(&spi->dev, "spi transfer error, %d", error);
+ else
+ memcpy(val_buf, buf + GOODIX_BERLIN_SPI_READ_PREFIX_LEN, val_size);
+
+ kfree(buf);
+ return error;
+}
+
+static int goodix_berlin_spi_write(void *context, const void *data,
+ size_t count)
+{
+ unsigned int len = count - GOODIX_BERLIN_REGISTER_WIDTH;
+ struct spi_device *spi = context;
+ struct spi_transfer xfers;
+ struct spi_message spi_msg;
+ const u32 *reg = data; /* reg is stored as native u32 at start of buffer */
+ u8 *buf;
+ int error;
+
+ buf = kzalloc(GOODIX_BERLIN_SPI_WRITE_PREFIX_LEN + len, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ spi_message_init(&spi_msg);
+ memset(&xfers, 0, sizeof(xfers));
+
+ buf[0] = GOODIX_BERLIN_SPI_WRITE_FLAG;
+ put_unaligned_be32(*reg, buf + GOODIX_BERLIN_SPI_TRANS_PREFIX_LEN);
+ memcpy(buf + GOODIX_BERLIN_SPI_WRITE_PREFIX_LEN,
+ data + GOODIX_BERLIN_REGISTER_WIDTH, len);
+
+ xfers.tx_buf = buf;
+ xfers.len = GOODIX_BERLIN_SPI_WRITE_PREFIX_LEN + len;
+ xfers.cs_change = 0;
+ spi_message_add_tail(&xfers, &spi_msg);
+
+ error = spi_sync(spi, &spi_msg);
+ if (error < 0)
+ dev_err(&spi->dev, "spi transfer error, %d", error);
+
+ kfree(buf);
+ return error;
+}
+
+static const struct regmap_config goodix_berlin_spi_regmap_conf = {
+ .reg_bits = 32,
+ .val_bits = 8,
+ .read = goodix_berlin_spi_read,
+ .write = goodix_berlin_spi_write,
+};
+
+/* vendor & product left unassigned here, should probably be updated from fw info */
+static const struct input_id goodix_berlin_spi_input_id = {
+ .bustype = BUS_SPI,
+};
+
+static int goodix_berlin_spi_probe(struct spi_device *spi)
+{
+ struct regmap_config regmap_config;
+ struct regmap *regmap;
+ size_t max_size;
+ int error = 0;
+
+ spi->mode = SPI_MODE_0;
+ spi->bits_per_word = 8;
+ error = spi_setup(spi);
+ if (error)
+ return error;
+
+ max_size = spi_max_transfer_size(spi);
+
+ regmap_config = goodix_berlin_spi_regmap_conf;
+ regmap_config.max_raw_read = max_size - GOODIX_BERLIN_SPI_READ_PREFIX_LEN;
+ regmap_config.max_raw_write = max_size - GOODIX_BERLIN_SPI_WRITE_PREFIX_LEN;
+
+ regmap = devm_regmap_init(&spi->dev, NULL, spi, ®map_config);
+ if (IS_ERR(regmap))
+ return PTR_ERR(regmap);
+
+ error = goodix_berlin_probe(&spi->dev, spi->irq,
+ &goodix_berlin_spi_input_id, regmap);
+ if (error)
+ return error;
+
+ return 0;
+}
+
+static const struct spi_device_id goodix_berlin_spi_ids[] = {
+ { "gt9916" },
+ { },
+};
+MODULE_DEVICE_TABLE(spi, goodix_berlin_spi_ids);
+
+static const struct of_device_id goodix_berlin_spi_of_match[] = {
+ { .compatible = "goodix,gt9916", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, goodix_berlin_spi_of_match);
+
+static struct spi_driver goodix_berlin_spi_driver = {
+ .driver = {
+ .name = "goodix-berlin-spi",
+ .of_match_table = goodix_berlin_spi_of_match,
+ .pm = pm_sleep_ptr(&goodix_berlin_pm_ops),
+ },
+ .probe = goodix_berlin_spi_probe,
+ .id_table = goodix_berlin_spi_ids,
+};
+module_spi_driver(goodix_berlin_spi_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Goodix Berlin SPI Touchscreen driver");
+MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>");
--
2.34.1
^ permalink raw reply related
* [PATCH v15 3/4] Input: goodix-berlin - add I2C support for Goodix Berlin Touchscreen IC
From: Neil Armstrong @ 2024-01-29 9:16 UTC (permalink / raw)
To: Dmitry Torokhov, linux-input
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bastien Nocera,
Hans de Goede, Henrik Rydberg, Jeff LaBundy, linux-arm-msm,
devicetree, linux-kernel, Neil Armstrong
In-Reply-To: <20240129-topic-goodix-berlin-upstream-initial-v15-0-6f7d096c0a0a@linaro.org>
Add initial support for the new Goodix "Berlin" touchscreen ICs
over the I2C interface.
This initial driver is derived from the Goodix goodix_ts_berlin
available at [1] and [2] and only supports the GT9916 IC
present on the Qualcomm SM8550 MTP & QRD touch panel.
The current implementation only supports BerlinD, aka GT9916.
[1] https://github.com/goodix/goodix_ts_berlin
[2] https://git.codelinaro.org/clo/la/platform/vendor/opensource/touch-drivers
Reviewed-by: Jeff LaBundy <jeff@labundy.com>
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
drivers/input/touchscreen/Kconfig | 14 +++++
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/goodix_berlin_i2c.c | 75 +++++++++++++++++++++++++++
3 files changed, 90 insertions(+)
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index 950da599ae1a..cc7b88118158 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -419,6 +419,20 @@ config TOUCHSCREEN_GOODIX
config TOUCHSCREEN_GOODIX_BERLIN_CORE
tristate
+config TOUCHSCREEN_GOODIX_BERLIN_I2C
+ tristate "Goodix Berlin I2C touchscreen"
+ depends on I2C
+ select REGMAP_I2C
+ select TOUCHSCREEN_GOODIX_BERLIN_CORE
+ help
+ Say Y here if you have a Goodix Berlin IC connected to
+ your system via I2C.
+
+ If unsure, say N.
+
+ To compile this driver as a module, choose M here: the
+ module will be called goodix_berlin_i2c.
+
config TOUCHSCREEN_HIDEEP
tristate "HiDeep Touch IC"
depends on I2C
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 2e2f3e70cd2c..7ef677cf7a10 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -48,6 +48,7 @@ obj-$(CONFIG_TOUCHSCREEN_EXC3000) += exc3000.o
obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_CORE) += goodix_berlin_core.o
+obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_I2C) += goodix_berlin_i2c.o
obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
diff --git a/drivers/input/touchscreen/goodix_berlin_i2c.c b/drivers/input/touchscreen/goodix_berlin_i2c.c
new file mode 100644
index 000000000000..6ed9aa8088cb
--- /dev/null
+++ b/drivers/input/touchscreen/goodix_berlin_i2c.c
@@ -0,0 +1,75 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Goodix Berlin Touchscreen Driver
+ *
+ * Copyright (C) 2020 - 2021 Goodix, Inc.
+ * Copyright (C) 2023 Linaro Ltd.
+ *
+ * Based on goodix_ts_berlin driver.
+ */
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/input.h>
+
+#include "goodix_berlin.h"
+
+#define I2C_MAX_TRANSFER_SIZE 256
+
+static const struct regmap_config goodix_berlin_i2c_regmap_conf = {
+ .reg_bits = 32,
+ .val_bits = 8,
+ .max_raw_read = I2C_MAX_TRANSFER_SIZE,
+ .max_raw_write = I2C_MAX_TRANSFER_SIZE,
+};
+
+/* vendor & product left unassigned here, should probably be updated from fw info */
+static const struct input_id goodix_berlin_i2c_input_id = {
+ .bustype = BUS_I2C,
+};
+
+static int goodix_berlin_i2c_probe(struct i2c_client *client)
+{
+ struct regmap *regmap;
+ int error;
+
+ regmap = devm_regmap_init_i2c(client, &goodix_berlin_i2c_regmap_conf);
+ if (IS_ERR(regmap))
+ return PTR_ERR(regmap);
+
+ error = goodix_berlin_probe(&client->dev, client->irq,
+ &goodix_berlin_i2c_input_id, regmap);
+ if (error)
+ return error;
+
+ return 0;
+}
+
+static const struct i2c_device_id goodix_berlin_i2c_id[] = {
+ { "gt9916", 0 },
+ { }
+};
+
+MODULE_DEVICE_TABLE(i2c, goodix_berlin_i2c_id);
+
+static const struct of_device_id goodix_berlin_i2c_of_match[] = {
+ { .compatible = "goodix,gt9916", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, goodix_berlin_i2c_of_match);
+
+static struct i2c_driver goodix_berlin_i2c_driver = {
+ .driver = {
+ .name = "goodix-berlin-i2c",
+ .of_match_table = goodix_berlin_i2c_of_match,
+ .pm = pm_sleep_ptr(&goodix_berlin_pm_ops),
+ },
+ .probe = goodix_berlin_i2c_probe,
+ .id_table = goodix_berlin_i2c_id,
+};
+module_i2c_driver(goodix_berlin_i2c_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Goodix Berlin I2C Touchscreen driver");
+MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>");
--
2.34.1
^ permalink raw reply related
* [PATCH v15 2/4] Input: add core support for Goodix Berlin Touchscreen IC
From: Neil Armstrong @ 2024-01-29 9:16 UTC (permalink / raw)
To: Dmitry Torokhov, linux-input
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bastien Nocera,
Hans de Goede, Henrik Rydberg, Jeff LaBundy, linux-arm-msm,
devicetree, linux-kernel, Neil Armstrong
In-Reply-To: <20240129-topic-goodix-berlin-upstream-initial-v15-0-6f7d096c0a0a@linaro.org>
Add initial support for the new Goodix "Berlin" touchscreen ICs.
These touchscreen ICs support SPI, I2C and I3C interface, up to
10 finger touch, stylus and gestures events.
This initial driver is derived from the Goodix goodix_ts_berlin
available at [1] and [2] and only supports the GT9916 IC
present on the Qualcomm SM8550 MTP & QRD touch panel.
The current implementation only supports BerlinD, aka GT9916.
Support for advanced features like:
- Firmware & config update
- Stylus events
- Gestures events
- Previous revisions support (BerlinA or BerlinB)
is not included in current version.
The current support will work with currently flashed firmware
and config, and bail out if firmware or config aren't flashed yet.
[1] https://github.com/goodix/goodix_ts_berlin
[2] https://git.codelinaro.org/clo/la/platform/vendor/opensource/touch-drivers
Reviewed-by: Jeff LaBundy <jeff@labundy.com>
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
drivers/input/touchscreen/Kconfig | 3 +
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/goodix_berlin.h | 24 +
drivers/input/touchscreen/goodix_berlin_core.c | 755 +++++++++++++++++++++++++
4 files changed, 783 insertions(+)
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index e3e2324547b9..950da599ae1a 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -416,6 +416,9 @@ config TOUCHSCREEN_GOODIX
To compile this driver as a module, choose M here: the
module will be called goodix.
+config TOUCHSCREEN_GOODIX_BERLIN_CORE
+ tristate
+
config TOUCHSCREEN_HIDEEP
tristate "HiDeep Touch IC"
depends on I2C
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 62bd24f3ac8e..2e2f3e70cd2c 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -47,6 +47,7 @@ obj-$(CONFIG_TOUCHSCREEN_EGALAX_SERIAL) += egalax_ts_serial.o
obj-$(CONFIG_TOUCHSCREEN_EXC3000) += exc3000.o
obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
+obj-$(CONFIG_TOUCHSCREEN_GOODIX_BERLIN_CORE) += goodix_berlin_core.o
obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
diff --git a/drivers/input/touchscreen/goodix_berlin.h b/drivers/input/touchscreen/goodix_berlin.h
new file mode 100644
index 000000000000..1fd77eb69c9a
--- /dev/null
+++ b/drivers/input/touchscreen/goodix_berlin.h
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Goodix Touchscreen Driver
+ * Copyright (C) 2020 - 2021 Goodix, Inc.
+ * Copyright (C) 2023 Linaro Ltd.
+ *
+ * Based on goodix_berlin_berlin driver.
+ */
+
+#ifndef __GOODIX_BERLIN_H_
+#define __GOODIX_BERLIN_H_
+
+#include <linux/pm.h>
+
+struct device;
+struct input_id;
+struct regmap;
+
+int goodix_berlin_probe(struct device *dev, int irq, const struct input_id *id,
+ struct regmap *regmap);
+
+extern const struct dev_pm_ops goodix_berlin_pm_ops;
+
+#endif
diff --git a/drivers/input/touchscreen/goodix_berlin_core.c b/drivers/input/touchscreen/goodix_berlin_core.c
new file mode 100644
index 000000000000..e7b41a926ef8
--- /dev/null
+++ b/drivers/input/touchscreen/goodix_berlin_core.c
@@ -0,0 +1,755 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Goodix "Berlin" Touchscreen IC driver
+ * Copyright (C) 2020 - 2021 Goodix, Inc.
+ * Copyright (C) 2023 Linaro Ltd.
+ *
+ * Based on goodix_ts_berlin driver.
+ *
+ * This driver is distinct from goodix.c since hardware interface
+ * is different enough to require a new driver.
+ * None of the register address or data structure are close enough
+ * to the previous generations.
+ *
+ * Currently the driver only handles Multitouch events with already
+ * programmed firmware and "config" for "Revision D" Berlin IC.
+ *
+ * Support is missing for:
+ * - ESD Management
+ * - Firmware update/flashing
+ * - "Config" update/flashing
+ * - Stylus Events
+ * - Gesture Events
+ * - Support for older revisions (A & B)
+ */
+
+#include <linux/bitfield.h>
+#include <linux/gpio/consumer.h>
+#include <linux/input.h>
+#include <linux/input/mt.h>
+#include <linux/input/touchscreen.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/sizes.h>
+#include <asm/unaligned.h>
+
+#include "goodix_berlin.h"
+
+#define GOODIX_BERLIN_MAX_TOUCH 10
+
+#define GOODIX_BERLIN_NORMAL_RESET_DELAY_MS 100
+
+#define GOODIX_BERLIN_TOUCH_EVENT BIT(7)
+#define GOODIX_BERLIN_REQUEST_EVENT BIT(6)
+#define GOODIX_BERLIN_TOUCH_COUNT_MASK GENMASK(3, 0)
+
+#define GOODIX_BERLIN_REQUEST_CODE_RESET 3
+
+#define GOODIX_BERLIN_POINT_TYPE_MASK GENMASK(3, 0)
+#define GOODIX_BERLIN_POINT_TYPE_STYLUS_HOVER 1
+#define GOODIX_BERLIN_POINT_TYPE_STYLUS 3
+
+#define GOODIX_BERLIN_TOUCH_ID_MASK GENMASK(7, 4)
+
+#define GOODIX_BERLIN_DEV_CONFIRM_VAL 0xAA
+#define GOODIX_BERLIN_BOOTOPTION_ADDR 0x10000
+#define GOODIX_BERLIN_FW_VERSION_INFO_ADDR 0x10014
+
+#define GOODIX_BERLIN_IC_INFO_MAX_LEN SZ_1K
+#define GOODIX_BERLIN_IC_INFO_ADDR 0x10070
+
+#define GOODIX_BERLIN_CHECKSUM_SIZE sizeof(u16)
+
+struct goodix_berlin_fw_version {
+ u8 rom_pid[6];
+ u8 rom_vid[3];
+ u8 rom_vid_reserved;
+ u8 patch_pid[8];
+ u8 patch_vid[4];
+ u8 patch_vid_reserved;
+ u8 sensor_id;
+ u8 reserved[2];
+ __le16 checksum;
+};
+
+struct goodix_berlin_ic_info_version {
+ u8 info_customer_id;
+ u8 info_version_id;
+ u8 ic_die_id;
+ u8 ic_version_id;
+ __le32 config_id;
+ u8 config_version;
+ u8 frame_data_customer_id;
+ u8 frame_data_version_id;
+ u8 touch_data_customer_id;
+ u8 touch_data_version_id;
+ u8 reserved[3];
+} __packed;
+
+struct goodix_berlin_ic_info_feature {
+ __le16 freqhop_feature;
+ __le16 calibration_feature;
+ __le16 gesture_feature;
+ __le16 side_touch_feature;
+ __le16 stylus_feature;
+} __packed;
+
+struct goodix_berlin_ic_info_misc {
+ __le32 cmd_addr;
+ __le16 cmd_max_len;
+ __le32 cmd_reply_addr;
+ __le16 cmd_reply_len;
+ __le32 fw_state_addr;
+ __le16 fw_state_len;
+ __le32 fw_buffer_addr;
+ __le16 fw_buffer_max_len;
+ __le32 frame_data_addr;
+ __le16 frame_data_head_len;
+ __le16 fw_attr_len;
+ __le16 fw_log_len;
+ u8 pack_max_num;
+ u8 pack_compress_version;
+ __le16 stylus_struct_len;
+ __le16 mutual_struct_len;
+ __le16 self_struct_len;
+ __le16 noise_struct_len;
+ __le32 touch_data_addr;
+ __le16 touch_data_head_len;
+ __le16 point_struct_len;
+ __le16 reserved1;
+ __le16 reserved2;
+ __le32 mutual_rawdata_addr;
+ __le32 mutual_diffdata_addr;
+ __le32 mutual_refdata_addr;
+ __le32 self_rawdata_addr;
+ __le32 self_diffdata_addr;
+ __le32 self_refdata_addr;
+ __le32 iq_rawdata_addr;
+ __le32 iq_refdata_addr;
+ __le32 im_rawdata_addr;
+ __le16 im_readata_len;
+ __le32 noise_rawdata_addr;
+ __le16 noise_rawdata_len;
+ __le32 stylus_rawdata_addr;
+ __le16 stylus_rawdata_len;
+ __le32 noise_data_addr;
+ __le32 esd_addr;
+} __packed;
+
+struct goodix_berlin_touch {
+ u8 status;
+ u8 reserved;
+ __le16 x;
+ __le16 y;
+ __le16 w;
+};
+#define GOODIX_BERLIN_TOUCH_SIZE sizeof(struct goodix_berlin_touch)
+
+struct goodix_berlin_header {
+ u8 status;
+ u8 reserved1;
+ u8 request_type;
+ u8 reserved2[3];
+ __le16 checksum;
+};
+#define GOODIX_BERLIN_HEADER_SIZE sizeof(struct goodix_berlin_header)
+
+struct goodix_berlin_event {
+ struct goodix_berlin_header hdr;
+ /* The data below is u16/__le16 aligned */
+ u8 data[GOODIX_BERLIN_TOUCH_SIZE * GOODIX_BERLIN_MAX_TOUCH +
+ GOODIX_BERLIN_CHECKSUM_SIZE];
+};
+
+struct goodix_berlin_core {
+ struct device *dev;
+ struct regmap *regmap;
+ struct regulator *avdd;
+ struct regulator *iovdd;
+ struct gpio_desc *reset_gpio;
+ struct touchscreen_properties props;
+ struct goodix_berlin_fw_version fw_version;
+ struct input_dev *input_dev;
+ int irq;
+
+ /* Runtime parameters extracted from IC_INFO buffer */
+ u32 touch_data_addr;
+
+ struct goodix_berlin_event event;
+};
+
+static bool goodix_berlin_checksum_valid(const u8 *data, int size)
+{
+ u32 cal_checksum = 0;
+ u16 r_checksum;
+ int i;
+
+ if (size < GOODIX_BERLIN_CHECKSUM_SIZE)
+ return false;
+
+ for (i = 0; i < size - GOODIX_BERLIN_CHECKSUM_SIZE; i++)
+ cal_checksum += data[i];
+
+ r_checksum = get_unaligned_le16(&data[i]);
+
+ return (u16)cal_checksum == r_checksum;
+}
+
+static bool goodix_berlin_is_dummy_data(struct goodix_berlin_core *cd,
+ const u8 *data, int size)
+{
+ int i;
+
+ /*
+ * If the device is missing or doesn't respond the buffer
+ * could be filled with bus default line state, 0x00 or 0xff,
+ * so declare success the first time we encounter neither.
+ */
+ for (i = 0; i < size; i++)
+ if (data[i] > 0 && data[i] < 0xff)
+ return false;
+
+ return true;
+}
+
+static int goodix_berlin_dev_confirm(struct goodix_berlin_core *cd)
+{
+ u8 tx_buf[8], rx_buf[8];
+ int retry = 3;
+ int error;
+
+ memset(tx_buf, GOODIX_BERLIN_DEV_CONFIRM_VAL, sizeof(tx_buf));
+ while (retry--) {
+ error = regmap_raw_write(cd->regmap,
+ GOODIX_BERLIN_BOOTOPTION_ADDR,
+ tx_buf, sizeof(tx_buf));
+ if (error)
+ return error;
+
+ error = regmap_raw_read(cd->regmap,
+ GOODIX_BERLIN_BOOTOPTION_ADDR,
+ rx_buf, sizeof(rx_buf));
+ if (error)
+ return error;
+
+ if (!memcmp(tx_buf, rx_buf, sizeof(tx_buf)))
+ return 0;
+
+ usleep_range(5000, 5100);
+ }
+
+ dev_err(cd->dev, "device confirm failed, rx_buf: %*ph\n",
+ (int)sizeof(rx_buf), rx_buf);
+
+ return -EINVAL;
+}
+
+static int goodix_berlin_power_on(struct goodix_berlin_core *cd)
+{
+ int error;
+
+ error = regulator_enable(cd->iovdd);
+ if (error) {
+ dev_err(cd->dev, "Failed to enable iovdd: %d\n", error);
+ return error;
+ }
+
+ /* Vendor waits 3ms for IOVDD to settle */
+ usleep_range(3000, 3100);
+
+ error = regulator_enable(cd->avdd);
+ if (error) {
+ dev_err(cd->dev, "Failed to enable avdd: %d\n", error);
+ goto err_iovdd_disable;
+ }
+
+ /* Vendor waits 15ms for IOVDD to settle */
+ usleep_range(15000, 15100);
+
+ gpiod_set_value_cansleep(cd->reset_gpio, 0);
+
+ /* Vendor waits 4ms for Firmware to initialize */
+ usleep_range(4000, 4100);
+
+ error = goodix_berlin_dev_confirm(cd);
+ if (error)
+ goto err_dev_reset;
+
+ /* Vendor waits 100ms for Firmware to fully boot */
+ msleep(GOODIX_BERLIN_NORMAL_RESET_DELAY_MS);
+
+ return 0;
+
+err_dev_reset:
+ gpiod_set_value_cansleep(cd->reset_gpio, 1);
+ regulator_disable(cd->avdd);
+err_iovdd_disable:
+ regulator_disable(cd->iovdd);
+ return error;
+}
+
+static void goodix_berlin_power_off(struct goodix_berlin_core *cd)
+{
+ gpiod_set_value_cansleep(cd->reset_gpio, 1);
+ regulator_disable(cd->avdd);
+ regulator_disable(cd->iovdd);
+}
+
+static int goodix_berlin_read_version(struct goodix_berlin_core *cd)
+{
+ int error;
+
+ error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_FW_VERSION_INFO_ADDR,
+ &cd->fw_version, sizeof(cd->fw_version));
+ if (error) {
+ dev_err(cd->dev, "error reading fw version, %d\n", error);
+ return error;
+ }
+
+ if (!goodix_berlin_checksum_valid((u8 *)&cd->fw_version,
+ sizeof(cd->fw_version))) {
+ dev_err(cd->dev, "invalid fw version: checksum error\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+/* Only extract necessary data for runtime */
+static int goodix_berlin_parse_ic_info(struct goodix_berlin_core *cd,
+ const u8 *data, u16 length)
+{
+ struct goodix_berlin_ic_info_misc *misc;
+ unsigned int offset = 0;
+
+ offset += sizeof(__le16); /* length */
+ offset += sizeof(struct goodix_berlin_ic_info_version);
+ offset += sizeof(struct goodix_berlin_ic_info_feature);
+
+ /* IC_INFO Parameters, variable width structure */
+ offset += 4 * sizeof(u8); /* drv_num, sen_num, button_num, force_num */
+ if (offset >= length)
+ goto invalid_offset;
+
+#define ADVANCE_LE16_PARAMS() \
+ do { \
+ u8 param_num = data[offset++]; \
+ offset += param_num * sizeof(__le16); \
+ if (offset >= length) \
+ goto invalid_offset; \
+ } while (0)
+ ADVANCE_LE16_PARAMS(); /* active_scan_rate_num */
+ ADVANCE_LE16_PARAMS(); /* mutual_freq_num*/
+ ADVANCE_LE16_PARAMS(); /* self_tx_freq_num */
+ ADVANCE_LE16_PARAMS(); /* self_rx_freq_num */
+ ADVANCE_LE16_PARAMS(); /* stylus_freq_num */
+#undef ADVANCE_LE16_PARAMS
+
+ misc = (struct goodix_berlin_ic_info_misc *)&data[offset];
+ cd->touch_data_addr = le32_to_cpu(misc->touch_data_addr);
+
+ return 0;
+
+invalid_offset:
+ dev_err(cd->dev, "ic_info length is invalid (offset %d length %d)\n",
+ offset, length);
+ return -EINVAL;
+}
+
+static int goodix_berlin_get_ic_info(struct goodix_berlin_core *cd)
+{
+ u8 *afe_data __free(kfree) = NULL;
+ __le16 length_raw;
+ u16 length;
+ int error;
+
+ afe_data = kzalloc(GOODIX_BERLIN_IC_INFO_MAX_LEN, GFP_KERNEL);
+ if (!afe_data)
+ return -ENOMEM;
+
+ error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_IC_INFO_ADDR,
+ &length_raw, sizeof(length_raw));
+ if (error) {
+ dev_err(cd->dev, "failed get ic info length, %d\n", error);
+ return error;
+ }
+
+ length = le16_to_cpu(length_raw);
+ if (length >= GOODIX_BERLIN_IC_INFO_MAX_LEN) {
+ dev_err(cd->dev, "invalid ic info length %d\n", length);
+ return -EINVAL;
+ }
+
+ error = regmap_raw_read(cd->regmap, GOODIX_BERLIN_IC_INFO_ADDR,
+ afe_data, length);
+ if (error) {
+ dev_err(cd->dev, "failed get ic info data, %d\n", error);
+ return error;
+ }
+
+ /* check whether the data is valid (ex. bus default values) */
+ if (goodix_berlin_is_dummy_data(cd, afe_data, length)) {
+ dev_err(cd->dev, "fw info data invalid\n");
+ return -EINVAL;
+ }
+
+ if (!goodix_berlin_checksum_valid(afe_data, length)) {
+ dev_err(cd->dev, "fw info checksum error\n");
+ return -EINVAL;
+ }
+
+ error = goodix_berlin_parse_ic_info(cd, afe_data, length);
+ if (error)
+ return error;
+
+ /* check some key info */
+ if (!cd->touch_data_addr) {
+ dev_err(cd->dev, "touch_data_addr is null\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int goodix_berlin_get_remaining_contacts(struct goodix_berlin_core *cd,
+ int n)
+{
+ size_t offset = 2 * GOODIX_BERLIN_TOUCH_SIZE +
+ GOODIX_BERLIN_CHECKSUM_SIZE;
+ u32 addr = cd->touch_data_addr + GOODIX_BERLIN_HEADER_SIZE + offset;
+ int error;
+
+ error = regmap_raw_read(cd->regmap, addr,
+ &cd->event.data[offset],
+ (n - 2) * GOODIX_BERLIN_TOUCH_SIZE);
+ if (error) {
+ dev_err_ratelimited(cd->dev, "failed to get touch data, %d\n",
+ error);
+ return error;
+ }
+
+ return 0;
+}
+
+static void goodix_berlin_report_state(struct goodix_berlin_core *cd, int n)
+{
+ struct goodix_berlin_touch *touch_data =
+ (struct goodix_berlin_touch *)cd->event.data;
+ struct goodix_berlin_touch *t;
+ int i;
+ u8 type, id;
+
+ for (i = 0; i < n; i++) {
+ t = &touch_data[i];
+
+ type = FIELD_GET(GOODIX_BERLIN_POINT_TYPE_MASK, t->status);
+ if (type == GOODIX_BERLIN_POINT_TYPE_STYLUS ||
+ type == GOODIX_BERLIN_POINT_TYPE_STYLUS_HOVER) {
+ dev_warn_once(cd->dev, "Stylus event type not handled\n");
+ continue;
+ }
+
+ id = FIELD_GET(GOODIX_BERLIN_TOUCH_ID_MASK, t->status);
+ if (id >= GOODIX_BERLIN_MAX_TOUCH) {
+ dev_warn_ratelimited(cd->dev, "invalid finger id %d\n", id);
+ continue;
+ }
+
+ input_mt_slot(cd->input_dev, id);
+ input_mt_report_slot_state(cd->input_dev, MT_TOOL_FINGER, true);
+
+ touchscreen_report_pos(cd->input_dev, &cd->props,
+ __le16_to_cpu(t->x), __le16_to_cpu(t->y),
+ true);
+ input_report_abs(cd->input_dev, ABS_MT_TOUCH_MAJOR,
+ __le16_to_cpu(t->w));
+ }
+
+ input_mt_sync_frame(cd->input_dev);
+ input_sync(cd->input_dev);
+}
+
+static void goodix_berlin_touch_handler(struct goodix_berlin_core *cd)
+{
+ u8 touch_num;
+ int error;
+
+ touch_num = FIELD_GET(GOODIX_BERLIN_TOUCH_COUNT_MASK,
+ cd->event.hdr.request_type);
+ if (touch_num > GOODIX_BERLIN_MAX_TOUCH) {
+ dev_warn(cd->dev, "invalid touch num %d\n", touch_num);
+ return;
+ }
+
+ if (touch_num > 2) {
+ /* read additional contact data if more than 2 touch events */
+ error = goodix_berlin_get_remaining_contacts(cd, touch_num);
+ if (error)
+ return;
+ }
+
+ if (touch_num) {
+ int len = touch_num * GOODIX_BERLIN_TOUCH_SIZE +
+ GOODIX_BERLIN_CHECKSUM_SIZE;
+ if (!goodix_berlin_checksum_valid(cd->event.data, len)) {
+ dev_err(cd->dev, "touch data checksum error: %*ph\n",
+ len, cd->event.data);
+ return;
+ }
+ }
+
+ goodix_berlin_report_state(cd, touch_num);
+}
+
+static int goodix_berlin_request_handle_reset(struct goodix_berlin_core *cd)
+{
+ gpiod_set_value_cansleep(cd->reset_gpio, 1);
+ usleep_range(2000, 2100);
+ gpiod_set_value_cansleep(cd->reset_gpio, 0);
+
+ msleep(GOODIX_BERLIN_NORMAL_RESET_DELAY_MS);
+
+ return 0;
+}
+
+static irqreturn_t goodix_berlin_irq(int irq, void *data)
+{
+ struct goodix_berlin_core *cd = data;
+ int error;
+
+ /*
+ * First, read buffer with space for 2 touch events:
+ * - GOODIX_BERLIN_HEADER_SIZE = 8 bytes
+ * - GOODIX_BERLIN_TOUCH_SIZE * 2 = 16 bytes
+ * - GOODIX_BERLIN_CHECKLSUM_SIZE = 2 bytes
+ * For a total of 26 bytes.
+ *
+ * If only a single finger is reported, we will read 8 bytes more than
+ * needed:
+ * - bytes 0-7: Header (GOODIX_BERLIN_HEADER_SIZE)
+ * - bytes 8-15: Finger 0 Data
+ * - bytes 24-25: Checksum
+ * - bytes 18-25: Unused 8 bytes
+ *
+ * If 2 fingers are reported, we would have read the exact needed
+ * amount of data and checksum would be at the end of the buffer:
+ * - bytes 0-7: Header (GOODIX_BERLIN_HEADER_SIZE)
+ * - bytes 8-15: Finger 0 Bytes 0-7
+ * - bytes 16-23: Finger 1 Bytes 0-7
+ * - bytes 24-25: Checksum
+ *
+ * If more than 2 fingers were reported, the "Checksum" bytes would
+ * in fact contain part of the next finger data, and then
+ * goodix_berlin_get_remaining_contacts() would complete the buffer
+ * with the missing bytes, including the trailing checksum.
+ * For example, if 3 fingers are reported, then we would do:
+ * Read 1:
+ * - bytes 0-7: Header (GOODIX_BERLIN_HEADER_SIZE)
+ * - bytes 8-15: Finger 0 Bytes 0-7
+ * - bytes 16-23: Finger 1 Bytes 0-7
+ * - bytes 24-25: Finger 2 Bytes 0-1
+ * Read 2 (with length of (3 - 2) * 8 = 8 bytes):
+ * - bytes 26-31: Finger 2 Bytes 2-7
+ * - bytes 32-33: Checksum
+ */
+ error = regmap_raw_read(cd->regmap, cd->touch_data_addr,
+ &cd->event,
+ GOODIX_BERLIN_HEADER_SIZE +
+ 2 * GOODIX_BERLIN_TOUCH_SIZE +
+ GOODIX_BERLIN_CHECKSUM_SIZE);
+ if (error) {
+ dev_warn_ratelimited(cd->dev,
+ "failed get event head data: %d\n", error);
+ goto out;
+ }
+
+ if (cd->event.hdr.status == 0)
+ goto out;
+
+ if (!goodix_berlin_checksum_valid((u8 *)&cd->event.hdr,
+ GOODIX_BERLIN_HEADER_SIZE)) {
+ dev_warn_ratelimited(cd->dev,
+ "touch head checksum error: %*ph\n",
+ (int)GOODIX_BERLIN_HEADER_SIZE,
+ &cd->event.hdr);
+ goto out_clear;
+ }
+
+ if (cd->event.hdr.status & GOODIX_BERLIN_TOUCH_EVENT)
+ goodix_berlin_touch_handler(cd);
+
+ if (cd->event.hdr.status & GOODIX_BERLIN_REQUEST_EVENT) {
+ switch (cd->event.hdr.request_type) {
+ case GOODIX_BERLIN_REQUEST_CODE_RESET:
+ if (cd->reset_gpio)
+ goodix_berlin_request_handle_reset(cd);
+ break;
+
+ default:
+ dev_warn(cd->dev, "unsupported request code 0x%x\n",
+ cd->event.hdr.request_type);
+ }
+ }
+
+
+out_clear:
+ /* Clear up status field */
+ regmap_write(cd->regmap, cd->touch_data_addr, 0);
+
+out:
+ return IRQ_HANDLED;
+}
+
+static int goodix_berlin_input_dev_config(struct goodix_berlin_core *cd,
+ const struct input_id *id)
+{
+ struct input_dev *input_dev;
+ int error;
+
+ input_dev = devm_input_allocate_device(cd->dev);
+ if (!input_dev)
+ return -ENOMEM;
+
+ cd->input_dev = input_dev;
+ input_set_drvdata(input_dev, cd);
+
+ input_dev->name = "Goodix Berlin Capacitive TouchScreen";
+ input_dev->phys = "input/ts";
+
+ input_dev->id = *id;
+
+ input_set_abs_params(cd->input_dev, ABS_MT_POSITION_X,
+ 0, SZ_64K - 1, 0, 0);
+ input_set_abs_params(cd->input_dev, ABS_MT_POSITION_Y,
+ 0, SZ_64K - 1, 0, 0);
+ input_set_abs_params(cd->input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
+
+ touchscreen_parse_properties(cd->input_dev, true, &cd->props);
+
+ error = input_mt_init_slots(cd->input_dev, GOODIX_BERLIN_MAX_TOUCH,
+ INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
+ if (error)
+ return error;
+
+ error = input_register_device(cd->input_dev);
+ if (error)
+ return error;
+
+ return 0;
+}
+
+static int goodix_berlin_suspend(struct device *dev)
+{
+ struct goodix_berlin_core *cd = dev_get_drvdata(dev);
+
+ disable_irq(cd->irq);
+ goodix_berlin_power_off(cd);
+
+ return 0;
+}
+
+static int goodix_berlin_resume(struct device *dev)
+{
+ struct goodix_berlin_core *cd = dev_get_drvdata(dev);
+ int error;
+
+ error = goodix_berlin_power_on(cd);
+ if (error)
+ return error;
+
+ enable_irq(cd->irq);
+
+ return 0;
+}
+
+EXPORT_GPL_SIMPLE_DEV_PM_OPS(goodix_berlin_pm_ops,
+ goodix_berlin_suspend, goodix_berlin_resume);
+
+static void goodix_berlin_power_off_act(void *data)
+{
+ struct goodix_berlin_core *cd = data;
+
+ goodix_berlin_power_off(cd);
+}
+
+int goodix_berlin_probe(struct device *dev, int irq, const struct input_id *id,
+ struct regmap *regmap)
+{
+ struct goodix_berlin_core *cd;
+ int error;
+
+ if (irq <= 0) {
+ dev_err(dev, "Missing interrupt number\n");
+ return -EINVAL;
+ }
+
+ cd = devm_kzalloc(dev, sizeof(*cd), GFP_KERNEL);
+ if (!cd)
+ return -ENOMEM;
+
+ cd->dev = dev;
+ cd->regmap = regmap;
+ cd->irq = irq;
+
+ cd->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+ if (IS_ERR(cd->reset_gpio))
+ return dev_err_probe(dev, PTR_ERR(cd->reset_gpio),
+ "Failed to request reset gpio\n");
+
+ cd->avdd = devm_regulator_get(dev, "avdd");
+ if (IS_ERR(cd->avdd))
+ return dev_err_probe(dev, PTR_ERR(cd->avdd),
+ "Failed to request avdd regulator\n");
+
+ cd->iovdd = devm_regulator_get(dev, "iovdd");
+ if (IS_ERR(cd->iovdd))
+ return dev_err_probe(dev, PTR_ERR(cd->iovdd),
+ "Failed to request iovdd regulator\n");
+
+ error = goodix_berlin_power_on(cd);
+ if (error) {
+ dev_err(dev, "failed power on");
+ return error;
+ }
+
+ error = devm_add_action_or_reset(dev, goodix_berlin_power_off_act, cd);
+ if (error)
+ return error;
+
+ error = goodix_berlin_read_version(cd);
+ if (error) {
+ dev_err(dev, "failed to get version info");
+ return error;
+ }
+
+ error = goodix_berlin_get_ic_info(cd);
+ if (error) {
+ dev_err(dev, "invalid ic info, abort");
+ return error;
+ }
+
+ error = goodix_berlin_input_dev_config(cd, id);
+ if (error) {
+ dev_err(dev, "failed set input device");
+ return error;
+ }
+
+ error = devm_request_threaded_irq(dev, cd->irq, NULL, goodix_berlin_irq,
+ IRQF_ONESHOT, "goodix-berlin", cd);
+ if (error) {
+ dev_err(dev, "request threaded irq failed: %d\n", error);
+ return error;
+ }
+
+ dev_set_drvdata(dev, cd);
+
+ dev_dbg(dev, "Goodix Berlin %s Touchscreen Controller",
+ cd->fw_version.patch_pid);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(goodix_berlin_probe);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Goodix Berlin Core Touchscreen driver");
+MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>");
--
2.34.1
^ permalink raw reply related
* [PATCH v15 1/4] dt-bindings: input: document Goodix Berlin Touchscreen IC
From: Neil Armstrong @ 2024-01-29 9:16 UTC (permalink / raw)
To: Dmitry Torokhov, linux-input
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bastien Nocera,
Hans de Goede, Henrik Rydberg, Jeff LaBundy, linux-arm-msm,
devicetree, linux-kernel, Neil Armstrong, Rob Herring
In-Reply-To: <20240129-topic-goodix-berlin-upstream-initial-v15-0-6f7d096c0a0a@linaro.org>
Document the Goodix GT9916 wich is part of the "Berlin" serie
of Touchscreen controllers IC from Goodix.
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
.../bindings/input/touchscreen/goodix,gt9916.yaml | 95 ++++++++++++++++++++++
1 file changed, 95 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix,gt9916.yaml b/Documentation/devicetree/bindings/input/touchscreen/goodix,gt9916.yaml
new file mode 100644
index 000000000000..d90f045ac06c
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/goodix,gt9916.yaml
@@ -0,0 +1,95 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/touchscreen/goodix,gt9916.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Goodix Berlin series touchscreen controller
+
+description: The Goodix Berlin series of touchscreen controllers
+ be connected to either I2C or SPI buses.
+
+maintainers:
+ - Neil Armstrong <neil.armstrong@linaro.org>
+
+allOf:
+ - $ref: touchscreen.yaml#
+ - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+ compatible:
+ enum:
+ - goodix,gt9916
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ reset-gpios:
+ maxItems: 1
+
+ avdd-supply:
+ description: Analog power supply regulator on AVDD pin
+
+ vddio-supply:
+ description: power supply regulator on VDDIO pin
+
+ spi-max-frequency: true
+ touchscreen-inverted-x: true
+ touchscreen-inverted-y: true
+ touchscreen-size-x: true
+ touchscreen-size-y: true
+ touchscreen-swapped-x-y: true
+
+additionalProperties: false
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - avdd-supply
+ - touchscreen-size-x
+ - touchscreen-size-y
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+ #include <dt-bindings/gpio/gpio.h>
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ touchscreen@5d {
+ compatible = "goodix,gt9916";
+ reg = <0x5d>;
+ interrupt-parent = <&gpio>;
+ interrupts = <25 IRQ_TYPE_LEVEL_LOW>;
+ reset-gpios = <&gpio1 1 GPIO_ACTIVE_LOW>;
+ avdd-supply = <&ts_avdd>;
+ touchscreen-size-x = <1024>;
+ touchscreen-size-y = <768>;
+ };
+ };
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+ #include <dt-bindings/gpio/gpio.h>
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ num-cs = <1>;
+ cs-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;
+ touchscreen@0 {
+ compatible = "goodix,gt9916";
+ reg = <0>;
+ interrupt-parent = <&gpio>;
+ interrupts = <25 IRQ_TYPE_LEVEL_LOW>;
+ reset-gpios = <&gpio1 1 GPIO_ACTIVE_LOW>;
+ avdd-supply = <&ts_avdd>;
+ spi-max-frequency = <1000000>;
+ touchscreen-size-x = <1024>;
+ touchscreen-size-y = <768>;
+ };
+ };
+
+...
--
2.34.1
^ permalink raw reply related
* [PATCH v15 0/4] Input: add initial support for Goodix Berlin touchscreen IC
From: Neil Armstrong @ 2024-01-29 9:16 UTC (permalink / raw)
To: Dmitry Torokhov, linux-input
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bastien Nocera,
Hans de Goede, Henrik Rydberg, Jeff LaBundy, linux-arm-msm,
devicetree, linux-kernel, Neil Armstrong, Rob Herring
These touchscreen ICs support SPI, I2C and I3C interface, up to
10 finger touch, stylus and gestures events.
This initial driver is derived from the Goodix goodix_ts_berlin
available at [1] and [2] and only supports the GT9916 IC
present on the Qualcomm SM8550/SM8650 MTP & QRD touch panel.
The current implementation only supports BerlinD, aka GT9916.
Support for advanced features like:
- Firmware & config update
- Stylus events
- Gestures events
- Previous revisions support (BerlinA or BerlinB)
is not included in current version.
The current support will work with currently flashed firmware
and config, and bail out if firmware or config aren't flashed yet.
[1] https://github.com/goodix/goodix_ts_berlin
[2] https://git.codelinaro.org/clo/la/platform/vendor/opensource/touch-drivers
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
Changes in v15:
- Applied changes/cleanups/fixes provided by Dmitry:
- Cleanup status/offsets defines, move in to structs
- Remove packed attribute when not needed
- Refactor touch data & header structs
- Store event into goodix_berlin_core
- simplify goodix_berlin_checksum_valid()
- simplify goodix_berlin_convert_ic_info() with macro
- simplify back-and-forth reads with goodix_berlin_get_remaining_contacts()
- simplify goodix_berlin_touch_handler()
- cleanup buffer handling comment
- clear up status field in case of checksum error
- Link to v14: https://lore.kernel.org/r/20231221-topic-goodix-berlin-upstream-initial-v14-0-04459853b640@linaro.org
Changes in v14:
- Rebased on next-20231221
- Fixed build on SPI/I2S drivers missing <linux/input.h> include
- Link to v13: https://lore.kernel.org/r/20231213-topic-goodix-berlin-upstream-initial-v13-0-5d7a26a5eaa2@linaro.org
Changes in v13:
- Thanks to Dmitry's suggestion:
- Move core defines and structs into goodix_berlin_core.c
- Comments typos and rephrasings
- Identation fixes
- Refactor of goodix_berlin_power_on
- goodix_berlin_get_ic_info move to __free(kfree) to avoid leaking
- Added comment explaining how data is retrieved in irq handler
- Link to v12: https://lore.kernel.org/r/20231209-topic-goodix-berlin-upstream-initial-v12-0-eaffaeb53fb5@linaro.org
Changes in v12:
- Rebased on next-20231208
- Link to v11: https://lore.kernel.org/r/20231106-topic-goodix-berlin-upstream-initial-v11-0-5c47e9707c03@linaro.org
Changes in v11:
- Fixes according to Jeff's review:
- s/dev_info/dev_err/ in goodix_berlin_get_ic_info()
- remove spurious return instead of goto in goodix_berlin_get_ic_info()
- added back dropped msleep() in goodix_berlin_request_handle_reset()
- Link to v10: https://lore.kernel.org/r/20231023-topic-goodix-berlin-upstream-initial-v10-0-88eec2e51c0b@linaro.org
Changes in v10:
- Fix according to Dmitry's review:
- move goodix_berlin_get_ic_info() afe_data to heap
- merge the goodix_berlin_parse_finger() loops and skip invalid fingers instead of returning
- remove unwanted goodix_berlin_touch_handler() "static" for buffer
- only call goodix_berlin_request_handle_reset() if gpio was provided
- use "error = func(); if(error) return error;" instead of "return func()" when function handles multiple error cases
- Link to v9: https://lore.kernel.org/r/20231021-topic-goodix-berlin-upstream-initial-v9-0-13fb4e887156@linaro.org
Changes in v9:
- Rebased on next-20231020
- Link to v8: https://lore.kernel.org/r/20231003-topic-goodix-berlin-upstream-initial-v8-0-171606102ed6@linaro.org
Changes in v8:
- Add missing bitfield.h include in core
- Link to v7: https://lore.kernel.org/r/20231002-topic-goodix-berlin-upstream-initial-v7-0-792fb91f5e88@linaro.org
Changes in v7:
- rebased on v6.6-rc3
- Link to v6: https://lore.kernel.org/r/20230912-topic-goodix-berlin-upstream-initial-v6-0-b4ecfa49fb9d@linaro.org
Changes in v6:
- rebased on v6.6-rc1
- changed commit message prefix to match the other Input commits
- Link to v5: https://lore.kernel.org/r/20230801-topic-goodix-berlin-upstream-initial-v5-0-079252935593@linaro.org
Changes in v5:
- rebased on next-20230801
- Link to v4: https://lore.kernel.org/r/20230606-topic-goodix-berlin-upstream-initial-v4-0-0947c489be17@linaro.org
Changes in v4:
- Core updates:
- drop kconfig depends, deps will be handled by _SPI and _I2C
- change power_on() error labels
- print errors on all dev_err() prints
- remove useless default variable initialization
- switch irq touch checksum error to dev_err()
- add Jeff's review tag
- I2C changes
- change REGMAP_I2C Kconfig from depends to select
- add Jeff's review tag
- SPI changes
- add select REGMAP to Kconfig
- added GOODIX_BERLIN_ prefix to defines
- switched from ret to error
- add Jeff's review tag
- Link to v3: https://lore.kernel.org/r/20230606-topic-goodix-berlin-upstream-initial-v3-0-f0577cead709@linaro.org
Changes in v3:
- Another guge cleanups after Jeff's review:
- appended goodix_berlin_ before all defines
- removed some unused defines
- removed retries on most of read functions, can be added back later
- added __le to ic_info structures
- reworked and simplified irq handling, dropped enum and ts_event structs
- added struct for touch data
- simplified and cleaned goodix_berlin_check_checksum & goodix_berlin_is_dummy_data
- moved touch_data_addr to the end of the main code_data
- reworked probe to get_irq last and right before setip input device
- cleaned probe by removing the "cd->dev"
- added short paragraph to justify new driver for berlin devices
- defined all offsets & masks
- Added bindings review tag
- Link to v2: https://lore.kernel.org/r/20230606-topic-goodix-berlin-upstream-initial-v2-0-26bc8fe1e90e@linaro.org
Changes in v2:
- Huge cleanups after Jeff's review:
- switch to error instead of ret
- drop dummy vendor/product ids
- drop unused defined/enums
- drop unused ic_info and only keep needes values
- cleanup namings and use goodix_berlin_ everywhere
- fix regulator setup
- fix default variables value when assigned afterwars
- removed indirections
- dropped debugfs
- cleaned input_dev setup
- dropped _remove()
- sync'ed i2c and spi drivers
- fixed yaml bindings
- Link to v1: https://lore.kernel.org/r/20230606-topic-goodix-berlin-upstream-initial-v1-0-4a0741b8aefd@linaro.org
---
Neil Armstrong (4):
dt-bindings: input: document Goodix Berlin Touchscreen IC
Input: add core support for Goodix Berlin Touchscreen IC
Input: goodix-berlin - add I2C support for Goodix Berlin Touchscreen IC
Input: goodix-berlin - add SPI support for Goodix Berlin Touchscreen IC
.../bindings/input/touchscreen/goodix,gt9916.yaml | 95 +++
drivers/input/touchscreen/Kconfig | 31 +
drivers/input/touchscreen/Makefile | 3 +
drivers/input/touchscreen/goodix_berlin.h | 24 +
drivers/input/touchscreen/goodix_berlin_core.c | 755 +++++++++++++++++++++
drivers/input/touchscreen/goodix_berlin_i2c.c | 75 ++
drivers/input/touchscreen/goodix_berlin_spi.c | 178 +++++
7 files changed, 1161 insertions(+)
---
base-commit: 596764183be8ebb13352b281a442a1f1151c9b06
change-id: 20230606-topic-goodix-berlin-upstream-initial-ba97e8ec8f4c
Best regards,
--
Neil Armstrong <neil.armstrong@linaro.org>
^ permalink raw reply
* Re: [PATCH v14 2/4] Input: add core support for Goodix Berlin Touchscreen IC
From: Neil Armstrong @ 2024-01-29 9:02 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-input, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Bastien Nocera, Hans de Goede, Henrik Rydberg, Jeff LaBundy,
linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <98ed4ee9-f381-43a0-a5cc-523ad108b374@linaro.org>
On 12/01/2024 16:08, Neil Armstrong wrote:
> Hi Dmitry,
>
> On 11/01/2024 08:20, Dmitry Torokhov wrote:
>> Hi Neil,
>>
>> On Thu, Dec 21, 2023 at 04:21:20PM +0100, Neil Armstrong wrote:
>>> Add initial support for the new Goodix "Berlin" touchscreen ICs.
>>
>> Thank you very much for explaining how reading of additional contacts
>> and checksum works, it makes sense now.
>>
>> I was a bit unhappy about number of times we copy/move the data over;
>> could you please try the patch below to see if the device still works
>> with it?
>
> Sure, I'll test it and report you.
Ok it still works fine!
Sending v15 with those changes and rebased on latest linux-next.
Thanks,
Neil
>
>>
>> I also shortened some #defines and defines some additional structures.
>> Also as far as I can see not everything needs to be packed as the data
>> is naturally aligned on the word boundaries.
>
> Great, thank!
>
> Neil
>
>>
>> Thanks!
>>
>
^ permalink raw reply
* RE: [PATCH v6 0/8] Convert DA906{1,2} bindings to json-schema
From: Biju Das @ 2024-01-29 8:29 UTC (permalink / raw)
To: Lee Jones, Wim Van Sebroeck, Guenter Roeck, Dmitry Torokhov,
Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: Support Opensource, Rafael J. Wysocki, Daniel Lezcano, Zhang Rui,
Lukasz Luba, Steve Twiss, linux-input@vger.kernel.org,
devicetree@vger.kernel.org, linux-pm@vger.kernel.org,
linux-watchdog@vger.kernel.org, Geert Uytterhoeven,
Prabhakar Mahadev Lad, biju.das.au,
linux-renesas-soc@vger.kernel.org
In-Reply-To: <20231221143318.GH10102@google.com>
Hi Lee Jones,
> -----Original Message-----
> From: Lee Jones <lee@kernel.org>
> Sent: Thursday, December 21, 2023 2:33 PM
> Subject: Re: [PATCH v6 0/8] Convert DA906{1,2} bindings to json-schema
>
> On Thu, 21 Dec 2023, Lee Jones wrote:
>
> > On Thu, 14 Dec 2023 08:09:03 +0000, Biju Das wrote:
> > > Convert the below bindings to json-schema
> > > 1) DA906{1,2} mfd bindings
> > > 2) DA906{1,2,3} onkey bindings
> > > 3) DA906{1,2,3} thermal bindings
> > >
> > > Also add fallback for DA9061 watchdog device and document
> > > DA9063 watchdog device.
> > >
> > > [...]
> >
> > Applied, thanks!
> >
> > [1/8] dt-bindings: mfd: da9062: Update watchdog description
> > commit: 9e7b13b805bcbe5335c2936d4c7ea0323ac69a81
> > [2/8] dt-bindings: watchdog: dlg,da9062-watchdog: Add fallback for
> DA9061 watchdog
> > commit: 28d34db7772f18490b52328f04a3bf69ed5390d2
> > [3/8] dt-bindings: watchdog: dlg,da9062-watchdog: Document DA9063
> watchdog
> > commit: d2a7dbb808870c17cffa2749ea877f61f298d098
> > [4/8] dt-bindings: mfd: dlg,da9063: Update watchdog child node
> > commit: d4018547a15a94c7e865b2cef82bff1cd43a32b3
> > [5/8] dt-bindings: input: Convert da906{1,2,3} onkey to json-schema
> > commit: db459d3da7bb9c37cb86897c7b321a49f8e40968
> > [6/8] dt-bindings: thermal: Convert da906{1,2} thermal to json-schema
> > commit: 998f499c843e360bcd9ee1fe9addc3b5d32f1234
> > [7/8] dt-bindings: mfd: dlg,da9063: Sort child devices
> > commit: 2bbf9d2a8e3bc933703dfda87cac953bed458496
> > [8/8] dt-bindings: mfd: dlg,da9063: Convert da9062 to json-schema
> > commit: 522225161830f6a93f2aaabda99226c1ffddc8c4
>
> Submitted for testing. Pull-request to follow.
The commit dc805ea058c0e ("MAINTAINERS: rectify entry for DIALOG SEMICONDUCTOR DRIVERS")
in mainline will give a conflict for patch#1.
Patch#2 and patch#3 are already in mainline.
Please let me know if you want me to rebase and resend the patch series
Or
you will take care the MAINTAINERS conflict while applying.
Cheers,
Biju
^ permalink raw reply
* psmouse touchpad bus support on kernel 6.5.0
From: Xavier Distelzweig @ 2024-01-29 2:01 UTC (permalink / raw)
To: linux-input
psmouse informed me that my touchpad could support a different bus, to
try setting psmouse.synaptics_intertouch to 1, and to email you. I'm
not sure what system information you would want, but here's neofetch:
`.:/ossyyyysso/:. xavier@khalat-ir
.:oyyyyyyyyyyyyyyyyyyo:` ----------------
-oyyyyyyyodMMyyyyyyyysyyyyo- OS: Kubuntu 22.04.3 LTS x86_64
-syyyyyyyyyydMMyoyyyydmMMyyyyys- Host: HP Elite Dragonfly
oyyysdMysyyyydMMMMMMMMMMMMMyyyyyyyo Kernel: 6.5.0-15-generic
`oyyyydMMMMysyysoooooodMMMMyyyyyyyyyo` Uptime: 4 mins
oyyyyyydMMMMyyyyyyyyyyyysdMMysssssyyyo Packages: 2498 (dpkg), 13 (snap)
-yyyyyyyydMysyyyyyyyyyyyyyysdMMMMMysyyy- Shell: bash 5.1.16
oyyyysoodMyyyyyyyyyyyyyyyyyyydMMMMysyyyo Resolution: 1920x1080
yyysdMMMMMyyyyyyyyyyyyyyyyyyysosyyyyyyyy DE: Plasma 5.27.10
yyysdMMMMMyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy WM: kwin
oyyyyysosdyyyyyyyyyyyyyyyyyyydMMMMysyyyo Theme: [Plasma], Breeze [GTK2/3]
-yyyyyyyydMysyyyyyyyyyyyyyysdMMMMMysyyy- Icons: [Plasma],
breeze-dark [GTK2/3]
oyyyyyydMMMysyyyyyyyyyyysdMMyoyyyoyyyo Terminal: yakuake
`oyyyydMMMysyyyoooooodMMMMyoyyyyyyyyo CPU: Intel i5-8265U (8) @ 3.900GHz
oyyysyyoyyyysdMMMMMMMMMMMyyyyyyyyo GPU: Intel WhiskeyLake-U GT2
[UHD Graphics 620]
-syyyyyyyyydMMMysyyydMMMysyyyys- Memory: 2239MiB / 15328MiB
-oyyyyyyydMMyyyyyyysosyyyyo-
./oyyyyyyyyyyyyyyyyyyo/.
`.:/oosyyyysso/:.`
Here's a link to my /var/log/syslog (on google drive):
https://drive.google.com/file/d/1vUGHHlfBgSJQQ1dcNBGxj0asZzMp8vPl/view?usp=drive_link
Today was the second time (since getting this computer and installing
Kubuntu) that my keyboard/touchpad assembly appeared not to work at
all, even after a reboot. (both this time and last, it started working
again, I know not why. That's how I'm typing this email.) Are these
connected in any way?
Thank you so much for your work,
Xavier Distelzweig
Xavier
^ permalink raw reply
* [PATCH bpf-next v4 3/3] bpf: treewide: Annotate BPF kfuncs in BTF
From: Daniel Xu @ 2024-01-29 1:24 UTC (permalink / raw)
To: mhiramat, daniel, edumazet, fw, hannes, tytso, ast, eddyz87, kuba,
tj, steffen.klassert, yonghong.song, hawk, rostedt,
john.fastabend, pablo, pabeni, jikos, davem, alexandre.torgue,
Herbert Xu, song, dsahern, mcoquelin.stm32, corbet, lizefan.x,
andrii, martin.lau, benjamin.tissoires, ebiggers, kadlec, shuah,
alexei.starovoitov, olsajiri, quentin, alan.maguire, memxor
Cc: kpsingh, sdf, haoluo, jolsa, mathieu.desnoyers, mykolal, bpf,
linux-doc, linux-kernel, linux-input, fsverity, cgroups,
linux-trace-kernel, netdev, netfilter-devel, coreteam,
linux-kselftest, linux-stm32, linux-arm-kernel
In-Reply-To: <cover.1706491398.git.dxu@dxuuu.xyz>
This commit marks kfuncs as such inside the .BTF_ids section. The upshot
of these annotations is that we'll be able to automatically generate
kfunc prototypes for downstream users. The process is as follows:
1. In source, use BTF_KFUNCS_START/END macro pair to mark kfuncs
2. During build, pahole injects into BTF a "bpf_kfunc" BTF_DECL_TAG for
each function inside BTF_KFUNCS sets
3. At runtime, vmlinux or module BTF is made available in sysfs
4. At runtime, bpftool (or similar) can look at provided BTF and
generate appropriate prototypes for functions with "bpf_kfunc" tag
To ensure future kfunc are similarly tagged, we now also return error
inside kfunc registration for untagged kfuncs. For vmlinux kfuncs,
we also WARN(), as initcall machinery does not handle errors.
Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
---
Documentation/bpf/kfuncs.rst | 8 ++++----
drivers/hid/bpf/hid_bpf_dispatch.c | 8 ++++----
fs/verity/measure.c | 4 ++--
kernel/bpf/btf.c | 8 ++++++++
kernel/bpf/cpumask.c | 4 ++--
kernel/bpf/helpers.c | 8 ++++----
kernel/bpf/map_iter.c | 4 ++--
kernel/cgroup/rstat.c | 4 ++--
kernel/trace/bpf_trace.c | 8 ++++----
net/bpf/test_run.c | 8 ++++----
net/core/filter.c | 20 +++++++++----------
net/core/xdp.c | 4 ++--
net/ipv4/bpf_tcp_ca.c | 4 ++--
net/ipv4/fou_bpf.c | 4 ++--
net/ipv4/tcp_bbr.c | 4 ++--
net/ipv4/tcp_cubic.c | 4 ++--
net/ipv4/tcp_dctcp.c | 4 ++--
net/netfilter/nf_conntrack_bpf.c | 4 ++--
net/netfilter/nf_nat_bpf.c | 4 ++--
net/xfrm/xfrm_interface_bpf.c | 4 ++--
net/xfrm/xfrm_state_bpf.c | 4 ++--
.../selftests/bpf/bpf_testmod/bpf_testmod.c | 8 ++++----
22 files changed, 70 insertions(+), 62 deletions(-)
diff --git a/Documentation/bpf/kfuncs.rst b/Documentation/bpf/kfuncs.rst
index 7985c6615f3c..a8f5782bd833 100644
--- a/Documentation/bpf/kfuncs.rst
+++ b/Documentation/bpf/kfuncs.rst
@@ -177,10 +177,10 @@ In addition to kfuncs' arguments, verifier may need more information about the
type of kfunc(s) being registered with the BPF subsystem. To do so, we define
flags on a set of kfuncs as follows::
- BTF_SET8_START(bpf_task_set)
+ BTF_KFUNCS_START(bpf_task_set)
BTF_ID_FLAGS(func, bpf_get_task_pid, KF_ACQUIRE | KF_RET_NULL)
BTF_ID_FLAGS(func, bpf_put_pid, KF_RELEASE)
- BTF_SET8_END(bpf_task_set)
+ BTF_KFUNCS_END(bpf_task_set)
This set encodes the BTF ID of each kfunc listed above, and encodes the flags
along with it. Ofcourse, it is also allowed to specify no flags.
@@ -347,10 +347,10 @@ Once the kfunc is prepared for use, the final step to making it visible is
registering it with the BPF subsystem. Registration is done per BPF program
type. An example is shown below::
- BTF_SET8_START(bpf_task_set)
+ BTF_KFUNCS_START(bpf_task_set)
BTF_ID_FLAGS(func, bpf_get_task_pid, KF_ACQUIRE | KF_RET_NULL)
BTF_ID_FLAGS(func, bpf_put_pid, KF_RELEASE)
- BTF_SET8_END(bpf_task_set)
+ BTF_KFUNCS_END(bpf_task_set)
static const struct btf_kfunc_id_set bpf_task_kfunc_set = {
.owner = THIS_MODULE,
diff --git a/drivers/hid/bpf/hid_bpf_dispatch.c b/drivers/hid/bpf/hid_bpf_dispatch.c
index d9ef45fcaeab..02c441aaa217 100644
--- a/drivers/hid/bpf/hid_bpf_dispatch.c
+++ b/drivers/hid/bpf/hid_bpf_dispatch.c
@@ -172,9 +172,9 @@ hid_bpf_get_data(struct hid_bpf_ctx *ctx, unsigned int offset, const size_t rdwr
* The following set contains all functions we agree BPF programs
* can use.
*/
-BTF_SET8_START(hid_bpf_kfunc_ids)
+BTF_KFUNCS_START(hid_bpf_kfunc_ids)
BTF_ID_FLAGS(func, hid_bpf_get_data, KF_RET_NULL)
-BTF_SET8_END(hid_bpf_kfunc_ids)
+BTF_KFUNCS_END(hid_bpf_kfunc_ids)
static const struct btf_kfunc_id_set hid_bpf_kfunc_set = {
.owner = THIS_MODULE,
@@ -440,12 +440,12 @@ static const struct btf_kfunc_id_set hid_bpf_fmodret_set = {
};
/* for syscall HID-BPF */
-BTF_SET8_START(hid_bpf_syscall_kfunc_ids)
+BTF_KFUNCS_START(hid_bpf_syscall_kfunc_ids)
BTF_ID_FLAGS(func, hid_bpf_attach_prog)
BTF_ID_FLAGS(func, hid_bpf_allocate_context, KF_ACQUIRE | KF_RET_NULL)
BTF_ID_FLAGS(func, hid_bpf_release_context, KF_RELEASE)
BTF_ID_FLAGS(func, hid_bpf_hw_request)
-BTF_SET8_END(hid_bpf_syscall_kfunc_ids)
+BTF_KFUNCS_END(hid_bpf_syscall_kfunc_ids)
static const struct btf_kfunc_id_set hid_bpf_syscall_kfunc_set = {
.owner = THIS_MODULE,
diff --git a/fs/verity/measure.c b/fs/verity/measure.c
index bf7a5f4cccaf..3969d54158d1 100644
--- a/fs/verity/measure.c
+++ b/fs/verity/measure.c
@@ -159,9 +159,9 @@ __bpf_kfunc int bpf_get_fsverity_digest(struct file *file, struct bpf_dynptr_ker
__bpf_kfunc_end_defs();
-BTF_SET8_START(fsverity_set_ids)
+BTF_KFUNCS_START(fsverity_set_ids)
BTF_ID_FLAGS(func, bpf_get_fsverity_digest, KF_TRUSTED_ARGS)
-BTF_SET8_END(fsverity_set_ids)
+BTF_KFUNCS_END(fsverity_set_ids)
static int bpf_get_fsverity_digest_filter(const struct bpf_prog *prog, u32 kfunc_id)
{
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index edef96ceffa3..bc446f37530c 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -8041,6 +8041,14 @@ int register_btf_kfunc_id_set(enum bpf_prog_type prog_type,
{
enum btf_kfunc_hook hook;
+ /* All kfuncs need to be tagged as such in BTF.
+ * WARN() for initcall registrations that do not check errors.
+ */
+ if (!(kset->set->flags & BTF_SET8_KFUNCS)) {
+ WARN_ON(!kset->owner);
+ return -EINVAL;
+ }
+
hook = bpf_prog_type_to_kfunc_hook(prog_type);
return __register_btf_kfunc_id_set(hook, kset);
}
diff --git a/kernel/bpf/cpumask.c b/kernel/bpf/cpumask.c
index 2e73533a3811..dad0fb1c8e87 100644
--- a/kernel/bpf/cpumask.c
+++ b/kernel/bpf/cpumask.c
@@ -424,7 +424,7 @@ __bpf_kfunc u32 bpf_cpumask_weight(const struct cpumask *cpumask)
__bpf_kfunc_end_defs();
-BTF_SET8_START(cpumask_kfunc_btf_ids)
+BTF_KFUNCS_START(cpumask_kfunc_btf_ids)
BTF_ID_FLAGS(func, bpf_cpumask_create, KF_ACQUIRE | KF_RET_NULL)
BTF_ID_FLAGS(func, bpf_cpumask_release, KF_RELEASE)
BTF_ID_FLAGS(func, bpf_cpumask_acquire, KF_ACQUIRE | KF_TRUSTED_ARGS)
@@ -450,7 +450,7 @@ BTF_ID_FLAGS(func, bpf_cpumask_copy, KF_RCU)
BTF_ID_FLAGS(func, bpf_cpumask_any_distribute, KF_RCU)
BTF_ID_FLAGS(func, bpf_cpumask_any_and_distribute, KF_RCU)
BTF_ID_FLAGS(func, bpf_cpumask_weight, KF_RCU)
-BTF_SET8_END(cpumask_kfunc_btf_ids)
+BTF_KFUNCS_END(cpumask_kfunc_btf_ids)
static const struct btf_kfunc_id_set cpumask_kfunc_set = {
.owner = THIS_MODULE,
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index bcb951a2ecf4..4db1c658254c 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -2544,7 +2544,7 @@ __bpf_kfunc void bpf_throw(u64 cookie)
__bpf_kfunc_end_defs();
-BTF_SET8_START(generic_btf_ids)
+BTF_KFUNCS_START(generic_btf_ids)
#ifdef CONFIG_KEXEC_CORE
BTF_ID_FLAGS(func, crash_kexec, KF_DESTRUCTIVE)
#endif
@@ -2573,7 +2573,7 @@ BTF_ID_FLAGS(func, bpf_task_get_cgroup1, KF_ACQUIRE | KF_RCU | KF_RET_NULL)
#endif
BTF_ID_FLAGS(func, bpf_task_from_pid, KF_ACQUIRE | KF_RET_NULL)
BTF_ID_FLAGS(func, bpf_throw)
-BTF_SET8_END(generic_btf_ids)
+BTF_KFUNCS_END(generic_btf_ids)
static const struct btf_kfunc_id_set generic_kfunc_set = {
.owner = THIS_MODULE,
@@ -2589,7 +2589,7 @@ BTF_ID(struct, cgroup)
BTF_ID(func, bpf_cgroup_release_dtor)
#endif
-BTF_SET8_START(common_btf_ids)
+BTF_KFUNCS_START(common_btf_ids)
BTF_ID_FLAGS(func, bpf_cast_to_kern_ctx)
BTF_ID_FLAGS(func, bpf_rdonly_cast)
BTF_ID_FLAGS(func, bpf_rcu_read_lock)
@@ -2618,7 +2618,7 @@ BTF_ID_FLAGS(func, bpf_dynptr_is_null)
BTF_ID_FLAGS(func, bpf_dynptr_is_rdonly)
BTF_ID_FLAGS(func, bpf_dynptr_size)
BTF_ID_FLAGS(func, bpf_dynptr_clone)
-BTF_SET8_END(common_btf_ids)
+BTF_KFUNCS_END(common_btf_ids)
static const struct btf_kfunc_id_set common_kfunc_set = {
.owner = THIS_MODULE,
diff --git a/kernel/bpf/map_iter.c b/kernel/bpf/map_iter.c
index 6abd7c5df4b3..9575314f40a6 100644
--- a/kernel/bpf/map_iter.c
+++ b/kernel/bpf/map_iter.c
@@ -213,9 +213,9 @@ __bpf_kfunc s64 bpf_map_sum_elem_count(const struct bpf_map *map)
__bpf_kfunc_end_defs();
-BTF_SET8_START(bpf_map_iter_kfunc_ids)
+BTF_KFUNCS_START(bpf_map_iter_kfunc_ids)
BTF_ID_FLAGS(func, bpf_map_sum_elem_count, KF_TRUSTED_ARGS)
-BTF_SET8_END(bpf_map_iter_kfunc_ids)
+BTF_KFUNCS_END(bpf_map_iter_kfunc_ids)
static const struct btf_kfunc_id_set bpf_map_iter_kfunc_set = {
.owner = THIS_MODULE,
diff --git a/kernel/cgroup/rstat.c b/kernel/cgroup/rstat.c
index a8350d2d63e6..07e2284bb499 100644
--- a/kernel/cgroup/rstat.c
+++ b/kernel/cgroup/rstat.c
@@ -562,10 +562,10 @@ void cgroup_base_stat_cputime_show(struct seq_file *seq)
}
/* Add bpf kfuncs for cgroup_rstat_updated() and cgroup_rstat_flush() */
-BTF_SET8_START(bpf_rstat_kfunc_ids)
+BTF_KFUNCS_START(bpf_rstat_kfunc_ids)
BTF_ID_FLAGS(func, cgroup_rstat_updated)
BTF_ID_FLAGS(func, cgroup_rstat_flush, KF_SLEEPABLE)
-BTF_SET8_END(bpf_rstat_kfunc_ids)
+BTF_KFUNCS_END(bpf_rstat_kfunc_ids)
static const struct btf_kfunc_id_set bpf_rstat_kfunc_set = {
.owner = THIS_MODULE,
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 64fdaf79d113..241ddf5e3895 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -1412,14 +1412,14 @@ __bpf_kfunc int bpf_verify_pkcs7_signature(struct bpf_dynptr_kern *data_ptr,
__bpf_kfunc_end_defs();
-BTF_SET8_START(key_sig_kfunc_set)
+BTF_KFUNCS_START(key_sig_kfunc_set)
BTF_ID_FLAGS(func, bpf_lookup_user_key, KF_ACQUIRE | KF_RET_NULL | KF_SLEEPABLE)
BTF_ID_FLAGS(func, bpf_lookup_system_key, KF_ACQUIRE | KF_RET_NULL)
BTF_ID_FLAGS(func, bpf_key_put, KF_RELEASE)
#ifdef CONFIG_SYSTEM_DATA_VERIFICATION
BTF_ID_FLAGS(func, bpf_verify_pkcs7_signature, KF_SLEEPABLE)
#endif
-BTF_SET8_END(key_sig_kfunc_set)
+BTF_KFUNCS_END(key_sig_kfunc_set)
static const struct btf_kfunc_id_set bpf_key_sig_kfunc_set = {
.owner = THIS_MODULE,
@@ -1475,9 +1475,9 @@ __bpf_kfunc int bpf_get_file_xattr(struct file *file, const char *name__str,
__bpf_kfunc_end_defs();
-BTF_SET8_START(fs_kfunc_set_ids)
+BTF_KFUNCS_START(fs_kfunc_set_ids)
BTF_ID_FLAGS(func, bpf_get_file_xattr, KF_SLEEPABLE | KF_TRUSTED_ARGS)
-BTF_SET8_END(fs_kfunc_set_ids)
+BTF_KFUNCS_END(fs_kfunc_set_ids)
static int bpf_get_file_xattr_filter(const struct bpf_prog *prog, u32 kfunc_id)
{
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index dfd919374017..5535f9adc658 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -617,21 +617,21 @@ CFI_NOSEAL(bpf_kfunc_call_memb_release_dtor);
__bpf_kfunc_end_defs();
-BTF_SET8_START(bpf_test_modify_return_ids)
+BTF_KFUNCS_START(bpf_test_modify_return_ids)
BTF_ID_FLAGS(func, bpf_modify_return_test)
BTF_ID_FLAGS(func, bpf_modify_return_test2)
BTF_ID_FLAGS(func, bpf_fentry_test1, KF_SLEEPABLE)
-BTF_SET8_END(bpf_test_modify_return_ids)
+BTF_KFUNCS_END(bpf_test_modify_return_ids)
static const struct btf_kfunc_id_set bpf_test_modify_return_set = {
.owner = THIS_MODULE,
.set = &bpf_test_modify_return_ids,
};
-BTF_SET8_START(test_sk_check_kfunc_ids)
+BTF_KFUNCS_START(test_sk_check_kfunc_ids)
BTF_ID_FLAGS(func, bpf_kfunc_call_test_release, KF_RELEASE)
BTF_ID_FLAGS(func, bpf_kfunc_call_memb_release, KF_RELEASE)
-BTF_SET8_END(test_sk_check_kfunc_ids)
+BTF_KFUNCS_END(test_sk_check_kfunc_ids)
static void *bpf_test_init(const union bpf_attr *kattr, u32 user_size,
u32 size, u32 headroom, u32 tailroom)
diff --git a/net/core/filter.c b/net/core/filter.c
index 358870408a51..524adf1fa6d0 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -11982,21 +11982,21 @@ int bpf_dynptr_from_skb_rdonly(struct sk_buff *skb, u64 flags,
return 0;
}
-BTF_SET8_START(bpf_kfunc_check_set_skb)
+BTF_KFUNCS_START(bpf_kfunc_check_set_skb)
BTF_ID_FLAGS(func, bpf_dynptr_from_skb)
-BTF_SET8_END(bpf_kfunc_check_set_skb)
+BTF_KFUNCS_END(bpf_kfunc_check_set_skb)
-BTF_SET8_START(bpf_kfunc_check_set_xdp)
+BTF_KFUNCS_START(bpf_kfunc_check_set_xdp)
BTF_ID_FLAGS(func, bpf_dynptr_from_xdp)
-BTF_SET8_END(bpf_kfunc_check_set_xdp)
+BTF_KFUNCS_END(bpf_kfunc_check_set_xdp)
-BTF_SET8_START(bpf_kfunc_check_set_sock_addr)
+BTF_KFUNCS_START(bpf_kfunc_check_set_sock_addr)
BTF_ID_FLAGS(func, bpf_sock_addr_set_sun_path)
-BTF_SET8_END(bpf_kfunc_check_set_sock_addr)
+BTF_KFUNCS_END(bpf_kfunc_check_set_sock_addr)
-BTF_SET8_START(bpf_kfunc_check_set_tcp_reqsk)
+BTF_KFUNCS_START(bpf_kfunc_check_set_tcp_reqsk)
BTF_ID_FLAGS(func, bpf_sk_assign_tcp_reqsk, KF_TRUSTED_ARGS)
-BTF_SET8_END(bpf_kfunc_check_set_tcp_reqsk)
+BTF_KFUNCS_END(bpf_kfunc_check_set_tcp_reqsk)
static const struct btf_kfunc_id_set bpf_kfunc_set_skb = {
.owner = THIS_MODULE,
@@ -12075,9 +12075,9 @@ __bpf_kfunc int bpf_sock_destroy(struct sock_common *sock)
__bpf_kfunc_end_defs();
-BTF_SET8_START(bpf_sk_iter_kfunc_ids)
+BTF_KFUNCS_START(bpf_sk_iter_kfunc_ids)
BTF_ID_FLAGS(func, bpf_sock_destroy, KF_TRUSTED_ARGS)
-BTF_SET8_END(bpf_sk_iter_kfunc_ids)
+BTF_KFUNCS_END(bpf_sk_iter_kfunc_ids)
static int tracing_iter_filter(const struct bpf_prog *prog, u32 kfunc_id)
{
diff --git a/net/core/xdp.c b/net/core/xdp.c
index 4869c1c2d8f3..034fb80f3fbe 100644
--- a/net/core/xdp.c
+++ b/net/core/xdp.c
@@ -771,11 +771,11 @@ __bpf_kfunc int bpf_xdp_metadata_rx_vlan_tag(const struct xdp_md *ctx,
__bpf_kfunc_end_defs();
-BTF_SET8_START(xdp_metadata_kfunc_ids)
+BTF_KFUNCS_START(xdp_metadata_kfunc_ids)
#define XDP_METADATA_KFUNC(_, __, name, ___) BTF_ID_FLAGS(func, name, KF_TRUSTED_ARGS)
XDP_METADATA_KFUNC_xxx
#undef XDP_METADATA_KFUNC
-BTF_SET8_END(xdp_metadata_kfunc_ids)
+BTF_KFUNCS_END(xdp_metadata_kfunc_ids)
static const struct btf_kfunc_id_set xdp_metadata_kfunc_set = {
.owner = THIS_MODULE,
diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c
index 834edc18463a..7f518ea5f4ac 100644
--- a/net/ipv4/bpf_tcp_ca.c
+++ b/net/ipv4/bpf_tcp_ca.c
@@ -201,13 +201,13 @@ bpf_tcp_ca_get_func_proto(enum bpf_func_id func_id,
}
}
-BTF_SET8_START(bpf_tcp_ca_check_kfunc_ids)
+BTF_KFUNCS_START(bpf_tcp_ca_check_kfunc_ids)
BTF_ID_FLAGS(func, tcp_reno_ssthresh)
BTF_ID_FLAGS(func, tcp_reno_cong_avoid)
BTF_ID_FLAGS(func, tcp_reno_undo_cwnd)
BTF_ID_FLAGS(func, tcp_slow_start)
BTF_ID_FLAGS(func, tcp_cong_avoid_ai)
-BTF_SET8_END(bpf_tcp_ca_check_kfunc_ids)
+BTF_KFUNCS_END(bpf_tcp_ca_check_kfunc_ids)
static const struct btf_kfunc_id_set bpf_tcp_ca_kfunc_set = {
.owner = THIS_MODULE,
diff --git a/net/ipv4/fou_bpf.c b/net/ipv4/fou_bpf.c
index 4da03bf45c9b..06e5572f296f 100644
--- a/net/ipv4/fou_bpf.c
+++ b/net/ipv4/fou_bpf.c
@@ -100,10 +100,10 @@ __bpf_kfunc int bpf_skb_get_fou_encap(struct __sk_buff *skb_ctx,
__bpf_kfunc_end_defs();
-BTF_SET8_START(fou_kfunc_set)
+BTF_KFUNCS_START(fou_kfunc_set)
BTF_ID_FLAGS(func, bpf_skb_set_fou_encap)
BTF_ID_FLAGS(func, bpf_skb_get_fou_encap)
-BTF_SET8_END(fou_kfunc_set)
+BTF_KFUNCS_END(fou_kfunc_set)
static const struct btf_kfunc_id_set fou_bpf_kfunc_set = {
.owner = THIS_MODULE,
diff --git a/net/ipv4/tcp_bbr.c b/net/ipv4/tcp_bbr.c
index 22358032dd48..05dc2d05bc7c 100644
--- a/net/ipv4/tcp_bbr.c
+++ b/net/ipv4/tcp_bbr.c
@@ -1155,7 +1155,7 @@ static struct tcp_congestion_ops tcp_bbr_cong_ops __read_mostly = {
.set_state = bbr_set_state,
};
-BTF_SET8_START(tcp_bbr_check_kfunc_ids)
+BTF_KFUNCS_START(tcp_bbr_check_kfunc_ids)
#ifdef CONFIG_X86
#ifdef CONFIG_DYNAMIC_FTRACE
BTF_ID_FLAGS(func, bbr_init)
@@ -1168,7 +1168,7 @@ BTF_ID_FLAGS(func, bbr_min_tso_segs)
BTF_ID_FLAGS(func, bbr_set_state)
#endif
#endif
-BTF_SET8_END(tcp_bbr_check_kfunc_ids)
+BTF_KFUNCS_END(tcp_bbr_check_kfunc_ids)
static const struct btf_kfunc_id_set tcp_bbr_kfunc_set = {
.owner = THIS_MODULE,
diff --git a/net/ipv4/tcp_cubic.c b/net/ipv4/tcp_cubic.c
index 0fd78ecb67e7..44869ea089e3 100644
--- a/net/ipv4/tcp_cubic.c
+++ b/net/ipv4/tcp_cubic.c
@@ -485,7 +485,7 @@ static struct tcp_congestion_ops cubictcp __read_mostly = {
.name = "cubic",
};
-BTF_SET8_START(tcp_cubic_check_kfunc_ids)
+BTF_KFUNCS_START(tcp_cubic_check_kfunc_ids)
#ifdef CONFIG_X86
#ifdef CONFIG_DYNAMIC_FTRACE
BTF_ID_FLAGS(func, cubictcp_init)
@@ -496,7 +496,7 @@ BTF_ID_FLAGS(func, cubictcp_cwnd_event)
BTF_ID_FLAGS(func, cubictcp_acked)
#endif
#endif
-BTF_SET8_END(tcp_cubic_check_kfunc_ids)
+BTF_KFUNCS_END(tcp_cubic_check_kfunc_ids)
static const struct btf_kfunc_id_set tcp_cubic_kfunc_set = {
.owner = THIS_MODULE,
diff --git a/net/ipv4/tcp_dctcp.c b/net/ipv4/tcp_dctcp.c
index bb23bb5b387a..e33fbe4933e4 100644
--- a/net/ipv4/tcp_dctcp.c
+++ b/net/ipv4/tcp_dctcp.c
@@ -260,7 +260,7 @@ static struct tcp_congestion_ops dctcp_reno __read_mostly = {
.name = "dctcp-reno",
};
-BTF_SET8_START(tcp_dctcp_check_kfunc_ids)
+BTF_KFUNCS_START(tcp_dctcp_check_kfunc_ids)
#ifdef CONFIG_X86
#ifdef CONFIG_DYNAMIC_FTRACE
BTF_ID_FLAGS(func, dctcp_init)
@@ -271,7 +271,7 @@ BTF_ID_FLAGS(func, dctcp_cwnd_undo)
BTF_ID_FLAGS(func, dctcp_state)
#endif
#endif
-BTF_SET8_END(tcp_dctcp_check_kfunc_ids)
+BTF_KFUNCS_END(tcp_dctcp_check_kfunc_ids)
static const struct btf_kfunc_id_set tcp_dctcp_kfunc_set = {
.owner = THIS_MODULE,
diff --git a/net/netfilter/nf_conntrack_bpf.c b/net/netfilter/nf_conntrack_bpf.c
index 475358ec8212..d2492d050fe6 100644
--- a/net/netfilter/nf_conntrack_bpf.c
+++ b/net/netfilter/nf_conntrack_bpf.c
@@ -467,7 +467,7 @@ __bpf_kfunc int bpf_ct_change_status(struct nf_conn *nfct, u32 status)
__bpf_kfunc_end_defs();
-BTF_SET8_START(nf_ct_kfunc_set)
+BTF_KFUNCS_START(nf_ct_kfunc_set)
BTF_ID_FLAGS(func, bpf_xdp_ct_alloc, KF_ACQUIRE | KF_RET_NULL)
BTF_ID_FLAGS(func, bpf_xdp_ct_lookup, KF_ACQUIRE | KF_RET_NULL)
BTF_ID_FLAGS(func, bpf_skb_ct_alloc, KF_ACQUIRE | KF_RET_NULL)
@@ -478,7 +478,7 @@ BTF_ID_FLAGS(func, bpf_ct_set_timeout, KF_TRUSTED_ARGS)
BTF_ID_FLAGS(func, bpf_ct_change_timeout, KF_TRUSTED_ARGS)
BTF_ID_FLAGS(func, bpf_ct_set_status, KF_TRUSTED_ARGS)
BTF_ID_FLAGS(func, bpf_ct_change_status, KF_TRUSTED_ARGS)
-BTF_SET8_END(nf_ct_kfunc_set)
+BTF_KFUNCS_END(nf_ct_kfunc_set)
static const struct btf_kfunc_id_set nf_conntrack_kfunc_set = {
.owner = THIS_MODULE,
diff --git a/net/netfilter/nf_nat_bpf.c b/net/netfilter/nf_nat_bpf.c
index 6e3b2f58855f..481be15609b1 100644
--- a/net/netfilter/nf_nat_bpf.c
+++ b/net/netfilter/nf_nat_bpf.c
@@ -54,9 +54,9 @@ __bpf_kfunc int bpf_ct_set_nat_info(struct nf_conn___init *nfct,
__bpf_kfunc_end_defs();
-BTF_SET8_START(nf_nat_kfunc_set)
+BTF_KFUNCS_START(nf_nat_kfunc_set)
BTF_ID_FLAGS(func, bpf_ct_set_nat_info, KF_TRUSTED_ARGS)
-BTF_SET8_END(nf_nat_kfunc_set)
+BTF_KFUNCS_END(nf_nat_kfunc_set)
static const struct btf_kfunc_id_set nf_bpf_nat_kfunc_set = {
.owner = THIS_MODULE,
diff --git a/net/xfrm/xfrm_interface_bpf.c b/net/xfrm/xfrm_interface_bpf.c
index 7d5e920141e9..5ea15037ebd1 100644
--- a/net/xfrm/xfrm_interface_bpf.c
+++ b/net/xfrm/xfrm_interface_bpf.c
@@ -93,10 +93,10 @@ __bpf_kfunc int bpf_skb_set_xfrm_info(struct __sk_buff *skb_ctx, const struct bp
__bpf_kfunc_end_defs();
-BTF_SET8_START(xfrm_ifc_kfunc_set)
+BTF_KFUNCS_START(xfrm_ifc_kfunc_set)
BTF_ID_FLAGS(func, bpf_skb_get_xfrm_info)
BTF_ID_FLAGS(func, bpf_skb_set_xfrm_info)
-BTF_SET8_END(xfrm_ifc_kfunc_set)
+BTF_KFUNCS_END(xfrm_ifc_kfunc_set)
static const struct btf_kfunc_id_set xfrm_interface_kfunc_set = {
.owner = THIS_MODULE,
diff --git a/net/xfrm/xfrm_state_bpf.c b/net/xfrm/xfrm_state_bpf.c
index 9e20d4a377f7..2248eda741f8 100644
--- a/net/xfrm/xfrm_state_bpf.c
+++ b/net/xfrm/xfrm_state_bpf.c
@@ -117,10 +117,10 @@ __bpf_kfunc void bpf_xdp_xfrm_state_release(struct xfrm_state *x)
__bpf_kfunc_end_defs();
-BTF_SET8_START(xfrm_state_kfunc_set)
+BTF_KFUNCS_START(xfrm_state_kfunc_set)
BTF_ID_FLAGS(func, bpf_xdp_get_xfrm_state, KF_RET_NULL | KF_ACQUIRE)
BTF_ID_FLAGS(func, bpf_xdp_xfrm_state_release, KF_RELEASE)
-BTF_SET8_END(xfrm_state_kfunc_set)
+BTF_KFUNCS_END(xfrm_state_kfunc_set)
static const struct btf_kfunc_id_set xfrm_state_xdp_kfunc_set = {
.owner = THIS_MODULE,
diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index 8befaf17d454..53f73fc86baa 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -343,12 +343,12 @@ static struct bin_attribute bin_attr_bpf_testmod_file __ro_after_init = {
.write = bpf_testmod_test_write,
};
-BTF_SET8_START(bpf_testmod_common_kfunc_ids)
+BTF_KFUNCS_START(bpf_testmod_common_kfunc_ids)
BTF_ID_FLAGS(func, bpf_iter_testmod_seq_new, KF_ITER_NEW)
BTF_ID_FLAGS(func, bpf_iter_testmod_seq_next, KF_ITER_NEXT | KF_RET_NULL)
BTF_ID_FLAGS(func, bpf_iter_testmod_seq_destroy, KF_ITER_DESTROY)
BTF_ID_FLAGS(func, bpf_kfunc_common_test)
-BTF_SET8_END(bpf_testmod_common_kfunc_ids)
+BTF_KFUNCS_END(bpf_testmod_common_kfunc_ids)
static const struct btf_kfunc_id_set bpf_testmod_common_kfunc_set = {
.owner = THIS_MODULE,
@@ -494,7 +494,7 @@ __bpf_kfunc static u32 bpf_kfunc_call_test_static_unused_arg(u32 arg, u32 unused
return arg;
}
-BTF_SET8_START(bpf_testmod_check_kfunc_ids)
+BTF_KFUNCS_START(bpf_testmod_check_kfunc_ids)
BTF_ID_FLAGS(func, bpf_testmod_test_mod_kfunc)
BTF_ID_FLAGS(func, bpf_kfunc_call_test1)
BTF_ID_FLAGS(func, bpf_kfunc_call_test2)
@@ -520,7 +520,7 @@ BTF_ID_FLAGS(func, bpf_kfunc_call_test_ref, KF_TRUSTED_ARGS | KF_RCU)
BTF_ID_FLAGS(func, bpf_kfunc_call_test_destructive, KF_DESTRUCTIVE)
BTF_ID_FLAGS(func, bpf_kfunc_call_test_static_unused_arg)
BTF_ID_FLAGS(func, bpf_kfunc_call_test_offset)
-BTF_SET8_END(bpf_testmod_check_kfunc_ids)
+BTF_KFUNCS_END(bpf_testmod_check_kfunc_ids)
static int bpf_testmod_ops_init(struct btf *btf)
{
--
2.42.1
^ permalink raw reply related
* [PATCH bpf-next v4 0/3] Annotate kfuncs in .BTF_ids section
From: Daniel Xu @ 2024-01-29 1:24 UTC (permalink / raw)
To: linux-trace-kernel, coreteam, bpf, linux-input, cgroups, netdev,
linux-stm32, linux-kselftest, linux-doc, fsverity, linux-kernel,
linux-arm-kernel, netfilter-devel, alexei.starovoitov, olsajiri,
quentin, alan.maguire, memxor
=== Description ===
This is a bpf-treewide change that annotates all kfuncs as such inside
.BTF_ids. This annotation eventually allows us to automatically generate
kfunc prototypes from bpftool.
We store this metadata inside a yet-unused flags field inside struct
btf_id_set8 (thanks Kumar!). pahole will be taught where to look.
More details about the full chain of events are available in commit 3's
description.
The accompanying pahole and bpftool changes can be viewed
here on these "frozen" branches [0][1].
[0]: https://github.com/danobi/pahole/tree/kfunc_btf-v3-mailed
[1]: https://github.com/danobi/linux/tree/kfunc_bpftool-mailed
=== Changelog ===
Changes from v3:
* Rebase to bpf-next and add missing annotation on new kfunc
Changes from v2:
* Only WARN() for vmlinux kfuncs
Changes from v1:
* Move WARN_ON() up a call level
* Also return error when kfunc set is not properly tagged
* Use BTF_KFUNCS_START/END instead of flags
* Rename BTF_SET8_KFUNC to BTF_SET8_KFUNCS
Daniel Xu (3):
bpf: btf: Support flags for BTF_SET8 sets
bpf: btf: Add BTF_KFUNCS_START/END macro pair
bpf: treewide: Annotate BPF kfuncs in BTF
Documentation/bpf/kfuncs.rst | 8 +++----
drivers/hid/bpf/hid_bpf_dispatch.c | 8 +++----
fs/verity/measure.c | 4 ++--
include/linux/btf_ids.h | 21 +++++++++++++++----
kernel/bpf/btf.c | 8 +++++++
kernel/bpf/cpumask.c | 4 ++--
kernel/bpf/helpers.c | 8 +++----
kernel/bpf/map_iter.c | 4 ++--
kernel/cgroup/rstat.c | 4 ++--
kernel/trace/bpf_trace.c | 8 +++----
net/bpf/test_run.c | 8 +++----
net/core/filter.c | 20 +++++++++---------
net/core/xdp.c | 4 ++--
net/ipv4/bpf_tcp_ca.c | 4 ++--
net/ipv4/fou_bpf.c | 4 ++--
net/ipv4/tcp_bbr.c | 4 ++--
net/ipv4/tcp_cubic.c | 4 ++--
net/ipv4/tcp_dctcp.c | 4 ++--
net/netfilter/nf_conntrack_bpf.c | 4 ++--
net/netfilter/nf_nat_bpf.c | 4 ++--
net/xfrm/xfrm_interface_bpf.c | 4 ++--
net/xfrm/xfrm_state_bpf.c | 4 ++--
.../selftests/bpf/bpf_testmod/bpf_testmod.c | 8 +++----
23 files changed, 87 insertions(+), 66 deletions(-)
--
2.42.1
^ 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