* [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 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 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 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
* 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
* 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 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] 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 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 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 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 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 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 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] 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 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 (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
* [PATCH] input : avoid too late kobject_uevent(KOBJ_REMOVE) call
From: Tetsuo Handa @ 2019-02-18 10:09 UTC (permalink / raw)
To: dmitry.torokhov, rydberg
Cc: linux-input, linux-kernel, syzkaller-bugs, Tetsuo Handa,
Kay Sievers, syzbot
syzbot is hitting use-after-free bug in uinput module [1]. This is because
kobject_uevent(KOBJ_REMOVE) is called again due to commit 0f4dafc0563c6c49
("Kobject: auto-cleanup on final unref") after memory allocation fault
injection made kobject_uevent(KOBJ_REMOVE) from device_del() from
input_unregister_device() fail, while uinput_destroy_device() is expecting
that kobject_uevent(KOBJ_REMOVE) is not called after device_del() from
input_unregister_device() completed.
Fix this problem by pretending as if kobject_uevent(KOBJ_REMOVE) from
device_del() from input_unregister_device() did not fail.
[1] https://syzkaller.appspot.com/bug?id=8b17c134fe938bbddd75a45afaa9e68af43a362d
Reported-by: syzbot <syzbot+f648cfb7e0b52bf7ae32@syzkaller.appspotmail.com>
Analyzed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Kay Sievers <kay@vrfy.org>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
drivers/input/input.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/drivers/input/input.c b/drivers/input/input.c
index 3304aaaffe87..6df3c33ef3aa 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -2032,6 +2032,19 @@ static void __input_unregister_device(struct input_dev *dev)
mutex_unlock(&input_mutex);
device_del(&dev->dev);
+ /*
+ * Regarding input subsystem, we always take care of sending uevent at
+ * "unregister" time, and we do not expect to have uevent sent out at
+ * the final "put" time. Therefore, if we failed to send uevent at
+ * "unregister" time (due to e.g. fault injection), complain it and
+ * do not allow the final "put" time to send the remove uevent again.
+ */
+ if (dev->dev.kobj.state_add_uevent_sent &&
+ !dev->dev.kobj.state_remove_uevent_sent) {
+ dev->dev.kobj.state_remove_uevent_sent = 1;
+ pr_warn("Failed to send remove uevent for %s\n",
+ dev_name(&dev->dev));
+ }
}
static void devm_input_device_unregister(struct device *dev, void *res)
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v3 1/4] dt-bindings: input: touchscreen: goodix: Document AVDD28-supply property
From: Jagan Teki @ 2019-02-18 9:33 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Rob Herring, devicetree, Chen-Yu Tsai, linux-input,
linux-kernel@vger.kernel.org, Michael Trimarchi, linux-amarula
In-Reply-To: <20190218071207.GE242714@dtor-ws>
Hi Dmitry,
On Mon, Feb 18, 2019 at 12:42 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Sun, Feb 17, 2019 at 02:45:52PM +0530, Jagan Teki wrote:
> > On Sun, Feb 17, 2019 at 1:21 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > >
> > > Hi Dmitry and Rob,
> > >
> > > On Thu, Feb 14, 2019 at 3:21 AM Rob Herring <robh@kernel.org> wrote:
> > > >
> > > > On Tue, Jan 22, 2019 at 7:39 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > > > >
> > > > > On Mon, Jan 21, 2019 at 9:46 PM Rob Herring <robh@kernel.org> wrote:
> > > > > >
> > > > > > On Fri, Jan 18, 2019 at 10:01 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > > > > > >
> > > > > > > On Wed, Jan 9, 2019 at 7:08 PM Rob Herring <robh@kernel.org> wrote:
> > > > > > > >
> > > > > > > > Please CC DT list if you want bindings reviewed.
> > > > > > >
> > > > > > > Sorry I forgot.
> > > > > > >
> > > > > > > >
> > > > > > > > On Wed, Jan 9, 2019 at 1:40 AM Dmitry Torokhov
> > > > > > > > <dmitry.torokhov@gmail.com> wrote:
> > > > > > > > >
> > > > > > > > > On Sat, Dec 15, 2018 at 08:47:59PM +0530, Jagan Teki wrote:
> > > > > > > > > > Most of the Goodix CTP controllers are supply with AVDD28 pin.
> > > > > > > > > > which need to supply for controllers like GT5663 on some boards
> > > > > > > > > > to trigger the power.
> > > > > > > > > >
> > > > > > > > > > So, document the supply property so-that the require boards
> > > > > > > > > > that used on GT5663 can enable it via device tree.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > > > > > > > ---
> > > > > > > > > > Documentation/devicetree/bindings/input/touchscreen/goodix.txt | 1 +
> > > > > > > > > > 1 file changed, 1 insertion(+)
> > > > > > > > > >
> > > > > > > > > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> > > > > > > > > > index f7e95c52f3c7..c4622c983e08 100644
> > > > > > > > > > --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> > > > > > > > > > +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> > > > > > > > > > @@ -23,6 +23,7 @@ Optional properties:
> > > > > > > > > > - touchscreen-inverted-y : Y axis is inverted (boolean)
> > > > > > > > > > - touchscreen-swapped-x-y : X and Y axis are swapped (boolean)
> > > > > > > > > > (swapping is done after inverting the axis)
> > > > > > > > > > + - AVDD28-supply : Analog power supply regulator on AVDD28 pin
> > > > > > > > >
> > > > > > > > > I think we normally use lower case in DT bindings and rarely encode
> > > > > > > > > voltage in the supply name unless we are dealing with several supplies
> > > > > > > > > of the same kind, but I'll let Ron comment on this.
> > > > > > > >
> > > > > > > > Yes on lowercase though there are some exceptions.
> > > > > > > >
> > > > > > > > There's also a AVDD22 supply as well as DVDD12 and VDDIO. So we
> > > > > > > > probably need to keep the voltage, but the binding is incomplete.
> > > > > > >
> > > > > > > What is incomplete here? can you please elaborate.
> > > > > >
> > > > > > You are missing the 3 other supplies the chip has: AVDD22, DVDD12 and VDDIO.
> > > > >
> > > > > Though it has other supplies, only AVDD28 is connected in the Host
> > > > > interface design similar like 9. Reference Schematic on Page, 23 in
> > > > > attached manual.
> > > >
> > > > That is for a particular board design. It still has other supplies.
> > > > Just make the binding complete please. You can make them optional. I
> > > > don't care if the driver supports controlling all the supplies or not
> > > > (though Dmitry might).
> > >
> > > So, Can I make bulk get and enable in all 4 regulators global to
> > > driver or specific to that chip, in either way the regulators which
> > > are not used to process via dummy regulators (which we all know).
> > >
> > > or regulator which are not using are get via devm_regulator_get_optional.
> > >
> > > Any suggestions?
> >
> > Just realized to go with bulk calls, please have a look at on v5 [1]
>
> I do not believe you can use bulk regulator API here as you need to
> implement the power up timing scheme mentioned on p. 12 of the spec you
> referenced earlier. And, because of that timing diagram, I believe if
> you want to control AVDD supply, you also have to control VDDIO supply
> as it has to be on only after enabling AVDD.
AVDD here is AVDD28 it' is an out name of pin (mentioned in p. 23). So
the power scheme goes to enable AVDD28 and then VDDIO does that really
mean bulk of two regulators?
>
> I also wonder if it is valid to have both AVDD28 and AVDD22 connected at
> the same time.
Yes, DVDD12 and AVDD22 not been part of power-up schema at-least from
the datasheet. I'm thinking we can skip these until any of other
goodix CTP needed, what do you think Rob?
^ permalink raw reply
* Re: [PATCH 2/2] Input: add Apple SPI keyboard and trackpad driver.
From: Life is hard, and then you die @ 2019-02-18 9:08 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Dmitry Torokhov, Henrik Rydberg, Lukas Wunner, Federico Lorenzi,
linux-input, linux-kernel
In-Reply-To: <20190216004426.GE9224@smile.fi.intel.com>
On Sat, Feb 16, 2019 at 02:44:26AM +0200, Andy Shevchenko wrote:
> On Sun, Feb 10, 2019 at 03:18:34AM -0800, Life is hard, and then you die wrote:
> > On Wed, Feb 06, 2019 at 10:22:56PM +0200, Andy Shevchenko wrote:
> > > On Mon, Feb 04, 2019 at 12:19:47AM -0800, Ronald Tschalär wrote:
>
> > > > +#define debug_print(mask, fmt, ...) \
> > > > + do { \
> > > > + if (debug & mask) \
> > > > + printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__); \
> > > > + } while (0)
> > > > +
> > > > +#define debug_print_header(mask) \
> > > > + debug_print(mask, "--- %s ---------------------------\n", \
> > > > + applespi_debug_facility(mask))
> > > > +
> > > > +#define debug_print_buffer(mask, fmt, ...) \
> > > > + do { \
> > > > + if (debug & mask) \
> > > > + print_hex_dump(KERN_DEBUG, pr_fmt(fmt), \
> > > > + DUMP_PREFIX_NONE, 32, 1, ##__VA_ARGS__, \
> > > > + false); \
> > > > + } while (0)
> > >
> > > I'm not sure we need all of these... But okay, the driver is
> > > reverse-engineered, perhaps we can drop it later on.
> >
> > These have been extremely useful for debugging; but yes, they are for
> > debugging only. They also explicitly do not use the dynamic-debug
> > facilities because
> > 1. those can't be enabled at a function or line level on the kernel
> > command line (the docs indicate this should work, but it doesn't,
> > or at the very least I've been unable to figure out how, at least
> > for those drivers built as modules)
>
> Hmm... Usually you put either module_name.dyndbg into kernel command line to
> enable Dynamic Debug on built-in modules, or modprobe module_name dyndbg if
> it's built as a module.
Right, but while that works with
applespi.dyndbg=+p
it does not appear to work with
applespi.dyndbg="func applespi_get_spi_settings +p"
(it is parsed correctly, but it just doesn't get applied when the
module is later loaded - I've not tried to chase this down any further
than that)
[snip]
> > > > +static int touchpad_dimensions[4];
> > > > +module_param_array(touchpad_dimensions, int, NULL, 0444);
> > > > +MODULE_PARM_DESC(touchpad_dimensions, "The pixel dimensions of the touchpad, as x_min,x_max,y_min,y_max .");
> > >
> > > We have some parsers for similar data as in format
> > >
> > > %dx%d%+d%+d ?
> > >
> > > For example, 200x100+20+10.
> >
> > Maybe it's just my grep-foo that is lacking, but I couldn't find
> > anything useful. There is some stuff for framebuffer video modes, but
> > that is too different and not really amenable for use here. OTOH, a
> > simple sscanf(buf, "%dx%d+%d+%d", ...) would parse this just fine
> > without the need for any fancier parser.
> >
> > OTOH, I'm not sure this format is any better: internally we need
> > x/y min/max, not x/y/w/h (though obviously the two are trivially
> > convertable); and for the user it doesn't really matter since they would
> > basically be getting the dimensions from running with debug set to
> > 0x10000 and using that output to set this param, i.e. I don't see any
> > inherent advantage of using x/y/w/h.
>
> I would stick with some more or less standard notation. The XxY+W+H is widely
> used in X11 world.
>
> If you have better examples for input devices, choose them. My point that it
> would be better to be a standard to some extent.
I couldn't find anything useful (looked in the kernel and libinput),
so going with XxY+W+H.
[snip]
> > > > + message->counter = applespi->cmd_msg_cntr++ & 0xff;
> > >
> > > Usual pattern for this kind of entries is
> > >
> > > x = ... % 256;
> > >
> > > Btw, isn't 256 is defined somewhere?
> >
> > Many things are defined as have a value of 256 :-) But I didn't find any
> > with the intended semantics; could use (U8_MAX+1), though.
>
> What I meant is that I saw in the same driver definition with value of 256.
> Isn't it about messages?
Ah, right: the packet length is 256 bytes. But the semantics are quite
different, so IMHO it wouldn't be good to use that here. I.e. I think
(U8_MAX+1) is still semantically the best.
[snip]
> > > > +/* lifted from the BCM5974 driver */
> > > > +/* convert 16-bit little endian to signed integer */
> > > > +static inline int raw2int(__le16 x)
> > > > +{
> > > > + return (signed short)le16_to_cpu(x);
> > > > +}
> > >
> > > Perhaps it's time to introduce it inside include/linux/input.h ?
> >
> > Perhaps as
> >
> > static inline int le16_to_signed_int(__le16 x)
> >
> > ? (raw2int seems to ambiguous for a global function)
>
> I'm fine. It would need a description though.
After looking at this in more detail I don't think that
include/linux/input.h is the proper place for this, because input.h
basically represents the interface to the input core, and as such it
is devoid of helpers like above or any driver specific definitions.
Instead I'd like to suggest a new include file specific to the bcm5974
driver, as follows:
--- /dev/null
+++ b/include/linux/input/bcm5974.h
@@ -0,0 +1,27 @@
+/*
+ * Copyright (C) 2008 Henrik Rydberg (rydberg@euromail.se)
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef _BCM5974_H
+#define _BCM5974_H
+
+#include <linux/kernel.h>
+
+/**
+ * raw2int - convert a 16-bit little endian value as received from the device
+ * to a signed integer in cpu endianness.
+ * @x: the received value
+ *
+ * Returns the converted value.
+ */
+static inline int raw2int(__le16 x)
+{
+ return (signed short)le16_to_cpu(x);
+}
+
+#endif
Thoughts?
I'll send out v2 of the patches with all the changes soon.
Cheers,
Ronald
^ permalink raw reply
* Re: [PATCH 2/7] [media] doc-rst: switch to new names for Full Screen/Aspect keys
From: Dmitry Torokhov @ 2019-02-18 7:26 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel,
linux-media
In-Reply-To: <20190118233037.87318-2-dmitry.torokhov@gmail.com>
On Fri, Jan 18, 2019 at 03:30:32PM -0800, Dmitry Torokhov wrote:
> We defined better names for keys to activate full screen mode or
> change aspect ratio (while keeping the existing keycodes to avoid
> breaking userspace), so let's use them in the document.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> Documentation/media/uapi/rc/rc-tables.rst | 4 ++--
Mauro, do you want to take this through your tree or I should pick it up
with the patch that does renames in uapi header?
Thanks!
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/media/uapi/rc/rc-tables.rst b/Documentation/media/uapi/rc/rc-tables.rst
> index c8ae9479f842..57797e56f45e 100644
> --- a/Documentation/media/uapi/rc/rc-tables.rst
> +++ b/Documentation/media/uapi/rc/rc-tables.rst
> @@ -616,7 +616,7 @@ the remote via /dev/input/event devices.
>
> - .. row 78
>
> - - ``KEY_SCREEN``
> + - ``KEY_ASPECT_RATIO``
>
> - Select screen aspect ratio
>
> @@ -624,7 +624,7 @@ the remote via /dev/input/event devices.
>
> - .. row 79
>
> - - ``KEY_ZOOM``
> + - ``KEY_FULL_SCREEN``
>
> - Put device into zoom/full screen mode
>
> --
> 2.20.1.321.g9e740568ce-goog
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: i8042 - Rework DT node name comparisons
From: Dmitry Torokhov @ 2019-02-18 7:23 UTC (permalink / raw)
To: Rob Herring; +Cc: linux-kernel, devicetree, linux-input
In-Reply-To: <20190213161631.17028-1-robh@kernel.org>
On Wed, Feb 13, 2019 at 10:16:31AM -0600, Rob Herring wrote:
> Convert string compares of DT node names to use of_node_name_eq helper
> instead. For the root node on SUN DT, we need to retrieve the 'name'
> property as it is the rare case where the 'name' property and node name
> differ. With both changes, it removes direct access to the node name
> pointer.
>
> While at it, comvert the open coded loop to use
> for_each_child_of_node().
>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: linux-input@vger.kernel.org
> Signed-off-by: Rob Herring <robh@kernel.org>
Applied, thank you.
> ---
> drivers/input/serio/i8042-sparcio.h | 21 ++++++++++-----------
> 1 file changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/input/serio/i8042-sparcio.h b/drivers/input/serio/i8042-sparcio.h
> index 796289846204..fce76812843b 100644
> --- a/drivers/input/serio/i8042-sparcio.h
> +++ b/drivers/input/serio/i8042-sparcio.h
> @@ -53,12 +53,11 @@ static struct resource *kbd_res;
>
> static int sparc_i8042_probe(struct platform_device *op)
> {
> - struct device_node *dp = op->dev.of_node;
> + struct device_node *dp;
>
> - dp = dp->child;
> - while (dp) {
> - if (!strcmp(dp->name, OBP_PS2KBD_NAME1) ||
> - !strcmp(dp->name, OBP_PS2KBD_NAME2)) {
> + for_each_child_of_node(op->dev.of_node, dp) {
> + if (of_node_name_eq(dp, OBP_PS2KBD_NAME1) ||
> + of_node_name_eq(dp, OBP_PS2KBD_NAME2)) {
> struct platform_device *kbd = of_find_device_by_node(dp);
> unsigned int irq = kbd->archdata.irqs[0];
> if (irq == 0xffffffff)
> @@ -67,16 +66,14 @@ static int sparc_i8042_probe(struct platform_device *op)
> kbd_iobase = of_ioremap(&kbd->resource[0],
> 0, 8, "kbd");
> kbd_res = &kbd->resource[0];
> - } else if (!strcmp(dp->name, OBP_PS2MS_NAME1) ||
> - !strcmp(dp->name, OBP_PS2MS_NAME2)) {
> + } else if (of_node_name_eq(dp, OBP_PS2MS_NAME1) ||
> + of_node_name_eq(dp, OBP_PS2MS_NAME2)) {
> struct platform_device *ms = of_find_device_by_node(dp);
> unsigned int irq = ms->archdata.irqs[0];
> if (irq == 0xffffffff)
> irq = op->archdata.irqs[0];
> i8042_aux_irq = irq;
> }
> -
> - dp = dp->sibling;
> }
>
> return 0;
> @@ -109,8 +106,9 @@ static struct platform_driver sparc_i8042_driver = {
> static int __init i8042_platform_init(void)
> {
> struct device_node *root = of_find_node_by_path("/");
> + const char *name = of_get_property(root, "name", NULL);
>
> - if (!strcmp(root->name, "SUNW,JavaStation-1")) {
> + if (name && !strcmp(name, "SUNW,JavaStation-1")) {
> /* Hardcoded values for MrCoffee. */
> i8042_kbd_irq = i8042_aux_irq = 13 | 0x20;
> kbd_iobase = ioremap(0x71300060, 8);
> @@ -139,8 +137,9 @@ static int __init i8042_platform_init(void)
> static inline void i8042_platform_exit(void)
> {
> struct device_node *root = of_find_node_by_path("/");
> + const char *name = of_get_property(root, "name", NULL);
>
> - if (strcmp(root->name, "SUNW,JavaStation-1"))
> + if (!name || strcmp(name, "SUNW,JavaStation-1"))
> platform_driver_unregister(&sparc_i8042_driver);
> }
>
> --
> 2.19.1
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH v3 1/4] dt-bindings: input: touchscreen: goodix: Document AVDD28-supply property
From: Dmitry Torokhov @ 2019-02-18 7:12 UTC (permalink / raw)
To: Jagan Teki
Cc: Rob Herring, devicetree, Chen-Yu Tsai, linux-input,
linux-kernel@vger.kernel.org, Michael Trimarchi, linux-amarula
In-Reply-To: <CAMty3ZDBu2Y9psNu9nrZ=BNVvhpJ2qYn-Qp7MDqdhK9_SrcpNg@mail.gmail.com>
On Sun, Feb 17, 2019 at 02:45:52PM +0530, Jagan Teki wrote:
> On Sun, Feb 17, 2019 at 1:21 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
> >
> > Hi Dmitry and Rob,
> >
> > On Thu, Feb 14, 2019 at 3:21 AM Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Tue, Jan 22, 2019 at 7:39 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > > >
> > > > On Mon, Jan 21, 2019 at 9:46 PM Rob Herring <robh@kernel.org> wrote:
> > > > >
> > > > > On Fri, Jan 18, 2019 at 10:01 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > > > > >
> > > > > > On Wed, Jan 9, 2019 at 7:08 PM Rob Herring <robh@kernel.org> wrote:
> > > > > > >
> > > > > > > Please CC DT list if you want bindings reviewed.
> > > > > >
> > > > > > Sorry I forgot.
> > > > > >
> > > > > > >
> > > > > > > On Wed, Jan 9, 2019 at 1:40 AM Dmitry Torokhov
> > > > > > > <dmitry.torokhov@gmail.com> wrote:
> > > > > > > >
> > > > > > > > On Sat, Dec 15, 2018 at 08:47:59PM +0530, Jagan Teki wrote:
> > > > > > > > > Most of the Goodix CTP controllers are supply with AVDD28 pin.
> > > > > > > > > which need to supply for controllers like GT5663 on some boards
> > > > > > > > > to trigger the power.
> > > > > > > > >
> > > > > > > > > So, document the supply property so-that the require boards
> > > > > > > > > that used on GT5663 can enable it via device tree.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > > > > > > ---
> > > > > > > > > Documentation/devicetree/bindings/input/touchscreen/goodix.txt | 1 +
> > > > > > > > > 1 file changed, 1 insertion(+)
> > > > > > > > >
> > > > > > > > > diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> > > > > > > > > index f7e95c52f3c7..c4622c983e08 100644
> > > > > > > > > --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> > > > > > > > > +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> > > > > > > > > @@ -23,6 +23,7 @@ Optional properties:
> > > > > > > > > - touchscreen-inverted-y : Y axis is inverted (boolean)
> > > > > > > > > - touchscreen-swapped-x-y : X and Y axis are swapped (boolean)
> > > > > > > > > (swapping is done after inverting the axis)
> > > > > > > > > + - AVDD28-supply : Analog power supply regulator on AVDD28 pin
> > > > > > > >
> > > > > > > > I think we normally use lower case in DT bindings and rarely encode
> > > > > > > > voltage in the supply name unless we are dealing with several supplies
> > > > > > > > of the same kind, but I'll let Ron comment on this.
> > > > > > >
> > > > > > > Yes on lowercase though there are some exceptions.
> > > > > > >
> > > > > > > There's also a AVDD22 supply as well as DVDD12 and VDDIO. So we
> > > > > > > probably need to keep the voltage, but the binding is incomplete.
> > > > > >
> > > > > > What is incomplete here? can you please elaborate.
> > > > >
> > > > > You are missing the 3 other supplies the chip has: AVDD22, DVDD12 and VDDIO.
> > > >
> > > > Though it has other supplies, only AVDD28 is connected in the Host
> > > > interface design similar like 9. Reference Schematic on Page, 23 in
> > > > attached manual.
> > >
> > > That is for a particular board design. It still has other supplies.
> > > Just make the binding complete please. You can make them optional. I
> > > don't care if the driver supports controlling all the supplies or not
> > > (though Dmitry might).
> >
> > So, Can I make bulk get and enable in all 4 regulators global to
> > driver or specific to that chip, in either way the regulators which
> > are not used to process via dummy regulators (which we all know).
> >
> > or regulator which are not using are get via devm_regulator_get_optional.
> >
> > Any suggestions?
>
> Just realized to go with bulk calls, please have a look at on v5 [1]
I do not believe you can use bulk regulator API here as you need to
implement the power up timing scheme mentioned on p. 12 of the spec you
referenced earlier. And, because of that timing diagram, I believe if
you want to control AVDD supply, you also have to control VDDIO supply
as it has to be on only after enabling AVDD.
I also wonder if it is valid to have both AVDD28 and AVDD22 connected at
the same time.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v4 2/4] Input: goodix - Add AVDD28-supply regulator support
From: Dmitry Torokhov @ 2019-02-18 7:08 UTC (permalink / raw)
To: Jagan Teki
Cc: Bastien Nocera, Rob Herring, Henrik Rydberg, linux-input,
linux-kernel, devicetree, Mark Rutland
In-Reply-To: <20190212112439.2025-3-jagan@amarulasolutions.com>
Hi Jagan,
On Tue, Feb 12, 2019 at 04:54:37PM +0530, Jagan Teki wrote:
> Goodix CTP controllers have AVDD28 pin connected to voltage
> regulator which may not be turned on by default, like for GT5663.
>
> Add support for such ctp used boards by adding voltage regulator
> handling code to goodix ctp driver.
>
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
> drivers/input/touchscreen/goodix.c | 44 ++++++++++++++++++++++++++----
> 1 file changed, 39 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
> index f2d9c2c41885..e92b90be1ac2 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -27,6 +27,7 @@
> #include <linux/delay.h>
> #include <linux/irq.h>
> #include <linux/interrupt.h>
> +#include <linux/regulator/consumer.h>
> #include <linux/slab.h>
> #include <linux/acpi.h>
> #include <linux/of.h>
> @@ -47,6 +48,7 @@ struct goodix_ts_data {
> struct touchscreen_properties prop;
> unsigned int max_touch_num;
> unsigned int int_trigger_type;
> + struct regulator *avdd28;
> struct gpio_desc *gpiod_int;
> struct gpio_desc *gpiod_rst;
> u16 id;
> @@ -761,6 +763,13 @@ static void goodix_config_cb(const struct firmware *cfg, void *ctx)
> complete_all(&ts->firmware_loading_complete);
> }
>
> +static void goodix_disable_regulator(void *arg)
> +{
> + struct goodix_ts_data *ts = arg;
> +
> + regulator_disable(ts->avdd28);
> +}
> +
> static int goodix_ts_probe(struct i2c_client *client,
> const struct i2c_device_id *id)
> {
> @@ -786,25 +795,46 @@ static int goodix_ts_probe(struct i2c_client *client,
> if (error)
> return error;
>
> + ts->avdd28 = devm_regulator_get(&client->dev, "AVDD28");
> + if (IS_ERR(ts->avdd28)) {
> + error = PTR_ERR(ts->avdd28);
> + if (error != -EPROBE_DEFER)
> + dev_err(&client->dev,
> + "Failed to get AVDD28 regulator: %d\n", error);
> + return error;
> + }
> +
> + /* power the controller */
> + error = regulator_enable(ts->avdd28);
> + if (error) {
> + dev_err(&client->dev, "Controller fail to enable AVDD28\n");
> + return error;
> + }
> +
> + error = devm_add_action_or_reset(&client->dev,
> + goodix_disable_regulator, ts);
> + if (error)
> + return error;
> +
> if (ts->gpiod_int && ts->gpiod_rst) {
> /* reset the controller */
> error = goodix_reset(ts);
> if (error) {
> dev_err(&client->dev, "Controller reset failed.\n");
> - return error;
> + goto error;
Why are you changing this? You added goodix_disable_regulator() as
custom devm action, so it will fire if probe() returns with error.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH (resend)] Input: uinput - Set name/phys to NULL before kfree().
From: Dmitry Torokhov @ 2019-02-17 21:07 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: rydberg, syzbot, linux-input, linux-kernel, syzkaller-bugs
In-Reply-To: <b937b59c-22b8-9113-229f-056738819467@I-love.SAKURA.ne.jp>
Hi Tetsuo,
On Fri, Feb 08, 2019 at 07:25:52PM +0900, Tetsuo Handa wrote:
> syzbot is hitting use-after-free bug in uinput module [1]. This is because
> uinput_destroy_device() sometimes kfree()s dev->name and dev->phys at
> uinput_destroy_device() before dev_uevent() is triggered by dropping the
> refcount to 0. Since the timing of triggering last input_put_device() is
> uncontrollable, this patch prepares for such race by setting dev->name and
> dev->phys to NULL before doing operations which might drop the refcount
> to 0.
>
> [1] https://syzkaller.appspot.com/bug?id=8b17c134fe938bbddd75a45afaa9e68af43a362d
Sorry it took me so long to sort out the issue and unfortunately I
disagree with your analysis. The issue here is not that we do not know
when last reference is being dropped (because we expect that KOBJ_REMOVE
uevent will be sent out when we call input_unregister_device, which is
quite deterministic) but the kobject cleanup logic added in commit
0f4dafc0563c6c49e17fe14b3f5f356e4c4b8806 ("Kobject: auto-cleanup on
final unref") coupled with the fault injected by the syzcaller.
The commit tries to send final uevent for objects for which "add" uevent
has been sent, but not "remove" event. However in uinput (and general
input case) we always take care of sending uevent at unregister, and do
not expect to have uevent sent out at the final "put" time.
I believe the real fix is to have kobj->state_remove_uevent_sent be set
to true as soon as we enter kobject_uevent(kobj, KOBJ_REMOVE) so that
it is being set even if memory allocation fails. Doing anything else may
violate expectations of subsystem owning the kobject.
Thanks.
--
Dmitry
^ permalink raw reply
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