* [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 (resend)] Input: uinput - Set name/phys to NULL before kfree().
From: Tetsuo Handa @ 2019-02-18 10:10 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: rydberg, syzbot, linux-input, linux-kernel, syzkaller-bugs
In-Reply-To: <20190217210713.GA145509@dtor-ws>
Thank you for responding.
On 2019/02/18 6:07, Dmitry Torokhov wrote:
> 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.
Then, we want to keep dev->name and dev->phys when calling "unregister" 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.
If we want to keep dev->name and dev->phys when calling "unregister" time,
we could do something like below. Does calling kobject_uevent(KOBJ_REMOVE)
without dev->name and dev->phys (to some degree) help (compared to not
triggering kobject_uevent(KOBJ_REMOVE) at all) ?
diff --git a/drivers/input/input.c b/drivers/input/input.c
index 3304aaa..da39a23 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -1587,6 +1587,7 @@ static int input_dev_uevent(struct device *device, struct kobj_uevent_env *env)
{
struct input_dev *dev = to_input_dev(device);
+ rcu_read_lock();
INPUT_ADD_HOTPLUG_VAR("PRODUCT=%x/%x/%x/%x",
dev->id.bustype, dev->id.vendor,
dev->id.product, dev->id.version);
@@ -1618,6 +1619,7 @@ static int input_dev_uevent(struct device *device, struct kobj_uevent_env *env)
INPUT_ADD_HOTPLUG_BM_VAR("SW=", dev->swbit, SW_MAX);
INPUT_ADD_HOTPLUG_MODALIAS_VAR(dev);
+ rcu_read_unlock();
return 0;
}
diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index 26ec603f..6689312 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -308,9 +308,12 @@ static void uinput_destroy_device(struct uinput_device *udev)
} else {
input_free_device(dev);
}
+ dev->name = NULL;
+ dev->phys = NULL;
+ udev->dev = NULL;
+ synchronize_rcu();
kfree(name);
kfree(phys);
- udev->dev = NULL;
}
}
^ permalink raw reply related
* Re: [PATCH 2/2] Input: add Apple SPI keyboard and trackpad driver.
From: Andy Shevchenko @ 2019-02-18 12:00 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: <20190218090851.GA24261@innovation.ch>
On Mon, Feb 18, 2019 at 01:08:51AM -0800, Life is hard, and then you die wrote:
> 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:
> > 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)
Of course it wouldn't work that way. If you have compiled driver as a module,
you need to supply the parameters during modprobe. Actually it's kinda luck
that +p works for you.
> > > > > + 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.
OK.
> > > > > +/* 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 see. Since there is no comment from input maintainers, let's stick with the
initial approach (copy'n'paste to your code with comment from where). Only one
suggestion is to name function for further use without changing it. So, in the
future we may move it to generic header.
> I'll send out v2 of the patches with all the changes soon.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH] Input: uinput - Allow uinput_request to be interrupted
From: Rodrigo Rivas Costa @ 2019-02-18 14:21 UTC (permalink / raw)
To: linux-kernel
Cc: dmitry.torokhov, rodrigorivascosta, Marcos Paulo de Souza,
Peter Hutterer, Paul E. McKenney, Martin Kepplinger,
open list:INPUT (KEYBOARD, MOUSE, JOYSTICK, TOUCHSCREEN)
In-Reply-To: <20190218004305.339758-1-marcos.souza.org@gmail.com>
On Sun, Feb 17, 2019 at 09:42:52PM -0300, Marcos Paulo de Souza wrote:
> - if (!wait_for_completion_timeout(&request->done, 30 * HZ)) {
> + if (!wait_for_completion_interruptible_timeout(&request->done,
> + 30 * HZ)) {
> retval = -ETIMEDOUT;
> goto out;
> }
Now this function can succeed or fail because of ETIMEDOUT or an
interrupt. I think you should return -EINTR or maybe -ESYSRESTART if
interrupted.
^ permalink raw reply
* Re: [PATCH v5 3/4] dt-bindings: input: touchscreen: goodix: Add GT5663 compatible
From: Rob Herring @ 2019-02-18 15:45 UTC (permalink / raw)
Cc: Dmitry Torokhov, Bastien Nocera, Henrik Rydberg, linux-input,
linux-kernel, devicetree, Mark Rutland, linux-amarula,
Michael Trimarchi, Jagan Teki
In-Reply-To: <20190217091436.3702-4-jagan@amarulasolutions.com>
On Sun, 17 Feb 2019 14:44:35 +0530, Jagan Teki wrote:
> 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(+)
>
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH v5 1/4] dt-bindings: input: touchscreen: goodix: Document regulator properties
From: Rob Herring @ 2019-02-18 15:45 UTC (permalink / raw)
To: Jagan Teki
Cc: Dmitry Torokhov, Bastien Nocera, Henrik Rydberg, linux-input,
linux-kernel, devicetree, Mark Rutland, linux-amarula,
Michael Trimarchi
In-Reply-To: <20190217091436.3702-2-jagan@amarulasolutions.com>
On Sun, Feb 17, 2019 at 02:44:33PM +0530, Jagan Teki wrote:
> 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(+)
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH v6 01/11] dt-bindings: mfd: add DT bindings for max77650
From: Rob Herring @ 2019-02-18 16:35 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Liam Girdwood, Greg Kroah-Hartman, linux-kernel, linux-gpio,
devicetree, linux-input, linux-leds, linux-pm,
Bartosz Golaszewski
In-Reply-To: <20190214140022.19201-2-brgl@bgdev.pl>
On Thu, 14 Feb 2019 15:00:12 +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Add a DT binding document for max77650 ultra-low power PMIC. This
> describes the core mfd device and the GPIO module.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
> .../devicetree/bindings/mfd/max77650.txt | 46 +++++++++++++++++++
> 1 file changed, 46 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/max77650.txt
>
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH v6 04/11] dt-bindings: input: add DT bindings for max77650
From: Rob Herring @ 2019-02-18 16:35 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Liam Girdwood, Greg Kroah-Hartman, linux-kernel, linux-gpio,
devicetree, linux-input, linux-leds, linux-pm,
Bartosz Golaszewski
In-Reply-To: <20190214140022.19201-5-brgl@bgdev.pl>
On Thu, 14 Feb 2019 15:00:15 +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Add the DT binding document for the onkey module of max77650.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
> .../bindings/input/max77650-onkey.txt | 26 +++++++++++++++++++
> 1 file changed, 26 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/input/max77650-onkey.txt
>
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH v2 1/5] dt-bindings: input: Add binding for bma150 sensor
From: Rob Herring @ 2019-02-18 19:18 UTC (permalink / raw)
To: Paweł Chmiel
Cc: dmitry.torokhov, mark.rutland, xc-racer2, devicetree, linux-input,
linux-kernel
In-Reply-To: <20190202151806.9064-2-pawel.mikolaj.chmiel@gmail.com>
On Sat, Feb 02, 2019 at 04:18:02PM +0100, Paweł Chmiel wrote:
> From: Jonathan Bakker <xc-racer2@live.ca>
>
> Add device tree bindings for Bosch BMA150 Accelerometer Sensor
>
> Changes from v1:
> - Add properties for all of bma150_cfg
> - Correct IRQ type in example
>
> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
> Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
> ---
> .../bindings/input/bosch,bma150.txt | 38 +++++++++++++++++++
> include/dt-bindings/input/bma150.h | 22 +++++++++++
> include/linux/bma150.h | 13 +------
> 3 files changed, 62 insertions(+), 11 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/input/bosch,bma150.txt
> create mode 100644 include/dt-bindings/input/bma150.h
>
> diff --git a/Documentation/devicetree/bindings/input/bosch,bma150.txt b/Documentation/devicetree/bindings/input/bosch,bma150.txt
> new file mode 100644
> index 000000000000..f644d132f79c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/bosch,bma150.txt
> @@ -0,0 +1,38 @@
> +* Bosch BMA150 Accelerometer Sensor
> +
> +Also works for the SMB380 and BMA023 accelerometers
> +
> +Required properties:
> +- compatible : Should be "bosch,bma150"
> +- reg : The I2C address of the sensor
> +
> +Optional properties:
> +- interrupt-parent : should be the phandle for the interrupt controller
This is implied and can be dropped.
> +- interrupts : Interrupt mapping for IRQ. If not present device will be polled
> +- any-motion-int : bool for if the any motion interrupt should be enabled
> +- hg-int : bool for if the high-G interrupt should be enabled
> +- lg-int : bool for if the low-G interrupt should be enabled
> +- any-motion-cfg : array of integers for any motion duration and threshold
> +- hg-cfg : array of integers for high-G hysterisis, duration, and threshold
> +- lg-cfg : array of integers for low-G hysterisis, duration, and threshold
> +- range : configuration of range, one of BMA150_RANGE_* as defined in [1]
> +- bandwidth : refresh rate of device, one of BMA150_BW_* as defined in [1]
These all need vendor prefixes if they stay.
What determines all this configuration? It seems like a user may want to
change at run-time in which case sysfs would be more appropriate.
I don't recall seeing other accelerometers with these, but seems these
could apply to other accelerometers. In which case, they should be
common.
> +
> +Example:
> +
> +bma150@38 {
accelerometer@38
> + compatible = "bosch,bma150";
> + reg = <0x38>;
> + interrupt-parent = <&gph0>;
> + interrupts = <1 IRQ_TYPE_EDGE_RISING>;
> + any-motion-int;
> + hg-int;
> + lg-int;
> + any-motion-cfg = <0 0>;
> + hg-cfg = <0 150 160>;
> + lg-cfg = <0 150 20>;
> + range = <BMA150_RANGE_2G>;
> + bandwidth = <BMA150_BW_50HZ>;
> +};
> +
> +[1] include/dt-bindings/input/bma150.h
> diff --git a/include/dt-bindings/input/bma150.h b/include/dt-bindings/input/bma150.h
> new file mode 100644
> index 000000000000..fb38ca787f0f
> --- /dev/null
> +++ b/include/dt-bindings/input/bma150.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * This header provides bindings for the BMA150 accelerometer
> + */
> +#ifndef _DT_BINDINGS_INPUT_BMA150_H
> +#define _DT_BINDINGS_INPUT_BMA150_H
> +
> +/* Range */
> +#define BMA150_RANGE_2G 0
> +#define BMA150_RANGE_4G 1
> +#define BMA150_RANGE_8G 2
> +
> +/* Refresh rate */
> +#define BMA150_BW_25HZ 0
> +#define BMA150_BW_50HZ 1
> +#define BMA150_BW_100HZ 2
> +#define BMA150_BW_190HZ 3
> +#define BMA150_BW_375HZ 4
> +#define BMA150_BW_750HZ 5
> +#define BMA150_BW_1500HZ 6
> +
> +#endif /* _DT_BINDINGS_INPUT_BMA150_H */
> diff --git a/include/linux/bma150.h b/include/linux/bma150.h
> index 97ade7cdc870..b85266a9c35c 100644
> --- a/include/linux/bma150.h
> +++ b/include/linux/bma150.h
> @@ -20,19 +20,10 @@
> #ifndef _BMA150_H_
> #define _BMA150_H_
>
> -#define BMA150_DRIVER "bma150"
> +#include <dt-bindings/input/bma150.h>
>
> -#define BMA150_RANGE_2G 0
> -#define BMA150_RANGE_4G 1
> -#define BMA150_RANGE_8G 2
> +#define BMA150_DRIVER "bma150"
>
> -#define BMA150_BW_25HZ 0
> -#define BMA150_BW_50HZ 1
> -#define BMA150_BW_100HZ 2
> -#define BMA150_BW_190HZ 3
> -#define BMA150_BW_375HZ 4
> -#define BMA150_BW_750HZ 5
> -#define BMA150_BW_1500HZ 6
>
> struct bma150_cfg {
> bool any_motion_int; /* Set to enable any-motion interrupt */
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH v3 1/4] dt-bindings: input: touchscreen: goodix: Document AVDD28-supply property
From: Jagan Teki @ 2019-02-18 19:30 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: <CAMty3ZDbFU-NpWQvkaUWgUiVTabjTZoFWUCUZSPKMA6VuHc+iQ@mail.gmail.com>
On Mon, Feb 18, 2019 at 3:03 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> 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?
The time between these two regulator operations is nearly 0, as per
power scheme diagram T1 >= 0us. I think I can use bulk since there is
no necessity of delay between these two regulator operations.
What do you think?
^ permalink raw reply
* Re: [PATCH] Input: uinput - Allow uinput_request to be interrupted
From: Dmitry Torokhov @ 2019-02-18 20:15 UTC (permalink / raw)
To: Rodrigo Rivas Costa
Cc: linux-kernel, Marcos Paulo de Souza, Peter Hutterer,
Paul E. McKenney, Martin Kepplinger,
open list:INPUT (KEYBOARD, MOUSE, JOYSTICK, TOUCHSCREEN)
In-Reply-To: <20190218142110.GA23087@casa>
On Mon, Feb 18, 2019 at 03:21:10PM +0100, Rodrigo Rivas Costa wrote:
> On Sun, Feb 17, 2019 at 09:42:52PM -0300, Marcos Paulo de Souza wrote:
> > - if (!wait_for_completion_timeout(&request->done, 30 * HZ)) {
> > + if (!wait_for_completion_interruptible_timeout(&request->done,
> > + 30 * HZ)) {
> > retval = -ETIMEDOUT;
> > goto out;
> > }
>
> Now this function can succeed or fail because of ETIMEDOUT or an
> interrupt. I think you should return -EINTR or maybe -ESYSRESTART if
> interrupted.
Rodrigo, you are right. Marcos, could you please send updated patch that
returns different error code for timeout vs interrupt condition?
I dropped the patch for now.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2 1/5] dt-bindings: input: Add binding for bma150 sensor
From: Jonathan Bakker @ 2019-02-18 22:17 UTC (permalink / raw)
To: Rob Herring, Paweł Chmiel
Cc: dmitry.torokhov@gmail.com, mark.rutland@arm.com,
devicetree@vger.kernel.org, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20190218191850.GA31221@bogus>
On 2019-02-18 11:18 a.m., Rob Herring wrote:
> On Sat, Feb 02, 2019 at 04:18:02PM +0100, Paweł Chmiel wrote:
>> From: Jonathan Bakker <xc-racer2@live.ca>
>>
>> Add device tree bindings for Bosch BMA150 Accelerometer Sensor
>>
>> Changes from v1:
>> - Add properties for all of bma150_cfg
>> - Correct IRQ type in example
>>
>> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
>> Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
>> ---
>> .../bindings/input/bosch,bma150.txt | 38 +++++++++++++++++++
>> include/dt-bindings/input/bma150.h | 22 +++++++++++
>> include/linux/bma150.h | 13 +------
>> 3 files changed, 62 insertions(+), 11 deletions(-)
>> create mode 100644 Documentation/devicetree/bindings/input/bosch,bma150.txt
>> create mode 100644 include/dt-bindings/input/bma150.h
>>
>> diff --git a/Documentation/devicetree/bindings/input/bosch,bma150.txt b/Documentation/devicetree/bindings/input/bosch,bma150.txt
>> new file mode 100644
>> index 000000000000..f644d132f79c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/input/bosch,bma150.txt
>> @@ -0,0 +1,38 @@
>> +* Bosch BMA150 Accelerometer Sensor
>> +
>> +Also works for the SMB380 and BMA023 accelerometers
>> +
>> +Required properties:
>> +- compatible : Should be "bosch,bma150"
>> +- reg : The I2C address of the sensor
>> +
>> +Optional properties:
>> +- interrupt-parent : should be the phandle for the interrupt controller
>
> This is implied and can be dropped.
>
Ok, sounds good.
>> +- interrupts : Interrupt mapping for IRQ. If not present device will be polled
>
>> +- any-motion-int : bool for if the any motion interrupt should be enabled
>> +- hg-int : bool for if the high-G interrupt should be enabled
>> +- lg-int : bool for if the low-G interrupt should be enabled
>> +- any-motion-cfg : array of integers for any motion duration and threshold
>> +- hg-cfg : array of integers for high-G hysterisis, duration, and threshold
>> +- lg-cfg : array of integers for low-G hysterisis, duration, and threshold
>> +- range : configuration of range, one of BMA150_RANGE_* as defined in [1]
>> +- bandwidth : refresh rate of device, one of BMA150_BW_* as defined in [1]
>
> These all need vendor prefixes if they stay.
>
> What determines all this configuration? It seems like a user may want to
> change at run-time in which case sysfs would be more appropriate.
>
> I don't recall seeing other accelerometers with these, but seems these
> could apply to other accelerometers. In which case, they should be
> common.
>
From my understanding of the pre-existing non-DT driver and the datasheet (available at https://media.digikey.com/pdf/Data%20Sheets/Bosch/BMA150.pdf), the *-int and *-cfg are for configuring of what will cause the chip to send an interrupt. A sysfs property would probably work for them as well although I don't know if they can be adjusted on the fly or not. My device doesn't have the interrupt line hooked up and instead uses the polling method, so the only tunables I can test are the range and bandwith which are used as a type of digital smoothing.
As I personally use the defaults and there are no current in-tree users, I am fine with dropping all of this configuration and going with the defaults that are said to provide "generic sensitivity performance". If someone wants to add it in the future, they can add the sysfs and/or DT properties. The similar bma180 IIO driver hardcodes everything.
However, looking through all this again, instead of making this driver DT-aware, it might make more sense to expand the bma180 IIO driver to add support for bma150. Most of the accelerometer drivers are already in iio plus it would add support for the temperature part of the sensor. Would it then make sense to remove this driver entirely or can the two co-exist?
>> +
>> +Example:
>> +
>> +bma150@38 {
>
> accelerometer@38
Got it.
>
>> + compatible = "bosch,bma150";
>> + reg = <0x38>;
>> + interrupt-parent = <&gph0>;
>> + interrupts = <1 IRQ_TYPE_EDGE_RISING>;
>> + any-motion-int;
>> + hg-int;
>> + lg-int;
>> + any-motion-cfg = <0 0>;
>> + hg-cfg = <0 150 160>;
>> + lg-cfg = <0 150 20>;
>> + range = <BMA150_RANGE_2G>;
>> + bandwidth = <BMA150_BW_50HZ>;
>> +};
>> +
>> +[1] include/dt-bindings/input/bma150.h
>> diff --git a/include/dt-bindings/input/bma150.h b/include/dt-bindings/input/bma150.h
>> new file mode 100644
>> index 000000000000..fb38ca787f0f
>> --- /dev/null
>> +++ b/include/dt-bindings/input/bma150.h
>> @@ -0,0 +1,22 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * This header provides bindings for the BMA150 accelerometer
>> + */
>> +#ifndef _DT_BINDINGS_INPUT_BMA150_H
>> +#define _DT_BINDINGS_INPUT_BMA150_H
>> +
>> +/* Range */
>> +#define BMA150_RANGE_2G 0
>> +#define BMA150_RANGE_4G 1
>> +#define BMA150_RANGE_8G 2
>> +
>> +/* Refresh rate */
>> +#define BMA150_BW_25HZ 0
>> +#define BMA150_BW_50HZ 1
>> +#define BMA150_BW_100HZ 2
>> +#define BMA150_BW_190HZ 3
>> +#define BMA150_BW_375HZ 4
>> +#define BMA150_BW_750HZ 5
>> +#define BMA150_BW_1500HZ 6
>> +
>> +#endif /* _DT_BINDINGS_INPUT_BMA150_H */
>> diff --git a/include/linux/bma150.h b/include/linux/bma150.h
>> index 97ade7cdc870..b85266a9c35c 100644
>> --- a/include/linux/bma150.h
>> +++ b/include/linux/bma150.h
>> @@ -20,19 +20,10 @@
>> #ifndef _BMA150_H_
>> #define _BMA150_H_
>>
>> -#define BMA150_DRIVER "bma150"
>> +#include <dt-bindings/input/bma150.h>
>>
>> -#define BMA150_RANGE_2G 0
>> -#define BMA150_RANGE_4G 1
>> -#define BMA150_RANGE_8G 2
>> +#define BMA150_DRIVER "bma150"
>>
>> -#define BMA150_BW_25HZ 0
>> -#define BMA150_BW_50HZ 1
>> -#define BMA150_BW_100HZ 2
>> -#define BMA150_BW_190HZ 3
>> -#define BMA150_BW_375HZ 4
>> -#define BMA150_BW_750HZ 5
>> -#define BMA150_BW_1500HZ 6
>>
>> struct bma150_cfg {
>> bool any_motion_int; /* Set to enable any-motion interrupt */
>> --
>> 2.17.1
>>
Thanks,
Jonathan
^ permalink raw reply
* Re: [PATCH v3 1/4] dt-bindings: input: touchscreen: goodix: Document AVDD28-supply property
From: Dmitry Torokhov @ 2019-02-19 8:15 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: <CAMty3ZBVpNY+JF_vEMs83M29WKQDhzmk7LyL-HHCCA7uD0Y+=g@mail.gmail.com>
On Tue, Feb 19, 2019 at 01:00:19AM +0530, Jagan Teki wrote:
> On Mon, Feb 18, 2019 at 3:03 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
> >
> > 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?
>
> The time between these two regulator operations is nearly 0, as per
> power scheme diagram T1 >= 0us. I think I can use bulk since there is
> no necessity of delay between these two regulator operations.
>
> What do you think?
Does the bulk enable provide any guarantees as to the order of
operations? If it does not then I think we should ensure that they are
powered up/down in the right order.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v3 1/4] dt-bindings: input: touchscreen: goodix: Document AVDD28-supply property
From: Jagan Teki @ 2019-02-19 8:45 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: <20190219081510.GB65896@dtor-ws>
On Tue, Feb 19, 2019 at 1:45 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Tue, Feb 19, 2019 at 01:00:19AM +0530, Jagan Teki wrote:
> > On Mon, Feb 18, 2019 at 3:03 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > >
> > > 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?
> >
> > The time between these two regulator operations is nearly 0, as per
> > power scheme diagram T1 >= 0us. I think I can use bulk since there is
> > no necessity of delay between these two regulator operations.
> >
> > What do you think?
>
> Does the bulk enable provide any guarantees as to the order of
> operations? If it does not then I think we should ensure that they are
> powered up/down in the right order.
At-least I can saw from the log, async enables based on the definition
of supply arrays. Indeed the regulators with bulk calls enables based
on the supply array order, this is what I usually think.
[ 3.676483] Goodix-TS 1-005d: 1-005d supply VDDIO not found, using
dummy regulator
[ 3.693159] regulator_bulk_enable_async: Supply AVDD28
[ 3.693162] regulator_bulk_enable_async: Supply VDDIO
[ 3.806095] Goodix-TS 1-005d: ID 5663, version: 0100
Do we need to go reverse order for power-down? I couldn't see this
information on the datasheet. If yes then we need to go for normal
calls.
^ permalink raw reply
* [PATCH v6 0/4] input: touchscreen: Add goodix GT5553 CTP support
From: Jagan Teki @ 2019-02-19 10:16 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 v6 patchset for supporting goodix GT5553 CTP. Here is the
previous version[1]
Changes for v5:
- document bindings for required regulators, which are need during
power-on sequence
- enable, disable required regulators as described in power-on sequence
using normal regulator calls
- update the proper commi messages
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/10816901/
Jagan Teki (4):
dt-bindings: input: touchscreen: goodix: Document regulator properties
Input: goodix - Add regulators suppot
dt-bindings: input: touchscreen: goodix: Add GT5663 compatible
Input: goodix - Add GT5663 CTP support
.../bindings/input/touchscreen/goodix.txt | 3 +
drivers/input/touchscreen/goodix.c | 60 +++++++++++++++++++
2 files changed, 63 insertions(+)
--
2.18.0.321.gffc6fa0e3
^ permalink raw reply
* [PATCH v6 1/4] dt-bindings: input: touchscreen: goodix: Document regulator properties
From: Jagan Teki @ 2019-02-19 10:16 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: <20190219101629.15977-1-jagan@amarulasolutions.com>
Goodix CTP controllers support analog, digital and gpio regulator
supplies on relevant controller pin configurations.
Out of which AVDD28 and VDDIO regulators are required in few goodix CTP
chips during power-on sequence.
AVDD22, DVDD12 regulators have no relevant functionality described from
datasheet [1].
So, document both AVDD28, VDDIO regulators into optional properties since
few of the goodix chip do work without these regulator power-on sequence.
[1] GT5663 Datasheet_English_20151106_Rev.01
Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
Documentation/devicetree/bindings/input/touchscreen/goodix.txt | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
index f7e95c52f3c7..b9b8f196dc90 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
@@ -23,6 +23,8 @@ 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
+ - VDDIO-supply : GPIO power supply regulator on VDDIO pin
Example:
--
2.18.0.321.gffc6fa0e3
^ permalink raw reply related
* [PATCH v6 2/4] Input: goodix - Add regulators suppot
From: Jagan Teki @ 2019-02-19 10:16 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: <20190219101629.15977-1-jagan@amarulasolutions.com>
Goodix CTP controllers require AVDD28, VDDIO regulators for power-on
sequence.
The delay between these regualtor operations as per Power-on Timing
from datasheet[1] is 0 (T1 >= 0 usec).
So, enable and disable these regulators in proper order using normal
regulator functions without any delay in between.
[1] GT5663 Datasheet_English_20151106_Rev.01
Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
drivers/input/touchscreen/goodix.c | 58 ++++++++++++++++++++++++++++++
1 file changed, 58 insertions(+)
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index f2d9c2c41885..5f9e755c5bc7 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,8 @@ struct goodix_ts_data {
struct touchscreen_properties prop;
unsigned int max_touch_num;
unsigned int int_trigger_type;
+ struct regulator *avdd28;
+ struct regulator *vddio;
struct gpio_desc *gpiod_int;
struct gpio_desc *gpiod_rst;
u16 id;
@@ -531,6 +534,24 @@ static int goodix_get_gpio_config(struct goodix_ts_data *ts)
return -EINVAL;
dev = &ts->client->dev;
+ ts->avdd28 = devm_regulator_get(dev, "AVDD28");
+ if (IS_ERR(ts->avdd28)) {
+ error = PTR_ERR(ts->avdd28);
+ if (error != -EPROBE_DEFER)
+ dev_err(dev,
+ "Failed to get AVDD28 regulator: %d\n", error);
+ return error;
+ }
+
+ ts->vddio = devm_regulator_get(dev, "VDDIO");
+ if (IS_ERR(ts->vddio)) {
+ error = PTR_ERR(ts->vddio);
+ if (error != -EPROBE_DEFER)
+ dev_err(dev,
+ "Failed to get VDDIO regulator: %d\n", error);
+ return error;
+ }
+
/* Get the interrupt GPIO pin number */
gpiod = devm_gpiod_get_optional(dev, GOODIX_GPIO_INT_NAME, GPIOD_IN);
if (IS_ERR(gpiod)) {
@@ -761,6 +782,17 @@ 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;
+
+ if (!IS_ERR(ts->vddio))
+ regulator_disable(ts->vddio);
+
+ if (!IS_ERR(ts->avdd28))
+ regulator_disable(ts->avdd28);
+}
+
static int goodix_ts_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
@@ -786,6 +818,32 @@ static int goodix_ts_probe(struct i2c_client *client,
if (error)
return error;
+ error = devm_add_action_or_reset(&client->dev,
+ goodix_disable_regulator, ts);
+ if (error)
+ return error;
+
+ /* power the controller */
+ if (!IS_ERR(ts->avdd28)) {
+ error = regulator_enable(ts->avdd28);
+ if (error) {
+ dev_err(&client->dev,
+ "Failed to enable AVDD28 regulator: %d\n",
+ error);
+ return error;
+ }
+ }
+
+ if (!IS_ERR(ts->vddio)) {
+ error = regulator_enable(ts->vddio);
+ if (error) {
+ dev_err(&client->dev,
+ "Failed to enable VDDIO regulator: %d\n",
+ error);
+ return error;
+ }
+ }
+
if (ts->gpiod_int && ts->gpiod_rst) {
/* reset the controller */
error = goodix_reset(ts);
--
2.18.0.321.gffc6fa0e3
^ permalink raw reply related
* [PATCH v6 3/4] dt-bindings: input: touchscreen: goodix: Add GT5663 compatible
From: Jagan Teki @ 2019-02-19 10:16 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: <20190219101629.15977-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>
Reviewed-by: Rob Herring <robh@kernel.org>
---
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 b9b8f196dc90..1dcae643427f 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 v6 4/4] Input: goodix - Add GT5663 CTP support
From: Jagan Teki @ 2019-02-19 10:16 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: <20190219101629.15977-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 5f9e755c5bc7..405246d61701 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -219,6 +219,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:
@@ -1000,6 +1001,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
* Re: [PATCH] HID: roccat: Mark expected switch fall-through
From: Jiri Kosina @ 2019-02-19 13:16 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: Stefan Achatz, Benjamin Tissoires, linux-input, linux-kernel,
Kees Cook
In-Reply-To: <20190211215334.GA6216@embeddedor>
On Mon, 11 Feb 2019, Gustavo A. R. Silva wrote:
> In preparation to enabling -Wimplicit-fallthrough, mark switch
> cases where we are expecting to fall through.
>
> This patch fixes the following warning:
>
> drivers/hid/hid-roccat-kone.c: In function ‘kone_keep_values_up_to_date’:
> drivers/hid/hid-roccat-kone.c:784:20: warning: this statement may fall through [-Wimplicit-fallthrough=]
> kone->actual_dpi = kone->profiles[event->value - 1].
> ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> startup_dpi;
> ~~~~~~~~~~~
> drivers/hid/hid-roccat-kone.c:786:2: note: here
> case kone_mouse_event_osd_profile:
> ^~~~
>
> Warning level 3 was used: -Wimplicit-fallthrough=3
>
> This patch is part of the ongoing efforts to enable
> -Wimplicit-fallthrough.
>
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> ---
> drivers/hid/hid-roccat-kone.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/hid/hid-roccat-kone.c b/drivers/hid/hid-roccat-kone.c
> index bf4675a27396..c4dd6162c1d6 100644
> --- a/drivers/hid/hid-roccat-kone.c
> +++ b/drivers/hid/hid-roccat-kone.c
> @@ -783,6 +783,7 @@ static void kone_keep_values_up_to_date(struct kone_device *kone,
> case kone_mouse_event_switch_profile:
> kone->actual_dpi = kone->profiles[event->value - 1].
> startup_dpi;
> + /* fall through */
> case kone_mouse_event_osd_profile:
> kone->actual_profile = event->value;
> break;
Stefan, could you please confirm that this is intended behavior?
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH v4 5/9] platform/chrome: Add sysfs attributes
From: Enric Balletbo Serra @ 2019-02-19 16:04 UTC (permalink / raw)
To: Nick Crews
Cc: linux-kernel, Simon Glass, Dmitry Torokhov, Sebastian Reichel,
linux-input, Guenter Roeck, dlaurie, Duncan Laurie, Nick Crews,
Enric Balletbo i Serra, Benson Leung
In-Reply-To: <20190123183325.92946-6-ncrews@chromium.org>
Hi Nick,
Missatge de Nick Crews <ncrews@chromium.org> del dia dc., 23 de gen.
2019 a les 19:38:
>
> From: Duncan Laurie <dlaurie@google.com>
>
> Add some sample sysfs attributes for the Wilco EC that show how
> the mailbox interface works. "Legacy" attributes are those that
> existed in the EC before it was adapted to ChromeOS.
>
> > cat /sys/bus/platform/devices/GOOG000C\:00/version
> Label : 99.99.99
> SVN Revision : 738ed.99
> Model Number : 08;8
> Build Date : 08/30/18
>
Note that example is not correct with the new attributes.
> Signed-off-by: Duncan Laurie <dlaurie@google.com>
> Signed-off-by: Nick Crews <ncrews@chromium.org>
> ---
>
> Changes in v4:
> - Move "Add RTC driver" before "Add sysfs attributes" so that
> it could get accepted earlier, since it is less contentious
>
> Changes in v3:
> - explicitly define toplevel_groups from the start,
> so adding telem later makes sense
> - Break version attribute into individual attributes
> - rm unused WILCO_EC_ATTR_RW macro
> - Moved some #defines from legacy.h to legacy.c
>
> Changes in v2:
> - Remove license boiler plate
> - Remove "wilco_ec_sysfs -" docstring prefix
> - Fix accidental Makefile deletion
> - Add documentation for sysfs entries
> - Change "enable ? 0 : 1" to "!enable"
> - No longer override error code from sysfs_init()
> - Put attributes in the legacy file to begin with, don't move later
> - Remove duplicate error messages when init()ing sysfs
>
> .../ABI/testing/sysfs-platform-wilco-ec | 42 +++++++
> drivers/platform/chrome/wilco_ec/Makefile | 2 +-
> drivers/platform/chrome/wilco_ec/core.c | 12 ++
> drivers/platform/chrome/wilco_ec/legacy.c | 103 ++++++++++++++++++
> drivers/platform/chrome/wilco_ec/legacy.h | 79 ++++++++++++++
> drivers/platform/chrome/wilco_ec/sysfs.c | 79 ++++++++++++++
> include/linux/platform_data/wilco-ec.h | 14 +++
> 7 files changed, 330 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/ABI/testing/sysfs-platform-wilco-ec
> create mode 100644 drivers/platform/chrome/wilco_ec/legacy.c
> create mode 100644 drivers/platform/chrome/wilco_ec/legacy.h
> create mode 100644 drivers/platform/chrome/wilco_ec/sysfs.c
>
> diff --git a/Documentation/ABI/testing/sysfs-platform-wilco-ec b/Documentation/ABI/testing/sysfs-platform-wilco-ec
> new file mode 100644
> index 000000000000..fd2400844470
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-platform-wilco-ec
> @@ -0,0 +1,42 @@
> +What: /sys/bus/platform/devices/GOOG000C\:00/version_label
> +Date: January 2019
> +KernelVersion: 4.19
> +Description:
> + Display Wilco Embedded Controller firmware version label.
> + Output will a version string be similar to the example below:
> + 95.00.06
> +
> +What: /sys/bus/platform/devices/GOOG000C\:00/version_svn_revision
nit: don't know why but svn always reminds me to subversion ... I see
that you always add the prefix version_, I assume this is to maintain
in some way the old directory structure but we cleaned a lot the
number of sysfs attributes, so makes sense always add the prefix
version_? On some cases looks weird to me. Why not just 'revision'
it's short an clear.
> +Date: January 2019
> +KernelVersion: 4.19
> +Description:
> + Display Wilco Embedded Controller SVN revision.
> + Output will a version string be similar to the example below:
> + 5960a.06
> +
> +What: /sys/bus/platform/devices/GOOG000C\:00/version_model_number
nit: model_number?
> +Date: January 2019
> +KernelVersion: 4.19
> +Description:
> + Display Wilco Embedded Controller model number.
> + Output will a version string be similar to the example below:
> + 08;8
Which format is this? DD/MM?
> +
> +What: /sys/bus/platform/devices/GOOG000C\:00/version_build_date
nit: build_date?
> +Date: January 2019
> +KernelVersion: 4.19
> +Description:
> + Display Wilco Embedded Controller firmware build date.
> + Output will a MM/DD/YY string.
> +
Should match with the above example
> +What: /sys/bus/platform/devices/GOOG000C\:00/stealth_mode
I guess that this should go on a separate patch for now, please.
> +Date: January 2019
> +KernelVersion: 4.19
> +Description:
> + Turn stealth_mode on or off on EC. Stealth mode means that the
> + various LEDs, the LCD backlight, and onboard speakers are turned
> + off and remain off even if activated from the off state.
> + External monitors connected to the system are not affected.
> + In addition Wireless devices are turned off.
> +
I understand correctly that's a power management feature? What's the
use case from userspace point of view?
> + Input should be parseable by kstrtobool().
> diff --git a/drivers/platform/chrome/wilco_ec/Makefile b/drivers/platform/chrome/wilco_ec/Makefile
> index 063e7fb4ea17..b4dadf8b1a07 100644
> --- a/drivers/platform/chrome/wilco_ec/Makefile
> +++ b/drivers/platform/chrome/wilco_ec/Makefile
> @@ -1,6 +1,6 @@
> # SPDX-License-Identifier: GPL-2.0
>
> -wilco_ec-objs := core.o mailbox.o
> +wilco_ec-objs := core.o mailbox.o sysfs.o legacy.o
> obj-$(CONFIG_WILCO_EC) += wilco_ec.o
> wilco_ec_debugfs-objs := debugfs.o
> obj-$(CONFIG_WILCO_EC_DEBUGFS) += wilco_ec_debugfs.o
> diff --git a/drivers/platform/chrome/wilco_ec/core.c b/drivers/platform/chrome/wilco_ec/core.c
> index 7cfb047e2c89..1a1cd8e59f3f 100644
> --- a/drivers/platform/chrome/wilco_ec/core.c
> +++ b/drivers/platform/chrome/wilco_ec/core.c
> @@ -101,6 +101,13 @@ static int wilco_ec_probe(struct platform_device *pdev)
> goto destroy_mec;
> }
>
> + /* Create sysfs attributes for userspace interaction */
> + ret = wilco_ec_sysfs_init(ec);
> + if (ret) {
> + dev_err(dev, "Failed to create sysfs attributes\n");
> + goto destroy_mec;
> + }
> +
> return 0;
>
> destroy_mec:
> @@ -110,6 +117,11 @@ static int wilco_ec_probe(struct platform_device *pdev)
>
> static int wilco_ec_remove(struct platform_device *pdev)
> {
> + struct wilco_ec_device *ec = platform_get_drvdata(pdev);
> +
> + /* Remove sysfs attributes */
> + wilco_ec_sysfs_remove(ec);
> +
> /* Teardown cros_ec interface */
> cros_ec_lpc_mec_destroy();
>
> diff --git a/drivers/platform/chrome/wilco_ec/legacy.c b/drivers/platform/chrome/wilco_ec/legacy.c
> new file mode 100644
> index 000000000000..f284088e70f4
> --- /dev/null
> +++ b/drivers/platform/chrome/wilco_ec/legacy.c
> @@ -0,0 +1,103 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Legacy (non-Chrome-specific) sysfs attributes for Wilco EC
> + *
> + * Copyright 2018 Google LLC
> + */
> +
> +#include <linux/ctype.h>
> +#include <linux/device.h>
> +#include <linux/platform_data/wilco-ec.h>
> +
> +#include "legacy.h"
> +
> +#define EC_COMMAND_EC_INFO 0x38
> +#define EC_INFO_SIZE 9
> +#define EC_COMMAND_STEALTH_MODE 0xfc
> +
> +#define VERSION_INDEX_LABEL 0
> +#define VERSION_INDEX_SVN_REVISION 1
> +#define VERSION_INDEX_MODEL_NUMBER 2
> +#define VERSION_INDEX_BUILD_DATE 3
> +
> +static ssize_t get_info(struct device *dev, char *buf, u8 index)
> +{
> + struct wilco_ec_device *ec = dev_get_drvdata(dev);
> + char result[EC_INFO_SIZE];
> + struct wilco_ec_message msg = {
> + .type = WILCO_EC_MSG_LEGACY,
> + .command = EC_COMMAND_EC_INFO,
> + .request_data = &index,
> + .request_size = sizeof(index),
> + .response_data = result,
> + .response_size = EC_INFO_SIZE,
> + };
> + int ret;
> +
> + ret = wilco_ec_mailbox(ec, &msg);
> + if (ret < 0)
> + return ret;
> + if (ret != EC_INFO_SIZE)
> + return -EBADMSG;
> +
> + return scnprintf(buf, PAGE_SIZE, "%s\n", result);
> +}
> +
> +ssize_t wilco_ec_version_label_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + return get_info(dev, buf, VERSION_INDEX_LABEL);
> +}
> +
> +ssize_t wilco_ec_version_svn_revision_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + return get_info(dev, buf, VERSION_INDEX_SVN_REVISION);
> +}
> +
> +ssize_t wilco_ec_version_model_number_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + return get_info(dev, buf, VERSION_INDEX_MODEL_NUMBER);
> +}
> +
> +ssize_t wilco_ec_version_build_date_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + return get_info(dev, buf, VERSION_INDEX_BUILD_DATE);
> +}
> +
> +ssize_t wilco_ec_stealth_mode_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct wilco_ec_device *ec = dev_get_drvdata(dev);
> + u8 param;
> + struct wilco_ec_message msg = {
> + .type = WILCO_EC_MSG_LEGACY,
> + .command = EC_COMMAND_STEALTH_MODE,
> + .request_data = ¶m,
> + .request_size = sizeof(param),
> + };
> + int ret;
> + bool enable;
> +
> + ret = kstrtobool(buf, &enable);
> + if (ret) {
> + dev_err(dev, "Unable to parse '%s' to bool", buf);
> + return ret;
> + }
> +
> + /* Invert input parameter, EC expects 0=on and 1=off */
> + param = !enable;
> +
> + ret = wilco_ec_mailbox(ec, &msg);
> + if (ret < 0)
> + return ret;
> +
> + return count;
> +}
> diff --git a/drivers/platform/chrome/wilco_ec/legacy.h b/drivers/platform/chrome/wilco_ec/legacy.h
> new file mode 100644
> index 000000000000..a7f51dec4ccb
> --- /dev/null
> +++ b/drivers/platform/chrome/wilco_ec/legacy.h
> @@ -0,0 +1,79 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Legacy (non-Chrome-specific) sysfs attributes for Wilco EC
> + *
> + * Copyright 2018 Google LLC
> + */
> +
> +#ifndef WILCO_EC_LEGACY_H
> +#define WILCO_EC_LEGACY_H
> +
> +#include <linux/device.h>
> +
> +/**
> + * wilco_ec_version_label_show() - Display Wilco EC version label
> + * @dev: The device underlying the struct wilco_ec_device
> + * @attr: The attribute being read from
> + * @buf: Output buffer to fill with the result
> + *
> + * Output will a version string be similar to the example below:
> + * 95.00.06
> + */
> +ssize_t wilco_ec_version_label_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf);
> +
> +/**
> + * wilco_ec_version_svn_revision_show() - Display Wilco EC SVN revision
> + * @dev: The device underlying the struct wilco_ec_device
> + * @attr: The attribute being read from
> + * @buf: Output buffer to fill with the result
> + *
> + * Output will a version string be similar to the example below:
> + * 5960a.06
> + */
> +ssize_t wilco_ec_version_svn_revision_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf);
> +
> +/**
> + * wilco_ec_version_model_number_show() - Display Wilco EC model number
> + * @dev: The device underlying the struct wilco_ec_device
> + * @attr: The attribute being read from
> + * @buf: Output buffer to fill with the result
> + *
> + * Output will a version string be similar to the example below:
> + * 08;8
> + */
> +ssize_t wilco_ec_version_model_number_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf);
> +
> +/**
> + * wilco_ec_version_build_date_show() - Display Wilco EC firmware build date
> + * @dev: The device underlying the struct wilco_ec_device
> + * @attr: The attribute being read from
> + * @buf: Output buffer to fill with the result
> + *
> + * Output will a MM/DD/YY string.
> + */
> +ssize_t wilco_ec_version_build_date_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf);
> +
> +
> +/**
> + * wilco_ec_stealth_mode_store() - Turn stealth_mode on or off on EC
> + * @dev: Device representing the EC
> + * @attr: The attribute in question
> + * @buf: Input buffer, should be parseable by kstrtobool(). Anything parsed to
> + * True means enable stealth mode (turn off screen, etc)
> + * @count: Number of bytes in input buffer
> + *
> + * Return: Number of bytes consumed from input, negative error code on failure
> + */
> +ssize_t wilco_ec_stealth_mode_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count);
> +
> +#endif /* WILCO_EC_LEGACY_H */
> diff --git a/drivers/platform/chrome/wilco_ec/sysfs.c b/drivers/platform/chrome/wilco_ec/sysfs.c
> new file mode 100644
> index 000000000000..a885026e5d24
> --- /dev/null
> +++ b/drivers/platform/chrome/wilco_ec/sysfs.c
> @@ -0,0 +1,79 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Sysfs attributes for Wilco Embedded Controller
> + *
> + * Copyright 2018 Google LLC
> + *
> + * The sysfs attributes appear under /sys/bus/platform/devices/GOOG000C\:00/
> + * To actually learn what each attribute does, read the corresponding _show() or
> + * _store() function source.
> + */
> +
> +#include <linux/ctype.h>
> +#include <linux/platform_data/wilco-ec.h>
> +#include <linux/platform_device.h>
> +#include <linux/sysfs.h>
> +
> +#include "legacy.h"
> +
> +#define WILCO_EC_ATTR_RO(_name) \
> +__ATTR(_name, 0444, wilco_ec_##_name##_show, NULL)
> +
This is only to add the wilco_ec_ prefix to the attributes, I'd prefer
you use the standard ATTR_ attibutes and just name the attributes
without the wilco_ec_ prefix.
> +#define WILCO_EC_ATTR_WO(_name) \
> +__ATTR(_name, 0200, NULL, wilco_ec_##_name##_store)
> +
ditto
> +/* Make top-level attributes, which will live inside GOOG000C:00/ */
> +static struct device_attribute stealth_attr = WILCO_EC_ATTR_WO(stealth_mode);
> +static struct device_attribute version_label_attr =
> + WILCO_EC_ATTR_RO(version_label);
> +static struct device_attribute version_svn_revision_attr =
> + WILCO_EC_ATTR_RO(version_svn_revision);
> +static struct device_attribute version_model_number_attr =
> + WILCO_EC_ATTR_RO(version_model_number);
> +static struct device_attribute version_build_date_attr =
> + WILCO_EC_ATTR_RO(version_build_date);
> +static struct attribute *wilco_ec_toplevel_attrs[] = {
> + &version_label_attr.attr,
> + &version_svn_revision_attr.attr,
> + &version_model_number_attr.attr,
> + &version_build_date_attr.attr,
> + &stealth_attr.attr,
> + NULL
> +};
> +static const struct attribute_group wilco_ec_toplevel_group = {
> + .attrs = wilco_ec_toplevel_attrs,
> +};
> +static const struct attribute_group *wilco_ec_toplevel_groups[] = {
> + &wilco_ec_toplevel_group,
> + NULL,
> +};
> +
> +/**
> + * wilco_ec_sysfs_init() - Initialize the sysfs directories and attributes
> + * @dev: The device representing the EC
> + *
> + * Creates the sysfs directory structure and populates it with all attributes.
> + * If there is a problem it will clean up the entire filesystem.
> + *
> + * Return 0 on success, -ENOMEM on failure creating directories or attibutes.
> + */
> +int wilco_ec_sysfs_init(struct wilco_ec_device *ec)
> +{
> + struct device *dev = ec->dev;
> + int ret;
> +
> + /* add the top-level attributes */
> + ret = sysfs_create_groups(&dev->kobj, wilco_ec_toplevel_groups);
> + if (ret)
> + return -ENOMEM;
> +
> + return 0;
> +}
> +
> +void wilco_ec_sysfs_remove(struct wilco_ec_device *ec)
> +{
> + struct device *dev = ec->dev;
> +
> + /* go upwards through the directory structure */
> + sysfs_remove_groups(&dev->kobj, wilco_ec_toplevel_groups);
> +}
> diff --git a/include/linux/platform_data/wilco-ec.h b/include/linux/platform_data/wilco-ec.h
> index 5477b8802f81..7c6ab6de7239 100644
> --- a/include/linux/platform_data/wilco-ec.h
> +++ b/include/linux/platform_data/wilco-ec.h
> @@ -135,4 +135,18 @@ struct wilco_ec_response {
> */
> int wilco_ec_mailbox(struct wilco_ec_device *ec, struct wilco_ec_message *msg);
>
> +/**
> + * wilco_ec_sysfs_init() - Create sysfs attributes.
> + * @ec: EC device.
> + *
> + * Return: 0 for success or negative error code on failure.
> + */
> +int wilco_ec_sysfs_init(struct wilco_ec_device *ec);
> +
> +/**
> + * wilco_ec_sysfs_remove() - Remove sysfs attributes.
> + * @ec: EC device.
> + */
> +void wilco_ec_sysfs_remove(struct wilco_ec_device *ec);
> +
> #endif /* WILCO_EC_H */
> --
> 2.20.1.321.g9e740568ce-goog
>
Regards,
Enric
^ permalink raw reply
* Re: [PATCH] input : avoid too late kobject_uevent(KOBJ_REMOVE) call
From: Dmitry Torokhov @ 2019-02-19 18:55 UTC (permalink / raw)
To: Tetsuo Handa
Cc: rydberg, linux-input, linux-kernel, syzkaller-bugs, Kay Sievers,
syzbot
In-Reply-To: <1550484563-13217-1-git-send-email-penguin-kernel@I-love.SAKURA.ne.jp>
Hi Tetsuo,
On Mon, Feb 18, 2019 at 07:09:23PM +0900, Tetsuo Handa wrote:
> 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));
Input has no business reaching into kobj internals. This should be
solved in lib/kobject_uevent.c, not anywhere else.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH (resend)] Input: uinput - Set name/phys to NULL before kfree().
From: Dmitry Torokhov @ 2019-02-19 18:58 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: rydberg, syzbot, linux-input, linux-kernel, syzkaller-bugs
In-Reply-To: <722c14a1-78cd-14b6-59ef-ba0d6fc82cb1@i-love.sakura.ne.jp>
On Mon, Feb 18, 2019 at 07:10:23PM +0900, Tetsuo Handa wrote:
> Thank you for responding.
>
> On 2019/02/18 6:07, Dmitry Torokhov wrote:
> > 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.
>
> Then, we want to keep dev->name and dev->phys when calling "unregister" 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.
>
> If we want to keep dev->name and dev->phys when calling "unregister" time,
> we could do something like below. Does calling kobject_uevent(KOBJ_REMOVE)
> without dev->name and dev->phys (to some degree) help (compared to not
> triggering kobject_uevent(KOBJ_REMOVE) at all) ?
We are talking about handling pretty bad failure (I am not sure if these
allocations can fail in real life) so not getting KOBJ_REMOVE uevent is
not a big deal.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] HID: roccat: Mark expected switch fall-through
From: Stefan Achatz @ 2019-02-20 8:15 UTC (permalink / raw)
To: Jiri Kosina, Gustavo A. R. Silva
Cc: Stefan Achatz, Benjamin Tissoires, linux-input, linux-kernel,
Kees Cook
In-Reply-To: <nycvar.YFH.7.76.1902191415550.11598@cbobk.fhfr.pm>
Am Dienstag, den 19.02.2019, 14:16 +0100 schrieb Jiri Kosina:
> On Mon, 11 Feb 2019, Gustavo A. R. Silva wrote:
>
> > In preparation to enabling -Wimplicit-fallthrough, mark switch
> > cases where we are expecting to fall through.
> >
> > This patch fixes the following warning:
> >
> > drivers/hid/hid-roccat-kone.c: In function
> > ‘kone_keep_values_up_to_date’:
> > drivers/hid/hid-roccat-kone.c:784:20: warning: this statement may
> > fall through [-Wimplicit-fallthrough=]
> > kone->actual_dpi = kone->profiles[event->value - 1].
> > ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > startup_dpi;
> > ~~~~~~~~~~~
> > drivers/hid/hid-roccat-kone.c:786:2: note: here
> > case kone_mouse_event_osd_profile:
> > ^~~~
> >
> > Warning level 3 was used: -Wimplicit-fallthrough=3
> >
> > This patch is part of the ongoing efforts to enable
> > -Wimplicit-fallthrough.
> >
> > Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> > ---
> > drivers/hid/hid-roccat-kone.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/hid/hid-roccat-kone.c b/drivers/hid/hid-
> > roccat-kone.c
> > index bf4675a27396..c4dd6162c1d6 100644
> > --- a/drivers/hid/hid-roccat-kone.c
> > +++ b/drivers/hid/hid-roccat-kone.c
> > @@ -783,6 +783,7 @@ static void kone_keep_values_up_to_date(struct
> > kone_device *kone,
> > case kone_mouse_event_switch_profile:
> > kone->actual_dpi = kone->profiles[event->value -
> > 1].
> > startup_dpi;
> > + /* fall through */
> > case kone_mouse_event_osd_profile:
> > kone->actual_profile = event->value;
> > break;
>
> Stefan, could you please confirm that this is intended behavior?
>
> Thanks,
I already confirmed this 7 months ago but as I see only in a reply to
you.
Yes, this patch is correct.
Stefan
^ permalink raw reply
* Re: [PATCH] HID: roccat: Mark expected switch fall-through
From: Jiri Kosina @ 2019-02-20 8:40 UTC (permalink / raw)
To: Stefan Achatz
Cc: Gustavo A. R. Silva, Stefan Achatz, Benjamin Tissoires,
linux-input, linux-kernel, Kees Cook
In-Reply-To: <1550650517.5256.2.camel@web.de>
On Wed, 20 Feb 2019, Stefan Achatz wrote:
> I already confirmed this 7 months ago but as I see only in a reply to
> you.
I guess that fell in between cracks somewhere.
> Yes, this patch is correct.
Applied, thanks.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox