* Re: [PATCH v4 2/3] Input: add new vibrator driver for various MSM SOCs
From: Dmitry Torokhov @ 2019-02-06 18:16 UTC (permalink / raw)
To: Brian Masney
Cc: andy.gross, david.brown, robh+dt, mark.rutland, linux-input,
linux-kernel, linux-arm-msm, linux-soc, devicetree, jonathan
In-Reply-To: <20190206013329.18195-3-masneyb@onstation.org>
On Tue, Feb 05, 2019 at 08:33:28PM -0500, Brian Masney wrote:
> This patch adds a new vibrator driver that supports various Qualcomm
> MSM SOCs. Driver was tested on a LG Nexus 5 (hammerhead) phone.
>
> Signed-off-by: Brian Masney <masneyb@onstation.org>
Applied together with binding patch, thank you.
> ---
> Changes since v3:
> - Made msm_vibrator_write a function instead of a macro.
> - Check for -EPROBE_DEFER for gpio and clk for consistency with the
> regulator.
> - Check for NULL return value from devm_ioremap() instead of using
> IS_ERR()
> - Don't set dev.parent
> - Use platform_get_drvdata() in suspend and resume
>
> drivers/input/misc/Kconfig | 10 ++
> drivers/input/misc/Makefile | 1 +
> drivers/input/misc/msm-vibrator.c | 282 ++++++++++++++++++++++++++++++
> 3 files changed, 293 insertions(+)
> create mode 100644 drivers/input/misc/msm-vibrator.c
>
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index ca59a2be9bc5..e39aef84f357 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -117,6 +117,16 @@ config INPUT_E3X0_BUTTON
> To compile this driver as a module, choose M here: the
> module will be called e3x0_button.
>
> +config INPUT_MSM_VIBRATOR
> + tristate "Qualcomm MSM vibrator driver"
> + select INPUT_FF_MEMLESS
> + help
> + Support for the vibrator that is found on various Qualcomm MSM
> + SOCs.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called msm_vibrator.
> +
> config INPUT_PCSPKR
> tristate "PC Speaker support"
> depends on PCSPKR_PLATFORM
> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
> index 9d0f9d1ff68f..96a6419cb1f2 100644
> --- a/drivers/input/misc/Makefile
> +++ b/drivers/input/misc/Makefile
> @@ -48,6 +48,7 @@ obj-$(CONFIG_INPUT_MAX8925_ONKEY) += max8925_onkey.o
> obj-$(CONFIG_INPUT_MAX8997_HAPTIC) += max8997_haptic.o
> obj-$(CONFIG_INPUT_MC13783_PWRBUTTON) += mc13783-pwrbutton.o
> obj-$(CONFIG_INPUT_MMA8450) += mma8450.o
> +obj-$(CONFIG_INPUT_MSM_VIBRATOR) += msm-vibrator.o
> obj-$(CONFIG_INPUT_PALMAS_PWRBUTTON) += palmas-pwrbutton.o
> obj-$(CONFIG_INPUT_PCAP) += pcap_keys.o
> obj-$(CONFIG_INPUT_PCF50633_PMU) += pcf50633-input.o
> diff --git a/drivers/input/misc/msm-vibrator.c b/drivers/input/misc/msm-vibrator.c
> new file mode 100644
> index 000000000000..c06941021447
> --- /dev/null
> +++ b/drivers/input/misc/msm-vibrator.c
> @@ -0,0 +1,282 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Qualcomm MSM vibrator driver
> + *
> + * Copyright (c) 2018 Brian Masney <masneyb@onstation.org>
> + *
> + * Based on qcom,pwm-vibrator.c from:
> + * Copyright (c) 2018 Jonathan Marek <jonathan@marek.ca>
> + *
> + * Based on msm_pwm_vibrator.c from downstream Android sources:
> + * Copyright (C) 2009-2014 LGE, Inc.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/gpio.h>
> +#include <linux/input.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +
> +#define REG_CMD_RCGR 0x00
> +#define REG_CFG_RCGR 0x04
> +#define REG_M 0x08
> +#define REG_N 0x0C
> +#define REG_D 0x10
> +#define REG_CBCR 0x24
> +#define MMSS_CC_M_DEFAULT 1
> +
> +struct msm_vibrator {
> + struct input_dev *input;
> + struct mutex mutex;
> + struct work_struct worker;
> + void __iomem *base;
> + struct regulator *vcc;
> + struct clk *clk;
> + struct gpio_desc *enable_gpio;
> + u16 magnitude;
> + bool enabled;
> +};
> +
> +static void msm_vibrator_write(struct msm_vibrator *vibrator, int offset,
> + u32 value)
> +{
> + writel(value, vibrator->base + offset);
> +}
> +
> +static int msm_vibrator_start(struct msm_vibrator *vibrator)
> +{
> + int d_reg_val, ret = 0;
> +
> + mutex_lock(&vibrator->mutex);
> +
> + if (!vibrator->enabled) {
> + ret = clk_set_rate(vibrator->clk, 24000);
> + if (ret) {
> + dev_err(&vibrator->input->dev,
> + "Failed to set clock rate: %d\n", ret);
> + goto unlock;
> + }
> +
> + ret = clk_prepare_enable(vibrator->clk);
> + if (ret) {
> + dev_err(&vibrator->input->dev,
> + "Failed to enable clock: %d\n", ret);
> + goto unlock;
> + }
> +
> + ret = regulator_enable(vibrator->vcc);
> + if (ret) {
> + dev_err(&vibrator->input->dev,
> + "Failed to enable regulator: %d\n", ret);
> + clk_disable(vibrator->clk);
> + goto unlock;
> + }
> +
> + gpiod_set_value_cansleep(vibrator->enable_gpio, 1);
> +
> + vibrator->enabled = true;
> + }
> +
> + d_reg_val = 127 - ((126 * vibrator->magnitude) / 0xffff);
> + msm_vibrator_write(vibrator, REG_CFG_RCGR,
> + (2 << 12) | /* dual edge mode */
> + (0 << 8) | /* cxo */
> + (7 << 0));
> + msm_vibrator_write(vibrator, REG_M, 1);
> + msm_vibrator_write(vibrator, REG_N, 128);
> + msm_vibrator_write(vibrator, REG_D, d_reg_val);
> + msm_vibrator_write(vibrator, REG_CMD_RCGR, 1);
> + msm_vibrator_write(vibrator, REG_CBCR, 1);
> +
> +unlock:
> + mutex_unlock(&vibrator->mutex);
> +
> + return ret;
> +}
> +
> +static void msm_vibrator_stop(struct msm_vibrator *vibrator)
> +{
> + mutex_lock(&vibrator->mutex);
> +
> + if (vibrator->enabled) {
> + gpiod_set_value_cansleep(vibrator->enable_gpio, 0);
> + regulator_disable(vibrator->vcc);
> + clk_disable(vibrator->clk);
> + vibrator->enabled = false;
> + }
> +
> + mutex_unlock(&vibrator->mutex);
> +}
> +
> +static void msm_vibrator_worker(struct work_struct *work)
> +{
> + struct msm_vibrator *vibrator = container_of(work,
> + struct msm_vibrator,
> + worker);
> +
> + if (vibrator->magnitude)
> + msm_vibrator_start(vibrator);
> + else
> + msm_vibrator_stop(vibrator);
> +}
> +
> +static int msm_vibrator_play_effect(struct input_dev *dev, void *data,
> + struct ff_effect *effect)
> +{
> + struct msm_vibrator *vibrator = input_get_drvdata(dev);
> +
> + mutex_lock(&vibrator->mutex);
> +
> + if (effect->u.rumble.strong_magnitude > 0)
> + vibrator->magnitude = effect->u.rumble.strong_magnitude;
> + else
> + vibrator->magnitude = effect->u.rumble.weak_magnitude;
> +
> + mutex_unlock(&vibrator->mutex);
> +
> + schedule_work(&vibrator->worker);
> +
> + return 0;
> +}
> +
> +static void msm_vibrator_close(struct input_dev *input)
> +{
> + struct msm_vibrator *vibrator = input_get_drvdata(input);
> +
> + cancel_work_sync(&vibrator->worker);
> + msm_vibrator_stop(vibrator);
> +}
> +
> +static int msm_vibrator_probe(struct platform_device *pdev)
> +{
> + struct msm_vibrator *vibrator;
> + struct resource *res;
> + int ret;
> +
> + vibrator = devm_kzalloc(&pdev->dev, sizeof(*vibrator), GFP_KERNEL);
> + if (!vibrator)
> + return -ENOMEM;
> +
> + vibrator->input = devm_input_allocate_device(&pdev->dev);
> + if (!vibrator->input)
> + return -ENOMEM;
> +
> + vibrator->vcc = devm_regulator_get(&pdev->dev, "vcc");
> + if (IS_ERR(vibrator->vcc)) {
> + if (PTR_ERR(vibrator->vcc) != -EPROBE_DEFER)
> + dev_err(&pdev->dev, "Failed to get regulator: %ld\n",
> + PTR_ERR(vibrator->vcc));
> + return PTR_ERR(vibrator->vcc);
> + }
> +
> + vibrator->enable_gpio = devm_gpiod_get(&pdev->dev, "enable",
> + GPIOD_OUT_LOW);
> + if (IS_ERR(vibrator->enable_gpio)) {
> + if (PTR_ERR(vibrator->enable_gpio) != -EPROBE_DEFER)
> + dev_err(&pdev->dev, "Failed to get enable gpio: %ld\n",
> + PTR_ERR(vibrator->enable_gpio));
> + return PTR_ERR(vibrator->enable_gpio);
> + }
> +
> + vibrator->clk = devm_clk_get(&pdev->dev, "pwm");
> + if (IS_ERR(vibrator->clk)) {
> + if (PTR_ERR(vibrator->clk) != -EPROBE_DEFER)
> + dev_err(&pdev->dev, "Failed to lookup pwm clock: %ld\n",
> + PTR_ERR(vibrator->clk));
> + return PTR_ERR(vibrator->clk);
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + dev_err(&pdev->dev, "Failed to get platform resource\n");
> + return -ENODEV;
> + }
> +
> + vibrator->base = devm_ioremap(&pdev->dev, res->start,
> + resource_size(res));
> + if (!vibrator->base) {
> + dev_err(&pdev->dev, "Failed to iomap resource: %ld\n",
> + PTR_ERR(vibrator->base));
> + return -ENOMEM;
> + }
> +
> + vibrator->enabled = false;
> + mutex_init(&vibrator->mutex);
> + INIT_WORK(&vibrator->worker, msm_vibrator_worker);
> +
> + vibrator->input->name = "msm-vibrator";
> + vibrator->input->id.bustype = BUS_HOST;
> + vibrator->input->close = msm_vibrator_close;
> +
> + input_set_drvdata(vibrator->input, vibrator);
> + input_set_capability(vibrator->input, EV_FF, FF_RUMBLE);
> +
> + ret = input_ff_create_memless(vibrator->input, NULL,
> + msm_vibrator_play_effect);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to create ff memless: %d", ret);
> + return ret;
> + }
> +
> + ret = input_register_device(vibrator->input);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to register input device: %d", ret);
> + return ret;
> + }
> +
> + platform_set_drvdata(pdev, vibrator);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused msm_vibrator_suspend(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct msm_vibrator *vibrator = platform_get_drvdata(pdev);
> +
> + cancel_work_sync(&vibrator->worker);
> +
> + if (vibrator->enabled)
> + msm_vibrator_stop(vibrator);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused msm_vibrator_resume(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct msm_vibrator *vibrator = platform_get_drvdata(pdev);
> +
> + if (vibrator->enabled)
> + msm_vibrator_start(vibrator);
> +
> + return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(msm_vibrator_pm_ops, msm_vibrator_suspend,
> + msm_vibrator_resume);
> +
> +static const struct of_device_id msm_vibrator_of_match[] = {
> + { .compatible = "qcom,msm8226-vibrator" },
> + { .compatible = "qcom,msm8974-vibrator" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, msm_vibrator_of_match);
> +
> +static struct platform_driver msm_vibrator_driver = {
> + .probe = msm_vibrator_probe,
> + .driver = {
> + .name = "msm-vibrator",
> + .pm = &msm_vibrator_pm_ops,
> + .of_match_table = of_match_ptr(msm_vibrator_of_match),
> + },
> +};
> +module_platform_driver(msm_vibrator_driver);
> +
> +MODULE_AUTHOR("Brian Masney <masneyb@onstation.org>");
> +MODULE_DESCRIPTION("Qualcomm MSM vibrator driver");
> +MODULE_LICENSE("GPL");
> --
> 2.17.2
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH v4 3/3] ARM: dts: qcom: msm8974-hammerhead: add device tree bindings for vibrator
From: Dmitry Torokhov @ 2019-02-06 18:18 UTC (permalink / raw)
To: Brian Masney
Cc: andy.gross, david.brown, robh+dt, mark.rutland, linux-input,
linux-kernel, linux-arm-msm, linux-soc, devicetree, jonathan
In-Reply-To: <20190206013329.18195-4-masneyb@onstation.org>
On Tue, Feb 05, 2019 at 08:33:29PM -0500, Brian Masney wrote:
> This patch adds device device tree bindings for the vibrator found on
> the LG Nexus 5 (hammerhead) phone.
>
> Signed-off-by: Brian Masney <masneyb@onstation.org>
This should go through Qualcomm tree with the other bindings?
> ---
> Changes since v3:
> - None
>
> .../qcom-msm8974-lge-nexus5-hammerhead.dts | 31 +++++++++++++++++++
> 1 file changed, 31 insertions(+)
>
> diff --git a/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts b/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts
> index b3b04736a159..1fd9f429f34a 100644
> --- a/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts
> +++ b/arch/arm/boot/dts/qcom-msm8974-lge-nexus5-hammerhead.dts
> @@ -5,6 +5,7 @@
> #include <dt-bindings/gpio/gpio.h>
> #include <dt-bindings/input/input.h>
> #include <dt-bindings/pinctrl/qcom,pmic-gpio.h>
> +#include <dt-bindings/clock/qcom,mmcc-msm8974.h>
>
> / {
> model = "LGE MSM 8974 HAMMERHEAD";
> @@ -306,6 +307,36 @@
> input-enable;
> };
> };
> +
> + vibrator_pin: vibrator {
> + pwm {
> + pins = "gpio27";
> + function = "gp1_clk";
> +
> + drive-strength = <6>;
> + bias-disable;
> + };
> +
> + enable {
> + pins = "gpio60";
> + function = "gpio";
> + };
> + };
> + };
> +
> + vibrator@fd8c3450 {
> + compatible = "qcom,msm8974-vibrator";
> + reg = <0xfd8c3450 0x400>;
> +
> + vcc-supply = <&pm8941_l19>;
> +
> + clocks = <&mmcc CAMSS_GP1_CLK>;
> + clock-names = "pwm";
> +
> + enable-gpios = <&msmgpio 60 GPIO_ACTIVE_HIGH>;
> +
> + pinctrl-names = "default";
> + pinctrl-0 = <&vibrator_pin>;
> };
>
> sdhci@f9824900 {
> --
> 2.17.2
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH v4 3/3] ARM: dts: qcom: msm8974-hammerhead: add device tree bindings for vibrator
From: Brian Masney @ 2019-02-06 18:51 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: andy.gross, david.brown, robh+dt, mark.rutland, linux-input,
linux-kernel, linux-arm-msm, linux-soc, devicetree, jonathan
In-Reply-To: <20190206181822.GB174258@dtor-ws>
On Wed, Feb 06, 2019 at 10:18:22AM -0800, Dmitry Torokhov wrote:
> On Tue, Feb 05, 2019 at 08:33:29PM -0500, Brian Masney wrote:
> > This patch adds device device tree bindings for the vibrator found on
> > the LG Nexus 5 (hammerhead) phone.
> >
> > Signed-off-by: Brian Masney <masneyb@onstation.org>
>
> This should go through Qualcomm tree with the other bindings?
Yes, this will go through Andy's tree.
Brian
^ permalink raw reply
* Re: [PATCH v2 2/5] input: misc: bma150: Use managed resources helpers
From: Dmitry Torokhov @ 2019-02-06 18:52 UTC (permalink / raw)
To: Paweł Chmiel
Cc: robh+dt, mark.rutland, xc-racer2, devicetree, linux-input,
linux-kernel
In-Reply-To: <20190202151806.9064-3-pawel.mikolaj.chmiel@gmail.com>
On Sat, Feb 02, 2019 at 04:18:03PM +0100, Paweł Chmiel wrote:
> From: Jonathan Bakker <xc-racer2@live.ca>
>
> The driver can be cleaned up by using managed resource helpers
>
> Changes from v1:
> - Correct devm input unregistering
>
> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
> Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
Applied, thank you.
> ---
> drivers/input/misc/bma150.c | 44 ++++++++++---------------------------
> 1 file changed, 12 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/input/misc/bma150.c b/drivers/input/misc/bma150.c
> index 1efcfdf9f8a8..79acaaf86b7e 100644
> --- a/drivers/input/misc/bma150.c
> +++ b/drivers/input/misc/bma150.c
> @@ -471,7 +471,7 @@ static int bma150_register_input_device(struct bma150_data *bma150)
> struct input_dev *idev;
> int error;
>
> - idev = input_allocate_device();
> + idev = devm_input_allocate_device(&bma150->client->dev);
> if (!idev)
> return -ENOMEM;
>
> @@ -482,10 +482,8 @@ static int bma150_register_input_device(struct bma150_data *bma150)
> input_set_drvdata(idev, bma150);
>
> error = input_register_device(idev);
> - if (error) {
> - input_free_device(idev);
> + if (error)
> return error;
> - }
>
> bma150->input = idev;
> return 0;
> @@ -496,7 +494,7 @@ static int bma150_register_polled_device(struct bma150_data *bma150)
> struct input_polled_dev *ipoll_dev;
> int error;
>
> - ipoll_dev = input_allocate_polled_device();
> + ipoll_dev = devm_input_allocate_polled_device(&bma150->client->dev);
> if (!ipoll_dev)
> return -ENOMEM;
>
> @@ -511,10 +509,8 @@ static int bma150_register_polled_device(struct bma150_data *bma150)
> bma150_init_input_device(bma150, ipoll_dev->input);
>
> error = input_register_polled_device(ipoll_dev);
> - if (error) {
> - input_free_polled_device(ipoll_dev);
> + if (error)
> return error;
> - }
>
> bma150->input_polled = ipoll_dev;
> bma150->input = ipoll_dev->input;
> @@ -543,7 +539,8 @@ static int bma150_probe(struct i2c_client *client,
> return -EINVAL;
> }
>
> - bma150 = kzalloc(sizeof(struct bma150_data), GFP_KERNEL);
> + bma150 = devm_kzalloc(&client->dev, sizeof(struct bma150_data),
> + GFP_KERNEL);
> if (!bma150)
> return -ENOMEM;
>
> @@ -556,7 +553,7 @@ static int bma150_probe(struct i2c_client *client,
> dev_err(&client->dev,
> "IRQ GPIO conf. error %d, error %d\n",
> client->irq, error);
> - goto err_free_mem;
> + return error;
> }
> }
> cfg = &pdata->cfg;
> @@ -566,14 +563,14 @@ static int bma150_probe(struct i2c_client *client,
>
> error = bma150_initialize(bma150, cfg);
> if (error)
> - goto err_free_mem;
> + return error;
>
> if (client->irq > 0) {
> error = bma150_register_input_device(bma150);
> if (error)
> - goto err_free_mem;
> + return error;
>
> - error = request_threaded_irq(client->irq,
> + error = devm_request_threaded_irq(&client->dev, client->irq,
> NULL, bma150_irq_thread,
> IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> BMA150_DRIVER, bma150);
> @@ -581,13 +578,12 @@ static int bma150_probe(struct i2c_client *client,
> dev_err(&client->dev,
> "irq request failed %d, error %d\n",
> client->irq, error);
> - input_unregister_device(bma150->input);
> - goto err_free_mem;
> + return error;
> }
> } else {
> error = bma150_register_polled_device(bma150);
> if (error)
> - goto err_free_mem;
> + return error;
> }
>
> i2c_set_clientdata(client, bma150);
> @@ -595,28 +591,12 @@ static int bma150_probe(struct i2c_client *client,
> pm_runtime_enable(&client->dev);
>
> return 0;
> -
> -err_free_mem:
> - kfree(bma150);
> - return error;
> }
>
> static int bma150_remove(struct i2c_client *client)
> {
> - struct bma150_data *bma150 = i2c_get_clientdata(client);
> -
> pm_runtime_disable(&client->dev);
>
> - if (client->irq > 0) {
> - free_irq(client->irq, bma150);
> - input_unregister_device(bma150->input);
> - } else {
> - input_unregister_polled_device(bma150->input_polled);
> - input_free_polled_device(bma150->input_polled);
> - }
> -
> - kfree(bma150);
> -
> return 0;
> }
>
> --
> 2.17.1
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2 5/5] input: misc: bma150: Register input device after setting private data
From: Dmitry Torokhov @ 2019-02-06 18:53 UTC (permalink / raw)
To: Paweł Chmiel
Cc: robh+dt, mark.rutland, xc-racer2, devicetree, linux-input,
linux-kernel
In-Reply-To: <20190202151806.9064-6-pawel.mikolaj.chmiel@gmail.com>
On Sat, Feb 02, 2019 at 04:18:06PM +0100, Paweł Chmiel wrote:
> From: Jonathan Bakker <xc-racer2@live.ca>
>
> Otherwise we introduce a race condition where userspace can request input
> before we're ready leading to null pointer dereference such as
>
> input: bma150 as /devices/platform/i2c-gpio-2/i2c-5/5-0038/input/input3
> Unable to handle kernel NULL pointer dereference at virtual address 00000018
> pgd = (ptrval)
> [00000018] *pgd=55dac831, *pte=00000000, *ppte=00000000
> Internal error: Oops: 17 [#1] PREEMPT ARM
> Modules linked in: bma150 input_polldev [last unloaded: bma150]
> CPU: 0 PID: 2870 Comm: accelerometer Not tainted 5.0.0-rc3-dirty #46
> Hardware name: Samsung S5PC110/S5PV210-based board
> PC is at input_event+0x8/0x60
> LR is at bma150_report_xyz+0x9c/0xe0 [bma150]
> pc : [<80450f70>] lr : [<7f0a614c>] psr: 800d0013
> sp : a4c1fd78 ip : 00000081 fp : 00020000
> r10: 00000000 r9 : a5e2944c r8 : a7455000
> r7 : 00000016 r6 : 00000101 r5 : a7617940 r4 : 80909048
> r3 : fffffff2 r2 : 00000000 r1 : 00000003 r0 : 00000000
> Flags: Nzcv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
> Control: 10c5387d Table: 54e34019 DAC: 00000051
> Process accelerometer (pid: 2870, stack limit = 0x(ptrval))
> Stackck: (0xa4c1fd78 to 0xa4c20000)
> fd60: fffffff3 fc813f6c
> fd80: 40410581 d7530ce3 a5e2817c a7617f00 a5e29404 a5e2817c 00000000 7f008324
> fda0: a5e28000 8044f59c a5fdd9d0 a5e2945c a46a4a00 a5e29668 a7455000 80454f10
> fdc0: 80909048 a5e29668 a5fdd9d0 a46a4a00 806316d0 00000000 a46a4a00 801df5f0
> fde0: 00000000 d7530ce3 a4c1fec0 a46a4a00 00000000 a5fdd9d0 a46a4a08 801df53c
> fe00: 00000000 801d74bc a4c1fec0 00000000 a4c1ff70 00000000 a7038da8 00000000
> fe20: a46a4a00 801e91fc a411bbe0 801f2e88 00000004 00000000 80909048 00000041
> fe40: 00000000 00020000 00000000 dead4ead a6a88da0 00000000 ffffe000 806fcae8
> fe60: a4c1fec8 00000000 80909048 00000002 a5fdd9d0 a7660110 a411bab0 00000001
> fe80: dead4ead ffffffff ffffffff a4c1fe8c a4c1fe8c d7530ce3 20000013 80909048
> fea0: 80909048 a4c1ff70 00000001 fffff000 a4c1e000 00000005 00026038 801eabd8
> fec0: a7660110 a411bab0 b9394901 00000006 a696201b 76fb3000 00000000 a7039720
> fee0: a5fdd9d0 00000101 00000002 00000096 00000000 00000000 00000000 a4c1ff00
> ff00: a6b310f4 805cb174 a6b310f4 00000010 00000fe0 00000010 a4c1e000 d7530ce3
> ff20: 00000003 a5f41400 a5f41424 00000000 a6962000 00000000 00000003 00000002
> ff40: ffffff9c 000a0000 80909048 d7530ce3 a6962000 00000003 80909048 ffffff9c
> ff60: a6962000 801d890c 00000000 00000000 00020000 a7590000 00000004 00000100
> ff80: 00000001 d7530ce3 000288b8 00026320 000288b8 00000005 80101204 a4c1e000
> ffa0: 00000005 80101000 000288b8 00026320 000288b8 000a0000 00000000 00000000
> ffc0: 000288b8 00026320 000288b8 00000005 7eef3bac 000264e8 00028ad8 00026038
> ffe0: 00000005 7eef3300 76f76e91 76f78546 800d0030 000288b8 00000000 00000000
> [<80450f70>] (input_event) from [<a5e2817c>] (0xa5e2817c)
> Code: e1a08148 eaffffa8 e351001f 812fff1e (e590c018)
> ---[ end trace 1c691ee85f2ff243 ]---
>
> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
> Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
Applied, thank you.
> ---
> drivers/input/misc/bma150.c | 15 +++------------
> 1 file changed, 3 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/input/misc/bma150.c b/drivers/input/misc/bma150.c
> index 1cdc8ce97968..64caf43e5bca 100644
> --- a/drivers/input/misc/bma150.c
> +++ b/drivers/input/misc/bma150.c
> @@ -470,7 +470,6 @@ static void bma150_init_input_device(struct bma150_data *bma150,
> static int bma150_register_input_device(struct bma150_data *bma150)
> {
> struct input_dev *idev;
> - int error;
>
> idev = devm_input_allocate_device(&bma150->client->dev);
> if (!idev)
> @@ -482,18 +481,14 @@ static int bma150_register_input_device(struct bma150_data *bma150)
> idev->close = bma150_irq_close;
> input_set_drvdata(idev, bma150);
>
> - error = input_register_device(idev);
> - if (error)
> - return error;
> -
> bma150->input = idev;
> - return 0;
> +
> + return input_register_device(idev);
> }
>
> static int bma150_register_polled_device(struct bma150_data *bma150)
> {
> struct input_polled_dev *ipoll_dev;
> - int error;
>
> ipoll_dev = devm_input_allocate_polled_device(&bma150->client->dev);
> if (!ipoll_dev)
> @@ -509,14 +504,10 @@ static int bma150_register_polled_device(struct bma150_data *bma150)
>
> bma150_init_input_device(bma150, ipoll_dev->input);
>
> - error = input_register_polled_device(ipoll_dev);
> - if (error)
> - return error;
> -
> bma150->input_polled = ipoll_dev;
> bma150->input = ipoll_dev->input;
>
> - return 0;
> + return input_register_polled_device(ipoll_dev);
> }
>
> int bma150_cfg_from_of(struct device_node *np)
> --
> 2.17.1
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: cap11xx - switch to using set_brightness_blocking()
From: Jacek Anaszewski @ 2019-02-06 19:09 UTC (permalink / raw)
To: Dmitry Torokhov, linux-input; +Cc: Sven Van Asbroeck, linux-kernel
In-Reply-To: <20190205222050.GA145676@dtor-ws>
Hi Dmitry,
On 2/5/19 11:20 PM, 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>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> drivers/input/keyboard/cap11xx.c | 47 ++++++++++++++------------------
> 1 file changed, 21 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/input/keyboard/cap11xx.c b/drivers/input/keyboard/cap11xx.c
> index 312916f99597..c0baf323ddda 100644
> --- a/drivers/input/keyboard/cap11xx.c
> +++ b/drivers/input/keyboard/cap11xx.c
> @@ -75,9 +75,8 @@
> struct cap11xx_led {
> struct cap11xx_priv *priv;
> struct led_classdev cdev;
> - struct work_struct work;
> u32 reg;
> - enum led_brightness new_brightness;
> + enum led_brightness brightness;
> };
> #endif
>
> @@ -233,30 +232,28 @@ static void cap11xx_input_close(struct input_dev *idev)
> }
>
> #ifdef CONFIG_LEDS_CLASS
> -static void cap11xx_led_work(struct work_struct *work)
> -{
> - struct cap11xx_led *led = container_of(work, struct cap11xx_led, work);
> - 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).
> - */
> - 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)
> +static int cap11xx_led_set(struct led_classdev *cdev,
> + enum led_brightness value)
> {
> struct cap11xx_led *led = container_of(cdev, struct cap11xx_led, cdev);
> + struct cap11xx_priv *priv = led->priv;
> + int error = 0;
> +
> + if (led->brightness != value) {
I'd say this check is not needed. If registers are not marked volatile
then regmap should not do the actual write to the hardware if the value
is equal to the cached one.
> + /*
> + * All LEDs share the same duty cycle as this is a HW
> + * limitation. Brightness levels per LED are either
> + * 0 (OFF) and 1 (ON).
> + */
> + error = regmap_update_bits(priv->regmap,
> + CAP11XX_REG_LED_OUTPUT_CONTROL,
> + BIT(led->reg),
> + value ? BIT(led->reg) : 0);
> + if (!error)
> + led->brightness = value;
> + }
>
> - if (led->new_brightness == value)
> - return;
> -
> - led->new_brightness = value;
> - schedule_work(&led->work);
> + return error;
> }
>
> static int cap11xx_init_leds(struct device *dev,
> @@ -299,7 +296,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 +309,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);
>
--
Best regards,
Jacek Anaszewski
^ permalink raw reply
* Re: [PATCH v2 5/5] input: misc: bma150: Register input device after setting private data
From: Dmitry Torokhov @ 2019-02-06 19:23 UTC (permalink / raw)
To: Paweł Chmiel
Cc: robh+dt, mark.rutland, xc-racer2, devicetree, linux-input,
linux-kernel
In-Reply-To: <20190206185307.GD174258@dtor-ws>
On Wed, Feb 06, 2019 at 10:53:07AM -0800, Dmitry Torokhov wrote:
> On Sat, Feb 02, 2019 at 04:18:06PM +0100, Paweł Chmiel wrote:
> > From: Jonathan Bakker <xc-racer2@live.ca>
> >
> > Otherwise we introduce a race condition where userspace can request input
> > before we're ready leading to null pointer dereference such as
> >
> > input: bma150 as /devices/platform/i2c-gpio-2/i2c-5/5-0038/input/input3
> > Unable to handle kernel NULL pointer dereference at virtual address 00000018
> > pgd = (ptrval)
> > [00000018] *pgd=55dac831, *pte=00000000, *ppte=00000000
> > Internal error: Oops: 17 [#1] PREEMPT ARM
> > Modules linked in: bma150 input_polldev [last unloaded: bma150]
> > CPU: 0 PID: 2870 Comm: accelerometer Not tainted 5.0.0-rc3-dirty #46
> > Hardware name: Samsung S5PC110/S5PV210-based board
> > PC is at input_event+0x8/0x60
> > LR is at bma150_report_xyz+0x9c/0xe0 [bma150]
> > pc : [<80450f70>] lr : [<7f0a614c>] psr: 800d0013
> > sp : a4c1fd78 ip : 00000081 fp : 00020000
> > r10: 00000000 r9 : a5e2944c r8 : a7455000
> > r7 : 00000016 r6 : 00000101 r5 : a7617940 r4 : 80909048
> > r3 : fffffff2 r2 : 00000000 r1 : 00000003 r0 : 00000000
> > Flags: Nzcv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
> > Control: 10c5387d Table: 54e34019 DAC: 00000051
> > Process accelerometer (pid: 2870, stack limit = 0x(ptrval))
> > Stackck: (0xa4c1fd78 to 0xa4c20000)
> > fd60: fffffff3 fc813f6c
> > fd80: 40410581 d7530ce3 a5e2817c a7617f00 a5e29404 a5e2817c 00000000 7f008324
> > fda0: a5e28000 8044f59c a5fdd9d0 a5e2945c a46a4a00 a5e29668 a7455000 80454f10
> > fdc0: 80909048 a5e29668 a5fdd9d0 a46a4a00 806316d0 00000000 a46a4a00 801df5f0
> > fde0: 00000000 d7530ce3 a4c1fec0 a46a4a00 00000000 a5fdd9d0 a46a4a08 801df53c
> > fe00: 00000000 801d74bc a4c1fec0 00000000 a4c1ff70 00000000 a7038da8 00000000
> > fe20: a46a4a00 801e91fc a411bbe0 801f2e88 00000004 00000000 80909048 00000041
> > fe40: 00000000 00020000 00000000 dead4ead a6a88da0 00000000 ffffe000 806fcae8
> > fe60: a4c1fec8 00000000 80909048 00000002 a5fdd9d0 a7660110 a411bab0 00000001
> > fe80: dead4ead ffffffff ffffffff a4c1fe8c a4c1fe8c d7530ce3 20000013 80909048
> > fea0: 80909048 a4c1ff70 00000001 fffff000 a4c1e000 00000005 00026038 801eabd8
> > fec0: a7660110 a411bab0 b9394901 00000006 a696201b 76fb3000 00000000 a7039720
> > fee0: a5fdd9d0 00000101 00000002 00000096 00000000 00000000 00000000 a4c1ff00
> > ff00: a6b310f4 805cb174 a6b310f4 00000010 00000fe0 00000010 a4c1e000 d7530ce3
> > ff20: 00000003 a5f41400 a5f41424 00000000 a6962000 00000000 00000003 00000002
> > ff40: ffffff9c 000a0000 80909048 d7530ce3 a6962000 00000003 80909048 ffffff9c
> > ff60: a6962000 801d890c 00000000 00000000 00020000 a7590000 00000004 00000100
> > ff80: 00000001 d7530ce3 000288b8 00026320 000288b8 00000005 80101204 a4c1e000
> > ffa0: 00000005 80101000 000288b8 00026320 000288b8 000a0000 00000000 00000000
> > ffc0: 000288b8 00026320 000288b8 00000005 7eef3bac 000264e8 00028ad8 00026038
> > ffe0: 00000005 7eef3300 76f76e91 76f78546 800d0030 000288b8 00000000 00000000
> > [<80450f70>] (input_event) from [<a5e2817c>] (0xa5e2817c)
> > Code: e1a08148 eaffffa8 e351001f 812fff1e (e590c018)
> > ---[ end trace 1c691ee85f2ff243 ]---
> >
> > Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
> > Signed-off-by: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
>
> Applied, thank you.
Actually I'll move it to the current release and mark for sable.
>
> > ---
> > drivers/input/misc/bma150.c | 15 +++------------
> > 1 file changed, 3 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/input/misc/bma150.c b/drivers/input/misc/bma150.c
> > index 1cdc8ce97968..64caf43e5bca 100644
> > --- a/drivers/input/misc/bma150.c
> > +++ b/drivers/input/misc/bma150.c
> > @@ -470,7 +470,6 @@ static void bma150_init_input_device(struct bma150_data *bma150,
> > static int bma150_register_input_device(struct bma150_data *bma150)
> > {
> > struct input_dev *idev;
> > - int error;
> >
> > idev = devm_input_allocate_device(&bma150->client->dev);
> > if (!idev)
> > @@ -482,18 +481,14 @@ static int bma150_register_input_device(struct bma150_data *bma150)
> > idev->close = bma150_irq_close;
> > input_set_drvdata(idev, bma150);
> >
> > - error = input_register_device(idev);
> > - if (error)
> > - return error;
> > -
> > bma150->input = idev;
> > - return 0;
> > +
> > + return input_register_device(idev);
> > }
> >
> > static int bma150_register_polled_device(struct bma150_data *bma150)
> > {
> > struct input_polled_dev *ipoll_dev;
> > - int error;
> >
> > ipoll_dev = devm_input_allocate_polled_device(&bma150->client->dev);
> > if (!ipoll_dev)
> > @@ -509,14 +504,10 @@ static int bma150_register_polled_device(struct bma150_data *bma150)
> >
> > bma150_init_input_device(bma150, ipoll_dev->input);
> >
> > - error = input_register_polled_device(ipoll_dev);
> > - if (error)
> > - return error;
> > -
> > bma150->input_polled = ipoll_dev;
> > bma150->input = ipoll_dev->input;
> >
> > - return 0;
> > + return input_register_polled_device(ipoll_dev);
> > }
> >
> > int bma150_cfg_from_of(struct device_node *np)
> > --
> > 2.17.1
> >
>
> --
> Dmitry
--
Dmitry
^ permalink raw reply
* 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
* [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
* [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
* 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
* 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
* [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 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: 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] 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: 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: 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
* 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: 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
* [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 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 (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 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
* 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