* Re: [RESEND PATCH v6 3/3] input: pm8xxx-vibrator: add new SPMI vibrator support
From: Fenglin Wu @ 2023-10-09 4:01 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Dmitry Baryshkov, linux-arm-msm, linux-kernel,
krzysztof.kozlowski+dt, robh+dt, agross, andersson, Konrad Dybcio,
linux-input, quic_collinsd, quic_subbaram, quic_kamalw, jestar,
Luca Weiss
In-Reply-To: <ZRhKAWYBLcBZHc73@google.com>
On 10/1/2023 12:17 AM, Dmitry Torokhov wrote:
> On Mon, Sep 25, 2023 at 10:54:45AM +0800, Fenglin Wu wrote:
>>
>>
>> On 9/24/2023 3:07 AM, Dmitry Baryshkov wrote:
>>>> +
>>>> + switch (vib->data->hw_type) {
>>>> + case SSBI_VIB:
>>>> mask = SSBI_VIB_DRV_LEVEL_MASK;
>>>> shift = SSBI_VIB_DRV_SHIFT;
>>>> + break;
>>>> + case SPMI_VIB:
>>>> + mask = SPMI_VIB_DRV_LEVEL_MASK;
>>>> + shift = SPMI_VIB_DRV_SHIFT;
>>>> + break;
>>>> + case SPMI_VIB_GEN2:
>>>> + mask = SPMI_VIB_GEN2_DRV_MASK;
>>>> + shift = SPMI_VIB_GEN2_DRV_SHIFT;
>>>> + break;
>>>> + default:
>>>> + return -EINVAL;
>>> Could you please move the switch to the previous patch? Then it would
>>> be more obvious that you are just adding the SPMI_VIB_GEN2 here.
>>>
>>> Other than that LGTM.
>>
>> Sure, I can move the switch to the previous refactoring patch.
>
> Actually, the idea of having a const "reg" or "chip", etc. structure is
> to avoid this kind of runtime checks based on hardware type and instead
> use common computation. I believe you need to move mask and shift into
> the chip-specific structure and avoid defining hw_type.
>
Actually, the main motivation for adding 'hw_type' is to avoid reading
'reg_base' from DT for SSBI_VIB. It can also help to simplify the
'pm8xxx_vib_data' structure and make following code logic more
straightforward and easier to understand(check hw_type instead of
checking specific constant reg/mask value), it has been used in
following places:
1) Avoid reading 'reg_base' from DT for SSBI_VIB.
2) Only do manual-mode-mask-write for SSBI_VIB. Previously, it was
achieved by giving a valid 'drv_en_manual_mask' value only for SSBI_VIB,
having hw_type make it more straightforward.
3) Not writing VIB_EN register for SSBI_VIB. A similar strategy was
used previously, only write VIB_EN register when 'enable_mask' is valid,
checking hw_type make it more straightforward.
4) To achieve different drive step size for SPMI_VIB （100mV per step)
and SPMI_VIB_GEN2 (1mV per step).
5) Do different VIB_DRV mask and shift assignment for SPMI_VIB and
SPMI_VIB_GEN2
6) Only write VIB_DRV2 for SPMI_VIB_GEN2.
> Thanks.
>
^ permalink raw reply
* Re: [PATCH 1/2] HID: uclogic: Fix user-memory-access bug in uclogic_params_ugee_v2_init_event_hooks()
From: Jinjie Ruan @ 2023-10-09 2:19 UTC (permalink / raw)
To: José Expósito; +Cc: jikos, benjamin.tissoires, linux-input
In-Reply-To: <ZSLk/JfhtJOp6Z81@fedora>
On 2023/10/9 1:21, José Expósito wrote:
> Hi Jinjie Ruan,
>
> Thanks a lot for finding and fixing this bug.
>
> On Thu, Sep 21, 2023 at 09:38:23PM +0800, Jinjie Ruan wrote:
>> When CONFIG_HID_UCLOGIC=y and CONFIG_KUNIT_ALL_TESTS=y, launch kernel and
>> then the below user-memory-access bug occurs.
>>
>> In hid_test_uclogic_params_cleanup_event_hooks(),it call
>> uclogic_params_ugee_v2_init_event_hooks() with the first arg=NULL, so
>> when it calls uclogic_params_ugee_v2_has_battery(), the hid_get_drvdata()
>> will access hdev->dev with hdev=NULL, which will cause below
>> user-memory-access.
>>
>> So add a fake_device with quirks member and call hid_set_drvdata()
>> to assign hdev->dev->driver_data which avoids the null-ptr-def bug
>> for drvdata->quirks in uclogic_params_ugee_v2_has_battery(). After applying
>> this patch, the below user-memory-access bug never occurs.
>>
>> general protection fault, probably for non-canonical address 0xdffffc0000000329: 0000 [#1] PREEMPT SMP KASAN
>> KASAN: probably user-memory-access in range [0x0000000000001948-0x000000000000194f]
>> CPU: 5 PID: 2189 Comm: kunit_try_catch Tainted: G B W N 6.6.0-rc2+ #30
>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
>> RIP: 0010:uclogic_params_ugee_v2_init_event_hooks+0x87/0x600
>> Code: f3 f3 65 48 8b 14 25 28 00 00 00 48 89 54 24 60 31 d2 48 89 fa c7 44 24 30 00 00 00 00 48 c7 44 24 28 02 f8 02 01 48 c1 ea 03 <80> 3c 02 00 0f 85 2c 04 00 00 48 8b 9d 48 19 00 00 48 b8 00 00 00
>> RSP: 0000:ffff88810679fc88 EFLAGS: 00010202
>> RAX: dffffc0000000000 RBX: 0000000000000004 RCX: 0000000000000000
>> RDX: 0000000000000329 RSI: ffff88810679fd88 RDI: 0000000000001948
>> RBP: 0000000000000000 R08: 0000000000000000 R09: ffffed1020f639f0
>> R10: ffff888107b1cf87 R11: 0000000000000400 R12: 1ffff11020cf3f92
>> R13: ffff88810679fd88 R14: ffff888100b97b08 R15: ffff8881030bb080
>> FS: 0000000000000000(0000) GS:ffff888119e80000(0000) knlGS:0000000000000000
>> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> CR2: 0000000000000000 CR3: 0000000005286001 CR4: 0000000000770ee0
>> DR0: ffffffff8fdd6cf4 DR1: ffffffff8fdd6cf5 DR2: ffffffff8fdd6cf6
>> DR3: ffffffff8fdd6cf7 DR6: 00000000fffe0ff0 DR7: 0000000000000600
>> PKRU: 55555554
>> Call Trace:
>> <TASK>
>> ? die_addr+0x3d/0xa0
>> ? exc_general_protection+0x144/0x220
>> ? asm_exc_general_protection+0x22/0x30
>> ? uclogic_params_ugee_v2_init_event_hooks+0x87/0x600
>> ? sched_clock_cpu+0x69/0x550
>> ? uclogic_parse_ugee_v2_desc_gen_params+0x70/0x70
>> ? load_balance+0x2950/0x2950
>> ? rcu_trc_cmpxchg_need_qs+0x67/0xa0
>> hid_test_uclogic_params_cleanup_event_hooks+0x9e/0x1a0
>> ? uclogic_params_ugee_v2_init_event_hooks+0x600/0x600
>> ? __switch_to+0x5cf/0xe60
>> ? migrate_enable+0x260/0x260
>> ? __kthread_parkme+0x83/0x150
>> ? kunit_try_run_case_cleanup+0xe0/0xe0
>> kunit_generic_run_threadfn_adapter+0x4a/0x90
>> ? kunit_try_catch_throw+0x80/0x80
>> kthread+0x2b5/0x380
>> ? kthread_complete_and_exit+0x20/0x20
>> ret_from_fork+0x2d/0x70
>> ? kthread_complete_and_exit+0x20/0x20
>> ret_from_fork_asm+0x11/0x20
>> </TASK>
>> Modules linked in:
>> Dumping ftrace buffer:
>> (ftrace buffer empty)
>> ---[ end trace 0000000000000000 ]---
>> RIP: 0010:uclogic_params_ugee_v2_init_event_hooks+0x87/0x600
>> Code: f3 f3 65 48 8b 14 25 28 00 00 00 48 89 54 24 60 31 d2 48 89 fa c7 44 24 30 00 00 00 00 48 c7 44 24 28 02 f8 02 01 48 c1 ea 03 <80> 3c 02 00 0f 85 2c 04 00 00 48 8b 9d 48 19 00 00 48 b8 00 00 00
>> RSP: 0000:ffff88810679fc88 EFLAGS: 00010202
>> RAX: dffffc0000000000 RBX: 0000000000000004 RCX: 0000000000000000
>> RDX: 0000000000000329 RSI: ffff88810679fd88 RDI: 0000000000001948
>> RBP: 0000000000000000 R08: 0000000000000000 R09: ffffed1020f639f0
>> R10: ffff888107b1cf87 R11: 0000000000000400 R12: 1ffff11020cf3f92
>> R13: ffff88810679fd88 R14: ffff888100b97b08 R15: ffff8881030bb080
>> FS: 0000000000000000(0000) GS:ffff888119e80000(0000) knlGS:0000000000000000
>> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> CR2: 0000000000000000 CR3: 0000000005286001 CR4: 0000000000770ee0
>> DR0: ffffffff8fdd6cf4 DR1: ffffffff8fdd6cf5 DR2: ffffffff8fdd6cf6
>> DR3: ffffffff8fdd6cf7 DR6: 00000000fffe0ff0 DR7: 0000000000000600
>> PKRU: 55555554
>> Kernel panic - not syncing: Fatal exception
>> Dumping ftrace buffer:
>> (ftrace buffer empty)
>> Kernel Offset: disabled
>> Rebooting in 1 seconds..
>>
>> Fixes: a251d6576d2a ("HID: uclogic: Handle wireless device reconnection")
>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
>> ---
>> drivers/hid/hid-uclogic-params-test.c | 15 ++++++++++++++-
>> 1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/hid/hid-uclogic-params-test.c b/drivers/hid/hid-uclogic-params-test.c
>> index 678f50cbb160..3938bae25982 100644
>> --- a/drivers/hid/hid-uclogic-params-test.c
>> +++ b/drivers/hid/hid-uclogic-params-test.c
>> @@ -174,12 +174,22 @@ static void hid_test_uclogic_parse_ugee_v2_desc(struct kunit *test)
>> KUNIT_EXPECT_EQ(test, params->frame_type, frame_type);
>> }
>>
>> +struct fake_device {
>> + unsigned long quirks;
>> +};
>> +
>> static void hid_test_uclogic_params_cleanup_event_hooks(struct kunit *test)
>> {
>> int res, n;
>> + struct hid_device *hdev;
>> + struct fake_device *fake_dev;
>> struct uclogic_params p = {0, };
>>
>> - res = uclogic_params_ugee_v2_init_event_hooks(NULL, &p);
>> + hdev = kzalloc(sizeof(struct hid_device), GFP_KERNEL);
>> + fake_dev = kzalloc(sizeof(struct fake_device), GFP_KERNEL);
>
> Intead of using `kzalloc()` to allocate memory for `hdev` and `fake_dev`
> we should use `kunit_kzalloc()`.
>
> It has 2 main advatages:
> - If an assertion fails, the memory is freed
> - No need for `kfree()`
Thank you! I'll fix it sooner.
>
>> + hid_set_drvdata(hdev, fake_dev);
>> +
>> + res = uclogic_params_ugee_v2_init_event_hooks(hdev, &p);
>> KUNIT_ASSERT_EQ(test, res, 0);
>>
>> /* Check that the function can be called repeatedly */
>> @@ -187,6 +197,9 @@ static void hid_test_uclogic_params_cleanup_event_hooks(struct kunit *test)
>> uclogic_params_cleanup_event_hooks(&p);
>> KUNIT_EXPECT_PTR_EQ(test, p.event_hooks, NULL);
>> }
>> +
>> + kfree(fake_dev);
>> + kfree(hdev);
>
> This 2 lines can be removed if `kunit_kzalloc()` is used.
>
>> }
>>
>> static struct kunit_case hid_uclogic_params_test_cases[] = {
>> --
>> 2.34.1
>>
>
> Once the `kunit_kzalloc()` change is appliyed:
> Reviewed-by: José Expósito <jose.exposito89@gmail.com>
>
^ permalink raw reply
* Re: [PATCH v3 1/2] dt-bindings: input: bindings for Adafruit Seesaw Gamepad
From: Rob Herring @ 2023-10-08 19:33 UTC (permalink / raw)
To: Anshul Dalal
Cc: Dmitry Torokhov, Krzysztof Kozlowski, linux-kernel-mentees,
Conor Dooley, linux-input, Rob Herring, devicetree, Shuah Khan
In-Reply-To: <20231008185709.2448423-1-anshulusr@gmail.com>
On Mon, 09 Oct 2023 00:27:06 +0530, Anshul Dalal wrote:
> A simple driver for a mini gamepad that communicates over i2c, the gamepad
> has bidirectional thumb stick input and six buttons.
>
> The gamepad chip utilizes the open framework from Adafruit called 'Seesaw'
> to transmit the ADC data for the joystick and digital pin state for the
> buttons. I have only implemented the functionality required to receive the
> thumb stick and button state.
>
> Product page:
> https://www.adafruit.com/product/5743
> Arduino driver:
> https://github.com/adafruit/Adafruit_Seesaw
>
> For testing on a RPi Zero 2W, I utilized the following devicetree overlay:
>
> /dts-v1/;
> /plugin/;
> / {
> compatible = "brcm,bcm2835";
> fragment@0 {
> target = <&i2c1>;
> __overlay__ {
> #address-cells = <1>;
> #size-cells = <0>;
> joystick@50 {
> compatible = "adafruit,seesaw-gamepad";
> reg = <0x50>;
> };
> };
> };
> };
>
> I used the above overlay as reference for writing this binding. Though the
> gamepad also has an interrupt pin that needs to be enabled explicitly (not
> currently implemented in driver). The pin triggers a rising edge when a
> button is pressed or joystick is moved which can be detected on a GPIO
> of the Microcontroller.
>
> I wasn't sure how to represent that functionality in the binding so I have
> left it out for now.
>
> Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
> ---
>
> Changes for v3:
> - Updated id field to reflect updated file name from previous version
> - Added `reg` property
>
> Changes for v2:
> - Renamed file to `adafruit,seesaw-gamepad.yaml`
> - Removed quotes for `$id` and `$schema`
> - Removed "Bindings for" from the description
> - Changed node name to the generic name "joystick"
> - Changed compatible to 'adafruit,seesaw-gamepad' instead of 'adafruit,seesaw_gamepad'
>
> .../input/adafruit,seesaw-gamepad.yaml | 54 +++++++++++++++++++
> 1 file changed, 54 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml: $id: Cannot determine base path from $id, relative path/filename doesn't match actual path or filename
$id: http://devicetree.org/schemas/input/adafruit-seesaw.yaml
file: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231008185709.2448423-1-anshulusr@gmail.com
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
^ permalink raw reply
* [PATCH] Input: bcm5974 - check endpoint type before starting traffic
From: Javier Carrasco @ 2023-10-08 19:16 UTC (permalink / raw)
To: John Horan, Henrik Rydberg, Dmitry Torokhov
Cc: linux-input, linux-kernel, Javier Carrasco,
syzbot+348331f63b034f89b622
syzbot has found a type mismatch between a USB pipe and the transfer
endpoint, which is triggered by the bcm5974 driver[1].
This driver expects the device to provide input interrupt endpoints and
if that is not the case, the driver registration should terminate.
Repros are available to reproduce this issue with a certain setup for
the dummy_hcd, leading to an interrupt/bulk mismatch which is caught in
the USB core after calling usb_submit_urb() with the following message:
"BOGUS urb xfer, pipe 1 != type 3"
Some other device drivers (like the appletouch driver bcm5974 is mainly
based on) provide some checking mechanism to make sure that an IN
interrupt endpoint is available. In this particular case the endpoint
addresses are provided by a config table, so the checking can be
targeted to the provided endpoints.
Add some basic checking to guarantee that the endpoints available match
the expected type for both the trackpad and button endpoints.
This issue was only found for the trackpad endpoint, but the checking
has been added to the button endpoint as well for the same reasons.
[1] https://syzkaller.appspot.com/bug?extid=348331f63b034f89b622
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
Reported-and-tested-by: syzbot+348331f63b034f89b622@syzkaller.appspotmail.com
---
drivers/input/mouse/bcm5974.c | 37 ++++++++++++++++++++++++++++++++++++-
1 file changed, 36 insertions(+), 1 deletion(-)
diff --git a/drivers/input/mouse/bcm5974.c b/drivers/input/mouse/bcm5974.c
index ca150618d32f..eb552bb4751e 100644
--- a/drivers/input/mouse/bcm5974.c
+++ b/drivers/input/mouse/bcm5974.c
@@ -891,6 +891,26 @@ static int bcm5974_resume(struct usb_interface *iface)
return error;
}
+static int bcm5974_int_in_endpoint(struct usb_host_interface *iface, int addr)
+{
+ struct usb_endpoint_descriptor *endpoint;
+ bool int_in_endpoint = false;
+ int i;
+
+ for (i = 0; i < iface->desc.bNumEndpoints; i++) {
+ endpoint = &iface->endpoint[i].desc;
+ if (endpoint->bEndpointAddress == addr) {
+ if (usb_endpoint_is_int_in(endpoint))
+ int_in_endpoint = true;
+ break;
+ }
+ }
+ if (!int_in_endpoint)
+ return -ENODEV;
+
+ return 0;
+}
+
static int bcm5974_probe(struct usb_interface *iface,
const struct usb_device_id *id)
{
@@ -898,16 +918,31 @@ static int bcm5974_probe(struct usb_interface *iface,
const struct bcm5974_config *cfg;
struct bcm5974 *dev;
struct input_dev *input_dev;
- int error = -ENOMEM;
+ int error;
/* find the product index */
cfg = bcm5974_get_config(udev);
+ if (cfg->tp_type == TYPE1) {
+ error = bcm5974_int_in_endpoint(iface->cur_altsetting, cfg->bt_ep);
+ if (error) {
+ dev_err(&iface->dev, "No int-in endpoint for the button\n");
+ return error;
+ }
+ }
+
+ error = bcm5974_int_in_endpoint(iface->cur_altsetting, cfg->tp_ep);
+ if (error) {
+ dev_err(&iface->dev, "No int-in endpoint for the trackpad\n");
+ return error;
+ }
+
/* allocate memory for our device state and initialize it */
dev = kzalloc(sizeof(struct bcm5974), GFP_KERNEL);
input_dev = input_allocate_device();
if (!dev || !input_dev) {
dev_err(&iface->dev, "out of memory\n");
+ error = -ENOMEM;
goto err_free_devs;
}
---
base-commit: b9ddbb0cde2adcedda26045cc58f31316a492215
change-id: 20231007-topic-bcm5974_bulk-c66b743ba7ba
Best regards,
--
Javier Carrasco <javier.carrasco.cruz@gmail.com>
^ permalink raw reply related
* Re: [PATCH v2 2/2] input: joystick: driver for Adafruit Seesaw Gamepad
From: Thomas Weißschuh @ 2023-10-08 19:10 UTC (permalink / raw)
To: Anshul Dalal
Cc: linux-input, devicetree, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shuah Khan,
linux-kernel-mentees
In-Reply-To: <20231008172435.2391009-2-anshulusr@gmail.com>
On 2023-10-08 22:54:34+0530, Anshul Dalal wrote:
> [..]
> +
> + input_report_abs(input, ABS_X, data.x);
> + input_report_abs(input, ABS_Y, data.y);
> + input_report_key(input, BTN_A, data.button_a);
> + input_report_key(input, BTN_B, data.button_b);
> + input_report_key(input, BTN_X, data.button_x);
> + input_report_key(input, BTN_Y, data.button_y);
FYI the button layout on this device is different from common commercial
gamepads like XBox, Playstation and Logitech.
This seems to be common to all gamepads from Adafruit.
Adafruit:
X
Y A
B
Others:
Y
X B
A
In input-event-codes.h the symbol BTN_A actually maps to BTN_SOUTH,
matching the common layout. But as you can see this is wrong for this
device.
(Same for BTN_B and BTN_EAST)
Weirdly enough for BTN_X/BTN_NORTH and BTN_Y/BNT_WEST the aliasing is
correct for Adafruit and wrong for the others.
Not sure how to fix this correctly. Maybe the input maintainers know.
> + input_report_key(input, BTN_START, data.button_start);
> + input_report_key(input, BTN_SELECT, data.button_select);
> + input_sync(input);
> +}
^ permalink raw reply
* [PATCH v3 2/2] input: joystick: driver for Adafruit Seesaw Gamepad
From: Anshul Dalal @ 2023-10-08 18:57 UTC (permalink / raw)
To: linux-input, devicetree
Cc: Anshul Dalal, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Shuah Khan, linux-kernel-mentees
In-Reply-To: <20231008185709.2448423-1-anshulusr@gmail.com>
A simple driver for a mini gamepad that communicates over i2c, the gamepad
has bidirectional thumb stick input and six buttons.
The gamepad chip utilizes the open framework from Adafruit called 'Seesaw'
to transmit the ADC data for the joystick and digital pin state for the
buttons. I have only implemented the functionality required to receive the
thumb stick and button state.
Steps in reading the gamepad state over i2c:
1. Reset the registers
2. Set the pin mode of the pins specified by the `BUTTON_MASK` to input
`BUTTON_MASK`: A bit-map for the six digital pins internally
connected to the joystick buttons.
3. Enable internal pullup resistors for the `BUTTON_MASK`
4. Bulk set the pin state HIGH for `BUTTON_MASK`
5. Poll the device for button and joystick state done by:
`seesaw_read_data(struct i2c_client *client, struct seesaw_data *data)`
Product page:
https://www.adafruit.com/product/5743
Arduino driver:
https://github.com/adafruit/Adafruit_Seesaw
Driver tested on RPi Zero 2W
Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
---
Changes for v3:
- no updates
Changes for v2:
adafruit-seesaw.c:
- Renamed file from 'adafruit_seesaw.c'
- Changed device name from 'seesaw_gamepad' to 'seesaw-gamepad'
- Changed count parameter for receiving joystick x on line 118:
`2` to `sizeof(write_buf)`
- Fixed invalid buffer size on line 123 and 126:
`data->y` to `sizeof(data->y)`
- Added comment for the `mdelay(10)` on line 169
- Changed inconsistent indentation on line 271
Kconfig:
- Fixed indentation for the help text
- Updated module name
Makefile:
- Updated module object file name
MAINTAINERS:
- Updated file name for the driver and bindings
MAINTAINERS | 7 +
drivers/input/joystick/Kconfig | 9 +
drivers/input/joystick/Makefile | 1 +
drivers/input/joystick/adafruit-seesaw.c | 277 +++++++++++++++++++++++
4 files changed, 294 insertions(+)
create mode 100644 drivers/input/joystick/adafruit-seesaw.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 6c4cce45a09d..a314f9b48e21 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -441,6 +441,13 @@ W: http://wiki.analog.com/AD7879
W: https://ez.analog.com/linux-software-drivers
F: drivers/input/touchscreen/ad7879.c
+ADAFRUIT MINI I2C GAMEPAD
+M: Anshul Dalal <anshulusr@gmail.com>
+L: linux-input@vger.kernel.org
+S: Maintained
+F: Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml
+F: drivers/input/joystick/adafruit-seesaw.c
+
ADDRESS SPACE LAYOUT RANDOMIZATION (ASLR)
M: Jiri Kosina <jikos@kernel.org>
S: Maintained
diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
index ac6925ce8366..df9cd1830b29 100644
--- a/drivers/input/joystick/Kconfig
+++ b/drivers/input/joystick/Kconfig
@@ -412,4 +412,13 @@ config JOYSTICK_SENSEHAT
To compile this driver as a module, choose M here: the
module will be called sensehat_joystick.
+config JOYSTICK_SEESAW
+ tristate "Adafruit Mini I2C Gamepad with Seesaw"
+ depends on I2C
+ help
+ Say Y here if you want to use the Adafruit Mini I2C Gamepad.
+
+ To compile this driver as a module, choose M here: the module will be
+ called adafruit-seesaw.
+
endif
diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile
index 3937535f0098..9976f596a920 100644
--- a/drivers/input/joystick/Makefile
+++ b/drivers/input/joystick/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_JOYSTICK_N64) += n64joy.o
obj-$(CONFIG_JOYSTICK_PSXPAD_SPI) += psxpad-spi.o
obj-$(CONFIG_JOYSTICK_PXRC) += pxrc.o
obj-$(CONFIG_JOYSTICK_QWIIC) += qwiic-joystick.o
+obj-$(CONFIG_JOYSTICK_SEESAW) += adafruit-seesaw.o
obj-$(CONFIG_JOYSTICK_SENSEHAT) += sensehat-joystick.o
obj-$(CONFIG_JOYSTICK_SIDEWINDER) += sidewinder.o
obj-$(CONFIG_JOYSTICK_SPACEBALL) += spaceball.o
diff --git a/drivers/input/joystick/adafruit-seesaw.c b/drivers/input/joystick/adafruit-seesaw.c
new file mode 100644
index 000000000000..1e8b3f5604d8
--- /dev/null
+++ b/drivers/input/joystick/adafruit-seesaw.c
@@ -0,0 +1,277 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2023 Anshul Dalal <anshulusr@gmail.com>
+ *
+ * Driver for Adafruit Mini I2C Gamepad
+ *
+ * Based on the work of:
+ * Oleh Kravchenko (Sparkfun Qwiic Joystick driver)
+ *
+ * Product page: https://www.adafruit.com/product/5743
+ * Firmware and hardware sources: https://github.com/adafruit/Adafruit_Seesaw
+ */
+
+#include <linux/bits.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/input.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+/* clang-format off */
+#define SEESAW_DEVICE_NAME "seesaw-gamepad"
+
+#define SEESAW_STATUS_BASE 0
+#define SEESAW_GPIO_BASE 1
+#define SEESAW_ADC_BASE 9
+
+#define SEESAW_GPIO_DIRCLR_BULK 3
+#define SEESAW_GPIO_BULK 4
+#define SEESAW_GPIO_BULK_SET 5
+#define SEESAW_GPIO_PULLENSET 11
+
+#define SEESAW_STATUS_HW_ID 1
+#define SEESAW_STATUS_SWRST 127
+
+#define SEESAW_ADC_OFFSET 7
+
+#define BUTTON_A 5
+#define BUTTON_B 1
+#define BUTTON_X 6
+#define BUTTON_Y 2
+#define BUTTON_START 16
+#define BUTTON_SELECT 0
+
+#define ANALOG_X 14
+#define ANALOG_Y 15
+
+#define SEESAW_JOYSTICK_MAX_AXIS 1023
+#define SEESAW_JOYSTICK_FUZZ 2
+#define SEESAW_JOYSTICK_FLAT 4
+
+#define SEESAW_GAMEPAD_POLL_INTERVAL 16
+#define SEESAW_GAMEPAD_POLL_MIN 8
+#define SEESAW_GAMEPAD_POLL_MAX 32
+/* clang-format on */
+
+u32 BUTTON_MASK = (1UL << BUTTON_A) | (1UL << BUTTON_B) | (1UL << BUTTON_X) |
+ (1UL << BUTTON_Y) | (1UL << BUTTON_START) |
+ (1UL << BUTTON_SELECT);
+
+struct seesaw_gamepad {
+ char physical_path[32];
+ unsigned char hardware_id;
+ struct input_dev *input_dev;
+ struct i2c_client *i2c_client;
+};
+
+struct seesaw_data {
+ __be16 x;
+ __be16 y;
+ u8 button_a, button_b, button_x, button_y, button_start, button_select;
+};
+
+static int seesaw_read_data(struct i2c_client *client, struct seesaw_data *data)
+{
+ int err;
+ unsigned char write_buf[2] = { SEESAW_GPIO_BASE, SEESAW_GPIO_BULK };
+ unsigned char read_buf[4];
+
+ err = i2c_master_send(client, write_buf, sizeof(write_buf));
+ if (err < 0)
+ return err;
+ if (err != sizeof(write_buf))
+ return -EIO;
+ err = i2c_master_recv(client, read_buf, sizeof(read_buf));
+ if (err < 0)
+ return err;
+ if (err != sizeof(read_buf))
+ return -EIO;
+ u32 result = ((u32)read_buf[0] << 24) | ((u32)read_buf[1] << 16) |
+ ((u32)read_buf[2] << 8) | (u32)read_buf[3];
+ data->button_a = !(result & (1UL << BUTTON_A));
+ data->button_b = !(result & (1UL << BUTTON_B));
+ data->button_x = !(result & (1UL << BUTTON_X));
+ data->button_y = !(result & (1UL << BUTTON_Y));
+ data->button_start = !(result & (1UL << BUTTON_START));
+ data->button_select = !(result & (1UL << BUTTON_SELECT));
+
+ write_buf[0] = SEESAW_ADC_BASE;
+ write_buf[1] = SEESAW_ADC_OFFSET + ANALOG_X;
+ err = i2c_master_send(client, write_buf, sizeof(write_buf));
+ if (err < 0)
+ return err;
+ if (err != sizeof(write_buf))
+ return -EIO;
+ err = i2c_master_recv(client, (char *)&data->x, sizeof(data->x));
+ if (err < 0)
+ return err;
+ if (err != sizeof(data->x))
+ return -EIO;
+ /*
+ * ADC reads left as max and right as 0, must be reversed since kernel
+ * expects reports in opposite order.
+ */
+ data->x = SEESAW_JOYSTICK_MAX_AXIS - be16_to_cpu(data->x);
+
+ write_buf[1] = SEESAW_ADC_OFFSET + ANALOG_Y;
+ err = i2c_master_send(client, write_buf, sizeof(write_buf));
+ if (err < 0)
+ return err;
+ if (err != sizeof(write_buf))
+ return -EIO;
+ err = i2c_master_recv(client, (char *)&data->y, sizeof(data->y));
+ if (err < 0)
+ return err;
+ if (err != sizeof(data->y))
+ return -EIO;
+ data->y = be16_to_cpu(data->y);
+
+ return 0;
+}
+
+static void seesaw_poll(struct input_dev *input)
+{
+ struct seesaw_gamepad *private = input_get_drvdata(input);
+ struct seesaw_data data;
+ int err;
+
+ err = seesaw_read_data(private->i2c_client, &data);
+ if (err != 0)
+ return;
+
+ input_report_abs(input, ABS_X, data.x);
+ input_report_abs(input, ABS_Y, data.y);
+ input_report_key(input, BTN_A, data.button_a);
+ input_report_key(input, BTN_B, data.button_b);
+ input_report_key(input, BTN_X, data.button_x);
+ input_report_key(input, BTN_Y, data.button_y);
+ input_report_key(input, BTN_START, data.button_start);
+ input_report_key(input, BTN_SELECT, data.button_select);
+ input_sync(input);
+}
+
+static int seesaw_probe(struct i2c_client *client)
+{
+ int err;
+ struct seesaw_gamepad *private;
+ unsigned char register_reset[] = { SEESAW_STATUS_BASE,
+ SEESAW_STATUS_SWRST, 0xFF };
+ unsigned char get_hw_id[] = { SEESAW_STATUS_BASE, SEESAW_STATUS_HW_ID };
+
+ err = i2c_master_send(client, register_reset, sizeof(register_reset));
+ if (err < 0)
+ return err;
+ if (err != sizeof(register_reset))
+ return -EIO;
+
+ /* Wait for the registers to reset before proceeding */
+ mdelay(10);
+
+ private = devm_kzalloc(&client->dev, sizeof(*private), GFP_KERNEL);
+ if (!private)
+ return -ENOMEM;
+
+ err = i2c_master_send(client, get_hw_id, sizeof(get_hw_id));
+ if (err < 0)
+ return err;
+ if (err != sizeof(get_hw_id))
+ return -EIO;
+ err = i2c_master_recv(client, &private->hardware_id, 1);
+ if (err < 0)
+ return err;
+ if (err != 1)
+ return -EIO;
+
+ dev_dbg(&client->dev, "Adafruit Seesaw Gamepad, Hardware ID: %02x\n",
+ private->hardware_id);
+
+ private->i2c_client = client;
+ scnprintf(private->physical_path, sizeof(private->physical_path),
+ "i2c/%s", dev_name(&client->dev));
+ i2c_set_clientdata(client, private);
+
+ private->input_dev = devm_input_allocate_device(&client->dev);
+ if (!private->input_dev)
+ return -ENOMEM;
+
+ private->input_dev->id.bustype = BUS_I2C;
+ private->input_dev->name = "Adafruit Seesaw Gamepad";
+ private->input_dev->phys = private->physical_path;
+ input_set_drvdata(private->input_dev, private);
+ input_set_abs_params(private->input_dev, ABS_X, 0,
+ SEESAW_JOYSTICK_MAX_AXIS, SEESAW_JOYSTICK_FUZZ,
+ SEESAW_JOYSTICK_FLAT);
+ input_set_abs_params(private->input_dev, ABS_Y, 0,
+ SEESAW_JOYSTICK_MAX_AXIS, SEESAW_JOYSTICK_FUZZ,
+ SEESAW_JOYSTICK_FLAT);
+ input_set_capability(private->input_dev, EV_KEY, BTN_A);
+ input_set_capability(private->input_dev, EV_KEY, BTN_B);
+ input_set_capability(private->input_dev, EV_KEY, BTN_X);
+ input_set_capability(private->input_dev, EV_KEY, BTN_Y);
+ input_set_capability(private->input_dev, EV_KEY, BTN_START);
+ input_set_capability(private->input_dev, EV_KEY, BTN_SELECT);
+
+ err = input_setup_polling(private->input_dev, seesaw_poll);
+ if (err) {
+ dev_err(&client->dev, "failed to set up polling: %d\n", err);
+ return err;
+ }
+
+ input_set_poll_interval(private->input_dev,
+ SEESAW_GAMEPAD_POLL_INTERVAL);
+ input_set_max_poll_interval(private->input_dev,
+ SEESAW_GAMEPAD_POLL_MAX);
+ input_set_min_poll_interval(private->input_dev,
+ SEESAW_GAMEPAD_POLL_MIN);
+
+ err = input_register_device(private->input_dev);
+ if (err) {
+ dev_err(&client->dev, "failed to register joystick: %d\n", err);
+ return err;
+ }
+
+ /* Set Pin Mode to input and enable pull-up resistors */
+ unsigned char pin_mode[] = { SEESAW_GPIO_BASE, SEESAW_GPIO_DIRCLR_BULK,
+ BUTTON_MASK >> 24, BUTTON_MASK >> 16,
+ BUTTON_MASK >> 8, BUTTON_MASK };
+ err = i2c_master_send(client, pin_mode, sizeof(pin_mode));
+ pin_mode[1] = SEESAW_GPIO_PULLENSET;
+ err |= i2c_master_send(client, pin_mode, sizeof(pin_mode));
+ pin_mode[1] = SEESAW_GPIO_BULK_SET;
+ err |= i2c_master_send(client, pin_mode, sizeof(pin_mode));
+ if (err < 0)
+ return err;
+ if (err != sizeof(pin_mode))
+ return -EIO;
+
+ return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id of_seesaw_match[] = {
+ {
+ .compatible = "adafruit,seesaw-gamepad",
+ },
+ { /* Sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, of_seesaw_match);
+#endif /* CONFIG_OF */
+
+static const struct i2c_device_id seesaw_id_table[] = { { KBUILD_MODNAME, 0 },
+ { /* Sentinel */ } };
+MODULE_DEVICE_TABLE(i2c, seesaw_id_table);
+
+static struct i2c_driver seesaw_driver = {
+ .driver = {
+ .name = SEESAW_DEVICE_NAME,
+ .of_match_table = of_match_ptr(of_seesaw_match),
+ },
+ .id_table = seesaw_id_table,
+ .probe = seesaw_probe,
+};
+module_i2c_driver(seesaw_driver);
+
+MODULE_AUTHOR("Anshul Dalal <anshulusr@gmail.com>");
+MODULE_DESCRIPTION("Adafruit Mini I2C Gamepad driver");
+MODULE_LICENSE("GPL");
--
2.42.0
^ permalink raw reply related
* [PATCH v3 1/2] dt-bindings: input: bindings for Adafruit Seesaw Gamepad
From: Anshul Dalal @ 2023-10-08 18:57 UTC (permalink / raw)
To: linux-input, devicetree
Cc: Anshul Dalal, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Shuah Khan, linux-kernel-mentees
A simple driver for a mini gamepad that communicates over i2c, the gamepad
has bidirectional thumb stick input and six buttons.
The gamepad chip utilizes the open framework from Adafruit called 'Seesaw'
to transmit the ADC data for the joystick and digital pin state for the
buttons. I have only implemented the functionality required to receive the
thumb stick and button state.
Product page:
https://www.adafruit.com/product/5743
Arduino driver:
https://github.com/adafruit/Adafruit_Seesaw
For testing on a RPi Zero 2W, I utilized the following devicetree overlay:
/dts-v1/;
/plugin/;
/ {
compatible = "brcm,bcm2835";
fragment@0 {
target = <&i2c1>;
__overlay__ {
#address-cells = <1>;
#size-cells = <0>;
joystick@50 {
compatible = "adafruit,seesaw-gamepad";
reg = <0x50>;
};
};
};
};
I used the above overlay as reference for writing this binding. Though the
gamepad also has an interrupt pin that needs to be enabled explicitly (not
currently implemented in driver). The pin triggers a rising edge when a
button is pressed or joystick is moved which can be detected on a GPIO
of the Microcontroller.
I wasn't sure how to represent that functionality in the binding so I have
left it out for now.
Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
---
Changes for v3:
- Updated id field to reflect updated file name from previous version
- Added `reg` property
Changes for v2:
- Renamed file to `adafruit,seesaw-gamepad.yaml`
- Removed quotes for `$id` and `$schema`
- Removed "Bindings for" from the description
- Changed node name to the generic name "joystick"
- Changed compatible to 'adafruit,seesaw-gamepad' instead of 'adafruit,seesaw_gamepad'
.../input/adafruit,seesaw-gamepad.yaml | 54 +++++++++++++++++++
1 file changed, 54 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml
diff --git a/Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml b/Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml
new file mode 100644
index 000000000000..610c99594439
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml
@@ -0,0 +1,54 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/adafruit-seesaw.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Adafruit Mini I2C Gamepad with seesaw
+
+maintainers:
+ - Anshul Dalal <anshulusr@gmail.com>
+
+description: |
+ Adafruit Mini I2C Gamepad
+
+ +-----------------------------+
+ | ___ |
+ | / \ (X) |
+ | | S | __ __ (Y) (A) |
+ | \___/ |ST| |SE| (B) |
+ | |
+ +-----------------------------+
+
+ S -> 10-bit percision bidirectional analog joystick
+ ST -> Start
+ SE -> Select
+ X, A, B, Y -> Digital action buttons
+
+ Product page: https://www.adafruit.com/product/5743
+ Arduino Driver: https://github.com/adafruit/Adafruit_Seesaw
+
+properties:
+ compatible:
+ const: adafruit,seesaw-gamepad
+
+ reg:
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ joystick@50 {
+ compatible = "adafruit,seesaw-gamepad";
+ reg = <0x50>;
+ };
+ };
--
2.42.0
^ permalink raw reply related
* Re: [PATCH v2 2/2] input: joystick: driver for Adafruit Seesaw Gamepad
From: Thomas Weißschuh @ 2023-10-08 18:44 UTC (permalink / raw)
To: Anshul Dalal
Cc: linux-input, devicetree, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Shuah Khan,
linux-kernel-mentees
In-Reply-To: <20231008172435.2391009-2-anshulusr@gmail.com>
Hi,
please also Cc linux-kernel@vger.kernel.org on all patches.
Some more comments inline.
On 2023-10-08 22:54:34+0530, Anshul Dalal wrote:
> A simple driver for a mini gamepad that communicates over i2c, the gamepad
> has bidirectional thumb stick input and six buttons.
>
> The gamepad chip utilizes the open framework from Adafruit called 'Seesaw'
> to transmit the ADC data for the joystick and digital pin state for the
> buttons. I have only implemented the functionality required to receive the
> thumb stick and button state.
>
> Steps in reading the gamepad state over i2c:
> 1. Reset the registers
> 2. Set the pin mode of the pins specified by the `BUTTON_MASK` to input
> `BUTTON_MASK`: A bit-map for the six digital pins internally
> connected to the joystick buttons.
> 3. Enable internal pullup resistors for the `BUTTON_MASK`
> 4. Bulk set the pin state HIGH for `BUTTON_MASK`
> 5. Poll the device for button and joystick state done by:
> `seesaw_read_data(struct i2c_client *client, struct seesaw_data *data)`
>
> Product page:
> https://www.adafruit.com/product/5743
> Arduino driver:
> https://github.com/adafruit/Adafruit_Seesaw
>
> Driver tested on RPi Zero 2W
>
> Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
> ---
>
> Changes for v2:
> adafruit-seesaw.c:
> - Renamed file from 'adafruit_seesaw.c'
> - Changed device name from 'seesaw_gamepad' to 'seesaw-gamepad'
> - Changed count parameter for receiving joystick x on line 118:
> `2` to `sizeof(write_buf)`
> - Fixed invalid buffer size on line 123 and 126:
> `data->y` to `sizeof(data->y)`
> - Added comment for the `mdelay(10)` on line 169
> - Changed inconsistent indentation on line 271
> Kconfig:
> - Fixed indentation for the help text
> - Updated module name
> Makefile:
> - Updated module object file name
> MAINTAINERS:
> - Updated file name for the driver and bindings
>
> MAINTAINERS | 7 +
> drivers/input/joystick/Kconfig | 9 +
> drivers/input/joystick/Makefile | 1 +
> drivers/input/joystick/adafruit-seesaw.c | 277 +++++++++++++++++++++++
> 4 files changed, 294 insertions(+)
> create mode 100644 drivers/input/joystick/adafruit-seesaw.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 81d5fc0bba68..cd4f9deb77e2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -441,6 +441,13 @@ W: http://wiki.analog.com/AD7879
> W: https://ez.analog.com/linux-software-drivers
> F: drivers/input/touchscreen/ad7879.c
>
> +ADAFRUIT MINI I2C GAMEPAD
> +M: Anshul Dalal <anshulusr@gmail.com>
> +L: linux-input@vger.kernel.org
> +S: Maintained
> +F: Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml
> +F: drivers/input/joystick/adafruit-seesaw.c
> +
> ADDRESS SPACE LAYOUT RANDOMIZATION (ASLR)
> M: Jiri Kosina <jikos@kernel.org>
> S: Maintained
> diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
> index ac6925ce8366..4d4fc38087bf 100644
> --- a/drivers/input/joystick/Kconfig
> +++ b/drivers/input/joystick/Kconfig
> @@ -412,4 +412,13 @@ config JOYSTICK_SENSEHAT
> To compile this driver as a module, choose M here: the
> module will be called sensehat_joystick.
>
> +config JOYSTICK_SEESAW
> + tristate "Adafruit Mini I2C Gamepad with Seesaw"
> + depends on I2C
> + help
> + Say Y here if you want to use the Adafruit Mini I2C Gamepad.
> +
> + To compile this driver as a module, choose M here: the module will be
> + called adafruit-seesaw.
> +
> endif
> diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile
> index 3937535f0098..fdc653209542 100644
> --- a/drivers/input/joystick/Makefile
> +++ b/drivers/input/joystick/Makefile
> @@ -28,6 +28,7 @@ obj-$(CONFIG_JOYSTICK_N64) += n64joy.o
> obj-$(CONFIG_JOYSTICK_PSXPAD_SPI) += psxpad-spi.o
> obj-$(CONFIG_JOYSTICK_PXRC) += pxrc.o
> obj-$(CONFIG_JOYSTICK_QWIIC) += qwiic-joystick.o
> +obj-$(CONFIG_JOYSTICK_SEESAW) += adafruit-seesaw.o
> obj-$(CONFIG_JOYSTICK_SENSEHAT) += sensehat-joystick.o
> obj-$(CONFIG_JOYSTICK_SIDEWINDER) += sidewinder.o
> obj-$(CONFIG_JOYSTICK_SPACEBALL) += spaceball.o
> diff --git a/drivers/input/joystick/adafruit-seesaw.c b/drivers/input/joystick/adafruit-seesaw.c
> new file mode 100644
> index 000000000000..1e8b3f5604d8
> --- /dev/null
> +++ b/drivers/input/joystick/adafruit-seesaw.c
> @@ -0,0 +1,277 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2023 Anshul Dalal <anshulusr@gmail.com>
> + *
> + * Driver for Adafruit Mini I2C Gamepad
> + *
> + * Based on the work of:
> + * Oleh Kravchenko (Sparkfun Qwiic Joystick driver)
> + *
> + * Product page: https://www.adafruit.com/product/5743
> + * Firmware and hardware sources: https://github.com/adafruit/Adafruit_Seesaw
> + */
> +
> +#include <linux/bits.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/input.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +
> +/* clang-format off */
> +#define SEESAW_DEVICE_NAME "seesaw-gamepad"
> +
> +#define SEESAW_STATUS_BASE 0
> +#define SEESAW_GPIO_BASE 1
> +#define SEESAW_ADC_BASE 9
> +
> +#define SEESAW_GPIO_DIRCLR_BULK 3
> +#define SEESAW_GPIO_BULK 4
> +#define SEESAW_GPIO_BULK_SET 5
> +#define SEESAW_GPIO_PULLENSET 11
> +
> +#define SEESAW_STATUS_HW_ID 1
> +#define SEESAW_STATUS_SWRST 127
> +
> +#define SEESAW_ADC_OFFSET 7
> +
> +#define BUTTON_A 5
> +#define BUTTON_B 1
> +#define BUTTON_X 6
> +#define BUTTON_Y 2
> +#define BUTTON_START 16
> +#define BUTTON_SELECT 0
> +
> +#define ANALOG_X 14
> +#define ANALOG_Y 15
> +
> +#define SEESAW_JOYSTICK_MAX_AXIS 1023
> +#define SEESAW_JOYSTICK_FUZZ 2
> +#define SEESAW_JOYSTICK_FLAT 4
> +
> +#define SEESAW_GAMEPAD_POLL_INTERVAL 16
> +#define SEESAW_GAMEPAD_POLL_MIN 8
> +#define SEESAW_GAMEPAD_POLL_MAX 32
> +/* clang-format on */
> +
> +u32 BUTTON_MASK = (1UL << BUTTON_A) | (1UL << BUTTON_B) | (1UL << BUTTON_X) |
> + (1UL << BUTTON_Y) | (1UL << BUTTON_START) |
> + (1UL << BUTTON_SELECT);
BIT(BUTTON_A), etc. Also below.
> +
> +struct seesaw_gamepad {
> + char physical_path[32];
> + unsigned char hardware_id;
This can be dropped.
> + struct input_dev *input_dev;
> + struct i2c_client *i2c_client;
> +};
> +
> +struct seesaw_data {
> + __be16 x;
> + __be16 y;
> + u8 button_a, button_b, button_x, button_y, button_start, button_select;
> +};
> +
> +static int seesaw_read_data(struct i2c_client *client, struct seesaw_data *data)
> +{
> + int err;
> + unsigned char write_buf[2] = { SEESAW_GPIO_BASE, SEESAW_GPIO_BULK };
> +
> + unsigned char read_buf[4];
> +
> + err = i2c_master_send(client, write_buf, sizeof(write_buf));
> + if (err < 0)
> + return err;
> + if (err != sizeof(write_buf))
> + return -EIO;
The return value of the i2c_master_ APIs should never be != the count
parameter.
So the addition sizeof() checks can go away.
> + err = i2c_master_recv(client, read_buf, sizeof(read_buf));
> + if (err < 0)
> + return err;
> + if (err != sizeof(read_buf))
> + return -EIO;
> + u32 result = ((u32)read_buf[0] << 24) | ((u32)read_buf[1] << 16) |
> + ((u32)read_buf[2] << 8) | (u32)read_buf[3];
get_unaligned_be32()
> + data->button_a = !(result & (1UL << BUTTON_A));
This should be able to use test_bit().
> + data->button_b = !(result & (1UL << BUTTON_B));
> + data->button_x = !(result & (1UL << BUTTON_X));
> + data->button_y = !(result & (1UL << BUTTON_Y));
> + data->button_start = !(result & (1UL << BUTTON_START));
> + data->button_select = !(result & (1UL << BUTTON_SELECT));
> +
> + write_buf[0] = SEESAW_ADC_BASE;
> + write_buf[1] = SEESAW_ADC_OFFSET + ANALOG_X;
> + err = i2c_master_send(client, write_buf, sizeof(write_buf));
> + if (err < 0)
> + return err;
> + if (err != sizeof(write_buf))
> + return -EIO;
> + err = i2c_master_recv(client, (char *)&data->x, sizeof(data->x));
> + if (err < 0)
> + return err;
> + if (err != sizeof(data->x))
> + return -EIO;
> + /*
> + * ADC reads left as max and right as 0, must be reversed since kernel
> + * expects reports in opposite order.
> + */
> + data->x = SEESAW_JOYSTICK_MAX_AXIS - be16_to_cpu(data->x);
> +
> + write_buf[1] = SEESAW_ADC_OFFSET + ANALOG_Y;
> + err = i2c_master_send(client, write_buf, sizeof(write_buf));
> + if (err < 0)
> + return err;
> + if (err != sizeof(write_buf))
> + return -EIO;
> + err = i2c_master_recv(client, (char *)&data->y, sizeof(data->y));
> + if (err < 0)
> + return err;
> + if (err != sizeof(data->y))
> + return -EIO;
> + data->y = be16_to_cpu(data->y);
> +
> + return 0;
> +}
> +
> +static void seesaw_poll(struct input_dev *input)
> +{
> + struct seesaw_gamepad *private = input_get_drvdata(input);
> + struct seesaw_data data;
> + int err;
> +
> + err = seesaw_read_data(private->i2c_client, &data);
> + if (err != 0)
> + return;
> +
> + input_report_abs(input, ABS_X, data.x);
> + input_report_abs(input, ABS_Y, data.y);
> + input_report_key(input, BTN_A, data.button_a);
> + input_report_key(input, BTN_B, data.button_b);
> + input_report_key(input, BTN_X, data.button_x);
> + input_report_key(input, BTN_Y, data.button_y);
> + input_report_key(input, BTN_START, data.button_start);
> + input_report_key(input, BTN_SELECT, data.button_select);
> + input_sync(input);
> +}
> +
> +static int seesaw_probe(struct i2c_client *client)
> +{
> + int err;
> + struct seesaw_gamepad *private;
> + unsigned char register_reset[] = { SEESAW_STATUS_BASE,
> + SEESAW_STATUS_SWRST, 0xFF };
> + unsigned char get_hw_id[] = { SEESAW_STATUS_BASE, SEESAW_STATUS_HW_ID };
> +
> + err = i2c_master_send(client, register_reset, sizeof(register_reset));
> + if (err < 0)
> + return err;
> + if (err != sizeof(register_reset))
> + return -EIO;
> +
> + /* Wait for the registers to reset before proceeding */
> + mdelay(10);
> +
> + private = devm_kzalloc(&client->dev, sizeof(*private), GFP_KERNEL);
> + if (!private)
> + return -ENOMEM;
> +
> + err = i2c_master_send(client, get_hw_id, sizeof(get_hw_id));
> + if (err < 0)
> + return err;
> + if (err != sizeof(get_hw_id))
> + return -EIO;
> + err = i2c_master_recv(client, &private->hardware_id, 1);
> + if (err < 0)
> + return err;
> + if (err != 1)
> + return -EIO;
> +
> + dev_dbg(&client->dev, "Adafruit Seesaw Gamepad, Hardware ID: %02x\n",
> + private->hardware_id);
> +
> + private->i2c_client = client;
> + scnprintf(private->physical_path, sizeof(private->physical_path),
> + "i2c/%s", dev_name(&client->dev));
> + i2c_set_clientdata(client, private);
> +
> + private->input_dev = devm_input_allocate_device(&client->dev);
> + if (!private->input_dev)
> + return -ENOMEM;
> +
> + private->input_dev->id.bustype = BUS_I2C;
> + private->input_dev->name = "Adafruit Seesaw Gamepad";
> + private->input_dev->phys = private->physical_path;
> + input_set_drvdata(private->input_dev, private);
> + input_set_abs_params(private->input_dev, ABS_X, 0,
> + SEESAW_JOYSTICK_MAX_AXIS, SEESAW_JOYSTICK_FUZZ,
> + SEESAW_JOYSTICK_FLAT);
> + input_set_abs_params(private->input_dev, ABS_Y, 0,
> + SEESAW_JOYSTICK_MAX_AXIS, SEESAW_JOYSTICK_FUZZ,
> + SEESAW_JOYSTICK_FLAT);
> + input_set_capability(private->input_dev, EV_KEY, BTN_A);
> + input_set_capability(private->input_dev, EV_KEY, BTN_B);
> + input_set_capability(private->input_dev, EV_KEY, BTN_X);
> + input_set_capability(private->input_dev, EV_KEY, BTN_Y);
> + input_set_capability(private->input_dev, EV_KEY, BTN_START);
> + input_set_capability(private->input_dev, EV_KEY, BTN_SELECT);
> +
> + err = input_setup_polling(private->input_dev, seesaw_poll);
> + if (err) {
> + dev_err(&client->dev, "failed to set up polling: %d\n", err);
> + return err;
> + }
> +
> + input_set_poll_interval(private->input_dev,
> + SEESAW_GAMEPAD_POLL_INTERVAL);
> + input_set_max_poll_interval(private->input_dev,
> + SEESAW_GAMEPAD_POLL_MAX);
> + input_set_min_poll_interval(private->input_dev,
> + SEESAW_GAMEPAD_POLL_MIN);
> +
> + err = input_register_device(private->input_dev);
> + if (err) {
> + dev_err(&client->dev, "failed to register joystick: %d\n", err);
> + return err;
> + }
> +
> + /* Set Pin Mode to input and enable pull-up resistors */
> + unsigned char pin_mode[] = { SEESAW_GPIO_BASE, SEESAW_GPIO_DIRCLR_BULK,
> + BUTTON_MASK >> 24, BUTTON_MASK >> 16,
> + BUTTON_MASK >> 8, BUTTON_MASK };
> + err = i2c_master_send(client, pin_mode, sizeof(pin_mode));
> + pin_mode[1] = SEESAW_GPIO_PULLENSET;
> + err |= i2c_master_send(client, pin_mode, sizeof(pin_mode));
This will mangle the errorcode if the calls to i2c_master_send() return
different errors.
> + pin_mode[1] = SEESAW_GPIO_BULK_SET;
> + err |= i2c_master_send(client, pin_mode, sizeof(pin_mode));
> + if (err < 0)
> + return err;
> + if (err != sizeof(pin_mode))
> + return -EIO;
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id of_seesaw_match[] = {
> + {
> + .compatible = "adafruit,seesaw-gamepad",
> + },
> + { /* Sentinel */ },
No comma after sentinel.
> +};
> +MODULE_DEVICE_TABLE(of, of_seesaw_match);
> +#endif /* CONFIG_OF */
> +
> +static const struct i2c_device_id seesaw_id_table[] = { { KBUILD_MODNAME, 0 },
> + { /* Sentinel */ } };
Borked formatting.
Also afaik KBUILD_MODNAME in id-tables is frowned upon.
Use the explicit string instead.
> +MODULE_DEVICE_TABLE(i2c, seesaw_id_table);
> +
> +static struct i2c_driver seesaw_driver = {
> + .driver = {
> + .name = SEESAW_DEVICE_NAME,
> + .of_match_table = of_match_ptr(of_seesaw_match),
> + },
> + .id_table = seesaw_id_table,
> + .probe = seesaw_probe,
> +};
> +module_i2c_driver(seesaw_driver);
> +
> +MODULE_AUTHOR("Anshul Dalal <anshulusr@gmail.com>");
> +MODULE_DESCRIPTION("Adafruit Mini I2C Gamepad driver");
> +MODULE_LICENSE("GPL");
> --
> 2.42.0
>
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: input: bindings for Adafruit Seesaw Gamepad
From: Rob Herring @ 2023-10-08 18:12 UTC (permalink / raw)
To: Anshul Dalal
Cc: Shuah Khan, Krzysztof Kozlowski, linux-input,
linux-kernel-mentees, devicetree, Dmitry Torokhov, Rob Herring,
Conor Dooley
In-Reply-To: <20231008172435.2391009-1-anshulusr@gmail.com>
On Sun, 08 Oct 2023 22:54:33 +0530, Anshul Dalal wrote:
> A simple driver for a mini gamepad that communicates over i2c, the gamepad
> has bidirectional thumb stick input and six buttons.
>
> The gamepad chip utilizes the open framework from Adafruit called 'Seesaw'
> to transmit the ADC data for the joystick and digital pin state for the
> buttons. I have only implemented the functionality required to receive the
> thumb stick and button state.
>
> Product page:
> https://www.adafruit.com/product/5743
> Arduino driver:
> https://github.com/adafruit/Adafruit_Seesaw
>
> For testing on a RPi Zero 2W, I utilized the following devicetree overlay:
>
> /dts-v1/;
> /plugin/;
> / {
> compatible = "brcm,bcm2835";
> fragment@0 {
> target = <&i2c1>;
> __overlay__ {
> #address-cells = <1>;
> #size-cells = <0>;
> joystick@50 {
> compatible = "adafruit,seesaw-gamepad";
> reg = <0x50>;
> };
> };
> };
> };
>
> I used the above overlay as reference for writing this binding. Though the
> gamepad also has an interrupt pin that needs to be enabled explicitly (not
> currently implemented in driver). The pin triggers a rising edge when a
> button is pressed or joystick is moved which can be detected on a GPIO
> of the Microcontroller.
>
> I wasn't sure how to represent that functionality in the binding so I have
> left it out for now.
>
> Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
> ---
>
> Changes for v2:
> - Renamed file to `adafruit,seesaw-gamepad.yaml`
> - Removed quotes for `$id` and `$schema`
> - Removed "Bindings for" from the description
> - Changed node name to the generic name "joystick"
> - Changed compatible to 'adafruit,seesaw-gamepad' instead of 'adafruit,seesaw_gamepad'
>
> .../input/adafruit,seesaw-gamepad.yaml | 51 +++++++++++++++++++
> 1 file changed, 51 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml: $id: Cannot determine base path from $id, relative path/filename doesn't match actual path or filename
$id: http://devicetree.org/schemas/input/adafruit_seesaw.yaml
file: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.example.dtb: joystick@50: 'reg' does not match any of the regexes: 'pinctrl-[0-9]+'
from schema $id: http://devicetree.org/schemas/input/adafruit_seesaw.yaml#
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20231008172435.2391009-1-anshulusr@gmail.com
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
^ permalink raw reply
* [PATCH v2 2/2] input: joystick: driver for Adafruit Seesaw Gamepad
From: Anshul Dalal @ 2023-10-08 17:24 UTC (permalink / raw)
To: linux-input, devicetree
Cc: Anshul Dalal, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Shuah Khan, linux-kernel-mentees
In-Reply-To: <20231008172435.2391009-1-anshulusr@gmail.com>
A simple driver for a mini gamepad that communicates over i2c, the gamepad
has bidirectional thumb stick input and six buttons.
The gamepad chip utilizes the open framework from Adafruit called 'Seesaw'
to transmit the ADC data for the joystick and digital pin state for the
buttons. I have only implemented the functionality required to receive the
thumb stick and button state.
Steps in reading the gamepad state over i2c:
1. Reset the registers
2. Set the pin mode of the pins specified by the `BUTTON_MASK` to input
`BUTTON_MASK`: A bit-map for the six digital pins internally
connected to the joystick buttons.
3. Enable internal pullup resistors for the `BUTTON_MASK`
4. Bulk set the pin state HIGH for `BUTTON_MASK`
5. Poll the device for button and joystick state done by:
`seesaw_read_data(struct i2c_client *client, struct seesaw_data *data)`
Product page:
https://www.adafruit.com/product/5743
Arduino driver:
https://github.com/adafruit/Adafruit_Seesaw
Driver tested on RPi Zero 2W
Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
---
Changes for v2:
adafruit-seesaw.c:
- Renamed file from 'adafruit_seesaw.c'
- Changed device name from 'seesaw_gamepad' to 'seesaw-gamepad'
- Changed count parameter for receiving joystick x on line 118:
`2` to `sizeof(write_buf)`
- Fixed invalid buffer size on line 123 and 126:
`data->y` to `sizeof(data->y)`
- Added comment for the `mdelay(10)` on line 169
- Changed inconsistent indentation on line 271
Kconfig:
- Fixed indentation for the help text
- Updated module name
Makefile:
- Updated module object file name
MAINTAINERS:
- Updated file name for the driver and bindings
MAINTAINERS | 7 +
drivers/input/joystick/Kconfig | 9 +
drivers/input/joystick/Makefile | 1 +
drivers/input/joystick/adafruit-seesaw.c | 277 +++++++++++++++++++++++
4 files changed, 294 insertions(+)
create mode 100644 drivers/input/joystick/adafruit-seesaw.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 81d5fc0bba68..cd4f9deb77e2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -441,6 +441,13 @@ W: http://wiki.analog.com/AD7879
W: https://ez.analog.com/linux-software-drivers
F: drivers/input/touchscreen/ad7879.c
+ADAFRUIT MINI I2C GAMEPAD
+M: Anshul Dalal <anshulusr@gmail.com>
+L: linux-input@vger.kernel.org
+S: Maintained
+F: Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml
+F: drivers/input/joystick/adafruit-seesaw.c
+
ADDRESS SPACE LAYOUT RANDOMIZATION (ASLR)
M: Jiri Kosina <jikos@kernel.org>
S: Maintained
diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
index ac6925ce8366..4d4fc38087bf 100644
--- a/drivers/input/joystick/Kconfig
+++ b/drivers/input/joystick/Kconfig
@@ -412,4 +412,13 @@ config JOYSTICK_SENSEHAT
To compile this driver as a module, choose M here: the
module will be called sensehat_joystick.
+config JOYSTICK_SEESAW
+ tristate "Adafruit Mini I2C Gamepad with Seesaw"
+ depends on I2C
+ help
+ Say Y here if you want to use the Adafruit Mini I2C Gamepad.
+
+ To compile this driver as a module, choose M here: the module will be
+ called adafruit-seesaw.
+
endif
diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile
index 3937535f0098..fdc653209542 100644
--- a/drivers/input/joystick/Makefile
+++ b/drivers/input/joystick/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_JOYSTICK_N64) += n64joy.o
obj-$(CONFIG_JOYSTICK_PSXPAD_SPI) += psxpad-spi.o
obj-$(CONFIG_JOYSTICK_PXRC) += pxrc.o
obj-$(CONFIG_JOYSTICK_QWIIC) += qwiic-joystick.o
+obj-$(CONFIG_JOYSTICK_SEESAW) += adafruit-seesaw.o
obj-$(CONFIG_JOYSTICK_SENSEHAT) += sensehat-joystick.o
obj-$(CONFIG_JOYSTICK_SIDEWINDER) += sidewinder.o
obj-$(CONFIG_JOYSTICK_SPACEBALL) += spaceball.o
diff --git a/drivers/input/joystick/adafruit-seesaw.c b/drivers/input/joystick/adafruit-seesaw.c
new file mode 100644
index 000000000000..1e8b3f5604d8
--- /dev/null
+++ b/drivers/input/joystick/adafruit-seesaw.c
@@ -0,0 +1,277 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2023 Anshul Dalal <anshulusr@gmail.com>
+ *
+ * Driver for Adafruit Mini I2C Gamepad
+ *
+ * Based on the work of:
+ * Oleh Kravchenko (Sparkfun Qwiic Joystick driver)
+ *
+ * Product page: https://www.adafruit.com/product/5743
+ * Firmware and hardware sources: https://github.com/adafruit/Adafruit_Seesaw
+ */
+
+#include <linux/bits.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/input.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+/* clang-format off */
+#define SEESAW_DEVICE_NAME "seesaw-gamepad"
+
+#define SEESAW_STATUS_BASE 0
+#define SEESAW_GPIO_BASE 1
+#define SEESAW_ADC_BASE 9
+
+#define SEESAW_GPIO_DIRCLR_BULK 3
+#define SEESAW_GPIO_BULK 4
+#define SEESAW_GPIO_BULK_SET 5
+#define SEESAW_GPIO_PULLENSET 11
+
+#define SEESAW_STATUS_HW_ID 1
+#define SEESAW_STATUS_SWRST 127
+
+#define SEESAW_ADC_OFFSET 7
+
+#define BUTTON_A 5
+#define BUTTON_B 1
+#define BUTTON_X 6
+#define BUTTON_Y 2
+#define BUTTON_START 16
+#define BUTTON_SELECT 0
+
+#define ANALOG_X 14
+#define ANALOG_Y 15
+
+#define SEESAW_JOYSTICK_MAX_AXIS 1023
+#define SEESAW_JOYSTICK_FUZZ 2
+#define SEESAW_JOYSTICK_FLAT 4
+
+#define SEESAW_GAMEPAD_POLL_INTERVAL 16
+#define SEESAW_GAMEPAD_POLL_MIN 8
+#define SEESAW_GAMEPAD_POLL_MAX 32
+/* clang-format on */
+
+u32 BUTTON_MASK = (1UL << BUTTON_A) | (1UL << BUTTON_B) | (1UL << BUTTON_X) |
+ (1UL << BUTTON_Y) | (1UL << BUTTON_START) |
+ (1UL << BUTTON_SELECT);
+
+struct seesaw_gamepad {
+ char physical_path[32];
+ unsigned char hardware_id;
+ struct input_dev *input_dev;
+ struct i2c_client *i2c_client;
+};
+
+struct seesaw_data {
+ __be16 x;
+ __be16 y;
+ u8 button_a, button_b, button_x, button_y, button_start, button_select;
+};
+
+static int seesaw_read_data(struct i2c_client *client, struct seesaw_data *data)
+{
+ int err;
+ unsigned char write_buf[2] = { SEESAW_GPIO_BASE, SEESAW_GPIO_BULK };
+ unsigned char read_buf[4];
+
+ err = i2c_master_send(client, write_buf, sizeof(write_buf));
+ if (err < 0)
+ return err;
+ if (err != sizeof(write_buf))
+ return -EIO;
+ err = i2c_master_recv(client, read_buf, sizeof(read_buf));
+ if (err < 0)
+ return err;
+ if (err != sizeof(read_buf))
+ return -EIO;
+ u32 result = ((u32)read_buf[0] << 24) | ((u32)read_buf[1] << 16) |
+ ((u32)read_buf[2] << 8) | (u32)read_buf[3];
+ data->button_a = !(result & (1UL << BUTTON_A));
+ data->button_b = !(result & (1UL << BUTTON_B));
+ data->button_x = !(result & (1UL << BUTTON_X));
+ data->button_y = !(result & (1UL << BUTTON_Y));
+ data->button_start = !(result & (1UL << BUTTON_START));
+ data->button_select = !(result & (1UL << BUTTON_SELECT));
+
+ write_buf[0] = SEESAW_ADC_BASE;
+ write_buf[1] = SEESAW_ADC_OFFSET + ANALOG_X;
+ err = i2c_master_send(client, write_buf, sizeof(write_buf));
+ if (err < 0)
+ return err;
+ if (err != sizeof(write_buf))
+ return -EIO;
+ err = i2c_master_recv(client, (char *)&data->x, sizeof(data->x));
+ if (err < 0)
+ return err;
+ if (err != sizeof(data->x))
+ return -EIO;
+ /*
+ * ADC reads left as max and right as 0, must be reversed since kernel
+ * expects reports in opposite order.
+ */
+ data->x = SEESAW_JOYSTICK_MAX_AXIS - be16_to_cpu(data->x);
+
+ write_buf[1] = SEESAW_ADC_OFFSET + ANALOG_Y;
+ err = i2c_master_send(client, write_buf, sizeof(write_buf));
+ if (err < 0)
+ return err;
+ if (err != sizeof(write_buf))
+ return -EIO;
+ err = i2c_master_recv(client, (char *)&data->y, sizeof(data->y));
+ if (err < 0)
+ return err;
+ if (err != sizeof(data->y))
+ return -EIO;
+ data->y = be16_to_cpu(data->y);
+
+ return 0;
+}
+
+static void seesaw_poll(struct input_dev *input)
+{
+ struct seesaw_gamepad *private = input_get_drvdata(input);
+ struct seesaw_data data;
+ int err;
+
+ err = seesaw_read_data(private->i2c_client, &data);
+ if (err != 0)
+ return;
+
+ input_report_abs(input, ABS_X, data.x);
+ input_report_abs(input, ABS_Y, data.y);
+ input_report_key(input, BTN_A, data.button_a);
+ input_report_key(input, BTN_B, data.button_b);
+ input_report_key(input, BTN_X, data.button_x);
+ input_report_key(input, BTN_Y, data.button_y);
+ input_report_key(input, BTN_START, data.button_start);
+ input_report_key(input, BTN_SELECT, data.button_select);
+ input_sync(input);
+}
+
+static int seesaw_probe(struct i2c_client *client)
+{
+ int err;
+ struct seesaw_gamepad *private;
+ unsigned char register_reset[] = { SEESAW_STATUS_BASE,
+ SEESAW_STATUS_SWRST, 0xFF };
+ unsigned char get_hw_id[] = { SEESAW_STATUS_BASE, SEESAW_STATUS_HW_ID };
+
+ err = i2c_master_send(client, register_reset, sizeof(register_reset));
+ if (err < 0)
+ return err;
+ if (err != sizeof(register_reset))
+ return -EIO;
+
+ /* Wait for the registers to reset before proceeding */
+ mdelay(10);
+
+ private = devm_kzalloc(&client->dev, sizeof(*private), GFP_KERNEL);
+ if (!private)
+ return -ENOMEM;
+
+ err = i2c_master_send(client, get_hw_id, sizeof(get_hw_id));
+ if (err < 0)
+ return err;
+ if (err != sizeof(get_hw_id))
+ return -EIO;
+ err = i2c_master_recv(client, &private->hardware_id, 1);
+ if (err < 0)
+ return err;
+ if (err != 1)
+ return -EIO;
+
+ dev_dbg(&client->dev, "Adafruit Seesaw Gamepad, Hardware ID: %02x\n",
+ private->hardware_id);
+
+ private->i2c_client = client;
+ scnprintf(private->physical_path, sizeof(private->physical_path),
+ "i2c/%s", dev_name(&client->dev));
+ i2c_set_clientdata(client, private);
+
+ private->input_dev = devm_input_allocate_device(&client->dev);
+ if (!private->input_dev)
+ return -ENOMEM;
+
+ private->input_dev->id.bustype = BUS_I2C;
+ private->input_dev->name = "Adafruit Seesaw Gamepad";
+ private->input_dev->phys = private->physical_path;
+ input_set_drvdata(private->input_dev, private);
+ input_set_abs_params(private->input_dev, ABS_X, 0,
+ SEESAW_JOYSTICK_MAX_AXIS, SEESAW_JOYSTICK_FUZZ,
+ SEESAW_JOYSTICK_FLAT);
+ input_set_abs_params(private->input_dev, ABS_Y, 0,
+ SEESAW_JOYSTICK_MAX_AXIS, SEESAW_JOYSTICK_FUZZ,
+ SEESAW_JOYSTICK_FLAT);
+ input_set_capability(private->input_dev, EV_KEY, BTN_A);
+ input_set_capability(private->input_dev, EV_KEY, BTN_B);
+ input_set_capability(private->input_dev, EV_KEY, BTN_X);
+ input_set_capability(private->input_dev, EV_KEY, BTN_Y);
+ input_set_capability(private->input_dev, EV_KEY, BTN_START);
+ input_set_capability(private->input_dev, EV_KEY, BTN_SELECT);
+
+ err = input_setup_polling(private->input_dev, seesaw_poll);
+ if (err) {
+ dev_err(&client->dev, "failed to set up polling: %d\n", err);
+ return err;
+ }
+
+ input_set_poll_interval(private->input_dev,
+ SEESAW_GAMEPAD_POLL_INTERVAL);
+ input_set_max_poll_interval(private->input_dev,
+ SEESAW_GAMEPAD_POLL_MAX);
+ input_set_min_poll_interval(private->input_dev,
+ SEESAW_GAMEPAD_POLL_MIN);
+
+ err = input_register_device(private->input_dev);
+ if (err) {
+ dev_err(&client->dev, "failed to register joystick: %d\n", err);
+ return err;
+ }
+
+ /* Set Pin Mode to input and enable pull-up resistors */
+ unsigned char pin_mode[] = { SEESAW_GPIO_BASE, SEESAW_GPIO_DIRCLR_BULK,
+ BUTTON_MASK >> 24, BUTTON_MASK >> 16,
+ BUTTON_MASK >> 8, BUTTON_MASK };
+ err = i2c_master_send(client, pin_mode, sizeof(pin_mode));
+ pin_mode[1] = SEESAW_GPIO_PULLENSET;
+ err |= i2c_master_send(client, pin_mode, sizeof(pin_mode));
+ pin_mode[1] = SEESAW_GPIO_BULK_SET;
+ err |= i2c_master_send(client, pin_mode, sizeof(pin_mode));
+ if (err < 0)
+ return err;
+ if (err != sizeof(pin_mode))
+ return -EIO;
+
+ return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id of_seesaw_match[] = {
+ {
+ .compatible = "adafruit,seesaw-gamepad",
+ },
+ { /* Sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, of_seesaw_match);
+#endif /* CONFIG_OF */
+
+static const struct i2c_device_id seesaw_id_table[] = { { KBUILD_MODNAME, 0 },
+ { /* Sentinel */ } };
+MODULE_DEVICE_TABLE(i2c, seesaw_id_table);
+
+static struct i2c_driver seesaw_driver = {
+ .driver = {
+ .name = SEESAW_DEVICE_NAME,
+ .of_match_table = of_match_ptr(of_seesaw_match),
+ },
+ .id_table = seesaw_id_table,
+ .probe = seesaw_probe,
+};
+module_i2c_driver(seesaw_driver);
+
+MODULE_AUTHOR("Anshul Dalal <anshulusr@gmail.com>");
+MODULE_DESCRIPTION("Adafruit Mini I2C Gamepad driver");
+MODULE_LICENSE("GPL");
--
2.42.0
^ permalink raw reply related
* [PATCH v2 1/2] dt-bindings: input: bindings for Adafruit Seesaw Gamepad
From: Anshul Dalal @ 2023-10-08 17:24 UTC (permalink / raw)
To: linux-input, devicetree
Cc: Anshul Dalal, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Shuah Khan, linux-kernel-mentees
A simple driver for a mini gamepad that communicates over i2c, the gamepad
has bidirectional thumb stick input and six buttons.
The gamepad chip utilizes the open framework from Adafruit called 'Seesaw'
to transmit the ADC data for the joystick and digital pin state for the
buttons. I have only implemented the functionality required to receive the
thumb stick and button state.
Product page:
https://www.adafruit.com/product/5743
Arduino driver:
https://github.com/adafruit/Adafruit_Seesaw
For testing on a RPi Zero 2W, I utilized the following devicetree overlay:
/dts-v1/;
/plugin/;
/ {
compatible = "brcm,bcm2835";
fragment@0 {
target = <&i2c1>;
__overlay__ {
#address-cells = <1>;
#size-cells = <0>;
joystick@50 {
compatible = "adafruit,seesaw-gamepad";
reg = <0x50>;
};
};
};
};
I used the above overlay as reference for writing this binding. Though the
gamepad also has an interrupt pin that needs to be enabled explicitly (not
currently implemented in driver). The pin triggers a rising edge when a
button is pressed or joystick is moved which can be detected on a GPIO
of the Microcontroller.
I wasn't sure how to represent that functionality in the binding so I have
left it out for now.
Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
---
Changes for v2:
- Renamed file to `adafruit,seesaw-gamepad.yaml`
- Removed quotes for `$id` and `$schema`
- Removed "Bindings for" from the description
- Changed node name to the generic name "joystick"
- Changed compatible to 'adafruit,seesaw-gamepad' instead of 'adafruit,seesaw_gamepad'
.../input/adafruit,seesaw-gamepad.yaml | 51 +++++++++++++++++++
1 file changed, 51 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml
diff --git a/Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml b/Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml
new file mode 100644
index 000000000000..af34f789da1c
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml
@@ -0,0 +1,51 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/adafruit_seesaw.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Adafruit Mini I2C Gamepad with seesaw
+
+maintainers:
+ - Anshul Dalal <anshulusr@gmail.com>
+
+description: |
+ Adafruit Mini I2C Gamepad
+
+ +-----------------------------+
+ | ___ |
+ | / \ (X) |
+ | | S | __ __ (Y) (A) |
+ | \___/ |ST| |SE| (B) |
+ | |
+ +-----------------------------+
+
+ S -> 10-bit percision bidirectional analog joystick
+ ST -> Start
+ SE -> Select
+ X, A, B, Y -> Digital action buttons
+
+ Product page: https://www.adafruit.com/product/5743
+ Arduino Driver: https://github.com/adafruit/Adafruit_Seesaw
+
+properties:
+ compatible:
+ const: adafruit,seesaw-gamepad
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ joystick@50 {
+ compatible = "adafruit,seesaw-gamepad";
+ reg = <0x50>;
+ };
+ };
--
2.42.0
^ permalink raw reply related
* Re: [PATCH 2/2] HID: uclogic: Fix a work->entry not empty bug in __queue_work()
From: José Expósito @ 2023-10-08 17:21 UTC (permalink / raw)
To: Jinjie Ruan; +Cc: jikos, benjamin.tissoires, linux-input
In-Reply-To: <20230921133824.605700-3-ruanjinjie@huawei.com>
On Thu, Sep 21, 2023 at 09:38:24PM +0800, Jinjie Ruan wrote:
> When CONFIG_HID_UCLOGIC=y and CONFIG_KUNIT_ALL_TESTS=y, launch
> kernel and then the below work->entry not empty bug occurs.
>
> In hid_test_uclogic_exec_event_hook_test(), the filter->work is not
> initialized to be added to p.event_hooks->list, and then the
> schedule_work() in uclogic_exec_event_hook() will call __queue_work(),
> which check whether the work->entry is empty and cause the below
> warning call trace.
>
> So call INIT_WORK() with a fake work to solve the issue. After applying
> this patch, the below work->entry not empty bug never occurs.
>
> WARNING: CPU: 0 PID: 2177 at kernel/workqueue.c:1787 __queue_work.part.0+0x780/0xad0
> Modules linked in:
> CPU: 0 PID: 2177 Comm: kunit_try_catch Tainted: G B W N 6.6.0-rc2+ #30
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
> RIP: 0010:__queue_work.part.0+0x780/0xad0
> Code: 44 24 20 0f b6 00 84 c0 74 08 3c 03 0f 8e 52 03 00 00 f6 83 00 01 00 00 02 74 6f 4c 89 ef e8 c7 d8 f1 02 f3 90 e9 e5 f8 ff ff <0f> 0b e9 63 fc ff ff 89 e9 49 8d 57 68 4c 89 e6 4c 89 ff 83 c9 02
> RSP: 0000:ffff888102bb7ce8 EFLAGS: 00010086
> RAX: 0000000000000000 RBX: ffff888106b8e460 RCX: ffffffff84141cc7
> RDX: 1ffff11020d71c8c RSI: 0000000000000004 RDI: ffff8881001d0118
> RBP: dffffc0000000000 R08: 0000000000000001 R09: ffffed1020576f92
> R10: 0000000000000003 R11: ffff888102bb7980 R12: ffff888106b8e458
> R13: ffff888119c38800 R14: 0000000000000000 R15: ffff8881001d0100
> FS: 0000000000000000(0000) GS:ffff888119c00000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: ffff888119506000 CR3: 0000000005286001 CR4: 0000000000770ef0
> DR0: ffffffff8fdd6ce0 DR1: ffffffff8fdd6ce1 DR2: ffffffff8fdd6ce3
> DR3: ffffffff8fdd6ce5 DR6: 00000000fffe0ff0 DR7: 0000000000000600
> PKRU: 55555554
> Call Trace:
> <TASK>
> ? __warn+0xc9/0x260
> ? __queue_work.part.0+0x780/0xad0
> ? report_bug+0x345/0x400
> ? handle_bug+0x3c/0x70
> ? exc_invalid_op+0x14/0x40
> ? asm_exc_invalid_op+0x16/0x20
> ? _raw_spin_lock+0x87/0xe0
> ? __queue_work.part.0+0x780/0xad0
> ? __queue_work.part.0+0x249/0xad0
> queue_work_on+0x48/0x50
> uclogic_exec_event_hook.isra.0+0xf7/0x160
> hid_test_uclogic_exec_event_hook_test+0x2f1/0x5d0
> ? try_to_wake_up+0x151/0x13e0
> ? uclogic_exec_event_hook.isra.0+0x160/0x160
> ? _raw_spin_lock_irqsave+0x8d/0xe0
> ? __sched_text_end+0xa/0xa
> ? __sched_text_end+0xa/0xa
> ? migrate_enable+0x260/0x260
> ? kunit_try_run_case_cleanup+0xe0/0xe0
> kunit_generic_run_threadfn_adapter+0x4a/0x90
> ? kunit_try_catch_throw+0x80/0x80
> kthread+0x2b5/0x380
> ? kthread_complete_and_exit+0x20/0x20
> ret_from_fork+0x2d/0x70
> ? kthread_complete_and_exit+0x20/0x20
> ret_from_fork_asm+0x11/0x20
> </TASK>
>
> Fixes: a251d6576d2a ("HID: uclogic: Handle wireless device reconnection")
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
Reviewed-by: José Expósito <jose.exposito89@gmail.com>
> ---
> drivers/hid/hid-uclogic-core-test.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/hid/hid-uclogic-core-test.c b/drivers/hid/hid-uclogic-core-test.c
> index 2bb916226a38..cb274cde3ad2 100644
> --- a/drivers/hid/hid-uclogic-core-test.c
> +++ b/drivers/hid/hid-uclogic-core-test.c
> @@ -56,6 +56,11 @@ static struct uclogic_raw_event_hook_test test_events[] = {
> },
> };
>
> +static void fake_work(struct work_struct *work)
> +{
> +
> +}
> +
> static void hid_test_uclogic_exec_event_hook_test(struct kunit *test)
> {
> struct uclogic_params p = {0, };
> @@ -77,6 +82,8 @@ static void hid_test_uclogic_exec_event_hook_test(struct kunit *test)
> KUNIT_ASSERT_NOT_ERR_OR_NULL(test, filter->event);
> memcpy(filter->event, &hook_events[n].event[0], filter->size);
>
> + INIT_WORK(&filter->work, fake_work);
> +
> list_add_tail(&filter->list, &p.event_hooks->list);
> }
>
> --
> 2.34.1
>
^ permalink raw reply
* Re: [PATCH 1/2] HID: uclogic: Fix user-memory-access bug in uclogic_params_ugee_v2_init_event_hooks()
From: José Expósito @ 2023-10-08 17:21 UTC (permalink / raw)
To: Jinjie Ruan; +Cc: jikos, benjamin.tissoires, linux-input
In-Reply-To: <20230921133824.605700-2-ruanjinjie@huawei.com>
Hi Jinjie Ruan,
Thanks a lot for finding and fixing this bug.
On Thu, Sep 21, 2023 at 09:38:23PM +0800, Jinjie Ruan wrote:
> When CONFIG_HID_UCLOGIC=y and CONFIG_KUNIT_ALL_TESTS=y, launch kernel and
> then the below user-memory-access bug occurs.
>
> In hid_test_uclogic_params_cleanup_event_hooks(),it call
> uclogic_params_ugee_v2_init_event_hooks() with the first arg=NULL, so
> when it calls uclogic_params_ugee_v2_has_battery(), the hid_get_drvdata()
> will access hdev->dev with hdev=NULL, which will cause below
> user-memory-access.
>
> So add a fake_device with quirks member and call hid_set_drvdata()
> to assign hdev->dev->driver_data which avoids the null-ptr-def bug
> for drvdata->quirks in uclogic_params_ugee_v2_has_battery(). After applying
> this patch, the below user-memory-access bug never occurs.
>
> general protection fault, probably for non-canonical address 0xdffffc0000000329: 0000 [#1] PREEMPT SMP KASAN
> KASAN: probably user-memory-access in range [0x0000000000001948-0x000000000000194f]
> CPU: 5 PID: 2189 Comm: kunit_try_catch Tainted: G B W N 6.6.0-rc2+ #30
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
> RIP: 0010:uclogic_params_ugee_v2_init_event_hooks+0x87/0x600
> Code: f3 f3 65 48 8b 14 25 28 00 00 00 48 89 54 24 60 31 d2 48 89 fa c7 44 24 30 00 00 00 00 48 c7 44 24 28 02 f8 02 01 48 c1 ea 03 <80> 3c 02 00 0f 85 2c 04 00 00 48 8b 9d 48 19 00 00 48 b8 00 00 00
> RSP: 0000:ffff88810679fc88 EFLAGS: 00010202
> RAX: dffffc0000000000 RBX: 0000000000000004 RCX: 0000000000000000
> RDX: 0000000000000329 RSI: ffff88810679fd88 RDI: 0000000000001948
> RBP: 0000000000000000 R08: 0000000000000000 R09: ffffed1020f639f0
> R10: ffff888107b1cf87 R11: 0000000000000400 R12: 1ffff11020cf3f92
> R13: ffff88810679fd88 R14: ffff888100b97b08 R15: ffff8881030bb080
> FS: 0000000000000000(0000) GS:ffff888119e80000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000000 CR3: 0000000005286001 CR4: 0000000000770ee0
> DR0: ffffffff8fdd6cf4 DR1: ffffffff8fdd6cf5 DR2: ffffffff8fdd6cf6
> DR3: ffffffff8fdd6cf7 DR6: 00000000fffe0ff0 DR7: 0000000000000600
> PKRU: 55555554
> Call Trace:
> <TASK>
> ? die_addr+0x3d/0xa0
> ? exc_general_protection+0x144/0x220
> ? asm_exc_general_protection+0x22/0x30
> ? uclogic_params_ugee_v2_init_event_hooks+0x87/0x600
> ? sched_clock_cpu+0x69/0x550
> ? uclogic_parse_ugee_v2_desc_gen_params+0x70/0x70
> ? load_balance+0x2950/0x2950
> ? rcu_trc_cmpxchg_need_qs+0x67/0xa0
> hid_test_uclogic_params_cleanup_event_hooks+0x9e/0x1a0
> ? uclogic_params_ugee_v2_init_event_hooks+0x600/0x600
> ? __switch_to+0x5cf/0xe60
> ? migrate_enable+0x260/0x260
> ? __kthread_parkme+0x83/0x150
> ? kunit_try_run_case_cleanup+0xe0/0xe0
> kunit_generic_run_threadfn_adapter+0x4a/0x90
> ? kunit_try_catch_throw+0x80/0x80
> kthread+0x2b5/0x380
> ? kthread_complete_and_exit+0x20/0x20
> ret_from_fork+0x2d/0x70
> ? kthread_complete_and_exit+0x20/0x20
> ret_from_fork_asm+0x11/0x20
> </TASK>
> Modules linked in:
> Dumping ftrace buffer:
> (ftrace buffer empty)
> ---[ end trace 0000000000000000 ]---
> RIP: 0010:uclogic_params_ugee_v2_init_event_hooks+0x87/0x600
> Code: f3 f3 65 48 8b 14 25 28 00 00 00 48 89 54 24 60 31 d2 48 89 fa c7 44 24 30 00 00 00 00 48 c7 44 24 28 02 f8 02 01 48 c1 ea 03 <80> 3c 02 00 0f 85 2c 04 00 00 48 8b 9d 48 19 00 00 48 b8 00 00 00
> RSP: 0000:ffff88810679fc88 EFLAGS: 00010202
> RAX: dffffc0000000000 RBX: 0000000000000004 RCX: 0000000000000000
> RDX: 0000000000000329 RSI: ffff88810679fd88 RDI: 0000000000001948
> RBP: 0000000000000000 R08: 0000000000000000 R09: ffffed1020f639f0
> R10: ffff888107b1cf87 R11: 0000000000000400 R12: 1ffff11020cf3f92
> R13: ffff88810679fd88 R14: ffff888100b97b08 R15: ffff8881030bb080
> FS: 0000000000000000(0000) GS:ffff888119e80000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000000 CR3: 0000000005286001 CR4: 0000000000770ee0
> DR0: ffffffff8fdd6cf4 DR1: ffffffff8fdd6cf5 DR2: ffffffff8fdd6cf6
> DR3: ffffffff8fdd6cf7 DR6: 00000000fffe0ff0 DR7: 0000000000000600
> PKRU: 55555554
> Kernel panic - not syncing: Fatal exception
> Dumping ftrace buffer:
> (ftrace buffer empty)
> Kernel Offset: disabled
> Rebooting in 1 seconds..
>
> Fixes: a251d6576d2a ("HID: uclogic: Handle wireless device reconnection")
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> ---
> drivers/hid/hid-uclogic-params-test.c | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-uclogic-params-test.c b/drivers/hid/hid-uclogic-params-test.c
> index 678f50cbb160..3938bae25982 100644
> --- a/drivers/hid/hid-uclogic-params-test.c
> +++ b/drivers/hid/hid-uclogic-params-test.c
> @@ -174,12 +174,22 @@ static void hid_test_uclogic_parse_ugee_v2_desc(struct kunit *test)
> KUNIT_EXPECT_EQ(test, params->frame_type, frame_type);
> }
>
> +struct fake_device {
> + unsigned long quirks;
> +};
> +
> static void hid_test_uclogic_params_cleanup_event_hooks(struct kunit *test)
> {
> int res, n;
> + struct hid_device *hdev;
> + struct fake_device *fake_dev;
> struct uclogic_params p = {0, };
>
> - res = uclogic_params_ugee_v2_init_event_hooks(NULL, &p);
> + hdev = kzalloc(sizeof(struct hid_device), GFP_KERNEL);
> + fake_dev = kzalloc(sizeof(struct fake_device), GFP_KERNEL);
Intead of using `kzalloc()` to allocate memory for `hdev` and `fake_dev`
we should use `kunit_kzalloc()`.
It has 2 main advatages:
- If an assertion fails, the memory is freed
- No need for `kfree()`
> + hid_set_drvdata(hdev, fake_dev);
> +
> + res = uclogic_params_ugee_v2_init_event_hooks(hdev, &p);
> KUNIT_ASSERT_EQ(test, res, 0);
>
> /* Check that the function can be called repeatedly */
> @@ -187,6 +197,9 @@ static void hid_test_uclogic_params_cleanup_event_hooks(struct kunit *test)
> uclogic_params_cleanup_event_hooks(&p);
> KUNIT_EXPECT_PTR_EQ(test, p.event_hooks, NULL);
> }
> +
> + kfree(fake_dev);
> + kfree(hdev);
This 2 lines can be removed if `kunit_kzalloc()` is used.
> }
>
> static struct kunit_case hid_uclogic_params_test_cases[] = {
> --
> 2.34.1
>
Once the `kunit_kzalloc()` change is appliyed:
Reviewed-by: José Expósito <jose.exposito89@gmail.com>
^ permalink raw reply
* Re: [PATCH v2 00/14] HID: logitech-hidpp: Avoid hidpp_connect_event() running while probe() restarts IO
From: Hans de Goede @ 2023-10-08 10:30 UTC (permalink / raw)
To: Hans de Goede, Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires
Cc: linux-input
In-Reply-To: <20231008095458.8926-1-hdegoede@redhat.com>
Hi,
On 10/8/23 11:54, Hans de Goede wrote:
> Hi Benjamin,
>
> Here is a v2 of my series to fix issues with hidpp_connect_event() running
> while restarting IO, which now also fixes the issues you mentioned with
> potentially missing connect events.
>
> This series is best explained by a brief sketch of how probe()
> looks at the end of the series (1):
>
> Prep work:
>
> 1. All code depending on a device being in connected state is moved to
> the hidpp_connect_event() workqueue item
>
> 2. hidpp_connect_event() now checks the connected state itself by
> checking that hidpp_root_get_protocol_version() succeeds, instead
> of relying on possibly stale (racy) data in struct hidpp_device
>
> With this in place the new probe() sequence looks like this:
>
> 1. enable_connect_event flag starts at 0, this filters out / ignores any
> connect-events in hidpp_raw_hidpp_event() avoiding
> hidpp_connect_event() getting queued before the IO restart
>
> 2. IO is started with connect-mask = 0
> this avoids hid-input and hidraw connecting while probe() is setting
> hdev->name and hdev->uniq
>
> 3. Name and serialnr are retrieved and stored in hdev
>
> 4. IO is fully restarted (including hw_open + io_start, not just hw_start)
> with the actual connect-mask.
>
> 5. enable_connect_event atomic_t is set to 1 to enable processing of
> connect events.
>
> 6. hidpp_connect_event() is queued + flushed to query the connected state
> and do the connect work if the device is connected.
>
> 7. probe() now ends with:
>
> /*
> * This relies on logi_dj_ll_close() being a no-op so that
> * DJ connection events will still be received.
> */
> hid_hw_close(hdev);
>
> Since on restarting IO it now is fully restarted so the hid_hw_open()
> there needs to be balanced.
>
> This series now obviously is no longer 6.6 material, instead I hope we
> can get this rework (and IMHO nice cleanup) into 6.7 .
>
> Regards,
>
> Hans
I forgot to add info on the list of devices I tested this on:
Logitech Bluetooth Laser Travel Mouse (bluetooth, HID++ 1.0)
Logitech M720 Triathlon (bluetooth, HID++ 4.5)
Logitech K400 Pro (unifying, HID++ 4.1)
Logitech K270 (eQUAD nano Lite, HID++ 2.0)
Logitech M185 (eQUAD nano Lite, HID++ 4.5)
Logitech Keyboard LX501 (27 Mhz, HID++ builtin scroll-wheel, HID++ 1.0)
Logitech 27Mhz mouse (27 Mhz, HID++ extra mouse buttons, HID++ 1.0)
Regards,
Hans
> 1) For reviewing you may also want to apply the entire series and look
> at the end result to help you understand why various intermediate steps
> are taken.
>
>
> Hans de Goede (14):
> HID: logitech-hidpp: Revert "Don't restart communication if not
> necessary"
> HID: logitech-hidpp: Move hidpp_overwrite_name() to before connect
> check
> HID: logitech-hidpp: Add hidpp_non_unifying_init() helper
> HID: logitech-hidpp: Remove connected check for non-unifying devices
> HID: logitech-hidpp: Move get_wireless_feature_index() check to
> hidpp_connect_event()
> HID: logitech-hidpp: Remove wtp_get_config() call from probe()
> HID: logitech-hidpp: Remove connected check for g920_get_config() call
> HID: logitech-hidpp: Add a hidpp_connect_and_start() helper
> HID: logitech-hidpp: Move the connected check to after restarting IO
> HID: logitech-hidpp: Move g920_get_config() to just before
> hidpp_ff_init()
> HID: logitech-hidpp: Remove unused connected param from *_connect()
> HID: logitech-hidpp: Fix connect event race
> HID: logitech-hidpp: Avoid hidpp_connect_event() running while probe()
> restarts IO
> HID: logitech-hidpp: Drop delayed_work_cb()
>
> drivers/hid/hid-logitech-hidpp.c | 211 +++++++++++++------------------
> 1 file changed, 91 insertions(+), 120 deletions(-)
>
^ permalink raw reply
* Re: [PATCH 2/2] input: add Adafruit Seesaw Gamepad driver
From: Krzysztof Kozlowski @ 2023-10-08 10:25 UTC (permalink / raw)
To: Anshul Dalal, linux-input, devicetree
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Shuah Khan, linux-kernel-mentees
In-Reply-To: <2281f924-30d1-41c5-b105-d8d28aada5b2@gmail.com>
On 07/10/2023 18:13, Anshul Dalal wrote:
>>> + err = i2c_master_send(client, register_reset, sizeof(register_reset));
>>> + if (err < 0)
>>> + return err;
>>> + if (err != sizeof(register_reset))
>>> + return -EIO;
>>> + mdelay(10);
>>
>> Why 10? This should be explained somehow in the code.
>
> The reason for the delay is to ensure the register reset process is
> finished before going further. The reference Arduino driver from the
> manufacturer also had a delay for the same amount though my hardware
> unit worked fine till 5ms delay. Is there some kernel abstraction I
> could use to indicate a short delay or will the previous explanation in
> a comment above the line do?
>
Just explain it briefly as a comment.
Best regards,
Krzysztof
^ permalink raw reply
* [PATCH v2 13/14] HID: logitech-hidpp: Avoid hidpp_connect_event() running while probe() restarts IO
From: Hans de Goede @ 2023-10-08 9:54 UTC (permalink / raw)
To: Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires
Cc: Hans de Goede, linux-input
In-Reply-To: <20231008095458.8926-1-hdegoede@redhat.com>
hidpp_probe() restarts IO after setting things up, if we get a connect
event just before hidpp_probe() stops all IO then hidpp_connect_event()
will be running from the workqueue and it will see IO errors.
To avoid this add an enable_connect_event flag and only set that
after the IO restart is done.
Since an hidpp_connect_event() run is queued after restarting IO
anyways, connect events before that run can safely be ignored.
Note this also means that hidpp_connect_event() is now guaranteed to
not get queued on hidpp_connect_and_start() failures so the work
for this no longer needs to be cancelled on failure.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Use an enable_connect_event flag instead of a mutex
---
drivers/hid/hid-logitech-hidpp.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 37213dcc9d9c..9e34a29619a0 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -192,6 +192,7 @@ struct hidpp_device {
void *private_data;
+ atomic_t enable_connect_event;
struct work_struct work;
struct kfifo delayed_work_fifo;
struct input_dev *delayed_input;
@@ -3892,6 +3893,9 @@ static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data,
}
if (unlikely(hidpp_report_is_connect_event(hidpp, report))) {
+ if (!atomic_read(&hidpp->enable_connect_event))
+ return 1;
+
if (schedule_work(&hidpp->work) == 0)
dbg_hid("%s: connect event already queued\n", __func__);
return 1;
@@ -4501,6 +4505,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
* again (which may cause missing packets) queue hidpp_connect_event()
* to check the connected state of the device.
*/
+ atomic_set(&hidpp->enable_connect_event, 1);
schedule_work(&hidpp->work);
flush_work(&hidpp->work);
@@ -4526,7 +4531,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
hid_hw_start_fail:
sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
- cancel_work_sync(&hidpp->work);
mutex_destroy(&hidpp->send_mutex);
return ret;
}
--
2.41.0
^ permalink raw reply related
* [PATCH v2 14/14] HID: logitech-hidpp: Drop delayed_work_cb()
From: Hans de Goede @ 2023-10-08 9:54 UTC (permalink / raw)
To: Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires
Cc: Hans de Goede, linux-input
In-Reply-To: <20231008095458.8926-1-hdegoede@redhat.com>
Drop delayed_work_cb() instead make hidpp_connect_event() the workqueue
function itself.
Besides resulting in a small cleanup this will hopefully also make
it clearer that going forward hidpp_connect_event() should only
be run from a workqueue and not be directly invoked.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Link: https://lore.kernel.org/r/20231006081858.17677-3-hdegoede@redhat.com
---
drivers/hid/hid-logitech-hidpp.c | 14 +++-----------
1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 9e34a29619a0..547544a5649b 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -235,8 +235,6 @@ struct hidpp_device {
#define HIDPP20_ERROR_UNSUPPORTED 0x09
#define HIDPP20_ERROR 0xff
-static void hidpp_connect_event(struct hidpp_device *hidpp_dev);
-
static int __hidpp_send_report(struct hid_device *hdev,
struct hidpp_report *hidpp_report)
{
@@ -450,13 +448,6 @@ static int hidpp_send_rap_command_sync(struct hidpp_device *hidpp_dev,
return ret;
}
-static void delayed_work_cb(struct work_struct *work)
-{
- struct hidpp_device *hidpp = container_of(work, struct hidpp_device,
- work);
- hidpp_connect_event(hidpp);
-}
-
static inline bool hidpp_match_answer(struct hidpp_report *question,
struct hidpp_report *answer)
{
@@ -4187,8 +4178,9 @@ static struct input_dev *hidpp_allocate_input(struct hid_device *hdev)
return input_dev;
}
-static void hidpp_connect_event(struct hidpp_device *hidpp)
+static void hidpp_connect_event(struct work_struct *work)
{
+ struct hidpp_device *hidpp = container_of(work, struct hidpp_device, work);
struct hid_device *hdev = hidpp->hid_dev;
struct input_dev *input;
char *name, *devm_name;
@@ -4461,7 +4453,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
return ret;
}
- INIT_WORK(&hidpp->work, delayed_work_cb);
+ INIT_WORK(&hidpp->work, hidpp_connect_event);
mutex_init(&hidpp->send_mutex);
init_waitqueue_head(&hidpp->wait);
--
2.41.0
^ permalink raw reply related
* [PATCH v2 03/14] HID: logitech-hidpp: Add hidpp_non_unifying_init() helper
From: Hans de Goede @ 2023-10-08 9:54 UTC (permalink / raw)
To: Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires
Cc: Hans de Goede, linux-input
In-Reply-To: <20231008095458.8926-1-hdegoede@redhat.com>
Turn hidpp_overwrite_name() into a hidpp_non_unifying_init() helper
which takes care of setting both the name and the serial for non
unifying devices.
This mirrors the hidpp_unifying_init() helper for unifying devices.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/hid/hid-logitech-hidpp.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index b1965b91c5bb..e4dbbdf46b97 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -4131,11 +4131,16 @@ static int hidpp_initialize_battery(struct hidpp_device *hidpp)
return ret;
}
-static void hidpp_overwrite_name(struct hid_device *hdev)
+/* Get name + serial for USB and Bluetooth HID++ devices */
+static void hidpp_non_unifying_init(struct hidpp_device *hidpp)
{
- struct hidpp_device *hidpp = hid_get_drvdata(hdev);
+ struct hid_device *hdev = hidpp->hid_dev;
char *name;
+ /* Bluetooth devices already have their serialnr set */
+ if (hid_is_usb(hdev))
+ hidpp_serial_init(hidpp);
+
name = hidpp_get_device_name(hidpp);
if (name) {
dbg_hid("HID++: Got name: %s\n", name);
@@ -4467,14 +4472,11 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
/* Allow incoming packets */
hid_device_io_start(hdev);
+ /* Get name + serial, store in hdev->name + hdev->uniq */
if (hidpp->quirks & HIDPP_QUIRK_UNIFYING)
hidpp_unifying_init(hidpp);
- else {
- if (hid_is_usb(hidpp->hid_dev))
- hidpp_serial_init(hidpp);
-
- hidpp_overwrite_name(hdev);
- }
+ else
+ hidpp_non_unifying_init(hidpp);
connected = hidpp_root_get_protocol_version(hidpp) == 0;
atomic_set(&hidpp->connected, connected);
--
2.41.0
^ permalink raw reply related
* [PATCH v2 09/14] HID: logitech-hidpp: Move the connected check to after restarting IO
From: Hans de Goede @ 2023-10-08 9:54 UTC (permalink / raw)
To: Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires
Cc: Hans de Goede, linux-input, Benjamin Tissoires
In-Reply-To: <20231008095458.8926-1-hdegoede@redhat.com>
While restarting IO incoming packets get disabled by calling
hid_device_io_stop() and they do not get re-enabled again until
the hid-core enables the when probe() is done.
This leaves a window where connect events may get lost, causing
hidpp to not be aware that a device has (dis)connected.
To fix this fully restart IO using the new hidpp_connect_and_start()
helper and move the connected check to after restarting IO.
This requires calling hid_hw_close() at the end of probe() to balance
the open() now done on restart.
Reported-by: Benjamin Tissoires <bentiss@kernel.org>
Closes: https://lore.kernel.org/linux-input/zjiang3fdy4o7r3daupwpnx6zesmeeerldpx5fno2adzialpre@cdp7tq4araww/
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/hid/hid-logitech-hidpp.c | 33 ++++++++++++++++++++------------
1 file changed, 21 insertions(+), 12 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 45b371e7b9ee..ff834f905eda 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -4482,8 +4482,10 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
hdev->name);
/*
- * Plain USB connections need to actually call start and open
- * on the transport driver to allow incoming data.
+ * First call hid_hw_start(hdev, 0) to allow IO without connecting any
+ * hid subdrivers (hid-input, hidraw). This allows retrieving the dev's
+ * name and serial number and store these in hdev->name and hdev->uniq,
+ * before the hid-input and hidraw drivers expose these to userspace.
*/
ret = hidpp_connect_and_start(hidpp, 0);
if (ret)
@@ -4495,18 +4497,12 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
else
hidpp_non_unifying_init(hidpp);
- connected = hidpp_root_get_protocol_version(hidpp) == 0;
- atomic_set(&hidpp->connected, connected);
-
if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) {
ret = g920_get_config(hidpp, &data);
if (ret)
goto hid_hw_init_fail;
}
- schedule_work(&hidpp->work);
- flush_work(&hidpp->work);
-
/* Reset the HID node state */
hid_device_io_stop(hdev);
hid_hw_close(hdev);
@@ -4516,11 +4512,19 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
connect_mask &= ~HID_CONNECT_HIDINPUT;
/* Now export the actual inputs and hidraw nodes to the world */
- ret = hid_hw_start(hdev, connect_mask);
- if (ret) {
- hid_err(hdev, "%s:hid_hw_start returned error\n", __func__);
+ ret = hidpp_connect_and_start(hidpp, connect_mask);
+ if (ret)
goto hid_hw_start_fail;
- }
+
+ /*
+ * Now that incoming packets are enabled and will not be disabled
+ * again (which may cause missing packets) check the connected state
+ * of the device.
+ */
+ connected = hidpp_root_get_protocol_version(hidpp) == 0;
+ atomic_set(&hidpp->connected, connected);
+ schedule_work(&hidpp->work);
+ flush_work(&hidpp->work);
if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) {
ret = hidpp_ff_init(hidpp, &data);
@@ -4530,6 +4534,11 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
ret);
}
+ /*
+ * This relies on logi_dj_ll_close() being a no-op so that DJ connection
+ * events will still be received.
+ */
+ hid_hw_close(hdev);
return ret;
hid_hw_init_fail:
--
2.41.0
^ permalink raw reply related
* [PATCH v2 07/14] HID: logitech-hidpp: Remove connected check for g920_get_config() call
From: Hans de Goede @ 2023-10-08 9:54 UTC (permalink / raw)
To: Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires
Cc: Hans de Goede, linux-input
In-Reply-To: <20231008095458.8926-1-hdegoede@redhat.com>
The G920 is a wired USB device, so it is always connected, remove
the unnecessary connected check.
This is a preparation patch for moving the connect check to after
restarting IO, in case we miss a connect packet coming in while IO
is disabled during the restart.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/hid/hid-logitech-hidpp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index d7375885b0c7..bbb1c6d8ccc9 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -4487,7 +4487,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
connected = hidpp_root_get_protocol_version(hidpp) == 0;
atomic_set(&hidpp->connected, connected);
- if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_G920)) {
+ if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) {
ret = g920_get_config(hidpp, &data);
if (ret)
goto hid_hw_init_fail;
--
2.41.0
^ permalink raw reply related
* [PATCH v2 06/14] HID: logitech-hidpp: Remove wtp_get_config() call from probe()
From: Hans de Goede @ 2023-10-08 9:54 UTC (permalink / raw)
To: Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires
Cc: Hans de Goede, linux-input
In-Reply-To: <20231008095458.8926-1-hdegoede@redhat.com>
For WTP devices which start disconnected (paired with the unifying
receiver, but not connected atm) hidpp_connect_event() takes care
of calling wtp_get_config() when the device later connects.
There is no need to have a separate code path for WTP devices which
are connected at probe() time, these can use the same code-path since
probe() will queue the hidpp_connect_event() for those at probe time.
Drop the unnecessary wtp_get_config() call from probe().
This is a preparation patch for moving the connect check to after
restarting IO, in case we miss a connect packet coming in while IO
is disabled during the restart.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/hid/hid-logitech-hidpp.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index fe4c4871ea5c..d7375885b0c7 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -4487,11 +4487,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
connected = hidpp_root_get_protocol_version(hidpp) == 0;
atomic_set(&hidpp->connected, connected);
- if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)) {
- ret = wtp_get_config(hidpp);
- if (ret)
- goto hid_hw_init_fail;
- } else if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_G920)) {
+ if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_G920)) {
ret = g920_get_config(hidpp, &data);
if (ret)
goto hid_hw_init_fail;
--
2.41.0
^ permalink raw reply related
* [PATCH v2 12/14] HID: logitech-hidpp: Fix connect event race
From: Hans de Goede @ 2023-10-08 9:54 UTC (permalink / raw)
To: Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires
Cc: Hans de Goede, linux-input
In-Reply-To: <20231008095458.8926-1-hdegoede@redhat.com>
There is a connect event race in hidpp_probe() in these 2 lines:
connected = hidpp_root_get_protocol_version(hidpp) == 0;
atomic_set(&hidpp->connected, connected);
Specifically the following can happen:
1. This line from hidpp_probe() is executed:
connected = hidpp_root_get_protocol_version(hidpp) == 0;
and sets connected to false;
2. A connect-event packet is received and does:
atomic_set(&hidpp->connected, true);
3. The next line from hidpp_probe() is executed:
atomic_set(&hidpp->connected, connected);
and sets the atomic_t back to 0 again.
4. hidpp_connect_event() runs and sees the connected device
as disconnected because of this.
To fix this make hidpp_connect_event() query the connection status
of the device itself instead of having it rely on possibly stale
data cached in struct hidpp_device.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/hid/hid-logitech-hidpp.c | 29 +++++++----------------------
1 file changed, 7 insertions(+), 22 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 2b0a2ea5da22..37213dcc9d9c 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -194,7 +194,6 @@ struct hidpp_device {
struct work_struct work;
struct kfifo delayed_work_fifo;
- atomic_t connected;
struct input_dev *delayed_input;
unsigned long quirks;
@@ -3893,8 +3892,6 @@ static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data,
}
if (unlikely(hidpp_report_is_connect_event(hidpp, report))) {
- atomic_set(&hidpp->connected,
- !(report->rap.params[0] & (1 << 6)));
if (schedule_work(&hidpp->work) == 0)
dbg_hid("%s: connect event already queued\n", __func__);
return 1;
@@ -4189,12 +4186,14 @@ static struct input_dev *hidpp_allocate_input(struct hid_device *hdev)
static void hidpp_connect_event(struct hidpp_device *hidpp)
{
struct hid_device *hdev = hidpp->hid_dev;
- int ret = 0;
- bool connected = atomic_read(&hidpp->connected);
struct input_dev *input;
char *name, *devm_name;
+ int ret;
- if (!connected) {
+ /* Get device version to check if it is connected */
+ ret = hidpp_root_get_protocol_version(hidpp);
+ if (ret) {
+ hid_info(hidpp->hid_dev, "Disconnected\n");
if (hidpp->battery.ps) {
hidpp->battery.online = false;
hidpp->battery.status = POWER_SUPPLY_STATUS_UNKNOWN;
@@ -4235,17 +4234,6 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
if (ret)
return;
}
-
- /* the device is already connected, we can ask for its name and
- * protocol */
- if (!hidpp->protocol_major) {
- ret = hidpp_root_get_protocol_version(hidpp);
- if (ret) {
- hid_err(hdev, "Can not get the protocol version.\n");
- return;
- }
- }
-
if (hidpp->protocol_major >= 2) {
u8 feature_index;
@@ -4418,7 +4406,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
{
struct hidpp_device *hidpp;
int ret;
- bool connected;
unsigned int connect_mask = HID_CONNECT_DEFAULT;
/* report_fixup needs drvdata to be set before we call hid_parse */
@@ -4511,11 +4498,9 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
/*
* Now that incoming packets are enabled and will not be disabled
- * again (which may cause missing packets) check the connected state
- * of the device.
+ * again (which may cause missing packets) queue hidpp_connect_event()
+ * to check the connected state of the device.
*/
- connected = hidpp_root_get_protocol_version(hidpp) == 0;
- atomic_set(&hidpp->connected, connected);
schedule_work(&hidpp->work);
flush_work(&hidpp->work);
--
2.41.0
^ permalink raw reply related
* [PATCH v2 10/14] HID: logitech-hidpp: Move g920_get_config() to just before hidpp_ff_init()
From: Hans de Goede @ 2023-10-08 9:54 UTC (permalink / raw)
To: Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires
Cc: Hans de Goede, linux-input
In-Reply-To: <20231008095458.8926-1-hdegoede@redhat.com>
The data retrieved by g920_get_config() is onyl used by hidpp_ff_init().
Now that the hw is kept open till the end of probe() the g920_get_config()
call can be moved to just before hidpp_ff_init() to have all
the HIDPP_QUIRK_CLASS_G920 together in a single place.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/hid/hid-logitech-hidpp.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index ff834f905eda..b278e2b8924a 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -4420,7 +4420,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
int ret;
bool connected;
unsigned int connect_mask = HID_CONNECT_DEFAULT;
- struct hidpp_ff_private_data data;
/* report_fixup needs drvdata to be set before we call hid_parse */
hidpp = devm_kzalloc(&hdev->dev, sizeof(*hidpp), GFP_KERNEL);
@@ -4497,12 +4496,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
else
hidpp_non_unifying_init(hidpp);
- if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) {
- ret = g920_get_config(hidpp, &data);
- if (ret)
- goto hid_hw_init_fail;
- }
-
/* Reset the HID node state */
hid_device_io_stop(hdev);
hid_hw_close(hdev);
@@ -4527,7 +4520,12 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
flush_work(&hidpp->work);
if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) {
- ret = hidpp_ff_init(hidpp, &data);
+ struct hidpp_ff_private_data data;
+
+ ret = g920_get_config(hidpp, &data);
+ if (!ret)
+ ret = hidpp_ff_init(hidpp, &data);
+
if (ret)
hid_warn(hidpp->hid_dev,
"Unable to initialize force feedback support, errno %d\n",
@@ -4541,9 +4539,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
hid_hw_close(hdev);
return ret;
-hid_hw_init_fail:
- hid_hw_close(hdev);
- hid_hw_stop(hdev);
hid_hw_start_fail:
sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
cancel_work_sync(&hidpp->work);
--
2.41.0
^ permalink raw reply related
* [PATCH v2 08/14] HID: logitech-hidpp: Add a hidpp_connect_and_start() helper
From: Hans de Goede @ 2023-10-08 9:54 UTC (permalink / raw)
To: Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires
Cc: Hans de Goede, linux-input
In-Reply-To: <20231008095458.8926-1-hdegoede@redhat.com>
Add a helper to take the 3 hid_hw_start() + hid_hw_open() +
hid_device_io_start() steps which are necessary to be able to
talk to the hw.
This is a preparation patch for moving the connect check to after
restarting IO, in case we miss a connect packet coming in while IO
is disabled during the restart.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/hid/hid-logitech-hidpp.c | 40 ++++++++++++++++++++------------
1 file changed, 25 insertions(+), 15 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index bbb1c6d8ccc9..45b371e7b9ee 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -4391,6 +4391,29 @@ static bool hidpp_application_equals(struct hid_device *hdev,
return report && report->application == application;
}
+static int hidpp_connect_and_start(struct hidpp_device *hidpp, unsigned int connect_mask)
+{
+ struct hid_device *hdev = hidpp->hid_dev;
+ int ret;
+
+ ret = hid_hw_start(hdev, connect_mask);
+ if (ret) {
+ hid_err(hdev, "hw start failed: %d\n", ret);
+ return ret;
+ }
+
+ ret = hid_hw_open(hdev);
+ if (ret) {
+ hid_err(hdev, "hw open failed: %d\n", ret);
+ hid_hw_stop(hdev);
+ return ret;
+ }
+
+ /* Allow incoming packets */
+ hid_device_io_start(hdev);
+ return 0;
+}
+
static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
{
struct hidpp_device *hidpp;
@@ -4462,21 +4485,9 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
* Plain USB connections need to actually call start and open
* on the transport driver to allow incoming data.
*/
- ret = hid_hw_start(hdev, 0);
- if (ret) {
- hid_err(hdev, "hw start failed\n");
+ ret = hidpp_connect_and_start(hidpp, 0);
+ if (ret)
goto hid_hw_start_fail;
- }
-
- ret = hid_hw_open(hdev);
- if (ret < 0) {
- dev_err(&hdev->dev, "%s:hid_hw_open returned error:%d\n",
- __func__, ret);
- goto hid_hw_open_fail;
- }
-
- /* Allow incoming packets */
- hid_device_io_start(hdev);
/* Get name + serial, store in hdev->name + hdev->uniq */
if (hidpp->quirks & HIDPP_QUIRK_UNIFYING)
@@ -4523,7 +4534,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
hid_hw_init_fail:
hid_hw_close(hdev);
-hid_hw_open_fail:
hid_hw_stop(hdev);
hid_hw_start_fail:
sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
--
2.41.0
^ permalink raw reply related
* [PATCH v2 11/14] HID: logitech-hidpp: Remove unused connected param from *_connect()
From: Hans de Goede @ 2023-10-08 9:54 UTC (permalink / raw)
To: Filipe Laíns, Bastien Nocera, Jiri Kosina,
Benjamin Tissoires
Cc: Hans de Goede, linux-input
In-Reply-To: <20231008095458.8926-1-hdegoede@redhat.com>
Remove the unused connected function parameter from wtp_connect(),
m560_send_config_command() and k400_connect().
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/hid/hid-logitech-hidpp.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index b278e2b8924a..2b0a2ea5da22 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -3141,7 +3141,7 @@ static int wtp_allocate(struct hid_device *hdev, const struct hid_device_id *id)
return 0;
};
-static int wtp_connect(struct hid_device *hdev, bool connected)
+static int wtp_connect(struct hid_device *hdev)
{
struct hidpp_device *hidpp = hid_get_drvdata(hdev);
struct wtp_data *wd = hidpp->private_data;
@@ -3203,7 +3203,7 @@ static const u8 m560_config_parameter[] = {0x00, 0xaf, 0x03};
#define M560_SUB_ID 0x0a
#define M560_BUTTON_MODE_REGISTER 0x35
-static int m560_send_config_command(struct hid_device *hdev, bool connected)
+static int m560_send_config_command(struct hid_device *hdev)
{
struct hidpp_report response;
struct hidpp_device *hidpp_dev;
@@ -3398,7 +3398,7 @@ static int k400_allocate(struct hid_device *hdev)
return 0;
};
-static int k400_connect(struct hid_device *hdev, bool connected)
+static int k400_connect(struct hid_device *hdev)
{
struct hidpp_device *hidpp = hid_get_drvdata(hdev);
@@ -4205,15 +4205,15 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
}
if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) {
- ret = wtp_connect(hdev, connected);
+ ret = wtp_connect(hdev);
if (ret)
return;
} else if (hidpp->quirks & HIDPP_QUIRK_CLASS_M560) {
- ret = m560_send_config_command(hdev, connected);
+ ret = m560_send_config_command(hdev);
if (ret)
return;
} else if (hidpp->quirks & HIDPP_QUIRK_CLASS_K400) {
- ret = k400_connect(hdev, connected);
+ ret = k400_connect(hdev);
if (ret)
return;
}
--
2.41.0
^ 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