* [PATCH] input : avoid too late kobject_uevent(KOBJ_REMOVE) call
From: Tetsuo Handa @ 2019-02-18 10:09 UTC (permalink / raw)
To: dmitry.torokhov, rydberg
Cc: linux-input, linux-kernel, syzkaller-bugs, Tetsuo Handa,
Kay Sievers, syzbot
syzbot is hitting use-after-free bug in uinput module [1]. This is because
kobject_uevent(KOBJ_REMOVE) is called again due to commit 0f4dafc0563c6c49
("Kobject: auto-cleanup on final unref") after memory allocation fault
injection made kobject_uevent(KOBJ_REMOVE) from device_del() from
input_unregister_device() fail, while uinput_destroy_device() is expecting
that kobject_uevent(KOBJ_REMOVE) is not called after device_del() from
input_unregister_device() completed.
Fix this problem by pretending as if kobject_uevent(KOBJ_REMOVE) from
device_del() from input_unregister_device() did not fail.
[1] https://syzkaller.appspot.com/bug?id=8b17c134fe938bbddd75a45afaa9e68af43a362d
Reported-by: syzbot <syzbot+f648cfb7e0b52bf7ae32@syzkaller.appspotmail.com>
Analyzed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Kay Sievers <kay@vrfy.org>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
drivers/input/input.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/drivers/input/input.c b/drivers/input/input.c
index 3304aaaffe87..6df3c33ef3aa 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -2032,6 +2032,19 @@ static void __input_unregister_device(struct input_dev *dev)
mutex_unlock(&input_mutex);
device_del(&dev->dev);
+ /*
+ * Regarding input subsystem, we always take care of sending uevent at
+ * "unregister" time, and we do not expect to have uevent sent out at
+ * the final "put" time. Therefore, if we failed to send uevent at
+ * "unregister" time (due to e.g. fault injection), complain it and
+ * do not allow the final "put" time to send the remove uevent again.
+ */
+ if (dev->dev.kobj.state_add_uevent_sent &&
+ !dev->dev.kobj.state_remove_uevent_sent) {
+ dev->dev.kobj.state_remove_uevent_sent = 1;
+ pr_warn("Failed to send remove uevent for %s\n",
+ dev_name(&dev->dev));
+ }
}
static void devm_input_device_unregister(struct device *dev, void *res)
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v3 1/4] dt-bindings: input: touchscreen: goodix: Document AVDD28-supply property
From: Jagan Teki @ 2019-02-18 9:33 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Rob Herring, devicetree, Chen-Yu Tsai, linux-input,
linux-kernel@vger.kernel.org, Michael Trimarchi, linux-amarula
In-Reply-To: <20190218071207.GE242714@dtor-ws>
Hi Dmitry,
On Mon, Feb 18, 2019 at 12:42 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Sun, Feb 17, 2019 at 02:45:52PM +0530, Jagan Teki wrote:
> > On Sun, Feb 17, 2019 at 1:21 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > >
> > > Hi Dmitry and Rob,
> > >
> > > On Thu, Feb 14, 2019 at 3:21 AM Rob Herring <robh@kernel.org> wrote:
> > > >
> > > > On Tue, Jan 22, 2019 at 7:39 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > > > >
> > > > > On Mon, Jan 21, 2019 at 9:46 PM Rob Herring <robh@kernel.org> wrote:
> > > > > >
> > > > > > On Fri, Jan 18, 2019 at 10:01 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > > > > > >
> > > > > > > On Wed, Jan 9, 2019 at 7:08 PM Rob Herring <robh@kernel.org> wrote:
> > > > > > > >
> > > > > > > > Please CC DT list if you want bindings reviewed.
> > > > > > >
> > > > > > > Sorry I forgot.
> > > > > > >
> > > > > > > >
> > > > > > > > On Wed, Jan 9, 2019 at 1:40 AM Dmitry Torokhov
> > > > > > > > <dmitry.torokhov@gmail.com> wrote:
> > > > > > > > >
> > > > > > > > > On Sat, Dec 15, 2018 at 08:47:59PM +0530, Jagan Teki wrote:
> > > > > > > > > > Most of the Goodix CTP controllers are supply with AVDD28 pin.
> > > > > > > > > > which need to supply for controllers like GT5663 on some boards
> > > > > > > > > > to trigger the power.
> > > > > > > > > >
> > > > > > > > > > So, document the supply property so-that the require boards
> > > > > > > > > > that used on GT5663 can enable it via device tree.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > > > > > > > ---
> > > > > > > > > > Documentation/devicetree/bindings/input/touchscreen/goodix.txt | 1 +
> > > > > > > > > > 1 file changed, 1 insertion(+)
> > > > > > > > > >
> > > > > > > > > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> > > > > > > > > > index f7e95c52f3c7..c4622c983e08 100644
> > > > > > > > > > --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> > > > > > > > > > +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> > > > > > > > > > @@ -23,6 +23,7 @@ Optional properties:
> > > > > > > > > > - touchscreen-inverted-y : Y axis is inverted (boolean)
> > > > > > > > > > - touchscreen-swapped-x-y : X and Y axis are swapped (boolean)
> > > > > > > > > > (swapping is done after inverting the axis)
> > > > > > > > > > + - AVDD28-supply : Analog power supply regulator on AVDD28 pin
> > > > > > > > >
> > > > > > > > > I think we normally use lower case in DT bindings and rarely encode
> > > > > > > > > voltage in the supply name unless we are dealing with several supplies
> > > > > > > > > of the same kind, but I'll let Ron comment on this.
> > > > > > > >
> > > > > > > > Yes on lowercase though there are some exceptions.
> > > > > > > >
> > > > > > > > There's also a AVDD22 supply as well as DVDD12 and VDDIO. So we
> > > > > > > > probably need to keep the voltage, but the binding is incomplete.
> > > > > > >
> > > > > > > What is incomplete here? can you please elaborate.
> > > > > >
> > > > > > You are missing the 3 other supplies the chip has: AVDD22, DVDD12 and VDDIO.
> > > > >
> > > > > Though it has other supplies, only AVDD28 is connected in the Host
> > > > > interface design similar like 9. Reference Schematic on Page, 23 in
> > > > > attached manual.
> > > >
> > > > That is for a particular board design. It still has other supplies.
> > > > Just make the binding complete please. You can make them optional. I
> > > > don't care if the driver supports controlling all the supplies or not
> > > > (though Dmitry might).
> > >
> > > So, Can I make bulk get and enable in all 4 regulators global to
> > > driver or specific to that chip, in either way the regulators which
> > > are not used to process via dummy regulators (which we all know).
> > >
> > > or regulator which are not using are get via devm_regulator_get_optional.
> > >
> > > Any suggestions?
> >
> > Just realized to go with bulk calls, please have a look at on v5 [1]
>
> I do not believe you can use bulk regulator API here as you need to
> implement the power up timing scheme mentioned on p. 12 of the spec you
> referenced earlier. And, because of that timing diagram, I believe if
> you want to control AVDD supply, you also have to control VDDIO supply
> as it has to be on only after enabling AVDD.
AVDD here is AVDD28 it' is an out name of pin (mentioned in p. 23). So
the power scheme goes to enable AVDD28 and then VDDIO does that really
mean bulk of two regulators?
>
> I also wonder if it is valid to have both AVDD28 and AVDD22 connected at
> the same time.
Yes, DVDD12 and AVDD22 not been part of power-up schema at-least from
the datasheet. I'm thinking we can skip these until any of other
goodix CTP needed, what do you think Rob?
^ permalink raw reply
* Re: [PATCH 2/2] Input: add Apple SPI keyboard and trackpad driver.
From: Life is hard, and then you die @ 2019-02-18 9:08 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Dmitry Torokhov, Henrik Rydberg, Lukas Wunner, Federico Lorenzi,
linux-input, linux-kernel
In-Reply-To: <20190216004426.GE9224@smile.fi.intel.com>
On Sat, Feb 16, 2019 at 02:44:26AM +0200, Andy Shevchenko wrote:
> On Sun, Feb 10, 2019 at 03:18:34AM -0800, Life is hard, and then you die wrote:
> > On Wed, Feb 06, 2019 at 10:22:56PM +0200, Andy Shevchenko wrote:
> > > On Mon, Feb 04, 2019 at 12:19:47AM -0800, Ronald Tschalär wrote:
>
> > > > +#define debug_print(mask, fmt, ...) \
> > > > + do { \
> > > > + if (debug & mask) \
> > > > + printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__); \
> > > > + } while (0)
> > > > +
> > > > +#define debug_print_header(mask) \
> > > > + debug_print(mask, "--- %s ---------------------------\n", \
> > > > + applespi_debug_facility(mask))
> > > > +
> > > > +#define debug_print_buffer(mask, fmt, ...) \
> > > > + do { \
> > > > + if (debug & mask) \
> > > > + print_hex_dump(KERN_DEBUG, pr_fmt(fmt), \
> > > > + DUMP_PREFIX_NONE, 32, 1, ##__VA_ARGS__, \
> > > > + false); \
> > > > + } while (0)
> > >
> > > I'm not sure we need all of these... But okay, the driver is
> > > reverse-engineered, perhaps we can drop it later on.
> >
> > These have been extremely useful for debugging; but yes, they are for
> > debugging only. They also explicitly do not use the dynamic-debug
> > facilities because
> > 1. those can't be enabled at a function or line level on the kernel
> > command line (the docs indicate this should work, but it doesn't,
> > or at the very least I've been unable to figure out how, at least
> > for those drivers built as modules)
>
> Hmm... Usually you put either module_name.dyndbg into kernel command line to
> enable Dynamic Debug on built-in modules, or modprobe module_name dyndbg if
> it's built as a module.
Right, but while that works with
applespi.dyndbg=+p
it does not appear to work with
applespi.dyndbg="func applespi_get_spi_settings +p"
(it is parsed correctly, but it just doesn't get applied when the
module is later loaded - I've not tried to chase this down any further
than that)
[snip]
> > > > +static int touchpad_dimensions[4];
> > > > +module_param_array(touchpad_dimensions, int, NULL, 0444);
> > > > +MODULE_PARM_DESC(touchpad_dimensions, "The pixel dimensions of the touchpad, as x_min,x_max,y_min,y_max .");
> > >
> > > We have some parsers for similar data as in format
> > >
> > > %dx%d%+d%+d ?
> > >
> > > For example, 200x100+20+10.
> >
> > Maybe it's just my grep-foo that is lacking, but I couldn't find
> > anything useful. There is some stuff for framebuffer video modes, but
> > that is too different and not really amenable for use here. OTOH, a
> > simple sscanf(buf, "%dx%d+%d+%d", ...) would parse this just fine
> > without the need for any fancier parser.
> >
> > OTOH, I'm not sure this format is any better: internally we need
> > x/y min/max, not x/y/w/h (though obviously the two are trivially
> > convertable); and for the user it doesn't really matter since they would
> > basically be getting the dimensions from running with debug set to
> > 0x10000 and using that output to set this param, i.e. I don't see any
> > inherent advantage of using x/y/w/h.
>
> I would stick with some more or less standard notation. The XxY+W+H is widely
> used in X11 world.
>
> If you have better examples for input devices, choose them. My point that it
> would be better to be a standard to some extent.
I couldn't find anything useful (looked in the kernel and libinput),
so going with XxY+W+H.
[snip]
> > > > + message->counter = applespi->cmd_msg_cntr++ & 0xff;
> > >
> > > Usual pattern for this kind of entries is
> > >
> > > x = ... % 256;
> > >
> > > Btw, isn't 256 is defined somewhere?
> >
> > Many things are defined as have a value of 256 :-) But I didn't find any
> > with the intended semantics; could use (U8_MAX+1), though.
>
> What I meant is that I saw in the same driver definition with value of 256.
> Isn't it about messages?
Ah, right: the packet length is 256 bytes. But the semantics are quite
different, so IMHO it wouldn't be good to use that here. I.e. I think
(U8_MAX+1) is still semantically the best.
[snip]
> > > > +/* lifted from the BCM5974 driver */
> > > > +/* convert 16-bit little endian to signed integer */
> > > > +static inline int raw2int(__le16 x)
> > > > +{
> > > > + return (signed short)le16_to_cpu(x);
> > > > +}
> > >
> > > Perhaps it's time to introduce it inside include/linux/input.h ?
> >
> > Perhaps as
> >
> > static inline int le16_to_signed_int(__le16 x)
> >
> > ? (raw2int seems to ambiguous for a global function)
>
> I'm fine. It would need a description though.
After looking at this in more detail I don't think that
include/linux/input.h is the proper place for this, because input.h
basically represents the interface to the input core, and as such it
is devoid of helpers like above or any driver specific definitions.
Instead I'd like to suggest a new include file specific to the bcm5974
driver, as follows:
--- /dev/null
+++ b/include/linux/input/bcm5974.h
@@ -0,0 +1,27 @@
+/*
+ * Copyright (C) 2008 Henrik Rydberg (rydberg@euromail.se)
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef _BCM5974_H
+#define _BCM5974_H
+
+#include <linux/kernel.h>
+
+/**
+ * raw2int - convert a 16-bit little endian value as received from the device
+ * to a signed integer in cpu endianness.
+ * @x: the received value
+ *
+ * Returns the converted value.
+ */
+static inline int raw2int(__le16 x)
+{
+ return (signed short)le16_to_cpu(x);
+}
+
+#endif
Thoughts?
I'll send out v2 of the patches with all the changes soon.
Cheers,
Ronald
^ permalink raw reply
* Re: [PATCH 2/7] [media] doc-rst: switch to new names for Full Screen/Aspect keys
From: Dmitry Torokhov @ 2019-02-18 7:26 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel,
linux-media
In-Reply-To: <20190118233037.87318-2-dmitry.torokhov@gmail.com>
On Fri, Jan 18, 2019 at 03:30:32PM -0800, Dmitry Torokhov wrote:
> We defined better names for keys to activate full screen mode or
> change aspect ratio (while keeping the existing keycodes to avoid
> breaking userspace), so let's use them in the document.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> Documentation/media/uapi/rc/rc-tables.rst | 4 ++--
Mauro, do you want to take this through your tree or I should pick it up
with the patch that does renames in uapi header?
Thanks!
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/media/uapi/rc/rc-tables.rst b/Documentation/media/uapi/rc/rc-tables.rst
> index c8ae9479f842..57797e56f45e 100644
> --- a/Documentation/media/uapi/rc/rc-tables.rst
> +++ b/Documentation/media/uapi/rc/rc-tables.rst
> @@ -616,7 +616,7 @@ the remote via /dev/input/event devices.
>
> - .. row 78
>
> - - ``KEY_SCREEN``
> + - ``KEY_ASPECT_RATIO``
>
> - Select screen aspect ratio
>
> @@ -624,7 +624,7 @@ the remote via /dev/input/event devices.
>
> - .. row 79
>
> - - ``KEY_ZOOM``
> + - ``KEY_FULL_SCREEN``
>
> - Put device into zoom/full screen mode
>
> --
> 2.20.1.321.g9e740568ce-goog
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: i8042 - Rework DT node name comparisons
From: Dmitry Torokhov @ 2019-02-18 7:23 UTC (permalink / raw)
To: Rob Herring; +Cc: linux-kernel, devicetree, linux-input
In-Reply-To: <20190213161631.17028-1-robh@kernel.org>
On Wed, Feb 13, 2019 at 10:16:31AM -0600, Rob Herring wrote:
> Convert string compares of DT node names to use of_node_name_eq helper
> instead. For the root node on SUN DT, we need to retrieve the 'name'
> property as it is the rare case where the 'name' property and node name
> differ. With both changes, it removes direct access to the node name
> pointer.
>
> While at it, comvert the open coded loop to use
> for_each_child_of_node().
>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: linux-input@vger.kernel.org
> Signed-off-by: Rob Herring <robh@kernel.org>
Applied, thank you.
> ---
> drivers/input/serio/i8042-sparcio.h | 21 ++++++++++-----------
> 1 file changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/input/serio/i8042-sparcio.h b/drivers/input/serio/i8042-sparcio.h
> index 796289846204..fce76812843b 100644
> --- a/drivers/input/serio/i8042-sparcio.h
> +++ b/drivers/input/serio/i8042-sparcio.h
> @@ -53,12 +53,11 @@ static struct resource *kbd_res;
>
> static int sparc_i8042_probe(struct platform_device *op)
> {
> - struct device_node *dp = op->dev.of_node;
> + struct device_node *dp;
>
> - dp = dp->child;
> - while (dp) {
> - if (!strcmp(dp->name, OBP_PS2KBD_NAME1) ||
> - !strcmp(dp->name, OBP_PS2KBD_NAME2)) {
> + for_each_child_of_node(op->dev.of_node, dp) {
> + if (of_node_name_eq(dp, OBP_PS2KBD_NAME1) ||
> + of_node_name_eq(dp, OBP_PS2KBD_NAME2)) {
> struct platform_device *kbd = of_find_device_by_node(dp);
> unsigned int irq = kbd->archdata.irqs[0];
> if (irq == 0xffffffff)
> @@ -67,16 +66,14 @@ static int sparc_i8042_probe(struct platform_device *op)
> kbd_iobase = of_ioremap(&kbd->resource[0],
> 0, 8, "kbd");
> kbd_res = &kbd->resource[0];
> - } else if (!strcmp(dp->name, OBP_PS2MS_NAME1) ||
> - !strcmp(dp->name, OBP_PS2MS_NAME2)) {
> + } else if (of_node_name_eq(dp, OBP_PS2MS_NAME1) ||
> + of_node_name_eq(dp, OBP_PS2MS_NAME2)) {
> struct platform_device *ms = of_find_device_by_node(dp);
> unsigned int irq = ms->archdata.irqs[0];
> if (irq == 0xffffffff)
> irq = op->archdata.irqs[0];
> i8042_aux_irq = irq;
> }
> -
> - dp = dp->sibling;
> }
>
> return 0;
> @@ -109,8 +106,9 @@ static struct platform_driver sparc_i8042_driver = {
> static int __init i8042_platform_init(void)
> {
> struct device_node *root = of_find_node_by_path("/");
> + const char *name = of_get_property(root, "name", NULL);
>
> - if (!strcmp(root->name, "SUNW,JavaStation-1")) {
> + if (name && !strcmp(name, "SUNW,JavaStation-1")) {
> /* Hardcoded values for MrCoffee. */
> i8042_kbd_irq = i8042_aux_irq = 13 | 0x20;
> kbd_iobase = ioremap(0x71300060, 8);
> @@ -139,8 +137,9 @@ static int __init i8042_platform_init(void)
> static inline void i8042_platform_exit(void)
> {
> struct device_node *root = of_find_node_by_path("/");
> + const char *name = of_get_property(root, "name", NULL);
>
> - if (strcmp(root->name, "SUNW,JavaStation-1"))
> + if (!name || strcmp(name, "SUNW,JavaStation-1"))
> platform_driver_unregister(&sparc_i8042_driver);
> }
>
> --
> 2.19.1
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH v3 1/4] dt-bindings: input: touchscreen: goodix: Document AVDD28-supply property
From: Dmitry Torokhov @ 2019-02-18 7:12 UTC (permalink / raw)
To: Jagan Teki
Cc: Rob Herring, devicetree, Chen-Yu Tsai, linux-input,
linux-kernel@vger.kernel.org, Michael Trimarchi, linux-amarula
In-Reply-To: <CAMty3ZDBu2Y9psNu9nrZ=BNVvhpJ2qYn-Qp7MDqdhK9_SrcpNg@mail.gmail.com>
On Sun, Feb 17, 2019 at 02:45:52PM +0530, Jagan Teki wrote:
> On Sun, Feb 17, 2019 at 1:21 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
> >
> > Hi Dmitry and Rob,
> >
> > On Thu, Feb 14, 2019 at 3:21 AM Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Tue, Jan 22, 2019 at 7:39 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > > >
> > > > On Mon, Jan 21, 2019 at 9:46 PM Rob Herring <robh@kernel.org> wrote:
> > > > >
> > > > > On Fri, Jan 18, 2019 at 10:01 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > > > > >
> > > > > > On Wed, Jan 9, 2019 at 7:08 PM Rob Herring <robh@kernel.org> wrote:
> > > > > > >
> > > > > > > Please CC DT list if you want bindings reviewed.
> > > > > >
> > > > > > Sorry I forgot.
> > > > > >
> > > > > > >
> > > > > > > On Wed, Jan 9, 2019 at 1:40 AM Dmitry Torokhov
> > > > > > > <dmitry.torokhov@gmail.com> wrote:
> > > > > > > >
> > > > > > > > On Sat, Dec 15, 2018 at 08:47:59PM +0530, Jagan Teki wrote:
> > > > > > > > > Most of the Goodix CTP controllers are supply with AVDD28 pin.
> > > > > > > > > which need to supply for controllers like GT5663 on some boards
> > > > > > > > > to trigger the power.
> > > > > > > > >
> > > > > > > > > So, document the supply property so-that the require boards
> > > > > > > > > that used on GT5663 can enable it via device tree.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > > > > > > ---
> > > > > > > > > Documentation/devicetree/bindings/input/touchscreen/goodix.txt | 1 +
> > > > > > > > > 1 file changed, 1 insertion(+)
> > > > > > > > >
> > > > > > > > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> > > > > > > > > index f7e95c52f3c7..c4622c983e08 100644
> > > > > > > > > --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> > > > > > > > > +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> > > > > > > > > @@ -23,6 +23,7 @@ Optional properties:
> > > > > > > > > - touchscreen-inverted-y : Y axis is inverted (boolean)
> > > > > > > > > - touchscreen-swapped-x-y : X and Y axis are swapped (boolean)
> > > > > > > > > (swapping is done after inverting the axis)
> > > > > > > > > + - AVDD28-supply : Analog power supply regulator on AVDD28 pin
> > > > > > > >
> > > > > > > > I think we normally use lower case in DT bindings and rarely encode
> > > > > > > > voltage in the supply name unless we are dealing with several supplies
> > > > > > > > of the same kind, but I'll let Ron comment on this.
> > > > > > >
> > > > > > > Yes on lowercase though there are some exceptions.
> > > > > > >
> > > > > > > There's also a AVDD22 supply as well as DVDD12 and VDDIO. So we
> > > > > > > probably need to keep the voltage, but the binding is incomplete.
> > > > > >
> > > > > > What is incomplete here? can you please elaborate.
> > > > >
> > > > > You are missing the 3 other supplies the chip has: AVDD22, DVDD12 and VDDIO.
> > > >
> > > > Though it has other supplies, only AVDD28 is connected in the Host
> > > > interface design similar like 9. Reference Schematic on Page, 23 in
> > > > attached manual.
> > >
> > > That is for a particular board design. It still has other supplies.
> > > Just make the binding complete please. You can make them optional. I
> > > don't care if the driver supports controlling all the supplies or not
> > > (though Dmitry might).
> >
> > So, Can I make bulk get and enable in all 4 regulators global to
> > driver or specific to that chip, in either way the regulators which
> > are not used to process via dummy regulators (which we all know).
> >
> > or regulator which are not using are get via devm_regulator_get_optional.
> >
> > Any suggestions?
>
> Just realized to go with bulk calls, please have a look at on v5 [1]
I do not believe you can use bulk regulator API here as you need to
implement the power up timing scheme mentioned on p. 12 of the spec you
referenced earlier. And, because of that timing diagram, I believe if
you want to control AVDD supply, you also have to control VDDIO supply
as it has to be on only after enabling AVDD.
I also wonder if it is valid to have both AVDD28 and AVDD22 connected at
the same time.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v4 2/4] Input: goodix - Add AVDD28-supply regulator support
From: Dmitry Torokhov @ 2019-02-18 7:08 UTC (permalink / raw)
To: Jagan Teki
Cc: Bastien Nocera, Rob Herring, Henrik Rydberg, linux-input,
linux-kernel, devicetree, Mark Rutland
In-Reply-To: <20190212112439.2025-3-jagan@amarulasolutions.com>
Hi Jagan,
On Tue, Feb 12, 2019 at 04:54:37PM +0530, Jagan Teki wrote:
> Goodix CTP controllers have AVDD28 pin connected to voltage
> regulator which may not be turned on by default, like for GT5663.
>
> Add support for such ctp used boards by adding voltage regulator
> handling code to goodix ctp driver.
>
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
> drivers/input/touchscreen/goodix.c | 44 ++++++++++++++++++++++++++----
> 1 file changed, 39 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> index f2d9c2c41885..e92b90be1ac2 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -27,6 +27,7 @@
> #include <linux/delay.h>
> #include <linux/irq.h>
> #include <linux/interrupt.h>
> +#include <linux/regulator/consumer.h>
> #include <linux/slab.h>
> #include <linux/acpi.h>
> #include <linux/of.h>
> @@ -47,6 +48,7 @@ struct goodix_ts_data {
> struct touchscreen_properties prop;
> unsigned int max_touch_num;
> unsigned int int_trigger_type;
> + struct regulator *avdd28;
> struct gpio_desc *gpiod_int;
> struct gpio_desc *gpiod_rst;
> u16 id;
> @@ -761,6 +763,13 @@ static void goodix_config_cb(const struct firmware *cfg, void *ctx)
> complete_all(&ts->firmware_loading_complete);
> }
>
> +static void goodix_disable_regulator(void *arg)
> +{
> + struct goodix_ts_data *ts = arg;
> +
> + regulator_disable(ts->avdd28);
> +}
> +
> static int goodix_ts_probe(struct i2c_client *client,
> const struct i2c_device_id *id)
> {
> @@ -786,25 +795,46 @@ static int goodix_ts_probe(struct i2c_client *client,
> if (error)
> return error;
>
> + ts->avdd28 = devm_regulator_get(&client->dev, "AVDD28");
> + if (IS_ERR(ts->avdd28)) {
> + error = PTR_ERR(ts->avdd28);
> + if (error != -EPROBE_DEFER)
> + dev_err(&client->dev,
> + "Failed to get AVDD28 regulator: %d\n", error);
> + return error;
> + }
> +
> + /* power the controller */
> + error = regulator_enable(ts->avdd28);
> + if (error) {
> + dev_err(&client->dev, "Controller fail to enable AVDD28\n");
> + return error;
> + }
> +
> + error = devm_add_action_or_reset(&client->dev,
> + goodix_disable_regulator, ts);
> + if (error)
> + return error;
> +
> if (ts->gpiod_int && ts->gpiod_rst) {
> /* reset the controller */
> error = goodix_reset(ts);
> if (error) {
> dev_err(&client->dev, "Controller reset failed.\n");
> - return error;
> + goto error;
Why are you changing this? You added goodix_disable_regulator() as
custom devm action, so it will fire if probe() returns with error.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH (resend)] Input: uinput - Set name/phys to NULL before kfree().
From: Dmitry Torokhov @ 2019-02-17 21:07 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: rydberg, syzbot, linux-input, linux-kernel, syzkaller-bugs
In-Reply-To: <b937b59c-22b8-9113-229f-056738819467@I-love.SAKURA.ne.jp>
Hi Tetsuo,
On Fri, Feb 08, 2019 at 07:25:52PM +0900, Tetsuo Handa wrote:
> syzbot is hitting use-after-free bug in uinput module [1]. This is because
> uinput_destroy_device() sometimes kfree()s dev->name and dev->phys at
> uinput_destroy_device() before dev_uevent() is triggered by dropping the
> refcount to 0. Since the timing of triggering last input_put_device() is
> uncontrollable, this patch prepares for such race by setting dev->name and
> dev->phys to NULL before doing operations which might drop the refcount
> to 0.
>
> [1] https://syzkaller.appspot.com/bug?id=8b17c134fe938bbddd75a45afaa9e68af43a362d
Sorry it took me so long to sort out the issue and unfortunately I
disagree with your analysis. The issue here is not that we do not know
when last reference is being dropped (because we expect that KOBJ_REMOVE
uevent will be sent out when we call input_unregister_device, which is
quite deterministic) but the kobject cleanup logic added in commit
0f4dafc0563c6c49e17fe14b3f5f356e4c4b8806 ("Kobject: auto-cleanup on
final unref") coupled with the fault injected by the syzcaller.
The commit tries to send final uevent for objects for which "add" uevent
has been sent, but not "remove" event. However in uinput (and general
input case) we always take care of sending uevent at unregister, and do
not expect to have uevent sent out at the final "put" time.
I believe the real fix is to have kobj->state_remove_uevent_sent be set
to true as soon as we enter kobject_uevent(kobj, KOBJ_REMOVE) so that
it is being set even if memory allocation fails. Doing anything else may
violate expectations of subsystem owning the kobject.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [git pull] Input updates for v5.0-rc6
From: pr-tracker-bot @ 2019-02-17 16:50 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Linus Torvalds, linux-kernel, linux-input
In-Reply-To: <20190217074123.GA248187@dtor-ws>
The pull request you sent on Sat, 16 Feb 2019 23:41:23 -0800:
> git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git for-linus
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/b8c82b6a3a8bee7858df08d25f5ddcfbe6210a69
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker
^ permalink raw reply
* Re: [PATCH v3 1/4] dt-bindings: input: touchscreen: goodix: Document AVDD28-supply property
From: Jagan Teki @ 2019-02-17 9:15 UTC (permalink / raw)
To: Rob Herring, Dmitry Torokhov
Cc: devicetree, Chen-Yu Tsai, linux-input,
linux-kernel@vger.kernel.org, Michael Trimarchi, linux-amarula
In-Reply-To: <CAMty3ZDMB=pwY8Rxdt3du=90pksBedLo0Pc_-=diqx9-Xndzqg@mail.gmail.com>
On Sun, Feb 17, 2019 at 1:21 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> Hi Dmitry and Rob,
>
> On Thu, Feb 14, 2019 at 3:21 AM Rob Herring <robh@kernel.org> wrote:
> >
> > On Tue, Jan 22, 2019 at 7:39 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > >
> > > On Mon, Jan 21, 2019 at 9:46 PM Rob Herring <robh@kernel.org> wrote:
> > > >
> > > > On Fri, Jan 18, 2019 at 10:01 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > > > >
> > > > > On Wed, Jan 9, 2019 at 7:08 PM Rob Herring <robh@kernel.org> wrote:
> > > > > >
> > > > > > Please CC DT list if you want bindings reviewed.
> > > > >
> > > > > Sorry I forgot.
> > > > >
> > > > > >
> > > > > > On Wed, Jan 9, 2019 at 1:40 AM Dmitry Torokhov
> > > > > > <dmitry.torokhov@gmail.com> wrote:
> > > > > > >
> > > > > > > On Sat, Dec 15, 2018 at 08:47:59PM +0530, Jagan Teki wrote:
> > > > > > > > Most of the Goodix CTP controllers are supply with AVDD28 pin.
> > > > > > > > which need to supply for controllers like GT5663 on some boards
> > > > > > > > to trigger the power.
> > > > > > > >
> > > > > > > > So, document the supply property so-that the require boards
> > > > > > > > that used on GT5663 can enable it via device tree.
> > > > > > > >
> > > > > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > > > > > ---
> > > > > > > > Documentation/devicetree/bindings/input/touchscreen/goodix.txt | 1 +
> > > > > > > > 1 file changed, 1 insertion(+)
> > > > > > > >
> > > > > > > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> > > > > > > > index f7e95c52f3c7..c4622c983e08 100644
> > > > > > > > --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> > > > > > > > +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> > > > > > > > @@ -23,6 +23,7 @@ Optional properties:
> > > > > > > > - touchscreen-inverted-y : Y axis is inverted (boolean)
> > > > > > > > - touchscreen-swapped-x-y : X and Y axis are swapped (boolean)
> > > > > > > > (swapping is done after inverting the axis)
> > > > > > > > + - AVDD28-supply : Analog power supply regulator on AVDD28 pin
> > > > > > >
> > > > > > > I think we normally use lower case in DT bindings and rarely encode
> > > > > > > voltage in the supply name unless we are dealing with several supplies
> > > > > > > of the same kind, but I'll let Ron comment on this.
> > > > > >
> > > > > > Yes on lowercase though there are some exceptions.
> > > > > >
> > > > > > There's also a AVDD22 supply as well as DVDD12 and VDDIO. So we
> > > > > > probably need to keep the voltage, but the binding is incomplete.
> > > > >
> > > > > What is incomplete here? can you please elaborate.
> > > >
> > > > You are missing the 3 other supplies the chip has: AVDD22, DVDD12 and VDDIO.
> > >
> > > Though it has other supplies, only AVDD28 is connected in the Host
> > > interface design similar like 9. Reference Schematic on Page, 23 in
> > > attached manual.
> >
> > That is for a particular board design. It still has other supplies.
> > Just make the binding complete please. You can make them optional. I
> > don't care if the driver supports controlling all the supplies or not
> > (though Dmitry might).
>
> So, Can I make bulk get and enable in all 4 regulators global to
> driver or specific to that chip, in either way the regulators which
> are not used to process via dummy regulators (which we all know).
>
> or regulator which are not using are get via devm_regulator_get_optional.
>
> Any suggestions?
Just realized to go with bulk calls, please have a look at on v5 [1]
[1] https://patchwork.kernel.org/cover/10816901/
^ permalink raw reply
* [PATCH v5 4/4] Input: goodix - Add GT5663 CTP support
From: Jagan Teki @ 2019-02-17 9:14 UTC (permalink / raw)
To: Dmitry Torokhov, Bastien Nocera, Rob Herring
Cc: Henrik Rydberg, linux-input, linux-kernel, devicetree,
Mark Rutland, linux-amarula, Michael Trimarchi, Jagan Teki
In-Reply-To: <20190217091436.3702-1-jagan@amarulasolutions.com>
GT5663 is capacitive touch controller with customized smart
wakeup gestures.
Add support for it by adding compatible and supported chip data.
The chip data on GT5663 is similar to GT1151, like
- config data register has 0x8050 address
- config data register max len is 240
- config data checksum has 16-bit
Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
drivers/input/touchscreen/goodix.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 294456a53fe0..80f8b920ef5e 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -227,6 +227,7 @@ static const struct goodix_chip_data *goodix_get_chip_data(u16 id)
{
switch (id) {
case 1151:
+ case 5663:
return >1x_chip_data;
case 911:
@@ -988,6 +989,7 @@ MODULE_DEVICE_TABLE(acpi, goodix_acpi_match);
#ifdef CONFIG_OF
static const struct of_device_id goodix_of_match[] = {
{ .compatible = "goodix,gt1151" },
+ { .compatible = "goodix,gt5663" },
{ .compatible = "goodix,gt911" },
{ .compatible = "goodix,gt9110" },
{ .compatible = "goodix,gt912" },
--
2.18.0.321.gffc6fa0e3
^ permalink raw reply related
* [PATCH v5 3/4] dt-bindings: input: touchscreen: goodix: Add GT5663 compatible
From: Jagan Teki @ 2019-02-17 9:14 UTC (permalink / raw)
To: Dmitry Torokhov, Bastien Nocera, Rob Herring
Cc: Henrik Rydberg, linux-input, linux-kernel, devicetree,
Mark Rutland, linux-amarula, Michael Trimarchi, Jagan Teki
In-Reply-To: <20190217091436.3702-1-jagan@amarulasolutions.com>
GT5663 is capacitive touch controller with customized smart
wakeup gestures, it support chipdata which is similar to
existing GT1151 and require AVDD28 supply for some boards.
Document the compatible for the same.
Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
Documentation/devicetree/bindings/input/touchscreen/goodix.txt | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
index 8f6e6eede64d..29c149d91a05 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
@@ -3,6 +3,7 @@ Device tree bindings for Goodix GT9xx series touchscreen controller
Required properties:
- compatible : Should be "goodix,gt1151"
+ or "goodix,gt5663"
or "goodix,gt911"
or "goodix,gt9110"
or "goodix,gt912"
--
2.18.0.321.gffc6fa0e3
^ permalink raw reply related
* [PATCH v5 2/4] Input: goodix - Add regulators support
From: Jagan Teki @ 2019-02-17 9:14 UTC (permalink / raw)
To: Dmitry Torokhov, Bastien Nocera, Rob Herring
Cc: Henrik Rydberg, linux-input, linux-kernel, devicetree,
Mark Rutland, linux-amarula, Michael Trimarchi, Jagan Teki
In-Reply-To: <20190217091436.3702-1-jagan@amarulasolutions.com>
Goodix CTP controllers support analog, digital and gpio regulator
supplies on relevant controller pin configurations.
Support them via regulator bulk calls.
Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
drivers/input/touchscreen/goodix.c | 58 ++++++++++++++++++++++++++----
1 file changed, 52 insertions(+), 6 deletions(-)
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index f2d9c2c41885..294456a53fe0 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -27,6 +27,7 @@
#include <linux/delay.h>
#include <linux/irq.h>
#include <linux/interrupt.h>
+#include <linux/regulator/consumer.h>
#include <linux/slab.h>
#include <linux/acpi.h>
#include <linux/of.h>
@@ -40,6 +41,15 @@ struct goodix_chip_data {
int (*check_config)(struct goodix_ts_data *, const struct firmware *);
};
+static const char * const goodix_supply_names[] = {
+ "AVDD28",
+ "AVDD22",
+ "DVDD12",
+ "VDDIO",
+};
+
+#define GOODIX_NUM_SUPPLIES ARRAY_SIZE(goodix_supply_names)
+
struct goodix_ts_data {
struct i2c_client *client;
struct input_dev *input_dev;
@@ -47,6 +57,7 @@ struct goodix_ts_data {
struct touchscreen_properties prop;
unsigned int max_touch_num;
unsigned int int_trigger_type;
+ struct regulator_bulk_data supplies[GOODIX_NUM_SUPPLIES];
struct gpio_desc *gpiod_int;
struct gpio_desc *gpiod_rst;
u16 id;
@@ -761,11 +772,18 @@ static void goodix_config_cb(const struct firmware *cfg, void *ctx)
complete_all(&ts->firmware_loading_complete);
}
+static void goodix_disable_regulator(void *arg)
+{
+ struct goodix_ts_data *ts = arg;
+
+ regulator_bulk_disable(GOODIX_NUM_SUPPLIES, ts->supplies);
+}
+
static int goodix_ts_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
struct goodix_ts_data *ts;
- int error;
+ int error, i;
dev_dbg(&client->dev, "I2C Address: 0x%02x\n", client->addr);
@@ -786,25 +804,49 @@ static int goodix_ts_probe(struct i2c_client *client,
if (error)
return error;
+ for (i = 0; i < GOODIX_NUM_SUPPLIES; i++)
+ ts->supplies[i].supply = goodix_supply_names[i];
+
+ error = devm_regulator_bulk_get(&client->dev, GOODIX_NUM_SUPPLIES,
+ ts->supplies);
+ if (error) {
+ dev_err(&client->dev, "Failed to get regulators (ret=%d)\n",
+ error);
+ return error;
+ }
+
+ error = devm_add_action_or_reset(&client->dev,
+ goodix_disable_regulator, ts);
+ if (error)
+ return error;
+
+ /* power the controller */
+ error = regulator_bulk_enable(GOODIX_NUM_SUPPLIES, ts->supplies);
+ if (error) {
+ dev_err(&client->dev, "Failed to enable regulators (ret=%d)\n",
+ error);
+ return error;
+ }
+
if (ts->gpiod_int && ts->gpiod_rst) {
/* reset the controller */
error = goodix_reset(ts);
if (error) {
dev_err(&client->dev, "Controller reset failed.\n");
- return error;
+ goto error;
}
}
error = goodix_i2c_test(client);
if (error) {
dev_err(&client->dev, "I2C communication failure: %d\n", error);
- return error;
+ goto error;
}
error = goodix_read_version(ts);
if (error) {
dev_err(&client->dev, "Read version failed.\n");
- return error;
+ goto error;
}
ts->chip = goodix_get_chip_data(ts->id);
@@ -823,17 +865,21 @@ static int goodix_ts_probe(struct i2c_client *client,
dev_err(&client->dev,
"Failed to invoke firmware loader: %d\n",
error);
- return error;
+ goto error;
}
return 0;
} else {
error = goodix_configure_dev(ts);
if (error)
- return error;
+ goto error;
}
return 0;
+
+error:
+ regulator_bulk_disable(GOODIX_NUM_SUPPLIES, ts->supplies);
+ return error;
}
static int goodix_ts_remove(struct i2c_client *client)
--
2.18.0.321.gffc6fa0e3
^ permalink raw reply related
* [PATCH v5 1/4] dt-bindings: input: touchscreen: goodix: Document regulator properties
From: Jagan Teki @ 2019-02-17 9:14 UTC (permalink / raw)
To: Dmitry Torokhov, Bastien Nocera, Rob Herring
Cc: Henrik Rydberg, linux-input, linux-kernel, devicetree,
Mark Rutland, linux-amarula, Michael Trimarchi, Jagan Teki
In-Reply-To: <20190217091436.3702-1-jagan@amarulasolutions.com>
Goodix CTP controllers support analog, digital and gpio regulator
supplies on relevant controller pin configurations.
Out of which AVDD28 regulator is mandatory supplied regulator in most
of the board designs, so document AVDD28 as required property and
rest marked as optional regulators.
Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
.../devicetree/bindings/input/touchscreen/goodix.txt | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
index f7e95c52f3c7..8f6e6eede64d 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
@@ -23,6 +23,13 @@ Optional properties:
- touchscreen-inverted-y : Y axis is inverted (boolean)
- touchscreen-swapped-x-y : X and Y axis are swapped (boolean)
(swapping is done after inverting the axis)
+ - AVDD28-supply : Analog power supply regulator on AVDD28 pin
+
+Optional properties:
+
+ - AVDD22-supply : Analog power supply regulator on AVDD22 pin
+ - DVDD12-supply : Digital power supply regulator on DVDD12 pin
+ - VDDIO-supply : GPIO power supply regulator on VDDIO pin
Example:
--
2.18.0.321.gffc6fa0e3
^ permalink raw reply related
* [PATCH v5 0/4] input: touchscreen: Add goodix GT5553 CTP support
From: Jagan Teki @ 2019-02-17 9:14 UTC (permalink / raw)
To: Dmitry Torokhov, Bastien Nocera, Rob Herring
Cc: Henrik Rydberg, linux-input, linux-kernel, devicetree,
Mark Rutland, linux-amarula, Michael Trimarchi, Jagan Teki
This is v5 patchset for supporting goodix GT5553 CTP. Here is the
previous version[1]
Changes for v4:
- document AVDD22, DVDD12, VDDIO as optional properties
- use regulator bulk calls, for get, enable and disable functionalities
Changes for v4:
- devm_add_action_or_reset for disabling regulator
Changes for v3:
- add cover-letter
- s/ADVV28/AVDD28 on commit head
- fix few typo
Changes for v2:
- Rename vcc-supply with AVDD28-supply
- disable regulator in remove
- fix to setup regulator in probe code
- add chipdata
- drop example node in dt-bindings
[1] https://patchwork.kernel.org/cover/10807779/
Jagan Teki (4):
dt-bindings: input: touchscreen: goodix: Document regulator properties
Input: goodix - Add regulators support
dt-bindings: input: touchscreen: goodix: Add GT5663 compatible
Input: goodix - Add GT5663 CTP support
.../bindings/input/touchscreen/goodix.txt | 8 +++
drivers/input/touchscreen/goodix.c | 60 +++++++++++++++++--
2 files changed, 62 insertions(+), 6 deletions(-)
--
2.18.0.321.gffc6fa0e3
^ permalink raw reply
* Re: [PATCH v3 1/4] dt-bindings: input: touchscreen: goodix: Document AVDD28-supply property
From: Jagan Teki @ 2019-02-17 7:51 UTC (permalink / raw)
To: Rob Herring, Dmitry Torokhov
Cc: devicetree, Chen-Yu Tsai, linux-input,
linux-kernel@vger.kernel.org, Michael Trimarchi, linux-amarula
In-Reply-To: <CAL_JsqJSMX7E2zQA2O8ofZQPDrm3S6PMCLG1iAQ95P10Pe2yFg@mail.gmail.com>
Hi Dmitry and Rob,
On Thu, Feb 14, 2019 at 3:21 AM Rob Herring <robh@kernel.org> wrote:
>
> On Tue, Jan 22, 2019 at 7:39 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
> >
> > On Mon, Jan 21, 2019 at 9:46 PM Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Fri, Jan 18, 2019 at 10:01 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > > >
> > > > On Wed, Jan 9, 2019 at 7:08 PM Rob Herring <robh@kernel.org> wrote:
> > > > >
> > > > > Please CC DT list if you want bindings reviewed.
> > > >
> > > > Sorry I forgot.
> > > >
> > > > >
> > > > > On Wed, Jan 9, 2019 at 1:40 AM Dmitry Torokhov
> > > > > <dmitry.torokhov@gmail.com> wrote:
> > > > > >
> > > > > > On Sat, Dec 15, 2018 at 08:47:59PM +0530, Jagan Teki wrote:
> > > > > > > Most of the Goodix CTP controllers are supply with AVDD28 pin.
> > > > > > > which need to supply for controllers like GT5663 on some boards
> > > > > > > to trigger the power.
> > > > > > >
> > > > > > > So, document the supply property so-that the require boards
> > > > > > > that used on GT5663 can enable it via device tree.
> > > > > > >
> > > > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > > > > ---
> > > > > > > Documentation/devicetree/bindings/input/touchscreen/goodix.txt | 1 +
> > > > > > > 1 file changed, 1 insertion(+)
> > > > > > >
> > > > > > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> > > > > > > index f7e95c52f3c7..c4622c983e08 100644
> > > > > > > --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> > > > > > > +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> > > > > > > @@ -23,6 +23,7 @@ Optional properties:
> > > > > > > - touchscreen-inverted-y : Y axis is inverted (boolean)
> > > > > > > - touchscreen-swapped-x-y : X and Y axis are swapped (boolean)
> > > > > > > (swapping is done after inverting the axis)
> > > > > > > + - AVDD28-supply : Analog power supply regulator on AVDD28 pin
> > > > > >
> > > > > > I think we normally use lower case in DT bindings and rarely encode
> > > > > > voltage in the supply name unless we are dealing with several supplies
> > > > > > of the same kind, but I'll let Ron comment on this.
> > > > >
> > > > > Yes on lowercase though there are some exceptions.
> > > > >
> > > > > There's also a AVDD22 supply as well as DVDD12 and VDDIO. So we
> > > > > probably need to keep the voltage, but the binding is incomplete.
> > > >
> > > > What is incomplete here? can you please elaborate.
> > >
> > > You are missing the 3 other supplies the chip has: AVDD22, DVDD12 and VDDIO.
> >
> > Though it has other supplies, only AVDD28 is connected in the Host
> > interface design similar like 9. Reference Schematic on Page, 23 in
> > attached manual.
>
> That is for a particular board design. It still has other supplies.
> Just make the binding complete please. You can make them optional. I
> don't care if the driver supports controlling all the supplies or not
> (though Dmitry might).
So, Can I make bulk get and enable in all 4 regulators global to
driver or specific to that chip, in either way the regulators which
are not used to process via dummy regulators (which we all know).
or regulator which are not using are get via devm_regulator_get_optional.
Any suggestions?
^ permalink raw reply
* [git pull] Input updates for v5.0-rc6
From: Dmitry Torokhov @ 2019-02-17 7:41 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-kernel, linux-input
Hi Linus,
Please pull from:
git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git for-linus
to receive updates for the input subsystem:
- tweaks to Elan drivers (both PS/2 and I2C) to support new devices.
Also revert of one of IDs as that device should really be driven by
i2c-hid + hid-multitouch
- a few drivers have been switched to set_brightness_blocking() call
because they either were sleeping the their set_brightness()
implementation or used workqueue but were not canceling it on unbind.
- ps2-gpio and matrix_keypad needed to [properly] flush their works to
avoid potential use-after-free on unbind.
- other miscellaneous fixes.
Changelog:
---------
Dmitry Torokhov (6):
Input: cap11xx - switch to using set_brightness_blocking()
Input: ps2-gpio - flush TX work when closing port
Input: matrix_keypad - use flush_delayed_work()
Input: qt2160 - switch to using brightness_set_blocking()
Revert "Input: elan_i2c - add ACPI ID for touchpad in ASUS Aspire F5-573G"
Input: apanel - switch to using brightness_set_blocking()
Gabriel Fernandez (1):
Input: st-keyscan - fix potential zalloc NULL dereference
Jonathan Bakker (2):
Input: pwm-vibra - prevent unbalanced regulator
Input: bma150 - register input device after setting private data
Matti Kurkela (1):
Input: elantech - enable 3rd button support on Fujitsu CELSIUS H780
Mauro Ciancio (1):
Input: elan_i2c - add ACPI ID for touchpad in Lenovo V330-15ISK
Paweł Chmiel (1):
Input: pwm-vibra - stop regulator after disabling pwm, not before
Stefan Agner (1):
Input: snvs_pwrkey - allow selecting driver for i.MX 7D
Diffstat:
--------
drivers/input/keyboard/Kconfig | 2 +-
drivers/input/keyboard/cap11xx.c | 35 ++++++-----------
drivers/input/keyboard/matrix_keypad.c | 2 +-
drivers/input/keyboard/qt2160.c | 69 +++++++++++++---------------------
drivers/input/keyboard/st-keyscan.c | 4 +-
drivers/input/misc/apanel.c | 24 ++----------
drivers/input/misc/bma150.c | 9 +++--
drivers/input/misc/pwm-vibra.c | 19 +++++++---
drivers/input/mouse/elan_i2c_core.c | 2 +-
drivers/input/mouse/elantech.c | 9 +++++
drivers/input/serio/ps2-gpio.c | 1 +
11 files changed, 75 insertions(+), 101 deletions(-)
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v3 0/3] input: goodix - support Goodix gt5688
From: Dmitry Torokhov @ 2019-02-17 7:07 UTC (permalink / raw)
To: Guido Günther
Cc: Rob Herring, Mark Rutland, Bastien Nocera, Matthias Brugger,
linux-input, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek
In-Reply-To: <cover.1550239328.git.agx@sigxcpu.org>
On Fri, Feb 15, 2019 at 03:11:50PM +0100, Guido Günther wrote:
> Besides the support for gt5688 two more minimal changes piled up so I'm
> folding these in here. One is a doc update for the goodix bindings, the
> other one makes it a bit simpler to figure out configuration problems
> (like lack of touchscreen-size-{x,y} in the device tree.
>
> I've kept the original subject to (hopefully) not break the series for
> patchwork. All patches can be applied independently. Series is based on
> next-20190208.
>
> Changes from v2
> * Add 'dt-bindings: input: Refer to touchscreen.txt for goodix ts'
> * Add 'input: goodix: Print values in case of inconsistencies'
> (sent out separtely before, no comments so far)
> * Collect 'Reviewed-By' on 'input: goodix - support Goodix gt5688'
Applied the lot, thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2] Input: synaptics_i2c - remove redundant spinlock
From: Dmitry Torokhov @ 2019-02-17 6:54 UTC (permalink / raw)
To: thesven73; +Cc: linux-input, linux-kernel, Tejun Heo
In-Reply-To: <20190217061053.4559-1-TheSven73@gmail.com>
On Sun, Feb 17, 2019 at 01:10:53AM -0500, thesven73@gmail.com wrote:
> From: Sven Van Asbroeck <TheSven73@gmail.com>
>
> Remove a leftover spinlock.
>
> This was required back when mod_delayed_work() did not exist,
> and had to be implemented with a cancel + queue. See
> commit e7c2f967445d ("workqueue: use mod_delayed_work() instead of
> __cancel + queue")
>
> schedule_delayed_work() and mod_delayed_work() can now be used
> concurrently. So the spinlock is no longer needed.
>
> Cc: Tejun Heo <tj@kernel.org>
> Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
Applied, thank you.
> ---
>
> v2: replace useless synaptics_i2c_reschedule_work() with
> mod_delayed_work().
>
> drivers/input/mouse/synaptics_i2c.c | 22 ++++------------------
> 1 file changed, 4 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/input/mouse/synaptics_i2c.c b/drivers/input/mouse/synaptics_i2c.c
> index 8538318d332c..fa304648d611 100644
> --- a/drivers/input/mouse/synaptics_i2c.c
> +++ b/drivers/input/mouse/synaptics_i2c.c
> @@ -219,7 +219,6 @@ struct synaptics_i2c {
> struct i2c_client *client;
> struct input_dev *input;
> struct delayed_work dwork;
> - spinlock_t lock;
> int no_data_count;
> int no_decel_param;
> int reduce_report_param;
> @@ -369,23 +368,11 @@ static bool synaptics_i2c_get_input(struct synaptics_i2c *touch)
> return xy_delta || gesture;
> }
>
> -static void synaptics_i2c_reschedule_work(struct synaptics_i2c *touch,
> - unsigned long delay)
> -{
> - unsigned long flags;
> -
> - spin_lock_irqsave(&touch->lock, flags);
> -
> - mod_delayed_work(system_wq, &touch->dwork, delay);
> -
> - spin_unlock_irqrestore(&touch->lock, flags);
> -}
> -
> static irqreturn_t synaptics_i2c_irq(int irq, void *dev_id)
> {
> struct synaptics_i2c *touch = dev_id;
>
> - synaptics_i2c_reschedule_work(touch, 0);
> + mod_delayed_work(system_wq, &touch->dwork, 0);
>
> return IRQ_HANDLED;
> }
> @@ -461,7 +448,7 @@ static void synaptics_i2c_work_handler(struct work_struct *work)
> * We poll the device once in THREAD_IRQ_SLEEP_SECS and
> * if error is detected, we try to reset and reconfigure the touchpad.
> */
> - synaptics_i2c_reschedule_work(touch, delay);
> + mod_delayed_work(system_wq, &touch->dwork, delay);
> }
>
> static int synaptics_i2c_open(struct input_dev *input)
> @@ -474,7 +461,7 @@ static int synaptics_i2c_open(struct input_dev *input)
> return ret;
>
> if (polling_req)
> - synaptics_i2c_reschedule_work(touch,
> + mod_delayed_work(system_wq, &touch->dwork,
> msecs_to_jiffies(NO_DATA_SLEEP_MSECS));
>
> return 0;
> @@ -530,7 +517,6 @@ static struct synaptics_i2c *synaptics_i2c_touch_create(struct i2c_client *clien
> touch->scan_rate_param = scan_rate;
> set_scan_rate(touch, scan_rate);
> INIT_DELAYED_WORK(&touch->dwork, synaptics_i2c_work_handler);
> - spin_lock_init(&touch->lock);
>
> return touch;
> }
> @@ -637,7 +623,7 @@ static int __maybe_unused synaptics_i2c_resume(struct device *dev)
> if (ret)
> return ret;
>
> - synaptics_i2c_reschedule_work(touch,
> + mod_delayed_work(system_wq, &touch->dwork,
> msecs_to_jiffies(NO_DATA_SLEEP_MSECS));
>
> return 0;
> --
> 2.17.1
>
--
Dmitry
^ permalink raw reply
* [PATCH v2] Input: synaptics_i2c - remove redundant spinlock
From: thesven73 @ 2019-02-17 6:10 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel, Sven Van Asbroeck, Tejun Heo
From: Sven Van Asbroeck <TheSven73@gmail.com>
Remove a leftover spinlock.
This was required back when mod_delayed_work() did not exist,
and had to be implemented with a cancel + queue. See
commit e7c2f967445d ("workqueue: use mod_delayed_work() instead of
__cancel + queue")
schedule_delayed_work() and mod_delayed_work() can now be used
concurrently. So the spinlock is no longer needed.
Cc: Tejun Heo <tj@kernel.org>
Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
---
v2: replace useless synaptics_i2c_reschedule_work() with
mod_delayed_work().
drivers/input/mouse/synaptics_i2c.c | 22 ++++------------------
1 file changed, 4 insertions(+), 18 deletions(-)
diff --git a/drivers/input/mouse/synaptics_i2c.c b/drivers/input/mouse/synaptics_i2c.c
index 8538318d332c..fa304648d611 100644
--- a/drivers/input/mouse/synaptics_i2c.c
+++ b/drivers/input/mouse/synaptics_i2c.c
@@ -219,7 +219,6 @@ struct synaptics_i2c {
struct i2c_client *client;
struct input_dev *input;
struct delayed_work dwork;
- spinlock_t lock;
int no_data_count;
int no_decel_param;
int reduce_report_param;
@@ -369,23 +368,11 @@ static bool synaptics_i2c_get_input(struct synaptics_i2c *touch)
return xy_delta || gesture;
}
-static void synaptics_i2c_reschedule_work(struct synaptics_i2c *touch,
- unsigned long delay)
-{
- unsigned long flags;
-
- spin_lock_irqsave(&touch->lock, flags);
-
- mod_delayed_work(system_wq, &touch->dwork, delay);
-
- spin_unlock_irqrestore(&touch->lock, flags);
-}
-
static irqreturn_t synaptics_i2c_irq(int irq, void *dev_id)
{
struct synaptics_i2c *touch = dev_id;
- synaptics_i2c_reschedule_work(touch, 0);
+ mod_delayed_work(system_wq, &touch->dwork, 0);
return IRQ_HANDLED;
}
@@ -461,7 +448,7 @@ static void synaptics_i2c_work_handler(struct work_struct *work)
* We poll the device once in THREAD_IRQ_SLEEP_SECS and
* if error is detected, we try to reset and reconfigure the touchpad.
*/
- synaptics_i2c_reschedule_work(touch, delay);
+ mod_delayed_work(system_wq, &touch->dwork, delay);
}
static int synaptics_i2c_open(struct input_dev *input)
@@ -474,7 +461,7 @@ static int synaptics_i2c_open(struct input_dev *input)
return ret;
if (polling_req)
- synaptics_i2c_reschedule_work(touch,
+ mod_delayed_work(system_wq, &touch->dwork,
msecs_to_jiffies(NO_DATA_SLEEP_MSECS));
return 0;
@@ -530,7 +517,6 @@ static struct synaptics_i2c *synaptics_i2c_touch_create(struct i2c_client *clien
touch->scan_rate_param = scan_rate;
set_scan_rate(touch, scan_rate);
INIT_DELAYED_WORK(&touch->dwork, synaptics_i2c_work_handler);
- spin_lock_init(&touch->lock);
return touch;
}
@@ -637,7 +623,7 @@ static int __maybe_unused synaptics_i2c_resume(struct device *dev)
if (ret)
return ret;
- synaptics_i2c_reschedule_work(touch,
+ mod_delayed_work(system_wq, &touch->dwork,
msecs_to_jiffies(NO_DATA_SLEEP_MSECS));
return 0;
--
2.17.1
^ permalink raw reply related
* Re: [PATCH] Input: st1232 - include gpio/consumer.h header for gpiod_set_value_cansleep()
From: Dmitry Torokhov @ 2019-02-17 5:35 UTC (permalink / raw)
To: Martin Kepplinger; +Cc: kuninori.morimoto.gx, linux-input, linux-kernel
In-Reply-To: <20190213111958.9031-1-martink@posteo.de>
On Wed, Feb 13, 2019 at 12:19:58PM +0100, Martin Kepplinger wrote:
> gpiod_set_value_cansleep() needs it's declaration in the corresponding
> header. This fixes build errors like
>
> drivers/input/touchscreen/st1232.c: In function 'st1232_ts_power':
> >> drivers/input/touchscreen/st1232.c:146:3: error: implicit declaration of
> function 'gpiod_set_value_cansleep'; did you mean 'gpio_set_value_cansleep'?
> [-Werror=implicit-function-declaration]
> gpiod_set_value_cansleep(ts->reset_gpio, !poweron);
> ^~~~~~~~~~~~~~~~~~~~~~~~
> gpio_set_value_cansleep
>
> Signed-off-by: Martin Kepplinger <martink@posteo.de>
Thanks Martin, I'll fold it into the original patch introducing gpiod so
we keep bisectability.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: synaptics_i2c - remove redundant spinlock
From: Dmitry Torokhov @ 2019-02-17 5:29 UTC (permalink / raw)
To: thesven73; +Cc: linux-input, linux-kernel, Tejun Heo
In-Reply-To: <20190212013442.26030-1-TheSven73@gmail.com>
Hi Sven,
On Mon, Feb 11, 2019 at 08:34:42PM -0500, thesven73@gmail.com wrote:
> @@ -372,13 +371,7 @@ static bool synaptics_i2c_get_input(struct synaptics_i2c *touch)
> static void synaptics_i2c_reschedule_work(struct synaptics_i2c *touch,
> unsigned long delay)
> {
> - unsigned long flags;
> -
> - spin_lock_irqsave(&touch->lock, flags);
> -
> mod_delayed_work(system_wq, &touch->dwork, delay);
> -
> - spin_unlock_irqrestore(&touch->lock, flags);
> }
This makes synaptics_i2c_reschedule_work() a useless wrapper for
mod_delayed_work(). Can we get rid of it?
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v1] Input: st-keyscan - fix potential zalloc NULL dereference
From: Dmitry Torokhov @ 2019-02-17 5:26 UTC (permalink / raw)
To: gabriel.fernandez
Cc: linux-clk, linux-stm32, linux-arm-kernel, linux-kernel,
linux-input, giuseppe.condorelli, Nate Drude, Dan Carpenter
In-Reply-To: <20190212153055.3925-1-gabriel.fernandez@st.com>
On Tue, Feb 12, 2019 at 04:30:55PM +0100, gabriel.fernandez@st.com wrote:
> From: Gabriel Fernandez <gabriel.fernandez@st.com>
>
> This patch fixes the following static checker warning:
>
> drivers/input/keyboard/st-keyscan.c:156 keyscan_probe()
> error: potential zalloc NULL dereference: 'keypad_data->input_dev'
>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com>
Applied, thank you.
> ---
> drivers/input/keyboard/st-keyscan.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/keyboard/st-keyscan.c b/drivers/input/keyboard/st-keyscan.c
> index babcfb165e4f..3b85631fde91 100644
> --- a/drivers/input/keyboard/st-keyscan.c
> +++ b/drivers/input/keyboard/st-keyscan.c
> @@ -153,6 +153,8 @@ static int keyscan_probe(struct platform_device *pdev)
>
> input_dev->id.bustype = BUS_HOST;
>
> + keypad_data->input_dev = input_dev;
> +
> error = keypad_matrix_key_parse_dt(keypad_data);
> if (error)
> return error;
> @@ -168,8 +170,6 @@ static int keyscan_probe(struct platform_device *pdev)
>
> input_set_drvdata(input_dev, keypad_data);
>
> - keypad_data->input_dev = input_dev;
> -
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> keypad_data->base = devm_ioremap_resource(&pdev->dev, res);
> if (IS_ERR(keypad_data->base))
> --
> 2.17.0
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH 2/2] Input: add Apple SPI keyboard and trackpad driver.
From: Andy Shevchenko @ 2019-02-16 0:44 UTC (permalink / raw)
To: Life is hard, and then you die
Cc: Dmitry Torokhov, Henrik Rydberg, Lukas Wunner, Federico Lorenzi,
linux-input, linux-kernel
In-Reply-To: <20190210111834.GA27627@innovation.ch>
On Sun, Feb 10, 2019 at 03:18:34AM -0800, Life is hard, and then you die wrote:
> On Wed, Feb 06, 2019 at 10:22:56PM +0200, Andy Shevchenko wrote:
> > On Mon, Feb 04, 2019 at 12:19:47AM -0800, Ronald Tschalär wrote:
> > > +#define debug_print(mask, fmt, ...) \
> > > + do { \
> > > + if (debug & mask) \
> > > + printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__); \
> > > + } while (0)
> > > +
> > > +#define debug_print_header(mask) \
> > > + debug_print(mask, "--- %s ---------------------------\n", \
> > > + applespi_debug_facility(mask))
> > > +
> > > +#define debug_print_buffer(mask, fmt, ...) \
> > > + do { \
> > > + if (debug & mask) \
> > > + print_hex_dump(KERN_DEBUG, pr_fmt(fmt), \
> > > + DUMP_PREFIX_NONE, 32, 1, ##__VA_ARGS__, \
> > > + false); \
> > > + } while (0)
> >
> > I'm not sure we need all of these... But okay, the driver is
> > reverse-engineered, perhaps we can drop it later on.
>
> These have been extremely useful for debugging; but yes, they are for
> debugging only. They also explicitly do not use the dynamic-debug
> facilities because
> 1. those can't be enabled at a function or line level on the kernel
> command line (the docs indicate this should work, but it doesn't,
> or at the very least I've been unable to figure out how, at least
> for those drivers built as modules)
Hmm... Usually you put either module_name.dyndbg into kernel command line to
enable Dynamic Debug on built-in modules, or modprobe module_name dyndbg if
it's built as a module.
> 2. the expressions to enable them quite brittle (in particular the
> line-based ones) since they (may) change any time the code is
> changed - having stable commands to give to users and put in
> documentation (e.g.
> "echo 0x200 > /sys/module/applespi/parameters/debug") is
> quite valuable.
> 3. In at least two places we log different types of packets in the
> same lines of code - dynamic-debug would only allow enabling or
> disabling logging of all packets, unless we do something like
> create separate functions for logging each type of packet.
> > > +static unsigned int fnmode = 1;
> > > +module_param(fnmode, uint, 0644);
> > > +MODULE_PARM_DESC(fnmode, "Mode of fn key on Apple keyboards (0 = disabled, [1] = fkeyslast, 2 = fkeysfirst)");
> >
> > fn -> Fn ?
>
> The physical key is actually labelled "fn" (no uppercase). But will
> certainly change it if you still wish.
I think Fn would be suitable (on the computer I'm typing this message the
special keys are all marked in small case, but we don't change HP driver to
state 'ctrl' instead 'Ctrl', for example, do we?).
> > Btw, do we need all these parameters? Can't we do modify behaviour run-time
> > using some Input framework facilities?
>
> I'm not aware of any way to do so. I suppose the event callback could
> be used/extended to send "configuration" data, but that would require
> changes to the input core.
>
> Also, for what it's worth, current drivers such as hid-apple (from
> which 'fnmode' and 'iso_layout' were copied and intentionally kept the
> same) use this approach too.
Hmm... Okay, I leave this to Input maintainers.
> > > +static int touchpad_dimensions[4];
> > > +module_param_array(touchpad_dimensions, int, NULL, 0444);
> > > +MODULE_PARM_DESC(touchpad_dimensions, "The pixel dimensions of the touchpad, as x_min,x_max,y_min,y_max .");
> >
> > We have some parsers for similar data as in format
> >
> > %dx%d%+d%+d ?
> >
> > For example, 200x100+20+10.
>
> Maybe it's just my grep-foo that is lacking, but I couldn't find
> anything useful. There is some stuff for framebuffer video modes, but
> that is too different and not really amenable for use here. OTOH, a
> simple sscanf(buf, "%dx%d+%d+%d", ...) would parse this just fine
> without the need for any fancier parser.
>
> OTOH, I'm not sure this format is any better: internally we need
> x/y min/max, not x/y/w/h (though obviously the two are trivially
> convertable); and for the user it doesn't really matter since they would
> basically be getting the dimensions from running with debug set to
> 0x10000 and using that output to set this param, i.e. I don't see any
> inherent advantage of using x/y/w/h.
I would stick with some more or less standard notation. The XxY+W+H is widely
used in X11 world.
If you have better examples for input devices, choose them. My point that it
would be better to be a standard to some extent.
> > > +/**
> > > + * struct keyboard_protocol - keyboard message.
> > > + * message.type = 0x0110, message.length = 0x000a
> > > + *
> > > + * @unknown1: unknown
> > > + * @modifiers: bit-set of modifier/control keys pressed
> > > + * @unknown2: unknown
> > > + * @keys_pressed: the (non-modifier) keys currently pressed
> > > + * @fn_pressed: whether the fn key is currently pressed
> > > + * @crc_16: crc over the whole message struct (message header +
> > > + * this struct) minus this @crc_16 field
> >
> > Something wrong with indentation. Check it over all your kernel doc comments.
>
> I used tabs here between the name and description, so it will show up
> odd in a diff or where quoted, but it is fine in the original. I've seen
> both styles (tabs and just spaces) used in the rest of code, so I'm not
> sure what the preferred one is. I'll switch to plain spaces if that's
> preferred.
Ah, if the result is okay, I'm fine.
> > > + */
> > > +struct touchpad_info_protocol {
> > > + __u8 unknown1[105];
> > > + __le16 model_id;
> > > + __u8 unknown2[3];
> > > + __le16 crc_16;
> > > +} __packed;
> >
> > 112 % 16 == 0, why do you need __packed?
>
> Because 'model_id' is not aligned on a 16-bit boundary. That this is a
> model id is just a guess (these are the only 2 bytes that differ
> between various touchpads, and those two bytes are always the same for
> a given touchpad size), and the fact that it's not aligned is
> suspicious (since everything else is), so it may actually well be two
> separate 8-bit fields. Looking at the known values (0x0417, 0x0557,
> and 0x06d7) it very well may be a 1-byte model (0x04, 0x05, 0x06) and
> a 1-byte "flags" (0x17, 0x57, 0xd7 - note how each one is a superset
> of the previous in terms of bits set).
>
> In short, I can change this to 2 1-byte fields instead and thereby
> avoid the need for __packed.
If you feel that it's indeed 16=bit value, better to leave it as is.
But your guess sounds very reasonable to me to split it to something like you
proposed.
> > > +struct applespi_tp_info {
> > > + int x_min;
> > > + int x_max;
> > > + int y_min;
> > > + int y_max;
> > > +};
> >
> > Perhaps use the same format as in struct drm_rect in order to possibility of
> > unifying them in the future?
>
> You mean change the order to be x_min, y_min, x_max, y_max? (the field
> types and semantics already match). Ok.
Yep.
> > > + switch (log_mask) {
> > > + case DBG_CMD_TP_INI:
> > > + return "Touchpad Initialization";
> > > + case DBG_CMD_BL:
> > > + return "Backlight Command";
> > > + case DBG_CMD_CL:
> > > + return "Caps-Lock Command";
> > > + case DBG_RD_KEYB:
> > > + return "Keyboard Event";
> > > + case DBG_RD_TPAD:
> > > + return "Touchpad Event";
> > > + case DBG_RD_UNKN:
> > > + return "Unknown Event";
> > > + case DBG_RD_IRQ:
> > > + return "Interrupt Request";
> > > + case DBG_RD_CRC:
> > > + return "Corrupted packet";
> > > + case DBG_TP_DIM:
> > > + return "Touchpad Dimensions";
> > > + default:
> > > + return "-Unknown-";
> >
> > What the difference to RD_UNKN ?
>
> RD_UNKN signifies an unknown packet type was received; default catches
> programming errors where a new log type was added without creating an
> entry here (i.e. defensive programmming).
I see.
> > Perhaps "Unrecognized ... "-ish better?
>
> Sure. I've updated these now to
>
> case DBG_RD_UNKN:
> return "Unrecognized Event";
>
> default:
> return "-Unrecognized log mask-";
What I suggested here (in case they are different) is to leave UNKN message as
is, but change default to use different word.
> > > + }
> > > +static int applespi_send_cmd_msg(struct applespi_data *applespi)
> > > +{
> >
> > > + if (applespi->want_tp_info_cmd) {
> >
> > > + } else if (applespi->want_mt_init_cmd) {
> >
> > > + } else if (applespi->want_cl_led_on != applespi->have_cl_led_on) {
> >
> > > + } else if (applespi->want_bl_level != applespi->have_bl_level) {
> >
> > > + } else {
> > > + return 0;
> > > + }
> >
> > Can we refactor to use some kind of enumeration and do switch-case here?
>
> Multiple of these want_xyz may be "active" simultaneously (e.g. both a
> keyboard brightness change and the caps-lock-led change may be
> outstanding), as well these not all being simple booleans (want_bl_level
> is an actual level value).
>
> The nearest I can think of right now would be to create a bitmap to hold
> potential change requests (so e.g. setting a new backlight level would
> set both the new wanted level as well a bit indicating that a backlight
> level change was requested, though the above check against the current
> level would still be needed), and use ffs() to pick a set bit and switch
> on the result. In pseudo code:
>
> applespi_set_bl_level()
> want_bl_level = new-level
> set_bit(BL_CMD, outstanding_cmds)
>
> applespi_set_capsl_led()
> want_cl_led_on = new-led-on
> set_bit(CL_CMD, outstanding_cmds)
>
> applespi_send_cmd_msg()
> bool found_cmd = false
> while (!found_cmd) {
> cmd = ffs(outstanding_cmds)
> switch (cmd) {
> case CL_CMD:
> if (applespi->want_cl_led_on != applespi->have_cl_led_on) {
> found_cmd = true
> ...
> }
> clear_bit(CL_CMD, outstanding_cmds)
> break
>
> case BL_CMD:
> if (applespi->want_bl_level != applespi->have_bl_level) {
> found_cmd = true
> ...
> }
> clear_bit(BL_CMD, outstanding_cmds)
> break
>
> ... other commands ...
>
> default:
> return
> }
> }
>
> Would this be preferrable?
I don't think it will make code better. Let's leave your initial proposal.
> > > + message->counter = applespi->cmd_msg_cntr++ & 0xff;
> >
> > Usual pattern for this kind of entries is
> >
> > x = ... % 256;
> >
> > Btw, isn't 256 is defined somewhere?
>
> Many things are defined as have a value of 256 :-) But I didn't find any
> with the intended semantics; could use (U8_MAX+1), though.
What I meant is that I saw in the same driver definition with value of 256.
Isn't it about messages?
> > Btw, consider to use dev_warn() and Co instead of pr_warn() or in cases where
> > appropriate acpi_handle_warn(), etc.
>
> I've changed all log calls to use dev_xyz() now, including the
> debug_print()'s (for consistency in the resulting log messages).
>
> Regarding acpi_handle_xyz(), though: isn't it better to have all log
> messages for a given device use the same logging prefix? I.e. in this
> case to use dev_xyz() instead of a mix and match of dev_xyz() and
> acpi_handle_xyz()? That makes it easier to grep for all messages related
> to a given module/device.
I'm fine with all being dev_<lvl>(). acpi_handle_<lvl>() makes sense when you
have interaction with ACPI handle and not a device.
> > > +/* lifted from the BCM5974 driver */
> > > +/* convert 16-bit little endian to signed integer */
> > > +static inline int raw2int(__le16 x)
> > > +{
> > > + return (signed short)le16_to_cpu(x);
> > > +}
> >
> > Perhaps it's time to introduce it inside include/linux/input.h ?
>
> Perhaps as
>
> static inline int le16_to_signed_int(__le16 x)
>
> ? (raw2int seems to ambiguous for a global function)
I'm fine. It would need a description though.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* [PATCH v3 3/3] input: goodix: Print values in case of inconsistencies
From: Guido Günther @ 2019-02-15 14:11 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Mark Rutland, Bastien Nocera,
Matthias Brugger, Guido Günther, linux-input, devicetree,
linux-kernel, linux-arm-kernel, linux-mediatek
In-Reply-To: <cover.1550239328.git.agx@sigxcpu.org>
"Invalid config" gives little idea what's wrong. Print the values that
must not be 0 so we know which ones are off.
Signed-off-by: Guido Günther <agx@sigxcpu.org>
---
drivers/input/touchscreen/goodix.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index 47b1ced41576..f57d82220a88 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -693,7 +693,9 @@ static int goodix_configure_dev(struct goodix_ts_data *ts)
touchscreen_parse_properties(ts->input_dev, true, &ts->prop);
if (!ts->prop.max_x || !ts->prop.max_y || !ts->max_touch_num) {
- dev_err(&ts->client->dev, "Invalid config, using defaults\n");
+ dev_err(&ts->client->dev,
+ "Invalid config (%d, %d, %d), using defaults\n",
+ ts->prop.max_x, ts->prop.max_y, ts->max_touch_num);
ts->prop.max_x = GOODIX_MAX_WIDTH - 1;
ts->prop.max_y = GOODIX_MAX_HEIGHT - 1;
ts->max_touch_num = GOODIX_MAX_CONTACTS;
--
2.20.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox