* Re: [PATCH v5 13/17] platform/x86/amd/pmf: Add PMF-AMDGPU get interface
From: Shyam Sundar S K @ 2023-11-20 6:42 UTC (permalink / raw)
To: Hans de Goede, markgross, ilpo.jarvinen, basavaraj.natikar, jikos,
benjamin.tissoires
Cc: Patil.Reddy, mario.limonciello, platform-driver-x86, linux-input,
Koenig, Christian, Deucher, Alexander
In-Reply-To: <ebe24400-39d0-44be-8a25-d69f26e64e64@redhat.com>
Hi Hans,
On 11/17/2023 5:01 PM, Hans de Goede wrote:
> Hi,
>
> On 11/17/23 12:08, Shyam Sundar S K wrote:
>> Adding AMDGPU folks (Alex and Christian) - I had to drop them as the
>> changes from amdgpu were moved to PMF driver.
>>
>> Hi Hans,
>>
>>
>> On 11/17/2023 4:18 PM, Hans de Goede wrote:
>>> Hi Shyam,
>>>
>>> On 11/17/23 09:07, Shyam Sundar S K wrote:
>>>> In order to provide GPU inputs to TA for the Smart PC solution to work, we
>>>> need to have interface between the PMF driver and the AMDGPU driver.
>>>>
>>>> Add the initial code path for get interface from AMDGPU.
>>>>
>>>> Co-developed-by: Mario Limonciello <mario.limonciello@amd.com>
>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>
> <snip>
>
>>>> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
>>>> index 81b1bd356e83..82ee2b1c627f 100644
>>>> --- a/drivers/platform/x86/amd/pmf/tee-if.c
>>>> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
>>>> @@ -10,6 +10,7 @@
>>>>
>>>> #include <linux/debugfs.h>
>>>> #include <linux/tee_drv.h>
>>>> +#include <linux/thermal.h>
>>>> #include <linux/uuid.h>
>>>> #include "pmf.h"
>>>>
>>>> @@ -422,6 +423,60 @@ static void amd_pmf_tee_deinit(struct amd_pmf_dev *dev)
>>>> tee_client_close_context(dev->tee_ctx);
>>>> }
>>>>
>>>> +static int amd_pmf_gpu_get_cur_state(struct thermal_cooling_device *cooling_dev,
>>>> + unsigned long *state)
>>>> +{
>>>> + struct backlight_device *bd;
>>>> +
>>>> + if (acpi_video_get_backlight_type() != acpi_backlight_native)
>>>> + return -ENODEV;
>>>> +
>>>> + bd = backlight_device_get_by_type(BACKLIGHT_RAW);
>>>> + if (!bd)
>>>> + return -ENODEV;
>>>> +
>>>> + *state = backlight_get_brightness(bd);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int amd_pmf_gpu_get_max_state(struct thermal_cooling_device *cooling_dev,
>>>> + unsigned long *state)
>>>> +{
>>>> + struct backlight_device *bd;
>>>> +
>>>> + if (acpi_video_get_backlight_type() != acpi_backlight_native)
>>>> + return -ENODEV;
>>>> +
>>>> + bd = backlight_device_get_by_type(BACKLIGHT_RAW);
>>>> + if (!bd)
>>>> + return -ENODEV;
>>>> +
>>>> + if (backlight_is_blank(bd))
>>>> + *state = 0;
>>>> + else
>>>> + *state = bd->props.max_brightness;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static const struct thermal_cooling_device_ops bd_cooling_ops = {
>>>> + .get_max_state = amd_pmf_gpu_get_max_state,
>>>> + .get_cur_state = amd_pmf_gpu_get_cur_state,
>>>> +};
>>>
>>> This is still the wrong thing to do. The new PMF code MUST only
>>> be a *consumer* of the thermal_cooling_device API.
>>>
>>> The thermal-cooling device for the backlight itself MUST be
>>> registered by the amdgpu driver.
>>>
>>> I believe that the correct fix for this is to add a new flag to
>>> the backlight_properties struct;
>>> And then make backlight_device_register() register
>>> a thermal_cooling_device for the backlight when this new flag is set.
>>>
>>> This way we get a nice generic way to use backlight class devices
>>> as thermal cooling devices and you can make the amdgpu driver
>>> register the thermal cooling device by simply adding the new flag
>>> to the backlight_properties struct used in the amdgpu driver.
>>
>> Agreed. I am also of the same opinion.
>
> So the change to the AMDGPU driver here would just be setting
> this one new flag in the backlight_properties struct.
>
> Alex, Christian, would that be acceptable to you ?
>
>
>>>> +static int amd_pmf_get_gpu_handle(struct pci_dev *pdev, void *data)
>>>> +{
>>>> + struct amd_pmf_dev *dev = data;
>>>> +
>>>> + if (pdev->vendor == PCI_VENDOR_ID_ATI && pdev->devfn == 0) {
>>>> + dev->gfx_data.gpu_dev = pdev;
>>>> + return 1; /* Stop walking */
>>>> + }
>>>> +
>>>> + return 0; /* Continue walking */
>>>> +}
>>>> +
>>>> int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
>>>> {
>>>> int ret;
>>>> @@ -433,10 +488,30 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
>>>> INIT_DELAYED_WORK(&dev->pb_work, amd_pmf_invoke_cmd);
>>>> amd_pmf_set_dram_addr(dev);
>>>> amd_pmf_get_bios_buffer(dev);
>>>> +
>>>> dev->prev_data = kzalloc(sizeof(*dev->prev_data), GFP_KERNEL);
>>>> if (!dev->prev_data)
>>>> return -ENOMEM;
>>>>
>>>> + pci_walk_bus(dev->root->bus, amd_pmf_get_gpu_handle, dev);
>>>> + if (dev->gfx_data.gpu_dev) {
>>>> + dev->drm_dev = pci_get_drvdata(dev->gfx_data.gpu_dev);
>>>> + if (!dev->drm_dev)
>>>> + return -EINVAL;
>>>
>>> You cannot just call pci_get_drvdata() on a device for which you
>>> are not the driver. You have no idea of the lifetime of this device,
>>> the driver may unbind and release the memory pci_get_drvdata()
>>> points to, leaving this code with a dangling pointer which will
>>> crash the kernel the first time you try to use it.
>>>
>>> Also since you are not the owner you MUST not assume any specific
>>> type for this memory, you cannot be sure this actually is of
>>> the type drm_device. Basically you MUST not touch another
>>> driver's drvdata *at all*.
>>>
>>> The proper way to fix this would be to add some API to the DRM
>>> subsystem to query the information you are looking for form
>>> the DRM subsystem.
>>>
>>> Poking directly inside other driver's internals is NOT acceptable,
>>> NACK for this patch.
>>>
>>
>> I am inline with your remarks, but I could find a way where the
>> thermal driver registration, handling the backlight without having the
>> changes in the amdgpu driver very tricky.
>
> As mentioned above I think there should be generic thermal cooling
> device support added to drivers/video/backlight/backlight.c, then
> the amdgpu code just needs to pass a flag when registering
> the backlight to enable this.
>
>> Like you said, I am also of the same opinion that PMF driver should
>> call the DRM/GPU subsystem APIs (like it does with other subsystems).
>>
>> But Christian had concerns on adding all of these into the GPU driver.
>> So I had to roll back these into the PMF driver, and hence you see a
>> pci_get_drvdata() call.
>
> Ok, so I can see how this AMD PMF code is all kinda special
> and how the DRM folks don't want to have to add APIs just for
> that. But IMHO calling pci_get_drvdata() on an unowned
> device is completely unacceptable.
>
> At a minimum we need life-cycle management for the drm_device
> which the PMF code is using, something like:
>
> struct drm_device *drm_device_find(const void *data,
> int (*match)(struct drm_device *dev, const void *data));
>
> which works similar to class_find_device() and returns
> a reference to the drm_device for which match has returned 0
> (which also stops it from looping over devices).
>
> Combined with a:
>
> void drm_device_put(struct drm_device *dev);
>
> for when the PMF code is done with the device.
>
> This way the PMF code can safely get a reference to drm_device
> and release it when it is done. Rather then just getting
> some random pointer which may or not actually be a drm_device
> and the lifetime of which is not guaranteed in anyway.
>
> E.g. if the PMF driver loads before amdgpu then
> pci_get_drvdata() will just return NULL.
>
> And as mentioned if the amdgpu driver gets unbound after
> the PMF code has called pci_get_drvdata() the PMF driver
> now has a dangling pointer.
>
> So IMHO adding: drm_device_find() + drm_device_put()
> to the DRM core are minimum which is necessary here.
>
> If the PMF code then ends up doing things like
> drm_for_each_connector_iter() on the gotten drm_device
> referemce so be it. But we must make sure we have
> a properly lifecycle managed reference first and
> pci_get_drvdata() does not give us that.
>
Ok I will work on your feedback.
>> For the sake of simplicity, I can drop patches 13/17 and 14/17 from
>> the series and send them separately later.
>
> Yes I think that dropping the GPU related patches for
> now would be best.
>
Thank you. Let me wait for feedback from others before I drop the GPU
patches.
Thanks,
Shyam
> Regards,
>
> Hans
>
>
>
>
^ permalink raw reply
* Re: supporting binary (near-far) proximity sensors over gpio
From: Jonathan Cameron @ 2023-11-20 17:31 UTC (permalink / raw)
To: David Lechner
Cc: Sicelo, linux-iio, maemo-leste, Ivaylo Dimitrov, linux-input
In-Reply-To: <CAMknhBE5A3w7ntdWC9cFDYSrPQNPoH7sQ5PVXKEy6MAJmZ93SA@mail.gmail.com>
On Sat, 18 Nov 2023 18:09:18 -0600
David Lechner <dlechner@baylibre.com> wrote:
> On Fri, Nov 17, 2023 at 12:22 PM Sicelo <absicsz@gmail.com> wrote:
> >
> > Hi
> >
> > Some phones have 1-bit proximity sensors, which simply toggle a GPIO
> > line to indicate that an object is near or far. Thresholds are set at
> > hardware level. One such sensor is OSRAM SFH 7741 [1], which is used on
> > the Nokia N900.
> >
> > It is currently exported over evdev, emitting the SW_FRONT_PROXIMITY key
> > code [2].
> >
> > So the question is: should a new, general purpose iio-gpio driver be
> > written, that would switch such a proximity sensor to the iio framework?
> > Or evdev is really the best place to support it?
> >
> > There are a couple of people who are willing to write such an iio
> > driver, if iio is the way to go.
> >
> > Regards,
> > Sicelo
> >
> > [1] https://media.digikey.com/pdf/Data%20Sheets/Osram%20PDFs/SFH_7741.pdf
> > [2] https://elixir.bootlin.com/linux/v6.6.1/source/arch/arm/boot/dts/ti/omap/omap3-n900.dts#L111
> >
> Since this is really a proximity switch (it is either on or off)
> rather than measuring a proximity value over a continuous range, it
> doesn't seem like a good fit for the iio subsystem. If the sensor is
> on a phone, then it is likely to detect human presence so the input
> subsystem does seem like the right one for that application.
>
> More at https://www.kernel.org/doc/html/latest/driver-api/iio/intro.html
>
Agreed. This one at least has a working distance of 30mm sensor, so
definitely switch type usecases where input tends to be the right choice.
If we wanted to use proximity range sensor for this usecase, we'd probably
bridge it to input (maybe in userspace, maybe in kernel) from the
underlying IIO driver.
Jonathan
^ permalink raw reply
* [RFC v2 1/7] HID: i2c-hid: Fold i2c_hid_execute_reset() into i2c_hid_hwreset()
From: Hans de Goede @ 2023-11-20 19:33 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Hans de Goede, Douglas Anderson, Julian Sax, ahormann,
Bruno Jesus, Dietrich, kloxdami, Tim Aldridge, Rene Wagner,
Federico Ricchiuto, linux-input
In-Reply-To: <20231120193313.666912-1-hdegoede@redhat.com>
i2c_hid_hwreset() is the only caller of i2c_hid_execute_reset(),
fold the latter into the former.
This is a preparation patch for removing the need for
I2C_HID_QUIRK_NO_IRQ_AFTER_RESET by making i2c-hid behave
more like Windows.
No functional changes intended.
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Move the i2c_hid_dbg(... "%s: waiting...\n" ...) to above the
msleep(100) for the I2C_HID_QUIRK_NO_IRQ_AFTER_RESET quirk
---
drivers/hid/i2c-hid/i2c-hid-core.c | 81 +++++++++++++-----------------
1 file changed, 35 insertions(+), 46 deletions(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 2735cd585af0..74dd145275f1 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -426,49 +426,9 @@ static int i2c_hid_set_power(struct i2c_hid *ihid, int power_state)
return ret;
}
-static int i2c_hid_execute_reset(struct i2c_hid *ihid)
-{
- size_t length = 0;
- int ret;
-
- i2c_hid_dbg(ihid, "resetting...\n");
-
- /* Prepare reset command. Command register goes first. */
- *(__le16 *)ihid->cmdbuf = ihid->hdesc.wCommandRegister;
- length += sizeof(__le16);
- /* Next is RESET command itself */
- length += i2c_hid_encode_command(ihid->cmdbuf + length,
- I2C_HID_OPCODE_RESET, 0, 0);
-
- set_bit(I2C_HID_RESET_PENDING, &ihid->flags);
-
- ret = i2c_hid_xfer(ihid, ihid->cmdbuf, length, NULL, 0);
- if (ret) {
- dev_err(&ihid->client->dev, "failed to reset device.\n");
- goto out;
- }
-
- if (ihid->quirks & I2C_HID_QUIRK_NO_IRQ_AFTER_RESET) {
- msleep(100);
- goto out;
- }
-
- i2c_hid_dbg(ihid, "%s: waiting...\n", __func__);
- if (!wait_event_timeout(ihid->wait,
- !test_bit(I2C_HID_RESET_PENDING, &ihid->flags),
- msecs_to_jiffies(5000))) {
- ret = -ENODATA;
- goto out;
- }
- i2c_hid_dbg(ihid, "%s: finished.\n", __func__);
-
-out:
- clear_bit(I2C_HID_RESET_PENDING, &ihid->flags);
- return ret;
-}
-
static int i2c_hid_hwreset(struct i2c_hid *ihid)
{
+ size_t length = 0;
int ret;
i2c_hid_dbg(ihid, "%s\n", __func__);
@@ -482,21 +442,50 @@ static int i2c_hid_hwreset(struct i2c_hid *ihid)
ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON);
if (ret)
- goto out_unlock;
+ goto err_unlock;
- ret = i2c_hid_execute_reset(ihid);
+ /* Prepare reset command. Command register goes first. */
+ *(__le16 *)ihid->cmdbuf = ihid->hdesc.wCommandRegister;
+ length += sizeof(__le16);
+ /* Next is RESET command itself */
+ length += i2c_hid_encode_command(ihid->cmdbuf + length,
+ I2C_HID_OPCODE_RESET, 0, 0);
+
+ set_bit(I2C_HID_RESET_PENDING, &ihid->flags);
+
+ ret = i2c_hid_xfer(ihid, ihid->cmdbuf, length, NULL, 0);
if (ret) {
dev_err(&ihid->client->dev,
"failed to reset device: %d\n", ret);
- i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP);
- goto out_unlock;
+ goto err_clear_reset;
}
+ i2c_hid_dbg(ihid, "%s: waiting...\n", __func__);
+
+ if (ihid->quirks & I2C_HID_QUIRK_NO_IRQ_AFTER_RESET) {
+ msleep(100);
+ clear_bit(I2C_HID_RESET_PENDING, &ihid->flags);
+ }
+
+ if (!wait_event_timeout(ihid->wait,
+ !test_bit(I2C_HID_RESET_PENDING, &ihid->flags),
+ msecs_to_jiffies(5000))) {
+ ret = -ENODATA;
+ goto err_clear_reset;
+ }
+ i2c_hid_dbg(ihid, "%s: finished.\n", __func__);
+
/* At least some SIS devices need this after reset */
if (!(ihid->quirks & I2C_HID_QUIRK_NO_WAKEUP_AFTER_RESET))
ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON);
-out_unlock:
+ mutex_unlock(&ihid->reset_lock);
+ return ret;
+
+err_clear_reset:
+ clear_bit(I2C_HID_RESET_PENDING, &ihid->flags);
+ i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP);
+err_unlock:
mutex_unlock(&ihid->reset_lock);
return ret;
}
--
2.41.0
^ permalink raw reply related
* [RFC v2 0/7] HID: i2c-hid: Rework wait for reset to match Windows
From: Hans de Goede @ 2023-11-20 19:33 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Hans de Goede, Douglas Anderson, Julian Sax, ahormann,
Bruno Jesus, Dietrich, kloxdami, Tim Aldridge, Rene Wagner,
Federico Ricchiuto, linux-input
Hi All,
Here is v2 of my i2c-hid series reworking how the i2c-hid-core waits
for reset to complete.
Further testing on the laptop for which I2C_HID_QUIRK_NO_IRQ_AFTER_RESET
was first introduced, shows that reading the report descriptor before
waiting for the reset helps with the missing reset IRQ, but it only helps
some of the time. About 50% of the time the reset still does not get
acked properly.
Therefor for this v2 I have dropped the patch dropping
the I2C_HID_QUIRK_NO_IRQ_AFTER_RESET quirks and I've demoted
this series to a RFC. It is basically ready for merging, but first
the question if this should be applied at all should be discussed:
Advantages of merging this series:
1. Reading descriptors before waiting for reset does make the reset ack IRQ
happen some of the time, so it does seem to improve things somewhat and
maybe it does fully fix the issue on some other models
2. This should reduce the probe time of the i2c-hid driver
Disadvantages of merging this series:
1. Non 0 chance of this causing regressions
Changes in v2:
- Drop the patch dropping the I2C_HID_QUIRK_NO_IRQ_AFTER_RESET quirks
- Add a patch changing a missing reset ack from an error into a warning
- Move the mutex_[un]lock(&ihid->reset_lock) calls out of
i2c_hid_start_hwreset() / i2c_hid_finish_hwreset() and into their
callers, as suggested by Douglas Anderson
Original v1 cover-letter:
Looking at:
"2247751 - Touchpad not working on Lenovo Thinkbook 15 Gen 5"
https://bugzilla.redhat.com/show_bug.cgi?id=2247751
I thought at first that this was another touchpad needing
a I2C_HID_QUIRK_NO_IRQ_AFTER_RESET quirk, further testing
has shown this seems to be an IRQ not working issue though,
so this series does not help for that bug.
The bug has made me revisit I2C_HID_QUIRK_NO_IRQ_AFTER_RESET though and it
made me look at Microsoft's i2c-hid docs again and I noticed the following:
"""
4. Issue a RESET (Host Initiated Reset) to the Device.
5. Retrieve report descriptor from the device.
Note: Steps 4 and 5 may be done in parallel to optimize for time on I²C.
Since report descriptors are (a) static and (b) quite long, Windows 8 may
issue a request for 5 while it is waiting for a response from the device
on 4.
"""
which made me think that maybe on some touchpads the reset ack is delayed
till after the report descriptor is read ?
Testing a T-BAO Tbook Air 12.5 with a 0911:5288 (SIPODEV SP1064?) touchpad,
for which the I2C_HID_QUIRK_NO_IRQ_AFTER_RESET quirk was first introduced,
shows that about 1 ms after the report descriptor read finishes the reset
indeed does get acked.
So this series refactors the reset handling to move the waiting for
the ack to after reading the report-descriptor, so that
the I2C_HID_QUIRK_NO_IRQ_AFTER_RESET quirk is no longer necessary.
Regards,
Hans
Hans de Goede (7):
HID: i2c-hid: Fold i2c_hid_execute_reset() into i2c_hid_hwreset()
HID: i2c-hid: Split i2c_hid_hwreset() in start() and finish()
functions
HID: i2c-hid: Switch i2c_hid_parse() to goto style error handling
HID: i2c-hid: Move i2c_hid_finish_hwreset() to after reading the
report-descriptor
HID: i2c-hid: Turn missing reset ack into a warning
HID: i2c-hid: Remove I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV quirk
HID: i2c-hid: Renumber I2C_HID_QUIRK_ defines
drivers/hid/i2c-hid/i2c-hid-core.c | 134 +++++++++++++++--------------
1 file changed, 70 insertions(+), 64 deletions(-)
--
2.41.0
^ permalink raw reply
* [RFC v2 2/7] HID: i2c-hid: Split i2c_hid_hwreset() in start() and finish() functions
From: Hans de Goede @ 2023-11-20 19:33 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Hans de Goede, Douglas Anderson, Julian Sax, ahormann,
Bruno Jesus, Dietrich, kloxdami, Tim Aldridge, Rene Wagner,
Federico Ricchiuto, linux-input
In-Reply-To: <20231120193313.666912-1-hdegoede@redhat.com>
Split i2c_hid_hwreset() into:
i2c_hid_start_hwreset() which sends the PWR_ON and reset commands; and
i2c_hid_finish_hwreset() which actually waits for the reset to complete.
This is a preparation patch for removing the need for
I2C_HID_QUIRK_NO_IRQ_AFTER_RESET by making i2c-hid behave
more like Windows.
No functional changes intended.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Move the mutex_[un]lock(&ihid->reset_lock) calls from
i2c_hid_start_hwreset() / i2c_hid_finish_hwreset() to the callers
to make the locking more clear
---
drivers/hid/i2c-hid/i2c-hid-core.c | 38 ++++++++++++++++++++++--------
1 file changed, 28 insertions(+), 10 deletions(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 74dd145275f1..607ed9b7ba1b 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -426,7 +426,7 @@ static int i2c_hid_set_power(struct i2c_hid *ihid, int power_state)
return ret;
}
-static int i2c_hid_hwreset(struct i2c_hid *ihid)
+static int i2c_hid_start_hwreset(struct i2c_hid *ihid)
{
size_t length = 0;
int ret;
@@ -438,11 +438,11 @@ static int i2c_hid_hwreset(struct i2c_hid *ihid)
* being reset. Otherwise we may lose the reset complete
* interrupt.
*/
- mutex_lock(&ihid->reset_lock);
+ lockdep_assert_held(&ihid->reset_lock);
ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON);
if (ret)
- goto err_unlock;
+ return ret;
/* Prepare reset command. Command register goes first. */
*(__le16 *)ihid->cmdbuf = ihid->hdesc.wCommandRegister;
@@ -460,6 +460,18 @@ static int i2c_hid_hwreset(struct i2c_hid *ihid)
goto err_clear_reset;
}
+ return 0;
+
+err_clear_reset:
+ clear_bit(I2C_HID_RESET_PENDING, &ihid->flags);
+ i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP);
+ return ret;
+}
+
+static int i2c_hid_finish_hwreset(struct i2c_hid *ihid)
+{
+ int ret = 0;
+
i2c_hid_dbg(ihid, "%s: waiting...\n", __func__);
if (ihid->quirks & I2C_HID_QUIRK_NO_IRQ_AFTER_RESET) {
@@ -479,14 +491,11 @@ static int i2c_hid_hwreset(struct i2c_hid *ihid)
if (!(ihid->quirks & I2C_HID_QUIRK_NO_WAKEUP_AFTER_RESET))
ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON);
- mutex_unlock(&ihid->reset_lock);
return ret;
err_clear_reset:
clear_bit(I2C_HID_RESET_PENDING, &ihid->flags);
i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP);
-err_unlock:
- mutex_unlock(&ihid->reset_lock);
return ret;
}
@@ -733,7 +742,11 @@ static int i2c_hid_parse(struct hid_device *hid)
}
do {
- ret = i2c_hid_hwreset(ihid);
+ mutex_lock(&ihid->reset_lock);
+ ret = i2c_hid_start_hwreset(ihid);
+ if (ret == 0)
+ ret = i2c_hid_finish_hwreset(ihid);
+ mutex_unlock(&ihid->reset_lock);
if (ret)
msleep(1000);
} while (tries-- > 0 && ret);
@@ -976,10 +989,15 @@ static int i2c_hid_core_resume(struct i2c_hid *ihid)
* However some ALPS touchpads generate IRQ storm without reset, so
* let's still reset them here.
*/
- if (ihid->quirks & I2C_HID_QUIRK_RESET_ON_RESUME)
- ret = i2c_hid_hwreset(ihid);
- else
+ if (ihid->quirks & I2C_HID_QUIRK_RESET_ON_RESUME) {
+ mutex_lock(&ihid->reset_lock);
+ ret = i2c_hid_start_hwreset(ihid);
+ if (ret == 0)
+ ret = i2c_hid_finish_hwreset(ihid);
+ mutex_unlock(&ihid->reset_lock);
+ } else {
ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON);
+ }
if (ret)
return ret;
--
2.41.0
^ permalink raw reply related
* [RFC v2 4/7] HID: i2c-hid: Move i2c_hid_finish_hwreset() to after reading the report-descriptor
From: Hans de Goede @ 2023-11-20 19:33 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Hans de Goede, Douglas Anderson, Julian Sax, ahormann,
Bruno Jesus, Dietrich, kloxdami, Tim Aldridge, Rene Wagner,
Federico Ricchiuto, linux-input
In-Reply-To: <20231120193313.666912-1-hdegoede@redhat.com>
A recent bug made me look at Microsoft's i2c-hid docs again
and I noticed the following:
"""
4. Issue a RESET (Host Initiated Reset) to the Device.
5. Retrieve report descriptor from the device.
Note: Steps 4 and 5 may be done in parallel to optimize for time on I²C.
Since report descriptors are (a) static and (b) quite long, Windows 8 may
issue a request for 5 while it is waiting for a response from the device
on 4.
"""
Which made me think that maybe on some touchpads the reset ack is delayed
till after the report descriptor is read ?
Testing a T-BAO Tbook Air 12.5 with a 0911:5288 (SIPODEV SP1064?) touchpad,
for which the I2C_HID_QUIRK_NO_IRQ_AFTER_RESET quirk was first introduced,
shows that reading the report descriptor before waiting for the reset
helps with the missing reset IRQ. Now the reset does get acked properly,
but the ack sometimes still does not happen unfortunately.
Still moving the wait for ack to after reading the report-descriptor,
is probably a good idea, both to make i2c-hid's behavior closer to
Windows as well as to speed up probing i2c-hid devices.
While at it drop the dbg_hid() for a malloc failure, malloc failures
already get logged extensively by malloc itself.
Link: https://bugzilla.redhat.com/show_bug.cgi?id=2247751
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Adjust commit message to note that moving the wait-for-reset
to after reading thr report-descriptor only partially fixes
the missing reset IRQ problem
- Adjust for the reset_lock now being taken in the callers of
i2c_hid_start_hwreset() / i2c_hid_finish_hwreset()
---
drivers/hid/i2c-hid/i2c-hid-core.c | 24 +++++++++++++++++-------
1 file changed, 17 insertions(+), 7 deletions(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 5e535480fed7..48582c75a51b 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -741,12 +741,9 @@ static int i2c_hid_parse(struct hid_device *hid)
return -EINVAL;
}
+ mutex_lock(&ihid->reset_lock);
do {
- mutex_lock(&ihid->reset_lock);
ret = i2c_hid_start_hwreset(ihid);
- if (ret == 0)
- ret = i2c_hid_finish_hwreset(ihid);
- mutex_unlock(&ihid->reset_lock);
if (ret)
msleep(1000);
} while (tries-- > 0 && ret);
@@ -764,8 +761,8 @@ static int i2c_hid_parse(struct hid_device *hid)
rdesc = kzalloc(rsize, GFP_KERNEL);
if (!rdesc) {
- dbg_hid("couldn't allocate rdesc memory\n");
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto abort_reset;
}
i2c_hid_dbg(ihid, "asking HID report descriptor\n");
@@ -775,10 +772,23 @@ static int i2c_hid_parse(struct hid_device *hid)
rdesc, rsize);
if (ret) {
hid_err(hid, "reading report descriptor failed\n");
- goto out;
+ goto abort_reset;
}
}
+ /*
+ * Windows directly reads the report-descriptor after sending reset
+ * and then waits for resets completion afterwards. Some touchpads
+ * actually wait for the report-descriptor to be read before signalling
+ * reset completion.
+ */
+ ret = i2c_hid_finish_hwreset(ihid);
+abort_reset:
+ clear_bit(I2C_HID_RESET_PENDING, &ihid->flags);
+ mutex_unlock(&ihid->reset_lock);
+ if (ret)
+ goto out;
+
i2c_hid_dbg(ihid, "Report Descriptor: %*ph\n", rsize, rdesc);
ret = hid_parse_report(hid, rdesc, rsize);
--
2.41.0
^ permalink raw reply related
* [RFC v2 3/7] HID: i2c-hid: Switch i2c_hid_parse() to goto style error handling
From: Hans de Goede @ 2023-11-20 19:33 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Hans de Goede, Douglas Anderson, Julian Sax, ahormann,
Bruno Jesus, Dietrich, kloxdami, Tim Aldridge, Rene Wagner,
Federico Ricchiuto, linux-input
In-Reply-To: <20231120193313.666912-1-hdegoede@redhat.com>
Switch i2c_hid_parse() to goto style error handling.
This is a preparation patch for removing the need for
I2C_HID_QUIRK_NO_IRQ_AFTER_RESET by making i2c-hid behave
more like Windows.
Note this changes the descriptor read error path to propagate
the actual i2c_hid_read_register() error code (which is always
negative) instead of hardcoding a -EIO return.
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/hid/i2c-hid/i2c-hid-core.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 607ed9b7ba1b..5e535480fed7 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -775,23 +775,21 @@ static int i2c_hid_parse(struct hid_device *hid)
rdesc, rsize);
if (ret) {
hid_err(hid, "reading report descriptor failed\n");
- kfree(rdesc);
- return -EIO;
+ goto out;
}
}
i2c_hid_dbg(ihid, "Report Descriptor: %*ph\n", rsize, rdesc);
ret = hid_parse_report(hid, rdesc, rsize);
+ if (ret)
+ dbg_hid("parsing report descriptor failed\n");
+
+out:
if (!use_override)
kfree(rdesc);
- if (ret) {
- dbg_hid("parsing report descriptor failed\n");
- return ret;
- }
-
- return 0;
+ return ret;
}
static int i2c_hid_start(struct hid_device *hid)
--
2.41.0
^ permalink raw reply related
* [RFC v2 5/7] HID: i2c-hid: Turn missing reset ack into a warning
From: Hans de Goede @ 2023-11-20 19:33 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Hans de Goede, Douglas Anderson, Julian Sax, ahormann,
Bruno Jesus, Dietrich, kloxdami, Tim Aldridge, Rene Wagner,
Federico Ricchiuto, linux-input
In-Reply-To: <20231120193313.666912-1-hdegoede@redhat.com>
On all i2c-hid devices seen sofar the reset-ack either works, or the hw is
somehow buggy and does not (always) ack the reset properly, yet it still
works fine.
Lower the very long reset timeout to 1 second which should be plenty
and change the reset not getting acked from an error into a warning.
This results in a bit cleaner code and avoids the need to add more
I2C_HID_QUIRK_NO_IRQ_AFTER_RESET quirks in the future.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/hid/i2c-hid/i2c-hid-core.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 48582c75a51b..96fb5aafc1fa 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -481,9 +481,9 @@ static int i2c_hid_finish_hwreset(struct i2c_hid *ihid)
if (!wait_event_timeout(ihid->wait,
!test_bit(I2C_HID_RESET_PENDING, &ihid->flags),
- msecs_to_jiffies(5000))) {
- ret = -ENODATA;
- goto err_clear_reset;
+ msecs_to_jiffies(1000))) {
+ dev_warn(&ihid->client->dev, "device did not ack reset within 1000 ms\n");
+ clear_bit(I2C_HID_RESET_PENDING, &ihid->flags);
}
i2c_hid_dbg(ihid, "%s: finished.\n", __func__);
@@ -492,11 +492,6 @@ static int i2c_hid_finish_hwreset(struct i2c_hid *ihid)
ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON);
return ret;
-
-err_clear_reset:
- clear_bit(I2C_HID_RESET_PENDING, &ihid->flags);
- i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP);
- return ret;
}
static void i2c_hid_get_input(struct i2c_hid *ihid)
--
2.41.0
^ permalink raw reply related
* [RFC v2 6/7] HID: i2c-hid: Remove I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV quirk
From: Hans de Goede @ 2023-11-20 19:33 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Hans de Goede, Douglas Anderson, Julian Sax, ahormann,
Bruno Jesus, Dietrich, kloxdami, Tim Aldridge, Rene Wagner,
Federico Ricchiuto, linux-input
In-Reply-To: <20231120193313.666912-1-hdegoede@redhat.com>
Re-trying the power-on command on failure on all devices should
not be a problem, drop the I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV quirk
and simply retry power-on on all devices.
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/hid/i2c-hid/i2c-hid-core.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 96fb5aafc1fa..3bdfd3e89de5 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -44,7 +44,6 @@
#include "i2c-hid.h"
/* quirks to control the device */
-#define I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV BIT(0)
#define I2C_HID_QUIRK_NO_IRQ_AFTER_RESET BIT(1)
#define I2C_HID_QUIRK_BOGUS_IRQ BIT(4)
#define I2C_HID_QUIRK_RESET_ON_RESUME BIT(5)
@@ -120,8 +119,6 @@ static const struct i2c_hid_quirks {
__u16 idProduct;
__u32 quirks;
} i2c_hid_quirks[] = {
- { USB_VENDOR_ID_WEIDA, HID_ANY_ID,
- I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV },
{ I2C_VENDOR_ID_HANTICK, I2C_PRODUCT_ID_HANTICK_5288,
I2C_HID_QUIRK_NO_IRQ_AFTER_RESET },
{ I2C_VENDOR_ID_ITE, I2C_DEVICE_ID_ITE_VOYO_WINPAD_A15,
@@ -395,8 +392,7 @@ static int i2c_hid_set_power(struct i2c_hid *ihid, int power_state)
* The call will get a return value (EREMOTEIO) but device will be
* triggered and activated. After that, it goes like a normal device.
*/
- if (power_state == I2C_HID_PWR_ON &&
- ihid->quirks & I2C_HID_QUIRK_SET_PWR_WAKEUP_DEV) {
+ if (power_state == I2C_HID_PWR_ON) {
ret = i2c_hid_set_power_command(ihid, I2C_HID_PWR_ON);
/* Device was already activated */
--
2.41.0
^ permalink raw reply related
* [RFC v2 7/7] HID: i2c-hid: Renumber I2C_HID_QUIRK_ defines
From: Hans de Goede @ 2023-11-20 19:33 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Hans de Goede, Douglas Anderson, Julian Sax, ahormann,
Bruno Jesus, Dietrich, kloxdami, Tim Aldridge, Rene Wagner,
Federico Ricchiuto, linux-input
In-Reply-To: <20231120193313.666912-1-hdegoede@redhat.com>
The quirks variable and the I2C_HID_QUIRK_ defines are never used /
exported outside of the i2c-hid code renumber them to start at
BIT(0) again.
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/hid/i2c-hid/i2c-hid-core.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 3bdfd3e89de5..151d5a5c87ca 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -44,11 +44,11 @@
#include "i2c-hid.h"
/* quirks to control the device */
-#define I2C_HID_QUIRK_NO_IRQ_AFTER_RESET BIT(1)
-#define I2C_HID_QUIRK_BOGUS_IRQ BIT(4)
-#define I2C_HID_QUIRK_RESET_ON_RESUME BIT(5)
-#define I2C_HID_QUIRK_BAD_INPUT_SIZE BIT(6)
-#define I2C_HID_QUIRK_NO_WAKEUP_AFTER_RESET BIT(7)
+#define I2C_HID_QUIRK_NO_IRQ_AFTER_RESET BIT(0)
+#define I2C_HID_QUIRK_BOGUS_IRQ BIT(1)
+#define I2C_HID_QUIRK_RESET_ON_RESUME BIT(2)
+#define I2C_HID_QUIRK_BAD_INPUT_SIZE BIT(3)
+#define I2C_HID_QUIRK_NO_WAKEUP_AFTER_RESET BIT(4)
/* Command opcodes */
#define I2C_HID_OPCODE_RESET 0x01
--
2.41.0
^ permalink raw reply related
* Re: supporting binary (near-far) proximity sensors over gpio
From: Sicelo @ 2023-11-20 20:23 UTC (permalink / raw)
To: Jonathan Cameron
Cc: David Lechner, linux-iio, maemo-leste, Ivaylo Dimitrov,
linux-input
In-Reply-To: <20231120173131.000058a2@Huawei.com>
Hi
On Mon, Nov 20, 2023 at 05:31:31PM +0000, Jonathan Cameron wrote:
> > Since this is really a proximity switch (it is either on or off)
> > rather than measuring a proximity value over a continuous range, it
> > doesn't seem like a good fit for the iio subsystem. If the sensor is
> > on a phone, then it is likely to detect human presence so the input
> > subsystem does seem like the right one for that application.
> >
> > More at https://www.kernel.org/doc/html/latest/driver-api/iio/intro.html
> >
> Agreed. This one at least has a working distance of 30mm sensor, so
> definitely switch type usecases where input tends to be the right choice.
>
> If we wanted to use proximity range sensor for this usecase, we'd probably
> bridge it to input (maybe in userspace, maybe in kernel) from the
> underlying IIO driver.
>
> Jonathan
Thank you so much for the input. It makes sense.
Sincerely
Sicelo
^ permalink raw reply
* Re: Implement per-key keyboard backlight as auxdisplay?
From: Pavel Machek @ 2023-11-20 20:52 UTC (permalink / raw)
To: Jani Nikula, hdegoede, jikos
Cc: Miguel Ojeda, Lee Jones, linux-kernel, Werner Sembach,
dri-devel@lists.freedesktop.org, linux-input, ojeda, linux-leds
In-Reply-To: <87sf61bm8t.fsf@intel.com>
[-- Attachment #1: Type: text/plain, Size: 2661 bytes --]
Hi!
> >> So... a bit of rationale. The keyboard does not really fit into the
> >> LED subsystem; LEDs are expected to be independent ("hdd led") and not
> >> a matrix of them.
> >
> > Makes sense.
> >
> >> We do see various strange displays these days -- they commonly have
> >> rounded corners and holes in them. I'm not sure how that's currently
> >> supported, but I believe it is reasonable to view keyboard as a
> >> display with slightly weird placing of pixels.
> >>
> >> Plus, I'd really like to play tetris on one of those :-).
> >>
> >> So, would presenting them as auxdisplay be acceptable? Or are there
> >> better options?
> >
> > It sounds like a fair use case -- auxdisplay are typically simple
> > character-based or small graphical displays, e.g. 128x64, that may not
> > be a "main" / usual screen as typically understood, but the concept is
> > a bit fuzzy and we are a bit of a catch-all.
> >
> > And "keyboard backlight display with a pixel/color per-key" does not
> > sound like a "main" screen, and having some cute effects displayed
> > there are the kind of thing that one could do in the usual small
> > graphical ones too. :)
> >
> > But if somebody prefers to create new categories (or subcategories
> > within auxdisplay) to hold these, that could be nice too (in the
> > latter case, I would perhaps suggest reorganizing all of the existing
> > ones while at it).
>
> One could also reasonably make the argument that controlling the
> individual keyboard key backlights should be part of the input
> subsystem. It's not a display per se. (Unless you actually have small
> displays on the keycaps, and I think that's a thing too.)
While it would not be completely crazy to do that... I believe the
backlight is more of a display and less of a keyboard. Plus input
subystem is very far away from supporting this, and we had no input
from input people here.
I don't think LED subsystem is right place for this, and I believe
auxdisplay makes slightly more sense than input.
Unless someone steps up, I'd suggest Werner tries to implement this as
an auxdisplay. [And yes, this will not be simple task. RGB on LED is
different from RGB on display. But there are other LED displays, so
auxdisplay should handle this. Plus pixels are really funnily
shaped. But displays with missing pixels -- aka holes for camera --
are common in phones, and I believe we'll get variable pixel densities
-- less dense over camera -- too. So displays will have to deal with
these in the end.]
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply
* Re: Implement per-key keyboard backlight as auxdisplay?
From: Pavel Machek @ 2023-11-20 20:53 UTC (permalink / raw)
To: Miguel Ojeda, jikos
Cc: Jani Nikula, Lee Jones, linux-kernel, Werner Sembach,
dri-devel@lists.freedesktop.org, linux-input, ojeda, linux-leds
In-Reply-To: <CANiq72kvZcNp2ocXoqBae9q2UW+RPQy3dXUr+QS-izKpM1yyoA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1193 bytes --]
On Mon 2023-10-23 13:44:46, Miguel Ojeda wrote:
> On Mon, Oct 23, 2023 at 1:40 PM Jani Nikula <jani.nikula@linux.intel.com> wrote:
> >
> > One could also reasonably make the argument that controlling the
> > individual keyboard key backlights should be part of the input
> > subsystem. It's not a display per se. (Unless you actually have small
> > displays on the keycaps, and I think that's a thing too.)
> >
> > There's force feedback, there could be light feedback? There's also
> > drivers/input/input-leds.c for the keycaps that have leds, like caps
> > lock, num lock, etc.
> >
> > Anyway, just throwing ideas around, no strong opinions, really.
>
> Yeah, sounds quite reasonable too, in fact it may make more sense
> there given the LEDs are associated per-key rather than being an
> uniform matrix in a rectangle if I understand correctly. If the input
> subsystem wants to take it, that would be great.
Unfortunately we are getting no input from input subsystem. Question
seems to be more of "is auxdisplay willing to take it if it is done
properly"?
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply
* Re: [RFC v2 1/7] HID: i2c-hid: Fold i2c_hid_execute_reset() into i2c_hid_hwreset()
From: Doug Anderson @ 2023-11-20 22:05 UTC (permalink / raw)
To: Hans de Goede
Cc: Jiri Kosina, Benjamin Tissoires, Julian Sax, ahormann,
Bruno Jesus, Dietrich, kloxdami, Tim Aldridge, Rene Wagner,
Federico Ricchiuto, linux-input
In-Reply-To: <20231120193313.666912-2-hdegoede@redhat.com>
Hi,
On Mon, Nov 20, 2023 at 11:33 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> @@ -482,21 +442,50 @@ static int i2c_hid_hwreset(struct i2c_hid *ihid)
>
> ret = i2c_hid_set_power(ihid, I2C_HID_PWR_ON);
> if (ret)
> - goto out_unlock;
> + goto err_unlock;
>
> - ret = i2c_hid_execute_reset(ihid);
> + /* Prepare reset command. Command register goes first. */
> + *(__le16 *)ihid->cmdbuf = ihid->hdesc.wCommandRegister;
> + length += sizeof(__le16);
> + /* Next is RESET command itself */
> + length += i2c_hid_encode_command(ihid->cmdbuf + length,
> + I2C_HID_OPCODE_RESET, 0, 0);
> +
> + set_bit(I2C_HID_RESET_PENDING, &ihid->flags);
> +
> + ret = i2c_hid_xfer(ihid, ihid->cmdbuf, length, NULL, 0);
> if (ret) {
> dev_err(&ihid->client->dev,
> "failed to reset device: %d\n", ret);
> - i2c_hid_set_power(ihid, I2C_HID_PWR_SLEEP);
> - goto out_unlock;
> + goto err_clear_reset;
> }
>
> + i2c_hid_dbg(ihid, "%s: waiting...\n", __func__);
> +
> + if (ihid->quirks & I2C_HID_QUIRK_NO_IRQ_AFTER_RESET) {
> + msleep(100);
> + clear_bit(I2C_HID_RESET_PENDING, &ihid->flags);
> + }
> +
> + if (!wait_event_timeout(ihid->wait,
optional: as mentioned in v1, I probably would have made the above an
"else if" to make it clear that it's not ever true if you ran the
"I2C_HID_QUIRK_NO_IRQ_AFTER_RESET" logic.
^ permalink raw reply
* Re: [RFC v2 2/7] HID: i2c-hid: Split i2c_hid_hwreset() in start() and finish() functions
From: Doug Anderson @ 2023-11-20 22:05 UTC (permalink / raw)
To: Hans de Goede
Cc: Jiri Kosina, Benjamin Tissoires, Julian Sax, ahormann,
Bruno Jesus, Dietrich, kloxdami, Tim Aldridge, Rene Wagner,
Federico Ricchiuto, linux-input
In-Reply-To: <20231120193313.666912-3-hdegoede@redhat.com>
Hi,
On Mon, Nov 20, 2023 at 11:33 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Split i2c_hid_hwreset() into:
>
> i2c_hid_start_hwreset() which sends the PWR_ON and reset commands; and
> i2c_hid_finish_hwreset() which actually waits for the reset to complete.
>
> This is a preparation patch for removing the need for
> I2C_HID_QUIRK_NO_IRQ_AFTER_RESET by making i2c-hid behave
> more like Windows.
>
> No functional changes intended.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -Move the mutex_[un]lock(&ihid->reset_lock) calls from
> i2c_hid_start_hwreset() / i2c_hid_finish_hwreset() to the callers
> to make the locking more clear
> ---
> drivers/hid/i2c-hid/i2c-hid-core.c | 38 ++++++++++++++++++++++--------
> 1 file changed, 28 insertions(+), 10 deletions(-)
Reviewed-by: Douglas Anderson <dianders@chromium.org>
^ permalink raw reply
* Re: [RFC v2 4/7] HID: i2c-hid: Move i2c_hid_finish_hwreset() to after reading the report-descriptor
From: Doug Anderson @ 2023-11-20 22:07 UTC (permalink / raw)
To: Hans de Goede
Cc: Jiri Kosina, Benjamin Tissoires, Julian Sax, ahormann,
Bruno Jesus, Dietrich, kloxdami, Tim Aldridge, Rene Wagner,
Federico Ricchiuto, linux-input
In-Reply-To: <20231120193313.666912-5-hdegoede@redhat.com>
Hi,
On Mon, Nov 20, 2023 at 11:33 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> @@ -741,12 +741,9 @@ static int i2c_hid_parse(struct hid_device *hid)
> return -EINVAL;
> }
>
> + mutex_lock(&ihid->reset_lock);
> do {
> - mutex_lock(&ihid->reset_lock);
> ret = i2c_hid_start_hwreset(ihid);
> - if (ret == 0)
> - ret = i2c_hid_finish_hwreset(ihid);
> - mutex_unlock(&ihid->reset_lock);
> if (ret)
> msleep(1000);
> } while (tries-- > 0 && ret);
Right after this loop, you have:
if (ret)
return ret;
That will return with the mutex held. It needs to be a "goto
abort_reset". You'd also need to init `use_override` then, I think.
I'll also note that it seems awkward that
`clear_bit(I2C_HID_RESET_PENDING, &ihid->flags)` is scattered in so
many places for error handling, but I couldn't really find a better
way to do it. :-P
-Doug
^ permalink raw reply
* Re: [RFC v2 5/7] HID: i2c-hid: Turn missing reset ack into a warning
From: Doug Anderson @ 2023-11-20 22:07 UTC (permalink / raw)
To: Hans de Goede
Cc: Jiri Kosina, Benjamin Tissoires, Julian Sax, ahormann,
Bruno Jesus, Dietrich, kloxdami, Tim Aldridge, Rene Wagner,
Federico Ricchiuto, linux-input
In-Reply-To: <20231120193313.666912-6-hdegoede@redhat.com>
Hi,
On Mon, Nov 20, 2023 at 11:33 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> On all i2c-hid devices seen sofar the reset-ack either works, or the hw is
> somehow buggy and does not (always) ack the reset properly, yet it still
> works fine.
>
> Lower the very long reset timeout to 1 second which should be plenty
> and change the reset not getting acked from an error into a warning.
>
> This results in a bit cleaner code and avoids the need to add more
> I2C_HID_QUIRK_NO_IRQ_AFTER_RESET quirks in the future.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> drivers/hid/i2c-hid/i2c-hid-core.c | 11 +++--------
> 1 file changed, 3 insertions(+), 8 deletions(-)
Reviewed-by: Douglas Anderson <dianders@chromium.org>
^ permalink raw reply
* [PATCH RESEND] Input: i8042 - add Fujitsu Lifebook U728 to i8042 quirk table
From: Szilard Fabian @ 2023-11-20 23:39 UTC (permalink / raw)
To: linux-kernel, linux-input, dmitry.torokhov; +Cc: Szilard Fabian
Another Fujitsu-related patch.
In the initial boot stage the integrated keyboard of Fujitsu Lifebook U728
refuses to work and it's not possible to type for example a dm-crypt
passphrase without the help of an external keyboard.
i8042.nomux kernel parameter resolves this issue but using that a PS/2
mouse is detected. This input device is unused even when the i2c-hid-acpi
kernel module is blacklisted making the integrated ELAN touchpad
(04F3:3092) not working at all.
So this notebook uses a hid-over-i2c touchpad which is managed by the
i2c_designware input driver. Since you can't find a PS/2 mouse port on this
computer and you can't connect a PS/2 mouse to it even with an official
port replicator I think it's safe to not use the PS/2 mouse port at all.
Signed-off-by: Szilard Fabian <szfabian@bluemarch.art>
---
I think the same configuration could be applied to Lifebook U748 and U758
too but I can't test this theory on these machines.
---
drivers/input/serio/i8042-acpipnpio.h | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/input/serio/i8042-acpipnpio.h b/drivers/input/serio/i8042-acpipnpio.h
index 028e45bd050b..983f31014330 100644
--- a/drivers/input/serio/i8042-acpipnpio.h
+++ b/drivers/input/serio/i8042-acpipnpio.h
@@ -618,6 +618,14 @@ static const struct dmi_system_id i8042_dmi_quirk_table[] __initconst = {
},
.driver_data = (void *)(SERIO_QUIRK_NOMUX)
},
+ {
+ /* Fujitsu Lifebook U728 */
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "FUJITSU"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "LIFEBOOK U728"),
+ },
+ .driver_data = (void *)(SERIO_QUIRK_NOAUX)
+ },
{
/* Gigabyte M912 */
.matches = {
--
2.42.1
^ permalink raw reply related
* Re: [PATCH v5 2/4] dt-bindings: touchscreen: add overlay-touchscreen and overlay-buttons properties
From: Javier Carrasco @ 2023-11-21 7:23 UTC (permalink / raw)
To: Jeff LaBundy
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Henrik Rydberg, Bastian Hecht, Michael Riesch, linux-kernel,
linux-input, devicetree
In-Reply-To: <ZTp72LUXxr+Z9gn8@nixie71>
Hi Jeff,
On 26.10.23 16:46, Jeff LaBundy wrote:
> I am still confused why the buttons need to live under an 'overlay-buttons'
> parent node, which seems like an imaginary boundary. In my view, the touch
> surface comprises the following types of rectangular areas:
>
> 1. A touchscreen, wherein granular coordinates and pressure are reported.
> 2. A momentary button, wherein pressure is quantized into a binary value
> (press or release), and coordinates are ignored.
>
> Any contact that falls outside of (1) and (2) is presumed to be part of a
> border or matting, and is hence ignored.
>
> Areas (1) and (2) exist in the same "plane", so why can they not reside
> under the same parent node? The following seems much more representative
> of the actual hardware we intend to describe in the device tree:
>
> touchscreen {
> compatible = "...";
> reg = <...>;
>
> /* raw coordinates reported here */
> touch-area-1 {
> x-origin = <...>;
> y-origin = <...>;
> x-size = <...>;
> y-size = <...>;
> };
>
> /* a button */
> touch-area-2a {
> x-origin = <...>;
> y-origin = <...>;
> x-size = <...>;
> y-size = <...>;
> linux,code = <KEY_POWER>;
> };
>
> /* another button */
> touch-area-2b {
> x-origin = <...>;
> y-origin = <...>;
> x-size = <...>;
> y-size = <...>;
> linux,code = <KEY_INFO>;
> };
> };
>
> With this method, the driver merely stores a list head. The parsing code
> then walks the client device node; for each touch* child encountered, it
> allocates memory for a structure of five members, and adds it to the list.
>
> The event handling code then simply iterates through the list and checks
> if the coordinates reported by the hardware fall within each rectangle. If
> so, and the keycode in the list element is equal to KEY_RESERVED (zero),
> we assume the rectangle is of type (1); the coordinates are passed to
> touchscreen_report_pos() and the pressure is reported as well.
>
> If the keycode is not equal to KEY_RESERVED (e.g. KEY_POWER), we assume
> the rectangle is of type (2); input_report_key() is called instead and
> the coordinates are ignored altogether.
>
> Instead, the existing implementation has two separate structures, one of
> which inherits the other. Its corresponding properties are then parsed in
> a separate function to account for the fact that the two structures exist
> at different layers in the heirarchy.
>
> My argument is that all nodes can be parsed in the same way, without having
> to understand whether they represent case (1) or (2). The existing method
> also has to count the nodes; this would not be necessary with a linked list.
>
> Can you help me understand why the buttons must be "wrapped" in their own
> node? As I mention in v2, this isn't a deal breaker necessarily, but I'd
> like to understand the reasoning behind what I interpret as additional
> complexity, and a degree of separation from a natural description of the
> touch surface.
>
> The only reason I can think to insert the 'overlay-buttons' parent is in
> case we want to specify a property within this node that applies to all
> buttons, but this does not exist in the current implementation, nor do I
> see a compelling reason to do so in the future.
>
> Kind regards,
> Jeff LaBundy
I was about to send a gentle ping when I saw that you have reviewed the
last version almost a month ago. Somehow I overlooked your emails, sorry
for the late reply.
As I said in a previous discussion, there is no unavoidable reason why
the buttons should have their own node. I just wanted to make clear that
there are different objects with different properties and in case of
some new appear, there is no need to check several properties to build a
decision tree. Moreover, the device tree is self-documented and even if
you never saw this feature before, there is no need to explain anything:
every object type has its node and the boundary I would be drawing would
be logical, not physical.
On the other hand, with the current implementation a single keycode
property is enough to tell what object is being handled as there are
only two types of objects. If some new objects appear and the decision
tree ends up getting too complex, some other solution might be more
suitable. But until that happens (if ever), I can give up on the button
nodes and simplify the code with a list head.
I will work on that approach for v7. This will require some major
modifications in the code and the bindings, so it will take some time
until the next version is ready and gets proper testing.
Your comments on 1/4 do not require further discussion and will be
applied as well.
Thanks again for your thorough review and enriching feedback.
Best regards,
Javier Carrasco
^ permalink raw reply
* Re: [PATCH v7] HID: mcp2200: added driver for GPIOs of MCP2200
From: Jiri Kosina @ 2023-11-21 8:28 UTC (permalink / raw)
To: Johannes Roith
Cc: sergeantsagara, andi.shyti, benjamin.tissoires,
christophe.jaillet, linux-input, linux-kernel, rdunlap
In-Reply-To: <20230921164928.170383-1-johannes@gnu-linux.rocks>
On Thu, 21 Sep 2023, Johannes Roith wrote:
> Added a gpiochip compatible driver to control the 8 GPIOs of
> the MCP2200 by using the HID interface.
>
> Using GPIOs with alternative functions (GP0<->SSPND, GP1<->USBCFG,
> GP6<->RXLED, GP7<->TXLED) will reset the functions, if set (unset by
> default).
>
> The driver was tested while also using the UART of the chip. Setting
> and reading the GPIOs has no effect on the UART communication. However,
> a reset is triggered after the CONFIGURE command. If the GPIO Direction
> is constantly changed, this will affect the communication at low baud
> rates. This is a hardware problem of the MCP2200 and is not caused by
> the driver.
>
> Signed-off-by: Johannes Roith <johannes@gnu-linux.rocks>
Now applied to hid.git#for-6.8/mcp2200. Thanks and sorry for the delay
in the review.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH 0/5] MCP2221 Improvements
From: Jiri Kosina @ 2023-11-21 8:31 UTC (permalink / raw)
To: Hamish Martin
Cc: gupt21, benjamin.tissoires, Enrik.Berkhan, sven.zuehlsdorf,
linux-i2c, linux-input
In-Reply-To: <20231025035514.3450123-1-hamish.martin@alliedtelesis.co.nz>
On Wed, 25 Oct 2023, Hamish Martin wrote:
> Recently I've been prototyping a new system which uses a MCP2221 chip as
> the I2C bus. During that development a few issues were found that form
> the patches in this series.
>
> The first two are resolving long standing issues that have both been
> reported before, patches submitted, but it appears never accepted.
> The ACPI patch resolves an issue in my x86_64 system.
> The final two address fundamental issues with the driver that have not
> worked correctly from the beginning it seems.
>
> Please review and let me know what you think.
Hi,
I have applied 1/5 and 2/5 for 6.7 and the rest of the series for 6.8.
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] HID: apple: add Jamesdonkey and A3R to non-apple keyboards list
From: Jiri Kosina @ 2023-11-21 8:32 UTC (permalink / raw)
To: Yihong Cao; +Cc: Benjamin Tissoires, open list:HID CORE LAYER, open list
In-Reply-To: <SYYP282MB2110B4E87983EAFEDC8741E49BA2A@SYYP282MB2110.AUSP282.PROD.OUTLOOK.COM>
On Mon, 30 Oct 2023, Yihong Cao wrote:
> Jamesdonkey A3R keyboard is identified as "Jamesdonkey A3R" in wired
> mode, "A3R-U" in wireless mode and "A3R" in bluetooth mode. Adding them
> to non-apple keyboards fixes function key.
Applied, thanks.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH v2] HID: fix a crash in hid_debug_events_release
From: Jiri Kosina @ 2023-11-21 8:36 UTC (permalink / raw)
To: Charles Yi; +Cc: benjamin.tissoires, linux-input, linux-kernel
In-Reply-To: <20231031043239.157943-1-be286@163.com>
On Tue, 31 Oct 2023, Charles Yi wrote:
> hid_debug_events_release() access released memory by
> hid_device_release(). This is fixed by the patch.
>
> When hid_debug_events_release() was being called, in most case,
> hid_device_release() finish already, the memory of list->hdev
> freed by hid_device_release(), if list->hdev memory
> reallocate by others, and it's modified, zeroed, then
> list->hdev->debug_list_lock occasioned crash come out.
I have applied the changelog changes proposed by Rahul and applied,
thanks.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] HID: glorious: fix Glorious Model I HID report
From: Jiri Kosina @ 2023-11-21 8:38 UTC (permalink / raw)
To: Brett Raye; +Cc: benjamin.tissoires, linux-input
In-Reply-To: <20231103011038.27462-1-braye@fastmail.com>
On Thu, 2 Nov 2023, Brett Raye wrote:
> The Glorious Model I mouse has a buggy HID report descriptor for its
> keyboard endpoint (used for programmable buttons). For report ID 2, there
> is a mismatch between Logical Minimum and Usage Minimum in the array that
> reports keycodes.
>
> The offending portion of the descriptor: (from hid-decode)
>
> 0x95, 0x05, // Report Count (5) 30
> 0x75, 0x08, // Report Size (8) 32
> 0x15, 0x00, // Logical Minimum (0) 34
> 0x25, 0x65, // Logical Maximum (101) 36
> 0x05, 0x07, // Usage Page (Keyboard) 38
> 0x19, 0x01, // Usage Minimum (1) 40
> 0x29, 0x65, // Usage Maximum (101) 42
> 0x81, 0x00, // Input (Data,Arr,Abs) 44
>
> This bug shifts all programmed keycodes up by 1. Importantly, this causes
> "empty" array indexes of 0x00 to be interpreted as 0x01, ErrorRollOver.
> The presence of ErrorRollOver causes the system to ignore all keypresses
> from the endpoint and breaks the ability to use the programmable buttons.
>
> Setting byte 41 to 0x00 fixes this, and causes keycodes to be interpreted
> correctly.
>
> Also, USB_VENDOR_ID_GLORIOUS is changed to USB_VENDOR_ID_SINOWEALTH,
> and a new ID for Laview Technology is added. Glorious seems to be
> white-labeling controller boards or mice from these vendors. There isn't a
> single canonical vendor ID for Glorious products.
>
> Signed-off-by: Brett Raye <braye@fastmail.com>
Applied, thanks.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] HID: multitouch: Add quirk for HONOR GLO-GXXX touchpad
From: Jiri Kosina @ 2023-11-21 8:40 UTC (permalink / raw)
To: Aoba K; +Cc: Dmitry Torokhov, Jiri Kosina, Benjamin Tissoires, linux-input
In-Reply-To: <DM6PR04MB412176BD43A29E1E873FB7E5CEA9A@DM6PR04MB4121.namprd04.prod.outlook.com>
On Tue, 7 Nov 2023, Aoba K wrote:
> Honor MagicBook 13 2023 has a touchpad which do not switch to the
> multitouch mode until the input mode feature is written by the host.
> The touchpad do report the input mode at touchpad(3), while itself
> working under mouse mode. As a workaround, it is possible to call
> MT_QUIRE_FORCE_GET_FEATURE to force set feature in mt_set_input_mode
> for such device.
> The touchpad reports as BLTP7853, which cannot retrive any useful
> manufacture information on the internel by this string at present.
> As the serial number of the laptop is GLO-G52, while DMI info reports
> the laptop serial number as GLO-GXXX, this workaround should applied
> to all models which has the GLO-GXXX.
Hi,
unfortunately, your patch has been whitespace-damaged by your mail client.
Could you please fix that and resend?
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox