* 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
* Re: [PATCH v4 07/10] gpio: max77650: add GPIO support
From: Linus Walleij @ 2019-02-08 11:22 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-8-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 GPIO support for max77650 mfd device. This PMIC exposes a single
> GPIO line.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Please merge this with the rest of the stuff through MFD!
Thanks,
Linus Walleij
^ permalink raw reply
* [PATCH (resend)] Input: uinput - Set name/phys to NULL before kfree().
From: Tetsuo Handa @ 2019-02-08 10:25 UTC (permalink / raw)
To: dmitry.torokhov, rydberg
Cc: syzbot, linux-input, linux-kernel, syzkaller-bugs
In-Reply-To: <47d5fdbe-120e-cf42-106f-b0cc0f2feb49@I-love.SAKURA.ne.jp>
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
Reported-by: syzbot <syzbot+f648cfb7e0b52bf7ae32@syzkaller.appspotmail.com>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
drivers/input/misc/uinput.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index 8ec483e8688b..131591b5babd 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -300,7 +300,9 @@ static void uinput_destroy_device(struct uinput_device *udev)
if (dev) {
name = dev->name;
+ dev->name = NULL;
phys = dev->phys;
+ dev->phys = NULL;
if (old_state == UIST_CREATED) {
uinput_flush_requests(udev);
input_unregister_device(dev);
--
2.17.1
^ permalink raw reply related
* Re: [PATCH 2/2] Input: st1232 - switch to gpiod API
From: Dmitry Torokhov @ 2019-02-08 8:05 UTC (permalink / raw)
To: Martin Kepplinger
Cc: Martin Kepplinger, devicetree, linux-input, robh+dt, mark.rutland,
linux-kernel
In-Reply-To: <83a86244-f40b-44b1-d7be-1fcbeb4c359e@ginzinger.com>
Hi Martin,
On Tue, Feb 05, 2019 at 11:20:16AM +0100, Martin Kepplinger wrote:
> On 29.01.19 11:23, Martin Kepplinger wrote:
> > From: Martin Kepplinger <martin.kepplinger@ginzinger.com>
> >
> > 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>
> > ---
> >
> > Tested and works. thanks for your help Dmitry,
> >
>
> is this what you had in mind? any problems or questions?
Yes, that is what I wanted, with one exception:
> > + ts->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
> > + GPIOD_OUT_HIGH);
This breaks compatibility with old DTSes, please try changing to:
devm_gpiod_get_optional(&client->dev, NULL, GPIOD_OUT_HIGH);
Thanks.
--
Dmitry
^ permalink raw reply
* [PATCH v2] Input: cap11xx - switch to using set_brightness_blocking()
From: Dmitry Torokhov @ 2019-02-08 7:57 UTC (permalink / raw)
To: Jacek Anaszewski, Sven Van Asbroeck; +Cc: linux-input, linux-kernel
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);
--
2.20.1.791.gb4d0f1c61a-goog
--
Dmitry
^ permalink raw reply related
* Re: [PATCH] input: keyboard: gpio-keys-polled: use input name from pdata if available
From: Dmitry Torokhov @ 2019-02-08 7:35 UTC (permalink / raw)
To: Enrico Weigelt, metux IT consult; +Cc: linux-kernel, linux-input
In-Reply-To: <1549559131-7423-1-git-send-email-info@metux.net>
Hi Enrico,
On Thu, Feb 07, 2019 at 06:05:31PM +0100, Enrico Weigelt, metux IT consult wrote:
> 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.
I thought the world is moving away from platform data and towards
OF/ACPI systems. What device are you targeting with this change?
I would want to convert gpio-keys[-polled] to generic device properties
and away form platform data...
>
> 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
>
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: ps2-gpio - flush TX work when closing port
From: Dmitry Torokhov @ 2019-02-08 7:31 UTC (permalink / raw)
To: Sven Van Asbroeck
Cc: linux-input, Danilo Krummrich, Linux Kernel Mailing List
In-Reply-To: <CAGngYiWzC92Lw+Z-BAtPfEHgys0vcQOxdxo7g_JHKHF36-Hm7Q@mail.gmail.com>
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.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: ps2-gpio - flush TX work when closing port
From: Sven Van Asbroeck @ 2019-02-07 23:03 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, Danilo Krummrich, Linux Kernel Mailing List
In-Reply-To: <20190207222740.GA38612@dtor-ws>
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() ?
^ permalink raw reply
* [PATCH] Input: matrix_keypad - use flush_delayed_work()
From: Dmitry Torokhov @ 2019-02-07 22:46 UTC (permalink / raw)
To: linux-input; +Cc: Sven Van Asbroeck, tj, linux-kernel
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).
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/keyboard/matrix_keypad.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/keyboard/matrix_keypad.c b/drivers/input/keyboard/matrix_keypad.c
index 403452ef00e6..3d1cb7bf5e35 100644
--- a/drivers/input/keyboard/matrix_keypad.c
+++ b/drivers/input/keyboard/matrix_keypad.c
@@ -222,7 +222,7 @@ static void matrix_keypad_stop(struct input_dev *dev)
keypad->stopped = true;
spin_unlock_irq(&keypad->lock);
- flush_work(&keypad->work.work);
+ flush_delayed_work(&keypad->work);
/*
* matrix_keypad_scan() will leave IRQs enabled;
* we should disable them now.
--
2.20.1.611.gfbb209baf1-goog
--
Dmitry
^ permalink raw reply related
* Re: [PATCH] Input: ps2-gpio - flush TX work when closing port
From: Dmitry Torokhov @ 2019-02-07 22:31 UTC (permalink / raw)
To: linux-input; +Cc: Sven Van Asbroeck, Danilo Krummrich, linux-kernel
In-Reply-To: <20190207222740.GA38612@dtor-ws>
On Thu, Feb 07, 2019 at 02:27:40PM -0800, Dmitry Torokhov wrote:
> To ensure that TX work is not running after serio port has been torn down,
> let's flush it when closing the port.
>
> Reported-by: Sven Van Asbroeck <thesven73@gmail.com>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> drivers/input/serio/ps2-gpio.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/input/serio/ps2-gpio.c b/drivers/input/serio/ps2-gpio.c
> index c62cceb97bb1..9e1dbde2e15b 100644
> --- a/drivers/input/serio/ps2-gpio.c
> +++ b/drivers/input/serio/ps2-gpio.c
> @@ -76,6 +76,7 @@ static void ps2_gpio_close(struct serio *serio)
> {
> struct ps2_gpio_data *drvdata = serio->port_data;
>
> + flush_work(&drvdata->tx_work.work);
Ah, we have flush_delayed_work() now, I'll change it before committing
once we agree on the patch in principle.
> disable_irq(drvdata->irq);
> }
>
> --
> 2.20.1.611.gfbb209baf1-goog
>
>
> --
> Dmitry
--
Dmitry
^ permalink raw reply
* [PATCH] Input: ps2-gpio - flush TX work when closing port
From: Dmitry Torokhov @ 2019-02-07 22:27 UTC (permalink / raw)
To: linux-input; +Cc: Sven Van Asbroeck, Danilo Krummrich, linux-kernel
To ensure that TX work is not running after serio port has been torn down,
let's flush it when closing the port.
Reported-by: Sven Van Asbroeck <thesven73@gmail.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/serio/ps2-gpio.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/input/serio/ps2-gpio.c b/drivers/input/serio/ps2-gpio.c
index c62cceb97bb1..9e1dbde2e15b 100644
--- a/drivers/input/serio/ps2-gpio.c
+++ b/drivers/input/serio/ps2-gpio.c
@@ -76,6 +76,7 @@ static void ps2_gpio_close(struct serio *serio)
{
struct ps2_gpio_data *drvdata = serio->port_data;
+ flush_work(&drvdata->tx_work.work);
disable_irq(drvdata->irq);
}
--
2.20.1.611.gfbb209baf1-goog
--
Dmitry
^ permalink raw reply related
* Re: [PATCH v4 4/9] platform/chrome: rtc: Add RTC driver
From: Alexandre Belloni @ 2019-02-07 19:50 UTC (permalink / raw)
To: Nick Crews
Cc: linux-kernel, sjg, dmitry.torokhov, sre, linux-input, groeck,
dlaurie, Duncan Laurie, Enric Balletbo i Serra, linux-rtc,
Alessandro Zummo, Benson Leung
In-Reply-To: <20190123183325.92946-5-ncrews@chromium.org>
On 23/01/2019 11:33:20-0700, Nick Crews wrote:
> From: Duncan Laurie <dlaurie@google.com>
>
> This Embedded Controller has an internal RTC that is exposed
> as a standard RTC class driver with read/write functionality.
>
> The driver is added to the drivers/rtc/ so that the maintainer of that
> directory will be able to comment on this change, as that maintainer is
> the expert on this system. In addition, the driver code is called
> indirectly after a corresponding device is registered from core.c,
> as opposed to core.c registering the driver callbacks directly.
>
> > hwclock --show --rtc /dev/rtc1
> 2007-12-31 16:01:20.460959-08:00
> > hwclock --systohc --rtc /dev/rtc1
> > hwclock --show --rtc /dev/rtc1
> 2018-11-29 17:08:00.780793-08:00
>
> Signed-off-by: Duncan Laurie <dlaurie@google.com>
> Acked-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> Signed-off-by: Nick Crews <ncrews@chromium.org>
Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> ---
>
> Changes in v4:
> - Change me email to @chromium.org from @google.com
> - Move "Add RTC driver" before "Add sysfs attributes" so that
> it could get accepted earlier, since it is less contentious
>
> Changes in v3:
> - rm #define for driver name
> - Don't compute weekday when reading from RTC.
> Still set weekday when writing, as RTC needs this
> to control advanced power scheduling
> - rm check for invalid month data
> - Set range_min and range_max on rtc_device
>
> Changes in v2:
> - rm license boiler plate
> - rm "wilco_ec_rtc -" prefix in docstring
> - Make rtc driver its own module within the drivers/rtc/ directory
> - Register a rtc device from core.c that is picked up by this driver
>
> drivers/platform/chrome/wilco_ec/core.c | 14 ++
> drivers/rtc/Kconfig | 11 ++
> drivers/rtc/Makefile | 1 +
> drivers/rtc/rtc-wilco-ec.c | 177 ++++++++++++++++++++++++
> 4 files changed, 203 insertions(+)
> create mode 100644 drivers/rtc/rtc-wilco-ec.c
>
> diff --git a/drivers/platform/chrome/wilco_ec/core.c b/drivers/platform/chrome/wilco_ec/core.c
> index e036d88b6dd8..7cfb047e2c89 100644
> --- a/drivers/platform/chrome/wilco_ec/core.c
> +++ b/drivers/platform/chrome/wilco_ec/core.c
> @@ -87,6 +87,20 @@ static int wilco_ec_probe(struct platform_device *pdev)
> goto destroy_mec;
> }
>
> + /*
> + * Register a RTC platform device that will get picked up by the RTC
> + * subsystem. This platform device will get freed when its parent device
> + * dev is unregistered.
> + */
> + child_pdev = platform_device_register_data(dev, "rtc-wilco-ec",
> + PLATFORM_DEVID_AUTO,
> + NULL, 0);
> + if (IS_ERR(child_pdev)) {
> + dev_err(dev, "Failed to create RTC platform device\n");
> + ret = PTR_ERR(child_pdev);
> + goto destroy_mec;
> + }
> +
> return 0;
>
> destroy_mec:
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 225b0b8516f3..d5063c791515 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -1814,4 +1814,15 @@ config RTC_DRV_GOLDFISH
> Goldfish is a code name for the virtual platform developed by Google
> for Android emulation.
>
> +config RTC_DRV_WILCO_EC
> + tristate "Wilco EC RTC"
> + depends on WILCO_EC
> + default m
> + help
> + If you say yes here, you get read/write support for the Real Time
> + Clock on the Wilco Embedded Controller (Wilco is a kind of Chromebook)
> +
> + This can also be built as a module. If so, the module will
> + be named "rtc_wilco_ec".
> +
> endif # RTC_CLASS
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index df022d820bee..6255ea78da25 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -172,6 +172,7 @@ obj-$(CONFIG_RTC_DRV_V3020) += rtc-v3020.o
> obj-$(CONFIG_RTC_DRV_VR41XX) += rtc-vr41xx.o
> obj-$(CONFIG_RTC_DRV_VRTC) += rtc-mrst.o
> obj-$(CONFIG_RTC_DRV_VT8500) += rtc-vt8500.o
> +obj-$(CONFIG_RTC_DRV_WILCO_EC) += rtc-wilco-ec.o
> obj-$(CONFIG_RTC_DRV_WM831X) += rtc-wm831x.o
> obj-$(CONFIG_RTC_DRV_WM8350) += rtc-wm8350.o
> obj-$(CONFIG_RTC_DRV_X1205) += rtc-x1205.o
> diff --git a/drivers/rtc/rtc-wilco-ec.c b/drivers/rtc/rtc-wilco-ec.c
> new file mode 100644
> index 000000000000..35cc56114c1c
> --- /dev/null
> +++ b/drivers/rtc/rtc-wilco-ec.c
> @@ -0,0 +1,177 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * RTC interface for Wilco Embedded Controller with R/W abilities
> + *
> + * Copyright 2018 Google LLC
> + *
> + * The corresponding platform device is typically registered in
> + * drivers/platform/chrome/wilco_ec/core.c
> + */
> +
> +#include <linux/bcd.h>
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/platform_data/wilco-ec.h>
> +#include <linux/rtc.h>
> +#include <linux/timekeeping.h>
> +
> +#define EC_COMMAND_CMOS 0x7c
> +#define EC_CMOS_TOD_WRITE 0x02
> +#define EC_CMOS_TOD_READ 0x08
> +
> +/**
> + * struct ec_rtc_read - Format of RTC returned by EC.
> + * @second: Second value (0..59)
> + * @minute: Minute value (0..59)
> + * @hour: Hour value (0..23)
> + * @day: Day value (1..31)
> + * @month: Month value (1..12)
> + * @year: Year value (full year % 100)
> + * @century: Century value (full year / 100)
> + *
> + * All values are presented in binary (not BCD).
> + */
> +struct ec_rtc_read {
> + u8 second;
> + u8 minute;
> + u8 hour;
> + u8 day;
> + u8 month;
> + u8 year;
> + u8 century;
> +} __packed;
> +
> +/**
> + * struct ec_rtc_write - Format of RTC sent to the EC.
> + * @param: EC_CMOS_TOD_WRITE
> + * @century: Century value (full year / 100)
> + * @year: Year value (full year % 100)
> + * @month: Month value (1..12)
> + * @day: Day value (1..31)
> + * @hour: Hour value (0..23)
> + * @minute: Minute value (0..59)
> + * @second: Second value (0..59)
> + * @weekday: Day of the week (0=Saturday)
> + *
> + * All values are presented in BCD.
> + */
> +struct ec_rtc_write {
> + u8 param;
> + u8 century;
> + u8 year;
> + u8 month;
> + u8 day;
> + u8 hour;
> + u8 minute;
> + u8 second;
> + u8 weekday;
> +} __packed;
> +
> +int wilco_ec_rtc_read(struct device *dev, struct rtc_time *tm)
> +{
> + struct wilco_ec_device *ec = dev_get_drvdata(dev->parent);
> + u8 param = EC_CMOS_TOD_READ;
> + struct ec_rtc_read rtc;
> + struct wilco_ec_message msg = {
> + .type = WILCO_EC_MSG_LEGACY,
> + .flags = WILCO_EC_FLAG_RAW_RESPONSE,
> + .command = EC_COMMAND_CMOS,
> + .request_data = ¶m,
> + .request_size = sizeof(param),
> + .response_data = &rtc,
> + .response_size = sizeof(rtc),
> + };
> + int ret;
> +
> + ret = wilco_ec_mailbox(ec, &msg);
> + if (ret < 0)
> + return ret;
> +
> + tm->tm_sec = rtc.second;
> + tm->tm_min = rtc.minute;
> + tm->tm_hour = rtc.hour;
> + tm->tm_mday = rtc.day;
> + tm->tm_mon = rtc.month - 1;
> + tm->tm_year = rtc.year + (rtc.century * 100) - 1900;
> + tm->tm_yday = rtc_year_days(tm->tm_mday, tm->tm_mon, tm->tm_year);
> +
> + /* Don't compute day of week, we don't need it. */
> + tm->tm_wday = -1;
> +
> + return 0;
> +}
> +
> +int wilco_ec_rtc_write(struct device *dev, struct rtc_time *tm)
> +{
> + struct wilco_ec_device *ec = dev_get_drvdata(dev->parent);
> + struct ec_rtc_write rtc;
> + struct wilco_ec_message msg = {
> + .type = WILCO_EC_MSG_LEGACY,
> + .flags = WILCO_EC_FLAG_RAW_RESPONSE,
> + .command = EC_COMMAND_CMOS,
> + .request_data = &rtc,
> + .request_size = sizeof(rtc),
> + };
> + int year = tm->tm_year + 1900;
> + /*
> + * Convert from 0=Sunday to 0=Saturday for the EC
> + * We DO need to set weekday because the EC controls battery charging
> + * schedules that depend on the day of the week.
> + */
> + int wday = tm->tm_wday == 6 ? 0 : tm->tm_wday + 1;
> + int ret;
> +
> + rtc.param = EC_CMOS_TOD_WRITE;
> + rtc.century = bin2bcd(year / 100);
> + rtc.year = bin2bcd(year % 100);
> + rtc.month = bin2bcd(tm->tm_mon + 1);
> + rtc.day = bin2bcd(tm->tm_mday);
> + rtc.hour = bin2bcd(tm->tm_hour);
> + rtc.minute = bin2bcd(tm->tm_min);
> + rtc.second = bin2bcd(tm->tm_sec);
> + rtc.weekday = bin2bcd(wday);
> +
> + ret = wilco_ec_mailbox(ec, &msg);
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +static const struct rtc_class_ops wilco_ec_rtc_ops = {
> + .read_time = wilco_ec_rtc_read,
> + .set_time = wilco_ec_rtc_write,
> +};
> +
> +static int wilco_ec_rtc_probe(struct platform_device *pdev)
> +{
> + struct rtc_device *rtc;
> +
> + rtc = devm_rtc_allocate_device(&pdev->dev);
> + if (IS_ERR(rtc))
> + return PTR_ERR(rtc);
> +
> + rtc->ops = &wilco_ec_rtc_ops;
> + /* EC only supports this century */
> + rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
> + rtc->range_max = RTC_TIMESTAMP_END_2099;
> + rtc->owner = THIS_MODULE;
> +
> + return rtc_register_device(rtc);
> +}
> +
> +static struct platform_driver wilco_ec_rtc_driver = {
> + .driver = {
> + .name = "rtc-wilco-ec",
> + },
> + .probe = wilco_ec_rtc_probe,
> +};
> +
> +module_platform_driver(wilco_ec_rtc_driver);
> +
> +MODULE_ALIAS("platform:rtc-wilco-ec");
> +MODULE_AUTHOR("Nick Crews <ncrews@chromium.org>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Wilco EC RTC driver");
> --
> 2.20.1.321.g9e740568ce-goog
>
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.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-07 17:05 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: tm2-touchkey - acknowledge that setting brightness is a blocking call
From: Andi Shyti @ 2019-02-07 7:42 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-input, Paweł Chmiel, Jonathan Bakker, Andi Shyti,
linux-kernel
In-Reply-To: <20190206181616.GA204279@dtor-ws>
Hi Dmitry,
On Wed, Feb 06, 2019 at 10:16:16AM -0800, Dmitry Torokhov wrote:
> We need to access I2C bus when switching brightness, and that may block,
> therefore we have to set stmfts_brightness_set() as LED's
> brightness_set_blocking() method.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
same here:
Acked-by: Andi Shyti <andi@etezian.org>
Thanks,
Andi
^ permalink raw reply
* Re: [PATCH] Input: stmfts - acknowledge that setting brightness is a blocking call
From: Andi Shyti @ 2019-02-07 7:36 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, Andi Shyti, Jacek Anaszewski, linux-kernel
In-Reply-To: <20190205224642.GA182156@dtor-ws>
Hi Dmitry,
On Tue, Feb 05, 2019 at 02:46:42PM -0800, Dmitry Torokhov wrote:
> We need to turn regulators on and off when switching brightness, and
> that may block, therefore we have to set stmfts_brightness_set() as
> LED's brightness_set_blocking() method.
>
> Fixes: 78bcac7b2ae1 ("Input: add support for the STMicroelectronics FingerTip touchscreen")
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Acked-by: Andi Shyti <andi@etezian.org>
Thanks,
^ permalink raw reply
* [PATCH] Input: ili210x - switch to using devm_device_add_group()
From: Dmitry Torokhov @ 2019-02-07 6:27 UTC (permalink / raw)
To: linux-input; +Cc: Marek Vasut, linux-kernel
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);
--
2.20.1.611.gfbb209baf1-goog
--
Dmitry
^ permalink raw reply related
* [PATCH] HID: steam: fix boot loop with bluetooth firmware.
From: Rodrigo Rivas Costa @ 2019-02-06 21:27 UTC (permalink / raw)
To: Benjamin Tissoires, Jiri Kosina, Pierre-Loup A. Griffais, lkml,
linux-input
Cc: Rodrigo Rivas Costa
There is a new firmware for the Steam Controller with support for BLE
connections. When using such a device with a wired connection, it
reboots itself every 10 seconds unless an application has opened it.
Doing hid_hw_open() unconditionally on probe fixes the issue, and the
code becomes simpler.
Signed-off-by: Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>
---
drivers/hid/hid-steam.c | 34 +++++++++++-----------------------
1 file changed, 11 insertions(+), 23 deletions(-)
diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index dc4128bfe2ca..8141cadfca0e 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -283,11 +283,6 @@ static void steam_set_lizard_mode(struct steam_device *steam, bool enable)
static int steam_input_open(struct input_dev *dev)
{
struct steam_device *steam = input_get_drvdata(dev);
- int ret;
-
- ret = hid_hw_open(steam->hdev);
- if (ret)
- return ret;
mutex_lock(&steam->mutex);
if (!steam->client_opened && lizard_mode)
@@ -304,8 +299,6 @@ static void steam_input_close(struct input_dev *dev)
if (!steam->client_opened && lizard_mode)
steam_set_lizard_mode(steam, true);
mutex_unlock(&steam->mutex);
-
- hid_hw_close(steam->hdev);
}
static enum power_supply_property steam_battery_props[] = {
@@ -623,11 +616,6 @@ static void steam_client_ll_stop(struct hid_device *hdev)
static int steam_client_ll_open(struct hid_device *hdev)
{
struct steam_device *steam = hdev->driver_data;
- int ret;
-
- ret = hid_hw_open(steam->hdev);
- if (ret)
- return ret;
mutex_lock(&steam->mutex);
steam->client_opened = true;
@@ -635,7 +623,7 @@ static int steam_client_ll_open(struct hid_device *hdev)
steam_input_unregister(steam);
- return ret;
+ return 0;
}
static void steam_client_ll_close(struct hid_device *hdev)
@@ -646,7 +634,6 @@ static void steam_client_ll_close(struct hid_device *hdev)
steam->client_opened = false;
mutex_unlock(&steam->mutex);
- hid_hw_close(steam->hdev);
if (steam->connected) {
steam_set_lizard_mode(steam, lizard_mode);
steam_input_register(steam);
@@ -759,14 +746,15 @@ static int steam_probe(struct hid_device *hdev,
if (ret)
goto client_hdev_add_fail;
+ ret = hid_hw_open(hdev);
+ if (ret) {
+ hid_err(hdev,
+ "%s:hid_hw_open\n",
+ __func__);
+ goto hid_hw_open_fail;
+ }
+
if (steam->quirks & STEAM_QUIRK_WIRELESS) {
- ret = hid_hw_open(hdev);
- if (ret) {
- hid_err(hdev,
- "%s:hid_hw_open for wireless\n",
- __func__);
- goto hid_hw_open_fail;
- }
hid_info(hdev, "Steam wireless receiver connected");
steam_request_conn_status(steam);
} else {
@@ -781,8 +769,8 @@ static int steam_probe(struct hid_device *hdev,
return 0;
-hid_hw_open_fail:
input_register_fail:
+hid_hw_open_fail:
client_hdev_add_fail:
hid_hw_stop(hdev);
hid_hw_start_fail:
@@ -809,8 +797,8 @@ static void steam_remove(struct hid_device *hdev)
cancel_work_sync(&steam->work_connect);
if (steam->quirks & STEAM_QUIRK_WIRELESS) {
hid_info(hdev, "Steam wireless receiver disconnected");
- hid_hw_close(hdev);
}
+ hid_hw_close(hdev);
hid_hw_stop(hdev);
steam_unregister(steam);
}
--
2.20.1
^ permalink raw reply related
* Re: [PATCH 2/2] Input: add Apple SPI keyboard and trackpad driver.
From: Andy Shevchenko @ 2019-02-06 20:22 UTC (permalink / raw)
To: Ronald Tschalär
Cc: Dmitry Torokhov, Henrik Rydberg, Lukas Wunner, Federico Lorenzi,
linux-input, linux-kernel
In-Reply-To: <20190204081947.25152-3-ronald@innovation.ch>
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.
> +/**
I'm not sure it's kernel doc format comment.
> + * The keyboard and touchpad controller on the MacBookAir6, MacBookPro12,
> + * MacBook8 and newer can be driven either by USB or SPI. However the USB
> + * pins are only connected on the MacBookAir6 and 7 and the MacBookPro12.
> + * All others need this driver. The interface is selected using ACPI methods:
> + *
> + * * UIEN ("USB Interface Enable"): If invoked with argument 1, disables SPI
> + * and enables USB. If invoked with argument 0, disables USB.
> + * * UIST ("USB Interface Status"): Returns 1 if USB is enabled, 0 otherwise.
> + * * SIEN ("SPI Interface Enable"): If invoked with argument 1, disables USB
> + * and enables SPI. If invoked with argument 0, disables SPI.
> + * * SIST ("SPI Interface Status"): Returns 1 if SPI is enabled, 0 otherwise.
> + * * ISOL: Resets the four GPIO pins used for SPI. Intended to be invoked with
> + * argument 1, then once more with argument 0.
> + *
> + * UIEN and UIST are only provided on models where the USB pins are connected.
> + *
> + * SPI-based Protocol
> + * ------------------
> + *
> + * The device and driver exchange messages (struct message); each message is
> + * encapsulated in one or more packets (struct spi_packet). There are two types
> + * of exchanges: reads, and writes. A read is signaled by a GPE, upon which one
> + * message can be read from the device. A write exchange consists of writing a
> + * command message, immediately reading a short status packet, and then, upon
> + * receiving a GPE, reading the response message. Write exchanges cannot be
> + * interleaved, i.e. a new write exchange must not be started till the previous
> + * write exchange is complete. Whether a received message is part of a read or
> + * write exchange is indicated in the encapsulating packet's flags field.
> + *
> + * A single message may be too large to fit in a single packet (which has a
> + * fixed, 256-byte size). In that case it will be split over multiple,
> + * consecutive packets.
> + */
> +
> +#define pr_fmt(fmt) "applespi: " fmt
Better to use usual pattern.
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +#include <linux/acpi.h>
> +#include <linux/spi/spi.h>
> +#include <linux/interrupt.h>
> +#include <linux/property.h>
> +#include <linux/delay.h>
> +#include <linux/spinlock.h>
> +#include <linux/crc16.h>
> +#include <linux/wait.h>
> +#include <linux/leds.h>
> +#include <linux/ktime.h>
> +#include <linux/input.h>
> +#include <linux/input/mt.h>
> +#include <linux/input-polldev.h>
> +#include <linux/workqueue.h>
> +#include <linux/efi.h>
Can we keep them sorted? Do you need, btw, all of them?
+ blank line.
> +#include <asm/barrier.h>
> +#define MIN_KBD_BL_LEVEL 32
> +#define MAX_KBD_BL_LEVEL 255
I would rather use similar pattern as below, i.e. KBD_..._MIN/_MAX.
> +#define KBD_BL_LEVEL_SCALE 1000000
> +#define KBD_BL_LEVEL_ADJ \
> + ((MAX_KBD_BL_LEVEL - MIN_KBD_BL_LEVEL) * KBD_BL_LEVEL_SCALE / 255)
> +#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.
> +#define SPI_RW_CHG_DLY 100 /* from experimentation, in us */
In _US would be good to have in a constant name, i.e.
SPI_RW_CHG_DELAY_US
> +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 ?
Ditto for the rest.
Btw, do we need all these parameters? Can't we do modify behaviour run-time
using some Input framework facilities?
> +static unsigned int iso_layout;
> +module_param(iso_layout, uint, 0644);
> +MODULE_PARM_DESC(iso_layout, "Enable/Disable hardcoded ISO-layout of the keyboard. ([0] = disabled, 1 = enabled)");
bool ?
> +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.
(But still same question, wouldn't input subsystem allows to do this run-time?)
> +/**
> + * 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.
> + */
> +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?
> + __le16 crc_16;
Can't you use crc16 everywhere for the name?
> +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?
> + { },
Terminators are better without comma.
> + 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 ?
Perhaps "Unrecognized ... "-ish better?
> + }
> +static inline bool applespi_check_write_status(struct applespi_data *applespi,
> + int sts)
Indentation broken.
> +{
> + static u8 sts_ok[] = { 0xac, 0x27, 0x68, 0xd5 };
Spell it fully, i.e. status_ok.
> + bool ret = true;
Directly return from each branch, it also will make 'else' redundant.
> +
> + if (sts < 0) {
> + ret = false;
> + pr_warn("Error writing to device: %d\n", sts);
> + } else if (memcmp(applespi->tx_status, sts_ok,
> + APPLESPI_STATUS_SIZE) != 0) {
Redundant ' != 0' part.
After removing this and 'else' would be fit on one line.
> + ret = false;
> + pr_warn("Error writing to device: %x %x %x %x\n",
> + applespi->tx_status[0], applespi->tx_status[1],
> + applespi->tx_status[2], applespi->tx_status[3]);
pr_warn("...: %ph\n", applespi->tx_status);
> + }
> +
> + return ret;
> +}
> +static int applespi_enable_spi(struct applespi_data *applespi)
> +{
> + int result;
Sometimes you have ret, sometimes this. Better to unify across the code.
> + unsigned long long spi_status;
> + return 0;
> +}
> +static void applespi_async_write_complete(void *context)
> +{
> + struct applespi_data *applespi = context;
> + if (!applespi_check_write_status(applespi, applespi->wr_m.status))
> + /*
> + * If we got an error, we presumably won't get the expected
> + * response message either.
> + */
> + applespi_msg_complete(applespi, true, false);
Style issue, either use {} or positive condition like
if (...)
return;
...
> +}
> +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?
> + message->counter = applespi->cmd_msg_cntr++ & 0xff;
Usual pattern for this kind of entries is
x = ... % 256;
Btw, isn't 256 is defined somewhere?
> + crc = crc16(0, (u8 *)message, le16_to_cpu(packet->length) - 2);
> + *((__le16 *)&message->data[msg_len - 2]) = cpu_to_le16(crc);
put_unaligned_le16() ?
> + if (sts != 0) {
> + pr_warn("Error queueing async write to device: %d\n", sts);
> + } else {
> + applespi->cmd_msg_queued = true;
> + applespi->write_active = true;
> + }
Usual pattern is
if (sts) {
...
return sts;
}
...
return 0;
Btw, consider to use dev_warn() and Co instead of pr_warn() or in cases where
appropriate acpi_handle_warn(), etc.
> +
> + return sts;
> +}
> +static void applespi_init(struct applespi_data *applespi, bool is_resume)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
> +
> + if (!is_resume)
> + applespi->want_tp_info_cmd = true;
> + else
> + applespi->want_mt_init_cmd = true;
Why not positive conditional?
> + applespi_send_cmd_msg(applespi);
> +
> + spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
> +}
> +static void applespi_set_bl_level(struct led_classdev *led_cdev,
> + enum led_brightness value)
> +{
> + struct applespi_data *applespi =
> + container_of(led_cdev, struct applespi_data, backlight_info);
> + unsigned long flags;
> + int sts;
> +
> + spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
> +
> + if (value == 0)
> + applespi->want_bl_level = value;
> + else
> + /*
> + * The backlight does not turn on till level 32, so we scale
> + * the range here so that from a user's perspective it turns
> + * on at 1.
> + */
> + applespi->want_bl_level = (unsigned int)
> + ((value * KBD_BL_LEVEL_ADJ) / KBD_BL_LEVEL_SCALE +
> + MIN_KBD_BL_LEVEL);
Why do you need casting? Your defines better to have U or UL suffixes where
appropriate.
Besides, run checkpatch.pl!
> +
> + sts = applespi_send_cmd_msg(applespi);
> +
> + spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
> +}
> +static int applespi_event(struct input_dev *dev, unsigned int type,
> + unsigned int code, int value)
> +{
> + struct applespi_data *applespi = input_get_drvdata(dev);
> +
> + switch (type) {
> + case EV_LED:
> + applespi_set_capsl_led(applespi,
> + !!test_bit(LED_CAPSL, dev->led));
I would put it on one line disregard checkpatch warnings.
> + return 0;
> + }
> +
> + return -1;
Can't you use appropriate Linux error code?
> +}
> +/* 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 ?
> +static void report_tp_state(struct applespi_data *applespi,
> + struct touchpad_protocol *t)
> +{
> + static int min_x, max_x, min_y, max_y;
> + static bool dim_updated;
> + static ktime_t last_print;
> +
Redundant.
> + 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++) {
?
> + f = &t->fingers[i];
> + if (raw2int(f->touch_major) == 0)
> + continue;
> + 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++;
> +
> + if (debug & DBG_TP_DIM) {
> + #define UPDATE_DIMENSIONS(val, op, last) \
> + do { \
> + if (raw2int(val) op last) { \
> + last = raw2int(val); \
> + dim_updated = true; \
> + } \
> + } while (0)
> +
> + UPDATE_DIMENSIONS(f->abs_x, <, min_x);
> + UPDATE_DIMENSIONS(f->abs_x, >, max_x);
> + UPDATE_DIMENSIONS(f->abs_y, <, min_y);
> + UPDATE_DIMENSIONS(f->abs_y, >, max_y);
#undef ...
> + }
...and better to move this to separate helper function.
> + }
> +
> + if (debug & DBG_TP_DIM) {
> + if (dim_updated &&
> + ktime_ms_delta(ktime_get(), last_print) > 1000) {
> + printk(KERN_DEBUG
> + pr_fmt("New touchpad dimensions: %d %d %d %d\n"),
> + min_x, max_x, min_y, max_y);
> + dim_updated = false;
> + last_print = ktime_get();
> + }
> + }
Same, helper function.
> +
> + input_mt_assign_slots(input, applespi->slots, applespi->pos, n, 0);
> +
> + for (i = 0; i < n; i++)
> + report_finger_data(input, applespi->slots[i],
> + &applespi->pos[i], &t->fingers[i]);
> +
> + input_mt_sync_frame(input);
> + input_report_key(input, BTN_LEFT, t->clicked);
> +
> + input_sync(input);
> +}
> +
> +static unsigned int applespi_code_to_key(u8 code, int fn_pressed)
> +{
> + unsigned int key = applespi_scancodes[code];
> + const struct applespi_key_translation *trans;
> +
> + if (fnmode) {
> + int do_translate;
> +
> + trans = applespi_find_translation(applespi_fn_codes, key);
> + if (trans) {
> + if (trans->flags & APPLE_FLAG_FKEY)
> + do_translate = (fnmode == 2 && fn_pressed) ||
> + (fnmode == 1 && !fn_pressed);
> + else
> + do_translate = fn_pressed;
> +
> + if (do_translate)
> + key = trans->to;
> + }
> + }
> +
> + if (iso_layout) {
> + trans = applespi_find_translation(apple_iso_keyboard, key);
> + if (trans)
> + key = trans->to;
> + }
I would split this to three helpers like
static unsigned int ..._code_to_fn_key()
{
}
static unsigned int ..._code_to_iso_key()
{
}
static unsigned int ..._code_to_key()
{
if (fnmode)
key = ..._code_to_fn_key();
if (iso_layout)
key = ..._code_to_iso_key();
return key;
}
> +
> + return key;
> +}
> +static void applespi_remap_fn_key(struct keyboard_protocol
> + *keyboard_protocol)
Better to split like
static void
fn(struct ...);
> +{
> + unsigned char tmp;
> + unsigned long *modifiers = (unsigned long *)
> + &keyboard_protocol->modifiers;
Don't split casting from the rest, better
... var =
(type)...;
> +
> + if (!fnremap || fnremap > ARRAY_SIZE(applespi_controlcodes) ||
> + !applespi_controlcodes[fnremap - 1])
> + return;
> +
> + tmp = keyboard_protocol->fn_pressed;
> + keyboard_protocol->fn_pressed = test_bit(fnremap - 1, modifiers);
> + if (tmp)
> + __set_bit(fnremap - 1, modifiers);
> + else
> + __clear_bit(fnremap - 1, modifiers);
> +}
> +
> +static void applespi_handle_keyboard_event(struct applespi_data *applespi,
> + struct keyboard_protocol
> + *keyboard_protocol)
Put this to one line, consider reformat like I mentioned above.
> +{
> + if (!still_pressed) {
> + key = applespi_code_to_key(
> + applespi->last_keys_pressed[i],
> + applespi->last_keys_fn_pressed[i]);
> + input_report_key(applespi->keyboard_input_dev, key, 0);
> + applespi->last_keys_fn_pressed[i] = 0;
> + }
This could come as
if (...)
continue;
...
> + }
> + 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) ?
> +}
> +static void applespi_register_touchpad_device(struct applespi_data *applespi,
> + struct touchpad_info_protocol *rcvd_tp_info)
> +{
> + /* create touchpad input device */
> + touchpad_input_dev = devm_input_allocate_device(&applespi->spi->dev);
> +
Redundant.
> + if (!touchpad_input_dev) {
> + pr_err("Failed to allocate touchpad input device\n");
dev_err();
> + return;
Shouldn't we return an error to a caller?
> + }
> + /* register input device */
> + res = input_register_device(touchpad_input_dev);
> + if (res)
> + pr_err("Unabled to register touchpad input device (%d)\n", res);
> + else
if (ret) {
dev_err(...);
return ret;
}
Btw, ret, res, sts, result, ... Choose one.
> + /* touchpad_input_dev is read async in spi callback */
> + smp_store_release(&applespi->touchpad_input_dev,
> + touchpad_input_dev);
> +}
> +static bool applespi_verify_crc(struct applespi_data *applespi, u8 *buffer,
> + size_t buflen)
> +{
> + u16 crc;
> +
> + crc = crc16(0, buffer, buflen);
> + if (crc != 0) {
if (crc) {
> + dev_warn_ratelimited(&applespi->spi->dev,
> + "Received corrupted packet (crc mismatch)\n");
> + debug_print_header(DBG_RD_CRC);
> + debug_print_buffer(DBG_RD_CRC, "read ", buffer, buflen);
> +
> + return false;
> + }
> +
> + return true;
> +}
> +static void applespi_got_data(struct applespi_data *applespi)
> +{
> + } else if (packet->flags == PACKET_TYPE_READ &&
> + packet->device == PACKET_DEV_TPAD) {
> + struct touchpad_protocol *tp = &message->touchpad;
> +
> + size_t tp_len = sizeof(*tp) +
> + tp->number_of_fingers * sizeof(tp->fingers[0]);
Would be better
struct ...;
size_t ...;
... = ...;
if (...) {
> + 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: ?
> + /* clean up */
Noise.
> + applespi->saved_msg_len = 0;
> +
> + applespi_msg_complete(applespi, packet->flags == PACKET_TYPE_WRITE,
> + true);
> +}
> +static u32 applespi_notify(acpi_handle gpe_device, u32 gpe, void *context)
> +{
> + struct applespi_data *applespi = context;
> + int sts;
> + unsigned long flags;
> +
> + debug_print_header(DBG_RD_IRQ);
> +
> + spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
> +
> + if (!applespi->suspended) {
> + sts = applespi_async(applespi, &applespi->rd_m,
> + applespi_async_read_complete);
> + if (sts != 0)
if (sts)
> + pr_warn("Error queueing async read to device: %d\n",
> + sts);
> + else
> + applespi->read_active = true;
> + }
> +
> + spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
> +
> + return ACPI_INTERRUPT_HANDLED;
> +}
> +
> +static int applespi_get_saved_bl_level(void)
> +{
> + struct efivar_entry *efivar_entry;
> + u16 efi_data = 0;
> + unsigned long efi_data_len;
> + int sts;
> +
> + efivar_entry = kmalloc(sizeof(*efivar_entry), GFP_KERNEL);
> + if (!efivar_entry)
> + return -1;
-ENOMEM
> +
> + memcpy(efivar_entry->var.VariableName, EFI_BL_LEVEL_NAME,
> + sizeof(EFI_BL_LEVEL_NAME));
> + efivar_entry->var.VendorGuid = EFI_BL_LEVEL_GUID;
> + efi_data_len = sizeof(efi_data);
> +
> + sts = efivar_entry_get(efivar_entry, NULL, &efi_data_len, &efi_data);
> + if (sts && sts != -ENOENT)
> + pr_warn("Error getting backlight level from EFI vars: %d\n",
> + sts);
> +
> + kfree(efivar_entry);
> +
> + return efi_data;
> +}
> +static int applespi_probe(struct spi_device *spi)
> +{
> + applespi->msg_buf = devm_kmalloc(&spi->dev, MAX_PKTS_PER_MSG *
> + APPLESPI_PACKET_SIZE,
> + GFP_KERNEL);
devm_kmalloc_array();
> +
> + if (!applespi->tx_buffer || !applespi->tx_status ||
> + !applespi->rx_buffer || !applespi->msg_buf)
> + return -ENOMEM;
> + /*
> + * 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);
> + result = devm_led_classdev_register(&spi->dev,
> + &applespi->backlight_info);
> + if (result) {
> + pr_err("Unable to register keyboard backlight class dev (%d)\n",
> + result);
> + /* not fatal */
Noise. Instead use dev_warn().
> + }
> + /* done */
> + pr_info("spi-device probe done: %s\n", dev_name(&spi->dev));
Noise.
One may use "initcall_debug".
> + return 0;
> +}
> +
> +static int applespi_remove(struct spi_device *spi)
> +{
> + struct applespi_data *applespi = spi_get_drvdata(spi);
> + unsigned long flags;
> + /* shut things down */
Noise.
> + acpi_disable_gpe(NULL, applespi->gpe);
> + acpi_remove_gpe_handler(NULL, applespi->gpe, applespi_notify);
> +
> + /* done */
> + pr_info("spi-device remove done: %s\n", dev_name(&spi->dev));
Noise.
It seems you still have wakeup enabled for the device.
> + return 0;
> +}
> +static int applespi_suspend(struct device *dev)
> +{
> + int rc;
Is it 6th type of naming for error code holding variable?
> + /* wait for all outstanding writes to finish */
> + spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
> +
> + applespi->drain = true;
> + wait_event_lock_irq(applespi->drain_complete, !applespi->write_active,
> + applespi->cmd_msg_lock);
> +
> + spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
Helper? It's used in ->remove() AFAICS.
> + pr_info("spi-device suspend done.\n");
Noise.
One may use ftrace instead.
> + return 0;
> +}
> +
> +static int applespi_resume(struct device *dev)
> +{
> + pr_info("spi-device resume done.\n");
Ditto.
> +
> + return 0;
> +}
> +static const struct acpi_device_id applespi_acpi_match[] = {
> + { "APP000D", 0 },
> + { },
No comma, please.
> +};
> +MODULE_DEVICE_TABLE(acpi, applespi_acpi_match);
> +static struct spi_driver applespi_driver = {
> + .driver = {
> + .name = "applespi",
> + .owner = THIS_MODULE,
This set by macro.
> +
Redundant.
> + .acpi_match_table = ACPI_PTR(applespi_acpi_match),
Do you need ACPI_PTR() ?
> + .pm = &applespi_pm_ops,
> + },
> + .probe = applespi_probe,
> + .remove = applespi_remove,
> + .shutdown = applespi_shutdown,
> +};
> +
> +module_spi_driver(applespi_driver)
> +MODULE_LICENSE("GPL");
License mismatch.
--
With Best Regards,
Andy Shevchenko
^ 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