* [PATCH v2 3/3] HID: i2c-hid-of: Add reset GPIO support to i2c-hid-of
From: Hans de Goede @ 2023-04-13 9:36 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Hans de Goede, Douglas Anderson, linux-input
In-Reply-To: <20230413093625.71146-1-hdegoede@redhat.com>
Add reset GPIO support to the generic i2c-hid-of driver
This is necessary to make the Wacom digitizer on the Lenovo Yoga Book 1
(yb1-x90f/l) work and this will also allow consolidating the 2 specialized
i2c-hid-of-elan.c and i2c-hid-of-goodix.c drivers into the generic
i2c-hid-of driver.
For now the new "post-reset-deassert-delay-ms" property is only used on
x86/ACPI (non devicetree) devs. IOW it is not used in actual devicetree
files and the same goes for the reset GPIO. The devicetree-bindings
maintainers have requested properties like these to not be added to
the devicetree-bindings, so the new property + GPIO are deliberately
not added to the existing devicetree-bindings.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Chanhes in v2:
- Add a comment to the "post-reset-deassert-delay-ms" property reading,
that it is a kernel internal (non public) property used between x86
platform code and the i2c-hid driver.
---
drivers/hid/i2c-hid/i2c-hid-of.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/drivers/hid/i2c-hid/i2c-hid-of.c b/drivers/hid/i2c-hid/i2c-hid-of.c
index 385f7460e03c..855f53092f4e 100644
--- a/drivers/hid/i2c-hid/i2c-hid-of.c
+++ b/drivers/hid/i2c-hid/i2c-hid-of.c
@@ -21,6 +21,7 @@
#include <linux/delay.h>
#include <linux/device.h>
+#include <linux/gpio/consumer.h>
#include <linux/hid.h>
#include <linux/i2c.h>
#include <linux/kernel.h>
@@ -35,8 +36,10 @@ struct i2c_hid_of {
struct i2chid_ops ops;
struct i2c_client *client;
+ struct gpio_desc *reset_gpio;
struct regulator_bulk_data supplies[2];
int post_power_delay_ms;
+ int post_reset_delay_ms;
};
static int i2c_hid_of_power_up(struct i2chid_ops *ops)
@@ -55,6 +58,10 @@ static int i2c_hid_of_power_up(struct i2chid_ops *ops)
if (ihid_of->post_power_delay_ms)
msleep(ihid_of->post_power_delay_ms);
+ gpiod_set_value_cansleep(ihid_of->reset_gpio, 0);
+ if (ihid_of->post_reset_delay_ms)
+ msleep(ihid_of->post_reset_delay_ms);
+
return 0;
}
@@ -62,6 +69,7 @@ static void i2c_hid_of_power_down(struct i2chid_ops *ops)
{
struct i2c_hid_of *ihid_of = container_of(ops, struct i2c_hid_of, ops);
+ gpiod_set_value_cansleep(ihid_of->reset_gpio, 1);
regulator_bulk_disable(ARRAY_SIZE(ihid_of->supplies),
ihid_of->supplies);
}
@@ -96,6 +104,19 @@ static int i2c_hid_of_probe(struct i2c_client *client)
if (!device_property_read_u32(dev, "post-power-on-delay-ms", &val))
ihid_of->post_power_delay_ms = val;
+ /*
+ * Note this is a kernel internal device-property set by x86 platform code,
+ * this MUST not be used in devicetree files without first adding it to
+ * the DT bindings.
+ */
+ if (!device_property_read_u32(dev, "post-reset-deassert-delay-ms", &val))
+ ihid_of->post_reset_delay_ms = val;
+
+ /* Start out with reset asserted */
+ ihid_of->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+ if (IS_ERR(ihid_of->reset_gpio))
+ return PTR_ERR(ihid_of->reset_gpio);
+
ihid_of->supplies[0].supply = "vdd";
ihid_of->supplies[1].supply = "vddl";
ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ihid_of->supplies),
--
2.39.1
^ permalink raw reply related
* [PATCH v2 0/3] HID: i2c-hid-of: Allow using i2c-hid-of on non OF platforms
From: Hans de Goede @ 2023-04-13 9:36 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Hans de Goede, Douglas Anderson, linux-input
Hi Benjamin,
Here is a v2 of my series to allow using i2c-hid-of on non OF platforms
to allow I2C-HID devices which are not enumerated by ACPI to work on ACPI
platforms (by manual i2c_client instantiation using i2c_client_id matching).
Changes in v2:
- As discussed Drop the patches to consolidate all the i2c-hid-of*
drivers into one
- Add a comment to the "post-reset-deassert-delay-ms" property reading,
that it is a kernel internal (non public) property used between x86
platform code and the i2c-hid driver.
Regards,
Hans
Hans de Goede (3):
HID: i2c-hid-of: Consistenly use dev local variable in probe()
HID: i2c-hid-of: Allow using i2c-hid-of on non OF platforms
HID: i2c-hid-of: Add reset GPIO support to i2c-hid-of
drivers/hid/i2c-hid/Kconfig | 6 +++--
drivers/hid/i2c-hid/i2c-hid-of.c | 38 ++++++++++++++++++++++++--------
2 files changed, 33 insertions(+), 11 deletions(-)
--
2.39.1
^ permalink raw reply
* [PATCH v2 1/3] HID: i2c-hid-of: Consistenly use dev local variable in probe()
From: Hans de Goede @ 2023-04-13 9:36 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Hans de Goede, Douglas Anderson, linux-input
In-Reply-To: <20230413093625.71146-1-hdegoede@redhat.com>
i2c_hid_of_probe() has a dev local variable pointing to &i2c_client->dev,
consistently use this everywhere in i2c_hid_of_probe().
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/hid/i2c-hid/i2c-hid-of.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-of.c b/drivers/hid/i2c-hid/i2c-hid-of.c
index 10176568133a..c82a5a54c3e6 100644
--- a/drivers/hid/i2c-hid/i2c-hid-of.c
+++ b/drivers/hid/i2c-hid/i2c-hid-of.c
@@ -75,7 +75,7 @@ static int i2c_hid_of_probe(struct i2c_client *client)
int ret;
u32 val;
- ihid_of = devm_kzalloc(&client->dev, sizeof(*ihid_of), GFP_KERNEL);
+ ihid_of = devm_kzalloc(dev, sizeof(*ihid_of), GFP_KERNEL);
if (!ihid_of)
return -ENOMEM;
@@ -84,24 +84,21 @@ static int i2c_hid_of_probe(struct i2c_client *client)
ret = of_property_read_u32(dev->of_node, "hid-descr-addr", &val);
if (ret) {
- dev_err(&client->dev, "HID register address not provided\n");
+ dev_err(dev, "HID register address not provided\n");
return -ENODEV;
}
if (val >> 16) {
- dev_err(&client->dev, "Bad HID register address: 0x%08x\n",
- val);
+ dev_err(dev, "Bad HID register address: 0x%08x\n", val);
return -EINVAL;
}
hid_descriptor_address = val;
- if (!device_property_read_u32(&client->dev, "post-power-on-delay-ms",
- &val))
+ if (!device_property_read_u32(dev, "post-power-on-delay-ms", &val))
ihid_of->post_power_delay_ms = val;
ihid_of->supplies[0].supply = "vdd";
ihid_of->supplies[1].supply = "vddl";
- ret = devm_regulator_bulk_get(&client->dev,
- ARRAY_SIZE(ihid_of->supplies),
+ ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ihid_of->supplies),
ihid_of->supplies);
if (ret)
return ret;
--
2.39.1
^ permalink raw reply related
* [PATCH] Input: vmmouse - add macros to enable vmmouse relative mode
From: Zongmin Zhou @ 2023-04-13 8:56 UTC (permalink / raw)
To: zackr, linux-graphics-maintainer, pv-drivers, dmitry.torokhov
Cc: linux-input, linux-kernel, Zongmin Zhou
Add macros to enable request relative mode.
Change the REL_Y value passed by input_report_rel function,
to match the direction of mouse movement.
Signed-off-by: Zongmin Zhou<zhouzongmin@kylinos.cn>
---
drivers/input/mouse/vmmouse.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/input/mouse/vmmouse.c b/drivers/input/mouse/vmmouse.c
index ea9eff7c8099..f39ce21f7af9 100644
--- a/drivers/input/mouse/vmmouse.c
+++ b/drivers/input/mouse/vmmouse.c
@@ -21,6 +21,12 @@
#include "psmouse.h"
#include "vmmouse.h"
+/*
+ * Enable this to request relative mode.
+ * Defaut to absolute mode.
+ */
+//#define VMMOUSE_RELATIVE_MODE
+
#define VMMOUSE_PROTO_MAGIC 0x564D5868U
/*
@@ -184,7 +190,7 @@ static psmouse_ret_t vmmouse_report_events(struct psmouse *psmouse)
if (status & VMMOUSE_RELATIVE_PACKET) {
pref_dev = rel_dev;
input_report_rel(rel_dev, REL_X, (s32)x);
- input_report_rel(rel_dev, REL_Y, -(s32)y);
+ input_report_rel(rel_dev, REL_Y, (s32)y);
} else {
pref_dev = abs_dev;
input_report_abs(abs_dev, ABS_X, x);
@@ -304,8 +310,13 @@ static int vmmouse_enable(struct psmouse *psmouse)
VMMOUSE_CMD(ABSPOINTER_RESTRICT, VMMOUSE_RESTRICT_CPL0,
dummy1, dummy2, dummy3, dummy4);
+#ifdef VMMOUSE_RELATIVE_MODE
+ VMMOUSE_CMD(ABSPOINTER_COMMAND, VMMOUSE_CMD_REQUEST_RELATIVE,
+ dummy1, dummy2, dummy3, dummy4);
+#else
VMMOUSE_CMD(ABSPOINTER_COMMAND, VMMOUSE_CMD_REQUEST_ABSOLUTE,
dummy1, dummy2, dummy3, dummy4);
+#endif
return 0;
}
--
2.34.1
No virus found
Checked by Hillstone Network AntiVirus
^ permalink raw reply related
* [PATCH] Input: synaptics - disable intertouch for Lenovo L440
From: Jonathan Denose @ 2023-04-12 22:54 UTC (permalink / raw)
To: LKML
Cc: Dmitry Torokhov, Wolfram Sang, linux-input, Jonathan Denose,
Aman Dhoot, Lyude Paul, Mark Pearson
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>
---
drivers/input/mouse/synaptics.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index fa021af8506e4..77a4f58128e84 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 */
NULL
};
@@ -1769,6 +1769,8 @@ static int synaptics_create_intertouch(struct psmouse *psmouse,
.flags = I2C_CLIENT_HOST_NOTIFY,
};
+ psmouse_dbg(psmouse, "topbuttonpad is: %s\n", topbuttonpad ? "true" : "false");
+
return psmouse_smbus_init(psmouse, &intertouch_board,
&pdata, sizeof(pdata), true,
leave_breadcrumbs);
--
2.40.0.577.gac1e443424-goog
^ permalink raw reply related
* Re: [PATCH 0/6] HID: i2c-hid-of: Allow using i2c-hid-of on non OF platforms + remove specialized drivers
From: Doug Anderson @ 2023-04-12 18:57 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: Hans de Goede, Jiri Kosina, linux-input
In-Reply-To: <20230411165653.rw3ivfjdhjv5dmc3@mail.corp.redhat.com>
Hi,
On Tue, Apr 11, 2023 at 9:57 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> > Either way is fine with me really. So you get to chose. If you
> > let me know which route you prefer, I'll go and prepare either
> > a v2 of this series, or a whole new patch for the new specialized
> > driver.
>
> Sorry for being a PITA, but having those driver separated allowed to
> move forward without having to have a spaghetti plate in i2c-hid, which
> was the case before the split (because *everything* was entangled: ACPI,
> DT, OF, properties). So that's why I'm trying to understand and
> minimize the changes.
>
> Also, before you sending v2 and involving too much, we could try to wait a
> few days for Doug to answer, and hear if he has an opinion. But if you
> rather send v2 right away, that's your choice obviously :)
I can test things if need be, but I want to make sure we're on the
right approach before going too deep into testing...
I guess a few notes here:
In general, I think DT maintainers are pretty leery of anything in the
device tree that tries to be "generic" and then a whole pile of
"kitchen sink" properties added to actually describe the device. Even
if it starts with just a few properties, the worry is that it will end
up being more and more over time. They would much rather specify which
exact device is present on the board and imply all the properties
based on knowing the device. Then the only things that are in the
device tree as properties are things that are board-specific. For
instance, if there was a hardware strapping that let you choose two
different hid descriptor addresses then that would be something to put
in the device tree.
The "post-power-on-delay-ms" was something that the DT maintainers
weren't too happy with. They would have much rather inferred this from
the specific compatible. You can actually see that the bindings say
that "Just "hid-over-i2c" alone is allowed, but not recommended."
Now, that being said, it's not always a hard-and-fast rule. For
instance, after years of needing to list every eDP panel directly in
device tree (often lying about it when multiple sources were listed),
we finally did manage to get the generic "panel-edp" bindings accepted
that has "just a few" properties needed to power up a device. ...then
the rest of the things we need are inferred once we start talking to
the device and get it to self-identify.
Bringing it back to i2c-hid-of: even though today the "goodix" and
"elan" drivers are largely the same, it wasn't always the case. For a
little while we had a whole pile of special logic in the "goodix"
driver to deal with the fact that if the touchscreen is powered up
(because it's shared or always-on) but the reset line is held asserted
that it draws a bunch of extra power. I had to end up taking that
logic out because it was too hard to reconcile with the second voltage
rail that I needed to add for a different board. See commit
557e05fa9fdd ("HID: i2c-hid: goodix: Stop tying the reset line to the
regulator") and eb16f59e8e58 ("HID: i2c-hid: goodix: Add
mainboard-vddio-supply"). The need for this special logic is, as far
as I know, Goodix specific. I'm not aware of other touchscreens
holding themselves in a high power state if they are powered while
their reset line is held low. I don't think upstream would have liked
a DT properly like "avoid-holding-reset-low-while-powered;"
Ironically, there is actually more work to be done here. It turns out
that a different Chromebook that I wasn't aware of (and that wasn't
upstream yet) actually was relying on behavior to not assert reset and
we still need to figure out how to reconcile all of this. :(
I guess in general the idea of combining the drivers vs. coming up
with generic bindings is actually two separate things. We could have
separate bindings and still have one driver. At the time I made
i2c-of-goodix I was specifically requested to make separate drivers
for it. If maintainers want to re-combine them now, I won't object.
...but at least at the time it was a conscious decision and a specific
request to make them separate.
Looking at i2c-of-goodix and i2c-of-elan, we could probably combine
_those_ two drivers at this point, unless we actually end up needing
to go back again and do something goodix-specific for the reset line
again.
-Doug
^ permalink raw reply
* Re: [PATCH 0/6] HID: i2c-hid-of: Allow using i2c-hid-of on non OF platforms + remove specialized drivers
From: Benjamin Tissoires @ 2023-04-12 17:18 UTC (permalink / raw)
To: Hans de Goede; +Cc: Jiri Kosina, Douglas Anderson, linux-input
In-Reply-To: <130ab0b0-6685-1707-3f82-1540958b2389@redhat.com>
On Apr 11 2023, Hans de Goede wrote:
> Hi Benjamin,
>
> On 4/11/23 18:56, Benjamin Tissoires wrote:
> > On Apr 11 2023, Hans de Goede wrote:
> >> Hi Benjamin,
> >>
> >> On 4/11/23 14:50, Benjamin Tissoires wrote:
> >>> On Apr 11 2023, Hans de Goede wrote:
> >>>> Hi Benjamin,
> >>>>
> >>>> On 4/11/23 11:02, Benjamin Tissoires wrote:
>
> <snip>
>
> >>>>> Also, the problem of "post-reset-deassert-delay-ms" is that you are not
> >>>>> documenting it, because the OF folks do not want this in device tree,
> >>>>> and I tend to agree with them. So this basically creates a brand new
> >>>>> undocumented property, which is less than ideal.
> >>>>
> >>>> I'm merely not documenting it because there are no devicetree users yet.
> >>>
> >>> AFAIU, the non devicetree properties should also be documented through
> >>> DT bindings, no? So not documenting feels very much like "I want to slip
> >>> this one in without having to deal with DT maintainers" (and before you
> >>> take it personaly, I know this is definitively not the intent). So I'd
> >>> rather much having a public API documented, even if there are no users.
> >>
> >> Right, so as a hobby I have a tendency to work on these somewhat niche/weird
> >> x86 devices, like x86 tablets which use Android as factory OS :)
> >>
> >> As such I have encountered the need for device-properties to pass info
> >> from drivers/platform/x86 code to more generic drivers a number of
> >> times before.
> >>
> >> Each time this happens, if I try to add them to bindings I get
> >> asked for some example devicetree code, I then respond that these
> >> are *device*-properties not of-properties and that there are no
> >> current devicetree users after which the DT maintainers tell me
> >> to then NOT document them in the DT bindings, at least not until
> >> actually DT users show up. I fully expect any attempt do add
> >> this to the DT bindings to go the same way.
> >>
> >> And now I have you telling me you really want to see this
> >> documented at the same time as it getting implemented. Which
> >> I fully understand, but does leads to a bit of a catch 22.
> >
> > Ouch. Sorry for that.
>
> No problem.
>
> > Then I guess if the DT maintainers have a tendency
> > to accept those hidden properties, this is the simplest solution from a
> > i2c-hid/HID maintainer point of view, no?
>
> Yes, I believe so, which is why I went this route in the first place.
>
> > It's going to be a pain for the
> > platform driver because you still have to hardcode those properties
> > somewhere I guess.
>
> Since the entire description is missing in ACPI for the digitizer (*)
> the x86-android-tablets.ko module which contains glue code to support
> these x86 android tablets already contains the i2c-busnumber,
> i2c-address, GPIO lookups, IRQ and other necessary device-properties
> like "hid-descr-addr", so adding one more device-property is very little
> trouble.
>
> *) and also for other devices both on this and other x86 android tablets
>
> <snip>
>
> >> So I see 2 options here:
> >>
> >> 1. Take the approach from patches 1-4 here, but drop the property and
> >> use match data on a new "wacom0169" i2c_device_id instead.
> >> This would also pave the way to merging patches 5 + 6 once tested
> >> by google to reduce some code duplication. Although you write below
> >> you would prefer to keep these around as example code for other
> >> specialized drivers...
> >>
> >> 2. Add a new specialized i2c-hid-of-wacom driver for this.
> >> Question: Since this will be using i2c_device_id binding not
> >> DT/OF binding the -of- in the name is technically incorrect,
> >> but it would be consistent with the other specialized drivers
> >> and could be seen as preparation (avoiding a rename/confusion)
> >> for when any DT enumerated devices which need special handling
> >> show up (note only relevant if you prefer this approach).
> >
> > Well, option 2 is probably too much work for little gain. So I would go
> > with option 1, but with the following questions:
> >
> > - a device property is public, so it can be seen as public API, right?
> > So should we document it some way (not through DT) so we "guarantee"
> > some behavior for it?
>
> I believe the whole idea from the DT maintainers behind not documenting
> it as DT binding when not actually used in dts files is to keep it as
> in kernel *private* API, in this case between the x86-android-tablets.ko
> code instantiating the i2c_client and the i2c-hid code consuming it.
>
> Take the hideep touchscreen on this same tablet for example. After
> this patch series we could also use it in i2c-hid mode (I did test
> that as an extra test for patches 1-3) but the stock Android uses
> it in its native hideep protocol mode which gives some more info
> (ABS_MT_PRESSURE and ABS_MT_TOUCH_MAJOR). So currently in -next
> the touchscreen is driven in its native mode. This requires sending
> a command to (re)set it to native mode since it comes up in i2c-hid
> mode by default.
>
> This command is only send if a device-property is set (to avoid
> causing issues on other hideep models) and the code consuming
> that property looks like this:
>
> /*
> * Reset touch report format to the native HiDeep 20 protocol if requested.
> * This is necessary to make touchscreens which come up in I2C-HID mode
> * work with this driver.
> *
> * Note this is a kernel internal device-property set by x86 platform code,
> * this MUST not be used in devicetree files without first adding it to
> * the DT bindings.
> */
> if (device_property_read_bool(&ts->client->dev, "hideep,force-native-protocol"))
> regmap_write(ts->reg, HIDEEP_WORK_MODE, 0x00);
>
> So maybe copy that and just add a:
>
> /*
> * Note this is a kernel internal device-property set by x86 platform code,
> * this MUST not be used in devicetree files without first adding it to
> * the DT bindings.
> */
>
> Comment to the code reading the "post-reset-deassert-delay-ms"
> property (patch 3/6) for v2 of this patch-set and leave it
> at that ?
This seems like a better compromised :)
>
> (and in hindsight I should have added that comment for v1 already)
>
> > If the above is correct, then that means that the device property can
> > be used, which makes little changes to your series.
>
> Sounds good to me.
>
> > But then, why aren't you using that property directly for those 2 other
> > drivers? Can't we have elan and goodix i2c-hid-of variants, be just a
> > stub around adding the gpio names and the specific reset? (A plain "this
> > is completely dumb" answer is fine, just trying to get my head around it).
>
> Only 1 driver can bind to an i2c_client, and if those stub drivers
> bind to it, then they must deal with it, or they would need to
> create some fake i2c_client and pass everything through, but that
> would be rather ugly.
I was thinking that the i2c-hid-elan driver would override the property,
but that is assuming a driver can change a property once the device is
created, which I am now unsure.
>
> > So, given the above, and your experience with the DT maintainers, I
> > would go with patches 1-3 + a documentation of the new property, likely
> > in the header or in kernel docs.
>
> I'm fine with going with just patches 1-3. With patch 3 updated to
> add the "this is a kernel internal only property" comment.
Sounds like a good plan :)
>
> > Patches 4-6 either dropped, reworked, or left as they are, and we would
> > merge them only if the maintainers of those files tested the changes.
>
> Patches 4-6 were meant to make adding support for more
> i2c-hid-of devices in the future easier, nothing more nothing
> less. So I'm fine with dropping them.
>
> I agree that at a minimum they should get tested before
> merging them.
We can keep them in a separate series, and wiat until we get some tests
on them before merging them, yes.
Cheers,
Benjamin
^ permalink raw reply
* Re: [PATCH] HID: intel-ish-hid: pci-ish: Fix use after free bug in ish_remove due to race condition
From: Zheng Hacker @ 2023-04-12 17:18 UTC (permalink / raw)
To: Zheng Wang
Cc: srinivas.pandruvada, jikos, benjamin.tissoires, rafael, hdegoede,
gregkh, linux-input, linux-kernel, 1395428693sheep, alex000young
In-Reply-To: <20230412171441.18958-1-zyytlz.wz@163.com>
Hi,
I missed some detail in the patch. bh_hbm_work_fn will call
ishtp_hbm_dispatch and access &dev->fw_clients in it.
Best regards,
Zheng
Zheng Wang <zyytlz.wz@163.com> 于2023年4月13日周四 01:17写道：
>
> In ish_probe, it calls ish_dev_init to init the device. In this function,
> ishtp_device_init is called and &dev->bh_hbm_work is bound with
> bh_hbm_work_fn. recv_hbm may be called to start the timer work.
>
> If we remove the module which will call ish_remove to make cleanup,
> there may be an unfinished work. The possible sequence is as follows:
>
> Fix it by canceling the work before cleanup in ishtp_bus_remove_all_clients
>
> CPU0 CPUc1
>
> |bh_hbm_work_fn
> ish_remove |
> ishtp_bus_remove_all_clients |
> kfree(ishtp_dev->fw_clients); |
> |
> |&dev->fw_clients[...]
> | //use
>
> Fixes: 3703f53b99e4 ("HID: intel_ish-hid: ISH Transport layer")
> Signed-off-by: Zheng Wang <zyytlz.wz@163.com>
> ---
> drivers/hid/intel-ish-hid/ishtp/bus.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/hid/intel-ish-hid/ishtp/bus.c b/drivers/hid/intel-ish-hid/ishtp/bus.c
> index 81385ab37fa9..ada7cd08dbeb 100644
> --- a/drivers/hid/intel-ish-hid/ishtp/bus.c
> +++ b/drivers/hid/intel-ish-hid/ishtp/bus.c
> @@ -744,6 +744,7 @@ void ishtp_bus_remove_all_clients(struct ishtp_device *ishtp_dev,
> */
> }
> spin_unlock_irqrestore(&ishtp_dev->cl_list_lock, flags);
> + cancel_work_sync(&ishtp_dev->bh_hbm_work);
>
> /* Release DMA buffers for client messages */
> ishtp_cl_free_dma_buf(ishtp_dev);
> --
> 2.25.1
>
^ permalink raw reply
* [PATCH] HID: intel-ish-hid: pci-ish: Fix use after free bug in ish_remove due to race condition
From: Zheng Wang @ 2023-04-12 17:14 UTC (permalink / raw)
To: srinivas.pandruvada
Cc: jikos, benjamin.tissoires, rafael, hdegoede, gregkh, linux-input,
linux-kernel, hackerzheng666, 1395428693sheep, alex000young,
Zheng Wang
In ish_probe, it calls ish_dev_init to init the device. In this function,
ishtp_device_init is called and &dev->bh_hbm_work is bound with
bh_hbm_work_fn. recv_hbm may be called to start the timer work.
If we remove the module which will call ish_remove to make cleanup,
there may be an unfinished work. The possible sequence is as follows:
Fix it by canceling the work before cleanup in ishtp_bus_remove_all_clients
CPU0 CPUc1
|bh_hbm_work_fn
ish_remove |
ishtp_bus_remove_all_clients |
kfree(ishtp_dev->fw_clients); |
|
|&dev->fw_clients[...]
| //use
Fixes: 3703f53b99e4 ("HID: intel_ish-hid: ISH Transport layer")
Signed-off-by: Zheng Wang <zyytlz.wz@163.com>
---
drivers/hid/intel-ish-hid/ishtp/bus.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/hid/intel-ish-hid/ishtp/bus.c b/drivers/hid/intel-ish-hid/ishtp/bus.c
index 81385ab37fa9..ada7cd08dbeb 100644
--- a/drivers/hid/intel-ish-hid/ishtp/bus.c
+++ b/drivers/hid/intel-ish-hid/ishtp/bus.c
@@ -744,6 +744,7 @@ void ishtp_bus_remove_all_clients(struct ishtp_device *ishtp_dev,
*/
}
spin_unlock_irqrestore(&ishtp_dev->cl_list_lock, flags);
+ cancel_work_sync(&ishtp_dev->bh_hbm_work);
/* Release DMA buffers for client messages */
ishtp_cl_free_dma_buf(ishtp_dev);
--
2.25.1
^ permalink raw reply related
* RE: [PATCH 2/2] input: imx_sc_key: add wakeup support
From: Peng Fan @ 2023-04-12 15:58 UTC (permalink / raw)
To: Dmitry Torokhov, Ulf Hansson
Cc: robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
shawnguo@kernel.org, Aisheng Dong, linux-input@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, s.hauer@pengutronix.de,
kernel@pengutronix.de, festevam@gmail.com, dl-linux-imx
In-Reply-To: <DU0PR04MB94172C2BBB554E472576B2BA889B9@DU0PR04MB9417.eurprd04.prod.outlook.com>
+Ulf
> Subject: RE: [PATCH 2/2] input: imx_sc_key: add wakeup support
>
> > Subject: Re: [PATCH 2/2] input: imx_sc_key: add wakeup support
> >
> > On Thu, Mar 23, 2023 at 05:31:41PM +0800, Peng Fan (OSS) wrote:
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > Add support for waking up from system wide suspend.
> > >
> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > ---
> > > drivers/input/keyboard/imx_sc_key.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/input/keyboard/imx_sc_key.c
> > b/drivers/input/keyboard/imx_sc_key.c
> > > index d18839f1f4f6..234f23cf9990 100644
> > > --- a/drivers/input/keyboard/imx_sc_key.c
> > > +++ b/drivers/input/keyboard/imx_sc_key.c
> > > @@ -151,6 +151,8 @@ static int imx_sc_key_probe(struct
> > platform_device *pdev)
> > > priv->input = input;
> > > platform_set_drvdata(pdev, priv);
> > >
> > > + device_init_wakeup(&pdev->dev,
> > device_property_read_bool(&pdev->dev, "wakeup-source"));
> > > +
> >
> > I wonder - could we move this to the device core?
>
> I see lots device drivers parse wakeup-source, so I also follow That. Not sure
> whether could move this feature to device core, but anyway I could give a
> try.
Do you think it is feasible to move device_init_wakeup into device core
part?
Thanks,
Peng.
>
> Thanks,
> Peng.
> >
> > > error = imx_scu_irq_group_enable(SC_IRQ_GROUP_WAKE,
> > SC_IRQ_BUTTON,
> > > true);
> > > if (error) {
> > > --
> > > 2.37.1
> > >
> >
> > Thanks.
> >
> > --
> > Dmitry
^ permalink raw reply
* Re: [PATCH 00/11] selftests: hid: import the tests from hid-tools
From: Benjamin Tissoires @ 2023-04-12 15:18 UTC (permalink / raw)
To: Roderick Colenbrander
Cc: Peter Hutterer, Jiri Kosina, Shuah Khan, linux-input,
linux-kselftest, linux-kernel, Candle Sun, Jose Torreguitar,
Roderick Colenbrander, Silvan Jegen, Kai-Heng Feng,
наб, Blaž Hrastnik, Jason Gerecke,
Nicolas Saenz Julienne
In-Reply-To: <CAEc3jaBaY4GAHTtXyCZGw=AoxfBQ_9-rxorrS0KwJjAFyN993A@mail.gmail.com>
On Wed, Apr 5, 2023 at 1:22 AM Roderick Colenbrander
<thunderbird2k@gmail.com> wrote:
>
> Hi Benjamin,
>
> I like the direction of bundling the tests with the kernel and should
> make it easier in case there are driver changes breaking tests as
> well.
>
> Signed-off-by: Roderick Colenbrander <roderick.colenbrander@sony.com>
Thanks everybody.
I have now pushed this series with the sony fixes from hid-tools in
the branch for-6.4/tests, in the hid tree.
Cheers,
Benjamin
>
> Thanks,
> Roderick Colenbrander
>
> On Mon, Apr 3, 2023 at 6:54 PM Peter Hutterer <peter.hutterer@who-t.net> wrote:
> >
> > On Mon, Apr 03, 2023 at 06:20:24PM +0200, Benjamin Tissoires wrote:
> > > On Feb 17 2023, Benjamin Tissoires wrote:
> > > > I have been running hid-tools for a while, but it was in its own
> > > > separate repository for multiple reasons. And the past few weeks
> > > > I finally managed to make the kernel tests in that repo in a
> > > > state where we can merge them in the kernel tree directly:
> > > >
> > > > - the tests run in ~2 to 3 minutes
> > > > - the tests are way more reliable than previously
> > > > - the tests are mostly self-contained now (to the exception
> > > > of the Sony ones)
> > > >
> > > > To be able to run the tests we need to use the latest release
> > > > of hid-tools, as this project still keeps the HID parsing logic
> > > > and is capable of generating the HID events.
> > > >
> > > > The series also ensures we can run the tests with vmtest.sh,
> > > > allowing for a quick development and test in the tree itself.
> > > >
> > > > This should allow us to require tests to be added to a series
> > > > when we see fit and keep them alive properly instead of having
> > > > to deal with 2 repositories.
> > > >
> > > > In Cc are all of the people who participated in the elaboration
> > > > of those tests, so please send back a signed-off-by for each
> > > > commit you are part of.
> > > >
> > > > This series applies on top of the for-6.3/hid-bpf branch, which
> > > > is the one that added the tools/testing/selftests/hid directory.
> > > > Given that this is unlikely this series will make the cut for
> > > > 6.3, we might just consider this series to be based on top of
> > > > the future 6.3-rc1.
> > > >
> > > > Cheers,
> > > > Benjamin
> > > >
> > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > > ---
> > >
> > > Jiri, do you mind if I push that code in the hid tree with the following
> > > changes:
> > > - Peter privately gave me his signed-off-by
> >
> > Apologies, this fell off my list after the initial ack in a meeting with
> > Benjamin. This time publicly:
> > Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
> > for the relevant commits.
> >
> > Cheers,
> > Peter
> >
> >
> > > - I included changes from https://gitlab.freedesktop.org/libevdev/hid-tools/-/merge_requests/143
> > > to fix the failing sony tests in v6.3
> > >
> > > I am not a big fan of sending a v2 because the ML are not happy with the
> > > amount of changes...
> > >
> > > Cheers,
> > > Benjamin
> > >
> > > > Benjamin Tissoires (11):
> > > > selftests: hid: make vmtest rely on make
> > > > selftests: hid: import hid-tools hid-core tests
> > > > selftests: hid: import hid-tools hid-gamepad tests
> > > > selftests: hid: import hid-tools hid-keyboards tests
> > > > selftests: hid: import hid-tools hid-mouse tests
> > > > selftests: hid: import hid-tools hid-multitouch and hid-tablets tests
> > > > selftests: hid: import hid-tools wacom tests
> > > > selftests: hid: import hid-tools hid-apple tests
> > > > selftests: hid: import hid-tools hid-ite tests
> > > > selftests: hid: import hid-tools hid-sony and hid-playstation tests
> > > > selftests: hid: import hid-tools usb-crash tests
> > > >
> > > > tools/testing/selftests/hid/Makefile | 12 +
> > > > tools/testing/selftests/hid/config | 11 +
> > > > tools/testing/selftests/hid/hid-apple.sh | 7 +
> > > > tools/testing/selftests/hid/hid-core.sh | 7 +
> > > > tools/testing/selftests/hid/hid-gamepad.sh | 7 +
> > > > tools/testing/selftests/hid/hid-ite.sh | 7 +
> > > > tools/testing/selftests/hid/hid-keyboard.sh | 7 +
> > > > tools/testing/selftests/hid/hid-mouse.sh | 7 +
> > > > tools/testing/selftests/hid/hid-multitouch.sh | 7 +
> > > > tools/testing/selftests/hid/hid-sony.sh | 7 +
> > > > tools/testing/selftests/hid/hid-tablet.sh | 7 +
> > > > tools/testing/selftests/hid/hid-usb_crash.sh | 7 +
> > > > tools/testing/selftests/hid/hid-wacom.sh | 7 +
> > > > tools/testing/selftests/hid/run-hid-tools-tests.sh | 28 +
> > > > tools/testing/selftests/hid/settings | 3 +
> > > > tools/testing/selftests/hid/tests/__init__.py | 2 +
> > > > tools/testing/selftests/hid/tests/base.py | 345 ++++
> > > > tools/testing/selftests/hid/tests/conftest.py | 81 +
> > > > .../selftests/hid/tests/descriptors_wacom.py | 1360 +++++++++++++
> > > > .../selftests/hid/tests/test_apple_keyboard.py | 440 +++++
> > > > tools/testing/selftests/hid/tests/test_gamepad.py | 209 ++
> > > > tools/testing/selftests/hid/tests/test_hid_core.py | 154 ++
> > > > .../selftests/hid/tests/test_ite_keyboard.py | 166 ++
> > > > tools/testing/selftests/hid/tests/test_keyboard.py | 485 +++++
> > > > tools/testing/selftests/hid/tests/test_mouse.py | 977 +++++++++
> > > > .../testing/selftests/hid/tests/test_multitouch.py | 2088 ++++++++++++++++++++
> > > > tools/testing/selftests/hid/tests/test_sony.py | 282 +++
> > > > tools/testing/selftests/hid/tests/test_tablet.py | 872 ++++++++
> > > > .../testing/selftests/hid/tests/test_usb_crash.py | 103 +
> > > > .../selftests/hid/tests/test_wacom_generic.py | 844 ++++++++
> > > > tools/testing/selftests/hid/vmtest.sh | 25 +-
> > > > 31 files changed, 8554 insertions(+), 10 deletions(-)
> > > > ---
> > > > base-commit: 2f7f4efb9411770b4ad99eb314d6418e980248b4
> > > > change-id: 20230217-import-hid-tools-tests-dc0cd4f3c8a8
> > > >
> > > > Best regards,
> > > > --
> > > > Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > >
> > >
>
^ permalink raw reply
* Re: [PATCH V3] dt-bindings: input: pwm-beeper: convert to dt schema
From: Rob Herring @ 2023-04-12 14:53 UTC (permalink / raw)
To: Peng Fan (OSS)
Cc: dmitry.torokhov, Peng Fan, linux-kernel, devicetree,
krzysztof.kozlowski+dt, robh+dt, s.hauer, linux-input
In-Reply-To: <20230407075259.1593739-1-peng.fan@oss.nxp.com>
On Fri, 07 Apr 2023 15:52:59 +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
>
> Convert the binding doc to dt schema, and also fixed the
> example from fixed-regulator to regulator-fixed.
>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>
> V3:
> Drop description for pwms
> Simplify description for amp-supply
> update beeper-hz range to 10-10000
>
> V2:
> License update
> Don't need to show providers
> Make example complete
> Decrease beeper hz
> Misc update
>
>
> .../devicetree/bindings/input/pwm-beeper.txt | 24 -----------
> .../devicetree/bindings/input/pwm-beeper.yaml | 41 +++++++++++++++++++
> 2 files changed, 41 insertions(+), 24 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/input/pwm-beeper.txt
> create mode 100644 Documentation/devicetree/bindings/input/pwm-beeper.yaml
>
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* RE: [PATCH 2/2] input: imx_sc_key: add wakeup support
From: Peng Fan @ 2023-04-12 12:19 UTC (permalink / raw)
To: Dmitry Torokhov, Peng Fan (OSS)
Cc: robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
shawnguo@kernel.org, Aisheng Dong, linux-input@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, s.hauer@pengutronix.de,
kernel@pengutronix.de, festevam@gmail.com, dl-linux-imx
In-Reply-To: <ZDN00vwyCOzFrDYt@google.com>
> Subject: Re: [PATCH 2/2] input: imx_sc_key: add wakeup support
>
> On Thu, Mar 23, 2023 at 05:31:41PM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > Add support for waking up from system wide suspend.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> > drivers/input/keyboard/imx_sc_key.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/input/keyboard/imx_sc_key.c
> b/drivers/input/keyboard/imx_sc_key.c
> > index d18839f1f4f6..234f23cf9990 100644
> > --- a/drivers/input/keyboard/imx_sc_key.c
> > +++ b/drivers/input/keyboard/imx_sc_key.c
> > @@ -151,6 +151,8 @@ static int imx_sc_key_probe(struct
> platform_device *pdev)
> > priv->input = input;
> > platform_set_drvdata(pdev, priv);
> >
> > + device_init_wakeup(&pdev->dev,
> device_property_read_bool(&pdev->dev, "wakeup-source"));
> > +
>
> I wonder - could we move this to the device core?
I see lots device drivers parse wakeup-source, so I also follow
That. Not sure whether could move this feature to device core,
but anyway I could give a try.
Thanks,
Peng.
>
> > error = imx_scu_irq_group_enable(SC_IRQ_GROUP_WAKE,
> SC_IRQ_BUTTON,
> > true);
> > if (error) {
> > --
> > 2.37.1
> >
>
> Thanks.
>
> --
> Dmitry
^ permalink raw reply
* Re: [PATCH] Input: cyttsp5 - fix sensing configuration data structure
From: Alistair @ 2023-04-12 11:06 UTC (permalink / raw)
To: hrdl, Linus Walleij, Dmitry Torokhov
Cc: linux-input, Linux Kernel Mailing List
In-Reply-To: <20230411211651.3791304-1-git@hrdl.eu>
On Wed, 12 Apr 2023, at 7:16 AM, hrdl wrote:
> Prior to this patch, the sensing configuration data was not parsed
> correctly, breaking detection of max_tch. The vendor driver includes
> this field. This change informs the driver about the correct maximum
> number of simultaneous touch inputs.
>
> Tested on a Pine64 PineNote with a modified touch screen controller
> firmware.
>
> Signed-off-by: hrdl <git@hrdl.eu>
Reviewed-by: Alistair Francis <alistair@alistair23.me>
Alistair
> ---
> drivers/input/touchscreen/cyttsp5.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/input/touchscreen/cyttsp5.c b/drivers/input/touchscreen/cyttsp5.c
> index 16caffa35dd9..30102cb80fac 100644
> --- a/drivers/input/touchscreen/cyttsp5.c
> +++ b/drivers/input/touchscreen/cyttsp5.c
> @@ -111,6 +111,7 @@ struct cyttsp5_sensing_conf_data_dev {
> __le16 max_z;
> u8 origin_x;
> u8 origin_y;
> + u8 panel_id;
> u8 btn;
> u8 scan_mode;
> u8 max_num_of_tch_per_refresh_cycle;
> --
> 2.39.2
>
>
^ permalink raw reply
* Re: [PATCH 1/2] Input: cs40l26: Support for CS40L26 Boosted Haptic Amplifier
From: Jeff LaBundy @ 2023-04-12 2:42 UTC (permalink / raw)
To: Charles Keepax
Cc: Fred Treven, dmitry.torokhov, ben.bright, james.ogletree, lee,
jdelvare, joel, cy_huang, rdunlap, eajames, ping.bai, msp, arnd,
bartosz.golaszewski, linux-kernel, linux-input, patches
In-Reply-To: <20230411092708.GX68926@ediswmail.ad.cirrus.com>
Hi Charles,
On Tue, Apr 11, 2023 at 09:27:08AM +0000, Charles Keepax wrote:
> On Mon, Apr 10, 2023 at 07:31:56PM -0500, Jeff LaBundy wrote:
> > On Mon, Apr 10, 2023 at 08:56:34AM +0000, Charles Keepax wrote:
> > > On Sat, Apr 08, 2023 at 10:44:39PM -0500, Jeff LaBundy wrote:
> > > I would far rather not have every single attempt to communicate
> > > with the device wrapped in a retry if the communication failed
> > > incase the device is hibernating. It seems much cleaner, and less
> > > likely to risk odd behaviour, to know we have brought the device
> > > out of hibernation.
>
> > A common way to deal with this is that of [1], where the bus calls
> > are simply wrapped with all retry logic limited to two places (read
> > and write). These functions could also print the register address
> > in case of failure, solving the problem of having dozens of custom
> > error messages thorughout the driver.
>
> I suspect this really comes down to a matter of taste, but my
> thoughts would be that the code is shorter that way, but not
> necessarily simpler. This feels far more error prone and likely
> to encounter issues where the device hibernates at a time someone
> hadn't properly thought through. I am far more comfortable with
> the device is blocked from hibernating whilst the driver is
> actively engaged with it and it keeps any special handling for
> exiting hibernate in one place.
Fair enough. I do concede that having this control in the driver as
opposed to DSP FW is more nimble and makes it easier to respond to
customer issues; I'm sure your battle scars will agree :)
>
> > Does the current implementation at least allow the device to hibernate
> > while the system is otherwise active, as opposed to _only_ during
> > runtime suspend? If so, that's still a marked improvement from L25
> > era where customers rightfully pointed out that the downstream driver
> > was not making efficient use of hibernation. ;)
>
> I am not entirely sure I follow this one, yes the device can only
> hibernate whilst it is runtime suspended. But I don't understand
> why that is a problem being runtime resumed implies this device
> is active, not the system is otherwise active. I am not sure if
> I am missing your point or there is some confusion here between
> runtime and system suspend. The device can only hibernate during
> runtime suspend, but the only thing that determines being runtime
> resumed is activity on this device so in general it shouldn't be
> hibernating at that point anyway.
D'oh! I meant to say suspend suspend; I'm aligned.
>
> > I don't feel particularly strongly about it, so if the current
> > implementation will stay, perhaps consider a few comments in this
> > area to describe how the device's state is managed.
> >
>
> I certainly never object to adding some comments.
>
> Thanks,
> Charles
Kind regards,
Jeff LaBundy
^ permalink raw reply
* [PATCH] Input: cyttsp5 - fix sensing configuration data structure
From: hrdl @ 2023-04-11 21:16 UTC (permalink / raw)
To: Linus Walleij, Dmitry Torokhov
Cc: linux-input, linux-kernel, Alistair Francis, hrdl
Prior to this patch, the sensing configuration data was not parsed
correctly, breaking detection of max_tch. The vendor driver includes
this field. This change informs the driver about the correct maximum
number of simultaneous touch inputs.
Tested on a Pine64 PineNote with a modified touch screen controller
firmware.
Signed-off-by: hrdl <git@hrdl.eu>
---
drivers/input/touchscreen/cyttsp5.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/input/touchscreen/cyttsp5.c b/drivers/input/touchscreen/cyttsp5.c
index 16caffa35dd9..30102cb80fac 100644
--- a/drivers/input/touchscreen/cyttsp5.c
+++ b/drivers/input/touchscreen/cyttsp5.c
@@ -111,6 +111,7 @@ struct cyttsp5_sensing_conf_data_dev {
__le16 max_z;
u8 origin_x;
u8 origin_y;
+ u8 panel_id;
u8 btn;
u8 scan_mode;
u8 max_num_of_tch_per_refresh_cycle;
--
2.39.2
^ permalink raw reply related
* Re: [PATCH v2 5/5] arm64: dts: qcom: sdm845-shift-axolotl: update focaltech touchscreen properties
From: Konrad Dybcio @ 2023-04-11 19:59 UTC (permalink / raw)
To: Joel Selvaraj, Caleb Connolly, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Andy Gross, Bjorn Andersson, Henrik Rydberg,
Arnd Bergmann, Robert Jarzmik, Jeff LaBundy, Markuss Broks,
Jean Delvare, Job Noorman, Alistair Francis, Chris Morgan,
Hans de Goede
Cc: linux-input, devicetree, linux-kernel, linux-arm-msm,
~postmarketos/upstreaming, phone-devel
In-Reply-To: <20230410160200.57261-6-joelselvaraj.oss@gmail.com>
On 10.04.2023 18:02, Joel Selvaraj wrote:
> The touchscreen nodes were added before the driver patches were merged.
> Update the focaltech touchscreen properties to match with the upstreamed
> focaltech driver. Also, the touchscreen used is in axolotl is fts5452
> and not fts8719.
>
> Signed-off-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>
> ---
same comments as 4/5
Konrad
> arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts | 15 +++++++--------
> 1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts b/arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts
> index b54e304abf71..96dd4628d9a7 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts
> +++ b/arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts
> @@ -474,23 +474,22 @@ &i2c5 {
> status = "okay";
>
> touchscreen@38 {
> - compatible = "focaltech,fts8719";
> + compatible = "focaltech,fts5452";
> reg = <0x38>;
> - wakeup-source;
> +
> interrupt-parent = <&tlmm>;
> - interrupts = <125 0x2>;
> - vdd-supply = <&vreg_l28a_3p0>;
> - vcc-i2c-supply = <&vreg_l14a_1p88>;
> + interrupts = <125 IRQ_TYPE_EDGE_FALLING>;
> + reset-gpios = <&tlmm 99 GPIO_ACTIVE_LOW>;
> +
> + avdd-supply = <&vreg_l28a_3p0>;
> + vddio-supply = <&vreg_l14a_1p88>;
>
> pinctrl-names = "default", "suspend";
> pinctrl-0 = <&ts_int_active &ts_reset_active>;
> pinctrl-1 = <&ts_int_suspend &ts_reset_suspend>;
>
> - reset-gpio = <&tlmm 99 GPIO_ACTIVE_HIGH>;
> - irq-gpio = <&tlmm 125 GPIO_TRANSITORY>;
> touchscreen-size-x = <1080>;
> touchscreen-size-y = <2160>;
> - focaltech,max-touch-number = <5>;
> };
> };
>
^ permalink raw reply
* Re: [PATCH v2 4/5] arm64: dts: qcom: sdm845-xiaomi-beryllium-ebbg: introduce support for fts touchscreen
From: Konrad Dybcio @ 2023-04-11 19:58 UTC (permalink / raw)
To: Joel Selvaraj, Caleb Connolly, Dmitry Torokhov, Rob Herring,
Krzysztof Kozlowski, Andy Gross, Bjorn Andersson, Henrik Rydberg,
Arnd Bergmann, Robert Jarzmik, Jeff LaBundy, Markuss Broks,
Jean Delvare, Job Noorman, Alistair Francis, Chris Morgan,
Hans de Goede
Cc: linux-input, devicetree, linux-kernel, linux-arm-msm,
~postmarketos/upstreaming, phone-devel
In-Reply-To: <20230410160200.57261-5-joelselvaraj.oss@gmail.com>
On 10.04.2023 18:01, Joel Selvaraj wrote:
> The Poco F1 EBBG variant uses Focaltech FTS8719 touchscreen. Introduce
> support for it.
>
> Signed-off-by: Joel Selvaraj <joelselvaraj.oss@gmail.com>
> ---
> .../dts/qcom/sdm845-xiaomi-beryllium-ebbg.dts | 26 +++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-ebbg.dts b/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-ebbg.dts
> index 76931ebad065..f857ed3e2df4 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-ebbg.dts
> +++ b/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-ebbg.dts
> @@ -13,3 +13,29 @@ &display_panel {
> compatible = "ebbg,ft8719";
> status = "okay";
> };
> +
> +&i2c14 {
> + status = "okay";
> +
> + dmas = <&gpi_dma1 0 6 QCOM_GPI_I2C>,
> + <&gpi_dma1 1 6 QCOM_GPI_I2C>;
> + dma-names = "tx", "rx";
That's totally in sdm845.dtsi already!
> +
> + touchscreen@38 {
> + compatible = "focaltech,fts8719";
> + reg = <0x38>;
> +
> + interrupt-parent = <&tlmm>;
> + interrupts = <31 IRQ_TYPE_EDGE_FALLING>;
interrupts-extended
> + reset-gpios = <&tlmm 32 GPIO_ACTIVE_LOW>;
> +
> + vddio-supply = <&vreg_l14a_1p8>;
> +
> + pinctrl-names = "default", "sleep";
> + pinctrl-0 = <&ts_int_default &ts_reset_default>;
> + pinctrl-1 = <&ts_int_sleep &ts_reset_sleep>;
property
property-names
Konrad
> +
> + touchscreen-size-x = <1080>;
> + touchscreen-size-y = <2246>;
> + };
> +};
^ permalink raw reply
* [PATCH] HID: kye: Fix rdesc for kye tablets
From: David Yang @ 2023-04-11 17:33 UTC (permalink / raw)
To: linux-input; +Cc: David Yang, Jiri Kosina, Benjamin Tissoires, linux-kernel
I forget to add them in previous commit 2dd438cdc2e9
("HID: kye: Add support for all kye tablets").
Signed-off-by: David Yang <mmyangfl@gmail.com>
---
drivers/hid/hid-kye.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/hid-kye.c b/drivers/hid/hid-kye.c
index b3bde4bb979d..eb9bf2829937 100644
--- a/drivers/hid/hid-kye.c
+++ b/drivers/hid/hid-kye.c
@@ -209,7 +209,7 @@ static const __u8 pensketch_t609a_control_rdesc[] = {
0xC0 /* End Collection */
};
-/* Fix indexes in kye_tablet_report_fixup if you change this */
+/* Fix indexes in kye_tablet_fixup if you change this */
static const __u8 kye_tablet_rdesc[] = {
0x06, 0x00, 0xFF, /* Usage Page (FF00h), */
0x09, 0x01, /* Usage (01h), */
@@ -493,12 +493,19 @@ static __u8 *kye_report_fixup(struct hid_device *hdev, __u8 *rdesc,
rdesc = kye_consumer_control_fixup(hdev, rdesc, rsize, 83,
"Genius Gx Imperator Keyboard");
break;
+ case USB_DEVICE_ID_KYE_EASYPEN_M406:
+ case USB_DEVICE_ID_KYE_EASYPEN_M506:
case USB_DEVICE_ID_KYE_EASYPEN_I405X:
case USB_DEVICE_ID_KYE_MOUSEPEN_I608X:
- case USB_DEVICE_ID_KYE_MOUSEPEN_I608X_V2:
+ case USB_DEVICE_ID_KYE_EASYPEN_M406W:
case USB_DEVICE_ID_KYE_EASYPEN_M610X:
- case USB_DEVICE_ID_KYE_EASYPEN_M406XE:
+ case USB_DEVICE_ID_KYE_EASYPEN_340:
case USB_DEVICE_ID_KYE_PENSKETCH_M912:
+ case USB_DEVICE_ID_KYE_MOUSEPEN_M508WX:
+ case USB_DEVICE_ID_KYE_MOUSEPEN_M508X:
+ case USB_DEVICE_ID_KYE_EASYPEN_M406XE:
+ case USB_DEVICE_ID_KYE_MOUSEPEN_I608X_V2:
+ case USB_DEVICE_ID_KYE_PENSKETCH_T609A:
rdesc = kye_tablet_fixup(hdev, rdesc, rsize);
break;
}
--
2.39.2
^ permalink raw reply related
* Re: [PATCH 0/6] HID: i2c-hid-of: Allow using i2c-hid-of on non OF platforms + remove specialized drivers
From: Hans de Goede @ 2023-04-11 17:28 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: Jiri Kosina, Douglas Anderson, linux-input
In-Reply-To: <20230411165653.rw3ivfjdhjv5dmc3@mail.corp.redhat.com>
Hi Benjamin,
On 4/11/23 18:56, Benjamin Tissoires wrote:
> On Apr 11 2023, Hans de Goede wrote:
>> Hi Benjamin,
>>
>> On 4/11/23 14:50, Benjamin Tissoires wrote:
>>> On Apr 11 2023, Hans de Goede wrote:
>>>> Hi Benjamin,
>>>>
>>>> On 4/11/23 11:02, Benjamin Tissoires wrote:
<snip>
>>>>> Also, the problem of "post-reset-deassert-delay-ms" is that you are not
>>>>> documenting it, because the OF folks do not want this in device tree,
>>>>> and I tend to agree with them. So this basically creates a brand new
>>>>> undocumented property, which is less than ideal.
>>>>
>>>> I'm merely not documenting it because there are no devicetree users yet.
>>>
>>> AFAIU, the non devicetree properties should also be documented through
>>> DT bindings, no? So not documenting feels very much like "I want to slip
>>> this one in without having to deal with DT maintainers" (and before you
>>> take it personaly, I know this is definitively not the intent). So I'd
>>> rather much having a public API documented, even if there are no users.
>>
>> Right, so as a hobby I have a tendency to work on these somewhat niche/weird
>> x86 devices, like x86 tablets which use Android as factory OS :)
>>
>> As such I have encountered the need for device-properties to pass info
>> from drivers/platform/x86 code to more generic drivers a number of
>> times before.
>>
>> Each time this happens, if I try to add them to bindings I get
>> asked for some example devicetree code, I then respond that these
>> are *device*-properties not of-properties and that there are no
>> current devicetree users after which the DT maintainers tell me
>> to then NOT document them in the DT bindings, at least not until
>> actually DT users show up. I fully expect any attempt do add
>> this to the DT bindings to go the same way.
>>
>> And now I have you telling me you really want to see this
>> documented at the same time as it getting implemented. Which
>> I fully understand, but does leads to a bit of a catch 22.
>
> Ouch. Sorry for that.
No problem.
> Then I guess if the DT maintainers have a tendency
> to accept those hidden properties, this is the simplest solution from a
> i2c-hid/HID maintainer point of view, no?
Yes, I believe so, which is why I went this route in the first place.
> It's going to be a pain for the
> platform driver because you still have to hardcode those properties
> somewhere I guess.
Since the entire description is missing in ACPI for the digitizer (*)
the x86-android-tablets.ko module which contains glue code to support
these x86 android tablets already contains the i2c-busnumber,
i2c-address, GPIO lookups, IRQ and other necessary device-properties
like "hid-descr-addr", so adding one more device-property is very little
trouble.
*) and also for other devices both on this and other x86 android tablets
<snip>
>> So I see 2 options here:
>>
>> 1. Take the approach from patches 1-4 here, but drop the property and
>> use match data on a new "wacom0169" i2c_device_id instead.
>> This would also pave the way to merging patches 5 + 6 once tested
>> by google to reduce some code duplication. Although you write below
>> you would prefer to keep these around as example code for other
>> specialized drivers...
>>
>> 2. Add a new specialized i2c-hid-of-wacom driver for this.
>> Question: Since this will be using i2c_device_id binding not
>> DT/OF binding the -of- in the name is technically incorrect,
>> but it would be consistent with the other specialized drivers
>> and could be seen as preparation (avoiding a rename/confusion)
>> for when any DT enumerated devices which need special handling
>> show up (note only relevant if you prefer this approach).
>
> Well, option 2 is probably too much work for little gain. So I would go
> with option 1, but with the following questions:
>
> - a device property is public, so it can be seen as public API, right?
> So should we document it some way (not through DT) so we "guarantee"
> some behavior for it?
I believe the whole idea from the DT maintainers behind not documenting
it as DT binding when not actually used in dts files is to keep it as
in kernel *private* API, in this case between the x86-android-tablets.ko
code instantiating the i2c_client and the i2c-hid code consuming it.
Take the hideep touchscreen on this same tablet for example. After
this patch series we could also use it in i2c-hid mode (I did test
that as an extra test for patches 1-3) but the stock Android uses
it in its native hideep protocol mode which gives some more info
(ABS_MT_PRESSURE and ABS_MT_TOUCH_MAJOR). So currently in -next
the touchscreen is driven in its native mode. This requires sending
a command to (re)set it to native mode since it comes up in i2c-hid
mode by default.
This command is only send if a device-property is set (to avoid
causing issues on other hideep models) and the code consuming
that property looks like this:
/*
* Reset touch report format to the native HiDeep 20 protocol if requested.
* This is necessary to make touchscreens which come up in I2C-HID mode
* work with this driver.
*
* Note this is a kernel internal device-property set by x86 platform code,
* this MUST not be used in devicetree files without first adding it to
* the DT bindings.
*/
if (device_property_read_bool(&ts->client->dev, "hideep,force-native-protocol"))
regmap_write(ts->reg, HIDEEP_WORK_MODE, 0x00);
So maybe copy that and just add a:
/*
* Note this is a kernel internal device-property set by x86 platform code,
* this MUST not be used in devicetree files without first adding it to
* the DT bindings.
*/
Comment to the code reading the "post-reset-deassert-delay-ms"
property (patch 3/6) for v2 of this patch-set and leave it
at that ?
(and in hindsight I should have added that comment for v1 already)
> If the above is correct, then that means that the device property can
> be used, which makes little changes to your series.
Sounds good to me.
> But then, why aren't you using that property directly for those 2 other
> drivers? Can't we have elan and goodix i2c-hid-of variants, be just a
> stub around adding the gpio names and the specific reset? (A plain "this
> is completely dumb" answer is fine, just trying to get my head around it).
Only 1 driver can bind to an i2c_client, and if those stub drivers
bind to it, then they must deal with it, or they would need to
create some fake i2c_client and pass everything through, but that
would be rather ugly.
> So, given the above, and your experience with the DT maintainers, I
> would go with patches 1-3 + a documentation of the new property, likely
> in the header or in kernel docs.
I'm fine with going with just patches 1-3. With patch 3 updated to
add the "this is a kernel internal only property" comment.
> Patches 4-6 either dropped, reworked, or left as they are, and we would
> merge them only if the maintainers of those files tested the changes.
Patches 4-6 were meant to make adding support for more
i2c-hid-of devices in the future easier, nothing more nothing
less. So I'm fine with dropping them.
I agree that at a minimum they should get tested before
merging them.
Regards,
Hans
^ permalink raw reply
* Re: [PATCH 0/6] HID: i2c-hid-of: Allow using i2c-hid-of on non OF platforms + remove specialized drivers
From: Benjamin Tissoires @ 2023-04-11 16:56 UTC (permalink / raw)
To: Hans de Goede; +Cc: Jiri Kosina, Douglas Anderson, linux-input
In-Reply-To: <eafe8744-49d4-31e3-2329-ddd452358915@redhat.com>
On Apr 11 2023, Hans de Goede wrote:
> Hi Benjamin,
>
> On 4/11/23 14:50, Benjamin Tissoires wrote:
> > On Apr 11 2023, Hans de Goede wrote:
> >> Hi Benjamin,
> >>
> >> On 4/11/23 11:02, Benjamin Tissoires wrote:
> >>> Hi Hans,
> >>>
> >>> On Apr 09 2023, Hans de Goede wrote:
> >>>> Hi All,
> >>>>
> >>>> This series consist of 2 parts:
> >>>>
> >>>> 1. Patches 1-3. Allow using i2c-hid-of on non OF platforms to allow I2C-HID
> >>>> devices which are not enumerated by ACPI to work on ACPI platforms
> >>>> (by manual i2c_client instantiation using i2c_client_id matching).
> >>>
> >>> Patches 1 and 2 are looking good, but I wonder if you can not achieve the
> >>> same result by relying on an ACPI SSDT override. I got something similar
> >>> working on this thread[0].
> >>
> >> Yes this could be made to work with an ACPI override. But the goal is
> >> to make things work OOTB for end users when they install Linux and
> >> ACPI overrides are very far from something which works OOTB.
> >
> > Fair enough.
> >
> >>
> >>> I understand the "post-reset-deassert-delay-ms" might be something hard
> >>> to express with an SSDT, but we should already have all the bits in
> >>> place, no?
> >>
> >> Actually if an ACPI override is used then the setting of the GPIO
> >> can be done in _PS0 and _PS3 (power on / off) methods and those
> >> can simply include a sleep after setting the GPIO.
> >
> > Though this is all conditional if we can make ACPI SSDT override
> > something that can be shipped while installing the device...
> >
> >>
> >>> Also, the problem of "post-reset-deassert-delay-ms" is that you are not
> >>> documenting it, because the OF folks do not want this in device tree,
> >>> and I tend to agree with them. So this basically creates a brand new
> >>> undocumented property, which is less than ideal.
> >>
> >> I'm merely not documenting it because there are no devicetree users yet.
> >
> > AFAIU, the non devicetree properties should also be documented through
> > DT bindings, no? So not documenting feels very much like "I want to slip
> > this one in without having to deal with DT maintainers" (and before you
> > take it personaly, I know this is definitively not the intent). So I'd
> > rather much having a public API documented, even if there are no users.
>
> Right, so as a hobby I have a tendency to work on these somewhat niche/weird
> x86 devices, like x86 tablets which use Android as factory OS :)
>
> As such I have encountered the need for device-properties to pass info
> from drivers/platform/x86 code to more generic drivers a number of
> times before.
>
> Each time this happens, if I try to add them to bindings I get
> asked for some example devicetree code, I then respond that these
> are *device*-properties not of-properties and that there are no
> current devicetree users after which the DT maintainers tell me
> to then NOT document them in the DT bindings, at least not until
> actually DT users show up. I fully expect any attempt do add
> this to the DT bindings to go the same way.
>
> And now I have you telling me you really want to see this
> documented at the same time as it getting implemented. Which
> I fully understand, but does leads to a bit of a catch 22.
Ouch. Sorry for that. Then I guess if the DT maintainers have a tendency
to accept those hidden properties, this is the simplest solution from a
i2c-hid/HID maintainer point of view, no? It's going to be a pain for the
platform driver because you still have to hardcode those properties
somewhere I guess.
>
> Anyways lets just go with the alternative of treating this
> similar as the existing specialized drivers, see below.
>
> <snip>
>
> >> Note if just the existence of the property is the main stumbling
> >> block I can go the match_data route for the wacom digitizer on
> >> the Yoga Book 1 too and add an extra i2c_device_id with match-data
> >> setting the delay. This could then either be its own specialized
> >> driver, or we could still go with the current patch-set
> >> (minus the property) and add an i2c_device_id with match-data
> >> to i2c-hid-of.c .
> >
> > I'd much rather have a i2c-hid-of.c internal API, yes. Whether it's a
> > function call, a callback or a match-data (or a driver-data), this is
> > something we are in control and we can change.
>
> Ok.
>
> So I see 2 options here:
>
> 1. Take the approach from patches 1-4 here, but drop the property and
> use match data on a new "wacom0169" i2c_device_id instead.
> This would also pave the way to merging patches 5 + 6 once tested
> by google to reduce some code duplication. Although you write below
> you would prefer to keep these around as example code for other
> specialized drivers...
>
> 2. Add a new specialized i2c-hid-of-wacom driver for this.
> Question: Since this will be using i2c_device_id binding not
> DT/OF binding the -of- in the name is technically incorrect,
> but it would be consistent with the other specialized drivers
> and could be seen as preparation (avoiding a rename/confusion)
> for when any DT enumerated devices which need special handling
> show up (note only relevant if you prefer this approach).
Well, option 2 is probably too much work for little gain. So I would go
with option 1, but with the following questions:
- a device property is public, so it can be seen as public API, right?
So should we document it some way (not through DT) so we "guarantee"
some behavior for it?
If the above is correct, then that means that the device property can
be used, which makes little changes to your series.
But then, why aren't you using that property directly for those 2 other
drivers? Can't we have elan and goodix i2c-hid-of variants, be just a
stub around adding the gpio names and the specific reset? (A plain "this
is completely dumb" answer is fine, just trying to get my head around it).
So, given the above, and your experience with the DT maintainers, I
would go with patches 1-3 + a documentation of the new property, likely
in the header or in kernel docs.
Patches 4-6 either dropped, reworked, or left as they are, and we would
merge them only if the maintainers of those files tested the changes.
And if you prefer storing the post-reset delay in the hid tree, that's
fine too, but I guess you would prefer having less friction by keeping
it in the platform tree.
>
> Either way is fine with me really. So you get to chose. If you
> let me know which route you prefer, I'll go and prepare either
> a v2 of this series, or a whole new patch for the new specialized
> driver.
Sorry for being a PITA, but having those driver separated allowed to
move forward without having to have a spaghetti plate in i2c-hid, which
was the case before the split (because *everything* was entangled: ACPI,
DT, OF, properties). So that's why I'm trying to understand and
minimize the changes.
Also, before you sending v2 and involving too much, we could try to wait a
few days for Doug to answer, and hear if he has an opinion. But if you
rather send v2 right away, that's your choice obviously :)
Cheers,
Benjamin
>
> Regards,
>
> Hans
>
>
^ permalink raw reply
* [PATCH] HID: amd_sfh: Support for additional light sensor
From: Basavaraj Natikar @ 2023-04-11 16:19 UTC (permalink / raw)
To: jikos, benjamin.tissoires, linux-input; +Cc: Basavaraj Natikar
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>
---
drivers/hid/amd-sfh-hid/amd_sfh_client.c | 1 +
drivers/hid/amd-sfh-hid/amd_sfh_pcie.c | 4 ++++
drivers/hid/amd-sfh-hid/amd_sfh_pcie.h | 1 +
drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_desc.c | 4 ++++
4 files changed, 10 insertions(+)
diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_client.c b/drivers/hid/amd-sfh-hid/amd_sfh_client.c
index c751d12f5df8..d9b7b01900b5 100644
--- a/drivers/hid/amd-sfh-hid/amd_sfh_client.c
+++ b/drivers/hid/amd-sfh-hid/amd_sfh_client.c
@@ -147,6 +147,7 @@ static const char *get_sensor_name(int idx)
case mag_idx:
return "magnetometer";
case als_idx:
+ case ACS_IDX: /* ambient color sensor */
return "ALS";
case HPD_IDX:
return "HPD";
diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
index 47774b9ab3de..f37f817737f2 100644
--- a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
+++ b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
@@ -29,6 +29,7 @@
#define MAGNO_EN BIT(2)
#define HPD_EN BIT(16)
#define ALS_EN BIT(19)
+#define ACS_EN BIT(22)
static int sensor_mask_override = -1;
module_param_named(sensor_mask, sensor_mask_override, int, 0444);
@@ -233,6 +234,9 @@ int amd_mp2_get_sensor_num(struct amd_mp2_dev *privdata, u8 *sensor_id)
if (HPD_EN & activestatus)
sensor_id[num_of_sensors++] = HPD_IDX;
+ if (ACS_EN & activestatus)
+ sensor_id[num_of_sensors++] = ACS_IDX;
+
return num_of_sensors;
}
diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.h b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.h
index dfb7cabd82ef..70add75fc506 100644
--- a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.h
+++ b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.h
@@ -23,6 +23,7 @@
#define V2_STATUS 0x2
#define HPD_IDX 16
+#define ACS_IDX 22
#define SENSOR_DISCOVERY_STATUS_MASK GENMASK(5, 3)
#define SENSOR_DISCOVERY_STATUS_SHIFT 3
diff --git a/drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_desc.c b/drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_desc.c
index f9a8c02d5a7b..8716a05950c8 100644
--- a/drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_desc.c
+++ b/drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_desc.c
@@ -48,6 +48,7 @@ static int get_report_descriptor(int sensor_idx, u8 *rep_desc)
sizeof(comp3_report_descriptor));
break;
case als_idx: /* ambient light sensor */
+ case ACS_IDX: /* ambient color sensor */
memset(rep_desc, 0, sizeof(als_report_descriptor));
memcpy(rep_desc, als_report_descriptor,
sizeof(als_report_descriptor));
@@ -97,6 +98,7 @@ static u32 get_descr_sz(int sensor_idx, int descriptor_name)
}
break;
case als_idx:
+ case ACS_IDX: /* ambient color sensor */
switch (descriptor_name) {
case descr_size:
return sizeof(als_report_descriptor);
@@ -174,6 +176,7 @@ static u8 get_feature_report(int sensor_idx, int report_id, u8 *feature_report)
report_size = sizeof(magno_feature);
break;
case als_idx: /* ambient light sensor */
+ case ACS_IDX: /* ambient color sensor */
get_common_features(&als_feature.common_property, report_id);
als_feature.als_change_sesnitivity = HID_DEFAULT_SENSITIVITY;
als_feature.als_sensitivity_min = HID_DEFAULT_MIN_VALUE;
@@ -245,6 +248,7 @@ static u8 get_input_report(u8 current_index, int sensor_idx, int report_id,
report_size = sizeof(magno_input);
break;
case als_idx: /* Als */
+ case ACS_IDX: /* ambient color sensor */
get_common_inputs(&als_input.common_property, report_id);
/* For ALS ,V2 Platforms uses C2P_MSG5 register instead of DRAM access method */
if (supported_input == V2_STATUS)
--
2.25.1
^ permalink raw reply related
* [PATCH 7/7] HID: amd_sfh: Handle "no sensors" enabled for SFH1.1
From: Basavaraj Natikar @ 2023-04-11 16:10 UTC (permalink / raw)
To: jikos, benjamin.tissoires, linux-input; +Cc: Basavaraj Natikar
In-Reply-To: <20230411161030.909350-1-Basavaraj.Natikar@amd.com>
Based on num_hid_devices, each sensor device is initialized. If
"no sensors" is initialized, amd_sfh work initialization and scheduling
doesn’t make sense and returns EOPNOTSUPP to stop driver probe. Hence,
add a check for "no sensors" enabled to handle the special case.
Fixes: 93ce5e0231d7 ("HID: amd_sfh: Implement SFH1.1 functionality")
Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
---
drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
index a1d6e08fab7d..bb8bd7892b67 100644
--- a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
+++ b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
@@ -112,6 +112,7 @@ static int amd_sfh1_1_hid_client_init(struct amd_mp2_dev *privdata)
cl_data->num_hid_devices = amd_sfh_get_sensor_num(privdata, &cl_data->sensor_idx[0]);
if (cl_data->num_hid_devices == 0)
return -ENODEV;
+ cl_data->is_any_sensor_enabled = false;
INIT_DELAYED_WORK(&cl_data->work, amd_sfh_work);
INIT_DELAYED_WORK(&cl_data->work_buffer, amd_sfh_work_buffer);
@@ -170,6 +171,7 @@ static int amd_sfh1_1_hid_client_init(struct amd_mp2_dev *privdata)
status = (status == 0) ? SENSOR_ENABLED : SENSOR_DISABLED;
if (status == SENSOR_ENABLED) {
+ cl_data->is_any_sensor_enabled = true;
cl_data->sensor_sts[i] = SENSOR_ENABLED;
rc = amdtp_hid_probe(i, cl_data);
if (rc) {
@@ -186,12 +188,21 @@ static int amd_sfh1_1_hid_client_init(struct amd_mp2_dev *privdata)
cl_data->sensor_sts[i]);
goto cleanup;
}
+ } else {
+ cl_data->sensor_sts[i] = SENSOR_DISABLED;
}
dev_dbg(dev, "sid 0x%x (%s) status 0x%x\n",
cl_data->sensor_idx[i], get_sensor_name(cl_data->sensor_idx[i]),
cl_data->sensor_sts[i]);
}
+ if (!cl_data->is_any_sensor_enabled) {
+ dev_warn(dev, "Failed to discover, sensors not enabled is %d\n",
+ cl_data->is_any_sensor_enabled);
+ rc = -EOPNOTSUPP;
+ goto cleanup;
+ }
+
schedule_delayed_work(&cl_data->work_buffer, msecs_to_jiffies(AMD_SFH_IDLE_LOOP));
return 0;
--
2.25.1
^ permalink raw reply related
* [PATCH 4/7] HID: amd_sfh: Add support for shutdown operation
From: Basavaraj Natikar @ 2023-04-11 16:10 UTC (permalink / raw)
To: jikos, benjamin.tissoires, linux-input; +Cc: Basavaraj Natikar
In-Reply-To: <20230411161030.909350-1-Basavaraj.Natikar@amd.com>
As soon as the system is booted after shutdown, the sensors may remain in
a weird state and fail to initialize. Therefore, all sensors should be
turned off during shutdown.
Fixes: 4f567b9f8141 ("SFH: PCIe driver to add support of AMD sensor fusion hub")
Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
---
drivers/hid/amd-sfh-hid/amd_sfh_pcie.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
index 47774b9ab3de..c936d6a51c0c 100644
--- a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
+++ b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
@@ -367,6 +367,14 @@ static int amd_mp2_pci_probe(struct pci_dev *pdev, const struct pci_device_id *i
return devm_add_action_or_reset(&pdev->dev, privdata->mp2_ops->remove, privdata);
}
+static void amd_sfh_shutdown(struct pci_dev *pdev)
+{
+ struct amd_mp2_dev *mp2 = pci_get_drvdata(pdev);
+
+ if (mp2 && mp2->mp2_ops)
+ mp2->mp2_ops->stop_all(mp2);
+}
+
static int __maybe_unused amd_mp2_pci_resume(struct device *dev)
{
struct amd_mp2_dev *mp2 = dev_get_drvdata(dev);
@@ -401,6 +409,7 @@ static struct pci_driver amd_mp2_pci_driver = {
.id_table = amd_mp2_pci_tbl,
.probe = amd_mp2_pci_probe,
.driver.pm = &amd_mp2_pm_ops,
+ .shutdown = amd_sfh_shutdown,
};
module_pci_driver(amd_mp2_pci_driver);
--
2.25.1
^ permalink raw reply related
* [PATCH 5/7] HID: amd_sfh: Correct the stop all command
From: Basavaraj Natikar @ 2023-04-11 16:10 UTC (permalink / raw)
To: jikos, benjamin.tissoires, linux-input; +Cc: Basavaraj Natikar
In-Reply-To: <20230411161030.909350-1-Basavaraj.Natikar@amd.com>
Misinterpreted the stop all command in SHF1.1 firmware. Therefore, it is
necessary to update the stop all command accordingly to disable all
sensors.
Fixes: 93ce5e0231d7 ("HID: amd_sfh: Implement SFH1.1 functionality")
Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
---
drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c
index 6e19ccc12450..6f6047f7f12e 100644
--- a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c
+++ b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c
@@ -58,8 +58,10 @@ static void amd_stop_all_sensor(struct amd_mp2_dev *privdata)
struct sfh_cmd_base cmd_base;
cmd_base.ul = 0;
- cmd_base.cmd.cmd_id = STOP_ALL_SENSORS;
+ cmd_base.cmd.cmd_id = DISABLE_SENSOR;
cmd_base.cmd.intr_disable = 0;
+ /* 0xf indicates all sensors */
+ cmd_base.cmd.sensor_id = 0xf;
writel(cmd_base.ul, privdata->mmio + AMD_C2P_MSG(0));
}
--
2.25.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox