| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-id: <548F8645.3070002@samsung.com>
Date: Tue, 16 Dec 2014 10:09:25 +0900
From: Jaewon Kim <jaewon02.kim@...sung.com>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc: Kukjin Kim <kgene.kim@...sung.com>, linux-kernel@...r.kernel.org,
linux-samsung-soc@...r.kernel.org, linux-input@...r.kernel.org,
Chanwoo Choi <cw00.choi@...sung.com>,
Hyunhee Kim <hyunhee.kim@...sung.com>
Subject: Re: [PATCH v6 1/2] Input: add regulator haptic driver
Hi Dmitry,
2014년 12월 14일 04:56에 Dmitry Torokhov 이(가) 쓴 글:
> Hi Jaewon,
>
> On Fri, Dec 12, 2014 at 07:32:28PM +0900, Jaewon Kim wrote:
>> This patch adds support for haptic driver controlled by
>> voltage of regulator. And this driver support for
>> Force Feedback interface from input framework
>>
>> Signed-off-by: Jaewon Kim <jaewon02.kim@...sung.com>
>> Signed-off-by: Hyunhee Kim <hyunhee.kim@...sung.com>
>> Acked-by: Kyungmin Park <kyungmin.park@...sung.com>
>> Tested-by: Chanwoo Choi <cw00.choi@...sung.com>
>> Reviewed-by: Chanwoo Choi <cw00.choi@...sung.com>
>> Reviewed-by: Pankaj Dubey <pankaj.dubey@...sung.com>
>> ---
>> .../devicetree/bindings/input/regulator-haptic.txt | 21 ++
>> drivers/input/misc/Kconfig | 11 +
>> drivers/input/misc/Makefile | 1 +
>> drivers/input/misc/regulator-haptic.c | 259 ++++++++++++++++++++
>> include/linux/input/regulator-haptic.h | 31 +++
>> 5 files changed, 323 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/input/regulator-haptic.txt
>> create mode 100644 drivers/input/misc/regulator-haptic.c
>> create mode 100644 include/linux/input/regulator-haptic.h
>>
>> diff --git a/Documentation/devicetree/bindings/input/regulator-haptic.txt b/Documentation/devicetree/bindings/input/regulator-haptic.txt
>> new file mode 100644
>> index 0000000..3ed1c7e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/input/regulator-haptic.txt
>> @@ -0,0 +1,21 @@
>> +* Regulator Haptic Device Tree Bindings
>> +
>> +Required Properties:
>> + - compatible : Should be "regulator-haptic"
>> + - haptic-supply : Power supply to the haptic motor.
>> + [*] refer Documentation/devicetree/bindings/regulator/regulator.txt
>> +
>> + - max-microvolt : The maximum voltage value supplied to the haptic motor.
>> + [The unit of the voltage is a micro]
>> +
>> + - min-microvolt : The minimum voltage value supplied to the haptic motor.
>> + [The unit of the voltage is a micro]
>> +
>> +Example:
>> +
>> + haptics {
>> + compatible = "regulator-haptic";
>> + haptic-supply = <&motor_regulator>;
>> + max-microvolt = <2700000>;
>> + min-microvolt = <1100000>;
>> + };
>> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
>> index 23297ab..e5e556d 100644
>> --- a/drivers/input/misc/Kconfig
>> +++ b/drivers/input/misc/Kconfig
>> @@ -394,6 +394,17 @@ config INPUT_CM109
>> To compile this driver as a module, choose M here: the module will be
>> called cm109.
>>
>> +config INPUT_REGULATOR_HAPTIC
>> + tristate "regulator haptics support"
>> + select INPUT_FF_MEMLESS
>> + help
>> + This option enables device driver support for the haptic controlled
>> + by regulator. This driver supports ff-memless interface
>> + from input framework.
>> +
>> + To compile this driver as a module, choose M here: the
>> + module will be called regulator-haptic.
>> +
>> config INPUT_RETU_PWRBUTTON
>> tristate "Retu Power button Driver"
>> depends on MFD_RETU
>> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
>> index 19c7603..1f135af 100644
>> --- a/drivers/input/misc/Makefile
>> +++ b/drivers/input/misc/Makefile
>> @@ -53,6 +53,7 @@ obj-$(CONFIG_INPUT_PMIC8XXX_PWRKEY) += pmic8xxx-pwrkey.o
>> obj-$(CONFIG_INPUT_POWERMATE) += powermate.o
>> obj-$(CONFIG_INPUT_PWM_BEEPER) += pwm-beeper.o
>> obj-$(CONFIG_INPUT_RB532_BUTTON) += rb532_button.o
>> +obj-$(CONFIG_INPUT_REGULATOR_HAPTIC) += regulator-haptic.o
>> obj-$(CONFIG_INPUT_RETU_PWRBUTTON) += retu-pwrbutton.o
>> obj-$(CONFIG_INPUT_GPIO_ROTARY_ENCODER) += rotary_encoder.o
>> obj-$(CONFIG_INPUT_SGI_BTNS) += sgi_btns.o
>> diff --git a/drivers/input/misc/regulator-haptic.c b/drivers/input/misc/regulator-haptic.c
>> new file mode 100644
>> index 0000000..2fa94bc
>> --- /dev/null
>> +++ b/drivers/input/misc/regulator-haptic.c
>> @@ -0,0 +1,259 @@
>> +/*
>> + * Regulator haptic driver
>> + *
>> + * Copyright (c) 2014 Samsung Electronics Co., Ltd.
>> + * Author: Jaewon Kim <jaewon02.kim@...sung.com>
>> + * Author: Hyunhee Kim <hyunhee.kim@...sung.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#include <linux/input.h>
>> +#include <linux/input/regulator-haptic.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/slab.h>
>> +
>> +#define MAX_MAGNITUDE_SHIFT 16
>> +
>> +struct regulator_haptic {
>> + struct device *dev;
>> + struct input_dev *input_dev;
>> + struct regulator *regulator;
>> +
>> + struct work_struct work;
>> + struct mutex mutex;
>> +
>> + bool enabled;
>> + bool suspend_state;
>> + unsigned int max_volt;
>> + unsigned int min_volt;
>> + unsigned int intensity;
>> + unsigned int magnitude;
>> +};
>> +
>> +static void regulator_haptic_enable(struct regulator_haptic *haptic, bool state)
>> +{
>> + int error;
>> +
>> + if (haptic->enabled == state)
>> + return;
>> +
>> + if (state)
>> + error = regulator_enable(haptic->regulator);
>> + else
>> + error = regulator_disable(haptic->regulator);
>> + if (error) {
> Hmm, maybe:
>
> error = state ? regulator_enable(haptic->regulator) :
> regulator_disable(haptic->regulator);
> if (error)
> ...
>
Okay, i will change it.
>> + dev_err(haptic->dev, "cannot enable regulator\n");
>> + return;
>> + }
>> +
>> + haptic->enabled = state;
>> +}
>> +
>> +static void regulator_haptic_work(struct work_struct *work)
>> +{
>> + struct regulator_haptic *haptic = container_of(work,
>> + struct regulator_haptic, work);
>> + int error;
>> +
>> + if (haptic->suspend_state)
>> + return;
>> +
> Why is this check outside of mutex?
I will include this in next version.
>
>> + mutex_lock(&haptic->mutex);
>> +
>> + error = regulator_set_voltage(haptic->regulator,
>> + haptic->intensity + haptic->min_volt, haptic->max_volt);
>> + if (error) {
>> + dev_err(haptic->dev, "cannot set regulator voltage\n");
>> + goto err;
>> + }
>> +
>> + if (haptic->magnitude)
>> + regulator_haptic_enable(haptic, true);
>> + else
>> + regulator_haptic_enable(haptic, false);
>> +
>> +err:
>> + mutex_unlock(&haptic->mutex);
>> +}
>> +
>> +static int regulator_haptic_play_effect(struct input_dev *input, void *data,
>> + struct ff_effect *effect)
>> +{
>> + struct regulator_haptic *haptic = input_get_drvdata(input);
>> + u64 volt_mag_multi;
>> +
>> + haptic->magnitude = effect->u.rumble.strong_magnitude;
>> + if (!haptic->magnitude)
>> + haptic->magnitude = effect->u.rumble.weak_magnitude;
>> +
>> +
>> + volt_mag_multi = (u64)(haptic->max_volt - haptic->min_volt) *
>> + haptic->magnitude;
>> + haptic->intensity = (unsigned int)(volt_mag_multi >>
>> + MAX_MAGNITUDE_SHIFT);
>> +
>> + schedule_work(&haptic->work);
>> +
>> + return 0;
>> +}
>> +
>> +static void regulator_haptic_close(struct input_dev *input)
>> +{
>> + struct regulator_haptic *haptic = input_get_drvdata(input);
>> +
>> + cancel_work_sync(&haptic->work);
>> + regulator_haptic_enable(haptic, false);
>> +}
>> +
>> +#ifdef CONFIG_OF
>> +static int regulator_haptic_parse_dt(struct regulator_haptic *haptic)
>> +{
>> + struct device_node *node = haptic->dev->of_node;
>> + int error;
>> +
>> + error = of_property_read_u32(node, "max-microvolt", &haptic->max_volt);
>> + if (error) {
>> + dev_err(haptic->dev, "cannot parse max-microvolt\n");
>> + return error;
>> + }
>> +
>> + error = of_property_read_u32(node, "min-microvolt", &haptic->min_volt);
>> + if (error) {
>> + dev_err(haptic->dev, "cannot parse min-microvolt\n");
>> + return error;
>> + }
>> +
>> + return 0;
>> +}
>> +#else
>> +static int regulator_haptic_parse_dt(struct regulator_haptic *haptic)
>> +{
>> + return 0;
> This does not seem right. If you do not have platform data and CONFOG_OF
> is disabled you can't continue initialization.
>
>> +}
>> +#endif /* CONFIG_OF */
>> +
>> +static int regulator_haptic_probe(struct platform_device *pdev)
>> +{
>> + struct regulator_haptic *haptic;
>> + struct regulator_haptic_data *data = dev_get_platdata(&pdev->dev);
>> + struct input_dev *input_dev;
>> + int error;
>> +
>> + haptic = devm_kzalloc(&pdev->dev, sizeof(*haptic), GFP_KERNEL);
>> + if (!haptic)
>> + return -ENOMEM;
>> +
>> + haptic->dev = &pdev->dev;
>> + haptic->enabled = false;
>> + haptic->suspend_state = false;
>> + mutex_init(&haptic->mutex);
>> + INIT_WORK(&haptic->work, regulator_haptic_work);
>> +
>> + if (!data) {
>> + if (pdev->dev.of_node) {
> I'd rather we moved check for presence of of_node into
> regulator_haptic_parse_dt().
Okay. I will move it to haptic_parse_dt() and remove CONFIG_OF.
>
>
>> + error = regulator_haptic_parse_dt(haptic);
>> + if (error) {
>> + dev_err(&pdev->dev, "failed to parse device tree\n");
>> + return error;
>> + }
>> + } else {
>> + dev_err(&pdev->dev, "failed to get platdata\n");
>> + return -EINVAL;
>> + }
>> + } else {
>> + haptic->regulator = data->regulator;
> What is the point of having regulator in platform data and doing
> assignment here if you are going to clobber it a few lines down?
You are right, this is unnecessary process.
I will remove it in next version.
>
>> + haptic->max_volt = data->max_volt;
>> + haptic->min_volt = data->min_volt;
>> + }
>> +
>> + haptic->regulator = devm_regulator_get_exclusive(&pdev->dev, "haptic");
>> + if (IS_ERR(haptic->regulator)) {
>> + dev_err(&pdev->dev, "failed to get regulator\n");
>> + return PTR_ERR(haptic->regulator);
>> + }
>> +
>> + input_dev = devm_input_allocate_device(&pdev->dev);
>> + if (!input_dev)
>> + return -ENOMEM;
> Nit: extra space between return and error value.
>
>> +
>> + haptic->input_dev = input_dev;
>> + haptic->input_dev->name = "regulator-haptic";
>> + haptic->input_dev->dev.parent = &pdev->dev;
>> + haptic->input_dev->close = regulator_haptic_close;
>> + input_set_drvdata(haptic->input_dev, haptic);
>> + input_set_capability(haptic->input_dev, EV_FF, FF_RUMBLE);
>> +
>> + error = input_ff_create_memless(input_dev, NULL,
>> + regulator_haptic_play_effect);
>> + if (error) {
>> + dev_err(&pdev->dev, "failed to create force-feedback\n");
>> + return error;
>> + }
>> +
>> + error = input_register_device(haptic->input_dev);
>> + if (error) {
>> + dev_err(&pdev->dev, "failed to register input device\n");
>> + return error;
>> + }
>> +
>> + platform_set_drvdata(pdev, haptic);
>> +
>> + return 0;
>> +}
>> +
>> +static int __maybe_unused regulator_haptic_suspend(struct device *dev)
>> +{
>> + struct platform_device *pdev = to_platform_device(dev);
>> + struct regulator_haptic *haptic = platform_get_drvdata(pdev);
>> +
>> + mutex_lock(&haptic->mutex);
>> + if (haptic->enabled) {
>> + regulator_haptic_enable(haptic, false);
>> + haptic->suspend_state = true;
> Why do we only set suspend_state if an effect was playing? I think we
> should always indicate that the device is suspended so that we do not
> try to start playing another effect - while it is true that normally
> effects are played by request from userspace which should be frozen by
> now, it is theoretically possible to trigger an effect from kernel as
> well.
This variable name seems to make you confuse.
I used this variable to restore the old state.
When kernel is entering suspend state while the motor is vibrating,
I store vibrating state for vibrate again after escape suspend state.
I will change variable name to "suspend_restore".
And prevent to start playing effect when kernel entering suspend state.
>
>> + }
>> + mutex_unlock(&haptic->mutex);
>> +
>> + return 0;
>> +}
>> +
>> +static int __maybe_unused regulator_haptic_resume(struct device *dev)
>> +{
>> + struct platform_device *pdev = to_platform_device(dev);
>> + struct regulator_haptic *haptic = platform_get_drvdata(pdev);
>> +
>> + if (haptic->suspend_state) {
> I think you should be gating enabling regulator not on suspend_state but
> rather non-zero magnitude. And also lock the mutex to make absolutely
> sure you are not racing with work item.
>
>> + regulator_haptic_enable(haptic, true);
>> + haptic->suspend_state = false;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static SIMPLE_DEV_PM_OPS(regulator_haptic_pm_ops,
>> + regulator_haptic_suspend, regulator_haptic_resume);
>> +
>> +static struct of_device_id regulator_haptic_dt_match[] = {
>> + { .compatible = "regulator-haptic" },
>> + { /* sentinel */ },
>> +};
>> +
>> +static struct platform_driver regulator_haptic_driver = {
>> + .probe = regulator_haptic_probe,
>> + .driver = {
>> + .name = "regulator-haptic",
>> + .of_match_table = regulator_haptic_dt_match,
>> + .pm = ®ulator_haptic_pm_ops,
>> + },
>> +};
>> +module_platform_driver(regulator_haptic_driver);
>> +
>> +MODULE_AUTHOR("Jaewon Kim <jaewon02.kim@...sung.com>");
>> +MODULE_AUTHOR("Hyunhee Kim <hyunhee.kim@...sung.com>");
>> +MODULE_DESCRIPTION("Regulator haptic driver");
>> +MODULE_LICENSE("GPL");
>> diff --git a/include/linux/input/regulator-haptic.h b/include/linux/input/regulator-haptic.h
>> new file mode 100644
>> index 0000000..05ae038
>> --- /dev/null
>> +++ b/include/linux/input/regulator-haptic.h
> Hmm, move it to include/linux/platform-data/ maybe?
Okay, i will move it.
>
>> @@ -0,0 +1,31 @@
>> +/*
>> + * Regulator Haptic Platform Data
>> + *
>> + * Copyright (c) 2014 Samsung Electronics Co., Ltd.
>> + * Author: Jaewon Kim <jaewon02.kim@...sung.com>
>> + * Author: Hyunhee Kim <hyunhee.kim@...sung.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#ifndef _REGULATOR_HAPTIC_H
>> +#define _REGULATOR_HAPTIC_H
>> +
>> +/*
>> + * struct regulator_haptic_data - Platform device data
>> + *
>> + * @regulator: Power supply to the haptic motor
>> + * @max_volt: maximum voltage value supplied to the haptic motor.
>> + * <The unit of the voltage is a micro>
>> + * @min_volt: minimum voltage value supplied to the haptic motor.
>> + * <The unit of the voltage is a micro>
>> + */
>> +struct regulator_haptic_data {
>> + struct regulator *regulator;
>> + unsigned int max_volt;
>> + unsigned int min_volt;
>> +};
>> +
>> +#endif /* _REGULATOR_HAPTIC_H */
>> --
>> 1.7.9.5
>>
> Thanks.
>
Thanks
Jaewon Kim
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists