* Re: [RFC PATCH 2/2] hid: lenovo: move type checks to lenovo_features_set_cptkbd()
From: Jamie Lentin @ 2023-09-30 9:58 UTC (permalink / raw)
To: Martin Kepplinger; +Cc: jikos, benjamin.tissoires, linux-kernel, linux-input
In-Reply-To: <2d1e21cc9677a5cfe828decd8cbd5e930237e76d.camel@posteo.de>
On 2023-09-30 10:26, Martin Kepplinger wrote:
> Am Donnerstag, dem 28.09.2023 um 22:06 +0100 schrieb Jamie Lentin:
>> On 2023-09-27 12:20, Martin Kepplinger wrote:
>> > Am Mittwoch, dem 27.09.2023 um 09:19 +0100 schrieb Jamie Lentin:
>> > > On 2023-09-25 11:23, Martin Kepplinger wrote:
>> > > > These custom commands will be sent to both the USB keyboard &
>> > > > mouse
>> > > > devices but only the mouse will respond. Avoid sending known-
>> > > > useless
>> > > > messages by always prepending the filter before sending them.
>> > > >
>> > > > Suggested-by: Jamie Lentin <jm@lentin.co.uk>
>> > > > Signed-off-by: Martin Kepplinger <martink@posteo.de>
>> > > > ---
>> > > > drivers/hid/hid-lenovo.c | 27 +++++++++------------------
>> > > > 1 file changed, 9 insertions(+), 18 deletions(-)
>> > > >
>> > > > diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-
>> > > > lenovo.c
>> > > > index 29aa6d372bad..922f3e5462f4 100644
>> > > > --- a/drivers/hid/hid-lenovo.c
>> > > > +++ b/drivers/hid/hid-lenovo.c
>> > > > @@ -521,6 +521,14 @@ static void
>> > > > lenovo_features_set_cptkbd(struct
>> > > > hid_device *hdev)
>> > > > int ret;
>> > > > struct lenovo_drvdata *cptkbd_data =
>> > > > hid_get_drvdata(hdev);
>> > > >
>> > > > + /* All the custom action happens on the USBMOUSE device
>> > > > for
>> > > > USB */
>> > > > + if (((hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD) ||
>> > > > + (hdev->product == USB_DEVICE_ID_LENOVO_TPIIUSBKBD))
>> > > > &&
>> > > > + hdev->type != HID_TYPE_USBMOUSE) {
>> > > > + hid_dbg(hdev, "Ignoring keyboard half of
>> > > > device\n");
>> > > > + return;
>> > > > + }
>> > > > +
>> > > > /*
>> > > > * Tell the keyboard a driver understands it, and turn
>> > > > F7,
>> > > > F9, F11
>> > > > into
>> > > > * regular keys
>> > > > @@ -1122,14 +1130,6 @@ static int lenovo_probe_cptkbd(struct
>> > > > hid_device
>> > > > *hdev)
>> > > > int ret;
>> > > > struct lenovo_drvdata *cptkbd_data;
>> > > >
>> > > > - /* All the custom action happens on the USBMOUSE device
>> > > > for
>> > > > USB */
>> > > > - if (((hdev->product == USB_DEVICE_ID_LENOVO_CUSBKBD) ||
>> > > > - (hdev->product == USB_DEVICE_ID_LENOVO_TPIIUSBKBD))
>> > > > &&
>> > > > - hdev->type != HID_TYPE_USBMOUSE) {
>> > > > - hid_dbg(hdev, "Ignoring keyboard half of
>> > > > device\n");
>> > > > - return 0;
>> > > > - }
>> > > > -
>> > >
>> > > I like the idea of doing it once then forgetting about it, but
>> > > removing
>> > > this will mean that the "keyboard half" will have it's own set of
>> > > non-functional sysfs parameters I think? Currently:-
>> > >
>> > > # evtest
>> > > . . .
>> > > /dev/input/event10: ThinkPad Compact Bluetooth Keyboard with
>> > > TrackPoint
>> > > /dev/input/event11: Lenovo ThinkPad Compact USB Keyboard with
>> > > TrackPoint
>> > > /dev/input/event12: Lenovo ThinkPad Compact USB Keyboard with
>> > > TrackPoint
>> > >
>> > > # ls -1 /sys/class/input/event*/device/device/fn_lock
>> > > /sys/class/input/event10/device/device/fn_lock
>> > > /sys/class/input/event12/device/device/fn_lock
>> > >
>> > > (note 11 is missing.)
>> > >
>> > > I think the easiest (but ugly) thing to do is to copy-paste this
>> > > lump
>> > > of
>> > > code to the top of lenovo_reset_resume.
>> > > Cheers,
>> > >
>> > > > cptkbd_data = devm_kzalloc(&hdev->dev,
>> > > > sizeof(*cptkbd_data),
>> > > > GFP_KERNEL);
>> > > > @@ -1264,16 +1264,7 @@ static int lenovo_probe(struct
>> > > > hid_device
>> > > > *hdev,
>> > > > #ifdef CONFIG_PM
>> > > > static int lenovo_reset_resume(struct hid_device *hdev)
>> > > > {
>> > > > - switch (hdev->product) {
>> > > > - case USB_DEVICE_ID_LENOVO_CUSBKBD:
>> > > > - if (hdev->type == HID_TYPE_USBMOUSE) {
>> > > > - lenovo_features_set_cptkbd(hdev);
>> > > > - }
>> > > > -
>> > > > - break;
>> > > > - default:
>> > > > - break;
>> > > > - }
>> > > > + lenovo_features_set_cptkbd(hdev);
>> >
>> > ok. ignore my change (this whole patch) and look at your addition
>> > here,
>> > don't you already make sure only the mouse-part gets the messages?
>> > you
>> > just write switch()case instead of if(); what do you think is
>> > missing
>> > here?
>>
>> Correct, this switch statement() that you're removing in this patch
>> already does exactly this, so replacing it with the
>> if()-and-return-early block would result in equivalent code (ignoring
>> the Trackpoint keyboard II). That suggestion wasn't the most helpful
>> of
>> mine, sorry!
>>
>> The reason I originally used a switch here is for symmetry with
>> lenovo_probe(), lenovo_remove(), etc. It might some day be useful to
>> add
>> something like:
>>
>> case USB_DEVICE_ID_LENOVO_X1_TAB:
>> lenovo_reset_resume_tp10ubkbd(hdev);
>> break;
>>
>> ...to the switch. For completeness, lenovo_reset_resume() should
>> probably call a separate lenovo_reset_resume_cptkbd() that does the
>> work. For just 3 lines of code it didn't seem worth it at the time
>> though.
>>
>> Cheers,
>
> ok your original patch seems to basically be a valid first fix. Should
> I send it on your behalf (with you as author) or do you want to send it
> yourself? Let's get this fixed :)
If it's working for you and you don't mind, feel free send it on my
behalf. I don't have the hardware to test the patch currently.
Thanks!
>
> thanks,
> martin
^ permalink raw reply
* Re: [PATCH 2/2] Input: add Himax HX852x(ES) touchscreen driver
From: Stephan Gerhold @ 2023-09-30 14:27 UTC (permalink / raw)
To: Jeff LaBundy
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, linux-input, devicetree, linux-kernel,
Jonathan Albrieux
In-Reply-To: <ZROaqRiWa6ReVH/D@nixie71>
Hi Jeff,
On Tue, Sep 26, 2023 at 09:59:53PM -0500, Jeff LaBundy wrote:
> On Sun, Sep 17, 2023 at 07:05:50PM +0200, Stephan Gerhold wrote:
> > On Sat, Sep 16, 2023 at 03:47:55PM -0500, Jeff LaBundy wrote:
> > > On Wed, Sep 13, 2023 at 03:25:30PM +0200, Stephan Gerhold wrote:
> > > > From: Jonathan Albrieux <jonathan.albrieux@gmail.com>
> [...]
> > > > +static int hx852x_probe(struct i2c_client *client)
> > > > +{
> > > > + struct device *dev = &client->dev;
> > > > + struct hx852x *hx;
> > > > + int error, i;
> > > > +
> > > > + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C |
> > > > + I2C_FUNC_SMBUS_WRITE_BYTE |
> > > > + I2C_FUNC_SMBUS_WRITE_BYTE_DATA |
> > > > + I2C_FUNC_SMBUS_WRITE_WORD_DATA)) {
> > > > + dev_err(dev, "not all i2c functionality supported\n");
> > > > + return -ENXIO;
> > > > + }
> > > > +
> > > > + hx = devm_kzalloc(dev, sizeof(*hx), GFP_KERNEL);
> > > > + if (!hx)
> > > > + return -ENOMEM;
> > > > +
> > > > + hx->client = client;
> > > > + hx->input_dev = devm_input_allocate_device(dev);
> > > > + if (!hx->input_dev)
> > > > + return -ENOMEM;
> > > > +
> > > > + hx->input_dev->name = "Himax HX852x";
> > > > + hx->input_dev->id.bustype = BUS_I2C;
> > > > + hx->input_dev->open = hx852x_input_open;
> > > > + hx->input_dev->close = hx852x_input_close;
> > > > +
> > > > + i2c_set_clientdata(client, hx);
> > > > + input_set_drvdata(hx->input_dev, hx);
> > > > +
> > > > + hx->supplies[0].supply = "vcca";
> > > > + hx->supplies[1].supply = "vccd";
> > > > + error = devm_regulator_bulk_get(dev, ARRAY_SIZE(hx->supplies), hx->supplies);
> > > > + if (error < 0)
> > > > + return dev_err_probe(dev, error, "failed to get regulators");
> > > > +
> > > > + hx->reset_gpiod = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> > > > + if (IS_ERR(hx->reset_gpiod))
> > > > + return dev_err_probe(dev, error, "failed to get reset gpio");
> > >
> > > Can the reset GPIO be optional?
> > >
> >
> > I'm afraid I have no idea if the controller needs this or not. Would it
> > be better to keep it required until someone confirms otherwise or have
> > it optional for the other way around?
>
> If you have a datasheet handy, or your hardware provides a means for you to
> test and confirm whether reset can be left out, I would make the reset GPIO
> optional. Often times, these controllers are part of a module and reset may
> be tied high locally as opposed to adding another signal to a flex cable.
>
> If you have no way to confirm, I would keep it as required for now; it is not
> too cumbersome to be changed later if the need arises on different hardware.
>
I don't have a datasheet unfortunately. :(
However, I tried to simulate this case on my board by keeping the reset
GPIO permanently de-asserted (i.e. high because of active-low). The
results are not entirely conclusive: The controller seems to respond to
commands and the initial configuration is read correctly. However, it
does not report any touch events. As soon as I add the temporary
assertion of the reset signal back it works fine again.
I suspect toggling the reset signal might be required to make the
controller come properly out of reset. I'll keep it required to be sure.
Thanks,
Stephan
^ permalink raw reply
* Re: [PATCH 2/2] Input: add Himax HX852x(ES) touchscreen driver
From: Stephan Gerhold @ 2023-09-30 15:28 UTC (permalink / raw)
To: Jeff LaBundy
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, linux-input, devicetree, linux-kernel,
Jonathan Albrieux
In-Reply-To: <ZQYUe46/rj8jqNvg@nixie71>
Hi Jeff,
On Sat, Sep 16, 2023 at 03:47:55PM -0500, Jeff LaBundy wrote:
> On Wed, Sep 13, 2023 at 03:25:30PM +0200, Stephan Gerhold wrote:
> > From: Jonathan Albrieux <jonathan.albrieux@gmail.com>
> >
> > Add a simple driver for the Himax HX852x(ES) touch panel controller,
> > with support for multi-touch and capacitive touch keys.
> >
> > The driver is somewhat based on sample code from Himax. However, that
> > code was so extremely confusing that we spent a significant amount of
> > time just trying to understand the packet format and register commands.
> > In this driver they are described with clean structs and defines rather
> > than lots of magic numbers and offset calculations.
> >
> > Signed-off-by: Jonathan Albrieux <jonathan.albrieux@gmail.com>
> > Co-developed-by: Stephan Gerhold <stephan@gerhold.net>
> > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > ---
> > MAINTAINERS | 7 +
> > drivers/input/touchscreen/Kconfig | 10 +
> > drivers/input/touchscreen/Makefile | 1 +
> > drivers/input/touchscreen/himax_hx852x.c | 491 +++++++++++++++++++++++++++++++
> > 4 files changed, 509 insertions(+)
> >
> > [...]
> > diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> > index 62bd24f3ac8e..f42a87faa86c 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_HIDEEP) += hideep.o
> > +obj-$(CONFIG_TOUCHSCREEN_HIMAX_HX852X) += himax_hx852x.o
> > obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
> > obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
> > obj-$(CONFIG_TOUCHSCREEN_ILITEK) += ilitek_ts_i2c.o
> > diff --git a/drivers/input/touchscreen/himax_hx852x.c b/drivers/input/touchscreen/himax_hx852x.c
> > new file mode 100644
> > index 000000000000..31616dcfc5ab
> > --- /dev/null
> > +++ b/drivers/input/touchscreen/himax_hx852x.c
> > @@ -0,0 +1,491 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Himax HX852x(ES) Touchscreen Driver
> > + * Copyright (c) 2020-2023 Stephan Gerhold <stephan@gerhold.net>
> > + * Copyright (c) 2020 Jonathan Albrieux <jonathan.albrieux@gmail.com>
> > + *
> > + * Based on the Himax Android Driver Sample Code Ver 0.3 for HMX852xES chipset:
> > + * Copyright (c) 2014 Himax Corporation.
> > + */
> > +
> > +#include <asm/unaligned.h>
> > +#include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/i2c.h>
> > +#include <linux/input.h>
> > +#include <linux/input/mt.h>
> > +#include <linux/input/touchscreen.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
>
> Please explicitly #include of_device.h.
>
In v2 I have added linux/of.h and linux/mod_devicetable.h, since I'm
actually using definitions from these two only. Seems like including
of_device.h is discouraged nowadays, see commit dbce1a7d5dce ("Input:
Explicitly include correct DT includes").
> > [...]
> > +static void hx852x_stop(struct hx852x *hx)
> > +{
> > + struct device *dev = &hx->client->dev;
> > + int error;
> > +
> > + error = i2c_smbus_write_byte(hx->client, HX852X_TS_SENSE_OFF);
> > + if (error)
> > + dev_err(dev, "failed to send TS_SENSE_OFF: %d\n", error);
>
> Granted the function is of void type, should we not still return if there
> is an error?
>
> Actually, I would still keep this function as an int for future re-use, even
> though hx852x_input_close() simply ignores the value. This way, the return
> value can be propagated to the return value of hx852x_suspend() and elsewhere.
>
> > +
> > + msleep(20);
> > +
> > + error = i2c_smbus_write_byte(hx->client, HX852X_TS_SLEEP_IN);
> > + if (error)
> > + dev_err(dev, "failed to send TS_SLEEP_IN: %d\n", error);
>
> Same here; no need to sleep following a register write that seemingly did
> not happen.
>
> > +
> > + msleep(30);
> > +}
> > +
> > +static void hx852x_power_off(struct hx852x *hx)
> > +{
> > + struct device *dev = &hx->client->dev;
> > + int error;
> > +
> > + error = regulator_bulk_disable(ARRAY_SIZE(hx->supplies), hx->supplies);
> > + if (error)
> > + dev_err(dev, "failed to disable regulators: %d\n", error);
> > +}
>
> Same comment with regard to function type; it's nice for internal helpers
> to be of type int, even if the core callback using it is void.
>
> > +
> > +static int hx852x_read_config(struct hx852x *hx)
> > +{
> > + struct device *dev = &hx->client->dev;
> > + struct hx852x_config conf = {0};
> > + int x_res, y_res;
> > + int error;
> > +
> > + error = hx852x_power_on(hx);
> > + if (error)
> > + return error;
> > +
> > + /* Sensing must be turned on briefly to load the config */
> > + error = hx852x_start(hx);
> > + if (error)
> > + goto power_off;
> > +
> > + hx852x_stop(hx);
>
> See my earlier comment about promoting this function's type to int.
>
> > +
> > + error = i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH,
> > + HX852X_SRAM_SWITCH_TEST_MODE);
> > + if (error)
> > + goto power_off;
> > +
> > + error = i2c_smbus_write_word_data(hx->client, HX852X_REG_SRAM_ADDR,
> > + HX852X_SRAM_ADDR_CONFIG);
> > + if (error)
> > + goto exit_test_mode;
> > +
> > + error = hx852x_i2c_read(hx, HX852X_REG_FLASH_RPLACE, &conf, sizeof(conf));
> > + if (error)
> > + goto exit_test_mode;
> > +
> > + x_res = be16_to_cpu(conf.x_res);
> > + y_res = be16_to_cpu(conf.y_res);
> > + hx->max_fingers = (conf.max_pt & 0xf0) >> 4;
> > + dev_dbg(dev, "x res: %d, y res: %d, max fingers: %d\n",
> > + x_res, y_res, hx->max_fingers);
> > +
> > + if (hx->max_fingers > HX852X_MAX_FINGERS) {
> > + dev_err(dev, "max supported fingers: %d, found: %d\n",
> > + HX852X_MAX_FINGERS, hx->max_fingers);
> > + error = -EINVAL;
> > + goto exit_test_mode;
> > + }
> > +
> > + if (x_res && y_res) {
> > + input_set_abs_params(hx->input_dev, ABS_MT_POSITION_X, 0, x_res - 1, 0, 0);
> > + input_set_abs_params(hx->input_dev, ABS_MT_POSITION_Y, 0, y_res - 1, 0, 0);
> > + }
> > +
> > +exit_test_mode:
> > + i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH, 0);
>
> Nit: it would still be nice to preserve as many return values as possible, perhaps
> as follows:
>
> +exit_test_mode:
> error = i2c_smbus_write_byte_data(...) ? : error;
>
> > +power_off:
> > + hx852x_power_off(hx);
> > + return error;
>
> Similarly, with hx852x_power_off() being promoted to int as suggested above,
> this could be:
>
> return hx852x_power_off(...) ? : error;
>
> There are other idiomatic ways to do the same thing based on your preference.
> Another (perhaps more clear) option would be to move some of these test mode
> functions into a helper, which would also avoid some goto statements.
>
I played with this for a bit. A problem of the "? : error" approach is
that it hides the original error in case the new calls error again.
Let's assume
error = hx852x_start(hx);
if (error)
goto power_off;
fails with error = -ENXIO. We jump to power_off:
power_off:
return hx852x_power_off(hx) ? : error;
Let's say for whatever reason hx852x_power_off() fails too but returns
-EINVAL. Then the final return value will be -EINVAL, while with the
current approach in this patch it would return the original cause
(-ENXIO). I think that's more clear.
I also played with moving code to a separate function to avoid the
gotos, but I feel like that makes the fairly focused logic of this
function (reading the configuration by temporarily entering the test
mode) just more confusing.
To still fix the error handling I ended up with duplicating the
"success" code path and the "error" code path (it's just two function
calls), i.e.:
error = i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH, 0);
if (error)
goto err_power_off;
return hx852x_power_off(hx);
err_test_mode:
i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH, 0);
err_power_off:
hx852x_power_off(hx);
return error;
I hope that's fine too. A bit ugly maybe but in this case I would prefer
having the main code path (reading the configuration) clearly readable.
Let me know if you have a better suggestion for these (I'll send v2 in a
bit so that you can see the full diff there).
Thanks!
Stephan
^ permalink raw reply
* [PATCH v2 2/2] Input: add Himax HX852x(ES) touchscreen driver
From: Stephan Gerhold @ 2023-09-30 15:32 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Henrik Rydberg,
linux-input, devicetree, linux-kernel, Jeff LaBundy,
Christophe JAILLET, Jonathan Albrieux, Stephan Gerhold
In-Reply-To: <20230930-hx852x-v2-0-c5821947b225@gerhold.net>
From: Jonathan Albrieux <jonathan.albrieux@gmail.com>
Add a simple driver for the Himax HX852x(ES) touch panel controller,
with support for multi-touch and capacitive touch keys.
The driver is somewhat based on sample code from Himax. However, that
code was so extremely confusing that we spent a significant amount of
time just trying to understand the packet format and register commands.
In this driver they are described with clean structs and defines rather
than lots of magic numbers and offset calculations.
Signed-off-by: Jonathan Albrieux <jonathan.albrieux@gmail.com>
Co-developed-by: Stephan Gerhold <stephan@gerhold.net>
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
MAINTAINERS | 7 +
drivers/input/touchscreen/Kconfig | 10 +
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/himax_hx852x.c | 499 +++++++++++++++++++++++++++++++
4 files changed, 517 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 90f13281d297..c551c60b0598 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9331,6 +9331,13 @@ S: Maintained
F: Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml
F: drivers/input/touchscreen/himax_hx83112b.c
+HIMAX HX852X TOUCHSCREEN DRIVER
+M: Stephan Gerhold <stephan@gerhold.net>
+L: linux-input@vger.kernel.org
+S: Maintained
+F: Documentation/devicetree/bindings/input/touchscreen/himax,hx852es.yaml
+F: drivers/input/touchscreen/himax_hx852x.c
+
HIPPI
M: Jes Sorensen <jes@trained-monkey.org>
L: linux-hippi@sunsite.dk
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index e3e2324547b9..8e5667ae5dab 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -427,6 +427,16 @@ config TOUCHSCREEN_HIDEEP
To compile this driver as a module, choose M here : the
module will be called hideep_ts.
+config TOUCHSCREEN_HIMAX_HX852X
+ tristate "Himax HX852x(ES) touchscreen"
+ depends on I2C
+ help
+ Say Y here if you have a Himax HX852x(ES) touchscreen.
+ If unsure, say N.
+
+ To compile this driver as a module, choose M here: the module
+ will be called himax_hx852x.
+
config TOUCHSCREEN_HYCON_HY46XX
tristate "Hycon hy46xx touchscreen support"
depends on I2C
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 62bd24f3ac8e..f42a87faa86c 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_HIDEEP) += hideep.o
+obj-$(CONFIG_TOUCHSCREEN_HIMAX_HX852X) += himax_hx852x.o
obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
obj-$(CONFIG_TOUCHSCREEN_ILITEK) += ilitek_ts_i2c.o
diff --git a/drivers/input/touchscreen/himax_hx852x.c b/drivers/input/touchscreen/himax_hx852x.c
new file mode 100644
index 000000000000..98a55be7891d
--- /dev/null
+++ b/drivers/input/touchscreen/himax_hx852x.c
@@ -0,0 +1,499 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Himax HX852x(ES) Touchscreen Driver
+ * Copyright (c) 2020-2023 Stephan Gerhold <stephan@gerhold.net>
+ * Copyright (c) 2020 Jonathan Albrieux <jonathan.albrieux@gmail.com>
+ *
+ * Based on the Himax Android Driver Sample Code Ver 0.3 for HMX852xES chipset:
+ * Copyright (c) 2014 Himax Corporation.
+ */
+
+#include <asm/unaligned.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/input.h>
+#include <linux/input/mt.h>
+#include <linux/input/touchscreen.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regulator/consumer.h>
+
+#define HX852X_COORD_SIZE(fingers) ((fingers) * sizeof(struct hx852x_coord))
+#define HX852X_WIDTH_SIZE(fingers) ALIGN(fingers, 4)
+#define HX852X_BUF_SIZE(fingers) (HX852X_COORD_SIZE(fingers) + \
+ HX852X_WIDTH_SIZE(fingers) + \
+ sizeof(struct hx852x_touch_info))
+
+#define HX852X_MAX_FINGERS 12
+#define HX852X_MAX_KEY_COUNT 4
+#define HX852X_MAX_BUF_SIZE HX852X_BUF_SIZE(HX852X_MAX_FINGERS)
+
+#define HX852X_TS_SLEEP_IN 0x80
+#define HX852X_TS_SLEEP_OUT 0x81
+#define HX852X_TS_SENSE_OFF 0x82
+#define HX852X_TS_SENSE_ON 0x83
+#define HX852X_READ_ONE_EVENT 0x85
+#define HX852X_READ_ALL_EVENTS 0x86
+#define HX852X_READ_LATEST_EVENT 0x87
+#define HX852X_CLEAR_EVENT_STACK 0x88
+
+#define HX852X_REG_SRAM_SWITCH 0x8c
+#define HX852X_REG_SRAM_ADDR 0x8b
+#define HX852X_REG_FLASH_RPLACE 0x5a
+
+#define HX852X_SRAM_SWITCH_TEST_MODE 0x14
+#define HX852X_SRAM_ADDR_CONFIG 0x7000
+
+struct hx852x {
+ struct i2c_client *client;
+ struct input_dev *input_dev;
+ struct touchscreen_properties props;
+ struct gpio_desc *reset_gpiod;
+ struct regulator_bulk_data supplies[2];
+ unsigned int max_fingers;
+ unsigned int keycount;
+ unsigned int keycodes[HX852X_MAX_KEY_COUNT];
+};
+
+struct hx852x_config {
+ u8 rx_num;
+ u8 tx_num;
+ u8 max_pt;
+ u8 padding1[3];
+ __be16 x_res;
+ __be16 y_res;
+ u8 padding2[2];
+} __packed __aligned(4);
+
+struct hx852x_coord {
+ __be16 x;
+ __be16 y;
+} __packed __aligned(4);
+
+struct hx852x_touch_info {
+ u8 finger_num;
+ __le16 finger_pressed;
+ u8 padding;
+} __packed __aligned(4);
+
+static int hx852x_i2c_read(struct hx852x *hx, u8 cmd, void *data, u16 len)
+{
+ struct i2c_client *client = hx->client;
+ int ret;
+
+ struct i2c_msg msg[] = {
+ {
+ .addr = client->addr,
+ .flags = 0,
+ .len = 1,
+ .buf = &cmd,
+ },
+ {
+ .addr = client->addr,
+ .flags = I2C_M_RD,
+ .len = len,
+ .buf = data,
+ }
+ };
+
+ ret = i2c_transfer(client->adapter, msg, ARRAY_SIZE(msg));
+ if (ret != ARRAY_SIZE(msg)) {
+ dev_err(&client->dev, "failed to read %#x: %d\n", cmd, ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int hx852x_power_on(struct hx852x *hx)
+{
+ struct device *dev = &hx->client->dev;
+ int error;
+
+ error = regulator_bulk_enable(ARRAY_SIZE(hx->supplies), hx->supplies);
+ if (error) {
+ dev_err(dev, "failed to enable regulators: %d\n", error);
+ return error;
+ }
+
+ gpiod_set_value_cansleep(hx->reset_gpiod, 1);
+ msleep(20);
+ gpiod_set_value_cansleep(hx->reset_gpiod, 0);
+ msleep(20);
+
+ return 0;
+}
+
+static int hx852x_start(struct hx852x *hx)
+{
+ struct device *dev = &hx->client->dev;
+ int error;
+
+ error = i2c_smbus_write_byte(hx->client, HX852X_TS_SLEEP_OUT);
+ if (error) {
+ dev_err(dev, "failed to send TS_SLEEP_OUT: %d\n", error);
+ return error;
+ }
+ msleep(30);
+
+ error = i2c_smbus_write_byte(hx->client, HX852X_TS_SENSE_ON);
+ if (error) {
+ dev_err(dev, "failed to send TS_SENSE_ON: %d\n", error);
+ return error;
+ }
+ msleep(20);
+
+ return 0;
+}
+
+static int hx852x_stop(struct hx852x *hx)
+{
+ struct device *dev = &hx->client->dev;
+ int error;
+
+ error = i2c_smbus_write_byte(hx->client, HX852X_TS_SENSE_OFF);
+ if (error) {
+ dev_err(dev, "failed to send TS_SENSE_OFF: %d\n", error);
+ return error;
+ }
+ msleep(20);
+
+ error = i2c_smbus_write_byte(hx->client, HX852X_TS_SLEEP_IN);
+ if (error) {
+ dev_err(dev, "failed to send TS_SLEEP_IN: %d\n", error);
+ return error;
+ }
+ msleep(30);
+
+ return 0;
+}
+
+static int hx852x_power_off(struct hx852x *hx)
+{
+ struct device *dev = &hx->client->dev;
+ int error;
+
+ error = regulator_bulk_disable(ARRAY_SIZE(hx->supplies), hx->supplies);
+ if (error)
+ dev_err(dev, "failed to disable regulators: %d\n", error);
+ return error;
+}
+
+static int hx852x_read_config(struct hx852x *hx)
+{
+ struct device *dev = &hx->client->dev;
+ struct hx852x_config conf;
+ int x_res, y_res;
+ int error;
+
+ error = hx852x_power_on(hx);
+ if (error)
+ return error;
+
+ /* Sensing must be turned on briefly to load the config */
+ error = hx852x_start(hx);
+ if (error)
+ goto err_power_off;
+
+ error = hx852x_stop(hx);
+ if (error)
+ goto err_power_off;
+
+ error = i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH,
+ HX852X_SRAM_SWITCH_TEST_MODE);
+ if (error)
+ goto err_power_off;
+
+ error = i2c_smbus_write_word_data(hx->client, HX852X_REG_SRAM_ADDR,
+ HX852X_SRAM_ADDR_CONFIG);
+ if (error)
+ goto err_test_mode;
+
+ error = hx852x_i2c_read(hx, HX852X_REG_FLASH_RPLACE, &conf, sizeof(conf));
+ if (error)
+ goto err_test_mode;
+
+ x_res = be16_to_cpu(conf.x_res);
+ y_res = be16_to_cpu(conf.y_res);
+ hx->max_fingers = (conf.max_pt & 0xf0) >> 4;
+ dev_dbg(dev, "x res: %u, y res: %u, max fingers: %u\n",
+ x_res, y_res, hx->max_fingers);
+
+ if (hx->max_fingers > HX852X_MAX_FINGERS) {
+ dev_err(dev, "max supported fingers: %u, found: %u\n",
+ HX852X_MAX_FINGERS, hx->max_fingers);
+ error = -EINVAL;
+ goto err_test_mode;
+ }
+
+ if (x_res && y_res) {
+ input_set_abs_params(hx->input_dev, ABS_MT_POSITION_X, 0, x_res - 1, 0, 0);
+ input_set_abs_params(hx->input_dev, ABS_MT_POSITION_Y, 0, y_res - 1, 0, 0);
+ }
+
+ error = i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH, 0);
+ if (error)
+ goto err_power_off;
+
+ return hx852x_power_off(hx);
+
+err_test_mode:
+ i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH, 0);
+err_power_off:
+ hx852x_power_off(hx);
+ return error;
+}
+
+static int hx852x_handle_events(struct hx852x *hx)
+{
+ /*
+ * The event packets have variable size, depending on the amount of
+ * supported fingers (hx->max_fingers). They are laid out as follows:
+ * - struct hx852x_coord[hx->max_fingers]: Coordinates for each finger
+ * - u8[ALIGN(hx->max_fingers, 4)]: Touch width for each finger
+ * with padding for 32-bit alignment
+ * - struct hx852x_touch_info
+ *
+ * Load everything into a 32-bit aligned buffer so the coordinates
+ * can be assigned directly, without using get_unaligned_*().
+ */
+ u8 buf[HX852X_MAX_BUF_SIZE] __aligned(4);
+ struct hx852x_coord *coord = (struct hx852x_coord *)buf;
+ u8 *width = &buf[HX852X_COORD_SIZE(hx->max_fingers)];
+ struct hx852x_touch_info *info = (struct hx852x_touch_info *)
+ &width[HX852X_WIDTH_SIZE(hx->max_fingers)];
+ unsigned long finger_pressed, key_pressed;
+ unsigned int i, x, y, w;
+ int error;
+
+ error = hx852x_i2c_read(hx, HX852X_READ_ALL_EVENTS, buf,
+ HX852X_BUF_SIZE(hx->max_fingers));
+ if (error)
+ return error;
+
+ finger_pressed = get_unaligned_le16(&info->finger_pressed);
+ key_pressed = finger_pressed >> HX852X_MAX_FINGERS;
+
+ /* All bits are set when no touch is detected */
+ if (info->finger_num == 0xff || !(info->finger_num & 0x0f))
+ finger_pressed = 0;
+ if (key_pressed == 0xf)
+ key_pressed = 0;
+
+ for_each_set_bit(i, &finger_pressed, hx->max_fingers) {
+ x = be16_to_cpu(coord[i].x);
+ y = be16_to_cpu(coord[i].y);
+ w = width[i];
+
+ input_mt_slot(hx->input_dev, i);
+ input_mt_report_slot_state(hx->input_dev, MT_TOOL_FINGER, 1);
+ touchscreen_report_pos(hx->input_dev, &hx->props, x, y, true);
+ input_report_abs(hx->input_dev, ABS_MT_TOUCH_MAJOR, w);
+ }
+ input_mt_sync_frame(hx->input_dev);
+
+ for (i = 0; i < hx->keycount; i++)
+ input_report_key(hx->input_dev, hx->keycodes[i], key_pressed & BIT(i));
+
+ input_sync(hx->input_dev);
+ return 0;
+}
+
+static irqreturn_t hx852x_interrupt(int irq, void *ptr)
+{
+ struct hx852x *hx = ptr;
+ int error;
+
+ error = hx852x_handle_events(hx);
+ if (error) {
+ dev_err_ratelimited(&hx->client->dev, "failed to handle events: %d\n", error);
+ return IRQ_NONE;
+ }
+
+ return IRQ_HANDLED;
+}
+
+static int hx852x_input_open(struct input_dev *dev)
+{
+ struct hx852x *hx = input_get_drvdata(dev);
+ int error;
+
+ error = hx852x_power_on(hx);
+ if (error)
+ return error;
+
+ error = hx852x_start(hx);
+ if (error) {
+ hx852x_power_off(hx);
+ return error;
+ }
+
+ enable_irq(hx->client->irq);
+ return 0;
+}
+
+static void hx852x_input_close(struct input_dev *dev)
+{
+ struct hx852x *hx = input_get_drvdata(dev);
+
+ hx852x_stop(hx);
+ disable_irq(hx->client->irq);
+ hx852x_power_off(hx);
+}
+
+static int hx852x_parse_properties(struct hx852x *hx)
+{
+ struct device *dev = &hx->client->dev;
+ int error;
+
+ hx->keycount = device_property_count_u32(dev, "linux,keycodes");
+ if (hx->keycount <= 0) {
+ hx->keycount = 0;
+ return 0;
+ }
+
+ if (hx->keycount > HX852X_MAX_KEY_COUNT) {
+ dev_err(dev, "max supported keys: %u, found: %u\n",
+ HX852X_MAX_KEY_COUNT, hx->keycount);
+ return -EINVAL;
+ }
+
+ error = device_property_read_u32_array(dev, "linux,keycodes",
+ hx->keycodes, hx->keycount);
+ if (error)
+ dev_err(dev, "failed to read linux,keycodes: %d\n", error);
+
+ return error;
+}
+
+static int hx852x_probe(struct i2c_client *client)
+{
+ struct device *dev = &client->dev;
+ struct hx852x *hx;
+ int error, i;
+
+ if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C |
+ I2C_FUNC_SMBUS_WRITE_BYTE |
+ I2C_FUNC_SMBUS_WRITE_BYTE_DATA |
+ I2C_FUNC_SMBUS_WRITE_WORD_DATA)) {
+ dev_err(dev, "not all required i2c functionality supported\n");
+ return -ENXIO;
+ }
+
+ hx = devm_kzalloc(dev, sizeof(*hx), GFP_KERNEL);
+ if (!hx)
+ return -ENOMEM;
+
+ hx->client = client;
+ hx->input_dev = devm_input_allocate_device(dev);
+ if (!hx->input_dev)
+ return -ENOMEM;
+
+ hx->input_dev->name = "Himax HX852x";
+ hx->input_dev->id.bustype = BUS_I2C;
+ hx->input_dev->open = hx852x_input_open;
+ hx->input_dev->close = hx852x_input_close;
+
+ i2c_set_clientdata(client, hx);
+ input_set_drvdata(hx->input_dev, hx);
+
+ hx->supplies[0].supply = "vcca";
+ hx->supplies[1].supply = "vccd";
+ error = devm_regulator_bulk_get(dev, ARRAY_SIZE(hx->supplies), hx->supplies);
+ if (error)
+ return dev_err_probe(dev, error, "failed to get regulators\n");
+
+ hx->reset_gpiod = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
+ if (IS_ERR(hx->reset_gpiod))
+ return dev_err_probe(dev, PTR_ERR(hx->reset_gpiod),
+ "failed to get reset gpio\n");
+
+ error = devm_request_threaded_irq(dev, client->irq, NULL, hx852x_interrupt,
+ IRQF_ONESHOT | IRQF_NO_AUTOEN, NULL, hx);
+ if (error)
+ return dev_err_probe(dev, error, "failed to request irq %d", client->irq);
+
+ error = hx852x_read_config(hx);
+ if (error)
+ return error;
+
+ input_set_capability(hx->input_dev, EV_ABS, ABS_MT_POSITION_X);
+ input_set_capability(hx->input_dev, EV_ABS, ABS_MT_POSITION_Y);
+ input_set_abs_params(hx->input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
+
+ touchscreen_parse_properties(hx->input_dev, true, &hx->props);
+ error = hx852x_parse_properties(hx);
+ if (error)
+ return error;
+
+ hx->input_dev->keycode = hx->keycodes;
+ hx->input_dev->keycodemax = hx->keycount;
+ hx->input_dev->keycodesize = sizeof(hx->keycodes[0]);
+ for (i = 0; i < hx->keycount; i++)
+ input_set_capability(hx->input_dev, EV_KEY, hx->keycodes[i]);
+
+ error = input_mt_init_slots(hx->input_dev, hx->max_fingers,
+ INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
+ if (error)
+ return dev_err_probe(dev, error, "failed to init MT slots\n");
+
+ error = input_register_device(hx->input_dev);
+ if (error)
+ return dev_err_probe(dev, error, "failed to register input device\n");
+
+ return 0;
+}
+
+static int hx852x_suspend(struct device *dev)
+{
+ struct hx852x *hx = dev_get_drvdata(dev);
+
+ mutex_lock(&hx->input_dev->mutex);
+ if (input_device_enabled(hx->input_dev))
+ hx852x_stop(hx);
+ mutex_unlock(&hx->input_dev->mutex);
+
+ return 0;
+}
+
+static int hx852x_resume(struct device *dev)
+{
+ struct hx852x *hx = dev_get_drvdata(dev);
+ int error = 0;
+
+ mutex_lock(&hx->input_dev->mutex);
+ if (input_device_enabled(hx->input_dev))
+ error = hx852x_start(hx);
+ mutex_unlock(&hx->input_dev->mutex);
+
+ return error;
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(hx852x_pm_ops, hx852x_suspend, hx852x_resume);
+
+#ifdef CONFIG_OF
+static const struct of_device_id hx852x_of_match[] = {
+ { .compatible = "himax,hx852es" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, hx852x_of_match);
+#endif
+
+static struct i2c_driver hx852x_driver = {
+ .probe = hx852x_probe,
+ .driver = {
+ .name = "himax_hx852x",
+ .pm = pm_sleep_ptr(&hx852x_pm_ops),
+ .of_match_table = of_match_ptr(hx852x_of_match),
+ },
+};
+module_i2c_driver(hx852x_driver);
+
+MODULE_DESCRIPTION("Himax HX852x(ES) Touchscreen Driver");
+MODULE_AUTHOR("Jonathan Albrieux <jonathan.albrieux@gmail.com>");
+MODULE_AUTHOR("Stephan Gerhold <stephan@gerhold.net>");
+MODULE_LICENSE("GPL");
--
2.42.0
^ permalink raw reply related
* [PATCH v2 0/2] Input: add Himax HX852x(ES) touchscreen driver
From: Stephan Gerhold @ 2023-09-30 15:31 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Henrik Rydberg,
linux-input, devicetree, linux-kernel, Jeff LaBundy,
Christophe JAILLET, Jonathan Albrieux, Stephan Gerhold,
Krzysztof Kozlowski
Add DT schema and driver for the Himax HX852x(ES) touch panel
controller, with support for multi-touch and capacitive touch keys.
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
Changes in v2:
- dt-bindings: Swap required:/additionalProperties: (Krzysztof)
- Use dev_err_ratelimited() for error in IRQ thread (Christophe)
- Use dev_err_probe() consistently (Christophe)
- Improve error handling of hx852x_power_off()/hx852x_stop() (Jeff)
- Add linux/of.h and linux/mod_devicetable.h include (Jeff)
- Fix %d -> %u in some format strings (Jeff)
- Fix other small comments from Jeff
- Link to v1: https://lore.kernel.org/r/20230913-hx852x-v1-0-9c1ebff536eb@gerhold.net
---
Jonathan Albrieux (1):
Input: add Himax HX852x(ES) touchscreen driver
Stephan Gerhold (1):
dt-bindings: input: touchscreen: document Himax HX852x(ES)
.../bindings/input/touchscreen/himax,hx852es.yaml | 81 ++++
MAINTAINERS | 7 +
drivers/input/touchscreen/Kconfig | 10 +
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/himax_hx852x.c | 499 +++++++++++++++++++++
5 files changed, 598 insertions(+)
---
base-commit: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d
change-id: 20230816-hx852x-3490d2773322
Best regards,
--
Stephan Gerhold <stephan@gerhold.net>
^ permalink raw reply
* [PATCH v2 1/2] dt-bindings: input: touchscreen: document Himax HX852x(ES)
From: Stephan Gerhold @ 2023-09-30 15:32 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Henrik Rydberg,
linux-input, devicetree, linux-kernel, Jeff LaBundy,
Christophe JAILLET, Jonathan Albrieux, Stephan Gerhold,
Krzysztof Kozlowski
In-Reply-To: <20230930-hx852x-v2-0-c5821947b225@gerhold.net>
Himax HX852x(ES) is a touch panel controller with optional support
for capacitive touch keys.
Unfortunately, the model naming is quite unclear and confusing. There
seems to be a distinction between models (e.g. HX8526) and the "series"
suffix (e.g. -A, -B, -C, -D, -E, -ES). But this doesn't seem to be
applied very consistently because e.g. HX8527-E(44) actually seems to
belong to the -ES series.
The compatible consists of the actual part number followed by the
"series" as fallback compatible. Typically only the latter will be
interesting for drivers as there is no relevant difference on the
driver side.
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
.../bindings/input/touchscreen/himax,hx852es.yaml | 81 ++++++++++++++++++++++
1 file changed, 81 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/himax,hx852es.yaml b/Documentation/devicetree/bindings/input/touchscreen/himax,hx852es.yaml
new file mode 100644
index 000000000000..40a60880111d
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/himax,hx852es.yaml
@@ -0,0 +1,81 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/touchscreen/himax,hx852es.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Himax HX852x(ES) touch panel controller
+
+maintainers:
+ - Stephan Gerhold <stephan@gerhold.net>
+
+allOf:
+ - $ref: touchscreen.yaml#
+
+properties:
+ compatible:
+ items:
+ - enum:
+ - himax,hx8525e
+ - himax,hx8526e
+ - himax,hx8527e
+ - const: himax,hx852es
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+ description: Touch Screen Interrupt (TSIX), active low
+
+ reset-gpios:
+ maxItems: 1
+ description: External Reset (XRES), active low
+
+ vcca-supply:
+ description: Analog power supply (VCCA)
+
+ vccd-supply:
+ description: Digital power supply (VCCD)
+
+ touchscreen-inverted-x: true
+ touchscreen-inverted-y: true
+ touchscreen-size-x: true
+ touchscreen-size-y: true
+ touchscreen-swapped-x-y: true
+
+ linux,keycodes:
+ minItems: 1
+ maxItems: 4
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - reset-gpios
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+ #include <dt-bindings/input/input.h>
+ #include <dt-bindings/interrupt-controller/irq.h>
+
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ touchscreen@48 {
+ compatible = "himax,hx8527e", "himax,hx852es";
+ reg = <0x48>;
+ interrupt-parent = <&tlmm>;
+ interrupts = <13 IRQ_TYPE_LEVEL_LOW>;
+ reset-gpios = <&tlmm 12 GPIO_ACTIVE_LOW>;
+ vcca-supply = <®_ts_vcca>;
+ vccd-supply = <&pm8916_l6>;
+ linux,keycodes = <KEY_BACK KEY_HOMEPAGE KEY_APPSELECT>;
+ };
+ };
+
+...
--
2.42.0
^ permalink raw reply related
* Re: [PATCH 00/52] input: Convert to platform remove callback returning void
From: Dmitry Torokhov @ 2023-09-30 15:37 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Lee Jones, Linus Walleij, Alexandre Torgue, Pavel Machek,
Guenter Roeck, Liang He, linux-stm32, Daniel Lezcano,
chrome-platform, Arnd Bergmann, Samuel Holland, Andrey Moiseev,
Michal Simek, Ruan Jinjie, Yangtao Li, Jernej Skrabec,
joewu (吳仲振), Miloslav Trmac, Robert Jarzmik,
Chen-Yu Tsai, Andy Gross, linux-input, Jeff LaBundy, linux-sunxi,
linux-arm-msm, Maxime Coquelin, Michael Hennerich, Rob Herring,
ye xingchen, Kalle Valo, Steven Rostedt (Google), Hans de Goede,
Siarhei Volkau, Christophe JAILLET, Jonathan Cameron,
Andy Shevchenko, Benson Leung, linux-arm-kernel, Paolo Abeni,
Support Opensource, Chen Jun, Greg Kroah-Hartman,
Mattijs Korpershoek, Rafael J. Wysocki, Konrad Dybcio,
Krzysztof Kozlowski, kernel, patches, Dmitry Baryshkov,
Jonathan Corbet, Bjorn Andersson
In-Reply-To: <20230924155057.e4k4ruv5iggbt6q6@pengutronix.de>
Hi Uwe,
Sorry for the spotty responses.
On Sun, Sep 24, 2023 at 05:50:57PM +0200, Uwe Kleine-König wrote:
> Hello Dmitry,
>
> On Sat, Sep 23, 2023 at 07:48:21PM -0700, Dmitry Torokhov wrote:
> > On Wed, Sep 20, 2023 at 02:57:37PM +0200, Uwe Kleine-König wrote:
> > > Hello,
> > >
> > > this series converts all platform drivers below drivers/input to use
> > > remove_new. The motivation is to get rid of an integer return code
> > > that is (mostly) ignored by the platform driver core and error prone on
> > > the driver side.
> > >
> > > See commit 5c5a7680e67b ("platform: Provide a remove callback that
> > > returns no value") for an extended explanation and the eventual goal.
> > >
> > > There are no interdependencies between the patches. As there are still
> > > quite a few drivers to convert, I'm happy about every patch that makes
> > > it in. So even if there is a merge conflict with one patch until you
> > > apply or a subject prefix is suboptimal, please apply the remainder of
> > > this series anyhow.
> >
> > Applied the lot (fixing the i8042-sparcio patch subject), thank you!
>
> Thanks. In the meantime I found out why my process failed here: I only
> fixed *.c, but this driver struct is defined in a header file
> i8042-sparcio.h.
>
> This file is only included by drivers/input/serio/i8042.h which in turn
> is only included by drivers/input/serio/i8042.c. So there is only one
> instance created, but I'd call that unusual anyhow.
Right, i8042 is essentially a singleton, and what you see here is an
attempt to bolt OF onto a legacy driver that is largely predates the
current driver model. I wanted to clean it up, but it is still widely
used and I am hesitant to disturb it too much.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: synaptics-rmi4 - replace deprecated strncpy
From: Dmitry Torokhov @ 2023-09-30 16:06 UTC (permalink / raw)
To: Justin Stitt; +Cc: linux-input, linux-kernel, linux-hardening
In-Reply-To: <20230921-strncpy-drivers-input-rmi4-rmi_f34-c-v1-1-4aef2e84b8d2@google.com>
On Thu, Sep 21, 2023 at 09:58:11AM +0000, Justin Stitt wrote:
> `strncpy` is deprecated for use on NUL-terminated destination strings [1]
>
> Let's use memcpy() as the bounds have already been checked and this
> decays into a simple byte copy from one buffer to another removing any
> ambiguity that strncpy has.
>
> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-hardening@vger.kernel.org
> Signed-off-by: Justin Stitt <justinstitt@google.com>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 1/2] input: generalize the Imagis touchscreen driver
From: Markuss Broks @ 2023-09-30 16:10 UTC (permalink / raw)
To: Karel Balej, Karel Balej
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
linux-input, devicetree, linux-kernel, Duje Mihanović,
~postmarketos/upstreaming, Jeff LaBundy, linmengbo0689
In-Reply-To: <CVVJR34G5A55.2LYQW8Z5PEEDA@gimli.ms.mff.cuni.cz>
Hi Karel,
On 9/29/23 19:37, Karel Balej wrote:
> Hello, Markuss,
>
>> If you don't mind the extra hassle, I'm all in for my generalization
>> thing going together with your series.
>>
>> Alternatively, I can resend it myself, but I believe it would be better
>> if they go in bulk since they need to be applied together.
> great. Do you wish to make any changes to your original series? If not,
> please let me know and I will use the v2 [1] as it is.
I believe that version should be fine.
>
>>>> As for the voltage set, I believe this does not belong in a kernel
>>>> driver. You should set it in device-tree with `regulator-max-microvolt`
>>>> and `regulator-min-microvolt`.
>>> Please see my response to Jeff regarding this. I will be happy to hear
>>> your thoughts on what I propose.
>> Actually, the regulator values belong to the device-tree, because the
>> device-tree for the board is what describes the board's regulators, and
>> since you know what components are installed on that specific board you
>> can know which regulator values are supposed to be set for it.
>>
>> [...]
>>
>> The actual min/max values for regulators or its voltage table is
>> provided by the regulator driver itself, so there's not much point in
>> specifying those again in the device-tree.
> I see. I think the reason why I thought what I wrote before is that
> downstream the regulator DTS lives separately from the board as a .dtsi
> file which made me think that it can be used universally. So if I
> understand correctly now, the hardware specifications of the regulator,
> such as the minimal and maximal voltage should be part of the driver,
> while the DT should contain requirements for the given use of the
> regulator (with a specific board) - is this correct?
That is correct.
>
>> This manual voltage setting can cause conflicts with other drivers for
>> example. Also some device can use a variable wide voltage range, and
>> some only specific narrow one, and e.g. the driver with wide range
>> would set it to voltage that isn't suitable for the narrow range
>> device, so it's much better to just specify it manually than have it
>> resolved.
> I would expect that in the case you describe, the kernel would set the
> voltage to a value which would satisfy both the ranges. I don't know
> what would happen if that was not possible (i. e. there was no
> intersection in the two requested voltage ranges), though. Or does a
> call to regulator_set_voltage set the voltage immediately taking notice
> only of the hardware contraints? I think I am having trouble
> understanding what this quote from the regulator_set_voltage
> documentation means:
>
>> If the regulator is shared between several devices then the lowest
>> request voltage that meets the system constraints will be used.
> But actually, it probably doesn't make sense that the kernel would try
> to resolve a range suitable for all calls to this function as, I assume,
> a single driver could call it multiple times with disjoint voltage
> intervals.
Well, I assume kernel has some sort of behaviour for that stuff, but I
believe setting the voltage by the device manually is discouraged in
general. It should be possible, though.
>
> Thank you for your patience.
>
> Kind regards,
> K. B.
- Markuss
^ permalink raw reply
* Re: [RESEND PATCH v6 3/3] input: pm8xxx-vibrator: add new SPMI vibrator support
From: Dmitry Torokhov @ 2023-09-30 16:17 UTC (permalink / raw)
To: Fenglin Wu
Cc: Dmitry Baryshkov, linux-arm-msm, linux-kernel,
krzysztof.kozlowski+dt, robh+dt, agross, andersson, Konrad Dybcio,
linux-input, quic_collinsd, quic_subbaram, quic_kamalw, jestar,
Luca Weiss
In-Reply-To: <12887370-0ada-359b-8a4f-18a28495c69a@quicinc.com>
On Mon, Sep 25, 2023 at 10:54:45AM +0800, Fenglin Wu wrote:
>
>
> On 9/24/2023 3:07 AM, Dmitry Baryshkov wrote:
> > > +
> > > + switch (vib->data->hw_type) {
> > > + case SSBI_VIB:
> > > mask = SSBI_VIB_DRV_LEVEL_MASK;
> > > shift = SSBI_VIB_DRV_SHIFT;
> > > + break;
> > > + case SPMI_VIB:
> > > + mask = SPMI_VIB_DRV_LEVEL_MASK;
> > > + shift = SPMI_VIB_DRV_SHIFT;
> > > + break;
> > > + case SPMI_VIB_GEN2:
> > > + mask = SPMI_VIB_GEN2_DRV_MASK;
> > > + shift = SPMI_VIB_GEN2_DRV_SHIFT;
> > > + break;
> > > + default:
> > > + return -EINVAL;
> > Could you please move the switch to the previous patch? Then it would
> > be more obvious that you are just adding the SPMI_VIB_GEN2 here.
> >
> > Other than that LGTM.
>
> Sure, I can move the switch to the previous refactoring patch.
Actually, the idea of having a const "reg" or "chip", etc. structure is
to avoid this kind of runtime checks based on hardware type and instead
use common computation. I believe you need to move mask and shift into
the chip-specific structure and avoid defining hw_type.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: xpad - add PXN V900 support
From: Dmitry Torokhov @ 2023-09-30 16:29 UTC (permalink / raw)
To: Matthias Berndt
Cc: Vicki Pfau, Ismael Ferreras Morezuelas, Lyude Paul,
Matthias Benkmann, John Butler, Pierre-Loup A. Griffais,
Jonathan Frederick, linux-input, linux-kernel
In-Reply-To: <2305012.ElGaqSPkdT@fedora>
Hi Matthias,
On Fri, Sep 29, 2023 at 07:45:52PM +0200, Matthias Berndt wrote:
> Hi everybody,
>
> I recently sent this patch to the linux-input list where it was ignored, so
> now I'm sending it again to every email address that get_maintainer.pl gives
> me in the hope that it'll somehow get merged.
> This is a trivial patch that enables support for the PXN V900 steering wheel
> in the xpad driver. It's just a matter of adding the relevant USB vendorId/
> productId to the list of supported IDs. I've tried it and
I need your "Signed-off-by: ..." in order to merge this patch. Please
resend following guidelines in Documentation/process/submitting-patches.rst
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2] Input: axp20x-pek - avoid needless newline removal
From: Dmitry Torokhov @ 2023-09-30 16:34 UTC (permalink / raw)
To: Justin Stitt
Cc: Chen-Yu Tsai, linux-input, linux-kernel, linux-hardening,
Kees Cook
In-Reply-To: <20230925-strncpy-drivers-input-misc-axp20x-pek-c-v2-1-ff7abe8498d6@google.com>
On Mon, Sep 25, 2023 at 04:31:05AM +0000, Justin Stitt wrote:
> This code is doing more work than it needs to.
>
> Before handing off `val_str` to `kstrtouint()` we are eagerly removing
> any trailing newline which requires copying `buf`, validating it's
> length and checking/replacing any potential newlines.
>
> kstrtouint() handles this implicitly:
> kstrtouint ->
> kstrotoull -> (documentation)
> | /**
> | * kstrtoull - convert a string to an unsigned long long
> | * @s: The start of the string. The string must be null-terminated, and may also
> | * include a single newline before its terminating null. The first character
> | ...
>
> Let's remove the redundant functionality and let kstrtouint handle it.
>
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-hardening@vger.kernel.org
> Suggested-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Justin Stitt <justinstitt@google.com>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] input: Annotate struct evdev_client with __counted_by
From: Dmitry Torokhov @ 2023-09-30 16:36 UTC (permalink / raw)
To: Kees Cook
Cc: linux-input, Nathan Chancellor, Nick Desaulniers, Tom Rix,
linux-kernel, llvm, linux-hardening
In-Reply-To: <20230922175027.work.563-kees@kernel.org>
On Fri, Sep 22, 2023 at 10:50:28AM -0700, Kees Cook wrote:
> Prepare for the coming implementation by GCC and Clang of the __counted_by
> attribute. Flexible array members annotated with __counted_by can have
> their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
> (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
> functions).
>
> As found with Coccinelle[1], add __counted_by for struct evdev_client.
>
> [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: linux-input@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: Annotate struct input_leds with __counted_by
From: Dmitry Torokhov @ 2023-09-30 16:37 UTC (permalink / raw)
To: Kees Cook
Cc: linux-input, Nathan Chancellor, Nick Desaulniers, Tom Rix,
linux-kernel, llvm, linux-hardening
In-Reply-To: <20230922175031.work.467-kees@kernel.org>
On Fri, Sep 22, 2023 at 10:50:32AM -0700, Kees Cook wrote:
> Prepare for the coming implementation by GCC and Clang of the __counted_by
> attribute. Flexible array members annotated with __counted_by can have
> their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
> (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
> functions).
>
> As found with Coccinelle[1], add __counted_by for struct input_leds.
>
> [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: linux-input@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] input: mt: Annotate struct input_mt with __counted_by
From: Dmitry Torokhov @ 2023-09-30 16:38 UTC (permalink / raw)
To: Kees Cook
Cc: linux-input, Nathan Chancellor, Nick Desaulniers, Tom Rix,
linux-kernel, llvm, linux-hardening
In-Reply-To: <20230922175036.work.762-kees@kernel.org>
On Fri, Sep 22, 2023 at 10:50:37AM -0700, Kees Cook wrote:
> Prepare for the coming implementation by GCC and Clang of the __counted_by
> attribute. Flexible array members annotated with __counted_by can have
> their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
> (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
> functions).
>
> As found with Coccinelle[1], add __counted_by for struct input_mt.
>
> [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: linux-input@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* [PATCH 0/7] Input: synaptics-rmi4: add quirks for third party touchscreen controllers
From: Caleb Connolly @ 2023-09-30 17:08 UTC (permalink / raw)
To: Dmitry Torokhov, Vincent Huang
Cc: methanal, linux-input, devicetree, phone-devel,
~postmarketos/upstreaming, Caleb Connolly, Jason A. Donenfeld,
Matthias Schiffer, Rob Herring, Krzysztof Kozlowski, Conor Dooley
With the growing popularity of running upstream Linux on mobile devices,
we're beginning to run into more and more edgecases. The OnePlus 6 is a
fairly well supported 2018 era smartphone, selling over a million units
in it's first 22 days. With this level of popularity, it's almost
inevitable that we get third party replacement displays, and as a
result, replacement touchscreen controllers.
The OnePlus 6 shipped with an extremely usecase specific touchscreen
driver, it implemented only the bare minimum parts of the highly generic
rmi4 protocol, instead hardcoding most of the register addresses.
As a result, the third party touchscreen controllers that are often
found in replacement screens, implement only the registers that the
downstream driver reads from. They additionally have other restrictions
such as heavy penalties on unaligned reads.
This series attempts to implement the necessary workaround to support
some of these chips with the rmi4 driver. Although it's worth noting
that at the time of writing there are other unofficial controllers in
the wild that don't work even with these patches.
We have been shipping these patches in postmarketOS for the last several
months, and they are known to not cause any regressions on the OnePlus
6/6T (with the official Synaptics controller), however I don't own any
other rmi4 hardware to further validate this.
These patches were contributed by a community developer who has given
permission for me to submit them on their behalf.
---
To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
To: Vincent Huang <vincent.huang@tw.synaptics.com>
Cc: methanal <baclofen@tuta.io>
Cc: linux-input@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: phone-devel@vger.kernel.org
Cc: ~postmarketos/upstreaming@lists.sr.ht
---
Caleb Connolly (2):
dt-bindings: input: syna,rmi4: document syna,pdt-fallback-desc
Input: synaptics-rmi4 - handle duplicate/unknown PDT entries
methanal (5):
Input: synaptics-rmi4 - f12: use hardcoded values for aftermarket touch ICs
Input: synaptics-rmi4 - f55: handle zero electrode count
Input: synaptics-rmi4 - don't do unaligned reads in IRQ context
Input: synaptics-rmi4 - read product ID on aftermarket touch ICs
Input: synaptics-rmi4 - support fallback values for PDT descriptor bytes
.../devicetree/bindings/input/syna,rmi4.yaml | 10 ++
drivers/input/rmi4/rmi_driver.c | 134 ++++++++++++++++++---
drivers/input/rmi4/rmi_driver.h | 8 ++
drivers/input/rmi4/rmi_f01.c | 14 +++
drivers/input/rmi4/rmi_f12.c | 117 ++++++++++++++----
drivers/input/rmi4/rmi_f55.c | 5 +
include/linux/rmi.h | 3 +
7 files changed, 247 insertions(+), 44 deletions(-)
---
base-commit: b0d95ff7653ef6ace66a24d6c09147d0731825ce
// Caleb (they/them)
^ permalink raw reply
* [PATCH 1/7] dt-bindings: input: syna,rmi4: document syna,pdt-fallback-desc
From: Caleb Connolly @ 2023-09-30 17:08 UTC (permalink / raw)
To: Dmitry Torokhov, Vincent Huang
Cc: methanal, linux-input, devicetree, phone-devel,
~postmarketos/upstreaming, Caleb Connolly, Jason A. Donenfeld,
Matthias Schiffer, Rob Herring, Krzysztof Kozlowski, Conor Dooley
In-Reply-To: <20230929-caleb-rmi4-quirks-v1-0-cc3c703f022d@linaro.org>
This new property allows devices to specify some register values which
are missing on units with third party replacement displays. These
displays use unofficial touch ICs which only implement a subset of the
RMI4 specification.
Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
---
To: "Jason A. Donenfeld" <Jason@zx2c4.com>
To: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
To: Rob Herring <robh+dt@kernel.org>
To: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
To: Conor Dooley <conor+dt@kernel.org>
Cc: devicetree@vger.kernel.org
---
Documentation/devicetree/bindings/input/syna,rmi4.yaml | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/syna,rmi4.yaml b/Documentation/devicetree/bindings/input/syna,rmi4.yaml
index 4d4e1a8e36be..bd6beb67ac21 100644
--- a/Documentation/devicetree/bindings/input/syna,rmi4.yaml
+++ b/Documentation/devicetree/bindings/input/syna,rmi4.yaml
@@ -49,6 +49,16 @@ properties:
description:
Delay to wait after powering on the device.
+ syna,pdt-fallback-desc:
+ $ref: /schemas/types.yaml#/definitions/uint8-array
+ description:
+ An array of pairs of function number and version. These are used
+ on some devices with replacement displays that use unofficial touch
+ controllers. These controllers do report the properties of their PDT
+ entries, but leave the function_number and function_version registers
+ blank. These values should match exactly the 5th and 4th bytes of each
+ PDT entry from the original display's touch controller.
+
vdd-supply: true
vio-supply: true
--
2.42.0
^ permalink raw reply related
* [PATCH 2/7] Input: synaptics-rmi4 - handle duplicate/unknown PDT entries
From: Caleb Connolly @ 2023-09-30 17:08 UTC (permalink / raw)
To: Dmitry Torokhov, Vincent Huang
Cc: methanal, linux-input, devicetree, phone-devel,
~postmarketos/upstreaming, Caleb Connolly
In-Reply-To: <20230929-caleb-rmi4-quirks-v1-0-cc3c703f022d@linaro.org>
Some third party rmi4-compatible ICs don't expose their PDT entries
very well. Add a few checks to skip duplicate entries as well as entries
for unsupported functions.
This is required to support some phones with third party displays.
Validated on a stock OnePlus 6T (original parts):
manufacturer: Synaptics, product: S3706B, fw id: 2852315
Co-developed-by: methanal <baclofen@tuta.io>
Signed-off-by: methanal <baclofen@tuta.io>
Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
---
drivers/input/rmi4/rmi_driver.c | 47 +++++++++++++++++++++++++++++++++++------
drivers/input/rmi4/rmi_driver.h | 6 ++++++
2 files changed, 47 insertions(+), 6 deletions(-)
diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index 258d5fe3d395..cd3e4e77ab9b 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c
@@ -493,12 +493,44 @@ static void rmi_driver_copy_pdt_to_fd(const struct pdt_entry *pdt,
fd->function_version = pdt->function_version;
}
+static bool rmi_pdt_entry_is_valid(struct rmi_device *rmi_dev,
+ struct pdt_scan_state *state, u8 fn)
+{
+ int i;
+
+ switch (fn) {
+ case 0x01:
+ case 0x03:
+ case 0x11:
+ case 0x12:
+ case 0x30:
+ case 0x34:
+ case 0x3a:
+ case 0x54:
+ case 0x55:
+ break;
+
+ default:
+ rmi_dbg(RMI_DEBUG_CORE, &rmi_dev->dev,
+ "PDT has unknown function number %#02x\n", fn);
+ return false;
+ }
+
+ for (i = 0; i < state->pdt_count; i++) {
+ if (state->pdts[i] == fn)
+ return false;
+ }
+
+ state->pdts[state->pdt_count++] = fn;
+ return true;
+}
+
#define RMI_SCAN_CONTINUE 0
#define RMI_SCAN_DONE 1
static int rmi_scan_pdt_page(struct rmi_device *rmi_dev,
int page,
- int *empty_pages,
+ struct pdt_scan_state *state,
void *ctx,
int (*callback)(struct rmi_device *rmi_dev,
void *ctx,
@@ -521,6 +553,9 @@ static int rmi_scan_pdt_page(struct rmi_device *rmi_dev,
if (RMI4_END_OF_PDT(pdt_entry.function_number))
break;
+ if (!rmi_pdt_entry_is_valid(rmi_dev, state, pdt_entry.function_number))
+ continue;
+
retval = callback(rmi_dev, ctx, &pdt_entry);
if (retval != RMI_SCAN_CONTINUE)
return retval;
@@ -531,11 +566,11 @@ static int rmi_scan_pdt_page(struct rmi_device *rmi_dev,
* or more is found, stop scanning.
*/
if (addr == pdt_start)
- ++*empty_pages;
+ ++state->empty_pages;
else
- *empty_pages = 0;
+ state->empty_pages = 0;
- return (data->bootloader_mode || *empty_pages >= 2) ?
+ return (data->bootloader_mode || state->empty_pages >= 2) ?
RMI_SCAN_DONE : RMI_SCAN_CONTINUE;
}
@@ -544,11 +579,11 @@ int rmi_scan_pdt(struct rmi_device *rmi_dev, void *ctx,
void *ctx, const struct pdt_entry *entry))
{
int page;
- int empty_pages = 0;
+ struct pdt_scan_state state = {0, 0, {0}};
int retval = RMI_SCAN_DONE;
for (page = 0; page <= RMI4_MAX_PAGE; page++) {
- retval = rmi_scan_pdt_page(rmi_dev, page, &empty_pages,
+ retval = rmi_scan_pdt_page(rmi_dev, page, &state,
ctx, callback);
if (retval != RMI_SCAN_CONTINUE)
break;
diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h
index 1c6c6086c0e5..e1a5412f2f8f 100644
--- a/drivers/input/rmi4/rmi_driver.h
+++ b/drivers/input/rmi4/rmi_driver.h
@@ -46,6 +46,12 @@ struct pdt_entry {
u8 function_number;
};
+struct pdt_scan_state {
+ u8 empty_pages;
+ u8 pdt_count;
+ u8 pdts[254];
+};
+
#define RMI_REG_DESC_PRESENSE_BITS (32 * BITS_PER_BYTE)
#define RMI_REG_DESC_SUBPACKET_BITS (37 * BITS_PER_BYTE)
--
2.42.0
^ permalink raw reply related
* [PATCH 3/7] Input: synaptics-rmi4 - f12: use hardcoded values for aftermarket touch ICs
From: Caleb Connolly @ 2023-09-30 17:08 UTC (permalink / raw)
To: Dmitry Torokhov, Vincent Huang
Cc: methanal, linux-input, devicetree, phone-devel,
~postmarketos/upstreaming, Caleb Connolly
In-Reply-To: <20230929-caleb-rmi4-quirks-v1-0-cc3c703f022d@linaro.org>
From: methanal <baclofen@tuta.io>
Some replacement displays include third-party touch ICs which are
devoid of register descriptors. Create a fake data register descriptor
for such ICs and provide hardcoded default values.
It isn't possible to reliably determine if the touch IC is original or
not, so these fallback values are offered as an alternative to the error
path when register descriptors aren't available.
Signed-off-by: methanal <baclofen@tuta.io>
[changes for readability / codeflow, checkpatch fixes]
Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
---
drivers/input/rmi4/rmi_f12.c | 117 +++++++++++++++++++++++++++++++++----------
1 file changed, 91 insertions(+), 26 deletions(-)
diff --git a/drivers/input/rmi4/rmi_f12.c b/drivers/input/rmi4/rmi_f12.c
index 7e97944f7616..719ee3b42550 100644
--- a/drivers/input/rmi4/rmi_f12.c
+++ b/drivers/input/rmi4/rmi_f12.c
@@ -195,6 +195,41 @@ static void rmi_f12_process_objects(struct f12_data *f12, u8 *data1, int size)
rmi_2d_sensor_abs_report(sensor, &sensor->objs[i], i);
}
+static void rmi_f12_set_hardcoded_desc(struct rmi_function *fn, struct f12_data *f12)
+{
+ struct rmi_2d_sensor *sensor = &f12->sensor;
+ struct rmi_register_desc_item *reg_desc;
+
+ /* We have no f12->data_reg_desc, so the pkt_size is 0, override it with
+ * a somewhat sensible default (this corresponds to 10 fingers).
+ */
+ sensor->pkt_size = 88;
+
+ /*
+ * There are no register descriptors to get these values from.
+ * We set them to high values to either be overwritten by the clip
+ * properties from devicetree, or to just not get in the way.
+ */
+ sensor->max_x = 65535;
+ sensor->max_y = 65535;
+
+ /*
+ * Create the Data1 register descriptor so that touch events
+ * can work properly.
+ */
+ reg_desc = devm_kcalloc(&fn->dev, 1,
+ sizeof(struct rmi_register_desc_item), GFP_KERNEL);
+ reg_desc->reg = 1;
+ reg_desc->reg_size = 80;
+ reg_desc->num_subpackets = 10;
+
+ f12->data1 = reg_desc;
+ f12->data1_offset = 0;
+ sensor->nbr_fingers = reg_desc->num_subpackets;
+ sensor->report_abs = 1;
+ sensor->attn_size += reg_desc->reg_size;
+}
+
static irqreturn_t rmi_f12_attention(int irq, void *ctx)
{
int retval;
@@ -315,6 +350,40 @@ static int rmi_f12_config(struct rmi_function *fn)
return 0;
}
+static int rmi_f12_sensor_init(struct rmi_function *fn, struct f12_data *f12)
+{
+ struct rmi_2d_sensor *sensor = &f12->sensor;
+
+ sensor->fn = fn;
+ f12->data_addr = fn->fd.data_base_addr;
+
+ /* On quirky devices that don't have a data_reg_desc we hardcode the packet
+ * in rmi_f12_set_hardcoded_desc(). Make sure not to set it to 0 here.
+ */
+ if (!sensor->pkt_size)
+ sensor->pkt_size = rmi_register_desc_calc_size(&f12->data_reg_desc);
+
+ sensor->axis_align =
+ f12->sensor_pdata.axis_align;
+
+ sensor->x_mm = f12->sensor_pdata.x_mm;
+ sensor->y_mm = f12->sensor_pdata.y_mm;
+ sensor->dribble = f12->sensor_pdata.dribble;
+
+ if (sensor->sensor_type == rmi_sensor_default)
+ sensor->sensor_type =
+ f12->sensor_pdata.sensor_type;
+
+ rmi_dbg(RMI_DEBUG_FN, &fn->dev, "%s: data packet size: %d\n", __func__,
+ sensor->pkt_size);
+
+ sensor->data_pkt = devm_kzalloc(&fn->dev, sensor->pkt_size, GFP_KERNEL);
+ if (!sensor->data_pkt)
+ return -ENOMEM;
+
+ return 0;
+}
+
static int rmi_f12_probe(struct rmi_function *fn)
{
struct f12_data *f12;
@@ -328,6 +397,7 @@ static int rmi_f12_probe(struct rmi_function *fn)
struct rmi_driver_data *drvdata = dev_get_drvdata(&rmi_dev->dev);
u16 data_offset = 0;
int mask_size;
+ bool hardcoded_desc_quirk = false;
rmi_dbg(RMI_DEBUG_FN, &fn->dev, "%s\n", __func__);
@@ -342,9 +412,9 @@ static int rmi_f12_probe(struct rmi_function *fn)
++query_addr;
if (!(buf & BIT(0))) {
- dev_err(&fn->dev,
- "Behavior of F12 without register descriptors is undefined.\n");
- return -ENODEV;
+ rmi_dbg(RMI_DEBUG_FN, &fn->dev,
+ "No register descriptors defined for F12, using fallback\n");
+ hardcoded_desc_quirk = true;
}
f12 = devm_kzalloc(&fn->dev, sizeof(struct f12_data) + mask_size * 2,
@@ -352,6 +422,8 @@ static int rmi_f12_probe(struct rmi_function *fn)
if (!f12)
return -ENOMEM;
+ dev_set_drvdata(&fn->dev, f12);
+
f12->abs_mask = (unsigned long *)((char *)f12
+ sizeof(struct f12_data));
f12->rel_mask = (unsigned long *)((char *)f12
@@ -370,6 +442,18 @@ static int rmi_f12_probe(struct rmi_function *fn)
f12->sensor_pdata = pdata->sensor_pdata;
}
+ sensor = &f12->sensor;
+
+ if (hardcoded_desc_quirk) {
+ rmi_f12_set_hardcoded_desc(fn, f12);
+
+ ret = rmi_f12_sensor_init(fn, f12);
+ if (ret)
+ return ret;
+
+ goto skip_register_desc;
+ }
+
ret = rmi_read_register_desc(rmi_dev, query_addr,
&f12->query_reg_desc);
if (ret) {
@@ -400,29 +484,9 @@ static int rmi_f12_probe(struct rmi_function *fn)
}
query_addr += 3;
- sensor = &f12->sensor;
- sensor->fn = fn;
- f12->data_addr = fn->fd.data_base_addr;
- sensor->pkt_size = rmi_register_desc_calc_size(&f12->data_reg_desc);
-
- sensor->axis_align =
- f12->sensor_pdata.axis_align;
-
- sensor->x_mm = f12->sensor_pdata.x_mm;
- sensor->y_mm = f12->sensor_pdata.y_mm;
- sensor->dribble = f12->sensor_pdata.dribble;
-
- if (sensor->sensor_type == rmi_sensor_default)
- sensor->sensor_type =
- f12->sensor_pdata.sensor_type;
-
- rmi_dbg(RMI_DEBUG_FN, &fn->dev, "%s: data packet size: %d\n", __func__,
- sensor->pkt_size);
- sensor->data_pkt = devm_kzalloc(&fn->dev, sensor->pkt_size, GFP_KERNEL);
- if (!sensor->data_pkt)
- return -ENOMEM;
-
- dev_set_drvdata(&fn->dev, f12);
+ ret = rmi_f12_sensor_init(fn, f12);
+ if (ret)
+ return ret;
ret = rmi_f12_read_sensor_tuning(f12);
if (ret)
@@ -520,6 +584,7 @@ static int rmi_f12_probe(struct rmi_function *fn)
data_offset += item->reg_size;
}
+skip_register_desc:
/* allocate the in-kernel tracking buffers */
sensor->tracking_pos = devm_kcalloc(&fn->dev,
sensor->nbr_fingers, sizeof(struct input_mt_pos),
--
2.42.0
^ permalink raw reply related
* [PATCH 5/7] Input: synaptics-rmi4 - don't do unaligned reads in IRQ context
From: Caleb Connolly @ 2023-09-30 17:08 UTC (permalink / raw)
To: Dmitry Torokhov, Vincent Huang
Cc: methanal, linux-input, devicetree, phone-devel,
~postmarketos/upstreaming, Caleb Connolly
In-Reply-To: <20230929-caleb-rmi4-quirks-v1-0-cc3c703f022d@linaro.org>
From: methanal <baclofen@tuta.io>
Some replacement displays include third-party touch ICs which incur a
significant penalty (1-2 seconds) when doing certain unaligned reads.
This is enough to break functionality when it happens in the hot path,
so adjust the interrupt handler to not read from an unaligned address.
Signed-off-by: methanal <baclofen@tuta.io>
Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
---
This solution does wind up doing the unaligned reads on the CPU instead,
although I'm not sure how significant of a penalty this is in practise.
---
drivers/input/rmi4/rmi_driver.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index cd3e4e77ab9b..58bf3dfbf81c 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c
@@ -136,9 +136,14 @@ static int rmi_process_interrupt_requests(struct rmi_device *rmi_dev)
return 0;
if (!data->attn_data.data) {
+ /*
+ * Read the device status register as well and ignore it.
+ * Some aftermarket ICs have issues with interrupt requests
+ * otherwise.
+ */
error = rmi_read_block(rmi_dev,
- data->f01_container->fd.data_base_addr + 1,
- data->irq_status, data->num_of_irq_regs);
+ data->f01_container->fd.data_base_addr,
+ data->irq_status - 1, data->num_of_irq_regs + 1);
if (error < 0) {
dev_err(dev, "Failed to read irqs, code=%d\n", error);
return error;
@@ -1083,16 +1088,17 @@ int rmi_probe_interrupts(struct rmi_driver_data *data)
data->num_of_irq_regs = (data->irq_count + 7) / 8;
size = BITS_TO_LONGS(data->irq_count) * sizeof(unsigned long);
- data->irq_memory = devm_kcalloc(dev, size, 4, GFP_KERNEL);
+ data->irq_memory = devm_kzalloc(dev, size * 4 + 1, GFP_KERNEL);
if (!data->irq_memory) {
dev_err(dev, "Failed to allocate memory for irq masks.\n");
return -ENOMEM;
}
- data->irq_status = data->irq_memory + size * 0;
- data->fn_irq_bits = data->irq_memory + size * 1;
- data->current_irq_mask = data->irq_memory + size * 2;
- data->new_irq_mask = data->irq_memory + size * 3;
+ /* The first byte is reserved for the device status register */
+ data->irq_status = data->irq_memory + size * 0 + 1;
+ data->fn_irq_bits = data->irq_memory + size * 1 + 1;
+ data->current_irq_mask = data->irq_memory + size * 2 + 1;
+ data->new_irq_mask = data->irq_memory + size * 3 + 1;
return retval;
}
--
2.42.0
^ permalink raw reply related
* [PATCH 4/7] Input: synaptics-rmi4 - f55: handle zero electrode count
From: Caleb Connolly @ 2023-09-30 17:08 UTC (permalink / raw)
To: Dmitry Torokhov, Vincent Huang
Cc: methanal, linux-input, devicetree, phone-devel,
~postmarketos/upstreaming, Caleb Connolly
In-Reply-To: <20230929-caleb-rmi4-quirks-v1-0-cc3c703f022d@linaro.org>
From: methanal <baclofen@tuta.io>
Some third party ICs claim to support f55 but report an electrode count
of 0. Catch this and bail out early so that we don't confuse the i2c bus
with 0 sized reads.
Signed-off-by: methanal <baclofen@tuta.io>
[simplify code, adjust wording]
Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
---
drivers/input/rmi4/rmi_f55.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/input/rmi4/rmi_f55.c b/drivers/input/rmi4/rmi_f55.c
index 488adaca4dd0..ad2ef14ae9f4 100644
--- a/drivers/input/rmi4/rmi_f55.c
+++ b/drivers/input/rmi4/rmi_f55.c
@@ -52,6 +52,11 @@ static int rmi_f55_detect(struct rmi_function *fn)
f55->num_rx_electrodes = f55->qry[F55_NUM_RX_OFFSET];
f55->num_tx_electrodes = f55->qry[F55_NUM_TX_OFFSET];
+ if (!f55->num_rx_electrodes || !f55->num_tx_electrodes) {
+ rmi_dbg(RMI_DEBUG_FN, &fn->dev,
+ "F55 query returned no electrodes, giving up\n");
+ return 0;
+ }
f55->cfg_num_rx_electrodes = f55->num_rx_electrodes;
f55->cfg_num_tx_electrodes = f55->num_rx_electrodes;
--
2.42.0
^ permalink raw reply related
* [PATCH 6/7] Input: synaptics-rmi4 - read product ID on aftermarket touch ICs
From: Caleb Connolly @ 2023-09-30 17:08 UTC (permalink / raw)
To: Dmitry Torokhov, Vincent Huang
Cc: methanal, linux-input, devicetree, phone-devel,
~postmarketos/upstreaming, Caleb Connolly
In-Reply-To: <20230929-caleb-rmi4-quirks-v1-0-cc3c703f022d@linaro.org>
From: methanal <baclofen@tuta.io>
Some replacement displays include third-party touch ICs which do not
report the product ID correctly unless we read directly from the
product ID register. Add a check and a fallback read to handle this.
Signed-off-by: methanal <baclofen@tuta.io>
Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
---
drivers/input/rmi4/rmi_f01.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
index d7603c50f864..4aee30d2dcde 100644
--- a/drivers/input/rmi4/rmi_f01.c
+++ b/drivers/input/rmi4/rmi_f01.c
@@ -250,6 +250,20 @@ static int rmi_f01_read_properties(struct rmi_device *rmi_dev,
}
}
+ /*
+ * Some aftermarket ICs put garbage into the product id field unless
+ * we read directly from the product id register.
+ */
+ if (props->product_id[0] < 0x20) {
+ ret = rmi_read_block(rmi_dev, query_base_addr + 11,
+ props->product_id, RMI_PRODUCT_ID_LENGTH);
+ if (ret) {
+ dev_err(&rmi_dev->dev,
+ "Failed to read product id: %d\n", ret);
+ return ret;
+ }
+ }
+
return 0;
}
--
2.42.0
^ permalink raw reply related
* [PATCH 7/7] Input: synaptics-rmi4 - support fallback values for PDT descriptor bytes
From: Caleb Connolly @ 2023-09-30 17:08 UTC (permalink / raw)
To: Dmitry Torokhov, Vincent Huang
Cc: methanal, linux-input, devicetree, phone-devel,
~postmarketos/upstreaming, Caleb Connolly
In-Reply-To: <20230929-caleb-rmi4-quirks-v1-0-cc3c703f022d@linaro.org>
From: methanal <baclofen@tuta.io>
Some replacement displays include third-party touch ICs which do not
expose the function number and the interrupt status in its PDT entries.
OnePlus 6 (original touch IC)
rmi4_i2c 12-0020: read 6 bytes at 0x00e3: 0 (2b 22 0d 06 01 01)
OnePlus 6 (aftermarket touch IC)
rmi4_i2c 12-0020: read 6 bytes at 0x00e3: 0 (2c 23 0d 06 00 00)
Introduce a new devicetree property `syna,pdt-desc` which can be used to
provide platform-specific fallback values for users with a replacement
display with an aftermarket touch IC.
Signed-off-by: methanal <baclofen@tuta.io>
[codeflow adjustments, checkpatch fixes, wording]
Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org>
---
drivers/input/rmi4/rmi_driver.c | 67 ++++++++++++++++++++++++++++++++++++++---
drivers/input/rmi4/rmi_driver.h | 2 ++
include/linux/rmi.h | 3 ++
3 files changed, 67 insertions(+), 5 deletions(-)
diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index 58bf3dfbf81c..5e1e3d5dd800 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c
@@ -461,9 +461,10 @@ static int rmi_driver_reset_handler(struct rmi_device *rmi_dev)
return 0;
}
-static int rmi_read_pdt_entry(struct rmi_device *rmi_dev,
- struct pdt_entry *entry, u16 pdt_address)
+static int rmi_read_pdt_entry(struct rmi_device *rmi_dev, struct pdt_entry *entry,
+ struct pdt_scan_state *state, u16 pdt_address)
{
+ const struct rmi_device_platform_data *pdata = rmi_get_platform_data(rmi_dev);
u8 buf[RMI_PDT_ENTRY_SIZE];
int error;
@@ -474,6 +475,21 @@ static int rmi_read_pdt_entry(struct rmi_device *rmi_dev,
return error;
}
+ if (pdata->pdt_fallback_size > state->pdt_count * RMI_OF_PDT_DESC_CELLS + 1) {
+ /* Use the description bytes from DT */
+ buf[5] = pdata->pdt_fallback_desc[state->pdt_count * RMI_OF_PDT_DESC_CELLS];
+ buf[4] = pdata->pdt_fallback_desc[state->pdt_count * RMI_OF_PDT_DESC_CELLS + 1];
+
+ error = rmi_read_block(rmi_dev, pdt_address, buf,
+ RMI_PDT_ENTRY_SIZE - 2);
+ if (error) {
+ dev_err(&rmi_dev->dev,
+ "Read PDT entry at %#06x failed, code: %d.\n",
+ pdt_address, error);
+ return error;
+ }
+ }
+
entry->page_start = pdt_address & RMI4_PAGE_MASK;
entry->query_base_addr = buf[0];
entry->command_base_addr = buf[1];
@@ -551,7 +567,7 @@ static int rmi_scan_pdt_page(struct rmi_device *rmi_dev,
int retval;
for (addr = pdt_start; addr >= pdt_end; addr -= RMI_PDT_ENTRY_SIZE) {
- error = rmi_read_pdt_entry(rmi_dev, &pdt_entry, addr);
+ error = rmi_read_pdt_entry(rmi_dev, &pdt_entry, state, addr);
if (error)
return error;
@@ -1028,9 +1044,11 @@ static int rmi_driver_remove(struct device *dev)
}
#ifdef CONFIG_OF
-static int rmi_driver_of_probe(struct device *dev,
+static int rmi_driver_of_probe(struct rmi_device *rmi_dev,
struct rmi_device_platform_data *pdata)
{
+ struct device *dev = rmi_dev->xport->dev;
+ u8 buf[RMI_PDT_ENTRY_SIZE];
int retval;
retval = rmi_of_property_read_u32(dev, &pdata->reset_delay_ms,
@@ -1038,6 +1056,45 @@ static int rmi_driver_of_probe(struct device *dev,
if (retval)
return retval;
+ /*
+ * In some aftermerket touch ICs, the first PDT entry is empty and
+ * the function number register is 0. If so, the platform
+ * may have provided backup PDT entries in the device tree.
+ */
+
+ retval = rmi_read_block(rmi_dev, PDT_START_SCAN_LOCATION,
+ buf, RMI_PDT_ENTRY_SIZE);
+ if (retval) {
+ dev_err(dev, "Read PDT entry at %#06x failed, code: %d.\n",
+ PDT_START_SCAN_LOCATION, retval);
+ return retval;
+ }
+
+ if (!RMI4_END_OF_PDT(buf[5]))
+ return 0;
+
+ pdata->pdt_fallback_size = of_property_count_u8_elems(dev->of_node,
+ "syna,pdt-fallback-desc");
+
+ /* The rmi4 driver would fail later anyway, so just error out now. */
+ if (pdata->pdt_fallback_size == -EINVAL) {
+ dev_err(dev, "First PDT entry is empty and no backup values provided\n");
+ return -EINVAL;
+ }
+
+ if (pdata->pdt_fallback_size < 0) {
+ dev_err(dev, "syna,ptd-desc property was not specified properly: %d\n",
+ pdata->pdt_fallback_size);
+ return pdata->pdt_fallback_size;
+ }
+
+ pdata->pdt_fallback_desc = devm_kzalloc(dev, pdata->pdt_fallback_size, GFP_KERNEL);
+
+ retval = of_property_read_u8_array(dev->of_node, "syna,pdt-fallback-desc",
+ pdata->pdt_fallback_desc, pdata->pdt_fallback_size);
+ if (retval < 0)
+ return retval;
+
return 0;
}
#else
@@ -1163,7 +1220,7 @@ static int rmi_driver_probe(struct device *dev)
pdata = rmi_get_platform_data(rmi_dev);
if (rmi_dev->xport->dev->of_node) {
- retval = rmi_driver_of_probe(rmi_dev->xport->dev, pdata);
+ retval = rmi_driver_of_probe(rmi_dev, pdata);
if (retval)
return retval;
}
diff --git a/drivers/input/rmi4/rmi_driver.h b/drivers/input/rmi4/rmi_driver.h
index e1a5412f2f8f..2531c32d6163 100644
--- a/drivers/input/rmi4/rmi_driver.h
+++ b/drivers/input/rmi4/rmi_driver.h
@@ -31,6 +31,8 @@
#define RMI_PDT_FUNCTION_VERSION_MASK 0x60
#define RMI_PDT_INT_SOURCE_COUNT_MASK 0x07
+#define RMI_OF_PDT_DESC_CELLS 2
+
#define PDT_START_SCAN_LOCATION 0x00e9
#define PDT_END_SCAN_LOCATION 0x0005
#define RMI4_END_OF_PDT(id) ((id) == 0x00 || (id) == 0xff)
diff --git a/include/linux/rmi.h b/include/linux/rmi.h
index ab7eea01ab42..974597960b5e 100644
--- a/include/linux/rmi.h
+++ b/include/linux/rmi.h
@@ -214,6 +214,9 @@ struct rmi_device_platform_data {
int reset_delay_ms;
int irq;
+ u8 *pdt_fallback_desc;
+ int pdt_fallback_size;
+
struct rmi_device_platform_data_spi spi_data;
/* function handler pdata */
--
2.42.0
^ permalink raw reply related
* Re: [PATCH v5 RESEND] HID: nintendo: cleanup LED code
From: Daniel Ogorchock @ 2023-09-30 19:52 UTC (permalink / raw)
To: Martino Fontana; +Cc: jikos, benjamin.tissoires, linux-input, linux-kernel
In-Reply-To: <20230924141547.11597-1-tinozzo123@gmail.com>
Hi Martino,
On Sun, Sep 24, 2023 at 10:16 AM Martino Fontana <tinozzo123@gmail.com> wrote:
>
> - Support player LED patterns up to 8 players.
> (Note that the behavior still consinsts in increasing the player number
> every time a controller is connected, never decreasing it. It should be
> as is described in https://bugzilla.kernel.org/show_bug.cgi?id=216225.
> However, any implementation here would stop making sense as soon as a
> non-Nintendo controller is connected, which is why I'm not bothering.)
>
> - Split part of `joycon_home_led_brightness_set` (which is called by hid)
> into `joycon_set_home_led` (which is what actually sets the LEDs), for
> consistency with player LEDs.
>
> - `joycon_player_led_brightness_set` won't try it to "determine which
> player led this is" anymore: it's already looking at every LED
> brightness value.
>
> - Instead of first registering the `led_classdev`, then attempting to set
> the LED and unregistering the `led_classdev` if it fails, first attempt
> to set the LED, then register the `led_classdev` only if it succeeds
> (the class is still filled up in either case).
>
> - If setting the player LEDs fails, still attempt setting the home LED.
> (I don't know there's a third party controller where this may actually
> happen, but who knows...)
>
> - Use `JC_NUM_LEDS` where appropriate instead of 4.
>
> - Print return codes in more places.
>
> - Use spinlock instead of mutex for `input_num`. Copy its value to a local
> variable, so that it can be unlocked immediately.
>
> - `input_num` starts counting from 0
>
> - Less holding of mutexes in general.
>
> Signed-off-by: Martino Fontana <tinozzo123@gmail.com>
> ---
> Changes for v2:
>
> Applied suggestions, commit message explains more stuff, restored `return ret`
> when `devm_led_classdev_register` fails (since all other hid drivers do this).
> If setting LED fails, `hid_warn` now explicitly says "skipping registration".
>
> Changes for v3 and v4:
>
> Fixed errors and warnings from test robot.
>
> Changes for v5:
>
> I thought that when connecting the controller on an actual Nintendo Switch,
> only the nth player LED would turn on, which is true... on Wii and Wii U.
> So I reverted that, and to compensate, now this supports the LED patterns
> up to 8 players.
>
> drivers/hid/hid-nintendo.c | 133 +++++++++++++++++++++----------------
> 1 file changed, 76 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
> index 10468f727..138f154fe 100644
> --- a/drivers/hid/hid-nintendo.c
> +++ b/drivers/hid/hid-nintendo.c
> @@ -410,6 +410,18 @@ static const char * const joycon_player_led_names[] = {
> LED_FUNCTION_PLAYER4,
> };
> #define JC_NUM_LEDS ARRAY_SIZE(joycon_player_led_names)
> +#define JC_NUM_LED_PATTERNS 8
> +/* Taken from https://www.nintendo.com/my/support/qa/detail/33822 */
> +static const enum led_brightness joycon_player_led_patterns[JC_NUM_LED_PATTERNS][JC_NUM_LEDS] = {
> + { 1, 0, 0, 0 },
> + { 1, 1, 0, 0 },
> + { 1, 1, 1, 0 },
> + { 1, 1, 1, 1 },
> + { 1, 0, 0, 1 },
> + { 1, 0, 1, 0 },
> + { 1, 0, 1, 1 },
> + { 0, 1, 1, 0 },
> +};
>
> /* Each physical controller is associated with a joycon_ctlr struct */
> struct joycon_ctlr {
> @@ -699,6 +711,25 @@ static int joycon_set_player_leds(struct joycon_ctlr *ctlr, u8 flash, u8 on)
> return joycon_send_subcmd(ctlr, req, 1, HZ/4);
> }
>
> +static int joycon_set_home_led(struct joycon_ctlr *ctlr, enum led_brightness brightness)
> +{
> + struct joycon_subcmd_request *req;
> + u8 buffer[sizeof(*req) + 5] = { 0 };
> + u8 *data;
> +
> + req = (struct joycon_subcmd_request *)buffer;
> + req->subcmd_id = JC_SUBCMD_SET_HOME_LIGHT;
> + data = req->data;
> + data[0] = 0x01;
> + data[1] = brightness << 4;
> + data[2] = brightness | (brightness << 4);
> + data[3] = 0x11;
> + data[4] = 0x11;
> +
> + hid_dbg(ctlr->hdev, "setting home led brightness\n");
> + return joycon_send_subcmd(ctlr, req, 5, HZ/4);
> +}
> +
> static int joycon_request_spi_flash_read(struct joycon_ctlr *ctlr,
> u32 start_addr, u8 size, u8 **reply)
> {
> @@ -1840,6 +1871,7 @@ static int joycon_input_create(struct joycon_ctlr *ctlr)
> return 0;
> }
>
> +/* Because the subcommand sets all the leds at once, the brightness argument is ignored */
> static int joycon_player_led_brightness_set(struct led_classdev *led,
> enum led_brightness brightness)
> {
> @@ -1849,7 +1881,6 @@ static int joycon_player_led_brightness_set(struct led_classdev *led,
> int val = 0;
> int i;
> int ret;
> - int num;
>
> ctlr = hid_get_drvdata(hdev);
> if (!ctlr) {
> @@ -1857,21 +1888,10 @@ static int joycon_player_led_brightness_set(struct led_classdev *led,
> return -ENODEV;
> }
>
> - /* determine which player led this is */
> - for (num = 0; num < JC_NUM_LEDS; num++) {
> - if (&ctlr->leds[num] == led)
> - break;
> - }
> - if (num >= JC_NUM_LEDS)
> - return -EINVAL;
> + for (i = 0; i < JC_NUM_LEDS; i++)
> + val |= ctlr->leds[i].brightness << i;
>
> mutex_lock(&ctlr->output_mutex);
> - for (i = 0; i < JC_NUM_LEDS; i++) {
> - if (i == num)
> - val |= brightness << i;
> - else
> - val |= ctlr->leds[i].brightness << i;
> - }
> ret = joycon_set_player_leds(ctlr, 0, val);
> mutex_unlock(&ctlr->output_mutex);
>
> @@ -1884,9 +1904,6 @@ static int joycon_home_led_brightness_set(struct led_classdev *led,
> struct device *dev = led->dev->parent;
> struct hid_device *hdev = to_hid_device(dev);
> struct joycon_ctlr *ctlr;
> - struct joycon_subcmd_request *req;
> - u8 buffer[sizeof(*req) + 5] = { 0 };
> - u8 *data;
> int ret;
>
> ctlr = hid_get_drvdata(hdev);
> @@ -1894,43 +1911,35 @@ static int joycon_home_led_brightness_set(struct led_classdev *led,
> hid_err(hdev, "No controller data\n");
> return -ENODEV;
> }
> -
> - req = (struct joycon_subcmd_request *)buffer;
> - req->subcmd_id = JC_SUBCMD_SET_HOME_LIGHT;
> - data = req->data;
> - data[0] = 0x01;
> - data[1] = brightness << 4;
> - data[2] = brightness | (brightness << 4);
> - data[3] = 0x11;
> - data[4] = 0x11;
> -
> - hid_dbg(hdev, "setting home led brightness\n");
> mutex_lock(&ctlr->output_mutex);
> - ret = joycon_send_subcmd(ctlr, req, 5, HZ/4);
> + ret = joycon_set_home_led(ctlr, brightness);
> mutex_unlock(&ctlr->output_mutex);
> -
> return ret;
> }
>
> -static DEFINE_MUTEX(joycon_input_num_mutex);
> +static DEFINE_SPINLOCK(joycon_input_num_spinlock);
> static int joycon_leds_create(struct joycon_ctlr *ctlr)
> {
> struct hid_device *hdev = ctlr->hdev;
> struct device *dev = &hdev->dev;
> const char *d_name = dev_name(dev);
> struct led_classdev *led;
> + int led_val = 0;
> char *name;
> - int ret = 0;
> + int ret;
> int i;
> - static int input_num = 1;
> + unsigned long flags;
> + int player_led_pattern;
> + static int input_num;
>
> - /* Set the default controller player leds based on controller number */
> - mutex_lock(&joycon_input_num_mutex);
> - mutex_lock(&ctlr->output_mutex);
> - ret = joycon_set_player_leds(ctlr, 0, 0xF >> (4 - input_num));
> - if (ret)
> - hid_warn(ctlr->hdev, "Failed to set leds; ret=%d\n", ret);
> - mutex_unlock(&ctlr->output_mutex);
> + /*
> + * Set the player leds based on controller number
> + * Because there is no standard concept of "player number", the pattern
> + * number will simply increase by 1 every time a controller is connected.
> + */
> + spin_lock_irqsave(&joycon_input_num_spinlock, flags);
> + player_led_pattern = input_num++ % JC_NUM_LED_PATTERNS;
> + spin_unlock_irqrestore(&joycon_input_num_spinlock, flags);
>
> /* configure the player LEDs */
> for (i = 0; i < JC_NUM_LEDS; i++) {
> @@ -1938,31 +1947,37 @@ static int joycon_leds_create(struct joycon_ctlr *ctlr)
> d_name,
> "green",
> joycon_player_led_names[i]);
> - if (!name) {
> - mutex_unlock(&joycon_input_num_mutex);
> + if (!name)
> return -ENOMEM;
> - }
>
> led = &ctlr->leds[i];
> led->name = name;
> - led->brightness = ((i + 1) <= input_num) ? 1 : 0;
> + led->brightness = joycon_player_led_patterns[player_led_pattern][i];
> led->max_brightness = 1;
> led->brightness_set_blocking =
> joycon_player_led_brightness_set;
> led->flags = LED_CORE_SUSPENDRESUME | LED_HW_PLUGGABLE;
>
> + led_val |= joycon_player_led_patterns[player_led_pattern][i] << i;
> + }
> + mutex_lock(&ctlr->output_mutex);
> + ret = joycon_set_player_leds(ctlr, 0, led_val);
> + mutex_unlock(&ctlr->output_mutex);
> + if (ret) {
> + hid_warn(hdev, "Failed to set players LEDs, skipping registration; ret=%d\n", ret);
> + goto home_led;
> + }
> +
> + for (i = 0; i < JC_NUM_LEDS; i++) {
> + led = &ctlr->leds[i];
> ret = devm_led_classdev_register(&hdev->dev, led);
> if (ret) {
> - hid_err(hdev, "Failed registering %s LED\n", led->name);
> - mutex_unlock(&joycon_input_num_mutex);
> + hid_err(hdev, "Failed to register player %d LED; ret=%d\n", i + 1, ret);
> return ret;
> }
> }
>
> - if (++input_num > 4)
> - input_num = 1;
> - mutex_unlock(&joycon_input_num_mutex);
> -
> +home_led:
> /* configure the home LED */
> if (jc_type_has_right(ctlr)) {
> name = devm_kasprintf(dev, GFP_KERNEL, "%s:%s:%s",
> @@ -1978,16 +1993,20 @@ static int joycon_leds_create(struct joycon_ctlr *ctlr)
> led->max_brightness = 0xF;
> led->brightness_set_blocking = joycon_home_led_brightness_set;
> led->flags = LED_CORE_SUSPENDRESUME | LED_HW_PLUGGABLE;
> - ret = devm_led_classdev_register(&hdev->dev, led);
> +
> + /* Set the home LED to 0 as default state */
> + mutex_lock(&ctlr->output_mutex);
> + ret = joycon_set_home_led(ctlr, 0);
> + mutex_unlock(&ctlr->output_mutex);
> if (ret) {
> - hid_err(hdev, "Failed registering home led\n");
> - return ret;
> + hid_warn(hdev, "Failed to set home LED, skipping registration; ret=%d\n", ret);
> + return 0;
> }
> - /* Set the home LED to 0 as default state */
> - ret = joycon_home_led_brightness_set(led, 0);
> +
> + ret = devm_led_classdev_register(&hdev->dev, led);
> if (ret) {
> - hid_warn(hdev, "Failed to set home LED default, unregistering home LED");
> - devm_led_classdev_unregister(&hdev->dev, led);
> + hid_err(hdev, "Failed to register home LED; ret=%d\n", ret);
> + return ret;
> }
> }
>
> --
> 2.42.0
>
This looks good to me. Thanks for the patch.
Reviewed-by: Daniel J. Ogorchock <djogorchock@gmail.com>
^ permalink raw reply
* Re: [PATCH v3] HID: nintendo: reinitialize USB Pro Controller after resuming from suspend
From: Daniel Ogorchock @ 2023-09-30 19:55 UTC (permalink / raw)
To: Martino Fontana; +Cc: jikos, benjamin.tissoires, linux-input, linux-kernel
In-Reply-To: <20230924140927.9844-2-tinozzo123@gmail.com>
Hi Martino,
On Sun, Sep 24, 2023 at 10:13 AM Martino Fontana <tinozzo123@gmail.com> wrote:
>
> When suspending the computer, a Switch Pro Controller connected via USB will
> lose its internal status. However, because the USB connection was technically
> never lost, when resuming the computer, the driver will attempt to communicate
> with the controller as if nothing happened (and fail).
> Because of this, the user was forced to manually disconnect the controller
> (or to press the sync button on the controller to power it off), so that it
> can be re-initialized.
>
> With this patch, the controller will be automatically re-initialized after
> resuming from suspend.
>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=216233
>
> Signed-off-by: Martino Fontana <tinozzo123@gmail.com>
>
> ---
> Changes for v2 and v3: Applied suggestions
>
> drivers/hid/hid-nintendo.c | 175 ++++++++++++++++++++++---------------
> 1 file changed, 103 insertions(+), 72 deletions(-)
>
> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
> index 250f5d2f8..10468f727 100644
> --- a/drivers/hid/hid-nintendo.c
> +++ b/drivers/hid/hid-nintendo.c
> @@ -2088,7 +2088,9 @@ static int joycon_read_info(struct joycon_ctlr *ctlr)
> struct joycon_input_report *report;
>
> req.subcmd_id = JC_SUBCMD_REQ_DEV_INFO;
> + mutex_lock(&ctlr->output_mutex);
> ret = joycon_send_subcmd(ctlr, &req, 0, HZ);
> + mutex_unlock(&ctlr->output_mutex);
> if (ret) {
> hid_err(ctlr->hdev, "Failed to get joycon info; ret=%d\n", ret);
> return ret;
> @@ -2117,6 +2119,85 @@ static int joycon_read_info(struct joycon_ctlr *ctlr)
> return 0;
> }
>
> +static int joycon_init(struct hid_device *hdev)
> +{
> + struct joycon_ctlr *ctlr = hid_get_drvdata(hdev);
> + int ret = 0;
> +
> + mutex_lock(&ctlr->output_mutex);
> + /* if handshake command fails, assume ble pro controller */
> + if ((jc_type_is_procon(ctlr) || jc_type_is_chrggrip(ctlr)) &&
> + !joycon_send_usb(ctlr, JC_USB_CMD_HANDSHAKE, HZ)) {
> + hid_dbg(hdev, "detected USB controller\n");
> + /* set baudrate for improved latency */
> + ret = joycon_send_usb(ctlr, JC_USB_CMD_BAUDRATE_3M, HZ);
> + if (ret) {
> + hid_err(hdev, "Failed to set baudrate; ret=%d\n", ret);
> + goto out_unlock;
> + }
> + /* handshake */
> + ret = joycon_send_usb(ctlr, JC_USB_CMD_HANDSHAKE, HZ);
> + if (ret) {
> + hid_err(hdev, "Failed handshake; ret=%d\n", ret);
> + goto out_unlock;
> + }
> + /*
> + * Set no timeout (to keep controller in USB mode).
> + * This doesn't send a response, so ignore the timeout.
> + */
> + joycon_send_usb(ctlr, JC_USB_CMD_NO_TIMEOUT, HZ/10);
> + } else if (jc_type_is_chrggrip(ctlr)) {
> + hid_err(hdev, "Failed charging grip handshake\n");
> + ret = -ETIMEDOUT;
> + goto out_unlock;
> + }
> +
> + /* get controller calibration data, and parse it */
> + ret = joycon_request_calibration(ctlr);
> + if (ret) {
> + /*
> + * We can function with default calibration, but it may be
> + * inaccurate. Provide a warning, and continue on.
> + */
> + hid_warn(hdev, "Analog stick positions may be inaccurate\n");
> + }
> +
> + /* get IMU calibration data, and parse it */
> + ret = joycon_request_imu_calibration(ctlr);
> + if (ret) {
> + /*
> + * We can function with default calibration, but it may be
> + * inaccurate. Provide a warning, and continue on.
> + */
> + hid_warn(hdev, "Unable to read IMU calibration data\n");
> + }
> +
> + /* Set the reporting mode to 0x30, which is the full report mode */
> + ret = joycon_set_report_mode(ctlr);
> + if (ret) {
> + hid_err(hdev, "Failed to set report mode; ret=%d\n", ret);
> + goto out_unlock;
> + }
> +
> + /* Enable rumble */
> + ret = joycon_enable_rumble(ctlr);
> + if (ret) {
> + hid_err(hdev, "Failed to enable rumble; ret=%d\n", ret);
> + goto out_unlock;
> + }
> +
> + /* Enable the IMU */
> + ret = joycon_enable_imu(ctlr);
> + if (ret) {
> + hid_err(hdev, "Failed to enable the IMU; ret=%d\n", ret);
> + goto out_unlock;
> + }
> +
> +out_unlock:
> + mutex_unlock(&ctlr->output_mutex);
> + return ret;
> +}
> +
> /* Common handler for parsing inputs */
> static int joycon_ctlr_read_handler(struct joycon_ctlr *ctlr, u8 *data,
> int size)
> @@ -2248,85 +2329,19 @@ static int nintendo_hid_probe(struct hid_device *hdev,
>
> hid_device_io_start(hdev);
>
> - /* Initialize the controller */
> - mutex_lock(&ctlr->output_mutex);
> - /* if handshake command fails, assume ble pro controller */
> - if ((jc_type_is_procon(ctlr) || jc_type_is_chrggrip(ctlr)) &&
> - !joycon_send_usb(ctlr, JC_USB_CMD_HANDSHAKE, HZ)) {
> - hid_dbg(hdev, "detected USB controller\n");
> - /* set baudrate for improved latency */
> - ret = joycon_send_usb(ctlr, JC_USB_CMD_BAUDRATE_3M, HZ);
> - if (ret) {
> - hid_err(hdev, "Failed to set baudrate; ret=%d\n", ret);
> - goto err_mutex;
> - }
> - /* handshake */
> - ret = joycon_send_usb(ctlr, JC_USB_CMD_HANDSHAKE, HZ);
> - if (ret) {
> - hid_err(hdev, "Failed handshake; ret=%d\n", ret);
> - goto err_mutex;
> - }
> - /*
> - * Set no timeout (to keep controller in USB mode).
> - * This doesn't send a response, so ignore the timeout.
> - */
> - joycon_send_usb(ctlr, JC_USB_CMD_NO_TIMEOUT, HZ/10);
> - } else if (jc_type_is_chrggrip(ctlr)) {
> - hid_err(hdev, "Failed charging grip handshake\n");
> - ret = -ETIMEDOUT;
> - goto err_mutex;
> - }
> -
> - /* get controller calibration data, and parse it */
> - ret = joycon_request_calibration(ctlr);
> + ret = joycon_init(hdev);
> if (ret) {
> - /*
> - * We can function with default calibration, but it may be
> - * inaccurate. Provide a warning, and continue on.
> - */
> - hid_warn(hdev, "Analog stick positions may be inaccurate\n");
> - }
> -
> - /* get IMU calibration data, and parse it */
> - ret = joycon_request_imu_calibration(ctlr);
> - if (ret) {
> - /*
> - * We can function with default calibration, but it may be
> - * inaccurate. Provide a warning, and continue on.
> - */
> - hid_warn(hdev, "Unable to read IMU calibration data\n");
> - }
> -
> - /* Set the reporting mode to 0x30, which is the full report mode */
> - ret = joycon_set_report_mode(ctlr);
> - if (ret) {
> - hid_err(hdev, "Failed to set report mode; ret=%d\n", ret);
> - goto err_mutex;
> - }
> -
> - /* Enable rumble */
> - ret = joycon_enable_rumble(ctlr);
> - if (ret) {
> - hid_err(hdev, "Failed to enable rumble; ret=%d\n", ret);
> - goto err_mutex;
> - }
> -
> - /* Enable the IMU */
> - ret = joycon_enable_imu(ctlr);
> - if (ret) {
> - hid_err(hdev, "Failed to enable the IMU; ret=%d\n", ret);
> - goto err_mutex;
> + hid_err(hdev, "Failed to initialize controller; ret=%d\n", ret);
> + goto err_close;
> }
>
> ret = joycon_read_info(ctlr);
> if (ret) {
> hid_err(hdev, "Failed to retrieve controller info; ret=%d\n",
> ret);
> - goto err_mutex;
> + goto err_close;
> }
>
> - mutex_unlock(&ctlr->output_mutex);
> -
> /* Initialize the leds */
> ret = joycon_leds_create(ctlr);
> if (ret) {
> @@ -2352,8 +2367,6 @@ static int nintendo_hid_probe(struct hid_device *hdev,
> hid_dbg(hdev, "probe - success\n");
> return 0;
>
> -err_mutex:
> - mutex_unlock(&ctlr->output_mutex);
> err_close:
> hid_hw_close(hdev);
> err_stop:
> @@ -2383,6 +2396,20 @@ static void nintendo_hid_remove(struct hid_device *hdev)
> hid_hw_stop(hdev);
> }
>
> +#ifdef CONFIG_PM
> +
> +static int nintendo_hid_resume(struct hid_device *hdev)
> +{
> + int ret = joycon_init(hdev);
> +
> + if (ret)
> + hid_err(hdev, "Failed to restore controller after resume");
> +
> + return ret;
> +}
> +
> +#endif
> +
> static const struct hid_device_id nintendo_hid_devices[] = {
> { HID_USB_DEVICE(USB_VENDOR_ID_NINTENDO,
> USB_DEVICE_ID_NINTENDO_PROCON) },
> @@ -2404,6 +2431,10 @@ static struct hid_driver nintendo_hid_driver = {
> .probe = nintendo_hid_probe,
> .remove = nintendo_hid_remove,
> .raw_event = nintendo_hid_event,
> +
> +#ifdef CONFIG_PM
> + .resume = nintendo_hid_resume,
> +#endif
> };
> module_hid_driver(nintendo_hid_driver);
>
> --
> 2.42.0
>
Thanks for adding the resume hook for usb controllers. Looks good to me.
Reviewed-by: Daniel J. Ogorchock <djogorchock@gmail.com>
^ 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