* [PATCH] Input: goodix - Add Lenovo Yoga Book X90F to nine_bytes_report DMI table
From: Hans de Goede @ 2023-03-15 13:44 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Hans de Goede, Bastien Nocera, linux-input
The Android Lenovo Yoga Book X90F / X90L uses the same goodix touchscreen
with 9 bytes touch reports for its touch keyboard as the already supported
Windows Lenovo Yoga Book X91F/L, add a DMI match for this to
the nine_bytes_report DMI table.
When the quirk for the X91F/L was initially added it was written to
also apply to the X90F/L but this does not work because the Android
version of the Yoga Book uses completely different DMI strings.
Also adjust the X91F/L quirk to reflect that it only applies to
the X91F/L models.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/input/touchscreen/goodix.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 3c71f0df9769..88c223a4da61 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -128,10 +128,18 @@ static const unsigned long goodix_irq_flags[] = {
static const struct dmi_system_id nine_bytes_report[] = {
#if defined(CONFIG_DMI) && defined(CONFIG_X86)
{
- .ident = "Lenovo YogaBook",
- /* YB1-X91L/F and YB1-X90L/F */
+ /* Lenovo Yoga Book X90F / X90L */
.matches = {
- DMI_MATCH(DMI_PRODUCT_NAME, "Lenovo YB1-X9")
+ DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Intel Corporation"),
+ DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "CHERRYVIEW D1 PLATFORM"),
+ DMI_EXACT_MATCH(DMI_PRODUCT_VERSION, "YETI-11"),
+ }
+ },
+ {
+ /* Lenovo Yoga Book X91F / X91L */
+ .matches = {
+ /* Non exact match to match F + L versions */
+ DMI_MATCH(DMI_PRODUCT_NAME, "Lenovo YB1-X91"),
}
},
#endif
--
2.39.1
^ permalink raw reply related
* Re: [regression] Bug 217182 - Dell Latitude E7450 Trackpoint not working as expected
From: Thorsten Leemhuis @ 2023-03-15 9:50 UTC (permalink / raw)
To: Pali Rohár
Cc: Steffen, Linux kernel regressions list, Dmitry Torokhov,
open list:HID CORE LAYER, LKML, lanelone
In-Reply-To: <6625ee0a-31a7-9fef-d299-457c0f98f5a0@leemhuis.info>
On 13.03.23 11:20, Linux regression tracking (Thorsten Leemhuis) wrote:
>
> Hi, Thorsten here, the Linux kernel's regression tracker.
>
> I noticed a regression report in bugzilla.kernel.org. As many (most?)
> kernel developer don't keep an eye on it, I decided to forward it by
> mail. Quoting from https://bugzilla.kernel.org/show_bug.cgi?id=217182 :
>
>> Steffen 2023-03-12 11:17:02 UTC
>>
>> Hi,
>> I'm new to this bug tracking system and just a user, not a developer.
>>
>> I use a Dell Latitude E7450 running with the Debian unstable based
>> distribution Siduction.
>> After upgrading to Linux kernel 6.2.x the Trackpoint does not work
>> anymore as expected. It uses an Alps PS/2 DualPoint Stick.
>> Even slight touches in any direction send the cursor to the top right
>> corner of the screen where it stays regardless of actual pointer
>> movement. This also appears in console mode. One can retrieve the
>> pointer again with the touchpad, but I'm more precise with the
>> Trackpoint and prefer using that.
>> The Trackpoint works well in kernel 6.1.14
>> Can somebody help?
>
> See the ticket for more details.
FWIW, another user reported seeing the same problem on a different machine:
```
Radek 2023-03-14 08:00:31 UTC
I am experiencing the same issue with kernel >6.2.x on Toshiba Z30
with Alps PS/2 DualPoint trackpoint / touchpad. The cursor jumps top
right even with the lowest sensitivity settings. It works just fine
with 6.1 or lower.
```
Pali, or anybody else: anyone any idea what's might be wrong there?
There afaics is only one recent commit to the alps driver (292a089d78d3
("treewide: Convert del_timer*() to timer_shutdown*()")); it it maybe
worth trying to revert it?
Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.
^ permalink raw reply
* Faktoring
From: Szczepan Kiełbasa @ 2023-03-15 9:05 UTC (permalink / raw)
To: linux-input
Dzień dobry,
rozważali Państwo wybór finansowania, które spełni potrzeby firmy, zapewniając natychmiastowy dostęp do gotówki, bez zbędnych przestojów?
Przygotowaliśmy rozwiązania faktoringowe dopasowane do Państwa branży i wielkości firmy, dzięki którym, nie muszą Państwo martwić się o niewypłacalność kontrahentów, ponieważ transakcje są zabezpieczone i posiadają gwarancję spłaty.
Chcą Państwo przeanalizować dostępne opcje?
Pozdrawiam
Szczepan Kiełbasa
^ permalink raw reply
* Re: [PATCH v3 13/38] Input: add HAS_IOPORT dependencies
From: Geert Uytterhoeven @ 2023-03-15 8:22 UTC (permalink / raw)
To: Niklas Schnelle
Cc: Arnd Bergmann, Dmitry Torokhov, Greg Kroah-Hartman, Bjorn Helgaas,
Uwe Kleine-König, Mauro Carvalho Chehab, Alan Stern,
Rafael J. Wysocki, Paul Walmsley, Palmer Dabbelt, Albert Ou,
linux-kernel, linux-arch, linux-pci, Arnd Bergmann, linux-input
In-Reply-To: <20230314121216.413434-14-schnelle@linux.ibm.com>
Hi Niklas,
On Tue, Mar 14, 2023 at 1:12 PM Niklas Schnelle <schnelle@linux.ibm.com> wrote:
> In a future patch HAS_IOPORT=n will result in inb()/outb() and friends
> not being declared. We thus need to add HAS_IOPORT as dependency for
> those drivers using them.
>
> Co-developed-by: Arnd Bergmann <arnd@kernel.org>
> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
Thanks for your patch!
> --- a/drivers/input/serio/Kconfig
> +++ b/drivers/input/serio/Kconfig
> @@ -75,6 +75,7 @@ config SERIO_Q40KBD
> config SERIO_PARKBD
> tristate "Parallel port keyboard adapter"
> depends on PARPORT
> + depends on HAS_IOPORT
> help
> Say Y here if you built a simple parallel port adapter to attach
> an additional AT keyboard, XT keyboard or PS/2 mouse.
This driver seems to use only the parport and serio APIs, so it might
work on systems without HAS_IOPORT. Dunno for sure.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH v2] Fix buffer overrun in HID-SENSOR name string
From: Krzysztof Kozlowski @ 2023-03-15 7:04 UTC (permalink / raw)
To: Todd Brandt, linux-input, linux-iio, linux-kernel
Cc: todd.e.brandt, srinivas.pandruvada, jic23, jikos, p.jungkamp
In-Reply-To: <20230313220653.3996-1-todd.e.brandt@intel.com>
On 13/03/2023 23:06, Todd Brandt wrote:
> On some platforms there are some platform devices created with
> invalid names. For example: "HID-SENSOR-INT-020b?.39.auto" instead
> of "HID-SENSOR-INT-020b.39.auto"
>
> This string include some invalid characters, hence it will fail to
> properly load the driver which will handle this custom sensor. Also
> it is a problem for some user space tools, which parse the device
> names from ftrace and dmesg.
>
> This is because the string, real_usage, is not NULL terminated and
> printed with %s to form device name.
>
> To address this, we initialize the real_usage string with 0s.
>
> Philipp Jungkamp created this fix, I'm simply submitting it. I've
> verified it fixes bugzilla issue 217169
>
> Reported-and-tested-by: Todd Brandt <todd.e.brandt@linux.intel.com>
> Signed-off-by: Todd Brandt <todd.e.brandt@intel.com>
SoB denotes that you reported this and tested. Otherwise shall we start
adding Reported and Tested tags to all of our commits?
Best regards,
Krzysztof
^ permalink raw reply
* [PATCH v2 4/4] Input: iqs269a - add support for OTP variants
From: Jeff LaBundy @ 2023-03-15 4:02 UTC (permalink / raw)
To: dmitry.torokhov, robh+dt; +Cc: linux-input, devicetree, jeff
In-Reply-To: <ZBFC+e/3JcYITClP@nixie71>
This patch adds support for each available OTP variant of the device.
The OTP configuration cannot be read over I2C, so it is derived from
a compatible string instead.
Early revisions of the D0 order code require their OTP-enabled func-
tionality to be manually restored following a soft reset; this patch
accommodates this erratum as well.
Signed-off-by: Jeff LaBundy <jeff@labundy.com>
---
Changes in v2:
- Rebased onto latest driver
drivers/input/misc/iqs269a.c | 94 ++++++++++++++++++++++++++++++++++--
1 file changed, 91 insertions(+), 3 deletions(-)
diff --git a/drivers/input/misc/iqs269a.c b/drivers/input/misc/iqs269a.c
index aadb24f8a638..5611d249d204 100644
--- a/drivers/input/misc/iqs269a.c
+++ b/drivers/input/misc/iqs269a.c
@@ -102,6 +102,11 @@
#define IQS269_MISC_B_TRACKING_UI_ENABLE BIT(4)
#define IQS269_MISC_B_FILT_STR_SLIDER GENMASK(1, 0)
+#define IQS269_TOUCH_HOLD_SLIDER_SEL 0x89
+#define IQS269_TOUCH_HOLD_DEFAULT 0x14
+#define IQS269_TOUCH_HOLD_MS_MIN 256
+#define IQS269_TOUCH_HOLD_MS_MAX 65280
+
#define IQS269_TIMEOUT_TAP_MS_MAX 4080
#define IQS269_TIMEOUT_SWIPE_MS_MAX 4080
#define IQS269_THRESH_SWIPE_MAX 255
@@ -151,6 +156,10 @@
#define IQS269_MAX_REG 0xFF
+#define IQS269_OTP_OPTION_DEFAULT 0x00
+#define IQS269_OTP_OPTION_TWS 0xD0
+#define IQS269_OTP_OPTION_HOLD BIT(7)
+
#define IQS269_NUM_CH 8
#define IQS269_NUM_SL 2
@@ -315,6 +324,7 @@ struct iqs269_private {
struct input_dev *slider[IQS269_NUM_SL];
unsigned int keycode[ARRAY_SIZE(iqs269_events) * IQS269_NUM_CH];
unsigned int sl_code[IQS269_NUM_SL][IQS269_NUM_GESTURES];
+ unsigned int otp_option;
unsigned int ch_num;
bool hall_enable;
bool ati_current;
@@ -325,6 +335,14 @@ static enum iqs269_slider_id iqs269_slider_type(struct iqs269_private *iqs269,
{
int i;
+ /*
+ * Slider 1 is unavailable if the touch-and-hold option is enabled via
+ * OTP. In that case, the channel selection register is repurposed for
+ * the touch-and-hold timer ceiling.
+ */
+ if (slider_num && (iqs269->otp_option & IQS269_OTP_OPTION_HOLD))
+ return IQS269_SLIDER_NONE;
+
if (!iqs269->sys_reg.slider_select[slider_num])
return IQS269_SLIDER_NONE;
@@ -565,7 +583,8 @@ static int iqs269_parse_chan(struct iqs269_private *iqs269,
if (fwnode_property_present(ch_node, "azoteq,slider0-select"))
iqs269->sys_reg.slider_select[0] |= BIT(reg);
- if (fwnode_property_present(ch_node, "azoteq,slider1-select"))
+ if (fwnode_property_present(ch_node, "azoteq,slider1-select") &&
+ !(iqs269->otp_option & IQS269_OTP_OPTION_HOLD))
iqs269->sys_reg.slider_select[1] |= BIT(reg);
ch_reg = &iqs269->sys_reg.ch_reg[reg];
@@ -990,7 +1009,43 @@ static int iqs269_parse_prop(struct iqs269_private *iqs269)
sys_reg->blocking = 0;
sys_reg->slider_select[0] = 0;
- sys_reg->slider_select[1] = 0;
+
+ /*
+ * If configured via OTP to do so, the device asserts a pulse on the
+ * GPIO4 pin for approximately 60 ms once a selected channel is held
+ * in a state of touch for a configurable length of time.
+ *
+ * In that case, the register used for slider 1 channel selection is
+ * repurposed for the touch-and-hold timer ceiling.
+ */
+ if (iqs269->otp_option & IQS269_OTP_OPTION_HOLD) {
+ if (!device_property_read_u32(&client->dev,
+ "azoteq,touch-hold-ms", &val)) {
+ if (val < IQS269_TOUCH_HOLD_MS_MIN ||
+ val > IQS269_TOUCH_HOLD_MS_MAX) {
+ dev_err(&client->dev,
+ "Invalid touch-and-hold ceiling: %u\n",
+ val);
+ return -EINVAL;
+ }
+
+ sys_reg->slider_select[1] = val / 256;
+ } else if (iqs269->ver_info.fw_num < IQS269_VER_INFO_FW_NUM_3) {
+ /*
+ * The default touch-and-hold timer ceiling initially
+ * read from early revisions of silicon is invalid if
+ * the device experienced a soft reset between power-
+ * on and the read operation.
+ *
+ * To protect against this case, explicitly cache the
+ * default value so that it is restored each time the
+ * device is re-initialized.
+ */
+ sys_reg->slider_select[1] = IQS269_TOUCH_HOLD_DEFAULT;
+ }
+ } else {
+ sys_reg->slider_select[1] = 0;
+ }
sys_reg->event_mask = ~((u8)IQS269_EVENT_MASK_SYS);
@@ -1137,12 +1192,30 @@ static int iqs269_parse_prop(struct iqs269_private *iqs269)
return 0;
}
+static const struct reg_sequence iqs269_tws_init[] = {
+ { IQS269_TOUCH_HOLD_SLIDER_SEL, IQS269_TOUCH_HOLD_DEFAULT },
+ { 0xF0, 0x580F },
+ { 0xF0, 0x59EF },
+};
+
static int iqs269_dev_init(struct iqs269_private *iqs269)
{
int error;
mutex_lock(&iqs269->lock);
+ /*
+ * Early revisions of silicon require the following workaround in order
+ * to restore any OTP-enabled functionality after a soft reset.
+ */
+ if (iqs269->otp_option == IQS269_OTP_OPTION_TWS &&
+ iqs269->ver_info.fw_num < IQS269_VER_INFO_FW_NUM_3) {
+ error = regmap_multi_reg_write(iqs269->regmap, iqs269_tws_init,
+ ARRAY_SIZE(iqs269_tws_init));
+ if (error)
+ goto err_mutex;
+ }
+
error = regmap_update_bits(iqs269->regmap, IQS269_HALL_UI,
IQS269_HALL_UI_ENABLE,
iqs269->hall_enable ? ~0 : 0);
@@ -1772,6 +1845,10 @@ static int iqs269_probe(struct i2c_client *client)
i2c_set_clientdata(client, iqs269);
iqs269->client = client;
+ if (client->dev.of_node)
+ iqs269->otp_option = (uintptr_t)
+ of_device_get_match_data(&client->dev);
+
iqs269->regmap = devm_regmap_init_i2c(client, &iqs269_regmap_config);
if (IS_ERR(iqs269->regmap)) {
error = PTR_ERR(iqs269->regmap);
@@ -1897,7 +1974,18 @@ static int iqs269_resume(struct device *dev)
static DEFINE_SIMPLE_DEV_PM_OPS(iqs269_pm, iqs269_suspend, iqs269_resume);
static const struct of_device_id iqs269_of_match[] = {
- { .compatible = "azoteq,iqs269a" },
+ {
+ .compatible = "azoteq,iqs269a",
+ .data = (void *)IQS269_OTP_OPTION_DEFAULT,
+ },
+ {
+ .compatible = "azoteq,iqs269a-00",
+ .data = (void *)IQS269_OTP_OPTION_DEFAULT,
+ },
+ {
+ .compatible = "azoteq,iqs269a-d0",
+ .data = (void *)IQS269_OTP_OPTION_TWS,
+ },
{ }
};
MODULE_DEVICE_TABLE(of, iqs269_of_match);
--
2.34.1
^ permalink raw reply related
* [PATCH v2 3/4] dt-bindings: input: iqs269a: Add bindings for OTP variants
From: Jeff LaBundy @ 2023-03-15 4:02 UTC (permalink / raw)
To: dmitry.torokhov, robh+dt; +Cc: linux-input, devicetree, jeff
In-Reply-To: <ZBFC+e/3JcYITClP@nixie71>
This patch adds bindings for the D0 order code of the device. This
order code represents an OTP variant that enables a touch-and-hold
function in place of slider 1.
Also included is the ability to specify the 00 order code (default
option with no OTP customization) explicitly.
Signed-off-by: Jeff LaBundy <jeff@labundy.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Changes in v2:
- None
.../devicetree/bindings/input/iqs269a.yaml | 30 ++++++++++++++++++-
1 file changed, 29 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/input/iqs269a.yaml b/Documentation/devicetree/bindings/input/iqs269a.yaml
index b42f07542d27..2c3f693b8982 100644
--- a/Documentation/devicetree/bindings/input/iqs269a.yaml
+++ b/Documentation/devicetree/bindings/input/iqs269a.yaml
@@ -20,7 +20,10 @@ description: |
properties:
compatible:
- const: azoteq,iqs269a
+ enum:
+ - azoteq,iqs269a
+ - azoteq,iqs269a-00
+ - azoteq,iqs269a-d0
reg:
maxItems: 1
@@ -207,6 +210,16 @@ properties:
default: 1
description: Specifies the slider coordinate filter strength.
+ azoteq,touch-hold-ms:
+ multipleOf: 256
+ minimum: 256
+ maximum: 65280
+ default: 5120
+ description:
+ Specifies the length of time (in ms) for which the channel selected by
+ 'azoteq,gpio3-select' must be held in a state of touch in order for an
+ approximately 60-ms pulse to be asserted on the GPIO4 pin.
+
linux,keycodes:
minItems: 1
maxItems: 8
@@ -514,6 +527,21 @@ patternProperties:
additionalProperties: false
+if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - azoteq,iqs269a-d0
+then:
+ patternProperties:
+ "^channel@[0-7]$":
+ properties:
+ azoteq,slider1-select: false
+else:
+ properties:
+ azoteq,touch-hold-ms: false
+
required:
- compatible
- reg
--
2.34.1
^ permalink raw reply related
* [PATCH v2 2/4] Input: iqs269a - add support for slider gestures
From: Jeff LaBundy @ 2023-03-15 4:02 UTC (permalink / raw)
To: dmitry.torokhov, robh+dt; +Cc: linux-input, devicetree, jeff
In-Reply-To: <ZBFC+e/3JcYITClP@nixie71>
This patch adds support for slider gestures that can be expressed
by the device. Each gesture (e.g. tap or hold) can be mapped to a
unique keycode for either slider 0 or 1.
With this change, raw slider coordinates are reported only if the
slider has no keycodes defined. This prevents unwanted mouse cur-
sor movement when expressing axial gestures (e.g. swipe) and also
eliminates some unnecessary I2C traffic.
Different revisions of silicon use different tap and swipe timeout
step sizes. Apply an appropriate scaling factor depending on which
revision is found.
To facilitate this change, store the iqs269_ver_info struct in the
driver's private data so that other functions can use it after the
driver has probed.
Last but not least, a former reserved field in iqs269_ver_info now
contains useful information; give it a name (fw_num).
Signed-off-by: Jeff LaBundy <jeff@labundy.com>
---
Changes in v2:
- Rebased onto latest driver
- Renamed 'azoteq,slider-swipe' to 'azoteq,gesture-swipe' to match IQS626A
- Renamed 'gs_code' to 'sl_code'
| 220 ++++++++++++++++++++++++++++++-----
1 file changed, 191 insertions(+), 29 deletions(-)
--git a/drivers/input/misc/iqs269a.c b/drivers/input/misc/iqs269a.c
index f4c3aff3895b..aadb24f8a638 100644
--- a/drivers/input/misc/iqs269a.c
+++ b/drivers/input/misc/iqs269a.c
@@ -9,6 +9,7 @@
* axial sliders presented by the device.
*/
+#include <linux/bits.h>
#include <linux/completion.h>
#include <linux/delay.h>
#include <linux/device.h>
@@ -26,6 +27,8 @@
#define IQS269_VER_INFO 0x00
#define IQS269_VER_INFO_PROD_NUM 0x4F
+#define IQS269_VER_INFO_FW_NUM_2 0x03
+#define IQS269_VER_INFO_FW_NUM_3 0x10
#define IQS269_SYS_FLAGS 0x02
#define IQS269_SYS_FLAGS_SHOW_RESET BIT(15)
@@ -53,6 +56,7 @@
#define IQS269_SYS_SETTINGS_ULP_UPDATE_MASK GENMASK(10, 8)
#define IQS269_SYS_SETTINGS_ULP_UPDATE_SHIFT 8
#define IQS269_SYS_SETTINGS_ULP_UPDATE_MAX 7
+#define IQS269_SYS_SETTINGS_SLIDER_SWIPE BIT(7)
#define IQS269_SYS_SETTINGS_RESEED_OFFSET BIT(6)
#define IQS269_SYS_SETTINGS_EVENT_MODE BIT(5)
#define IQS269_SYS_SETTINGS_EVENT_MODE_LP BIT(4)
@@ -69,6 +73,7 @@
#define IQS269_FILT_STR_MAX 3
#define IQS269_EVENT_MASK_SYS BIT(6)
+#define IQS269_EVENT_MASK_GESTURE BIT(3)
#define IQS269_EVENT_MASK_DEEP BIT(2)
#define IQS269_EVENT_MASK_TOUCH BIT(1)
#define IQS269_EVENT_MASK_PROX BIT(0)
@@ -97,6 +102,10 @@
#define IQS269_MISC_B_TRACKING_UI_ENABLE BIT(4)
#define IQS269_MISC_B_FILT_STR_SLIDER GENMASK(1, 0)
+#define IQS269_TIMEOUT_TAP_MS_MAX 4080
+#define IQS269_TIMEOUT_SWIPE_MS_MAX 4080
+#define IQS269_THRESH_SWIPE_MAX 255
+
#define IQS269_CHx_ENG_A_MEAS_CAP_SIZE BIT(15)
#define IQS269_CHx_ENG_A_RX_GND_INACTIVE BIT(13)
#define IQS269_CHx_ENG_A_LOCAL_CAP_SIZE BIT(12)
@@ -175,6 +184,20 @@ enum iqs269_event_id {
IQS269_EVENT_DEEP_UP,
};
+enum iqs269_slider_id {
+ IQS269_SLIDER_NONE,
+ IQS269_SLIDER_KEY,
+ IQS269_SLIDER_RAW,
+};
+
+enum iqs269_gesture_id {
+ IQS269_GESTURE_TAP,
+ IQS269_GESTURE_HOLD,
+ IQS269_GESTURE_FLICK_POS,
+ IQS269_GESTURE_FLICK_NEG,
+ IQS269_NUM_GESTURES,
+};
+
struct iqs269_switch_desc {
unsigned int code;
bool enabled;
@@ -234,7 +257,7 @@ struct iqs269_ver_info {
u8 prod_num;
u8 sw_num;
u8 hw_num;
- u8 padding;
+ u8 fw_num;
} __packed;
struct iqs269_ch_reg {
@@ -285,16 +308,33 @@ struct iqs269_private {
struct regmap *regmap;
struct mutex lock;
struct iqs269_switch_desc switches[ARRAY_SIZE(iqs269_events)];
+ struct iqs269_ver_info ver_info;
struct iqs269_sys_reg sys_reg;
struct completion ati_done;
struct input_dev *keypad;
struct input_dev *slider[IQS269_NUM_SL];
unsigned int keycode[ARRAY_SIZE(iqs269_events) * IQS269_NUM_CH];
+ unsigned int sl_code[IQS269_NUM_SL][IQS269_NUM_GESTURES];
unsigned int ch_num;
bool hall_enable;
bool ati_current;
};
+static enum iqs269_slider_id iqs269_slider_type(struct iqs269_private *iqs269,
+ int slider_num)
+{
+ int i;
+
+ if (!iqs269->sys_reg.slider_select[slider_num])
+ return IQS269_SLIDER_NONE;
+
+ for (i = 0; i < IQS269_NUM_GESTURES; i++)
+ if (iqs269->sl_code[slider_num][i] != KEY_RESERVED)
+ return IQS269_SLIDER_KEY;
+
+ return IQS269_SLIDER_RAW;
+}
+
static int iqs269_ati_mode_set(struct iqs269_private *iqs269,
unsigned int ch_num, unsigned int mode)
{
@@ -1004,6 +1044,76 @@ static int iqs269_parse_prop(struct iqs269_private *iqs269)
general |= (val << IQS269_SYS_SETTINGS_ULP_UPDATE_SHIFT);
}
+ if (device_property_present(&client->dev, "linux,keycodes")) {
+ int scale = 1;
+ int count = device_property_count_u32(&client->dev,
+ "linux,keycodes");
+ if (count > IQS269_NUM_GESTURES * IQS269_NUM_SL) {
+ dev_err(&client->dev, "Too many keycodes present\n");
+ return -EINVAL;
+ } else if (count < 0) {
+ dev_err(&client->dev, "Failed to count keycodes: %d\n",
+ count);
+ return count;
+ }
+
+ error = device_property_read_u32_array(&client->dev,
+ "linux,keycodes",
+ *iqs269->sl_code, count);
+ if (error) {
+ dev_err(&client->dev, "Failed to read keycodes: %d\n",
+ error);
+ return error;
+ }
+
+ if (device_property_present(&client->dev,
+ "azoteq,gesture-swipe"))
+ general |= IQS269_SYS_SETTINGS_SLIDER_SWIPE;
+
+ /*
+ * Early revisions of silicon use a more granular step size for
+ * tap and swipe gesture timeouts; scale them appropriately.
+ */
+ if (iqs269->ver_info.fw_num < IQS269_VER_INFO_FW_NUM_3)
+ scale = 4;
+
+ if (!device_property_read_u32(&client->dev,
+ "azoteq,timeout-tap-ms", &val)) {
+ if (val > IQS269_TIMEOUT_TAP_MS_MAX / scale) {
+ dev_err(&client->dev, "Invalid timeout: %u\n",
+ val);
+ return -EINVAL;
+ }
+
+ sys_reg->timeout_tap = val / (16 / scale);
+ }
+
+ if (!device_property_read_u32(&client->dev,
+ "azoteq,timeout-swipe-ms",
+ &val)) {
+ if (val > IQS269_TIMEOUT_SWIPE_MS_MAX / scale) {
+ dev_err(&client->dev, "Invalid timeout: %u\n",
+ val);
+ return -EINVAL;
+ }
+
+ sys_reg->timeout_swipe = val / (16 / scale);
+ }
+
+ if (!device_property_read_u32(&client->dev,
+ "azoteq,thresh-swipe", &val)) {
+ if (val > IQS269_THRESH_SWIPE_MAX) {
+ dev_err(&client->dev, "Invalid threshold: %u\n",
+ val);
+ return -EINVAL;
+ }
+
+ sys_reg->thresh_swipe = val;
+ }
+
+ sys_reg->event_mask &= ~IQS269_EVENT_MASK_GESTURE;
+ }
+
general &= ~IQS269_SYS_SETTINGS_RESEED_OFFSET;
if (device_property_present(&client->dev, "azoteq,reseed-offset"))
general |= IQS269_SYS_SETTINGS_RESEED_OFFSET;
@@ -1012,10 +1122,11 @@ static int iqs269_parse_prop(struct iqs269_private *iqs269)
/*
* As per the datasheet, enable streaming during normal-power mode if
- * either slider is in use. In that case, the device returns to event
- * mode during low-power mode.
+ * raw coordinates will be read from either slider. In that case, the
+ * device returns to event mode during low-power mode.
*/
- if (sys_reg->slider_select[0] || sys_reg->slider_select[1])
+ if (iqs269_slider_type(iqs269, 0) == IQS269_SLIDER_RAW ||
+ iqs269_slider_type(iqs269, 1) == IQS269_SLIDER_RAW)
general |= IQS269_SYS_SETTINGS_EVENT_MODE_LP;
general |= IQS269_SYS_SETTINGS_REDO_ATI;
@@ -1106,19 +1217,37 @@ static int iqs269_input_init(struct iqs269_private *iqs269)
}
for (i = 0; i < IQS269_NUM_SL; i++) {
- if (!iqs269->sys_reg.slider_select[i])
+ if (iqs269_slider_type(iqs269, i) == IQS269_SLIDER_NONE)
continue;
iqs269->slider[i] = devm_input_allocate_device(&client->dev);
if (!iqs269->slider[i])
return -ENOMEM;
+ iqs269->slider[i]->keycodemax = ARRAY_SIZE(iqs269->sl_code[i]);
+ iqs269->slider[i]->keycode = iqs269->sl_code[i];
+ iqs269->slider[i]->keycodesize = sizeof(**iqs269->sl_code);
+
iqs269->slider[i]->name = i ? "iqs269a_slider_1"
: "iqs269a_slider_0";
iqs269->slider[i]->id.bustype = BUS_I2C;
- input_set_capability(iqs269->slider[i], EV_KEY, BTN_TOUCH);
- input_set_abs_params(iqs269->slider[i], ABS_X, 0, 255, 0, 0);
+ for (j = 0; j < IQS269_NUM_GESTURES; j++)
+ if (iqs269->sl_code[i][j] != KEY_RESERVED)
+ input_set_capability(iqs269->slider[i], EV_KEY,
+ iqs269->sl_code[i][j]);
+
+ /*
+ * Present the slider as a narrow trackpad if one or more chan-
+ * nels have been selected to participate, but no gestures have
+ * been mapped to a keycode.
+ */
+ if (iqs269_slider_type(iqs269, i) == IQS269_SLIDER_RAW) {
+ input_set_capability(iqs269->slider[i],
+ EV_KEY, BTN_TOUCH);
+ input_set_abs_params(iqs269->slider[i],
+ ABS_X, 0, 255, 0, 0);
+ }
error = input_register_device(iqs269->slider[i]);
if (error) {
@@ -1167,28 +1296,62 @@ static int iqs269_report(struct iqs269_private *iqs269)
if (be16_to_cpu(flags.system) & IQS269_SYS_FLAGS_IN_ATI)
return 0;
- error = regmap_raw_read(iqs269->regmap, IQS269_SLIDER_X, slider_x,
- sizeof(slider_x));
- if (error) {
- dev_err(&client->dev, "Failed to read slider position: %d\n",
- error);
- return error;
+ if (iqs269_slider_type(iqs269, 0) == IQS269_SLIDER_RAW ||
+ iqs269_slider_type(iqs269, 1) == IQS269_SLIDER_RAW) {
+ error = regmap_raw_read(iqs269->regmap, IQS269_SLIDER_X,
+ slider_x, sizeof(slider_x));
+ if (error) {
+ dev_err(&client->dev,
+ "Failed to read slider position: %d\n", error);
+ return error;
+ }
}
for (i = 0; i < IQS269_NUM_SL; i++) {
- if (!iqs269->sys_reg.slider_select[i])
+ flags.gesture >>= (i * IQS269_NUM_GESTURES);
+
+ switch (iqs269_slider_type(iqs269, i)) {
+ case IQS269_SLIDER_NONE:
continue;
- /*
- * Report BTN_TOUCH if any channel that participates in the
- * slider is in a state of touch.
- */
- if (flags.states[IQS269_ST_OFFS_TOUCH] &
- iqs269->sys_reg.slider_select[i]) {
- input_report_key(iqs269->slider[i], BTN_TOUCH, 1);
- input_report_abs(iqs269->slider[i], ABS_X, slider_x[i]);
- } else {
- input_report_key(iqs269->slider[i], BTN_TOUCH, 0);
+ case IQS269_SLIDER_KEY:
+ for (j = 0; j < IQS269_NUM_GESTURES; j++)
+ input_report_key(iqs269->slider[i],
+ iqs269->sl_code[i][j],
+ flags.gesture & BIT(j));
+
+ if (!(flags.gesture & (BIT(IQS269_GESTURE_FLICK_NEG) |
+ BIT(IQS269_GESTURE_FLICK_POS) |
+ BIT(IQS269_GESTURE_TAP))))
+ break;
+
+ input_sync(iqs269->slider[i]);
+
+ /*
+ * Momentary gestures are followed by a complementary
+ * release cycle so as to emulate a full keystroke.
+ */
+ for (j = 0; j < IQS269_NUM_GESTURES; j++)
+ if (j != IQS269_GESTURE_HOLD)
+ input_report_key(iqs269->slider[i],
+ iqs269->sl_code[i][j],
+ 0);
+ break;
+
+ case IQS269_SLIDER_RAW:
+ /*
+ * The slider is considered to be in a state of touch
+ * if any selected channels are in a state of touch.
+ */
+ state = flags.states[IQS269_ST_OFFS_TOUCH];
+ state &= iqs269->sys_reg.slider_select[i];
+
+ input_report_key(iqs269->slider[i], BTN_TOUCH, state);
+
+ if (state)
+ input_report_abs(iqs269->slider[i],
+ ABS_X, slider_x[i]);
+ break;
}
input_sync(iqs269->slider[i]);
@@ -1599,7 +1762,6 @@ static const struct regmap_config iqs269_regmap_config = {
static int iqs269_probe(struct i2c_client *client)
{
- struct iqs269_ver_info ver_info;
struct iqs269_private *iqs269;
int error;
@@ -1621,14 +1783,14 @@ static int iqs269_probe(struct i2c_client *client)
mutex_init(&iqs269->lock);
init_completion(&iqs269->ati_done);
- error = regmap_raw_read(iqs269->regmap, IQS269_VER_INFO, &ver_info,
- sizeof(ver_info));
+ error = regmap_raw_read(iqs269->regmap, IQS269_VER_INFO,
+ &iqs269->ver_info, sizeof(iqs269->ver_info));
if (error)
return error;
- if (ver_info.prod_num != IQS269_VER_INFO_PROD_NUM) {
+ if (iqs269->ver_info.prod_num != IQS269_VER_INFO_PROD_NUM) {
dev_err(&client->dev, "Unrecognized product number: 0x%02X\n",
- ver_info.prod_num);
+ iqs269->ver_info.prod_num);
return -EINVAL;
}
--
2.34.1
^ permalink raw reply related
* [PATCH v2 1/4] dt-bindings: input: iqs269a: Add bindings for slider gestures
From: Jeff LaBundy @ 2023-03-15 4:02 UTC (permalink / raw)
To: dmitry.torokhov, robh+dt; +Cc: linux-input, devicetree, jeff
In-Reply-To: <ZBFC+e/3JcYITClP@nixie71>
This patch adds bindings for slider gestures that can be expressed
by the device.
Signed-off-by: Jeff LaBundy <jeff@labundy.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Changes in v2:
- Renamed 'azoteq,slider-swipe' to 'azoteq,gesture-swipe' to match IQS626A and
noted it as depending upon 'linux,keycodes'
- Dropped the $ref declaration for 'linux,keycodes' and referenced input.yaml,
which has since been updated to define this property
.../devicetree/bindings/input/iqs269a.yaml | 68 +++++++++++++++++++
1 file changed, 68 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/iqs269a.yaml b/Documentation/devicetree/bindings/input/iqs269a.yaml
index 3c430d38594f..b42f07542d27 100644
--- a/Documentation/devicetree/bindings/input/iqs269a.yaml
+++ b/Documentation/devicetree/bindings/input/iqs269a.yaml
@@ -9,6 +9,9 @@ title: Azoteq IQS269A Capacitive Touch Controller
maintainers:
- Jeff LaBundy <jeff@labundy.com>
+allOf:
+ - $ref: input.yaml#
+
description: |
The Azoteq IQS269A is an 8-channel capacitive touch controller that features
additional Hall-effect and inductive sensing capabilities.
@@ -204,6 +207,63 @@ properties:
default: 1
description: Specifies the slider coordinate filter strength.
+ linux,keycodes:
+ minItems: 1
+ maxItems: 8
+ description: |
+ Specifies the numeric keycodes associated with each available gesture in
+ the following order (enter 0 for unused gestures):
+ 0: Slider 0 tap
+ 1: Slider 0 hold
+ 2: Slider 0 positive flick or swipe
+ 3: Slider 0 negative flick or swipe
+ 4: Slider 1 tap
+ 5: Slider 1 hold
+ 6: Slider 1 positive flick or swipe
+ 7: Slider 1 negative flick or swipe
+
+ azoteq,gesture-swipe:
+ type: boolean
+ description:
+ Directs the device to interpret axial gestures as a swipe (finger remains
+ on slider) instead of a flick (finger leaves slider).
+
+ azoteq,timeout-tap-ms:
+ multipleOf: 16
+ minimum: 0
+ maximum: 4080
+ default: 400
+ description:
+ Specifies the length of time (in ms) within which a slider touch must be
+ released in order to be interpreted as a tap. Default and maximum values
+ as well as step size are reduced by a factor of 4 with device version 2.
+
+ azoteq,timeout-swipe-ms:
+ multipleOf: 16
+ minimum: 0
+ maximum: 4080
+ default: 2000
+ description:
+ Specifies the length of time (in ms) within which an axial gesture must be
+ completed in order to be interpreted as a flick or swipe. Default and max-
+ imum values as well as step size are reduced by a factor of 4 with device
+ version 2.
+
+ azoteq,thresh-swipe:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ minimum: 0
+ maximum: 255
+ default: 128
+ description:
+ Specifies the number of points across which an axial gesture must travel
+ in order to be interpreted as a flick or swipe.
+
+dependencies:
+ azoteq,gesture-swipe: ["linux,keycodes"]
+ azoteq,timeout-tap-ms: ["linux,keycodes"]
+ azoteq,timeout-swipe-ms: ["linux,keycodes"]
+ azoteq,thresh-swipe: ["linux,keycodes"]
+
patternProperties:
"^channel@[0-7]$":
type: object
@@ -484,6 +544,14 @@ examples:
azoteq,hall-enable;
azoteq,suspend-mode = <2>;
+ linux,keycodes = <KEY_PLAYPAUSE>,
+ <KEY_STOPCD>,
+ <KEY_NEXTSONG>,
+ <KEY_PREVIOUSSONG>;
+
+ azoteq,timeout-tap-ms = <400>;
+ azoteq,timeout-swipe-ms = <800>;
+
channel@0 {
reg = <0x0>;
--
2.34.1
^ permalink raw reply related
* [PATCH v2 0/4] Add support for slider gestures and OTP variants
From: Jeff LaBundy @ 2023-03-15 4:00 UTC (permalink / raw)
To: dmitry.torokhov, robh+dt; +Cc: linux-input, devicetree, jeff
This series introduces support for some additional features offered by the
Azoteq IQS269A capacitive touch controller.
Patches 1 and 2 add support for slider gestures (e.g. tap or swipe). Gestures
are recognized by the hardware itself based on touch activity across the chan-
nels associated with the slider. This feature is useful for lightweight systems
that do not post-process absolute coordinates to determine gestures expressed
by the user.
Gestures are presented to user space as keycodes. An example use-case is an
array of multimedia keys as seen in the following demo:
https://youtu.be/k_vMRQiHLgA
Patches 3 and 4 add support for the device's available OTP variants, which
trade features or exhibit errata that require workarounds.
The original series was first introduced in [1]. Patch 5 was determined to be
non-optimal, and has since been dropped. Patch 6 has been dropped in favor of
[2] which has since been merged.
[1] https://patchwork.kernel.org/patch/11716215/
[2] https://patchwork.kernel.org/patch/13087783/
Jeff LaBundy (4):
dt-bindings: input: iqs269a: Add bindings for slider gestures
Input: iqs269a - add support for slider gestures
dt-bindings: input: iqs269a: Add bindings for OTP variants
Input: iqs269a - add support for OTP variants
.../devicetree/bindings/input/iqs269a.yaml | 98 +++++-
drivers/input/misc/iqs269a.c | 314 ++++++++++++++++--
2 files changed, 379 insertions(+), 33 deletions(-)
--
2.34.1
^ permalink raw reply
* Re: [PATCH 3/4] Input: sun4i-ts: drop of_match_ptr for ID table
From: Jernej Škrabec @ 2023-03-14 20:04 UTC (permalink / raw)
To: Dmitry Torokhov, Ray Jui, Scott Branden,
Broadcom internal kernel review list, Chen-Yu Tsai,
Samuel Holland, linux-input, linux-kernel, linux-arm-kernel,
linux-sunxi, Krzysztof Kozlowski
Cc: Krzysztof Kozlowski
In-Reply-To: <20230312131514.351603-3-krzysztof.kozlowski@linaro.org>
Dne nedelja, 12. marec 2023 ob 14:15:13 CET je Krzysztof Kozlowski napisal(a):
> The driver can match only via the DT table so the table should be always
> used and the of_match_ptr does not have any sense (this also allows ACPI
> matching via PRP0001, even though it might not be relevant here). This
> also fixes !CONFIG_OF error:
>
> drivers/input/touchscreen/sun4i-ts.c:392:34: error: ‘sun4i_ts_of_match’
> defined but not used [-Werror=unused-const-variable=]
>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
> drivers/input/touchscreen/sun4i-ts.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
Acked-by: Jernej Skrabec <jernej.skrabec@gmail.com>
Best regards,
Jernej
> diff --git a/drivers/input/touchscreen/sun4i-ts.c
> b/drivers/input/touchscreen/sun4i-ts.c index 1117fba30020..577c75c83e25
> 100644
> --- a/drivers/input/touchscreen/sun4i-ts.c
> +++ b/drivers/input/touchscreen/sun4i-ts.c
> @@ -400,7 +400,7 @@ MODULE_DEVICE_TABLE(of, sun4i_ts_of_match);
> static struct platform_driver sun4i_ts_driver = {
> .driver = {
> .name = "sun4i-ts",
> - .of_match_table = of_match_ptr(sun4i_ts_of_match),
> + .of_match_table = sun4i_ts_of_match,
> },
> .probe = sun4i_ts_probe,
> .remove = sun4i_ts_remove,
^ permalink raw reply
* Re: [PATCH v4] HID:hid-sensor-custom: Fix buffer overrun in device name
From: srinivas pandruvada @ 2023-03-14 18:18 UTC (permalink / raw)
To: Todd Brandt, linux-input, linux-iio, linux-kernel
Cc: todd.e.brandt, jic23, jikos, p.jungkamp, stable
In-Reply-To: <20230314181256.15283-1-todd.e.brandt@intel.com>
On Tue, 2023-03-14 at 11:12 -0700, Todd Brandt wrote:
> On some platforms there are some platform devices created with
> invalid names. For example: "HID-SENSOR-INT-020b?.39.auto" instead
> of "HID-SENSOR-INT-020b.39.auto"
>
> This string include some invalid characters, hence it will fail to
> properly load the driver which will handle this custom sensor. Also
> it is a problem for some user space tools, which parses the device
> names from ftrace and dmesg.
>
> This is because the string, real_usage, is not NULL terminated and
> printed with %s to form device name.
>
> To address this, initialize the real_usage string with 0s.
>
> Reported-and-tested-by: Todd Brandt <todd.e.brandt@linux.intel.com>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217169
> Fixes: 98c062e82451 ("HID: hid-sensor-custom: Allow more custom iio
> sensors")
> Cc: stable@vger.kernel.org
> Suggested-by: Philipp Jungkamp <p.jungkamp@gmx.net>
> Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>
> Signed-off-by: Todd Brandt <todd.e.brandt@intel.com>
> Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
> Changes in v4:
> - add the Fixes line
> - add patch version change list
> Changes in v3:
> - update the changelog
> - add proper reviewed/signed/suggested links
> Changes in v2:
> - update the changelog
>
> drivers/hid/hid-sensor-custom.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-
> sensor-custom.c
> index 3e3f89e01d81..d85398721659 100644
> --- a/drivers/hid/hid-sensor-custom.c
> +++ b/drivers/hid/hid-sensor-custom.c
> @@ -940,7 +940,7 @@ hid_sensor_register_platform_device(struct
> platform_device *pdev,
> struct hid_sensor_hub_device
> *hsdev,
> const struct
> hid_sensor_custom_match *match)
> {
> - char real_usage[HID_SENSOR_USAGE_LENGTH];
> + char real_usage[HID_SENSOR_USAGE_LENGTH] = { 0 };
> struct platform_device *custom_pdev;
> const char *dev_name;
> char *c;
^ permalink raw reply
* [PATCH v4] HID:hid-sensor-custom: Fix buffer overrun in device name
From: Todd Brandt @ 2023-03-14 18:12 UTC (permalink / raw)
To: linux-input, linux-iio, linux-kernel
Cc: todd.e.brandt, todd.e.brandt, srinivas.pandruvada, jic23, jikos,
p.jungkamp, stable
On some platforms there are some platform devices created with
invalid names. For example: "HID-SENSOR-INT-020b?.39.auto" instead
of "HID-SENSOR-INT-020b.39.auto"
This string include some invalid characters, hence it will fail to
properly load the driver which will handle this custom sensor. Also
it is a problem for some user space tools, which parses the device
names from ftrace and dmesg.
This is because the string, real_usage, is not NULL terminated and
printed with %s to form device name.
To address this, initialize the real_usage string with 0s.
Reported-and-tested-by: Todd Brandt <todd.e.brandt@linux.intel.com>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=217169
Fixes: 98c062e82451 ("HID: hid-sensor-custom: Allow more custom iio sensors")
Cc: stable@vger.kernel.org
Suggested-by: Philipp Jungkamp <p.jungkamp@gmx.net>
Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>
Signed-off-by: Todd Brandt <todd.e.brandt@intel.com>
Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
Changes in v4:
- add the Fixes line
- add patch version change list
Changes in v3:
- update the changelog
- add proper reviewed/signed/suggested links
Changes in v2:
- update the changelog
drivers/hid/hid-sensor-custom.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
index 3e3f89e01d81..d85398721659 100644
--- a/drivers/hid/hid-sensor-custom.c
+++ b/drivers/hid/hid-sensor-custom.c
@@ -940,7 +940,7 @@ hid_sensor_register_platform_device(struct platform_device *pdev,
struct hid_sensor_hub_device *hsdev,
const struct hid_sensor_custom_match *match)
{
- char real_usage[HID_SENSOR_USAGE_LENGTH];
+ char real_usage[HID_SENSOR_USAGE_LENGTH] = { 0 };
struct platform_device *custom_pdev;
const char *dev_name;
char *c;
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v2 1/2] dt-bindings: input: touchscreen: Add 'hold-in-reset-in-suspend' property to goodix
From: Krzysztof Kozlowski @ 2023-03-14 17:47 UTC (permalink / raw)
To: Jan Jasper de Kroon
Cc: devicetree, dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt,
broonie, alexandre.belloni, kernel, linux-input
In-Reply-To: <20230312183106.551840-1-jajadekroon@gmail.com>
On 12/03/2023 19:31, Jan Jasper de Kroon wrote:
> This patch adds a new optional property, 'hold-in-reset-in-suspend', to the
Do not use "This commit/patch", but imperative mood. See:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
> Goodix touchscreen device tree binding. When set to true, the touchscreen
> controller will be held in reset mode during system suspend, reducing
> power consumption. If not present, the property defaults to false.
>
> I am submitting this patch to the Device Tree mailing list to add a new
Drop the "I am ..." Same comment as above.
> property to the Goodix touchscreen device tree binding. This patch
> supplements a related patch sent to the linux-input mailing list, which
> updates the Goodix touchscreen driver to use this new property.
Anyway entire paragraph does not look related to commit msg. Drop it.
> The
> linux-input patch can be found at:
> https://lore.kernel.org/all/20230311131534.484700-1-jajadekroon@gmail.com/
Also this. It should be rather places under ---.
>
> Signed-off-by: Jan Jasper de Kroon <jajadekroon@gmail.com>
> ---
> V1 -> V2:
> - Updated subject prefix to match subsystem
> - Added more detailed description of the change
> - Fixed formatting issues in commit message
> .../devicetree/bindings/input/touchscreen/goodix.yaml | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
> index 3d016b87c8df..cd4dd3d25364 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
> +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
> @@ -56,6 +56,15 @@ properties:
> touchscreen-size-y: true
> touchscreen-swapped-x-y: true
>
> + hold-in-reset-in-suspend:
Missing vendor prefix.
> + type: boolean
> + default: false
Drop default.
> + description: |
> + When set to true, the touchscreen controller will be held in reset mode
> + during system suspend. This can help reduce power consumption, but may
> + cause the touchscreen to take longer to resume when the system is woken
> + up from suspend. Defaults to false if not present.
Drop last sentence.
Anyway, the property does not look suitable for Devicetree. You describe
system policy - trade off between energy saving and responsivness to the
user. DT is not for policies. Use other interfaces for configuring it,
e.g. some user-space, existing PM interfaces or /sysfs (which is ABI and
needs its Documentation).
> +
> additionalProperties: false
>
> required:
> @@ -75,6 +84,7 @@ examples:
> interrupts = <0 0>;
> irq-gpios = <&gpio1 0 0>;
> reset-gpios = <&gpio1 1 0>;
> + hold-in-reset-in-suspend;
> };
> };
>
Best regards,
Krzysztof
^ permalink raw reply
* [PATCH v3] HID:hid-sensor-custom: Fix buffer overrun in device name
From: Todd Brandt @ 2023-03-14 17:28 UTC (permalink / raw)
To: linux-input, linux-iio, linux-kernel
Cc: todd.e.brandt, todd.e.brandt, srinivas.pandruvada, jic23, jikos,
p.jungkamp
On some platforms there are some platform devices created with
invalid names. For example: "HID-SENSOR-INT-020b?.39.auto" instead
of "HID-SENSOR-INT-020b.39.auto"
This string include some invalid characters, hence it will fail to
properly load the driver which will handle this custom sensor. Also
it is a problem for some user space tools, which parses the device
names from ftrace and dmesg.
This is because the string, real_usage, is not NULL terminated and
printed with %s to form device name.
To address this, initialize the real_usage string with 0s.
Reported-and-tested-by: Todd Brandt <todd.e.brandt@linux.intel.com>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=217169
Suggested-by: Philipp Jungkamp <p.jungkamp@gmx.net>
Signed-off-by: Philipp Jungkamp <p.jungkamp@gmx.net>
Signed-off-by: Todd Brandt <todd.e.brandt@intel.com>
Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
drivers/hid/hid-sensor-custom.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor-custom.c
index 3e3f89e01d81..d85398721659 100644
--- a/drivers/hid/hid-sensor-custom.c
+++ b/drivers/hid/hid-sensor-custom.c
@@ -940,7 +940,7 @@ hid_sensor_register_platform_device(struct platform_device *pdev,
struct hid_sensor_hub_device *hsdev,
const struct hid_sensor_custom_match *match)
{
- char real_usage[HID_SENSOR_USAGE_LENGTH];
+ char real_usage[HID_SENSOR_USAGE_LENGTH] = { 0 };
struct platform_device *custom_pdev;
const char *dev_name;
char *c;
--
2.17.1
^ permalink raw reply related
* Re: [PATCH 1/5] dt-bindings: input: touchscreen: add bindings for focaltech,fts
From: Rob Herring @ 2023-03-14 14:10 UTC (permalink / raw)
To: Joel Selvaraj
Cc: phone-devel, linux-arm-msm, Bjorn Andersson, Rob Herring,
Konrad Dybcio, Dmitry Torokhov, linux-kernel, Neil Armstrong,
Arnd Bergmann, Krzysztof Kozlowski, Andy Gross, Chris Morgan,
Robert Jarzmik, Max Krummenacher, linux-input, Caleb Connolly,
~postmarketos/upstreaming, Jeff LaBundy, devicetree, Jean Delvare,
Job Noorman, Alistair Francis, Markuss Broks, Henrik Rydberg
In-Reply-To: <20230312093249.1846993-2-joelselvaraj.oss@gmail.com>
On Sun, 12 Mar 2023 04:32:45 -0500, Joel Selvaraj wrote:
> Add devicetree bindings for the Focaltech FTS touchscreen drivers.
>
> Signed-off-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>
> Signed-off-by: Caleb Connolly <caleb@connolly.tech>
> ---
> .../input/touchscreen/focaltech,fts.yaml | 81 +++++++++++++++++++
> 1 file changed, 81 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/input/touchscreen/focaltech,fts.yaml
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/input/touchscreen/focaltech,fts.example.dts:23.9-14 syntax error
FATAL ERROR: Unable to parse input tree
make[1]: *** [scripts/Makefile.lib:419: Documentation/devicetree/bindings/input/touchscreen/focaltech,fts.example.dtb] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1512: dt_binding_check] Error 2
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230312093249.1846993-2-joelselvaraj.oss@gmail.com
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
^ permalink raw reply
* Re: [lvc-project] [PATCH] Input: synaptics - remove unreachable code
From: Andi Shyti @ 2023-03-14 14:04 UTC (permalink / raw)
To: Igor Artemiev; +Cc: Dmitry Torokhov, linux-input, linux-kernel, lvc-project
In-Reply-To: <20230314124336.1494716-1-Igor.A.Artemiev@mcst.ru>
Hi Igor,
On Tue, Mar 14, 2023 at 03:43:36PM +0300, Igor Artemiev wrote:
> The synaptics_resolution() function always returnd 0.
> And there is no need to check its result.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Fixes: e839ffab0289 ("Input: synaptics - add support for Intertouch devices")
> Signed-off-by: Igor Artemiev <Igor.A.Artemiev@mcst.ru>
> ---
> drivers/input/mouse/synaptics.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index fa021af8506e..4f1182bf9667 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -444,9 +444,7 @@ static int synaptics_query_hardware(struct psmouse *psmouse,
> if (error)
> return error;
>
> - error = synaptics_resolution(psmouse, info);
> - if (error)
> - return error;
> + synaptics_resolution(psmouse, info);
Again, please make "synaptics_resolution()" void. As it is you
are just ignoring a return value, even if you are sure it's '0'.
Andi
^ permalink raw reply
* [lvc-project] [PATCH] Input: synaptics - remove unreachable code
From: Igor Artemiev @ 2023-03-14 12:43 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Igor Artemiev, linux-input, linux-kernel, lvc-project
The synaptics_resolution() function always returnd 0.
And there is no need to check its result.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes: e839ffab0289 ("Input: synaptics - add support for Intertouch devices")
Signed-off-by: Igor Artemiev <Igor.A.Artemiev@mcst.ru>
---
drivers/input/mouse/synaptics.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index fa021af8506e..4f1182bf9667 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -444,9 +444,7 @@ static int synaptics_query_hardware(struct psmouse *psmouse,
if (error)
return error;
- error = synaptics_resolution(psmouse, info);
- if (error)
- return error;
+ synaptics_resolution(psmouse, info);
return 0;
}
--
2.30.2
^ permalink raw reply related
* Re: [lvc-project] [PATCH] Input: trackpoint - remove unreachable code
From: Andi Shyti @ 2023-03-14 13:13 UTC (permalink / raw)
To: Igor Artemiev; +Cc: Dmitry Torokhov, linux-input, linux-kernel, lvc-project
In-Reply-To: <20230314122714.1494260-1-Igor.A.Artemiev@mcst.ru>
Hi Igor,
On Tue, Mar 14, 2023 at 03:27:14PM +0300, Igor Artemiev wrote:
> The trackpoint_sync() function always returnd 0.
> And there is no need to check its result.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Fixes: 2a924d71794c ("Input: trackpoint - only expose supported controls for Elan, ALPS and NXP")
> Signed-off-by: Igor Artemiev <Igor.A.Artemiev@mcst.ru>
I don't think it requires the Fixes tag... it's not really
broken.
> ---
> drivers/input/mouse/trackpoint.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/input/mouse/trackpoint.c b/drivers/input/mouse/trackpoint.c
> index 4a86b3e31d3b..561a4d2d81ff 100644
> --- a/drivers/input/mouse/trackpoint.c
> +++ b/drivers/input/mouse/trackpoint.c
> @@ -386,9 +386,7 @@ static int trackpoint_reconnect(struct psmouse *psmouse)
> was_reset = tp->variant_id == TP_VARIANT_IBM &&
> trackpoint_power_on_reset(&psmouse->ps2dev) == 0;
>
> - error = trackpoint_sync(psmouse, was_reset);
> - if (error)
> - return error;
> + trackpoint_sync(psmouse, was_reset);
what worries me here is that if this returns always '0' who tells
me that it will always return '0'?
One day someone might add an error return path and you would miss
it here.
Would it make sense to make the trackpoint_sync() a void function
as well?
Andi
>
> return 0;
> }
> --
> 2.30.2
>
^ permalink raw reply
* [lvc-project] [PATCH] Input: trackpoint - remove unreachable code
From: Igor Artemiev @ 2023-03-14 12:27 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Igor Artemiev, linux-input, linux-kernel, lvc-project
The trackpoint_sync() function always returnd 0.
And there is no need to check its result.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Fixes: 2a924d71794c ("Input: trackpoint - only expose supported controls for Elan, ALPS and NXP")
Signed-off-by: Igor Artemiev <Igor.A.Artemiev@mcst.ru>
---
drivers/input/mouse/trackpoint.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/input/mouse/trackpoint.c b/drivers/input/mouse/trackpoint.c
index 4a86b3e31d3b..561a4d2d81ff 100644
--- a/drivers/input/mouse/trackpoint.c
+++ b/drivers/input/mouse/trackpoint.c
@@ -386,9 +386,7 @@ static int trackpoint_reconnect(struct psmouse *psmouse)
was_reset = tp->variant_id == TP_VARIANT_IBM &&
trackpoint_power_on_reset(&psmouse->ps2dev) == 0;
- error = trackpoint_sync(psmouse, was_reset);
- if (error)
- return error;
+ trackpoint_sync(psmouse, was_reset);
return 0;
}
--
2.30.2
^ permalink raw reply related
* [PATCH v3 14/38] Input: gameport: add ISA and HAS_IOPORT dependencies
From: Niklas Schnelle @ 2023-03-14 12:11 UTC (permalink / raw)
To: Arnd Bergmann, Dmitry Torokhov
Cc: Greg Kroah-Hartman, Bjorn Helgaas, Uwe Kleine-König,
Mauro Carvalho Chehab, Alan Stern, Rafael J. Wysocki,
Geert Uytterhoeven, Paul Walmsley, Palmer Dabbelt, Albert Ou,
linux-kernel, linux-arch, linux-pci, Arnd Bergmann, linux-input
In-Reply-To: <20230314121216.413434-1-schnelle@linux.ibm.com>
In a future patch HAS_IOPORT=n will result in inb()/outb() and friends
not being declared. As ISA already implies HAS_IOPORT we can simply add
this dependency and guard sections of code using inb()/outb() as
alternative access methods.
Co-developed-by: Arnd Bergmann <arnd@kernel.org>
Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
drivers/input/gameport/Kconfig | 4 +++-
include/linux/gameport.h | 9 +++++++--
2 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/input/gameport/Kconfig b/drivers/input/gameport/Kconfig
index 5a2c2fb3217d..fe73b26e647a 100644
--- a/drivers/input/gameport/Kconfig
+++ b/drivers/input/gameport/Kconfig
@@ -25,6 +25,7 @@ if GAMEPORT
config GAMEPORT_NS558
tristate "Classic ISA and PnP gameport support"
+ depends on ISA
help
Say Y here if you have an ISA or PnP gameport.
@@ -35,6 +36,7 @@ config GAMEPORT_NS558
config GAMEPORT_L4
tristate "PDPI Lightning 4 gamecard support"
+ depends on ISA
help
Say Y here if you have a PDPI Lightning 4 gamecard.
@@ -53,7 +55,7 @@ config GAMEPORT_EMU10K1
config GAMEPORT_FM801
tristate "ForteMedia FM801 gameport support"
- depends on PCI
+ depends on PCI && HAS_IOPORT
help
Say Y here if you have ForteMedia FM801 PCI audio controller
(Abit AU10, Genius Sound Maker, HP Workstation zx2000,
diff --git a/include/linux/gameport.h b/include/linux/gameport.h
index 8c2f00018e89..4d5720022b63 100644
--- a/include/linux/gameport.h
+++ b/include/linux/gameport.h
@@ -167,16 +167,21 @@ static inline void gameport_trigger(struct gameport *gameport)
{
if (gameport->trigger)
gameport->trigger(gameport);
+#ifdef CONFIG_HAS_IOPORT
else
outb(0xff, gameport->io);
+#endif
}
static inline unsigned char gameport_read(struct gameport *gameport)
{
if (gameport->read)
return gameport->read(gameport);
- else
- return inb(gameport->io);
+#ifdef CONFIG_HAS_IOPORT
+ return inb(gameport->io);
+#else
+ return 0xff;
+#endif
}
static inline int gameport_cooked_read(struct gameport *gameport, int *axes, int *buttons)
--
2.37.2
^ permalink raw reply related
* [PATCH v3 13/38] Input: add HAS_IOPORT dependencies
From: Niklas Schnelle @ 2023-03-14 12:11 UTC (permalink / raw)
To: Arnd Bergmann, Dmitry Torokhov
Cc: Greg Kroah-Hartman, Bjorn Helgaas, Uwe Kleine-König,
Mauro Carvalho Chehab, Alan Stern, Rafael J. Wysocki,
Geert Uytterhoeven, Paul Walmsley, Palmer Dabbelt, Albert Ou,
linux-kernel, linux-arch, linux-pci, Arnd Bergmann, linux-input
In-Reply-To: <20230314121216.413434-1-schnelle@linux.ibm.com>
In a future patch HAS_IOPORT=n will result in inb()/outb() and friends
not being declared. We thus need to add HAS_IOPORT as dependency for
those drivers using them.
Co-developed-by: Arnd Bergmann <arnd@kernel.org>
Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
drivers/input/serio/Kconfig | 2 ++
drivers/input/touchscreen/Kconfig | 1 +
2 files changed, 3 insertions(+)
diff --git a/drivers/input/serio/Kconfig b/drivers/input/serio/Kconfig
index f39b7b3f7942..5d125627c595 100644
--- a/drivers/input/serio/Kconfig
+++ b/drivers/input/serio/Kconfig
@@ -75,6 +75,7 @@ config SERIO_Q40KBD
config SERIO_PARKBD
tristate "Parallel port keyboard adapter"
depends on PARPORT
+ depends on HAS_IOPORT
help
Say Y here if you built a simple parallel port adapter to attach
an additional AT keyboard, XT keyboard or PS/2 mouse.
@@ -148,6 +149,7 @@ config HIL_MLC
config SERIO_PCIPS2
tristate "PCI PS/2 keyboard and PS/2 mouse controller"
depends on PCI
+ depends on HAS_IOPORT
help
Say Y here if you have a Mobility Docking station with PS/2
keyboard and mice ports.
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index 1a2049b336a6..6c268b8f0d19 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -690,6 +690,7 @@ config TOUCHSCREEN_INEXIO
config TOUCHSCREEN_MK712
tristate "ICS MicroClock MK712 touchscreen"
+ depends on ISA
help
Say Y here if you have the ICS MicroClock MK712 touchscreen
controller chip in your system.
--
2.37.2
^ permalink raw reply related
* [PATCH] HID: Ignore battery for ELAN touchscreen on HP ENVY X360 13-ar0xxx
From: Luka Guzenko @ 2023-03-14 8:39 UTC (permalink / raw)
To: benjamin.tissoires; +Cc: linux-input, linux-kernel, Luka Guzenko
Ignore the reported battery level of the built-in touchscreen to suppress
battery warnings when a stylus is used. The device ID was added and the
battery ignore quirk was enabled.
Signed-off-by: Luka Guzenko <l.guzenko@web.de>
---
drivers/hid/hid-ids.h | 1 +
drivers/hid/hid-input.c | 2 ++
2 files changed, 3 insertions(+)
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 63545cd307e5..b76462881cea 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -409,6 +409,7 @@
#define USB_DEVICE_ID_TOSHIBA_CLICK_L9W 0x0401
#define USB_DEVICE_ID_HP_X2 0x074d
#define USB_DEVICE_ID_HP_X2_10_COVER 0x0755
+#define I2C_DEVICE_ID_HP_ENVY_X360_13_AR0XXX 0x23B9
#define I2C_DEVICE_ID_HP_ENVY_X360_15 0x2d05
#define I2C_DEVICE_ID_HP_ENVY_X360_15T_DR100 0x29CF
#define I2C_DEVICE_ID_HP_ENVY_X360_EU0009NV 0x2CF9
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 7fc967964dd8..9639479600f4 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -382,6 +382,8 @@ static const struct hid_device_id hid_battery_quirks[] = {
HID_BATTERY_QUIRK_AVOID_QUERY },
{ HID_USB_DEVICE(USB_VENDOR_ID_UGEE, USB_DEVICE_ID_UGEE_XPPEN_TABLET_DECO_PRO_SW),
HID_BATTERY_QUIRK_AVOID_QUERY },
+ { HID_USB_DEVICE(USB_VENDOR_ID_ELAN, I2C_DEVICE_ID_HP_ENVY_X360_13_AR0XXX),
+ HID_BATTERY_QUIRK_IGNORE },
{ HID_I2C_DEVICE(USB_VENDOR_ID_ELAN, I2C_DEVICE_ID_HP_ENVY_X360_15),
HID_BATTERY_QUIRK_IGNORE },
{ HID_I2C_DEVICE(USB_VENDOR_ID_ELAN, I2C_DEVICE_ID_HP_ENVY_X360_15T_DR100),
--
2.39.2
^ permalink raw reply related
* Re: [PATCH 2/5] Input: add driver for Focaltech FTS touchscreen
From: Joel Selvaraj @ 2023-03-14 4:33 UTC (permalink / raw)
To: Jeff LaBundy
Cc: Caleb Connolly, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Andy Gross, Bjorn Andersson, Konrad Dybcio, Henrik Rydberg,
Arnd Bergmann, Robert Jarzmik, Neil Armstrong, Markuss Broks,
Jean Delvare, Max Krummenacher, Job Noorman, Alistair Francis,
Chris Morgan, linux-input, devicetree, linux-kernel,
linux-arm-msm, ~postmarketos/upstreaming, phone-devel
In-Reply-To: <ZA/tCDOceYUZI88N@nixie71>
Hi Jeff LaBundy,
Thanks for your clear and in-depth review! It has been very helpful. I
will address them in v2.
Thanks,
Joel
^ permalink raw reply
* Re: [PATCH 2/5] Input: add driver for Focaltech FTS touchscreen
From: Jeff LaBundy @ 2023-03-14 3:42 UTC (permalink / raw)
To: Joel Selvaraj
Cc: Caleb Connolly, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Andy Gross, Bjorn Andersson, Konrad Dybcio, Henrik Rydberg,
Arnd Bergmann, Robert Jarzmik, Neil Armstrong, Markuss Broks,
Jean Delvare, Max Krummenacher, Job Noorman, Alistair Francis,
Chris Morgan, linux-input, devicetree, linux-kernel,
linux-arm-msm, ~postmarketos/upstreaming, phone-devel
In-Reply-To: <20230312093249.1846993-3-joelselvaraj.oss@gmail.com>
Hi Joel (and Caleb),
Great work! It's nice to see these devices get one step closer to
upstream. In addition to Markuss' valuable feedback, I have left a
few comments throughout.
On Sun, Mar 12, 2023 at 04:32:46AM -0500, Joel Selvaraj wrote:
> The Focaltech FTS driver supports several variants of focaltech
> touchscreens found in ~2018 era smartphones including variants found on
> the PocoPhone F1 and the SHIFT6mq which are already present in mainline.
> This driver is loosely based on the original driver from Focaltech
> but has been simplified and largely reworked.
>
> Co-developed-by: Caleb Connolly <caleb@connolly.tech>
> Signed-off-by: Caleb Connolly <caleb@connolly.tech>
> Signed-off-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>
> ---
> MAINTAINERS | 8 +
> drivers/input/touchscreen/Kconfig | 12 +
> drivers/input/touchscreen/Makefile | 1 +
> drivers/input/touchscreen/focaltech_fts.c | 448 ++++++++++++++++++++++
> 4 files changed, 469 insertions(+)
> create mode 100644 drivers/input/touchscreen/focaltech_fts.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2892858cb040..deb561c356f2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7993,6 +7993,14 @@ L: linux-input@vger.kernel.org
> S: Maintained
> F: drivers/input/joystick/fsia6b.c
>
> +FOCALTECH FTS TOUCHSCREEN DRIVER
> +M: Joel Selvaraj <joelselvaraj.oss@gmail.com>
> +M: Caleb Connolly <caleb@connolly.tech>
> +L: linux-input@vger.kernel.org
> +S: Maintained
> +F: Documentation/devicetree/bindings/input/touchscreen/focaltech,fts.yaml
> +F: drivers/input/touchscreen/focaltech_fts.c
> +
> FOCUSRITE SCARLETT GEN 2/3 MIXER DRIVER
> M: Geoffrey D. Bennett <g@b4.vu>
> L: alsa-devel@alsa-project.org (moderated for non-subscribers)
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 1a2049b336a6..320925bac3a1 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -388,6 +388,18 @@ config TOUCHSCREEN_EXC3000
> To compile this driver as a module, choose M here: the
> module will be called exc3000.
>
> +config TOUCHSCREEN_FOCALTECH_FTS
> + tristate "Focaltech FTS Touchscreen"
> + depends on I2C
> + help
> + Say Y here to enable support for I2C connected Focaltech FTS
> + based touch panels, including the 5452 and 8917 panels.
> +
> + If unsure, say N.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called focaltech_fts.
> +
> config TOUCHSCREEN_FUJITSU
> tristate "Fujitsu serial touchscreen"
> select SERIO
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index f2fd28cc34a6..83ea2e3ce754 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -45,6 +45,7 @@ obj-$(CONFIG_TOUCHSCREEN_ELO) += elo.o
> obj-$(CONFIG_TOUCHSCREEN_EGALAX) += egalax_ts.o
> obj-$(CONFIG_TOUCHSCREEN_EGALAX_SERIAL) += egalax_ts_serial.o
> obj-$(CONFIG_TOUCHSCREEN_EXC3000) += exc3000.o
> +obj-$(CONFIG_TOUCHSCREEN_FOCALTECH_FTS) += focaltech_fts.o
> obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
> obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
> obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
> diff --git a/drivers/input/touchscreen/focaltech_fts.c b/drivers/input/touchscreen/focaltech_fts.c
> new file mode 100644
> index 000000000000..d389c8b88944
> --- /dev/null
> +++ b/drivers/input/touchscreen/focaltech_fts.c
> @@ -0,0 +1,448 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + *
> + * FocalTech touchscreen driver.
> + *
> + * Copyright (c) 2010-2017, FocalTech Systems, Ltd., all rights reserved.
> + * Copyright (C) 2018 XiaoMi, Inc.
> + * Copyright (c) 2021 Caleb Connolly <caleb@connolly.tech>
> + * Copyright (c) 2023 Joel Selvaraj <joelselvaraj.oss@gmail.com>
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
The SPDX tag relieves you of the need to repeat any license terms. These
two paragraphs can be dropped.
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/input.h>
> +#include <linux/input/mt.h>
> +#include <linux/input/touchscreen.h>
> +#include <linux/slab.h>
> +#include <linux/interrupt.h>
> +#include <linux/i2c.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/delay.h>
> +#include <linux/regmap.h>
> +#include <linux/mutex.h>
> +
> +#define FTS_REG_CHIP_ID_H 0xA3
> +#define FTS_REG_CHIP_ID_L 0x9F
> +
> +#define FTS_MAX_POINTS_SUPPORT 10
> +#define FTS_ONE_TOUCH_LEN 6
> +
> +#define FTS_TOUCH_X_H_OFFSET 3
> +#define FTS_TOUCH_X_L_OFFSET 4
> +#define FTS_TOUCH_Y_H_OFFSET 5
> +#define FTS_TOUCH_Y_L_OFFSET 6
> +#define FTS_TOUCH_PRESSURE_OFFSET 7
> +#define FTS_TOUCH_AREA_OFFSET 8
> +#define FTS_TOUCH_TYPE_OFFSET 3
> +#define FTS_TOUCH_ID_OFFSET 5
> +
> +#define FTS_TOUCH_DOWN 0
> +#define FTS_TOUCH_UP 1
> +#define FTS_TOUCH_CONTACT 2
> +
> +#define FTS_DRIVER_NAME "fts-i2c"
> +#define INTERVAL_READ_REG 100 /* unit:ms */
> +#define TIMEOUT_READ_REG 2000 /* unit:ms */
In addition to what Markuss has mentioned, please namespace all #defines
(i.e. FTS_xxx). I also find these easier to read if they are all aligned
horizontally.
> +
> +#define CHIP_TYPE_5452 0x5452
> +#define CHIP_TYPE_8719 0x8719
A #define with the name equal to the value is a bit unnecessary; below I
will suggest an alternative.
> +
> +struct fts_ts_data {
> + struct i2c_client *client;
> + struct input_dev *input_dev;
> + struct touchscreen_properties prop;
> +
> + struct regmap *regmap;
> + int irq;
> +
> + struct regulator_bulk_data regulators[2];
> +
> + /* Touch data */
> + u8 max_touch_number;
> + u8 *point_buf;
> + int point_buf_size;
> +
> + /* DT data */
> + struct gpio_desc *reset_gpio;
> +};
Technically the touchscreen_properties can be DT data too. I think the
comments and newlines in the struct definition are unnecessary.
> +
> +static const struct regmap_config fts_ts_i2c_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> +};
I personally find the driver easier to read if this definition is closer
to probe, where it is used.
> +
> +static bool fts_chip_is_valid(struct fts_ts_data *data, u16 id)
> +{
> + if (id != CHIP_TYPE_5452 && id != CHIP_TYPE_8719)
> + return false;
> +
> + return true;
> +}
This isn't particularly scalable; we would end up with a massive if block
as we add compatible devices. Instead, define an array toward the beginning
of the driver as follows:
static const u16 fts_chip_types[] = {
0x5452,
0x8719,
};
And then this function could turn into something like this:
for (i = 0; i < ARRAY_SIZE(fts_chip_types); i++)
if (id = fts_chip_types[i])
return true;
return false;
This would also get rid of those obvious #defines.
> +
> +int fts_check_status(struct fts_ts_data *data)
> +{
> + int count = 0;
> + unsigned int val, id;
> +
> + do {
> + regmap_read(data->regmap, FTS_REG_CHIP_ID_H, &val);
> + id = val << 8;
> + regmap_read(data->regmap, FTS_REG_CHIP_ID_L, &val);
> + id |= val;
There are cleaner alternatives for this, such as:
__be16 val;
do {
error = regmap_raw_read(data->regmap, FTS_REG_CHIP_ID_H,
&val, sizeof(val));
if (error)
return error;
if (fts_chip_is_valid(data, be16_to_cpu(id)) {
dev_dbg(...);
return 0;
}
/* ... */
}
Please also note that there is no reason for fts_chip_is_valid() to
accept 'data' as an argument. One could also argue this function is
not doing enough work or being re-used enough to warrant being its
own function.
> +
> + if (fts_chip_is_valid(data, id)) {
> + dev_dbg(&data->client->dev, "TS Ready: Chip ID = 0x%x", id);
> + return 0;
> + }
> +
> + count++;
> + msleep(INTERVAL_READ_REG);
> + } while ((count * INTERVAL_READ_REG) < TIMEOUT_READ_REG);
> +
> + return -EIO;
> +}
> +
> +static void fts_report_touch(struct fts_ts_data *data)
> +{
> + struct input_dev *input_dev = data->input_dev;
> + int base;
> + unsigned int x, y, z, maj;
> + u8 slot, type;
> + int error, i = 0;
> +
> + u8 *buf = data->point_buf;
> +
> + memset(buf, 0, data->point_buf_size);
> +
> + error = regmap_bulk_read(data->regmap, 0, buf, data->point_buf_size);
> + if (error) {
> + dev_err_ratelimited(&data->client->dev, "I2C read failed: %d\n",
> + error);
> + return;
> + }
> +
> + for (i = 0; i < data->max_touch_number; i++) {
> + base = FTS_ONE_TOUCH_LEN * i;
> +
> + slot = (buf[base + FTS_TOUCH_ID_OFFSET]) >> 4;
> + if (slot >= data->max_touch_number)
> + break;
> +
> + x = ((buf[base + FTS_TOUCH_X_H_OFFSET] & 0x0F) << 8) +
> + (buf[base + FTS_TOUCH_X_L_OFFSET] & 0xFF);
> + y = ((buf[base + FTS_TOUCH_Y_H_OFFSET] & 0x0F) << 8) +
> + (buf[base + FTS_TOUCH_Y_L_OFFSET] & 0xFF);
It seems that the interrupt status registers can be represented with a packed
struct and then unpacked with be16_to_cpu().
> +
> + z = buf[base + FTS_TOUCH_PRESSURE_OFFSET];
> + if (z <= 0)
> + z = 0x3f;
z is unsigned; how can this happen?
> +
> + maj = buf[base + FTS_TOUCH_AREA_OFFSET] >> 4;
> + if (maj <= 0)
> + maj = 0x09;
> +
> + type = buf[base + FTS_TOUCH_TYPE_OFFSET] >> 6;
> +
> + input_mt_slot(input_dev, slot);
> + if (type == FTS_TOUCH_DOWN || type == FTS_TOUCH_CONTACT) {
> + input_mt_report_slot_state(input_dev, MT_TOOL_FINGER, true);
> + touchscreen_report_pos(data->input_dev, &data->prop, x, y, true);
> + input_report_abs(input_dev, ABS_MT_PRESSURE, z);
> + input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR, maj);
> + input_report_key(data->input_dev, BTN_TOUCH, 1);
> + } else {
> + input_report_key(data->input_dev, BTN_TOUCH, 0);
> + input_mt_report_slot_inactive(input_dev);
> + }
> + }
> + input_sync(input_dev);
We need to call input_mt_sync_frame() as well.
> +}
> +
> +static irqreturn_t fts_ts_interrupt(int irq, void *dev_id)
> +{
> + struct fts_ts_data *data = dev_id;
> +
> + fts_report_touch(data);
Assuming the act of reading the interrupt status registers is what prompts the
device to deassert its interrupt pin, I recommend promoting fts_report_touch()
to int type and evaluating its return value as follows:
return fts_report_touch(data) ? IRQ_NONE : IRQ_HANDLED;
This way if the register read fails and the interrupt is in theory not serviced,
the IRQ subsystem can mute the device and prevent an interrupt storm.
> +
> + return IRQ_HANDLED;
> +}
> +
> +static void fts_power_off(void *d)
> +{
> + struct fts_ts_data *data = d;
> +
> + regulator_bulk_disable(ARRAY_SIZE(data->regulators),
> + data->regulators);
> +}
> +
> +static int fts_start(struct fts_ts_data *data)
> +{
> + int error;
> +
> + error = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
> + data->regulators);
> + if (error) {
> + dev_err(&data->client->dev, "failed to enable regulators\n");
> + return error;
> + }
> +
> + gpiod_set_value_cansleep(data->reset_gpio, 0);
> + msleep(200);
> +
> + enable_irq(data->irq);
> +
> + return 0;
> +}
> +
> +static int fts_stop(struct fts_ts_data *data)
> +{
> + disable_irq(data->irq);
> + gpiod_set_value_cansleep(data->reset_gpio, 1);
> + fts_power_off(data);
> +
> + return 0;
> +}
> +
> +static int fts_input_open(struct input_dev *dev)
> +{
> + struct fts_ts_data *data = input_get_drvdata(dev);
> + int error;
> +
> + error = fts_start(data);
> + if (error)
> + return error;
> +
> + error = fts_check_status(data);
> + if (error) {
> + dev_err(&data->client->dev, "Failed to start or unsupported chip");
> + return error;
> + }
> +
> + return 0;
> +}
> +
> +static void fts_input_close(struct input_dev *dev)
> +{
> + struct fts_ts_data *data = input_get_drvdata(dev);
> +
> + fts_stop(data);
> +}
> +
> +static int fts_input_init(struct fts_ts_data *data)
> +{
> + struct device *dev = &data->client->dev;
> + struct input_dev *input_dev;
> + int error = 0;
> +
> + input_dev = devm_input_allocate_device(dev);
> + if (!input_dev)
> + return -ENOMEM;
> +
> + data->input_dev = input_dev;
> +
> + /* Init and register Input device */
This comment is obvious and not entirely true, as we're not actually registering
the device until farther down.
> + input_dev->name = FTS_DRIVER_NAME;
> + input_dev->id.bustype = BUS_I2C;
> + input_dev->dev.parent = dev;
> + input_dev->open = fts_input_open;
> + input_dev->close = fts_input_close;
> +
> + input_set_drvdata(input_dev, data);
> +
> + __set_bit(EV_SYN, input_dev->evbit);
No need to set EV_SYN; input_register_device() does that for us.
> + __set_bit(EV_ABS, input_dev->evbit);
No need to set EV_ABS; input_set_capability(..., EV_ABS, ...) does that for us.
> + __set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
Same here; input_mt_init_slots() is already doing this.
> +
> + input_mt_init_slots(input_dev, data->max_touch_number,
> + INPUT_MT_DIRECT);
Please check the return value of input_mt_init_slots(), and move this call
_after_ your axes have been defined. Otherwise, this function cannot copy
the multitouch axes to single-touch axes, and drivers such as libinput will
ignore this device.
> + input_set_capability(input_dev, EV_ABS, ABS_MT_POSITION_X);
> + input_set_capability(input_dev, EV_ABS, ABS_MT_POSITION_Y);
> + input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
> + input_set_abs_params(input_dev, ABS_MT_PRESSURE, 0, 255, 0, 0);
> +
> + touchscreen_parse_properties(input_dev, true, &data->prop);
> + if (!data->prop.max_x || !data->prop.max_y) {
> + dev_err(dev,
> + "touchscreen-size-x and/or touchscreen-size-y not set in dts\n");
Note that dts may not always be the source of these (see comments below).
> + return -EINVAL;
> + }
> +
> + data->point_buf_size = (data->max_touch_number * FTS_ONE_TOUCH_LEN) + 3;
> + data->point_buf = devm_kzalloc(dev, data->point_buf_size, GFP_KERNEL);
> + if (!data->point_buf) {
> + dev_err(dev, "Failed to alloc memory for point buffer\n");
Did checkpatch not gripe about this? No need to print a warning for failed
memory allocation.
> + return -ENOMEM;
> + }
> +
> + error = input_register_device(input_dev);
> + if (error) {
> + dev_err(dev, "Failed to register input device\n");
> + return error;
> + }
> +
> + return 0;
> +}
> +
> +static int fts_parse_dt(struct fts_ts_data *data)
> +{
> + int error = 0;
> + struct device *dev = &data->client->dev;
> + struct device_node *np = dev->of_node;
> + u32 val;
> +
> + error = of_property_read_u32(np, "focaltech,max-touch-number", &val);
> + if (error) {
> + dev_err(dev, "Unable to read property 'focaltech,max-touch-number'");
> + return -EINVAL;
Just return 'error' rather than papering over the actual error code.
> + }
Please switch to the device_property API to allow easy adoption of ACPI
at a later time. To that end, it is not necessarily appropriate to use
the term 'dt' in the name of this function.
That being said, I do agree with Krzysztof's point in the binding review;
it seems you are relying on the user to know the hardware limitation. In
my opinion this is the driver's responsibility.
I think it is OK if you want the user to have the option to artificially
limit the number of points, but in that case the property should be optional.
> + if (val < 2 || val > FTS_MAX_POINTS_SUPPORT) {
> + dev_err(dev, "'focaltech,max-touch-number' out of range [2, %d]",
> + FTS_MAX_POINTS_SUPPORT);
> + return -EINVAL;
> + }
Since FTS_MAX_POINTS_SUPPORT exists, it seems FTS_MIN_POINTS_SUPPORT should too.
That being said, why not to support single-touch applications? It does not seem
you write this value back to a device register, so will the device not simply
report one contact if that is all that exists? input_mt_init_slots() is perfectly
happy with one slot.
> + data->max_touch_number = val;
> +
> + data->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(data->reset_gpio)) {
> + error = PTR_ERR(data->reset_gpio);
> + dev_err(dev, "Failed to request reset gpio, error %d\n", error);
> + return error;
> + }
Having a hardware reset pin is nice, but often it is given up in case the board
designer runs out of GPIO and pulls the reset pin up locally. Can it be optional?
> +
> + return 0;
> +}
> +
> +static int fts_ts_probe(struct i2c_client *client)
> +{
> + int error = 0;
> + struct fts_ts_data *data;
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> + dev_err(&client->dev, "I2C not supported");
> + return -ENODEV;
> + }
> +
> + if (!client->irq) {
> + dev_err(&client->dev, "No irq specified\n");
> + return -EINVAL;
> + }
> +
> + data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + data->client = client;
> +
> + error = fts_parse_dt(data);
> + if (error)
> + return error;
> +
> + i2c_set_clientdata(client, data);
> +
> + data->regmap = devm_regmap_init_i2c(client, &fts_ts_i2c_regmap_config);
> + if (IS_ERR(data->regmap)) {
> + dev_err(&client->dev, "regmap allocation failed\n");
It would be nice to save and print the error code as you do above for the
reset GPIO.
> + return PTR_ERR(data->regmap);
> + }
> +
> + /*
> + * AVDD is the analog voltage supply (2.6V to 3.3V)
> + * VDDIO is the digital voltage supply (1.8V)
> + */
> + data->regulators[0].supply = "avdd";
> + data->regulators[1].supply = "vddio";
> + error = devm_regulator_bulk_get(&client->dev, ARRAY_SIZE(data->regulators),
> + data->regulators);
> + if (error) {
> + dev_err(&client->dev, "Failed to get regulators %d\n", error);
> + return error;
> + }
> +
> + error = devm_add_action_or_reset(&client->dev, fts_power_off, data);
> + if (error) {
> + dev_err(&client->dev, "failed to install power off handler\n");
> + return error;
> + }
> +
> + error = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> + fts_ts_interrupt, IRQF_ONESHOT,
> + client->name, data);
Nit: alignment
> + if (error) {
> + dev_err(&client->dev, "Failed to request IRQ: %d\n", error);
> + return error;
> + }
> +
> + error = fts_input_init(data);
> + if (error)
> + return error;
> +
> + return 0;
> +}
> +
> +static int fts_pm_suspend(struct device *dev)
> +{
> + struct fts_ts_data *data = dev_get_drvdata(dev);
> +
> + mutex_lock(&data->input_dev->mutex);
> +
> + if (input_device_enabled(data->input_dev))
> + fts_stop(data);
> +
> + mutex_unlock(&data->input_dev->mutex);
> +
> + return 0;
> +}
> +
> +static int fts_pm_resume(struct device *dev)
> +{
> + struct fts_ts_data *data = dev_get_drvdata(dev);
> + int error = 0;
> +
> + mutex_lock(&data->input_dev->mutex);
> +
> + if (input_device_enabled(data->input_dev))
> + error = fts_start(data);
> +
> + mutex_unlock(&data->input_dev->mutex);
> +
> + return error;
> +}
> +
> +static const struct dev_pm_ops fts_dev_pm_ops = {
> + .suspend = fts_pm_suspend,
> + .resume = fts_pm_resume,
> +};
Please use the new DEFINE_SIMPLE_DEV_PM_OPS and pm_sleep_ptr().
> +
> +static const struct of_device_id fts_match_table[] = {
> + { .compatible = "focaltech,fts5452", },
> + { .compatible = "focaltech,fts8719", },
> + { /* sentinel */ },
Nit: no need for a trailing comma after the sentinel, as no patch would
ever add a line below it.
> +};
> +
> +MODULE_DEVICE_TABLE(of, fts_match_table);
> +
> +static struct i2c_driver fts_ts_driver = {
> + .probe_new = fts_ts_probe,
> + .driver = {
> + .name = FTS_DRIVER_NAME,
> + .pm = &fts_dev_pm_ops,
> + .of_match_table = fts_match_table,
> + },
> +};
> +module_i2c_driver(fts_ts_driver);
> +
> +MODULE_AUTHOR("Joel Selvaraj <joelselvaraj.oss@gmail.com>");
> +MODULE_AUTHOR("Caleb Connolly <caleb@connolly.tech>");
> +MODULE_DESCRIPTION("FocalTech touchscreen Driver");
Nit: inconsistent capitalization
> +MODULE_LICENSE("GPL");
> --
> 2.39.2
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox