Linux Input/HID development
 help / color / mirror / Atom feed
* 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 &gt1x_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
* [PATCH v3 2/3] dt-bindings: input: Refer to touchscreen.txt for goodix ts
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>

Refer to touchscreen.txt for generic touch properties. This avoids
duplication and we're using the generic code to parse these in the
driver. While at that add touchscreen-size-{x,y} which are respected by
the driver as well.

Signed-off-by: Guido Günther <agx@sigxcpu.org>
---
 .../bindings/input/touchscreen/goodix.txt           | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
index 57d3d8870a09..8cf0b4d38a7e 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
@@ -19,11 +19,14 @@ Optional properties:
  - irq-gpios		: GPIO pin used for IRQ. The driver uses the
 			  interrupt gpio pin as output to reset the device.
  - reset-gpios		: GPIO pin used for reset
-
- - touchscreen-inverted-x  : X axis is inverted (boolean)
- - 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)
+ - touchscreen-inverted-x
+ - touchscreen-inverted-y
+ - touchscreen-size-x
+ - touchscreen-size-y
+ - touchscreen-swapped-x-y
+
+The touchscreen-* properties are documented in touchscreen.txt in this
+directory.
 
 Example:
 
-- 
2.20.1

^ permalink raw reply related
* [PATCH v3 1/3] input: goodix - support Goodix gt5688
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>

>From what I've seen in vendor trees it's fine to treat this as gt1x¹.
Tested on the Purism Librem 5 Devkit (Rocktech JH057N00900 panel).

[1]: https://github.com/TadiT7/android_kernel_mtk-4.4/tree/master/drivers/input/touchscreen/mediatek/GT5688

Signed-off-by: Guido Günther <agx@sigxcpu.org>
Reviewed-by: Bastien Nocera <hadess@hadess.net>
---
 Documentation/devicetree/bindings/input/touchscreen/goodix.txt | 1 +
 drivers/input/touchscreen/goodix.c                             | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
index f7e95c52f3c7..57d3d8870a09 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,gt5688"
 				 or "goodix,gt911"
 				 or "goodix,gt9110"
 				 or "goodix,gt912"
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index f2d9c2c41885..47b1ced41576 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -216,6 +216,7 @@ static const struct goodix_chip_data *goodix_get_chip_data(u16 id)
 {
 	switch (id) {
 	case 1151:
+	case 5688:
 		return &gt1x_chip_data;
 
 	case 911:
@@ -942,6 +943,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,gt5688" },
 	{ .compatible = "goodix,gt911" },
 	{ .compatible = "goodix,gt9110" },
 	{ .compatible = "goodix,gt912" },
-- 
2.20.1

^ permalink raw reply related
* [PATCH v3 0/3] input: goodix - support Goodix gt5688
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

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'

Changes from v1
* Add tested board to commit message

Guido Günther (3):
  input: goodix - support Goodix gt5688
  dt-bindings: input: Refer to touchscreen.txt for goodix ts
  input: goodix: Print values in case of inconsistencies

 .../bindings/input/touchscreen/goodix.txt          | 14 +++++++++-----
 drivers/input/touchscreen/goodix.c                 |  6 +++++-
 2 files changed, 14 insertions(+), 6 deletions(-)

-- 
2.20.1

^ permalink raw reply
* Re: [PATCH] HID: wacom: Mark expected switch fall-through
From: Jiri Kosina @ 2019-02-15  7:46 UTC (permalink / raw)
  To: Ping Cheng
  Cc: Gustavo A. R. Silva, Benjamin Tissoires, linux-input,
	linux-kernel, Kees Cook, Jason Gerecke, Ping Cheng
In-Reply-To: <CAF8JNh+EzcCoOvyTm59p99Cg_3S4adGcnpKs4QkpZStsU=XURA@mail.gmail.com>

On Thu, 14 Feb 2019, Ping Cheng wrote:

> Yes, it was intended. You can have my Acked-by: Ping Cheng <
> ping.cheng@wacom.com>

Applied, thanks.

-- 
Jiri Kosina
SUSE Labs

^ permalink raw reply
* [PATCH AUTOSEL 4.20 15/77] HID: i2c-hid: Disable runtime PM on Goodix touchpad
From: Sasha Levin @ 2019-02-15  2:07 UTC (permalink / raw)
  To: linux-kernel, stable; +Cc: Kai-Heng Feng, Jiri Kosina, Sasha Levin, linux-input
In-Reply-To: <20190215020855.176727-1-sashal@kernel.org>

From: Kai-Heng Feng <kai.heng.feng@canonical.com>

[ Upstream commit 77ae0d8e401f083ca69c202502da4fc0e38cb1b7 ]

A Goodix touchpad doesn't work. Touching the touchpad can trigger IRQ
but there's no input event from HID subsystem.

Turns out it reports some invalid data:
[   22.136630] i2c_hid i2c-DELL091F:00: input: 0b 00 01 00 00 00 00 00 00 00 00

After some trial and error, it's another device that doesn't work well
with ON/SLEEP commands. Disable runtime PM to fix the issue.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/hid/hid-ids.h              | 3 +++
 drivers/hid/i2c-hid/i2c-hid-core.c | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 27519eb8ee63..81bc2f6b93a3 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -459,6 +459,9 @@
 #define USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PIT_010A 0x010a
 #define USB_DEVICE_ID_GENERAL_TOUCH_WIN8_PIT_E100 0xe100
 
+#define I2C_VENDOR_ID_GOODIX		0x27c6
+#define I2C_DEVICE_ID_GOODIX_01F0	0x01f0
+
 #define USB_VENDOR_ID_GOODTOUCH		0x1aad
 #define USB_DEVICE_ID_GOODTOUCH_000f	0x000f
 
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 8555ce7e737b..c5edfa966343 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -179,6 +179,8 @@ static const struct i2c_hid_quirks {
 		I2C_HID_QUIRK_DELAY_AFTER_SLEEP },
 	{ USB_VENDOR_ID_LG, I2C_DEVICE_ID_LG_8001,
 		I2C_HID_QUIRK_NO_RUNTIME_PM },
+	{ I2C_VENDOR_ID_GOODIX, I2C_DEVICE_ID_GOODIX_01F0,
+		I2C_HID_QUIRK_NO_RUNTIME_PM },
 	{ 0, 0 }
 };
 
-- 
2.19.1

^ permalink raw reply related
* Re: [PATCH] HID: wacom: Mark expected switch fall-through
From: Gustavo A. R. Silva @ 2019-02-14 23:06 UTC (permalink / raw)
  To: Ping Cheng, Jiri Kosina
  Cc: Benjamin Tissoires, linux-input, linux-kernel, Kees Cook,
	Jason Gerecke, Ping Cheng
In-Reply-To: <CAF8JNh+EzcCoOvyTm59p99Cg_3S4adGcnpKs4QkpZStsU=XURA@mail.gmail.com>



On 2/14/19 4:33 PM, Ping Cheng wrote:
> On Thu, Feb 14, 2019 at 6:01 AM Jiri Kosina <jikos@kernel.org> wrote:
> 
>> 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/wacom_wac.c: In function
>> ‘wacom_setup_pen_input_capabilities’:
>>> drivers/hid/wacom_wac.c:3506:3: warning: this statement may fall through
>> [-Wimplicit-fallthrough=]
>>>    __clear_bit(ABS_MISC, input_dev->absbit);
>>>    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> drivers/hid/wacom_wac.c:3508:2: note: here
>>>   case WACOM_MO:
>>>   ^~~~
>>>
>>> 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>
>>
>> Let's CC Jason and Ping to make sure it's really intended :)
>>
> 
> Yes, it was intended. You can have my Acked-by: Ping Cheng <
> ping.cheng@wacom.com>
> 

Great.

Thanks, Ping.

--
Gustavo

> Cheers,
> Ping
> 
> 
>>
>>> ---
>>>  drivers/hid/wacom_wac.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
>>> index 72477e872324..5ecda99bf431 100644
>>> --- a/drivers/hid/wacom_wac.c
>>> +++ b/drivers/hid/wacom_wac.c
>>> @@ -3504,6 +3504,7 @@ int wacom_setup_pen_input_capabilities(struct
>> input_dev *input_dev,
>>>       switch (features->type) {
>>>       case GRAPHIRE_BT:
>>>               __clear_bit(ABS_MISC, input_dev->absbit);
>>> +             /* fall through */
>>>
>>>       case WACOM_MO:
>>>       case WACOM_G4:
>>> --
>>> 2.20.1
>>>
>>
>> --
>> Jiri Kosina
>> SUSE Labs
>>
>>
> 

^ permalink raw reply
* Re: [PATCH v5 02/11] dt-bindings: power: supply: add DT bindings for max77650
From: Rob Herring @ 2019-02-14 14:18 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: Mark Rutland, Linus Walleij, Dmitry Torokhov, Jacek Anaszewski,
	Pavel Machek, Lee Jones, Sebastian Reichel, Liam Girdwood,
	Greg Kroah-Hartman, Linux Kernel Mailing List,
	open list:GPIO SUBSYSTEM, devicetree, Linux Input,
	Linux LED Subsystem, Linux PM list, Bartosz Golaszewski
In-Reply-To: <CAMRc=MeyMnRMzmexgNBF5PkP-Hs2G2sS4hCP4mTZR5jxC=ej1w@mail.gmail.com>

On Thu, Feb 14, 2019 at 3:29 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> śr., 13 lut 2019 o 21:48 Rob Herring <robh@kernel.org> napisał(a):
> >
> > On Wed, Feb 13, 2019 at 02:43:26PM +0100, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > Add the DT binding document for the battery charger module of max77650.
> > >
> > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > ---
> > >  .../power/supply/max77650-charger.txt         | 27 +++++++++++++++++++
> > >  1 file changed, 27 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/power/supply/max77650-charger.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/power/supply/max77650-charger.txt b/Documentation/devicetree/bindings/power/supply/max77650-charger.txt
> > > new file mode 100644
> > > index 000000000000..16e24173c80e
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/power/supply/max77650-charger.txt
> > > @@ -0,0 +1,27 @@
> > > +Battery charger driver for MAX77650 PMIC from Maxim Integrated.
> > > +
> > > +This module is part of the MAX77650 MFD device. For more details
> > > +see Documentation/devicetree/bindings/mfd/max77650.txt.
> > > +
> > > +The charger is represented as a sub-node of the PMIC node on the device tree.
> > > +
> > > +Required properties:
> > > +--------------------
> > > +- compatible:                Must be "maxim,max77650-charger"
> > > +
> > > +Optional properties:
> > > +--------------------
> > > +- min-input-voltage: Minimum CHGIN regulation voltage (in microvolts). Must be
> > > +                     one of: 4000000, 4100000, 4200000, 4300000, 4400000,
> > > +                     4500000, 4600000, 4700000.
> > > +- current-limit:     CHGIN input current limit (in microamps). Must be one of:
> > > +                     95000, 190000, 285000, 380000, 475000.
> >
> > Unit suffix? Really, these should be common properties. We have
> > some I think. Charger bindings are a mess though...
> >
>
> Do you mean a suffix like: ua for microamps and uv for microvolts? I'm
> not seeing any other such properties.

See bindings/property-units.txt

Rob

^ permalink raw reply
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox