* [PATCH] HID: steelseries: Fix signedness bug in steelseries_headset_arctis_1_fetch_battery()
From: Dan Carpenter @ 2023-09-07 9:55 UTC (permalink / raw)
To: Bastien Nocera
Cc: Jiri Kosina, Benjamin Tissoires, linux-input, kernel-janitors
The hid_hw_raw_request() function returns negative error codes or the
number bytes transferred. If it returns negative error codes, then the
problem is that when we check if "ret < sizeof(arctis_1_battery_request)",
negative values are type promoted to high unsigned values and treated as
success. Add an explicit check for negatives to address this issue.
Fixes: a0c76896c3fb ("HID: steelseries: Add support for Arctis 1 XBox")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
drivers/hid/hid-steelseries.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid-steelseries.c
index 43d2cf7153d7..485d2287d58a 100644
--- a/drivers/hid/hid-steelseries.c
+++ b/drivers/hid/hid-steelseries.c
@@ -390,7 +390,7 @@ static int steelseries_headset_arctis_1_fetch_battery(struct hid_device *hdev)
ret = hid_hw_raw_request(hdev, arctis_1_battery_request[0],
write_buf, sizeof(arctis_1_battery_request),
HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
- if (ret < sizeof(arctis_1_battery_request)) {
+ if (ret < 0 || ret < sizeof(arctis_1_battery_request)) {
hid_err(hdev, "hid_hw_raw_request() failed with %d\n", ret);
ret = -ENODATA;
}
--
2.39.2
^ permalink raw reply related
* Re: [PATCH v5] hid-mcp2200: added driver for GPIOs of MCP2200
From: Johannes Roith @ 2023-09-07 16:41 UTC (permalink / raw)
To: sergeantsagara
Cc: ak, andi.shyti, benjamin.tissoires, christophe.jaillet, jikos,
johannes, linux-input, linux-kernel, rdunlap
In-Reply-To: <87ledpvhgm.fsf@protonmail.com>
Hi Rahul,
thank you for the explanation, now I got it.
I have added hid_hw_stop and hid_hw_close to my remove function and removed the
devm_add_action_or_reset. The driver still worked well.
So, if it is okay for you, I would go this way and generate a new patch.
Best regards,
Johannes
^ permalink raw reply
* Missing ABS_PRESSURE/ABS_MT_PRESSURE for SYNA7DB5:00 06CB:CD7E (Acer Swift Edge 16 SFE16-43)
From: Rain @ 2023-09-07 22:00 UTC (permalink / raw)
To: linux-input
Hi there --
I have a new Acer Swift Edge 16 (model SFE16-43) I'm trying out and it
looks like on OpenSUSE Tumbleweed, as of kernel 6.4.12, the touchpad
is missing the ABS_PRESSURE and/or ABS_MT_PRESSURE device bits. The
device itself works fine, but without pressure information I think
there seem to be some finger detection issues (specifically, light
enough touches don't appear to register).
I'm not sure if the hardware itself doesn't support these bits
or if a quirk needs to be added to the kernel. I'm happy to help try
and figure this out. My understanding is that all modern Synaptics
touchpads support pressure information, though I could certainly be
wrong about that.)
Some debugging output that might be useful. Happy to provide more
as necessary:
---
# uname -a
Linux cumulus 6.4.12-1-default #1 SMP PREEMPT_DYNAMIC Fri Aug 25 08:26:31 UTC 2023 (f5aa89b) x86_64 x86_64 x86_64 GNU/Linux
# libinput measure touchpad-pressure
Using SYNA7DB5:00 06CB:CD7E Touchpad: /dev/input/event2
This device does not have the capabilities for pressure-based touch detection.
Details: Device does not have ABS_PRESSURE or ABS_MT_PRESSURE
# dmesg | grep SYNA
[ 2.034760] input: SYNA7DB5:00 06CB:CD7E Mouse as /devices/platform/AMDI0010:00/i2c-0/i2c-SYNA7DB5:00/0018:06CB:CD7E.0001/input/input1
[ 2.034865] input: SYNA7DB5:00 06CB:CD7E Touchpad as /devices/platform/AMDI0010:00/i2c-0/i2c-SYNA7DB5:00/0018:06CB:CD7E.0001/input/input2
[ 2.036331] hid-generic 0018:06CB:CD7E.0001: input,hidraw0: I2C HID v1.00 Mouse [SYNA7DB5:00 06CB:CD7E] on i2c-SYNA7DB5:00
[ 2.266243] input: SYNA7DB5:00 06CB:CD7E Mouse as /devices/platform/AMDI0010:00/i2c-0/i2c-SYNA7DB5:00/0018:06CB:CD7E.0001/input/input5
[ 2.266501] input: SYNA7DB5:00 06CB:CD7E Touchpad as /devices/platform/AMDI0010:00/i2c-0/i2c-SYNA7DB5:00/0018:06CB:CD7E.0001/input/input6
[ 2.266821] hid-multitouch 0018:06CB:CD7E.0001: input,hidraw0: I2C HID v1.00 Mouse [SYNA7DB5:00 06CB:CD7E] on i2c-SYNA7DB5:00
---
A thing that's a bit strange to me is the separate reporting of mouse and
touchpad devices. I don't know if that's a red herring or a clue.
Reporting this directly here per similarity to [1] which was closed in libinput.
Thank you!
[1] https://gitlab.freedesktop.org/libinput/libinput/-/issues/780
^ permalink raw reply
* Re: [PATCH v2 RESEND] HID: nintendo: reinitialize USB Pro Controller after resuming from suspend
From: Daniel Ogorchock @ 2023-09-08 1:10 UTC (permalink / raw)
To: Martino Fontana; +Cc: jikos, benjamin.tissoires, linux-input, linux-kernel
In-Reply-To: <20230906102507.15504-2-tinozzo123@gmail.com>
On Wed, Sep 6, 2023 at 6:25 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>
Hi Martino,
Thanks for the patch. This has been on the backlog for a long time, so
it's great to see that USB resume is handled now.
Have you tested how this behaves for bluetooth controllers? Does the
pm resume hook always result in error logs for bluetooth controllers,
or has the kernel already removed the device before resume?
> ---
> drivers/hid/hid-nintendo.c | 178 ++++++++++++++++++++++---------------
> 1 file changed, 106 insertions(+), 72 deletions(-)
>
> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
> index 250f5d2f8..a5ebe857a 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,88 @@ 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 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);
> + 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;
> + }
> +
> + mutex_unlock(&ctlr->output_mutex);
> + return 0;
If desired, you could remove the above two lines and allow flow to
continue through err_mutex even in the success case.
> +
> +err_mutex:
> + 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 +2332,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);
> - 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);
> + ret = joycon_init(hdev);
> 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 +2370,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 +2399,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 +2434,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
Something else we should do is add a suspend hook to power off the
bluetooth-connected controllers. As of now, they remain powered on
during suspend.
> };
> module_hid_driver(nintendo_hid_driver);
>
> --
> 2.41.0
>
Thanks,
Daniel
^ permalink raw reply
* Re: [PATCH v4] HID: nintendo: cleanup LED code
From: Daniel Ogorchock @ 2023-09-08 1:22 UTC (permalink / raw)
To: Martino Fontana; +Cc: jikos, benjamin.tissoires, linux-input, linux-kernel
In-Reply-To: <20230907094926.8278-1-tinozzo123@gmail.com>
On Thu, Sep 7, 2023 at 5:51 AM Martino Fontana <tinozzo123@gmail.com> wrote:
>
> - Only turn on the nth LED, instead of all the LEDs up to n. This better
> matches Nintendo consoles' behaviour, as they never turn on more than
> one LED.
> (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.
>
> 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.
>
> Signed-off-by: Martino Fontana <tinozzo123@gmail.com>
Hi Martino,
This cleanup looks pretty good to me.
Regarding the player LED setting though, I'm pretty sure the driver's
existing behavior matches that of an actual Switch console (at least
for the first 4 players).
1 LED lights up for P1, 2 for P2, etc.
See the nintendo documentation here:
https://www.nintendo.com/my/support/qa/detail/33822
Thanks,
Daniel
> ---
> drivers/hid/hid-nintendo.c | 113 ++++++++++++++++++-------------------
> 1 file changed, 56 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
> index a5ebe857a..543098a8c 100644
> --- a/drivers/hid/hid-nintendo.c
> +++ b/drivers/hid/hid-nintendo.c
> @@ -699,6 +699,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)
> {
> @@ -1849,7 +1868,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 +1875,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 +1891,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,25 +1898,13 @@ 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;
> @@ -1920,17 +1912,16 @@ static int joycon_leds_create(struct joycon_ctlr *ctlr)
> const char *d_name = dev_name(dev);
> struct led_classdev *led;
> char *name;
> - int ret = 0;
> + int ret;
> int i;
> - static int input_num = 1;
> + unsigned long flags;
> + int player_led_number;
> + 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 */
> + spin_lock_irqsave(&joycon_input_num_spinlock, flags);
> + player_led_number = input_num++ % JC_NUM_LEDS;
> + spin_unlock_irqrestore(&joycon_input_num_spinlock, flags);
>
> /* configure the player LEDs */
> for (i = 0; i < JC_NUM_LEDS; i++) {
> @@ -1938,31 +1929,35 @@ 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 = (i == player_led_number) ? 1 : 0;
> led->max_brightness = 1;
> led->brightness_set_blocking =
> joycon_player_led_brightness_set;
> led->flags = LED_CORE_SUSPENDRESUME | LED_HW_PLUGGABLE;
> + }
> + mutex_lock(&ctlr->output_mutex);
> + ret = joycon_set_player_leds(ctlr, 0, 0x1 << player_led_number);
> + 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 +1973,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.41.0
>
--
Daniel Ogorchock
^ permalink raw reply
* Re: [PATCH v5] hid-mcp2200: added driver for GPIOs of MCP2200
From: Rahul Rameshbabu @ 2023-09-08 1:34 UTC (permalink / raw)
To: Johannes Roith
Cc: ak, andi.shyti, benjamin.tissoires, christophe.jaillet, jikos,
linux-input, linux-kernel, rdunlap
In-Reply-To: <20230907164121.21092-1-johannes@gnu-linux.rocks>
Hi Johannes,
On Thu, 07 Sep, 2023 18:41:21 +0200 "Johannes Roith" <johannes@gnu-linux.rocks> wrote:
> Hi Rahul,
>
> thank you for the explanation, now I got it.
>
> I have added hid_hw_stop and hid_hw_close to my remove function and removed the
> devm_add_action_or_reset. The driver still worked well.
Glad to hear this worked out. This discussion has motivated me to take a
deeper look at hid_device_remove in the near future to see what can be
done for devices that need to invoke hid_hw_open without needing to
explicitly implement a remove callback.
>
> So, if it is okay for you, I would go this way and generate a new patch.
This sounds great. Excited to see your patch on the mailing list.
--
Thanks,
Rahul Rameshbabu
^ permalink raw reply
* [PATCH 0/1] Add support for aXiom TouchNetix touchscreen
From: Kamel Bouhara @ 2023-09-08 15:32 UTC (permalink / raw)
To: linux-input
Cc: mark.satterthwaite, pedro.torruella, bartp, hannah.rossiter,
Thomas Petazzoni, Gregory Clement, bsp-development.geo,
Kamel Bouhara
Hello,
This patch add support for an I2C touchscreen controller
from TouchNetix's aXiom family devices.
It is based on the vendor version:
https://github.com/TouchNetix/aXiom_Linux_Kernel_Module
I only have the I2C variant of the aXiom device but this
could also support SPI and USB interfaces in the future.
Regards,
Kamel
Kamel Bouhara (1):
Input: add driver for TouchNetix aXiom touchscreen
.../devicetree/bindings/vendor-prefixes.yaml | 2 +
MAINTAINERS | 7 +
drivers/input/touchscreen/Kconfig | 11 +
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/axiom_core.c | 382 ++++++++++++++++++
drivers/input/touchscreen/axiom_core.h | 142 +++++++
drivers/input/touchscreen/axiom_i2c.c | 353 ++++++++++++++++
7 files changed, 898 insertions(+)
create mode 100644 drivers/input/touchscreen/axiom_core.c
create mode 100644 drivers/input/touchscreen/axiom_core.h
create mode 100644 drivers/input/touchscreen/axiom_i2c.c
--
2.25.1
^ permalink raw reply
* [PATCH] Input: add driver for TouchNetix aXiom touchscreen
From: Kamel Bouhara @ 2023-09-08 15:32 UTC (permalink / raw)
To: linux-input
Cc: mark.satterthwaite, pedro.torruella, bartp, hannah.rossiter,
Thomas Petazzoni, Gregory Clement, bsp-development.geo,
Kamel Bouhara
In-Reply-To: <20230908153203.122062-1-kamel.bouhara@bootlin.com>
Add a new driver for the TouchNetix's aXiom family of
multi-touch controller. This driver only support i2c
and can be later adapted for SPI and USB support.
Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
---
.../devicetree/bindings/vendor-prefixes.yaml | 2 +
MAINTAINERS | 7 +
drivers/input/touchscreen/Kconfig | 11 +
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/axiom_core.c | 382 ++++++++++++++++++
drivers/input/touchscreen/axiom_core.h | 140 +++++++
drivers/input/touchscreen/axiom_i2c.c | 349 ++++++++++++++++
7 files changed, 892 insertions(+)
create mode 100644 drivers/input/touchscreen/axiom_core.c
create mode 100644 drivers/input/touchscreen/axiom_core.h
create mode 100644 drivers/input/touchscreen/axiom_i2c.c
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 573578db9509..b0a3ed451f15 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -175,6 +175,8 @@ patternProperties:
"^awinic,.*":
description: Shanghai Awinic Technology Co., Ltd.
"^axentia,.*":
+ description: TouchNetix
+ "^axiom,.*":
description: Axentia Technologies AB
"^axis,.*":
description: Axis Communications AB
diff --git a/MAINTAINERS b/MAINTAINERS
index 389fe9e38884..43add48257d8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3373,6 +3373,13 @@ W: https://ez.analog.com/linux-software-drivers
F: Documentation/devicetree/bindings/hwmon/adi,axi-fan-control.yaml
F: drivers/hwmon/axi-fan-control.c
+AXIOM I2C TOUCHSCREEN DRIVER
+M: Kamel Bouhara <kamel.bouhara@bootlin.com>
+L: linux-input@vger.kernel.org
+S: Maintained
+F: drivers/input/touchscreen/axiom_core.c
+F: drivers/input/touchscreen/axiom_i2.c
+
AXXIA I2C CONTROLLER
M: Krzysztof Adamski <krzysztof.adamski@nokia.com>
L: linux-i2c@vger.kernel.org
diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index e3e2324547b9..08a770a0c5e5 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -150,6 +150,17 @@ config TOUCHSCREEN_AUO_PIXCIR
To compile this driver as a module, choose M here: the
module will be called auo-pixcir-ts.
+config TOUCHSCREEN_AXIOM_I2C
+ tristate "AXIOM based multi-touch panel controllers"
+ help
+ Say Y here if you have a axiom touchscreen connected to
+ your system.
+
+ If unsure, say N.
+
+ To compile this driver as a module, choose M here: the
+ module will be called axiom_i2c.
+
config TOUCHSCREEN_BU21013
tristate "BU21013 based touch panel controllers"
depends on I2C
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 62bd24f3ac8e..59a3234ddb09 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_TOUCHSCREEN_ADS7846) += ads7846.o
obj-$(CONFIG_TOUCHSCREEN_AR1021_I2C) += ar1021_i2c.o
obj-$(CONFIG_TOUCHSCREEN_ATMEL_MXT) += atmel_mxt_ts.o
obj-$(CONFIG_TOUCHSCREEN_AUO_PIXCIR) += auo-pixcir-ts.o
+obj-$(CONFIG_TOUCHSCREEN_AXIOM_I2C) += axiom_core.o axiom_i2c.o
obj-$(CONFIG_TOUCHSCREEN_BU21013) += bu21013_ts.o
obj-$(CONFIG_TOUCHSCREEN_BU21029) += bu21029_ts.o
obj-$(CONFIG_TOUCHSCREEN_CHIPONE_ICN8318) += chipone_icn8318.o
diff --git a/drivers/input/touchscreen/axiom_core.c b/drivers/input/touchscreen/axiom_core.c
new file mode 100644
index 000000000000..d381afd7fb84
--- /dev/null
+++ b/drivers/input/touchscreen/axiom_core.c
@@ -0,0 +1,382 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * TouchNetix aXiom Touchscreen Driver
+ *
+ * Copyright (C) 2020-2023 TouchNetix Ltd.
+ *
+ * Author(s): Mark Satterthwaite <mark.satterthwaite@touchnetix.com>
+ * Pedro Torruella <pedro.torruella@touchnetix.com>
+ * Bart Prescott <bartp@baasheep.co.uk>
+ * Hannah Rossiter <hannah.rossiter@touchnetix.com>
+ *
+ */
+
+#include <linux/crc16.h>
+#include <linux/device.h>
+#include <linux/input.h>
+#include <linux/input/mt.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+
+#include "axiom_core.h"
+
+/**
+ * Decodes and populates the local u31 structure.
+ * Given a buffer of data read from page 0 of u31 in an aXiom device.
+ */
+void axiom_get_dev_info(struct axiom_data *ts, char *info)
+{
+ struct axiom_devinfo *devinfo = &ts->devinfo;
+
+ if (devinfo) {
+ devinfo->bootloader_mode = ((info[1] & 0x80) != 0) ? 1 : 0;
+ devinfo->device_id = ((info[1] & 0x7F) << 8) | info[0];
+ devinfo->fw_minor = info[2];
+ devinfo->fw_major = info[3];
+ devinfo->fw_info_extra = (info[4]) | (info[5] << 8);
+ devinfo->bootloader_fw_ver_minor = info[6];
+ devinfo->bootloader_fw_ver_major = info[7];
+ devinfo->jedec_id = (info[8]) | (info[9] << 8);
+ devinfo->num_usages = info[10];
+ devinfo->silicon_revision = info[11];
+ }
+}
+EXPORT_SYMBOL_GPL(axiom_get_dev_info);
+
+/**
+ * Decodes and populates the local Usage Table.
+ * Given a buffer of data read from page 1 onwards of u31 from an aXiom
+ * device.
+ */
+char axiom_populate_usage_table(struct axiom_data *ts, char *rx_data)
+{
+ u32 usage_id = 0;
+ char max_report_len = 0;
+ struct axiom_devinfo *device_info;
+ struct usage_entry *usage_table;
+
+ device_info = &ts->devinfo;
+ usage_table = ts->usage_table;
+
+ for (usage_id = 0; usage_id < device_info->num_usages; usage_id++) {
+ u16 offset = (usage_id * U31_BYTES_PER_USAGE);
+ char id = rx_data[offset + 0];
+ char start_page = rx_data[offset + 1];
+ char num_pages = rx_data[offset + 2];
+ char max_offset = ((rx_data[offset + 3] & 0x7F) + 1) * 2;
+
+ /* Store the entry into the usage table */
+ usage_table[usage_id].id = id;
+ usage_table[usage_id].is_report = ((num_pages == 0) ? 1 : 0);
+ usage_table[usage_id].start_page = start_page;
+ usage_table[usage_id].num_pages = num_pages;
+
+ dev_dbg(ts->pdev, "Usage %2u Info: %*ph\n", usage_id,
+ U31_BYTES_PER_USAGE, &rx_data[offset]);
+
+ /* Identify the max report length the module will receive */
+ if ((usage_table[usage_id].is_report)
+ && (max_offset > max_report_len))
+ max_report_len = max_offset;
+ }
+ ts->usage_table_populated = true;
+
+ return max_report_len;
+}
+EXPORT_SYMBOL_GPL(axiom_populate_usage_table);
+
+/**
+ * Translate usage/page/offset triplet into physical address.
+ *
+ * @usage: groups of registers
+ * @page: page to which the usage belongs to offset
+ * @offset of the usage
+ */
+u16
+usage_to_target_address(struct axiom_data *ts, char usage, char page,
+ char offset)
+{
+ struct axiom_devinfo *device_info;
+ struct usage_entry *usage_table;
+ u16 target_address;
+ u32 i;
+
+ device_info = &ts->devinfo;
+ usage_table = ts->usage_table;
+
+ /* At the moment the convention is that u31 is always at physical address 0x0 */
+ if (!ts->usage_table_populated && (usage == DEVINFO_USAGE_ID)) {
+ target_address = ((page << 8) + offset);
+ } else if (ts->usage_table_populated) {
+ for (i = 0; i < device_info->num_usages; i++) {
+ if (usage_table[i].id == usage) {
+ if (page < usage_table[i].num_pages) {
+ target_address =
+ ((usage_table[i].start_page + page) << 8) + offset;
+ } else {
+ target_address = 0xFFFF;
+ dev_err(ts->pdev,
+ "Invalid usage table! usage: %u, page: %u, offset: %u\n",
+ usage, page, offset);
+ }
+ break;
+ }
+ }
+ } else {
+ target_address = 0xFFFF;
+ dev_err(ts->pdev, "Unpopulated usage table for usage: %u\n",
+ usage);
+ }
+
+ dev_dbg(ts->pdev, "target_address is 0x%04x for usage: %u page %u\n",
+ target_address, usage, page);
+
+ return target_address;
+}
+EXPORT_SYMBOL_GPL(usage_to_target_address);
+
+void axiom_remove(struct axiom_data *ts)
+{
+ if (ts->usage_table)
+ ts->usage_table_populated = false;
+
+ if (ts->input_dev)
+ input_unregister_device(ts->input_dev);
+}
+EXPORT_SYMBOL_GPL(axiom_remove);
+
+/*
+ * Support function to axiom_process_u41_report.
+ * It generates input-subsystem events for every target.
+ * After calling this function the caller shall issue
+ * a Sync to the input sub-system.
+ */
+static bool
+axiom_process_u41_report_target(struct axiom_data *ts,
+ struct axiom_target_report *target)
+{
+ struct input_dev *input_dev = ts->input_dev;
+ enum axiom_target_state current_state;
+ struct u41_target *target_prev_state;
+ struct device *pdev = ts->pdev;
+ bool update = false;
+ int slot;
+
+ /* Verify the target index */
+ if (target->index >= U41_MAX_TARGETS) {
+ dev_dbg(pdev, "Invalid target index! %u\n", target->index);
+ return false;
+ }
+
+ target_prev_state = &ts->targets[target->index];
+
+ current_state =
+ ((target->present == 0) ? TARGET_STATE_NOT_PRESENT : (target->z >=
+ 0) ?
+ TARGET_STATE_TOUCHING : (target->z > PROX_LEVEL)
+ && (target->z < 0) ? TARGET_STATE_HOVER : (target->z ==
+ PROX_LEVEL) ?
+ TARGET_STATE_PROX : TARGET_STATE_NOT_PRESENT);
+ if ((target_prev_state->state == current_state)
+ && (target_prev_state->x == target->x)
+ && (target_prev_state->y == target->y)
+ && (target_prev_state->z == target->z)) {
+ return false;
+ }
+
+ slot = target->index;
+
+ dev_dbg(pdev, "U41 Target T%u, slot:%u present:%u, x:%u, y:%u, z:%d\n",
+ target->index, slot, target->present,
+ target->x, target->y, target->z);
+
+ switch (current_state) {
+ default:
+ case TARGET_STATE_NOT_PRESENT:
+ case TARGET_STATE_PROX:
+ {
+ if (target_prev_state->insert) {
+ update = true;
+ target_prev_state->insert = false;
+ input_mt_slot(input_dev, slot);
+
+ if (slot == 0)
+ input_report_key(input_dev, BTN_LEFT,
+ 0);
+
+ input_mt_report_slot_inactive(input_dev);
+ /* make sure the previous coordinates are all off */
+ /* screen when the finger comes back */
+ target->x = target->y = 65535;
+ target->z = PROX_LEVEL;
+ }
+ break;
+ }
+ case TARGET_STATE_HOVER:
+ case TARGET_STATE_TOUCHING:
+ {
+ target_prev_state->insert = true;
+ update = true;
+ input_mt_slot(input_dev, slot);
+ input_report_abs(input_dev, ABS_MT_TRACKING_ID, slot);
+ input_report_abs(input_dev, ABS_MT_POSITION_X,
+ target->x);
+ input_report_abs(input_dev, ABS_X, target->x);
+ input_report_abs(input_dev, ABS_MT_POSITION_Y,
+ target->y);
+ input_report_abs(input_dev, ABS_Y, target->y);
+
+ if (current_state == TARGET_STATE_TOUCHING) {
+ input_report_abs(input_dev, ABS_MT_DISTANCE, 0);
+ input_report_abs(input_dev, ABS_DISTANCE, 0);
+ input_report_abs(input_dev, ABS_MT_PRESSURE,
+ target->z);
+ input_report_abs(input_dev, ABS_PRESSURE,
+ target->z);
+ } else {
+ input_report_abs(input_dev, ABS_MT_DISTANCE,
+ -target->z);
+ input_report_abs(input_dev, ABS_DISTANCE,
+ -target->z);
+ input_report_abs(input_dev, ABS_MT_PRESSURE, 0);
+ input_report_abs(input_dev, ABS_PRESSURE, 0);
+ }
+
+ if (slot == 0)
+ input_report_key(input_dev, BTN_LEFT,
+ (current_state ==
+ TARGET_STATE_TOUCHING));
+
+ break;
+ }
+ }
+
+ target_prev_state->state = current_state;
+ target_prev_state->x = target->x;
+ target_prev_state->y = target->y;
+ target_prev_state->z = target->z;
+
+ if (update)
+ input_mt_sync_frame(input_dev);
+
+ return update;
+}
+
+/**
+ * Take a raw buffer with u41 report data and decode it.
+ * Also generate input events if needed.
+ * @rx_buf: ptr to a byte array [0]: Usage number [1]: Status LSB [2]: Status MSB
+ */
+void axiom_process_u41_report(struct axiom_data *ts, char *rx_buf)
+{
+ struct axiom_target_report target;
+ struct input_dev *input_dev = ts->input_dev;
+ bool update_done = false;
+ u16 target_status;
+ u32 i;
+
+ if (rx_buf[0] != 0x41) {
+ dev_err(ts->pdev,
+ "Data in buffer does not have expected u41 format.\n");
+ return;
+ }
+
+ target_status = ((rx_buf[1]) | (rx_buf[2] << 8));
+
+ for (i = 0; i < U41_MAX_TARGETS; i++) {
+ target.index = i;
+ target.present = ((target_status & (1 << i)) != 0) ? 1 : 0;
+ target.x = (rx_buf[(i * 4) + 3]) | (rx_buf[(i * 4) + 4] << 8);
+ target.y = (rx_buf[(i * 4) + 5]) | (rx_buf[(i * 4) + 6] << 8);
+ target.z = (s8) (rx_buf[i + 43]);
+ update_done |= axiom_process_u41_report_target(ts, &target);
+ }
+
+ if (update_done)
+ input_sync(input_dev);
+}
+EXPORT_SYMBOL_GPL(axiom_process_u41_report);
+
+void axiom_process_u46_report(struct axiom_data *ts, char *rx_buf)
+{
+ struct input_dev *input_dev = ts->input_dev;
+ u16 aux_value;
+ u32 event_value;
+ u32 i = 0;
+
+ for (i = 0; i < U46_AUX_CHANNELS; i++) {
+ aux_value =
+ ((rx_buf[(i * 2) + 2] << 8) | (rx_buf[(i * 2) + 1])) &
+ U46_AUX_MASK;
+ event_value = (i << 16) | (aux_value);
+ input_event(input_dev, EV_MSC, MSC_RAW, event_value);
+ }
+
+ input_mt_sync(input_dev);
+ input_sync(input_dev);
+}
+EXPORT_SYMBOL_GPL(axiom_process_u46_report);
+
+/**
+ * Support function to axiom_process_report.
+ * It validates the crc and multiplexes the axiom reports to the appropriate
+ * report handler
+ */
+void axiom_process_report(struct axiom_data *ts, char *report_data)
+{
+ struct device *pdev = ts->pdev;
+ char usage = report_data[1];
+ u16 crc_report;
+ u16 crc_calc;
+ char len;
+
+ if ((report_data[0] & COMMS_OVERFLOW_MSK) != 0)
+ ts->report_overflow_counter++;
+
+ len = (report_data[0] & COMMS_REPORT_LEN_MSK) << 1;
+ if (len == 0) {
+ dev_err(pdev, "Zero length report discarded.\n");
+ return;
+ }
+
+ dev_dbg(pdev, "Payload Data %*ph\n", len, report_data);
+
+ /* Validate the report CRC */
+ crc_report = (report_data[len - 1] << 8) | (report_data[len - 2]);
+ /* Length is in 16 bit words and remove the size of the CRC16 itself */
+ crc_calc = crc16(0, report_data, (len - 2));
+
+ if (crc_calc != crc_report) {
+ dev_err(pdev,
+ "CRC mismatch! Expected: %#x, Calculated CRC: %#x.\n",
+ crc_report, crc_calc);
+ return;
+ }
+
+ switch (usage) {
+ case USAGE_2DCTS_REPORT_ID:
+ axiom_process_u41_report(ts, &report_data[1]);
+ break;
+
+ case USAGE_2AUX_REPORT_ID:
+ /* This is an aux report (force) */
+ axiom_process_u46_report(ts, &report_data[1]);
+ break;
+
+ case USAGE_2HB_REPORT_ID:
+ /* This is a heartbeat report */
+ break;
+
+ default:
+ break;
+ }
+
+ ts->report_counter++;
+}
+EXPORT_SYMBOL_GPL(axiom_process_report);
+
+MODULE_AUTHOR("Kamel Bouhara <kamel.bouhara@bootlin.com>");
+MODULE_DESCRIPTION("aXiom touchscreen core logic");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("axiom");
diff --git a/drivers/input/touchscreen/axiom_core.h b/drivers/input/touchscreen/axiom_core.h
new file mode 100644
index 000000000000..f129d28671ff
--- /dev/null
+++ b/drivers/input/touchscreen/axiom_core.h
@@ -0,0 +1,140 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * TouchNetix aXiom Touchscreen Driver
+ *
+ * Copyright (C) 2020-2023 TouchNetix Ltd.
+ *
+ * Author(s): Mark Satterthwaite <mark.satterthwaite@touchnetix.com>
+ * Pedro Torruella <pedro.torruella@touchnetix.com>
+ * Bart Prescott <bartp@baasheep.co.uk>
+ * Hannah Rossiter <hannah.rossiter@touchnetix.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ *
+ */
+
+#ifndef __AXIOM_CORE_H
+#define __AXIOM_CORE_H
+
+/* Enables the raw data for up to 4 force channels to be sent to the input subsystem */
+#define U46_ENABLE_RAW_FORCE_DATA
+
+/**
+ * u31 has 2 pages for usage table entries.
+ * (2 * AX_COMMS_PAGE_SIZE) / U31_BYTES_PER_USAGE = 85
+ */
+#define U31_MAX_USAGES (85U)
+#define U41_MAX_TARGETS (10U)
+#define U46_AUX_CHANNELS (4U)
+#define U46_AUX_MASK (0xFFFU)
+#define U31_BYTES_PER_USAGE (6U)
+#define USAGE_2DCTS_REPORT_ID (0x41U)
+#define USAGE_2AUX_REPORT_ID (0x46U)
+#define USAGE_2HB_REPORT_ID (0x01U)
+#define PROX_LEVEL (-128)
+#define AX_U31_PAGE0_LENGTH (0x0C)
+#define AX_COMMS_WRITE (0x00U)
+#define AX_COMMS_READ (0x80U)
+#define AX_COMMS_BYTES_MASK (0xFFU)
+
+#define DEVINFO_USAGE_ID 0x31
+#define REPORT_USAGE_ID 0x34
+
+#define REBASELINE_CMD 0x03
+
+#define COMMS_MAX_USAGE_PAGES (3)
+#define AX_COMMS_PAGE_SIZE (256)
+
+#define COMMS_OVERFLOW_MSK (0x80)
+#define COMMS_REPORT_LEN_MSK (0x7F)
+
+#include <linux/input.h>
+
+struct axiom_devinfo {
+ char bootloader_fw_ver_major;
+ char bootloader_fw_ver_minor;
+ char bootloader_mode;
+ u16 device_id;
+ char fw_major;
+ char fw_minor;
+ u16 fw_info_extra;
+ u16 jedec_id;
+ char num_usages;
+ char silicon_revision;
+};
+
+/**
+ * Describes parameters of a specific usage, essenstially a single element of
+ * the "Usage Table"
+ */
+struct usage_entry {
+ char id;
+ char is_report;
+ char start_page;
+ char num_pages;
+};
+
+/**
+ * Holds state of a "Target", A.K.A. as a "touch", but called a target as it
+ * can be a detected "target" prior to touch, eg, hovering.
+ */
+enum axiom_target_state {
+ TARGET_STATE_NOT_PRESENT = 0,
+ TARGET_STATE_PROX = 1,
+ TARGET_STATE_HOVER = 2,
+ TARGET_STATE_TOUCHING = 3,
+ TARGET_STATE_MIN = TARGET_STATE_NOT_PRESENT,
+ TARGET_STATE_MAX = TARGET_STATE_TOUCHING,
+};
+
+struct u41_target {
+ enum axiom_target_state state;
+ u16 x;
+ u16 y;
+ s8 z;
+ bool insert;
+ bool touch;
+};
+
+struct axiom_target_report {
+ u8 index;
+ u8 present;
+ u16 x;
+ u16 y;
+ s8 z;
+};
+
+struct axiom_cmd_header {
+ u16 target_address;
+ u16 length:15;
+ u16 read:1;
+ char write_data[];
+};
+
+struct axiom_data {
+ struct axiom_devinfo devinfo;
+ struct device *pdev;
+ struct gpio_desc *reset_gpio;
+ struct gpio_desc *irq_gpio;
+ struct i2c_client *client;
+ struct input_dev *input_dev;
+ char max_report_len;
+ u32 report_overflow_counter;
+ u32 report_counter;
+ char rx_buf[COMMS_MAX_USAGE_PAGES * AX_COMMS_PAGE_SIZE];
+ struct u41_target targets[U41_MAX_TARGETS];
+ struct usage_entry usage_table[U31_MAX_USAGES];
+ bool usage_table_populated;
+};
+
+extern u16 usage_to_target_address(struct axiom_data *ts, char usage,
+ char page, char offset);
+extern void axiom_process_report(struct axiom_data *ts, char *report_data);
+extern char axiom_populate_usage_table(struct axiom_data *ts, char *rx_data);
+extern void axiom_remove(struct axiom_data *ts);
+extern void axiom_get_dev_info(struct axiom_data *ts, char *info);
+
+#endif /* __AXIOM_CORE_H */
diff --git a/drivers/input/touchscreen/axiom_i2c.c b/drivers/input/touchscreen/axiom_i2c.c
new file mode 100644
index 000000000000..ddb898ad3744
--- /dev/null
+++ b/drivers/input/touchscreen/axiom_i2c.c
@@ -0,0 +1,349 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * TouchNetix aXiom Touchscreen Driver
+ *
+ * Copyright (C) 2020-2023 TouchNetix Ltd.
+ *
+ * Author(s): Bart Prescott <bartp@baasheep.co.uk>
+ * Pedro Torruella <pedro.torruella@touchnetix.com>
+ * Mark Satterthwaite <mark.satterthwaite@touchnetix.com>
+ * Hannah Rossiter <hannah.rossiter@touchnetix.com>
+ *
+ */
+
+#include "axiom_core.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/interrupt.h>
+#include <linux/irqreturn.h>
+#include <linux/kernel.h>
+#include <linux/kobject.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pm.h>
+#include <linux/string.h>
+
+/**
+ * aXiom devices are typically configured to report
+ * touches at a rate of 100Hz (10ms). For systems
+ * that require polling for reports, 100ms seems like
+ * an acceptable polling rate.
+ * When reports are polled, it will be expected to
+ * occasionally observe the overflow bit being set
+ * in the reports. This indicates that reports are not
+ * being read fast enough.
+ */
+#define POLL_INTERVAL_DEFAULT_MS 100
+
+static int
+axiom_i2c_read(struct i2c_client *client, u8 usage, u8 page, u8 *buf, u16 len)
+{
+ struct axiom_cmd_header cmd_header;
+ struct axiom_data *ts = i2c_get_clientdata(client);
+ struct i2c_msg msg[2];
+ int ret;
+
+ /* Build the header */
+ cmd_header.target_address = usage_to_target_address(ts, usage, page, 0);
+ cmd_header.length = len;
+ cmd_header.read = 1;
+
+ msg[0].addr = client->addr;
+ msg[0].flags = 0;
+ msg[0].len = sizeof(cmd_header);
+ msg[0].buf = (u8 *)&cmd_header;
+
+ msg[1].addr = client->addr;
+ msg[1].flags = I2C_M_RD;
+ msg[1].len = len;
+ msg[1].buf = (char *)buf;
+
+ ret = i2c_transfer(client->adapter, msg, 2);
+ if (ret != ARRAY_SIZE(msg)) {
+ dev_err(&client->dev,
+ "Failed reading usage %#x page %#x, error=%d\n", usage,
+ page, ret);
+ return -EIO;
+ }
+
+ return 0;
+}
+
+static int
+axiom_i2c_write(struct i2c_client *client, u8 usage, u8 page, u8 *buf, u16 len)
+{
+ struct axiom_cmd_header cmd_header;
+ struct axiom_data *ts = i2c_get_clientdata(client);
+ struct i2c_msg msg[2];
+ int ret;
+
+ cmd_header.target_address = usage_to_target_address(ts, usage, page, 0);
+ cmd_header.length = len;
+ cmd_header.read = 0;
+
+ msg[0].addr = client->addr;
+ msg[0].flags = 0;
+ msg[0].len = sizeof(cmd_header);
+ msg[0].buf = (u8 *)&cmd_header;
+
+ msg[1].addr = client->addr;
+ msg[1].flags = 0;
+ msg[1].len = len;
+ msg[1].buf = (char *)buf;
+
+ ret = i2c_transfer(client->adapter, msg, 2);
+ if (ret != 2) {
+ dev_err(&client->dev,
+ "Failed to write usage %#x page %#x, error=%d\n", usage,
+ page, ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static void axiom_i2c_poll(struct input_dev *input_dev)
+{
+ struct axiom_data *ts = input_get_drvdata(input_dev);
+ char *rx_data = ts->rx_buf;
+
+ axiom_i2c_read(ts->client, REPORT_USAGE_ID, 0, rx_data,
+ ts->max_report_len);
+ axiom_process_report(ts, rx_data);
+}
+
+/**
+ * Retrieve, store and print the axiom device information
+ */
+int axiom_discover(struct axiom_data *ts)
+{
+ char *rx_data = &ts->rx_buf[0];
+ struct device *pdev = ts->pdev;
+ int ret;
+
+ /* First the first page of u31 to get the device information and */
+ /* the number of usages */
+ ret =
+ axiom_i2c_read(ts->client, DEVINFO_USAGE_ID, 0, rx_data,
+ AX_U31_PAGE0_LENGTH);
+ if (ret)
+ return ret;
+
+ axiom_get_dev_info(ts, rx_data);
+
+ dev_dbg(pdev, "Data Decode:\n");
+ dev_dbg(pdev, " Bootloader Mode: %u\n", ts->devinfo.bootloader_mode);
+ dev_dbg(pdev, " Device ID : %04x\n", ts->devinfo.device_id);
+ dev_dbg(pdev, " Firmware Rev : %02x.%02x\n", ts->devinfo.fw_major,
+ ts->devinfo.fw_minor);
+ dev_dbg(pdev, " Bootloader Rev : %02x.%02x\n",
+ ts->devinfo.bootloader_fw_ver_major,
+ ts->devinfo.bootloader_fw_ver_minor);
+ dev_dbg(pdev, " FW Extra Info : %04x\n", ts->devinfo.fw_info_extra);
+ dev_dbg(pdev, " Silicon : %02x\n", ts->devinfo.jedec_id);
+ dev_dbg(pdev, " Num Usages : %04x\n", ts->devinfo.num_usages);
+
+ /* Read the second page of u31 to get the usage table */
+ ret = axiom_i2c_read(ts->client, DEVINFO_USAGE_ID, 1, rx_data,
+ (U31_BYTES_PER_USAGE * ts->devinfo.num_usages));
+ if (ret)
+ return ret;
+
+ ts->max_report_len = axiom_populate_usage_table(ts, rx_data);
+ dev_dbg(pdev, "Max Report Length: %u\n", ts->max_report_len);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(axiom_discover);
+
+/**
+ * Rebaseline the touchscreen, effectively zero-ing it
+ */
+void axiom_rebaseline(struct axiom_data *ts)
+{
+ struct device *pdev = ts->pdev;
+ char buffer[8] = { 0 };
+ int ret;
+
+ memset(buffer, 0, sizeof(buffer));
+
+ buffer[0] = REBASELINE_CMD;
+
+ ret = axiom_i2c_write(ts->client, 0x02, 0, buffer, sizeof(buffer));
+ if (ret)
+ dev_err(pdev, "Rebaseline failed\n");
+
+ dev_info(pdev, "Capture Baseline Requested\n");
+}
+EXPORT_SYMBOL_GPL(axiom_rebaseline);
+
+static irqreturn_t axiom_irq(int irq, void *handle)
+{
+ struct axiom_data *ts = handle;
+ u8 *rx_data = &ts->rx_buf[0];
+
+ axiom_i2c_read(ts->client, REPORT_USAGE_ID, 0, rx_data, ts->max_report_len);
+ axiom_process_report(ts, rx_data);
+
+ return IRQ_HANDLED;
+}
+
+static int axiom_i2c_probe(struct i2c_client *client)
+{
+ struct axiom_data *ts;
+ struct device *pdev = &client->dev;
+ struct input_dev *input_dev;
+ int error;
+ int target;
+
+ ts = devm_kzalloc(pdev, sizeof(*ts), GFP_KERNEL);
+ if (!ts)
+ return -ENOMEM;
+
+ ts->irq_gpio = devm_gpiod_get_optional(pdev, "irq", GPIOD_IN);
+ if (IS_ERR(ts->irq_gpio)) {
+ error = PTR_ERR(ts->irq_gpio);
+ dev_err(pdev, "failed to request irq GPIO: %d", error);
+ return error;
+ }
+
+ ts->reset_gpio = devm_gpiod_get_optional(pdev, "reset", GPIOD_OUT_HIGH);
+ if (IS_ERR(ts->reset_gpio)) {
+ error = PTR_ERR(ts->reset_gpio);
+ dev_err(pdev, "failed to request reset GPIO: %d", error);
+ return error;
+ }
+
+ if (ts->irq_gpio) {
+ error =
+ devm_request_threaded_irq(pdev, client->irq, NULL,
+ axiom_irq,
+ IRQF_TRIGGER_LOW | IRQF_ONESHOT,
+ "axiom_irq", ts);
+ if (error != 0) {
+ dev_err(pdev, "Failed to request IRQ %u (error: %d)\n",
+ client->irq, error);
+ return error;
+ }
+ }
+
+ ts->client = client;
+ ts->pdev = pdev;
+ ts->usage_table_populated = false;
+
+ i2c_set_clientdata(client, ts);
+
+ axiom_discover(ts);
+ axiom_rebaseline(ts);
+
+ input_dev = devm_input_allocate_device(ts->pdev);
+ if (!input_dev) {
+ dev_err(pdev, "Failed to allocate input device\n");
+ return -ENOMEM;
+ }
+
+ input_dev->name = "TouchNetix aXiom Touchscreen";
+ input_dev->phys = "input/axiom_ts";
+
+ /* Single Touch */
+ input_set_abs_params(input_dev, ABS_X, 0, 65535, 0, 0);
+ input_set_abs_params(input_dev, ABS_Y, 0, 65535, 0, 0);
+
+ /* Multi Touch */
+ /* Min, Max, Fuzz (expected noise in px, try 4?) and Flat */
+ input_set_abs_params(input_dev, ABS_MT_POSITION_X, 0, 65535, 0, 0);
+ /* Min, Max, Fuzz (expected noise in px, try 4?) and Flat */
+ input_set_abs_params(input_dev, ABS_MT_POSITION_Y, 0, 65535, 0, 0);
+ input_set_abs_params(input_dev, ABS_MT_TOOL_TYPE, 0, MT_TOOL_MAX, 0, 0);
+ input_set_abs_params(input_dev, ABS_MT_DISTANCE, 0, 127, 0, 0);
+ input_set_abs_params(input_dev, ABS_MT_PRESSURE, 0, 127, 0, 0);
+
+ /* Registers the axiom device as a touch screen instead of as a mouse pointer */
+ input_mt_init_slots(input_dev, U41_MAX_TARGETS, INPUT_MT_DIRECT);
+
+ input_set_capability(input_dev, EV_KEY, BTN_LEFT);
+
+ /* Enables the raw data for up to 4 force channels to be sent to the */
+ /* input subsystem */
+ set_bit(EV_REL, input_dev->evbit);
+ set_bit(EV_MSC, input_dev->evbit);
+ /* Declare that we support "RAW" Miscellaneous events */
+ set_bit(MSC_RAW, input_dev->mscbit);
+
+ if (!ts->irq_gpio) {
+ error = input_setup_polling(input_dev, axiom_i2c_poll);
+ if (error) {
+ dev_err(ts->pdev, "Unable to set up polling: %d\n",
+ error);
+ return error;
+ }
+ input_set_poll_interval(input_dev, POLL_INTERVAL_DEFAULT_MS);
+ }
+
+ error = input_register_device(input_dev);
+ if (error != 0) {
+ dev_err(ts->pdev,
+ "Could not register with Input Sub-system., error=%d\n",
+ error);
+ return error;
+ }
+
+ ts->input_dev = input_dev;
+ input_set_drvdata(ts->input_dev, ts);
+
+ dev_info(pdev, "AXIOM: I2C driver registered with Input Sub-System.\n");
+
+ /* Delay just a smidge before enabling the IRQ */
+ udelay(10);
+
+ /* Ensure that all reports are initialised to not be present. */
+ for (target = 0; target < U41_MAX_TARGETS; target++)
+ ts->targets[target].state = TARGET_STATE_NOT_PRESENT;
+
+ return 0;
+}
+
+static void axiom_i2c_remove(struct i2c_client *client)
+{
+ struct axiom_data *ts = i2c_get_clientdata(client);
+
+ axiom_remove(ts);
+}
+
+static const struct i2c_device_id axiom_i2c_id_table[] = {
+ {"ax54a"},
+ {},
+};
+
+MODULE_DEVICE_TABLE(i2c, axiom_i2c_id_table);
+
+static const struct of_device_id axiom_i2c_dt_ids[] = {
+ {
+ .compatible = "axiom,ax54a",
+ .data = "axiom",
+ },
+ {}
+};
+
+MODULE_DEVICE_TABLE(of, axiom_i2c_dt_ids);
+
+static struct i2c_driver axiom_i2c_driver = {
+ .driver = {
+ .name = "axiom_i2c",
+ .of_match_table = of_match_ptr(axiom_i2c_dt_ids),
+ },
+ .id_table = axiom_i2c_id_table,
+ .probe = axiom_i2c_probe,
+ .remove = axiom_i2c_remove,
+};
+
+module_i2c_driver(axiom_i2c_driver);
+
+MODULE_AUTHOR("Kamel Bouhara <kamel.bouhara@bootlin.com>");
+MODULE_DESCRIPTION("aXiom touchscreen I2C bus driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("axiom");
--
2.25.1
^ permalink raw reply related
* [PATCH v2 0/3] selftests/hid: fix building for older kernels
From: Justin Stitt @ 2023-09-08 22:22 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Shuah Khan
Cc: Eduard Zingerman, linux-input, linux-kselftest, linux-kernel, bpf,
Benjamin Tissoires, Justin Stitt
Hi, I am sending this series on behalf of myself and Benjamin Tissoires. There
existed an initial n=3 patch series which was later expanded to n=4 and
is now back to n=3 with some fixes added in and rebased against
mainline.
This patch series aims to ensure that the hid/bpf selftests can be built
without errors.
Here's Benjamin's initial cover letter for context:
| These fixes have been triggered by [0]:
| basically, if you do not recompile the kernel first, and are
| running on an old kernel, vmlinux.h doesn't have the required
| symbols and the compilation fails.
|
| The tests will fail if you run them on that very same machine,
| of course, but the binary should compile.
|
| And while I was sorting out why it was failing, I realized I
| could do a couple of improvements on the Makefile.
|
| [0] https://lore.kernel.org/linux-input/56ba8125-2c6f-a9c9-d498-0ca1c153dcb2@redhat.com/T/#t
Changes from v1 -> v2:
- roll Justin's fix into patch 1/3
- add __attribute__((preserve_access_index)) (thanks Eduard)
- rebased onto mainline (2dde18cd1d8fac735875f2e4987f11817cc0bc2c)
- Link to v1: https://lore.kernel.org/all/20230825-wip-selftests-v1-0-c862769020a8@kernel.org/
Link: https://github.com/ClangBuiltLinux/linux/issues/1698
Link: https://github.com/ClangBuiltLinux/continuous-integration2/issues/61
---
Benjamin Tissoires (3):
selftests/hid: ensure we can compile the tests on kernels pre-6.3
selftests/hid: do not manually call headers_install
selftests/hid: force using our compiled libbpf headers
tools/testing/selftests/hid/Makefile | 10 ++---
tools/testing/selftests/hid/progs/hid.c | 3 --
.../testing/selftests/hid/progs/hid_bpf_helpers.h | 49 ++++++++++++++++++++++
3 files changed, 53 insertions(+), 9 deletions(-)
---
base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
change-id: 20230908-kselftest-09-08-56d7f4a8d5c4
Best regards,
--
Justin Stitt <justinstitt@google.com>
^ permalink raw reply
* [PATCH v2 1/3] selftests/hid: ensure we can compile the tests on kernels pre-6.3
From: Justin Stitt @ 2023-09-08 22:22 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Shuah Khan
Cc: Eduard Zingerman, linux-input, linux-kselftest, linux-kernel, bpf,
Benjamin Tissoires, Justin Stitt
In-Reply-To: <20230908-kselftest-09-08-v2-0-0def978a4c1b@google.com>
From: Benjamin Tissoires <bentiss@kernel.org>
For the hid-bpf tests to compile, we need to have the definition of
struct hid_bpf_ctx. This definition is an internal one from the kernel
and it is supposed to be defined in the generated vmlinux.h.
This vmlinux.h header is generated based on the currently running kernel
or if the kernel was already compiled in the tree. If you just compile
the selftests without compiling the kernel beforehand and you are running
on a 6.2 kernel, you'll end up with a vmlinux.h without the hid_bpf_ctx
definition.
Use the clever trick from tools/testing/selftests/bpf/progs/bpf_iter.h
to force the definition of that symbol in case we don't find it in the
BTF and also add __attribute__((preserve_access_index)) to further
support CO-RE functionality for these tests.
Signed-off-by: Justin Stitt <justinstitt@google.com>
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
tools/testing/selftests/hid/progs/hid.c | 3 --
.../testing/selftests/hid/progs/hid_bpf_helpers.h | 49 ++++++++++++++++++++++
2 files changed, 49 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/hid/progs/hid.c b/tools/testing/selftests/hid/progs/hid.c
index 88c593f753b5..1e558826b809 100644
--- a/tools/testing/selftests/hid/progs/hid.c
+++ b/tools/testing/selftests/hid/progs/hid.c
@@ -1,8 +1,5 @@
// SPDX-License-Identifier: GPL-2.0
/* Copyright (c) 2022 Red hat */
-#include "vmlinux.h"
-#include <bpf/bpf_helpers.h>
-#include <bpf/bpf_tracing.h>
#include "hid_bpf_helpers.h"
char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
index 4fff31dbe0e7..ab3b18ba48c4 100644
--- a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
+++ b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
@@ -5,6 +5,55 @@
#ifndef __HID_BPF_HELPERS_H
#define __HID_BPF_HELPERS_H
+/* "undefine" structs in vmlinux.h, because we "override" them below */
+#define hid_bpf_ctx hid_bpf_ctx___not_used
+#define hid_report_type hid_report_type___not_used
+#define hid_class_request hid_class_request___not_used
+#define hid_bpf_attach_flags hid_bpf_attach_flags___not_used
+#include "vmlinux.h"
+#undef hid_bpf_ctx
+#undef hid_report_type
+#undef hid_class_request
+#undef hid_bpf_attach_flags
+
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include <linux/const.h>
+
+enum hid_report_type {
+ HID_INPUT_REPORT = 0,
+ HID_OUTPUT_REPORT = 1,
+ HID_FEATURE_REPORT = 2,
+
+ HID_REPORT_TYPES,
+};
+
+struct hid_bpf_ctx {
+ __u32 index;
+ const struct hid_device *hid;
+ __u32 allocated_size;
+ enum hid_report_type report_type;
+ union {
+ __s32 retval;
+ __s32 size;
+ };
+} __attribute__((preserve_access_index));
+
+enum hid_class_request {
+ HID_REQ_GET_REPORT = 0x01,
+ HID_REQ_GET_IDLE = 0x02,
+ HID_REQ_GET_PROTOCOL = 0x03,
+ HID_REQ_SET_REPORT = 0x09,
+ HID_REQ_SET_IDLE = 0x0A,
+ HID_REQ_SET_PROTOCOL = 0x0B,
+};
+
+enum hid_bpf_attach_flags {
+ HID_BPF_FLAG_NONE = 0,
+ HID_BPF_FLAG_INSERT_HEAD = _BITUL(0),
+ HID_BPF_FLAG_MAX,
+};
+
/* following are kfuncs exported by HID for HID-BPF */
extern __u8 *hid_bpf_get_data(struct hid_bpf_ctx *ctx,
unsigned int offset,
--
2.42.0.283.g2d96d420d3-goog
^ permalink raw reply related
* [PATCH v2 2/3] selftests/hid: do not manually call headers_install
From: Justin Stitt @ 2023-09-08 22:22 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Shuah Khan
Cc: Eduard Zingerman, linux-input, linux-kselftest, linux-kernel, bpf,
Benjamin Tissoires
In-Reply-To: <20230908-kselftest-09-08-v2-0-0def978a4c1b@google.com>
From: Benjamin Tissoires <bentiss@kernel.org>
"make headers" is a requirement before calling make on the selftests
dir, so we should not have to manually install those headers
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
tools/testing/selftests/hid/Makefile | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/hid/Makefile b/tools/testing/selftests/hid/Makefile
index 01c0491d64da..c5522088ece4 100644
--- a/tools/testing/selftests/hid/Makefile
+++ b/tools/testing/selftests/hid/Makefile
@@ -21,7 +21,7 @@ CXX ?= $(CROSS_COMPILE)g++
HOSTPKG_CONFIG := pkg-config
-CFLAGS += -g -O0 -rdynamic -Wall -Werror -I$(KHDR_INCLUDES) -I$(OUTPUT)
+CFLAGS += -g -O0 -rdynamic -Wall -Werror -I$(OUTPUT)
LDLIBS += -lelf -lz -lrt -lpthread
# Silence some warnings when compiled with clang
@@ -65,7 +65,6 @@ BPFTOOLDIR := $(TOOLSDIR)/bpf/bpftool
SCRATCH_DIR := $(OUTPUT)/tools
BUILD_DIR := $(SCRATCH_DIR)/build
INCLUDE_DIR := $(SCRATCH_DIR)/include
-KHDR_INCLUDES := $(SCRATCH_DIR)/uapi/include
BPFOBJ := $(BUILD_DIR)/libbpf/libbpf.a
ifneq ($(CROSS_COMPILE),)
HOST_BUILD_DIR := $(BUILD_DIR)/host
@@ -151,9 +150,6 @@ else
$(Q)cp "$(VMLINUX_H)" $@
endif
-$(KHDR_INCLUDES)/linux/hid.h: $(top_srcdir)/include/uapi/linux/hid.h
- $(MAKE) -C $(top_srcdir) INSTALL_HDR_PATH=$(SCRATCH_DIR)/uapi headers_install
-
$(RESOLVE_BTFIDS): $(HOST_BPFOBJ) | $(HOST_BUILD_DIR)/resolve_btfids \
$(TOOLSDIR)/bpf/resolve_btfids/main.c \
$(TOOLSDIR)/lib/rbtree.c \
@@ -231,7 +227,7 @@ $(BPF_SKELS): %.skel.h: %.bpf.o $(BPFTOOL) | $(OUTPUT)
$(Q)$(BPFTOOL) gen object $(<:.o=.linked1.o) $<
$(Q)$(BPFTOOL) gen skeleton $(<:.o=.linked1.o) name $(notdir $(<:.bpf.o=)) > $@
-$(OUTPUT)/%.o: %.c $(BPF_SKELS) $(KHDR_INCLUDES)/linux/hid.h
+$(OUTPUT)/%.o: %.c $(BPF_SKELS)
$(call msg,CC,,$@)
$(Q)$(CC) $(CFLAGS) -c $(filter %.c,$^) $(LDLIBS) -o $@
--
2.42.0.283.g2d96d420d3-goog
^ permalink raw reply related
* [PATCH v2 3/3] selftests/hid: force using our compiled libbpf headers
From: Justin Stitt @ 2023-09-08 22:22 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Shuah Khan
Cc: Eduard Zingerman, linux-input, linux-kselftest, linux-kernel, bpf,
Benjamin Tissoires
In-Reply-To: <20230908-kselftest-09-08-v2-0-0def978a4c1b@google.com>
From: Benjamin Tissoires <bentiss@kernel.org>
Turns out that we were relying on the globally installed headers, not
the ones we freshly compiled.
Add a manual include in CFLAGS to sort this out.
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
tools/testing/selftests/hid/Makefile | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tools/testing/selftests/hid/Makefile b/tools/testing/selftests/hid/Makefile
index c5522088ece4..b01c14077c5d 100644
--- a/tools/testing/selftests/hid/Makefile
+++ b/tools/testing/selftests/hid/Makefile
@@ -22,6 +22,8 @@ CXX ?= $(CROSS_COMPILE)g++
HOSTPKG_CONFIG := pkg-config
CFLAGS += -g -O0 -rdynamic -Wall -Werror -I$(OUTPUT)
+CFLAGS += -I$(OUTPUT)/tools/include
+
LDLIBS += -lelf -lz -lrt -lpthread
# Silence some warnings when compiled with clang
--
2.42.0.283.g2d96d420d3-goog
^ permalink raw reply related
* Re: [PATCH v2 0/3] selftests/hid: fix building for older kernels
From: Nick Desaulniers @ 2023-09-08 23:02 UTC (permalink / raw)
To: Justin Stitt
Cc: Jiri Kosina, Benjamin Tissoires, Shuah Khan, Eduard Zingerman,
linux-input, linux-kselftest, linux-kernel, bpf,
Benjamin Tissoires, llvm
In-Reply-To: <20230908-kselftest-09-08-v2-0-0def978a4c1b@google.com>
On Fri, Sep 08, 2023 at 10:22:37PM +0000, Justin Stitt wrote:
> Hi, I am sending this series on behalf of myself and Benjamin Tissoires. There
> existed an initial n=3 patch series which was later expanded to n=4 and
> is now back to n=3 with some fixes added in and rebased against
> mainline.
>
> This patch series aims to ensure that the hid/bpf selftests can be built
> without errors.
>
> Here's Benjamin's initial cover letter for context:
> | These fixes have been triggered by [0]:
> | basically, if you do not recompile the kernel first, and are
> | running on an old kernel, vmlinux.h doesn't have the required
> | symbols and the compilation fails.
> |
> | The tests will fail if you run them on that very same machine,
> | of course, but the binary should compile.
> |
> | And while I was sorting out why it was failing, I realized I
> | could do a couple of improvements on the Makefile.
> |
> | [0] https://lore.kernel.org/linux-input/56ba8125-2c6f-a9c9-d498-0ca1c153dcb2@redhat.com/T/#t
>
> Changes from v1 -> v2:
> - roll Justin's fix into patch 1/3
> - add __attribute__((preserve_access_index)) (thanks Eduard)
> - rebased onto mainline (2dde18cd1d8fac735875f2e4987f11817cc0bc2c)
> - Link to v1: https://lore.kernel.org/all/20230825-wip-selftests-v1-0-c862769020a8@kernel.org/
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/1698
> Link: https://github.com/ClangBuiltLinux/continuous-integration2/issues/61
Thanks to you and Benjamin for sorting all of this out! With this series
applied, I was able to build the hid selftests now without the previous
-Wvisibility diagnostics failing the build.
Tested-by: Nick Desaulniers <ndesaulniers@google.com> # Build
> ---
> Benjamin Tissoires (3):
> selftests/hid: ensure we can compile the tests on kernels pre-6.3
> selftests/hid: do not manually call headers_install
> selftests/hid: force using our compiled libbpf headers
>
> tools/testing/selftests/hid/Makefile | 10 ++---
> tools/testing/selftests/hid/progs/hid.c | 3 --
> .../testing/selftests/hid/progs/hid_bpf_helpers.h | 49 ++++++++++++++++++++++
> 3 files changed, 53 insertions(+), 9 deletions(-)
> ---
> base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
> change-id: 20230908-kselftest-09-08-56d7f4a8d5c4
>
> Best regards,
> --
> Justin Stitt <justinstitt@google.com>
>
^ permalink raw reply
* Re: [PATCH v2 RESEND] HID: nintendo: reinitialize USB Pro Controller after resuming from suspend
From: Martino Fontana @ 2023-09-10 10:33 UTC (permalink / raw)
To: Daniel Ogorchock; +Cc: jikos, benjamin.tissoires, linux-input, linux-kernel
In-Reply-To: <CAEVj2tk3uUa625YVO+2Mv80FbNT6gE=16_GTxuLUB1ncT_jV2A@mail.gmail.com>
On Fri, 8 Sept 2023 at 03:11, Daniel Ogorchock <djogorchock@gmail.com> wrote:
>
> Have you tested how this behaves for bluetooth controllers? Does the
> pm resume hook always result in error logs for bluetooth controllers,
> or has the kernel already removed the device before resume?
Tested on a Bluetooth connection, it behaves like it did before: the
controller disconnects as the computer sleeps, and you can have
to reconnect it by pressing a button on the controller.
I also don't see any log from that wasn't there before on journalctl.
> > + mutex_unlock(&ctlr->output_mutex);
> > + return 0;
>
> If desired, you could remove the above two lines and allow flow to
> continue through err_mutex even in the success case.
>
> > +
> > +err_mutex:
> > + mutex_unlock(&ctlr->output_mutex);
> > + return ret;
> > +}
> > +
Do I make a patch v3 for that, or is that not necessary?
(Asking because this is my first Linux kernel patch)
> > static const struct hid_device_id nintendo_hid_devices[] = {
> > { HID_USB_DEVICE(USB_VENDOR_ID_NINTENDO,
> > USB_DEVICE_ID_NINTENDO_PROCON) },
> > @@ -2404,6 +2434,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
>
> Something else we should do is add a suspend hook to power off the
> bluetooth-connected controllers. As of now, they remain powered on
> during suspend.
No, they power off when you suspend the computer. If you press a button,
the controller will attempt to pair, just like if you disconnected it manually.
As I said before, Bluetooth behavior isn't changed.
Regards,
Martino
^ permalink raw reply
* Re: [PATCH v4] HID: nintendo: cleanup LED code
From: Martino Fontana @ 2023-09-10 10:34 UTC (permalink / raw)
To: Daniel Ogorchock; +Cc: jikos, benjamin.tissoires, linux-input, linux-kernel
In-Reply-To: <CAEVj2tnNm-Ucq0R7D5-eA0yTpD6h+TUpTFUwJttnoFpDDSPaZQ@mail.gmail.com>
On Fri, 8 Sept 2023 at 03:22, Daniel Ogorchock <djogorchock@gmail.com> wrote:
> Regarding the player LED setting though, I'm pretty sure the driver's
> existing behavior matches that of an actual Switch console (at least
> for the first 4 players).
>
> 1 LED lights up for P1, 2 for P2, etc.
> See the nintendo documentation here:
> https://www.nintendo.com/my/support/qa/detail/33822
Whoops, I assumed that it behaves like it does on Wii and Wii U.
Sending new patch.
Regards,
Martino.
^ permalink raw reply
* [PATCH v5] HID: nintendo: cleanup LED code
From: Martino Fontana @ 2023-09-10 10:41 UTC (permalink / raw)
To: djogorchock, jikos, benjamin.tissoires, linux-input, linux-kernel
Cc: Martino Fontana
- 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.
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.
Signed-off-by: Martino Fontana <tinozzo123@gmail.com>
---
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.41.0
^ permalink raw reply related
* Re: [PATCH] Input: add driver for TouchNetix aXiom touchscreen
From: Krzysztof Kozlowski @ 2023-09-11 6:59 UTC (permalink / raw)
To: Kamel Bouhara, linux-input
Cc: mark.satterthwaite, pedro.torruella, bartp, hannah.rossiter,
Thomas Petazzoni, Gregory Clement, bsp-development.geo
In-Reply-To: <20230908153203.122062-2-kamel.bouhara@bootlin.com>
On 08/09/2023 17:32, Kamel Bouhara wrote:
> Add a new driver for the TouchNetix's aXiom family of
> multi-touch controller. This driver only support i2c
> and can be later adapted for SPI and USB support.
>
> Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> ---
> .../devicetree/bindings/vendor-prefixes.yaml | 2 +
Thank you for your patch. There is something to discuss/improve.
1. Bindings are separate patches.
I am surprised that you did not send this through your collegagues at
Bootlin for some internal review. It would spot such easy things to fix.
2. Please use subject prefixes matching the subsystem. You can get them
for example with `git log --oneline -- DIRECTORY_OR_FILE` on the
directory your patch is touching.
3. Please run scripts/checkpatch.pl and fix reported warnings. Some
warnings can be ignored, but the code here looks like it needs a fix.
Feel free to get in touch if the warning is not clear.
4. Please use scripts/get_maintainers.pl to get a list of necessary
people and lists to CC. It might happen, that command when run on an
older kernel, gives you outdated entries. Therefore please be sure you
base your patches on recent Linux kernel.
You missed at least devicetree list (maybe more), so this won't be
tested by automated tooling. Performing review on untested code might be
a waste of time, thus I will skip this patch entirely till you follow
the process allowing the patch to be tested.
Please kindly resend and include all necessary To/Cc entries.
Limited review follows.
> MAINTAINERS | 7 +
> drivers/input/touchscreen/Kconfig | 11 +
> drivers/input/touchscreen/Makefile | 1 +
> drivers/input/touchscreen/axiom_core.c | 382 ++++++++++++++++++
> drivers/input/touchscreen/axiom_core.h | 140 +++++++
> drivers/input/touchscreen/axiom_i2c.c | 349 ++++++++++++++++
> 7 files changed, 892 insertions(+)
> create mode 100644 drivers/input/touchscreen/axiom_core.c
> create mode 100644 drivers/input/touchscreen/axiom_core.h
> create mode 100644 drivers/input/touchscreen/axiom_i2c.c
>
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> index 573578db9509..b0a3ed451f15 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> @@ -175,6 +175,8 @@ patternProperties:
> "^awinic,.*":
> description: Shanghai Awinic Technology Co., Ltd.
> "^axentia,.*":
> + description: TouchNetix
> + "^axiom,.*":
> description: Axentia Technologies AB
> "^axis,.*":
> description: Axis Communications AB
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 389fe9e38884..43add48257d8 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3373,6 +3373,13 @@ W: https://ez.analog.com/linux-software-drivers
> F: Documentation/devicetree/bindings/hwmon/adi,axi-fan-control.yaml
> F: drivers/hwmon/axi-fan-control.c
>
> +AXIOM I2C TOUCHSCREEN DRIVER
> +M: Kamel Bouhara <kamel.bouhara@bootlin.com>
> +L: linux-input@vger.kernel.org
> +S: Maintained
> +F: drivers/input/touchscreen/axiom_core.c
> +F: drivers/input/touchscreen/axiom_i2.c
> +
> AXXIA I2C CONTROLLER
> M: Krzysztof Adamski <krzysztof.adamski@nokia.com>
> L: linux-i2c@vger.kernel.org
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index e3e2324547b9..08a770a0c5e5 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -150,6 +150,17 @@ config TOUCHSCREEN_AUO_PIXCIR
> To compile this driver as a module, choose M here: the
> module will be called auo-pixcir-ts.
>
> +config TOUCHSCREEN_AXIOM_I2C
> + tristate "AXIOM based multi-touch panel controllers"
> + help
> + Say Y here if you have a axiom touchscreen connected to
> + your system.
> +
> + If unsure, say N.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called axiom_i2c.
> +
> config TOUCHSCREEN_BU21013
> tristate "BU21013 based touch panel controllers"
> depends on I2C
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index 62bd24f3ac8e..59a3234ddb09 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -18,6 +18,7 @@ obj-$(CONFIG_TOUCHSCREEN_ADS7846) += ads7846.o
> obj-$(CONFIG_TOUCHSCREEN_AR1021_I2C) += ar1021_i2c.o
> obj-$(CONFIG_TOUCHSCREEN_ATMEL_MXT) += atmel_mxt_ts.o
> obj-$(CONFIG_TOUCHSCREEN_AUO_PIXCIR) += auo-pixcir-ts.o
> +obj-$(CONFIG_TOUCHSCREEN_AXIOM_I2C) += axiom_core.o axiom_i2c.o
> obj-$(CONFIG_TOUCHSCREEN_BU21013) += bu21013_ts.o
> obj-$(CONFIG_TOUCHSCREEN_BU21029) += bu21029_ts.o
> obj-$(CONFIG_TOUCHSCREEN_CHIPONE_ICN8318) += chipone_icn8318.o
> diff --git a/drivers/input/touchscreen/axiom_core.c b/drivers/input/touchscreen/axiom_core.c
> new file mode 100644
> index 000000000000..d381afd7fb84
> --- /dev/null
> +++ b/drivers/input/touchscreen/axiom_core.c
> @@ -0,0 +1,382 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * TouchNetix aXiom Touchscreen Driver
> + *
> + * Copyright (C) 2020-2023 TouchNetix Ltd.
> + *
> + * Author(s): Mark Satterthwaite <mark.satterthwaite@touchnetix.com>
> + * Pedro Torruella <pedro.torruella@touchnetix.com>
> + * Bart Prescott <bartp@baasheep.co.uk>
> + * Hannah Rossiter <hannah.rossiter@touchnetix.com>
> + *
> + */
> +
> +#include <linux/crc16.h>
> +#include <linux/device.h>
> +#include <linux/input.h>
> +#include <linux/input/mt.h>
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +
> +#include "axiom_core.h"
> +
> +/**
> + * Decodes and populates the local u31 structure.
> + * Given a buffer of data read from page 0 of u31 in an aXiom device.
> + */
> +void axiom_get_dev_info(struct axiom_data *ts, char *info)
> +{
> + struct axiom_devinfo *devinfo = &ts->devinfo;
> +
> + if (devinfo) {
> + devinfo->bootloader_mode = ((info[1] & 0x80) != 0) ? 1 : 0;
> + devinfo->device_id = ((info[1] & 0x7F) << 8) | info[0];
> + devinfo->fw_minor = info[2];
> + devinfo->fw_major = info[3];
> + devinfo->fw_info_extra = (info[4]) | (info[5] << 8);
> + devinfo->bootloader_fw_ver_minor = info[6];
> + devinfo->bootloader_fw_ver_major = info[7];
> + devinfo->jedec_id = (info[8]) | (info[9] << 8);
> + devinfo->num_usages = info[10];
> + devinfo->silicon_revision = info[11];
> + }
> +}
> +EXPORT_SYMBOL_GPL(axiom_get_dev_info);
Nope
> +
> +/**
> + * Decodes and populates the local Usage Table.
> + * Given a buffer of data read from page 1 onwards of u31 from an aXiom
> + * device.
> + */
> +char axiom_populate_usage_table(struct axiom_data *ts, char *rx_data)
> +{
> + u32 usage_id = 0;
> + char max_report_len = 0;
> + struct axiom_devinfo *device_info;
> + struct usage_entry *usage_table;
> +
> + device_info = &ts->devinfo;
> + usage_table = ts->usage_table;
> +
> + for (usage_id = 0; usage_id < device_info->num_usages; usage_id++) {
> + u16 offset = (usage_id * U31_BYTES_PER_USAGE);
> + char id = rx_data[offset + 0];
> + char start_page = rx_data[offset + 1];
> + char num_pages = rx_data[offset + 2];
> + char max_offset = ((rx_data[offset + 3] & 0x7F) + 1) * 2;
> +
> + /* Store the entry into the usage table */
> + usage_table[usage_id].id = id;
> + usage_table[usage_id].is_report = ((num_pages == 0) ? 1 : 0);
> + usage_table[usage_id].start_page = start_page;
> + usage_table[usage_id].num_pages = num_pages;
> +
> + dev_dbg(ts->pdev, "Usage %2u Info: %*ph\n", usage_id,
> + U31_BYTES_PER_USAGE, &rx_data[offset]);
> +
> + /* Identify the max report length the module will receive */
> + if ((usage_table[usage_id].is_report)
> + && (max_offset > max_report_len))
> + max_report_len = max_offset;
> + }
> + ts->usage_table_populated = true;
> +
> + return max_report_len;
> +}
> +EXPORT_SYMBOL_GPL(axiom_populate_usage_table);
> +
> +/**
> + * Translate usage/page/offset triplet into physical address.
> + *
> + * @usage: groups of registers
> + * @page: page to which the usage belongs to offset
> + * @offset of the usage
> + */
> +u16
> +usage_to_target_address(struct axiom_data *ts, char usage, char page,
> + char offset)
> +{
> + struct axiom_devinfo *device_info;
> + struct usage_entry *usage_table;
> + u16 target_address;
> + u32 i;
> +
> + device_info = &ts->devinfo;
> + usage_table = ts->usage_table;
> +
> + /* At the moment the convention is that u31 is always at physical address 0x0 */
> + if (!ts->usage_table_populated && (usage == DEVINFO_USAGE_ID)) {
> + target_address = ((page << 8) + offset);
> + } else if (ts->usage_table_populated) {
> + for (i = 0; i < device_info->num_usages; i++) {
> + if (usage_table[i].id == usage) {
> + if (page < usage_table[i].num_pages) {
> + target_address =
> + ((usage_table[i].start_page + page) << 8) + offset;
> + } else {
> + target_address = 0xFFFF;
> + dev_err(ts->pdev,
> + "Invalid usage table! usage: %u, page: %u, offset: %u\n",
> + usage, page, offset);
> + }
> + break;
> + }
> + }
> + } else {
> + target_address = 0xFFFF;
> + dev_err(ts->pdev, "Unpopulated usage table for usage: %u\n",
> + usage);
> + }
> +
> + dev_dbg(ts->pdev, "target_address is 0x%04x for usage: %u page %u\n",
> + target_address, usage, page);
> +
> + return target_address;
> +}
> +EXPORT_SYMBOL_GPL(usage_to_target_address);
Nope, drop.
> +
> +void axiom_remove(struct axiom_data *ts)
> +{
> + if (ts->usage_table)
> + ts->usage_table_populated = false;
> +
> + if (ts->input_dev)
> + input_unregister_device(ts->input_dev);
> +}
> +EXPORT_SYMBOL_GPL(axiom_remove);
Nope, drop.
> +
> +
> +/**
> + * Take a raw buffer with u41 report data and decode it.
> + * Also generate input events if needed.
> + * @rx_buf: ptr to a byte array [0]: Usage number [1]: Status LSB [2]: Status MSB
> + */
> +void axiom_process_u41_report(struct axiom_data *ts, char *rx_buf)
> +{
> + struct axiom_target_report target;
> + struct input_dev *input_dev = ts->input_dev;
> + bool update_done = false;
> + u16 target_status;
> + u32 i;
> +
> + if (rx_buf[0] != 0x41) {
> + dev_err(ts->pdev,
> + "Data in buffer does not have expected u41 format.\n");
> + return;
> + }
> +
> + target_status = ((rx_buf[1]) | (rx_buf[2] << 8));
> +
> + for (i = 0; i < U41_MAX_TARGETS; i++) {
> + target.index = i;
> + target.present = ((target_status & (1 << i)) != 0) ? 1 : 0;
> + target.x = (rx_buf[(i * 4) + 3]) | (rx_buf[(i * 4) + 4] << 8);
> + target.y = (rx_buf[(i * 4) + 5]) | (rx_buf[(i * 4) + 6] << 8);
> + target.z = (s8) (rx_buf[i + 43]);
> + update_done |= axiom_process_u41_report_target(ts, &target);
> + }
> +
> + if (update_done)
> + input_sync(input_dev);
> +}
> +EXPORT_SYMBOL_GPL(axiom_process_u41_report);
Nope, drop.
> +
> +void axiom_process_u46_report(struct axiom_data *ts, char *rx_buf)
> +{
> + struct input_dev *input_dev = ts->input_dev;
> + u16 aux_value;
> + u32 event_value;
> + u32 i = 0;
> +
> + for (i = 0; i < U46_AUX_CHANNELS; i++) {
> + aux_value =
> + ((rx_buf[(i * 2) + 2] << 8) | (rx_buf[(i * 2) + 1])) &
> + U46_AUX_MASK;
> + event_value = (i << 16) | (aux_value);
> + input_event(input_dev, EV_MSC, MSC_RAW, event_value);
> + }
> +
> + input_mt_sync(input_dev);
> + input_sync(input_dev);
> +}
> +EXPORT_SYMBOL_GPL(axiom_process_u46_report);
Nope, drop.
Clean your driver before sending upstream. :(
> +
> +/**
> + * Support function to axiom_process_report.
> + * It validates the crc and multiplexes the axiom reports to the appropriate
> + * report handler
> + */
> +void axiom_process_report(struct axiom_data *ts, char *report_data)
> +{
> + struct device *pdev = ts->pdev;
> + char usage = report_data[1];
> + u16 crc_report;
> + u16 crc_calc;
> + char len;
> +
> + if ((report_data[0] & COMMS_OVERFLOW_MSK) != 0)
> + ts->report_overflow_counter++;
> +
> + len = (report_data[0] & COMMS_REPORT_LEN_MSK) << 1;
> + if (len == 0) {
> + dev_err(pdev, "Zero length report discarded.\n");
> + return;
> + }
> +
> + dev_dbg(pdev, "Payload Data %*ph\n", len, report_data);
> +
> + /* Validate the report CRC */
> + crc_report = (report_data[len - 1] << 8) | (report_data[len - 2]);
> + /* Length is in 16 bit words and remove the size of the CRC16 itself */
> + crc_calc = crc16(0, report_data, (len - 2));
> +
> + if (crc_calc != crc_report) {
> + dev_err(pdev,
> + "CRC mismatch! Expected: %#x, Calculated CRC: %#x.\n",
> + crc_report, crc_calc);
> + return;
> + }
> +
> + switch (usage) {
> + case USAGE_2DCTS_REPORT_ID:
> + axiom_process_u41_report(ts, &report_data[1]);
> + break;
> +
> + case USAGE_2AUX_REPORT_ID:
> + /* This is an aux report (force) */
> + axiom_process_u46_report(ts, &report_data[1]);
> + break;
> +
> + case USAGE_2HB_REPORT_ID:
> + /* This is a heartbeat report */
> + break;
> +
> + default:
> + break;
> + }
> +
> + ts->report_counter++;
> +}
> +EXPORT_SYMBOL_GPL(axiom_process_report);
Nope, drop.
> +
> +MODULE_AUTHOR("Kamel Bouhara <kamel.bouhara@bootlin.com>");
> +MODULE_DESCRIPTION("aXiom touchscreen core logic");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("axiom");
???? How can you have multiple drivers with the same alias? Even if your
Makefile allowed it (but it doesn't!), it would not make sense.
> diff --git a/drivers/input/touchscreen/axiom_core.h b/drivers/input/touchscreen/axiom_core.h
> new file mode 100644
> index 000000000000..f129d28671ff
> --- /dev/null
> +++ b/drivers/input/touchscreen/axiom_core.h
> @@ -0,0 +1,140 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * TouchNetix aXiom Touchscreen Driver
> + *
> + * Copyright (C) 2020-2023 TouchNetix Ltd.
> + *
> + * Author(s): Mark Satterthwaite <mark.satterthwaite@touchnetix.com>
> + * Pedro Torruella <pedro.torruella@touchnetix.com>
> + * Bart Prescott <bartp@baasheep.co.uk>
> + * Hannah Rossiter <hannah.rossiter@touchnetix.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or (at your
> + * option) any later version.
OK, so this is some old, vendor crappy code. We do not have it since
some years!
> + *
...
> +
> +static void axiom_i2c_poll(struct input_dev *input_dev)
> +{
> + struct axiom_data *ts = input_get_drvdata(input_dev);
> + char *rx_data = ts->rx_buf;
> +
> + axiom_i2c_read(ts->client, REPORT_USAGE_ID, 0, rx_data,
> + ts->max_report_len);
> + axiom_process_report(ts, rx_data);
> +}
> +
> +/**
> + * Retrieve, store and print the axiom device information
> + */
> +int axiom_discover(struct axiom_data *ts)
This has to be static.
> +{
> + char *rx_data = &ts->rx_buf[0];
> + struct device *pdev = ts->pdev;
> + int ret;
> +
> + /* First the first page of u31 to get the device information and */
> + /* the number of usages */
> + ret =
> + axiom_i2c_read(ts->client, DEVINFO_USAGE_ID, 0, rx_data,
> + AX_U31_PAGE0_LENGTH);
> + if (ret)
> + return ret;
> +
> + axiom_get_dev_info(ts, rx_data);
> +
> + dev_dbg(pdev, "Data Decode:\n");
> + dev_dbg(pdev, " Bootloader Mode: %u\n", ts->devinfo.bootloader_mode);
> + dev_dbg(pdev, " Device ID : %04x\n", ts->devinfo.device_id);
> + dev_dbg(pdev, " Firmware Rev : %02x.%02x\n", ts->devinfo.fw_major,
> + ts->devinfo.fw_minor);
> + dev_dbg(pdev, " Bootloader Rev : %02x.%02x\n",
> + ts->devinfo.bootloader_fw_ver_major,
> + ts->devinfo.bootloader_fw_ver_minor);
> + dev_dbg(pdev, " FW Extra Info : %04x\n", ts->devinfo.fw_info_extra);
> + dev_dbg(pdev, " Silicon : %02x\n", ts->devinfo.jedec_id);
> + dev_dbg(pdev, " Num Usages : %04x\n", ts->devinfo.num_usages);
> +
> + /* Read the second page of u31 to get the usage table */
> + ret = axiom_i2c_read(ts->client, DEVINFO_USAGE_ID, 1, rx_data,
> + (U31_BYTES_PER_USAGE * ts->devinfo.num_usages));
> + if (ret)
> + return ret;
> +
> + ts->max_report_len = axiom_populate_usage_table(ts, rx_data);
> + dev_dbg(pdev, "Max Report Length: %u\n", ts->max_report_len);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(axiom_discover);
Nope.
> +
> +/**
> + * Rebaseline the touchscreen, effectively zero-ing it
> + */
> +void axiom_rebaseline(struct axiom_data *ts)
Missing static.
> +{
> + struct device *pdev = ts->pdev;
> + char buffer[8] = { 0 };
> + int ret;
> +
> + memset(buffer, 0, sizeof(buffer));
> +
> + buffer[0] = REBASELINE_CMD;
> +
> + ret = axiom_i2c_write(ts->client, 0x02, 0, buffer, sizeof(buffer));
> + if (ret)
> + dev_err(pdev, "Rebaseline failed\n");
> +
> + dev_info(pdev, "Capture Baseline Requested\n");
> +}
> +EXPORT_SYMBOL_GPL(axiom_rebaseline);
Nope.
> +
> +static irqreturn_t axiom_irq(int irq, void *handle)
> +{
> + struct axiom_data *ts = handle;
> + u8 *rx_data = &ts->rx_buf[0];
> +
> + axiom_i2c_read(ts->client, REPORT_USAGE_ID, 0, rx_data, ts->max_report_len);
> + axiom_process_report(ts, rx_data);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int axiom_i2c_probe(struct i2c_client *client)
> +{
> + struct axiom_data *ts;
> + struct device *pdev = &client->dev;
> + struct input_dev *input_dev;
> + int error;
> + int target;
> +
> + ts = devm_kzalloc(pdev, sizeof(*ts), GFP_KERNEL);
> + if (!ts)
> + return -ENOMEM;
> +
> + ts->irq_gpio = devm_gpiod_get_optional(pdev, "irq", GPIOD_IN);
> + if (IS_ERR(ts->irq_gpio)) {
> + error = PTR_ERR(ts->irq_gpio);
> + dev_err(pdev, "failed to request irq GPIO: %d", error);
Heh... syntax is: return dev_err_probe()
> + return error;
> + }
> +
> + ts->reset_gpio = devm_gpiod_get_optional(pdev, "reset", GPIOD_OUT_HIGH);
You do not use this GPIO at all. Dead code, especially that you keep the
device in reset. This was absolutely never tested (unless with incorrect
DTS...).
> + if (IS_ERR(ts->reset_gpio)) {
> + error = PTR_ERR(ts->reset_gpio);
return dev_err_probe()
> + dev_err(pdev, "failed to request reset GPIO: %d", error);
> + return error;
> + }
> +
> + if (ts->irq_gpio) {
> + error =
> + devm_request_threaded_irq(pdev, client->irq, NULL,
> + axiom_irq,
> + IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> + "axiom_irq", ts);
> + if (error != 0) {
> + dev_err(pdev, "Failed to request IRQ %u (error: %d)\n",
> + client->irq, error);
return dev_err_probe()
> + return error;
> + }
> + }
> +
> + ts->client = client;
> + ts->pdev = pdev;
> + ts->usage_table_populated = false;
> +
> + i2c_set_clientdata(client, ts);
> +
> + axiom_discover(ts);
> + axiom_rebaseline(ts);
> +
> + input_dev = devm_input_allocate_device(ts->pdev);
> + if (!input_dev) {
> + dev_err(pdev, "Failed to allocate input device\n");
I don't think we print memory allocation failures.
> + return -ENOMEM;
> + }
> +
> + input_dev->name = "TouchNetix aXiom Touchscreen";
> + input_dev->phys = "input/axiom_ts";
> +
> + /* Single Touch */
> + input_set_abs_params(input_dev, ABS_X, 0, 65535, 0, 0);
> + input_set_abs_params(input_dev, ABS_Y, 0, 65535, 0, 0);
> +
> + /* Multi Touch */
> + /* Min, Max, Fuzz (expected noise in px, try 4?) and Flat */
> + input_set_abs_params(input_dev, ABS_MT_POSITION_X, 0, 65535, 0, 0);
> + /* Min, Max, Fuzz (expected noise in px, try 4?) and Flat */
> + input_set_abs_params(input_dev, ABS_MT_POSITION_Y, 0, 65535, 0, 0);
> + input_set_abs_params(input_dev, ABS_MT_TOOL_TYPE, 0, MT_TOOL_MAX, 0, 0);
> + input_set_abs_params(input_dev, ABS_MT_DISTANCE, 0, 127, 0, 0);
> + input_set_abs_params(input_dev, ABS_MT_PRESSURE, 0, 127, 0, 0);
> +
> + /* Registers the axiom device as a touch screen instead of as a mouse pointer */
> + input_mt_init_slots(input_dev, U41_MAX_TARGETS, INPUT_MT_DIRECT);
> +
> + input_set_capability(input_dev, EV_KEY, BTN_LEFT);
> +
> + /* Enables the raw data for up to 4 force channels to be sent to the */
> + /* input subsystem */
> + set_bit(EV_REL, input_dev->evbit);
> + set_bit(EV_MSC, input_dev->evbit);
> + /* Declare that we support "RAW" Miscellaneous events */
> + set_bit(MSC_RAW, input_dev->mscbit);
> +
> + if (!ts->irq_gpio) {
> + error = input_setup_polling(input_dev, axiom_i2c_poll);
> + if (error) {
> + dev_err(ts->pdev, "Unable to set up polling: %d\n",
> + error);
> + return error;
> + }
> + input_set_poll_interval(input_dev, POLL_INTERVAL_DEFAULT_MS);
> + }
> +
> + error = input_register_device(input_dev);
> + if (error != 0) {
> + dev_err(ts->pdev,
> + "Could not register with Input Sub-system., error=%d\n",
> + error);
> + return error;
> + }
> +
> + ts->input_dev = input_dev;
> + input_set_drvdata(ts->input_dev, ts);
> +
> + dev_info(pdev, "AXIOM: I2C driver registered with Input Sub-System.\n");
Drop silly tracing messages.
> +
> + /* Delay just a smidge before enabling the IRQ */
> + udelay(10);
??? So where is the IRQ being enabled? This is confusing.
> +
> + /* Ensure that all reports are initialised to not be present. */
> + for (target = 0; target < U41_MAX_TARGETS; target++)
> + ts->targets[target].state = TARGET_STATE_NOT_PRESENT;
> +
> + return 0;
> +}
> +
> +static void axiom_i2c_remove(struct i2c_client *client)
> +{
> + struct axiom_data *ts = i2c_get_clientdata(client);
> +
> + axiom_remove(ts);
> +}
> +
> +static const struct i2c_device_id axiom_i2c_id_table[] = {
> + {"ax54a"},
> + {},
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, axiom_i2c_id_table);
> +
> +static const struct of_device_id axiom_i2c_dt_ids[] = {
> + {
> + .compatible = "axiom,ax54a",
That's not documented. And you would know it if you run basic checks
before sending patches upstream.
Please run scripts/checkpatch.pl and fix reported warnings. Some
warnings can be ignored, but the code here looks like it needs a fix.
Feel free to get in touch if the warning is not clear.
> + .data = "axiom",
Why?
That's not readable. Indent it like in other drivers.
> + },
> + {}
> +};
> +
> +MODULE_DEVICE_TABLE(of, axiom_i2c_dt_ids);
> +
> +static struct i2c_driver axiom_i2c_driver = {
> + .driver = {
> + .name = "axiom_i2c",
> + .of_match_table = of_match_ptr(axiom_i2c_dt_ids),
Drop of_match_ptr(), it causes warnings in your code...
> + },
> + .id_table = axiom_i2c_id_table,
> + .probe = axiom_i2c_probe,
> + .remove = axiom_i2c_remove,
> +};
> +
> +module_i2c_driver(axiom_i2c_driver);
> +
> +MODULE_AUTHOR("Kamel Bouhara <kamel.bouhara@bootlin.com>");
> +MODULE_DESCRIPTION("aXiom touchscreen I2C bus driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("axiom");
Nope, you cannot have such alias and it is not even needed.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2 1/3] selftests/hid: ensure we can compile the tests on kernels pre-6.3
From: Eduard Zingerman @ 2023-09-11 13:19 UTC (permalink / raw)
To: Justin Stitt, Jiri Kosina, Benjamin Tissoires, Shuah Khan
Cc: linux-input, linux-kselftest, linux-kernel, bpf,
Benjamin Tissoires
In-Reply-To: <20230908-kselftest-09-08-v2-1-0def978a4c1b@google.com>
On Fri, 2023-09-08 at 22:22 +0000, Justin Stitt wrote:
> From: Benjamin Tissoires <bentiss@kernel.org>
>
> For the hid-bpf tests to compile, we need to have the definition of
> struct hid_bpf_ctx. This definition is an internal one from the kernel
> and it is supposed to be defined in the generated vmlinux.h.
>
> This vmlinux.h header is generated based on the currently running kernel
> or if the kernel was already compiled in the tree. If you just compile
> the selftests without compiling the kernel beforehand and you are running
> on a 6.2 kernel, you'll end up with a vmlinux.h without the hid_bpf_ctx
> definition.
>
> Use the clever trick from tools/testing/selftests/bpf/progs/bpf_iter.h
> to force the definition of that symbol in case we don't find it in the
> BTF and also add __attribute__((preserve_access_index)) to further
> support CO-RE functionality for these tests.
>
> Signed-off-by: Justin Stitt <justinstitt@google.com>
> Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
> ---
> tools/testing/selftests/hid/progs/hid.c | 3 --
> .../testing/selftests/hid/progs/hid_bpf_helpers.h | 49 ++++++++++++++++++++++
> 2 files changed, 49 insertions(+), 3 deletions(-)
>
> diff --git a/tools/testing/selftests/hid/progs/hid.c b/tools/testing/selftests/hid/progs/hid.c
> index 88c593f753b5..1e558826b809 100644
> --- a/tools/testing/selftests/hid/progs/hid.c
> +++ b/tools/testing/selftests/hid/progs/hid.c
> @@ -1,8 +1,5 @@
> // SPDX-License-Identifier: GPL-2.0
> /* Copyright (c) 2022 Red hat */
> -#include "vmlinux.h"
> -#include <bpf/bpf_helpers.h>
> -#include <bpf/bpf_tracing.h>
> #include "hid_bpf_helpers.h"
>
> char _license[] SEC("license") = "GPL";
> diff --git a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
> index 4fff31dbe0e7..ab3b18ba48c4 100644
> --- a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
> +++ b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
> @@ -5,6 +5,55 @@
> #ifndef __HID_BPF_HELPERS_H
> #define __HID_BPF_HELPERS_H
>
> +/* "undefine" structs in vmlinux.h, because we "override" them below */
Hi Justin,
What you have here should work, however I still think that the trick
with "___local" suffix I refer to in [1] is a bit less hacky, e.g.:
enum hid_report_type___local { ... };
struct hid_bpf_ctx___local {
__u32 index;
const struct hid_device *hid; // this one should be in vmlinux.h with any config
__u32 allocated_size;
enum hid_report_type___local report_type;
union {
__s32 retval;
__s32 size;
};
} __attribute__((preserve_access_index));
enum hid_class_request___local { ... };
enum hid_bpf_attach_flags___local { ... };
...
extern __u8 *hid_bpf_get_data(struct hid_bpf_ctx___local *ctx,
unsigned int offset,
(sorry for being a bore, won't bring this up anymore).
Thanks,
Eduard
[1] https://lore.kernel.org/bpf/e99b4226bd450fedfebd4eb5c37054f032432b4f.camel@gmail.com/
> +#define hid_bpf_ctx hid_bpf_ctx___not_used
> +#define hid_report_type hid_report_type___not_used
> +#define hid_class_request hid_class_request___not_used
> +#define hid_bpf_attach_flags hid_bpf_attach_flags___not_used
> +#include "vmlinux.h"
> +#undef hid_bpf_ctx
> +#undef hid_report_type
> +#undef hid_class_request
> +#undef hid_bpf_attach_flags
> +
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +#include <linux/const.h>
> +
> +enum hid_report_type {
> + HID_INPUT_REPORT = 0,
> + HID_OUTPUT_REPORT = 1,
> + HID_FEATURE_REPORT = 2,
> +
> + HID_REPORT_TYPES,
> +};
> +
> +struct hid_bpf_ctx {
> + __u32 index;
> + const struct hid_device *hid;
> + __u32 allocated_size;
> + enum hid_report_type report_type;
> + union {
> + __s32 retval;
> + __s32 size;
> + };
> +} __attribute__((preserve_access_index));
> +
> +enum hid_class_request {
> + HID_REQ_GET_REPORT = 0x01,
> + HID_REQ_GET_IDLE = 0x02,
> + HID_REQ_GET_PROTOCOL = 0x03,
> + HID_REQ_SET_REPORT = 0x09,
> + HID_REQ_SET_IDLE = 0x0A,
> + HID_REQ_SET_PROTOCOL = 0x0B,
> +};
> +
> +enum hid_bpf_attach_flags {
> + HID_BPF_FLAG_NONE = 0,
> + HID_BPF_FLAG_INSERT_HEAD = _BITUL(0),
> + HID_BPF_FLAG_MAX,
> +};
> +
> /* following are kfuncs exported by HID for HID-BPF */
> extern __u8 *hid_bpf_get_data(struct hid_bpf_ctx *ctx,
> unsigned int offset,
>
^ permalink raw reply
* Fwd: amd-sfh module causes reboot and shutdown hangs randomly on hp aero 13
From: Bagas Sanjaya @ 2023-09-11 13:40 UTC (permalink / raw)
To: Basavaraj Natikar, Benjamin Tissoires, Jiri Kosina, Mehmet
Cc: Linux Kernel Mailing List, Linux Regressions, Linux Input Devices
Hi,
I notice a regression report on Bugzilla [1]. Quoting from it:
> My laptop (hp aero 13-be0024t) hangs at reboot and poweroff requiring physical poweroffs (long pressing the power button) when attached dmesg output is generated. But this seems to be random as sometimes I have a dmesg with no errors related to amd_sfh and I can cleanly reboot/poweroff. Blacklisting amd_sfh module fixes the problem. This problem started with kernel 6.2.x and still present in 6.5.2.
>
> During shutdown/reboot console outputs:
>
> "Failed to umount /oldroot..."
> "kvm exiting virtualization..."
>
> but cannot complete the process (waited for more than 1 hour).
See Bugzilla for the full thread and attached dmesg output for both hanging
and no hanging conditions.
Anyway, I'm adding this regression to be tracked by regzbot:
#regzbot introduced: v6.1..v6.2 https://bugzilla.kernel.org/show_bug.cgi?id=217900
Thanks.
[1]: https://bugzilla.kernel.org/show_bug.cgi?id=217900
--
An old man doll... just what I always wanted! - Clara
^ permalink raw reply
* Re: [PATCH v2 1/3] selftests/hid: ensure we can compile the tests on kernels pre-6.3
From: Eduard Zingerman @ 2023-09-11 13:43 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Justin Stitt, Jiri Kosina, Benjamin Tissoires, Shuah Khan,
linux-input, linux-kselftest, linux-kernel, bpf
In-Reply-To: <hnmbc2vo6ylihwvxbmtiylw6kseqbyk5iydne4vmshssjhrcac@ijbyzhoeag34>
On Mon, 2023-09-11 at 15:39 +0200, Benjamin Tissoires wrote:
> On Sep 11 2023, Eduard Zingerman wrote:
> > On Fri, 2023-09-08 at 22:22 +0000, Justin Stitt wrote:
> > > From: Benjamin Tissoires <bentiss@kernel.org>
> > >
> > > For the hid-bpf tests to compile, we need to have the definition of
> > > struct hid_bpf_ctx. This definition is an internal one from the kernel
> > > and it is supposed to be defined in the generated vmlinux.h.
> > >
> > > This vmlinux.h header is generated based on the currently running kernel
> > > or if the kernel was already compiled in the tree. If you just compile
> > > the selftests without compiling the kernel beforehand and you are running
> > > on a 6.2 kernel, you'll end up with a vmlinux.h without the hid_bpf_ctx
> > > definition.
> > >
> > > Use the clever trick from tools/testing/selftests/bpf/progs/bpf_iter.h
> > > to force the definition of that symbol in case we don't find it in the
> > > BTF and also add __attribute__((preserve_access_index)) to further
> > > support CO-RE functionality for these tests.
> > >
> > > Signed-off-by: Justin Stitt <justinstitt@google.com>
> > > Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
> > > ---
> > > tools/testing/selftests/hid/progs/hid.c | 3 --
> > > .../testing/selftests/hid/progs/hid_bpf_helpers.h | 49 ++++++++++++++++++++++
> > > 2 files changed, 49 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/hid/progs/hid.c b/tools/testing/selftests/hid/progs/hid.c
> > > index 88c593f753b5..1e558826b809 100644
> > > --- a/tools/testing/selftests/hid/progs/hid.c
> > > +++ b/tools/testing/selftests/hid/progs/hid.c
> > > @@ -1,8 +1,5 @@
> > > // SPDX-License-Identifier: GPL-2.0
> > > /* Copyright (c) 2022 Red hat */
> > > -#include "vmlinux.h"
> > > -#include <bpf/bpf_helpers.h>
> > > -#include <bpf/bpf_tracing.h>
> > > #include "hid_bpf_helpers.h"
> > >
> > > char _license[] SEC("license") = "GPL";
> > > diff --git a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
> > > index 4fff31dbe0e7..ab3b18ba48c4 100644
> > > --- a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
> > > +++ b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
> > > @@ -5,6 +5,55 @@
> > > #ifndef __HID_BPF_HELPERS_H
> > > #define __HID_BPF_HELPERS_H
> > >
> > > +/* "undefine" structs in vmlinux.h, because we "override" them below */
> >
> > Hi Justin,
> >
> > What you have here should work, however I still think that the trick
> > with "___local" suffix I refer to in [1] is a bit less hacky, e.g.:
> >
> > enum hid_report_type___local { ... };
> > struct hid_bpf_ctx___local {
> > __u32 index;
> > const struct hid_device *hid; // this one should be in vmlinux.h with any config
> > __u32 allocated_size;
> > enum hid_report_type___local report_type;
> > union {
> > __s32 retval;
> > __s32 size;
> > };
> > } __attribute__((preserve_access_index));
> >
> > enum hid_class_request___local { ... };
> > enum hid_bpf_attach_flags___local { ... };
> > ...
> > extern __u8 *hid_bpf_get_data(struct hid_bpf_ctx___local *ctx,
> > unsigned int offset,
> >
> >
> > (sorry for being a bore, won't bring this up anymore).
>
> No need to apologies for trying to make the code better :)
>
> I specifically asked Justin to not use this because I intend the
> examples to be here to use the same API in the selftests dir than users
> in the wild. So if your suggestion definitely makes the header code
> much better, it also means that everybody will start using `___local`
> annotations for anything HID-BPF related, which is not what I want.
>
> This header file is supposed to be included in the BPF, and IMO it's not
> that important that we have the cleanest code, as long as the users have
> the proper API.
>
> Feel free to share your concerns :)
Got it, thank you for explanation :)
>
> Cheers,
> Benjamin
>
> >
> > Thanks,
> > Eduard
> >
> > [1] https://lore.kernel.org/bpf/e99b4226bd450fedfebd4eb5c37054f032432b4f.camel@gmail.com/
> >
> > > +#define hid_bpf_ctx hid_bpf_ctx___not_used
> > > +#define hid_report_type hid_report_type___not_used
> > > +#define hid_class_request hid_class_request___not_used
> > > +#define hid_bpf_attach_flags hid_bpf_attach_flags___not_used
> > > +#include "vmlinux.h"
> > > +#undef hid_bpf_ctx
> > > +#undef hid_report_type
> > > +#undef hid_class_request
> > > +#undef hid_bpf_attach_flags
> > > +
> > > +#include <bpf/bpf_helpers.h>
> > > +#include <bpf/bpf_tracing.h>
> > > +#include <linux/const.h>
> > > +
> > > +enum hid_report_type {
> > > + HID_INPUT_REPORT = 0,
> > > + HID_OUTPUT_REPORT = 1,
> > > + HID_FEATURE_REPORT = 2,
> > > +
> > > + HID_REPORT_TYPES,
> > > +};
> > > +
> > > +struct hid_bpf_ctx {
> > > + __u32 index;
> > > + const struct hid_device *hid;
> > > + __u32 allocated_size;
> > > + enum hid_report_type report_type;
> > > + union {
> > > + __s32 retval;
> > > + __s32 size;
> > > + };
> > > +} __attribute__((preserve_access_index));
> > > +
> > > +enum hid_class_request {
> > > + HID_REQ_GET_REPORT = 0x01,
> > > + HID_REQ_GET_IDLE = 0x02,
> > > + HID_REQ_GET_PROTOCOL = 0x03,
> > > + HID_REQ_SET_REPORT = 0x09,
> > > + HID_REQ_SET_IDLE = 0x0A,
> > > + HID_REQ_SET_PROTOCOL = 0x0B,
> > > +};
> > > +
> > > +enum hid_bpf_attach_flags {
> > > + HID_BPF_FLAG_NONE = 0,
> > > + HID_BPF_FLAG_INSERT_HEAD = _BITUL(0),
> > > + HID_BPF_FLAG_MAX,
> > > +};
> > > +
> > > /* following are kfuncs exported by HID for HID-BPF */
> > > extern __u8 *hid_bpf_get_data(struct hid_bpf_ctx *ctx,
> > > unsigned int offset,
> > >
> >
^ permalink raw reply
* Re: [PATCH v2 1/3] selftests/hid: ensure we can compile the tests on kernels pre-6.3
From: Benjamin Tissoires @ 2023-09-11 13:39 UTC (permalink / raw)
To: Eduard Zingerman
Cc: Justin Stitt, Jiri Kosina, Benjamin Tissoires, Shuah Khan,
linux-input, linux-kselftest, linux-kernel, bpf
In-Reply-To: <d168d22ba2133d3b38a09ee0e8dbbe0fa97f72d0.camel@gmail.com>
On Sep 11 2023, Eduard Zingerman wrote:
> On Fri, 2023-09-08 at 22:22 +0000, Justin Stitt wrote:
> > From: Benjamin Tissoires <bentiss@kernel.org>
> >
> > For the hid-bpf tests to compile, we need to have the definition of
> > struct hid_bpf_ctx. This definition is an internal one from the kernel
> > and it is supposed to be defined in the generated vmlinux.h.
> >
> > This vmlinux.h header is generated based on the currently running kernel
> > or if the kernel was already compiled in the tree. If you just compile
> > the selftests without compiling the kernel beforehand and you are running
> > on a 6.2 kernel, you'll end up with a vmlinux.h without the hid_bpf_ctx
> > definition.
> >
> > Use the clever trick from tools/testing/selftests/bpf/progs/bpf_iter.h
> > to force the definition of that symbol in case we don't find it in the
> > BTF and also add __attribute__((preserve_access_index)) to further
> > support CO-RE functionality for these tests.
> >
> > Signed-off-by: Justin Stitt <justinstitt@google.com>
> > Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
> > ---
> > tools/testing/selftests/hid/progs/hid.c | 3 --
> > .../testing/selftests/hid/progs/hid_bpf_helpers.h | 49 ++++++++++++++++++++++
> > 2 files changed, 49 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/testing/selftests/hid/progs/hid.c b/tools/testing/selftests/hid/progs/hid.c
> > index 88c593f753b5..1e558826b809 100644
> > --- a/tools/testing/selftests/hid/progs/hid.c
> > +++ b/tools/testing/selftests/hid/progs/hid.c
> > @@ -1,8 +1,5 @@
> > // SPDX-License-Identifier: GPL-2.0
> > /* Copyright (c) 2022 Red hat */
> > -#include "vmlinux.h"
> > -#include <bpf/bpf_helpers.h>
> > -#include <bpf/bpf_tracing.h>
> > #include "hid_bpf_helpers.h"
> >
> > char _license[] SEC("license") = "GPL";
> > diff --git a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
> > index 4fff31dbe0e7..ab3b18ba48c4 100644
> > --- a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
> > +++ b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
> > @@ -5,6 +5,55 @@
> > #ifndef __HID_BPF_HELPERS_H
> > #define __HID_BPF_HELPERS_H
> >
> > +/* "undefine" structs in vmlinux.h, because we "override" them below */
>
> Hi Justin,
>
> What you have here should work, however I still think that the trick
> with "___local" suffix I refer to in [1] is a bit less hacky, e.g.:
>
> enum hid_report_type___local { ... };
> struct hid_bpf_ctx___local {
> __u32 index;
> const struct hid_device *hid; // this one should be in vmlinux.h with any config
> __u32 allocated_size;
> enum hid_report_type___local report_type;
> union {
> __s32 retval;
> __s32 size;
> };
> } __attribute__((preserve_access_index));
>
> enum hid_class_request___local { ... };
> enum hid_bpf_attach_flags___local { ... };
> ...
> extern __u8 *hid_bpf_get_data(struct hid_bpf_ctx___local *ctx,
> unsigned int offset,
>
>
> (sorry for being a bore, won't bring this up anymore).
No need to apologies for trying to make the code better :)
I specifically asked Justin to not use this because I intend the
examples to be here to use the same API in the selftests dir than users
in the wild. So if your suggestion definitely makes the header code
much better, it also means that everybody will start using `___local`
annotations for anything HID-BPF related, which is not what I want.
This header file is supposed to be included in the BPF, and IMO it's not
that important that we have the cleanest code, as long as the users have
the proper API.
Feel free to share your concerns :)
Cheers,
Benjamin
>
> Thanks,
> Eduard
>
> [1] https://lore.kernel.org/bpf/e99b4226bd450fedfebd4eb5c37054f032432b4f.camel@gmail.com/
>
> > +#define hid_bpf_ctx hid_bpf_ctx___not_used
> > +#define hid_report_type hid_report_type___not_used
> > +#define hid_class_request hid_class_request___not_used
> > +#define hid_bpf_attach_flags hid_bpf_attach_flags___not_used
> > +#include "vmlinux.h"
> > +#undef hid_bpf_ctx
> > +#undef hid_report_type
> > +#undef hid_class_request
> > +#undef hid_bpf_attach_flags
> > +
> > +#include <bpf/bpf_helpers.h>
> > +#include <bpf/bpf_tracing.h>
> > +#include <linux/const.h>
> > +
> > +enum hid_report_type {
> > + HID_INPUT_REPORT = 0,
> > + HID_OUTPUT_REPORT = 1,
> > + HID_FEATURE_REPORT = 2,
> > +
> > + HID_REPORT_TYPES,
> > +};
> > +
> > +struct hid_bpf_ctx {
> > + __u32 index;
> > + const struct hid_device *hid;
> > + __u32 allocated_size;
> > + enum hid_report_type report_type;
> > + union {
> > + __s32 retval;
> > + __s32 size;
> > + };
> > +} __attribute__((preserve_access_index));
> > +
> > +enum hid_class_request {
> > + HID_REQ_GET_REPORT = 0x01,
> > + HID_REQ_GET_IDLE = 0x02,
> > + HID_REQ_GET_PROTOCOL = 0x03,
> > + HID_REQ_SET_REPORT = 0x09,
> > + HID_REQ_SET_IDLE = 0x0A,
> > + HID_REQ_SET_PROTOCOL = 0x0B,
> > +};
> > +
> > +enum hid_bpf_attach_flags {
> > + HID_BPF_FLAG_NONE = 0,
> > + HID_BPF_FLAG_INSERT_HEAD = _BITUL(0),
> > + HID_BPF_FLAG_MAX,
> > +};
> > +
> > /* following are kfuncs exported by HID for HID-BPF */
> > extern __u8 *hid_bpf_get_data(struct hid_bpf_ctx *ctx,
> > unsigned int offset,
> >
>
^ permalink raw reply
* Re: [PATCH 2/2] hid-sony: DS3: Report analog buttons for Sixaxis
From: Max Staudt @ 2023-09-11 16:51 UTC (permalink / raw)
To: Roderick Colenbrander
Cc: Dmitry Torokhov, Jiri Kosina, Benjamin Tissoires, Vicki Pfau,
Pavel Rojtberg, Roderick Colenbrander, linux-input, linux-kernel
In-Reply-To: <CAEc3jaDoRESqJ_6KAa6FHvbF=R4ZRV0P+=4KY5pjYPCrwroqCQ@mail.gmail.com>
Hi Roderick,
Thanks for pointing out the 2018 thread!
Since lore.kernel.org doesn't seem to have an archive of it, I hope this one is complete:
https://www.spinics.net/lists/linux-input/msg57662.html
It's been 5 years since the thread you mentioned. There were many outstanding ideas, and yet as of today, Linux still has no support for pressure sensitive buttons. Hence, maybe we can find a "good enough" solution that covers most or all of what we've seen so far, without future-proofing too much?
My experience with controllers comes from sniffing and emulating wire protocols on older consoles, and from looking at the reports from USB devices such as DualShock 3. I presume you have a wider overview, but maybe we can complement each other here :)
As for keyboards, I think that we could simply report a pressure value in parallel with EV_KEY events - like you originally suggested. Maybe we can bundle the two using EV_SYN, given that I see my keyboard combining EV_MSC and EV_KEY in this manner?
As for gamepads, it seems to me like the world has converged to an Xbox360/PS3 style layout for the face buttons and joysticks. SDL, which is used for a vast array of games, maintains a mapping from raw evdev events onto a virtual Xbox 360 gamepad - gamecontrollerdb.txt - which allows games to consistently map, say, the NORTH button, according to the physical geometry rather than what evdev claims. This is something that unfortunately is not unified in the kernel UAPI, and now userspace needs to have knowledge of all devices.
The original thread mentioned the Gamecube controller. I feel that designs that stray from the Xbox360/PS3 layout, such as the Gamecube's separate digital buttons that are hidden beneath the triggers (beyond the 255 point), have disappeared. Please correct me if I'm too short-sighted, but since I don't see such designs on the horizon, I wouldn't worry about describing them in the kernel. Those buttons can be exported as e.g. L3/R3 (which the GC does not have), and userspace then needs to know about the odd physical geometry anyway. The geometry is really a separate property of the controller and breaks modern games' assumptions (see SDL above), so I'd worry about this on the kernel side only once this really comes up as a kernel problem.
There was the idea of Multitouch like event slots, to allow for future expandability. I like it. But do we really need this? We could always add this as yet another event type, or maybe even in a reserved zone within EV_PSB, but for now, all devices that I've seen report a value in the exact range 0-255, with 0 meaning that the button is released. I also don't remember seeing a controller that flakes in its idle state - it has always been a solid 0 when I released the buttons. A flaking button would currently report as EV_KEY 1 anyway, since e.g. the DS3 treats an analog reading of 1 as digital down, I believe.
Hence, how about simply adding an EV_PSB report in parallel with EV_KEY, and this report works exactly the same, except that the range is not 0-1, but 0-255? This keeps backwards compatibility through EV_KEY, and is easy to handle on all ends.
There was also the idea of an expandable array of PSB properties. In the end, it is still up to userspace to make sense of anything we feed it, and there are things we can add in retrospect, and things we can't add without breaking userspace's assumptions.
For example, which value is interpreted as "down" or "up" is something I'd again leave to userspace, or even the hardware itself - after all, up until now, userspace has happily lived with the kernel's binary EV_KEY. If we really want to, we can always add a non-binding "hint" or "suggestion" later.
The only thing I can see as really important right now is pretty much the same info as in input_absinfo - namely, the minimum/maximum values. If we avoid such a structure, and simply tell userspace that values are always 0-255, then we cannot change this afterwards.
But are there any devices that report PSB in a range that is not 0-255, or at least easily scaled into this range? DS2, DS3, Xbox face buttons are all 0-255. So are all L2/R2 triggers that I've seen. Do you know about controllers or keyboards that don't fit well in this range? If there are none, then we could skip describing minimum/maximum values, and cross our fingers for the future.
As for letting userspace detect which buttons support PSB, we could keep a bitmap like for EV_KEY. Or, we could guarantee (in the kernel driver spec) to always send EV_PSB before EV_KEY, and then userspace can dynamically learn which keys are PSBs. Since EV_PSB comes first, it can then ignore any following EV_KEY for that keycode. This way, we don't need to keep a key bitmap either.
As for high-speed chatter overloading the event pipeline, I'd report only changes to a button's value (just like EV_KEY and EV_ABS), not all buttons' state all the time, like the DS3/4 do in their wire protocols.
To avoid analog keyboards overloading classic event loops, I suggest hiding EV_PSB events until they are enabled via some ioctl() or write().
So many ideas, and I hope we can pare it down to an easy to manage minimum - maybe we can get away without any ioctl(EVIOCGPSR) at all :)
Thanks for your help!
Max
^ permalink raw reply
* [PATCH v3 2/5] dt-bindings: mfd: ti,twl: Add clock provider properties
From: Andreas Kemnade @ 2023-09-11 22:13 UTC (permalink / raw)
To: dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt, lee,
bcousson, tony, mturquette, sboyd, andreas, linux-input,
devicetree, linux-kernel, linux-omap, linux-clk
Cc: Conor Dooley
In-Reply-To: <20230911221346.1484543-1-andreas@kemnade.info>
Since these devices provide clock outputs, add the corresponding
property.
Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
---
Documentation/devicetree/bindings/mfd/ti,twl.yaml | 3 +++
1 file changed, 3 insertions(+)
diff --git a/Documentation/devicetree/bindings/mfd/ti,twl.yaml b/Documentation/devicetree/bindings/mfd/ti,twl.yaml
index f125b254a4b93..c04d57ba22b49 100644
--- a/Documentation/devicetree/bindings/mfd/ti,twl.yaml
+++ b/Documentation/devicetree/bindings/mfd/ti,twl.yaml
@@ -37,6 +37,9 @@ properties:
"#interrupt-cells":
const: 1
+ "#clock-cells":
+ const: 1
+
additionalProperties: false
required:
--
2.39.2
^ permalink raw reply related
* [PATCH v3 0/5] ARM: omap: omap4-embt2ws: 32K clock for WLAN
From: Andreas Kemnade @ 2023-09-11 22:13 UTC (permalink / raw)
To: dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt, lee,
bcousson, tony, mturquette, sboyd, andreas, linux-input,
devicetree, linux-kernel, linux-omap, linux-clk
To have WLAN working properly, enable a 32K clock of the TWL6032.
In earlier tests, it was still enabled from a previous boot into
the vendor system.
Changes in V3:
- maintainer change in binding doc
- fix references to binding doc
- additionalProperties: false
- remove subdevices also from examples until
subdevices are referenced/added
Changes in V2:
- no separate device node for the clock
- converted toplevel node of TWL
Andreas Kemnade (5):
dt-bindings: mfd: convert twl-family.txt to json-schema
dt-bindings: mfd: ti,twl: Add clock provider properties
mfd: twl-core: Add a clock subdevice for the TWL6032
clk: twl: add clock driver for TWL6032
ARM: dts: omap4-embt2ws: enable 32K clock on WLAN
.../bindings/input/twl4030-pwrbutton.txt | 2 +-
.../devicetree/bindings/mfd/ti,twl.yaml | 67 ++++++
.../devicetree/bindings/mfd/twl-family.txt | 46 ----
.../boot/dts/ti/omap/omap4-epson-embt2ws.dts | 8 +
drivers/clk/Kconfig | 9 +
drivers/clk/Makefile | 1 +
drivers/clk/clk-twl.c | 197 ++++++++++++++++++
drivers/mfd/twl-core.c | 16 ++
8 files changed, 299 insertions(+), 47 deletions(-)
create mode 100644 Documentation/devicetree/bindings/mfd/ti,twl.yaml
delete mode 100644 Documentation/devicetree/bindings/mfd/twl-family.txt
create mode 100644 drivers/clk/clk-twl.c
--
2.39.2
^ permalink raw reply
* [PATCH v3 3/5] mfd: twl-core: Add a clock subdevice for the TWL6032
From: Andreas Kemnade @ 2023-09-11 22:13 UTC (permalink / raw)
To: dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt, lee,
bcousson, tony, mturquette, sboyd, andreas, linux-input,
devicetree, linux-kernel, linux-omap, linux-clk
In-Reply-To: <20230911221346.1484543-1-andreas@kemnade.info>
Clock device needs no separate devicetree node, so add it as
a platform device. Other devices in the family also have controllable
clocks, but due to the lack of testing, just add it for the TWL6032
now.
Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
drivers/mfd/twl-core.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
index ce01a87f8dc39..234500b2e53fc 100644
--- a/drivers/mfd/twl-core.c
+++ b/drivers/mfd/twl-core.c
@@ -31,6 +31,8 @@
#include <linux/regulator/machine.h>
#include <linux/i2c.h>
+
+#include <linux/mfd/core.h>
#include <linux/mfd/twl.h>
/* Register descriptions for audio */
@@ -690,6 +692,10 @@ static struct of_dev_auxdata twl_auxdata_lookup[] = {
{ /* sentinel */ },
};
+static const struct mfd_cell twl6032_cells[] = {
+ { .name = "twl6032-clk" },
+};
+
/* NOTE: This driver only handles a single twl4030/tps659x0 chip */
static int
twl_probe(struct i2c_client *client)
@@ -836,6 +842,16 @@ twl_probe(struct i2c_client *client)
TWL4030_DCDC_GLOBAL_CFG);
}
+ if (id->driver_data == (TWL6030_CLASS | TWL6032_SUBCLASS)) {
+ status = devm_mfd_add_devices(&client->dev,
+ PLATFORM_DEVID_NONE,
+ twl6032_cells,
+ ARRAY_SIZE(twl6032_cells),
+ NULL, 0, NULL);
+ if (status < 0)
+ goto free;
+ }
+
status = of_platform_populate(node, NULL, twl_auxdata_lookup,
&client->dev);
--
2.39.2
^ permalink raw reply related
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