* Re: [PATCH v2 1/2] dt-bindings: input: gpio-keys: Allow optional dedicated wakeirq
From: Rob Herring @ 2023-09-21 15:29 UTC (permalink / raw)
To: Tony Lindgren
Cc: Dmitry Torokhov, Krzysztof Kozlowski, Conor Dooley, linux-input,
devicetree, linux-kernel, linux-arm-kernel
In-Reply-To: <20230920115044.53098-1-tony@atomide.com>
On Wed, Sep 20, 2023 at 02:50:43PM +0300, Tony Lindgren wrote:
> Allow configuring an optional dedicated wakeirq for gpio-keys that
> some SoCs have.
>
> Let's use the common interrupt naming "irq" and "wakeup" that we already
> have in use for some drivers and subsystems like i2c framework.
>
> Note that the gpio-keys interrupt property is optional. If only a gpio
> property is specified, the driver tries to translate the gpio into an
> interrupt.
>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>
> Changes since v1:
>
> - Run make dt_binding_check on the binding
>
> - Add better checks for interrupt-names as suggested by Rob, it is
> now required if two interrupts are configured
>
> - Add more decription entries
>
> - Add a new example for key-wakeup
>
> ---
> .../devicetree/bindings/input/gpio-keys.yaml | 41 ++++++++++++++++++-
> 1 file changed, 40 insertions(+), 1 deletion(-)
With the indentation fixed,
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup
From: Rafael J. Wysocki @ 2023-09-21 9:33 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: Rafael J. Wysocki, srinivas pandruvada, Xu, Even,
jikos@kernel.org, benjamin.tissoires@redhat.com,
linux-pm@vger.kernel.org, linux-pci@vger.kernel.org,
Lee, Jian Hui, Zhang, Lixu, Ba, Najumon,
linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <CAAd53p67tiP0xXwhn=NviU_rvrSveSxbAhDieYG9AmUWF2e__Q@mail.gmail.com>
On Thu, Sep 21, 2023 at 8:08 AM Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
>
> On Wed, Sep 20, 2023 at 2:00 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Tue, Sep 19, 2023 at 6:54 PM srinivas pandruvada
> > <srinivas.pandruvada@linux.intel.com> wrote:
> > >
> > > On Tue, 2023-09-19 at 15:36 +0800, Kai-Heng Feng wrote:
> > > > On Mon, Sep 18, 2023 at 11:57 PM srinivas pandruvada
> > > > <srinivas.pandruvada@linux.intel.com> wrote:
> > > > >
> > > > > Hi Kai-Heng,
> > > > > On Mon, 2023-09-18 at 09:17 +0800, Kai-Heng Feng wrote:
> > > > > > Hi Even,
> > > > > >
> > > > > > On Mon, Sep 18, 2023 at 8:33 AM Xu, Even <even.xu@intel.com>
> > > > > > wrote:
> > > > > > >
> > > > > > > Hi, Kai-Heng,
> > > > > > >
> > > > > > > I just got feedback, for testing EHL S5 wakeup feature, you
> > > > > > > need
> > > > > > > several steps to setup and access
> > > > > > > "https://portal.devicewise.com/things/browse" to trigger wake.
> > > > > > > But currently, our test account of this website are all out of
> > > > > > > data.
> > > > > > > So maybe you need double check with the team who required you
> > > > > > > preparing the patch for the verification.
> > > > > >
> > > > > > The patch is to solve the GPE refcount overflow, while
> > > > > > maintaining S5
> > > > > > wakeup. I don't have any mean to test S5 wake.
> > > > > >
> > > > > The issue is not calling acpi_disable_gpe(). To reduce the scope of
> > > > > change can we just add that instead of a adding new callbacks. This
> > > > > way
> > > > > scope is reduced.
> > > >
> > > > This patch does exactly the same thing by letting PCI and ACPI handle
> > > > the PME and GPE.
> > > > Though the change seems to be bigger, it actually reduces the duped
> > > > code, while keep the S5 wakeup ability intact.
> > > It may be doing the same. But with long chain of calls without
> > > verification, I am not comfortable.
> > > This can be another patch by itself to use the framework.
> >
> > I agree.
> >
> > Let's change one thing at a time.
> >
> > > But you are targeting a fix for overflow issue, which is separate from
> > > the use of PCI/ACPI framework.
> >
> > Yes, let's fix the bug first and make things look nicer separately.
>
> Right, please use the fix from Srinivas and I'll send a separate patch
> to make things looks better.
OK
Srinivas, please resend the patch with a changelog etc.
^ permalink raw reply
* [PATCH] Input: synaptics-rmi4 - replace deprecated strncpy
From: Justin Stitt @ 2023-09-21 9:58 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel, linux-hardening, Justin Stitt
`strncpy` is deprecated for use on NUL-terminated destination strings [1]
Let's use memcpy() as the bounds have already been checked and this
decays into a simple byte copy from one buffer to another removing any
ambiguity that strncpy has.
Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
Link: https://github.com/KSPP/linux/issues/90
Cc: linux-hardening@vger.kernel.org
Signed-off-by: Justin Stitt <justinstitt@google.com>
---
Note: build-tested only.
Similar to Kees' suggestion here [2]
[2]: https://lore.kernel.org/all/202309142045.7CBAE940E@keescook/
---
drivers/input/rmi4/rmi_f34.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/rmi4/rmi_f34.c b/drivers/input/rmi4/rmi_f34.c
index 0d9a5756e3f5..3b3ac71e53dc 100644
--- a/drivers/input/rmi4/rmi_f34.c
+++ b/drivers/input/rmi4/rmi_f34.c
@@ -471,7 +471,7 @@ static ssize_t rmi_driver_update_fw_store(struct device *dev,
if (buf[count - 1] == '\0' || buf[count - 1] == '\n')
copy_count -= 1;
- strncpy(fw_name, buf, copy_count);
+ memcpy(fw_name, buf, copy_count);
fw_name[copy_count] = '\0';
ret = request_firmware(&fw, fw_name, dev);
---
base-commit: 2cf0f715623872823a72e451243bbf555d10d032
change-id: 20230921-strncpy-drivers-input-rmi4-rmi_f34-c-4a6945316cea
Best regards,
--
Justin Stitt <justinstitt@google.com>
^ permalink raw reply related
* Re: [PATCH] hid: cp2112: Fix duplicate workqueue initialization
From: andriy.shevchenko @ 2023-09-21 9:42 UTC (permalink / raw)
To: Danny Kaehn
Cc: jikos@kernel.org, benjamin.tissoires@redhat.com,
linux-input@vger.kernel.org, Ethan Twardy
In-Reply-To: <216f73660b7c55ab247345120468d0cd9463e622.camel@plexus.com>
On Wed, Sep 20, 2023 at 07:10:15PM +0000, Danny Kaehn wrote:
> On Wed, 2023-09-20 at 16:04 +0300, Andy Shevchenko wrote:
> > On Tue, Sep 19, 2023 at 04:22:45PM -0500, Danny Kaehn wrote:
> > > Previously the cp2112 driver called INIT_DELAYED_WORK within
> > > cp2112_gpio_irq_startup, resulting in duplicate initilizations of the
> > > workqueue on subsequent IRQ startups following an initial request. This
> > > resulted in a warning in set_work_data in workqueue.c, as well as a rare
> > > NULL dereference within process_one_work in workqueue.c.
> > >
> > > Initialize the workqueue within _probe instead.
> >
> > Does it deserve a Fixes tag?
>
> I'm not sure -- it does technically fix 13de9cca514ed63604263cad87ca8cb36e9b6489
> (HID: cp2112: add IRQ chip handling), but does not apply cleanly to that
> revision (a.e. with git am)
It's not a problem.
> (my understanding is that 'Fixes' is used for stable kernel maintenance?)
Not only, for anybody to track the issues and fixes.
For stable it's more important to follow their procedure, where the Fixes
is just one small piece of.
Fixes: 13de9cca514e ("HID: cp2112: add IRQ chip handling")
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* [PATCH] Input: axp20x-pek - refactor deprecated strncpy
From: Justin Stitt @ 2023-09-21 9:17 UTC (permalink / raw)
To: Dmitry Torokhov, Chen-Yu Tsai
Cc: linux-input, linux-kernel, linux-hardening, Justin Stitt
`strncpy` is deprecated for use on NUL-terminated destination strings
[1] and as such we should prefer more robust and less ambiguous string
interfaces.
Ensuring we have a trailing NUL-byte and checking the length of bytes
copied are both intrinsic behavior of strscpy.
Therefore, a suitable replacement is `strscpy` [2] due to the fact that
it guarantees NUL-termination on the destination buffer without
unnecessarily NUL-padding.
It should be noted that the original code can silently truncate and so
can this refactoring. However, a check could be added if truncation
is an issue:
| len = strscpy(val_str, buf, sizeof(val_str));
| if (len < 0) { // add this
| return -E2BIG; // or -EINVAL
| }
Also, now check for `len > 0` instead of just a truthy `len` because
`len` is now a signed type and we could run into problems if strscpy
returned -E2BIG which would pass the truthy test.
Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2]
Link: https://github.com/KSPP/linux/issues/90
Cc: linux-hardening@vger.kernel.org
Signed-off-by: Justin Stitt <justinstitt@google.com>
---
Note: build-tested only.
---
drivers/input/misc/axp20x-pek.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/input/misc/axp20x-pek.c b/drivers/input/misc/axp20x-pek.c
index 4581606a28d6..abcf78785b45 100644
--- a/drivers/input/misc/axp20x-pek.c
+++ b/drivers/input/misc/axp20x-pek.c
@@ -134,16 +134,14 @@ static ssize_t axp20x_store_attr(struct device *dev,
{
struct axp20x_pek *axp20x_pek = dev_get_drvdata(dev);
char val_str[20];
- size_t len;
+ ssize_t len;
int ret, i;
unsigned int val, idx = 0;
unsigned int best_err = UINT_MAX;
- val_str[sizeof(val_str) - 1] = '\0';
- strncpy(val_str, buf, sizeof(val_str) - 1);
- len = strlen(val_str);
+ len = strscpy(val_str, buf, sizeof(val_str));
- if (len && val_str[len - 1] == '\n')
+ if (len > 0 && val_str[len - 1] == '\n')
val_str[len - 1] = '\0';
ret = kstrtouint(val_str, 10, &val);
---
base-commit: 2cf0f715623872823a72e451243bbf555d10d032
change-id: 20230921-strncpy-drivers-input-misc-axp20x-pek-c-3c4708924bbd
Best regards,
--
Justin Stitt <justinstitt@google.com>
^ permalink raw reply related
* [PATCH 1/2] HID: uclogic: Fix user-memory-access bug in uclogic_params_ugee_v2_init_event_hooks()
From: Jinjie Ruan @ 2023-09-21 13:38 UTC (permalink / raw)
To: jikos, benjamin.tissoires, jose.exposito89, linux-input; +Cc: ruanjinjie
In-Reply-To: <20230921133824.605700-1-ruanjinjie@huawei.com>
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);
+ 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);
}
static struct kunit_case hid_uclogic_params_test_cases[] = {
--
2.34.1
^ permalink raw reply related
* [PATCH 0/2] HID: uclogic: Fix two bugs in uclogic
From: Jinjie Ruan @ 2023-09-21 13:38 UTC (permalink / raw)
To: jikos, benjamin.tissoires, jose.exposito89, linux-input; +Cc: ruanjinjie
When CONFIG_HID_UCLOGIC=y and CONFIG_KUNIT_ALL_TESTS=y, launch
kernel and then there are a user-memory-access bug and a work->entry
not empty bug. This patchset fix these issues.
Jinjie Ruan (2):
HID: uclogic: Fix user-memory-access bug in
uclogic_params_ugee_v2_init_event_hooks()
HID: uclogic: Fix a work->entry not empty bug in __queue_work()
drivers/hid/hid-uclogic-core-test.c | 7 +++++++
drivers/hid/hid-uclogic-params-test.c | 15 ++++++++++++++-
2 files changed, 21 insertions(+), 1 deletion(-)
--
2.34.1
^ permalink raw reply
* [PATCH 2/2] HID: uclogic: Fix a work->entry not empty bug in __queue_work()
From: Jinjie Ruan @ 2023-09-21 13:38 UTC (permalink / raw)
To: jikos, benjamin.tissoires, jose.exposito89, linux-input; +Cc: ruanjinjie
In-Reply-To: <20230921133824.605700-1-ruanjinjie@huawei.com>
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>
---
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 related
* [PATCH] HID: lenovo: Fix middle-button behaviour for system suspend
From: Martin Kepplinger @ 2023-09-21 9:21 UTC (permalink / raw)
To: jikos, benjamin.tissoires, jm, linux-kernel
Cc: linux-input, Martin Kepplinger
After system suspend the middle-button mode is being reset to
compatibility mode which simply breaks functionality for the devices
where native mode is configured during probe().
Fix this by setting native mode in reset_resume() for the appropriate
devices.
Fixes: 94eefa271323 ("HID: lenovo: Use native middle-button mode for compact keyboards")
Signed-off-by: Martin Kepplinger <martink@posteo.de>
---
drivers/hid/hid-lenovo.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
index 44763c0da444..d20562b9eca6 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -1344,6 +1344,28 @@ static int lenovo_input_configured(struct hid_device *hdev,
return 0;
}
+static int __maybe_unused lenovo_resume(struct hid_device *hdev)
+{
+ int ret;
+
+ switch (hdev->product) {
+ case USB_DEVICE_ID_LENOVO_CUSBKBD:
+ case USB_DEVICE_ID_LENOVO_CBTKBD:
+ case USB_DEVICE_ID_LENOVO_TPIIUSBKBD:
+ case USB_DEVICE_ID_LENOVO_TPIIBTKBD:
+ /* Switch middle button to native mode again */
+ ret = lenovo_send_cmd_cptkbd(hdev, 0x09, 0x01);
+ if (ret)
+ hid_warn(hdev, "Failed to switch middle button: %d\n",
+ ret);
+ break;
+ default:
+ ret = 0;
+ break;
+ }
+
+ return ret;
+}
static const struct hid_device_id lenovo_devices[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_LENOVO, USB_DEVICE_ID_LENOVO_TPKBD) },
@@ -1380,6 +1402,9 @@ static struct hid_driver lenovo_driver = {
.raw_event = lenovo_raw_event,
.event = lenovo_event,
.report_fixup = lenovo_report_fixup,
+#ifdef CONFIG_PM
+ .reset_resume = lenovo_resume,
+#endif
};
module_hid_driver(lenovo_driver);
--
2.39.2
^ permalink raw reply related
* Re: [PATCH 04/52] input: iqs62x-keys - Convert to platform remove callback returning void
From: Jeff LaBundy @ 2023-09-21 11:29 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Dmitry Torokhov, Mattijs Korpershoek, linux-input, kernel
In-Reply-To: <20230920125829.1478827-5-u.kleine-koenig@pengutronix.de>
On Wed, Sep 20, 2023 at 02:57:41PM +0200, Uwe Kleine-König wrote:
> The .remove() callback for a platform driver returns an int which makes
> many driver authors wrongly assume it's possible to do error handling by
> returning an error code. However the value returned is ignored (apart
> from emitting a warning) and this typically results in resource leaks.
> To improve here there is a quest to make the remove callback return
> void. In the first step of this quest all drivers are converted to
> .remove_new() which already returns void. Eventually after all drivers
> are converted, .remove_new() will be renamed to .remove().
>
> Trivially convert this driver from always returning zero in the remove
> callback to the void returning variant.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Reviewed-by: Jeff LaBundy <jeff@labundy.com>
> ---
> drivers/input/keyboard/iqs62x-keys.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/input/keyboard/iqs62x-keys.c b/drivers/input/keyboard/iqs62x-keys.c
> index 02ceebad7bda..688d61244b5f 100644
> --- a/drivers/input/keyboard/iqs62x-keys.c
> +++ b/drivers/input/keyboard/iqs62x-keys.c
> @@ -310,7 +310,7 @@ static int iqs62x_keys_probe(struct platform_device *pdev)
> return ret;
> }
>
> -static int iqs62x_keys_remove(struct platform_device *pdev)
> +static void iqs62x_keys_remove(struct platform_device *pdev)
> {
> struct iqs62x_keys_private *iqs62x_keys = platform_get_drvdata(pdev);
> int ret;
> @@ -319,8 +319,6 @@ static int iqs62x_keys_remove(struct platform_device *pdev)
> &iqs62x_keys->notifier);
> if (ret)
> dev_err(&pdev->dev, "Failed to unregister notifier: %d\n", ret);
> -
> - return 0;
> }
>
> static struct platform_driver iqs62x_keys_platform_driver = {
> @@ -328,7 +326,7 @@ static struct platform_driver iqs62x_keys_platform_driver = {
> .name = "iqs62x-keys",
> },
> .probe = iqs62x_keys_probe,
> - .remove = iqs62x_keys_remove,
> + .remove_new = iqs62x_keys_remove,
> };
> module_platform_driver(iqs62x_keys_platform_driver);
>
> --
> 2.40.1
>
^ permalink raw reply
* [PATCH v7] HID: mcp2200: added driver for GPIOs of MCP2200
From: Johannes Roith @ 2023-09-21 16:49 UTC (permalink / raw)
To: jikos
Cc: sergeantsagara, andi.shyti, benjamin.tissoires,
christophe.jaillet, johannes, linux-input, linux-kernel, rdunlap
Added a gpiochip compatible driver to control the 8 GPIOs of
the MCP2200 by using the HID interface.
Using GPIOs with alternative functions (GP0<->SSPND, GP1<->USBCFG,
GP6<->RXLED, GP7<->TXLED) will reset the functions, if set (unset by
default).
The driver was tested while also using the UART of the chip. Setting
and reading the GPIOs has no effect on the UART communication. However,
a reset is triggered after the CONFIGURE command. If the GPIO Direction
is constantly changed, this will affect the communication at low baud
rates. This is a hardware problem of the MCP2200 and is not caused by
the driver.
Signed-off-by: Johannes Roith <johannes@gnu-linux.rocks>
---
drivers/hid/Kconfig | 9 +
drivers/hid/Makefile | 1 +
drivers/hid/hid-ids.h | 1 +
drivers/hid/hid-mcp2200.c | 392 ++++++++++++++++++++++++++++++++++++++
4 files changed, 403 insertions(+)
create mode 100644 drivers/hid/hid-mcp2200.c
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 0cea301cc9a9..3c14644b593d 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -1298,6 +1298,15 @@ config HID_ALPS
Say Y here if you have a Alps touchpads over i2c-hid or usbhid
and want support for its special functionalities.
+config HID_MCP2200
+ tristate "Microchip MCP2200 HID USB-to-GPIO bridge"
+ depends on USB_HID && GPIOLIB
+ help
+ Provides GPIO functionality over USB-HID through MCP2200 device.
+
+ To compile this driver as a module, choose M here: the module
+ will be called hid-mcp2200.ko.
+
config HID_MCP2221
tristate "Microchip MCP2221 HID USB-to-I2C/SMbus host support"
depends on USB_HID && I2C
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 8a06d0f840bc..082a728eac60 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -79,6 +79,7 @@ obj-$(CONFIG_HID_LOGITECH_HIDPP) += hid-logitech-hidpp.o
obj-$(CONFIG_HID_MACALLY) += hid-macally.o
obj-$(CONFIG_HID_MAGICMOUSE) += hid-magicmouse.o
obj-$(CONFIG_HID_MALTRON) += hid-maltron.o
+obj-$(CONFIG_HID_MCP2200) += hid-mcp2200.o
obj-$(CONFIG_HID_MCP2221) += hid-mcp2221.o
obj-$(CONFIG_HID_MAYFLASH) += hid-mf.o
obj-$(CONFIG_HID_MEGAWORLD_FF) += hid-megaworld.o
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 7e499992a793..bb87ad4eb2aa 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -915,6 +915,7 @@
#define USB_DEVICE_ID_PICK16F1454 0x0042
#define USB_DEVICE_ID_PICK16F1454_V2 0xf2f7
#define USB_DEVICE_ID_LUXAFOR 0xf372
+#define USB_DEVICE_ID_MCP2200 0x00df
#define USB_DEVICE_ID_MCP2221 0x00dd
#define USB_VENDOR_ID_MICROSOFT 0x045e
diff --git a/drivers/hid/hid-mcp2200.c b/drivers/hid/hid-mcp2200.c
new file mode 100644
index 000000000000..bf57f7f6caa0
--- /dev/null
+++ b/drivers/hid/hid-mcp2200.c
@@ -0,0 +1,392 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * MCP2200 - Microchip USB to GPIO bridge
+ *
+ * Copyright (c) 2023, Johannes Roith <johannes@gnu-linux.rocks>
+ *
+ * Datasheet: https://ww1.microchip.com/downloads/en/DeviceDoc/22228A.pdf
+ * App Note for HID: https://ww1.microchip.com/downloads/en/DeviceDoc/93066A.pdf
+ */
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/gpio/driver.h>
+#include <linux/hid.h>
+#include <linux/hidraw.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include "hid-ids.h"
+
+/* Commands codes in a raw output report */
+#define SET_CLEAR_OUTPUTS 0x08
+#define CONFIGURE 0x10
+#define READ_EE 0x20
+#define WRITE_EE 0x40
+#define READ_ALL 0x80
+
+/* MCP GPIO direction encoding */
+enum MCP_IO_DIR {
+ MCP2200_DIR_OUT = 0x00,
+ MCP2200_DIR_IN = 0x01,
+};
+
+/* Altternative pin assignments */
+#define TXLED 2
+#define RXLED 3
+#define USBCFG 6
+#define SSPND 7
+#define MCP_NGPIO 8
+
+/* CMD to set or clear a GPIO output */
+struct mcp_set_clear_outputs {
+ u8 cmd;
+ u8 dummys1[10];
+ u8 set_bmap;
+ u8 clear_bmap;
+ u8 dummys2[3];
+} __packed;
+
+/* CMD to configure the IOs */
+struct mcp_configure {
+ u8 cmd;
+ u8 dummys1[3];
+ u8 io_bmap;
+ u8 config_alt_pins;
+ u8 io_default_val_bmap;
+ u8 config_alt_options;
+ u8 baud_h;
+ u8 baud_l;
+ u8 dummys2[6];
+} __packed;
+
+/* CMD to read all parameters */
+struct mcp_read_all {
+ u8 cmd;
+ u8 dummys[15];
+} __packed;
+
+/* Response to the read all cmd */
+struct mcp_read_all_resp {
+ u8 cmd;
+ u8 eep_addr;
+ u8 dummy;
+ u8 eep_val;
+ u8 io_bmap;
+ u8 config_alt_pins;
+ u8 io_default_val_bmap;
+ u8 config_alt_options;
+ u8 baud_h;
+ u8 baud_l;
+ u8 io_port_val_bmap;
+ u8 dummys[5];
+} __packed;
+
+struct mcp2200 {
+ struct hid_device *hdev;
+ struct mutex lock;
+ struct completion wait_in_report;
+ u8 gpio_dir;
+ u8 gpio_val;
+ u8 gpio_inval;
+ u8 baud_h;
+ u8 baud_l;
+ u8 config_alt_pins;
+ u8 gpio_reset_val;
+ u8 config_alt_options;
+ int status;
+ struct gpio_chip gc;
+ u8 hid_report[16];
+};
+
+/* this executes the READ_ALL cmd */
+static int mcp_cmd_read_all(struct mcp2200 *mcp)
+{
+ struct mcp_read_all *read_all;
+ int len, t;
+
+ reinit_completion(&mcp->wait_in_report);
+
+ mutex_lock(&mcp->lock);
+
+ read_all = (struct mcp_read_all *) mcp->hid_report;
+ read_all->cmd = READ_ALL;
+ len = hid_hw_output_report(mcp->hdev, (u8 *) read_all,
+ sizeof(struct mcp_read_all));
+
+ mutex_unlock(&mcp->lock);
+
+ if (len != sizeof(struct mcp_read_all))
+ return -EINVAL;
+
+ t = wait_for_completion_timeout(&mcp->wait_in_report,
+ msecs_to_jiffies(4000));
+ if (!t)
+ return -ETIMEDOUT;
+
+ /* return status, negative value if wrong response was received */
+ return mcp->status;
+}
+
+static void mcp_set_multiple(struct gpio_chip *gc, unsigned long *mask,
+ unsigned long *bits)
+{
+ struct mcp2200 *mcp = gpiochip_get_data(gc);
+ u8 value;
+ int status;
+ struct mcp_set_clear_outputs *cmd;
+
+ mutex_lock(&mcp->lock);
+ cmd = (struct mcp_set_clear_outputs *) mcp->hid_report;
+
+ value = mcp->gpio_val & ~*mask;
+ value |= (*mask & *bits);
+
+ cmd->cmd = SET_CLEAR_OUTPUTS;
+ cmd->set_bmap = value;
+ cmd->clear_bmap = ~(value);
+
+ status = hid_hw_output_report(mcp->hdev, (u8 *) cmd,
+ sizeof(struct mcp_set_clear_outputs));
+
+ if (status == sizeof(struct mcp_set_clear_outputs))
+ mcp->gpio_val = value;
+
+ mutex_unlock(&mcp->lock);
+}
+
+static void mcp_set(struct gpio_chip *gc, unsigned int gpio_nr, int value)
+{
+ unsigned long mask = 1 << gpio_nr;
+ unsigned long bmap_value = value << gpio_nr;
+
+ mcp_set_multiple(gc, &mask, &bmap_value);
+}
+
+static int mcp_get_multiple(struct gpio_chip *gc, unsigned long *mask,
+ unsigned long *bits)
+{
+ u32 val;
+ struct mcp2200 *mcp = gpiochip_get_data(gc);
+ int status;
+
+ status = mcp_cmd_read_all(mcp);
+ if (status)
+ return status;
+
+ val = mcp->gpio_inval;
+ *bits = (val & *mask);
+ return 0;
+}
+
+static int mcp_get(struct gpio_chip *gc, unsigned int gpio_nr)
+{
+ unsigned long mask = 0, bits = 0;
+
+ mask = (1 << gpio_nr);
+ mcp_get_multiple(gc, &mask, &bits);
+ return bits > 0;
+}
+
+static int mcp_get_direction(struct gpio_chip *gc, unsigned int gpio_nr)
+{
+ struct mcp2200 *mcp = gpiochip_get_data(gc);
+
+ return (mcp->gpio_dir & (MCP2200_DIR_IN << gpio_nr))
+ ? GPIO_LINE_DIRECTION_IN : GPIO_LINE_DIRECTION_OUT;
+}
+
+static int mcp_set_direction(struct gpio_chip *gc, unsigned int gpio_nr,
+ enum MCP_IO_DIR io_direction)
+{
+ struct mcp2200 *mcp = gpiochip_get_data(gc);
+ struct mcp_configure *conf;
+ int status;
+ /* after the configure cmd we will need to set the outputs again */
+ unsigned long mask = ~(mcp->gpio_dir); /* only set outputs */
+ unsigned long bits = mcp->gpio_val;
+ /* Offsets of alternative pins in config_alt_pins, 0 is not used */
+ u8 alt_pin_conf[8] = {SSPND, USBCFG, 0, 0, 0, 0, RXLED, TXLED};
+ u8 config_alt_pins = mcp->config_alt_pins;
+
+ /* Read in the reset baudrate first, we need it later */
+ status = mcp_cmd_read_all(mcp);
+ if (status != 0)
+ return status;
+
+ mutex_lock(&mcp->lock);
+ conf = (struct mcp_configure *) mcp->hid_report;
+
+ /* configure will reset the chip! */
+ conf->cmd = CONFIGURE;
+ conf->io_bmap = (mcp->gpio_dir & ~(1 << gpio_nr))
+ | (io_direction << gpio_nr);
+ /* Don't overwrite the reset parameters */
+ conf->baud_h = mcp->baud_h;
+ conf->baud_l = mcp->baud_l;
+ conf->config_alt_options = mcp->config_alt_options;
+ conf->io_default_val_bmap = mcp->gpio_reset_val;
+ /* Adjust alt. func if necessary */
+ if (alt_pin_conf[gpio_nr])
+ config_alt_pins &= ~(1 << alt_pin_conf[gpio_nr]);
+ conf->config_alt_pins = config_alt_pins;
+
+ status = hid_hw_output_report(mcp->hdev, (u8 *) conf,
+ sizeof(struct mcp_set_clear_outputs));
+
+ if (status == sizeof(struct mcp_set_clear_outputs)) {
+ mcp->gpio_dir = conf->io_bmap;
+ mcp->config_alt_pins = config_alt_pins;
+ } else {
+ mutex_unlock(&mcp->lock);
+ return -EIO;
+ }
+
+ mutex_unlock(&mcp->lock);
+
+ /* Configure CMD will clear all IOs -> rewrite them */
+ mcp_set_multiple(gc, &mask, &bits);
+ return 0;
+}
+
+static int mcp_direction_input(struct gpio_chip *gc, unsigned int gpio_nr)
+{
+ return mcp_set_direction(gc, gpio_nr, MCP2200_DIR_IN);
+}
+
+static int mcp_direction_output(struct gpio_chip *gc, unsigned int gpio_nr,
+ int value)
+{
+ int ret;
+ unsigned long mask, bmap_value;
+
+ mask = 1 << gpio_nr;
+ bmap_value = value << gpio_nr;
+
+ ret = mcp_set_direction(gc, gpio_nr, MCP2200_DIR_OUT);
+ if (!ret)
+ mcp_set_multiple(gc, &mask, &bmap_value);
+ return ret;
+}
+
+static const struct gpio_chip template_chip = {
+ .label = "mcp2200",
+ .owner = THIS_MODULE,
+ .get_direction = mcp_get_direction,
+ .direction_input = mcp_direction_input,
+ .direction_output = mcp_direction_output,
+ .set = mcp_set,
+ .set_multiple = mcp_set_multiple,
+ .get = mcp_get,
+ .get_multiple = mcp_get_multiple,
+ .base = -1,
+ .ngpio = MCP_NGPIO,
+ .can_sleep = true,
+};
+
+/*
+ * MCP2200 uses interrupt endpoint for input reports. This function
+ * is called by HID layer when it receives i/p report from mcp2200,
+ * which is actually a response to the previously sent command.
+ */
+static int mcp2200_raw_event(struct hid_device *hdev, struct hid_report *report,
+ u8 *data, int size)
+{
+ struct mcp2200 *mcp = hid_get_drvdata(hdev);
+ struct mcp_read_all_resp *all_resp;
+
+ switch (data[0]) {
+ case READ_ALL:
+ all_resp = (struct mcp_read_all_resp *) data;
+ mcp->status = 0;
+ mcp->gpio_inval = all_resp->io_port_val_bmap;
+ mcp->baud_h = all_resp->baud_h;
+ mcp->baud_l = all_resp->baud_l;
+ mcp->gpio_reset_val = all_resp->io_default_val_bmap;
+ mcp->config_alt_pins = all_resp->config_alt_pins;
+ mcp->config_alt_options = all_resp->config_alt_options;
+ break;
+ default:
+ mcp->status = -EIO;
+ break;
+ }
+
+ complete(&mcp->wait_in_report);
+ return 0;
+}
+
+static int mcp2200_probe(struct hid_device *hdev, const struct hid_device_id *id)
+{
+ int ret;
+ struct mcp2200 *mcp;
+
+ mcp = devm_kzalloc(&hdev->dev, sizeof(*mcp), GFP_KERNEL);
+ if (!mcp)
+ return -ENOMEM;
+
+ ret = hid_parse(hdev);
+ if (ret) {
+ hid_err(hdev, "can't parse reports\n");
+ return ret;
+ }
+
+ ret = hid_hw_start(hdev, 0);
+ if (ret) {
+ hid_err(hdev, "can't start hardware\n");
+ return ret;
+ }
+
+ hid_info(hdev, "USB HID v%x.%02x Device [%s] on %s\n", hdev->version >> 8,
+ hdev->version & 0xff, hdev->name, hdev->phys);
+
+ ret = hid_hw_open(hdev);
+ if (ret) {
+ hid_err(hdev, "can't open device\n");
+ hid_hw_stop(hdev);
+ return ret;
+ }
+
+ mutex_init(&mcp->lock);
+ init_completion(&mcp->wait_in_report);
+ hid_set_drvdata(hdev, mcp);
+ mcp->hdev = hdev;
+
+ mcp->gc = template_chip;
+ mcp->gc.parent = &hdev->dev;
+
+ ret = devm_gpiochip_add_data(&hdev->dev, &mcp->gc, mcp);
+ if (ret < 0) {
+ hid_err(hdev, "Unable to register gpiochip\n");
+ hid_hw_close(hdev);
+ hid_hw_stop(hdev);
+ return ret;
+ }
+
+ return 0;
+}
+
+static void mcp2200_remove(struct hid_device *hdev)
+{
+ hid_hw_close(hdev);
+ hid_hw_stop(hdev);
+}
+
+static const struct hid_device_id mcp2200_devices[] = {
+ { HID_USB_DEVICE(USB_VENDOR_ID_MICROCHIP, USB_DEVICE_ID_MCP2200) },
+ { }
+};
+MODULE_DEVICE_TABLE(hid, mcp2200_devices);
+
+static struct hid_driver mcp2200_driver = {
+ .name = "mcp2200",
+ .id_table = mcp2200_devices,
+ .probe = mcp2200_probe,
+ .remove = mcp2200_remove,
+ .raw_event = mcp2200_raw_event,
+};
+
+/* Register with HID core */
+module_hid_driver(mcp2200_driver);
+
+MODULE_AUTHOR("Johannes Roith <johannes@gnu-linux.rocks>");
+MODULE_DESCRIPTION("MCP2200 Microchip HID USB to GPIO bridge");
+MODULE_LICENSE("GPL");
--
2.42.0
^ permalink raw reply related
* Re: [PATCH 02/52] input: cros_ec_keyb - Convert to platform remove callback returning void
From: Tzung-Bi Shih @ 2023-09-21 3:21 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Dmitry Torokhov, Benson Leung, Guenter Roeck, Greg Kroah-Hartman,
Jonathan Cameron, joewu (吳仲振), linux-input,
chrome-platform, kernel
In-Reply-To: <20230920125829.1478827-3-u.kleine-koenig@pengutronix.de>
On Wed, Sep 20, 2023 at 02:57:39PM +0200, Uwe Kleine-König wrote:
> The .remove() callback for a platform driver returns an int which makes
> many driver authors wrongly assume it's possible to do error handling by
> returning an error code. However the value returned is ignored (apart
> from emitting a warning) and this typically results in resource leaks.
> To improve here there is a quest to make the remove callback return
> void. In the first step of this quest all drivers are converted to
> .remove_new() which already returns void. Eventually after all drivers
> are converted, .remove_new() will be renamed to .remove().
>
> Trivially convert this driver from always returning zero in the remove
> callback to the void returning variant.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Reviewed-by: Tzung-Bi Shih <tzungbi@kernel.org>
^ permalink raw reply
* Re: [PATCH v6] HID: mcp2200: added driver for GPIOs of MCP2200
From: Rahul Rameshbabu @ 2023-09-21 2:43 UTC (permalink / raw)
To: Johannes Roith
Cc: jikos, andi.shyti, benjamin.tissoires, christophe.jaillet,
linux-input, linux-kernel, rdunlap
In-Reply-To: <20230918151644.568998-1-johannes@gnu-linux.rocks>
Hi Johannes,
On Mon, 18 Sep, 2023 17:16:44 +0200 "Johannes Roith" <johannes@gnu-linux.rocks> wrote:
> Added a gpiochip compatible driver to control the 8 GPIOs of
> the MCP2200 by using the HID interface.
>
> Using GPIOs with alternative functions (GP0<->SSPND, GP1<->USBCFG,
> GP6<->RXLED, GP7<->TXLED) will reset the functions, if set (unset by
> default).
>
> The driver was tested while also using the UART of the chip. Setting
> and reading the GPIOs has no effect on the UART communication. However,
> a reset is triggered after the CONFIGURE command. If the GPIO Direction
> is constantly changed, this will affect the communication at low baud
> rates. This is a hardware problem of the MCP2200 and is not caused by
> the driver.
>
> Signed-off-by: Johannes Roith <johannes@gnu-linux.rocks>
> ---
> drivers/hid/Kconfig | 9 +
> drivers/hid/Makefile | 1 +
> drivers/hid/hid-ids.h | 1 +
> drivers/hid/hid-mcp2200.c | 390 ++++++++++++++++++++++++++++++++++++++
> 4 files changed, 401 insertions(+)
> create mode 100644 drivers/hid/hid-mcp2200.c
>
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 0cea301cc9a9..3c14644b593d 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -1298,6 +1298,15 @@ config HID_ALPS
> Say Y here if you have a Alps touchpads over i2c-hid or usbhid
> and want support for its special functionalities.
>
> +config HID_MCP2200
> + tristate "Microchip MCP2200 HID USB-to-GPIO bridge"
> + depends on USB_HID && GPIOLIB
> + help
> + Provides GPIO functionality over USB-HID through MCP2200 device.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called hid-mcp2200.ko.
> +
> config HID_MCP2221
> tristate "Microchip MCP2221 HID USB-to-I2C/SMbus host support"
> depends on USB_HID && I2C
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index 8a06d0f840bc..082a728eac60 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -79,6 +79,7 @@ obj-$(CONFIG_HID_LOGITECH_HIDPP) += hid-logitech-hidpp.o
> obj-$(CONFIG_HID_MACALLY) += hid-macally.o
> obj-$(CONFIG_HID_MAGICMOUSE) += hid-magicmouse.o
> obj-$(CONFIG_HID_MALTRON) += hid-maltron.o
> +obj-$(CONFIG_HID_MCP2200) += hid-mcp2200.o
> obj-$(CONFIG_HID_MCP2221) += hid-mcp2221.o
> obj-$(CONFIG_HID_MAYFLASH) += hid-mf.o
> obj-$(CONFIG_HID_MEGAWORLD_FF) += hid-megaworld.o
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 7e499992a793..bb87ad4eb2aa 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -915,6 +915,7 @@
> #define USB_DEVICE_ID_PICK16F1454 0x0042
> #define USB_DEVICE_ID_PICK16F1454_V2 0xf2f7
> #define USB_DEVICE_ID_LUXAFOR 0xf372
> +#define USB_DEVICE_ID_MCP2200 0x00df
> #define USB_DEVICE_ID_MCP2221 0x00dd
>
> #define USB_VENDOR_ID_MICROSOFT 0x045e
> diff --git a/drivers/hid/hid-mcp2200.c b/drivers/hid/hid-mcp2200.c
> new file mode 100644
> index 000000000000..477a3915d2f0
> --- /dev/null
> +++ b/drivers/hid/hid-mcp2200.c
> @@ -0,0 +1,390 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * MCP2200 - Microchip USB to GPIO bridge
> + *
> + * Copyright (c) 2023, Johannes Roith <johannes@gnu-linux.rocks>
> + *
> + * Datasheet: https://ww1.microchip.com/downloads/en/DeviceDoc/22228A.pdf
> + * App Note for HID: https://ww1.microchip.com/downloads/en/DeviceDoc/93066A.pdf
> + */
> +#include <linux/completion.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/hid.h>
> +#include <linux/hidraw.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include "hid-ids.h"
> +
> +/* Commands codes in a raw output report */
> +#define SET_CLEAR_OUTPUTS 0x08
> +#define CONFIGURE 0x10
> +#define READ_EE 0x20
> +#define WRITE_EE 0x40
> +#define READ_ALL 0x80
> +
> +/* MCP GPIO direction encoding */
> +enum MCP_IO_DIR {
> + MCP2200_DIR_OUT = 0x00,
> + MCP2200_DIR_IN = 0x01,
> +};
> +
> +/* Altternative pin assignments */
> +#define TXLED 2
> +#define RXLED 3
> +#define USBCFG 6
> +#define SSPND 7
> +#define MCP_NGPIO 8
> +
> +/* CMD to set or clear a GPIO output */
> +struct mcp_set_clear_outputs {
> + u8 cmd;
> + u8 dummys1[10];
> + u8 set_bmap;
> + u8 clear_bmap;
> + u8 dummys2[3];
> +} __packed;
> +
> +/* CMD to configure the IOs */
> +struct mcp_configure {
> + u8 cmd;
> + u8 dummys1[3];
> + u8 io_bmap;
> + u8 config_alt_pins;
> + u8 io_default_val_bmap;
> + u8 config_alt_options;
> + u8 baud_h;
> + u8 baud_l;
> + u8 dummys2[6];
> +} __packed;
> +
> +/* CMD to read all parameters */
> +struct mcp_read_all {
> + u8 cmd;
> + u8 dummys[15];
> +} __packed;
> +
> +/* Response to the read all cmd */
> +struct mcp_read_all_resp {
> + u8 cmd;
> + u8 eep_addr;
> + u8 dummy;
> + u8 eep_val;
> + u8 io_bmap;
> + u8 config_alt_pins;
> + u8 io_default_val_bmap;
> + u8 config_alt_options;
> + u8 baud_h;
> + u8 baud_l;
> + u8 io_port_val_bmap;
> + u8 dummys[5];
> +} __packed;
> +
> +struct mcp2200 {
> + struct hid_device *hdev;
> + struct mutex lock;
> + struct completion wait_in_report;
> + u8 gpio_dir;
> + u8 gpio_val;
> + u8 gpio_inval;
> + u8 baud_h;
> + u8 baud_l;
> + u8 config_alt_pins;
> + u8 gpio_reset_val;
> + u8 config_alt_options;
> + int status;
> + struct gpio_chip gc;
> + u8 hid_report[16];
> +};
> +
> +/* this executes the READ_ALL cmd */
> +static int mcp_cmd_read_all(struct mcp2200 *mcp)
> +{
> + struct mcp_read_all *read_all;
> + int len, t;
> +
> + reinit_completion(&mcp->wait_in_report);
> +
> + mutex_lock(&mcp->lock);
> +
> + read_all = (struct mcp_read_all *) mcp->hid_report;
> + read_all->cmd = READ_ALL;
> + len = hid_hw_output_report(mcp->hdev, (u8 *) read_all,
> + sizeof(struct mcp_read_all));
> +
> + mutex_unlock(&mcp->lock);
> +
> + if (len != sizeof(struct mcp_read_all))
> + return -EINVAL;
> +
> + t = wait_for_completion_timeout(&mcp->wait_in_report,
> + msecs_to_jiffies(4000));
> + if (!t)
> + return -ETIMEDOUT;
> +
> + /* return status, negative value if wrong response was received */
> + return mcp->status;
> +}
> +
> +static void mcp_set_multiple(struct gpio_chip *gc, unsigned long *mask,
> + unsigned long *bits)
> +{
> + struct mcp2200 *mcp = gpiochip_get_data(gc);
> + u8 value;
> + int status;
> + struct mcp_set_clear_outputs *cmd;
> +
> + mutex_lock(&mcp->lock);
> + cmd = (struct mcp_set_clear_outputs *) mcp->hid_report;
> +
> + value = mcp->gpio_val & ~*mask;
> + value |= (*mask & *bits);
> +
> + cmd->cmd = SET_CLEAR_OUTPUTS;
> + cmd->set_bmap = value;
> + cmd->clear_bmap = ~(value);
> +
> + status = hid_hw_output_report(mcp->hdev, (u8 *) cmd,
> + sizeof(struct mcp_set_clear_outputs));
> +
> + if (status == sizeof(struct mcp_set_clear_outputs))
> + mcp->gpio_val = value;
> +
> + mutex_unlock(&mcp->lock);
> +}
> +
> +static void mcp_set(struct gpio_chip *gc, unsigned int gpio_nr, int value)
> +{
> + unsigned long mask = 1 << gpio_nr;
> + unsigned long bmap_value = value << gpio_nr;
> +
> + mcp_set_multiple(gc, &mask, &bmap_value);
> +}
> +
> +static int mcp_get_multiple(struct gpio_chip *gc, unsigned long *mask,
> + unsigned long *bits)
> +{
> + u32 val;
> + struct mcp2200 *mcp = gpiochip_get_data(gc);
> + int status;
> +
> + status = mcp_cmd_read_all(mcp);
> + if (status)
> + return status;
> +
> + val = mcp->gpio_inval;
> + *bits = (val & *mask);
> + return 0;
> +}
> +
> +static int mcp_get(struct gpio_chip *gc, unsigned int gpio_nr)
> +{
> + unsigned long mask = 0, bits = 0;
> +
> + mask = (1 << gpio_nr);
> + mcp_get_multiple(gc, &mask, &bits);
> + return bits > 0;
> +}
> +
> +static int mcp_get_direction(struct gpio_chip *gc, unsigned int gpio_nr)
> +{
> + struct mcp2200 *mcp = gpiochip_get_data(gc);
> +
> + return (mcp->gpio_dir & (MCP2200_DIR_IN << gpio_nr))
> + ? GPIO_LINE_DIRECTION_IN : GPIO_LINE_DIRECTION_OUT;
> +}
> +
> +static int mcp_set_direction(struct gpio_chip *gc, unsigned int gpio_nr,
> + enum MCP_IO_DIR io_direction)
> +{
> + struct mcp2200 *mcp = gpiochip_get_data(gc);
> + struct mcp_configure *conf;
> + int status;
> + /* after the configure cmd we will need to set the outputs again */
> + unsigned long mask = ~(mcp->gpio_dir); /* only set outputs */
> + unsigned long bits = mcp->gpio_val;
> + /* Offsets of alternative pins in config_alt_pins, 0 is not used */
> + u8 alt_pin_conf[8] = {SSPND, USBCFG, 0, 0, 0, 0, RXLED, TXLED};
> + u8 config_alt_pins = mcp->config_alt_pins;
> +
> + /* Read in the reset baudrate first, we need it later */
> + status = mcp_cmd_read_all(mcp);
> + if (status != 0)
> + return status;
> +
> + mutex_lock(&mcp->lock);
> + conf = (struct mcp_configure *) mcp->hid_report;
> +
> + /* configure will reset the chip! */
> + conf->cmd = CONFIGURE;
> + conf->io_bmap = (mcp->gpio_dir & ~(1 << gpio_nr))
> + | (io_direction << gpio_nr);
> + /* Don't overwrite the reset parameters */
> + conf->baud_h = mcp->baud_h;
> + conf->baud_l = mcp->baud_l;
> + conf->config_alt_options = mcp->config_alt_options;
> + conf->io_default_val_bmap = mcp->gpio_reset_val;
> + /* Adjust alt. func if necessary */
> + if (alt_pin_conf[gpio_nr])
> + config_alt_pins &= ~(1 << alt_pin_conf[gpio_nr]);
> + conf->config_alt_pins = config_alt_pins;
> +
> + status = hid_hw_output_report(mcp->hdev, (u8 *) conf,
> + sizeof(struct mcp_set_clear_outputs));
> +
> + if (status == sizeof(struct mcp_set_clear_outputs)) {
> + mcp->gpio_dir = conf->io_bmap;
> + mcp->config_alt_pins = config_alt_pins;
> + } else {
> + mutex_unlock(&mcp->lock);
> + return -EIO;
> + }
> +
> + mutex_unlock(&mcp->lock);
> +
> + /* Configure CMD will clear all IOs -> rewrite them */
> + mcp_set_multiple(gc, &mask, &bits);
> + return 0;
> +}
> +
> +static int mcp_direction_input(struct gpio_chip *gc, unsigned int gpio_nr)
> +{
> + return mcp_set_direction(gc, gpio_nr, MCP2200_DIR_IN);
> +}
> +
> +static int mcp_direction_output(struct gpio_chip *gc, unsigned int gpio_nr,
> + int value)
> +{
> + int ret;
> + unsigned long mask, bmap_value;
> +
> + mask = 1 << gpio_nr;
> + bmap_value = value << gpio_nr;
> +
> + ret = mcp_set_direction(gc, gpio_nr, MCP2200_DIR_OUT);
> + if (!ret)
> + mcp_set_multiple(gc, &mask, &bmap_value);
> + return ret;
> +}
> +
> +static const struct gpio_chip template_chip = {
> + .label = "mcp2200",
> + .owner = THIS_MODULE,
> + .get_direction = mcp_get_direction,
> + .direction_input = mcp_direction_input,
> + .direction_output = mcp_direction_output,
> + .set = mcp_set,
> + .set_multiple = mcp_set_multiple,
> + .get = mcp_get,
> + .get_multiple = mcp_get_multiple,
> + .base = -1,
> + .ngpio = MCP_NGPIO,
> + .can_sleep = true,
> +};
> +
> +/*
> + * MCP2200 uses interrupt endpoint for input reports. This function
> + * is called by HID layer when it receives i/p report from mcp2200,
> + * which is actually a response to the previously sent command.
> + */
> +static int mcp2200_raw_event(struct hid_device *hdev, struct hid_report *report,
> + u8 *data, int size)
> +{
> + struct mcp2200 *mcp = hid_get_drvdata(hdev);
> + struct mcp_read_all_resp *all_resp;
> +
> + switch (data[0]) {
> + case READ_ALL:
> + all_resp = (struct mcp_read_all_resp *) data;
> + mcp->status = 0;
> + mcp->gpio_inval = all_resp->io_port_val_bmap;
> + mcp->baud_h = all_resp->baud_h;
> + mcp->baud_l = all_resp->baud_l;
> + mcp->gpio_reset_val = all_resp->io_default_val_bmap;
> + mcp->config_alt_pins = all_resp->config_alt_pins;
> + mcp->config_alt_options = all_resp->config_alt_options;
> + break;
> + default:
> + mcp->status = -EIO;
> + break;
> + }
> +
> + complete(&mcp->wait_in_report);
> + return 0;
> +}
> +
> +static int mcp2200_probe(struct hid_device *hdev, const struct hid_device_id *id)
> +{
> + int ret;
> + struct mcp2200 *mcp;
> +
> + mcp = devm_kzalloc(&hdev->dev, sizeof(*mcp), GFP_KERNEL);
> + if (!mcp)
> + return -ENOMEM;
> +
> + ret = hid_parse(hdev);
> + if (ret) {
> + hid_err(hdev, "can't parse reports\n");
> + return ret;
> + }
> +
> + ret = hid_hw_start(hdev, 0);
> + if (ret) {
> + hid_err(hdev, "can't start hardware\n");
> + return ret;
> + }
> +
> + hid_info(hdev, "USB HID v%x.%02x Device [%s] on %s\n", hdev->version >> 8,
> + hdev->version & 0xff, hdev->name, hdev->phys);
> +
> + ret = hid_hw_open(hdev);
> + if (ret) {
> + hid_err(hdev, "can't open device\n");
> + hid_hw_stop(hdev);
> + return ret;
> + }
> +
> + mutex_init(&mcp->lock);
> + init_completion(&mcp->wait_in_report);
> + hid_set_drvdata(hdev, mcp);
> + mcp->hdev = hdev;
> +
> + mcp->gc = template_chip;
> + mcp->gc.parent = &hdev->dev;
> +
> + ret = devm_gpiochip_add_data(&hdev->dev, &mcp->gc, mcp);
> + if (ret < 0) {
You will want to call mcp2200_remove here in this error path.
> + hid_err(hdev, "Unable to register gpiochip\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static void mcp2200_remove(struct hid_device *hdev)
> +{
> + hid_hw_close(hdev);
> + hid_hw_stop(hdev);
> +}
> +
> +static const struct hid_device_id mcp2200_devices[] = {
> + { HID_USB_DEVICE(USB_VENDOR_ID_MICROCHIP, USB_DEVICE_ID_MCP2200) },
> + { }
> +};
> +MODULE_DEVICE_TABLE(hid, mcp2200_devices);
> +
> +static struct hid_driver mcp2200_driver = {
> + .name = "mcp2200",
> + .id_table = mcp2200_devices,
> + .probe = mcp2200_probe,
> + .remove = mcp2200_remove,
> + .raw_event = mcp2200_raw_event,
> +};
> +
> +/* Register with HID core */
> +module_hid_driver(mcp2200_driver);
> +
> +MODULE_AUTHOR("Johannes Roith <johannes@gnu-linux.rocks>");
> +MODULE_DESCRIPTION("MCP2200 Microchip HID USB to GPIO bridge");
> +MODULE_LICENSE("GPL");
This patch looks great. I just have one minor comment with regards to
the refactor now that devm is not being used to handle hid cleanup.
--
Thanks,
Rahul Rameshbabu
^ permalink raw reply
* Re: [PATCH] hid: cp2112: Fix IRQ shutdown stopping polling for all IRQs on chip
From: Danny Kaehn @ 2023-09-20 19:12 UTC (permalink / raw)
To: andriy.shevchenko@linux.intel.com
Cc: jikos@kernel.org, benjamin.tissoires@redhat.com,
linux-input@vger.kernel.org, Ethan Twardy
In-Reply-To: <ZQr0r3Ux/rkWQ8N5@smile.fi.intel.com>
On Wed, 2023-09-20 at 16:33 +0300, Andy Shevchenko wrote:
> On Tue, Sep 19, 2023 at 04:24:26PM -0500, Danny Kaehn wrote:
> > Previously cp2112_gpio_irq_shutdown always cancelled the
>
> cp2112_gpio_irq_shutdown()
> > _remove occurred (a.e. the cp2112 is unplugged or system rebooted).
>
> _remove()
>
> > + if (!dev->irq_mask)
> > + {
>
> The style tells to use these on a single line.
ACK -- will fix all in v2. Thanks for the comments.
Danny Kaehn
^ permalink raw reply
* Re: [PATCH] hid: cp2112: Fix duplicate workqueue initialization
From: Danny Kaehn @ 2023-09-20 19:10 UTC (permalink / raw)
To: andriy.shevchenko@linux.intel.com
Cc: jikos@kernel.org, benjamin.tissoires@redhat.com,
linux-input@vger.kernel.org, Ethan Twardy
In-Reply-To: <ZQrt6k/usVXlB223@smile.fi.intel.com>
On Wed, 2023-09-20 at 16:04 +0300, Andy Shevchenko wrote:
> On Tue, Sep 19, 2023 at 04:22:45PM -0500, Danny Kaehn wrote:
> > Previously the cp2112 driver called INIT_DELAYED_WORK within
> > cp2112_gpio_irq_startup, resulting in duplicate initilizations of the
> > workqueue on subsequent IRQ startups following an initial request. This
> > resulted in a warning in set_work_data in workqueue.c, as well as a rare
> > NULL dereference within process_one_work in workqueue.c.
> >
> > Initialize the workqueue within _probe instead.
>
> Does it deserve a Fixes tag?
I'm not sure -- it does technically fix 13de9cca514ed63604263cad87ca8cb36e9b6489
(HID: cp2112: add IRQ chip handling), but does not apply cleanly to that
revision (a.e. with git am)
(my understanding is that 'Fixes' is used for stable kernel maintenance?)
Thanks,
Danny Kaehn
^ permalink raw reply
* Re: [PATCH] Input: powermate - fix use-after-free in powermate_config_complete
From: kernel test robot @ 2023-09-20 18:35 UTC (permalink / raw)
To: Javier Carrasco, Dmitry Torokhov
Cc: oe-kbuild-all, linux-input, linux-kernel, Javier Carrasco,
syzbot+0434ac83f907a1dbdd1e
In-Reply-To: <20230916-topic-powermate_use_after_free-v1-1-2ffa46652869@gmail.com>
Hi Javier,
kernel test robot noticed the following build errors:
[auto build test ERROR on 0bb80ecc33a8fb5a682236443c1e740d5c917d1d]
url: https://github.com/intel-lab-lkp/linux/commits/Javier-Carrasco/Input-powermate-fix-use-after-free-in-powermate_config_complete/20230917-052943
base: 0bb80ecc33a8fb5a682236443c1e740d5c917d1d
patch link: https://lore.kernel.org/r/20230916-topic-powermate_use_after_free-v1-1-2ffa46652869%40gmail.com
patch subject: [PATCH] Input: powermate - fix use-after-free in powermate_config_complete
config: powerpc-ppc6xx_defconfig (https://download.01.org/0day-ci/archive/20230921/202309210232.d7MpKEIm-lkp@intel.com/config)
compiler: powerpc-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230921/202309210232.d7MpKEIm-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309210232.d7MpKEIm-lkp@intel.com/
All errors (new ones prefixed by >>):
drivers/input/misc/powermate.c: In function 'powermate_config_complete':
>> drivers/input/misc/powermate.c:201:21: error: 'status' undeclared (first use in this function); did you mean 'kstatfs'?
201 | if (status == -ENOENT || status == -ESHUTDOWN)
| ^~~~~~
| kstatfs
drivers/input/misc/powermate.c:201:21: note: each undeclared identifier is reported only once for each function it appears in
vim +201 drivers/input/misc/powermate.c
192
193 /* Called when our asynchronous control message completes. We may need to issue another immediately */
194 static void powermate_config_complete(struct urb *urb)
195 {
196 struct powermate_device *pm = urb->context;
197 unsigned long flags;
198
199 if (urb->status) {
200 printk(KERN_ERR "powermate: config urb returned %d\n", urb->status);
> 201 if (status == -ENOENT || status == -ESHUTDOWN)
202 return;
203 }
204
205 spin_lock_irqsave(&pm->lock, flags);
206 powermate_sync_state(pm);
207 spin_unlock_irqrestore(&pm->lock, flags);
208 }
209
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH v2 7/9] iio: hid-sensor-als: Add light chromaticity support
From: srinivas pandruvada @ 2023-09-20 17:10 UTC (permalink / raw)
To: Basavaraj Natikar, jikos, benjamin.tissoires, jic23, lars,
linux-input, linux-iio
In-Reply-To: <20230919081054.2050714-8-Basavaraj.Natikar@amd.com>
On Tue, 2023-09-19 at 13:40 +0530, Basavaraj Natikar wrote:
> In most cases, ambient color sensors also support the x and y light
> colors, which represent the coordinates on the CIE 1931 chromaticity
> diagram. Thus, add light chromaticity x and y.
>
> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
Acked-by: Srinivas Pandruvada<srinivas.pandruvada@linux.intel.com>
> ---
> drivers/iio/light/hid-sensor-als.c | 63
> ++++++++++++++++++++++++++++++
> include/linux/hid-sensor-ids.h | 3 ++
> 2 files changed, 66 insertions(+)
>
> diff --git a/drivers/iio/light/hid-sensor-als.c
> b/drivers/iio/light/hid-sensor-als.c
> index 16a3f1941c27..c9d114ff080a 100644
> --- a/drivers/iio/light/hid-sensor-als.c
> +++ b/drivers/iio/light/hid-sensor-als.c
> @@ -17,6 +17,8 @@ enum {
> CHANNEL_SCAN_INDEX_INTENSITY,
> CHANNEL_SCAN_INDEX_ILLUM,
> CHANNEL_SCAN_INDEX_COLOR_TEMP,
> + CHANNEL_SCAN_INDEX_CHROMATICITY_X,
> + CHANNEL_SCAN_INDEX_CHROMATICITY_Y,
> CHANNEL_SCAN_INDEX_MAX
> };
>
> @@ -76,6 +78,30 @@ static const struct iio_chan_spec als_channels[] =
> {
> BIT(IIO_CHAN_INFO_HYSTERESIS_RELATIVE),
> .scan_index = CHANNEL_SCAN_INDEX_COLOR_TEMP,
> },
> + {
> + .type = IIO_CHROMATICITY,
> + .modified = 1,
> + .channel2 = IIO_MOD_X,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OFFSET)
> |
> + BIT(IIO_CHAN_INFO_SCALE) |
> + BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> + BIT(IIO_CHAN_INFO_HYSTERESIS) |
> + BIT(IIO_CHAN_INFO_HYSTERESIS_RELATIVE),
> + .scan_index = CHANNEL_SCAN_INDEX_CHROMATICITY_X,
> + },
> + {
> + .type = IIO_CHROMATICITY,
> + .modified = 1,
> + .channel2 = IIO_MOD_Y,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OFFSET)
> |
> + BIT(IIO_CHAN_INFO_SCALE) |
> + BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> + BIT(IIO_CHAN_INFO_HYSTERESIS) |
> + BIT(IIO_CHAN_INFO_HYSTERESIS_RELATIVE),
> + .scan_index = CHANNEL_SCAN_INDEX_CHROMATICITY_Y,
> + },
> IIO_CHAN_SOFT_TIMESTAMP(CHANNEL_SCAN_INDEX_TIMESTAMP)
> };
>
> @@ -119,6 +145,16 @@ static int als_read_raw(struct iio_dev
> *indio_dev,
> min = als_state->als[chan-
> >scan_index].logical_minimum;
> address =
> HID_USAGE_SENSOR_LIGHT_COLOR_TEMPERATURE;
> break;
> + case CHANNEL_SCAN_INDEX_CHROMATICITY_X:
> + report_id = als_state->als[chan-
> >scan_index].report_id;
> + min = als_state->als[chan-
> >scan_index].logical_minimum;
> + address =
> HID_USAGE_SENSOR_LIGHT_CHROMATICITY_X;
> + break;
> + case CHANNEL_SCAN_INDEX_CHROMATICITY_Y:
> + report_id = als_state->als[chan-
> >scan_index].report_id;
> + min = als_state->als[chan-
> >scan_index].logical_minimum;
> + address =
> HID_USAGE_SENSOR_LIGHT_CHROMATICITY_Y;
> + break;
> default:
> report_id = -1;
> break;
> @@ -243,6 +279,14 @@ static int als_capture_sample(struct
> hid_sensor_hub_device *hsdev,
> als_state->scan.illum[CHANNEL_SCAN_INDEX_COLOR_TEMP]
> = sample_data;
> ret = 0;
> break;
> + case HID_USAGE_SENSOR_LIGHT_CHROMATICITY_X:
> + als_state-
> >scan.illum[CHANNEL_SCAN_INDEX_CHROMATICITY_X] = sample_data;
> + ret = 0;
> + break;
> + case HID_USAGE_SENSOR_LIGHT_CHROMATICITY_Y:
> + als_state-
> >scan.illum[CHANNEL_SCAN_INDEX_CHROMATICITY_Y] = sample_data;
> + ret = 0;
> + break;
> case HID_USAGE_SENSOR_TIME_TIMESTAMP:
> als_state->timestamp =
> hid_sensor_convert_timestamp(&als_state->common_attributes,
>
> *(s64 *)raw_data);
> @@ -291,6 +335,25 @@ static int als_parse_report(struct
> platform_device *pdev,
> st->als[CHANNEL_SCAN_INDEX_COLOR_TEMP].index,
> st->als[CHANNEL_SCAN_INDEX_COLOR_TEMP].report_id);
>
> + for (i = 0; i < 2; i++) {
> + int next_scan_index =
> CHANNEL_SCAN_INDEX_CHROMATICITY_X + i;
> +
> + ret = sensor_hub_input_get_attribute_info(hsdev,
> + HID_INPUT_REPORT, usage_id,
> + HID_USAGE_SENSOR_LIGHT_CHROMATICITY_X
> + i,
> + &st->als[next_scan_index]);
> + if (ret < 0)
> + return ret;
> +
> + als_adjust_channel_bit_mask(channels,
> + CHANNEL_SCAN_INDEX_CHROMATICI
> TY_X + i,
> + st-
> >als[next_scan_index].size);
> +
> + dev_dbg(&pdev->dev, "als %x:%x\n",
> + st->als[next_scan_index].index,
> + st->als[next_scan_index].report_id);
> + }
> +
> st->scale_precision = hid_sensor_format_scale(usage_id,
> &st-
> >als[CHANNEL_SCAN_INDEX_INTENSITY],
> &st->scale_pre_decml, &st-
> >scale_post_decml);
> diff --git a/include/linux/hid-sensor-ids.h b/include/linux/hid-
> sensor-ids.h
> index 8af4fb3e0254..6730ee900ee1 100644
> --- a/include/linux/hid-sensor-ids.h
> +++ b/include/linux/hid-sensor-ids.h
> @@ -22,6 +22,9 @@
> #define
> HID_USAGE_SENSOR_DATA_LIGHT 0x2004d0
> #define
> HID_USAGE_SENSOR_LIGHT_ILLUM 0x2004d1
> #define
> HID_USAGE_SENSOR_LIGHT_COLOR_TEMPERATURE 0x2004d2
> +#define
> HID_USAGE_SENSOR_LIGHT_CHROMATICITY 0x2004d3
> +#define
> HID_USAGE_SENSOR_LIGHT_CHROMATICITY_X 0x2004d4
> +#define
> HID_USAGE_SENSOR_LIGHT_CHROMATICITY_Y 0x2004d5
>
> /* PROX (200011) */
> #define HID_USAGE_SENSOR_PROX
> 0x200011
^ permalink raw reply
* Re: [PATCH v2 3/9] iio: hid-sensor-als: Add light color temperature support
From: srinivas pandruvada @ 2023-09-20 17:00 UTC (permalink / raw)
To: Basavaraj Natikar, jikos, benjamin.tissoires, jic23, lars,
linux-input, linux-iio
In-Reply-To: <20230919081054.2050714-4-Basavaraj.Natikar@amd.com>
On Tue, 2023-09-19 at 13:40 +0530, Basavaraj Natikar wrote:
> In most cases, ambient color sensors also support light color
> temperature.
> As a result, add support of light color temperature.
>
> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
Acked-by: Srinivas Pandruvada<srinivas.pandruvada@linux.intel.com>
> ---
> drivers/iio/light/hid-sensor-als.c | 37
> ++++++++++++++++++++++++++++--
> include/linux/hid-sensor-ids.h | 1 +
> 2 files changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/light/hid-sensor-als.c
> b/drivers/iio/light/hid-sensor-als.c
> index efb1f8862b28..16a3f1941c27 100644
> --- a/drivers/iio/light/hid-sensor-als.c
> +++ b/drivers/iio/light/hid-sensor-als.c
> @@ -14,8 +14,9 @@
> #include "../common/hid-sensors/hid-sensor-trigger.h"
>
> enum {
> - CHANNEL_SCAN_INDEX_INTENSITY = 0,
> - CHANNEL_SCAN_INDEX_ILLUM = 1,
> + CHANNEL_SCAN_INDEX_INTENSITY,
> + CHANNEL_SCAN_INDEX_ILLUM,
> + CHANNEL_SCAN_INDEX_COLOR_TEMP,
> CHANNEL_SCAN_INDEX_MAX
> };
>
> @@ -65,6 +66,16 @@ static const struct iio_chan_spec als_channels[] =
> {
> BIT(IIO_CHAN_INFO_HYSTERESIS_RELATIVE),
> .scan_index = CHANNEL_SCAN_INDEX_ILLUM,
> },
> + {
> + .type = IIO_COLORTEMP,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OFFSET)
> |
> + BIT(IIO_CHAN_INFO_SCALE) |
> + BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> + BIT(IIO_CHAN_INFO_HYSTERESIS) |
> + BIT(IIO_CHAN_INFO_HYSTERESIS_RELATIVE),
> + .scan_index = CHANNEL_SCAN_INDEX_COLOR_TEMP,
> + },
> IIO_CHAN_SOFT_TIMESTAMP(CHANNEL_SCAN_INDEX_TIMESTAMP)
> };
>
> @@ -103,6 +114,11 @@ static int als_read_raw(struct iio_dev
> *indio_dev,
> min = als_state->als[chan-
> >scan_index].logical_minimum;
> address = HID_USAGE_SENSOR_LIGHT_ILLUM;
> break;
> + case CHANNEL_SCAN_INDEX_COLOR_TEMP:
> + report_id = als_state->als[chan-
> >scan_index].report_id;
> + min = als_state->als[chan-
> >scan_index].logical_minimum;
> + address =
> HID_USAGE_SENSOR_LIGHT_COLOR_TEMPERATURE;
> + break;
> default:
> report_id = -1;
> break;
> @@ -223,6 +239,10 @@ static int als_capture_sample(struct
> hid_sensor_hub_device *hsdev,
> als_state->scan.illum[CHANNEL_SCAN_INDEX_ILLUM] =
> sample_data;
> ret = 0;
> break;
> + case HID_USAGE_SENSOR_LIGHT_COLOR_TEMPERATURE:
> + als_state->scan.illum[CHANNEL_SCAN_INDEX_COLOR_TEMP]
> = sample_data;
> + ret = 0;
> + break;
> case HID_USAGE_SENSOR_TIME_TIMESTAMP:
> als_state->timestamp =
> hid_sensor_convert_timestamp(&als_state->common_attributes,
>
> *(s64 *)raw_data);
> @@ -258,6 +278,19 @@ static int als_parse_report(struct
> platform_device *pdev,
> st->als[i].report_id);
> }
>
> + ret = sensor_hub_input_get_attribute_info(hsdev,
> HID_INPUT_REPORT,
> + usage_id,
> + HID_USAGE_SENSOR_LIGHT_COLOR_TEMPERAT
> URE,
> + &st-
> >als[CHANNEL_SCAN_INDEX_COLOR_TEMP]);
> + if (ret < 0)
> + return ret;
> + als_adjust_channel_bit_mask(channels,
> CHANNEL_SCAN_INDEX_COLOR_TEMP,
> + st-
> >als[CHANNEL_SCAN_INDEX_COLOR_TEMP].size);
> +
> + dev_dbg(&pdev->dev, "als %x:%x\n",
> + st->als[CHANNEL_SCAN_INDEX_COLOR_TEMP].index,
> + st->als[CHANNEL_SCAN_INDEX_COLOR_TEMP].report_id);
> +
> st->scale_precision = hid_sensor_format_scale(usage_id,
> &st-
> >als[CHANNEL_SCAN_INDEX_INTENSITY],
> &st->scale_pre_decml, &st-
> >scale_post_decml);
> diff --git a/include/linux/hid-sensor-ids.h b/include/linux/hid-
> sensor-ids.h
> index 13b1e65fbdcc..8af4fb3e0254 100644
> --- a/include/linux/hid-sensor-ids.h
> +++ b/include/linux/hid-sensor-ids.h
> @@ -21,6 +21,7 @@
> #define
> HID_USAGE_SENSOR_ALS 0x200041
> #define
> HID_USAGE_SENSOR_DATA_LIGHT 0x2004d0
> #define
> HID_USAGE_SENSOR_LIGHT_ILLUM 0x2004d1
> +#define
> HID_USAGE_SENSOR_LIGHT_COLOR_TEMPERATURE 0x2004d2
>
> /* PROX (200011) */
> #define HID_USAGE_SENSOR_PROX
> 0x200011
^ permalink raw reply
* Re: [PATCH v2 1/9] iio: hid-sensor-als: Use channel index to support more hub attributes
From: srinivas pandruvada @ 2023-09-20 16:59 UTC (permalink / raw)
To: Basavaraj Natikar, jikos, benjamin.tissoires, jic23, lars,
linux-input, linux-iio
In-Reply-To: <20230919081054.2050714-2-Basavaraj.Natikar@amd.com>
On Tue, 2023-09-19 at 13:40 +0530, Basavaraj Natikar wrote:
> Sensor hub attributes can be extended to support more channels.
> Repeat
> the reading for the two existing channels and store them separately.
> It
> still operates in the same manner as before where there was just one
> entry. So in order to support more sensor hub attributes for ALS use
> channel index to get specific sensor hub attributes.
>
> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
Acked-by: Srinivas Pandruvada<srinivas.pandruvada@linux.intel.com>
> ---
> drivers/iio/light/hid-sensor-als.c | 38 ++++++++++++++++------------
> --
> 1 file changed, 20 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/iio/light/hid-sensor-als.c
> b/drivers/iio/light/hid-sensor-als.c
> index eb1aedad7edc..efb1f8862b28 100644
> --- a/drivers/iio/light/hid-sensor-als.c
> +++ b/drivers/iio/light/hid-sensor-als.c
> @@ -24,7 +24,7 @@ enum {
> struct als_state {
> struct hid_sensor_hub_callbacks callbacks;
> struct hid_sensor_common common_attributes;
> - struct hid_sensor_hub_attribute_info als_illum;
> + struct hid_sensor_hub_attribute_info
> als[CHANNEL_SCAN_INDEX_MAX];
> struct {
> u32 illum[CHANNEL_SCAN_INDEX_MAX];
> u64 timestamp __aligned(8);
> @@ -99,8 +99,8 @@ static int als_read_raw(struct iio_dev *indio_dev,
> switch (chan->scan_index) {
> case CHANNEL_SCAN_INDEX_INTENSITY:
> case CHANNEL_SCAN_INDEX_ILLUM:
> - report_id = als_state->als_illum.report_id;
> - min = als_state->als_illum.logical_minimum;
> + report_id = als_state->als[chan-
> >scan_index].report_id;
> + min = als_state->als[chan-
> >scan_index].logical_minimum;
> address = HID_USAGE_SENSOR_LIGHT_ILLUM;
> break;
> default:
> @@ -242,22 +242,24 @@ static int als_parse_report(struct
> platform_device *pdev,
> struct als_state *st)
> {
> int ret;
> + int i;
> +
> + for (i = 0; i <= CHANNEL_SCAN_INDEX_ILLUM; ++i) {
> + ret = sensor_hub_input_get_attribute_info(hsdev,
> + HID_INPUT_REPORT,
> + usage_id,
> + HID_USAGE_SENSOR_LIGH
> T_ILLUM,
> + &st->als[i]);
> + if (ret < 0)
> + return ret;
> + als_adjust_channel_bit_mask(channels, i, st-
> >als[i].size);
> +
> + dev_dbg(&pdev->dev, "als %x:%x\n", st->als[i].index,
> + st->als[i].report_id);
> + }
>
> - ret = sensor_hub_input_get_attribute_info(hsdev,
> HID_INPUT_REPORT,
> - usage_id,
> - HID_USAGE_SENSOR_LIGHT_ILLUM,
> - &st->als_illum);
> - if (ret < 0)
> - return ret;
> - als_adjust_channel_bit_mask(channels,
> CHANNEL_SCAN_INDEX_INTENSITY,
> - st->als_illum.size);
> - als_adjust_channel_bit_mask(channels,
> CHANNEL_SCAN_INDEX_ILLUM,
> - st->als_illum.size);
> -
> - dev_dbg(&pdev->dev, "als %x:%x\n", st->als_illum.index,
> - st->als_illum.report_id);
> -
> - st->scale_precision = hid_sensor_format_scale(usage_id, &st-
> >als_illum,
> + st->scale_precision = hid_sensor_format_scale(usage_id,
> + &st-
> >als[CHANNEL_SCAN_INDEX_INTENSITY],
> &st->scale_pre_decml, &st-
> >scale_post_decml);
>
> return ret;
^ permalink raw reply
* Re: [PATCH 42/52] input: sun4i-ps2 - Convert to platform remove callback returning void
From: Jernej Škrabec @ 2023-09-20 16:07 UTC (permalink / raw)
To: Dmitry Torokhov, Uwe Kleine-König
Cc: Chen-Yu Tsai, Samuel Holland, linux-input, linux-arm-kernel,
linux-sunxi, kernel
In-Reply-To: <20230920125829.1478827-43-u.kleine-koenig@pengutronix.de>
Dne sreda, 20. september 2023 ob 14:58:19 CEST je Uwe Kleine-König napisal(a):
> The .remove() callback for a platform driver returns an int which makes
> many driver authors wrongly assume it's possible to do error handling by
> returning an error code. However the value returned is ignored (apart
> from emitting a warning) and this typically results in resource leaks.
> To improve here there is a quest to make the remove callback return
> void. In the first step of this quest all drivers are converted to
> .remove_new() which already returns void. Eventually after all drivers
> are converted, .remove_new() will be renamed to .remove().
>
> Trivially convert this driver from always returning zero in the remove
> callback to the void returning variant.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Acked-by: Jernej Skrabec <jernej.skrabec@gmail.com>
Best regards,
Jernej
> ---
> drivers/input/serio/sun4i-ps2.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/input/serio/sun4i-ps2.c
> b/drivers/input/serio/sun4i-ps2.c index eb262640192e..aec66d9f5176 100644
> --- a/drivers/input/serio/sun4i-ps2.c
> +++ b/drivers/input/serio/sun4i-ps2.c
> @@ -297,7 +297,7 @@ static int sun4i_ps2_probe(struct platform_device *pdev)
> return error;
> }
>
> -static int sun4i_ps2_remove(struct platform_device *pdev)
> +static void sun4i_ps2_remove(struct platform_device *pdev)
> {
> struct sun4i_ps2data *drvdata = platform_get_drvdata(pdev);
>
> @@ -311,8 +311,6 @@ static int sun4i_ps2_remove(struct platform_device
> *pdev) iounmap(drvdata->reg_base);
>
> kfree(drvdata);
> -
> - return 0;
> }
>
> static const struct of_device_id sun4i_ps2_match[] = {
> @@ -324,7 +322,7 @@ MODULE_DEVICE_TABLE(of, sun4i_ps2_match);
>
> static struct platform_driver sun4i_ps2_driver = {
> .probe = sun4i_ps2_probe,
> - .remove = sun4i_ps2_remove,
> + .remove_new = sun4i_ps2_remove,
> .driver = {
> .name = DRIVER_NAME,
> .of_match_table = sun4i_ps2_match,
^ permalink raw reply
* Re: [PATCH 49/52] input: sun4i-ts - Convert to platform remove callback returning void
From: Jernej Škrabec @ 2023-09-20 16:07 UTC (permalink / raw)
To: Dmitry Torokhov, Uwe Kleine-König
Cc: Chen-Yu Tsai, Samuel Holland, Rafael J. Wysocki,
Krzysztof Kozlowski, Daniel Lezcano, Jonathan Corbet, linux-input,
linux-arm-kernel, linux-sunxi, kernel
In-Reply-To: <20230920125829.1478827-50-u.kleine-koenig@pengutronix.de>
Dne sreda, 20. september 2023 ob 14:58:26 CEST je Uwe Kleine-König napisal(a):
> The .remove() callback for a platform driver returns an int which makes
> many driver authors wrongly assume it's possible to do error handling by
> returning an error code. However the value returned is ignored (apart
> from emitting a warning) and this typically results in resource leaks.
> To improve here there is a quest to make the remove callback return
> void. In the first step of this quest all drivers are converted to
> .remove_new() which already returns void. Eventually after all drivers
> are converted, .remove_new() will be renamed to .remove().
>
> Trivially convert this driver from always returning zero in the remove
> callback to the void returning variant.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Acked-by: Jernej Skrabec <jernej.skrabec@gmail.com>
Best regards,
Jernej
> ---
> drivers/input/touchscreen/sun4i-ts.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/input/touchscreen/sun4i-ts.c
> b/drivers/input/touchscreen/sun4i-ts.c index bb3c6072fc82..92b2b840b4b7
> 100644
> --- a/drivers/input/touchscreen/sun4i-ts.c
> +++ b/drivers/input/touchscreen/sun4i-ts.c
> @@ -375,7 +375,7 @@ static int sun4i_ts_probe(struct platform_device *pdev)
> return 0;
> }
>
> -static int sun4i_ts_remove(struct platform_device *pdev)
> +static void sun4i_ts_remove(struct platform_device *pdev)
> {
> struct sun4i_ts_data *ts = platform_get_drvdata(pdev);
>
> @@ -385,8 +385,6 @@ static int sun4i_ts_remove(struct platform_device *pdev)
>
> /* Deactivate all IRQs */
> writel(0, ts->base + TP_INT_FIFOC);
> -
> - return 0;
> }
>
> static const struct of_device_id sun4i_ts_of_match[] = {
> @@ -403,7 +401,7 @@ static struct platform_driver sun4i_ts_driver = {
> .of_match_table = sun4i_ts_of_match,
> },
> .probe = sun4i_ts_probe,
> - .remove = sun4i_ts_remove,
> + .remove_new = sun4i_ts_remove,
> };
>
> module_platform_driver(sun4i_ts_driver);
^ permalink raw reply
* Re: [PATCH] HID: i2c-hid: fix handling of unpopulated devices
From: Doug Anderson @ 2023-09-20 15:41 UTC (permalink / raw)
To: Johan Hovold
Cc: Johan Hovold, Jiri Kosina, Benjamin Tissoires, linux-input,
linux-kernel, Maxime Ripard, Dmitry Torokhov, LinusW, Rob Herring,
Krzysztof Kozlowski, Conor Dooley,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list:GPIO SUBSYSTEM
In-Reply-To: <ZQqemN8P2VKgxhsV@hovoldconsulting.com>
Hi,
On Wed, Sep 20, 2023 at 12:26 AM Johan Hovold <johan@kernel.org> wrote:
>
> On Tue, Sep 19, 2023 at 11:15:46AM -0700, Doug Anderson wrote:
> > On Tue, Sep 19, 2023 at 12:07 AM Johan Hovold <johan@kernel.org> wrote:
>
> > > But regardless of what a long-term proper solution to this may look
> > > like, we need to fix the regression in 6.6-rc1 by restoring the old
> > > behaviour.
> >
> > OK, fair enough. I'll take a look at your patch, though I think the
> > person that really needs to approve it is Benjamin...
> >
> > Style-wise, I will say that Benjamin really wanted to keep the "panel
> > follower" code out of the main probe routine. Some of my initial
> > patches adding "panel follower" looked more like the results after
> > your patch but Benjamin really wasn't happy until there were no
> > special cases for panel-followers in the main probe routine. This is
> > why the code is structured as it is.
>
> Ok, I prefer not hiding away things like that as it obscures what's
> really going on, for example, in this case, that you register a device
> without really having probed it.
I can see your reasoning and I think that intuition is why the earlier
versions of my patches had explicit "panel follower" logic in probe.
However, Benjamin really liked the logic abstracted out.
> As I alluded to in the commit message, you probably want to be able to
> support second-source touchscreen panel followers as well at some point
> and then deferring checking whether device is populated until the panel
> is powered on is not going to work.
Yeah, I've been pondering this too. I _think_ it would work OK-ish if
both devices probed and then only one of the two would actually make
the sub-HID devices. So you'd actually see both devices succeed at
probing but only one of them would actually be functional. It's a bit
ugly, though. :( Maybe marginally better would be if we could figure
out how to have the device which fails to get its interrupt later
unbind itself, if that's possible...
The only other thought I had would be to have the parent i2c bus
understand that it had children that were panel followers, which it
should be able to do by seeing the "panel" attribute in their device
tree. Then the i2c bus could itself register as a panel follower and
could wait to probe its children until they were powered on. This
could happen in the i2c core so we didn't have to add code like this
to all i2c bus drivers. ...and, if necessary, we could add this to
other busses like SPI. It feels a little awkward but could work.
> I skimmed the thread were you added this, but I'm not sure I saw any
> reason for why powering on the panel follower temporarily during probe
> would not work?
My first instinct says we can't do this, but let's think about it...
In general the "panel follower" API is designed to give all the
decision making about when to power things on and off to the panel
driver, which is controlled by DRM.
The reason for this is from experience I had when dealing with the
Samsung ATNA33XC20 panel that's on "sc7180-trogdor-homestar". The TCON
on that panel tended to die if you didn't sequence it just right.
Specifically, if you were sending pixels to the panel and then stopped
then you absolutely needed to power the panel off and on again. Folks
I talked to even claimed that the panel was working "to spec" since,
in the "Power Sequencing" section of the eDP spec it clearly shows
that you _must_ turn the panel off and on again after you stop giving
it bits. ...this is despite the fact that no other panel I've worked
with cares. ;-)
On homestar, since we didn't have the "panel follower" API, we ended
up adding cost to the hardware and putting the panel and touchscreens
on different power rails. However, I wanted to make sure that if we
ran into a similar situation in the future (or maybe if we were trying
to make hardware work that we didn't have control over) that we could
solve it.
The other reason for giving full control to the panel driver is just
how userspace usually works. Right now userspace tends to power off
panels if they're not used (like if a lid is closed on a laptop) but
doesn't necessarily power off the touchscreen. Thus if the touchscreen
has the ability to keep things powered on then we'd never get to a low
power state.
The above all explains why panel followers like the touchscreen
shouldn't be able to keep power on. However, you are specifically
suggesting that we just turn the power on temporarily during probe. As
I think about that, it might be possible? I guess you'd have to
temporarily block DRM from changing the state of the panel while the
touchscreen is probing. Then if the panel was off then you'd turn it
on briefly, do your probe, and then turn it off again. If the panel
was on then by blocking DRM you'd ensure that it stayed on. I'm not
sure how palatable that would be or if there are any other tricky
parts I'm not thinking about.
> > Thinking that way, is there any reason you can't just move the
> > i2c_hid_init_irq() into __do_i2c_hid_core_initial_power_up()? You
> > could replace the call to enable_irq() with it and then remove the
> > `IRQF_NO_AUTOEN` flag? I think that would also solve the issue if you
> > wanted to use a 2nd source + the panel follower concept? Both devices
> > would probe, but only one of them would actually grab the interrupt
> > and only one of them would actually create real HID devices. We might
> > need to do some work to keep from trying again at every poweron of the
> > panel, but it would probably be workable? I think this would also be a
> > smaller change...
>
> That was my first idea as well, but conceptually it is more correct to
> request resources at probe time and not at some later point when you can
> no longer fail probe.
>
> You'd also need to handle the fact that the interrupt may never have
> been requested when remove() is called, which adds unnecessary
> complexity.
I don't think it's a lot of complexity, is it? Just an extra "if" statement...
-Doug
^ permalink raw reply
* Re: [PATCH v2 9/9] HID: amd_sfh: Add light chromaticity for SFH1.1
From: Jiri Kosina @ 2023-09-20 15:26 UTC (permalink / raw)
To: Basavaraj Natikar
Cc: benjamin.tissoires, jic23, lars, srinivas.pandruvada, linux-input,
linux-iio
In-Reply-To: <20230919081054.2050714-10-Basavaraj.Natikar@amd.com>
On Tue, 19 Sep 2023, Basavaraj Natikar wrote:
> In most cases, ambient color sensors also support the x and y light
> colors, which represent the coordinates on the CIE 1931 chromaticity
> diagram. Thus, add light chromaticity x and y for SFH1.1.
>
> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
Acked-by: Jiri Kosina <jkosina@suse.cz>
> ---
> drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_desc.c | 5 ++++-
> drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.h | 2 ++
> 2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_desc.c b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_desc.c
> index f100aaafa167..8a037de08e92 100644
> --- a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_desc.c
> +++ b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_desc.c
> @@ -238,8 +238,11 @@ static u8 get_input_rep(u8 current_index, int sensor_idx, int report_id,
> als_input.illuminance_value = float_to_int(als_data.lux);
>
> memcpy_fromio(&binfo, mp2->vsbase, sizeof(struct sfh_base_info));
> - if (binfo.sbase.s_prop[ALS_IDX].sf.feat & 0x2)
> + if (binfo.sbase.s_prop[ALS_IDX].sf.feat & 0x2) {
> als_input.light_color_temp = als_data.light_color_temp;
> + als_input.chromaticity_x_value = float_to_int(als_data.chromaticity_x);
> + als_input.chromaticity_y_value = float_to_int(als_data.chromaticity_y);
> + }
>
> report_size = sizeof(als_input);
> memcpy(input_report, &als_input, sizeof(als_input));
> diff --git a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.h b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.h
> index 6f6f5db150c3..656c3e95ef8c 100644
> --- a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.h
> +++ b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.h
> @@ -147,6 +147,8 @@ struct sfh_als_data {
> struct sfh_common_data commondata;
> u32 lux;
> u32 light_color_temp;
> + u32 chromaticity_x;
> + u32 chromaticity_y;
> };
>
> struct hpd_status {
> --
> 2.25.1
>
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH v2 8/9] HID: amd_sfh: Add light chromaticity support
From: Jiri Kosina @ 2023-09-20 15:25 UTC (permalink / raw)
To: Basavaraj Natikar
Cc: benjamin.tissoires, jic23, lars, srinivas.pandruvada, linux-input,
linux-iio
In-Reply-To: <20230919081054.2050714-9-Basavaraj.Natikar@amd.com>
On Tue, 19 Sep 2023, Basavaraj Natikar wrote:
> In most cases, ambient color sensors also support the x and y light
> colors, which represent the coordinates on the CIE 1931 chromaticity
> diagram. Thus, add light chromaticity x and y.
>
> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
Acked-by: Jiri Kosina <jkosina@suse.cz>
> ---
> .../amd-sfh-hid/hid_descriptor/amd_sfh_hid_desc.c | 5 ++++-
> .../amd-sfh-hid/hid_descriptor/amd_sfh_hid_desc.h | 2 ++
> .../hid_descriptor/amd_sfh_hid_report_desc.h | 14 ++++++++++++++
> 3 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_desc.c b/drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_desc.c
> index b7e732ec4806..ef1f9be8b893 100644
> --- a/drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_desc.c
> +++ b/drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_desc.c
> @@ -258,8 +258,11 @@ static u8 get_input_report(u8 current_index, int sensor_idx, int report_id,
> als_input.illuminance_value =
> (int)sensor_virt_addr[0] / AMD_SFH_FW_MULTIPLIER;
>
> - if (sensor_idx == ACS_IDX)
> + if (sensor_idx == ACS_IDX) {
> als_input.light_color_temp = sensor_virt_addr[1];
> + als_input.chromaticity_x_value = sensor_virt_addr[2];
> + als_input.chromaticity_y_value = sensor_virt_addr[3];
> + }
>
> report_size = sizeof(als_input);
> memcpy(input_report, &als_input, sizeof(als_input));
> diff --git a/drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_desc.h b/drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_desc.h
> index a7fc50deca4d..882434b1501f 100644
> --- a/drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_desc.h
> +++ b/drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_desc.h
> @@ -100,6 +100,8 @@ struct als_input_report {
> /* values specific to this sensor */
> int illuminance_value;
> int light_color_temp;
> + int chromaticity_x_value;
> + int chromaticity_y_value;
> } __packed;
>
> struct hpd_feature_report {
> diff --git a/drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_report_desc.h b/drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_report_desc.h
> index 26e994e54ded..67ec2d6a417d 100644
> --- a/drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_report_desc.h
> +++ b/drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_report_desc.h
> @@ -648,6 +648,20 @@ static const u8 als_report_descriptor[] = {
> 0x75, 32, /* HID report size(32) */
> 0x95, 1, /* HID report count (1) */
> 0X81, 0x02, /* HID Input (Data_Arr_Abs) */
> +0x0A, 0xD4, 0x04, /* HID usage sensor data light chromaticity_x */
> +0x17, 0x00, 0x00, 0x01, 0x80, /* HID logical Min_32 */
> +0x27, 0xFF, 0xFF, 0xFF, 0x7F, /* HID logical Max_32 */
> +0x55, 0x0, /* HID unit exponent(0x0) */
> +0x75, 32, /* HID report size(32) */
> +0x95, 1, /* HID report count(1) */
> +0X81, 0x02, /* HID Input (Data_Var_Abs) */
> +0x0A, 0xD5, 0x04, /* HID usage sensor data light chromaticity_y */
> +0x17, 0x00, 0x00, 0x01, 0x80, /* HID logical Min_32 */
> +0x27, 0xFF, 0xFF, 0xFF, 0x7F, /* HID logical Max_32 */
> +0x55, 0x0, /* HID unit exponent(0x0) */
> +0x75, 32, /* HID report size(32) */
> +0x95, 1, /* HID report count (1) */
> +0X81, 0x02, /* HID Input (Data_Var_Abs) */
> 0xC0 /* HID end collection */
> };
>
> --
> 2.25.1
>
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH v2 5/9] HID: amd_sfh: Add support for SFH1.1 light color temperature
From: Jiri Kosina @ 2023-09-20 15:25 UTC (permalink / raw)
To: Basavaraj Natikar
Cc: benjamin.tissoires, jic23, lars, srinivas.pandruvada, linux-input,
linux-iio
In-Reply-To: <20230919081054.2050714-6-Basavaraj.Natikar@amd.com>
On Tue, 19 Sep 2023, Basavaraj Natikar wrote:
> In most cases, ambient color sensors also support light color temperature.
> As a result, add support of light color temperature for SFH1.1.
>
> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
Acked-by: Jiri Kosina <jkosina@suse.cz>
> ---
> drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_desc.c | 6 ++++++
> drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.h | 13 +++++++++++++
> 2 files changed, 19 insertions(+)
>
> diff --git a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_desc.c b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_desc.c
> index 06bdcf072d10..f100aaafa167 100644
> --- a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_desc.c
> +++ b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_desc.c
> @@ -188,6 +188,7 @@ static u8 get_input_rep(u8 current_index, int sensor_idx, int report_id,
> struct sfh_mag_data mag_data;
> struct sfh_als_data als_data;
> struct hpd_status hpdstatus;
> + struct sfh_base_info binfo;
> void __iomem *sensoraddr;
> u8 report_size = 0;
>
> @@ -235,6 +236,11 @@ static u8 get_input_rep(u8 current_index, int sensor_idx, int report_id,
> memcpy_fromio(&als_data, sensoraddr, sizeof(struct sfh_als_data));
> get_common_inputs(&als_input.common_property, report_id);
> als_input.illuminance_value = float_to_int(als_data.lux);
> +
> + memcpy_fromio(&binfo, mp2->vsbase, sizeof(struct sfh_base_info));
> + if (binfo.sbase.s_prop[ALS_IDX].sf.feat & 0x2)
> + als_input.light_color_temp = als_data.light_color_temp;
> +
> report_size = sizeof(als_input);
> memcpy(input_report, &als_input, sizeof(als_input));
> break;
> diff --git a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.h b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.h
> index 9d31d5b510eb..6f6f5db150c3 100644
> --- a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.h
> +++ b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.h
> @@ -88,6 +88,16 @@ struct sfh_sensor_list {
> };
> };
>
> +struct sfh_sensor_prop {
> + union {
> + u32 sprop;
> + struct {
> + u32 elist : 16;
> + u32 feat : 16;
> + } sf;
> + };
> +};
> +
> struct sfh_base_info {
> union {
> u32 sfh_base[24];
> @@ -95,6 +105,8 @@ struct sfh_base_info {
> struct sfh_platform_info plat_info;
> struct sfh_firmware_info fw_info;
> struct sfh_sensor_list s_list;
> + u32 rsvd;
> + struct sfh_sensor_prop s_prop[16];
> } sbase;
> };
> };
> @@ -134,6 +146,7 @@ struct sfh_mag_data {
> struct sfh_als_data {
> struct sfh_common_data commondata;
> u32 lux;
> + u32 light_color_temp;
> };
>
> struct hpd_status {
> --
> 2.25.1
>
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox