* Re: [PATCH v2] Add rumble support to latest xbox controllers
From: Siarhei Vishniakou @ 2023-04-25 1:07 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: Bastien Nocera, Jiri Kosina, linux-input, linux-kernel
In-Reply-To: <CAO-hwJKOjfThF_CdF8kj=SiXKKLe5gr-Zh4iqgDpLmLNyjmbnw@mail.gmail.com>
Thanks folks,
I uploaded a v3 patch with the following changes:
1. Added link to wikipedia
2. Removed Change-Id from commit message
3. Added Bastien's 'Reviewed-by'.
On Thu, Mar 9, 2023 at 6:56 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On Wed, Mar 8, 2023 at 7:54 PM Bastien Nocera <hadess@hadess.net> wrote:
> >
> > On Wed, 2023-03-08 at 09:55 -0800, Siarhei Vishniakou wrote:
> > > Thanks Bastien!
> > >
> > > I can definitely add a link to the wikipedia page.
> > >
> > > Are you talking about adding the link to the commit message, or to
> > > hid-ids.h ?
> >
> > I think the commit message would be enough so we know which device
> > we're talking about, but now that you mention it, maybe the source code
> > would be a good idea too.
>
> Agree, adding the link in the source would be fine.
>
> >
> > As far as I've understood, and Benjamin can correct me, we don't need
> > to have IDs be in hid-ids.h because we don't need to declare them both
> > as a blocklist in the hid core, and then again in the driver itself.
>
> We don't need to have the blocklist anymore, but using hid-ids.h is
> still used, and sometimes has the benefit of raising eyebrows when you
> add support for a new device and realize that it was already defined.
> So keeping the list around is not so much of a bad thing.
>
> >
> > My take is that the patches could move the IDs from hid-ids.h to hid-
> > microsoft.c as they're changed. You then wouldn't need the macros, just
> > add a comment for each of variants, and that Wikipedia article can be
> > linked above the whole XBox controller section.
>
> We are definitely in the bikeshedding phase, but I would leave the
> code as it is in this patch :)
>
> One more comment below:
>
> >
> > What do you think?
> >
> > >
> > >
> > > On Wed, Mar 8, 2023 at 2:23 AM Bastien Nocera <hadess@hadess.net>
> > > wrote:
> > > >
> > > > On Tue, 2023-03-07 at 13:35 -0800, Siarhei Vishniakou wrote:
> > > > > Currently, rumble is only supported via bluetooth on a single
> > > > > xbox
> > > > > controller, called 'model 1708'. On the back of the device, it's
> > > > > named
> > > > > 'wireless controller for xbox one'. However, in 2021, Microsoft
> > > > > released
> > > > > a firmware update for this controller. As part of this update,
> > > > > the
> > > > > HID
> > > > > descriptor of the device changed. The product ID was also changed
> > > > > from
> > > > > 0x02fd to 0x0b20. On this controller, rumble was supported via
> > > > > hid-microsoft, which matched against the old product id (0x02fd).
> > > > > As
> > > > > a
> > > > > result, the firmware update broke rumble support on this
> > > > > controller.
> > > > >
> > > > > The hid-microsoft driver actually supports rumble on the new
> > > > > firmware,
> > > > > as well. So simply adding new product id is sufficient to bring
> > > > > back
> > > > > this support.
> > > > >
> > > > > After discussing further with the xbox team, it was pointed out
> > > > > that
> > > > > another xbox controller, xbox elite series 2, can be supported in
> > > > > a
> > > > > similar way.
> > > > >
> > > > > Add rumble support for all of these devices in this patch. Two of
> > > > > the
> > > > > devices have received firmware updates that caused their product
> > > > > id's
> > > > > to
> > > > > change. Both old and new firmware versions of these devices were
> > > > > tested.
> > > > >
> > > > > The tested controllers are:
> > > > >
> > > > > 1. 'wireless controller for xbox one', model 1708
> > > > > 2. 'xbox wireless controller', model 1914. This is also sometimes
> > > > > referred to as 'xbox series S|X'.
> > > > > 3. 'elite series 2', model 1797.
> > > > >
> > > > > The tested configurations are:
> > > > > 1. model 1708, pid 0x02fd (old firmware)
> > > > > 2. model 1708, pid 0x0b20 (new firmware)
> > > > > 3. model 1914, pid 0x0b13
> > > > > 4. model 1797, pid 0x0b05 (old firmware)
> > > > > 5. model 1797, pid 0x0b22 (new firmware)
> > > > >
> > > > > I verified rumble support on both bluetooth and usb.
> > > >
> > > > Looks good although I would personally have preferred separate
> > > > patches
> > > > for each controller.
> > > >
> > > > Reviewed-by: Bastien Nocera <hadess@hadess.net>
> > > >
> > > > Would a link to:
> > > > https://en.wikipedia.org/wiki/Xbox_Wireless_Controller#Summary
> > > > also be useful to make which model is which clearer in the minds of
> > > > future readers?
> > > >
> > > > Cheers
> > > >
> > > > >
> > > > > Signed-off-by: Siarhei Vishniakou <svv@google.com>
> > > > > Change-Id: I3337a7ab5f40759c85bf67bf0dbe5d4de31ce1ff
>
> That change-id should be dropped, it has no meaning to us.
>
> Cheers,
> Benjamin
>
> > > > > ---
> > > > > drivers/hid/hid-ids.h | 6 +++++-
> > > > > drivers/hid/hid-microsoft.c | 11 ++++++++++-
> > > > > 2 files changed, 15 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> > > > > index 053853a891c5..c9b75f8ba49a 100644
> > > > > --- a/drivers/hid/hid-ids.h
> > > > > +++ b/drivers/hid/hid-ids.h
> > > > > @@ -903,7 +903,11 @@
> > > > > #define USB_DEVICE_ID_MS_TYPE_COVER_2 0x07a9
> > > > > #define USB_DEVICE_ID_MS_POWER_COVER 0x07da
> > > > > #define USB_DEVICE_ID_MS_SURFACE3_COVER 0x07de
> > > > > -#define USB_DEVICE_ID_MS_XBOX_ONE_S_CONTROLLER 0x02fd
> > > > > +#define USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1708 0x02fd
> > > > > +#define
> > > > > USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1708_BLE 0x0b20
> > > > > +#define USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1914 0x0b13
> > > > > +#define USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1797 0x0b05
> > > > > +#define
> > > > > USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1797_BLE 0x0b22
> > > > > #define USB_DEVICE_ID_MS_PIXART_MOUSE 0x00cb
> > > > > #define USB_DEVICE_ID_8BITDO_SN30_PRO_PLUS 0x02e0
> > > > >
> > > > > diff --git a/drivers/hid/hid-microsoft.c b/drivers/hid/hid-
> > > > > microsoft.c
> > > > > index 071fd093a5f4..9345e2bfd56e 100644
> > > > > --- a/drivers/hid/hid-microsoft.c
> > > > > +++ b/drivers/hid/hid-microsoft.c
> > > > > @@ -446,7 +446,16 @@ static const struct hid_device_id
> > > > > ms_devices[] =
> > > > > {
> > > > > .driver_data = MS_PRESENTER },
> > > > > { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, 0x091B),
> > > > > .driver_data = MS_SURFACE_DIAL },
> > > > > - { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT,
> > > > > USB_DEVICE_ID_MS_XBOX_ONE_S_CONTROLLER),
> > > > > +
> > > > > + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT,
> > > > > USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1708),
> > > > > + .driver_data = MS_QUIRK_FF },
> > > > > + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT,
> > > > > USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1708_BLE),
> > > > > + .driver_data = MS_QUIRK_FF },
> > > > > + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT,
> > > > > USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1914),
> > > > > + .driver_data = MS_QUIRK_FF },
> > > > > + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT,
> > > > > USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1797),
> > > > > + .driver_data = MS_QUIRK_FF },
> > > > > + { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT,
> > > > > USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1797_BLE),
> > > > > .driver_data = MS_QUIRK_FF },
> > > > > { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT,
> > > > > USB_DEVICE_ID_8BITDO_SN30_PRO_PLUS),
> > > > > .driver_data = MS_QUIRK_FF },
> > > >
> >
>
^ permalink raw reply
* [PATCH v3] Add rumble support to latest xbox controllers
From: Siarhei Vishniakou @ 2023-04-25 1:06 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel
Cc: Siarhei Vishniakou, Bastien Nocera
Currently, rumble is only supported via bluetooth on a single xbox
controller, called 'model 1708'. On the back of the device, it's named
'wireless controller for xbox one'. However, in 2021, Microsoft released
a firmware update for this controller. As part of this update, the HID
descriptor of the device changed. The product ID was also changed from
0x02fd to 0x0b20. On this controller, rumble was supported via
hid-microsoft, which matched against the old product id (0x02fd). As a
result, the firmware update broke rumble support on this controller.
The hid-microsoft driver actually supports rumble on the new firmware,
as well. So simply adding new product id is sufficient to bring back
this support.
After discussing further with the xbox team, it was pointed out that
another xbox controller, xbox elite series 2, can be supported in a
similar way.
Add rumble support for all of these devices in this patch. Two of the
devices have received firmware updates that caused their product id's to
change. Both old and new firmware versions of these devices were tested.
The tested controllers are:
1. 'wireless controller for xbox one', model 1708
2. 'xbox wireless controller', model 1914. This is also sometimes
referred to as 'xbox series S|X'.
3. 'elite series 2', model 1797.
The tested configurations are:
1. model 1708, pid 0x02fd (old firmware)
2. model 1708, pid 0x0b20 (new firmware)
3. model 1914, pid 0x0b13
4. model 1797, pid 0x0b05 (old firmware)
5. model 1797, pid 0x0b22 (new firmware)
I verified rumble support on both bluetooth and usb.
Reviewed-by: Bastien Nocera <hadess@hadess.net>
Signed-off-by: Siarhei Vishniakou <svv@google.com>
---
drivers/hid/hid-ids.h | 8 +++++++-
drivers/hid/hid-microsoft.c | 11 ++++++++++-
2 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 053853a891c5..5d98b2a3a5e4 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -903,7 +903,13 @@
#define USB_DEVICE_ID_MS_TYPE_COVER_2 0x07a9
#define USB_DEVICE_ID_MS_POWER_COVER 0x07da
#define USB_DEVICE_ID_MS_SURFACE3_COVER 0x07de
-#define USB_DEVICE_ID_MS_XBOX_ONE_S_CONTROLLER 0x02fd
+// For a description of the Xbox controller models, refer to:
+// https://en.wikipedia.org/wiki/Xbox_Wireless_Controller#Summary
+#define USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1708 0x02fd
+#define USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1708_BLE 0x0b20
+#define USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1914 0x0b13
+#define USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1797 0x0b05
+#define USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1797_BLE 0x0b22
#define USB_DEVICE_ID_MS_PIXART_MOUSE 0x00cb
#define USB_DEVICE_ID_8BITDO_SN30_PRO_PLUS 0x02e0
diff --git a/drivers/hid/hid-microsoft.c b/drivers/hid/hid-microsoft.c
index 071fd093a5f4..9345e2bfd56e 100644
--- a/drivers/hid/hid-microsoft.c
+++ b/drivers/hid/hid-microsoft.c
@@ -446,7 +446,16 @@ static const struct hid_device_id ms_devices[] = {
.driver_data = MS_PRESENTER },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, 0x091B),
.driver_data = MS_SURFACE_DIAL },
- { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_XBOX_ONE_S_CONTROLLER),
+
+ { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1708),
+ .driver_data = MS_QUIRK_FF },
+ { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1708_BLE),
+ .driver_data = MS_QUIRK_FF },
+ { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1914),
+ .driver_data = MS_QUIRK_FF },
+ { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1797),
+ .driver_data = MS_QUIRK_FF },
+ { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_MS_XBOX_CONTROLLER_MODEL_1797_BLE),
.driver_data = MS_QUIRK_FF },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_MICROSOFT, USB_DEVICE_ID_8BITDO_SN30_PRO_PLUS),
.driver_data = MS_QUIRK_FF },
--
2.40.0.634.g4ca3ef3211-goog
^ permalink raw reply related
* Re: [PATCH v2] Input: synaptics - disable intertouch for Lenovo L440
From: Jonathan Denose @ 2023-04-24 19:11 UTC (permalink / raw)
To: Andrew Duggan
Cc: Lyude Paul, Andrew Duggan, amandhoot12@gmail.com, Dmitry Torokhov,
jdenose@google.com, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org, markpearson@lenovo.com,
wsa+renesas@sang-engineering.com, benjamin.tissoires
In-Reply-To: <65C23A49-5A55-4CF4-9AFD-2DA504DAABF5@duggan.us>
Hi Andrew,
Thanks for your reply. As an update, I was able to try the patch here:
https://lore.kernel.org/all/YgHTYrODoo2ou49J@google.com/ and it
resolves the suspend/resume issue on this device. I'm not sure what
the status of the linked patch is, to me it doesn't look like it was
merged anywhere.
On Mon, Apr 17, 2023 at 3:14 PM Andrew Duggan <andrew@duggan.us> wrote:
>
> Hi Lyude and Jonathan,
>
> I was just about to reply and suggest that we look into this issue a little more since the touchpad in the L440 would benefit from the additional data from the intertouch interface. Especially, since it has a large area and several buttons. PS/2 only reports position data for two fingers so three finger gestures is another example.
>
> Generally, these types of suspend / resume issues are the result of the touchpad resetting and the firmware expecting commands from the PS/2 interface. On resume, the PS/2 driver should send a command over the PS/2 interface to switch the touchpad firmware back into intertouch (SMBus) mode. The logs you provided look like that's what is happening here. The SMBus driver is sending commands, but the touchpad firmware won't respond until it is switch back into intertouch mode. It has been a while since I have worked on these touchpads, but from what I remember I think there is code in the psmouse-smbus driver to handle these situations. I added Benjamin Tissoires to CC since I think he worked on that handling. I thought suspend / resume was tested on with these "top button" touchpads when support for them was added. I don't know if the L440 specifically included in the testing. I'm curious if this is a regression or not.
>
> Regarding the patch, I do have one comment below:
>
> > On Apr 17, 2023, at 11:52, Jonathan Denose <jdenose@chromium.org> wrote:
> >
> > CAUTION: Email originated externally, do not click links or open attachments unless you recognize the sender and know the content is safe.
> >
> >
> > Sorry, I thought I sent this as plain text but I think maybe not.
> > Trying once more, the message was:
> >
> > I think that disabling synaptics_intertouch would resolve the issue
> > mentioned in the commit, but cause a regression in the functionality
> > that intertouch is supposed to bring, like three-finger gestures. I'm
> > attaching some of the logs that I captured when the touchpad was
> > failing on resume. I think the main culprit is
> > i2c_smbus_read_byte_data where the driver is unable to get the SMBus
> > version number. I get the following lines in dmesg:
> >
> > [ 2869.745860] rmi4_smbus 0-002c: failed to get SMBus version number!
> > [ 2869.746060] rmi4_physical rmi4-00: rmi_driver_reset_handler: Failed
> > to read current IRQ mask.
> > [ 2869.746260] rmi4_f01 rmi4-00.fn01: Failed to restore normal operation: -6.
> > [ 2869.746262] rmi4_f01 rmi4-00.fn01: Resume failed with code -6.
> > [ 2869.746264] rmi4_physical rmi4-00: Failed to suspend functions: -6
> > [ 2869.746265] rmi4_smbus 0-002c: Failed to resume device: -6
> > [ 2869.746268] rmi4_smbus 0-002c: rmi_smb_resume+0x0/0x6b [rmi_smbus]
> > returned 0 after 549 usecs
> > [ 2869.746446] rmi4_physical rmi4-00: Failed to read irqs, code=-6
> >
> > Any ideas on what might be causing this, only on resume from deep sleep?
> >
> >
> > On Mon, Apr 17, 2023 at 1:47 PM Jonathan Denose <jdenose@chromium.org> wrote:
> >>
> >> I think that disabling synaptics_intertouch would resolve the issue mentioned in the commit, but cause a regression in the functionality that intertouch is supposed to bring, like three-finger gestures. I'm attaching some of the logs that I captured when the touchpad was failing on resume. I think the main culprit is i2c_smbus_read_byte_data where the driver is unable to get the SMBus version number. I get the following lines in dmesg:
> >>
> >> [ 2869.745860] rmi4_smbus 0-002c: failed to get SMBus version number!
> >> [ 2869.746060] rmi4_physical rmi4-00: rmi_driver_reset_handler: Failed to read current IRQ mask.
> >> [ 2869.746260] rmi4_f01 rmi4-00.fn01: Failed to restore normal operation: -6.
> >> [ 2869.746262] rmi4_f01 rmi4-00.fn01: Resume failed with code -6.
> >> [ 2869.746264] rmi4_physical rmi4-00: Failed to suspend functions: -6
> >> [ 2869.746265] rmi4_smbus 0-002c: Failed to resume device: -6
> >> [ 2869.746268] rmi4_smbus 0-002c: rmi_smb_resume+0x0/0x6b [rmi_smbus] returned 0 after 549 usecs
> >> [ 2869.746446] rmi4_physical rmi4-00: Failed to read irqs, code=-6
> >>
> >> Any ideas on what might be causing this, only on resume from deep sleep?
> >>
> >>
> >> On Fri, Apr 14, 2023 at 11:41 AM Jonathan Denose <jdenose@chromium.org> wrote:
> >>>
> >>> When intertouch is enabled for the L440 a (deep)sleep/resume
> >>> cycle causes the touchpad driver to hang which causes the
> >>> touchpad to become unresponsive. Disable intertouch resolves
> >>> this issue and the touchpad is fine after resume from sleep.
> >>>
> >>> Additionally, when the PNP id for the L440 is only removed
> >>> from the topbuttonpad_pnp_ids list, a message is logged to
> >>> enable psmouse.synaptics_intertouch, which would cause the
> >>> sleep/resume issue again. By removing the PNP id from
> >>> topbutton_pnp_ids and then adding it to the
> >>> forcepad_pnp_ids array, intertouch is disabled and the
> >>> message is not logged.
> >>>
> >>> Signed-off-by: Jonathan Denose <jdenose@google.com>
> >>> ---
> >>>
> >>> Changes in v2:
> >>> - remove debug statement
> >>>
> >>> drivers/input/mouse/synaptics.c | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> >>> index fa021af8506e4..b7218b7652c20 100644
> >>> --- a/drivers/input/mouse/synaptics.c
> >>> +++ b/drivers/input/mouse/synaptics.c
> >>> @@ -150,7 +150,6 @@ static const char * const topbuttonpad_pnp_ids[] = {
> >>> "LEN2001", /* Edge E431 */
> >>> "LEN2002", /* Edge E531 */
> >>> "LEN2003",
> >>> - "LEN2004", /* L440 */
> >>> "LEN2005",
> >>> "LEN2006", /* Edge E440/E540 */
> >>> "LEN2007",
> >>> @@ -198,6 +197,7 @@ static const char * const smbus_pnp_ids[] = {
> >>> static const char * const forcepad_pnp_ids[] = {
> >>> "SYN300D",
> >>> "SYN3014",
> >>> + "LEN2004", /* L440 */
>
> While this does seem to elliminate the message, the touchpad in the L440 is not a forcepad. Adding the L440 PnP ID here implies that it is one of these special forcepads which reports "force" data for contacts and that is not the case here.
>
> >>> NULL
> >>> };
> >>>
> >>> —
> >>> 2.39.2
> >>>
>
>
> Andrew
>
^ permalink raw reply
* Re: [PATCH 2/2] HID: i2c-hid: goodix: Add support for powered-in-suspend property
From: Doug Anderson @ 2023-04-24 18:16 UTC (permalink / raw)
To: Jeff LaBundy
Cc: Fei Shao, Benjamin Tissoires, Rob Herring, linux-mediatek,
Dmitry Torokhov, Jiri Kosina, Matthias Kaehlcke, Stephen Kitt,
linux-input, linux-kernel
In-Reply-To: <ZEX5tc2LSZoVswc2@nixie71>
Hi,
On Sun, Apr 23, 2023 at 8:38 PM Jeff LaBundy <jeff@labundy.com> wrote:
>
> > @@ -37,13 +38,34 @@ static int goodix_i2c_hid_power_up(struct i2chid_ops *ops)
> > container_of(ops, struct i2c_hid_of_goodix, ops);
> > int ret;
> >
> > - ret = regulator_enable(ihid_goodix->vdd);
> > - if (ret)
> > - return ret;
> > -
> > - ret = regulator_enable(ihid_goodix->vddio);
> > - if (ret)
> > - return ret;
> > + /*
> > + * This is to ensure that the reset GPIO will be asserted and the
> > + * regulators will be enabled for all cases.
> > + */
> > + if (ihid_goodix->powered_in_suspend) {
> > + /*
> > + * This is not mandatory, but we assert reset here (instead of
> > + * in power-down) to ensure that the device will have a clean
> > + * state later on just like the normal scenarios would have.
> > + *
> > + * Also, since the regulators were not disabled in power-down,
> > + * we don't need to enable them here.
> > + */
> > + gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
> > + } else {
> > + /*
> > + * In this case, the reset is already asserted (either in
> > + * probe or power-down).
> > + * All we need is to enable the regulators.
> > + */
> > + ret = regulator_enable(ihid_goodix->vdd);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regulator_enable(ihid_goodix->vddio);
> > + if (ret)
> > + return ret;
> > + }
>
> Please let me know in case I have misunderstood, but I don't see a need
> to change the regulator_enable/disable() logic if this property is set.
> If the regulators are truly always-on, the regulator core already knows
> what to do and we should not duplicate that logic here.
>
> Based on the alleged silicon erratum discussed in patch [1/2], it seems
> we only want to control the behavior of the reset GPIO. Therefore, only
> the calls to gpiod_set_value_cansleep() should be affected and the name
> of the property updated to reflect what it's actually doing.
This would be OK w/ me.
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: input: goodix: Add powered-in-suspend property
From: Doug Anderson @ 2023-04-24 18:14 UTC (permalink / raw)
To: Jeff LaBundy
Cc: Fei Shao, Benjamin Tissoires, Rob Herring, linux-mediatek,
Dmitry Torokhov, Krzysztof Kozlowski, devicetree, linux-input,
linux-kernel
In-Reply-To: <ZEX4BEVMf6GtvaAP@nixie71>
Hi,
On Sun, Apr 23, 2023 at 8:31 PM Jeff LaBundy <jeff@labundy.com> wrote:
>
> Back-powering can come in two forms:
>
> 1. The one you've described, which is by far the most common. As you mention,
> it is not the case here since the issue happens only when we drive the GPIO
> low and not high.
>
> 2. Through a forbidden power supply sequence, an input pin of an IC with
> multiple power supplies becomes a weak power supply itself. Grounding the
> GPIO then sinks current into the SoC.
>
> This problem smelled like (2), especially since asserting the GPIO or powering
> down the supply alleviates the symptom. Most Goodix controllers I've worked
> with have two or more supplies, and the datasheet requires them to be enabled
> or disabled in a specific order.
>
> Based on Fei's feedback, however, this IC does not seem to be one of those
> since there is reportedly only a single rail. I guess either vdd or vddio is
> tied to a dummy regulator? If so, then perhaps we can scratch this theory.
There is one rail provided from the main board, but there can be two
voltage levels involved depending on stuffings on the touchscreen
itself. I talked a bit about this in commit 1d18c1f3b7d9
("dt-bindings: HID: i2c-hid: goodix: Add mainboard-vddio-supply").
> Fair enough, I was simply sketpical that this was the _right_ workaround.
> Were this an issue of only supply A left on yet the IC datasheet requires
> supply B to remain on if supply A is on, I would rather see this solved by
> updating a regulator dts node, trusted FW sequence, etc.
Right. That type of stuff was looked at in detail by two separate
teams, so I don't think this is the issue.
> Thanks for your due diligence; if this really is a silicon issue, then
> the driver should indeed accommodate it.
>
> That being said, the name 'powered-in-suspend' seems a bit conflated. We
> should not be duplicating regulator policy in this driver; the existing
> naming also may cause confusion for other HW configurations that do leave
> vdd and vddio on during suspend, but don't have this problem.
>
> Here, we actually mean to control the behavior of the reset GPIO through
> suspend and therefore something like 'goodix,no-reset-during-suspend' is
> closer to what I understand us to intend to do. I will add more details
> in patch [2/2].
I'd be fine with that name. ...ah, and adding the "goodix," prefix is
a good call.
-Doug
^ permalink raw reply
* Re: [PATCH 26/43] dt-bindings: input: Add DT bindings ep93xx keypad
From: Rob Herring @ 2023-04-24 16:21 UTC (permalink / raw)
To: Nikita Shubin
Cc: Arnd Bergmann, Linus Walleij, Alexander Sverdlin, Dmitry Torokhov,
Krzysztof Kozlowski, linux-input, devicetree, linux-kernel
In-Reply-To: <20230424123522.18302-27-nikita.shubin@maquefel.me>
On Mon, Apr 24, 2023 at 03:34:42PM +0300, Nikita Shubin wrote:
> Add YAML bindings ep93xx SoC.
>
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> ---
>
> Notes:
> Linus Walleij:
> - replaced hex with proper <KEY_UP>, etc
>
> .../bindings/input/cirrus,ep93xx-keypad.yaml | 123 ++++++++++++++++++
> 1 file changed, 123 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/input/cirrus,ep93xx-keypad.yaml
>
> diff --git a/Documentation/devicetree/bindings/input/cirrus,ep93xx-keypad.yaml b/Documentation/devicetree/bindings/input/cirrus,ep93xx-keypad.yaml
> new file mode 100644
> index 000000000000..0310114de22e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/cirrus,ep93xx-keypad.yaml
> @@ -0,0 +1,123 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/cirrus,ep93xx-keypad.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Cirrus ep93xx keypad
> +
> +maintainers:
> + - Dmitry Torokhov <dmitry.torokhov@gmail.com>
> + - Alexander Sverdlin <alexander.sverdlin@gmail.com>
> +
> +allOf:
> + - $ref: "/schemas/input/matrix-keymap.yaml#"
Drop quotes.
> +
> +description: |
> + The KPP is designed to interface with a keypad matrix with 2-point contact
> + or 3-point contact keys. The KPP is designed to simplify the software task
> + of scanning a keypad matrix. The KPP is capable of detecting, debouncing,
> + and decoding one or multiple keys pressed simultaneously on a keypad.
> +
> +properties:
> + compatible:
> + enum:
> + - cirrus,ep9301-keypad
> +
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + maxItems: 1
> +
> + clocks:
> + maxItems: 1
> +
> + clock-names:
> + enum:
> + - ep93xx-keypad
> +
> + debounce:
I think we have a common property for this.
> + description: |
> + Time in microseconds that key must be pressed or
> + released for state change interrupt to trigger.
> + $ref: /schemas/types.yaml#/definitions/uint32
> +
> + prescale:
> + description: row/column counter pre-scaler load value
> + $ref: /schemas/types.yaml#/definitions/uint32
constraints? Or 0-2^32 is valid?
> +
> + clk-rate:
No, have standard bindings for this.
> + description: clock rate setting
> + $ref: /schemas/types.yaml#/definitions/uint32
> +
> + disable-3-key:
Needs a vendor prefix.
> + type: boolean
> + description:
> + Disable 3 Key reset. Setting this disables the three key reset
> + output to the watchdog reset block.
> +
> + diag-mode:
Why do we need this in DT. Shouldn't this be a runtime setting.
> + type: boolean
> + description:
> + Key scan diagnostic mode. Setting this allows key scanning to be
> + directly controlled through the key register by writes from the
> + ARM Core.
> +
> + back-drive:
Needs a vendor prefix.
> + type: boolean
> + description:
> + Key scan back driving enable. Setting this enables the key
> + scanning logic to back drive the row and column pins of the
> + chip high during the first two column counts in the
> + row/column counter.
> +
> + test-mode:
Why do we need this in DT. Shouldn't this be a runtime setting.
> + type: boolean
> + description:
> + Test mode. When this is set, the counter RC_COUNT is advanced
> + by 8 counts when EN is active. The effect is that only column 0
> + is checked in each row. This test mode allows a faster test
> + of the ROW pins.
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - clocks
> + - clock-names
> + - linux,keymap
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/clock/cirrus,ep93xx-clock.h>
> + #include <dt-bindings/input/input.h>
> + keypad@800f0000 {
> + compatible = "cirrus,ep9301-keypad";
> + reg = <0x800f0000 0x0c>;
> + interrupt-parent = <&vic0>;
> + interrupts = <29>;
> + clocks = <&syscon EP93XX_CLK_KEYPAD>;
> + clock-names = "ep93xx-keypad";
> + pinctrl-names = "default";
> + pinctrl-0 = <&keypad_default_pins>;
> + linux,keymap = <KEY_UP>,
> + <KEY_DOWN>,
> + <KEY_VOLUMEDOWN>,
> + <KEY_HOME>,
> + <KEY_RIGHT>,
> + <KEY_LEFT>,
> + <KEY_ENTER>,
> + <KEY_VOLUMEUP>,
> + <KEY_F6>,
> + <KEY_F8>,
> + <KEY_F9>,
> + <KEY_F10>,
> + <KEY_F1>,
> + <KEY_F2>,
> + <KEY_F3>,
> + <KEY_POWER>;
> + };
> +
> --
> 2.39.2
>
^ permalink raw reply
* Re: [PATCH] HID: amd_sfh: Support for additional light sensor
From: Basavaraj Natikar @ 2023-04-24 16:13 UTC (permalink / raw)
To: Linux regressions mailing list, Jiri Kosina, Basavaraj Natikar
Cc: benjamin.tissoires, linux-input
In-Reply-To: <0b83318b-5ae2-9b90-5d5a-11d95f3501c4@leemhuis.info>
On 4/24/2023 2:02 PM, Linux regression tracking (Thorsten Leemhuis) wrote:
> On 13.04.23 15:57, Jiri Kosina wrote:
>> On Tue, 11 Apr 2023, Basavaraj Natikar wrote:
>>
>>> There is support for additional light sensors in the SFH firmware.
>>> As a result, add support for additional light sensors.
>>>
>>> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
>> Applied.
> FWIW, there is a report about a hang at boot that was bisected to this
> patch. For details see:
> https://bugzilla.kernel.org/show_bug.cgi?id=217354
Thanks a lot for information.
Added a Fix in
https://lore.kernel.org/all/20230424160406.2579888-1-Basavaraj.Natikar@amd.com/
Thanks,
--
Basavaraj
>
> Ciao, Thorsten
>
> P.S. I normally don't track regressions in -next, I just noticed that
> report and wanted to let you know about it.
^ permalink raw reply
* [PATCH linux-next] HID: amd_sfh: Fix max supported HID devices
From: Basavaraj Natikar @ 2023-04-24 16:04 UTC (permalink / raw)
To: jikos, benjamin.tissoires, linux-input; +Cc: Basavaraj Natikar
commit 4bd763568dbd ("HID: amd_sfh: Support for additional light sensor")
adds additional sensor devices, but forgets to add the number of HID
devices to match. Thus, the number of HID devices does not match the
actual number of sensors.
In order to prevent corruption and system hangs when more than the
allowed number of HID devices are accessed, the number of HID devices is
increased accordingly.
Fixes: 4bd763568dbd ("HID: amd_sfh: Support for additional light sensor")
Link: https://bugzilla.kernel.org/show_bug.cgi?id=217354
Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
---
drivers/hid/amd-sfh-hid/amd_sfh_hid.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_hid.h b/drivers/hid/amd-sfh-hid/amd_sfh_hid.h
index 528036892c9d..97296f587bc7 100644
--- a/drivers/hid/amd-sfh-hid/amd_sfh_hid.h
+++ b/drivers/hid/amd-sfh-hid/amd_sfh_hid.h
@@ -11,7 +11,7 @@
#ifndef AMDSFH_HID_H
#define AMDSFH_HID_H
-#define MAX_HID_DEVICES 5
+#define MAX_HID_DEVICES 6
#define AMD_SFH_HID_VENDOR 0x1022
#define AMD_SFH_HID_PRODUCT 0x0001
--
2.25.1
^ permalink raw reply related
* Re: [PATCH 27/43] input: keypad: ep93xx: add DT support for Cirrus EP93xx
From: Andy Shevchenko @ 2023-04-24 14:45 UTC (permalink / raw)
To: Nikita Shubin
Cc: Arnd Bergmann, Linus Walleij, Alexander Sverdlin, Dmitry Torokhov,
Jonathan Cameron, Lv Ruyi, linux-input, linux-kernel
In-Reply-To: <20230424123522.18302-28-nikita.shubin@maquefel.me>
On Mon, Apr 24, 2023 at 03:34:43PM +0300, Nikita Shubin wrote:
> - get keymap from the device tree
> - find register range from the device tree
> - get interrupts from device tree
Note, the below comments may be applied to the whole series where it makes sense.
...
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
Why?
You do not use them. Please, check all your patches to follow the rule: include
only the headers you are really using (or their up level wrappers if required).
(Hint: in this case you have to include mod_devicetable.h)
...
> +static const struct of_device_id ep93xx_keypad_of_ids[] = {
> + { .compatible = "cirrus,ep9301-keypad" },
> + {},
No comma for the terminator entry.
> +};
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: Proposal: Add a means to disable kernel driver logic when associated hidraw is opened
From: Benjamin Tissoires @ 2023-04-24 13:07 UTC (permalink / raw)
To: Vicki Pfau; +Cc: open list:HID CORE LAYER, Pierre-Loup Griffais
In-Reply-To: <a3b1f768-27e9-c9d5-ad2c-c56a48522d41@endrift.com>
Hi Vicki,
On Fri, Apr 21, 2023 at 4:36 AM Vicki Pfau <vi@endrift.com> wrote:
>
> Hello,
>
> Following up on a conversation from last year (cf. https://lore.kernel.org/linux-input/CAO-hwJLfY+D0NyCUCncrjcXETKwOBqj1CuHtB_mtGhYfKV0Bww@mail.gmail.com/) about how to approach differing opinions about how drivers should work between userspace and kernel, we're looking for a way to effectively stop the kernel from doing anything "smart" with a a HID device, e.g. a hid-sony device, when the associated hidraw is opened. At the moment, we have a specialized daemon that will find the mouse evdev associated with the controller when it's created and inhibit it, but this is anything but ideal and only handles the mouse itself, not the kernel logic in general. You can also see the hackish way this is implemented in the hid-steam driver, which uses an intermediary hid ll_driver to intercept the open and close commands and stop passing data if the hidraw gets opened. I consider this implementation to be very much a hack and an anti-pattern, and I think other HID implementers would agree, but I'm not sure there's a cleaner way to detect this in the kernel at the moment.
>
> I see a bit of a difference of opinion as to what should be happening here between developers on our end and the kernel end. Our position is "we have a userspace driver that does everything we want, we'd like the kernel to stop trying to be smart when our driver is active", and I expect the kernel developer opinion is "why should you have a userspace driver at all when our kernel driver is fine, and if it's not you can upstream patches?", so there's probably some need to find a middle ground if we want things to work well for users in the end.
The main problem we have here, is that if userspace is actively using
hidraw to configure the device, the kernel is not aware, and thus its
state will be wrong. So my initial reaction is "we need to have ad-hoc
and opt-in functionality for these, because the states the kernel
needs to monitor will differ from one device to one other". But this
opt-in could (should) be controlled from userspace through eBPF.
OTOH, maybe the simplest solution would be to entirely unbind the
device from the kernel driver when steam opens and rebinds it when it
closes. This way the kernel will reinitialize the device itself
properly and keep its state coherent. We can think of multiple
solutions for disabling the driver and still keeping hidraw open, and
guess what, eBPF is one of them :)
Note though that disabling the kernel module might require some
changes in the kernel driver itself, so maybe that's not the best path
too. (eBPF can not change any quirks on a given device, but maybe that
is something we can think of).
>
> We'd previously discussed an ioctl for the hidraw, or perhaps an eBPF approach, though I think at the time of that discussion, eBPF wasn't mature for the HID subsystem. I don't know the current state of that, or if it's possible to do this with just write access to the hidraw device (the ideal case for how we want to handle this--root is pretty much out of the question for an "ideal case"). Since we still don't have a good answer for this, as far as I'm aware, I'd like to try to reach an approach that's amicable for both sides.
Good timing. Kernel 6.3 was out just yesterday, and HID-BPF is in :)
(well, not all use cases I envisioned are implemented, but at least
the bulk is there).
>
> I know that the ioctl approach meant having to introduce logic for programs in userspace, but in this specific case, and possibly others, that's actually what we're looking for. Further, the reason root is out is because Steam isn't the OS (in most cases), and we want this to only happen when Steam is running. Having to run a daemon as root underneath Steam would require something like a setuid binary or custom sudo/polkit rules, which of course need root to set up anyway. So while eBPF may fit some use cases, e.g. full control over the system via DE or systemd, etc, it's not what we're looking for here.
I think you are dismissing eBPF too quickly :)
Basically, eBPF doesn't require you to have a daemon loaded: you can
load the BPF program, attach it to the device, pin it in the kernel
and close your eBPF loader. So all you need is a new udev rule that
calls for a program to load the eBPF program just at plugging time.
Eventually, for such "no-daemon" programs, the kernel itself will
load those programs, and userspace can just override them if they are
not up to date.
Which means that when you load the eBPF program, you just "patched"
the kernel with your current API.
[Side note: my push for eBPF is because this is exactly the kind of
use case I envisioned for it: an app (or set of apps) wants to tweak
the kernel behavior only when it is started, and wants to be in
control. If this app wants to change some behaviour, fixing the eBPF
program is way simpler and can be done in sync with the app update
rather than updating the API introduced in the kernel]
I think the following (in pseudo-ish-code) would work for
hid-playstation with the current 6.3 kernel:
```
/*
* variable that is set to true when steam is opening the device
* we might as well use a map instead
*/
static bool steam_opened = false;
/*
* HID unique ID used by the device we are monitoring (set during attach)
*/
static u16 hid_steam_device_id;
/*
* take the current report descriptor, and extend it by adding more
opaque report IDs
* for example, if the mouse is reporting on report ID 0x02, we set a
new hid collection
* with opaque data (const) as report ID 0xf2
*/
SEC("fmod_ret/hid_bpf_rdesc_fixup")
int BPF_PROG(hid_fix_rdesc, struct hid_bpf_ctx *hctx)
{
__u8 *data = hid_bpf_get_data(hctx, 0 /* offset */, 4096 /* size */);
if (!data)
return 0; /* EPERM check */
/* append a new report descriptor to the current one */
return old_size + appended;
}
SEC("fmod_ret/hid_bpf_device_event")
int BPF_PROG(disable_when_opened, struct hid_bpf_ctx *hid_ctx)
{
__u8 *data = hid_bpf_get_data(hid_ctx, 0, 1);
if (!data)
return 0; /* EPERM check */
if (steam_opened)
data[0] += 0xf0;
return 0;
}
/*
* completely untested
*/
int hid_id_from_file(struct inode *inode, struct file *file)
{
struct hidraw_list *list;
struct hidraw *dev;
if (!file->private_data)
return -EINVAL;
list = file->private_data;
if (!list->hidraw)
return -EINVAL;
dev = list->hidraw;
return dev->hid->id;
}
SEC("fexit/hidraw_open")
int BPF_PROG(fexit_hidraw_open, struct inode *inode, struct file *file, int ret)
{
/* exit if any error happened */
if (ret)
return 0;
if (hid_id_from_file(inode, file) == hid_steam_device_id)
steam_opened = true;
return 0;
}
SEC("fenty/hidraw_release")
int BPF_PROG(fentry_hidraw_release, struct inode *inode, struct file *file)
{
struct hidraw_list *dev;
if (file->private_data)
dev = file->private_data;
if (hid_id_from_file(inode, file) == hid_steam_device_id)
steam_opened = false;
return 0;
}
```
Minus the completely untested bit to go from the inode to the HID id
(which I already did once or twice IIRC), this basically diverts any
incoming report from 0x0N to 0xfN. And then in Steam, or any other
client, you can either revert that change or just handle it as if this
was a normal report from the device.
The bits to reset the kernel driver would consist in detaching the hid
report fixup and re-attaching it, which would trigger 2 reprobes of
the device from the kernel. This might require some userspace control
because we need a syscall bpf type to do so. But OTOH, this might not
be needed for some devices, given that you can pretty much design and
tailor one BPF program per device type (so you can probably handle the
"client disconnected abruptly" case from within the eBPF program
itself).
You could also handle the "steam tells eBPF to offset the report ID"
if we add one other eBPF hook in hid_hw_raw_request(). FWIW, that hook
would allow for having a HID firewall to prevent unwanted accesses to
the device.
Basically steam could call a non-declared feature on the device with a
given secret knock, and when the eBPF program sees it, it turns the
no-report mode on. (To handle the abruptly disconnected client, you
can check for the last known knock, and if it's more than 2 min for
instance, reset to the "normal" mode without report offset).
Compared to a new hidraw ioctl you get:
- immediate support on the existing 6.3 kernel
- you can change this in the future without having to wait for a new
kernel (and then supporting several kernels with different behaviors)
- you can target only tested devices (because some might need a
different offset)
- you can revise any part without bothering to change the kernel
- you will fill the dmesg with a bunch of "Unhandled reportID=0xf2"
with the current hid-playstation driver, so this needs to be fixed
there...
- [for the hid-playstation case, you might just change the report size
by adding 1 and setting that last extra value to 0 -> hid-playstation
will reject the report if it is not exactly the expected size, but
hidraw will forward it properly]
>
> Does anyone have opinions on how to proceed from here?
>
Basically, I have the strong belief that any kernel API you can
imagine can be implemented in eBPF first, and once this stabilized we
could think of making it an official one. But this will have the cost
of flexibility, so why really bother stabilizing it?
Cheers,
Benjamin
^ permalink raw reply
* Re: [PATCH 00/43] ep93xx device tree conversion
From: Arnd Bergmann @ 2023-04-24 11:31 UTC (permalink / raw)
To: Nikita Shubin
Cc: Arnd Bergmann, Linus Walleij, Alexander Sverdlin,
David S . Miller, Jonathan Neuschäfer, Russell King,
Uwe Kleine-König, Alessandro Zummo, Alexander Gordeev,
Alexandre Belloni, Andy Shevchenko, Bartosz Golaszewski,
Brian Norris, Chuanhong Guo, Conor.Dooley, Damien Le Moal,
Daniel Lezcano, Dmitry Torokhov, Emil Renner Berthing,
Eric Dumazet, Florian Fainelli, Guenter Roeck, Hartley Sweeten,
Heiko Stübner, Hitomi Hasegawa, Jakub Kicinski,
Jaroslav Kysela, Jean Delvare, Joel Stanley, Jonathan Cameron,
Krzysztof Kozlowski, Damien Le Moal, Liam Girdwood, Liang Yang,
Linus Walleij, Lukasz Majewski, Lv Ruyi, Mark Brown,
Masahiro Yamada, Michael Turquette, Miquel Raynal,
Nathan Chancellor, Nick Desaulniers, Nicolas Saenz Julienne,
Olof Johansson, Paolo Abeni, Qin Jian, Richard Weinberger,
Rob Herring, Robert Jarzmik, Russell King, Sebastian Reichel,
Sergey Shtylyov, Stephen Boyd, Sumanth Korikkar, Sven Peter,
Takashi Iwai, Thierry Reding, Thomas Gleixner, Ulf Hansson,
Vasily Gorbik, Vignesh Raghavendra, Vinod Koul, Walker Chen,
Wim Van Sebroeck, Yinbo Zhu, alsa-devel, devicetree, dmaengine,
linux-arm-kernel, linux-clk, open list:GPIO SUBSYSTEM, linux-ide,
linux-input, linux-kernel, linux-mtd, linux-pm, linux-pwm,
linux-rtc, linux-spi, linux-watchdog, Netdev, soc
In-Reply-To: <20230424123522.18302-1-nikita.shubin@maquefel.me>
On Mon, Apr 24, 2023, at 14:34, Nikita Shubin wrote:
> This series aims to convert ep93xx from platform to full device tree support.
>
> Tested on ts7250 64 RAM/128 MiB Nand flash, edb9302.
>
> Thank you Linus and Arnd for your support, review and comments, sorry
> if i missed something -
> these series are quite big for me.
>
> Big thanks to Alexander Sverdlin for his testing, support, review,
> fixes and patches.
Thanks a lot for your continued work. I can't merge any of this at
the moment since the upstream merge window just opened, but I'm
happy to take this all through the soc tree for 6.5, provided we
get the sufficient Acks from the subsystem maintainers. Merging
it through each individual tree would take a lot longer, so I
hope we can avoid that.
Arnd
^ permalink raw reply
* Re: [PATCH v3 2/5] Input: add driver for Focaltech FTS touchscreen
From: Hans de Goede @ 2023-04-24 11:02 UTC (permalink / raw)
To: Jeff LaBundy, Joel Selvaraj
Cc: Caleb Connolly, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Andy Gross, Bjorn Andersson, Konrad Dybcio, Henrik Rydberg,
Arnd Bergmann, Robert Jarzmik, Markuss Broks, Jean Delvare,
Max Krummenacher, Chris Morgan, Job Noorman, Alistair Francis,
Maxime Ripard, linux-input, devicetree, linux-kernel,
linux-arm-msm, ~postmarketos/upstreaming, phone-devel
In-Reply-To: <ZEXr1hC+Q5Bo/3Tc@nixie71>
Hi,
On 4/24/23 04:39, Jeff LaBundy wrote:
> Hi Joel,
>
> Great work so far! It's coming along nicely. Please find my latest
> feedback below.
>
> On Fri, Apr 14, 2023 at 09:02:19PM -0500, Joel Selvaraj wrote:
>> The Focaltech FTS driver supports several variants of focaltech
>> touchscreens found in ~2018 era smartphones including variants found on
>> the PocoPhone F1 and the SHIFT6mq which are already present in mainline.
>> This driver is loosely based on the original driver from Focaltech
>> but has been simplified and largely reworked.
>>
>> Co-developed-by: Caleb Connolly <caleb@connolly.tech>
>> Signed-off-by: Caleb Connolly <caleb@connolly.tech>
>> Signed-off-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>
Sorry for jumping into this thread a bit late.
I've been reading the archived discussion, but AFAICT the following question is not answered there:
Why do a new driver at all ? I have a couple of devices (Nextbook Ares 8, Nextbook Ares 8A) with focaltech FT5416 touchscreens and they both work fine with the existing drivers/input/touchscreen/edt-ft5x06.c driver.
Is there any reason we need a whole new driver for the ft5452 instead of
using (with maybe some tweaks?) the existing edt-ft5x06 driver ?
Note that despite the name the edt-ft5x06 is a generic Focaltect touchscreen driver.
Regards,
Hans
>> ---
>> MAINTAINERS | 8 +
>> drivers/input/touchscreen/Kconfig | 12 +
>> drivers/input/touchscreen/Makefile | 1 +
>> drivers/input/touchscreen/focaltech_fts5452.c | 432 ++++++++++++++++++
>> 4 files changed, 453 insertions(+)
>> create mode 100644 drivers/input/touchscreen/focaltech_fts5452.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 7ec4ce64f66d..1a3ea61e1f52 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -8028,6 +8028,14 @@ L: linux-input@vger.kernel.org
>> S: Maintained
>> F: drivers/input/joystick/fsia6b.c
>>
>> +FOCALTECH FTS5452 TOUCHSCREEN DRIVER
>> +M: Joel Selvaraj <joelselvaraj.oss@gmail.com>
>> +M: Caleb Connolly <caleb@connolly.tech>
>> +L: linux-input@vger.kernel.org
>> +S: Maintained
>> +F: Documentation/devicetree/bindings/input/touchscreen/focaltech,fts5452.yaml
>> +F: drivers/input/touchscreen/focaltech_fts5452.c
>> +
>> FOCUSRITE SCARLETT GEN 2/3 MIXER DRIVER
>> M: Geoffrey D. Bennett <g@b4.vu>
>> L: alsa-devel@alsa-project.org (moderated for non-subscribers)
>> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
>> index 1feecd7ed3cb..11af91504969 100644
>> --- a/drivers/input/touchscreen/Kconfig
>> +++ b/drivers/input/touchscreen/Kconfig
>> @@ -388,6 +388,18 @@ config TOUCHSCREEN_EXC3000
>> To compile this driver as a module, choose M here: the
>> module will be called exc3000.
>>
>> +config TOUCHSCREEN_FOCALTECH_FTS5452
>> + tristate "Focaltech FTS Touchscreen"
>> + depends on I2C
>> + help
>> + Say Y here to enable support for I2C connected Focaltech FTS
>> + based touch panels, including the 5452 and 8917 panels.
>
> This language is a bit misleading, as it seems to suggest three or more
> models are supported. It seems the title should simply be "FocalTech
> FTS5452 touchscreen controller" with the description as "...FocalTech
> FTS5452 and compatible touchscreen controllers."
>
> As more are found to be compatible (e.g. FTS8917), the compatible strings
> can simply be appended.
>
>> +
>> + If unsure, say N.
>> +
>> + To compile this driver as a module, choose M here: the
>> + module will be called focaltech_fts.
>> +
>> config TOUCHSCREEN_FUJITSU
>> tristate "Fujitsu serial touchscreen"
>> select SERIO
>> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
>> index 159cd5136fdb..47d78c9cff21 100644
>> --- a/drivers/input/touchscreen/Makefile
>> +++ b/drivers/input/touchscreen/Makefile
>> @@ -45,6 +45,7 @@ obj-$(CONFIG_TOUCHSCREEN_ELO) += elo.o
>> obj-$(CONFIG_TOUCHSCREEN_EGALAX) += egalax_ts.o
>> obj-$(CONFIG_TOUCHSCREEN_EGALAX_SERIAL) += egalax_ts_serial.o
>> obj-$(CONFIG_TOUCHSCREEN_EXC3000) += exc3000.o
>> +obj-$(CONFIG_TOUCHSCREEN_FOCALTECH_FTS5452) += focaltech_fts5452.o
>> obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
>> obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
>> obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
>> diff --git a/drivers/input/touchscreen/focaltech_fts5452.c b/drivers/input/touchscreen/focaltech_fts5452.c
>> new file mode 100644
>> index 000000000000..abf8a2f271ca
>> --- /dev/null
>> +++ b/drivers/input/touchscreen/focaltech_fts5452.c
>> @@ -0,0 +1,432 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * FocalTech touchscreen driver.
>> + *
>> + * Copyright (c) 2010-2017, FocalTech Systems, Ltd., all rights reserved.
>> + * Copyright (C) 2018 XiaoMi, Inc.
>> + * Copyright (c) 2021 Caleb Connolly <caleb@connolly.tech>
>> + * Copyright (c) 2023 Joel Selvaraj <joelselvaraj.oss@gmail.com>
>> + */
>
> Nit: inconsistent copyright capitalization.
>
>> +
>> +#include <linux/delay.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>
>> +#include <linux/mutex.h>
>> +#include <linux/regmap.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/slab.h>
>> +
>> +#define FTS_REG_CHIP_ID_H 0xA3
>> +#define FTS_REG_CHIP_ID_L 0x9F
>> +
>> +#define FTS_MAX_POINTS_SUPPORT 10
>> +#define FTS_ONE_TOUCH_LEN 6
>> +
>> +#define FTS_TOUCH_X_H_OFFSET 3
>> +#define FTS_TOUCH_X_L_OFFSET 4
>> +#define FTS_TOUCH_Y_H_OFFSET 5
>> +#define FTS_TOUCH_Y_L_OFFSET 6
>> +#define FTS_TOUCH_PRESSURE_OFFSET 7
>> +#define FTS_TOUCH_AREA_OFFSET 8
>> +#define FTS_TOUCH_TYPE_OFFSET 3
>> +#define FTS_TOUCH_ID_OFFSET 5
>> +
>> +#define FTS_TOUCH_DOWN 0
>> +#define FTS_TOUCH_UP 1
>> +#define FTS_TOUCH_CONTACT 2
>> +
>> +#define FTS_INTERVAL_READ_REG_MS 100
>> +#define FTS_TIMEOUT_READ_REG_MS 2000
>> +
>> +#define FTS_DRIVER_NAME "fts-i2c"
>> +
>> +static const u16 fts_chip_types[] = {
>> + 0x5452,
>> + 0x8719,
>> +};
>> +
>> +struct fts_ts_data {
>> + struct i2c_client *client;
>> + struct input_dev *input_dev;
>> + struct regmap *regmap;
>> + int irq;
>> + struct regulator_bulk_data regulators[2];
>> + u8 max_touch_points;
>> + u8 *point_buf;
>> + int point_buf_size;
>> + struct gpio_desc *reset_gpio;
>> + struct touchscreen_properties prop;
>> +};
>> +
>> +struct fts_i2c_chip_data {
>> + int max_touch_points;
>> +};
>
> There is no reason to wrap a single member in a struct; just define an array
> and point each driver_data member to the appropriate element.
>
> An even better solution, however, would be to merge the device ID into this.
> Then you would have a single array of structs with very clear association
> between device ID and number of points.
>
>> +
>> +int fts_check_status(struct fts_ts_data *data)
>
> This function can be static. It also seems to be inappropriately named. Here
> we are checking the device's ID, not its status.
>
>> +{
>> + int error, i = 0, count = 0;
>> + unsigned int val, id;
>> +
>> + do {
>> + error = regmap_read(data->regmap, FTS_REG_CHIP_ID_L, &id);
>> + if (error)
>> + dev_err(&data->client->dev, "I2C read failed: %d\n", error);
>
> If this read fails, there is no point in continuing further in this loop. Most
> likely the second read would fail as well but if it doesn't, you are computing
> the id using an uninitialized variable.
>
> Can you also explain, and possibly add comments, as to why the device ID must
> be checked in a retry loop? Is it because the device may be in a deep sleep and
> must be hit with I2C traffic a couple of times?
>
> If so, then you likely want to briefly sleep and then start over (i.e. continue)
> in the event of an error.
>
>> +
>> + error = regmap_read(data->regmap, FTS_REG_CHIP_ID_H, &val);
>> + if (error)
>> + dev_err(&data->client->dev, "I2C read failed: %d\n", error);
>
> Same problem here.
>
>> +
>> + id |= val << 8;
>> +
>> + for (i = 0; i < ARRAY_SIZE(fts_chip_types); i++)
>> + if (id == fts_chip_types[i])
>> + return 0;
>
> This retry loop in general seems a bit non-optimal. If for example the driver
> is simply communicating with an incompatible device, there is no need to go
> through all N loops.
>
>> +
>> + count++;
>> + msleep(FTS_INTERVAL_READ_REG_MS);
>> + } while ((count * FTS_INTERVAL_READ_REG_MS) < FTS_TIMEOUT_READ_REG_MS);
>
> This multiplication seems unnecessarily complicated; can we not simply have
> FTS_MAX_RETRIES or similar?
>
>> +
>> + return -EIO;
>> +}
>> +
>> +static int fts_report_touch(struct fts_ts_data *data)
>> +{
>> + struct input_dev *input_dev = data->input_dev;
>> + int base;
>> + unsigned int x, y, z, maj;
>> + u8 slot, type;
>> + int error, i = 0;
>> +
>> + u8 *buf = data->point_buf;
>> +
>> + memset(buf, 0, data->point_buf_size);
>> +
>> + error = regmap_bulk_read(data->regmap, 0, buf, data->point_buf_size);
>> + if (error) {
>> + dev_err(&data->client->dev, "I2C read failed: %d\n", error);
>> + return error;
>> + }
>> +
>> + for (i = 0; i < data->max_touch_points; i++) {
>> + base = FTS_ONE_TOUCH_LEN * i;
>> +
>> + slot = buf[base + FTS_TOUCH_ID_OFFSET] >> 4;
>> + if (slot >= data->max_touch_points)
>> + break;
>> +
>> + x = ((buf[base + FTS_TOUCH_X_H_OFFSET] & 0x0F) << 8) +
>> + (buf[base + FTS_TOUCH_X_L_OFFSET] & 0xFF);
>> + y = ((buf[base + FTS_TOUCH_Y_H_OFFSET] & 0x0F) << 8) +
>> + (buf[base + FTS_TOUCH_Y_L_OFFSET] & 0xFF);
>
> Sorry, I did not quite follow the image that was shared in an earlier thread.
> It is unclear to me why we cannot represent the interrupt status registers
> as an array of __be16 values and then do something like the following:
>
> x = be16_to_cpu(buf[FTS_TOUCH_X_OFFSET]) & GENMASK(11, 0);
>
> I would be surprised if the mask is even necessary; you would need to refer
> to a datasheet however. Perhaps the vendor would be willing to give one to
> you if it means they get an upstream driver?
>
>> +
>> + z = buf[base + FTS_TOUCH_PRESSURE_OFFSET];
>> + if (z == 0)
>> + z = 0x3f;
>> +
>> + maj = buf[base + FTS_TOUCH_AREA_OFFSET] >> 4;
>> + if (maj == 0)
>> + maj = 0x09;
>
> I think we need some comments and possibly some #defines to explain what is
> happening here.
>
>> +
>> + type = buf[base + FTS_TOUCH_TYPE_OFFSET] >> 6;
>> +
>> + input_mt_slot(input_dev, slot);
>> + if (type == FTS_TOUCH_DOWN || type == FTS_TOUCH_CONTACT) {
>> + input_mt_report_slot_state(input_dev, MT_TOOL_FINGER, true);
>> + touchscreen_report_pos(data->input_dev, &data->prop, x, y, true);
>> + input_report_abs(input_dev, ABS_MT_PRESSURE, z);
>> + input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR, maj);
>> + input_report_key(data->input_dev, BTN_TOUCH, 1);
>> + } else {
>> + input_report_key(data->input_dev, BTN_TOUCH, 0);
>> + input_mt_report_slot_inactive(input_dev);
>> + }
>> + }
>> + input_mt_sync_frame(input_dev);
>> + input_sync(input_dev);
>> +
>> + return 0;
>> +}
>> +
>> +static irqreturn_t fts_ts_interrupt(int irq, void *dev_id)
>> +{
>> + struct fts_ts_data *data = dev_id;
>> +
>> + return fts_report_touch(data) ? IRQ_NONE : IRQ_HANDLED;
>> +}
>> +
>> +static void fts_power_off(void *d)
>> +{
>> + struct fts_ts_data *data = d;
>> +
>> + regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
>> +}
>> +
>> +static int fts_start(struct fts_ts_data *data)
>> +{
>> + int error;
>> +
>> + error = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
>> + data->regulators);
>> + if (error) {
>> + dev_err(&data->client->dev, "failed to enable regulators\n");
>> + return error;
>> + }
>> +
>> + gpiod_set_value_cansleep(data->reset_gpio, 0);
>> + msleep(200);
>
> Same here with respect to comments; what happens during these first 200 ms after
> reset is released? Does the interrupt pin toggle several times? 200 ms is also
> quite a while to wait each time the input handler opens the device; is it really
> necessary?
>
>> +
>> + enable_irq(data->irq);
>> +
>> + return 0;
>> +}
>> +
>> +static int fts_stop(struct fts_ts_data *data)
>> +{
>> + disable_irq(data->irq);
>> + gpiod_set_value_cansleep(data->reset_gpio, 1);
>> + fts_power_off(data);
>> +
>> + return 0;
>> +}
>> +
>> +static int fts_input_open(struct input_dev *dev)
>> +{
>> + struct fts_ts_data *data = input_get_drvdata(dev);
>> + int error;
>> +
>> + error = fts_start(data);
>> + if (error)
>> + return error;
>> +
>> + error = fts_check_status(data);
>> + if (error) {
>> + dev_err(&data->client->dev, "Failed to start or unsupported chip");
>> + return error;
>> + }
>
> It seems unnecessary and wasteful to check the device ID every time the input
> handler opens the device. We also don't want to go through all the trouble of
> registering the device, only to find out later it wasn't even the right part.
>
> Instead, you should power up the device during probe, validate its ID and then
> power it back down.
>
>> +
>> + return 0;
>> +}
>> +
>> +static void fts_input_close(struct input_dev *dev)
>> +{
>> + struct fts_ts_data *data = input_get_drvdata(dev);
>> +
>> + fts_stop(data);
>> +}
>> +
>> +static int fts_input_init(struct fts_ts_data *data)
>> +{
>> + struct device *dev = &data->client->dev;
>> + struct input_dev *input_dev;
>> + int error = 0;
>
> No need to initialize this, only for it to get overwritten later.
>
>> +
>> + input_dev = devm_input_allocate_device(dev);
>> + if (!input_dev)
>> + return -ENOMEM;
>> +
>> + data->input_dev = input_dev;
>> +
>> + input_dev->name = FTS_DRIVER_NAME;
>> + input_dev->id.bustype = BUS_I2C;
>> + input_dev->dev.parent = dev;
>> + input_dev->open = fts_input_open;
>> + input_dev->close = fts_input_close;
>> + input_set_drvdata(input_dev, data);
>> +
>> + input_set_capability(input_dev, EV_ABS, ABS_MT_POSITION_X);
>> + input_set_capability(input_dev, EV_ABS, ABS_MT_POSITION_Y);
>> + input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
>> + input_set_abs_params(input_dev, ABS_MT_PRESSURE, 0, 255, 0, 0);
>> +
>> + touchscreen_parse_properties(input_dev, true, &data->prop);
>> + if (!data->prop.max_x || !data->prop.max_y) {
>> + dev_err(dev,
>> + "touchscreen-size-x and/or touchscreen-size-y not set in device properties\n");
>
> "Device properties" is vague; one could interpret it to mean the controller's
> embedded FW. Just cut the message off at "...not set".
>
>> + return -EINVAL;
>> + }
>> +
>> + error = input_mt_init_slots(input_dev, data->max_touch_points,
>> + INPUT_MT_DIRECT);
>> + if (error)
>> + return error;
>> +
>> + data->point_buf_size = (data->max_touch_points * FTS_ONE_TOUCH_LEN) + 3;
>> + data->point_buf = devm_kzalloc(dev, data->point_buf_size, GFP_KERNEL);
>> + if (!data->point_buf)
>> + return -ENOMEM;
>> +
>> + error = input_register_device(input_dev);
>> + if (error) {
>> + dev_err(dev, "Failed to register input device\n");
>> + return error;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static const struct regmap_config fts_ts_i2c_regmap_config = {
>> + .reg_bits = 8,
>> + .val_bits = 8,
>> +};
>> +
>> +static int fts_ts_probe(struct i2c_client *client)
>> +{
>> + const struct i2c_device_id *id = i2c_client_get_device_id(client);
>> + const struct fts_i2c_chip_data *chip_data;
>> + struct fts_ts_data *data;
>> + int error = 0;
>> +
>> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
>> + dev_err(&client->dev, "I2C not supported");
>> + return -ENODEV;
>> + }
>> +
>> + if (!client->irq) {
>> + dev_err(&client->dev, "No irq specified\n");
>> + return -EINVAL;
>> + }
>> +
>> + data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
>> + if (!data)
>> + return -ENOMEM;
>> +
>> + chip_data = device_get_match_data(&client->dev);
>> + if (!chip_data)
>> + chip_data = (const struct fts_i2c_chip_data *)id->driver_data;
>> + if (!chip_data || !chip_data->max_touch_points) {
>> + dev_err(&client->dev, "invalid or missing chip data\n");
>> + return -EINVAL;
>> + }
>> + if (chip_data->max_touch_points > FTS_MAX_POINTS_SUPPORT) {
>> + dev_err(&client->dev,
>> + "invalid chip data, max_touch_points should be less than or equal to %d\n",
>> + FTS_MAX_POINTS_SUPPORT);
>> + return -EINVAL;
>> + }
>
> This check is not necessary; if someone adds an invalid max_touch_points, then the
> driver was updated incorrectly. There is no need to check it at every runtime.
>
>> + data->max_touch_points = chip_data->max_touch_points;
>> +
>> + data->client = client;
>> + i2c_set_clientdata(client, data);
>> +
>> + data->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
>> + if (IS_ERR(data->reset_gpio)) {
>> + error = PTR_ERR(data->reset_gpio);
>> + dev_err(&client->dev, "Failed to request reset gpio, error %d\n", error);
>> + return error;
>> + }
>> +
>> + data->regmap = devm_regmap_init_i2c(client, &fts_ts_i2c_regmap_config);
>> + if (IS_ERR(data->regmap)) {
>> + error = PTR_ERR(data->regmap);
>> + dev_err(&client->dev, "regmap allocation failed, error %d\n", error);
>> + return error;
>> + }
>> +
>> + /*
>> + * AVDD is the analog voltage supply (2.6V to 3.3V)
>> + * VDDIO is the digital voltage supply (1.8V)
>> + */
>> + data->regulators[0].supply = "avdd";
>> + data->regulators[1].supply = "vddio";
>> + error = devm_regulator_bulk_get(&client->dev, ARRAY_SIZE(data->regulators),
>> + data->regulators);
>> + if (error) {
>> + dev_err(&client->dev, "Failed to get regulators %d\n", error);
>> + return error;
>> + }
>> +
>> + error = devm_add_action_or_reset(&client->dev, fts_power_off, data);
>> + if (error) {
>> + dev_err(&client->dev, "failed to install power off handler\n");
>> + return error;
>> + }
>
> Christophe makes a great point. If this or any other call throughout the rest of
> probe as you have written it fails, you will try to disable a disabled regulator.
>
> The same will happen when the driver is torn down, as the input handler should
> have already powered down the device by way of the close callback. Did you build
> this driver as a module and test removal? I suspect you will get a stack trace.
>
> I think the call needs to go away altogether.
>
>> +
>> + error = devm_request_threaded_irq(&client->dev, client->irq, NULL,
>> + fts_ts_interrupt, IRQF_ONESHOT,
>> + client->name, data);
>> + if (error) {
>> + dev_err(&client->dev, "Failed to request IRQ: %d\n", error);
>> + return error;
>> + }
>> +
>> + error = fts_input_init(data);
>> + if (error)
>> + return error;
>> +
>> + return 0;
>
> This is idiomatic, but I find "return fts_input_init(data);" to be simpler.
>
>> +}
>> +
>> +static int fts_pm_suspend(struct device *dev)
>> +{
>> + struct fts_ts_data *data = dev_get_drvdata(dev);
>> +
>> + mutex_lock(&data->input_dev->mutex);
>> +
>> + if (input_device_enabled(data->input_dev))
>> + fts_stop(data);
>> +
>> + mutex_unlock(&data->input_dev->mutex);
>> +
>> + return 0;
>> +}
>> +
>> +static int fts_pm_resume(struct device *dev)
>> +{
>> + struct fts_ts_data *data = dev_get_drvdata(dev);
>> + int error = 0;
>
> Same here, there is no point in initializating this.
>
>> +
>> + mutex_lock(&data->input_dev->mutex);
>> +
>> + if (input_device_enabled(data->input_dev))
>> + error = fts_start(data);
>> +
>> + mutex_unlock(&data->input_dev->mutex);
>> +
>> + return error;
>> +}
>> +
>> +static DEFINE_SIMPLE_DEV_PM_OPS(fts_dev_pm_ops, fts_pm_suspend, fts_pm_resume);
>> +
>> +static const struct fts_i2c_chip_data fts5452_chip_data = {
>> + .max_touch_points = 5,
>> +};
>> +
>> +static const struct fts_i2c_chip_data fts8719_chip_data = {
>> + .max_touch_points = 10,
>> +};
>> +
>> +static const struct i2c_device_id fts_i2c_id[] = {
>> + { .name = "fts5452", .driver_data = (long)&fts5452_chip_data },
>> + { .name = "fts8719", .driver_data = (long)&fts8719_chip_data },
>> + { /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, fts_i2c_id);
>> +
>> +static const struct of_device_id fts_of_match[] = {
>> + { .compatible = "focaltech,fts5452", .data = &fts5452_chip_data },
>> + { .compatible = "focaltech,fts8719", .data = &fts8719_chip_data },
>> + { /* sentinel */ }
>> +};
>> +
>> +MODULE_DEVICE_TABLE(of, fts_of_match);
>> +
>> +static struct i2c_driver fts_ts_driver = {
>> + .probe_new = fts_ts_probe,
>> + .id_table = fts_i2c_id,
>> + .driver = {
>> + .name = FTS_DRIVER_NAME,
>> + .pm = pm_sleep_ptr(&fts_dev_pm_ops),
>> + .of_match_table = fts_of_match,
>> + },
>> +};
>> +module_i2c_driver(fts_ts_driver);
>> +
>> +MODULE_AUTHOR("Joel Selvaraj <joelselvaraj.oss@gmail.com>");
>> +MODULE_AUTHOR("Caleb Connolly <caleb@connolly.tech>");
>> +MODULE_DESCRIPTION("Focaltech Touchscreen Driver");
>
> Nit: mixing 'FocalTech' and 'Focaltech' throughout.
>
>> +MODULE_LICENSE("GPL");
>> --
>> 2.40.0
>>
>
> Kind regards,
> Jeff LaBundy
>
^ permalink raw reply
* [PATCH 27/43] input: keypad: ep93xx: add DT support for Cirrus EP93xx
From: Nikita Shubin @ 2023-04-24 12:34 UTC (permalink / raw)
Cc: Arnd Bergmann, Linus Walleij, Alexander Sverdlin, Dmitry Torokhov,
Jonathan Cameron, Andy Shevchenko, Lv Ruyi, linux-input,
linux-kernel
In-Reply-To: <20230424123522.18302-1-nikita.shubin@maquefel.me>
- get keymap from the device tree
- find register range from the device tree
- get interrupts from device tree
Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
---
Notes:
Arnd Bergmann:
- wildcards ep93xx to something meaningful, i.e. ep9301
- drop wrappers
drivers/input/keyboard/ep93xx_keypad.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/input/keyboard/ep93xx_keypad.c b/drivers/input/keyboard/ep93xx_keypad.c
index 55075addcac2..bf77754fa4c7 100644
--- a/drivers/input/keyboard/ep93xx_keypad.c
+++ b/drivers/input/keyboard/ep93xx_keypad.c
@@ -20,6 +20,8 @@
#include <linux/bits.h>
#include <linux/module.h>
#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
#include <linux/interrupt.h>
#include <linux/clk.h>
#include <linux/io.h>
@@ -315,10 +317,17 @@ static int ep93xx_keypad_remove(struct platform_device *pdev)
return 0;
}
+static const struct of_device_id ep93xx_keypad_of_ids[] = {
+ { .compatible = "cirrus,ep9301-keypad" },
+ {},
+};
+MODULE_DEVICE_TABLE(of, ep93xx_keypad_of_ids);
+
static struct platform_driver ep93xx_keypad_driver = {
.driver = {
.name = "ep93xx-keypad",
.pm = pm_sleep_ptr(&ep93xx_keypad_pm_ops),
+ .of_match_table = ep93xx_keypad_of_ids,
},
.probe = ep93xx_keypad_probe,
.remove = ep93xx_keypad_remove,
--
2.39.2
^ permalink raw reply related
* [PATCH 26/43] dt-bindings: input: Add DT bindings ep93xx keypad
From: Nikita Shubin @ 2023-04-24 12:34 UTC (permalink / raw)
Cc: Arnd Bergmann, Linus Walleij, Alexander Sverdlin, Dmitry Torokhov,
Rob Herring, Krzysztof Kozlowski, linux-input, devicetree,
linux-kernel
In-Reply-To: <20230424123522.18302-1-nikita.shubin@maquefel.me>
Add YAML bindings ep93xx SoC.
Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
---
Notes:
Linus Walleij:
- replaced hex with proper <KEY_UP>, etc
.../bindings/input/cirrus,ep93xx-keypad.yaml | 123 ++++++++++++++++++
1 file changed, 123 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/cirrus,ep93xx-keypad.yaml
diff --git a/Documentation/devicetree/bindings/input/cirrus,ep93xx-keypad.yaml b/Documentation/devicetree/bindings/input/cirrus,ep93xx-keypad.yaml
new file mode 100644
index 000000000000..0310114de22e
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/cirrus,ep93xx-keypad.yaml
@@ -0,0 +1,123 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/cirrus,ep93xx-keypad.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Cirrus ep93xx keypad
+
+maintainers:
+ - Dmitry Torokhov <dmitry.torokhov@gmail.com>
+ - Alexander Sverdlin <alexander.sverdlin@gmail.com>
+
+allOf:
+ - $ref: "/schemas/input/matrix-keymap.yaml#"
+
+description: |
+ The KPP is designed to interface with a keypad matrix with 2-point contact
+ or 3-point contact keys. The KPP is designed to simplify the software task
+ of scanning a keypad matrix. The KPP is capable of detecting, debouncing,
+ and decoding one or multiple keys pressed simultaneously on a keypad.
+
+properties:
+ compatible:
+ enum:
+ - cirrus,ep9301-keypad
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ clocks:
+ maxItems: 1
+
+ clock-names:
+ enum:
+ - ep93xx-keypad
+
+ debounce:
+ description: |
+ Time in microseconds that key must be pressed or
+ released for state change interrupt to trigger.
+ $ref: /schemas/types.yaml#/definitions/uint32
+
+ prescale:
+ description: row/column counter pre-scaler load value
+ $ref: /schemas/types.yaml#/definitions/uint32
+
+ clk-rate:
+ description: clock rate setting
+ $ref: /schemas/types.yaml#/definitions/uint32
+
+ disable-3-key:
+ type: boolean
+ description:
+ Disable 3 Key reset. Setting this disables the three key reset
+ output to the watchdog reset block.
+
+ diag-mode:
+ type: boolean
+ description:
+ Key scan diagnostic mode. Setting this allows key scanning to be
+ directly controlled through the key register by writes from the
+ ARM Core.
+
+ back-drive:
+ type: boolean
+ description:
+ Key scan back driving enable. Setting this enables the key
+ scanning logic to back drive the row and column pins of the
+ chip high during the first two column counts in the
+ row/column counter.
+
+ test-mode:
+ type: boolean
+ description:
+ Test mode. When this is set, the counter RC_COUNT is advanced
+ by 8 counts when EN is active. The effect is that only column 0
+ is checked in each row. This test mode allows a faster test
+ of the ROW pins.
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - clocks
+ - clock-names
+ - linux,keymap
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/cirrus,ep93xx-clock.h>
+ #include <dt-bindings/input/input.h>
+ keypad@800f0000 {
+ compatible = "cirrus,ep9301-keypad";
+ reg = <0x800f0000 0x0c>;
+ interrupt-parent = <&vic0>;
+ interrupts = <29>;
+ clocks = <&syscon EP93XX_CLK_KEYPAD>;
+ clock-names = "ep93xx-keypad";
+ pinctrl-names = "default";
+ pinctrl-0 = <&keypad_default_pins>;
+ linux,keymap = <KEY_UP>,
+ <KEY_DOWN>,
+ <KEY_VOLUMEDOWN>,
+ <KEY_HOME>,
+ <KEY_RIGHT>,
+ <KEY_LEFT>,
+ <KEY_ENTER>,
+ <KEY_VOLUMEUP>,
+ <KEY_F6>,
+ <KEY_F8>,
+ <KEY_F9>,
+ <KEY_F10>,
+ <KEY_F1>,
+ <KEY_F2>,
+ <KEY_F3>,
+ <KEY_POWER>;
+ };
+
--
2.39.2
^ permalink raw reply related
* [PATCH 37/43] input: keypad: ep93xx: drop legacy pinctrl
From: Nikita Shubin @ 2023-04-24 12:34 UTC (permalink / raw)
Cc: Arnd Bergmann, Linus Walleij, Alexander Sverdlin, Dmitry Torokhov,
Andy Shevchenko, Jonathan Cameron, Lv Ruyi, linux-input,
linux-kernel
In-Reply-To: <20230424123522.18302-1-nikita.shubin@maquefel.me>
Drop legacy acquire/release since we are using
pinctrl for this now.
Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
---
drivers/input/keyboard/ep93xx_keypad.c | 16 ----------------
1 file changed, 16 deletions(-)
diff --git a/drivers/input/keyboard/ep93xx_keypad.c b/drivers/input/keyboard/ep93xx_keypad.c
index bf77754fa4c7..a5f5d7d453e7 100644
--- a/drivers/input/keyboard/ep93xx_keypad.c
+++ b/drivers/input/keyboard/ep93xx_keypad.c
@@ -222,13 +222,6 @@ static int ep93xx_keypad_resume(struct device *dev)
static DEFINE_SIMPLE_DEV_PM_OPS(ep93xx_keypad_pm_ops,
ep93xx_keypad_suspend, ep93xx_keypad_resume);
-static void ep93xx_keypad_release_gpio_action(void *_pdev)
-{
- struct platform_device *pdev = _pdev;
-
- ep93xx_keypad_release_gpio(pdev);
-}
-
static int ep93xx_keypad_probe(struct platform_device *pdev)
{
struct ep93xx_keypad *keypad;
@@ -256,15 +249,6 @@ static int ep93xx_keypad_probe(struct platform_device *pdev)
if (IS_ERR(keypad->mmio_base))
return PTR_ERR(keypad->mmio_base);
- err = ep93xx_keypad_acquire_gpio(pdev);
- if (err)
- return err;
-
- err = devm_add_action_or_reset(&pdev->dev,
- ep93xx_keypad_release_gpio_action, pdev);
- if (err)
- return err;
-
keypad->clk = devm_clk_get(&pdev->dev, NULL);
if (IS_ERR(keypad->clk))
return PTR_ERR(keypad->clk);
--
2.39.2
^ permalink raw reply related
* [PATCH 00/43] ep93xx device tree conversion
From: Nikita Shubin @ 2023-04-24 12:34 UTC (permalink / raw)
Cc: Arnd Bergmann, Linus Walleij, Alexander Sverdlin, David S. Miller,
Jonathan Neuschäfer, Russell King (Oracle),
Uwe Kleine-König, Alessandro Zummo, Alexander Gordeev,
Alexandre Belloni, Andy Shevchenko, Arnd Bergmann,
Bartosz Golaszewski, Brian Norris, Chuanhong Guo, Conor Dooley,
Damien Le Moal, Daniel Lezcano, Dmitry Torokhov,
Emil Renner Berthing, Eric Dumazet, Florian Fainelli,
Guenter Roeck, Hartley Sweeten, Heiko Stuebner, Hitomi Hasegawa,
Jakub Kicinski, Jaroslav Kysela, Jean Delvare, Joel Stanley,
Jonathan Cameron, Krzysztof Kozlowski, Le Moal, Liam Girdwood,
Liang Yang, Linus Walleij, Lukasz Majewski, Lv Ruyi, Mark Brown,
Masahiro Yamada, Michael Turquette, Miquel Raynal,
Nathan Chancellor, Nick Desaulniers, Nicolas Saenz Julienne,
Olof Johansson, Paolo Abeni, Qin Jian, Richard Weinberger,
Rob Herring, Robert Jarzmik, Russell King, Sebastian Reichel,
Sergey Shtylyov, Stephen Boyd, Sumanth Korikkar, Sven Peter,
Takashi Iwai, Thierry Reding, Thomas Gleixner, Ulf Hansson,
Vasily Gorbik, Vignesh Raghavendra, Vinod Koul, Walker Chen,
Wim Van Sebroeck, Yinbo Zhu, alsa-devel, devicetree, dmaengine,
linux-arm-kernel, linux-clk, linux-gpio, linux-ide, linux-input,
linux-kernel, linux-mtd, linux-pm, linux-pwm, linux-rtc,
linux-spi, linux-watchdog, netdev, soc
This series aims to convert ep93xx from platform to full device tree support.
Tested on ts7250 64 RAM/128 MiB Nand flash, edb9302.
Thank you Linus and Arnd for your support, review and comments, sorry if i missed something -
these series are quite big for me.
Big thanks to Alexander Sverdlin for his testing, support, review, fixes and patches.
Alexander Sverdlin (4):
ARM: dts: ep93xx: Add ADC node
ARM: dts: ep93xx: Add I2S and AC97 nodes
ARM: dts: ep93xx: Add EDB9302 DT
ASoC: cirrus: edb93xx: Delete driver
Nikita Shubin (39):
gpio: ep93xx: split device in multiple
soc: Add SoC driver for Cirrus ep93xx
dt-bindings: pinctrl: Add DT bindings ep93xx pinctrl
pinctrl: add a Cirrus ep93xx SoC pin controller
dt-bindings: timers: add DT bindings for Cirrus EP93xx
clocksource: ep93xx: Add driver for Cirrus Logic EP93xx
dt-bindings: rtc: add DT bindings for Cirrus EP93xx
rtc: ep93xx: add DT support for Cirrus EP93xx
dt-bindings: watchdog: add DT bindings for Cirrus EP93x
watchdog: ep93xx: add DT support for Cirrus EP93xx
dt-bindings: clock: add DT bindings for Cirrus EP93xx
clk: ep93xx: add DT support for Cirrus EP93xx
power: reset: Add a driver for the ep93xx reset
dt-bindings: pwm: Add DT bindings ep93xx PWM
pwm: ep93xx: add DT support for Cirrus EP93xx
dt-bindings: spi: Add DT bindings ep93xx spi
spi: ep93xx: add DT support for Cirrus EP93xx
dt-bindings: net: Add DT bindings ep93xx eth
net: cirrus: add DT support for Cirrus EP93xx
dt-bindings: dma: Add DT bindings ep93xx dma
dma: cirrus: add DT support for Cirrus EP93xx
dt-bindings: mtd: add DT bindings for ts7250 nand
mtd: ts72xx_nand: add platform helper
dt-bindings: ata: Add DT bindings ep93xx pata
pata: cirrus: add DT support for Cirrus EP93xx
dt-bindings: input: Add DT bindings ep93xx keypad
input: keypad: ep93xx: add DT support for Cirrus EP93xx
dt-bindings: rtc: Add DT binding m48t86 rtc
rtc: m48t86: add DT support for m48t86
dt-bindings: wdt: Add DT binding ts72xx wdt
wdt: ts72xx: add DT support for ts72xx
dt-bindings: gpio: Add DT bindings ep93xx gpio
gpio: ep93xx: add DT support for gpio-ep93xx
ARM: dts: add device tree for ep93xx Soc
ARM: ep93xx: DT for the Cirrus ep93xx SoC platforms
pwm: ep93xx: drop legacy pinctrl
input: keypad: ep93xx: drop legacy pinctrl
ARM: ep93xx: soc: drop defines
ARM: ep93xx: delete all boardfiles
.../devicetree/bindings/arm/ep93xx.yaml | 99 +
.../bindings/ata/cirrus,ep93xx-pata.yaml | 40 +
.../bindings/dma/cirrus,ep93xx-dma-m2m.yaml | 66 +
.../bindings/dma/cirrus,ep93xx-dma-m2p.yaml | 102 +
.../devicetree/bindings/gpio/gpio-ep93xx.yaml | 161 ++
.../bindings/input/cirrus,ep93xx-keypad.yaml | 123 ++
.../bindings/mtd/technologic,nand.yaml | 56 +
.../bindings/net/cirrus,ep93xx_eth.yaml | 51 +
.../pinctrl/cirrus,ep93xx-pinctrl.yaml | 66 +
.../bindings/pwm/cirrus,ep93xx-pwm.yaml | 45 +
.../bindings/rtc/cirrus,ep93xx-rtc.yaml | 32 +
.../bindings/rtc/dallas,rtc-m48t86.yaml | 33 +
.../devicetree/bindings/spi/spi-ep93xx.yaml | 68 +
.../bindings/timer/cirrus,ep93xx-timer.yaml | 41 +
.../bindings/watchdog/cirrus,ep93xx-wdt.yaml | 38 +
.../watchdog/technologic,ts72xx-wdt.yaml | 39 +
arch/arm/Makefile | 1 -
arch/arm/boot/dts/Makefile | 1 +
arch/arm/boot/dts/ep93xx-bk3.dts | 96 +
arch/arm/boot/dts/ep93xx-edb9302.dts | 150 ++
arch/arm/boot/dts/ep93xx-ts7250.dts | 113 ++
arch/arm/boot/dts/ep93xx.dtsi | 466 +++++
arch/arm/mach-ep93xx/Kconfig | 20 +-
arch/arm/mach-ep93xx/Makefile | 11 -
arch/arm/mach-ep93xx/core.c | 1017 ----------
arch/arm/mach-ep93xx/dma.c | 114 --
arch/arm/mach-ep93xx/edb93xx.c | 344 ----
arch/arm/mach-ep93xx/ep93xx-regs.h | 38 -
arch/arm/mach-ep93xx/gpio-ep93xx.h | 111 --
arch/arm/mach-ep93xx/hardware.h | 25 -
arch/arm/mach-ep93xx/irqs.h | 76 -
arch/arm/mach-ep93xx/platform.h | 42 -
arch/arm/mach-ep93xx/soc.h | 212 --
arch/arm/mach-ep93xx/ts72xx.c | 422 ----
arch/arm/mach-ep93xx/ts72xx.h | 94 -
arch/arm/mach-ep93xx/vision_ep9307.c | 311 ---
drivers/ata/pata_ep93xx.c | 9 +
drivers/clk/Kconfig | 8 +
drivers/clk/Makefile | 1 +
.../clock.c => drivers/clk/clk-ep93xx.c | 491 +++--
drivers/clocksource/Kconfig | 11 +
drivers/clocksource/Makefile | 1 +
.../clocksource}/timer-ep93xx.c | 143 +-
drivers/dma/ep93xx_dma.c | 119 +-
drivers/gpio/gpio-ep93xx.c | 329 ++--
drivers/input/keyboard/ep93xx_keypad.c | 25 +-
drivers/mtd/nand/raw/Kconfig | 8 +
drivers/mtd/nand/raw/Makefile | 1 +
drivers/mtd/nand/raw/ts72xx_nand.c | 94 +
drivers/net/ethernet/cirrus/ep93xx_eth.c | 49 +-
drivers/pinctrl/Kconfig | 7 +
drivers/pinctrl/Makefile | 1 +
drivers/pinctrl/pinctrl-ep93xx.c | 1698 +++++++++++++++++
drivers/power/reset/Kconfig | 10 +
drivers/power/reset/Makefile | 1 +
drivers/power/reset/ep93xx-restart.c | 65 +
drivers/pwm/pwm-ep93xx.c | 24 +-
drivers/rtc/rtc-ep93xx.c | 8 +
drivers/rtc/rtc-m48t86.c | 10 +
drivers/soc/Kconfig | 1 +
drivers/soc/Makefile | 1 +
drivers/soc/cirrus/Kconfig | 11 +
drivers/soc/cirrus/Makefile | 2 +
drivers/soc/cirrus/soc-ep93xx.c | 134 ++
drivers/spi/spi-ep93xx.c | 31 +-
drivers/watchdog/ep93xx_wdt.c | 8 +
drivers/watchdog/ts72xx_wdt.c | 8 +
.../dt-bindings/clock/cirrus,ep93xx-clock.h | 53 +
include/linux/platform_data/dma-ep93xx.h | 3 +
include/linux/soc/cirrus/ep93xx.h | 28 +-
sound/soc/cirrus/Kconfig | 9 -
sound/soc/cirrus/Makefile | 4 -
sound/soc/cirrus/edb93xx.c | 119 --
73 files changed, 4796 insertions(+), 3453 deletions(-)
create mode 100644 Documentation/devicetree/bindings/arm/ep93xx.yaml
create mode 100644 Documentation/devicetree/bindings/ata/cirrus,ep93xx-pata.yaml
create mode 100644 Documentation/devicetree/bindings/dma/cirrus,ep93xx-dma-m2m.yaml
create mode 100644 Documentation/devicetree/bindings/dma/cirrus,ep93xx-dma-m2p.yaml
create mode 100644 Documentation/devicetree/bindings/gpio/gpio-ep93xx.yaml
create mode 100644 Documentation/devicetree/bindings/input/cirrus,ep93xx-keypad.yaml
create mode 100644 Documentation/devicetree/bindings/mtd/technologic,nand.yaml
create mode 100644 Documentation/devicetree/bindings/net/cirrus,ep93xx_eth.yaml
create mode 100644 Documentation/devicetree/bindings/pinctrl/cirrus,ep93xx-pinctrl.yaml
create mode 100644 Documentation/devicetree/bindings/pwm/cirrus,ep93xx-pwm.yaml
create mode 100644 Documentation/devicetree/bindings/rtc/cirrus,ep93xx-rtc.yaml
create mode 100644 Documentation/devicetree/bindings/rtc/dallas,rtc-m48t86.yaml
create mode 100644 Documentation/devicetree/bindings/spi/spi-ep93xx.yaml
create mode 100644 Documentation/devicetree/bindings/timer/cirrus,ep93xx-timer.yaml
create mode 100644 Documentation/devicetree/bindings/watchdog/cirrus,ep93xx-wdt.yaml
create mode 100644 Documentation/devicetree/bindings/watchdog/technologic,ts72xx-wdt.yaml
create mode 100644 arch/arm/boot/dts/ep93xx-bk3.dts
create mode 100644 arch/arm/boot/dts/ep93xx-edb9302.dts
create mode 100644 arch/arm/boot/dts/ep93xx-ts7250.dts
create mode 100644 arch/arm/boot/dts/ep93xx.dtsi
delete mode 100644 arch/arm/mach-ep93xx/Makefile
delete mode 100644 arch/arm/mach-ep93xx/core.c
delete mode 100644 arch/arm/mach-ep93xx/dma.c
delete mode 100644 arch/arm/mach-ep93xx/edb93xx.c
delete mode 100644 arch/arm/mach-ep93xx/ep93xx-regs.h
delete mode 100644 arch/arm/mach-ep93xx/gpio-ep93xx.h
delete mode 100644 arch/arm/mach-ep93xx/hardware.h
delete mode 100644 arch/arm/mach-ep93xx/irqs.h
delete mode 100644 arch/arm/mach-ep93xx/platform.h
delete mode 100644 arch/arm/mach-ep93xx/soc.h
delete mode 100644 arch/arm/mach-ep93xx/ts72xx.c
delete mode 100644 arch/arm/mach-ep93xx/ts72xx.h
delete mode 100644 arch/arm/mach-ep93xx/vision_ep9307.c
rename arch/arm/mach-ep93xx/clock.c => drivers/clk/clk-ep93xx.c (60%)
rename {arch/arm/mach-ep93xx => drivers/clocksource}/timer-ep93xx.c (51%)
create mode 100644 drivers/mtd/nand/raw/ts72xx_nand.c
create mode 100644 drivers/pinctrl/pinctrl-ep93xx.c
create mode 100644 drivers/power/reset/ep93xx-restart.c
create mode 100644 drivers/soc/cirrus/Kconfig
create mode 100644 drivers/soc/cirrus/Makefile
create mode 100644 drivers/soc/cirrus/soc-ep93xx.c
create mode 100644 include/dt-bindings/clock/cirrus,ep93xx-clock.h
delete mode 100644 sound/soc/cirrus/edb93xx.c
--
2.39.2
^ permalink raw reply
* Re: [PATCH] HID: amd_sfh: Support for additional light sensor
From: Linux regression tracking (Thorsten Leemhuis) @ 2023-04-24 8:32 UTC (permalink / raw)
To: Jiri Kosina, Basavaraj Natikar
Cc: benjamin.tissoires, linux-input, Linux kernel regressions list
In-Reply-To: <nycvar.YFH.7.76.2304131557420.29760@cbobk.fhfr.pm>
On 13.04.23 15:57, Jiri Kosina wrote:
> On Tue, 11 Apr 2023, Basavaraj Natikar wrote:
>
>> There is support for additional light sensors in the SFH firmware.
>> As a result, add support for additional light sensors.
>>
>> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
>
> Applied.
FWIW, there is a report about a hang at boot that was bisected to this
patch. For details see:
https://bugzilla.kernel.org/show_bug.cgi?id=217354
Ciao, Thorsten
P.S. I normally don't track regressions in -next, I just noticed that
report and wanted to let you know about it.
^ permalink raw reply
* Re: [PATCH 2/2] HID: i2c-hid: goodix: Add support for powered-in-suspend property
From: Jeff LaBundy @ 2023-04-24 3:38 UTC (permalink / raw)
To: Fei Shao
Cc: Benjamin Tissoires, Douglas Anderson, Rob Herring, linux-mediatek,
Dmitry Torokhov, Jiri Kosina, Matthias Kaehlcke, Stephen Kitt,
linux-input, linux-kernel
In-Reply-To: <20230418124953.3170028-3-fshao@chromium.org>
Hi Fei,
On Tue, Apr 18, 2023 at 08:49:52PM +0800, Fei Shao wrote:
> In the beginning, commit 18eeef46d359 ("HID: i2c-hid: goodix: Tie the
> reset line to true state of the regulator") introduced a change to tie
> the reset line of the Goodix touchscreen to the state of the regulator
> to fix a power leakage issue in suspend.
>
> After some time, the change was deemed unnecessary and was reverted in
> commit 557e05fa9fdd ("HID: i2c-hid: goodix: Stop tying the reset line to
> the regulator") due to difficulties in managing regulator notifiers for
> designs like Evoker, which provides a second power rail to touchscreen.
>
> However, the revert caused a power regression on another Chromebook
> device Steelix in the field, which has a dedicated always-on regulator
> for touchscreen and was covered by the workaround in the first commit.
>
> To address both cases, this patch adds the support for the
> `powered-in-suspend` property in the driver that allows the driver to
> determine whether the touchscreen is still powered in suspend, and
> handle the reset GPIO accordingly as below:
> - When set to true, the driver does not assert the reset GPIO in power
> down. To ensure a clean start and the consistent behavior, it does the
> assertion in power up instead.
> This is for designs with a dedicated always-on regulator.
> - When set to false, the driver uses the original control flow and
> asserts GPIO and disable regulators normally.
> This is for the two-regulator and shared-regulator designs.
>
> Signed-off-by: Fei Shao <fshao@chromium.org>
>
> ---
>
> drivers/hid/i2c-hid/i2c-hid-of-goodix.c | 46 +++++++++++++++++++++----
> 1 file changed, 39 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> index 0060e3dcd775..b438db8ca6f4 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-of-goodix.c
> @@ -28,6 +28,7 @@ struct i2c_hid_of_goodix {
> struct regulator *vdd;
> struct regulator *vddio;
> struct gpio_desc *reset_gpio;
> + bool powered_in_suspend;
> const struct goodix_i2c_hid_timing_data *timings;
> };
>
> @@ -37,13 +38,34 @@ static int goodix_i2c_hid_power_up(struct i2chid_ops *ops)
> container_of(ops, struct i2c_hid_of_goodix, ops);
> int ret;
>
> - ret = regulator_enable(ihid_goodix->vdd);
> - if (ret)
> - return ret;
> -
> - ret = regulator_enable(ihid_goodix->vddio);
> - if (ret)
> - return ret;
> + /*
> + * This is to ensure that the reset GPIO will be asserted and the
> + * regulators will be enabled for all cases.
> + */
> + if (ihid_goodix->powered_in_suspend) {
> + /*
> + * This is not mandatory, but we assert reset here (instead of
> + * in power-down) to ensure that the device will have a clean
> + * state later on just like the normal scenarios would have.
> + *
> + * Also, since the regulators were not disabled in power-down,
> + * we don't need to enable them here.
> + */
> + gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
> + } else {
> + /*
> + * In this case, the reset is already asserted (either in
> + * probe or power-down).
> + * All we need is to enable the regulators.
> + */
> + ret = regulator_enable(ihid_goodix->vdd);
> + if (ret)
> + return ret;
> +
> + ret = regulator_enable(ihid_goodix->vddio);
> + if (ret)
> + return ret;
> + }
Please let me know in case I have misunderstood, but I don't see a need
to change the regulator_enable/disable() logic if this property is set.
If the regulators are truly always-on, the regulator core already knows
what to do and we should not duplicate that logic here.
Based on the alleged silicon erratum discussed in patch [1/2], it seems
we only want to control the behavior of the reset GPIO. Therefore, only
the calls to gpiod_set_value_cansleep() should be affected and the name
of the property updated to reflect what it's actually doing.
>
> if (ihid_goodix->timings->post_power_delay_ms)
> msleep(ihid_goodix->timings->post_power_delay_ms);
> @@ -60,6 +82,13 @@ static void goodix_i2c_hid_power_down(struct i2chid_ops *ops)
> struct i2c_hid_of_goodix *ihid_goodix =
> container_of(ops, struct i2c_hid_of_goodix, ops);
>
> + /*
> + * Don't assert reset GPIO or disable regulators if we're keeping the
> + * device powered in suspend.
> + */
> + if (ihid_goodix->powered_in_suspend)
> + return;
> +
> gpiod_set_value_cansleep(ihid_goodix->reset_gpio, 1);
> regulator_disable(ihid_goodix->vddio);
> regulator_disable(ihid_goodix->vdd);
> @@ -91,6 +120,9 @@ static int i2c_hid_of_goodix_probe(struct i2c_client *client)
> if (IS_ERR(ihid_goodix->vddio))
> return PTR_ERR(ihid_goodix->vddio);
>
> + ihid_goodix->powered_in_suspend =
> + of_property_read_bool(client->dev.of_node, "powered-in-suspend");
> +
> ihid_goodix->timings = device_get_match_data(&client->dev);
>
> return i2c_hid_core_probe(client, &ihid_goodix->ops, 0x0001, 0);
> --
> 2.40.0.634.g4ca3ef3211-goog
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: input: goodix: Add powered-in-suspend property
From: Jeff LaBundy @ 2023-04-24 3:31 UTC (permalink / raw)
To: Fei Shao
Cc: Doug Anderson, Benjamin Tissoires, Rob Herring, linux-mediatek,
Dmitry Torokhov, Krzysztof Kozlowski, devicetree, linux-input,
linux-kernel
In-Reply-To: <CAC=S1nj8VTXOovvianPWDYnsrbek0APD76SBNquFObaw8Vg4BQ@mail.gmail.com>
Hi Doug and Fei,
Thank you for this additional information, and your continued patience
with my many questions :)
On Thu, Apr 20, 2023 at 04:13:37PM +0800, Fei Shao wrote:
> Hi,
>
> On Wed, Apr 19, 2023 at 11:41 PM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Wed, Apr 19, 2023 at 8:18 AM Jeff LaBundy <jeff@labundy.com> wrote:
> > >
> > > Hi Doug and Fei,
> > >
> > > Thank you for the informative discussion; I can empathize with the pain
> > > these issues bring.
> > >
> > > On Wed, Apr 19, 2023 at 07:38:13AM -0700, Doug Anderson wrote:
> > > > Hi,
> > > >
> > > > On Wed, Apr 19, 2023 at 3:44 AM Fei Shao <fshao@chromium.org> wrote:
> > > > >
> > > > > Hi Jeff,
> > > > >
> > > > > On Wed, Apr 19, 2023 at 8:21 AM Jeff LaBundy <jeff@labundy.com> wrote:
> > > > > >
> > > > > > Hi Fei,
> > > > > >
> > > > > > On Tue, Apr 18, 2023 at 08:49:51PM +0800, Fei Shao wrote:
> > > > > > > We observed that on Chromebook device Steelix, if Goodix GT7375P
> > > > > > > touchscreen is powered in suspend (because, for example, it connects to
> > > > > > > an always-on regulator) and with the reset GPIO asserted, it will
> > > > > > > introduce about 14mW power leakage.
> > > > > > >
> > > > > > > This property is used to indicate that the touchscreen is powered in
> > > > > > > suspend. If it's set, the driver will stop asserting the reset GPIO in
> > > > > > > power-down, and it will do it in power-up instead to ensure that the
> > > > > > > state is always reset after resuming.
> > > > > > >
> > > > > > > Signed-off-by: Fei Shao <fshao@chromium.org>
> > > > > > > ---
> > > > > >
> > > > > > This is an interesting problem; were you able to root-cause why the silicon
> > > > > > exhibits this behavior? Simply asserting reset should not cause it to draw
> > > > > > additional power, let alone 14 mW. This almost sounds like a back-powering
> > > > > > problem during suspend.
> > > > > >
> > > > > There was a fix for this behavior before so I didn't dig into it on
> > > > > the silicon side.
> > > > > I can ask internally and see if we can have Goodix to confirm this is
> > > > > a known HW erratum.
> > > >
> > > > Certainly it doesn't hurt to check, but it's not really that shocking
> > > > to me that asserting reset could cause a power draw on some hardware.
> > > > Reset puts hardware into a default state and that's not necessarily
> > > > low power. I guess ideally hardware would act like it's "off" when
> > > > reset is asserted and then then init to the default state on the edge
> > > > as reset was deasserted, but I not all hardware is designed in an
> > > > ideal way.
> > >
> > > While that is true in theory, I have never, ever seen that to be the case
> > > when there is not some other underlying problem.
> > >
> > > What I have seen, however, is that asserting reset actually causes the GPIO
> > > to sink current from some other supply and through the IC. I loosely suspect
> > > that if you probe the IC's rails and digital I/O during the failure condition,
> > > you may find one of them resting at some mid-rail voltage or diode drop. It
> > > seems you have a similar suspicion.
> > >
>
> I reached out to our EE with your thoughts.
> He said that he understands the concern, but this doesn't apply in our
> schematics because there's only one supply.
> Also he simulated a few scenarios that could explain the
> back-powering, but none of them is possible without having the
> problematic circuit/rsense layout from within the IC itself.
>
> > > In that case, it may mean that some other supply in the system should actually
> > > be kept on, or that supplies are being brought down out of order. In which
> > > case, the solution should actually be a patch to the affected platform(s) dts
> > > and not the mainline driver.
> >
> > I agree that it's a bandaid, but I'm not hopeful that a better
> > solution will be found.
> >
> > Specifically, I'd expect a current leak in the system when you turn a
> > supply off and then assert a GPIO high. That's when the device can
> > start backpowering itself from a GPIO. In this case, it's the
> > opposite. We're keeping the supply on and asserting the (active low)
> > reset GPIO to cause the higher power draw. In another design it was
> > confirmed that the power draw went away when we were able to turn the
> > regulator off (but still keep the active low reset GPIO asserted).
> > We've also confirmed that power is good if we keep the supply on and
> > _don't_ assert the reset GPIO. Both of these two experiments provide
> > some evidence that the system is configured properly and we're not
> > backpowering something.
Back-powering can come in two forms:
1. The one you've described, which is by far the most common. As you mention,
it is not the case here since the issue happens only when we drive the GPIO
low and not high.
2. Through a forbidden power supply sequence, an input pin of an IC with
multiple power supplies becomes a weak power supply itself. Grounding the
GPIO then sinks current into the SoC.
This problem smelled like (2), especially since asserting the GPIO or powering
down the supply alleviates the symptom. Most Goodix controllers I've worked
with have two or more supplies, and the datasheet requires them to be enabled
or disabled in a specific order.
Based on Fei's feedback, however, this IC does not seem to be one of those
since there is reportedly only a single rail. I guess either vdd or vddio is
tied to a dummy regulator? If so, then perhaps we can scratch this theory.
> >
> > I guess I should revise the above, though. I could believe that there
> > is a current leak but on the touchscreen controller board itself,
> > which is a black box to us. I have certainly been involved in products
> > in the past where the default state of the system at reset caused a
> > minor current leak (I remember an EE telling me that as soon as
> > software started running I should quickly change the direction of a
> > GPIO) and it wouldn't shock me if the touchscreen controller board had
> > a problem like this. If there is a problem like this on the
> > touchscreen controller board there's not much we can do to workaround
> > it.
> >
> > Unfortunately, if the problem ends up needing a hardware change to fix
> > more correctly (which I suspect it does), our hands are tied a bit.
> > This is not prototype hardware but is final hardware.
Fair enough, I was simply sketpical that this was the _right_ workaround.
Were this an issue of only supply A left on yet the IC datasheet requires
supply B to remain on if supply A is on, I would rather see this solved by
updating a regulator dts node, trusted FW sequence, etc.
> >
> > I guess one further note is that, at least on the project I was
> > involved in that had a similar problem, folks in China did a bunch of
> > analysis on this and went as far as adding an extra regulator to the
> > main board schematic to "fix" it. Had the issue just been something
> > where we were misconfiguing GPIOs or leaving a regulator in the wrong
> > state then they (probably) would have identified it rather than
> > spinning the board.
>
> Thank you Doug for providing the details and explanation, and sorry
> that I also missed your original reply...
> I only considered the ideal scenarios for the always_on usage but not
> the cases you brought up. The concerns make sense to me.
>
> I'm still awaiting the response from Goodix, but +1 that if it turns
> out to be a GT7375P hw issue, we're not able to do much about that
> except relying on the driver workaround.
> One more note I want to add is that the first attempt of the fix was
> added ~2yrs ago, so it's not an one-off on a particular platform, plus
> `sc7180-trogdor-homestar` and `mt8186-corsola-steelix` are two
> different designs come from two different teams, but they ended up
> with the same symptom.
> With that said, I think we have more confidence to say it's a
> component misbehavior, and we just fell into the edge case that was
> not covered or considered by its design.
Thanks for your due diligence; if this really is a silicon issue, then
the driver should indeed accommodate it.
That being said, the name 'powered-in-suspend' seems a bit conflated. We
should not be duplicating regulator policy in this driver; the existing
naming also may cause confusion for other HW configurations that do leave
vdd and vddio on during suspend, but don't have this problem.
Here, we actually mean to control the behavior of the reset GPIO through
suspend and therefore something like 'goodix,no-reset-during-suspend' is
closer to what I understand us to intend to do. I will add more details
in patch [2/2].
>
> Regards,
> Fei
>
> >
> > -Doug
Kind regards,
Jeff LaBundy
^ permalink raw reply
* Re: [PATCH v3 2/5] Input: add driver for Focaltech FTS touchscreen
From: Jeff LaBundy @ 2023-04-24 2:39 UTC (permalink / raw)
To: Joel Selvaraj
Cc: Caleb Connolly, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Andy Gross, Bjorn Andersson, Konrad Dybcio, Henrik Rydberg,
Arnd Bergmann, Robert Jarzmik, Markuss Broks, Jean Delvare,
Max Krummenacher, Chris Morgan, Job Noorman, Alistair Francis,
Hans de Goede, Maxime Ripard, linux-input, devicetree,
linux-kernel, linux-arm-msm, ~postmarketos/upstreaming,
phone-devel
In-Reply-To: <20230415020222.216232-3-joelselvaraj.oss@gmail.com>
Hi Joel,
Great work so far! It's coming along nicely. Please find my latest
feedback below.
On Fri, Apr 14, 2023 at 09:02:19PM -0500, Joel Selvaraj wrote:
> The Focaltech FTS driver supports several variants of focaltech
> touchscreens found in ~2018 era smartphones including variants found on
> the PocoPhone F1 and the SHIFT6mq which are already present in mainline.
> This driver is loosely based on the original driver from Focaltech
> but has been simplified and largely reworked.
>
> Co-developed-by: Caleb Connolly <caleb@connolly.tech>
> Signed-off-by: Caleb Connolly <caleb@connolly.tech>
> Signed-off-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>
> ---
> MAINTAINERS | 8 +
> drivers/input/touchscreen/Kconfig | 12 +
> drivers/input/touchscreen/Makefile | 1 +
> drivers/input/touchscreen/focaltech_fts5452.c | 432 ++++++++++++++++++
> 4 files changed, 453 insertions(+)
> create mode 100644 drivers/input/touchscreen/focaltech_fts5452.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7ec4ce64f66d..1a3ea61e1f52 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8028,6 +8028,14 @@ L: linux-input@vger.kernel.org
> S: Maintained
> F: drivers/input/joystick/fsia6b.c
>
> +FOCALTECH FTS5452 TOUCHSCREEN DRIVER
> +M: Joel Selvaraj <joelselvaraj.oss@gmail.com>
> +M: Caleb Connolly <caleb@connolly.tech>
> +L: linux-input@vger.kernel.org
> +S: Maintained
> +F: Documentation/devicetree/bindings/input/touchscreen/focaltech,fts5452.yaml
> +F: drivers/input/touchscreen/focaltech_fts5452.c
> +
> FOCUSRITE SCARLETT GEN 2/3 MIXER DRIVER
> M: Geoffrey D. Bennett <g@b4.vu>
> L: alsa-devel@alsa-project.org (moderated for non-subscribers)
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 1feecd7ed3cb..11af91504969 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -388,6 +388,18 @@ config TOUCHSCREEN_EXC3000
> To compile this driver as a module, choose M here: the
> module will be called exc3000.
>
> +config TOUCHSCREEN_FOCALTECH_FTS5452
> + tristate "Focaltech FTS Touchscreen"
> + depends on I2C
> + help
> + Say Y here to enable support for I2C connected Focaltech FTS
> + based touch panels, including the 5452 and 8917 panels.
This language is a bit misleading, as it seems to suggest three or more
models are supported. It seems the title should simply be "FocalTech
FTS5452 touchscreen controller" with the description as "...FocalTech
FTS5452 and compatible touchscreen controllers."
As more are found to be compatible (e.g. FTS8917), the compatible strings
can simply be appended.
> +
> + If unsure, say N.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called focaltech_fts.
> +
> config TOUCHSCREEN_FUJITSU
> tristate "Fujitsu serial touchscreen"
> select SERIO
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index 159cd5136fdb..47d78c9cff21 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -45,6 +45,7 @@ obj-$(CONFIG_TOUCHSCREEN_ELO) += elo.o
> obj-$(CONFIG_TOUCHSCREEN_EGALAX) += egalax_ts.o
> obj-$(CONFIG_TOUCHSCREEN_EGALAX_SERIAL) += egalax_ts_serial.o
> obj-$(CONFIG_TOUCHSCREEN_EXC3000) += exc3000.o
> +obj-$(CONFIG_TOUCHSCREEN_FOCALTECH_FTS5452) += focaltech_fts5452.o
> obj-$(CONFIG_TOUCHSCREEN_FUJITSU) += fujitsu_ts.o
> obj-$(CONFIG_TOUCHSCREEN_GOODIX) += goodix_ts.o
> obj-$(CONFIG_TOUCHSCREEN_HIDEEP) += hideep.o
> diff --git a/drivers/input/touchscreen/focaltech_fts5452.c b/drivers/input/touchscreen/focaltech_fts5452.c
> new file mode 100644
> index 000000000000..abf8a2f271ca
> --- /dev/null
> +++ b/drivers/input/touchscreen/focaltech_fts5452.c
> @@ -0,0 +1,432 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * FocalTech touchscreen driver.
> + *
> + * Copyright (c) 2010-2017, FocalTech Systems, Ltd., all rights reserved.
> + * Copyright (C) 2018 XiaoMi, Inc.
> + * Copyright (c) 2021 Caleb Connolly <caleb@connolly.tech>
> + * Copyright (c) 2023 Joel Selvaraj <joelselvaraj.oss@gmail.com>
> + */
Nit: inconsistent copyright capitalization.
> +
> +#include <linux/delay.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>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +
> +#define FTS_REG_CHIP_ID_H 0xA3
> +#define FTS_REG_CHIP_ID_L 0x9F
> +
> +#define FTS_MAX_POINTS_SUPPORT 10
> +#define FTS_ONE_TOUCH_LEN 6
> +
> +#define FTS_TOUCH_X_H_OFFSET 3
> +#define FTS_TOUCH_X_L_OFFSET 4
> +#define FTS_TOUCH_Y_H_OFFSET 5
> +#define FTS_TOUCH_Y_L_OFFSET 6
> +#define FTS_TOUCH_PRESSURE_OFFSET 7
> +#define FTS_TOUCH_AREA_OFFSET 8
> +#define FTS_TOUCH_TYPE_OFFSET 3
> +#define FTS_TOUCH_ID_OFFSET 5
> +
> +#define FTS_TOUCH_DOWN 0
> +#define FTS_TOUCH_UP 1
> +#define FTS_TOUCH_CONTACT 2
> +
> +#define FTS_INTERVAL_READ_REG_MS 100
> +#define FTS_TIMEOUT_READ_REG_MS 2000
> +
> +#define FTS_DRIVER_NAME "fts-i2c"
> +
> +static const u16 fts_chip_types[] = {
> + 0x5452,
> + 0x8719,
> +};
> +
> +struct fts_ts_data {
> + struct i2c_client *client;
> + struct input_dev *input_dev;
> + struct regmap *regmap;
> + int irq;
> + struct regulator_bulk_data regulators[2];
> + u8 max_touch_points;
> + u8 *point_buf;
> + int point_buf_size;
> + struct gpio_desc *reset_gpio;
> + struct touchscreen_properties prop;
> +};
> +
> +struct fts_i2c_chip_data {
> + int max_touch_points;
> +};
There is no reason to wrap a single member in a struct; just define an array
and point each driver_data member to the appropriate element.
An even better solution, however, would be to merge the device ID into this.
Then you would have a single array of structs with very clear association
between device ID and number of points.
> +
> +int fts_check_status(struct fts_ts_data *data)
This function can be static. It also seems to be inappropriately named. Here
we are checking the device's ID, not its status.
> +{
> + int error, i = 0, count = 0;
> + unsigned int val, id;
> +
> + do {
> + error = regmap_read(data->regmap, FTS_REG_CHIP_ID_L, &id);
> + if (error)
> + dev_err(&data->client->dev, "I2C read failed: %d\n", error);
If this read fails, there is no point in continuing further in this loop. Most
likely the second read would fail as well but if it doesn't, you are computing
the id using an uninitialized variable.
Can you also explain, and possibly add comments, as to why the device ID must
be checked in a retry loop? Is it because the device may be in a deep sleep and
must be hit with I2C traffic a couple of times?
If so, then you likely want to briefly sleep and then start over (i.e. continue)
in the event of an error.
> +
> + error = regmap_read(data->regmap, FTS_REG_CHIP_ID_H, &val);
> + if (error)
> + dev_err(&data->client->dev, "I2C read failed: %d\n", error);
Same problem here.
> +
> + id |= val << 8;
> +
> + for (i = 0; i < ARRAY_SIZE(fts_chip_types); i++)
> + if (id == fts_chip_types[i])
> + return 0;
This retry loop in general seems a bit non-optimal. If for example the driver
is simply communicating with an incompatible device, there is no need to go
through all N loops.
> +
> + count++;
> + msleep(FTS_INTERVAL_READ_REG_MS);
> + } while ((count * FTS_INTERVAL_READ_REG_MS) < FTS_TIMEOUT_READ_REG_MS);
This multiplication seems unnecessarily complicated; can we not simply have
FTS_MAX_RETRIES or similar?
> +
> + return -EIO;
> +}
> +
> +static int fts_report_touch(struct fts_ts_data *data)
> +{
> + struct input_dev *input_dev = data->input_dev;
> + int base;
> + unsigned int x, y, z, maj;
> + u8 slot, type;
> + int error, i = 0;
> +
> + u8 *buf = data->point_buf;
> +
> + memset(buf, 0, data->point_buf_size);
> +
> + error = regmap_bulk_read(data->regmap, 0, buf, data->point_buf_size);
> + if (error) {
> + dev_err(&data->client->dev, "I2C read failed: %d\n", error);
> + return error;
> + }
> +
> + for (i = 0; i < data->max_touch_points; i++) {
> + base = FTS_ONE_TOUCH_LEN * i;
> +
> + slot = buf[base + FTS_TOUCH_ID_OFFSET] >> 4;
> + if (slot >= data->max_touch_points)
> + break;
> +
> + x = ((buf[base + FTS_TOUCH_X_H_OFFSET] & 0x0F) << 8) +
> + (buf[base + FTS_TOUCH_X_L_OFFSET] & 0xFF);
> + y = ((buf[base + FTS_TOUCH_Y_H_OFFSET] & 0x0F) << 8) +
> + (buf[base + FTS_TOUCH_Y_L_OFFSET] & 0xFF);
Sorry, I did not quite follow the image that was shared in an earlier thread.
It is unclear to me why we cannot represent the interrupt status registers
as an array of __be16 values and then do something like the following:
x = be16_to_cpu(buf[FTS_TOUCH_X_OFFSET]) & GENMASK(11, 0);
I would be surprised if the mask is even necessary; you would need to refer
to a datasheet however. Perhaps the vendor would be willing to give one to
you if it means they get an upstream driver?
> +
> + z = buf[base + FTS_TOUCH_PRESSURE_OFFSET];
> + if (z == 0)
> + z = 0x3f;
> +
> + maj = buf[base + FTS_TOUCH_AREA_OFFSET] >> 4;
> + if (maj == 0)
> + maj = 0x09;
I think we need some comments and possibly some #defines to explain what is
happening here.
> +
> + type = buf[base + FTS_TOUCH_TYPE_OFFSET] >> 6;
> +
> + input_mt_slot(input_dev, slot);
> + if (type == FTS_TOUCH_DOWN || type == FTS_TOUCH_CONTACT) {
> + input_mt_report_slot_state(input_dev, MT_TOOL_FINGER, true);
> + touchscreen_report_pos(data->input_dev, &data->prop, x, y, true);
> + input_report_abs(input_dev, ABS_MT_PRESSURE, z);
> + input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR, maj);
> + input_report_key(data->input_dev, BTN_TOUCH, 1);
> + } else {
> + input_report_key(data->input_dev, BTN_TOUCH, 0);
> + input_mt_report_slot_inactive(input_dev);
> + }
> + }
> + input_mt_sync_frame(input_dev);
> + input_sync(input_dev);
> +
> + return 0;
> +}
> +
> +static irqreturn_t fts_ts_interrupt(int irq, void *dev_id)
> +{
> + struct fts_ts_data *data = dev_id;
> +
> + return fts_report_touch(data) ? IRQ_NONE : IRQ_HANDLED;
> +}
> +
> +static void fts_power_off(void *d)
> +{
> + struct fts_ts_data *data = d;
> +
> + regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators);
> +}
> +
> +static int fts_start(struct fts_ts_data *data)
> +{
> + int error;
> +
> + error = regulator_bulk_enable(ARRAY_SIZE(data->regulators),
> + data->regulators);
> + if (error) {
> + dev_err(&data->client->dev, "failed to enable regulators\n");
> + return error;
> + }
> +
> + gpiod_set_value_cansleep(data->reset_gpio, 0);
> + msleep(200);
Same here with respect to comments; what happens during these first 200 ms after
reset is released? Does the interrupt pin toggle several times? 200 ms is also
quite a while to wait each time the input handler opens the device; is it really
necessary?
> +
> + enable_irq(data->irq);
> +
> + return 0;
> +}
> +
> +static int fts_stop(struct fts_ts_data *data)
> +{
> + disable_irq(data->irq);
> + gpiod_set_value_cansleep(data->reset_gpio, 1);
> + fts_power_off(data);
> +
> + return 0;
> +}
> +
> +static int fts_input_open(struct input_dev *dev)
> +{
> + struct fts_ts_data *data = input_get_drvdata(dev);
> + int error;
> +
> + error = fts_start(data);
> + if (error)
> + return error;
> +
> + error = fts_check_status(data);
> + if (error) {
> + dev_err(&data->client->dev, "Failed to start or unsupported chip");
> + return error;
> + }
It seems unnecessary and wasteful to check the device ID every time the input
handler opens the device. We also don't want to go through all the trouble of
registering the device, only to find out later it wasn't even the right part.
Instead, you should power up the device during probe, validate its ID and then
power it back down.
> +
> + return 0;
> +}
> +
> +static void fts_input_close(struct input_dev *dev)
> +{
> + struct fts_ts_data *data = input_get_drvdata(dev);
> +
> + fts_stop(data);
> +}
> +
> +static int fts_input_init(struct fts_ts_data *data)
> +{
> + struct device *dev = &data->client->dev;
> + struct input_dev *input_dev;
> + int error = 0;
No need to initialize this, only for it to get overwritten later.
> +
> + input_dev = devm_input_allocate_device(dev);
> + if (!input_dev)
> + return -ENOMEM;
> +
> + data->input_dev = input_dev;
> +
> + input_dev->name = FTS_DRIVER_NAME;
> + input_dev->id.bustype = BUS_I2C;
> + input_dev->dev.parent = dev;
> + input_dev->open = fts_input_open;
> + input_dev->close = fts_input_close;
> + input_set_drvdata(input_dev, data);
> +
> + input_set_capability(input_dev, EV_ABS, ABS_MT_POSITION_X);
> + input_set_capability(input_dev, EV_ABS, ABS_MT_POSITION_Y);
> + input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
> + input_set_abs_params(input_dev, ABS_MT_PRESSURE, 0, 255, 0, 0);
> +
> + touchscreen_parse_properties(input_dev, true, &data->prop);
> + if (!data->prop.max_x || !data->prop.max_y) {
> + dev_err(dev,
> + "touchscreen-size-x and/or touchscreen-size-y not set in device properties\n");
"Device properties" is vague; one could interpret it to mean the controller's
embedded FW. Just cut the message off at "...not set".
> + return -EINVAL;
> + }
> +
> + error = input_mt_init_slots(input_dev, data->max_touch_points,
> + INPUT_MT_DIRECT);
> + if (error)
> + return error;
> +
> + data->point_buf_size = (data->max_touch_points * FTS_ONE_TOUCH_LEN) + 3;
> + data->point_buf = devm_kzalloc(dev, data->point_buf_size, GFP_KERNEL);
> + if (!data->point_buf)
> + return -ENOMEM;
> +
> + error = input_register_device(input_dev);
> + if (error) {
> + dev_err(dev, "Failed to register input device\n");
> + return error;
> + }
> +
> + return 0;
> +}
> +
> +static const struct regmap_config fts_ts_i2c_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> +};
> +
> +static int fts_ts_probe(struct i2c_client *client)
> +{
> + const struct i2c_device_id *id = i2c_client_get_device_id(client);
> + const struct fts_i2c_chip_data *chip_data;
> + struct fts_ts_data *data;
> + int error = 0;
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> + dev_err(&client->dev, "I2C not supported");
> + return -ENODEV;
> + }
> +
> + if (!client->irq) {
> + dev_err(&client->dev, "No irq specified\n");
> + return -EINVAL;
> + }
> +
> + data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + chip_data = device_get_match_data(&client->dev);
> + if (!chip_data)
> + chip_data = (const struct fts_i2c_chip_data *)id->driver_data;
> + if (!chip_data || !chip_data->max_touch_points) {
> + dev_err(&client->dev, "invalid or missing chip data\n");
> + return -EINVAL;
> + }
> + if (chip_data->max_touch_points > FTS_MAX_POINTS_SUPPORT) {
> + dev_err(&client->dev,
> + "invalid chip data, max_touch_points should be less than or equal to %d\n",
> + FTS_MAX_POINTS_SUPPORT);
> + return -EINVAL;
> + }
This check is not necessary; if someone adds an invalid max_touch_points, then the
driver was updated incorrectly. There is no need to check it at every runtime.
> + data->max_touch_points = chip_data->max_touch_points;
> +
> + data->client = client;
> + i2c_set_clientdata(client, data);
> +
> + data->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(data->reset_gpio)) {
> + error = PTR_ERR(data->reset_gpio);
> + dev_err(&client->dev, "Failed to request reset gpio, error %d\n", error);
> + return error;
> + }
> +
> + data->regmap = devm_regmap_init_i2c(client, &fts_ts_i2c_regmap_config);
> + if (IS_ERR(data->regmap)) {
> + error = PTR_ERR(data->regmap);
> + dev_err(&client->dev, "regmap allocation failed, error %d\n", error);
> + return error;
> + }
> +
> + /*
> + * AVDD is the analog voltage supply (2.6V to 3.3V)
> + * VDDIO is the digital voltage supply (1.8V)
> + */
> + data->regulators[0].supply = "avdd";
> + data->regulators[1].supply = "vddio";
> + error = devm_regulator_bulk_get(&client->dev, ARRAY_SIZE(data->regulators),
> + data->regulators);
> + if (error) {
> + dev_err(&client->dev, "Failed to get regulators %d\n", error);
> + return error;
> + }
> +
> + error = devm_add_action_or_reset(&client->dev, fts_power_off, data);
> + if (error) {
> + dev_err(&client->dev, "failed to install power off handler\n");
> + return error;
> + }
Christophe makes a great point. If this or any other call throughout the rest of
probe as you have written it fails, you will try to disable a disabled regulator.
The same will happen when the driver is torn down, as the input handler should
have already powered down the device by way of the close callback. Did you build
this driver as a module and test removal? I suspect you will get a stack trace.
I think the call needs to go away altogether.
> +
> + error = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> + fts_ts_interrupt, IRQF_ONESHOT,
> + client->name, data);
> + if (error) {
> + dev_err(&client->dev, "Failed to request IRQ: %d\n", error);
> + return error;
> + }
> +
> + error = fts_input_init(data);
> + if (error)
> + return error;
> +
> + return 0;
This is idiomatic, but I find "return fts_input_init(data);" to be simpler.
> +}
> +
> +static int fts_pm_suspend(struct device *dev)
> +{
> + struct fts_ts_data *data = dev_get_drvdata(dev);
> +
> + mutex_lock(&data->input_dev->mutex);
> +
> + if (input_device_enabled(data->input_dev))
> + fts_stop(data);
> +
> + mutex_unlock(&data->input_dev->mutex);
> +
> + return 0;
> +}
> +
> +static int fts_pm_resume(struct device *dev)
> +{
> + struct fts_ts_data *data = dev_get_drvdata(dev);
> + int error = 0;
Same here, there is no point in initializating this.
> +
> + mutex_lock(&data->input_dev->mutex);
> +
> + if (input_device_enabled(data->input_dev))
> + error = fts_start(data);
> +
> + mutex_unlock(&data->input_dev->mutex);
> +
> + return error;
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(fts_dev_pm_ops, fts_pm_suspend, fts_pm_resume);
> +
> +static const struct fts_i2c_chip_data fts5452_chip_data = {
> + .max_touch_points = 5,
> +};
> +
> +static const struct fts_i2c_chip_data fts8719_chip_data = {
> + .max_touch_points = 10,
> +};
> +
> +static const struct i2c_device_id fts_i2c_id[] = {
> + { .name = "fts5452", .driver_data = (long)&fts5452_chip_data },
> + { .name = "fts8719", .driver_data = (long)&fts8719_chip_data },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(i2c, fts_i2c_id);
> +
> +static const struct of_device_id fts_of_match[] = {
> + { .compatible = "focaltech,fts5452", .data = &fts5452_chip_data },
> + { .compatible = "focaltech,fts8719", .data = &fts8719_chip_data },
> + { /* sentinel */ }
> +};
> +
> +MODULE_DEVICE_TABLE(of, fts_of_match);
> +
> +static struct i2c_driver fts_ts_driver = {
> + .probe_new = fts_ts_probe,
> + .id_table = fts_i2c_id,
> + .driver = {
> + .name = FTS_DRIVER_NAME,
> + .pm = pm_sleep_ptr(&fts_dev_pm_ops),
> + .of_match_table = fts_of_match,
> + },
> +};
> +module_i2c_driver(fts_ts_driver);
> +
> +MODULE_AUTHOR("Joel Selvaraj <joelselvaraj.oss@gmail.com>");
> +MODULE_AUTHOR("Caleb Connolly <caleb@connolly.tech>");
> +MODULE_DESCRIPTION("Focaltech Touchscreen Driver");
Nit: mixing 'FocalTech' and 'Focaltech' throughout.
> +MODULE_LICENSE("GPL");
> --
> 2.40.0
>
Kind regards,
Jeff LaBundy
^ permalink raw reply
* [PATCH] HID: google: add jewel USB id
From: Sung-Chi Li @ 2023-04-24 2:37 UTC (permalink / raw)
To: LKML
Cc: phoenixshen, chentsung, Sung-Chi Li, Benjamin Tissoires,
Jiri Kosina, linux-input
Add 1 additional hammer-like device.
Signed-off-by: Sung-Chi Li <lschyi@chromium.org>
---
drivers/hid/hid-google-hammer.c | 2 ++
drivers/hid/hid-ids.h | 1 +
2 files changed, 3 insertions(+)
diff --git a/drivers/hid/hid-google-hammer.c b/drivers/hid/hid-google-hammer.c
index 7ae5f27df54d..c6bdb9c4ef3e 100644
--- a/drivers/hid/hid-google-hammer.c
+++ b/drivers/hid/hid-google-hammer.c
@@ -586,6 +586,8 @@ static const struct hid_device_id hammer_devices[] = {
USB_VENDOR_ID_GOOGLE, USB_DEVICE_ID_GOOGLE_EEL) },
{ HID_DEVICE(BUS_USB, HID_GROUP_GENERIC,
USB_VENDOR_ID_GOOGLE, USB_DEVICE_ID_GOOGLE_HAMMER) },
+ { HID_DEVICE(BUS_USB, HID_GROUP_GENERIC,
+ USB_VENDOR_ID_GOOGLE, USB_DEVICE_ID_GOOGLE_JEWEL) },
{ HID_DEVICE(BUS_USB, HID_GROUP_GENERIC,
USB_VENDOR_ID_GOOGLE, USB_DEVICE_ID_GOOGLE_MAGNEMITE) },
{ HID_DEVICE(BUS_USB, HID_GROUP_GENERIC,
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index c2e9b6d1fd7d..c40cd6f1095f 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -528,6 +528,7 @@
#define USB_DEVICE_ID_GOOGLE_MOONBALL 0x5044
#define USB_DEVICE_ID_GOOGLE_DON 0x5050
#define USB_DEVICE_ID_GOOGLE_EEL 0x5057
+#define USB_DEVICE_ID_GOOGLE_JEWEL 0x5061
#define USB_VENDOR_ID_GOTOP 0x08f2
#define USB_DEVICE_ID_SUPER_Q2 0x007f
--
2.40.0.634.g4ca3ef3211-goog
^ permalink raw reply related
* Re: [git pull] Input updates for v6.3-rc7
From: pr-tracker-bot @ 2023-04-23 15:50 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Linus Torvalds, linux-kernel, linux-input
In-Reply-To: <ZES2ldRn0nqkf9v2@google.com>
The pull request you sent on Sat, 22 Apr 2023 21:39:49 -0700:
> git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git tags/input-for-v6.3-rc7
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/d6b78224c65e395db6aa66ea74522d9899c8aefb
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html
^ permalink raw reply
* [git pull] Input updates for v6.3-rc7
From: Dmitry Torokhov @ 2023-04-23 4:39 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-kernel, linux-input
Hi Linus,
Please pull from:
git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git tags/input-for-v6.3-rc7
to receive updates for the input subsystem. You will get 2 fixes:
- a check in pegasus-notetaker driver to validate the type of pipe
when probing a new device
- a fix for Cypress touch controller to correctly parse maximum
number of touches.
Changelog:
---------
Soumya Negi (1):
Input: pegasus-notetaker - check pipe type when probing
hrdl (1):
Input: cyttsp5 - fix sensing configuration data structure
Diffstat:
--------
drivers/input/tablet/pegasus_notetaker.c | 6 ++++++
drivers/input/touchscreen/cyttsp5.c | 1 +
2 files changed, 7 insertions(+)
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: xpad - fix GPF in xpad_probe
From: Dongliang Mu @ 2023-04-23 2:33 UTC (permalink / raw)
To: Dan Carpenter, Dan Carpenter
Cc: Vicki Pfau, kernel-janitors, Dmitry Torokhov, Pavel Rojtberg,
Nate Yocom, Mattijs Korpershoek, John Butler, Matthias Benkmann,
Christopher Crockett, Santosh De Massari, hust-os-kernel-patches,
syzbot+a3f758b8d8cb7e49afec, Pierre-Loup A. Griffais, linux-input,
linux-kernel
In-Reply-To: <d49bb5a2-ed78-4b16-b7ea-b9c7748ca0e4@kili.mountain>
On 2023/4/23 03:48, Dan Carpenter wrote:
> The warnings from this are quite promising.
>
> When you're writing a check, you start with a simple idea and then try
> it and then filter out the common false positives.
>
> The first 10 warnings are from loops like:
>
> p = NULL;
>
> for (i = 0; i < limit; i++) {
> if (i == 0)
> p = non_null();
> else
> *p = something();
> }
>
> Smatch doesn't handle loops correctly. (I know how to fix this but I've
> never gotten around to it because it would make Smatch slow)...
>
> So instead of that maybe I would do a hack to silence this type of
> warning. Not sure what...
>
> drivers/usb/gadget/udc/amd5536udc_pci.c:61 udc_pci_remove() warn: pointer dereferenced without being set '&udc->gadget'
> This one is interesting. Seems like a real bug.
>
> drivers/mtd/ubi/block.c:391 ubiblock_create() warn: pointer dereferenced without being set 'dev->gd'
>
> This one too. So maybe we could make this a separate warning where
> NULL dereferences happen on error paths. Or maybe when they happen in
> printks.
>
> So there are ways to take this first draft and massage it and get
> fewer false positives, by filtering false positives or taking things
> which work and creating new checks instead.
Hi Dan,
thanks for your efforts. After finishing the current task list, we can
first ask senior students to check this result quickly and then assign
highly-to-be True Positive to students.
BTW, do you have any plans to improve the code readability, directory
orgranization, documentation etc. of Smatch? It's hard even for senior
students to start with.
> Anyway, results attached.
>
> regards,
> dan carpenter
>
^ permalink raw reply
* Re: [PATCH] Input: xpad - fix GPF in xpad_probe
From: Dan Carpenter @ 2023-04-22 19:56 UTC (permalink / raw)
To: Dan Carpenter
Cc: Dongliang Mu, Vicki Pfau, kernel-janitors, Dmitry Torokhov,
Pavel Rojtberg, Nate Yocom, Mattijs Korpershoek, John Butler,
Matthias Benkmann, Christopher Crockett, Santosh De Massari,
hust-os-kernel-patches, syzbot+a3f758b8d8cb7e49afec,
Pierre-Loup A. Griffais, linux-input, linux-kernel
In-Reply-To: <d49bb5a2-ed78-4b16-b7ea-b9c7748ca0e4@kili.mountain>
On Sat, Apr 22, 2023 at 10:48:32PM +0300, Dan Carpenter wrote:
> The warnings from this are quite promising.
Well... The results are over 90% false positives. But there are still
probably 15 bugs in there. The printk warning trick would probably
work.
regards,
dan carpenter
^ 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