lkml.org 
[lkml]   [2018]   [Mar]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH 1/2] input: Add RAVE SP Powerbutton driver
On Tue, Feb 27, 2018 at 8:59 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi Andrey,
>
> On Mon, Feb 26, 2018 at 07:41:29AM -0800, Andrey Smirnov wrote:
>> Add driver that properly handles input event emitted by RAVE SP
>> devices.
>>
>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Cc: linux-input@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: devicetree@vger.kernel.org
>> Cc: Guenter Roeck <linux@roeck-us.net>
>> Cc: Chris Healy <cphealy@gmail.com>
>> Cc: Lucas Stach <l.stach@pengutronix.de>
>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>> ---
>> drivers/input/misc/Kconfig | 9 ++++
>> drivers/input/misc/Makefile | 1 +
>> drivers/input/misc/rave-sp-pwrbutton.c | 92 ++++++++++++++++++++++++++++++++++
>> 3 files changed, 102 insertions(+)
>> create mode 100644 drivers/input/misc/rave-sp-pwrbutton.c
>>
>> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
>> index 62a1312a7387..6a3c753b093b 100644
>> --- a/drivers/input/misc/Kconfig
>> +++ b/drivers/input/misc/Kconfig
>> @@ -841,4 +841,13 @@ config INPUT_HISI_POWERKEY
>> To compile this driver as a module, choose M here: the
>> module will be called hisi_powerkey.
>>
>> +config INPUT_RAVE_SP_PWRBUTTON
>> + tristate "RAVE SP Power button Driver"
>> + depends on RAVE_SP_CORE
>> + help
>> + Say Y here if you want to enable power key reporting from RAVE SP
>> +
>> + To compile this driver as a module, choose M here: the
>> + module will be called rave-sp-pwrbutton.
>> +
>> endif
>> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
>> index a8f61af865aa..6ebeec1f4dd0 100644
>> --- a/drivers/input/misc/Makefile
>> +++ b/drivers/input/misc/Makefile
>> @@ -80,3 +80,4 @@ obj-$(CONFIG_INPUT_WM831X_ON) += wm831x-on.o
>> obj-$(CONFIG_INPUT_XEN_KBDDEV_FRONTEND) += xen-kbdfront.o
>> obj-$(CONFIG_INPUT_YEALINK) += yealink.o
>> obj-$(CONFIG_INPUT_IDEAPAD_SLIDEBAR) += ideapad_slidebar.o
>> +obj-$(CONFIG_INPUT_RAVE_SP_PWRBUTTON) += rave-sp-pwrbutton.o
>
> Please put it in [somewhat] alphabetical order (even though ideapad is
> out of order).
>

OK, will do in v2.

>
>> diff --git a/drivers/input/misc/rave-sp-pwrbutton.c b/drivers/input/misc/rave-sp-pwrbutton.c
>> new file mode 100644
>> index 000000000000..7511062c724b
>> --- /dev/null
>> +++ b/drivers/input/misc/rave-sp-pwrbutton.c
>> @@ -0,0 +1,92 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +
>> +/*
>> + * Power Button driver for RAVE SP
>> + *
>> + * Copyright (C) 2017 Zodiac Inflight Innovations
>> + *
>> + */
>
> Let's keep SPDX and copyright in the same // comment block.
>

This goes against kernel coding style, but sure.

>> +
>> +#include <linux/input.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/mfd/rave-sp.h>
>> +#include <linux/platform_device.h>
>> +
>> +#define RAVE_SP_EVNT_BUTTON_PRESS (RAVE_SP_EVNT_BASE + 0x00)
>> +
>> +struct rave_sp_power_button {
>> + struct input_dev *idev;
>> + struct notifier_block nb;
>> +};
>> +
>> +static int rave_sp_power_button_event(struct notifier_block *nb,
>> + unsigned long action, void *data)
>> +{
>> + struct rave_sp_power_button *pb =
>> + container_of(nb, struct rave_sp_power_button, nb);
>> + const u8 event = rave_sp_action_unpack_event(action);
>> + const u8 value = rave_sp_action_unpack_value(action);
>> + struct input_dev *idev = pb->idev;
>> +
>> + if (event == RAVE_SP_EVNT_BUTTON_PRESS) {
>> + input_report_key(idev, KEY_POWER, value);
>> + input_sync(idev);
>> +
>> + return NOTIFY_STOP;
>> + }
>> +
>> + return NOTIFY_DONE;
>> +}
>> +
>> +static int rave_sp_pwrbutton_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct rave_sp_power_button *pb;
>> + struct input_dev *idev;
>> + int ret;
>> +
>> + pb = devm_kzalloc(dev, sizeof(*pb), GFP_KERNEL);
>> + if (!pb)
>> + return -ENOMEM;
>> +
>> + idev = devm_input_allocate_device(dev);
>> + if (!idev)
>> + return -ENOMEM;
>> +
>> + idev->name = pdev->name;
>> + idev->dev.parent = dev;
>
> Not needed - done by devm_input_allocate_device().
>

Good point, will remove.

>> +
>> + input_set_capability(idev, EV_KEY, KEY_POWER);
>> +
>> + ret = input_register_device(idev);
>> + if (ret)
>> + return ret;
>
> Please call this "error".

Sure, will do.

>
>> +
>> + pb->idev = idev;
>> + pb->nb.notifier_call = rave_sp_power_button_event;
>> + pb->nb.priority = 128;
>> +
>> + return devm_rave_sp_register_event_notifier(dev, &pb->nb);
>
> Prefer
>
> error = devm_rave...();
> if (error)
> return error;
>
> return 0;
>

OK, will do in v2.

Thanks,
Andrey Smirnov

\
 
 \ /
  Last update: 2018-03-01 17:06    [W:0.047 / U:0.164 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site