Re: [PATCH v1 1/1] Input: gpio-keys - expose wakeup keys in sysfs
Hi Guido, On Thu, May 09, 2024 at 02:00:28PM +0200, Guido Günther wrote: > This helps user space to figure out which keys should be used to unidle a > device. E.g on phones the volume rocker should usually not unblank the > screen. How exactly this is supposed to be used? We have "disabled" keys and switches attribute because this function can be controlled at runtime from userspace while wakeup control is a static device setting. Kernel also does not really know if the screen should be unblanked or not, if a button or switch is configured for wake up the kernel will go through wakeup process all the same and then userspace can decide if it should stay woken up or not. Thanks. -- Dmitry
Re: [RFC PATCH v3 4/5] input: add onkey driver for Marvell 88PM886 PMIC
On Mon, Mar 11, 2024 at 11:26:16AM +0100, Karel Balej wrote:
> Krzysztof Kozlowski, 2024-03-10T21:35:36+01:00:
> > On 10/03/2024 12:35, Karel Balej wrote:
> > > Dmitry Torokhov, 2024-03-04T17:10:59-08:00:
> > >> On Mon, Mar 04, 2024 at 09:28:45PM +0100, Karel Balej wrote:
> > >>> Dmitry,
> > >>>
> > >>> Dmitry Torokhov, 2024-03-03T12:39:46-08:00:
> > >>>> On Sun, Mar 03, 2024 at 11:04:25AM +0100, Karel Balej wrote:
> > >>>>> From: Karel Balej
> > >>>>>
> > >>>>> Marvell 88PM886 PMIC provides onkey among other things. Add client
> > >>>>> driver to handle it. The driver currently only provides a basic
> > >>>>> support
> > >>>>> omitting additional functions found in the vendor version, such as
> > >>>>> long
> > >>>>> onkey and GPIO integration.
> > >>>>>
> > >>>>> Signed-off-by: Karel Balej
> > >>>>> ---
> > >>>>>
> > >>>>> Notes:
> > >>>>> RFC v3:
> > >>>>> - Drop wakeup-source.
> > >>>>> RFC v2:
> > >>>>> - Address Dmitry's feedback:
> > >>>>> - Sort includes alphabetically.
> > >>>>> - Drop onkey->irq.
> > >>>>> - ret -> err in irq_handler and no initialization.
> > >>>>> - Break long lines and other formatting.
> > >>>>> - Do not clobber platform_get_irq error.
> > >>>>> - Do not set device parent manually.
> > >>>>> - Use input_set_capability.
> > >>>>> - Use the wakeup-source DT property.
> > >>>>> - Drop of_match_table.
> > >>>>
> > >>>> I only said that you should not be using of_match_ptr(), but you still
> > >>>> need to have of_match_table set and have MODULE_DEVICE_TABLE() for the
> > >>>> proper module loading support.
> > >>>
> > >>> I removed of_match_table because I no longer need compatible for this --
> > >>> there are no device tree properties and the driver is being instantiated
> > >>> by the MFD driver.
> > >>>
> > >>> Is the MODULE_DEVICE_TABLE() entry needed for the driver to probe when
> > >>> compiled as module? If that is the case, given what I write above, am I
> > >>> correct that MODULE_DEVICE_TABLE(platform,...) would be the right thing
> > >>> to use here?
> > >>
> > >> Yes, if uevent generated for the device is "platform:" then
> > >> MODULE_DEVICE_TABLE(platform,...) will suffice. I am not sure how MFD
> > >> sets it up (OF modalias or platform), but you should be able to check
> > >> the format looking at the "uevent" attribute for your device in sysfs
> > >> (/sys/devices/bus/platform/...).
> > >
> > > The uevent is indeed platform.
> > >
> > > But since there is only one device, perhaps having a device table is
> > > superfluous and using `MODULE_ALIAS("platform:88pm886-onkey")` is more
> > > fitting?
> >
> > Adding aliases for standard IDs and standard cases is almost never
> > correct. If you need module alias, it means your ID table is wrong (or
> > missing, which is usually wrong).
> >
> > >
> > > Although I don't understand why this is even necessary when the driver
> > > name is such and the module is registered using
> > > `module_platform_driver`...
> >
> > ID table and MODULE_DEVICE_TABLE() are necessary for modprobe to work.
>
> I think I understand the practical reasons. My point was that I would
> expect the alias to be added automatically even in the case that the
> device table is absent based solely on the driver name and the
> registration method (*module*_*platform*_driver). Why is that not the
> case? Obviously the driver name matching the mfd_cell name is sufficient
> for the driver to probe when it is built in so the name does seem to
> serve as some identification for the device just as a device table entry
> would.
>
> Furthermore, drivers/input/serio/ioc3kbd.c does not seem to have an ID
> table either, nor a MODULE_ALIAS -- is that a mistake? If not, what
> mechanism causes the driver to probe when compiled as a module? It seems
> to me to effectively be the same setup as with my driver and that does
> not load automatically (because of the missing alias).
Yes, ioc3kbd is broken as far as module auto-loading goes. It probably
did not get noticed before because the driver is likely to be built-in
on the target architecture.
I'll take patches.
Thanks.
--
Dmitry
Re: [RFC PATCH v3 4/5] input: add onkey driver for Marvell 88PM886 PMIC
On Sun, Mar 10, 2024 at 09:35:36PM +0100, Krzysztof Kozlowski wrote:
> On 10/03/2024 12:35, Karel Balej wrote:
> > Dmitry Torokhov, 2024-03-04T17:10:59-08:00:
> >> On Mon, Mar 04, 2024 at 09:28:45PM +0100, Karel Balej wrote:
> >>> Dmitry,
> >>>
> >>> Dmitry Torokhov, 2024-03-03T12:39:46-08:00:
> >>>> On Sun, Mar 03, 2024 at 11:04:25AM +0100, Karel Balej wrote:
> >>>>> From: Karel Balej
> >>>>>
> >>>>> Marvell 88PM886 PMIC provides onkey among other things. Add client
> >>>>> driver to handle it. The driver currently only provides a basic support
> >>>>> omitting additional functions found in the vendor version, such as long
> >>>>> onkey and GPIO integration.
> >>>>>
> >>>>> Signed-off-by: Karel Balej
> >>>>> ---
> >>>>>
> >>>>> Notes:
> >>>>> RFC v3:
> >>>>> - Drop wakeup-source.
> >>>>> RFC v2:
> >>>>> - Address Dmitry's feedback:
> >>>>> - Sort includes alphabetically.
> >>>>> - Drop onkey->irq.
> >>>>> - ret -> err in irq_handler and no initialization.
> >>>>> - Break long lines and other formatting.
> >>>>> - Do not clobber platform_get_irq error.
> >>>>> - Do not set device parent manually.
> >>>>> - Use input_set_capability.
> >>>>> - Use the wakeup-source DT property.
> >>>>> - Drop of_match_table.
> >>>>
> >>>> I only said that you should not be using of_match_ptr(), but you still
> >>>> need to have of_match_table set and have MODULE_DEVICE_TABLE() for the
> >>>> proper module loading support.
> >>>
> >>> I removed of_match_table because I no longer need compatible for this --
> >>> there are no device tree properties and the driver is being instantiated
> >>> by the MFD driver.
> >>>
> >>> Is the MODULE_DEVICE_TABLE() entry needed for the driver to probe when
> >>> compiled as module? If that is the case, given what I write above, am I
> >>> correct that MODULE_DEVICE_TABLE(platform,...) would be the right thing
> >>> to use here?
> >>
> >> Yes, if uevent generated for the device is "platform:" then
> >> MODULE_DEVICE_TABLE(platform,...) will suffice. I am not sure how MFD
> >> sets it up (OF modalias or platform), but you should be able to check
> >> the format looking at the "uevent" attribute for your device in sysfs
> >> (/sys/devices/bus/platform/...).
> >
> > The uevent is indeed platform.
> >
> > But since there is only one device, perhaps having a device table is
> > superfluous and using `MODULE_ALIAS("platform:88pm886-onkey")` is more
> > fitting?
>
> Adding aliases for standard IDs and standard cases is almost never
> correct. If you need module alias, it means your ID table is wrong (or
> missing, which is usually wrong).
>
> >
> > Although I don't understand why this is even necessary when the driver
> > name is such and the module is registered using
> > `module_platform_driver`...
>
> ID table and MODULE_DEVICE_TABLE() are necessary for modprobe to work.
> Just run `modinfo`.
MODULE_DEVICE_TABLE() and MODULE_ALIAS() reduce to the same thing, but I
agree that we should not try to be too clever and simply use the ID
table.
Thanks.
--
Dmitry
Re: [PATCH v3 0/3] Imagis touch keys and FIELD_GET cleanup
On Wed, Mar 06, 2024 at 03:40:05PM +0100, Duje Mihanović wrote:
> Tiny series to clean up the field extraction and add touch key support.
> This version is based on the next branch of Dmitry's input tree.
>
> Signed-off-by: Duje Mihanović
> ---
> Changes in v3:
> - Rebase on input/next
> - Add changelog to binding patch
> - Fix binding constraint
> - Allow changing keycodes in userspace as in 872e57abd171 ("Input:
> tm2-touchkey - allow changing keycodes from userspace")
> - Allow up to 5 keycodes (the key status field has 5 bits)
> - Link to v2:
> https://lore.kernel.org/r/20240120-b4-imagis-keys-v2-0-d7fc16f2e...@skole.hr
>
> Changes in v2:
> - Fix compile error
> - Add FIELD_GET patch
> - Allow specifying custom keycodes
> - Link to v1:
> https://lore.kernel.org/20231112194124.24916-1-duje.mihano...@skole.hr
>
> ---
> Duje Mihanović (3):
> input: touchscreen: imagis: use FIELD_GET where applicable
> dt-bindings: input: imagis: Document touch keys
> input: touchscreen: imagis: Add touch key support
>
> .../input/touchscreen/imagis,ist3038c.yaml | 19 +++--
> drivers/input/touchscreen/imagis.c | 46
> --
> 2 files changed, 50 insertions(+), 15 deletions(-)
Applied the lot, thank you.
--
Dmitry
Re: [RFC PATCH v3 4/5] input: add onkey driver for Marvell 88PM886 PMIC
On Mon, Mar 04, 2024 at 09:28:45PM +0100, Karel Balej wrote: > Dmitry, > > Dmitry Torokhov, 2024-03-03T12:39:46-08:00: > > On Sun, Mar 03, 2024 at 11:04:25AM +0100, Karel Balej wrote: > > > From: Karel Balej > > > > > > Marvell 88PM886 PMIC provides onkey among other things. Add client > > > driver to handle it. The driver currently only provides a basic support > > > omitting additional functions found in the vendor version, such as long > > > onkey and GPIO integration. > > > > > > Signed-off-by: Karel Balej > > > --- > > > > > > Notes: > > > RFC v3: > > > - Drop wakeup-source. > > > RFC v2: > > > - Address Dmitry's feedback: > > > - Sort includes alphabetically. > > > - Drop onkey->irq. > > > - ret -> err in irq_handler and no initialization. > > > - Break long lines and other formatting. > > > - Do not clobber platform_get_irq error. > > > - Do not set device parent manually. > > > - Use input_set_capability. > > > - Use the wakeup-source DT property. > > > - Drop of_match_table. > > > > I only said that you should not be using of_match_ptr(), but you still > > need to have of_match_table set and have MODULE_DEVICE_TABLE() for the > > proper module loading support. > > I removed of_match_table because I no longer need compatible for this -- > there are no device tree properties and the driver is being instantiated > by the MFD driver. > > Is the MODULE_DEVICE_TABLE() entry needed for the driver to probe when > compiled as module? If that is the case, given what I write above, am I > correct that MODULE_DEVICE_TABLE(platform,...) would be the right thing > to use here? Yes, if uevent generated for the device is "platform:" then MODULE_DEVICE_TABLE(platform,...) will suffice. I am not sure how MFD sets it up (OF modalias or platform), but you should be able to check the format looking at the "uevent" attribute for your device in sysfs (/sys/devices/bus/platform/...). Thanks. -- Dmitry
Re: [RESEND PATCH v5 0/5] input/touchscreen: imagis: add support for IST3032C
On Fri, Mar 01, 2024 at 05:40:59PM +0100, Karel Balej wrote: > From: Karel Balej > > Hello, > > this patch series generalizes the Imagis touchscreen driver to support > other Imagis chips, namely IST3038B and IST3032C. > > The motivation for IST3032C is the samsung,coreprimevelte smartphone > with which this series has been tested. However, the support for this > device is not yet in-tree, the effort is happening at [1]. Preliminary > version of the regulator driver needed to use the touchscreen on this > phone can be found here [2]. > > Note that this is a prerequisite for (at least a part of) this series > [3] which among other things implements support for touch keys for > Imagis touchscreens that have it. > > [1] > https://lore.kernel.org/all/20240110-pxa1908-lkml-v8-0-fea768a59...@skole.hr/ > [2] > https://lore.kernel.org/all/20240211094609.2223-1-kar...@gimli.ms.mff.cuni.cz/ > [3] > https://lore.kernel.org/all/20240120-b4-imagis-keys-v2-0-d7fc16f2e...@skole.hr/ Applied the lot, thank you. -- Dmitry
Re: [RFC PATCH v3 4/5] input: add onkey driver for Marvell 88PM886 PMIC
Hi Karel, On Sun, Mar 03, 2024 at 11:04:25AM +0100, Karel Balej wrote: > From: Karel Balej > > Marvell 88PM886 PMIC provides onkey among other things. Add client > driver to handle it. The driver currently only provides a basic support > omitting additional functions found in the vendor version, such as long > onkey and GPIO integration. > > Signed-off-by: Karel Balej > --- > > Notes: > RFC v3: > - Drop wakeup-source. > RFC v2: > - Address Dmitry's feedback: > - Sort includes alphabetically. > - Drop onkey->irq. > - ret -> err in irq_handler and no initialization. > - Break long lines and other formatting. > - Do not clobber platform_get_irq error. > - Do not set device parent manually. > - Use input_set_capability. > - Use the wakeup-source DT property. > - Drop of_match_table. I only said that you should not be using of_match_ptr(), but you still need to have of_match_table set and have MODULE_DEVICE_TABLE() for the proper module loading support. With that fixed: Acked-by: Dmitry Torokhov Thanks. -- Dmitry
Re: [RFC PATCH 4/5] input: add onkey driver for Marvell 88PM88X PMICs
Hi Karel,
On Sun, Dec 17, 2023 at 02:17:02PM +0100, Karel Balej wrote:
> From: Karel Balej
>
> The Marvell 88PM88X PMICs provide onkey among other things. Add client
> driver to handle it. The driver currently only provides a basic support
> omitting additional functions found in the vendor version, such as long
> onkey and GPIO integration.
>
> Signed-off-by: Karel Balej
> ---
> drivers/input/misc/88pm88x-onkey.c | 103 +
> drivers/input/misc/Kconfig | 10 +++
> drivers/input/misc/Makefile| 1 +
> 3 files changed, 114 insertions(+)
> create mode 100644 drivers/input/misc/88pm88x-onkey.c
>
> diff --git a/drivers/input/misc/88pm88x-onkey.c
> b/drivers/input/misc/88pm88x-onkey.c
> new file mode 100644
> index ..0d6056a3cab2
> --- /dev/null
> +++ b/drivers/input/misc/88pm88x-onkey.c
> @@ -0,0 +1,103 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
Please sort alphabetically.
> +
> +#include
> +
> +struct pm88x_onkey {
> + struct input_dev *idev;
> + struct pm88x_chip *chip;
> + int irq;
Since you are using devm to request interrupt I do not think you need to
store it here.
> +};
> +
> +static irqreturn_t pm88x_onkey_irq_handler(int irq, void *data)
> +{
> + struct pm88x_onkey *onkey = data;
> + unsigned int val;
> + int ret = 0;
"error". Also no need to initialize it to 0.
> +
> + ret = regmap_read(onkey->chip->regmaps[PM88X_REGMAP_BASE],
> PM88X_REG_STATUS1, );
I still prefer to keep withing 80 columns, unless longer lines are a
clear win.
> + if (ret) {
> + dev_err(onkey->idev->dev.parent, "Failed to read status: %d\n",
> ret);
Maybe have "dev" in onkey instead of poking through input?
> + return IRQ_NONE;
> + }
> + val &= PM88X_ONKEY_STS1;
> +
> + input_report_key(onkey->idev, KEY_POWER, val);
> + input_sync(onkey->idev);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int pm88x_onkey_probe(struct platform_device *pdev)
> +{
> + struct pm88x_chip *chip = dev_get_drvdata(pdev->dev.parent);
> + struct pm88x_onkey *onkey;
> + int err;
> +
> + onkey = devm_kzalloc(>dev, sizeof(*onkey), GFP_KERNEL);
> + if (!onkey)
> + return -ENOMEM;
> +
> + onkey->chip = chip;
> +
> + onkey->irq = platform_get_irq(pdev, 0);
> + if (onkey->irq < 0) {
> + dev_err(>dev, "Failed to get IRQ\n");
> + return -EINVAL;
Do not clobber the return from platform_get_irq(). So "return
onkey->irq" (or simply irq if you drop it from onkey and use temporary).
> + }
> +
> + onkey->idev = devm_input_allocate_device(>dev);
> + if (!onkey->idev) {
> + dev_err(>dev, "Failed to allocate input device\n");
> + return -ENOMEM;
> + }
> +
> + onkey->idev->name = "88pm88x-onkey";
> + onkey->idev->phys = "88pm88x-onkey/input0";
> + onkey->idev->id.bustype = BUS_I2C;
> + onkey->idev->dev.parent = >dev;
No need to do this since you are using devm_input_allocate_device().
> + onkey->idev->evbit[0] = BIT_MASK(EV_KEY);
> + onkey->idev->keybit[BIT_WORD(KEY_POWER)] = BIT_MASK(KEY_POWER);
Please use
input_set_capability(onkey->idev, EV_KEY, KEY_POWER);
> +
> + err = devm_request_threaded_irq(>dev, onkey->irq, NULL,
> pm88x_onkey_irq_handler,
> + IRQF_ONESHOT | IRQF_NO_SUSPEND, "onkey", onkey);
Please align arguments.
> + if (err) {
> + dev_err(>dev, "Failed to request IRQ: %d\n", err);
I was persuaded to stop my crusade against dev_err_probe() so you may
use it here.
> + return err;
> + }
> +
> + err = input_register_device(onkey->idev);
> + if (err) {
> + dev_err(>dev, "Failed to register input device: %d\n",
> err);
> + return err;
> + }
> +
> + device_init_wakeup(>dev, 1);
Are there cases where we woudl not want wakeup? Maybe use standard
"wakeup-source" property?
> +
> + return 0;
> +}
> +
> +static const struct of_device_id pm88x_onkey_of_match[] = {
> + { .compatible = "marvell,88pm88x-onkey", },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, pm88x_onkey_of_match);
> +
> +static struct platform_driver pm88x_onkey_driver = {
> + .driver = {
> + .name = "88pm88x-onkey",
> + .of_match_table = of_match_ptr(pm88x_onkey_of_match),
Given that you are not guarding pm88x_onkey_of_match definition with
#ifdef CONFIG_OF you shoudl not use of_match_ptr().
Thanks.
--
Dmitry
Re: [PATCH] driver: input: touchscreen: modify Raydium i2c touchscreen driver
HI,
On Thu, Apr 15, 2021 at 04:58:29PM +0800, simba.hsu wrote:
> This path makes auto-update available when IC's status is
> Recovery mode.
Could you please explain in more detail what the issue is. Also please
improve the patch subject, as "modify Raydium i2c touchscreen driver"
does not really convey the substance of the patch.
Also, the patch does not apply to my tree, was it based on some other
tree?
>
> Signed-off-by: simba@raydium.corp-partner.google.com
Please use your Raydium email for signoff.
> Change-Id: I5ae54896a201b949eba7514500a7e75574f5726b
No need to send change-id tags, they are not used in mainline kernel.
> ---
> drivers/input/touchscreen/raydium_i2c_ts.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/input/touchscreen/raydium_i2c_ts.c
> b/drivers/input/touchscreen/raydium_i2c_ts.c
> index 79ef699e..a97403c55f75 100644
> --- a/drivers/input/touchscreen/raydium_i2c_ts.c
> +++ b/drivers/input/touchscreen/raydium_i2c_ts.c
> @@ -298,6 +298,7 @@ static int raydium_i2c_query_ts_BL_info(struct
> raydium_data *ts)
>0x10, 0xc0, 0x01, 0x00, 0x04, 0x00};
> int error;
> u8 rbuf[5] = {0, 0, 0, 0, 0};
> + u32 tmpdata = 0;
>
> error = raydium_i2c_send(client,
>RM_CMD_BOOT_WRT, get_hwid, sizeof(get_hwid));
> @@ -315,7 +316,8 @@ static int raydium_i2c_query_ts_BL_info(struct
> raydium_data *ts)
> error = raydium_i2c_read(client,
>RM_CMD_BOOT_CHK, rbuf, sizeof(rbuf));
> if (!error) {
> - ts->info.hw_ver =
> cpu_to_le32(rbuf[1]<<24|rbuf[2]<<16|rbuf[3]<<8|rbuf[4]);
> + tmpdata = (rbuf[1]<<24|rbuf[2]<<16|rbuf[3]<<8|rbuf[4]);
> + ts->info.hw_ver = cpu_to_le32(tmpdata);
On the face of it I can't see why the code would behave differently,
nut then there is no raydium_i2c_query_ts_BL_info() in the copy of the
driver I have here.
> dev_err(>dev, "HWID %08X\n", ts->info.hw_ver);
> } else {
> ts->info.hw_ver = cpu_to_le32(0xUL);
> --
> 2.25.1
>
Thanks.
--
Dmitry
[git pull] Input updates for v5.12-rc7
Hi Linus, Please pull from: git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git for-linus to receive updates for the input subsystem. Just a few driver fixes here. Changelog: - Arnd Bergmann (1): Input: i8042 - fix Pegatron C15B ID entry Caleb Connolly (1): Input: s6sy761 - fix coordinate read bit shift Dmitry Osipenko (2): Input: elants_i2c - fix division by zero if firmware reports zero phys size Input: elants_i2c - drop zero-checking of ABS_MT_TOUCH_MAJOR resolution Fabian Vogt (1): Input: nspire-keypad - enable interrupts only when opened Wei Yongjun (1): Input: n64joy - fix return value check in n64joy_probe() Diffstat: drivers/input/joystick/n64joy.c| 4 +-- drivers/input/keyboard/nspire-keypad.c | 56 +++--- drivers/input/serio/i8042-x86ia64io.h | 1 + drivers/input/touchscreen/elants_i2c.c | 5 ++- drivers/input/touchscreen/s6sy761.c| 4 +-- 5 files changed, 38 insertions(+), 32 deletions(-) Thanks. -- Dmitry
Re: [Ping for Dmitry] Re: [PATCH v5 3/3] iio: adc: add ADC driver for the TI TSC2046 controller
Hi Oleksij, Jonathan, On Tue, Apr 13, 2021 at 11:31:05AM +0200, Oleksij Rempel wrote: > Hi Dmitry, > > probably this mail passed under your radar. Can you please add your > statement here. Sorry, my bad, I saw "iio" and thought there is nothing for me to comment on ;) > > On Mon, Mar 29, 2021 at 11:58:26AM +0100, Jonathan Cameron wrote: > > On Mon, 29 Mar 2021 09:31:31 +0200 > > Oleksij Rempel wrote: > > > > > Basically the TI TSC2046 touchscreen controller is 8 channel ADC > > > optimized for > > > the touchscreen use case. By implementing it as an IIO ADC device, we can > > > make use of resistive-adc-touch and iio-hwmon drivers. > > > > > > Polled readings are currently not implemented to keep this patch small, so > > > iio-hwmon will not work out of the box for now. > > > > > > So far, this driver was tested with a custom version of > > > resistive-adc-touch driver, > > > since it needs to be extended to make use of Z1 and Z2 channels. The X/Y > > > are working without additional changes. > > > > > > Signed-off-by: Oleksij Rempel > > > Reviewed-by: Andy Shevchenko > > Hi Oleksij, > > > > Couple of things in here I missed before, but big question is still whether > > Dmitry is happy with what you mention in the cover letter: > > > > "This driver can replace drivers/input/touchscreen/ads7846.c and has > > following advantages over it: > > - less code to maintain > > - shared code paths (resistive-adc-touch, iio-hwmon, etc) > > - can be used as plain IIO ADC to investigate signaling issues or test > > real capacity of the plates and attached low-pass filters > > (or use the touchscreen as a microphone if you like ;) )" I am all for code unification and reuse, so please go ahead. If there are regressions we can re-evaluate and see if they can be addressed in this driver or if we need to resurrect ads7846. Thanks. -- Dmitry
Re: [PATCH v7 3/3] Input: add driver for the Hycon HY46XX touchpanel series
Hi Giulio, Peter, On Wed, Apr 14, 2021 at 01:22:55PM +0200, Giulio Benetti wrote: > Hi Peter, Dmitry, > > On 4/14/21 8:46 AM, Peter Hutterer wrote: > > On Tue, Apr 13, 2021 at 10:44:07PM -0700, Dmitry Torokhov wrote: > > > Hi Giulio, > > > > > > On Tue, Apr 13, 2021 at 04:44:46PM +0200, Giulio Benetti wrote: > > > > + > > > > + input_mt_report_pointer_emulation(tsdata->input, true); > > > > > > For touchscreens it does not make much sense to report BTN_DOUBLETAP, > > > BTN_TRIPLETAP, etc, events (they are really for touchpads), so I changed > > > this to > > > > > > input_mt_report_pointer_emulation(tsdata->input, false); > > > > > > to only report ABS_X, ABS_Y, and BTN_TOUCH, and applied. > > > > Can you expand on this please, just to make sure I'm not misinterpreting > > those codes? Those bits are just for how many fingers are down (but without > > position), dropping those bits means you restrict the device to a pure > > single-touch screen. Or am I missing something here? They are indeed represent number of fingers on the surface. I think I over-simplified this a bit by saying these events are only for touchpads, as there is allowance for BTN_TOOL_*TAP in Documentation/input/multi-touch-protocol.rst for MT devices that may report more contacts than what they can distinctly track, and it is not restricted to touchpads (but I believe in reality only used by a couple of "semi-MT" touchpad drivers). What I meant to say that for ST fallback of MT-capable devices we typically provide BTN_TOOL_*TAP for devices declared as INPUT_MT_POINTER (which is touchpads) and for INPUT_MT_DIRECT and others we only provide ABS_X, ABS_Y, ABS_PRESSURE and BTN_TOUCH (see input_mt_sync_frame()), and I think this driver should follow the suit. > > I've re-tested the driver after setting input_mt_report_pointer_emulation() > use_count to false. It works correctly with all touch points expected. That > use_count refers to finger count of Touchpad[1]. Also you can see that even > with use_count=false this for loop[2] is entered by counting all the > mt->slots and then input_event() reports all of them. That is not quite true. The loop in question locates the oldest contact still on the surface and MT->ST mapping uses it as the "primary" point for ST events. It is reported in addition to sending all contacts via MT protocol as ABS_MT_SLOT, ABS_MT_POSITION_X, ABS_MT_POSITION_Y, ... > > I hope I've understood correctly :-) > > [1]: > https://elixir.bootlin.com/linux/v5.12-rc7/source/drivers/input/input-mt.c#L190 > [2]: > https://elixir.bootlin.com/linux/v5.12-rc7/source/drivers/input/input-mt.c#L208 > > > then again, MT support has been in the kernel for long enough that by now > > everything should understand it, so there's a certain "meh" factor. Yeah, there is that too. Thanks. -- Dmitry
Re: [PATCH v7 2/3] dt-bindings: touchscreen: Add HY46XX bindings
On Tue, Apr 13, 2021 at 04:44:45PM +0200, Giulio Benetti wrote: > This adds device tree bindings for the Hycon HY46XX touchscreen series. > > Signed-off-by: Giulio Benetti Applied, thank you. -- Dmitry
Re: [PATCH v7 1/3] dt-bindings: Add Hycon Technology vendor prefix
On Tue, Apr 13, 2021 at 04:44:44PM +0200, Giulio Benetti wrote: > Update Documentation/devicetree/bindings/vendor-prefixes.yaml to > include "hycon" as a vendor prefix for "Hycon Technology". > Company website: https://www.hycontek.com/ > > Signed-off-by: Giulio Benetti > Reviewed-by: Jonathan Neuschäfer > Acked-by: Rob Herring Applied, thank you. -- Dmitry
Re: [PATCH v7 3/3] Input: add driver for the Hycon HY46XX touchpanel series
Hi Giulio, On Tue, Apr 13, 2021 at 04:44:46PM +0200, Giulio Benetti wrote: > + > + input_mt_report_pointer_emulation(tsdata->input, true); For touchscreens it does not make much sense to report BTN_DOUBLETAP, BTN_TRIPLETAP, etc, events (they are really for touchpads), so I changed this to input_mt_report_pointer_emulation(tsdata->input, false); to only report ABS_X, ABS_Y, and BTN_TOUCH, and applied. Thanks. -- Dmitry
Re: [PATCH v2 4/6] devres: handle zero size in devm_kmalloc()
Hi Bartosz, On Mon, Jun 29, 2020 at 1:56 PM Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski > > Make devm_kmalloc() behave similarly to non-managed kmalloc(): return > ZERO_SIZE_PTR when requested size is 0. Update devm_kfree() to handle > this case. This is wrong if you consider devm_krealloc API that you added. The premise of devm_krealloc() is that it does not disturb devres "stack", however in this case there is no entry in the stack. Consider: ptr = devm_kzalloc(dev, 0, GFP_KERNEL); ... more devm API calls ... /* This allocation will be on top of devm stack, not bottom ! */ ptr = devm_krealloc(dev, ptr, 16, GFP_KERNEL); And also: ptr = devm_kzalloc(dev, 16, GFP_KERNEL); ... more devm API calls ... /* Here we lose out position */ ptr = devm_krealloc(dev, ptr, 0, GFP_KERNEL); ... /* and now our memory allocation will be released first */ ptr = devm_krealloc(dev, ptr, 16, GFP_KERNEL); IMO special-casing 0-size allocations for managed memory allocations should not be done. Thanks. -- Dmitry
[PATCH 1/2] HID: hid-input: add mapping for emoji picker key
HUTRR101 added a new usage code for a key that is supposed to invoke and
dismiss an emoji picker widget to assist users to locate and enter emojis.
This patch adds a new key definition KEY_EMOJI_PICKER and maps 0x0c/0x0d9
usage code to this new keycode. Additionally hid-debug is adjusted to
recognize this new usage code as well.
Signed-off-by: Dmitry Torokhov
---
drivers/hid/hid-debug.c| 1 +
drivers/hid/hid-input.c| 3 +++
include/uapi/linux/input-event-codes.h | 1 +
3 files changed, 5 insertions(+)
diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index d7eaf9100370..982737827b87 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -929,6 +929,7 @@ static const char *keys[KEY_MAX + 1] = {
[KEY_APPSELECT] = "AppSelect",
[KEY_SCREENSAVER] = "ScreenSaver",
[KEY_VOICECOMMAND] = "VoiceCommand",
+ [KEY_EMOJI_PICKER] = "EmojiPicker",
[KEY_BRIGHTNESS_MIN] = "BrightnessMin",
[KEY_BRIGHTNESS_MAX] = "BrightnessMax",
[KEY_BRIGHTNESS_AUTO] = "BrightnessAuto",
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 236bccd37760..e982d8173c9c 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -963,6 +963,9 @@ static void hidinput_configure_usage(struct hid_input
*hidinput, struct hid_fiel
case 0x0cd: map_key_clear(KEY_PLAYPAUSE); break;
case 0x0cf: map_key_clear(KEY_VOICECOMMAND);break;
+
+ case 0x0d9: map_key_clear(KEY_EMOJI_PICKER);break;
+
case 0x0e0: map_abs_clear(ABS_VOLUME); break;
case 0x0e2: map_key_clear(KEY_MUTE);break;
case 0x0e5: map_key_clear(KEY_BASSBOOST); break;
diff --git a/include/uapi/linux/input-event-codes.h
b/include/uapi/linux/input-event-codes.h
index ee93428ced9a..225ec87d4f22 100644
--- a/include/uapi/linux/input-event-codes.h
+++ b/include/uapi/linux/input-event-codes.h
@@ -611,6 +611,7 @@
#define KEY_VOICECOMMAND 0x246 /* Listening Voice Command */
#define KEY_ASSISTANT 0x247 /* AL Context-aware desktop assistant */
#define KEY_KBD_LAYOUT_NEXT0x248 /* AC Next Keyboard Layout Select */
+#define KEY_EMOJI_PICKER 0x249 /* Show/hide emoji picker (HUTRR101) */
#define KEY_BRIGHTNESS_MIN 0x250 /* Set Brightness to Minimum */
#define KEY_BRIGHTNESS_MAX 0x251 /* Set Brightness to Maximum */
--
2.31.1.295.g9ea45b61b8-goog
[PATCH 2/2] HID: hid-debug: recognize KEY_ASSISTANT and KEY_KBD_LAYOUT_NEXT
Add missing descriptions for KEY_ASSISTANT and KEY_KBD_LAYOUT_NEXT.
Signed-off-by: Dmitry Torokhov
---
drivers/hid/hid-debug.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index 982737827b87..4d2b699daf8d 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -929,6 +929,8 @@ static const char *keys[KEY_MAX + 1] = {
[KEY_APPSELECT] = "AppSelect",
[KEY_SCREENSAVER] = "ScreenSaver",
[KEY_VOICECOMMAND] = "VoiceCommand",
+ [KEY_ASSISTANT] = "Assistant",
+ [KEY_KBD_LAYOUT_NEXT] = "KbdLayoutNext",
[KEY_EMOJI_PICKER] = "EmojiPicker",
[KEY_BRIGHTNESS_MIN] = "BrightnessMin",
[KEY_BRIGHTNESS_MAX] = "BrightnessMax",
--
2.31.1.295.g9ea45b61b8-goog
Re: [PATCH] Input: apbps2 - remove useless variable
On Fri, Apr 09, 2021 at 05:00:59PM +0800, Jiapeng Chong wrote: > Fix the following gcc warning: > > drivers/input/serio/apbps2.c:106:16: warning: variable ‘tmp’ set but not > used [-Wunused-but-set-variable]. > > Reported-by: Abaci Robot > Signed-off-by: Jiapeng Chong Applied, thank you. -- Dmitry
Re: [PATCH v6 2/2] Input: add MStar MSG2638 touchscreen driver
Hi Vincent,
On Fri, Mar 05, 2021 at 04:38:05PM +0100, Vincent Knecht wrote:
> Add support for the msg2638 touchscreen IC from MStar.
> Firmware handling, wakeup gestures and other specialties are not supported.
> This driver reuses zinitix.c structure, while the checksum and irq handler
> functions are based on out-of-tree driver for Alcatel Idol 3 (4.7").
>
> Signed-off-by: Vincent Knecht
> ---
> Changed in v6:
> - minor formatting changes
> - mention wakeup gestures not being supported (yet?) in commit message
> - do not scale coordinates in the driver (Dmitry)
> - multiple suggestions from Linus W.
> - include linux/gpio/consumer.h instead of linux/gpio.h
> - rename delay defines to include time unit like _MS and _US
> - rename `error` variable to `ret`
> - move enable_irq() call from msg2638_power_on() to msg2638_start()
> - remove CONFIG_OF #ifdef around of_device_id struct
> Changed in v5:
> - use gpiod_set_value_cansleep() (Stephan G)
> - use msleep/usleep_range() rathen than mdelay() (Stephan G)
> Changed in v4:
> - rename from msg26xx to msg2638, following compatible string change
> - rename mstar_* functions to msg2638_* for consistency
> - add RESET_DELAY define and use it in msg2638_power_on()
> - change a few dev_err() calls to be on one line only
> - add missing \n in a few dev_err() strings
> Changed in v3:
> - no change
> Changed in v2:
> - don't use bitfields in packet struct, to prevent endian-ness related
> problems (Dmitry)
> - fix reset gpiod calls order in mstar_power_on() (Dmitry)
> ---
> drivers/input/touchscreen/Kconfig | 12 +
> drivers/input/touchscreen/Makefile | 1 +
> drivers/input/touchscreen/msg2638.c | 354
> 3 files changed, 367 insertions(+)
> create mode 100644 drivers/input/touchscreen/msg2638.c
>
> diff --git a/drivers/input/touchscreen/Kconfig
> b/drivers/input/touchscreen/Kconfig
> index f012fe746df0..fefbe1c1bb10 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -1334,4 +1334,16 @@ config TOUCHSCREEN_ZINITIX
> To compile this driver as a module, choose M here: the
> module will be called zinitix.
>
> +config TOUCHSCREEN_MSG2638
> + tristate "MStar msg2638 touchscreen support"
> + depends on I2C
> + depends on GPIOLIB || COMPILE_TEST
> + help
> + Say Y here if you have an I2C touchscreen using MStar msg2638.
> +
> + If unsure, say N.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called msg2638.
> +
> endif
> diff --git a/drivers/input/touchscreen/Makefile
> b/drivers/input/touchscreen/Makefile
> index 6233541e9173..83e516cb3d33 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -112,3 +112,4 @@ obj-$(CONFIG_TOUCHSCREEN_ROHM_BU21023)+=
> rohm_bu21023.o
> obj-$(CONFIG_TOUCHSCREEN_RASPBERRYPI_FW) += raspberrypi-ts.o
> obj-$(CONFIG_TOUCHSCREEN_IQS5XX) += iqs5xx.o
> obj-$(CONFIG_TOUCHSCREEN_ZINITIX)+= zinitix.o
> +obj-$(CONFIG_TOUCHSCREEN_MSG2638)+= msg2638.o
> diff --git a/drivers/input/touchscreen/msg2638.c
> b/drivers/input/touchscreen/msg2638.c
> new file mode 100644
> index ..8eb3f195d743
> --- /dev/null
> +++ b/drivers/input/touchscreen/msg2638.c
> @@ -0,0 +1,354 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Driver for MStar msg2638 touchscreens
> + *
> + * Copyright (c) 2021 Vincent Knecht
> + *
> + * Checksum and IRQ handler based on mstar_drv_common.c and
> mstar_drv_mutual_fw_control.c
> + * Copyright (c) 2006-2012 MStar Semiconductor, Inc.
> + *
> + * Driver structure based on zinitix.c by Michael Srba
>
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define MODE_DATA_RAW0x5A
> +
> +#define MAX_SUPPORTED_FINGER_NUM 5
> +
> +#define CHIP_ON_DELAY_MS 15
> +#define FIRMWARE_ON_DELAY_MS 50
> +#define RESET_DELAY_MIN_US 1
> +#define RESET_DELAY_MAX_US 11000
> +
> +struct point_coord {
> + u16 x;
> + u16 y;
> +};
> +
> +struct packet {
> + u8 xy_hi; /* higher bits of x and y coordinates */
> + u8 x_low;
> + u8 y_low;
> + u8 pressure;
> +};
> +
> +struct touch_event {
> + u8 mode;
> + struct packet pkt[MAX_SUPPORTED_FINGER_NUM];
> + u8 proximity;
> + u8 checksum;
> +};
> +
> +struct msg2638_ts_data {
> + struct i2c_client *client;
> + struct input_dev *input_dev;
> + struct touchscreen_properties prop;
> + struct regulator_bulk_data supplies[2];
> + struct gpio_desc *reset_gpiod;
> +};
> +
> +static int msg2638_init_regulators(struct msg2638_ts_data *msg2638)
> +{
> + struct i2c_client *client = msg2638->client;
> + int ret;
> +
> +
Re: [PATCH 1/3] input: pm8941-pwrkey: add support for PMK8350 PON_HLOS PMIC peripheral
Hi Satya, On Wed, Apr 07, 2021 at 08:59:39PM +0530, ska...@codeaurora.org wrote: > Gentle Reminder! Sorry, please address Rob's comments on the bindings, the driver code looks OK to me. Thanks. -- Dmitry
Re: [PATCH] Input: cyapa - Fix rumtime PM imbalance on error
Hi Dinghao,
On Wed, Apr 07, 2021 at 12:07:38PM +0800, Dinghao Liu wrote:
> When mutex_lock_interruptible() fails, a pairing PM usage
> counter decrement is needed to keep the counter balanced.
Thank you for the patch.
>
> Signed-off-by: Dinghao Liu
> ---
> drivers/input/mouse/cyapa.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/input/mouse/cyapa.c b/drivers/input/mouse/cyapa.c
> index 77cc653edca2..e411ab45a218 100644
> --- a/drivers/input/mouse/cyapa.c
> +++ b/drivers/input/mouse/cyapa.c
> @@ -904,8 +904,10 @@ static ssize_t cyapa_update_rt_suspend_scanrate(struct
> device *dev,
> pm_runtime_get_sync(dev);
>
> error = mutex_lock_interruptible(>state_sync_lock);
> - if (error)
> + if (error) {
> + pm_runtime_put_noidle(dev);
Why "noidle" and not pm_runtime_put_sync_autosuspend() like we do in
case of normal flow?
> return error;
> + }
>
> cyapa->runtime_suspend_sleep_time = min_t(u16, time, 1000);
> cyapa->runtime_suspend_power_mode =
> --
> 2.17.1
>
Thanks.
--
Dmitry
[PATCH] Input: gpio-keys - fix crash when disabliing GPIO-less buttons
My brain-damaged adjustments to Paul's patch caused crashes in
gpio_keys_disable_button() when driver is used in GPIO-less (i.e.
purely interrupt-driven) setups, because I mixed together debounce and
release timers when they are in fact separate:
Unable to handle kernel NULL pointer dereference at virtual address 000c
...
PC is at hrtimer_active+0xc/0x98
LR is at hrtimer_try_to_cancel+0x24/0x140
...
[] (hrtimer_active) from []
(hrtimer_try_to_cancel+0x24/0x140)
[] (hrtimer_try_to_cancel) from []
(hrtimer_cancel+0x14/0x4c)
[] (hrtimer_cancel) from []
(gpio_keys_attr_store_helper+0x1b8/0x1d8 [gpio_keys])
[] (gpio_keys_attr_store_helper [gpio_keys]) from []
(gpio_keys_store_disabled_keys+0x18/0x24 [gpio_keys])
[] (gpio_keys_store_disabled_keys [gpio_keys]) from []
(kernfs_fop_write_iter+0x10c/0x1cc)
[] (kernfs_fop_write_iter) from [] (vfs_write+0x2ac/0x404)
[] (vfs_write) from [] (ksys_write+0x64/0xdc)
[] (ksys_write) from [] (ret_fast_syscall+0x0/0x58)
Let's fix it up.
Fixes: c9efb0ba281e ("Input: gpio-keys - use hrtimer for software debounce, if
possible")
Reported-by: Tony Lindgren
Signed-off-by: Dmitry Torokhov
---
Tony, could you please try this patch and see if it fixes the crash you
observed?
Thanks!
drivers/input/keyboard/gpio_keys.c | 30 +-
1 file changed, 13 insertions(+), 17 deletions(-)
diff --git a/drivers/input/keyboard/gpio_keys.c
b/drivers/input/keyboard/gpio_keys.c
index fe8fc76ee22e..8dbf1e69c90a 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -125,6 +125,18 @@ static const unsigned long *get_bm_events_by_type(struct
input_dev *dev,
return (type == EV_KEY) ? dev->keybit : dev->swbit;
}
+static void gpio_keys_quiesce_key(void *data)
+{
+ struct gpio_button_data *bdata = data;
+
+ if (!bdata->gpiod)
+ hrtimer_cancel(>release_timer);
+ if (bdata->debounce_use_hrtimer)
+ hrtimer_cancel(>debounce_timer);
+ else
+ cancel_delayed_work_sync(>work);
+}
+
/**
* gpio_keys_disable_button() - disables given GPIO button
* @bdata: button data for button to be disabled
@@ -145,12 +157,7 @@ static void gpio_keys_disable_button(struct
gpio_button_data *bdata)
* Disable IRQ and associated timer/work structure.
*/
disable_irq(bdata->irq);
-
- if (bdata->debounce_use_hrtimer)
- hrtimer_cancel(>release_timer);
- else
- cancel_delayed_work_sync(>work);
-
+ gpio_keys_quiesce_key(bdata);
bdata->disabled = true;
}
}
@@ -492,16 +499,6 @@ static irqreturn_t gpio_keys_irq_isr(int irq, void *dev_id)
return IRQ_HANDLED;
}
-static void gpio_keys_quiesce_key(void *data)
-{
- struct gpio_button_data *bdata = data;
-
- if (bdata->debounce_use_hrtimer)
- hrtimer_cancel(>debounce_timer);
- else
- cancel_delayed_work_sync(>work);
-}
-
static int gpio_keys_setup_key(struct platform_device *pdev,
struct input_dev *input,
struct gpio_keys_drvdata *ddata,
@@ -635,7 +632,6 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
}
bdata->release_delay = button->debounce_interval;
- bdata->debounce_use_hrtimer = true;
hrtimer_init(>release_timer,
CLOCK_REALTIME, HRTIMER_MODE_REL_HARD);
bdata->release_timer.function = gpio_keys_irq_timer;
--
2.31.0.208.g409f899ff0-goog
--
Dmitry
Re: [PATCH v3 3/3] input: gpio-keys: Use hrtimer for software debounce, if possible
On Tue, Apr 06, 2021 at 11:37:07AM +0300, Tony Lindgren wrote: > Hi, > > * Dmitry Torokhov [700101 02:00]: > > On Sun, Mar 07, 2021 at 10:22:40PM +, Paul Cercueil wrote: > > > We want to be able to report the input event as soon as the debounce > > > delay elapsed. However, the current code does not really ensure that, > > > as it uses the jiffies-based schedule_delayed_work() API. With a small > > > enough HZ value (HZ <= 100), this results in some input events being > > > lost, when a key is quickly pressed then released (on a human's time > > > scale). > > > > > > Switching to hrtimers fixes this issue, and will work even on extremely > > > low HZ values (tested at HZ=24). This is however only possible if > > > reading the GPIO is possible without sleeping. If this condition is not > > > met, the previous approach of using a jiffies-based timer is taken. > > > > > > Signed-off-by: Paul Cercueil > > > > Applied with minor edits to make more use of debounce_use_hrtimer flag. > > While testing Linux next I noticed that this patch causes a null pointer > dereference at least when unbinding a gpio-keys instance, see below. Ugh, my "minor edits" did screw things up ;( as I mixed up release and debounce timers. I'll fix it up. > > Regards, > > Tony > > 8< - > Unable to handle kernel NULL pointer dereference at virtual address 000c > ... > PC is at hrtimer_active+0xc/0x98 > LR is at hrtimer_try_to_cancel+0x24/0x140 > ... > [] (hrtimer_active) from [] > (hrtimer_try_to_cancel+0x24/0x140) > [] (hrtimer_try_to_cancel) from [] > (hrtimer_cancel+0x14/0x4c) > [] (hrtimer_cancel) from [] > (gpio_keys_attr_store_helper+0x1b8/0x1d8 [gpio_keys]) > [] (gpio_keys_attr_store_helper [gpio_keys]) from [] > (gpio_keys_store_disabled_keys+0x18/0x24 [gpio_keys]) > [] (gpio_keys_store_disabled_keys [gpio_keys]) from [] > (kernfs_fop_write_iter+0x10c/0x1cc) > [] (kernfs_fop_write_iter) from [] (vfs_write+0x2ac/0x404) > [] (vfs_write) from [] (ksys_write+0x64/0xdc) > [] (ksys_write) from [] (ret_fast_syscall+0x0/0x58) > -- Dmitry
Re: [PATCH v4 07/10] Input: wacom_i2c - Add support for reset control
Hi Alistair,
On Thu, Mar 25, 2021 at 09:52:27PM -0400, Alistair Francis wrote:
> From: Alistair Francis
>
> Signed-off-by: Alistair Francis
> ---
> v4:
> - Initial commit
>
> drivers/input/touchscreen/wacom_i2c.c | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/input/touchscreen/wacom_i2c.c
> b/drivers/input/touchscreen/wacom_i2c.c
> index 84c7ccb737bd..28004b1180c9 100644
> --- a/drivers/input/touchscreen/wacom_i2c.c
> +++ b/drivers/input/touchscreen/wacom_i2c.c
> @@ -55,6 +55,7 @@ struct wacom_features {
> struct wacom_i2c {
> struct i2c_client *client;
> struct input_dev *input;
> + struct reset_control *rstc;
> struct touchscreen_properties props;
> u8 data[WACOM_QUERY_SIZE];
> bool prox;
> @@ -175,6 +176,8 @@ static int wacom_i2c_open(struct input_dev *dev)
> struct wacom_i2c *wac_i2c = input_get_drvdata(dev);
> struct i2c_client *client = wac_i2c->client;
>
> + reset_control_reset(wac_i2c->rstc);
Why does this device need to be reset on every open compared to doing it
in probe?
> +
> enable_irq(client->irq);
>
> return 0;
> @@ -193,6 +196,7 @@ static int wacom_i2c_probe(struct i2c_client *client,
> {
> struct wacom_i2c *wac_i2c;
> struct input_dev *input;
> + struct reset_control *rstc;
> struct wacom_features features = { 0 };
> int error;
>
> @@ -201,6 +205,12 @@ static int wacom_i2c_probe(struct i2c_client *client,
> return -EIO;
> }
>
> + rstc = devm_reset_control_get_optional_exclusive(>dev, NULL);
> + if (IS_ERR(rstc)) {
> + dev_err(>dev, "Failed to get reset control before
> init\n");
> + return PTR_ERR(rstc);
> + }
I think majority users will have this controller reset line connected to
a GPIO. I briefly looked into reset controller code and I do not see it
supporting this case. How is this device connected on your board?
> +
> error = wacom_query_device(client, );
> if (error)
> return error;
> @@ -214,6 +224,7 @@ static int wacom_i2c_probe(struct i2c_client *client,
>
> wac_i2c->client = client;
> wac_i2c->input = input;
> + wac_i2c->rstc = rstc;
>
> input->name = "Wacom I2C Digitizer";
> input->id.bustype = BUS_I2C;
> --
> 2.31.0
>
Thanks.
--
Dmitry
Re: [PATCH v4 05/10] Input: wacom_i2c - Add support for distance and tilt x/y
On Thu, Mar 25, 2021 at 09:52:25PM -0400, Alistair Francis wrote:
> This is based on the out of tree rM2 driver.
>
> Signed-off-by: Alistair Francis
> ---
> drivers/input/touchscreen/wacom_i2c.c | 25 +++--
> 1 file changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/touchscreen/wacom_i2c.c
> b/drivers/input/touchscreen/wacom_i2c.c
> index ee1829dd35f4..3b4bc514dc3f 100644
> --- a/drivers/input/touchscreen/wacom_i2c.c
> +++ b/drivers/input/touchscreen/wacom_i2c.c
> @@ -22,12 +22,16 @@
> #define WACOM_CMD_QUERY3 0x02
> #define WACOM_CMD_THROW0 0x05
> #define WACOM_CMD_THROW1 0x00
> -#define WACOM_QUERY_SIZE 19
> +#define WACOM_QUERY_SIZE 22
>
> struct wacom_features {
> int x_max;
> int y_max;
> int pressure_max;
> + int distance_max;
> + int distance_physical_max;
> + int tilt_x_max;
> + int tilt_y_max;
> char fw_version;
> };
>
> @@ -79,6 +83,10 @@ static int wacom_query_device(struct i2c_client *client,
> features->y_max = get_unaligned_le16([5]);
> features->pressure_max = get_unaligned_le16([11]);
> features->fw_version = get_unaligned_le16([13]);
> + features->distance_max = data[15];
> + features->distance_physical_max = data[16];
> + features->tilt_x_max = get_unaligned_le16([17]);
> + features->tilt_y_max = get_unaligned_le16([19]);
Do other models also support distance and tilt? If not, this needs to be
made conditional.
Thanks.
--
Dmitry
Re: [PATCH v4 03/10] Input: wacom_i2c - Add device tree support to wacom_i2c
Hi Alistair,
On Thu, Mar 25, 2021 at 09:52:23PM -0400, Alistair Francis wrote:
> Allow the wacom-i2c device to be exposed via device tree.
>
> Signed-off-by: Alistair Francis
> ---
> v4:
> - Avoid unused variable warning by not using of_match_ptr()
>
> drivers/input/touchscreen/wacom_i2c.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/input/touchscreen/wacom_i2c.c
> b/drivers/input/touchscreen/wacom_i2c.c
> index 1afc6bde2891..eada68770671 100644
> --- a/drivers/input/touchscreen/wacom_i2c.c
> +++ b/drivers/input/touchscreen/wacom_i2c.c
> @@ -12,6 +12,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> #define WACOM_CMD_QUERY0 0x04
> @@ -262,10 +263,17 @@ static const struct i2c_device_id wacom_i2c_id[] = {
> };
> MODULE_DEVICE_TABLE(i2c, wacom_i2c_id);
>
> +static const struct of_device_id wacom_i2c_of_match_table[] = {
> + { .compatible = "wacom,generic" },
No, "generic" is not something we want in device tree binding. What is
the version of the controller used in your device? Put it instead of
"generic". Or if you know the earliest model with this protocol then it
can be used.
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, wacom_i2c_of_match_table);
> +
> static struct i2c_driver wacom_i2c_driver = {
> .driver = {
> .name = "wacom_i2c",
> .pm = _i2c_pm,
> + .of_match_table = wacom_i2c_of_match_table,
> },
>
> .probe = wacom_i2c_probe,
> --
> 2.31.0
>
Thanks.
--
Dmitry
Re: [PATCH v4 04/10] Input: wacom_i2c - Add touchscren properties
On Thu, Mar 25, 2021 at 09:52:24PM -0400, Alistair Francis wrote:
> Connect touchscreen properties to the wacom_i2c.
>
> Signed-off-by: Alistair Francis
> ---
> v4:
> - Add touchscreen_report_pos() as well
>
> drivers/input/touchscreen/wacom_i2c.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/input/touchscreen/wacom_i2c.c
> b/drivers/input/touchscreen/wacom_i2c.c
> index eada68770671..ee1829dd35f4 100644
> --- a/drivers/input/touchscreen/wacom_i2c.c
> +++ b/drivers/input/touchscreen/wacom_i2c.c
> @@ -11,6 +11,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -33,6 +34,7 @@ struct wacom_features {
> struct wacom_i2c {
> struct i2c_client *client;
> struct input_dev *input;
> + struct touchscreen_properties props;
> u8 data[WACOM_QUERY_SIZE];
> bool prox;
> int tool;
> @@ -188,6 +190,9 @@ static int wacom_i2c_probe(struct i2c_client *client,
> __set_bit(BTN_STYLUS2, input->keybit);
> __set_bit(BTN_TOUCH, input->keybit);
>
> + touchscreen_parse_properties(input, true, _i2c->props);
> + touchscreen_report_pos(input, _i2c->props, features.x_max,
> +features.y_max, true);
??? This goes into wacom_i2c_irq() where it previously used
input_report_abs() for X and Y so that transformations (swap, mirrot)
requested via device properties are applied to the coordinates.
Thanks.
--
Dmitry
Re: [PATCH 11/12] Input: elantech - Prepare a complete software node for the device
On Mon, Mar 29, 2021 at 01:50:46PM +0300, Heikki Krogerus wrote: > Creating a software node and supplying that for the device > instead of only the device properties in it. A software > node was always created in any case to hold the additional > device properties, so this change does not have any real > effect. > > This change makes it possible to remove support for the > problematic "dangling" device properties from i2c subsystem, > i.e. the "properties" member from struct i2c_board_info. The > problems caused by them are not related to this driver. > > Signed-off-by: Heikki Krogerus > Cc: Dmitry Torokhov Acked-by: Dmitry Torokhov Thanks. -- Dmitry
Re: [PATCH v1] Input: elants_i2c - drop zero-checking of ABS_MT_TOUCH_MAJOR resolution
On Mon, Mar 29, 2021 at 02:55:07AM +0300, Dmitry Osipenko wrote: > Drop unnecessary zero-checking of ABS_MT_TOUCH_MAJOR resolution since > there is no difference between setting resolution to 0 vs not setting > it at all. This change makes code cleaner a tad. > > Suggested-by: Dmitry Torokhov > Signed-off-by: Dmitry Osipenko Applied, thank you. -- Dmitry
Re: [PATCH v1] Input: elants_i2c - fix division by zero if firmware reports zero phys size
Hi Dmitry,
On Tue, Mar 02, 2021 at 01:08:24PM +0300, Dmitry Osipenko wrote:
> Touchscreen firmware of ASUS Transformer TF700T reports zeros for the phys
> size. Hence check whether the size is zero and don't set the resolution in
> this case.
>
> Reported-by: Jasper Korten
> Signed-off-by: Dmitry Osipenko
> ---
>
> Please note that ASUS TF700T isn't yet supported by upstream kernel,
> hence this is not a critical fix.
>
> drivers/input/touchscreen/elants_i2c.c | 8 +---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/input/touchscreen/elants_i2c.c
> b/drivers/input/touchscreen/elants_i2c.c
> index 4c2b579f6c8b..a2e1cc4192b0 100644
> --- a/drivers/input/touchscreen/elants_i2c.c
> +++ b/drivers/input/touchscreen/elants_i2c.c
> @@ -1441,14 +1441,16 @@ static int elants_i2c_probe(struct i2c_client *client,
>
> touchscreen_parse_properties(ts->input, true, >prop);
>
> - if (ts->chip_id == EKTF3624) {
> + if (ts->chip_id == EKTF3624 && ts->phy_x && ts->phy_y) {
> /* calculate resolution from size */
> ts->x_res = DIV_ROUND_CLOSEST(ts->prop.max_x, ts->phy_x);
> ts->y_res = DIV_ROUND_CLOSEST(ts->prop.max_y, ts->phy_y);
> }
>
> - input_abs_set_res(ts->input, ABS_MT_POSITION_X, ts->x_res);
> - input_abs_set_res(ts->input, ABS_MT_POSITION_Y, ts->y_res);
> + if (ts->x_res > 0)
> + input_abs_set_res(ts->input, ABS_MT_POSITION_X, ts->x_res);
There is absolutely no difference between setting respluton to 0 vs not
setting it at all, so I dropped the conditionals and applied.
> + if (ts->y_res > 0)
> + input_abs_set_res(ts->input, ABS_MT_POSITION_Y, ts->y_res);
> if (ts->major_res > 0)
We could drop this condition as well.
> input_abs_set_res(ts->input, ABS_MT_TOUCH_MAJOR, ts->major_res);
>
> --
> 2.29.2
>
Thanks.
--
Dmitry
Re: [tip: irq/core] genirq: Add IRQF_NO_AUTOEN for request_irq/nmi()
On Thu, Mar 25, 2021 at 07:59:44AM +0100, Thomas Gleixner wrote: > Dmitry, > > On Tue, Mar 23 2021 at 15:44, Dmitry Torokhov wrote: > > On Thu, Mar 04, 2021 at 07:50:31PM +0100, Thomas Gleixner wrote: > >> Please hold on: > >> > >> > >> https://lkml.kernel.org/r/CAHk-=wgZjJ89jeH72TC3i6N%2Bz9WEY=3ysp8zr9narucsqca...@mail.gmail.com > >> > >> I'll recreate a tag for you once rc2 is out. > > > > It looks like the change has been picked up as > > cbe16f35bee6880becca6f20d2ebf6b457148552i on top of -rc2, > > but I don't think there is tag for it? > > Sorry, forgot about it. Here you go: > > git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git > irq-no-autoen-2021-03-25 Thank you! -- Dmitry
Re: [PATCH v5 2/2] Input: move to use request_irq by IRQF_NO_AUTOEN flag
On Wed, Mar 03, 2021 at 11:49:16AM +1300, Barry Song wrote: > disable_irq() after request_irq() still has a time gap in which > interrupts can come. request_irq() with IRQF_NO_AUTOEN flag will > disable IRQ auto-enable because of requesting. > > On the other hand, request_irq() after setting IRQ_NOAUTOEN as > below > irq_set_status_flags(irq, IRQ_NOAUTOEN); > request_irq(dev, irq...); > can also be replaced by request_irq() with IRQF_NO_AUTOEN flag. > > Signed-off-by: Barry Song Applied, thank you. -- Dmitry
Re: [GIT PULL] Immutable branch between MFD and Input due for the v5.13 merge window
Hi Lee, On Wed, Mar 10, 2021 at 11:12:50AM +, Lee Jones wrote: > On Tue, 09 Mar 2021, Lee Jones wrote: > > > On Tue, 09 Mar 2021, Lee Jones wrote: > > > > > Enjoy! > > > > > > The following changes since commit > > > fe07bfda2fb9cdef8a4d4008a409bb02f35f1bd8: > > > > > > Linux 5.12-rc1 (2021-02-28 16:05:19 -0800) > > > > > > are available in the Git repository at: > > > > > > git://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git > > > ib-mfd-input-v5.13 > > > > > > for you to fetch changes up to b58c808ca46c163c1924ec5d3285e67e9217ec74: > > > > > > MAINTAINERS: Add entry for ATC260x PMIC (2021-03-09 13:50:39 +) > > > > > > > > > Immutable branch between MFD and Input due for the v5.13 merge window > > > > > > > > > Cristian Ciocaltea (4): > > > dt-bindings: input: Add reset-time-sec common property > > > dt-bindings: mfd: Add Actions Semi ATC260x PMIC binding > > > mfd: Add MFD driver for ATC260x PMICs > > > input: atc260x: Add onkey driver for ATC260x PMICs > > > > > > Manivannan Sadhasivam (1): > > > MAINTAINERS: Add entry for ATC260x PMIC > > > > > > Documentation/devicetree/bindings/input/input.yaml | 7 + > > > .../devicetree/bindings/mfd/actions,atc260x.yaml | 183 > > > MAINTAINERS| 12 + > > > drivers/input/misc/Kconfig | 11 + > > > drivers/input/misc/Makefile| 2 +- > > > drivers/input/misc/atc260x-onkey.c | 305 > > > > > > drivers/mfd/Kconfig| 18 ++ > > > drivers/mfd/Makefile | 3 + > > > drivers/mfd/atc260x-core.c | 310 > > > + > > > drivers/mfd/atc260x-i2c.c | 64 + > > > include/linux/mfd/atc260x/atc2603c.h | 281 > > > +++ > > > include/linux/mfd/atc260x/atc2609a.h | 308 > > > > > > include/linux/mfd/atc260x/core.h | 58 > > > 13 files changed, 1561 insertions(+), 1 deletion(-) > > > create mode 100644 > > > Documentation/devicetree/bindings/mfd/actions,atc260x.yaml > > > create mode 100644 drivers/input/misc/atc260x-onkey.c > > > create mode 100644 drivers/mfd/atc260x-core.c > > > create mode 100644 drivers/mfd/atc260x-i2c.c > > > create mode 100644 include/linux/mfd/atc260x/atc2603c.h > > > create mode 100644 include/linux/mfd/atc260x/atc2609a.h > > > create mode 100644 include/linux/mfd/atc260x/core.h > > > > FYI, if anyone has pulled this, they should probably rebase it onto > > v5.12-rc2 and delete the v5.12-rc1 tag from their tree: > > > > https://lwn.net/Articles/848431/ > > In case you haven't pulled this yet, I created a new tag: > > ib-mfd-input-v5.13-1 Did you push this one out? I can't seem to see it. Thanks. -- Dmitry
Re: [PATCH v6 3/3] ARM: tegra: acer-a500: Add atmel,wakeup-method property
On Thu, Mar 25, 2021 at 03:10:25PM +0100, Thierry Reding wrote: > On Sun, Mar 21, 2021 at 03:40:28PM -0700, Dmitry Torokhov wrote: > > On Tue, Mar 02, 2021 at 01:21:58PM +0300, Dmitry Osipenko wrote: > > > Acer A500 uses Atmel Maxtouch 1386 touchscreen controller. This controller > > > has WAKE line which could be connected to I2C clock lane, dedicated GPIO > > > or fixed to HIGH level. Controller wakes up from a deep sleep when WAKE > > > line is asserted low. Acer A500 has WAKE line connected to I2C clock and > > > Linux device driver doesn't work property without knowing what wakeup > > > method is used by h/w. > > > > > > Add atmel,wakeup-method property to the touchscreen node. > > > > > > Signed-off-by: Dmitry Osipenko > > > > Applied, thank you. > > I noticed that you had applied this as I was applying a different patch > that touches the same area and it causes a conflict. In general I prefer > to pick up all device tree changes into the Tegra tree, specifically to > avoid such conflicts. > > That said, I didn't see an email from Stephen about this causing a > conflict in linux-next, so perhaps it's fine. If this pops up again it > might be worth considering to drop this from your tree so that I can > resolve the conflict in the Tegra tree. Sorry about that, I went ahead and dropped the patch from my branch. Thanks. -- Dmitry
Re: [PATCH] spi: ensure timely release of driver-allocated resources
On Wed, Mar 24, 2021 at 11:39:35PM +, Mark Brown wrote: > On Wed, Mar 24, 2021 at 03:27:23PM -0700, Dmitry Torokhov wrote: > > On Wed, Mar 24, 2021 at 09:32:26PM +, Mark Brown wrote: > > > > TBH that looks like a fairly standard case where you probably don't want > > > to be using devm for the interrupts in the first place. Leaving the > > > interrupts live after the bus thinks it freed the device doesn't seem > > > like the best idea, I'm not sure I'd expect that to work reliably when > > > the device tries to call into the bus code to interact with the device > > > that the bus thought was already freed anyway. > > > > That is not an argument really. By that token we should not be using > > devm for anything but memory, and that is only until we implement some > > kind of memleak checker that will ensure that driver-allocated memory is > > released after driver's remove() method exits. > > I pretty much agree with that perspective TBH, I rarely see interrupt > users that I conside safe. It's the things that actively do stuff that > cause issues, interrupts and registration of userspace interfaces both > being particularly likely to do so as work comes in asynchronously. > > > You also misread that the issue is limited to interrupts, when i fact > > in this particular driver it is the input device that is being > > unregistered too late and fails to timely quiesce the device. Resulting > > interrupt storm is merely a side effect of this. > > My understanding was that the issue is that the driver is generating > work because the interrupt is firing, if the interrupt had been > unregistered we'd not be getting the interupts delivered (probably > they'd have been disabled at the interrupt controller level) and not > have the problem of trying to handle them on a partially unwound device. Yes, but the root of the problem is that we rely that the device will be stopped when input core "closes" it upon device unregistering, so even if we did not have this particular issue with interrupt storm we would still be at risk of accessing half-dead device. What I am trying to say (and I believe we are actually agreeing with each other) is that not only interrupts, but other devm-allocated objects are also can cause problems here. > > > > looked at. Possibly we want a driver core callback which is scheduled > > > via devm (remove_devm(), cleanup() or something). We'd still need to > > > move things about in all the buses but it seems preferable to do it that > > > way rather than open coding opening a group and the comments about > > > what's going on and the ordering requirements everywhere, it's a little > > > less error prone going forward. > > > From the driver's perspective they expect devm-allocated resources to > > happen immediately after ->remove() method is run. I do not believe any > > driver is interested in additional callback, and you'd need to modify > > a boatload of drivers to fix this issue. > > Not a callback for the device drivers, for the buses. This is > essentially about converting the unwinding the bus does to be sequenced > for devm. Ah, OK. > > > I agree with you that manual group handling might be a bit confusing > > and sprinkling the same comment everywhere is not too nice, so how about > > we provide: > > > void *devm_mark_driver_resources(struct device *dev) > > > and > > > void devm_release_driver_resources() > > > and keep the commentary there? The question is where to keep > > driver_res_id field - in generic device structure or put it into bus' > > device structure as some buses and classes do not need it and we'd be > > sawing 4-8 bytes per device structure this way. > > I guess bus' device :/ > > > Another way is to force buses to use devm for their resource management > > (I.e. in case of SPI wrap dev_pm_domain_detach() in > > devm_add_action_or_release()). It works for buses that have small number > > of resource allocated, but gets more unwieldy and more expensive the > > more resources are managed at bus level, that is why I opted for opening > > a group. > > If the driver core were doing it and scheduling a single callback the > bus provides then that callback could do as much as it likes... OK, I can look into it. In the meantime wills something like below a bit easier for you to stomach? If so I'll resubmit formally. diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index c19a09201358..a0db83bcb3d4 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -375,6 +375,11 @@ static int spi_uevent(struct device *dev, struct kobj_uevent_env *env
Re: [PATCH] spi: ensure timely release of driver-allocated resources
On Wed, Mar 24, 2021 at 09:32:26PM +, Mark Brown wrote: > On Tue, Mar 23, 2021 at 12:04:34PM -0700, Dmitry Torokhov wrote: > > On Tue, Mar 23, 2021 at 05:36:06PM +, Mark Brown wrote: > > > No it is ordering issue. I do not have a proven real-life example for > > SPI, but we do have one for I2C: > > > https://lore.kernel.org/linux-devicetree/20210305041236.3489-7-j...@labundy.com/ > > TBH that looks like a fairly standard case where you probably don't want > to be using devm for the interrupts in the first place. Leaving the > interrupts live after the bus thinks it freed the device doesn't seem > like the best idea, I'm not sure I'd expect that to work reliably when > the device tries to call into the bus code to interact with the device > that the bus thought was already freed anyway. That is not an argument really. By that token we should not be using devm for anything but memory, and that is only until we implement some kind of memleak checker that will ensure that driver-allocated memory is released after driver's remove() method exits. If we have devm API we need to make sure it works. You also misread that the issue is limited to interrupts, when i fact in this particular driver it is the input device that is being unregistered too late and fails to timely quiesce the device. Resulting interrupt storm is merely a side effect of this. > > If we want this to work reliably it really feels like we should have two > remove callbacks in the driver core doing this rather than open coding > in every single bus which is what we'd need to do - this is going to > affect any bus that does anything other than just call the device's > remove() callback. PCI looks like it might have issues too for example, > and platform does as well and those were simply the first two buses I > looked at. Possibly we want a driver core callback which is scheduled > via devm (remove_devm(), cleanup() or something). We'd still need to > move things about in all the buses but it seems preferable to do it that > way rather than open coding opening a group and the comments about > what's going on and the ordering requirements everywhere, it's a little > less error prone going forward. >From the driver's perspective they expect devm-allocated resources to happen immediately after ->remove() method is run. I do not believe any driver is interested in additional callback, and you'd need to modify a boatload of drivers to fix this issue. I agree with you that manual group handling might be a bit confusing and sprinkling the same comment everywhere is not too nice, so how about we provide: void *devm_mark_driver_resources(struct device *dev) and void devm_release_driver_resources() and keep the commentary there? The question is where to keep driver_res_id field - in generic device structure or put it into bus' device structure as some buses and classes do not need it and we'd be sawing 4-8 bytes per device structure this way. Another way is to force buses to use devm for their resource management (I.e. in case of SPI wrap dev_pm_domain_detach() in devm_add_action_or_release()). It works for buses that have small number of resource allocated, but gets more unwieldy and more expensive the more resources are managed at bus level, that is why I opted for opening a group. > > > Note how dev_pm_domain_detach() jumped ahead of everything, and > > strictly speaking past this point we can no longer guarantee that we can > > access the chip and disable it. > > Frankly it looks like the PM domain stuff shouldn't be in the probe() > and remove() paths at all and this has been bogusly copies from other > buses, it should be in the device registration paths. The device is in > the domain no matter what's going on with binding it. Given how generic > code is I'm not even sure why it's in the buses. Here I will agree with you, bit I think it comes from power domain "duality". In OF power domain represents grouping of devices, and is static as devices do not move around, whereas in ACPI domain means control, and we are putting a device under control of ACPI PM when we bind it to a driver. As part of that control we bring the device into _D0, etc. Yay for mixing concepts, but this is not really material to the question of how to orderly release resources. Thanks. -- Dmitry
Re: [tip: irq/core] genirq: Add IRQF_NO_AUTOEN for request_irq/nmi()
Hi Thomas, On Thu, Mar 04, 2021 at 07:50:31PM +0100, Thomas Gleixner wrote: > Dmitry, > > On Thu, Mar 04 2021 at 11:57, Thomas Gleixner wrote: > > On Thu, Mar 04 2021 at 10:53, tip-bot wrote: > > > >> The following commit has been merged into the irq/core branch of tip: > >> > >> Commit-ID: e749df1bbd23f4472082210650514548d8a39e9b > >> Gitweb: > >> https://git.kernel.org/tip/e749df1bbd23f4472082210650514548d8a39e9b > >> Author:Barry Song > >> AuthorDate:Wed, 03 Mar 2021 11:49:15 +13:00 > >> Committer: Thomas Gleixner > >> CommitterDate: Thu, 04 Mar 2021 11:47:52 +01:00 > >> > >> genirq: Add IRQF_NO_AUTOEN for request_irq/nmi() > > > > this commit is immutable and I tagged it so you can pull it into your > > tree to add the input changes on top: > > > >git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git > > irq-no-autoen-2021-03-04 > > Please hold on: > > > https://lkml.kernel.org/r/CAHk-=wgZjJ89jeH72TC3i6N%2Bz9WEY=3ysp8zr9narucsqca...@mail.gmail.com > > I'll recreate a tag for you once rc2 is out. It looks like the change has been picked up as cbe16f35bee6880becca6f20d2ebf6b457148552i on top of -rc2, but I don't think there is tag for it? Thanks! -- Dmitry
Re: [PATCH v3 3/3] input: gpio-keys: Use hrtimer for software debounce, if possible
On Sun, Mar 07, 2021 at 10:22:40PM +, Paul Cercueil wrote: > We want to be able to report the input event as soon as the debounce > delay elapsed. However, the current code does not really ensure that, > as it uses the jiffies-based schedule_delayed_work() API. With a small > enough HZ value (HZ <= 100), this results in some input events being > lost, when a key is quickly pressed then released (on a human's time > scale). > > Switching to hrtimers fixes this issue, and will work even on extremely > low HZ values (tested at HZ=24). This is however only possible if > reading the GPIO is possible without sleeping. If this condition is not > met, the previous approach of using a jiffies-based timer is taken. > > Signed-off-by: Paul Cercueil Applied with minor edits to make more use of debounce_use_hrtimer flag. Thanks. -- Dmitry
Re: [PATCH v3 2/3] input: gpio-keys: Use hrtimer for release timer
On Sun, Mar 07, 2021 at 10:22:39PM +, Paul Cercueil wrote: > Dealing with input, timing is important; if the button should be > released in one millisecond, then it should be done in one millisecond > and not a hundred milliseconds. > > Therefore, the standard timer API is not really suitable for this task. > > Convert the gpio-keys driver to use a hrtimer instead of the standard > timer to address this issue. > > Note that by using a hard IRQ for the hrtimer callback, we can get rid > of the spin_lock_irqsave() and spin_unlock_irqrestore(). > > Signed-off-by: Paul Cercueil Applied, thank you. -- Dmitry
Re: [PATCH v3 1/3] input: gpio-keys: Remove extra call to input_sync
On Sun, Mar 07, 2021 at 10:22:38PM +, Paul Cercueil wrote: > The input_sync() function is already called after the loop in > gpio_keys_report_state(), so it does not need to be called after each > iteration within gpio_keys_gpio_report_event(). > > Signed-off-by: Paul Cercueil Applied, thank you. -- Dmitry
Re: [PATCH] Input: elan_i2c - fix a typo in parameter name
On Thu, Mar 11, 2021 at 02:41:43PM +0300, Nikolai Kostrigin wrote: > s/max_baseliune/max_baseline/ > > Signed-off-by: Nikolai Kostrigin Applied, thank you. -- Dmitry
Re: [PATCH] spi: ensure timely release of driver-allocated resources
On Tue, Mar 23, 2021 at 05:36:06PM +, Mark Brown wrote: > On Mon, Mar 22, 2021 at 12:38:15PM -0700, Dmitry Torokhov wrote: > > On Mon, Mar 22, 2021 at 12:37:07PM +, Mark Brown wrote: > > > > This feels like it might make sense to push up to the driver core level > > > then rather than doing in individual buses? > > > That is exactly the issue: we can't. Driver core already releases all > > resources when a device is being unbound but that happens after bus > > "remove" code is executed and therefore is too late. The device might > > already be powered down, but various devm release() callbacks will be > > trying to access it. > > Can you provide a concrete example of something that is causing problems > here? If something is trying to access the device after remove() has > run that sounds like it's abusing devres somehow. It sounded from your > commit log like this was something to do with the amount of time it took > the driver core to action the frees rather than an ordering issue. No it is ordering issue. I do not have a proven real-life example for SPI, but we do have one for I2C: https://lore.kernel.org/linux-devicetree/20210305041236.3489-7-j...@labundy.com/ However, if we consider fairly typical SPI driver, such as drivers/input/touchscreen/ad7877.c, you can see that it uses devm in its probe() and because all resources are managed, it does not define remove() at all. So during proble we have: SPI: dev_pm_domain_attach AD7877: devm_kzalloc driver structure AD7877: devm allocation of input device AD7877: devm custom action to disable the chip on removal AD7877: devm IRQ request AD7877: devm sysfs attribute group AD7877: devm input registration And on remove: SPI: dev_pm_domain_detach !! AD7877: devm input unregistration AD7877: devm sysfs attribute group removal AD7877: devm freeing IRQ AD7877: devm disable the chip AD7877: devm freeing of input device AD7877: devm free driver structure Note how dev_pm_domain_detach() jumped ahead of everything, and strictly speaking past this point we can no longer guarantee that we can access the chip and disable it. > > > devm only works when you do not mix manual resources with managed ones, > > and when bus code allocates resources themselves (attaching a device to > > a power domain can be viewed as resource acquisition) we violate this > > principle. We could, of course, to make SPI bus' probe() use > > devm_add_action_or_reset() to work in removal of the device from the > > power domain into the stream of devm resources, but that still requires > > changes at bus code, and I believe will complicate matters if we need to > > extend SPI bus code to allocate more resources in probe(). So I opted > > for opening a devm group to separate resources allocated before and > > after probe() to be able to release them in the right order. > > Sure, these are standard issues that people create with excessive use of devm is a fact of life and we need to live with it. I am unconvinced if it solved more issues that it brought in, but it is something that driver authors like to use and are pushed towards. > devm but the device's remove() callback is surely already a concern by > itself here? In the example above there is not one, but even if it exists, it is called first, so in some limited cases you could have non-managed resources allocated very last and released first in remove(), and then have devm release the rest. However driver's remove() is not issue here, it is bus' non-trivial remove. Thanks. -- Dmitry
Re: [PATCH] Input: analog - fix invalid snprintf() call
On Tue, Mar 23, 2021 at 02:29:15PM +0100, Rasmus Villemoes wrote:
> On 23/03/2021 14.14, Arnd Bergmann wrote:
> > From: Arnd Bergmann
> >
> > overlapping input and output arguments to snprintf() are
> > undefined behavior in C99:
> >
>
> Good luck:
> https://lore.kernel.org/lkml/1457469654-17059-1-git-send-email-li...@rasmusvillemoes.dk/
>
> At least 5 years ago the consensus from old-timers was that "the
> kernel's snprintf supports this use case, just keep it working that way".
>
> > diff --git a/drivers/input/joystick/analog.c
> > b/drivers/input/joystick/analog.c
> > index f798922a4598..8c9fed3f13e2 100644
> > --- a/drivers/input/joystick/analog.c
> > +++ b/drivers/input/joystick/analog.c
> > @@ -419,14 +419,16 @@ static void analog_calibrate_timer(struct analog_port
> > *port)
> >
> > static void analog_name(struct analog *analog)
> > {
> > - snprintf(analog->name, sizeof(analog->name), "Analog %d-axis %d-button",
> > + int len;
> > +
> > + len = snprintf(analog->name, sizeof(analog->name), "Analog %d-axis
> > %d-button",
> > hweight8(analog->mask & ANALOG_AXES_STD),
> > hweight8(analog->mask & ANALOG_BTNS_STD) + !!(analog->mask &
> > ANALOG_BTNS_CHF) * 2 +
> > hweight16(analog->mask & ANALOG_BTNS_GAMEPAD) +
> > !!(analog->mask & ANALOG_HBTN_CHF) * 4);
> >
> > if (analog->mask & ANALOG_HATS_ALL)
> > - snprintf(analog->name, sizeof(analog->name), "%s %d-hat",
> > -analog->name, hweight16(analog->mask &
> > ANALOG_HATS_ALL));
> > + len += snprintf(analog->name + len, sizeof(analog->name) - len,
> > "%d-hat",
> > +hweight16(analog->mask & ANALOG_HATS_ALL));
>
> Use scnprintf, this is too fragile and hard to verify. If the first
> snprintf overflows, the second passes a huge size_t to snprintf which
> will WARN.
Also the format needs to be " %d-hat" (note the leading space).
Thanks.
--
Dmitry
Re: [PATCH] Input: i8042 - fix Pegatron C15B ID entry
On Tue, Mar 23, 2021 at 02:06:13PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> The Zenbook Flip entry that was added overwrites a previous one
> because of a typo:
>
> In file included from drivers/input/serio/i8042.h:23,
> from drivers/input/serio/i8042.c:131:
> drivers/input/serio/i8042-x86ia64io.h:591:28: error: initialized field
> overwritten [-Werror=override-init]
> 591 | .matches = {
> |^
> drivers/input/serio/i8042-x86ia64io.h:591:28: note: (near initialization for
> 'i8042_dmi_noselftest_table[0].matches')
>
> Add the missing separator between the two.
>
> Fixes: b5d6e7ab7fe7 ("Input: i8042 - add ASUS Zenbook Flip to noselftest
> list")
> Signed-off-by: Arnd Bergmann
Applied, thank you.
--
Dmitry
Re: [PATCH] spi: ensure timely release of driver-allocated resources
On Mon, Mar 22, 2021 at 12:37:07PM +, Mark Brown wrote: > On Sun, Mar 21, 2021 at 06:43:32PM -0700, Dmitry Torokhov wrote: > > > Note that this is not SPI-specific issue. I already send a similar > > patch for I2C and will be sending more. > > This feels like it might make sense to push up to the driver core level > then rather than doing in individual buses? That is exactly the issue: we can't. Driver core already releases all resources when a device is being unbound but that happens after bus "remove" code is executed and therefore is too late. The device might already be powered down, but various devm release() callbacks will be trying to access it. devm only works when you do not mix manual resources with managed ones, and when bus code allocates resources themselves (attaching a device to a power domain can be viewed as resource acquisition) we violate this principle. We could, of course, to make SPI bus' probe() use devm_add_action_or_reset() to work in removal of the device from the power domain into the stream of devm resources, but that still requires changes at bus code, and I believe will complicate matters if we need to extend SPI bus code to allocate more resources in probe(). So I opted for opening a devm group to separate resources allocated before and after probe() to be able to release them in the right order. Thanks. -- Dmitry
[PATCH] Input: serio - make write method mandatory
Given that all serio drivers except one implement write() method
let's make it mandatory to avoid testing for its presence whenever
we attempt to use it.
Signed-off-by: Dmitry Torokhov
---
drivers/input/serio/ams_delta_serio.c | 6 ++
drivers/input/serio/serio.c | 5 +
include/linux/serio.h | 5 +
3 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/input/serio/ams_delta_serio.c
b/drivers/input/serio/ams_delta_serio.c
index 1c0be299f179..a1c314897951 100644
--- a/drivers/input/serio/ams_delta_serio.c
+++ b/drivers/input/serio/ams_delta_serio.c
@@ -89,6 +89,11 @@ static irqreturn_t ams_delta_serio_interrupt(int irq, void
*dev_id)
return IRQ_HANDLED;
}
+static int ams_delta_serio_write(struct serio *serio, u8 data)
+{
+ return -EINVAL;
+}
+
static int ams_delta_serio_open(struct serio *serio)
{
struct ams_delta_serio *priv = serio->port_data;
@@ -157,6 +162,7 @@ static int ams_delta_serio_init(struct platform_device
*pdev)
priv->serio = serio;
serio->id.type = SERIO_8042;
+ serio->write = ams_delta_serio_write;
serio->open = ams_delta_serio_open;
serio->close = ams_delta_serio_close;
strlcpy(serio->name, "AMS DELTA keyboard adapter", sizeof(serio->name));
diff --git a/drivers/input/serio/serio.c b/drivers/input/serio/serio.c
index 29f491082926..8d229a11bb6b 100644
--- a/drivers/input/serio/serio.c
+++ b/drivers/input/serio/serio.c
@@ -694,6 +694,11 @@ EXPORT_SYMBOL(serio_reconnect);
*/
void __serio_register_port(struct serio *serio, struct module *owner)
{
+ if (!serio->write) {
+ pr_err("%s: refusing to register %s without write method\n",
+ __func__, serio->name);
+ return;
+ }
serio_init_port(serio);
serio_queue_event(serio, owner, SERIO_REGISTER_PORT);
}
diff --git a/include/linux/serio.h b/include/linux/serio.h
index 6c27d413da92..075f1b8d76fa 100644
--- a/include/linux/serio.h
+++ b/include/linux/serio.h
@@ -121,10 +121,7 @@ void serio_unregister_driver(struct serio_driver *drv);
static inline int serio_write(struct serio *serio, unsigned char data)
{
- if (serio->write)
- return serio->write(serio, data);
- else
- return -1;
+ return serio->write(serio, data);
}
static inline void serio_drv_write_wakeup(struct serio *serio)
--
2.31.0.rc2.261.g7f71774620-goog
--
Dmitry
Re: [PATCH] Input: Fix a typo
On Mon, Mar 22, 2021 at 07:50:30AM +0530, Bhaskar Chowdhury wrote: > > s/subsytem/subsystem/ > > Signed-off-by: Bhaskar Chowdhury Applied, thank you. -- Dmitry
[PATCH] spi: ensure timely release of driver-allocated resources
More and more drivers rely on devres to manage their resources, however
if bus' probe() and release() methods are not trivial and control some
of resources as well (for example enable or disable clocks, or attach
device to a power domain), we need to make sure that driver-allocated
resources are released immediately after driver's remove() method
returns, and not postponed until driver core gets around to releasing
resources. To fix that we open a new devres group before calling
driver's probe() and explicitly release it when we return from driver's
remove().
Signed-off-by: Dmitry Torokhov
---
Note that this is not SPI-specific issue. I already send a similar
patch for I2C and will be sending more.
drivers/spi/spi.c | 25 +++--
include/linux/spi/spi.h | 4
2 files changed, 27 insertions(+), 2 deletions(-)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index c19a09201358..7c369cebebbd 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -421,29 +421,50 @@ static int spi_probe(struct device *dev)
if (ret)
return ret;
+ spi->devres_group_id = devres_open_group(dev, NULL, GFP_KERNEL);
+ if (!spi->devres_group_id) {
+ ret = -ENOMEM;
+ goto err_detach_pm_domain;
+ }
+
if (sdrv->probe) {
ret = sdrv->probe(spi);
if (ret)
- dev_pm_domain_detach(dev, true);
+ goto err_release_driver_resources;
}
+ /*
+* Note that we are not closing the devres group opened above so
+* even resources that were attached to the device after probe has
+* run are released when spi_remove() is executed. This is needed as
+* some drivers might allocate additional resources.
+*/
+
+ return 0;
+
+err_release_driver_resources:
+ devres_release_group(dev, spi->devres_group_id);
+err_detach_pm_domain:
+ dev_pm_domain_detach(dev, true);
return ret;
}
static int spi_remove(struct device *dev)
{
const struct spi_driver *sdrv = to_spi_driver(dev->driver);
+ struct spi_device *spi = to_spi_device(dev);
if (sdrv->remove) {
int ret;
- ret = sdrv->remove(to_spi_device(dev));
+ ret = sdrv->remove(spi);
if (ret)
dev_warn(dev,
"Failed to unbind driver (%pe), ignoring\n",
ERR_PTR(ret));
}
+ devres_release_group(dev, spi->devres_group_id);
dev_pm_domain_detach(dev, true);
return 0;
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index aa09fdc8042d..969dd8ccc657 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -144,6 +144,8 @@ extern int spi_delay_exec(struct spi_delay *_delay, struct
spi_transfer *xfer);
* not using a GPIO line)
* @word_delay: delay to be inserted between consecutive
* words of a transfer
+ * @devres_group_id: id of the devres group that will be created for resources
+ * acquired when probing this device.
*
* @statistics: statistics for the spi_device
*
@@ -195,6 +197,8 @@ struct spi_device {
struct gpio_desc*cs_gpiod; /* chip select gpio desc */
struct spi_delayword_delay; /* inter-word delay */
+ void*devres_group_id;
+
/* the statistics */
struct spi_statistics statistics;
--
2.31.0.rc2.261.g7f71774620-goog
--
Dmitry
[PATCH] i2c: ensure timely release of driver-allocated resources
More and more drivers rely on devres to manage their resources, however
if bus' probe() and release() methods are not trivial and control some
of resources as well (for example enable or disable clocks, or attach
device to a power domain), we need to make sure that driver-allocated
resources are released immediately after driver's remove() method
returns, and not postponed until driver core gets around to releasing
resources. To fix that we open a new devres group before calling
driver's probe() and explicitly release it when we return from driver's
remove().
Tested-by: Jeff LaBundy
Signed-off-by: Dmitry Torokhov
---
Note that this problem is not I2C-specific and I will be sending patches
for other buses as well.
drivers/i2c/i2c-core-base.c | 21 -
include/linux/i2c.h | 3 +++
2 files changed, 23 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 63ebf722a424..b38ecd888b90 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -518,6 +518,13 @@ static int i2c_device_probe(struct device *dev)
if (status)
goto err_clear_wakeup_irq;
+ client->devres_group_id = devres_open_group(>dev, NULL,
+ GFP_KERNEL);
+ if (!client->devres_group_id) {
+ status = -ENOMEM;
+ goto err_detach_pm_domain;
+ }
+
/*
* When there are no more users of probe(),
* rename probe_new to probe.
@@ -530,11 +537,21 @@ static int i2c_device_probe(struct device *dev)
else
status = -EINVAL;
+ /*
+* Note that we are not closing the devres group opened above so
+* even resources that were attached to the device after probe is
+* run are released when i2c_device_remove() is executed. This is
+* needed as some drivers would allocate additional resources,
+* for example when updating firmware.
+*/
+
if (status)
- goto err_detach_pm_domain;
+ goto err_release_driver_resources;
return 0;
+err_release_driver_resources:
+ devres_release_group(>dev, client->devres_group_id);
err_detach_pm_domain:
dev_pm_domain_detach(>dev, true);
err_clear_wakeup_irq:
@@ -563,6 +580,8 @@ static int i2c_device_remove(struct device *dev)
dev_warn(dev, "remove failed (%pe), will be ignored\n",
ERR_PTR(status));
}
+ devres_release_group(>dev, client->devres_group_id);
+
dev_pm_domain_detach(>dev, true);
dev_pm_clear_wake_irq(>dev);
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 56622658b215..5d1f11c0deaa 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -306,6 +306,8 @@ struct i2c_driver {
* userspace_devices list
* @slave_cb: Callback when I2C slave mode of an adapter is used. The adapter
* calls it to pass on slave events to the slave driver.
+ * @devres_group_id: id of the devres group that will be created for resources
+ * acquired when probing this device.
*
* An i2c_client identifies a single device (i.e. chip) connected to an
* i2c bus. The behaviour exposed to Linux is defined by the driver
@@ -334,6 +336,7 @@ struct i2c_client {
#if IS_ENABLED(CONFIG_I2C_SLAVE)
i2c_slave_cb_t slave_cb;/* callback for slave mode */
#endif
+ void *devres_group_id; /* ID of probe devres group */
};
#define to_i2c_client(d) container_of(d, struct i2c_client, dev)
--
2.31.0.rc2.261.g7f71774620-goog
--
Dmitry
Re: [PATCH] Input: docs - Update according to the latest API changes
On Thu, Mar 04, 2021 at 12:09:48PM +0300, Heikki Krogerus wrote: > The old device property API is about to be removed, so > explaing how to use complete software nodes instead. > > Signed-off-by: Heikki Krogerus Applied, thank you. -- Dmitry
Re: [PATCH] dt-bindings: More cleanup of standard unit properties
On Tue, Mar 16, 2021 at 01:48:24PM -0600, Rob Herring wrote: > Properties with standard unit suffixes already have a type and don't need > type references. Fix a few more cases which have gotten added. > > Cc: Luca Ceresoli > Cc: Jonathan Cameron > Cc: Dmitry Torokhov > Cc: Bjorn Andersson > Cc: Zhang Rui > Cc: Daniel Lezcano > Cc: Linus Walleij > Cc: Kevin Tsai > Cc: Dmitry Baryshkov > Cc: Sebastian Reichel > Cc: Mark Brown > Cc: linux-...@vger.kernel.org > Cc: linux-in...@vger.kernel.org > Cc: linux...@vger.kernel.org > Signed-off-by: Rob Herring Acked-by: Dmitry Torokhov Thanks. -- Dmitry
Re: [PATCH] dt-bindings: input: atmel,maxtouch: add wakeup-source
On Fri, Feb 12, 2021 at 05:38:06PM +0100, Krzysztof Kozlowski wrote: > The touchscreen can be a wake up source and it's being used in DTS: > > arch/arm/boot/dts/exynos5250-spring.dt.yaml: > trackpad@4b: 'wakeup-source' does not match any of the regexes: > 'pinctrl-[0-9]+' > > Signed-off-by: Krzysztof Kozlowski Applied, thank you. -- Dmitry
Re: [PATCH v6 0/3] Support wakeup methods of Atmel maXTouch controllers
Hi Dmitry, On Sat, Mar 20, 2021 at 07:02:43PM +0300, Dmitry Osipenko wrote: > 02.03.2021 13:21, Dmitry Osipenko пишет: > > Some Atmel maXTouch controllers, like mXT1386 and mXT3432S1 for example, > > have a WAKE line that needs to be asserted in order to wake controller > > from a deep sleep, otherwise it will be unusable. This series implements > > support for the wakeup methods in accordance to the mXT1386 datasheet [1], > > see page 29 (chapter "5.8 WAKE Line"). > > > > The mXT1386 is a widely used controller found on many older Android tablet > > devices. Touchscreen on Acer A500 tablet now works properly after this > > series. > > > > This patchset is a continuation of the work originally started by > > Jiada Wang [2]. > > > > [1] > > https://ww1.microchip.com/downloads/en/DeviceDoc/mXT1386_1vx_Datasheet_LX.pdf > > [2] https://patchwork.kernel.org/project/linux-input/list/?series=357875 > > Hi, > > This series is very wanted by Android tablet devices from Acer, Asus and > other vendors which use Maxtouch 1386 controller. Touchscreens don't > work without the wakeup support, i.e. without this series. The wakeup > support is implemented in accordance to the datasheet and touchscreens > are working excellent using these patches. > > Could you please take this series into v5.13? > > Or could you please let me know what exactly needs to be improved? Sorry, I was still slightly unhappy that we still are not tracking the state of controller and opportunistically retrying failed I2C transfers, but as I am failing to find time to come up with another solution I have just applied your series. Thanks. -- Dmitry
Re: [PATCH v6 3/3] ARM: tegra: acer-a500: Add atmel,wakeup-method property
On Tue, Mar 02, 2021 at 01:21:58PM +0300, Dmitry Osipenko wrote: > Acer A500 uses Atmel Maxtouch 1386 touchscreen controller. This controller > has WAKE line which could be connected to I2C clock lane, dedicated GPIO > or fixed to HIGH level. Controller wakes up from a deep sleep when WAKE > line is asserted low. Acer A500 has WAKE line connected to I2C clock and > Linux device driver doesn't work property without knowing what wakeup > method is used by h/w. > > Add atmel,wakeup-method property to the touchscreen node. > > Signed-off-by: Dmitry Osipenko Applied, thank you. -- Dmitry
Re: [PATCH v6 2/3] Input: atmel_mxt_ts - support wakeup methods
On Tue, Mar 02, 2021 at 01:21:57PM +0300, Dmitry Osipenko wrote: > According to datasheets, chips like mXT1386 have a WAKE line, it is used > to wake the chip up from deep sleep mode before communicating with it via > the I2C-compatible interface. > > If the WAKE line is connected to a GPIO line, the line must be asserted > 25 ms before the host attempts to communicate with the controller. If the > WAKE line is connected to the SCL pin, the controller will send a NACK on > the first attempt to address it, the host must then retry 25 ms later. > > Implement the wake-up methods in the driver. Touchscreen now works > properly on devices like Acer A500 tablet, fixing problems like this: > > atmel_mxt_ts 0-004c: __mxt_read_reg: i2c transfer failed (-121) > atmel_mxt_ts 0-004c: mxt_bootloader_read: i2c recv failed (-121) > atmel_mxt_ts 0-004c: Trying alternate bootloader address > atmel_mxt_ts 0-004c: mxt_bootloader_read: i2c recv failed (-121) > atmel_mxt_ts: probe of 0-004c failed with error -121 > > Reviewed-by: Linus Walleij > Signed-off-by: Jiada Wang > Signed-off-by: Dmitry Osipenko Applied, thank you. -- Dmitry
Re: [PATCH v6 1/3] dt-bindings: input: atmel_mxt_ts: Document atmel,wakeup-method and WAKE line GPIO
On Tue, Mar 02, 2021 at 01:21:56PM +0300, Dmitry Osipenko wrote: > Some Atmel touchscreen controllers have a WAKE line that needs to be > asserted low in order to wake up controller from a deep sleep. Document > the wakeup methods and the new GPIO properties. > > Reviewed-by: Rob Herring > Reviewed-by: Linus Walleij > Signed-off-by: Dmitry Osipenko Applied, thank you. -- Dmitry
[PATCH 2/2] Input: wacom_i2c - switch to using managed resources
This simplifies error unwinding path and allows us to get rid of
remove() method.
Signed-off-by: Dmitry Torokhov
---
drivers/input/touchscreen/wacom_i2c.c | 55 +--
1 file changed, 17 insertions(+), 38 deletions(-)
diff --git a/drivers/input/touchscreen/wacom_i2c.c
b/drivers/input/touchscreen/wacom_i2c.c
index 609ff84e7693..22826c387da5 100644
--- a/drivers/input/touchscreen/wacom_i2c.c
+++ b/drivers/input/touchscreen/wacom_i2c.c
@@ -145,15 +145,16 @@ static void wacom_i2c_close(struct input_dev *dev)
}
static int wacom_i2c_probe(struct i2c_client *client,
-const struct i2c_device_id *id)
+ const struct i2c_device_id *id)
{
+ struct device *dev = >dev;
struct wacom_i2c *wac_i2c;
struct input_dev *input;
struct wacom_features features = { 0 };
int error;
if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
- dev_err(>dev, "i2c_check_functionality error\n");
+ dev_err(dev, "i2c_check_functionality error\n");
return -EIO;
}
@@ -161,21 +162,22 @@ static int wacom_i2c_probe(struct i2c_client *client,
if (error)
return error;
- wac_i2c = kzalloc(sizeof(*wac_i2c), GFP_KERNEL);
- input = input_allocate_device();
- if (!wac_i2c || !input) {
- error = -ENOMEM;
- goto err_free_mem;
- }
+ wac_i2c = devm_kzalloc(dev, sizeof(*wac_i2c), GFP_KERNEL);
+ if (!wac_i2c)
+ return -ENOMEM;
wac_i2c->client = client;
+
+ input = devm_input_allocate_device(dev);
+ if (!input)
+ return -ENOMEM;
+
wac_i2c->input = input;
input->name = "Wacom I2C Digitizer";
input->id.bustype = BUS_I2C;
input->id.vendor = 0x56a;
input->id.version = features.fw_version;
- input->dev.parent = >dev;
input->open = wacom_i2c_open;
input->close = wacom_i2c_close;
@@ -194,12 +196,11 @@ static int wacom_i2c_probe(struct i2c_client *client,
input_set_drvdata(input, wac_i2c);
- error = request_threaded_irq(client->irq, NULL, wacom_i2c_irq,
-IRQF_ONESHOT, "wacom_i2c", wac_i2c);
+ error = devm_request_threaded_irq(dev, client->irq, NULL, wacom_i2c_irq,
+ IRQF_ONESHOT, "wacom_i2c", wac_i2c);
if (error) {
- dev_err(>dev,
- "Failed to enable IRQ, error: %d\n", error);
- goto err_free_mem;
+ dev_err(dev, "Failed to request IRQ: %d\n", error);
+ return error;
}
/* Disable the IRQ, we'll enable it in wac_i2c_open() */
@@ -207,31 +208,10 @@ static int wacom_i2c_probe(struct i2c_client *client,
error = input_register_device(wac_i2c->input);
if (error) {
- dev_err(>dev,
- "Failed to register input device, error: %d\n", error);
- goto err_free_irq;
+ dev_err(dev, "Failed to register input device: %d\n", error);
+ return error;
}
- i2c_set_clientdata(client, wac_i2c);
- return 0;
-
-err_free_irq:
- free_irq(client->irq, wac_i2c);
-err_free_mem:
- input_free_device(input);
- kfree(wac_i2c);
-
- return error;
-}
-
-static int wacom_i2c_remove(struct i2c_client *client)
-{
- struct wacom_i2c *wac_i2c = i2c_get_clientdata(client);
-
- free_irq(client->irq, wac_i2c);
- input_unregister_device(wac_i2c->input);
- kfree(wac_i2c);
-
return 0;
}
@@ -268,7 +248,6 @@ static struct i2c_driver wacom_i2c_driver = {
},
.probe = wacom_i2c_probe,
- .remove = wacom_i2c_remove,
.id_table = wacom_i2c_id,
};
module_i2c_driver(wacom_i2c_driver);
--
2.31.0.rc2.261.g7f71774620-goog
[PATCH 1/2] Input: wacom_i2c - do not force interrupt trigger
Instead of forcing interrupt trigger to "level low" rely on the
platform to set it up according to how it is wired on the given
board.
Signed-off-by: Dmitry Torokhov
---
drivers/input/touchscreen/wacom_i2c.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/input/touchscreen/wacom_i2c.c
b/drivers/input/touchscreen/wacom_i2c.c
index 1afc6bde2891..609ff84e7693 100644
--- a/drivers/input/touchscreen/wacom_i2c.c
+++ b/drivers/input/touchscreen/wacom_i2c.c
@@ -195,8 +195,7 @@ static int wacom_i2c_probe(struct i2c_client *client,
input_set_drvdata(input, wac_i2c);
error = request_threaded_irq(client->irq, NULL, wacom_i2c_irq,
-IRQF_TRIGGER_LOW | IRQF_ONESHOT,
-"wacom_i2c", wac_i2c);
+IRQF_ONESHOT, "wacom_i2c", wac_i2c);
if (error) {
dev_err(>dev,
"Failed to enable IRQ, error: %d\n", error);
--
2.31.0.rc2.261.g7f71774620-goog
Re: [PATCH v3 7/9] Input: wacom_i2c - Add support for vdd regulator
On Sun, Mar 21, 2021 at 10:10:47AM -0400, Alistair Francis wrote:
> Add support for a VDD regulator. This allows the kernel to prove the
> Wacom-I2C device on the rM2.
>
> Signed-off-by: Alistair Francis
> ---
> drivers/input/touchscreen/wacom_i2c.c | 22 ++
> 1 file changed, 22 insertions(+)
>
> diff --git a/drivers/input/touchscreen/wacom_i2c.c
> b/drivers/input/touchscreen/wacom_i2c.c
> index 7aa0d1c3dbc9..00db516fa3de 100644
> --- a/drivers/input/touchscreen/wacom_i2c.c
> +++ b/drivers/input/touchscreen/wacom_i2c.c
> @@ -13,6 +13,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -56,6 +57,7 @@ struct wacom_i2c {
> struct i2c_client *client;
> struct input_dev *input;
> struct touchscreen_properties props;
> + struct regulator *vdd;
> u8 data[WACOM_QUERY_SIZE];
> bool prox;
> int tool;
> @@ -203,11 +205,29 @@ static int wacom_i2c_probe(struct i2c_client *client,
> struct wacom_features features = { 0 };
> int error;
>
> + wac_i2c = kzalloc(sizeof(*wac_i2c), GFP_KERNEL);
> + if (!wac_i2c)
> + return -ENOMEM;
> +
> if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> dev_err(>dev, "i2c_check_functionality error\n");
> return -EIO;
You are leaking memory here. Additionally, I do not see you removing the
original allocation below, so I think you end up with 2 instances of
structure wacom_i2c.
Thanks.
--
Dmitry
Re: [PATCH v3 6/9] Input: wacom_i2c - Clean up the query device fields
On Sun, Mar 21, 2021 at 10:10:46AM -0400, Alistair Francis wrote: > Improve the query device fields to be more verbose. > > Signed-off-by: Alistair Francis > --- > drivers/input/touchscreen/wacom_i2c.c | 71 +++ ... > > + rstc = devm_reset_control_get_optional_exclusive(>dev, NULL); > + if (IS_ERR(rstc)) > + dev_err(>dev, "Failed to get reset control before > init\n"); > + else > + reset_control_reset(rstc); This seems misplaced. Also, reset controls are typically for resetting the system, for resetting peripherals we expose reset GPIO lines. Thanks. -- Dmitry
Re: [PATCH v3 4/9] Input: wacom_i2c - Add touchscren properties
On Sun, Mar 21, 2021 at 10:10:44AM -0400, Alistair Francis wrote:
> Connect touchscreen properties to the wacom_i2c.
>
> Signed-off-by: Alistair Francis
> ---
> drivers/input/touchscreen/wacom_i2c.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/input/touchscreen/wacom_i2c.c
> b/drivers/input/touchscreen/wacom_i2c.c
> index fc0bf583d33b..9b2ed0463d09 100644
> --- a/drivers/input/touchscreen/wacom_i2c.c
> +++ b/drivers/input/touchscreen/wacom_i2c.c
> @@ -11,6 +11,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -33,6 +34,7 @@ struct wacom_features {
> struct wacom_i2c {
> struct i2c_client *client;
> struct input_dev *input;
> + struct touchscreen_properties props;
> u8 data[WACOM_QUERY_SIZE];
> bool prox;
> int tool;
> @@ -188,6 +190,7 @@ static int wacom_i2c_probe(struct i2c_client *client,
> __set_bit(BTN_STYLUS2, input->keybit);
> __set_bit(BTN_TOUCH, input->keybit);
>
> + touchscreen_parse_properties(input, true, _i2c->props);
> input_set_abs_params(input, ABS_X, 0, features.x_max, 0, 0);
> input_set_abs_params(input, ABS_Y, 0, features.y_max, 0, 0);
> input_set_abs_params(input, ABS_PRESSURE,
Please also use touchscreen_report_pos() so flipping and swapping of
axes also works.
Thanks.
--
Dmitry
Re: [PATCH v3 3/9] Input: wacom_i2c - Add device tree support to wacom_i2c
Hi Alistair,
On Sun, Mar 21, 2021 at 10:10:43AM -0400, Alistair Francis wrote:
> Allow the wacom-i2c device to be exposed via device tree.
>
> Signed-off-by: Alistair Francis
> ---
> drivers/input/touchscreen/wacom_i2c.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/input/touchscreen/wacom_i2c.c
> b/drivers/input/touchscreen/wacom_i2c.c
> index 1afc6bde2891..fc0bf583d33b 100644
> --- a/drivers/input/touchscreen/wacom_i2c.c
> +++ b/drivers/input/touchscreen/wacom_i2c.c
> @@ -12,6 +12,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> #define WACOM_CMD_QUERY0 0x04
> @@ -262,10 +263,17 @@ static const struct i2c_device_id wacom_i2c_id[] = {
> };
> MODULE_DEVICE_TABLE(i2c, wacom_i2c_id);
>
> +static const struct of_device_id wacom_i2c_of_match_table[] __maybe_unused =
> {
> + { .compatible = "wacom,generic" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, wacom_i2c_of_match_table);
> +
> static struct i2c_driver wacom_i2c_driver = {
> .driver = {
> .name = "wacom_i2c",
> .pm = _i2c_pm,
> + .of_match_table = of_match_ptr(wacom_i2c_of_match_table),
You need to either guard wacom_i2c_of_match_table by #ifdef CONFIG_OF or
drop of_match_ptr() and assign directly as otherwise you will get
"unused variable" warning.
Thanks.
--
Dmitry
Re: [PATCH] Input: ims-pcu - drop redundant driver-data assignment
On Thu, Mar 18, 2021 at 04:55:25PM +0100, Johan Hovold wrote: > The driver data for the data interface has already been set by > usb_driver_claim_interface() so drop the subsequent redundant > assignment. > > Signed-off-by: Johan Hovold Applied, thank you. -- Dmitry
[PATCH] HID: do not use down_interruptible() when unbinding devices
Action of unbinding driver from a device is not cancellable and should not
fail, and driver core does not pay attention to the result of "remove"
method, therefore using down_interruptible() in hid_device_remove() does
not make sense.
Signed-off-by: Dmitry Torokhov
---
drivers/hid/hid-core.c | 10 +++---
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 56172fe6995c..ec63a9ff40dc 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -2300,12 +2300,8 @@ static int hid_device_remove(struct device *dev)
{
struct hid_device *hdev = to_hid_device(dev);
struct hid_driver *hdrv;
- int ret = 0;
- if (down_interruptible(>driver_input_lock)) {
- ret = -EINTR;
- goto end;
- }
+ down(>driver_input_lock);
hdev->io_started = false;
hdrv = hdev->driver;
@@ -2320,8 +2316,8 @@ static int hid_device_remove(struct device *dev)
if (!hdev->io_started)
up(>driver_input_lock);
-end:
- return ret;
+
+ return 0;
}
static ssize_t modalias_show(struct device *dev, struct device_attribute *a,
--
2.31.0.rc2.261.g7f71774620-goog
--
Dmitry
Re: [PATCH] Input: elan_i2c - Reduce the resume time for new dev ices
On Tue, Mar 09, 2021 at 10:53:34PM +0800, jingle.wu wrote: > Hi Dmitry: > > Was this the only issue with the updated patch? Did it work for you > otherwise? > -> Yes, the updated patch can work successfully after fix this issue. OK, great, I applied it. Thanks. -- Dmitry
Re: [PATCH] Input: elan_i2c - Reduce the resume time for new dev ices
Hi Jingle,
On Mon, Mar 08, 2021 at 04:56:14PM +0800, jingle wrote:
> Hi Dmitry:
>
> 1. missing "i<"
> + u32 quirks = 0;
> + int i;
> +
> + for (i = 0; ARRAY_SIZE(elan_i2c_quirks); i++) {
>
> -> for (i = 0; i
> 2. elan_resume () funtion are different with at Chromeos driver.
> @@ -1384,7 +1422,7 @@ static int __maybe_unused elan_resume(struct device
> *dev)
> goto err;
> }
>
> - error = elan_initialize(data);
> + error = elan_initialize(data, data->quirks &
> ETP_QUIRK_QUICK_WAKEUP);
> if (error)
> dev_err(dev, "initialize when resuming failed: %d\n",
> error);
>
> -> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/ref
> -> s/heads/chromeos-5.4/drivers/input/mouse/elan_i2c_core.c#1434
> -> error = elan_initialize(data); this code is in elan_reactivate()
> function at Chromeos driver.
> -> Will this change affect cherrypick from linux kernel to chromeos?
Yes, we would need to adjust the patch for Chrome OS and have
elan_reactivate() to call elan_initialize() with appropriate argument.
Thanks.
--
Dmitry
Re: [PATCH] Input: elan_i2c - Reduce the resume time for new dev ices
Hi Jingle,
On Fri, Mar 05, 2021 at 09:50:35AM +0800, jingle wrote:
> HI Dmitry:
>
> 1. You mean to let all devices ignore skipping reset/sleep part of device
> initialization?
> 2. The test team found that some old firmware will have errors (invalid
> report etc...), so ELAN can only ensure that the new device can meet the
> newer parts.
I see. OK, fair enough.
I would prefer if we were more explicit about when we skip resetting the
device, what do you think about the version of your patch below?
Thanks.
--
Dmitry
Input: elan_i2c - reduce the resume time for new devices
From: Jingle Wu
Newer controllers, such as Voxel, Delbin, Magple, Bobba and others, do not
need to be reset after issuing power-on command, and skipping reset saves
at least 100ms from resume time.
Note that if first attempt of re-initializing device fails we will not be
skipping reset on the subsequent ones.
Signed-off-by: Jingle Wu
Link: https://lore.kernel.org/r/20210226073537.4926-1-jingle...@emc.com.tw
Patchwork-Id: 12105967
Signed-off-by: Dmitry Torokhov
---
drivers/input/mouse/elan_i2c.h |5 +++
drivers/input/mouse/elan_i2c_core.c | 58 +--
2 files changed, 53 insertions(+), 10 deletions(-)
diff --git a/drivers/input/mouse/elan_i2c.h b/drivers/input/mouse/elan_i2c.h
index e12da5b024b0..838b3b346316 100644
--- a/drivers/input/mouse/elan_i2c.h
+++ b/drivers/input/mouse/elan_i2c.h
@@ -55,6 +55,11 @@
#define ETP_FW_PAGE_SIZE_512 512
#define ETP_FW_SIGNATURE_SIZE 6
+#define ETP_PRODUCT_ID_DELBIN 0x00C2
+#define ETP_PRODUCT_ID_VOXEL 0x00BF
+#define ETP_PRODUCT_ID_MAGPIE 0x0120
+#define ETP_PRODUCT_ID_BOBBA 0x0121
+
struct i2c_client;
struct completion;
diff --git a/drivers/input/mouse/elan_i2c_core.c
b/drivers/input/mouse/elan_i2c_core.c
index bef73822315d..51a65f6bf1e3 100644
--- a/drivers/input/mouse/elan_i2c_core.c
+++ b/drivers/input/mouse/elan_i2c_core.c
@@ -46,6 +46,9 @@
#define ETP_FINGER_WIDTH 15
#define ETP_RETRY_COUNT3
+/* quirks to control the device */
+#define ETP_QUIRK_QUICK_WAKEUP BIT(0)
+
/* The main device structure */
struct elan_tp_data {
struct i2c_client *client;
@@ -90,8 +93,38 @@ struct elan_tp_data {
boolbaseline_ready;
u8 clickpad;
boolmiddle_button;
+
+ u32 quirks; /* Various quirks */
};
+static u32 elan_i2c_lookup_quirks(u16 ic_type, u16 product_id)
+{
+ static const struct {
+ u16 ic_type;
+ u16 product_id;
+ u32 quirks;
+ } elan_i2c_quirks[] = {
+ { 0x0D, ETP_PRODUCT_ID_DELBIN, ETP_QUIRK_QUICK_WAKEUP },
+ { 0x10, ETP_PRODUCT_ID_VOXEL, ETP_QUIRK_QUICK_WAKEUP },
+ { 0x14, ETP_PRODUCT_ID_MAGPIE, ETP_QUIRK_QUICK_WAKEUP },
+ { 0x14, ETP_PRODUCT_ID_BOBBA, ETP_QUIRK_QUICK_WAKEUP },
+ };
+ u32 quirks = 0;
+ int i;
+
+ for (i = 0; ARRAY_SIZE(elan_i2c_quirks); i++) {
+ if (elan_i2c_quirks[i].ic_type == ic_type &&
+ elan_i2c_quirks[i].product_id == product_id) {
+ quirks = elan_i2c_quirks[i].quirks;
+ }
+ }
+
+ if (ic_type >= 0x0D && product_id >= 0x123)
+ quirks |= ETP_QUIRK_QUICK_WAKEUP;
+
+ return quirks;
+}
+
static int elan_get_fwinfo(u16 ic_type, u8 iap_version, u16 *validpage_count,
u32 *signature_address, u16 *page_size)
{
@@ -258,16 +291,18 @@ static int elan_check_ASUS_special_fw(struct elan_tp_data
*data)
return false;
}
-static int __elan_initialize(struct elan_tp_data *data)
+static int __elan_initialize(struct elan_tp_data *data, bool skip_reset)
{
struct i2c_client *client = data->client;
bool woken_up = false;
int error;
- error = data->ops->initialize(client);
- if (error) {
- dev_err(>dev, "device initialize failed: %d\n", error);
- return error;
+ if (!skip_reset) {
+ error = data->ops->initialize(client);
+ if (error) {
+ dev_err(>dev, "device initialize failed: %d\n",
error);
+ return error;
+ }
}
error = elan_query_product(data);
@@ -311,16 +346,17 @@ static int __elan_initialize(struct elan_tp_data *data)
return 0;
}
-static int elan_initialize(struct elan_tp_data *data)
+static int elan_initialize(struct elan_tp_data *data, bool skip_reset)
{
int repeat = ETP_RETRY_COUNT;
int error;
do {
- error = __elan_initialize(data);
+ error = __elan_initialize(data, skip_reset);
if (!error)
return 0;
+ skip_reset = f
Re: [PATCH v2] input: s6sy761: fix coordinate read bit shift
On Fri, Mar 05, 2021 at 06:58:10PM +, Caleb Connolly wrote:
> The touch coordinate register contains the following:
>
> byte 3 byte 2 byte 1
> +++ +-+ +-+
> ||| | | | |
> | X[3:0] | Y[3:0] | | Y[11:4] | | X[11:4] |
> ||| | | | |
> +++ +-+ +-+
>
> Bytes 2 and 1 need to be shifted left by 4 bits, the least significant
> nibble of each is stored in byte 3. Currently they are only
> being shifted by 3 causing the reported coordinates to be incorrect.
>
> This matches downstream examples, and has been confirmed on my
> device (OnePlus 7 Pro).
>
> Fixes: 0145a7141e59 ("Input: add support for the Samsung S6SY761
> touchscreen")
> Signed-off-by: Caleb Connolly
Applied, thank you.
--
Dmitry
Re: [PATCH v1 2/2] iio: adc: add ADC driver for the TI TSC2046 controller
Hi Oleksij, On Fri, Mar 05, 2021 at 02:38:13PM +0100, Oleksij Rempel wrote: > + > + /* TODO: remove IRQ_NOAUTOEN after needed patches are mainline */ > + irq_set_status_flags(spi->irq, IRQ_NOAUTOEN); > + ret = devm_request_threaded_irq(dev, spi->irq, > + NULL, > + _adc_irq, > + IRQF_TRIGGER_LOW | IRQF_ONESHOT, > + name, indio_dev); I'd recommend dropping IRQF_TRIGGER_LOW and only using IRQF_ONESHOT and rely on the platform (ACPI, DT) to specify trigger polarity according to how device is wired in a given system. In general I believe newer drivers should not specify interrupt triggers themselves. Thanks. -- Dmitry
Re: [PATCH 3/3] input: gpio-keys: Use hrtimer for software debounce
On Sun, Mar 07, 2021 at 09:11:18PM +, Paul Cercueil wrote:
> Hi Dmitry,
>
> Le dim. 7 mars 2021 à 12:20, Dmitry Torokhov a
> écrit :
> > On Fri, Mar 05, 2021 at 08:00:43PM +, Paul Cercueil wrote:
> > > Hi Dmitry,
> > >
> > > Le ven. 5 mars 2021 à 10:35, Dmitry Torokhov
> > > a
> > > écrit :
> > > > Hi Paul,
> > > >
> > > > On Fri, Mar 05, 2021 at 05:01:11PM +, Paul Cercueil wrote:
> > > > > -static void gpio_keys_gpio_work_func(struct work_struct *work)
> > > > > +static enum hrtimer_restart gpio_keys_debounce_timer(struct
> > > > > hrtimer *t)
> > > > > {
> > > > > - struct gpio_button_data *bdata =
> > > > > - container_of(work, struct gpio_button_data, work.work);
> > > > > + struct gpio_button_data *bdata = container_of(t,
> > > > > + struct
> > > gpio_button_data,
> > > > > + debounce_timer);
> > > > >
> > > > > gpio_keys_gpio_report_event(bdata);
> > > >
> > > > I am not sure how this works. As far as I know, even
> > > > HRTIMER_MODE_REL_SOFT do not allow sleeping in the timer
> > > handlers, and
> > > > gpio_keys_gpio_report_event() use sleeping variant of GPIOD API
> > > (and
> > > > that is not going to change).
> > >
> > > Quoting , the "timer callback will be executed in
> > > soft irq
> > > context", so sleeping should be possible.
> >
> > I am afraid you misunderstand what soft irq context is, as softirqs and
> > tasklets still run in interrupt context and therefore can not sleep,
> > only code running in process context may sleep.
>
> I probably do. My understanding of "softirq" is that the callback runs in a
> threaded interrupt handler.
No, you are thinking about threaded interrupts, which are separate
beasts. Softirqs are traditional bottom halfs that run after exit of
hard interrupt, but still are non-preemptible so sleeping is not
allowed.
>
> > You can test it yourself by sticking "msleep(1)" in
> > gpio_keys_debounce_timer() and see if you will get "scheduling while
> > atomic" in logs.
>
> I tested it, it locks up.
>
> > >
> > > But I guess in this case I can use HRTIMER_MODE_REL.
> >
> > This changes selected clock source, but has no effect on whether timer
> > handler can sleep or not.
> >
> > >
> > > > It seems to me that if you want to use software debounce in gpio
> > > keys
> > > > driver you need to set up sufficiently high HZ for your system.
> > > Maybe we
> > > > could thrown a warning when we see low debounce delay and low HZ
> > > to
> > > > alert system developer.
> > >
> > > This is exactly what we should not do. I certainly don't want to
> > > have 250+
> > > timer interrupts per second just so that input events aren't lost,
> > > to work
> > > around a sucky debounce implementation. Besides, if you consider the
> > > hrtimers doc (Documentation/timers/hrtimers.rst), hrtimers really
> > > are what
> > > should be used here.
> >
> > I explained why they can't. They could be if you restrict gpio_keys to
> > only be used with GPIOs that do not require sleep to read their state,
> > but I am not willing to accept such restriction. You either need to have
> > longer debounce, higher HZ, or see if you can use GPIO controller that
> > supports debounce handling. See also if you can enable dynamic
> > ticks/NO_HZ to limit number of timer interrupts on idle system.
>
> We can also use the hrtimer approach if the GPIO doesn't require sleep, and
> fall back to the standard timer if it does. It's possible to detect that
> with gpiod_cansleep(). The diff would be pretty slim. Would you accept
> something like that?
>
> Switching from HZ=250 to HZ=24 leads to a 3% overall performance increase
> across all apps on our system, so a pretty big optimization, and this is the
> only blocker.
Let me take a look at the updated patch and we will see.
Thanks.
--
Dmitry
Re: [PATCH 3/3] input: gpio-keys: Use hrtimer for software debounce
On Fri, Mar 05, 2021 at 08:00:43PM +, Paul Cercueil wrote:
> Hi Dmitry,
>
> Le ven. 5 mars 2021 à 10:35, Dmitry Torokhov a
> écrit :
> > Hi Paul,
> >
> > On Fri, Mar 05, 2021 at 05:01:11PM +, Paul Cercueil wrote:
> > > -static void gpio_keys_gpio_work_func(struct work_struct *work)
> > > +static enum hrtimer_restart gpio_keys_debounce_timer(struct
> > > hrtimer *t)
> > > {
> > > -struct gpio_button_data *bdata =
> > > -container_of(work, struct gpio_button_data, work.work);
> > > +struct gpio_button_data *bdata = container_of(t,
> > > + struct
> > > gpio_button_data,
> > > + debounce_timer);
> > >
> > > gpio_keys_gpio_report_event(bdata);
> >
> > I am not sure how this works. As far as I know, even
> > HRTIMER_MODE_REL_SOFT do not allow sleeping in the timer handlers, and
> > gpio_keys_gpio_report_event() use sleeping variant of GPIOD API (and
> > that is not going to change).
>
> Quoting , the "timer callback will be executed in soft irq
> context", so sleeping should be possible.
I am afraid you misunderstand what soft irq context is, as softirqs and
tasklets still run in interrupt context and therefore can not sleep,
only code running in process context may sleep.
You can test it yourself by sticking "msleep(1)" in
gpio_keys_debounce_timer() and see if you will get "scheduling while
atomic" in logs.
>
> But I guess in this case I can use HRTIMER_MODE_REL.
This changes selected clock source, but has no effect on whether timer
handler can sleep or not.
>
> > It seems to me that if you want to use software debounce in gpio keys
> > driver you need to set up sufficiently high HZ for your system. Maybe we
> > could thrown a warning when we see low debounce delay and low HZ to
> > alert system developer.
>
> This is exactly what we should not do. I certainly don't want to have 250+
> timer interrupts per second just so that input events aren't lost, to work
> around a sucky debounce implementation. Besides, if you consider the
> hrtimers doc (Documentation/timers/hrtimers.rst), hrtimers really are what
> should be used here.
I explained why they can't. They could be if you restrict gpio_keys to
only be used with GPIOs that do not require sleep to read their state,
but I am not willing to accept such restriction. You either need to have
longer debounce, higher HZ, or see if you can use GPIO controller that
supports debounce handling. See also if you can enable dynamic
ticks/NO_HZ to limit number of timer interrupts on idle system.
Thanks.
--
Dmitry
Re: [PATCH 3/3] Input: add driver for the Hycon HY46XX touchpanel series
Hi Giulio,
On Fri, Mar 05, 2021 at 05:38:34PM +0100, Giulio Benetti wrote:
> From: Giulio Benetti
>
> This patch adds support for Hycon HY46XX.
Could you please tell me where patches 1/3 and 2/3. I am guessing they
are dealing with DT bindings and are most likely to go through my tree
after Rob's ACK.
>
> Signed-off-by: Giulio Benetti
> ---
> MAINTAINERS | 1 +
> drivers/input/touchscreen/Kconfig| 12 +
> drivers/input/touchscreen/Makefile | 1 +
> drivers/input/touchscreen/hycon-hy46xx.c | 571 +++
> 4 files changed, 585 insertions(+)
> create mode 100644 drivers/input/touchscreen/hycon-hy46xx.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3f83daf6b2bf..7a1331657e4b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8250,6 +8250,7 @@ M: Giulio Benetti
> L: linux-in...@vger.kernel.org
> S: Maintained
> F: Documentation/devicetree/bindings/input/touchscreen/hycon,hy46xx.yaml
> +F: drivers/input/touchscreen/hy46xx.c
>
> HYGON PROCESSOR SUPPORT
> M: Pu Wen
> diff --git a/drivers/input/touchscreen/Kconfig
> b/drivers/input/touchscreen/Kconfig
> index 529614d364fe..5d4751d901cb 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -1335,4 +1335,16 @@ config TOUCHSCREEN_ZINITIX
> To compile this driver as a module, choose M here: the
> module will be called zinitix.
>
> +config TOUCHSCREEN_HYCON_HY46XX
> + tristate "Hycon hy46xx touchscreen support"
> + depends on I2C
> + help
> + Say Y here if you have a touchscreen using Hycon hy46xx,
> + or something similar enough.
> +
> + If unsure, say N.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called hycon-hy46xx.
> +
> endif
> diff --git a/drivers/input/touchscreen/Makefile
> b/drivers/input/touchscreen/Makefile
> index 6233541e9173..8b68cf3a3e6d 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -112,3 +112,4 @@ obj-$(CONFIG_TOUCHSCREEN_ROHM_BU21023)+=
> rohm_bu21023.o
> obj-$(CONFIG_TOUCHSCREEN_RASPBERRYPI_FW) += raspberrypi-ts.o
> obj-$(CONFIG_TOUCHSCREEN_IQS5XX) += iqs5xx.o
> obj-$(CONFIG_TOUCHSCREEN_ZINITIX)+= zinitix.o
> +obj-$(CONFIG_TOUCHSCREEN_HYCON_HY46XX) += hycon-hy46xx.o
> diff --git a/drivers/input/touchscreen/hycon-hy46xx.c
> b/drivers/input/touchscreen/hycon-hy46xx.c
> new file mode 100644
> index ..ef0dee9a43a9
> --- /dev/null
> +++ b/drivers/input/touchscreen/hycon-hy46xx.c
> @@ -0,0 +1,571 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2021
> + * Author(s): Giulio Benetti
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +
> +#define HY46XX_CHKSUM_CODE 0x1
> +#define HY46XX_FINGER_NUM0x2
> +#define HY46XX_CHKSUM_LEN0x7
> +#define HY46XX_THRESHOLD 0x80
> +#define HY46XX_PROX_SENS_SW 0x81
> +#define HY46XX_GLOVE_EN 0x84
> +#define HY46XX_REPORT_SPEED 0x88
> +#define HY46XX_PWR_NOISE_EN 0x89
> +#define HY46XX_FILTER_DATA 0x8A
> +#define HY46XX_GAIN 0x92
> +#define HY46XX_EDGE_OFFSET 0x93
> +#define HY46XX_RX_NR_USED0x94
> +#define HY46XX_TX_NR_USED0x95
> +#define HY46XX_PWR_MODE 0xA5
> +#define HY46XX_FW_VERSION0xA6
> +#define HY46XX_LIB_VERSION 0xA7
> +#define HY46XX_TP_INFO 0xA8
> +#define HY46XX_TP_CHIP_ID0xA9
> +#define HY46XX_BOOT_VER 0xB0
> +
> +#define HY46XX_TPLEN 0x6
> +
> +#define HY46XX_MAX_SUPPORTED_POINTS 11
> +
> +#define TOUCH_EVENT_DOWN 0x00
> +#define TOUCH_EVENT_UP 0x01
> +#define TOUCH_EVENT_CONTACT 0x02
> +#define TOUCH_EVENT_RESERVED 0x03
> +
> +struct hycon_hy46xx_data {
> + struct i2c_client *client;
> + struct input_dev *input;
> + struct touchscreen_properties prop;
> + struct regulator *vcc;
> +
> + struct gpio_desc *reset_gpio;
> +
> + struct mutex mutex;
> +
> + int threshold;
> + int proximity_sensor_switch;
> + int glove_enable;
> + int report_speed;
> + int power_noise_enable;
> + int filter_data;
> + int gain;
> + int edge_offset;
> + int rx_number_used;
> + int tx_number_used;
> + int power_mode;
> + int fw_version;
> + int lib_version;
> + int tp_information;
> + int tp_chip_id;
> + int bootloader_version;
> +};
> +
> +static int hycon_hy46xx_readwrite(struct i2c_client *client, u16 wr_len, u8
> *wr_buf,
> + u16 rd_len, u8 *rd_buf)
> +{
> + struct i2c_msg msgs[2];
> + int i = 0;
> + int ret;
> +
> + if (wr_len) {
> +
Re: [PATCH 1/3] input: gpio-keys: Remove extra call to input_sync
On Fri, Mar 05, 2021 at 05:01:09PM +, Paul Cercueil wrote: > The input_sync() function will already be called within > gpio_keys_gpio_report_event(), so there's no need to call it again after > the loop in gpio_keys_report_state(). I'd probably go other way around and remove the sync from gpio_keys_report_state() and add one to gpio_keys_gpio_work_func() so that we do not have to send bunch of input_sync() when there are several keys/buttons. Thanks. -- Dmitry
Re: [PATCH 3/3] input: gpio-keys: Use hrtimer for software debounce
Hi Paul,
On Fri, Mar 05, 2021 at 05:01:11PM +, Paul Cercueil wrote:
> -static void gpio_keys_gpio_work_func(struct work_struct *work)
> +static enum hrtimer_restart gpio_keys_debounce_timer(struct hrtimer *t)
> {
> - struct gpio_button_data *bdata =
> - container_of(work, struct gpio_button_data, work.work);
> + struct gpio_button_data *bdata = container_of(t,
> + struct gpio_button_data,
> + debounce_timer);
>
> gpio_keys_gpio_report_event(bdata);
I am not sure how this works. As far as I know, even
HRTIMER_MODE_REL_SOFT do not allow sleeping in the timer handlers, and
gpio_keys_gpio_report_event() use sleeping variant of GPIOD API (and
that is not going to change).
It seems to me that if you want to use software debounce in gpio keys
driver you need to set up sufficiently high HZ for your system. Maybe we
could thrown a warning when we see low debounce delay and low HZ to
alert system developer.
Thanks.
--
Dmitry
Re: [PATCH] Input: elan_i2c - Reduce the resume time for new dev ices
Hi Jingle, On Fri, Mar 05, 2021 at 09:24:05AM +0800, jingle wrote: > HI Dmitry: > > In this case (in the newer parts behavior regarding need to reset after > powering them on), it is consistent with the original driver behavior with > any new or old device > (be called data->ops->initialize(client) : usleep(100) , etc.. , because > this times "data->quirks" is equal 0 at probe state.) You misunderstood my question. I was asking what specifically, if anything, was changed in the firmware to allow skipping reset/sleep part of device initialization on newer parts during resume process. Because of there were no specific changes I would say let's not do a quirk and change the driver to skip reset on resume. Thanks. -- Dmitry
Re: [PATCH] Input: elan_i2c - Reduce the resume time for new dev ices
Hi Jingle, On Tue, Mar 02, 2021 at 09:04:57AM +0800, jingle.wu wrote: > HI Dmitry: > > So data->ops->initialize(client) essentially performs reset of the > controller (we may want to rename it even) and as far as I understand > you would want to avoid resetting the controller on newer devices, > right? > > -> YES > > My question is how behavior of older devices differ from the new ones > (are they stay in "undefined" state at power up) and whether it is > possible to determine if controller is in operating mode. For example, > what would happen on older devices if we call elan_query_product() below > without resetting the controller? > > -> But there may be other problems, because ELAN can't test all the older > devices , > -> so use quirk to divide this part. OK, but could you please tell me what exactly was changed in the newer parts behavior regarding need to reset after powering them on? Thanks. -- Dmitry
Re: [PATCH] Input: elan_i2c - Reduce the resume time for new devices
Hi Jingle,
On Fri, Feb 26, 2021 at 03:35:37PM +0800, jingle.wu wrote:
> @@ -273,10 +318,12 @@ static int __elan_initialize(struct elan_tp_data *data)
> bool woken_up = false;
> int error;
>
> - error = data->ops->initialize(client);
> - if (error) {
> - dev_err(>dev, "device initialize failed: %d\n", error);
> - return error;
> + if (!(data->quirks & ETP_QUIRK_SET_QUICK_WAKEUP_DEV)) {
> + error = data->ops->initialize(client);
> + if (error) {
> + dev_err(>dev, "device initialize failed: %d\n",
> error);
> + return error;
> + }
So data->ops->initialize(client) essentially performs reset of the
controller (we may want to rename it even) and as far as I understand
you would want to avoid resetting the controller on newer devices,
right?
My question is how behavior of older devices differ from the new ones
(are they stay in "undefined" state at power up) and whether it is
possible to determine if controller is in operating mode. For example,
what would happen on older devices if we call elan_query_product() below
without resetting the controller?
I also think that while I can see us skipping reset in resume paths we
probably want to keep it in probe as we really do not know the state of
the device (was it powered up properly earlier, etc).
> }
>
> error = elan_query_product(data);
> @@ -366,6 +413,8 @@ static int elan_query_device_info(struct elan_tp_data
> *data)
> if (error)
> return error;
>
> + data->quirks = elan_i2c_lookup_quirk(data->ic_type, data->product_id);
> +
> error = elan_get_fwinfo(data->ic_type, data->iap_version,
> >fw_validpage_count,
> >fw_signature_address,
> --
> 2.17.1
>
Thanks.
--
Dmitry
Re: [PATCH] Input: Add "Share" button to Microsoft Xbox One controller.
On Wed, Feb 24, 2021 at 08:44:37PM -0800, Chris Ye wrote:
> Hi Dmitry,
> The latest Xbox One X series has this button, I can add a new
> XTYPE_XBOXONE_X and only apply the change to the new type.
Sounds good to me. Cameron, what do you think?
> The controller supports bluetooth and the HID usage for this button is
> consumer 0xB2:
> 0x05, 0x0C,// Usage Page (Consumer)
> 0x0A, 0xB2, 0x00, // Usage (Record)
I see, thank you.
>
> Thanks!
> Chris
>
> On Wed, Feb 24, 2021 at 8:33 PM Dmitry Torokhov
> wrote:
> >
> > Hi Chris,
> >
> > On Thu, Feb 25, 2021 at 04:00:32AM +, Chris Ye wrote:
> > > Add "Share" button input capability and input event mapping for
> > > Microsoft Xbox One controller.
> > > Fixed Microsoft Xbox One controller share button not working under USB
> > > connection.
> > >
> > > Signed-off-by: Chris Ye
> > > ---
> > > drivers/input/joystick/xpad.c | 16 ++--
> > > 1 file changed, 14 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> > > index 9f0d07dcbf06..08c3e93ccb2f 100644
> > > --- a/drivers/input/joystick/xpad.c
> > > +++ b/drivers/input/joystick/xpad.c
> > > @@ -368,6 +368,14 @@ static const signed short xpad360_btn[] = { /*
> > > buttons for x360 controller */
> > > -1
> > > };
> > >
> > > +static const signed short xpad_xboxone_btn[] = {
> > > + /* buttons for xbox one controller */
> > > + BTN_TL, BTN_TR, /* Button LB/RB */
> > > + BTN_MODE, /* The big X button */
> > > + KEY_RECORD, /* The share button */
> >
> > If I understand this correctly, not all Xbox One controllers have this
> > new key. Is it possible to determine if it is present and only set
> > capability for controllers that actually have it?
> >
> > Also, I am unsure if KEY_RECORD is the best keycode for this. It might,
> > but does your controller supports bluetooth? What HID usage code does it
> > send for this key?
> >
> > Thanks.
> >
> > --
> > Dmitry
--
Dmitry
Re: [PATCH] Input: edt-ft5x06: Replace DEFINE_SIMPLE_ATTRIBUTE with DEFINE_DEBUGFS_ATTRIBUTE
Hi Yang, On Wed, Feb 24, 2021 at 04:43:26PM +0800, Yang Li wrote: > Fix the following coccicheck warning: > ./drivers/input/touchscreen/edt-ft5x06.c:697:0-23: WARNING: > debugfs_mode_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE Thank you for the patch, however reading the coccinelle script not only we need to change the attribute definition, but also use debugfs_create_file_unsafe() to register it. I do not see the 2nd part of the change in your patch... > > Reported-by: Abaci Robot > Signed-off-by: Yang Li > --- > drivers/input/touchscreen/edt-ft5x06.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/input/touchscreen/edt-ft5x06.c > b/drivers/input/touchscreen/edt-ft5x06.c > index 2eefbc2..12bbc58 100644 > --- a/drivers/input/touchscreen/edt-ft5x06.c > +++ b/drivers/input/touchscreen/edt-ft5x06.c > @@ -694,7 +694,7 @@ static int edt_ft5x06_debugfs_mode_set(void *data, u64 > mode) > return retval; > }; > > -DEFINE_SIMPLE_ATTRIBUTE(debugfs_mode_fops, edt_ft5x06_debugfs_mode_get, > +DEFINE_DEBUGFS_ATTRIBUTE(debugfs_mode_fops, edt_ft5x06_debugfs_mode_get, > edt_ft5x06_debugfs_mode_set, "%llu\n"); > > static ssize_t edt_ft5x06_debugfs_raw_data_read(struct file *file, > -- > 1.8.3.1 > Thanks. -- Dmitry
Re: [PATCH] Input: Add "Share" button to Microsoft Xbox One controller.
Hi Chris,
On Thu, Feb 25, 2021 at 04:00:32AM +, Chris Ye wrote:
> Add "Share" button input capability and input event mapping for
> Microsoft Xbox One controller.
> Fixed Microsoft Xbox One controller share button not working under USB
> connection.
>
> Signed-off-by: Chris Ye
> ---
> drivers/input/joystick/xpad.c | 16 ++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/joystick/xpad.c b/drivers/input/joystick/xpad.c
> index 9f0d07dcbf06..08c3e93ccb2f 100644
> --- a/drivers/input/joystick/xpad.c
> +++ b/drivers/input/joystick/xpad.c
> @@ -368,6 +368,14 @@ static const signed short xpad360_btn[] = { /* buttons
> for x360 controller */
> -1
> };
>
> +static const signed short xpad_xboxone_btn[] = {
> + /* buttons for xbox one controller */
> + BTN_TL, BTN_TR, /* Button LB/RB */
> + BTN_MODE, /* The big X button */
> + KEY_RECORD, /* The share button */
If I understand this correctly, not all Xbox One controllers have this
new key. Is it possible to determine if it is present and only set
capability for controllers that actually have it?
Also, I am unsure if KEY_RECORD is the best keycode for this. It might,
but does your controller supports bluetooth? What HID usage code does it
send for this key?
Thanks.
--
Dmitry
[git pull] Input updates for v5.12-rc0
Hi Linus, Please pull from: git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git for-linus to receive updates for the input subsystem. Mostly existing driver fixes plus a new driver for game controllers directly connected to Nintendo 64, and an enhancement for keyboards driven by Chrome OS EC to communicate layout of the top row to userspace. Changelog: - Bhaskar Chowdhury (1): Input: alps - fix spelling of "positive" Dan Carpenter (3): Input: sur40 - fix an error code in sur40_probe() Input: elo - fix an error code in elo_connect() Input: joydev - prevent potential read overflow in ioctl Dmitry Torokhov (6): Input: da7280 - fix missing error test Input: da7280 - protect OF match table with CONFIG_OF Input: imx_keypad - add dependency on HAS_IOMEM Input: omap4-keypad - switch to use managed resources Input: add missing dependencies on CONFIG_HAS_IOMEM Input: zinitix - fix return type of zinitix_init_touch() Geert Uytterhoeven (1): Input: st1232 - fix NORMAL vs. IDLE state handling Jeff LaBundy (9): Input: iqs5xx - minor cosmetic improvements Input: iqs5xx - preserve bootloader errors Input: iqs5xx - accommodate bootloader latency Input: iqs5xx - re-initialize device upon warm reset Input: iqs5xx - simplify axis setup logic Input: iqs5xx - eliminate unnecessary register read Input: iqs5xx - allow more time for ATI to complete Input: iqs5xx - allow device to be a wake-up source Input: iqs5xx - initialize an uninitialized variable Jiapeng Chong (1): Input: aiptek - convert sysfs sprintf/snprintf family to sysfs_emit Josh Poimboeuf (1): Input: elants_i2c - detect enum overflow Lauri Kasanen (1): Input: Add N64 controller driver Lee Jones (5): Input: synaptics - replace NOOP with suitable commentary Input: melfas_mip4 - mark a bunch of variables as __always_unused Input: usbtouchscreen - actually check return value of usb_submit_urb() Input: surface3_spi - remove set but unused variable 'timestamp' Input: stmpe-ts - add description for 'prop' struct member Marcos Paulo de Souza (1): Input: i8042 - add ASUS Zenbook Flip to noselftest list Michael Tretter (1): Input: st1232 - add IDLE state as ready condition Michał Mirosław (1): Input: elants_i2c - add support for eKTF3624 Oleksij Rempel (1): Input: ads7846 - convert to one message Olivier Crête (1): Input: xpad - add support for PowerA Enhanced Wired Controller for Xbox Series X|S Philip Chen (5): dt-bindings: input: cros-ec-keyb: Add a new property describing top row Input: cros-ec-keyb - expose function row physical map to userspace dt-bindings: input: Create macros for cros-ec keymap dt-bindings: input: Fix the keymap for LOCK key ARM: dts: cros-ec-keyboard: Use keymap macros Ronald Tschalär (2): Input: applespi - don't wait for responses to commands indefinitely. Input: applespi - fix occasional crc errors under load. Tony Lindgren (5): Input: omap4-keypad - disable unused long interrupts Input: omap4-keypad - scan keys in two phases and simplify with bitmask Input: omap4-keypad - move rest of key scanning to a separate function Input: omap4-keypad - use PM runtime autosuspend Input: omap4-keypad - implement errata check for lost key-up events Yang Li (1): Input: zinitix - remove unneeded semicolon jeffrey.lin (1): Input: raydium_ts_i2c - do not send zero length Diffstat: .../ABI/testing/sysfs-driver-input-cros-ec-keyb| 6 + .../bindings/input/google,cros-ec-keyb.yaml| 24 ++ arch/arm/boot/dts/cros-ec-keyboard.dtsi| 93 + drivers/input/joydev.c | 7 +- drivers/input/joystick/Kconfig | 7 + drivers/input/joystick/Makefile| 2 +- drivers/input/joystick/n64joy.c| 345 +++ drivers/input/joystick/xpad.c | 1 + drivers/input/keyboard/Kconfig | 6 +- drivers/input/keyboard/applespi.c | 23 +- drivers/input/keyboard/cros_ec_keyb.c | 79 + drivers/input/keyboard/omap4-keypad.c | 302 ++--- drivers/input/misc/da7280.c| 3 + drivers/input/mouse/alps.c | 2 +- drivers/input/mouse/synaptics.c| 7 +- drivers/input/serio/Kconfig| 2 +- drivers/input/serio/i8042-x86ia64io.h | 4 + drivers/input/tablet/aiptek.c | 80 ++--- drivers/input/touchscreen/Kconfig | 2 +- drivers/input/touchscreen/ads7846.c| 376 +++-- drivers/input/touchscreen/elants_i2c.c | 151 - drivers/input/touchsc
Re: [PATCH] Input: st1232 - Fix NORMAL vs. IDLE state handling
On Tue, Feb 23, 2021 at 11:29:00AM +0100, Michael Tretter wrote:
> On Tue, 23 Feb 2021 10:02:01 +0100, Geert Uytterhoeven wrote:
> > NORMAL (0x0) and IDLE (0x4) are really two different states. Hence you
> > cannot check for both using a bitmask, as that checks for IDLE only,
> > breaking operation for devices that are in NORMAL state.
>
> Thanks. I missed that the state is actually a value and not a bitfield.
>
> >
> > Fix the wait function to report either state as ready.
> >
> > Fixes: 6524d8eac258452e ("Input: st1232 - add IDLE state as ready
> > condition")
> > Signed-off-by: Geert Uytterhoeven
>
> Reviewed-by: Michael Tretter
Applied, thank you.
--
Dmitry
Re: [PATCH v6 3/3] dt-bindings: input: Fix the keymap for LOCK key
On Fri, Jan 15, 2021 at 02:36:17PM -0800, Philip Chen wrote: > Decouple LOCK from F13 and directly map the LOCK key (KSI3/KSO9) to > KEY_SLEEP action key code. > > Signed-off-by: Philip Chen Applied, thank you. -- Dmitry
Re: [PATCH v6 1/3] dt-bindings: input: Create macros for cros-ec keymap
On Fri, Jan 15, 2021 at 02:36:15PM -0800, Philip Chen wrote: > In Chrome OS, the keyboard matrix can be split to two groups: > > The keymap for the top row keys can be customized based on OEM > preference, while the keymap for the other keys is generic/fixed > across boards. > > This patch creates marcos for the keymaps of these two groups, making > it easier to reuse the generic portion of keymap when we override the > keymap in the board-specific dts for custom top row design. > > Signed-off-by: Philip Chen Applied, thank you. -- Dmitry
Re: [PATCH v6 2/3] ARM: dts: cros-ec-keyboard: Use keymap macros
On Fri, Jan 15, 2021 at 02:36:16PM -0800, Philip Chen wrote: > The common cros-ec keymap has been defined as macros. This patch uses > the macros to simply linux,keymap in cros-ec-keyboard.dtsi file. > > This patch also creates an alias for keyboard-controller to make it > easier to override the keymap in board-specific dts later. > > Signed-off-by: Philip Chen Applied, thank you. -- Dmitry
Re: [PATCH v7 2/2] Input: cros-ec-keyb - Expose function row physical map to userspace
On Fri, Jan 15, 2021 at 12:24:30PM -0800, Philip Chen wrote: > The top-row keys in a keyboard usually have dual functionalities. > E.g. A function key "F1" is also an action key "Browser back". > > Therefore, when an application receives an action key code from > a top-row key press, the application needs to know how to correlate > the action key code with the function key code and do the conversion > whenever necessary. > > Since the userpace already knows the key scanlines (row/column) > associated with a received key code. Essentially, the userspace only > needs a mapping between the key row/column and the matching physical > location in the top row. > > So, enhance the cros-ec-keyb driver to create such a mapping > and expose it to userspace in the form of a function_row_physmap > attribute. The attribute would be a space separated ordered list of > row/column codes for the keys in the function row, in a left-to-right > order. > > The attribute will only be present when the device has a custom design > for the top-row keys. > > Signed-off-by: Philip Chen Applied, thank you. -- Dmitry
Re: [PATCH v7 1/2] dt-bindings: input: cros-ec-keyb: Add a new property
On Fri, Jan 15, 2021 at 12:24:29PM -0800, Philip Chen wrote: > Add a new property `function-row-physmap` to the > device tree for the custom keyboard top row design. > > The property describes the rows/columns of the top row keys > from left to right. > > Signed-off-by: Philip Chen Applied, thank you. -- Dmitry
Re: [PATCH v4 2/2] Input: add MStar MSG2638 touchscreen driver
Hi Vincent,
On Wed, Feb 10, 2021 at 06:33:52PM +0100, Vincent Knecht wrote:
> +
> + for (i = 0; i < MAX_SUPPORTED_FINGER_NUM; i++) {
> + p = _event.pkt[i];
> + /* Ignore non-pressed finger data */
> + if (p->xy_hi == 0xFF && p->x_low == 0xFF && p->y_low == 0xFF)
> + continue;
> +
> + coord.x = (((p->xy_hi & 0xF0) << 4) | p->x_low) *
> msg2638->prop.max_x / TPD_WIDTH;
> + coord.y = (((p->xy_hi & 0x0F) << 8) | p->y_low) *
> msg2638->prop.max_y / TPD_HEIGHT;
> + msg2638_report_finger(msg2638, i, );
We do not scale the coordinates in the kernel. Rather we provide
resolution, if known, and min/max coordinates reported by the hardware,
and let userspace handle the rest.
> +static int __maybe_unused msg2638_suspend(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct msg2638_ts_data *msg2638 = i2c_get_clientdata(client);
> +
> + mutex_lock(>input_dev->mutex);
> +
> + if (input_device_enabled(msg2638->input_dev))
> + msg2638_stop(msg2638);
I believe that you should power down the device only if it is not
configures as wakeup source. In fact (and I think most drivers are
wrong in this), you may want to power up the device if it is a wakeup
source and it does not have any users.
Thanks.
--
Dmitry
Re: [PATCH 2/3] Input: applespi: Fix occasional crc errors under load.
On Wed, Feb 17, 2021 at 11:07:17AM -0800, Ronald Tschalär wrote: > For some reason, when the system is under heavy CPU load, the read > following the write sometimes occurs unusually quickly, resulting in > the read data not being quite ready and hence a bad packet getting read. > Adding another delay after reading the status message appears to fix > this. > > Signed-off-by: Ronald Tschalär Applied, thank you. -- Dmitry
Re: [PATCH 1/3] Input: applespi: Don't wait for responses to commands indefinitely.
On Wed, Feb 17, 2021 at 12:45:51PM -0800, Life is hard, and then you die wrote: > > Hi Dmitry, > > On Wed, Feb 17, 2021 at 12:23:23PM -0800, Dmitry Torokhov wrote: > > > > On Wed, Feb 17, 2021 at 11:07:16AM -0800, Ronald Tschalär wrote: > > > @@ -869,7 +878,7 @@ static int applespi_send_cmd_msg(struct applespi_data > > > *applespi) > > > return sts; > > > } > > > > > > - applespi->cmd_msg_queued = true; > > > + applespi->cmd_msg_queued = ktime_get(); > > > > Will it be OK if I change this to ktime_get_coarse()? I do not think we > > need exact time here, and the coarse variant is cheaper I believe. > > Certainly - I just wasn't aware of the coarse variant. Applied, thank you. -- Dmitry
[PATCH] Input: zinitix - fix return type of zinitix_init_touch()
zinitix_init_touch() returns error code or 0 for success and therefore
return type must be int, not bool.
Fixes: 26822652c85e ("Input: add zinitix touchscreen driver")
Reported-by: kernel test robot
Reported-by: Jiapeng Chong
Signed-off-by: Dmitry Torokhov
---
drivers/input/touchscreen/zinitix.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/touchscreen/zinitix.c
b/drivers/input/touchscreen/zinitix.c
index f64d88170fac..3b636beb583c 100644
--- a/drivers/input/touchscreen/zinitix.c
+++ b/drivers/input/touchscreen/zinitix.c
@@ -190,7 +190,7 @@ static int zinitix_write_cmd(struct i2c_client *client, u16
reg)
return 0;
}
-static bool zinitix_init_touch(struct bt541_ts_data *bt541)
+static int zinitix_init_touch(struct bt541_ts_data *bt541)
{
struct i2c_client *client = bt541->client;
int i;
--
2.30.0.617.g56c4b15f3c-goog
--
Dmitry
Re: [PATCH] Input: fix boolreturn.cocci warnings
Hi,
On Thu, Feb 18, 2021 at 05:59:53PM +0800, kernel test robot wrote:
> From: kernel test robot
>
> drivers/input/touchscreen/zinitix.c:250:8-9: WARNING: return of 0/1 in
> function 'zinitix_init_touch' with return type bool
>
> Return statements in functions returning bool should use
> true/false instead of 1/0.
> Generated by: scripts/coccinelle/misc/boolreturn.cocci
>
> Fixes: 26822652c85e ("Input: add zinitix touchscreen driver")
> CC: Michael Srba
> Reported-by: kernel test robot
> Signed-off-by: kernel test robot
> ---
>
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> master
> head: f40ddce88593482919761f74910f42f4b84c004b
> commit: 26822652c85eff14e40115255727b2693400c524 Input: add zinitix
> touchscreen driver
>
> zinitix.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> --- a/drivers/input/touchscreen/zinitix.c
> +++ b/drivers/input/touchscreen/zinitix.c
> @@ -247,7 +247,7 @@ static bool zinitix_init_touch(struct bt
> udelay(10);
> }
>
> - return 0;
> + return false;
This is incorrect as function is trying to return error codes. It needs
to be changed to return int. I'll take care of it.
Thanks.
--
Dmitry
Re: [PATCH] Input: Use true and false for bool variable
Hi, On Thu, Feb 18, 2021 at 06:23:55PM +0800, Jiapeng Chong wrote: > Fix the following coccicheck warnings: > > ./drivers/input/touchscreen/zinitix.c:250:8-9: WARNING: return of 0/1 in > function 'zinitix_init_touch' with return type bool. > > Reported-by: Abaci Robot > Signed-off-by: Jiapeng Chong > --- > drivers/input/touchscreen/zinitix.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/input/touchscreen/zinitix.c > b/drivers/input/touchscreen/zinitix.c > index a3e3adb..acb1d53 100644 > --- a/drivers/input/touchscreen/zinitix.c > +++ b/drivers/input/touchscreen/zinitix.c > @@ -247,7 +247,7 @@ static bool zinitix_init_touch(struct bt541_ts_data > *bt541) > udelay(10); > } > > - return 0; > + return false; This is incorrect, as earlier we try to return error codes from this function. It needs to be changed to return int, I'll take care of it. Thanks. -- Dmitry
Re: [PATCH 3/3] Input: applespi: Add trace_event module param for early tracing.
On Wed, Feb 17, 2021 at 12:52:57PM -0800, Life is hard, and then you die wrote: > > Hi Dmitry, > > On Wed, Feb 17, 2021 at 12:26:18PM -0800, Dmitry Torokhov wrote: > > > > On Wed, Feb 17, 2021 at 11:07:18AM -0800, Ronald Tschalär wrote: > > > The problem is that tracing can't be set via sysfs until the module is > > > loaded, at which point the keyboard and trackpad initialization commands > > > have already been run and hence tracing can't be used to debug problems > > > here. > > > > > > Adding this param allows tracing to be enabled for messages sent and > > > received during module probing. It takes comma-separated list of events, > > > e.g. > > > > > > trace_event=applespi_tp_ini_cmd,applespi_bad_crc > > > > You can unbind and rebind a device to a driver via sysfs as many times > > as needed (see bind and unbind driver sysfs attributes), so I believe > > Hmm, I'm going to have to play with that a bit, but one place it still > won't help I think is something we ran into in practise: init was > failing during boot, but was successfull later on. Maybe compiling module as a built-in and then using kernel command line option to initiate the trace would work? If this facility is really needed, it would be beneficial for other modules as well, and thus better implemented in the module loading code to activate desired tracing after loading the module but before invoking module init code. Thanks. -- Dmitry
Re: [PATCH 3/3] Input: applespi: Add trace_event module param for early tracing.
Hi Ronald, On Wed, Feb 17, 2021 at 11:07:18AM -0800, Ronald Tschalär wrote: > The problem is that tracing can't be set via sysfs until the module is > loaded, at which point the keyboard and trackpad initialization commands > have already been run and hence tracing can't be used to debug problems > here. > > Adding this param allows tracing to be enabled for messages sent and > received during module probing. It takes comma-separated list of events, > e.g. > > trace_event=applespi_tp_ini_cmd,applespi_bad_crc You can unbind and rebind a device to a driver via sysfs as many times as needed (see bind and unbind driver sysfs attributes), so I believe Thanks. -- Dmitry
Re: [PATCH 1/3] Input: applespi: Don't wait for responses to commands indefinitely.
Hi Ronald, On Wed, Feb 17, 2021 at 11:07:16AM -0800, Ronald Tschalär wrote: > @@ -869,7 +878,7 @@ static int applespi_send_cmd_msg(struct applespi_data > *applespi) > return sts; > } > > - applespi->cmd_msg_queued = true; > + applespi->cmd_msg_queued = ktime_get(); Will it be OK if I change this to ktime_get_coarse()? I do not think we need exact time here, and the coarse variant is cheaper I believe. Thanks. -- Dmitry
[git pull] Input updates for v5.11-rc6
Hi Linus, Please pull from: git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git for-linus to receive updates for the input subsystem. Nothing terribly interesting, just a few fixups. Changelog: - Alexey Dobriyan (1): Input: i8042 - unbreak Pegatron C15B AngeloGioacchino Del Regno (2): dt-bindings: input: touchscreen: goodix: Add binding for GT9286 IC Input: goodix - add support for Goodix GT9286 chip Benjamin Valentin (1): Input: xpad - sync supported devices with fork on GitHub Geert Uytterhoeven (3): Input: st1232 - fix off-by-one error in resolution handling Input: st1232 - do not read more bytes than needed Input: st1232 - wait until device is ready before reading resolution Heinrich Schuchardt (1): dt-bindings: input: adc-keys: clarify description Marek Vasut (1): Input: ili210x - implement pressure reporting for ILI251x Souptick Joarder (1): Input: ariel-pwrbutton - remove unused variable ariel_pwrbutton_id_table Diffstat: .../devicetree/bindings/input/adc-keys.txt | 22 +- .../bindings/input/touchscreen/goodix.yaml | 1 + drivers/input/joystick/xpad.c | 17 +++- drivers/input/misc/ariel-pwrbutton.c | 6 --- drivers/input/serio/i8042-x86ia64io.h | 2 + drivers/input/touchscreen/goodix.c | 2 + drivers/input/touchscreen/ili210x.c| 26 drivers/input/touchscreen/st1232.c | 48 +++--- 8 files changed, 102 insertions(+), 22 deletions(-) Thanks. -- Dmitry
Re: [PATCH v5 05/20] Input: axp20x-pek: Bail out if AXP has no interrupt line connected
Hi Andre, On Wed, Jan 27, 2021 at 05:24:45PM +, Andre Przywara wrote: > On at least one board (Orangepi Zero2) the AXP305 PMIC does not have its > interrupt line connected to the CPU (mostly because the H616 SoC does > not feature an NMI pin anymore). > After allowing the AXP driver to proceed without an "interrupts" > property [1], the axp20x-pek driver crashes with a NULL pointer > dereference (see below). > > Check for the regmap_irqc member to be not NULL before proceeding with > probe. This gets normally filled by the call to regmap_add_irq_chip(), > which we allow to skip now, when the DT node lacks an interrupt > property. No, the driver is not the right place to patch this; regmap should be fixed so it does not crash instead. Thanks. -- Dmitry
Re: [PATCH v9] Input: elants_i2c - support eKTF3624
On Sun, Jan 24, 2021 at 10:54:14PM +0300, Dmitry Osipenko wrote: > From: Michał Mirosław > > Add ELAN KTF3624 touchscreen support to the elants_i2c driver. > The eKTF3624 TS is found on a series of ASUS Transformer tablet devices, > Nexus 7 tablet and etc. The firmware interface of eKTF3624 is nearly > identical to eKTH3500, which is already supported by the driver. > The minor differences of the firmware interface are now handled by > the driver. The eKTF3624 support was tested on ASUS Transformer TF700T, > TF300T and Nexus 7 tablets. > > Signed-off-by: Michał Mirosław > Signed-off-by: Dmitry Osipenko Applied, thank you. -- Dmitry
