* [PATCH] HID: wacom: Mark expected switch fall-through
From: Gustavo A. R. Silva @ 2019-02-11 22:04 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: linux-input, linux-kernel, Gustavo A. R. Silva, Kees Cook
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>
---
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
^ permalink raw reply related
* [PATCH] HID: roccat: Mark expected switch fall-through
From: Gustavo A. R. Silva @ 2019-02-11 21:53 UTC (permalink / raw)
To: Stefan Achatz, Jiri Kosina, Benjamin Tissoires
Cc: linux-input, linux-kernel, Gustavo A. R. Silva, Kees Cook
In preparation to enabling -Wimplicit-fallthrough, mark switch
cases where we are expecting to fall through.
This patch fixes the following warning:
drivers/hid/hid-roccat-kone.c: In function ‘kone_keep_values_up_to_date’:
drivers/hid/hid-roccat-kone.c:784:20: warning: this statement may fall through [-Wimplicit-fallthrough=]
kone->actual_dpi = kone->profiles[event->value - 1].
~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
startup_dpi;
~~~~~~~~~~~
drivers/hid/hid-roccat-kone.c:786:2: note: here
case kone_mouse_event_osd_profile:
^~~~
Warning level 3 was used: -Wimplicit-fallthrough=3
This patch is part of the ongoing efforts to enable
-Wimplicit-fallthrough.
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
drivers/hid/hid-roccat-kone.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/hid/hid-roccat-kone.c b/drivers/hid/hid-roccat-kone.c
index bf4675a27396..c4dd6162c1d6 100644
--- a/drivers/hid/hid-roccat-kone.c
+++ b/drivers/hid/hid-roccat-kone.c
@@ -783,6 +783,7 @@ static void kone_keep_values_up_to_date(struct kone_device *kone,
case kone_mouse_event_switch_profile:
kone->actual_dpi = kone->profiles[event->value - 1].
startup_dpi;
+ /* fall through */
case kone_mouse_event_osd_profile:
kone->actual_profile = event->value;
break;
--
2.20.1
^ permalink raw reply related
* Re: [PATCH] Input: matrix_keypad - use flush_delayed_work()
From: Sven Van Asbroeck @ 2019-02-11 12:42 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, Tejun Heo, Linux Kernel Mailing List
In-Reply-To: <20190211082902.GA95126@dtor-ws>
On Mon, Feb 11, 2019 at 3:29 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> It will not schedule new work because we check keypad->stopped flag
> in ISR.
>
Yes, you are correct. Sorry for the confusion.
Reviewed-by: Sven Van Asbroeck <TheSven73@gmail.com>
^ permalink raw reply
* [PATCH] input: keyboard: gpio-keys-polled: use input name from pdata if available
From: Enrico Weigelt, metux IT consult @ 2019-02-11 11:19 UTC (permalink / raw)
To: linux-kernel; +Cc: linux-input, dmitry.torokhov
Instead of hardcoding the input name to the driver name
('gpio-keys-polled'), allow the passing a name via platform data
('name' field was already present), but default to old behaviour
in case of NULL.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
---
drivers/input/keyboard/gpio_keys_polled.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/keyboard/gpio_keys_polled.c b/drivers/input/keyboard/gpio_keys_polled.c
index edc7262..3312186 100644
--- a/drivers/input/keyboard/gpio_keys_polled.c
+++ b/drivers/input/keyboard/gpio_keys_polled.c
@@ -272,7 +272,7 @@ static int gpio_keys_polled_probe(struct platform_device *pdev)
input = poll_dev->input;
- input->name = pdev->name;
+ input->name = (pdata->name ? pdata->name : pdev->name);
input->phys = DRV_NAME"/input0";
input->id.bustype = BUS_HOST;
--
1.9.1
^ permalink raw reply related
* Re: [PATCH] Input: matrix_keypad - use flush_delayed_work()
From: Dmitry Torokhov @ 2019-02-11 8:29 UTC (permalink / raw)
To: Sven Van Asbroeck; +Cc: linux-input, Tejun Heo, Linux Kernel Mailing List
In-Reply-To: <CAGngYiXytDjjAVh_gAaqJa0qLZAM-cTko92s6=PjS4tjZMvfHg@mail.gmail.com>
Hi Sven,
On Sun, Feb 10, 2019 at 12:43:21PM -0500, Sven Van Asbroeck wrote:
> Hi Dmitry,
>
> On Thu, Feb 7, 2019 at 5:46 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > We should be using flush_delayed_work() instead of flush_work() in
> > matrix_keypad_stop() to ensure that we are not missing work that is
> > scheduled but not yet put in the workqueue (i.e. its delay timer has not
> > expired yet).
> >
>
> Could the following scenario cause a use-after-free?
> (I am adding comments on lines starting with -->)
>
> a) user closes the device handle:
>
> static void matrix_keypad_stop(struct input_dev *dev)
> {
> struct matrix_keypad *keypad = input_get_drvdata(dev);
>
> spin_lock_irq(&keypad->lock);
> keypad->stopped = true;
> spin_unlock_irq(&keypad->lock);
>
> flush_work(&keypad->work.work);
> -->
> --> new interrupt comes in, and schedules new delayed keypad->work (1)
It will not schedule new work because we check keypad->stopped flag
in ISR.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2] dt-bindings: input: ti-tsc-adc: Add new compatible for AM654 SoCs
From: Vignesh R @ 2019-02-11 5:40 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Rob Herring, linux-input, devicetree, linux-kernel, linux-omap
In-Reply-To: <20190108052631.4169-1-vigneshr@ti.com>
Hi Dmitry,
On 08/01/19 10:56 AM, Vignesh R wrote:
> AM654 SoCs has ADC IP which is similar to AM335x, but without the
> touchscreen part. Add new compatible to handle AM654 SoCs. Also, it
> seems that existing compatible strings used in the kernel DTs were never
> documented. So, document them now.
>
> Signed-off-by: Vignesh R <vigneshr@ti.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---
If there are no comments, could you queue this bindings patch? This will
help me in getting ADC DT nodes to be merged into ARM SoC tree.
Regards
Vignesh
> v2:
> Fix subject line to include subsystem name
> Rebase onto v5.0-rc1
> v1: https://lkml.org/lkml/2018/11/19/313
>
> .../devicetree/bindings/input/touchscreen/ti-tsc-adc.txt | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
> index b1163bf97146..aad5e34965eb 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
> +++ b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
> @@ -2,7 +2,12 @@
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Required properties:
> +- mfd
> + compatible: Should be
> + "ti,am3359-tscadc" for AM335x/AM437x SoCs
> + "ti,am654-tscadc", "ti,am3359-tscadc" for AM654 SoCs
> - child "tsc"
> + compatible: Should be "ti,am3359-tsc".
> ti,wires: Wires refer to application modes i.e. 4/5/8 wire touchscreen
> support on the platform.
> ti,x-plate-resistance: X plate resistance
> @@ -25,6 +30,9 @@ Required properties:
> AIN0 = 0, AIN1 = 1 and so on till AIN7 = 7.
> XP = 0, XN = 1, YP = 2, YN = 3.
> - child "adc"
> + compatible: Should be
> + "ti,am3359-adc" for AM335x/AM437x SoCs
> + "ti,am654-adc", "ti,am3359-adc" for AM654 SoCs
> ti,adc-channels: List of analog inputs available for ADC.
> AIN0 = 0, AIN1 = 1 and so on till AIN7 = 7.
>
>
--
Regards
Vignesh
^ permalink raw reply
* [PATCH] Input: qt2160 - remove redundant spinlock
From: thesven73 @ 2019-02-11 2:32 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 spinlock which prevents schedule_delayed_work() and
mod_delayed_work() from executing concurrently.
This was required back when mod_delayed_work() did not exist,
and had to be implemented with a cancel + schedule. 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>
---
drivers/input/keyboard/qt2160.c | 9 ---------
1 file changed, 9 deletions(-)
diff --git a/drivers/input/keyboard/qt2160.c b/drivers/input/keyboard/qt2160.c
index 43b86482dda0..23dc7c0c7d78 100644
--- a/drivers/input/keyboard/qt2160.c
+++ b/drivers/input/keyboard/qt2160.c
@@ -69,7 +69,6 @@ struct qt2160_data {
struct i2c_client *client;
struct input_dev *input;
struct delayed_work dwork;
- spinlock_t lock; /* Protects canceling/rescheduling of dwork */
unsigned short keycodes[ARRAY_SIZE(qt2160_key2code)];
u16 key_matrix;
#ifdef CONFIG_LEDS_CLASS
@@ -221,22 +220,15 @@ static int qt2160_get_key_matrix(struct qt2160_data *qt2160)
static irqreturn_t qt2160_irq(int irq, void *_qt2160)
{
struct qt2160_data *qt2160 = _qt2160;
- unsigned long flags;
-
- spin_lock_irqsave(&qt2160->lock, flags);
mod_delayed_work(system_wq, &qt2160->dwork, 0);
- spin_unlock_irqrestore(&qt2160->lock, flags);
-
return IRQ_HANDLED;
}
static void qt2160_schedule_read(struct qt2160_data *qt2160)
{
- spin_lock_irq(&qt2160->lock);
schedule_delayed_work(&qt2160->dwork, QT2160_CYCLE_INTERVAL);
- spin_unlock_irq(&qt2160->lock);
}
static void qt2160_worker(struct work_struct *work)
@@ -406,7 +398,6 @@ static int qt2160_probe(struct i2c_client *client,
qt2160->client = client;
qt2160->input = input;
INIT_DELAYED_WORK(&qt2160->dwork, qt2160_worker);
- spin_lock_init(&qt2160->lock);
input->name = "AT42QT2160 Touch Sense Keyboard";
input->id.bustype = BUS_I2C;
--
2.17.1
^ permalink raw reply related
* Re: [PATCH] Input: qt2160 - switch to using brightness_set_blocking()
From: Sven Van Asbroeck @ 2019-02-10 23:47 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, Linux Kernel Mailing List
In-Reply-To: <20190209172058.GA24435@dtor-ws>
On Sat, Feb 9, 2019 at 12:21 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Now that LEDs core allows "blocking" flavor of "set brightness" method we
> can use it and get rid of private work items.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Looking good !
Reviewed-by: Sven Van Asbroeck <TheSven73@gmail.com>
Good job to get rid of that led_lock mutex. It didn't serve any purpose
but more importantly, it could introduce a sleep in brightness_set,
which cannot sleep...
^ permalink raw reply
* Re: [PATCH] Input: apanel - switch to using brightness_set_blocking()
From: Sven Van Asbroeck @ 2019-02-10 23:21 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, Linux Kernel Mailing List
In-Reply-To: <20190209171925.GA24193@dtor-ws>
Hi Dmitry,
On Sat, Feb 9, 2019 at 12:19 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Now that LEDs core allows "blocking" flavor of "set brightness" method we
> can use it and get rid of private work item. As a bonus, we are no longer
> forgetting to cancel it when we unbind the driver.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
This is the blocking set_brightness callback after applying the patch :
(it's not immediately obvious when looking at only the patch)
struct apanel {
...
u16 led_bits;
...
};
static int mail_led_set(struct led_classdev *led,
enum led_brightness value)
{
struct apanel *ap = container_of(led, struct apanel, mail_led);
if (value != LED_OFF)
ap->led_bits |= 0x8000;
else
ap->led_bits &= ~0x8000;
return i2c_smbus_write_word_data(ap->client, 0x10, ap->led_bits);
}
ap->led_bits was previously used as a 'mailslot' between the
set_brightness callback and the deferred work, right?
After the patch, it's used only in this function. Maybe it could be
replaced by a local variable? As a bonus, that would make
the driver's private struct slightly smaller.
^ permalink raw reply
* Re: [PATCH] Input: matrix_keypad - use flush_delayed_work()
From: Sven Van Asbroeck @ 2019-02-10 17:43 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, Tejun Heo, Linux Kernel Mailing List
In-Reply-To: <20190207224650.GA49861@dtor-ws>
Hi Dmitry,
On Thu, Feb 7, 2019 at 5:46 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> We should be using flush_delayed_work() instead of flush_work() in
> matrix_keypad_stop() to ensure that we are not missing work that is
> scheduled but not yet put in the workqueue (i.e. its delay timer has not
> expired yet).
>
Could the following scenario cause a use-after-free?
(I am adding comments on lines starting with -->)
a) user closes the device handle:
static void matrix_keypad_stop(struct input_dev *dev)
{
struct matrix_keypad *keypad = input_get_drvdata(dev);
spin_lock_irq(&keypad->lock);
keypad->stopped = true;
spin_unlock_irq(&keypad->lock);
flush_work(&keypad->work.work);
-->
--> new interrupt comes in, and schedules new delayed keypad->work (1)
-->
/*
* matrix_keypad_scan() will leave IRQs enabled;
* we should disable them now.
*/
disable_row_irqs(keypad);
}
b) user removes the keypad, or unloads the module:
static int matrix_keypad_remove(struct platform_device *pdev)
{
struct matrix_keypad *keypad = platform_get_drvdata(pdev);
matrix_keypad_free_gpio(keypad);
input_unregister_device(keypad->input_dev);
kfree(keypad);
-->
--> delayed keypad->work scheduled at (1) above executes anywhere past here,
--> and causes a user-after-free.
-->
return 0;
}
^ 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-10 11:18 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Dmitry Torokhov, Henrik Rydberg, Lukas Wunner, Federico Lorenzi,
linux-input, linux-kernel
In-Reply-To: <20190206202256.GZ9224@smile.fi.intel.com>
Hi Andy,
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:
> > The keyboard and trackpad on recent MacBook's (since 8,1) and
> > MacBookPro's (13,* and 14,*) are attached to an SPI controller instead
> > of USB, as previously. The higher level protocol is not publicly
> > documented and hence has been reverse engineered. As a consequence there
> > are still a number of unknown fields and commands. However, the known
> > parts have been working well and received extensive testing and use.
> >
> > In order for this driver to work, the proper SPI drivers need to be
> > loaded too; for MB8,1 these are spi_pxa2xx_platform and spi_pxa2xx_pci;
> > for all others they are spi_pxa2xx_platform and intel_lpss_pci. For this
> > reason enabling this driver in the config implies enabling the above
> > drivers.
>
> Thanks for doing this. My review below.
Many thanks for your extensive review. Sorry for the delay, managed to
mess up my machine...
I've made most of the changes you suggested - below are my comments
and answers where I have questsions or I think there are good reasons
for the current approach.
> > +/**
>
> I'm not sure it's kernel doc format comment.
Sorry, you mean you're not sure whether this should be a kernel doc vs
a regular comment? Ok, making it a regular comment.
[snip]
> > +#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)
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.
[snip]
> > +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.
> 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.
[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.
> > +/**
> > + * 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.
> > + */
>
> > +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.
[snip]
> > +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.
> > + 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).
> Perhaps "Unrecognized ... "-ish better?
Sure. I've updated these now to
case DBG_RD_UNKN:
return "Unrecognized Event";
default:
return "-Unrecognized log mask-";
> > + }
>
> > +static inline bool applespi_check_write_status(struct applespi_data *applespi,
> > + int sts)
>
> Indentation broken.
Not in the original - again, an artifact of using tabs for
indentation and the first line being quoted.
[snip]
> > +static int applespi_enable_spi(struct applespi_data *applespi)
> > +{
> > + int result;
>
> Sometimes you have ret, sometimes this. Better to unify across the code.
Sorry, that is indeed ugly and lazy - all status/result variables now
have the same name.
[snip]
> > +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?
> > + 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.
[snip]
> 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.
[snip]
> Besides, run checkpatch.pl!
I did/do, with --strict.
[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)
> > +static void report_tp_state(struct applespi_data *applespi,
> > + struct touchpad_protocol *t)
> > +{
[snip]
> > + const struct tp_finger *f;
> > + struct input_dev *input;
> > + const struct applespi_tp_info *tp_info = &applespi->tp_info;
> > + int i, n;
> > +
> > + /* touchpad_input_dev is set async in worker */
> > + input = smp_load_acquire(&applespi->touchpad_input_dev);
> > + if (!input)
> > + return; /* touchpad isn't initialized yet */
> > +
>
> > + n = 0;
> > +
> > + for (i = 0; i < t->number_of_fingers; i++) {
>
> for (n = 0, i = 0; i < ...; i++, n++) {
>
> ?
n is only incremented for fingers that are down. See below.
> > + f = &t->fingers[i];
> > + if (raw2int(f->touch_major) == 0)
> > + continue;
This here skips incrementing n.
> > + applespi->pos[n].x = raw2int(f->abs_x);
> > + applespi->pos[n].y = tp_info->y_min + tp_info->y_max -
> > + raw2int(f->abs_y);
> > + n++;
> > +
[snip]
> > +static void applespi_remap_fn_key(struct keyboard_protocol
> > + *keyboard_protocol)
>
> Better to split like
>
> static void
> fn(struct ...);
Ah, wasn't aware that was an acceptable variant. Thanks. So I've also
changed a few other places that I think would benefit from this sort
of split:
-static const struct applespi_key_translation *applespi_find_translation(
- const struct applespi_key_translation *table, u16 key)
+static const struct applespi_key_translation *
+applespi_find_translation(const struct applespi_key_translation *table, u16 key)
and
-static void applespi_handle_keyboard_event(struct applespi_data *applespi,
- struct keyboard_protocol
- *keyboard_protocol)
+static void
+applespi_handle_keyboard_event(struct applespi_data *applespi,
+ struct keyboard_protocol *keyboard_protocol)
and
-static void applespi_register_touchpad_device(struct applespi_data *applespi,
- struct touchpad_info_protocol *rcvd_tp_info)
+static int
+applespi_register_touchpad_device(struct applespi_data *applespi,
+ struct touchpad_info_protocol *rcvd_tp_info)
[snip]
> > + memcpy(&applespi->last_keys_pressed, keyboard_protocol->keys_pressed,
> > + sizeof(applespi->last_keys_pressed));
>
> applespi->last_keys_pressed = *keyboard_protocol->keys_pressed;
>
> (if they are of the same type) ?
AFAIK that only works for structs, but these are arrays.
[snip]
> > +static void applespi_got_data(struct applespi_data *applespi)
[snip]
> > + if (le16_to_cpu(message->length) + 2 != tp_len) {
> > + dev_warn_ratelimited(&applespi->spi->dev,
> > + "Received corrupted packet (invalid message length)\n");
> > + goto cleanup;
> > + }
>
> > + } else if (packet->flags == PACKET_TYPE_WRITE) {
> > + applespi_handle_cmd_response(applespi, packet, message);
> > + }
> > +
>
> > +cleanup:
>
> err_msg_complete: ?
This is for both successful and failed messages, so "err" seems wrong.
Renamed it to 'msg_complete:' instead.
[snip]
> > + /*
> > + * By default this device is not enable for wakeup; but USB keyboards
> > + * generally are, so the expectation is that by default the keyboard
> > + * will wake the system.
> > + */
> > + device_wakeup_enable(&spi->dev);
[snip]
> > +static int applespi_remove(struct spi_device *spi)
> > +{
[snip]
> It seems you still have wakeup enabled for the device.
Good catch - disabling on remove now.
[snip]
Cheers,
Ronald
^ permalink raw reply
* [PATCH] HID: multitouch: Lenovo X1 Tablet Gen3 trackpoint and buttons
From: leakim.wikstrom @ 2019-02-09 18:05 UTC (permalink / raw)
To: Jiri Kosina
Cc: Mikael Wikström, Benjamin Tissoires, linux-input,
linux-kernel
From: Mikael Wikström <leakim.wikstrom@gmail.com>
Add support for the trackpoint and three mouse buttons on the type cover
of the Lenovo X1 Tablet Gen3.
This is the same as with the 2nd generation Lenovo X1 Tablet.
Signed-off-by: Mikael Wikström <leakim.wikstrom@gmail.com>
---
drivers/hid/hid-ids.h | 1 +
drivers/hid/hid-multitouch.c | 6 ++++++
2 files changed, 7 insertions(+)
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index f63489c882bb..500c8f64f7ae 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -703,6 +703,7 @@
#define USB_DEVICE_ID_LENOVO_TPPRODOCK 0x6067
#define USB_DEVICE_ID_LENOVO_X1_COVER 0x6085
#define USB_DEVICE_ID_LENOVO_X1_TAB 0x60a3
+#define USB_DEVICE_ID_LENOVO_X1_TAB3 0x60b5
#define USB_VENDOR_ID_LG 0x1fd2
#define USB_DEVICE_ID_LG_MULTITOUCH 0x0064
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index f7c6de2b6730..03a69049ca52 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -1780,6 +1780,12 @@ static const struct hid_device_id mt_devices[] = {
USB_VENDOR_ID_LENOVO,
USB_DEVICE_ID_LENOVO_X1_TAB) },
+ /* Lenovo X1 TAB Gen 3 */
+ { .driver_data = MT_CLS_WIN_8_DUAL,
+ HID_DEVICE(BUS_USB, HID_GROUP_MULTITOUCH_WIN_8,
+ USB_VENDOR_ID_LENOVO,
+ USB_DEVICE_ID_LENOVO_X1_TAB3) },
+
/* Anton devices */
{ .driver_data = MT_CLS_EXPORT_ALL_INPUTS,
MT_USB_DEVICE(USB_VENDOR_ID_ANTON,
--
2.20.1
^ permalink raw reply related
* [PATCH] Input: qt2160 - switch to using brightness_set_blocking()
From: Dmitry Torokhov @ 2019-02-09 17:20 UTC (permalink / raw)
To: linux-input; +Cc: Sven Van Asbroeck, linux-kernel
Now that LEDs core allows "blocking" flavor of "set brightness" method we
can use it and get rid of private work items.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/keyboard/qt2160.c | 69 +++++++++++++--------------------
1 file changed, 27 insertions(+), 42 deletions(-)
diff --git a/drivers/input/keyboard/qt2160.c b/drivers/input/keyboard/qt2160.c
index 43b86482dda0..d466bc07aebb 100644
--- a/drivers/input/keyboard/qt2160.c
+++ b/drivers/input/keyboard/qt2160.c
@@ -58,10 +58,9 @@ static unsigned char qt2160_key2code[] = {
struct qt2160_led {
struct qt2160_data *qt2160;
struct led_classdev cdev;
- struct work_struct work;
char name[32];
int id;
- enum led_brightness new_brightness;
+ enum led_brightness brightness;
};
#endif
@@ -74,7 +73,6 @@ struct qt2160_data {
u16 key_matrix;
#ifdef CONFIG_LEDS_CLASS
struct qt2160_led leds[QT2160_NUM_LEDS_X];
- struct mutex led_lock;
#endif
};
@@ -83,46 +81,39 @@ static int qt2160_write(struct i2c_client *client, u8 reg, u8 data);
#ifdef CONFIG_LEDS_CLASS
-static void qt2160_led_work(struct work_struct *work)
+static int qt2160_led_set(struct led_classdev *cdev,
+ enum led_brightness value)
{
- struct qt2160_led *led = container_of(work, struct qt2160_led, work);
+ struct qt2160_led *led = container_of(cdev, struct qt2160_led, cdev);
struct qt2160_data *qt2160 = led->qt2160;
struct i2c_client *client = qt2160->client;
- int value = led->new_brightness;
u32 drive, pwmen;
- mutex_lock(&qt2160->led_lock);
-
- drive = qt2160_read(client, QT2160_CMD_DRIVE_X);
- pwmen = qt2160_read(client, QT2160_CMD_PWMEN_X);
- if (value != LED_OFF) {
- drive |= (1 << led->id);
- pwmen |= (1 << led->id);
-
- } else {
- drive &= ~(1 << led->id);
- pwmen &= ~(1 << led->id);
- }
- qt2160_write(client, QT2160_CMD_DRIVE_X, drive);
- qt2160_write(client, QT2160_CMD_PWMEN_X, pwmen);
+ if (value != led->brightness) {
+ drive = qt2160_read(client, QT2160_CMD_DRIVE_X);
+ pwmen = qt2160_read(client, QT2160_CMD_PWMEN_X);
+ if (value != LED_OFF) {
+ drive |= BIT(led->id);
+ pwmen |= BIT(led->id);
- /*
- * Changing this register will change the brightness
- * of every LED in the qt2160. It's a HW limitation.
- */
- if (value != LED_OFF)
- qt2160_write(client, QT2160_CMD_PWM_DUTY, value);
+ } else {
+ drive &= ~BIT(led->id);
+ pwmen &= ~BIT(led->id);
+ }
+ qt2160_write(client, QT2160_CMD_DRIVE_X, drive);
+ qt2160_write(client, QT2160_CMD_PWMEN_X, pwmen);
- mutex_unlock(&qt2160->led_lock);
-}
+ /*
+ * Changing this register will change the brightness
+ * of every LED in the qt2160. It's a HW limitation.
+ */
+ if (value != LED_OFF)
+ qt2160_write(client, QT2160_CMD_PWM_DUTY, value);
-static void qt2160_led_set(struct led_classdev *cdev,
- enum led_brightness value)
-{
- struct qt2160_led *led = container_of(cdev, struct qt2160_led, cdev);
+ led->brightness = value;
+ }
- led->new_brightness = value;
- schedule_work(&led->work);
+ return 0;
}
#endif /* CONFIG_LEDS_CLASS */
@@ -293,20 +284,16 @@ static int qt2160_register_leds(struct qt2160_data *qt2160)
int ret;
int i;
- mutex_init(&qt2160->led_lock);
-
for (i = 0; i < QT2160_NUM_LEDS_X; i++) {
struct qt2160_led *led = &qt2160->leds[i];
snprintf(led->name, sizeof(led->name), "qt2160:x%d", i);
led->cdev.name = led->name;
- led->cdev.brightness_set = qt2160_led_set;
+ led->cdev.brightness_set_blocking = qt2160_led_set;
led->cdev.brightness = LED_OFF;
led->id = i;
led->qt2160 = qt2160;
- INIT_WORK(&led->work, qt2160_led_work);
-
ret = led_classdev_register(&client->dev, &led->cdev);
if (ret < 0)
return ret;
@@ -324,10 +311,8 @@ static void qt2160_unregister_leds(struct qt2160_data *qt2160)
{
int i;
- for (i = 0; i < QT2160_NUM_LEDS_X; i++) {
+ for (i = 0; i < QT2160_NUM_LEDS_X; i++)
led_classdev_unregister(&qt2160->leds[i].cdev);
- cancel_work_sync(&qt2160->leds[i].work);
- }
}
#else
--
2.20.1.791.gb4d0f1c61a-goog
--
Dmitry
^ permalink raw reply related
* [PATCH] Input: apanel - switch to using brightness_set_blocking()
From: Dmitry Torokhov @ 2019-02-09 17:19 UTC (permalink / raw)
To: linux-input; +Cc: iSven Van Asbroeck, linux-kernel
Now that LEDs core allows "blocking" flavor of "set brightness" method we
can use it and get rid of private work item. As a bonus, we are no longer
forgetting to cancel it when we unbind the driver.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/misc/apanel.c | 17 +++--------------
1 file changed, 3 insertions(+), 14 deletions(-)
diff --git a/drivers/input/misc/apanel.c b/drivers/input/misc/apanel.c
index 094bddf56755..2e138fef1274 100644
--- a/drivers/input/misc/apanel.c
+++ b/drivers/input/misc/apanel.c
@@ -22,7 +22,6 @@
#include <linux/io.h>
#include <linux/input-polldev.h>
#include <linux/i2c.h>
-#include <linux/workqueue.h>
#include <linux/leds.h>
#define APANEL_NAME "Fujitsu Application Panel"
@@ -60,7 +59,6 @@ struct apanel {
unsigned short keymap[MAX_PANEL_KEYS];
u16 nkeys;
u16 led_bits;
- struct work_struct led_work;
struct led_classdev mail_led;
};
@@ -109,15 +107,7 @@ static void apanel_poll(struct input_polled_dev *ipdev)
report_key(idev, ap->keymap[i]);
}
-/* Track state changes of LED */
-static void led_update(struct work_struct *work)
-{
- struct apanel *ap = container_of(work, struct apanel, led_work);
-
- i2c_smbus_write_word_data(ap->client, 0x10, ap->led_bits);
-}
-
-static void mail_led_set(struct led_classdev *led,
+static int mail_led_set(struct led_classdev *led,
enum led_brightness value)
{
struct apanel *ap = container_of(led, struct apanel, mail_led);
@@ -127,7 +117,7 @@ static void mail_led_set(struct led_classdev *led,
else
ap->led_bits &= ~0x8000;
- schedule_work(&ap->led_work);
+ return i2c_smbus_write_word_data(ap->client, 0x10, ap->led_bits);
}
static int apanel_remove(struct i2c_client *client)
@@ -179,7 +169,7 @@ static struct apanel apanel = {
},
.mail_led = {
.name = "mail:blue",
- .brightness_set = mail_led_set,
+ .brightness_set_blocking = mail_led_set,
},
};
@@ -235,7 +225,6 @@ static int apanel_probe(struct i2c_client *client,
if (err)
goto out3;
- INIT_WORK(&ap->led_work, led_update);
if (device_chip[APANEL_DEV_LED] != CHIP_NONE) {
err = led_classdev_register(&client->dev, &ap->mail_led);
if (err)
--
2.20.1.791.gb4d0f1c61a-goog
--
Dmitry
^ permalink raw reply related
* Re: [PATCH v2 2/2] input: misc: pwm-vibra: Stop regulator after disabling pwm, not before
From: Dmitry Torokhov @ 2019-02-09 17:13 UTC (permalink / raw)
To: Paweł Chmiel; +Cc: linux-input, linux-kernel
In-Reply-To: <20190124202732.14723-2-pawel.mikolaj.chmiel@gmail.com>
On Thu, Jan 24, 2019 at 09:27:32PM +0100, Paweł Chmiel wrote:
> This patch fixes order of disable calls in pwm_vibrator_stop.
> Currently when starting device, we first enable vcc regulator and then
> setup and enable pwm. When stopping, we should do this in oposite order,
> so first disable pwm and then disable regulator.
> Previously order was the same as in start.
>
> Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
Applied, thank you.
> ---
> drivers/input/misc/pwm-vibra.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/input/misc/pwm-vibra.c b/drivers/input/misc/pwm-vibra.c
> index 9df87431d7d4..dbb6d9e1b947 100644
> --- a/drivers/input/misc/pwm-vibra.c
> +++ b/drivers/input/misc/pwm-vibra.c
> @@ -80,14 +80,14 @@ static int pwm_vibrator_start(struct pwm_vibrator *vibrator)
>
> static void pwm_vibrator_stop(struct pwm_vibrator *vibrator)
> {
> + if (vibrator->pwm_dir)
> + pwm_disable(vibrator->pwm_dir);
> + pwm_disable(vibrator->pwm);
> +
> if (vibrator->vcc_on) {
> regulator_disable(vibrator->vcc);
> vibrator->vcc_on = false;
> }
> -
> - if (vibrator->pwm_dir)
> - pwm_disable(vibrator->pwm_dir);
> - pwm_disable(vibrator->pwm);
> }
>
> static void pwm_vibrator_play_work(struct work_struct *work)
> --
> 2.17.1
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2 1/2] input: misc: pwm-vibra: Prevent unbalanced regulator
From: Dmitry Torokhov @ 2019-02-09 17:13 UTC (permalink / raw)
To: Paweł Chmiel; +Cc: linux-input, linux-kernel, Jonathan Bakker
In-Reply-To: <20190124202732.14723-1-pawel.mikolaj.chmiel@gmail.com>
On Thu, Jan 24, 2019 at 09:27:31PM +0100, Paweł Chmiel wrote:
> From: Jonathan Bakker <xc-racer2@live.ca>
>
> pwm_vibrator_stop disables the regulator, but it can be called from
> multiple places, even when the regulator is already disabled. Fix this
> by using regulator_is_enabled check when starting and stopping device.
>
> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
> Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
Applied, thank you.
> ---
> Changes from v1:
> - Rather than using regulator_is_enabled api, use local flag for
> checking if regulator is enabled
> ---
> drivers/input/misc/pwm-vibra.c | 17 ++++++++++++-----
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/input/misc/pwm-vibra.c b/drivers/input/misc/pwm-vibra.c
> index 55da191ae550..9df87431d7d4 100644
> --- a/drivers/input/misc/pwm-vibra.c
> +++ b/drivers/input/misc/pwm-vibra.c
> @@ -34,6 +34,7 @@ struct pwm_vibrator {
> struct work_struct play_work;
> u16 level;
> u32 direction_duty_cycle;
> + bool vcc_on;
> };
>
> static int pwm_vibrator_start(struct pwm_vibrator *vibrator)
> @@ -42,10 +43,13 @@ static int pwm_vibrator_start(struct pwm_vibrator *vibrator)
> struct pwm_state state;
> int err;
>
> - err = regulator_enable(vibrator->vcc);
> - if (err) {
> - dev_err(pdev, "failed to enable regulator: %d", err);
> - return err;
> + if (!vibrator->vcc_on) {
> + err = regulator_enable(vibrator->vcc);
> + if (err) {
> + dev_err(pdev, "failed to enable regulator: %d", err);
> + return err;
> + }
> + vibrator->vcc_on = true;
> }
>
> pwm_get_state(vibrator->pwm, &state);
> @@ -76,7 +80,10 @@ static int pwm_vibrator_start(struct pwm_vibrator *vibrator)
>
> static void pwm_vibrator_stop(struct pwm_vibrator *vibrator)
> {
> - regulator_disable(vibrator->vcc);
> + if (vibrator->vcc_on) {
> + regulator_disable(vibrator->vcc);
> + vibrator->vcc_on = false;
> + }
>
> if (vibrator->pwm_dir)
> pwm_disable(vibrator->pwm_dir);
> --
> 2.17.1
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2] Input: st1232 - switch to gpiod API
From: Dmitry Torokhov @ 2019-02-09 16:54 UTC (permalink / raw)
To: Martin Kepplinger; +Cc: linux-input, linux-kernel
In-Reply-To: <20190208161533.25230-1-martin.kepplinger@ginzinger.com>
On Fri, Feb 08, 2019 at 05:15:33PM +0100, Martin Kepplinger wrote:
> Use devm_gpiod_get_optional() and gpiod_set_value_cansleep() instead
> of the old API. The st1232_ts_power() now passes on the inverted "poweron"
> value to reflect the correct logical value.
>
> Signed-off-by: Martin Kepplinger <martin.kepplinger@ginzinger.com>
Applied, thank you.
> ---
>
> ok. tested. If i get that right, assigning different gpio functions
> would now be difficult, but afaik isn't needed anyways.
>
> thanks
> martin
>
>
>
> revision history
> ----------------
> v2: retain dts compatibility by allowing current reset-gpios setting
>
>
> drivers/input/touchscreen/st1232.c | 22 ++++++++++------------
> 1 file changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/input/touchscreen/st1232.c b/drivers/input/touchscreen/st1232.c
> index 634d6c243845..1fbc0847416b 100644
> --- a/drivers/input/touchscreen/st1232.c
> +++ b/drivers/input/touchscreen/st1232.c
> @@ -45,7 +45,7 @@ struct st1232_ts_data {
> struct i2c_client *client;
> struct input_dev *input_dev;
> struct dev_pm_qos_request low_latency_req;
> - int reset_gpio;
> + struct gpio_desc *reset_gpio;
> const struct st_chip_info *chip_info;
> int read_buf_len;
> u8 *read_buf;
> @@ -142,8 +142,8 @@ static irqreturn_t st1232_ts_irq_handler(int irq, void *dev_id)
>
> static void st1232_ts_power(struct st1232_ts_data *ts, bool poweron)
> {
> - if (gpio_is_valid(ts->reset_gpio))
> - gpio_direction_output(ts->reset_gpio, poweron);
> + if (ts->reset_gpio)
> + gpiod_set_value_cansleep(ts->reset_gpio, !poweron);
> }
>
> static const struct st_chip_info st1232_chip_info = {
> @@ -215,15 +215,13 @@ static int st1232_ts_probe(struct i2c_client *client,
> ts->client = client;
> ts->input_dev = input_dev;
>
> - ts->reset_gpio = of_get_gpio(client->dev.of_node, 0);
> - if (gpio_is_valid(ts->reset_gpio)) {
> - error = devm_gpio_request(&client->dev, ts->reset_gpio, NULL);
> - if (error) {
> - dev_err(&client->dev,
> - "Unable to request GPIO pin %d.\n",
> - ts->reset_gpio);
> - return error;
> - }
> + ts->reset_gpio = devm_gpiod_get_optional(&client->dev, NULL,
> + GPIOD_OUT_HIGH);
> + if (IS_ERR(ts->reset_gpio)) {
> + error = PTR_ERR(ts->reset_gpio);
> + dev_err(&client->dev, "Unable to request GPIO pin: %d.\n",
> + error);
> + return error;
> }
>
> st1232_ts_power(ts, true);
> --
> 2.20.1
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: ili210x - switch to using devm_device_add_group()
From: Marek Vasut @ 2019-02-09 10:16 UTC (permalink / raw)
To: Dmitry Torokhov, linux-input; +Cc: linux-kernel
In-Reply-To: <20190207062746.GA30406@dtor-ws>
On 2/7/19 7:27 AM, Dmitry Torokhov wrote:
> By switching to devm_device_add_group() we can complete driver conversion
> to using managed resources and get rid of ili210x_i2c_remove().
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> drivers/input/touchscreen/ili210x.c | 16 ++--------------
> 1 file changed, 2 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
> index 6cfe463ac118..af1dd9cff12a 100644
> --- a/drivers/input/touchscreen/ili210x.c
> +++ b/drivers/input/touchscreen/ili210x.c
> @@ -376,7 +376,7 @@ static int ili210x_i2c_probe(struct i2c_client *client,
> return error;
> }
>
> - error = sysfs_create_group(&dev->kobj, &ili210x_attr_group);
> + error = devm_device_add_group(dev, &ili210x_attr_group);
> if (error) {
> dev_err(dev, "Unable to create sysfs attributes, err: %d\n",
> error);
> @@ -386,7 +386,7 @@ static int ili210x_i2c_probe(struct i2c_client *client,
> error = input_register_device(priv->input);
> if (error) {
> dev_err(dev, "Cannot register input device, err: %d\n", error);
> - goto err_remove_sysfs;
> + return error;
> }
>
> device_init_wakeup(dev, 1);
> @@ -396,17 +396,6 @@ static int ili210x_i2c_probe(struct i2c_client *client,
> client->irq, firmware.id, firmware.major, firmware.minor);
>
> return 0;
> -
> -err_remove_sysfs:
> - sysfs_remove_group(&dev->kobj, &ili210x_attr_group);
> - return error;
> -}
> -
> -static int ili210x_i2c_remove(struct i2c_client *client)
> -{
> - sysfs_remove_group(&client->dev.kobj, &ili210x_attr_group);
> -
> - return 0;
> }
>
> static int __maybe_unused ili210x_i2c_suspend(struct device *dev)
> @@ -454,7 +443,6 @@ static struct i2c_driver ili210x_ts_driver = {
> },
> .id_table = ili210x_i2c_id,
> .probe = ili210x_i2c_probe,
> - .remove = ili210x_i2c_remove,
> };
>
> module_i2c_driver(ili210x_ts_driver);
>
Reviewed-by: Marek Vasut <marex@denx.de>
On ILI251x
Tested-by: Marek Vasut <marex@denx.de>
--
Best regards,
Marek Vasut
^ permalink raw reply
* Re: [PATCH v2] Input: cap11xx - switch to using set_brightness_blocking()
From: Jacek Anaszewski @ 2019-02-08 21:37 UTC (permalink / raw)
To: Dmitry Torokhov, Sven Van Asbroeck; +Cc: linux-input, linux-kernel
In-Reply-To: <20190208075739.GA95091@dtor-ws>
Hi Dmitry,
Thank you for the update.
On 2/8/19 8:57 AM, Dmitry Torokhov wrote:
> Updating LED state requires access to regmap and therefore we may sleep,
> so we could not do that directly form set_brightness() method.
> Historically we used private work to adjust the brightness, but with the
> introduction of set_brightness_blocking() we no longer need it.
>
> As a bonus, not having our own work item means we do not have
> use-after-free issue as we neglected to cancel outstanding work on
> driver unbind.
>
> Reported-by: Sven Van Asbroeck <thesven73@gmail.com>
> Reviewed-by: Sven Van Asbroeck <TheSven73@googlemail.com>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>
> v2: no longer checking current brightness before trying to update since
> regmap should take care of caching and skipping updates when needed
> (Jacek).
>
> drivers/input/keyboard/cap11xx.c | 35 ++++++++++----------------------
> 1 file changed, 11 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/input/keyboard/cap11xx.c b/drivers/input/keyboard/cap11xx.c
> index 312916f99597..73686c2460ce 100644
> --- a/drivers/input/keyboard/cap11xx.c
> +++ b/drivers/input/keyboard/cap11xx.c
> @@ -75,9 +75,7 @@
> struct cap11xx_led {
> struct cap11xx_priv *priv;
> struct led_classdev cdev;
> - struct work_struct work;
> u32 reg;
> - enum led_brightness new_brightness;
> };
> #endif
>
> @@ -233,30 +231,21 @@ static void cap11xx_input_close(struct input_dev *idev)
> }
>
> #ifdef CONFIG_LEDS_CLASS
> -static void cap11xx_led_work(struct work_struct *work)
> +static int cap11xx_led_set(struct led_classdev *cdev,
> + enum led_brightness value)
> {
> - struct cap11xx_led *led = container_of(work, struct cap11xx_led, work);
> + struct cap11xx_led *led = container_of(cdev, struct cap11xx_led, cdev);
> struct cap11xx_priv *priv = led->priv;
> - int value = led->new_brightness;
>
> /*
> - * All LEDs share the same duty cycle as this is a HW limitation.
> - * Brightness levels per LED are either 0 (OFF) and 1 (ON).
> + * All LEDs share the same duty cycle as this is a HW
> + * limitation. Brightness levels per LED are either
> + * 0 (OFF) and 1 (ON).
> */
> - regmap_update_bits(priv->regmap, CAP11XX_REG_LED_OUTPUT_CONTROL,
> - BIT(led->reg), value ? BIT(led->reg) : 0);
> -}
> -
> -static void cap11xx_led_set(struct led_classdev *cdev,
> - enum led_brightness value)
> -{
> - struct cap11xx_led *led = container_of(cdev, struct cap11xx_led, cdev);
> -
> - if (led->new_brightness == value)
> - return;
> -
> - led->new_brightness = value;
> - schedule_work(&led->work);
> + return regmap_update_bits(priv->regmap,
> + CAP11XX_REG_LED_OUTPUT_CONTROL,
> + BIT(led->reg),
> + value ? BIT(led->reg) : 0);
> }
>
> static int cap11xx_init_leds(struct device *dev,
> @@ -299,7 +288,7 @@ static int cap11xx_init_leds(struct device *dev,
> led->cdev.default_trigger =
> of_get_property(child, "linux,default-trigger", NULL);
> led->cdev.flags = 0;
> - led->cdev.brightness_set = cap11xx_led_set;
> + led->cdev.brightness_set_blocking = cap11xx_led_set;
> led->cdev.max_brightness = 1;
> led->cdev.brightness = LED_OFF;
>
> @@ -312,8 +301,6 @@ static int cap11xx_init_leds(struct device *dev,
> led->reg = reg;
> led->priv = priv;
>
> - INIT_WORK(&led->work, cap11xx_led_work);
> -
> error = devm_led_classdev_register(dev, &led->cdev);
> if (error) {
> of_node_put(child);
>
Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
--
Best regards,
Jacek Anaszewski
^ permalink raw reply
* [RFC PATCH 2/2] Input: lm8323: remove recursive mutex lock/unlock
From: Sven Van Asbroeck @ 2019-02-08 19:34 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel
In-Reply-To: <20190208193434.5629-1-TheSven73@gmail.com>
Recursive mutex lock/unlock is not permitted. Remove.
Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
---
drivers/input/keyboard/lm8323.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/input/keyboard/lm8323.c b/drivers/input/keyboard/lm8323.c
index ea4ed1750eb5..4363c60f7845 100644
--- a/drivers/input/keyboard/lm8323.c
+++ b/drivers/input/keyboard/lm8323.c
@@ -359,11 +359,9 @@ static int lm8323_configure(struct lm8323_chip *lm)
static void pwm_done(struct lm8323_pwm *pwm)
{
- mutex_lock(&pwm->lock);
pwm->running = false;
if (pwm->desired_brightness != pwm->brightness)
led_set_brightness(&pwm->cdev, pwm->desired_brightness);
- mutex_unlock(&pwm->lock);
}
/*
--
2.17.1
^ permalink raw reply related
* [RFC PATCH 1/2] Input: lm8323: switch to using set_brightness_blocking
From: Sven Van Asbroeck @ 2019-02-08 19:34 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel
Disclaimer:
I cannot test this driver as I do not have any h/w.
This RFC patch is not intended as a ready-made solution,
but to provoke discussion.
Problem 1:
lm8323_pwm_work() could still run after the device has been removed, which
would result in a use-after-free.
Problem 2:
The brightness_set callback must not sleep, but in this case it grabs a
mutex, which can potentially sleep.
Solution:
lm8323_pwm_work() grabs a mutex and performs i2c accesses, therefore it may
sleep, and that is not allowed in brightness_set callback.
Use brightness_set_blocking callback instead. This has its own workqueue,
which is handled correctly on device removal. So the use-after-free
disappears. As a bonus, we no longer require a separate work_struct.
Question:
The original set_brightness had a separate code path when the device is in
suspend:
/*
* Schedule PWM work as usual unless we are going into suspend
*/
mutex_lock(&lm->lock);
if (likely(!lm->pm_suspend))
schedule_work(&pwm->work);
else
lm8323_pwm_work(&pwm->work);
mutex_unlock(&lm->lock);
Why was it there, and does the current led core code handle this case
correctly?
Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
---
drivers/input/keyboard/lm8323.c | 49 ++++++---------------------------
1 file changed, 8 insertions(+), 41 deletions(-)
diff --git a/drivers/input/keyboard/lm8323.c b/drivers/input/keyboard/lm8323.c
index 04a5d7e134d7..ea4ed1750eb5 100644
--- a/drivers/input/keyboard/lm8323.c
+++ b/drivers/input/keyboard/lm8323.c
@@ -137,7 +137,6 @@ struct lm8323_pwm {
bool running;
/* pwm lock */
struct mutex lock;
- struct work_struct work;
struct led_classdev cdev;
struct lm8323_chip *chip;
};
@@ -148,7 +147,6 @@ struct lm8323_chip {
struct i2c_client *client;
struct input_dev *idev;
bool kp_enabled;
- bool pm_suspend;
unsigned keys_down;
char phys[32];
unsigned short keymap[LM8323_KEYMAP_SIZE];
@@ -162,7 +160,6 @@ struct lm8323_chip {
#define client_to_lm8323(c) container_of(c, struct lm8323_chip, client)
#define dev_to_lm8323(d) container_of(d, struct lm8323_chip, client->dev)
#define cdev_to_pwm(c) container_of(c, struct lm8323_pwm, cdev)
-#define work_to_pwm(w) container_of(w, struct lm8323_pwm, work)
#define LM8323_MAX_DATA 8
@@ -365,7 +362,7 @@ static void pwm_done(struct lm8323_pwm *pwm)
mutex_lock(&pwm->lock);
pwm->running = false;
if (pwm->desired_brightness != pwm->brightness)
- schedule_work(&pwm->work);
+ led_set_brightness(&pwm->cdev, pwm->desired_brightness);
mutex_unlock(&pwm->lock);
}
@@ -450,15 +447,18 @@ static void lm8323_write_pwm(struct lm8323_pwm *pwm, int kill,
pwm->running = true;
}
-static void lm8323_pwm_work(struct work_struct *work)
+static int lm8323_pwm_set_brightness(struct led_classdev *led_cdev,
+ enum led_brightness brightness)
{
- struct lm8323_pwm *pwm = work_to_pwm(work);
+ struct lm8323_pwm *pwm = cdev_to_pwm(led_cdev);
int div512, perstep, steps, hz, up, kill;
u16 pwm_cmds[3];
int num_cmds = 0;
mutex_lock(&pwm->lock);
+ pwm->desired_brightness = brightness;
+
/*
* Do nothing if we're already at the requested level,
* or previous setting is not yet complete. In the latter
@@ -504,31 +504,7 @@ static void lm8323_pwm_work(struct work_struct *work)
out:
mutex_unlock(&pwm->lock);
-}
-
-static void lm8323_pwm_set_brightness(struct led_classdev *led_cdev,
- enum led_brightness brightness)
-{
- struct lm8323_pwm *pwm = cdev_to_pwm(led_cdev);
- struct lm8323_chip *lm = pwm->chip;
-
- mutex_lock(&pwm->lock);
- pwm->desired_brightness = brightness;
- mutex_unlock(&pwm->lock);
-
- if (in_interrupt()) {
- schedule_work(&pwm->work);
- } else {
- /*
- * Schedule PWM work as usual unless we are going into suspend
- */
- mutex_lock(&lm->lock);
- if (likely(!lm->pm_suspend))
- schedule_work(&pwm->work);
- else
- lm8323_pwm_work(&pwm->work);
- mutex_unlock(&lm->lock);
- }
+ return 0;
}
static ssize_t lm8323_pwm_show_time(struct device *dev,
@@ -579,13 +555,12 @@ static int init_pwm(struct lm8323_chip *lm, int id, struct device *dev,
pwm->desired_brightness = 0;
pwm->running = false;
pwm->enabled = false;
- INIT_WORK(&pwm->work, lm8323_pwm_work);
mutex_init(&pwm->lock);
pwm->chip = lm;
if (name) {
pwm->cdev.name = name;
- pwm->cdev.brightness_set = lm8323_pwm_set_brightness;
+ pwm->cdev.brightness_set_blocking = lm8323_pwm_set_brightness;
pwm->cdev.groups = lm8323_pwm_groups;
if (led_classdev_register(dev, &pwm->cdev) < 0) {
dev_err(dev, "couldn't register PWM %d\n", id);
@@ -799,10 +774,6 @@ static int lm8323_suspend(struct device *dev)
irq_set_irq_wake(client->irq, 0);
disable_irq(client->irq);
- mutex_lock(&lm->lock);
- lm->pm_suspend = true;
- mutex_unlock(&lm->lock);
-
for (i = 0; i < 3; i++)
if (lm->pwm[i].enabled)
led_classdev_suspend(&lm->pwm[i].cdev);
@@ -816,10 +787,6 @@ static int lm8323_resume(struct device *dev)
struct lm8323_chip *lm = i2c_get_clientdata(client);
int i;
- mutex_lock(&lm->lock);
- lm->pm_suspend = false;
- mutex_unlock(&lm->lock);
-
for (i = 0; i < 3; i++)
if (lm->pwm[i].enabled)
led_classdev_resume(&lm->pwm[i].cdev);
--
2.17.1
^ permalink raw reply related
* Re: [PATCH] Input: ps2-gpio - flush TX work when closing port
From: Sven Van Asbroeck @ 2019-02-08 16:21 UTC (permalink / raw)
To: Danilo Krummrich; +Cc: Dmitry Torokhov, linux-input, Linux Kernel Mailing List
In-Reply-To: <fad6ebf83d088d676f0ba825ad4572ed@dk-develop.de>
On Fri, Feb 8, 2019 at 10:51 AM Danilo Krummrich
<danilokrummrich@dk-develop.de> wrote:
>
> I agree with Dmitry
>
So do I, you guys are absolutely right.
As far as I can see, this patch fixes the user-after-free.
So, after Dmitry changes flush_work() to flush_delayed_work() :
Reviewed-by: Sven Van Asbroeck <TheSven73@gmail.com>
^ permalink raw reply
* [PATCH v2] Input: st1232 - switch to gpiod API
From: Martin Kepplinger @ 2019-02-08 16:15 UTC (permalink / raw)
To: dmitry.torokhov, linux-input; +Cc: linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2236 bytes --]
Use devm_gpiod_get_optional() and gpiod_set_value_cansleep() instead
of the old API. The st1232_ts_power() now passes on the inverted "poweron"
value to reflect the correct logical value.
Signed-off-by: Martin Kepplinger <martin.kepplinger@ginzinger.com>
---
ok. tested. If i get that right, assigning different gpio functions
would now be difficult, but afaik isn't needed anyways.
thanks
martin
revision history
----------------
v2: retain dts compatibility by allowing current reset-gpios setting
drivers/input/touchscreen/st1232.c | 22 ++++++++++------------
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/drivers/input/touchscreen/st1232.c b/drivers/input/touchscreen/st1232.c
index 634d6c243845..1fbc0847416b 100644
--- a/drivers/input/touchscreen/st1232.c
+++ b/drivers/input/touchscreen/st1232.c
@@ -45,7 +45,7 @@ struct st1232_ts_data {
struct i2c_client *client;
struct input_dev *input_dev;
struct dev_pm_qos_request low_latency_req;
- int reset_gpio;
+ struct gpio_desc *reset_gpio;
const struct st_chip_info *chip_info;
int read_buf_len;
u8 *read_buf;
@@ -142,8 +142,8 @@ static irqreturn_t st1232_ts_irq_handler(int irq, void *dev_id)
static void st1232_ts_power(struct st1232_ts_data *ts, bool poweron)
{
- if (gpio_is_valid(ts->reset_gpio))
- gpio_direction_output(ts->reset_gpio, poweron);
+ if (ts->reset_gpio)
+ gpiod_set_value_cansleep(ts->reset_gpio, !poweron);
}
static const struct st_chip_info st1232_chip_info = {
@@ -215,15 +215,13 @@ static int st1232_ts_probe(struct i2c_client *client,
ts->client = client;
ts->input_dev = input_dev;
- ts->reset_gpio = of_get_gpio(client->dev.of_node, 0);
- if (gpio_is_valid(ts->reset_gpio)) {
- error = devm_gpio_request(&client->dev, ts->reset_gpio, NULL);
- if (error) {
- dev_err(&client->dev,
- "Unable to request GPIO pin %d.\n",
- ts->reset_gpio);
- return error;
- }
+ ts->reset_gpio = devm_gpiod_get_optional(&client->dev, NULL,
+ GPIOD_OUT_HIGH);
+ if (IS_ERR(ts->reset_gpio)) {
+ error = PTR_ERR(ts->reset_gpio);
+ dev_err(&client->dev, "Unable to request GPIO pin: %d.\n",
+ error);
+ return error;
}
st1232_ts_power(ts, true);
--
2.20.1
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3616 bytes --]
^ permalink raw reply related
* Re: [PATCH] Input: ps2-gpio - flush TX work when closing port
From: Danilo Krummrich @ 2019-02-08 15:51 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Sven Van Asbroeck, linux-input, Linux Kernel Mailing List
In-Reply-To: <20190208073102.GA31622@dtor-ws>
On 2019-02-08 08:31, Dmitry Torokhov wrote:
> On Thu, Feb 07, 2019 at 06:03:03PM -0500, Sven Van Asbroeck wrote:
>> On Thu, Feb 7, 2019 at 5:27 PM Dmitry Torokhov
>> <dmitry.torokhov@gmail.com> wrote:
>> >
>> > + flush_work(&drvdata->tx_work.work);
>>
>> Would cancel_work_sync() be better than flush_work() ?
>
> No, because we want to have interrupt and gpios in a consistent state.
> If we cancel then we need to see if we should disable it or it may
> already be disabled, etc. This way we know it is enabled after
> flush_delayed_work() returns, and we need to disable it.
>
> Thanks.
I agree with Dmitry - thanks for the fix.
Acked-by: Danilo Krummrich <danilokrummrich@dk-develop.de>
^ permalink raw reply
* Re: [PATCH v4 01/10] dt-bindings: mfd: add DT bindings for max77650
From: Linus Walleij @ 2019-02-08 12:15 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Rob Herring, Mark Rutland, Dmitry Torokhov, Jacek Anaszewski,
Pavel Machek, Lee Jones, Sebastian Reichel, Liam Girdwood,
Greg Kroah-Hartman, linux-kernel@vger.kernel.org,
open list:GPIO SUBSYSTEM,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux Input, Linux LED Subsystem, Linux PM list,
Bartosz Golaszewski
In-Reply-To: <20190205091237.6448-2-brgl@bgdev.pl>
On Tue, Feb 5, 2019 at 10:12 AM Bartosz Golaszewski <brgl@bgdev.pl> 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>
This is a good solution!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ 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