* Re: [PATCH] HID: uhid: refactor deprecated strncpy
From: David Rheinsberg @ 2023-09-18 7:37 UTC (permalink / raw)
To: Kees Cook
Cc: Justin Stitt, Jiri Kosina, Benjamin Tissoires, linux-input,
linux-kernel, linux-hardening, David Herrmann
In-Reply-To: <202309151342.DFA6CA5C7@keescook>
Hey
On Fri, Sep 15, 2023, at 10:48 PM, Kees Cook wrote:
> On Fri, Sep 15, 2023 at 09:36:23AM +0200, David Rheinsberg wrote:
>> Hi
>>
>> On Fri, Sep 15, 2023, at 7:13 AM, Kees Cook wrote:
>> >> - /* @hid is zero-initialized, strncpy() is correct, strlcpy() not */
>> >> - len = min(sizeof(hid->name), sizeof(ev->u.create2.name)) - 1;
>> >> - strncpy(hid->name, ev->u.create2.name, len);
>> >> - len = min(sizeof(hid->phys), sizeof(ev->u.create2.phys)) - 1;
>> >> - strncpy(hid->phys, ev->u.create2.phys, len);
>> >> - len = min(sizeof(hid->uniq), sizeof(ev->u.create2.uniq)) - 1;
>> >> - strncpy(hid->uniq, ev->u.create2.uniq, len);
>> >
>> > ev->u.create2 is:
>> > struct uhid_create2_req {
>> > __u8 name[128];
>> > __u8 phys[64];
>> > __u8 uniq[64];
>> > ...
>> >
>> > hid is:
>> > struct hid_device { /* device report descriptor */
>> > ...
>> > char name[128]; /* Device name */
>> > char phys[64]; /* Device physical location */
>> > char uniq[64]; /* Device unique identifier (serial #) */
>> >
>> > So these "min" calls are redundant -- it wants to copy at most 1 less so
>> > it can be %NUL terminated. Which is what strscpy() already does. And
>> > source and dest are the same size, so we can't over-read source if it
>> > weren't terminated (since strscpy won't overread like strlcpy).
>>
>> I *really* think we should keep the `min` calls. The compiler
>> should already optimize them away, as both arguments are compile-time
>> constants. There is no inherent reason why source and target are equal in
>> size. Yes, it is unlikely to change, but I don't understand why we would
>> want to implicitly rely on it, rather than make the compiler verify it for
>> us. And `struct hid_device` is very much allowed to change in the future.
>>
>> As an alternative, you can use BUILD_BUG_ON() and verify both are equal in length.
>
> If we can't depend on ev->u.create2.name/phys/uniq being %NUL-terminated,
> we've already done the "min" calculations, and we've already got the
> dest zeroed, then I suspect the thing to do is just use memcpy instead
> of strncpy (or strscpy).
If you use memcpy, you might copy garbage trailing the terminating zero. This is not particularly wrong, but also not really nice if user-space relies on the kernel to treat it as a string. You don't know whether a query of the string returns trailing bytes, and thus might expose data that user-space did not intend to share.
I mean, this is why the code uses strncpy().
Thanks
David
^ permalink raw reply
* Re: [PATCH v4 30/42] input: keypad: ep93xx: add DT support for Cirrus EP93xx
From: Andy Shevchenko @ 2023-09-18 7:27 UTC (permalink / raw)
To: nikita.shubin
Cc: Hartley Sweeten, Alexander Sverdlin, Russell King,
Dmitry Torokhov, Jonathan Cameron, Uwe Kleine-König,
Sergey Shtylyov, Damien Le Moal, Linus Walleij, linux-arm-kernel,
linux-kernel, linux-input, Arnd Bergmann
In-Reply-To: <20230915-ep93xx-v4-30-a1d779dcec10@maquefel.me>
On Fri, Sep 15, 2023 at 11:11:12AM +0300, Nikita Shubin via B4 Relay wrote:
> From: Nikita Shubin <nikita.shubin@maquefel.me>
>
> - drop flags, they were not used anyway
> - add OF ID match table
> - process "autorepeat", "debounce-delay-ms", prescale from device tree
> - drop platform data usage and it's header
> - keymap goes from device tree now on
...
> #include <linux/bits.h>
> #include <linux/module.h>
> +#include <linux/of.h>
Is this correct header? Shouldn't be property.h?
> #include <linux/platform_device.h>
> +#include <linux/mod_devicetable.h>
Can you. please, try to squeeze them to most ordered parts?
> #include <linux/interrupt.h>
> #include <linux/clk.h>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v2 0/4] Input: zforce_ts: standard properties
From: Andreas Kemnade @ 2023-09-18 6:42 UTC (permalink / raw)
To: dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt,
shawnguo, s.hauer, kernel, festevam, linux-imx, rydberg, andreas,
u.kleine-koenig, Jonathan.Cameron, linus.walleij, heiko,
linux-input, devicetree, linux-kernel, linux-arm-kernel
In-Reply-To: <20230821171151.555091-1-andreas@kemnade.info>
ping
On Mon, 21 Aug 2023 19:11:47 +0200
Andreas Kemnade <andreas@kemnade.info> wrote:
> Accept standard touchscreen properties to also enable specification
> of touchscreen orientation.
>
> Changes in V2:
> - correct mail address in .yaml
>
> Andreas Kemnade (4):
> dt-bindings: touchscreen: convert neonode,zforce to json-schema
> dt-bindings: touchscreen: neonode,zforce: Use standard properties
> Input: zforce_ts: Accept standard touchscreen properties
> ARM: dts: imx6sl-tolino-shine2hd: fix touchscreen rotation
>
> .../input/touchscreen/neonode,zforce.yaml | 72 +++++++++++++++++++
> .../bindings/input/touchscreen/zforce_ts.txt | 34 ---------
> .../dts/nxp/imx/imx6sl-tolino-shine2hd.dts | 6 +-
> drivers/input/touchscreen/zforce_ts.c | 36 +++++-----
> 4 files changed, 94 insertions(+), 54 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/input/touchscreen/neonode,zforce.yaml
> delete mode 100644 Documentation/devicetree/bindings/input/touchscreen/zforce_ts.txt
>
^ permalink raw reply
* Re: [PATCH RESEND] Input: xpad - Add HyperX Clutch Gladiate Support
From: Dmitry Torokhov @ 2023-09-18 5:29 UTC (permalink / raw)
To: HP Dev; +Cc: maxwell.nguyen, chris.toledanes, carl.ng, linux-input
In-Reply-To: <20230916014452.5241-1-hphyperxdev@gmail.com>
On Fri, Sep 15, 2023 at 06:44:52PM -0700, HP Dev wrote:
> Hi Dmitry,
> That should be ok. Thanks again for your support.
Great, applied, thank you.
--
Dmitry
^ permalink raw reply
* [PATCH v2 3/3] HID: nvidia-shield: Introduce thunderstrike_destroy()
From: Rahul Rameshbabu @ 2023-09-18 4:13 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Christophe JAILLET, kernel-janitors, linux-input, linux-kernel,
Rahul Rameshbabu
In-Reply-To: <20230918041345.59859-1-rrameshbabu@nvidia.com>
From: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
In order to simplify some error handling paths and avoid code duplication,
introduce thunderstrike_destroy() which undoes thunderstrike_create().
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Reviewed-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
---
Notes:
Changes:
v1->v2:
- Re-order operations in thunderstrike_destroy to be in LIFO order with
regards to the operations in thunderstrike_create.
drivers/hid/hid-nvidia-shield.c | 26 ++++++++++++++++----------
1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/drivers/hid/hid-nvidia-shield.c b/drivers/hid/hid-nvidia-shield.c
index a566f9cdc97d..817ad6c01129 100644
--- a/drivers/hid/hid-nvidia-shield.c
+++ b/drivers/hid/hid-nvidia-shield.c
@@ -915,6 +915,20 @@ static struct shield_device *thunderstrike_create(struct hid_device *hdev)
return ERR_PTR(ret);
}
+static void thunderstrike_destroy(struct hid_device *hdev)
+{
+ struct shield_device *dev = hid_get_drvdata(hdev);
+ struct thunderstrike *ts;
+
+ ts = container_of(dev, struct thunderstrike, base);
+
+ led_classdev_unregister(&ts->led_dev);
+ power_supply_unregister(ts->base.battery_dev.psy);
+ if (ts->haptics_dev)
+ input_unregister_device(ts->haptics_dev);
+ ida_free(&thunderstrike_ida, ts->id);
+}
+
static int android_input_mapping(struct hid_device *hdev, struct hid_input *hi,
struct hid_field *field,
struct hid_usage *usage, unsigned long **bit,
@@ -1074,11 +1088,7 @@ static int shield_probe(struct hid_device *hdev, const struct hid_device_id *id)
err_stop:
hid_hw_stop(hdev);
err_ts_create:
- power_supply_unregister(ts->base.battery_dev.psy);
- if (ts->haptics_dev)
- input_unregister_device(ts->haptics_dev);
- led_classdev_unregister(&ts->led_dev);
- ida_free(&thunderstrike_ida, ts->id);
+ thunderstrike_destroy(hdev);
return ret;
}
@@ -1090,11 +1100,7 @@ static void shield_remove(struct hid_device *hdev)
ts = container_of(dev, struct thunderstrike, base);
hid_hw_close(hdev);
- power_supply_unregister(dev->battery_dev.psy);
- if (ts->haptics_dev)
- input_unregister_device(ts->haptics_dev);
- led_classdev_unregister(&ts->led_dev);
- ida_free(&thunderstrike_ida, ts->id);
+ thunderstrike_destroy(hdev);
del_timer_sync(&ts->psy_stats_timer);
cancel_work_sync(&ts->hostcmd_req_work);
hid_hw_stop(hdev);
--
2.40.1
^ permalink raw reply related
* [PATCH v2 2/3] HID: nvidia-shield: Fix some missing function calls() in the probe error handling path
From: Rahul Rameshbabu @ 2023-09-18 4:13 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Christophe JAILLET, kernel-janitors, linux-input, linux-kernel,
Rahul Rameshbabu
In-Reply-To: <20230918041345.59859-1-rrameshbabu@nvidia.com>
From: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
The commit in Fixes updated the error handling path of
thunderstrike_create() and the remove function but not the error handling
path of shield_probe(), should an error occur after a successful
thunderstrike_create() call.
Add the missing calls.
Fixes: 3ab196f88237 ("HID: nvidia-shield: Add battery support for Thunderstrike")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Reviewed-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
---
Notes:
Changes:
v1->v2:
- Rename err_haptics label to err_ts_create to make the label name more
accurate.
drivers/hid/hid-nvidia-shield.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-nvidia-shield.c b/drivers/hid/hid-nvidia-shield.c
index c144641452d3..a566f9cdc97d 100644
--- a/drivers/hid/hid-nvidia-shield.c
+++ b/drivers/hid/hid-nvidia-shield.c
@@ -1058,7 +1058,7 @@ static int shield_probe(struct hid_device *hdev, const struct hid_device_id *id)
ret = hid_hw_start(hdev, HID_CONNECT_HIDINPUT);
if (ret) {
hid_err(hdev, "Failed to start HID device\n");
- goto err_haptics;
+ goto err_ts_create;
}
ret = hid_hw_open(hdev);
@@ -1073,10 +1073,12 @@ static int shield_probe(struct hid_device *hdev, const struct hid_device_id *id)
err_stop:
hid_hw_stop(hdev);
-err_haptics:
+err_ts_create:
+ power_supply_unregister(ts->base.battery_dev.psy);
if (ts->haptics_dev)
input_unregister_device(ts->haptics_dev);
led_classdev_unregister(&ts->led_dev);
+ ida_free(&thunderstrike_ida, ts->id);
return ret;
}
--
2.40.1
^ permalink raw reply related
* [PATCH v2 0/3] HID: nvidia-shield: Fix the error handling path of shield_probe()
From: Rahul Rameshbabu @ 2023-09-18 4:13 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Christophe JAILLET, kernel-janitors, linux-input, linux-kernel,
Rahul Rameshbabu
This series fixes some missing clean-up function calls in the error handling of
the probe.
Patch 1 and 2 fix some similar issues introduced in 2 different commits (hence 2
patches)
Patch 3 is an enhancement that creates a common function for cleaning up
thunderstrike instances.
Changes:
v1->v2:
- Add the LED_RETAIN_AT_SHUTDOWN flag to prevent
led_classdev_unregister from trying to set the LED to off before a
successful call to hid_hw_start.
- Rename err_haptics label to err_ts_create to make the label name more
accurate.
- Re-order operations in thunderstrike_destroy to be in LIFO order with
regards to the operations in thunderstrike_create.
Link: https://lore.kernel.org/linux-input/cover.1693070958.git.christophe.jaillet@wanadoo.fr/
Notes from Rahul:
- Thank you so much Christophe for these patches.
Christophe JAILLET (3):
HID: nvidia-shield: Fix a missing led_classdev_unregister() in the
probe error handling path
HID: nvidia-shield: Fix some missing function calls() in the probe
error handling path
HID: nvidia-shield: Introduce thunderstrike_destroy()
drivers/hid/hid-nvidia-shield.c | 29 +++++++++++++++++++----------
1 file changed, 19 insertions(+), 10 deletions(-)
--
2.40.1
^ permalink raw reply
* [PATCH v2 1/3] HID: nvidia-shield: Fix a missing led_classdev_unregister() in the probe error handling path
From: Rahul Rameshbabu @ 2023-09-18 4:13 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Christophe JAILLET, kernel-janitors, linux-input, linux-kernel,
Rahul Rameshbabu
In-Reply-To: <20230918041345.59859-1-rrameshbabu@nvidia.com>
From: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
The commit in Fixes updated the error handling path of
thunderstrike_create() and the remove function but not the error handling
path of shield_probe(), should an error occur after a successful
thunderstrike_create() call.
Add the missing call. Make sure it is safe to call in the probe error
handling path by preventing the led_classdev from attempting to set the LED
brightness to the off state on unregister.
Fixes: f88af60e74a5 ("HID: nvidia-shield: Support LED functionality for Thunderstrike")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Reviewed-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
---
Notes:
Changes:
v1->v2:
- Add the LED_RETAIN_AT_SHUTDOWN flag to prevent
led_classdev_unregister from trying to set the LED to off before a
successful call to hid_hw_start.
drivers/hid/hid-nvidia-shield.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-nvidia-shield.c b/drivers/hid/hid-nvidia-shield.c
index 43784bb57d3f..c144641452d3 100644
--- a/drivers/hid/hid-nvidia-shield.c
+++ b/drivers/hid/hid-nvidia-shield.c
@@ -801,7 +801,7 @@ static inline int thunderstrike_led_create(struct thunderstrike *ts)
led->name = devm_kasprintf(&ts->base.hdev->dev, GFP_KERNEL,
"thunderstrike%d:blue:led", ts->id);
led->max_brightness = 1;
- led->flags = LED_CORE_SUSPENDRESUME;
+ led->flags = LED_CORE_SUSPENDRESUME | LED_RETAIN_AT_SHUTDOWN;
led->brightness_get = &thunderstrike_led_get_brightness;
led->brightness_set = &thunderstrike_led_set_brightness;
@@ -1076,6 +1076,7 @@ static int shield_probe(struct hid_device *hdev, const struct hid_device_id *id)
err_haptics:
if (ts->haptics_dev)
input_unregister_device(ts->haptics_dev);
+ led_classdev_unregister(&ts->led_dev);
return ret;
}
--
2.40.1
^ permalink raw reply related
* [PATCH] HID: holtek: fix slab-out-of-bounds Write in holtek_kbd_input_event
From: Ma Ke @ 2023-09-18 2:40 UTC (permalink / raw)
To: jikos, benjamin.tissoires; +Cc: linux-input, linux-kernel, Ma Ke
There is a slab-out-of-bounds Write bug in hid-holtek-kbd driver.
The problem is the driver assumes the device must have an input
but some malicious devices violate this assumption.
Fix this by checking hid_device's input is non-empty before its usage.
Signed-off-by: Ma Ke <make_ruc2021@163.com>
---
drivers/hid/hid-holtek-kbd.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/hid/hid-holtek-kbd.c b/drivers/hid/hid-holtek-kbd.c
index 403506b9697e..b346d68a06f5 100644
--- a/drivers/hid/hid-holtek-kbd.c
+++ b/drivers/hid/hid-holtek-kbd.c
@@ -130,6 +130,10 @@ static int holtek_kbd_input_event(struct input_dev *dev, unsigned int type,
return -ENODEV;
boot_hid = usb_get_intfdata(boot_interface);
+ if (list_empty(&boot_hid->inputs)) {
+ hid_err(hid, "no inputs found\n");
+ return -ENODEV;
+ }
boot_hid_input = list_first_entry(&boot_hid->inputs,
struct hid_input, list);
--
2.37.2
^ permalink raw reply related
* Re: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup
From: Kai-Heng Feng @ 2023-09-18 1:17 UTC (permalink / raw)
To: Xu, Even
Cc: srinivas pandruvada, 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: <DM6PR11MB26184A8A3F955589F5FC6836F4FBA@DM6PR11MB2618.namprd11.prod.outlook.com>
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.
So if you also don't have ways to verify S5 wake functionality, maybe
we can simply revert 2e23a70edabe ("HID: intel-ish-hid: ipc: finish
power flow for EHL OOB") as alternative?
Kai-Heng
> Thanks!
>
> Best Regards,
> Even Xu
>
> -----Original Message-----
> From: Xu, Even
> Sent: Friday, September 15, 2023 3:27 PM
> To: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Cc: srinivas pandruvada <srinivas.pandruvada@linux.intel.com>; jikos@kernel.org; benjamin.tissoires@redhat.com; linux-pm@vger.kernel.org; linux-pci@vger.kernel.org; Lee, Jian Hui <jianhui.lee@canonical.com>; Zhang, Lixu <Lixu.Zhang@intel.com>; Ba, Najumon <najumon.ba@intel.com>; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: RE: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup
>
> Hi, Kai-Heng,
>
> I am also not familiar with this S5 wakeup test case.
> I already sent out mails to ask for help on it.
> Will come back to you once I get feedback.
> Thanks!
>
> Best Regards,
> Even Xu
>
> -----Original Message-----
> From: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Sent: Friday, September 15, 2023 2:01 PM
> To: Xu, Even <even.xu@intel.com>
> Cc: srinivas pandruvada <srinivas.pandruvada@linux.intel.com>; jikos@kernel.org; benjamin.tissoires@redhat.com; linux-pm@vger.kernel.org; linux-pci@vger.kernel.org; Lee, Jian Hui <jianhui.lee@canonical.com>; Zhang, Lixu <lixu.zhang@intel.com>; Ba, Najumon <najumon.ba@intel.com>; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup
>
> Hi Even,
>
> On Fri, Sep 15, 2023 at 1:31 PM Xu, Even <even.xu@intel.com> wrote:
> >
> > Hi, Srinivas,
> >
> > Sure, I will test it.
> > As long term not working on EHL, I doesn't have EHL board on hand right now, I can test this patch on other ISH related platforms.
> > From the patch, it's focus on EHL platform, I assume Kai-Heng already verified the function on EHL board.
>
> I only made sure the GPE overflow issue is fixed by the patch, but I didn't test the S5 wakeup.
> That's because I don't know how to test it on the EHL system I have.
> I'll test it if you can let me know how to test the S5 wakeup.
>
> Kai-Heng
>
> > I don't think it will take effect on other platforms, anyway, I will test it on the platforms I have to provide cross platform verification.
> >
> > Thanks!
> >
> > Best Regards,
> > Even Xu
> >
> > -----Original Message-----
> > From: srinivas pandruvada <srinivas.pandruvada@linux.intel.com>
> > Sent: Friday, September 15, 2023 12:11 AM
> > To: Kai-Heng Feng <kai.heng.feng@canonical.com>; jikos@kernel.org;
> > benjamin.tissoires@redhat.com
> > Cc: linux-pm@vger.kernel.org; linux-pci@vger.kernel.org; Lee, Jian Hui
> > <jianhui.lee@canonical.com>; Xu, Even <even.xu@intel.com>; Zhang, Lixu
> > <lixu.zhang@intel.com>; Ba, Najumon <najumon.ba@intel.com>;
> > linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup
> >
> > Hi Even,
> >
> > On Thu, 2023-09-14 at 12:18 +0800, Kai-Heng Feng wrote:
> > > System cannot suspend more than 255 times because the driver doesn't
> > > have corresponding acpi_disable_gpe() for acpi_enable_gpe(), so the
> > > GPE refcount overflows.
> > >
> > > Since PCI core and ACPI core already handles PCI PME wake and GPE
> > > wake when the device has wakeup capability, use device_init_wakeup()
> > > to let them do the wakeup setting work.
> > >
> > > Also add a shutdown callback which uses pci_prepare_to_sleep() to
> > > let PCI and ACPI set OOB wakeup for S5.
> > >
> > Please test this change.
> >
> > Thanks,
> > Srinivas
> >
> > > Fixes: 2e23a70edabe ("HID: intel-ish-hid: ipc: finish power flow for
> > > EHL OOB")
> > > Cc: Jian Hui Lee <jianhui.lee@canonical.com>
> > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > > ---
> > > drivers/hid/intel-ish-hid/ipc/pci-ish.c | 59
> > > +++++++----------------
> > > --
> > > 1 file changed, 15 insertions(+), 44 deletions(-)
> > >
> > > diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > > b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > > index 55cb25038e63..65e7eeb2fa64 100644
> > > --- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > > +++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > > @@ -119,42 +119,6 @@ static inline bool ish_should_leave_d0i3(struct
> > > pci_dev *pdev)
> > > return !pm_resume_via_firmware() || pdev->device ==
> > > CHV_DEVICE_ID; }
> > >
> > > -static int enable_gpe(struct device *dev) -{ -#ifdef CONFIG_ACPI
> > > - acpi_status acpi_sts;
> > > - struct acpi_device *adev;
> > > - struct acpi_device_wakeup *wakeup;
> > > -
> > > - adev = ACPI_COMPANION(dev);
> > > - if (!adev) {
> > > - dev_err(dev, "get acpi handle failed\n");
> > > - return -ENODEV;
> > > - }
> > > - wakeup = &adev->wakeup;
> > > -
> > > - acpi_sts = acpi_enable_gpe(wakeup->gpe_device, wakeup-
> > > >gpe_number);
> > > - if (ACPI_FAILURE(acpi_sts)) {
> > > - dev_err(dev, "enable ose_gpe failed\n");
> > > - return -EIO;
> > > - }
> > > -
> > > - return 0;
> > > -#else
> > > - return -ENODEV;
> > > -#endif
> > > -}
> > > -
> > > -static void enable_pme_wake(struct pci_dev *pdev) -{
> > > - if ((pci_pme_capable(pdev, PCI_D0) ||
> > > - pci_pme_capable(pdev, PCI_D3hot) ||
> > > - pci_pme_capable(pdev, PCI_D3cold)) && !enable_gpe(&pdev-
> > > >dev)) {
> > > - pci_pme_active(pdev, true);
> > > - dev_dbg(&pdev->dev, "ish ipc driver pme wake
> > > enabled\n");
> > > - }
> > > -}
> > > -
> > > /**
> > > * ish_probe() - PCI driver probe callback
> > > * @pdev: pci device
> > > @@ -225,7 +189,7 @@ static int ish_probe(struct pci_dev *pdev, const
> > > struct pci_device_id *ent)
> > >
> > > /* Enable PME for EHL */
> > > if (pdev->device == EHL_Ax_DEVICE_ID)
> > > - enable_pme_wake(pdev);
> > > + device_init_wakeup(dev, true);
> > >
> > > ret = ish_init(ishtp);
> > > if (ret)
> > > @@ -248,6 +212,19 @@ static void ish_remove(struct pci_dev *pdev)
> > > ish_device_disable(ishtp_dev); }
> > >
> > > +
> > > +/**
> > > + * ish_shutdown() - PCI driver shutdown callback
> > > + * @pdev: pci device
> > > + *
> > > + * This function sets up wakeup for S5 */ static void
> > > +ish_shutdown(struct pci_dev *pdev) {
> > > + if (pdev->device == EHL_Ax_DEVICE_ID)
> > > + pci_prepare_to_sleep(pdev); }
> > > +
> > > static struct device __maybe_unused *ish_resume_device;
> > >
> > > /* 50ms to get resume response */
> > > @@ -370,13 +347,6 @@ static int __maybe_unused ish_resume(struct
> > > device *device)
> > > struct pci_dev *pdev = to_pci_dev(device);
> > > struct ishtp_device *dev = pci_get_drvdata(pdev);
> > >
> > > - /* add this to finish power flow for EHL */
> > > - if (dev->pdev->device == EHL_Ax_DEVICE_ID) {
> > > - pci_set_power_state(pdev, PCI_D0);
> > > - enable_pme_wake(pdev);
> > > - dev_dbg(dev->devc, "set power state to D0 for ehl\n");
> > > - }
> > > -
> > > ish_resume_device = device;
> > > dev->resume_flag = 1;
> > >
> > > @@ -392,6 +362,7 @@ static struct pci_driver ish_driver = {
> > > .id_table = ish_pci_tbl,
> > > .probe = ish_probe,
> > > .remove = ish_remove,
> > > + .shutdown = ish_shutdown,
> > > .driver.pm = &ish_pm_ops,
> > > };
> > >
> >
^ permalink raw reply
* RE: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup
From: Xu, Even @ 2023-09-18 0:33 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: srinivas pandruvada, 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: <SN6PR11MB262473E2BF4057F4D285A613F4F6A@SN6PR11MB2624.namprd11.prod.outlook.com>
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.
Thanks!
Best Regards,
Even Xu
-----Original Message-----
From: Xu, Even
Sent: Friday, September 15, 2023 3:27 PM
To: Kai-Heng Feng <kai.heng.feng@canonical.com>
Cc: srinivas pandruvada <srinivas.pandruvada@linux.intel.com>; jikos@kernel.org; benjamin.tissoires@redhat.com; linux-pm@vger.kernel.org; linux-pci@vger.kernel.org; Lee, Jian Hui <jianhui.lee@canonical.com>; Zhang, Lixu <Lixu.Zhang@intel.com>; Ba, Najumon <najumon.ba@intel.com>; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: RE: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup
Hi, Kai-Heng,
I am also not familiar with this S5 wakeup test case.
I already sent out mails to ask for help on it.
Will come back to you once I get feedback.
Thanks!
Best Regards,
Even Xu
-----Original Message-----
From: Kai-Heng Feng <kai.heng.feng@canonical.com>
Sent: Friday, September 15, 2023 2:01 PM
To: Xu, Even <even.xu@intel.com>
Cc: srinivas pandruvada <srinivas.pandruvada@linux.intel.com>; jikos@kernel.org; benjamin.tissoires@redhat.com; linux-pm@vger.kernel.org; linux-pci@vger.kernel.org; Lee, Jian Hui <jianhui.lee@canonical.com>; Zhang, Lixu <lixu.zhang@intel.com>; Ba, Najumon <najumon.ba@intel.com>; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup
Hi Even,
On Fri, Sep 15, 2023 at 1:31 PM Xu, Even <even.xu@intel.com> wrote:
>
> Hi, Srinivas,
>
> Sure, I will test it.
> As long term not working on EHL, I doesn't have EHL board on hand right now, I can test this patch on other ISH related platforms.
> From the patch, it's focus on EHL platform, I assume Kai-Heng already verified the function on EHL board.
I only made sure the GPE overflow issue is fixed by the patch, but I didn't test the S5 wakeup.
That's because I don't know how to test it on the EHL system I have.
I'll test it if you can let me know how to test the S5 wakeup.
Kai-Heng
> I don't think it will take effect on other platforms, anyway, I will test it on the platforms I have to provide cross platform verification.
>
> Thanks!
>
> Best Regards,
> Even Xu
>
> -----Original Message-----
> From: srinivas pandruvada <srinivas.pandruvada@linux.intel.com>
> Sent: Friday, September 15, 2023 12:11 AM
> To: Kai-Heng Feng <kai.heng.feng@canonical.com>; jikos@kernel.org;
> benjamin.tissoires@redhat.com
> Cc: linux-pm@vger.kernel.org; linux-pci@vger.kernel.org; Lee, Jian Hui
> <jianhui.lee@canonical.com>; Xu, Even <even.xu@intel.com>; Zhang, Lixu
> <lixu.zhang@intel.com>; Ba, Najumon <najumon.ba@intel.com>;
> linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] HID: intel-ish-hid: ipc: Rework EHL OOB wakeup
>
> Hi Even,
>
> On Thu, 2023-09-14 at 12:18 +0800, Kai-Heng Feng wrote:
> > System cannot suspend more than 255 times because the driver doesn't
> > have corresponding acpi_disable_gpe() for acpi_enable_gpe(), so the
> > GPE refcount overflows.
> >
> > Since PCI core and ACPI core already handles PCI PME wake and GPE
> > wake when the device has wakeup capability, use device_init_wakeup()
> > to let them do the wakeup setting work.
> >
> > Also add a shutdown callback which uses pci_prepare_to_sleep() to
> > let PCI and ACPI set OOB wakeup for S5.
> >
> Please test this change.
>
> Thanks,
> Srinivas
>
> > Fixes: 2e23a70edabe ("HID: intel-ish-hid: ipc: finish power flow for
> > EHL OOB")
> > Cc: Jian Hui Lee <jianhui.lee@canonical.com>
> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > ---
> > drivers/hid/intel-ish-hid/ipc/pci-ish.c | 59
> > +++++++----------------
> > --
> > 1 file changed, 15 insertions(+), 44 deletions(-)
> >
> > diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > index 55cb25038e63..65e7eeb2fa64 100644
> > --- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > +++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> > @@ -119,42 +119,6 @@ static inline bool ish_should_leave_d0i3(struct
> > pci_dev *pdev)
> > return !pm_resume_via_firmware() || pdev->device ==
> > CHV_DEVICE_ID; }
> >
> > -static int enable_gpe(struct device *dev) -{ -#ifdef CONFIG_ACPI
> > - acpi_status acpi_sts;
> > - struct acpi_device *adev;
> > - struct acpi_device_wakeup *wakeup;
> > -
> > - adev = ACPI_COMPANION(dev);
> > - if (!adev) {
> > - dev_err(dev, "get acpi handle failed\n");
> > - return -ENODEV;
> > - }
> > - wakeup = &adev->wakeup;
> > -
> > - acpi_sts = acpi_enable_gpe(wakeup->gpe_device, wakeup-
> > >gpe_number);
> > - if (ACPI_FAILURE(acpi_sts)) {
> > - dev_err(dev, "enable ose_gpe failed\n");
> > - return -EIO;
> > - }
> > -
> > - return 0;
> > -#else
> > - return -ENODEV;
> > -#endif
> > -}
> > -
> > -static void enable_pme_wake(struct pci_dev *pdev) -{
> > - if ((pci_pme_capable(pdev, PCI_D0) ||
> > - pci_pme_capable(pdev, PCI_D3hot) ||
> > - pci_pme_capable(pdev, PCI_D3cold)) && !enable_gpe(&pdev-
> > >dev)) {
> > - pci_pme_active(pdev, true);
> > - dev_dbg(&pdev->dev, "ish ipc driver pme wake
> > enabled\n");
> > - }
> > -}
> > -
> > /**
> > * ish_probe() - PCI driver probe callback
> > * @pdev: pci device
> > @@ -225,7 +189,7 @@ static int ish_probe(struct pci_dev *pdev, const
> > struct pci_device_id *ent)
> >
> > /* Enable PME for EHL */
> > if (pdev->device == EHL_Ax_DEVICE_ID)
> > - enable_pme_wake(pdev);
> > + device_init_wakeup(dev, true);
> >
> > ret = ish_init(ishtp);
> > if (ret)
> > @@ -248,6 +212,19 @@ static void ish_remove(struct pci_dev *pdev)
> > ish_device_disable(ishtp_dev); }
> >
> > +
> > +/**
> > + * ish_shutdown() - PCI driver shutdown callback
> > + * @pdev: pci device
> > + *
> > + * This function sets up wakeup for S5 */ static void
> > +ish_shutdown(struct pci_dev *pdev) {
> > + if (pdev->device == EHL_Ax_DEVICE_ID)
> > + pci_prepare_to_sleep(pdev); }
> > +
> > static struct device __maybe_unused *ish_resume_device;
> >
> > /* 50ms to get resume response */
> > @@ -370,13 +347,6 @@ static int __maybe_unused ish_resume(struct
> > device *device)
> > struct pci_dev *pdev = to_pci_dev(device);
> > struct ishtp_device *dev = pci_get_drvdata(pdev);
> >
> > - /* add this to finish power flow for EHL */
> > - if (dev->pdev->device == EHL_Ax_DEVICE_ID) {
> > - pci_set_power_state(pdev, PCI_D0);
> > - enable_pme_wake(pdev);
> > - dev_dbg(dev->devc, "set power state to D0 for ehl\n");
> > - }
> > -
> > ish_resume_device = device;
> > dev->resume_flag = 1;
> >
> > @@ -392,6 +362,7 @@ static struct pci_driver ish_driver = {
> > .id_table = ish_pci_tbl,
> > .probe = ish_probe,
> > .remove = ish_remove,
> > + .shutdown = ish_shutdown,
> > .driver.pm = &ish_pm_ops,
> > };
> >
>
^ permalink raw reply
* amd-sfh bug: Accelerometer will not initialize on Lenovo 300e 2nd generation
From: Steven Presser @ 2023-09-17 20:56 UTC (permalink / raw)
To: linux-input, Basavaraj Natikar
Dear Basavaraj, dear list,
Per the maintainers file, I believe this is the best way to contact you
about this issue. If that's not true, I'm happy to move this
conversation somewhere else.
I have a Lenovo 300e 2nd Generation, a "2-in-1". This device uses an
accelerometer via the AMD Sensor Fusion Hub to detect laptop and tablet
mode. Despite being attached to the (supported) AMD SFH, this
accelerometer does not function. When the system tries to initialize the
accelerometer, it simply reports:
[ 8.850222] pcie_mp2_amd 0000:04:00.7: sid 0x0 (accelerometer) status
0x5
[ 8.850240] pcie_mp2_amd 0000:04:00.7: Failed to discover, sensors
not enabled is 0
[ 8.850276] pcie_mp2_amd: probe of 0000:04:00.7 failed with error -95
The device works correctly under Windows, so I know this isn't broken
hardware.
I've captured a dmesg (with debug enabled in the kernel module), lspci,
and dmidecide. All of them are available here:
https://gist.github.com/spresse1/4739eeec0becf6110d605d5f567b7a69. These
were captured using git revision
b84acc11b1c9552c9ca3a099b1610a6018619332 of the main branch. I'm aware
there's a patchset currently in progress for the SFH. I'm happy to
retest with that, but it doesn't look to me as if it would affect this
issue.
I am happy to assist in debugging this however I can, including giving
remote access to the system. At this point, I have debugged this issue
as far as I am able without documentation (that I believe is only
available with an AMD NDA).
How can I move this forward and how can we get this sensor working?
Thank you in advance for your help!
Steve
^ permalink raw reply
* Re: [PATCH 0/3] HID: nvidia-shield: Fix the error handling path of shield_probe()
From: Christophe JAILLET @ 2023-09-17 20:37 UTC (permalink / raw)
To: Rahul Rameshbabu
Cc: jikos, benjamin.tissoires, linux-input, linux-kernel,
kernel-janitors
In-Reply-To: <87msxnupmc.fsf@nvidia.com>
Le 15/09/2023 à 22:51, Rahul Rameshbabu a écrit :
> On Fri, 15 Sep, 2023 22:14:18 +0200 Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:
>> Le 15/09/2023 à 20:16, Rahul Rameshbabu a écrit :
>>> Hi Christophe,
>>> On Sat, 26 Aug, 2023 19:42:16 +0200 Christophe JAILLET
>>> <christophe.jaillet@wanadoo.fr> wrote:
>>>> This serie fixes some missing clean-up function calls in the error handling of
>>>> the probe.
>>>>
>>>> Patch 1 and 2 fix some similar issues introduced in 2 different commits (hence 2
>>>> patches)
>>>>
>>>> Patch 3 is a proposal to be more future proof.
>>>>
>>>>
>>>> *Note*: I'm not 100% sure that the order of the functions is the best one in
>>>> thunderstrike_destroy(), but it is the way it was.
>>>>
>>>> My personal preference would be to undo things in reverse order they are
>>>> allocated, such as:
>>>> led_classdev_unregister(&ts->led_dev);
>>>> power_supply_unregister(ts->base.battery_dev.psy);
>>>> if (ts->haptics_dev)
>>>> input_unregister_device(ts->haptics_dev);
>>>> ida_free(&thunderstrike_ida, ts->id);
>>>> This order was explicitly chnaged by 3ab196f88237, so, as I can't test the
>>>> changes on a real harware, I've left it as-is.
>>>>
>>>> Christophe JAILLET (3):
>>>> HID: nvidia-shield: Fix a missing led_classdev_unregister() in the
>>>> probe error handling path
>>>> HID: nvidia-shield: Fix some missing function calls() in the probe
>>>> error handling path
>>>> HID: nvidia-shield: Introduce thunderstrike_destroy()
>>>>
>>>> drivers/hid/hid-nvidia-shield.c | 23 ++++++++++++++++-------
>>>> 1 file changed, 16 insertions(+), 7 deletions(-)
>>> I was wondering if you have time to address the comments in this
>>> submission. If not, I can re-spin the patches with the needed changes in
>>> upcoming days.
>>
>> I can send an update tomorrow, but I'm only working with -next, so should using
>> for-6.6/nvidia (as said in your comment in #1/3) be a must have, then it would
>> be more convenient for me if you make the changes by yourself.
>
> Luckily, it does not have to be on top of for-6.6/nvidia to add the fix
> I mentioned with regards to the led_classdev flag for not trying to
> power off the led when unregistering the led_classdev. That should still
> merge nicely on top of for-6.6/nvidia. The main reason I mentioned it
> was due to the commit living there with regards to the issue involving
> unregistering the led_classdev without the mentioned flag.
Well, because of your comment on patch #1/3, I would prefer you to make
the relevant changes.
Understanding this code if more time consuming than I first expected.
CJ
>
> --
> Thanks for the patches,
>
> Rahul Rameshbabu
>
>>
>> CJ
>>
>>> --
>>> Thanks,
>>> Rahul Rameshbabu
>>>
>
^ permalink raw reply
* Re: [PATCH 2/2] Input: add Himax HX852x(ES) touchscreen driver
From: Christophe JAILLET @ 2023-09-17 17:58 UTC (permalink / raw)
To: stephan, Jeff LaBundy
Cc: conor+dt, devicetree, dmitry.torokhov, jonathan.albrieux,
krzysztof.kozlowski+dt, linux-input, linux-kernel, robh+dt,
rydberg
In-Reply-To: <ZQcrQIfXYCv5aMK7@nixie71>
Le 17/09/2023 à 18:37, Jeff LaBundy a écrit :
>>> + error = input_register_device(hx->input_dev);
>>> + if (error) {
>>
>> input_mt_destroy_slots() should be called here, or in an error handling path
>> below, or via a devm_add_action_or_reset().
>
> This seems like a memory leak in every touchscreen driver; maybe it is more
> practical to have the input core handle this clean-up.
>
> Other drivers can and do insert other return paths between input_mt_init_slots()
> and input_register_device(), so it seems that we cannot solve this by calling
> input_mt_destroy_slots() from the error path within input_register_device().
>
> Maybe a better option is to update input_mt_init_slots() to use device-managed
> allocation instead?
I think that devm_ is the way to go:
$ git grep input_mt_init_slots | wc -l
82
$ git grep input_mt_destroy_slots | wc -l
6
I'll send a patch for it.
>
>>
>> It should also be called in a .remove function (unless
>> devm_add_action_or_reset is prefered)
>
> I think the remove path is OK, as input_dev_release() handles this for us. In
> case I have misunderstood, please let me know.
Agreed. I missed that.
CJ
^ permalink raw reply
* Re: [PATCH 2/2] Input: add Himax HX852x(ES) touchscreen driver
From: Stephan Gerhold @ 2023-09-17 17:15 UTC (permalink / raw)
To: Jeff LaBundy
Cc: Christophe JAILLET, conor+dt, devicetree, dmitry.torokhov,
jonathan.albrieux, krzysztof.kozlowski+dt, linux-input,
linux-kernel, robh+dt, rydberg
In-Reply-To: <ZQcrQIfXYCv5aMK7@nixie71>
Hi Christophe and Jeff,
Thanks for your comments!
On Sun, Sep 17, 2023 at 11:37:20AM -0500, Jeff LaBundy wrote:
> On Sun, Sep 17, 2023 at 08:03:48AM +0200, Christophe JAILLET wrote:
> > Le 13/09/2023 à 15:25, Stephan Gerhold a écrit :
> > > From: Jonathan Albrieux <jonathan.albrieux-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > >
> > > Add a simple driver for the Himax HX852x(ES) touch panel controller,
> > > with support for multi-touch and capacitive touch keys.
> > >
> > > The driver is somewhat based on sample code from Himax. However, that
> > > code was so extremely confusing that we spent a significant amount of
> > > time just trying to understand the packet format and register commands.
> > > In this driver they are described with clean structs and defines rather
> > > than lots of magic numbers and offset calculations.
> > >
> > > Signed-off-by: Jonathan Albrieux <jonathan.albrieux-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > Co-developed-by: Stephan Gerhold <stephan-3XONVrnlUWDR7s880joybQ@public.gmane.org>
> > > Signed-off-by: Stephan Gerhold <stephan-3XONVrnlUWDR7s880joybQ@public.gmane.org>
> > > ---
> >
> > ...
> >
> > > +static irqreturn_t hx852x_interrupt(int irq, void *ptr)
> > > +{
> > > + struct hx852x *hx = ptr;
> > > + int error;
> > > +
> > > + error = hx852x_handle_events(hx);
> > > + if (error) {
> > > + dev_err(&hx->client->dev, "failed to handle events: %d\n", error);
> >
> > Should dev_err_ratelimited() be preferred?
> >
I haven't ever seen this but I guess you're right. It could spam
potentially. :-) I will change it in v2.
> > > + return IRQ_NONE;
> > > + }
> > > +
> > > + return IRQ_HANDLED;
> > > +}
> >
> > ...
> >
> > > +static int hx852x_probe(struct i2c_client *client)
> > > +{
> > > + struct device *dev = &client->dev;
> > > + struct hx852x *hx;
> > > + int error, i;
> >
> > Nit: err or ret is shorter and maybe more "standard".
>
> For what it's worth, 'error' tends to be more common in input.
>
Yep, this is the only reason why we used it. I usually use "ret" but got
the feeling "error" is preferred for the input subsystem.
> >
> > > +
> > > + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C |
> > > + I2C_FUNC_SMBUS_WRITE_BYTE |
> > > + I2C_FUNC_SMBUS_WRITE_BYTE_DATA |
> > > + I2C_FUNC_SMBUS_WRITE_WORD_DATA)) {
> > > + dev_err(dev, "not all i2c functionality supported\n");
> > > + return -ENXIO;
> > > + }
> > > +
> > > + hx = devm_kzalloc(dev, sizeof(*hx), GFP_KERNEL);
> > > + if (!hx)
> > > + return -ENOMEM;
> > > +
> > > + hx->client = client;
> > > + hx->input_dev = devm_input_allocate_device(dev);
> > > + if (!hx->input_dev)
> > > + return -ENOMEM;
> > > +
> > > + hx->input_dev->name = "Himax HX852x";
> > > + hx->input_dev->id.bustype = BUS_I2C;
> > > + hx->input_dev->open = hx852x_input_open;
> > > + hx->input_dev->close = hx852x_input_close;
> > > +
> > > + i2c_set_clientdata(client, hx);
> > > + input_set_drvdata(hx->input_dev, hx);
> > > +
> > > + hx->supplies[0].supply = "vcca";
> > > + hx->supplies[1].supply = "vccd";
> > > + error = devm_regulator_bulk_get(dev, ARRAY_SIZE(hx->supplies), hx->supplies);
> > > + if (error < 0)
> > > + return dev_err_probe(dev, error, "failed to get regulators");
> > > +
> > > + hx->reset_gpiod = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> > > + if (IS_ERR(hx->reset_gpiod))
> > > + return dev_err_probe(dev, error, "failed to get reset gpio");
> > > +
> > > + error = devm_request_threaded_irq(dev, client->irq, NULL, hx852x_interrupt,
> > > + IRQF_ONESHOT | IRQF_NO_AUTOEN, NULL, hx);
> > > + if (error) {
> > > + dev_err(dev, "failed to request irq %d: %d\n", client->irq, error);
> >
> > dev_err_probe() could be used to be consistent with above code.
> > Same for below dev_err() calls.
> >
Right, will change it!
> > > + return error;
> > > + }
> > > +
> > > + error = hx852x_read_config(hx);
> > > + if (error)
> > > + return error;
> > > +
> > > + input_set_capability(hx->input_dev, EV_ABS, ABS_MT_POSITION_X);
> > > + input_set_capability(hx->input_dev, EV_ABS, ABS_MT_POSITION_Y);
> > > + input_set_abs_params(hx->input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
> > > +
> > > + touchscreen_parse_properties(hx->input_dev, true, &hx->props);
> > > + error = hx852x_parse_properties(hx);
> > > + if (error)
> > > + return error;
> > > +
> > > + hx->input_dev->keycode = hx->keycodes;
> > > + hx->input_dev->keycodemax = hx->keycount;
> > > + hx->input_dev->keycodesize = sizeof(hx->keycodes[0]);
> > > + for (i = 0; i < hx->keycount; i++)
> > > + input_set_capability(hx->input_dev, EV_KEY, hx->keycodes[i]);
> > > +
> > > + error = input_mt_init_slots(hx->input_dev, hx->max_fingers,
> > > + INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
> > > + if (error) {
> > > + dev_err(dev, "failed to init MT slots: %d\n", error);
> > > + return error;
> > > + }
> > > +
> > > + error = input_register_device(hx->input_dev);
> > > + if (error) {
> >
> > input_mt_destroy_slots() should be called here, or in an error handling path
> > below, or via a devm_add_action_or_reset().
>
> This seems like a memory leak in every touchscreen driver; maybe it is more
> practical to have the input core handle this clean-up.
>
> Other drivers can and do insert other return paths between input_mt_init_slots()
> and input_register_device(), so it seems that we cannot solve this by calling
> input_mt_destroy_slots() from the error path within input_register_device().
>
> Maybe a better option is to update input_mt_init_slots() to use device-managed
> allocation instead?
>
Hmm, it would be fairly easy to add the input_mt_destroy_slots() call as
part of the single if statement I have here, but yeah, someone would
need to make a patch for literally all of the other touchscreen drivers.
Both options (add call or some devm magic) would be fine for me. :-)
> >
> > It should also be called in a .remove function (unless
> > devm_add_action_or_reset is prefered)
>
> I think the remove path is OK, as input_dev_release() handles this for us. In
> case I have misunderstood, please let me know.
>
Yep, I think so too!
Thanks,
Stephan
^ permalink raw reply
* Re: [PATCH 2/2] Input: add Himax HX852x(ES) touchscreen driver
From: Stephan Gerhold @ 2023-09-17 17:05 UTC (permalink / raw)
To: Jeff LaBundy
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, linux-input, devicetree, linux-kernel,
Jonathan Albrieux
In-Reply-To: <ZQYUe46/rj8jqNvg@nixie71>
Hi Jeff,
Thanks a lot for your detailed review!
On Sat, Sep 16, 2023 at 03:47:55PM -0500, Jeff LaBundy wrote:
> On Wed, Sep 13, 2023 at 03:25:30PM +0200, Stephan Gerhold wrote:
> > From: Jonathan Albrieux <jonathan.albrieux@gmail.com>
> >
> > Add a simple driver for the Himax HX852x(ES) touch panel controller,
> > with support for multi-touch and capacitive touch keys.
> >
> > The driver is somewhat based on sample code from Himax. However, that
> > code was so extremely confusing that we spent a significant amount of
> > time just trying to understand the packet format and register commands.
> > In this driver they are described with clean structs and defines rather
> > than lots of magic numbers and offset calculations.
> >
> > Signed-off-by: Jonathan Albrieux <jonathan.albrieux@gmail.com>
> > Co-developed-by: Stephan Gerhold <stephan@gerhold.net>
> > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > ---
> > MAINTAINERS | 7 +
> > drivers/input/touchscreen/Kconfig | 10 +
> > drivers/input/touchscreen/Makefile | 1 +
> > drivers/input/touchscreen/himax_hx852x.c | 491 +++++++++++++++++++++++++++++++
> > 4 files changed, 509 insertions(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 90f13281d297..c551c60b0598 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -9331,6 +9331,13 @@ S: Maintained
> > F: Documentation/devicetree/bindings/input/touchscreen/himax,hx83112b.yaml
> > F: drivers/input/touchscreen/himax_hx83112b.c
> >
> > +HIMAX HX852X TOUCHSCREEN DRIVER
> > +M: Stephan Gerhold <stephan@gerhold.net>
> > +L: linux-input@vger.kernel.org
> > +S: Maintained
> > +F: Documentation/devicetree/bindings/input/touchscreen/himax,hx852es.yaml
> > +F: drivers/input/touchscreen/himax_hx852x.c
> > +
> > HIPPI
> > M: Jes Sorensen <jes@trained-monkey.org>
> > L: linux-hippi@sunsite.dk
> > diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> > index e3e2324547b9..8e5667ae5dab 100644
> > --- a/drivers/input/touchscreen/Kconfig
> > +++ b/drivers/input/touchscreen/Kconfig
> > @@ -427,6 +427,16 @@ config TOUCHSCREEN_HIDEEP
> > To compile this driver as a module, choose M here : the
> > module will be called hideep_ts.
> >
> > +config TOUCHSCREEN_HIMAX_HX852X
> > + tristate "Himax HX852x(ES) touchscreen"
> > + depends on I2C
> > + help
> > + Say Y here if you have a Himax HX852x(ES) touchscreen.
> > + If unsure, say N.
> > +
> > + To compile this driver as a module, choose M here: the module
> > + will be called himax_hx852x.
> > +
> > config TOUCHSCREEN_HYCON_HY46XX
> > tristate "Hycon hy46xx touchscreen support"
> > depends on I2C
> > diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> > index 62bd24f3ac8e..f42a87faa86c 100644
> > --- a/drivers/input/touchscreen/Makefile
> > +++ b/drivers/input/touchscreen/Makefile
> > @@ -48,6 +48,7 @@ obj-$(CONFIG_TOUCHSCREEN_EXC3000) += exc3000.o
> > obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
> > obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
> > obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
> > +obj-$(CONFIG_TOUCHSCREEN_HIMAX_HX852X) += himax_hx852x.o
> > obj-$(CONFIG_TOUCHSCREEN_HYNITRON_CSTXXX) += hynitron_cstxxx.o
> > obj-$(CONFIG_TOUCHSCREEN_ILI210X) += ili210x.o
> > obj-$(CONFIG_TOUCHSCREEN_ILITEK) += ilitek_ts_i2c.o
> > diff --git a/drivers/input/touchscreen/himax_hx852x.c b/drivers/input/touchscreen/himax_hx852x.c
> > new file mode 100644
> > index 000000000000..31616dcfc5ab
> > --- /dev/null
> > +++ b/drivers/input/touchscreen/himax_hx852x.c
> > @@ -0,0 +1,491 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Himax HX852x(ES) Touchscreen Driver
> > + * Copyright (c) 2020-2023 Stephan Gerhold <stephan@gerhold.net>
> > + * Copyright (c) 2020 Jonathan Albrieux <jonathan.albrieux@gmail.com>
> > + *
> > + * Based on the Himax Android Driver Sample Code Ver 0.3 for HMX852xES chipset:
> > + * Copyright (c) 2014 Himax Corporation.
> > + */
> > +
> > +#include <asm/unaligned.h>
> > +#include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/i2c.h>
> > +#include <linux/input.h>
> > +#include <linux/input/mt.h>
> > +#include <linux/input/touchscreen.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
>
> Please explicitly #include of_device.h.
>
Ack, thanks!
> > +#include <linux/regulator/consumer.h>
> > +
> > +#define HX852X_COORD_SIZE(fingers) ((fingers) * sizeof(struct hx852x_coord))
> > +#define HX852X_WIDTH_SIZE(fingers) ALIGN(fingers, 4)
> > +#define HX852X_BUF_SIZE(fingers) (HX852X_COORD_SIZE(fingers) + \
> > + HX852X_WIDTH_SIZE(fingers) + \
> > + sizeof(struct hx852x_touch_info))
> > +
> > +#define HX852X_MAX_FINGERS 12
>
> Well, that's a new one :)
>
FWIW: I'm not sure if any controller exists that actually supports 12,
but the bits are layed out in a way that it would be possible. :-)
> > +#define HX852X_MAX_KEY_COUNT 4
> > +#define HX852X_MAX_BUF_SIZE HX852X_BUF_SIZE(HX852X_MAX_FINGERS)
> > +
> > +#define HX852X_TS_SLEEP_IN 0x80
> > +#define HX852X_TS_SLEEP_OUT 0x81
> > +#define HX852X_TS_SENSE_OFF 0x82
> > +#define HX852X_TS_SENSE_ON 0x83
> > +#define HX852X_READ_ONE_EVENT 0x85
> > +#define HX852X_READ_ALL_EVENTS 0x86
> > +#define HX852X_READ_LATEST_EVENT 0x87
> > +#define HX852X_CLEAR_EVENT_STACK 0x88
> > +
> > +#define HX852X_REG_SRAM_SWITCH 0x8c
> > +#define HX852X_REG_SRAM_ADDR 0x8b
> > +#define HX852X_REG_FLASH_RPLACE 0x5a
> > +
> > +#define HX852X_SRAM_SWITCH_TEST_MODE 0x14
> > +#define HX852X_SRAM_ADDR_CONFIG 0x7000
> > +
> > +struct hx852x {
> > + struct i2c_client *client;
> > + struct input_dev *input_dev;
> > + struct touchscreen_properties props;
> > +
> > + struct gpio_desc *reset_gpiod;
> > + struct regulator_bulk_data supplies[2];
> > +
> > + unsigned int max_fingers;
> > + unsigned int keycount;
> > + u32 keycodes[HX852X_MAX_KEY_COUNT];
>
> Nit: might as well make keycodes 'unsigned int' for consistency; I also do not
> feel the newlines are necessary.
>
I don't mind having the newlines but also don't mind removing them,
will drop them in v2!
> > +};
> > +
> > +struct hx852x_config {
> > + u8 rx_num;
> > + u8 tx_num;
> > + u8 max_pt;
> > + u8 padding1[3];
> > + __be16 x_res;
> > + __be16 y_res;
> > + u8 padding2[2];
> > +} __packed __aligned(4);
>
> What is the reason to include trailing padding in this packed struct? Does the
> controller require the entire register length to be read for some reason?
>
Given your question I guess "padding" might be the wrong word here.
Basically the driver we based this on reads this config in a
semi-obfuscated way like this:
char data[12];
i2c_himax_read(..., data, 12, ...);
rx_num = data[0];
/* ... */
x_res = data[6]*256 + data[7];
/* ... */
data[10] and data[11] are not used in the vendor code, so we don't know
what is encoded there, if there is something encoded there, if only on
some models or only on some firmware versions.
I would prefer to keep the read/write operations similar to the vendor
driver. Who knows if there are peculiar firmware versions that would
fail if the read size is not correct. And maybe there is actually
interesting data in there?
Maybe "unknown" would be more clear than "padding"?
> > +
> > +struct hx852x_coord {
> > + __be16 x;
> > + __be16 y;
> > +} __packed __aligned(4);
> > +
> > +struct hx852x_touch_info {
> > + u8 finger_num;
> > + __le16 finger_pressed;
>
> It's interesting that most registers/packets use big endian (e.g. x_res) while
> only this uses little endian. Is that expected?
>
Yes, it's really like that. If you look closely the __le16 is also the
only 16-bit number that isn't aligned properly. I guess for the
"protocol designers" this fields was maybe not a 16-bit number but two
separate fields. No idea what they were thinking.
> > + u8 padding;
>
> Same question with regard to trailing padding.
>
I think the same answer as above applies here. Additionally here, the
packet format seems to be intentionally 4-byte/32-bit aligned (see
comment in hx852x_handle_events()) so I would say it makes sense to also
read an aligned size from the controller.
> > +} __packed __aligned(4);
> > +
> > +static int hx852x_i2c_read(struct hx852x *hx, u8 cmd, void *data, u16 len)
> > +{
> > + struct i2c_client *client = hx->client;
> > + int ret;
> > +
> > + struct i2c_msg msg[] = {
> > + {
> > + .addr = client->addr,
> > + .flags = 0,
> > + .len = 1,
> > + .buf = &cmd,
> > + },
> > + {
> > + .addr = client->addr,
> > + .flags = I2C_M_RD,
> > + .len = len,
> > + .buf = data,
> > + }
> > + };
> > +
> > + ret = i2c_transfer(client->adapter, msg, ARRAY_SIZE(msg));
> > + if (ret != ARRAY_SIZE(msg)) {
> > + dev_err(&client->dev, "failed to read %#x: %d\n", cmd, ret);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
>
> This function does not appear to be doing anything unique (e.g. retry loop to
> deal with special HW condition, etc.), so I do not see any reason to open-code
> a standard write-then-read operation.
>
> Can i2c_smbus API simply replace this function,
As far as I know the SMBus standard and API is limited to reading a
maximum of 32 bytes (I2C_SMBUS_BLOCK_MAX). With >= 6 fingers the touch
packet sizes will exceed that.
> or better yet, can this driver
> simply use regmap? In fact, that is what the other mainline Himax driver uses.
Regmap could maybe work, but I think it does not really fit. The I2C
interface here does not provide a consistent register map. Problem is,
for regmap_config we would need to define "reg_bits" and "val_bits".
This does not really exist here: The commands are usually sent without
any arguments, sometimes with a single byte (HX852X_REG_SRAM_SWITCH) and
sometimes with a word (HX852X_REG_SRAM_ADDR). There isn't a specific
register set with values here but more like random "commands".
Since SMBus doesn't allow reading more than 32 bytes and regmap does not
fit I think this custom but fairly standard routine is necessary. :/
>
> > +
> > +static int hx852x_power_on(struct hx852x *hx)
> > +{
> > + struct device *dev = &hx->client->dev;
> > + int error;
> > +
> > + error = regulator_bulk_enable(ARRAY_SIZE(hx->supplies), hx->supplies);
> > + if (error < 0) {
>
> Nit: if (error) as you have done elsewhere.
>
Thanks, will fix this.
> > + dev_err(dev, "failed to enable regulators: %d\n", error);
> > + return error;
> > + }
> > +
> > + gpiod_set_value_cansleep(hx->reset_gpiod, 1);
> > + msleep(20);
> > + gpiod_set_value_cansleep(hx->reset_gpiod, 0);
> > + msleep(20);
> > +
> > + return 0;
> > +}
> > +
> > +static int hx852x_start(struct hx852x *hx)
> > +{
> > + struct device *dev = &hx->client->dev;
> > + int error;
> > +
> > + error = i2c_smbus_write_byte(hx->client, HX852X_TS_SLEEP_OUT);
> > + if (error) {
> > + dev_err(dev, "failed to send TS_SLEEP_OUT: %d\n", error);
> > + return error;
> > + }
> > + msleep(30);
> > +
> > + error = i2c_smbus_write_byte(hx->client, HX852X_TS_SENSE_ON);
> > + if (error) {
> > + dev_err(dev, "failed to send TS_SENSE_ON: %d\n", error);
> > + return error;
> > + }
> > + msleep(20);
> > +
> > + return 0;
> > +}
> > +
> > +static void hx852x_stop(struct hx852x *hx)
> > +{
> > + struct device *dev = &hx->client->dev;
> > + int error;
> > +
> > + error = i2c_smbus_write_byte(hx->client, HX852X_TS_SENSE_OFF);
> > + if (error)
> > + dev_err(dev, "failed to send TS_SENSE_OFF: %d\n", error);
>
> Granted the function is of void type, should we not still return if there
> is an error?
>
> Actually, I would still keep this function as an int for future re-use, even
> though hx852x_input_close() simply ignores the value. This way, the return
> value can be propagated to the return value of hx852x_suspend() and elsewhere.
>
> > +
> > + msleep(20);
> > +
> > + error = i2c_smbus_write_byte(hx->client, HX852X_TS_SLEEP_IN);
> > + if (error)
> > + dev_err(dev, "failed to send TS_SLEEP_IN: %d\n", error);
>
> Same here; no need to sleep following a register write that seemingly did
> not happen.
>
> > +
> > + msleep(30);
> > +}
> > +
> > +static void hx852x_power_off(struct hx852x *hx)
> > +{
> > + struct device *dev = &hx->client->dev;
> > + int error;
> > +
> > + error = regulator_bulk_disable(ARRAY_SIZE(hx->supplies), hx->supplies);
> > + if (error)
> > + dev_err(dev, "failed to disable regulators: %d\n", error);
> > +}
>
> Same comment with regard to function type; it's nice for internal helpers
> to be of type int, even if the core callback using it is void.
>
> > +
> > +static int hx852x_read_config(struct hx852x *hx)
> > +{
> > + struct device *dev = &hx->client->dev;
> > + struct hx852x_config conf = {0};
>
> No need to initialize.
>
> > + int x_res, y_res;
> > + int error;
> > +
> > + error = hx852x_power_on(hx);
> > + if (error)
> > + return error;
> > +
> > + /* Sensing must be turned on briefly to load the config */
> > + error = hx852x_start(hx);
> > + if (error)
> > + goto power_off;
> > +
> > + hx852x_stop(hx);
>
> See my earlier comment about promoting this function's type to int.
>
> > +
> > + error = i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH,
> > + HX852X_SRAM_SWITCH_TEST_MODE);
> > + if (error)
> > + goto power_off;
> > +
> > + error = i2c_smbus_write_word_data(hx->client, HX852X_REG_SRAM_ADDR,
> > + HX852X_SRAM_ADDR_CONFIG);
> > + if (error)
> > + goto exit_test_mode;
> > +
> > + error = hx852x_i2c_read(hx, HX852X_REG_FLASH_RPLACE, &conf, sizeof(conf));
> > + if (error)
> > + goto exit_test_mode;
> > +
> > + x_res = be16_to_cpu(conf.x_res);
> > + y_res = be16_to_cpu(conf.y_res);
> > + hx->max_fingers = (conf.max_pt & 0xf0) >> 4;
> > + dev_dbg(dev, "x res: %d, y res: %d, max fingers: %d\n",
> > + x_res, y_res, hx->max_fingers);
> > +
> > + if (hx->max_fingers > HX852X_MAX_FINGERS) {
> > + dev_err(dev, "max supported fingers: %d, found: %d\n",
> > + HX852X_MAX_FINGERS, hx->max_fingers);
> > + error = -EINVAL;
> > + goto exit_test_mode;
> > + }
> > +
> > + if (x_res && y_res) {
> > + input_set_abs_params(hx->input_dev, ABS_MT_POSITION_X, 0, x_res - 1, 0, 0);
> > + input_set_abs_params(hx->input_dev, ABS_MT_POSITION_Y, 0, y_res - 1, 0, 0);
> > + }
> > +
> > +exit_test_mode:
> > + i2c_smbus_write_byte_data(hx->client, HX852X_REG_SRAM_SWITCH, 0);
>
> Nit: it would still be nice to preserve as many return values as possible, perhaps
> as follows:
>
> +exit_test_mode:
> error = i2c_smbus_write_byte_data(...) ? : error;
>
> > +power_off:
> > + hx852x_power_off(hx);
> > + return error;
>
> Similarly, with hx852x_power_off() being promoted to int as suggested above,
> this could be:
>
> return hx852x_power_off(...) ? : error;
>
> There are other idiomatic ways to do the same thing based on your preference.
> Another (perhaps more clear) option would be to move some of these test mode
> functions into a helper, which would also avoid some goto statements.
>
Thanks for your suggestions regarding the error handling / return codes.
For v2 I will play with the different options you gave and look which
one feels best. :-)
> > +}
> > +
> > +static int hx852x_handle_events(struct hx852x *hx)
> > +{
> > + /*
> > + * The event packets have variable size, depending on the amount of
> > + * supported fingers (hx->max_fingers). They are laid out as follows:
> > + * - struct hx852x_coord[hx->max_fingers]: Coordinates for each finger
> > + * - u8[ALIGN(hx->max_fingers, 4)]: Touch width for each finger
> > + * with padding for 32-bit alignment
> > + * - struct hx852x_touch_info
> > + *
> > + * Load everything into a 32-bit aligned buffer so the coordinates
> > + * can be assigned directly, without using get_unaligned_*().
> > + */
> > + u8 buf[HX852X_MAX_BUF_SIZE] __aligned(4);
> > + struct hx852x_coord *coord = (struct hx852x_coord *)buf;
> > + u8 *width = &buf[HX852X_COORD_SIZE(hx->max_fingers)];
> > + struct hx852x_touch_info *info = (struct hx852x_touch_info *)
> > + &width[HX852X_WIDTH_SIZE(hx->max_fingers)];
> > + unsigned long finger_pressed, key_pressed;
>
> It seems these only need to be u16.
>
Right. However, unsigned long is necessary for using it with
for_each_set_bit(), which wants a pointer to an unsigned long.
I can change it for key_pressed though.
> > + unsigned int i, x, y, w;
> > + int error;
> > +
> > + error = hx852x_i2c_read(hx, HX852X_READ_ALL_EVENTS, buf,
> > + HX852X_BUF_SIZE(hx->max_fingers));
> > + if (error)
> > + return error;
> > +
> > + finger_pressed = get_unaligned_le16(&info->finger_pressed);
> > + key_pressed = finger_pressed >> HX852X_MAX_FINGERS;
> > +
> > + /* All bits are set when no touch is detected */
> > + if (info->finger_num == 0xff || !(info->finger_num & 0x0f))
> > + finger_pressed = 0;
> > + if (key_pressed == 0xf)
> > + key_pressed = 0;
> > +
> > + for_each_set_bit(i, &finger_pressed, hx->max_fingers) {
> > + x = be16_to_cpu(coord[i].x);
> > + y = be16_to_cpu(coord[i].y);
> > + w = width[i];
> > +
> > + input_mt_slot(hx->input_dev, i);
> > + input_mt_report_slot_state(hx->input_dev, MT_TOOL_FINGER, 1);
> > + touchscreen_report_pos(hx->input_dev, &hx->props, x, y, true);
> > + input_report_abs(hx->input_dev, ABS_MT_TOUCH_MAJOR, w);
> > + }
> > + input_mt_sync_frame(hx->input_dev);
> > +
> > + for (i = 0; i < hx->keycount; i++)
> > + input_report_key(hx->input_dev, hx->keycodes[i], key_pressed & BIT(i));
> > +
> > + input_sync(hx->input_dev);
> > + return 0;
> > +}
> > +
> > +static irqreturn_t hx852x_interrupt(int irq, void *ptr)
> > +{
> > + struct hx852x *hx = ptr;
> > + int error;
> > +
> > + error = hx852x_handle_events(hx);
> > + if (error) {
> > + dev_err(&hx->client->dev, "failed to handle events: %d\n", error);
> > + return IRQ_NONE;
> > + }
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int hx852x_input_open(struct input_dev *dev)
> > +{
> > + struct hx852x *hx = input_get_drvdata(dev);
> > + int error;
> > +
> > + error = hx852x_power_on(hx);
> > + if (error)
> > + return error;
> > +
> > + error = hx852x_start(hx);
> > + if (error) {
> > + hx852x_power_off(hx);
> > + return error;
> > + }
> > +
> > + enable_irq(hx->client->irq);
> > + return 0;
> > +}
> > +
> > +static void hx852x_input_close(struct input_dev *dev)
> > +{
> > + struct hx852x *hx = input_get_drvdata(dev);
> > +
> > + hx852x_stop(hx);
> > + disable_irq(hx->client->irq);
> > + hx852x_power_off(hx);
> > +}
> > +
> > +static int hx852x_parse_properties(struct hx852x *hx)
> > +{
> > + struct device *dev = &hx->client->dev;
> > + int error;
> > +
> > + hx->keycount = device_property_count_u32(dev, "linux,keycodes");
> > + if (hx->keycount <= 0) {
> > + hx->keycount = 0;
> > + return 0;
> > + }
> > +
> > + if (hx->keycount > HX852X_MAX_KEY_COUNT) {
> > + dev_err(dev, "max supported keys: %d, found: %d\n",
>
> Nit: these should both be printed as %u.
>
Right, thanks!
> > + HX852X_MAX_KEY_COUNT, hx->keycount);
> > + return -EINVAL;
> > + }
> > +
> > + error = device_property_read_u32_array(dev, "linux,keycodes",
> > + hx->keycodes, hx->keycount);
> > + if (error)
> > + dev_err(dev, "failed to read linux,keycodes: %d\n", error);
> > +
> > + return error;
> > +}
> > +
> > +static int hx852x_probe(struct i2c_client *client)
> > +{
> > + struct device *dev = &client->dev;
> > + struct hx852x *hx;
> > + int error, i;
> > +
> > + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C |
> > + I2C_FUNC_SMBUS_WRITE_BYTE |
> > + I2C_FUNC_SMBUS_WRITE_BYTE_DATA |
> > + I2C_FUNC_SMBUS_WRITE_WORD_DATA)) {
> > + dev_err(dev, "not all i2c functionality supported\n");
> > + return -ENXIO;
> > + }
> > +
> > + hx = devm_kzalloc(dev, sizeof(*hx), GFP_KERNEL);
> > + if (!hx)
> > + return -ENOMEM;
> > +
> > + hx->client = client;
> > + hx->input_dev = devm_input_allocate_device(dev);
> > + if (!hx->input_dev)
> > + return -ENOMEM;
> > +
> > + hx->input_dev->name = "Himax HX852x";
> > + hx->input_dev->id.bustype = BUS_I2C;
> > + hx->input_dev->open = hx852x_input_open;
> > + hx->input_dev->close = hx852x_input_close;
> > +
> > + i2c_set_clientdata(client, hx);
> > + input_set_drvdata(hx->input_dev, hx);
> > +
> > + hx->supplies[0].supply = "vcca";
> > + hx->supplies[1].supply = "vccd";
> > + error = devm_regulator_bulk_get(dev, ARRAY_SIZE(hx->supplies), hx->supplies);
> > + if (error < 0)
> > + return dev_err_probe(dev, error, "failed to get regulators");
> > +
> > + hx->reset_gpiod = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> > + if (IS_ERR(hx->reset_gpiod))
> > + return dev_err_probe(dev, error, "failed to get reset gpio");
>
> Can the reset GPIO be optional?
>
I'm afraid I have no idea if the controller needs this or not. Would it
be better to keep it required until someone confirms otherwise or have
it optional for the other way around?
Thanks!
Stephan
^ permalink raw reply
* Re: [PATCH 2/2] Input: add Himax HX852x(ES) touchscreen driver
From: Jeff LaBundy @ 2023-09-17 16:37 UTC (permalink / raw)
To: Christophe JAILLET
Cc: stephan, conor+dt, devicetree, dmitry.torokhov, jonathan.albrieux,
krzysztof.kozlowski+dt, linux-input, linux-kernel, robh+dt,
rydberg
In-Reply-To: <abf36591-3b3c-dc47-b1aa-e574325499f4@wanadoo.fr>
Hi Christophe,
On Sun, Sep 17, 2023 at 08:03:48AM +0200, Christophe JAILLET wrote:
> Le 13/09/2023 à 15:25, Stephan Gerhold a écrit :
> > From: Jonathan Albrieux <jonathan.albrieux-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >
> > Add a simple driver for the Himax HX852x(ES) touch panel controller,
> > with support for multi-touch and capacitive touch keys.
> >
> > The driver is somewhat based on sample code from Himax. However, that
> > code was so extremely confusing that we spent a significant amount of
> > time just trying to understand the packet format and register commands.
> > In this driver they are described with clean structs and defines rather
> > than lots of magic numbers and offset calculations.
> >
> > Signed-off-by: Jonathan Albrieux <jonathan.albrieux-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > Co-developed-by: Stephan Gerhold <stephan-3XONVrnlUWDR7s880joybQ@public.gmane.org>
> > Signed-off-by: Stephan Gerhold <stephan-3XONVrnlUWDR7s880joybQ@public.gmane.org>
> > ---
>
> ...
>
> > +static irqreturn_t hx852x_interrupt(int irq, void *ptr)
> > +{
> > + struct hx852x *hx = ptr;
> > + int error;
> > +
> > + error = hx852x_handle_events(hx);
> > + if (error) {
> > + dev_err(&hx->client->dev, "failed to handle events: %d\n", error);
>
> Should dev_err_ratelimited() be preferred?
>
> > + return IRQ_NONE;
> > + }
> > +
> > + return IRQ_HANDLED;
> > +}
>
> ...
>
> > +static int hx852x_probe(struct i2c_client *client)
> > +{
> > + struct device *dev = &client->dev;
> > + struct hx852x *hx;
> > + int error, i;
>
> Nit: err or ret is shorter and maybe more "standard".
For what it's worth, 'error' tends to be more common in input.
>
> > +
> > + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C |
> > + I2C_FUNC_SMBUS_WRITE_BYTE |
> > + I2C_FUNC_SMBUS_WRITE_BYTE_DATA |
> > + I2C_FUNC_SMBUS_WRITE_WORD_DATA)) {
> > + dev_err(dev, "not all i2c functionality supported\n");
> > + return -ENXIO;
> > + }
> > +
> > + hx = devm_kzalloc(dev, sizeof(*hx), GFP_KERNEL);
> > + if (!hx)
> > + return -ENOMEM;
> > +
> > + hx->client = client;
> > + hx->input_dev = devm_input_allocate_device(dev);
> > + if (!hx->input_dev)
> > + return -ENOMEM;
> > +
> > + hx->input_dev->name = "Himax HX852x";
> > + hx->input_dev->id.bustype = BUS_I2C;
> > + hx->input_dev->open = hx852x_input_open;
> > + hx->input_dev->close = hx852x_input_close;
> > +
> > + i2c_set_clientdata(client, hx);
> > + input_set_drvdata(hx->input_dev, hx);
> > +
> > + hx->supplies[0].supply = "vcca";
> > + hx->supplies[1].supply = "vccd";
> > + error = devm_regulator_bulk_get(dev, ARRAY_SIZE(hx->supplies), hx->supplies);
> > + if (error < 0)
> > + return dev_err_probe(dev, error, "failed to get regulators");
> > +
> > + hx->reset_gpiod = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> > + if (IS_ERR(hx->reset_gpiod))
> > + return dev_err_probe(dev, error, "failed to get reset gpio");
> > +
> > + error = devm_request_threaded_irq(dev, client->irq, NULL, hx852x_interrupt,
> > + IRQF_ONESHOT | IRQF_NO_AUTOEN, NULL, hx);
> > + if (error) {
> > + dev_err(dev, "failed to request irq %d: %d\n", client->irq, error);
>
> dev_err_probe() could be used to be consistent with above code.
> Same for below dev_err() calls.
>
> > + return error;
> > + }
> > +
> > + error = hx852x_read_config(hx);
> > + if (error)
> > + return error;
> > +
> > + input_set_capability(hx->input_dev, EV_ABS, ABS_MT_POSITION_X);
> > + input_set_capability(hx->input_dev, EV_ABS, ABS_MT_POSITION_Y);
> > + input_set_abs_params(hx->input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
> > +
> > + touchscreen_parse_properties(hx->input_dev, true, &hx->props);
> > + error = hx852x_parse_properties(hx);
> > + if (error)
> > + return error;
> > +
> > + hx->input_dev->keycode = hx->keycodes;
> > + hx->input_dev->keycodemax = hx->keycount;
> > + hx->input_dev->keycodesize = sizeof(hx->keycodes[0]);
> > + for (i = 0; i < hx->keycount; i++)
> > + input_set_capability(hx->input_dev, EV_KEY, hx->keycodes[i]);
> > +
> > + error = input_mt_init_slots(hx->input_dev, hx->max_fingers,
> > + INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
> > + if (error) {
> > + dev_err(dev, "failed to init MT slots: %d\n", error);
> > + return error;
> > + }
> > +
> > + error = input_register_device(hx->input_dev);
> > + if (error) {
>
> input_mt_destroy_slots() should be called here, or in an error handling path
> below, or via a devm_add_action_or_reset().
This seems like a memory leak in every touchscreen driver; maybe it is more
practical to have the input core handle this clean-up.
Other drivers can and do insert other return paths between input_mt_init_slots()
and input_register_device(), so it seems that we cannot solve this by calling
input_mt_destroy_slots() from the error path within input_register_device().
Maybe a better option is to update input_mt_init_slots() to use device-managed
allocation instead?
>
> It should also be called in a .remove function (unless
> devm_add_action_or_reset is prefered)
I think the remove path is OK, as input_dev_release() handles this for us. In
case I have misunderstood, please let me know.
>
> CJ
>
> > + dev_err(dev, "failed to register input device: %d\n", error);
> > + return error;
> > + }
> > +
> > + return 0;
> > +}
>
> ...
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
* [PATCH] HID: multitouch: Add required quirk for Synaptics 0xcd7e device
From: Rahul Rameshbabu @ 2023-09-17 16:18 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Henrik Rydberg
Cc: linux-input, linux-kernel, Rahul Rameshbabu, Rain
Register the Synaptics device as a special multitouch device with certain
quirks that may improve usability of the touchpad device.
Reported-by: Rain <rain@sunshowers.io>
Closes: https://lore.kernel.org/linux-input/2bbb8e1d-1793-4df1-810f-cb0137341ff4@app.fastmail.com/
Signed-off-by: Rahul Rameshbabu <sergeantsagara@protonmail.com>
---
Notes:
Theory:
I think the Synaptics device in the related email to the linux-input
mailing list requires certain quirks like MT_QUIRK_HOVERING to correctly
reconfigure the distance configuration for multitouch events. This might
explain why light touches were not registered originally when
MT_CLS_DEFAULT was used by default for the device. Would like to have
this patch tested before being merged. A Tested-by: git trailer can then
be appended.
drivers/hid/hid-multitouch.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 521b2ffb4244..8db4ae05febc 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -2144,6 +2144,10 @@ static const struct hid_device_id mt_devices[] = {
USB_DEVICE_ID_MTP_STM)},
/* Synaptics devices */
+ { .driver_data = MT_CLS_WIN_8_FORCE_MULTI_INPUT,
+ HID_DEVICE(BUS_I2C, HID_GROUP_MULTITOUCH_WIN_8,
+ USB_VENDOR_ID_SYNAPTICS, 0xcd7e) },
+
{ .driver_data = MT_CLS_WIN_8_FORCE_MULTI_INPUT,
HID_DEVICE(BUS_I2C, HID_GROUP_MULTITOUCH_WIN_8,
USB_VENDOR_ID_SYNAPTICS, 0xce08) },
--
2.40.1
^ permalink raw reply related
* [PATCH] HID: nvidia-shield: Select POWER_SUPPLY Kconfig option
From: Rahul Rameshbabu @ 2023-09-17 15:18 UTC (permalink / raw)
To: Benjamin Tissoires, Jiri Kosina; +Cc: linux-input, Rahul Rameshbabu
Battery information reported by the driver depends on the power supply
subsystem. Select the required subsystem when the HID_NVIDIA_SHIELD Kconfig
option is enabled.
Fixes: 3ab196f88237 ("HID: nvidia-shield: Add battery support for Thunderstrike")
Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com>
---
drivers/hid/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index e11c1c803676..dc227f477601 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -792,6 +792,7 @@ config HID_NVIDIA_SHIELD
tristate "NVIDIA SHIELD devices"
depends on USB_HID
depends on BT_HIDP
+ select POWER_SUPPLY
help
Support for NVIDIA SHIELD accessories.
--
2.40.1
^ permalink raw reply related
* Re: [PATCH 2/8] iio: hid-sensor-als: Add light color temperature support
From: Basavaraj Natikar @ 2023-09-17 14:43 UTC (permalink / raw)
To: Jonathan Cameron, Basavaraj Natikar
Cc: jikos, benjamin.tissoires, lars, srinivas.pandruvada, linux-input,
linux-iio
In-Reply-To: <20230917120054.04e040e4@jic23-huawei>
On 9/17/2023 4:30 PM, Jonathan Cameron wrote:
> On Fri, 15 Sep 2023 10:46:57 +0530
> Basavaraj Natikar <Basavaraj.Natikar@amd.com> 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>
>> ---
>> drivers/iio/light/hid-sensor-als.c | 33 ++++++++++++++++++++++++++++++
>> include/linux/hid-sensor-ids.h | 1 +
>> 2 files changed, 34 insertions(+)
>>
>> diff --git a/drivers/iio/light/hid-sensor-als.c b/drivers/iio/light/hid-sensor-als.c
>> index 48879e233aec..220fb93fea6d 100644
>> --- a/drivers/iio/light/hid-sensor-als.c
>> +++ b/drivers/iio/light/hid-sensor-als.c
>> @@ -16,6 +16,7 @@
>> enum {
>> CHANNEL_SCAN_INDEX_INTENSITY = 0,
>> CHANNEL_SCAN_INDEX_ILLUM = 1,
> Either drop, the = 1 or keep consistency for TEMP.
> I don't think the = 1 is useful so I'd drop it.
>
>> + CHANNEL_SCAN_INDEX_COLOR_TEMP,
>> CHANNEL_SCAN_INDEX_MAX
>> };
>>
>> @@ -65,6 +66,18 @@ static const struct iio_chan_spec als_channels[] = {
>> BIT(IIO_CHAN_INFO_HYSTERESIS_RELATIVE),
>> .scan_index = CHANNEL_SCAN_INDEX_ILLUM,
>> },
>> + {
>> + .type = IIO_TEMP,
> Using a temperature channel for color temp is a bit of a stretch,
> Particularly as it's likely we will see light sensors with actual
> air temperature sensors in them at somepoint even if we don't have
> any already.
>
> So this needs a new channel type
> IIO_COLORTEMP or similar.
>
> Units for this probably want to be kelvin unlike the mili decrees centigrade
> used for IIO_TEMP.
>
>> + .modified = 1,
>> + .channel2 = IIO_MOD_TEMP_AMBIENT,
> I don't really see the modifier as useful here. That exists for thermocouple
> type systems where it is necessary to know ambient vs sample temperatures.
Sure Jonathan, I will address all comments in this series in v2.
Also, can i add new channel type IIO_COLORTEMP with following channel index
for light color temperature ?
{
.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,
}
Thanks,
--
Basavaraj
>
>
>> + .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 +116,11 @@ static int als_read_raw(struct iio_dev *indio_dev,
>> min = als_state->als_illum[chan->scan_index].logical_minimum;
>> address = HID_USAGE_SENSOR_LIGHT_ILLUM;
>> break;
>> + case CHANNEL_SCAN_INDEX_COLOR_TEMP:
>> + report_id = als_state->als_illum[chan->scan_index].report_id;
>> + min = als_state->als_illum[chan->scan_index].logical_minimum;
>> + address = HID_USAGE_SENSOR_LIGHT_COLOR_TEMPERATURE;
>> + break;
>> default:
>> report_id = -1;
>> break;
>> @@ -223,6 +241,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);
>> @@ -256,6 +278,17 @@ static int als_parse_report(struct platform_device *pdev,
>> st->als_illum[i].report_id);
>> }
>>
>> + ret = sensor_hub_input_get_attribute_info(hsdev, HID_INPUT_REPORT, usage_id,
>> + HID_USAGE_SENSOR_LIGHT_COLOR_TEMPERATURE,
>> + &st->als_illum[CHANNEL_SCAN_INDEX_COLOR_TEMP]);
>> + if (ret < 0)
>> + return ret;
>> + als_adjust_channel_bit_mask(channels, CHANNEL_SCAN_INDEX_COLOR_TEMP,
>> + st->als_illum[CHANNEL_SCAN_INDEX_COLOR_TEMP].size);
>> +
>> + dev_dbg(&pdev->dev, "als %x:%x\n", st->als_illum[CHANNEL_SCAN_INDEX_COLOR_TEMP].index,
>> + st->als_illum[CHANNEL_SCAN_INDEX_COLOR_TEMP].report_id);
>> +
>> st->scale_precision = hid_sensor_format_scale(usage_id,
>> &st->als_illum[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 0/8] Multiple light sensor support
From: Jonathan Cameron @ 2023-09-17 11:09 UTC (permalink / raw)
To: Basavaraj Natikar
Cc: jikos, benjamin.tissoires, lars, srinivas.pandruvada, linux-input,
linux-iio
In-Reply-To: <20230915051703.1689578-1-Basavaraj.Natikar@amd.com>
On Fri, 15 Sep 2023 10:46:55 +0530
Basavaraj Natikar <Basavaraj.Natikar@amd.com> wrote:
> This series adds support for light color temperature and chromaticity.
Hi Basavaraj,
I'd rename the series to make it clearer that this is about adding
colour temperature and chromaticity support rather than simply lots
of boring ambient light sensors.
Jonathan
>
> Basavaraj Natikar (8):
> iio: hid-sensor-als: Use channel index to support more hub attributes
> iio: hid-sensor-als: Add light color temperature support
> HID: amd_sfh: Add support for light color temperature
> HID: amd_sfh: Add support for SFH1.1 light color temperature
> iio: Add channel for chromaticity
> iio: hid-sensor-als: Add light chromaticity support
> HID: amd_sfh: Add light chromaticity support
> HID: amd_sfh: Add light chromaticity for SFH1.1
>
> Documentation/ABI/testing/sysfs-bus-iio | 8 ++
> .../hid_descriptor/amd_sfh_hid_desc.c | 7 +
> .../hid_descriptor/amd_sfh_hid_desc.h | 3 +
> .../hid_descriptor/amd_sfh_hid_report_desc.h | 21 +++
> drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_desc.c | 9 ++
> .../amd-sfh-hid/sfh1_1/amd_sfh_interface.h | 15 +++
> drivers/iio/industrialio-core.c | 1 +
> drivers/iio/light/hid-sensor-als.c | 124 +++++++++++++++---
> include/linux/hid-sensor-ids.h | 4 +
> include/uapi/linux/iio/types.h | 1 +
> tools/iio/iio_event_monitor.c | 2 +
> 11 files changed, 180 insertions(+), 15 deletions(-)
>
^ permalink raw reply
* Re: [PATCH 5/8] iio: Add channel for chromaticity
From: Jonathan Cameron @ 2023-09-17 11:04 UTC (permalink / raw)
To: Basavaraj Natikar
Cc: jikos, benjamin.tissoires, lars, srinivas.pandruvada, linux-input,
linux-iio
In-Reply-To: <20230915051703.1689578-6-Basavaraj.Natikar@amd.com>
On Fri, 15 Sep 2023 10:47:00 +0530
Basavaraj Natikar <Basavaraj.Natikar@amd.com> 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 channel for chromaticity.
Adding a channel type.
Otherwise looks reasonable to me.
>
> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
> ---
> Documentation/ABI/testing/sysfs-bus-iio | 8 ++++++++
> drivers/iio/industrialio-core.c | 1 +
> include/uapi/linux/iio/types.h | 1 +
> tools/iio/iio_event_monitor.c | 2 ++
> 4 files changed, 12 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index a2854dc9a839..6a810771f5e4 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -2179,3 +2179,11 @@ Contact: linux-iio@vger.kernel.org
> Description:
> Number of conditions that must occur, during a running
> period, before an event is generated.
> +
> +What: /sys/bus/iio/devices/iio:deviceX/in_chromaticity_x_raw
> +What: /sys/bus/iio/devices/iio:deviceX/in_chromaticity_y_raw
> +KernelVersion: 6.6
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + The x and y light color coordinate on the CIE 1931 chromaticity
> + diagram.
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index d752e9c0499b..c1df66cdfdf6 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -90,6 +90,7 @@ static const char * const iio_chan_type_name_spec[] = {
> [IIO_POSITIONRELATIVE] = "positionrelative",
> [IIO_PHASE] = "phase",
> [IIO_MASSCONCENTRATION] = "massconcentration",
> + [IIO_CHROMATICITY] = "chromaticity",
> };
>
> static const char * const iio_modifier_names[] = {
> diff --git a/include/uapi/linux/iio/types.h b/include/uapi/linux/iio/types.h
> index c79f2f046a0b..8952d48cfc64 100644
> --- a/include/uapi/linux/iio/types.h
> +++ b/include/uapi/linux/iio/types.h
> @@ -47,6 +47,7 @@ enum iio_chan_type {
> IIO_POSITIONRELATIVE,
> IIO_PHASE,
> IIO_MASSCONCENTRATION,
> + IIO_CHROMATICITY,
> };
>
> enum iio_modifier {
> diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
> index 0a5c2bb60030..115ba1fbf3ac 100644
> --- a/tools/iio/iio_event_monitor.c
> +++ b/tools/iio/iio_event_monitor.c
> @@ -59,6 +59,7 @@ static const char * const iio_chan_type_name_spec[] = {
> [IIO_POSITIONRELATIVE] = "positionrelative",
> [IIO_PHASE] = "phase",
> [IIO_MASSCONCENTRATION] = "massconcentration",
> + [IIO_CHROMATICITY] = "chromaticity",
> };
>
> static const char * const iio_ev_type_text[] = {
> @@ -173,6 +174,7 @@ static bool event_is_known(struct iio_event_data *event)
> case IIO_POSITIONRELATIVE:
> case IIO_PHASE:
> case IIO_MASSCONCENTRATION:
> + case IIO_CHROMATICITY:
> break;
> default:
> return false;
^ permalink raw reply
* Re: [PATCH 2/8] iio: hid-sensor-als: Add light color temperature support
From: Jonathan Cameron @ 2023-09-17 11:00 UTC (permalink / raw)
To: Basavaraj Natikar
Cc: jikos, benjamin.tissoires, lars, srinivas.pandruvada, linux-input,
linux-iio
In-Reply-To: <20230915051703.1689578-3-Basavaraj.Natikar@amd.com>
On Fri, 15 Sep 2023 10:46:57 +0530
Basavaraj Natikar <Basavaraj.Natikar@amd.com> 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>
> ---
> drivers/iio/light/hid-sensor-als.c | 33 ++++++++++++++++++++++++++++++
> include/linux/hid-sensor-ids.h | 1 +
> 2 files changed, 34 insertions(+)
>
> diff --git a/drivers/iio/light/hid-sensor-als.c b/drivers/iio/light/hid-sensor-als.c
> index 48879e233aec..220fb93fea6d 100644
> --- a/drivers/iio/light/hid-sensor-als.c
> +++ b/drivers/iio/light/hid-sensor-als.c
> @@ -16,6 +16,7 @@
> enum {
> CHANNEL_SCAN_INDEX_INTENSITY = 0,
> CHANNEL_SCAN_INDEX_ILLUM = 1,
Either drop, the = 1 or keep consistency for TEMP.
I don't think the = 1 is useful so I'd drop it.
> + CHANNEL_SCAN_INDEX_COLOR_TEMP,
> CHANNEL_SCAN_INDEX_MAX
> };
>
> @@ -65,6 +66,18 @@ static const struct iio_chan_spec als_channels[] = {
> BIT(IIO_CHAN_INFO_HYSTERESIS_RELATIVE),
> .scan_index = CHANNEL_SCAN_INDEX_ILLUM,
> },
> + {
> + .type = IIO_TEMP,
Using a temperature channel for color temp is a bit of a stretch,
Particularly as it's likely we will see light sensors with actual
air temperature sensors in them at somepoint even if we don't have
any already.
So this needs a new channel type
IIO_COLORTEMP or similar.
Units for this probably want to be kelvin unlike the mili decrees centigrade
used for IIO_TEMP.
> + .modified = 1,
> + .channel2 = IIO_MOD_TEMP_AMBIENT,
I don't really see the modifier as useful here. That exists for thermocouple
type systems where it is necessary to know ambient vs sample temperatures.
> + .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 +116,11 @@ static int als_read_raw(struct iio_dev *indio_dev,
> min = als_state->als_illum[chan->scan_index].logical_minimum;
> address = HID_USAGE_SENSOR_LIGHT_ILLUM;
> break;
> + case CHANNEL_SCAN_INDEX_COLOR_TEMP:
> + report_id = als_state->als_illum[chan->scan_index].report_id;
> + min = als_state->als_illum[chan->scan_index].logical_minimum;
> + address = HID_USAGE_SENSOR_LIGHT_COLOR_TEMPERATURE;
> + break;
> default:
> report_id = -1;
> break;
> @@ -223,6 +241,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);
> @@ -256,6 +278,17 @@ static int als_parse_report(struct platform_device *pdev,
> st->als_illum[i].report_id);
> }
>
> + ret = sensor_hub_input_get_attribute_info(hsdev, HID_INPUT_REPORT, usage_id,
> + HID_USAGE_SENSOR_LIGHT_COLOR_TEMPERATURE,
> + &st->als_illum[CHANNEL_SCAN_INDEX_COLOR_TEMP]);
> + if (ret < 0)
> + return ret;
> + als_adjust_channel_bit_mask(channels, CHANNEL_SCAN_INDEX_COLOR_TEMP,
> + st->als_illum[CHANNEL_SCAN_INDEX_COLOR_TEMP].size);
> +
> + dev_dbg(&pdev->dev, "als %x:%x\n", st->als_illum[CHANNEL_SCAN_INDEX_COLOR_TEMP].index,
> + st->als_illum[CHANNEL_SCAN_INDEX_COLOR_TEMP].report_id);
> +
> st->scale_precision = hid_sensor_format_scale(usage_id,
> &st->als_illum[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 1/8] iio: hid-sensor-als: Use channel index to support more hub attributes
From: Jonathan Cameron @ 2023-09-17 10:56 UTC (permalink / raw)
To: Basavaraj Natikar, linux-iio
Cc: jikos, benjamin.tissoires, lars, srinivas.pandruvada, linux-input
In-Reply-To: <20230915051703.1689578-2-Basavaraj.Natikar@amd.com>
On Fri, 15 Sep 2023 10:46:56 +0530
Basavaraj Natikar <Basavaraj.Natikar@amd.com> wrote:
> Sensor hub attributes can be extended to support more channels. 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>
Hi Basavaraj,
> ---
> drivers/iio/light/hid-sensor-als.c | 38 +++++++++++++++---------------
> 1 file changed, 19 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/iio/light/hid-sensor-als.c b/drivers/iio/light/hid-sensor-als.c
> index eb1aedad7edc..48879e233aec 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_illum[CHANNEL_SCAN_INDEX_MAX];
If being used for other channels, probably want to rename it as no longer
als_*illum*
> 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_illum[chan->scan_index].report_id;
> + min = als_state->als_illum[chan->scan_index].logical_minimum;
> address = HID_USAGE_SENSOR_LIGHT_ILLUM;
> break;
> default:
> @@ -242,23 +242,23 @@ 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_LIGHT_ILLUM,
> + &st->als_illum[i]);
I would call out either as a comment here or in the patch description that we repeat
the same reading for the two existing channels and hence whilst we store them in
separate entries things continue to work as before where there was just one entry.
> + if (ret < 0)
> + return ret;
> + als_adjust_channel_bit_mask(channels, i, st->als_illum[i].size);
> +
> + dev_dbg(&pdev->dev, "als %x:%x\n", st->als_illum[i].index,
> + st->als_illum[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_pre_decml, &st->scale_post_decml);
> + st->scale_precision = hid_sensor_format_scale(usage_id,
> + &st->als_illum[CHANNEL_SCAN_INDEX_INTENSITY],
> + &st->scale_pre_decml, &st->scale_post_decml);
Keep line lengths shorter anywhere it doesn't hurt readability. Whilst the hard limit has
gone up we still prefer to keep under 80chars where it is easy to do.
Sometimes that means relaxing the alignment with opening bracket.
>
> return ret;
> }
^ permalink raw reply
* Re: [PATCH] Input: add driver for TouchNetix aXiom touchscreen
From: Kamel Bouhara @ 2023-09-17 8:04 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: linux-input, mark.satterthwaite, pedro.torruella, bartp,
hannah.rossiter, Thomas Petazzoni, Gregory Clement,
bsp-development.geo
In-Reply-To: <42d9358d-b5ef-f68f-45e0-088cde4361a5@kernel.org>
Le Mon, Sep 11, 2023 at 08:59:21AM +0200, Krzysztof Kozlowski a écrit :
> On 08/09/2023 17:32, Kamel Bouhara wrote:
> > Add a new driver for the TouchNetix's aXiom family of
> > multi-touch controller. This driver only support i2c
> > and can be later adapted for SPI and USB support.
> >
> > Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> > ---
> > .../devicetree/bindings/vendor-prefixes.yaml | 2 +
>
>
> Thank you for your patch. There is something to discuss/improve.
>
> 1. Bindings are separate patches.
>
> I am surprised that you did not send this through your collegagues at
> Bootlin for some internal review. It would spot such easy things to fix.
>
> 2. Please use subject prefixes matching the subsystem. You can get them
> for example with `git log --oneline -- DIRECTORY_OR_FILE` on the
> directory your patch is touching.
>
> 3. Please run scripts/checkpatch.pl and fix reported warnings. Some
> warnings can be ignored, but the code here looks like it needs a fix.
> Feel free to get in touch if the warning is not clear.
>
> 4. Please use scripts/get_maintainers.pl to get a list of necessary
> people and lists to CC. It might happen, that command when run on an
> older kernel, gives you outdated entries. Therefore please be sure you
> base your patches on recent Linux kernel.
>
> You missed at least devicetree list (maybe more), so this won't be
> tested by automated tooling. Performing review on untested code might be
> a waste of time, thus I will skip this patch entirely till you follow
> the process allowing the patch to be tested.
>
> Please kindly resend and include all necessary To/Cc entries.
>
> Limited review follows.
Hello Krzysztof,
Sorry for this late answer and thanks for this not so limited review,
much appreciate :) !
>
>
> > MAINTAINERS | 7 +
> > drivers/input/touchscreen/Kconfig | 11 +
> > drivers/input/touchscreen/Makefile | 1 +
> > drivers/input/touchscreen/axiom_core.c | 382 ++++++++++++++++++
> > drivers/input/touchscreen/axiom_core.h | 140 +++++++
> > drivers/input/touchscreen/axiom_i2c.c | 349 ++++++++++++++++
> > 7 files changed, 892 insertions(+)
> > create mode 100644 drivers/input/touchscreen/axiom_core.c
> > create mode 100644 drivers/input/touchscreen/axiom_core.h
> > create mode 100644 drivers/input/touchscreen/axiom_i2c.c
> >
> > diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> > index 573578db9509..b0a3ed451f15 100644
> > --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> > +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> > @@ -175,6 +175,8 @@ patternProperties:
> > "^awinic,.*":
> > description: Shanghai Awinic Technology Co., Ltd.
> > "^axentia,.*":
> > + description: TouchNetix
> > + "^axiom,.*":
> > description: Axentia Technologies AB
> > "^axis,.*":
> > description: Axis Communications AB
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 389fe9e38884..43add48257d8 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -3373,6 +3373,13 @@ W: https://ez.analog.com/linux-software-drivers
> > F: Documentation/devicetree/bindings/hwmon/adi,axi-fan-control.yaml
> > F: drivers/hwmon/axi-fan-control.c
> >
> > +AXIOM I2C TOUCHSCREEN DRIVER
> > +M: Kamel Bouhara <kamel.bouhara@bootlin.com>
> > +L: linux-input@vger.kernel.org
> > +S: Maintained
> > +F: drivers/input/touchscreen/axiom_core.c
> > +F: drivers/input/touchscreen/axiom_i2.c
> > +
> > AXXIA I2C CONTROLLER
> > M: Krzysztof Adamski <krzysztof.adamski@nokia.com>
> > L: linux-i2c@vger.kernel.org
> > diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> > index e3e2324547b9..08a770a0c5e5 100644
> > --- a/drivers/input/touchscreen/Kconfig
> > +++ b/drivers/input/touchscreen/Kconfig
> > @@ -150,6 +150,17 @@ config TOUCHSCREEN_AUO_PIXCIR
> > To compile this driver as a module, choose M here: the
> > module will be called auo-pixcir-ts.
> >
> > +config TOUCHSCREEN_AXIOM_I2C
> > + tristate "AXIOM based multi-touch panel controllers"
> > + help
> > + Say Y here if you have a axiom touchscreen connected to
> > + your system.
> > +
> > + If unsure, say N.
> > +
> > + To compile this driver as a module, choose M here: the
> > + module will be called axiom_i2c.
> > +
> > config TOUCHSCREEN_BU21013
> > tristate "BU21013 based touch panel controllers"
> > depends on I2C
> > diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> > index 62bd24f3ac8e..59a3234ddb09 100644
> > --- a/drivers/input/touchscreen/Makefile
> > +++ b/drivers/input/touchscreen/Makefile
> > @@ -18,6 +18,7 @@ obj-$(CONFIG_TOUCHSCREEN_ADS7846) += ads7846.o
> > obj-$(CONFIG_TOUCHSCREEN_AR1021_I2C) += ar1021_i2c.o
> > obj-$(CONFIG_TOUCHSCREEN_ATMEL_MXT) += atmel_mxt_ts.o
> > obj-$(CONFIG_TOUCHSCREEN_AUO_PIXCIR) += auo-pixcir-ts.o
> > +obj-$(CONFIG_TOUCHSCREEN_AXIOM_I2C) += axiom_core.o axiom_i2c.o
> > obj-$(CONFIG_TOUCHSCREEN_BU21013) += bu21013_ts.o
> > obj-$(CONFIG_TOUCHSCREEN_BU21029) += bu21029_ts.o
> > obj-$(CONFIG_TOUCHSCREEN_CHIPONE_ICN8318) += chipone_icn8318.o
> > diff --git a/drivers/input/touchscreen/axiom_core.c b/drivers/input/touchscreen/axiom_core.c
> > new file mode 100644
> > index 000000000000..d381afd7fb84
> > --- /dev/null
> > +++ b/drivers/input/touchscreen/axiom_core.c
> > @@ -0,0 +1,382 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * TouchNetix aXiom Touchscreen Driver
> > + *
> > + * Copyright (C) 2020-2023 TouchNetix Ltd.
> > + *
> > + * Author(s): Mark Satterthwaite <mark.satterthwaite@touchnetix.com>
> > + * Pedro Torruella <pedro.torruella@touchnetix.com>
> > + * Bart Prescott <bartp@baasheep.co.uk>
> > + * Hannah Rossiter <hannah.rossiter@touchnetix.com>
> > + *
> > + */
> > +
> > +#include <linux/crc16.h>
> > +#include <linux/device.h>
> > +#include <linux/input.h>
> > +#include <linux/input/mt.h>
> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/slab.h>
> > +
> > +#include "axiom_core.h"
> > +
> > +/**
> > + * Decodes and populates the local u31 structure.
> > + * Given a buffer of data read from page 0 of u31 in an aXiom device.
> > + */
> > +void axiom_get_dev_info(struct axiom_data *ts, char *info)
> > +{
> > + struct axiom_devinfo *devinfo = &ts->devinfo;
> > +
> > + if (devinfo) {
> > + devinfo->bootloader_mode = ((info[1] & 0x80) != 0) ? 1 : 0;
> > + devinfo->device_id = ((info[1] & 0x7F) << 8) | info[0];
> > + devinfo->fw_minor = info[2];
> > + devinfo->fw_major = info[3];
> > + devinfo->fw_info_extra = (info[4]) | (info[5] << 8);
> > + devinfo->bootloader_fw_ver_minor = info[6];
> > + devinfo->bootloader_fw_ver_major = info[7];
> > + devinfo->jedec_id = (info[8]) | (info[9] << 8);
> > + devinfo->num_usages = info[10];
> > + devinfo->silicon_revision = info[11];
> > + }
> > +}
> > +EXPORT_SYMBOL_GPL(axiom_get_dev_info);
>
> Nope
I guess this means no need to export the function ?
I agree this is not required currently as we only have support the i2c
variant.
>
> > +
> > +/**
> > + * Decodes and populates the local Usage Table.
> > + * Given a buffer of data read from page 1 onwards of u31 from an aXiom
> > + * device.
> > + */
> > +char axiom_populate_usage_table(struct axiom_data *ts, char *rx_data)
> > +{
> > + u32 usage_id = 0;
> > + char max_report_len = 0;
> > + struct axiom_devinfo *device_info;
> > + struct usage_entry *usage_table;
> > +
> > + device_info = &ts->devinfo;
> > + usage_table = ts->usage_table;
> > +
> > + for (usage_id = 0; usage_id < device_info->num_usages; usage_id++) {
> > + u16 offset = (usage_id * U31_BYTES_PER_USAGE);
> > + char id = rx_data[offset + 0];
> > + char start_page = rx_data[offset + 1];
> > + char num_pages = rx_data[offset + 2];
> > + char max_offset = ((rx_data[offset + 3] & 0x7F) + 1) * 2;
> > +
> > + /* Store the entry into the usage table */
> > + usage_table[usage_id].id = id;
> > + usage_table[usage_id].is_report = ((num_pages == 0) ? 1 : 0);
> > + usage_table[usage_id].start_page = start_page;
> > + usage_table[usage_id].num_pages = num_pages;
> > +
> > + dev_dbg(ts->pdev, "Usage %2u Info: %*ph\n", usage_id,
> > + U31_BYTES_PER_USAGE, &rx_data[offset]);
> > +
> > + /* Identify the max report length the module will receive */
> > + if ((usage_table[usage_id].is_report)
> > + && (max_offset > max_report_len))
> > + max_report_len = max_offset;
> > + }
> > + ts->usage_table_populated = true;
> > +
> > + return max_report_len;
> > +}
> > +EXPORT_SYMBOL_GPL(axiom_populate_usage_table);
> > +
> > +/**
> > + * Translate usage/page/offset triplet into physical address.
> > + *
> > + * @usage: groups of registers
> > + * @page: page to which the usage belongs to offset
> > + * @offset of the usage
> > + */
> > +u16
> > +usage_to_target_address(struct axiom_data *ts, char usage, char page,
> > + char offset)
> > +{
> > + struct axiom_devinfo *device_info;
> > + struct usage_entry *usage_table;
> > + u16 target_address;
> > + u32 i;
> > +
> > + device_info = &ts->devinfo;
> > + usage_table = ts->usage_table;
> > +
> > + /* At the moment the convention is that u31 is always at physical address 0x0 */
> > + if (!ts->usage_table_populated && (usage == DEVINFO_USAGE_ID)) {
> > + target_address = ((page << 8) + offset);
> > + } else if (ts->usage_table_populated) {
> > + for (i = 0; i < device_info->num_usages; i++) {
> > + if (usage_table[i].id == usage) {
> > + if (page < usage_table[i].num_pages) {
> > + target_address =
> > + ((usage_table[i].start_page + page) << 8) + offset;
> > + } else {
> > + target_address = 0xFFFF;
> > + dev_err(ts->pdev,
> > + "Invalid usage table! usage: %u, page: %u, offset: %u\n",
> > + usage, page, offset);
> > + }
> > + break;
> > + }
> > + }
> > + } else {
> > + target_address = 0xFFFF;
> > + dev_err(ts->pdev, "Unpopulated usage table for usage: %u\n",
> > + usage);
> > + }
> > +
> > + dev_dbg(ts->pdev, "target_address is 0x%04x for usage: %u page %u\n",
> > + target_address, usage, page);
> > +
> > + return target_address;
> > +}
> > +EXPORT_SYMBOL_GPL(usage_to_target_address);
>
> Nope, drop.
>
> > +
> > +void axiom_remove(struct axiom_data *ts)
> > +{
> > + if (ts->usage_table)
> > + ts->usage_table_populated = false;
> > +
> > + if (ts->input_dev)
> > + input_unregister_device(ts->input_dev);
> > +}
> > +EXPORT_SYMBOL_GPL(axiom_remove);
>
> Nope, drop.
>
> > +
>
>
> > +
> > +/**
> > + * Take a raw buffer with u41 report data and decode it.
> > + * Also generate input events if needed.
> > + * @rx_buf: ptr to a byte array [0]: Usage number [1]: Status LSB [2]: Status MSB
> > + */
> > +void axiom_process_u41_report(struct axiom_data *ts, char *rx_buf)
> > +{
> > + struct axiom_target_report target;
> > + struct input_dev *input_dev = ts->input_dev;
> > + bool update_done = false;
> > + u16 target_status;
> > + u32 i;
> > +
> > + if (rx_buf[0] != 0x41) {
> > + dev_err(ts->pdev,
> > + "Data in buffer does not have expected u41 format.\n");
> > + return;
> > + }
> > +
> > + target_status = ((rx_buf[1]) | (rx_buf[2] << 8));
> > +
> > + for (i = 0; i < U41_MAX_TARGETS; i++) {
> > + target.index = i;
> > + target.present = ((target_status & (1 << i)) != 0) ? 1 : 0;
> > + target.x = (rx_buf[(i * 4) + 3]) | (rx_buf[(i * 4) + 4] << 8);
> > + target.y = (rx_buf[(i * 4) + 5]) | (rx_buf[(i * 4) + 6] << 8);
> > + target.z = (s8) (rx_buf[i + 43]);
> > + update_done |= axiom_process_u41_report_target(ts, &target);
> > + }
> > +
> > + if (update_done)
> > + input_sync(input_dev);
> > +}
> > +EXPORT_SYMBOL_GPL(axiom_process_u41_report);
>
> Nope, drop.
>
> > +
> > +void axiom_process_u46_report(struct axiom_data *ts, char *rx_buf)
> > +{
> > + struct input_dev *input_dev = ts->input_dev;
> > + u16 aux_value;
> > + u32 event_value;
> > + u32 i = 0;
> > +
> > + for (i = 0; i < U46_AUX_CHANNELS; i++) {
> > + aux_value =
> > + ((rx_buf[(i * 2) + 2] << 8) | (rx_buf[(i * 2) + 1])) &
> > + U46_AUX_MASK;
> > + event_value = (i << 16) | (aux_value);
> > + input_event(input_dev, EV_MSC, MSC_RAW, event_value);
> > + }
> > +
> > + input_mt_sync(input_dev);
> > + input_sync(input_dev);
> > +}
> > +EXPORT_SYMBOL_GPL(axiom_process_u46_report);
>
> Nope, drop.
>
> Clean your driver before sending upstream. :(
>
> > +
> > +/**
> > + * Support function to axiom_process_report.
> > + * It validates the crc and multiplexes the axiom reports to the appropriate
> > + * report handler
> > + */
> > +void axiom_process_report(struct axiom_data *ts, char *report_data)
> > +{
> > + struct device *pdev = ts->pdev;
> > + char usage = report_data[1];
> > + u16 crc_report;
> > + u16 crc_calc;
> > + char len;
> > +
> > + if ((report_data[0] & COMMS_OVERFLOW_MSK) != 0)
> > + ts->report_overflow_counter++;
> > +
> > + len = (report_data[0] & COMMS_REPORT_LEN_MSK) << 1;
> > + if (len == 0) {
> > + dev_err(pdev, "Zero length report discarded.\n");
> > + return;
> > + }
> > +
> > + dev_dbg(pdev, "Payload Data %*ph\n", len, report_data);
> > +
> > + /* Validate the report CRC */
> > + crc_report = (report_data[len - 1] << 8) | (report_data[len - 2]);
> > + /* Length is in 16 bit words and remove the size of the CRC16 itself */
> > + crc_calc = crc16(0, report_data, (len - 2));
> > +
> > + if (crc_calc != crc_report) {
> > + dev_err(pdev,
> > + "CRC mismatch! Expected: %#x, Calculated CRC: %#x.\n",
> > + crc_report, crc_calc);
> > + return;
> > + }
> > +
> > + switch (usage) {
> > + case USAGE_2DCTS_REPORT_ID:
> > + axiom_process_u41_report(ts, &report_data[1]);
> > + break;
> > +
> > + case USAGE_2AUX_REPORT_ID:
> > + /* This is an aux report (force) */
> > + axiom_process_u46_report(ts, &report_data[1]);
> > + break;
> > +
> > + case USAGE_2HB_REPORT_ID:
> > + /* This is a heartbeat report */
> > + break;
> > +
> > + default:
> > + break;
> > + }
> > +
> > + ts->report_counter++;
> > +}
> > +EXPORT_SYMBOL_GPL(axiom_process_report);
>
> Nope, drop.
>
> > +
> > +MODULE_AUTHOR("Kamel Bouhara <kamel.bouhara@bootlin.com>");
> > +MODULE_DESCRIPTION("aXiom touchscreen core logic");
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS("axiom");
>
> ???? How can you have multiple drivers with the same alias? Even if your
> Makefile allowed it (but it doesn't!), it would not make sense.
>
>
> > diff --git a/drivers/input/touchscreen/axiom_core.h b/drivers/input/touchscreen/axiom_core.h
> > new file mode 100644
> > index 000000000000..f129d28671ff
> > --- /dev/null
> > +++ b/drivers/input/touchscreen/axiom_core.h
> > @@ -0,0 +1,140 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * TouchNetix aXiom Touchscreen Driver
> > + *
> > + * Copyright (C) 2020-2023 TouchNetix Ltd.
> > + *
> > + * Author(s): Mark Satterthwaite <mark.satterthwaite@touchnetix.com>
> > + * Pedro Torruella <pedro.torruella@touchnetix.com>
> > + * Bart Prescott <bartp@baasheep.co.uk>
> > + * Hannah Rossiter <hannah.rossiter@touchnetix.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License as published by the
> > + * Free Software Foundation; either version 2 of the License, or (at your
> > + * option) any later version.
>
> OK, so this is some old, vendor crappy code. We do not have it since
> some years!
Yes :).
>
> > + *
>
> ...
>
> > +
> > +static void axiom_i2c_poll(struct input_dev *input_dev)
> > +{
> > + struct axiom_data *ts = input_get_drvdata(input_dev);
> > + char *rx_data = ts->rx_buf;
> > +
> > + axiom_i2c_read(ts->client, REPORT_USAGE_ID, 0, rx_data,
> > + ts->max_report_len);
> > + axiom_process_report(ts, rx_data);
> > +}
> > +
> > +/**
> > + * Retrieve, store and print the axiom device information
> > + */
> > +int axiom_discover(struct axiom_data *ts)
>
> This has to be static.
ok.
>
> > +{
> > + char *rx_data = &ts->rx_buf[0];
> > + struct device *pdev = ts->pdev;
> > + int ret;
> > +
> > + /* First the first page of u31 to get the device information and */
> > + /* the number of usages */
> > + ret =
> > + axiom_i2c_read(ts->client, DEVINFO_USAGE_ID, 0, rx_data,
> > + AX_U31_PAGE0_LENGTH);
> > + if (ret)
> > + return ret;
> > +
> > + axiom_get_dev_info(ts, rx_data);
> > +
> > + dev_dbg(pdev, "Data Decode:\n");
> > + dev_dbg(pdev, " Bootloader Mode: %u\n", ts->devinfo.bootloader_mode);
> > + dev_dbg(pdev, " Device ID : %04x\n", ts->devinfo.device_id);
> > + dev_dbg(pdev, " Firmware Rev : %02x.%02x\n", ts->devinfo.fw_major,
> > + ts->devinfo.fw_minor);
> > + dev_dbg(pdev, " Bootloader Rev : %02x.%02x\n",
> > + ts->devinfo.bootloader_fw_ver_major,
> > + ts->devinfo.bootloader_fw_ver_minor);
> > + dev_dbg(pdev, " FW Extra Info : %04x\n", ts->devinfo.fw_info_extra);
> > + dev_dbg(pdev, " Silicon : %02x\n", ts->devinfo.jedec_id);
> > + dev_dbg(pdev, " Num Usages : %04x\n", ts->devinfo.num_usages);
> > +
> > + /* Read the second page of u31 to get the usage table */
> > + ret = axiom_i2c_read(ts->client, DEVINFO_USAGE_ID, 1, rx_data,
> > + (U31_BYTES_PER_USAGE * ts->devinfo.num_usages));
> > + if (ret)
> > + return ret;
> > +
> > + ts->max_report_len = axiom_populate_usage_table(ts, rx_data);
> > + dev_dbg(pdev, "Max Report Length: %u\n", ts->max_report_len);
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(axiom_discover);
>
> Nope.
>
> > +
> > +/**
> > + * Rebaseline the touchscreen, effectively zero-ing it
> > + */
> > +void axiom_rebaseline(struct axiom_data *ts)
>
> Missing static.
>
> > +{
> > + struct device *pdev = ts->pdev;
> > + char buffer[8] = { 0 };
> > + int ret;
> > +
> > + memset(buffer, 0, sizeof(buffer));
> > +
> > + buffer[0] = REBASELINE_CMD;
> > +
> > + ret = axiom_i2c_write(ts->client, 0x02, 0, buffer, sizeof(buffer));
> > + if (ret)
> > + dev_err(pdev, "Rebaseline failed\n");
> > +
> > + dev_info(pdev, "Capture Baseline Requested\n");
> > +}
> > +EXPORT_SYMBOL_GPL(axiom_rebaseline);
>
> Nope.
>
> > +
> > +static irqreturn_t axiom_irq(int irq, void *handle)
> > +{
> > + struct axiom_data *ts = handle;
> > + u8 *rx_data = &ts->rx_buf[0];
> > +
> > + axiom_i2c_read(ts->client, REPORT_USAGE_ID, 0, rx_data, ts->max_report_len);
> > + axiom_process_report(ts, rx_data);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int axiom_i2c_probe(struct i2c_client *client)
> > +{
> > + struct axiom_data *ts;
> > + struct device *pdev = &client->dev;
> > + struct input_dev *input_dev;
> > + int error;
> > + int target;
> > +
> > + ts = devm_kzalloc(pdev, sizeof(*ts), GFP_KERNEL);
> > + if (!ts)
> > + return -ENOMEM;
> > +
> > + ts->irq_gpio = devm_gpiod_get_optional(pdev, "irq", GPIOD_IN);
> > + if (IS_ERR(ts->irq_gpio)) {
> > + error = PTR_ERR(ts->irq_gpio);
> > + dev_err(pdev, "failed to request irq GPIO: %d", error);
>
> Heh... syntax is: return dev_err_probe()
>
> > + return error;
> > + }
> > +
> > + ts->reset_gpio = devm_gpiod_get_optional(pdev, "reset", GPIOD_OUT_HIGH);
>
> You do not use this GPIO at all. Dead code, especially that you keep the
> device in reset. This was absolutely never tested (unless with incorrect
> DTS...).
Actually this has been exclusively tested on a X86 platform with ACPI
overrides however this still need to be fixed indeed.
>
> > + if (IS_ERR(ts->reset_gpio)) {
> > + error = PTR_ERR(ts->reset_gpio);
>
> return dev_err_probe()
>
> > + dev_err(pdev, "failed to request reset GPIO: %d", error);
> > + return error;
> > + }
> > +
> > + if (ts->irq_gpio) {
> > + error =
> > + devm_request_threaded_irq(pdev, client->irq, NULL,
> > + axiom_irq,
> > + IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> > + "axiom_irq", ts);
> > + if (error != 0) {
> > + dev_err(pdev, "Failed to request IRQ %u (error: %d)\n",
> > + client->irq, error);
>
> return dev_err_probe()
>
> > + return error;
> > + }
> > + }
> > +
> > + ts->client = client;
> > + ts->pdev = pdev;
> > + ts->usage_table_populated = false;
> > +
> > + i2c_set_clientdata(client, ts);
> > +
> > + axiom_discover(ts);
> > + axiom_rebaseline(ts);
> > +
> > + input_dev = devm_input_allocate_device(ts->pdev);
> > + if (!input_dev) {
> > + dev_err(pdev, "Failed to allocate input device\n");
>
> I don't think we print memory allocation failures.
>
> > + return -ENOMEM;
> > + }
> > +
> > + input_dev->name = "TouchNetix aXiom Touchscreen";
> > + input_dev->phys = "input/axiom_ts";
> > +
> > + /* Single Touch */
> > + input_set_abs_params(input_dev, ABS_X, 0, 65535, 0, 0);
> > + input_set_abs_params(input_dev, ABS_Y, 0, 65535, 0, 0);
> > +
> > + /* Multi Touch */
> > + /* Min, Max, Fuzz (expected noise in px, try 4?) and Flat */
> > + input_set_abs_params(input_dev, ABS_MT_POSITION_X, 0, 65535, 0, 0);
> > + /* Min, Max, Fuzz (expected noise in px, try 4?) and Flat */
> > + input_set_abs_params(input_dev, ABS_MT_POSITION_Y, 0, 65535, 0, 0);
> > + input_set_abs_params(input_dev, ABS_MT_TOOL_TYPE, 0, MT_TOOL_MAX, 0, 0);
> > + input_set_abs_params(input_dev, ABS_MT_DISTANCE, 0, 127, 0, 0);
> > + input_set_abs_params(input_dev, ABS_MT_PRESSURE, 0, 127, 0, 0);
> > +
> > + /* Registers the axiom device as a touch screen instead of as a mouse pointer */
> > + input_mt_init_slots(input_dev, U41_MAX_TARGETS, INPUT_MT_DIRECT);
> > +
> > + input_set_capability(input_dev, EV_KEY, BTN_LEFT);
> > +
> > + /* Enables the raw data for up to 4 force channels to be sent to the */
> > + /* input subsystem */
> > + set_bit(EV_REL, input_dev->evbit);
> > + set_bit(EV_MSC, input_dev->evbit);
> > + /* Declare that we support "RAW" Miscellaneous events */
> > + set_bit(MSC_RAW, input_dev->mscbit);
> > +
> > + if (!ts->irq_gpio) {
> > + error = input_setup_polling(input_dev, axiom_i2c_poll);
> > + if (error) {
> > + dev_err(ts->pdev, "Unable to set up polling: %d\n",
> > + error);
> > + return error;
> > + }
> > + input_set_poll_interval(input_dev, POLL_INTERVAL_DEFAULT_MS);
> > + }
> > +
> > + error = input_register_device(input_dev);
> > + if (error != 0) {
> > + dev_err(ts->pdev,
> > + "Could not register with Input Sub-system., error=%d\n",
> > + error);
> > + return error;
> > + }
> > +
> > + ts->input_dev = input_dev;
> > + input_set_drvdata(ts->input_dev, ts);
> > +
> > + dev_info(pdev, "AXIOM: I2C driver registered with Input Sub-System.\n");
>
> Drop silly tracing messages.
>
> > +
> > + /* Delay just a smidge before enabling the IRQ */
> > + udelay(10);
>
> ??? So where is the IRQ being enabled? This is confusing.
This need to move before IRQ is enabled.
>
> > +
> > + /* Ensure that all reports are initialised to not be present. */
> > + for (target = 0; target < U41_MAX_TARGETS; target++)
> > + ts->targets[target].state = TARGET_STATE_NOT_PRESENT;
> > +
> > + return 0;
> > +}
> > +
> > +static void axiom_i2c_remove(struct i2c_client *client)
> > +{
> > + struct axiom_data *ts = i2c_get_clientdata(client);
> > +
> > + axiom_remove(ts);
> > +}
> > +
> > +static const struct i2c_device_id axiom_i2c_id_table[] = {
> > + {"ax54a"},
> > + {},
> > +};
> > +
> > +MODULE_DEVICE_TABLE(i2c, axiom_i2c_id_table);
> > +
> > +static const struct of_device_id axiom_i2c_dt_ids[] = {
> > + {
> > + .compatible = "axiom,ax54a",
>
> That's not documented. And you would know it if you run basic checks
> before sending patches upstream.
>
I was actually suprised to found not all touchscreen drivers have
a documented binding and I know understand this is bad :) !
> Please run scripts/checkpatch.pl and fix reported warnings. Some
> warnings can be ignored, but the code here looks like it needs a fix.
> Feel free to get in touch if the warning is not clear.
OK, actually those are the only checkpatch warnings reported:
$ ./scripts/checkpatch.pl 0001-Input-add-driver-for-TouchNetix-aXiom-touchscreen.patch
WARNING: DT binding docs and includes should be a separate patch. See: Documentation/devicetree/bindings/submitting-patches.rst
WARNING: DT compatible string "axiom,ax54a" appears un-documented -- check ./Documentation/devicetree/bindings/
#954: FILE: drivers/input/touchscreen/axiom_i2c.c:326:
+ .compatible = "axiom,ax54a",
total: 0 errors, 2 warnings, 916 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
>
> > + .data = "axiom",
>
> Why?
Actually not used, more clean up required.
>
> That's not readable. Indent it like in other drivers.
>
> > + },
> > + {}
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, axiom_i2c_dt_ids);
> > +
> > +static struct i2c_driver axiom_i2c_driver = {
> > + .driver = {
> > + .name = "axiom_i2c",
> > + .of_match_table = of_match_ptr(axiom_i2c_dt_ids),
>
> Drop of_match_ptr(), it causes warnings in your code...
Ok.
>
> > + },
> > + .id_table = axiom_i2c_id_table,
> > + .probe = axiom_i2c_probe,
> > + .remove = axiom_i2c_remove,
> > +};
> > +
> > +module_i2c_driver(axiom_i2c_driver);
> > +
> > +MODULE_AUTHOR("Kamel Bouhara <kamel.bouhara@bootlin.com>");
> > +MODULE_DESCRIPTION("aXiom touchscreen I2C bus driver");
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS("axiom");
>
> Nope, you cannot have such alias and it is not even needed.
Ack, thanks !
>
>
> Best regards,
> Krzysztof
>
--
Kamel Bouhara, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox