* Re: [PATCH 1/4] Input: st-keyscan: drop of_match_ptr for ID table
From: Dmitry Torokhov @ 2023-03-17 11:19 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Ray Jui, Scott Branden, Broadcom internal kernel review list,
Chen-Yu Tsai, Jernej Skrabec, Samuel Holland, linux-input,
linux-kernel, linux-arm-kernel, linux-sunxi
In-Reply-To: <20230312131514.351603-1-krzysztof.kozlowski@linaro.org>
On Sun, Mar 12, 2023 at 02:15:11PM +0100, Krzysztof Kozlowski wrote:
> The driver can match only via the DT table so the table should be always
> used and the of_match_ptr does not have any sense (this also allows ACPI
> matching via PRP0001, even though it might not be relevant here). This
> also fixes !CONFIG_OF error:
>
> drivers/input/keyboard/st-keyscan.c:251:34: error: ‘keyscan_of_match’ defined but not used [-Werror=unused-const-variable=]
>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v5 2/2] Input: hideep - Optionally reset controller work mode to native HiDeep protocol
From: Dmitry Torokhov @ 2023-03-17 10:58 UTC (permalink / raw)
To: Hans de Goede; +Cc: Krzysztof Kozlowski, linux-input
In-Reply-To: <20230311114726.182789-3-hdegoede@redhat.com>
On Sat, Mar 11, 2023 at 12:47:26PM +0100, Hans de Goede wrote:
> The HiDeep IST940E touchscreen controller used on the Lenovo Yoga Book X90F
> convertible comes up in HID mode by default.
>
> This works well on the X91F Windows model where the touchscreen is
> correctly described in ACPI and ACPI takes care of controlling
> the reset GPIO and regulators.
>
> But the X90F ships with Android and the ACPI tables on this model don't
> describe the touchscreen. Instead this is hardcoded in the vendor kernel.
>
> The vendor kernel uses the touchscreen in native HiDeep 20 (2.0?) protocol
> mode and switches the controller to this mode by writing 0 to reg 0x081e.
>
> Adjusting the i2c-hid code to deal with the reset-gpio and regulators on
> this non devicetree (but rather broken ACPI) convertible is somewhat tricky
> and the native protocol reports ABS_MT_PRESSURE and ABS_MT_TOUCH_MAJOR
> which are not reported in HID mode, so it is preferable to use the native
> mode.
>
> Add support to the hideep driver to reset the work-mode to the native
> HiDeep protocol to allow using it on the Lenovo Yoga Book X90F.
> This is guarded behind a new "hideep,force-native-protocol" boolean
> property, to avoid changing behavior on other devices.
>
> For the record: I did test using the i2c-hid driver with some quick hacks
> and it does work. The I2C-HID descriptor is available from address 0x0020,
> just like on the X91F Windows model.
>
> So far the new "hideep,force-native-protocol" property is only used on
> x86/ACPI (non devicetree) devs. IOW it is not used in actual devicetree
> files. The devicetree-bindings maintainers have requested properties like
> these to not be added to the devicetree-bindings, so the new property is
> deliberately not added to the existing devicetree-bindings.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v5 1/2] Input: hideep - Silence error in SW_RESET()
From: Dmitry Torokhov @ 2023-03-17 10:57 UTC (permalink / raw)
To: Hans de Goede; +Cc: Krzysztof Kozlowski, linux-input
In-Reply-To: <20230311114726.182789-2-hdegoede@redhat.com>
On Sat, Mar 11, 2023 at 12:47:25PM +0100, Hans de Goede wrote:
> On some models the first HIDEEP_SYSCON_WDT_CON write alone is enough to
> cause the controller to reset, causing the second write to fail:
>
> i2c-hideep_ts: write to register 0x52000014 (0x000001) failed: -121
>
> Switch this write to a raw hideep_pgm_w_mem() to avoid an error getting
> logged in this case.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH -next] Input: hideep: clean up some inconsistent indenting
From: Dmitry Torokhov @ 2023-03-17 10:57 UTC (permalink / raw)
To: Yang Li; +Cc: linux-input, linux-kernel, Abaci Robot
In-Reply-To: <20220429070103.4747-1-yang.lee@linux.alibaba.com>
On Fri, Apr 29, 2022 at 03:01:03PM +0800, Yang Li wrote:
> Turn the space into a tab to Eliminate the follow smatch warning:
> drivers/input/touchscreen/hideep.c:470 hideep_program_nvm() warn:
> inconsistent indenting
>
> Reported-by: Abaci Robot <abaci@linux.alibaba.com>
> Signed-off-by: Yang Li <yang.lee@linux.alibaba.com>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v3 1/2] dt-bindings: input: touchscreen: Add 'goodix-hold-in-reset' property to Goodix
From: Jan Jasper de Kroon @ 2023-03-17 10:39 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: alexandre.belloni, broonie, devicetree, dmitry.torokhov, kernel,
krzysztof.kozlowski+dt, linux-input, robh+dt
In-Reply-To: <1ae4767f-563a-6425-f20b-32be6ba8a7c7@linaro.org>
Op 16-03-2023 om 20:25 schreef Krzysztof Kozlowski:
> On 16/03/2023 16:29, Jan Jasper de Kroon wrote:
>> Add an optional 'goodix-hold-in-reset', to the Goodix touchscreen
>> device tree binding. When set to true, the touchscreen controller will
>> be held in reset mode during system suspend, reducing power consumption.
>> If not present, the property defaults to false.
>>
>> Signed-off-by: Jan Jasper de Kroon <jajadekroon@gmail.com>
> Don't attach new patchsets to some other threads. It messes with our
> tools and reading/reviewing process.
Thank you for bringing this to my attention. I apologize for any
inconvenience caused by attaching the patchset to the wrong threads. As a
new user of LKML, I'm still learning the appropriate protocol for
submitting patches. Going forward, I will ensure to attach patchsets to
the correct threads.
>> ---
>> Changes from v2 to v3:
>> - Used imperative mood instead of "This patch adds".
>> - Dropped "I am submitting this patch to..." as it is redundant.
>> - Removed the paragraph related to the related patch sent to the
>> linux-input mailing list as it is not necessary.
>> - Renamed the hold-in-reset-in-suspend function to
>> goodix-hold-in-reset to prevent potential naming conflicts with other
>> functions in the codebase. No functional changes were made.
>>
>> Changes from v1 to v2:
>> - Updated subject prefix to match subsystem.
>> - Added more detailed description of the change.
>> - Fixed formatting issues in commit message.
>> .../devicetree/bindings/input/touchscreen/goodix.yaml | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
>> index 3d016b87c8df..197f8db9acc2 100644
>> --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
>> +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
>> @@ -56,6 +56,13 @@ properties:
>> touchscreen-size-y: true
>> touchscreen-swapped-x-y: true
>>
>> + goodix-hold-in-reset:
> That's not a vendor prefix... missing coma.
Thank you for pointing out the mistake in the vendor prefix. I appreciate
your feedback and apologize for any inconvenience caused. I wasn't aware
of the correct vendor prefix style, but I've learned from developer Hans
de Goede that it should be "goodix,hold-in-reset." I will make sure to
correct this in my local branch and ensure that it is applied correctly in
the future. Thanks again for bringing this to my attention.
>> + description: |
>> + When set to true, the touchscreen controller will be held in reset mode
>> + during system suspend. This can help reduce power consumption, but may
>> + cause the touchscreen to take longer to resume when the system is woken
>> + up from suspend.
> Anyway, my concerns were not answered, so to be clear:
>
> NAK till you answer them. Do not send new versions without answering
> existing concerns and discussion.
Thank you again for reviewing my patchset and providing feedback. I
appreciate your time and effort in ensuring the quality and suitability
of the DeviceTree.
Regarding the concerns you raised about the proposed feature, I would
like to address them directly. You mentioned that the property does not
look suitable for Devicetree because it describes system policies that are
not within the scope of Devicetree. While I understand your point, I
believe this property is appropriate for Devicetree for the following
reasons:
- The property directly relates to the hardware configuration of the
device, specifically the touchscreen controller, and is not a software
policy.
- The property is required for proper system operation and is not optional
in specific device use cases. To be more specific in the case of the
PinePhone Original and Pro. The original commit message of the driver
implementation in driver/input/touchscreen contained the following:
"It consumes quite a bit of power (~40mW) during system sleep, and more
when the screen is touched."
Because the phone is usually kept in your pocket, so prone to a lot of
screen touches, this is highly undesired behavior for the touchscreen in
this case. This in my opinion makes it a mandatory property in this
situation.
- The property is not a user-facing configuration option and is not meant
to be changed by the end-user.
- The property, although in separate device specific kernel, and still
called 'poweroff-in-suspend' is already in use on specific devices,
including the PinePhone Original and PinePhone Pro.
However, I understand your concern that Devicetree should not be used for
policies. To address this concern, I would like to propose the following
changes to the property description:
1. Remove the sentence about reducing power consumption, as this could be
considered a policy.
2. Emphasize that the property is a required hardware configuration and
not an optional feature on certain devices.
3. Recommend that any changes to the property value should only be made by
experienced system administrators and not end-users.
I hope these changes address your concerns and make the property more
suitable for inclusion in Devicetree. If you have any further suggestions
or feedback, please let me know. Thank you again for your time and
guidance.
Best regards,
Jan Jasper de Kroon
>
>
> Best regards,
> Krzysztof
>
^ permalink raw reply
* Re: [PATCH] Input: i8042 - Add quirk for Fujitsu Lifebook A574/H
From: Dmitry Torokhov @ 2023-03-17 10:37 UTC (permalink / raw)
To: Jonathan Denose
Cc: LKML, Jonathan Denose, Hans de Goede, Huacai Chen,
Mattijs Korpershoek, Takashi Iwai, Werner Sembach, Wolfram Sang,
linux-input
In-Reply-To: <20230303152623.45859-1-jdenose@google.com>
On Fri, Mar 03, 2023 at 09:26:23AM -0600, Jonathan Denose wrote:
> Fujitsu Lifebook A574/H requires the nomux option to properly
> probe the touchpad, especially when waking from sleep.
>
> Signed-off-by: Jonathan Denose <jdenose@google.com>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: goodix - Add Lenovo Yoga Book X90F to nine_bytes_report DMI table
From: Dmitry Torokhov @ 2023-03-17 10:36 UTC (permalink / raw)
To: Hans de Goede; +Cc: Bastien Nocera, linux-input
In-Reply-To: <20230315134442.71787-1-hdegoede@redhat.com>
On Wed, Mar 15, 2023 at 02:44:42PM +0100, Hans de Goede wrote:
> The Android Lenovo Yoga Book X90F / X90L uses the same goodix touchscreen
> with 9 bytes touch reports for its touch keyboard as the already supported
> Windows Lenovo Yoga Book X91F/L, add a DMI match for this to
> the nine_bytes_report DMI table.
>
> When the quirk for the X91F/L was initially added it was written to
> also apply to the X90F/L but this does not work because the Android
> version of the Yoga Book uses completely different DMI strings.
> Also adjust the X91F/L quirk to reflect that it only applies to
> the X91F/L models.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* Re: [lvc-project] [PATCH] Input: trackpoint - remove unreachable code
From: Dmitry Torokhov @ 2023-03-17 10:18 UTC (permalink / raw)
To: Andi Shyti; +Cc: Igor Artemiev, linux-input, linux-kernel, lvc-project
In-Reply-To: <20230314131356.cwb4nd7i43sws75j@intel.intel>
On Tue, Mar 14, 2023 at 02:13:56PM +0100, Andi Shyti wrote:
> Hi Igor,
>
> On Tue, Mar 14, 2023 at 03:27:14PM +0300, Igor Artemiev wrote:
> > The trackpoint_sync() function always returnd 0.
> > And there is no need to check its result.
> >
> > Found by Linux Verification Center (linuxtesting.org) with SVACE.
> >
> > Fixes: 2a924d71794c ("Input: trackpoint - only expose supported controls for Elan, ALPS and NXP")
> > Signed-off-by: Igor Artemiev <Igor.A.Artemiev@mcst.ru>
>
> I don't think it requires the Fixes tag... it's not really
> broken.
>
> > ---
> > drivers/input/mouse/trackpoint.c | 4 +---
> > 1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/drivers/input/mouse/trackpoint.c b/drivers/input/mouse/trackpoint.c
> > index 4a86b3e31d3b..561a4d2d81ff 100644
> > --- a/drivers/input/mouse/trackpoint.c
> > +++ b/drivers/input/mouse/trackpoint.c
> > @@ -386,9 +386,7 @@ static int trackpoint_reconnect(struct psmouse *psmouse)
> > was_reset = tp->variant_id == TP_VARIANT_IBM &&
> > trackpoint_power_on_reset(&psmouse->ps2dev) == 0;
> >
> > - error = trackpoint_sync(psmouse, was_reset);
> > - if (error)
> > - return error;
> > + trackpoint_sync(psmouse, was_reset);
>
> what worries me here is that if this returns always '0' who tells
> me that it will always return '0'?
>
> One day someone might add an error return path and you would miss
> it here.
>
> Would it make sense to make the trackpoint_sync() a void function
> as well?
Yes if we are dropping the check we should also change the function to
not return anything.
Thanks.
--
Dmitry
^ permalink raw reply
* [PATCH] Input: elan_i2c - Implement inhibit/uninhibit functions.
From: jingle.wu @ 2023-03-17 7:16 UTC (permalink / raw)
To: linux-kernel, linux-input, dmitry.torokhov
Cc: phoenix, josh.chen, dave.wang, jingle.wu
Add inhibit/uninhibit functions.
Signed-off-by: Jingle.wu <jingle.wu@emc.com.tw>
---
drivers/input/mouse/elan_i2c_core.c | 86 +++++++++++++++++++++++++++++
1 file changed, 86 insertions(+)
diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
index 5f0d75a45c80..cc0375265659 100644
--- a/drivers/input/mouse/elan_i2c_core.c
+++ b/drivers/input/mouse/elan_i2c_core.c
@@ -329,6 +329,89 @@ static int elan_initialize(struct elan_tp_data *data, bool skip_reset)
return error;
}
+static int elan_reactivate(struct elan_tp_data *data)
+{
+ struct device *dev = &data->client->dev;
+ int ret;
+
+ ret = elan_set_power(data, true);
+ if (ret)
+ dev_err(dev, "failed to restore power: %d\n", ret);
+
+ ret = data->ops->sleep_control(data->client, false);
+ if (ret) {
+ dev_err(dev,
+ "failed to wake device up: %d\n", ret);
+ return ret;
+ }
+
+ return ret;
+}
+
+static void elan_inhibit(struct input_dev *input_dev)
+{
+ struct elan_tp_data *data = input_get_drvdata(input_dev);
+ struct i2c_client *client = data->client;
+ int ret;
+
+ if (data->in_fw_update)
+ return;
+
+ dev_dbg(&client->dev, "inhibiting\n");
+ /*
+ * We are taking the mutex to make sure sysfs operations are
+ * complete before we attempt to bring the device into low[er]
+ * power mode.
+ */
+ ret = mutex_lock_interruptible(&data->sysfs_mutex);
+ if (ret)
+ return;
+
+ disable_irq(client->irq);
+
+ ret = elan_set_power(data, false);
+ if (ret)
+ enable_irq(client->irq);
+
+ mutex_unlock(&data->sysfs_mutex);
+
+}
+
+static void elan_close(struct input_dev *input_dev)
+{
+ if ((input_dev->users) && (!input_dev->inhibited))
+ elan_inhibit(input_dev);
+
+}
+
+static int elan_uninhibit(struct input_dev *input_dev)
+{
+ struct elan_tp_data *data = input_get_drvdata(input_dev);
+ struct i2c_client *client = data->client;
+ int ret;
+
+ dev_dbg(&client->dev, "uninhibiting\n");
+ ret = mutex_lock_interruptible(&data->sysfs_mutex);
+ if (ret)
+ return ret;
+
+ ret = elan_reactivate(data);
+ if (ret == 0)
+ enable_irq(client->irq);
+
+ mutex_unlock(&data->sysfs_mutex);
+
+ return ret;
+}
+
+static int elan_open(struct input_dev *input_dev)
+{
+ if ((input_dev->users) && (input_dev->inhibited))
+ return elan_uninhibit(input_dev);
+
+ return 0;
+}
+
static int elan_query_device_info(struct elan_tp_data *data)
{
int error;
@@ -1175,6 +1258,9 @@ static int elan_setup_input_device(struct elan_tp_data *data)
0, ETP_FINGER_WIDTH * min_width, 0, 0);
}
+ input->open = elan_open;
+ input->close = elan_close;
+
data->input = input;
return 0;
--
2.34.1
^ permalink raw reply related
* RE: BUG: hid-sensor-ids code includes binary data in device name
From: Xu, Even @ 2023-03-17 5:49 UTC (permalink / raw)
To: todd.e.brandt@linux.intel.com, Philipp Jungkamp,
srinivas pandruvada, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org
Cc: Jonathan.Cameron@huawei.com, jkosina@suse.cz, Brandt, Todd E
In-Reply-To: <fe7b41aa975128bd44d351911b0faa17b837033c.camel@linux.intel.com>
Hi, All,
Sorry for response later.
From below description, it seems not a buffer overrun issue, it's just caused by NULL terminated string, right?
Best Regards,
Even Xu
-----Original Message-----
From: Todd Brandt <todd.e.brandt@linux.intel.com>
Sent: Saturday, March 11, 2023 7:36 AM
To: Philipp Jungkamp <p.jungkamp@gmx.net>; srinivas pandruvada <srinivas.pandruvada@linux.intel.com>; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org; Xu, Even <even.xu@intel.com>
Cc: Jonathan.Cameron@huawei.com; jkosina@suse.cz; Brandt, Todd E <todd.e.brandt@intel.com>
Subject: Re: BUG: hid-sensor-ids code includes binary data in device name
On Fri, 2023-03-10 at 15:35 +0100, Philipp Jungkamp wrote:
> Hello,
>
> on v3 of the patchset I had this comment on the 'real_usage'
> initialization:
>
> > > - char real_usage[HID_SENSOR_USAGE_LENGTH] = { 0 };
> > > + char real_usage[HID_SENSOR_USAGE_LENGTH];
> > > struct platform_device *custom_pdev;
> > > const char *dev_name;
> > > char *c;
> > >
> > > - /* copy real usage id */
> > > - memcpy(real_usage, known_sensor_luid[index], 4);
> > > + memcpy(real_usage, match->luid, 4);
> > > + real_usage[4] = '\0';
> >
> > Why the change in approach for setting the NULL character?
> > Doesn't seem relevant to main purpose of this patch.
>
> Based on the comment, I changed that in the final v4 revision to:
>
> > - char real_usage[HID_SENSOR_USAGE_LENGTH] = { 0 };
> > + char real_usage[HID_SENSOR_USAGE_LENGTH];
> > struct platform_device *custom_pdev;
> > const char *dev_name;
> > char *c;
> >
> > - /* copy real usage id */
> > - memcpy(real_usage, known_sensor_luid[index], 4);
> > + memcpy(real_usage, match->luid, 4);
>
> I ommitted the line adding the null terminator to the string but kept
> that I didn't initialize the 'real_usage' as { 0 } anymore. The string
> now misses the null terminator which leads to the broken utf-8.
>
> The simple fix is to reintroduce the 0 initialization in
> hid_sensor_register_platform_device. E.g.
>
> - char real_usage[HID_SENSOR_USAGE_LENGTH];
> + char real_usage[HID_SENSOR_USAGE_LENGTH] = { 0 };
>
I didn't realize that the issue was a buffer overrun. I tested the kernel built with this simple fix and it works ok now. i.e. this patch is is all that's needed:
diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-sensor- custom.c index 3e3f89e01d81..d85398721659 100644
--- a/drivers/hid/hid-sensor-custom.c
+++ b/drivers/hid/hid-sensor-custom.c
@@ -940,7 +940,7 @@ hid_sensor_register_platform_device(struct
platform_device *pdev,
struct hid_sensor_hub_device *hsdev,
const struct hid_sensor_custom_match *match) {
- char real_usage[HID_SENSOR_USAGE_LENGTH];
+ char real_usage[HID_SENSOR_USAGE_LENGTH] = { 0 };
struct platform_device *custom_pdev;
const char *dev_name;
char *c;
> Where do I need to submit a patch for this? And on which tree should I
> base the patch?
>
The change is so small it shouldn't require any rebasing. Just send the
patch to these emails (from MAINTAINERS):
HID SENSOR HUB DRIVERS
M: Jiri Kosina <jikos@kernel.org>
M: Jonathan Cameron <jic23@kernel.org>
M: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
L: linux-input@vger.kernel.org
L: linux-iio@vger.kernel.org
> I'm sorry for the problems my patch caused.
>
No problem. It actually made sleepgraph better because it exposed a bug
in the ftrace processing code. I wasn't properly handling the corner
case where ftrace had binary data in it.
> Regards,
> Philipp Jungkamp
>
> On Fri, 2023-03-10 at 01:51 -0800, srinivas pandruvada wrote:
> > +Even
> >
> > On Thu, 2023-03-09 at 15:33 -0800, Todd Brandt wrote:
> > > Hi all, I've run into an issue in 6.3.0-rc1 that causes problems
> > > with
> > > ftrace and I've bisected it to this commit:
> > >
> > > commit 98c062e8245199fa9121141a0bf1035dc45ae90e (HEAD,
> > > refs/bisect/bad)
> > > Author: Philipp Jungkamp p.jungkamp@gmx.net
> > > Date: Fri Nov 25 00:38:38 2022 +0100
> > >
> > > HID: hid-sensor-custom: Allow more custom iio sensors
> > >
> > > The known LUID table for established/known custom HID sensors
> > > was
> > > limited to sensors with "INTEL" as manufacturer. But some
> > > vendors
> > > such
> > > as Lenovo also include fairly standard iio sensors (e.g.
> > > ambient
> > > light)
> > > in their custom sensors.
> > >
> > > Expand the known custom sensors table by a tag used for the
> > > platform
> > > device name and match sensors based on the LUID as well as
> > > optionally
> > > on model and manufacturer properties.
> > >
> > > Signed-off-by: Philipp Jungkamp p.jungkamp@gmx.net
> > > Reviewed-by: Jonathan Cameron Jonathan.Cameron@huawei.com
> > > Acked-by: Srinivas Pandruvada
> > > srinivas.pandruvada@linux.intel.com
> > > Signed-off-by: Jiri Kosina jkosina@suse.cz
> > >
> > > You're using raw data as part of the devname in the "real_usage"
> > > string, but it includes chars other than ASCII, and those chars
> > > end
> > > up being printed out in the ftrace log which is meant to be ASCII
> > > only.
> > >
> > > - /* HID-SENSOR-INT-REAL_USAGE_ID */
> > > - dev_name = kasprintf(GFP_KERNEL, "HID-SENSOR-INT-%s",
> > > real_usage);
> > > + /* HID-SENSOR-TAG-REAL_USAGE_ID */
> > > + dev_name = kasprintf(GFP_KERNEL, "HID-SENSOR-%s-%s",
> > > + match->tag, real_usage);
> > >
> > > My sleepgraph tool started to crash because it read these lines
> > > from
> > > ftrace:
> > >
> > > device_pm_callback_start: platform HID-SENSOR-INT-020b?.39.auto,
> > > parent: 001F:8087:0AC2.0003, [suspend]
> > > device_pm_callback_end: platform HID-SENSOR-INT-020b?.39.auto,
> > > err=0
> > >
> >
> > Here tag is:
> > .tag = "INT",
> > .luid = "020B000000000000",
> >
> >
> > The LUID is still a string. Probably too long for a dev_name.
> >
> > Even,
> >
> > Please check.
> >
> > Thanks.
> > Srinivas
> >
> >
> > > The "HID-SENSOR-INT-020b?.39.auto" string includes a binary char
> > > that
> > > kills
> > > python3 code that loops through an ascii file as such:
> > >
> > > File "/usr/bin/sleepgraph", line 5579, in executeSuspend
> > > for line in fp:
> > > File "/usr/lib/python3.10/codecs.py", line 322, in decode
> > > (result, consumed) = self._buffer_decode(data, self.errors,
> > > final)
> > > UnicodeDecodeError: 'utf-8' codec can't decode byte 0xff in
> > > position
> > > 1568: invalid start byte
> > >
> > > I've updated sleepgraph to handle random non-ascii chars, but
> > > other
> > > tools
> > > may suffer the same fate. Can you rewrite this to ensure that no
> > > binary
> > > chars make it into the devname?
> > >
>
>
^ permalink raw reply
* Re: [PATCH v3 1/2] dt-bindings: input: touchscreen: Add 'goodix-hold-in-reset' property to Goodix
From: Krzysztof Kozlowski @ 2023-03-16 19:26 UTC (permalink / raw)
To: Jan Jasper de Kroon
Cc: alexandre.belloni, broonie, devicetree, dmitry.torokhov, kernel,
krzysztof.kozlowski+dt, linux-input, robh+dt
In-Reply-To: <20230316154749.68304-1-jajadekroon@gmail.com>
On 16/03/2023 16:47, Jan Jasper de Kroon wrote:
> Add an optional 'goodix-hold-in-reset', to the Goodix touchscreen
> device tree binding. When set to true, the touchscreen controller will
> be held in reset mode during system suspend, reducing power consumption.
> If not present, the property defaults to false.
>
> Signed-off-by: Jan Jasper de Kroon <jajadekroon@gmail.com>
Duplicated message, so just for formality: discussion did not finish.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 1/2] dt-bindings: input: touchscreen: Add 'goodix-hold-in-reset' property to Goodix
From: Krzysztof Kozlowski @ 2023-03-16 19:25 UTC (permalink / raw)
To: Jan Jasper de Kroon
Cc: alexandre.belloni, broonie, devicetree, dmitry.torokhov, kernel,
krzysztof.kozlowski+dt, linux-input, robh+dt
In-Reply-To: <20230316152949.67441-1-jajadekroon@gmail.com>
On 16/03/2023 16:29, Jan Jasper de Kroon wrote:
> Add an optional 'goodix-hold-in-reset', to the Goodix touchscreen
> device tree binding. When set to true, the touchscreen controller will
> be held in reset mode during system suspend, reducing power consumption.
> If not present, the property defaults to false.
>
> Signed-off-by: Jan Jasper de Kroon <jajadekroon@gmail.com>
Don't attach new patchsets to some other threads. It messes with our
tools and reading/reviewing process.
> ---
> Changes from v2 to v3:
> - Used imperative mood instead of "This patch adds".
> - Dropped "I am submitting this patch to..." as it is redundant.
> - Removed the paragraph related to the related patch sent to the
> linux-input mailing list as it is not necessary.
> - Renamed the hold-in-reset-in-suspend function to
> goodix-hold-in-reset to prevent potential naming conflicts with other
> functions in the codebase. No functional changes were made.
>
> Changes from v1 to v2:
> - Updated subject prefix to match subsystem.
> - Added more detailed description of the change.
> - Fixed formatting issues in commit message.
> .../devicetree/bindings/input/touchscreen/goodix.yaml | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
> index 3d016b87c8df..197f8db9acc2 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
> +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
> @@ -56,6 +56,13 @@ properties:
> touchscreen-size-y: true
> touchscreen-swapped-x-y: true
>
> + goodix-hold-in-reset:
That's not a vendor prefix... missing coma.
> + description: |
> + When set to true, the touchscreen controller will be held in reset mode
> + during system suspend. This can help reduce power consumption, but may
> + cause the touchscreen to take longer to resume when the system is woken
> + up from suspend.
Anyway, my concerns were not answered, so to be clear:
NAK till you answer them. Do not send new versions without answering
existing concerns and discussion.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: input: touchscreen: Add 'hold-in-reset-in-suspend' property to goodix
From: Krzysztof Kozlowski @ 2023-03-16 19:24 UTC (permalink / raw)
To: Jan Jasper de Kroon
Cc: devicetree, dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt,
broonie, alexandre.belloni, kernel, linux-input
In-Reply-To: <e33fbb3b-0ca2-f13c-607d-23e28f3c2cf4@gmail.com>
On 16/03/2023 16:41, Jan Jasper de Kroon wrote:
>> Anyway, the property does not look suitable for Devicetree. You describe
>> system policy - trade off between energy saving and responsivness to the
>> user. DT is not for policies. Use other interfaces for configuring it,
>> e.g. some user-space, existing PM interfaces or /sysfs (which is ABI and
>> needs its Documentation).
>>
>>
>>> +
>>> additionalProperties: false
>>>
>>> required:
>>> @@ -75,6 +84,7 @@ examples:
>>> interrupts = <0 0>;
>>> irq-gpios = <&gpio1 0 0>;
>>> reset-gpios = <&gpio1 1 0>;
>>> + hold-in-reset-in-suspend;
>>> };
>>> };
>>>
>> Best regards,
>> Krzysztof
>>
> I think the latest patch covers most of the feedback you provided.
> Regarding the addition of this feature to the DeviceTree. Currently this
> is only used on the Linux powered PinePhone Original and PinePhone Pro. It
> also isn't really a policy change,
What is "policy change"? I said it is system policy.
> but rather an attempt to minimize
> battery consumption on these power hungry devices.
We do not talk about the goal, but whether this is property of
Devicetree or not.
> Developers made a lot
> of tweaks here and there, to make the PinePhone get through a day of light
> use. This is one of these tweaks.
Awesome, how is this related to my concerns that it is not suitable for
Devicetree? Developers can invent thousands of things, shall we put them
all to Devicetree?
Bring specific arguments to my questions. Arguing that it is not a
"policy change" is not related to my question. Just like calling
something tweaks.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v4 2/2] input: touchscreen: goodix: Hold controller in reset during suspend
From: Hans de Goede @ 2023-03-16 17:25 UTC (permalink / raw)
To: Jan Jasper de Kroon
Cc: dmitry.torokhov, hadess, linux-input, linux-kernel, megi
In-Reply-To: <20230316152159.66922-1-jajadekroon@gmail.com>
Hi,
On 3/16/23 16:22, Jan Jasper de Kroon wrote:
> From: Ondřej Jirman <megi@xff.cz>
>
> The Goodix touchscreen controller used in PinePhone is not properly
> suspended during system sleep, leading to high power consumption. This
> commit modifies the driver to hold the controller in reset during
> system sleep, reducing power consumption by around 40mW.
>
> The original patch also disabled the regulators during system sleep, but
> this could cause issues with other peripherals attached to the same power
> supplies. Hence, this commit only holds the controller in reset during
> system sleep and does not attempt to power it off.
>
> A separate commit adds a new device tree property "goodix-hold-in-reset"
> to control the hold-in-reset behavior of the controller during system sleep.
>
> Signed-off-by: Ondrej Jirman <megi@xff.cz>
> Signed-off-by: Jan Jasper de Kroon <jajadekroon@gmail.com>
> ---
> Changes from v3 to v4:
> - Renamed the hold-in-reset-in-suspend function to
> goodix-hold-in-reset to prevent potential naming conflicts with other
> functions in the codebase. No functional changes were made.
>
> Changes from v2 to v3:
> - Some patch style cleaning up, to confirm to submission standard.
>
> Changes from v1 to v2:
> - Rewrote the commit message and function name to better reflect it's
> behavior.
> drivers/input/touchscreen/goodix.c | 23 +++++++++++++++++++++++
> drivers/input/touchscreen/goodix.h | 1 +
> 2 files changed, 24 insertions(+)
>
> diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> index b348172f19c3..9f86bdfbfc73 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -1284,6 +1284,7 @@ static void goodix_disable_regulators(void *arg)
>
> static int goodix_ts_probe(struct i2c_client *client)
> {
> + struct device_node *np = client->dev.of_node;
> struct goodix_ts_data *ts;
> const char *cfg_name;
> int error;
> @@ -1303,6 +1304,7 @@ static int goodix_ts_probe(struct i2c_client *client)
> i2c_set_clientdata(client, ts);
> init_completion(&ts->firmware_loading_complete);
> ts->contact_size = GOODIX_CONTACT_SIZE;
> + ts->goodix_hold_in_reset = of_property_read_bool(np, "goodix-hold-in-reset");
I only noticed this just now, sorry. Can you please use
"device_property_read_bool(&client-dev, "goodix-hold-in-reset");
instead, this way this will also work with non device-tree (aka OF)
platforms.
Also with devicetree properties if you prefix them by the vendor
name the vendor-prefix should end with a comma, so you get:
"goodix,hold-in-reset"
And I think this name is not very descriptive, this really should be something like:
"goodix,hold-in-reset-during-suspend"
to make what it does more clear.
I assume that this is going to get set inside a dts file, in that case this
new property should be documented in:
Documentation/devicetree/bindings/input/touchscree /goodix.yaml
and these devicetree-bindings changes must be in a separate patch
(and the dt-bindings maintainers should be Cc-ed on that patch).
Regards,
Hans
>
> error = goodix_get_gpio_config(ts);
> if (error)
> @@ -1410,6 +1412,13 @@ static int goodix_suspend(struct device *dev)
> if (ts->load_cfg_from_disk)
> wait_for_completion(&ts->firmware_loading_complete);
>
> + if (ts->goodix_hold_in_reset) {
> + goodix_free_irq(ts);
> + goodix_irq_direction_output(ts, 0);
> + gpiod_direction_output(ts->gpiod_rst, 0);
> + return 0;
> + }
> +
> /* We need gpio pins to suspend/resume */
> if (ts->irq_pin_access_method == IRQ_PIN_ACCESS_NONE) {
> disable_irq(client->irq);
> @@ -1455,6 +1464,20 @@ static int goodix_resume(struct device *dev)
> u8 config_ver;
> int error;
>
> + if (ts->goodix_hold_in_reset) {
> + error = goodix_reset(ts);
> + if (error) {
> + dev_err(dev, "Controller reset failed.\n");
> + return error;
> + }
> +
> + error = goodix_request_irq(ts);
> + if (error)
> + return error;
> +
> + return 0;
> + }
> +
> if (ts->irq_pin_access_method == IRQ_PIN_ACCESS_NONE) {
> enable_irq(client->irq);
> return 0;
> diff --git a/drivers/input/touchscreen/goodix.h b/drivers/input/touchscreen/goodix.h
> index 87797cc88b32..7be7eb1a8f27 100644
> --- a/drivers/input/touchscreen/goodix.h
> +++ b/drivers/input/touchscreen/goodix.h
> @@ -104,6 +104,7 @@ struct goodix_ts_data {
> u8 main_clk[GOODIX_MAIN_CLK_LEN];
> int bak_ref_len;
> u8 *bak_ref;
> + bool goodix_hold_in_reset;
> };
>
> int goodix_i2c_read(struct i2c_client *client, u16 reg, u8 *buf, int len);
^ permalink raw reply
* [PATCH v3 1/2] dt-bindings: input: touchscreen: Add 'goodix-hold-in-reset' property to Goodix
From: Jan Jasper de Kroon @ 2023-03-16 15:47 UTC (permalink / raw)
To: jajadekroon
Cc: alexandre.belloni, broonie, devicetree, dmitry.torokhov, kernel,
krzysztof.kozlowski+dt, linux-input, robh+dt
In-Reply-To: <20230311134655.486973-1-jajadekroon@gmail.com>
Add an optional 'goodix-hold-in-reset', to the Goodix touchscreen
device tree binding. When set to true, the touchscreen controller will
be held in reset mode during system suspend, reducing power consumption.
If not present, the property defaults to false.
Signed-off-by: Jan Jasper de Kroon <jajadekroon@gmail.com>
---
Changes from v2 to v3:
- Used imperative mood instead of "This patch adds".
- Dropped "I am submitting this patch to..." as it is redundant.
- Removed the paragraph related to the related patch sent to the
linux-input mailing list as it is not necessary.
- Renamed the hold-in-reset-in-suspend function to
goodix-hold-in-reset to prevent potential naming conflicts with other
functions in the codebase. No functional changes were made.
Changes from v1 to v2:
- Updated subject prefix to match subsystem.
- Added more detailed description of the change.
- Fixed formatting issues in commit message.
.../devicetree/bindings/input/touchscreen/goodix.yaml | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
index 3d016b87c8df..197f8db9acc2 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
+++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
@@ -56,6 +56,13 @@ properties:
touchscreen-size-y: true
touchscreen-swapped-x-y: true
+ goodix-hold-in-reset:
+ description: |
+ When set to true, the touchscreen controller will be held in reset mode
+ during system suspend. This can help reduce power consumption, but may
+ cause the touchscreen to take longer to resume when the system is woken
+ up from suspend.
+
additionalProperties: false
required:
@@ -75,6 +82,7 @@ examples:
interrupts = <0 0>;
irq-gpios = <&gpio1 0 0>;
reset-gpios = <&gpio1 1 0>;
+ goodix-hold-in-reset;
};
};
--
2.34.3
^ permalink raw reply related
* Re: [PATCH v2 1/2] dt-bindings: input: touchscreen: Add 'hold-in-reset-in-suspend' property to goodix
From: Jan Jasper de Kroon @ 2023-03-16 15:41 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: devicetree, dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt,
broonie, alexandre.belloni, kernel, linux-input
In-Reply-To: <4cdefb22-52ea-8f8f-12d2-7a07478f167c@linaro.org>
Op 14-03-2023 om 18:47 schreef Krzysztof Kozlowski:
> On 12/03/2023 19:31, Jan Jasper de Kroon wrote:
>> This patch adds a new optional property, 'hold-in-reset-in-suspend', to the
> Do not use "This commit/patch", but imperative mood. See:
> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
>
>> Goodix touchscreen device tree binding. When set to true, the touchscreen
>> controller will be held in reset mode during system suspend, reducing
>> power consumption. If not present, the property defaults to false.
>>
>> I am submitting this patch to the Device Tree mailing list to add a new
> Drop the "I am ..." Same comment as above.
>
>> property to the Goodix touchscreen device tree binding. This patch
>> supplements a related patch sent to the linux-input mailing list, which
>> updates the Goodix touchscreen driver to use this new property.
> Anyway entire paragraph does not look related to commit msg. Drop it.
>
>> The
>> linux-input patch can be found at:
>> https://lore.kernel.org/all/20230311131534.484700-1-jajadekroon@gmail.com/
> Also this. It should be rather places under ---.
>
>> Signed-off-by: Jan Jasper de Kroon <jajadekroon@gmail.com>
>> ---
>> V1 -> V2:
>> - Updated subject prefix to match subsystem
>> - Added more detailed description of the change
>> - Fixed formatting issues in commit message
>> .../devicetree/bindings/input/touchscreen/goodix.yaml | 10 ++++++++++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
>> index 3d016b87c8df..cd4dd3d25364 100644
>> --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
>> +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
>> @@ -56,6 +56,15 @@ properties:
>> touchscreen-size-y: true
>> touchscreen-swapped-x-y: true
>>
>> + hold-in-reset-in-suspend:
> Missing vendor prefix.
>
>> + type: boolean
>> + default: false
> Drop default.
>
>> + description: |
>> + When set to true, the touchscreen controller will be held in reset mode
>> + during system suspend. This can help reduce power consumption, but may
>> + cause the touchscreen to take longer to resume when the system is woken
>> + up from suspend. Defaults to false if not present.
> Drop last sentence.
>
> Anyway, the property does not look suitable for Devicetree. You describe
> system policy - trade off between energy saving and responsivness to the
> user. DT is not for policies. Use other interfaces for configuring it,
> e.g. some user-space, existing PM interfaces or /sysfs (which is ABI and
> needs its Documentation).
>
>
>> +
>> additionalProperties: false
>>
>> required:
>> @@ -75,6 +84,7 @@ examples:
>> interrupts = <0 0>;
>> irq-gpios = <&gpio1 0 0>;
>> reset-gpios = <&gpio1 1 0>;
>> + hold-in-reset-in-suspend;
>> };
>> };
>>
> Best regards,
> Krzysztof
>
I think the latest patch covers most of the feedback you provided.
Regarding the addition of this feature to the DeviceTree. Currently this
is only used on the Linux powered PinePhone Original and PinePhone Pro. It
also isn't really a policy change, but rather an attempt to minimize
battery consumption on these power hungry devices. Developers made a lot
of tweaks here and there, to make the PinePhone get through a day of light
use. This is one of these tweaks.
Best regards,
Jan Jasper de Kroon
^ permalink raw reply
* [PATCH v3 1/2] dt-bindings: input: touchscreen: Add 'goodix-hold-in-reset' property to Goodix
From: Jan Jasper de Kroon @ 2023-03-16 15:29 UTC (permalink / raw)
To: jajadekroon
Cc: alexandre.belloni, broonie, devicetree, dmitry.torokhov, kernel,
krzysztof.kozlowski+dt, linux-input, robh+dt
In-Reply-To: <20230312183106.551840-1-jajadekroon@gmail.com>
Add an optional 'goodix-hold-in-reset', to the Goodix touchscreen
device tree binding. When set to true, the touchscreen controller will
be held in reset mode during system suspend, reducing power consumption.
If not present, the property defaults to false.
Signed-off-by: Jan Jasper de Kroon <jajadekroon@gmail.com>
---
Changes from v2 to v3:
- Used imperative mood instead of "This patch adds".
- Dropped "I am submitting this patch to..." as it is redundant.
- Removed the paragraph related to the related patch sent to the
linux-input mailing list as it is not necessary.
- Renamed the hold-in-reset-in-suspend function to
goodix-hold-in-reset to prevent potential naming conflicts with other
functions in the codebase. No functional changes were made.
Changes from v1 to v2:
- Updated subject prefix to match subsystem.
- Added more detailed description of the change.
- Fixed formatting issues in commit message.
.../devicetree/bindings/input/touchscreen/goodix.yaml | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
index 3d016b87c8df..197f8db9acc2 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
+++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
@@ -56,6 +56,13 @@ properties:
touchscreen-size-y: true
touchscreen-swapped-x-y: true
+ goodix-hold-in-reset:
+ description: |
+ When set to true, the touchscreen controller will be held in reset mode
+ during system suspend. This can help reduce power consumption, but may
+ cause the touchscreen to take longer to resume when the system is woken
+ up from suspend.
+
additionalProperties: false
required:
@@ -75,6 +82,7 @@ examples:
interrupts = <0 0>;
irq-gpios = <&gpio1 0 0>;
reset-gpios = <&gpio1 1 0>;
+ goodix-hold-in-reset;
};
};
--
2.34.3
^ permalink raw reply related
* [PATCH v4 2/2] input: touchscreen: goodix: Hold controller in reset during suspend
From: Jan Jasper de Kroon @ 2023-03-16 15:22 UTC (permalink / raw)
To: jajadekroon
Cc: dmitry.torokhov, hadess, hdegoede, linux-input, linux-kernel,
megi
In-Reply-To: <20230310170026.415739-1-jajadekroon@gmail.com>
From: Ondřej Jirman <megi@xff.cz>
The Goodix touchscreen controller used in PinePhone is not properly
suspended during system sleep, leading to high power consumption. This
commit modifies the driver to hold the controller in reset during
system sleep, reducing power consumption by around 40mW.
The original patch also disabled the regulators during system sleep, but
this could cause issues with other peripherals attached to the same power
supplies. Hence, this commit only holds the controller in reset during
system sleep and does not attempt to power it off.
A separate commit adds a new device tree property "goodix-hold-in-reset"
to control the hold-in-reset behavior of the controller during system sleep.
Signed-off-by: Ondrej Jirman <megi@xff.cz>
Signed-off-by: Jan Jasper de Kroon <jajadekroon@gmail.com>
---
Changes from v3 to v4:
- Renamed the hold-in-reset-in-suspend function to
goodix-hold-in-reset to prevent potential naming conflicts with other
functions in the codebase. No functional changes were made.
Changes from v2 to v3:
- Some patch style cleaning up, to confirm to submission standard.
Changes from v1 to v2:
- Rewrote the commit message and function name to better reflect it's
behavior.
drivers/input/touchscreen/goodix.c | 23 +++++++++++++++++++++++
drivers/input/touchscreen/goodix.h | 1 +
2 files changed, 24 insertions(+)
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index b348172f19c3..9f86bdfbfc73 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -1284,6 +1284,7 @@ static void goodix_disable_regulators(void *arg)
static int goodix_ts_probe(struct i2c_client *client)
{
+ struct device_node *np = client->dev.of_node;
struct goodix_ts_data *ts;
const char *cfg_name;
int error;
@@ -1303,6 +1304,7 @@ static int goodix_ts_probe(struct i2c_client *client)
i2c_set_clientdata(client, ts);
init_completion(&ts->firmware_loading_complete);
ts->contact_size = GOODIX_CONTACT_SIZE;
+ ts->goodix_hold_in_reset = of_property_read_bool(np, "goodix-hold-in-reset");
error = goodix_get_gpio_config(ts);
if (error)
@@ -1410,6 +1412,13 @@ static int goodix_suspend(struct device *dev)
if (ts->load_cfg_from_disk)
wait_for_completion(&ts->firmware_loading_complete);
+ if (ts->goodix_hold_in_reset) {
+ goodix_free_irq(ts);
+ goodix_irq_direction_output(ts, 0);
+ gpiod_direction_output(ts->gpiod_rst, 0);
+ return 0;
+ }
+
/* We need gpio pins to suspend/resume */
if (ts->irq_pin_access_method == IRQ_PIN_ACCESS_NONE) {
disable_irq(client->irq);
@@ -1455,6 +1464,20 @@ static int goodix_resume(struct device *dev)
u8 config_ver;
int error;
+ if (ts->goodix_hold_in_reset) {
+ error = goodix_reset(ts);
+ if (error) {
+ dev_err(dev, "Controller reset failed.\n");
+ return error;
+ }
+
+ error = goodix_request_irq(ts);
+ if (error)
+ return error;
+
+ return 0;
+ }
+
if (ts->irq_pin_access_method == IRQ_PIN_ACCESS_NONE) {
enable_irq(client->irq);
return 0;
diff --git a/drivers/input/touchscreen/goodix.h b/drivers/input/touchscreen/goodix.h
index 87797cc88b32..7be7eb1a8f27 100644
--- a/drivers/input/touchscreen/goodix.h
+++ b/drivers/input/touchscreen/goodix.h
@@ -104,6 +104,7 @@ struct goodix_ts_data {
u8 main_clk[GOODIX_MAIN_CLK_LEN];
int bak_ref_len;
u8 *bak_ref;
+ bool goodix_hold_in_reset;
};
int goodix_i2c_read(struct i2c_client *client, u16 reg, u8 *buf, int len);
--
2.34.3
^ permalink raw reply related
* Re: [PATCH] Input: goodix - Add Lenovo Yoga Book X90F to nine_bytes_report DMI table
From: Bastien Nocera @ 2023-03-16 9:19 UTC (permalink / raw)
To: Hans de Goede, Dmitry Torokhov; +Cc: linux-input
In-Reply-To: <20230315134442.71787-1-hdegoede@redhat.com>
On Wed, 2023-03-15 at 14:44 +0100, Hans de Goede wrote:
> The Android Lenovo Yoga Book X90F / X90L uses the same goodix
> touchscreen
> with 9 bytes touch reports for its touch keyboard as the already
> supported
> Windows Lenovo Yoga Book X91F/L, add a DMI match for this to
> the nine_bytes_report DMI table.
>
> When the quirk for the X91F/L was initially added it was written to
> also apply to the X90F/L but this does not work because the Android
> version of the Yoga Book uses completely different DMI strings.
> Also adjust the X91F/L quirk to reflect that it only applies to
> the X91F/L models.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
LGTM
Reviewed-by: Bastien Nocera <hadess@hadess.net>
> ---
> drivers/input/touchscreen/goodix.c | 14 +++++++++++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/input/touchscreen/goodix.c
> b/drivers/input/touchscreen/goodix.c
> index 3c71f0df9769..88c223a4da61 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -128,10 +128,18 @@ static const unsigned long goodix_irq_flags[] =
> {
> static const struct dmi_system_id nine_bytes_report[] = {
> #if defined(CONFIG_DMI) && defined(CONFIG_X86)
> {
> - .ident = "Lenovo YogaBook",
> - /* YB1-X91L/F and YB1-X90L/F */
> + /* Lenovo Yoga Book X90F / X90L */
> .matches = {
> - DMI_MATCH(DMI_PRODUCT_NAME, "Lenovo YB1-X9")
> + DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Intel
> Corporation"),
> + DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "CHERRYVIEW
> D1 PLATFORM"),
> + DMI_EXACT_MATCH(DMI_PRODUCT_VERSION, "YETI-
> 11"),
> + }
> + },
> + {
> + /* Lenovo Yoga Book X91F / X91L */
> + .matches = {
> + /* Non exact match to match F + L versions */
> + DMI_MATCH(DMI_PRODUCT_NAME, "Lenovo YB1-
> X91"),
> }
> },
> #endif
^ permalink raw reply
* Re: [PATCH] Input: i8042 - Add quirk for Fujitsu Lifebook A574/H
From: Hans de Goede @ 2023-03-16 8:49 UTC (permalink / raw)
To: Jonathan Denose
Cc: LKML, Jonathan Denose, Dmitry Torokhov, Huacai Chen,
Mattijs Korpershoek, Takashi Iwai, Werner Sembach, Wolfram Sang,
linux-input
In-Reply-To: <CALNJtpWsvZEdGJFA30cv0cSq43Djm7q+trDQVxx5aRDzg7u3Gw@mail.gmail.com>
Hi,
On 3/15/23 22:39, Jonathan Denose wrote:
> Hello Hans,
>
> Thank you very much for your review.
>
> For my knowledge, what is the timeline for patches in the input
> mailing list getting applied to a maintainer branch after review?
It depends on when Dmitry has time to go through the patch queue. Not a really helpful answer I know, sorry.
Regards,
Hans
> On Mon, Mar 6, 2023 at 5:00 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 3/3/23 16:26, Jonathan Denose wrote:
>>> Fujitsu Lifebook A574/H requires the nomux option to properly
>>> probe the touchpad, especially when waking from sleep.
>>>
>>> Signed-off-by: Jonathan Denose <jdenose@google.com>
>>
>> Thanks, patch looks good to me:
>>
>> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>>
>> Regards,
>>
>> Hans
>>
>>
>>> ---
>>>
>>> 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 efc61736099b..fe7ffe30997c 100644
>>> --- a/drivers/input/serio/i8042-acpipnpio.h
>>> +++ b/drivers/input/serio/i8042-acpipnpio.h
>>> @@ -610,6 +610,14 @@ static const struct dmi_system_id i8042_dmi_quirk_table[] __initconst = {
>>> },
>>> .driver_data = (void *)(SERIO_QUIRK_NOMUX)
>>> },
>>> + {
>>> + /* Fujitsu Lifebook A574/H */
>>> + .matches = {
>>> + DMI_MATCH(DMI_SYS_VENDOR, "FUJITSU"),
>>> + DMI_MATCH(DMI_PRODUCT_NAME, "FMVA0501PZ"),
>>> + },
>>> + .driver_data = (void *)(SERIO_QUIRK_NOMUX)
>>> + },
>>> {
>>> /* Gigabyte M912 */
>>> .matches = {
>>
>
^ permalink raw reply
* Rubber Molded
From: Zbynek Spacek @ 2023-03-16 8:40 UTC (permalink / raw)
To: linux-input
Good morning,
we can offer unbeatable conditions for the supply of various silicone compounds and rubbers, liquid silicone rubber (LSR).
In our assortment you will also find cross-linking agents, stabilizers, dyes and individual silicone mixtures that we will develop for your needs.
Do you want to know what we can offer you?
Best regards
Zbynek Spacek
^ permalink raw reply
* Re: [PATCH] Input: i8042 - Add quirk for Fujitsu Lifebook A574/H
From: Jonathan Denose @ 2023-03-15 21:39 UTC (permalink / raw)
To: Hans de Goede
Cc: LKML, Jonathan Denose, Dmitry Torokhov, Huacai Chen,
Mattijs Korpershoek, Takashi Iwai, Werner Sembach, Wolfram Sang,
linux-input
In-Reply-To: <e8f5e2aa-d7fa-88ff-6306-4c1ee8feeade@redhat.com>
Hello Hans,
Thank you very much for your review.
For my knowledge, what is the timeline for patches in the input
mailing list getting applied to a maintainer branch after review?
Best,
Jonathan
On Mon, Mar 6, 2023 at 5:00 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 3/3/23 16:26, Jonathan Denose wrote:
> > Fujitsu Lifebook A574/H requires the nomux option to properly
> > probe the touchpad, especially when waking from sleep.
> >
> > Signed-off-by: Jonathan Denose <jdenose@google.com>
>
> Thanks, patch looks good to me:
>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>
> Regards,
>
> Hans
>
>
> > ---
> >
> > 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 efc61736099b..fe7ffe30997c 100644
> > --- a/drivers/input/serio/i8042-acpipnpio.h
> > +++ b/drivers/input/serio/i8042-acpipnpio.h
> > @@ -610,6 +610,14 @@ static const struct dmi_system_id i8042_dmi_quirk_table[] __initconst = {
> > },
> > .driver_data = (void *)(SERIO_QUIRK_NOMUX)
> > },
> > + {
> > + /* Fujitsu Lifebook A574/H */
> > + .matches = {
> > + DMI_MATCH(DMI_SYS_VENDOR, "FUJITSU"),
> > + DMI_MATCH(DMI_PRODUCT_NAME, "FMVA0501PZ"),
> > + },
> > + .driver_data = (void *)(SERIO_QUIRK_NOMUX)
> > + },
> > {
> > /* Gigabyte M912 */
> > .matches = {
>
^ permalink raw reply
* Re: [PATCH v2] Fix buffer overrun in HID-SENSOR name string
From: Todd Brandt @ 2023-03-15 20:35 UTC (permalink / raw)
To: Krzysztof Kozlowski, Todd Brandt, linux-input, linux-iio,
linux-kernel
Cc: srinivas.pandruvada, jic23, jikos, p.jungkamp
In-Reply-To: <cdfe3d41-5ea4-c6a8-fbab-4920d08c6303@kernel.org>
On Wed, 2023-03-15 at 08:04 +0100, Krzysztof Kozlowski wrote:
> On 13/03/2023 23:06, Todd Brandt wrote:
> > On some platforms there are some platform devices created with
> > invalid names. For example: "HID-SENSOR-INT-020b?.39.auto" instead
> > of "HID-SENSOR-INT-020b.39.auto"
> >
> > This string include some invalid characters, hence it will fail to
> > properly load the driver which will handle this custom sensor. Also
> > it is a problem for some user space tools, which parse the device
> > names from ftrace and dmesg.
> >
> > This is because the string, real_usage, is not NULL terminated and
> > printed with %s to form device name.
> >
> > To address this, we initialize the real_usage string with 0s.
> >
> > Philipp Jungkamp created this fix, I'm simply submitting it. I've
> > verified it fixes bugzilla issue 217169
> >
> > Reported-and-tested-by: Todd Brandt <todd.e.brandt@linux.intel.com>
> > Signed-off-by: Todd Brandt <todd.e.brandt@intel.com>
>
> SoB denotes that you reported this and tested. Otherwise shall we
> start
> adding Reported and Tested tags to all of our commits?
>
I just copied it from someone else's post on the mailing list, I
thought it was part of the standard lkml vernacular but I just looked
it up and it's explicitly forbidden in
Documentation/process/maintainer-top.rst line 385. So the answer is no.
I'll not use it again.
>
> Best regards,
> Krzysztof
>
^ permalink raw reply
* [PATCH] Input: support pre-stored effects
From: James Ogletree @ 2023-03-15 19:43 UTC (permalink / raw)
To: dmitry.torokhov; +Cc: linux-input, linux-kernel, James Ogletree
At present, the best way to define effects that
pre-exist in device memory is by utilizing
the custom_data field, which it was not intended
for, and requires arbitrary interpretation by
the driver to make meaningful.
Provide option for defining pre-stored effects in
device memory.
Signed-off-by: James Ogletree <james.ogletree@cirrus.com>
---
include/uapi/linux/input.h | 32 ++++++++++++++++++++++----------
1 file changed, 22 insertions(+), 10 deletions(-)
diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
index 2557eb7b0561..689e5fa10647 100644
--- a/include/uapi/linux/input.h
+++ b/include/uapi/linux/input.h
@@ -428,17 +428,27 @@ struct ff_rumble_effect {
__u16 weak_magnitude;
};
+/**
+ * struct ff_prestored_effect - defines parameters of a pre-stored force-feedback effect
+ * @index: index of effect
+ * @bank: memory bank of effect
+ */
+struct ff_prestored_effect {
+ __u16 index;
+ __u16 bank;
+};
+
/**
* struct ff_effect - defines force feedback effect
* @type: type of the effect (FF_CONSTANT, FF_PERIODIC, FF_RAMP, FF_SPRING,
- * FF_FRICTION, FF_DAMPER, FF_RUMBLE, FF_INERTIA, or FF_CUSTOM)
+ * FF_FRICTION, FF_DAMPER, FF_RUMBLE, FF_INERTIA, FF_PRESTORED, or FF_CUSTOM)
* @id: an unique id assigned to an effect
* @direction: direction of the effect
* @trigger: trigger conditions (struct ff_trigger)
* @replay: scheduling of the effect (struct ff_replay)
* @u: effect-specific structure (one of ff_constant_effect, ff_ramp_effect,
- * ff_periodic_effect, ff_condition_effect, ff_rumble_effect) further
- * defining effect parameters
+ * ff_periodic_effect, ff_condition_effect, ff_rumble_effect, ff_prestored_effect)
+ * further defining effect parameters
*
* This structure is sent through ioctl from the application to the driver.
* To create a new effect application should set its @id to -1; the kernel
@@ -464,6 +474,7 @@ struct ff_effect {
struct ff_periodic_effect periodic;
struct ff_condition_effect condition[2]; /* One for each axis */
struct ff_rumble_effect rumble;
+ struct ff_prestored_effect prestored;
} u;
};
@@ -479,20 +490,21 @@ struct ff_effect {
#define FF_DAMPER 0x55
#define FF_INERTIA 0x56
#define FF_RAMP 0x57
+#define FF_PRESTORED 0x58
#define FF_EFFECT_MIN FF_RUMBLE
-#define FF_EFFECT_MAX FF_RAMP
+#define FF_EFFECT_MAX FF_PRESTORED
/*
* Force feedback periodic effect types
*/
-#define FF_SQUARE 0x58
-#define FF_TRIANGLE 0x59
-#define FF_SINE 0x5a
-#define FF_SAW_UP 0x5b
-#define FF_SAW_DOWN 0x5c
-#define FF_CUSTOM 0x5d
+#define FF_SQUARE 0x59
+#define FF_TRIANGLE 0x5a
+#define FF_SINE 0x5b
+#define FF_SAW_UP 0x5c
+#define FF_SAW_DOWN 0x5d
+#define FF_CUSTOM 0x5e
#define FF_WAVEFORM_MIN FF_SQUARE
#define FF_WAVEFORM_MAX FF_CUSTOM
--
2.25.1
^ permalink raw reply related
* [PATCH] Input: goodix - Add Lenovo Yoga Book X90F to nine_bytes_report DMI table
From: Hans de Goede @ 2023-03-15 13:44 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Hans de Goede, Bastien Nocera, linux-input
The Android Lenovo Yoga Book X90F / X90L uses the same goodix touchscreen
with 9 bytes touch reports for its touch keyboard as the already supported
Windows Lenovo Yoga Book X91F/L, add a DMI match for this to
the nine_bytes_report DMI table.
When the quirk for the X91F/L was initially added it was written to
also apply to the X90F/L but this does not work because the Android
version of the Yoga Book uses completely different DMI strings.
Also adjust the X91F/L quirk to reflect that it only applies to
the X91F/L models.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/input/touchscreen/goodix.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 3c71f0df9769..88c223a4da61 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -128,10 +128,18 @@ static const unsigned long goodix_irq_flags[] = {
static const struct dmi_system_id nine_bytes_report[] = {
#if defined(CONFIG_DMI) && defined(CONFIG_X86)
{
- .ident = "Lenovo YogaBook",
- /* YB1-X91L/F and YB1-X90L/F */
+ /* Lenovo Yoga Book X90F / X90L */
.matches = {
- DMI_MATCH(DMI_PRODUCT_NAME, "Lenovo YB1-X9")
+ DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Intel Corporation"),
+ DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "CHERRYVIEW D1 PLATFORM"),
+ DMI_EXACT_MATCH(DMI_PRODUCT_VERSION, "YETI-11"),
+ }
+ },
+ {
+ /* Lenovo Yoga Book X91F / X91L */
+ .matches = {
+ /* Non exact match to match F + L versions */
+ DMI_MATCH(DMI_PRODUCT_NAME, "Lenovo YB1-X91"),
}
},
#endif
--
2.39.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