From: Baolin Wang <baolin.wang@linaro.org>
To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Rob Herring <robh+dt@kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
Orson Zhai <orsonzhai@gmail.com>,
Chunyan Zhang <zhang.lyra@gmail.com>,
"open list:HID CORE LAYER" <linux-input@vger.kernel.org>,
DTML <devicetree@vger.kernel.org>,
LKML <linux-kernel@vger.kernel.org>,
xiaotong.lu@spreadtrum.com
Subject: Re: [PATCH v2 2/2] input: misc: Add Spreadtrum vibrator driver
Date: Tue, 24 Apr 2018 10:44:23 +0800 [thread overview]
Message-ID: <CAMz4kuL8qsbQPfSE9vKkCuq41zK_RKq8KmnjaZ00Tga3zXHHKw@mail.gmail.com> (raw)
In-Reply-To: <20180423172937.GA66646@dtor-ws>
Hi Dmitry,
On 24 April 2018 at 01:29, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> Hi Xiaotong,
>
> On Mon, Apr 23, 2018 at 10:33:36AM +0800, Baolin Wang wrote:
>> From: Xiaotong Lu <xiaotong.lu@spreadtrum.com>
>>
>> This patch adds the Spreadtrum vibrator driver, which embedded in the
>> Spreadtrum SC27xx series PMICs.
>>
>> Signed-off-by: Xiaotong Lu <xiaotong.lu@spreadtrum.com>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> ---
>> Changes since v1:
>> - Remove input_ff_destroy() and input_unregister_device()
>> ---
>> drivers/input/misc/Kconfig | 10 +++
>> drivers/input/misc/Makefile | 1 +
>> drivers/input/misc/sc27xx-vibra.c | 156 +++++++++++++++++++++++++++++++++++++
>> 3 files changed, 167 insertions(+)
>> create mode 100644 drivers/input/misc/sc27xx-vibra.c
>>
>> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
>> index 572b15f..c761c0c 100644
>> --- a/drivers/input/misc/Kconfig
>> +++ b/drivers/input/misc/Kconfig
>> @@ -841,4 +841,14 @@ config INPUT_RAVE_SP_PWRBUTTON
>> To compile this driver as a module, choose M here: the
>> module will be called rave-sp-pwrbutton.
>>
>> +config INPUT_SC27XX_VIBRA
>> + tristate "Spreadtrum sc27xx vibrator support"
>> + depends on MFD_SC27XX_PMIC || COMPILE_TEST
>> + select INPUT_FF_MEMLESS
>> + help
>> + This option enables support for Spreadtrum sc27xx vibrator driver.
>> +
>> + To compile this driver as a module, choose M here. The module will
>> + be called sc27xx_vibra.
>> +
>> endif
>> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
>> index 72cde28..9d0f9d1 100644
>> --- a/drivers/input/misc/Makefile
>> +++ b/drivers/input/misc/Makefile
>> @@ -66,6 +66,7 @@ obj-$(CONFIG_INPUT_RETU_PWRBUTTON) += retu-pwrbutton.o
>> obj-$(CONFIG_INPUT_AXP20X_PEK) += axp20x-pek.o
>> obj-$(CONFIG_INPUT_GPIO_ROTARY_ENCODER) += rotary_encoder.o
>> obj-$(CONFIG_INPUT_RK805_PWRKEY) += rk805-pwrkey.o
>> +obj-$(CONFIG_INPUT_SC27XX_VIBRA) += sc27xx-vibra.o
>> obj-$(CONFIG_INPUT_SGI_BTNS) += sgi_btns.o
>> obj-$(CONFIG_INPUT_SIRFSOC_ONKEY) += sirfsoc-onkey.o
>> obj-$(CONFIG_INPUT_SOC_BUTTON_ARRAY) += soc_button_array.o
>> diff --git a/drivers/input/misc/sc27xx-vibra.c b/drivers/input/misc/sc27xx-vibra.c
>> new file mode 100644
>> index 0000000..f78e70f
>> --- /dev/null
>> +++ b/drivers/input/misc/sc27xx-vibra.c
>> @@ -0,0 +1,156 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2018 Spreadtrum Communications Inc.
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/of_address.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/input.h>
>> +#include <linux/workqueue.h>
>> +
>> +#define CUR_DRV_CAL_SEL GENMASK(13, 12)
>> +#define SLP_LDOVIBR_PD_EN BIT(9)
>> +#define LDO_VIBR_PD BIT(8)
>> +
>> +struct vibra_info {
>> + struct input_dev *input_dev;
>> + struct work_struct play_work;
>> + struct regmap *regmap;
>> + u32 base;
>> + u32 strength;
>> + bool enabled;
>> +};
>> +
>> +static void sc27xx_vibra_set(struct vibra_info *info, bool on)
>> +{
>> + if (on) {
>> + regmap_update_bits(info->regmap, info->base, LDO_VIBR_PD, 0);
>> + regmap_update_bits(info->regmap, info->base,
>> + SLP_LDOVIBR_PD_EN, 0);
>> + info->enabled = true;
>> + } else {
>> + regmap_update_bits(info->regmap, info->base, LDO_VIBR_PD,
>> + LDO_VIBR_PD);
>> + regmap_update_bits(info->regmap, info->base,
>> + SLP_LDOVIBR_PD_EN, SLP_LDOVIBR_PD_EN);
>> + info->enabled = false;
>> + }
>> +}
>> +
>> +static int sc27xx_vibra_hw_init(struct vibra_info *info)
>> +{
>> + return regmap_update_bits(info->regmap, info->base, CUR_DRV_CAL_SEL, 0);
>> +}
>> +
>> +static void sc27xx_vibra_play_work(struct work_struct *work)
>> +{
>> + struct vibra_info *info = container_of(work, struct vibra_info,
>> + play_work);
>> +
>> + if (info->strength && !info->enabled)
>> + sc27xx_vibra_set(info, true);
>> + else if (info->enabled)
>> + sc27xx_vibra_set(info, false);
>
> I do not think this is correct. If you issue 2 play requests with
> info->strength that is not 0 then you'll end up disabling the vibrator.
>
> I think you want the 2nd condition to be:
>
> else if (info->strength == 0 && info->enabled)
> sc27xx_vibra_set(info, false);
>
Yes, you are right. Will fix in next patch.
>
>> +}
>> +
>> +static int sc27xx_vibra_play(struct input_dev *input, void *data,
>> + struct ff_effect *effect)
>> +{
>> + struct vibra_info *info = input_get_drvdata(input);
>> +
>> + info->strength = effect->u.rumble.weak_magnitude;
>> + schedule_work(&info->play_work);
>> +
>> + return 0;
>> +}
>> +
>> +static void sc27xx_vibra_close(struct input_dev *input)
>> +{
>> + struct vibra_info *info = input_get_drvdata(input);
>> +
>> + cancel_work_sync(&info->play_work);
>> + if (info->enabled)
>> + sc27xx_vibra_set(info, false);
>> +}
>> +
>> +static int sc27xx_vibra_probe(struct platform_device *pdev)
>> +{
>> + struct device_node *node = pdev->dev.of_node;
>> + struct vibra_info *info;
>> + int ret;
>
> Can we please call this variable "error"?
Sure.
>
>> +
>> + info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
>> + if (!info)
>> + return -ENOMEM;
>> +
>> + info->regmap = dev_get_regmap(pdev->dev.parent, NULL);
>> + if (!info->regmap) {
>> + dev_err(&pdev->dev, "failed to get vibrator regmap.\n");
>> + return -ENODEV;
>> + }
>> +
>> + ret = of_property_read_u32(node, "reg", &info->base);
>
> I am fan of generic device properties, please change to
> device_property_read_u32().
OK.
>
>> + if (ret) {
>> + dev_err(&pdev->dev, "failed to get vibrator base address.\n");
>> + return ret;
>> + }
>> +
>> + info->input_dev = devm_input_allocate_device(&pdev->dev);
>> + if (!info->input_dev) {
>> + dev_err(&pdev->dev, "failed to allocate input device.\n");
>> + return -ENOMEM;
>> + }
>> +
>> + info->input_dev->name = "sc27xx:vibrator";
>> + info->input_dev->id.version = 0;
>> + info->input_dev->dev.parent = pdev->dev.parent;
>
> Why is device reparented to the parent of platform device? This breaks
> devm implementation for the input device. If you really need this,
> you'll have to switch to unmanaged API.
Sorry, this is one mistake and will fix it in next version.
>
>> + info->input_dev->close = sc27xx_vibra_close;
>> +
>> + input_set_drvdata(info->input_dev, info);
>> + input_set_capability(info->input_dev, EV_FF, FF_RUMBLE);
>> + INIT_WORK(&info->play_work, sc27xx_vibra_play_work);
>> + info->enabled = false;
>> +
>> + ret = input_ff_create_memless(info->input_dev, NULL, sc27xx_vibra_play);
>> + if (ret) {
>> + dev_err(&pdev->dev, "failed to register vibrator to FF.\n");
>> + return ret;
>> + }
>> +
>> + ret = input_register_device(info->input_dev);
>> + if (ret) {
>> + dev_err(&pdev->dev, "failed to register input device.\n");
>> + return ret;
>> + }
>> +
>> + ret = sc27xx_vibra_hw_init(info);
>> + if (ret) {
>> + dev_err(&pdev->dev, "failed to initialize the vibrator.\n");
>> + return ret;
>> + }
>
> It is too late to initialize hardware after registering the input
> device, as once registered it should be fully functional. I'd recommend
> calling this before calling input_register_device(), or (maybe even
> better) doing this as part of open() method.
OK.
>
>> +
>> + platform_set_drvdata(pdev, info);
>
> I do not think you are using platform device's driver data anywhere.
We can remove it.
>
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id sc27xx_vibra_of_match[] = {
>> + { .compatible = "sprd,sc27xx-vibrator", },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(of, sc27xx_vibra_of_match);
>> +
>> +static struct platform_driver sc27xx_vibra_driver = {
>> + .driver = {
>> + .name = "sc27xx-vibrator",
>> + .of_match_table = sc27xx_vibra_of_match,
>
> Do you need suspend support by chance? To shut off the vibrator when
> system transitions to sleep?
We do not need the suspend support, disabling the vibrator is enough.
Thanks for your comments.
--
Baolin.wang
Best Regards
prev parent reply other threads:[~2018-04-24 2:44 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-04-23 2:33 [PATCH v2 1/2] dt-bindings: input: Add Add Spreadtrum SC27xx vibrator documentation Baolin Wang
2018-04-23 2:33 ` [PATCH v2 2/2] input: misc: Add Spreadtrum vibrator driver Baolin Wang
2018-04-23 17:29 ` Dmitry Torokhov
2018-04-24 2:44 ` Baolin Wang [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAMz4kuL8qsbQPfSE9vKkCuq41zK_RKq8KmnjaZ00Tga3zXHHKw@mail.gmail.com \
--to=baolin.wang@linaro.org \
--cc=devicetree@vger.kernel.org \
--cc=dmitry.torokhov@gmail.com \
--cc=linux-input@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=orsonzhai@gmail.com \
--cc=robh+dt@kernel.org \
--cc=xiaotong.lu@spreadtrum.com \
--cc=zhang.lyra@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.