* [PATCH 0/4] input: sirfsoc-onkey - a bundle of minor clean or fix
From: Barry Song @ 2014-02-13 6:40 UTC (permalink / raw)
To: dmitry.torokhov, linux-input; +Cc: workgroup.linux, Barry Song
From: Barry Song <Baohua.Song@csr.com>
Hi Dmitry,
this patchset depends on Xianglong's
"[PATCH v2] input: sirfsoc-onkey - report onkey untouch event by
detecting pin status"
Barry Song (2):
input: sirfsoc-onkey - drop the IRQF_SHARED flag
input: sirfsoc-onkey - update copyright years to 2014
Xianglong Du (2):
input: sirfsoc-onkey - namespace pwrc_resume function
input: sirfsoc-onkey - use dev_get_drvdata instead of
platform_get_drvdata
drivers/input/misc/sirfsoc-onkey.c | 12 ++++++------
1 files changed, 6 insertions(+), 6 deletions(-)
--
1.7.5.4
^ permalink raw reply
* [PATCH 1/4] input: sirfsoc-onkey - drop the IRQF_SHARED flag
From: Barry Song @ 2014-02-13 6:40 UTC (permalink / raw)
To: dmitry.torokhov, linux-input; +Cc: workgroup.linux, Barry Song
In-Reply-To: <1392273624-22920-1-git-send-email-21cnbao@gmail.com>
From: Barry Song <Baohua.Song@csr.com>
since the irq handler always return IRQ_HANDLED, it means this irq is not
a shared IRQ at all. or at least, the SW is not self-consistent now.
Signed-off-by: Barry Song <Baohua.Song@csr.com>
---
drivers/input/misc/sirfsoc-onkey.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/input/misc/sirfsoc-onkey.c b/drivers/input/misc/sirfsoc-onkey.c
index 4d13903..4d54744 100644
--- a/drivers/input/misc/sirfsoc-onkey.c
+++ b/drivers/input/misc/sirfsoc-onkey.c
@@ -114,7 +114,7 @@ static int sirfsoc_pwrc_probe(struct platform_device *pdev)
pwrcdrv->irq = platform_get_irq(pdev, 0);
error = devm_request_irq(&pdev->dev, pwrcdrv->irq,
- sirfsoc_pwrc_isr, IRQF_SHARED,
+ sirfsoc_pwrc_isr, 0,
"sirfsoc_pwrc_int", pwrcdrv);
if (error) {
dev_err(&pdev->dev, "unable to claim irq %d, error: %d\n",
--
1.7.5.4
^ permalink raw reply related
* [PATCH 2/4] input: sirfsoc-onkey - namespace pwrc_resume function
From: Barry Song @ 2014-02-13 6:40 UTC (permalink / raw)
To: dmitry.torokhov, linux-input; +Cc: workgroup.linux, Xianglong Du, Barry Song
In-Reply-To: <1392273624-22920-1-git-send-email-21cnbao@gmail.com>
From: Xianglong Du <Xianglong.Du@csr.com>
this function lost namespace, this patch fixes it.
Signed-off-by: Xianglong Du <Xianglong.Du@csr.com>
Signed-off-by: Barry Song <Baohua.Song@csr.com>
---
drivers/input/misc/sirfsoc-onkey.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/input/misc/sirfsoc-onkey.c b/drivers/input/misc/sirfsoc-onkey.c
index 4d54744..097c10a 100644
--- a/drivers/input/misc/sirfsoc-onkey.c
+++ b/drivers/input/misc/sirfsoc-onkey.c
@@ -155,7 +155,7 @@ static int sirfsoc_pwrc_remove(struct platform_device *pdev)
}
#ifdef CONFIG_PM_SLEEP
-static int pwrc_resume(struct device *dev)
+static int sirfsoc_pwrc_resume(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
struct sirfsoc_pwrc_drvdata *pwrcdrv = platform_get_drvdata(pdev);
@@ -173,7 +173,7 @@ static int pwrc_resume(struct device *dev)
}
#endif
-static SIMPLE_DEV_PM_OPS(sirfsoc_pwrc_pm_ops, NULL, pwrc_resume);
+static SIMPLE_DEV_PM_OPS(sirfsoc_pwrc_pm_ops, NULL, sirfsoc_pwrc_resume);
static struct platform_driver sirfsoc_pwrc_driver = {
.probe = sirfsoc_pwrc_probe,
--
1.7.5.4
^ permalink raw reply related
* [PATCH 3/4] input: sirfsoc-onkey - use dev_get_drvdata instead of platform_get_drvdata
From: Barry Song @ 2014-02-13 6:40 UTC (permalink / raw)
To: dmitry.torokhov, linux-input; +Cc: workgroup.linux, Xianglong Du, Barry Song
In-Reply-To: <1392273624-22920-1-git-send-email-21cnbao@gmail.com>
From: Xianglong Du <Xianglong.Du@csr.com>
In resume entry, use dev_get_drvdata() instead of to_platform_device(dev) +
platform_get_drvdata(pdev).
Signed-off-by: Xianglong Du <Xianglong.Du@csr.com>
Signed-off-by: Barry Song <Baohua.Song@csr.com>
---
drivers/input/misc/sirfsoc-onkey.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/drivers/input/misc/sirfsoc-onkey.c b/drivers/input/misc/sirfsoc-onkey.c
index 097c10a..8e45bf11 100644
--- a/drivers/input/misc/sirfsoc-onkey.c
+++ b/drivers/input/misc/sirfsoc-onkey.c
@@ -157,8 +157,7 @@ static int sirfsoc_pwrc_remove(struct platform_device *pdev)
#ifdef CONFIG_PM_SLEEP
static int sirfsoc_pwrc_resume(struct device *dev)
{
- struct platform_device *pdev = to_platform_device(dev);
- struct sirfsoc_pwrc_drvdata *pwrcdrv = platform_get_drvdata(pdev);
+ struct sirfsoc_pwrc_drvdata *pwrcdrv = dev_get_drvdata(dev);
/*
* Do not mask pwrc interrupt as we want pwrc work as a wakeup source
--
1.7.5.4
^ permalink raw reply related
* [PATCH 4/4] input: sirfsoc-onkey - update copyright years to 2014
From: Barry Song @ 2014-02-13 6:40 UTC (permalink / raw)
To: dmitry.torokhov, linux-input; +Cc: workgroup.linux, Barry Song
In-Reply-To: <1392273624-22920-1-git-send-email-21cnbao@gmail.com>
From: Barry Song <Baohua.Song@csr.com>
Happy the year of horse, 2014.
,((((^`\
(((( (6 \
,((((( , \
,,,_ ,((((( /"._ ,`,
((((\\ ,... ,(((( / `-.-'
))) ;' `"'"'""(((( (
((( / ((( \
)) | |
(( | . ' |
)) \ _ ' `t ,.')
( | y;- -,-""'"-.\ \/
) / ./ ) / `\ \
|./ ( ( / /'
|| \\ //'|
|| \\ _//'||
|| )) |_/ ||
\_\ |_/ ||
`'" \_\
`'"
Signed-off-by: Barry Song <Baohua.Song@csr.com>
---
drivers/input/misc/sirfsoc-onkey.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/input/misc/sirfsoc-onkey.c b/drivers/input/misc/sirfsoc-onkey.c
index 8e45bf11..1a11207 100644
--- a/drivers/input/misc/sirfsoc-onkey.c
+++ b/drivers/input/misc/sirfsoc-onkey.c
@@ -1,7 +1,8 @@
/*
* Power key driver for SiRF PrimaII
*
- * Copyright (c) 2013 Cambridge Silicon Radio Limited, a CSR plc group company.
+ * Copyright (c) 2013 - 2014 Cambridge Silicon Radio Limited, a CSR plc group
+ * company.
*
* Licensed under GPLv2 or later.
*/
--
1.7.5.4
^ permalink raw reply related
* Re: [PATCH] input: sirfsoc-onkey - report onkey untouch event by detecting pin status
From: Barry Song @ 2014-02-13 6:48 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-input@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
DL-SHA-WorkGroupLinux, Xianglong Du, Rongjun Ying, Barry Song
In-Reply-To: <892ec8be-638e-48d6-9e7d-5175694a7eaf@email.android.com>
2014-02-13 11:41 GMT+08:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> On February 12, 2014 6:32:03 PM PST, Barry Song <21cnbao@gmail.com> wrote:
>>2014-02-13 7:11 GMT+08:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
>>> Hi Barry,
>>>
>>> On Mon, Feb 10, 2014 at 06:07:39PM +0800, Barry Song wrote:
>>>>
>>>> static int sirfsoc_pwrc_remove(struct platform_device *pdev)
>>>> {
>>>> + struct sirfsoc_pwrc_drvdata *pwrcdrv =
>>dev_get_drvdata(&pdev->dev);
>>>> +
>>>> device_init_wakeup(&pdev->dev, 0);
>>>>
>>>> + cancel_delayed_work_sync(&pwrcdrv->work);
>>>> +
>>>
>>> This is racy: interrupt is freed later and can schedule work again.
>>
>>thanks, Dmitry. i will do a manual devm_free_irq() before cancelling
>>the work and before devres removes the resources.
>
> Another option would be to use devm custom action to ensure that work is canceled after freeing IRQ.
yes. you did a great job to have a devm_add_action() API.
>
>
> --
> Dmitry
-barry
^ permalink raw reply
* Re: [PATCH v2] input: sirfsoc-onkey - report onkey untouch event by detecting pin status
From: Dmitry Torokhov @ 2014-02-13 7:23 UTC (permalink / raw)
To: Barry Song
Cc: linux-input, workgroup.linux, Xianglong Du, Rongjun Ying,
Barry Song
In-Reply-To: <1392273536-22848-1-git-send-email-21cnbao@gmail.com>
On Thu, Feb 13, 2014 at 02:38:56PM +0800, Barry Song wrote:
> From: Xianglong Du <Xianglong.Du@csr.com>
>
> this patch adds a delayed_work to detect the untouch of onkey since HW will
> not generate interrupt for it.
>
> at the same time, we move the KEY event to POWER instead of SUSPEND, which
> will be suitable for both Android and Linux. Userspace PowerManager Daemon
> will decide to suspend or shutdown based on how long we have touched onkey
>
> Signed-off-by: Xianglong Du <Xianglong.Du@csr.com>
> Signed-off-by: Rongjun Ying <Rongjun.Ying@csr.com>
> Signed-off-by: Barry Song <Baohua.Song@csr.com>
> ---
> -v2:
> avoid the race of reschedule the work in remove;
> fix the typo about reporting KEY_POWER;
>
> drivers/input/misc/sirfsoc-onkey.c | 60 +++++++++++++++++++++++++++---------
> 1 files changed, 45 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/input/misc/sirfsoc-onkey.c b/drivers/input/misc/sirfsoc-onkey.c
> index e8897c3..4d13903 100644
> --- a/drivers/input/misc/sirfsoc-onkey.c
> +++ b/drivers/input/misc/sirfsoc-onkey.c
> @@ -13,16 +13,45 @@
> #include <linux/input.h>
> #include <linux/rtc/sirfsoc_rtciobrg.h>
> #include <linux/of.h>
> +#include <linux/workqueue.h>
>
> struct sirfsoc_pwrc_drvdata {
> u32 pwrc_base;
> struct input_dev *input;
> + int irq;
> + struct delayed_work work;
> };
>
> #define PWRC_ON_KEY_BIT (1 << 0)
>
> #define PWRC_INT_STATUS 0xc
> #define PWRC_INT_MASK 0x10
> +#define PWRC_PIN_STATUS 0x14
> +#define PWRC_KEY_DETECT_UP_TIME 20 /* ms*/
> +
> +static inline int sirfsoc_pwrc_is_on_key_down(
> + struct sirfsoc_pwrc_drvdata *pwrcdrv)
> +{
> + int state = sirfsoc_rtc_iobrg_readl(
> + pwrcdrv->pwrc_base + PWRC_PIN_STATUS)
> + & PWRC_ON_KEY_BIT;
> + return !state; /* ON_KEY is active low */
> +}
> +
> +static void sirfsoc_pwrc_report_event(struct work_struct *work)
> +{
> + struct sirfsoc_pwrc_drvdata *pwrcdrv =
> + container_of((struct delayed_work *)work,
> + struct sirfsoc_pwrc_drvdata, work);
> +
> + if (!sirfsoc_pwrc_is_on_key_down(pwrcdrv)) {
> + input_event(pwrcdrv->input, EV_KEY, KEY_POWER, 0);
> + input_sync(pwrcdrv->input);
> + } else {
> + schedule_delayed_work(&pwrcdrv->work,
> + msecs_to_jiffies(PWRC_KEY_DETECT_UP_TIME));
> + }
> +}
>
> static irqreturn_t sirfsoc_pwrc_isr(int irq, void *dev_id)
> {
> @@ -34,17 +63,11 @@ static irqreturn_t sirfsoc_pwrc_isr(int irq, void *dev_id)
> sirfsoc_rtc_iobrg_writel(int_status & ~PWRC_ON_KEY_BIT,
> pwrcdrv->pwrc_base + PWRC_INT_STATUS);
>
> - /*
> - * For a typical Linux system, we report KEY_SUSPEND to trigger apm-power.c
> - * to queue a SUSPEND APM event
> - */
> - input_event(pwrcdrv->input, EV_PWR, KEY_SUSPEND, 1);
> - input_sync(pwrcdrv->input);
>
> - /*
> - * Todo: report KEY_POWER event for Android platforms, Android PowerManager
> - * will handle the suspend and powerdown/hibernation
> - */
> + input_event(pwrcdrv->input, EV_KEY, KEY_POWER, 1);
> + input_sync(pwrcdrv->input);
> + schedule_delayed_work(&pwrcdrv->work,
> + msecs_to_jiffies(PWRC_KEY_DETECT_UP_TIME));
>
> return IRQ_HANDLED;
> }
> @@ -59,7 +82,6 @@ static int sirfsoc_pwrc_probe(struct platform_device *pdev)
> {
> struct device_node *np = pdev->dev.of_node;
> struct sirfsoc_pwrc_drvdata *pwrcdrv;
> - int irq;
> int error;
>
> pwrcdrv = devm_kzalloc(&pdev->dev, sizeof(struct sirfsoc_pwrc_drvdata),
> @@ -86,15 +108,17 @@ static int sirfsoc_pwrc_probe(struct platform_device *pdev)
>
> pwrcdrv->input->name = "sirfsoc pwrckey";
> pwrcdrv->input->phys = "pwrc/input0";
> - pwrcdrv->input->evbit[0] = BIT_MASK(EV_PWR);
> + pwrcdrv->input->evbit[0] = BIT_MASK(EV_KEY);
>
> - irq = platform_get_irq(pdev, 0);
> - error = devm_request_irq(&pdev->dev, irq,
> + INIT_DELAYED_WORK(&pwrcdrv->work, sirfsoc_pwrc_report_event);
> +
> + pwrcdrv->irq = platform_get_irq(pdev, 0);
> + error = devm_request_irq(&pdev->dev, pwrcdrv->irq,
> sirfsoc_pwrc_isr, IRQF_SHARED,
> "sirfsoc_pwrc_int", pwrcdrv);
> if (error) {
> dev_err(&pdev->dev, "unable to claim irq %d, error: %d\n",
> - irq, error);
> + pwrcdrv->irq, error);
> return error;
> }
>From this point on you should ensure that work is cancelled in error
unwinding path, similarly to sirfsoc_pwrc_probe().
I think custom devm action is way to go - it follows the devm model
instead of going against it with forced devm_free_irq().
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 3/4] input: sirfsoc-onkey - use dev_get_drvdata instead of platform_get_drvdata
From: Dmitry Torokhov @ 2014-02-13 7:24 UTC (permalink / raw)
To: Barry Song; +Cc: linux-input, workgroup.linux, Xianglong Du, Barry Song
In-Reply-To: <1392273624-22920-4-git-send-email-21cnbao@gmail.com>
On Thu, Feb 13, 2014 at 02:40:23PM +0800, Barry Song wrote:
> From: Xianglong Du <Xianglong.Du@csr.com>
>
> In resume entry, use dev_get_drvdata() instead of to_platform_device(dev) +
> platform_get_drvdata(pdev).
>
> Signed-off-by: Xianglong Du <Xianglong.Du@csr.com>
> Signed-off-by: Barry Song <Baohua.Song@csr.com>
> ---
> drivers/input/misc/sirfsoc-onkey.c | 3 +--
> 1 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/misc/sirfsoc-onkey.c b/drivers/input/misc/sirfsoc-onkey.c
> index 097c10a..8e45bf11 100644
> --- a/drivers/input/misc/sirfsoc-onkey.c
> +++ b/drivers/input/misc/sirfsoc-onkey.c
> @@ -157,8 +157,7 @@ static int sirfsoc_pwrc_remove(struct platform_device *pdev)
> #ifdef CONFIG_PM_SLEEP
> static int sirfsoc_pwrc_resume(struct device *dev)
> {
> - struct platform_device *pdev = to_platform_device(dev);
> - struct sirfsoc_pwrc_drvdata *pwrcdrv = platform_get_drvdata(pdev);
> + struct sirfsoc_pwrc_drvdata *pwrcdrv = dev_get_drvdata(dev);
I believe we should use accessors matching the object type. Currently
dev_get_drvdata and platform_get_drvdata return the same object, but
they do not have to.
IOW I prefer the original code.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2] input: sirfsoc-onkey - report onkey untouch event by detecting pin status
From: Barry Song @ 2014-02-13 7:30 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-input@vger.kernel.org, DL-SHA-WorkGroupLinux, Xianglong Du,
Rongjun Ying, Barry Song
In-Reply-To: <20140213072339.GA23392@core.coreip.homeip.net>
2014-02-13 15:23 GMT+08:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
>
> On Thu, Feb 13, 2014 at 02:38:56PM +0800, Barry Song wrote:
> > From: Xianglong Du <Xianglong.Du@csr.com>
> >
> > this patch adds a delayed_work to detect the untouch of onkey since HW will
> > not generate interrupt for it.
> >
> > at the same time, we move the KEY event to POWER instead of SUSPEND, which
> > will be suitable for both Android and Linux. Userspace PowerManager Daemon
> > will decide to suspend or shutdown based on how long we have touched onkey
> >
> > Signed-off-by: Xianglong Du <Xianglong.Du@csr.com>
> > Signed-off-by: Rongjun Ying <Rongjun.Ying@csr.com>
> > Signed-off-by: Barry Song <Baohua.Song@csr.com>
> > ---
> > -v2:
> > avoid the race of reschedule the work in remove;
> > fix the typo about reporting KEY_POWER;
> >
> > drivers/input/misc/sirfsoc-onkey.c | 60 +++++++++++++++++++++++++++---------
> > 1 files changed, 45 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/input/misc/sirfsoc-onkey.c b/drivers/input/misc/sirfsoc-onkey.c
> > index e8897c3..4d13903 100644
> > --- a/drivers/input/misc/sirfsoc-onkey.c
> > +++ b/drivers/input/misc/sirfsoc-onkey.c
> > @@ -13,16 +13,45 @@
> > #include <linux/input.h>
> > #include <linux/rtc/sirfsoc_rtciobrg.h>
> > #include <linux/of.h>
> > +#include <linux/workqueue.h>
> >
> > struct sirfsoc_pwrc_drvdata {
> > u32 pwrc_base;
> > struct input_dev *input;
> > + int irq;
> > + struct delayed_work work;
> > };
> >
> > #define PWRC_ON_KEY_BIT (1 << 0)
> >
> > #define PWRC_INT_STATUS 0xc
> > #define PWRC_INT_MASK 0x10
> > +#define PWRC_PIN_STATUS 0x14
> > +#define PWRC_KEY_DETECT_UP_TIME 20 /* ms*/
> > +
> > +static inline int sirfsoc_pwrc_is_on_key_down(
> > + struct sirfsoc_pwrc_drvdata *pwrcdrv)
> > +{
> > + int state = sirfsoc_rtc_iobrg_readl(
> > + pwrcdrv->pwrc_base + PWRC_PIN_STATUS)
> > + & PWRC_ON_KEY_BIT;
> > + return !state; /* ON_KEY is active low */
> > +}
> > +
> > +static void sirfsoc_pwrc_report_event(struct work_struct *work)
> > +{
> > + struct sirfsoc_pwrc_drvdata *pwrcdrv =
> > + container_of((struct delayed_work *)work,
> > + struct sirfsoc_pwrc_drvdata, work);
> > +
> > + if (!sirfsoc_pwrc_is_on_key_down(pwrcdrv)) {
> > + input_event(pwrcdrv->input, EV_KEY, KEY_POWER, 0);
> > + input_sync(pwrcdrv->input);
> > + } else {
> > + schedule_delayed_work(&pwrcdrv->work,
> > + msecs_to_jiffies(PWRC_KEY_DETECT_UP_TIME));
> > + }
> > +}
> >
> > static irqreturn_t sirfsoc_pwrc_isr(int irq, void *dev_id)
> > {
> > @@ -34,17 +63,11 @@ static irqreturn_t sirfsoc_pwrc_isr(int irq, void *dev_id)
> > sirfsoc_rtc_iobrg_writel(int_status & ~PWRC_ON_KEY_BIT,
> > pwrcdrv->pwrc_base + PWRC_INT_STATUS);
> >
> > - /*
> > - * For a typical Linux system, we report KEY_SUSPEND to trigger apm-power.c
> > - * to queue a SUSPEND APM event
> > - */
> > - input_event(pwrcdrv->input, EV_PWR, KEY_SUSPEND, 1);
> > - input_sync(pwrcdrv->input);
> >
> > - /*
> > - * Todo: report KEY_POWER event for Android platforms, Android PowerManager
> > - * will handle the suspend and powerdown/hibernation
> > - */
> > + input_event(pwrcdrv->input, EV_KEY, KEY_POWER, 1);
> > + input_sync(pwrcdrv->input);
> > + schedule_delayed_work(&pwrcdrv->work,
> > + msecs_to_jiffies(PWRC_KEY_DETECT_UP_TIME));
> >
> > return IRQ_HANDLED;
> > }
> > @@ -59,7 +82,6 @@ static int sirfsoc_pwrc_probe(struct platform_device *pdev)
> > {
> > struct device_node *np = pdev->dev.of_node;
> > struct sirfsoc_pwrc_drvdata *pwrcdrv;
> > - int irq;
> > int error;
> >
> > pwrcdrv = devm_kzalloc(&pdev->dev, sizeof(struct sirfsoc_pwrc_drvdata),
> > @@ -86,15 +108,17 @@ static int sirfsoc_pwrc_probe(struct platform_device *pdev)
> >
> > pwrcdrv->input->name = "sirfsoc pwrckey";
> > pwrcdrv->input->phys = "pwrc/input0";
> > - pwrcdrv->input->evbit[0] = BIT_MASK(EV_PWR);
> > + pwrcdrv->input->evbit[0] = BIT_MASK(EV_KEY);
> >
> > - irq = platform_get_irq(pdev, 0);
> > - error = devm_request_irq(&pdev->dev, irq,
> > + INIT_DELAYED_WORK(&pwrcdrv->work, sirfsoc_pwrc_report_event);
> > +
> > + pwrcdrv->irq = platform_get_irq(pdev, 0);
> > + error = devm_request_irq(&pdev->dev, pwrcdrv->irq,
> > sirfsoc_pwrc_isr, IRQF_SHARED,
> > "sirfsoc_pwrc_int", pwrcdrv);
> > if (error) {
> > dev_err(&pdev->dev, "unable to claim irq %d, error: %d\n",
> > - irq, error);
> > + pwrcdrv->irq, error);
> > return error;
> > }
>
> From this point on you should ensure that work is cancelled in error
> unwinding path, similarly to sirfsoc_pwrc_probe().
>
> I think custom devm action is way to go - it follows the devm model
> instead of going against it with forced devm_free_irq().
yes. in case probe fails.
>
> Thanks.
>
> --
> Dmitry
-barry
^ permalink raw reply
* Re: [PATCH 3/4] input: sirfsoc-onkey - use dev_get_drvdata instead of platform_get_drvdata
From: Barry Song @ 2014-02-13 7:47 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-input@vger.kernel.org, DL-SHA-WorkGroupLinux, Xianglong Du,
Barry Song
In-Reply-To: <20140213072459.GB23392@core.coreip.homeip.net>
2014-02-13 15:24 GMT+08:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> On Thu, Feb 13, 2014 at 02:40:23PM +0800, Barry Song wrote:
>> From: Xianglong Du <Xianglong.Du@csr.com>
>>
>> In resume entry, use dev_get_drvdata() instead of to_platform_device(dev) +
>> platform_get_drvdata(pdev).
>>
>> Signed-off-by: Xianglong Du <Xianglong.Du@csr.com>
>> Signed-off-by: Barry Song <Baohua.Song@csr.com>
>> ---
>> drivers/input/misc/sirfsoc-onkey.c | 3 +--
>> 1 files changed, 1 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/input/misc/sirfsoc-onkey.c b/drivers/input/misc/sirfsoc-onkey.c
>> index 097c10a..8e45bf11 100644
>> --- a/drivers/input/misc/sirfsoc-onkey.c
>> +++ b/drivers/input/misc/sirfsoc-onkey.c
>> @@ -157,8 +157,7 @@ static int sirfsoc_pwrc_remove(struct platform_device *pdev)
>> #ifdef CONFIG_PM_SLEEP
>> static int sirfsoc_pwrc_resume(struct device *dev)
>> {
>> - struct platform_device *pdev = to_platform_device(dev);
>> - struct sirfsoc_pwrc_drvdata *pwrcdrv = platform_get_drvdata(pdev);
>> + struct sirfsoc_pwrc_drvdata *pwrcdrv = dev_get_drvdata(dev);
>
> I believe we should use accessors matching the object type. Currently
> dev_get_drvdata and platform_get_drvdata return the same object, but
> they do not have to.
>
> IOW I prefer the original code.
i did see many commits in kernel which did same jobs with this one. e.g:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=a1216394e620d0dfbb03c712ae3210e7b77c9e11
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=bab8b563ef08455440badca7fe79b2c700bd1b75
...
in my understand, the changed context is a general pm_ops to general
"driver" and not a legacy suspend/resume in "platform_driver" bound
with pdev, so in the context, we are caring about "device" more than
"pdev".
how do you think if i do a more change in probe() with this by:
- platform_set_drvdata(pdev, pwrcdrv);
+ dev_set_drvdata(&pdev->dev, pwrcdrv);
would this make everything look fine?
>
> Thanks.
>
> --
> Dmitry
-barry
^ permalink raw reply
* Re: [PATCH 1/2] gpio: adp5588: get value from data out when dir is out
From: Linus Walleij @ 2014-02-13 12:50 UTC (permalink / raw)
To: Jean-Francois Dagenais
Cc: Michael Hennerich, Dmitry Torokhov, dtor, Alexandre Courbot,
linux-gpio@vger.kernel.org, Linux Input
In-Reply-To: <1392051929-32290-1-git-send-email-jeff.dagenais@gmail.com>
On Mon, Feb 10, 2014 at 6:05 PM, Jean-Francois Dagenais
<jeff.dagenais@gmail.com> wrote:
> As discussed here: http://ez.analog.com/message/35852,
> the 5587 revC and 5588 revB spec sheets contain a mistake
> in the GPIO_DAT_STATx register description.
>
> According to R.Shnell at ADI, as well as my own
> observations, it should read:
> "GPIO data status (shows GPIO state when read for inputs)".
>
> This commit changes the get value function accordingly.
>
> Signed-off-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
Patch applied with Michael's ACK.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 1/2] gpio: adp5588 - use "unsigned" for the setup and teardown callbacks
From: Linus Walleij @ 2014-02-13 12:52 UTC (permalink / raw)
To: Jean-Francois Dagenais
Cc: Michael Hennerich, Dmitry Torokhov, dtor, Alexandre Courbot,
linux-gpio@vger.kernel.org, Linux Input
In-Reply-To: <1392053045-12121-1-git-send-email-jeff.dagenais@gmail.com>
On Mon, Feb 10, 2014 at 6:24 PM, Jean-Francois Dagenais
<jeff.dagenais@gmail.com> wrote:
> to comply with the rest of the GPIO drivers.
>
> Signed-off-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
Patch applied with Michael's ACK.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 2/2] gpio: adp5588 - add support for gpio names
From: Linus Walleij @ 2014-02-13 12:53 UTC (permalink / raw)
To: Jean-Francois Dagenais
Cc: Michael Hennerich, Dmitry Torokhov, dtor, Alexandre Courbot,
linux-gpio@vger.kernel.org, Linux Input
In-Reply-To: <1392053045-12121-2-git-send-email-jeff.dagenais@gmail.com>
On Mon, Feb 10, 2014 at 6:24 PM, Jean-Francois Dagenais
<jeff.dagenais@gmail.com> wrote:
> which is already found in the common header for adp5588
>
> Signed-off-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com>
Patch applied with Michael's ACK.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH v2 0/9] HID: spring cleanup v2
From: Benjamin Tissoires @ 2014-02-13 15:11 UTC (permalink / raw)
To: David Herrmann, Nestor Lopez Casado
Cc: Benjamin Tissoires, Jiri Kosina, Frank Praznik,
open list:HID CORE LAYER, linux-kernel
In-Reply-To: <CANq1E4RvavkaPvz8vozUvgOs6fSmYNR=2=sn76aFFCbR5caaBA@mail.gmail.com>
Hi David,
On Wed, Feb 12, 2014 at 5:13 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Wed, Feb 5, 2014 at 10:33 PM, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
>> Hi guys,
>>
>> well, here comes the promised v2 of the ll_transport cleanup.
>>
>> As I said, I removed patches which need some more work, and kept only the
>> trivial ones. I also added David's documentation, which gives us a net
>> difference of +210 lines of code :(
>> Let's say that we still have a net worth of -106 lines of actual code :)
>>
>> Cheers,
>> Benjamin
>>
>> Changes since v1:
>> - removed uhid, i2c-hid patches
>> - removed the previous 11/11 (move hid_output_raw_report to hid_ll_driver)
>> - hid-logitech-dj: use hid_hw_raw_request instead of hid_output_report (2/9)
>> - add documentation - I removed the hid_input_event() doc (9/9)
>>
>> Benjamin Tissoires (9):
>> HID: add inliners for ll_driver transport-layer callbacks
>> HID: logitech-dj: remove hidinput_input_event
>
> Apart from this logitech-dj patch, which I feel uncomfortable with
> reviewing if I don't have the hardware, this series looks really good.
> I think all patches already carry my r-b, otherwise feel free to add
> them.
Thanks a lot for the review!
I tested the logitech one with a TK820 which has a LED on the
capslock, and it's working fine. Still adding Nestor in the loop if he
manages to find some time to review it.
Nestor, could you have a look at 2/9
(https://patchwork.kernel.org/patch/3591381/)?
Cheers,
Benjamin
>
> Thanks a lot for the cleanup!
> David
>
>> HID: HIDp: remove hidp_hidinput_event
>> HID: remove hidinput_input_event handler
>> HID: HIDp: remove duplicated coded
>> HID: usbhid: remove duplicated code
>> HID: remove hid_get_raw_report in struct hid_device
>> HID: introduce helper to access hid_output_raw_report()
>> HID: Add HID transport driver documentation
>>
>> Documentation/hid/hid-transport.txt | 316 ++++++++++++++++++++++++++++++++++++
>> drivers/hid/hid-input.c | 12 +-
>> drivers/hid/hid-lg.c | 6 +-
>> drivers/hid/hid-logitech-dj.c | 106 +++++-------
>> drivers/hid/hid-magicmouse.c | 2 +-
>> drivers/hid/hid-sony.c | 9 +-
>> drivers/hid/hid-thingm.c | 4 +-
>> drivers/hid/hid-wacom.c | 16 +-
>> drivers/hid/hid-wiimote-core.c | 2 +-
>> drivers/hid/hidraw.c | 9 +-
>> drivers/hid/i2c-hid/i2c-hid.c | 1 -
>> drivers/hid/uhid.c | 1 -
>> drivers/hid/usbhid/hid-core.c | 65 ++------
>> include/linux/hid.h | 68 +++++++-
>> net/bluetooth/hidp/core.c | 115 ++-----------
>> 15 files changed, 471 insertions(+), 261 deletions(-)
>> create mode 100644 Documentation/hid/hid-transport.txt
>>
>> --
>> 1.8.3.1
>>
^ permalink raw reply
* Re: [PATCH 05/14] HID: i2c-hid: use generic .request() implementation
From: Benjamin Tissoires @ 2014-02-13 15:14 UTC (permalink / raw)
To: David Herrmann
Cc: Benjamin Tissoires, Jiri Kosina, open list:HID CORE LAYER,
linux-kernel
In-Reply-To: <CANq1E4SYvLAhH3WqGNCZBrxwd8RiwL-wH90tZP9Je1doDxvhXQ@mail.gmail.com>
On Wed, Feb 12, 2014 at 5:30 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
>> Having our own .request() implementation does not give anything,
>> so use the generic binding.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> ---
>> drivers/hid/i2c-hid/i2c-hid.c | 31 -------------------------------
>> 1 file changed, 31 deletions(-)
>>
>> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
>> index 07a054a..925fb8d 100644
>> --- a/drivers/hid/i2c-hid/i2c-hid.c
>> +++ b/drivers/hid/i2c-hid/i2c-hid.c
>> @@ -632,36 +632,6 @@ static int i2c_hid_raw_request(struct hid_device *hid, unsigned char reportnum,
>> }
>> }
>>
>> -static void i2c_hid_request(struct hid_device *hid, struct hid_report *rep,
>> - int reqtype)
>> -{
>> - struct i2c_client *client = hid->driver_data;
>> - char *buf;
>> - int ret;
>> - int len = i2c_hid_get_report_length(rep) - 2;
>> -
>> - buf = kzalloc(len, GFP_KERNEL);
>
> Haven't you recently fixed this to use hid_alloc_buffer()? Anyhow,
yes, it has been fixed. But Jiri carried it through a *-upstream
branch, which is not merged into his for-next currently (or at the
time I sent the patch). hopefully the merge will be easy, or I can
wait for it to be in for-next before resending a smoother v2.
Cheers,
Benjamin
> patch obviously looks good:
>
> Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
>
> Thanks
> David
>
>> - if (!buf)
>> - return;
>> -
>> - switch (reqtype) {
>> - case HID_REQ_GET_REPORT:
>> - ret = i2c_hid_get_raw_report(hid, rep->id, buf, len, rep->type);
>> - if (ret < 0)
>> - dev_err(&client->dev, "%s: unable to get report: %d\n",
>> - __func__, ret);
>> - else
>> - hid_input_report(hid, rep->type, buf, ret, 0);
>> - break;
>> - case HID_REQ_SET_REPORT:
>> - hid_output_report(rep, buf);
>> - i2c_hid_output_raw_report(hid, buf, len, rep->type, true);
>> - break;
>> - }
>> -
>> - kfree(buf);
>> -}
>> -
>> static int i2c_hid_parse(struct hid_device *hid)
>> {
>> struct i2c_client *client = hid->driver_data;
>> @@ -817,7 +787,6 @@ static struct hid_ll_driver i2c_hid_ll_driver = {
>> .open = i2c_hid_open,
>> .close = i2c_hid_close,
>> .power = i2c_hid_power,
>> - .request = i2c_hid_request,
>> .output_report = i2c_hid_output_report,
>> .raw_request = i2c_hid_raw_request,
>> };
>> --
>> 1.8.3.1
>>
^ permalink raw reply
* Re: [PATCH 12/14] HID: hidraw: replace hid_output_raw_report() calls by appropriates ones
From: Benjamin Tissoires @ 2014-02-13 15:16 UTC (permalink / raw)
To: David Herrmann
Cc: Benjamin Tissoires, Jiri Kosina, open list:HID CORE LAYER,
linux-kernel
In-Reply-To: <CANq1E4RvnReiak20x2HPeZ6evYzDuVnLtbT9-WNeZR9yQbeHXg@mail.gmail.com>
On Wed, Feb 12, 2014 at 5:49 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
>> Remove hid_output_raw_report() call as it is not a ll_driver callbacj,
>> and switch to the hid_hw_* implementation. USB-HID used to fallback
>> into SET_REPORT when there were no output interrupt endpoint,
>> so emulating this if hid_hw_output_report() returns -ENOSYS.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> ---
>> drivers/hid/hidraw.c | 22 +++++++++++++++++-----
>> 1 file changed, 17 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
>> index f8708c9..704718b 100644
>> --- a/drivers/hid/hidraw.c
>> +++ b/drivers/hid/hidraw.c
>> @@ -123,10 +123,8 @@ static ssize_t hidraw_send_report(struct file *file, const char __user *buffer,
>>
>> dev = hidraw_table[minor]->hid;
>>
>> - if (!dev->hid_output_raw_report) {
>> - ret = -ENODEV;
>> - goto out;
>> - }
>> + if (!dev->ll_driver->raw_request || !dev->ll_driver->output_report)
>> + return -ENODEV;
>
> You still have a "goto out;" above (not visible in the patch). The
> error paths look quite ugly now. I'd prefer consistency here, so
> either keep the jump or fix the error-path above, too. Otherwise looks
> good.
ok, will use "goto out" in the v2 then.
Cheers,
Benjamin
>
> Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
>
> Thanks
> David
>
>>
>> if (count > HID_MAX_BUFFER_SIZE) {
>> hid_warn(dev, "pid %d passed too large report\n",
>> @@ -153,7 +151,21 @@ static ssize_t hidraw_send_report(struct file *file, const char __user *buffer,
>> goto out_free;
>> }
>>
>> - ret = hid_output_raw_report(dev, buf, count, report_type);
>> + if (report_type == HID_OUTPUT_REPORT) {
>> + ret = hid_hw_output_report(dev, buf, count);
>> + /*
>> + * compatibility with old implementation of USB-HID and I2C-HID:
>> + * if the device does not support receiving output reports,
>> + * on an interrupt endpoint, then fallback to the SET_REPORT
>> + * HID command.
>> + */
>> + if (ret != -ENOSYS)
>> + goto out_free;
>> + }
>> +
>> + ret = hid_hw_raw_request(dev, buf[0], buf, count, report_type,
>> + HID_REQ_SET_REPORT);
>> +
>> out_free:
>> kfree(buf);
>> out:
>> --
>> 1.8.3.1
>>
^ permalink raw reply
* Re: [PATCH 03/14] HID: core: implement generic .request()
From: Benjamin Tissoires @ 2014-02-13 15:21 UTC (permalink / raw)
To: David Herrmann
Cc: Benjamin Tissoires, Jiri Kosina, open list:HID CORE LAYER,
linux-kernel
In-Reply-To: <CANq1E4TjLJUts20nVYE3WT5FXu_eswKrM0CLwQAu25df3GEDig@mail.gmail.com>
On Wed, Feb 12, 2014 at 5:25 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
>> .request() can be emulated through .raw_request()
>> we can implement this emulation in hid-core, and make .request
>> not mandatory for transport layer drivers.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> ---
>> drivers/hid/hid-core.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
>> include/linux/hid.h | 5 ++++-
>> 2 files changed, 48 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index 18fe49b..f36778a 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -1248,6 +1248,11 @@ void hid_output_report(struct hid_report *report, __u8 *data)
>> }
>> EXPORT_SYMBOL_GPL(hid_output_report);
>>
>> +static int hid_report_len(struct hid_report *report)
>> +{
>> + return ((report->size - 1) >> 3) + 1 + (report->id > 0) + 7;
>
> Just for clarity, this is equivalent to the following, right?
>
> return DIV_ROUND_UP(report->size, 8) + !!(report->id > 0) + 7;
yes, it should (at least that's what I understand too :)
>
> I always have to read that shifting code twice to get it.. Maybe add a
> comment explaining the different entries here.
good idea.
>
>> +}
>> +
>> /*
>> * Allocator for buffer that is going to be passed to hid_output_report()
>> */
>> @@ -1258,7 +1263,7 @@ u8 *hid_alloc_report_buf(struct hid_report *report, gfp_t flags)
>> * of implement() working on 8 byte chunks
>> */
>>
>> - int len = ((report->size - 1) >> 3) + 1 + (report->id > 0) + 7;
>> + int len = hid_report_len(report);
>>
>> return kmalloc(len, flags);
>> }
>> @@ -1314,6 +1319,44 @@ static struct hid_report *hid_get_report(struct hid_report_enum *report_enum,
>> return report;
>> }
>>
>> +/*
>> + * Implement a generic .request() callback, using .raw_request()
>> + * DO NOT USE in hid drivers directly, but through hid_hw_request instead.
>> + */
>> +void __hid_request(struct hid_device *hid, struct hid_report *report,
>> + int reqtype)
>> +{
>> + char *buf;
>> + int ret;
>> + int len;
>> +
>> + if (!hid->ll_driver->raw_request)
>> + return;
>> +
>> + buf = hid_alloc_report_buf(report, GFP_KERNEL);
>> + if (!buf)
>> + return;
>> +
>> + len = hid_report_len(report);
actually, after sending the patches, I was wondering if we should use
the +7 in hid_report_len.
"len" is used in .raw_request(), and the +7 was only for the implement(), right?
So maybe a device can reject this because the size of the report is too big...
Jiri, David, any ideas?
Cheers,
Benjamin
>> +
>> + if (reqtype == HID_REQ_SET_REPORT)
>> + hid_output_report(report, buf);
>> +
>> + ret = hid->ll_driver->raw_request(hid, report->id, buf, len,
>> + report->type, reqtype);
>> + if (ret < 0) {
>> + dbg_hid("unable to complete request: %d\n", ret);
>> + goto out;
>> + }
>> +
>> + if (reqtype == HID_REQ_GET_REPORT)
>> + hid_input_report(hid, report->type, buf, ret, 0);
>> +
>> +out:
>> + kfree(buf);
>> +}
>> +EXPORT_SYMBOL_GPL(__hid_request);
>> +
>
> Looks good to me.
>
> Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
>
> Thanks
> David
>
>> int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size,
>> int interrupt)
>> {
>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>> index a837ede..09fbbd7 100644
>> --- a/include/linux/hid.h
>> +++ b/include/linux/hid.h
>> @@ -753,6 +753,7 @@ struct hid_field *hidinput_get_led_field(struct hid_device *hid);
>> unsigned int hidinput_count_leds(struct hid_device *hid);
>> __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code);
>> void hid_output_report(struct hid_report *report, __u8 *data);
>> +void __hid_request(struct hid_device *hid, struct hid_report *rep, int reqtype);
>> u8 *hid_alloc_report_buf(struct hid_report *report, gfp_t flags);
>> struct hid_device *hid_allocate_device(void);
>> struct hid_report *hid_register_report(struct hid_device *device, unsigned type, unsigned id);
>> @@ -965,7 +966,9 @@ static inline void hid_hw_request(struct hid_device *hdev,
>> struct hid_report *report, int reqtype)
>> {
>> if (hdev->ll_driver->request)
>> - hdev->ll_driver->request(hdev, report, reqtype);
>> + return hdev->ll_driver->request(hdev, report, reqtype);
>> +
>> + __hid_request(hdev, report, reqtype);
>> }
>>
>> /**
>> --
>> 1.8.3.1
>>
^ permalink raw reply
* Re: [PATCH 07/14] HID: input: hid-input remove hid_output_raw_report call
From: Benjamin Tissoires @ 2014-02-13 15:38 UTC (permalink / raw)
To: David Herrmann
Cc: Benjamin Tissoires, Jiri Kosina, open list:HID CORE LAYER,
linux-kernel
In-Reply-To: <CANq1E4QvC936A_r6GPgO_gikpXGrD6UwBdBEcwUBwtjxW9iw6Q@mail.gmail.com>
On Wed, Feb 12, 2014 at 5:35 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
>> hid_output_raw_report() is not a ll_driver callback and should not be used.
>> To keep the same code path than before, we are forced to play with the
>> different hid_hw_* calls: if the usb or i2c device does not support
>> direct output reports, then we will rely on the SET_REPORT HID call.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> ---
>> drivers/hid/hid-input.c | 10 +++++++---
>> 1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
>> index eb00a5b..6b7bdca 100644
>> --- a/drivers/hid/hid-input.c
>> +++ b/drivers/hid/hid-input.c
>> @@ -1153,7 +1153,7 @@ static void hidinput_led_worker(struct work_struct *work)
>> led_work);
>> struct hid_field *field;
>> struct hid_report *report;
>> - int len;
>> + int len, ret;
>> __u8 *buf;
>>
>> field = hidinput_get_led_field(hid);
>> @@ -1187,7 +1187,10 @@ static void hidinput_led_worker(struct work_struct *work)
>>
>> hid_output_report(report, buf);
>> /* synchronous output report */
>> - hid_output_raw_report(hid, buf, len, HID_OUTPUT_REPORT);
>> + ret = hid_hw_output_report(hid, buf, len);
>> + if (ret == -ENOSYS)
>> + hid_hw_raw_request(hid, buf[0], buf, len, HID_OUTPUT_REPORT,
>> + HID_REQ_SET_REPORT);
>
> Does HID core always set the report-id in buf[0]? Even if none are
> used? I know the incoming data may lack the report-id, but I always
> thought we do the same for outgoing if it's implicit?
oh, yes, you are right. The HID spec says: "The meaning of the request
fields for the Set_Report request is the same as for the Get_Report
request, however the data direction is reversed and the Report Data is
sent from host to device. "
So this is wrong... We should use (report->id) instead of buf[0].
Will fix in v2.
>
> I also already see devices with broken OUTPUT_REPORTs.. I guess at
> some point we have to introduce a quirk-flag to choose between both
> calls. But lets wait for that to happen, maybe we're lucky.
>
>> kfree(buf);
>> }
>>
>> @@ -1266,7 +1269,8 @@ static struct hid_input *hidinput_allocate(struct hid_device *hid)
>> }
>>
>> input_set_drvdata(input_dev, hid);
>> - if (hid->ll_driver->request || hid->hid_output_raw_report)
>> + if (hid->ll_driver->request || hid->ll_driver->output_report ||
>> + hid->ll_driver->raw_request)
>
> Isn't raw_request mandatory? So we could remove that whole if() thing here.
Currently, it's not (see hid_hw_raw_request). However, all the
upstream hid transport drivers implement it.
We can make it mandatory, but we should check it while adding the
device in hid_add_device.
will do for v2 too.
Cheers,
Benjamin
>
> Thanks
> David
>
>> input_dev->event = hidinput_input_event;
>> input_dev->open = hidinput_open;
>> input_dev->close = hidinput_close;
>> --
>> 1.8.3.1
>>
^ permalink raw reply
* Re: [PATCH 11/14] HID: sony: remove hid_output_raw_report calls
From: Benjamin Tissoires @ 2014-02-13 15:46 UTC (permalink / raw)
To: David Herrmann
Cc: Benjamin Tissoires, Jiri Kosina, open list:HID CORE LAYER,
linux-kernel
In-Reply-To: <CANq1E4TjAdiMMPmqX9N__3Bqf2SVu2sV6_P-kOZ0dvZHwXDWwQ@mail.gmail.com>
On Wed, Feb 12, 2014 at 5:47 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
>> We can not directly change the underlying struct hid_ll_driver here
>> as we did for hdev->hid_output_raw_report.
>> So allocate a struct ll_driver, and replace the old one when removing
>> the device.
>> To get a fully functional driver, we must also split the function
>> sixaxis_usb_output_raw_report() in 2, one regarding output_report, and
>> the other for raw_request.
>
> Sorry, i cannot follow here. Could you explain why this is needed? And
Well, as mentioned in the original comments, the sixaxis has two problem:
- when you send an output report, you should call hid_hw_raw_request
instead (though there is an interrupt output endpoint)
- when you send a raw_request, you should drop the reportID in the
buffer you are sending, but keep it in the SET_REPORT call... (quite
annoying).
Splitting this in two (it was all implemented in hid_output_report)
allows to transparently use the hid_hw_call, not matter who calls
what.
> I also don't think you're supposed to change the ll_driver unlocked.
> The real ll_driver might access it in any way. I don't think we do it
> right now, but it might happen.
Yeah, I am not a big fan of this either. My other concern regarding
that is the dependency against usb, while it could be a uHID device.
>
> Why can't we introduce QUIRK flags that make HID core drop the
> report_id for outgoing messages?
I did not want to come with because it would make the bright new
ll_transport interface ugly, but this may be the only way to have
something generic and drop the usb dependency and the races that may
appears (plus the ugly alloc/free).
Cheers,
Benjamin
>
> Thanks
> David
>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> ---
>> drivers/hid/hid-sony.c | 75 ++++++++++++++++++++++++++++++++++----------------
>> 1 file changed, 52 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
>> index d980943..dbbcd0c8 100644
>> --- a/drivers/hid/hid-sony.c
>> +++ b/drivers/hid/hid-sony.c
>> @@ -507,6 +507,8 @@ struct sony_sc {
>> struct work_struct state_worker;
>> struct power_supply battery;
>>
>> + struct hid_ll_driver *prev_ll_driver;
>> +
>> #ifdef CONFIG_SONY_FF
>> __u8 left;
>> __u8 right;
>> @@ -760,38 +762,52 @@ static int sony_mapping(struct hid_device *hdev, struct hid_input *hi,
>>
>> /*
>> * The Sony Sixaxis does not handle HID Output Reports on the Interrupt EP
>> - * like it should according to usbhid/hid-core.c::usbhid_output_raw_report()
>> + * like it should according to usbhid/hid-core.c::usbhid_output_report()
>> * so we need to override that forcing HID Output Reports on the Control EP.
>> - *
>> - * There is also another issue about HID Output Reports via USB, the Sixaxis
>> - * does not want the report_id as part of the data packet, so we have to
>> - * discard buf[0] when sending the actual control message, even for numbered
>> - * reports, humpf!
>> */
>> -static int sixaxis_usb_output_raw_report(struct hid_device *hid, __u8 *buf,
>> - size_t count, unsigned char report_type)
>> +static int sixaxis_usb_output_report(struct hid_device *hid, __u8 *buf,
>> + size_t count)
>> +{
>> + return hid_hw_raw_request(hid, buf[0], buf, count, HID_OUTPUT_REPORT,
>> + HID_REQ_SET_REPORT);
>> +}
>> +
>> +/*
>> + * There is also another issue about the SET_REPORT command via USB,
>> + * the Sixaxis does not want the report_id as part of the data packet, so
>> + * we have to discard buf[0] when sending the actual control message, even
>> + * for numbered reports, humpf!
>> + */
>> +static int sixaxis_usb_raw_request(struct hid_device *hid,
>> + unsigned char reportnum, __u8 *buf,
>> + size_t len, unsigned char rtype, int reqtype)
>> {
>> struct usb_interface *intf = to_usb_interface(hid->dev.parent);
>> struct usb_device *dev = interface_to_usbdev(intf);
>> struct usb_host_interface *interface = intf->cur_altsetting;
>> - int report_id = buf[0];
>> int ret;
>> + u8 usb_dir;
>> + unsigned int usb_pipe;
>>
>> - if (report_type == HID_OUTPUT_REPORT) {
>> + if (reqtype == HID_REQ_SET_REPORT) {
>> /* Don't send the Report ID */
>> buf++;
>> - count--;
>> + len--;
>> + usb_dir = USB_DIR_OUT;
>> + usb_pipe = usb_sndctrlpipe(dev, 0);
>> + } else {
>> + usb_dir = USB_DIR_IN;
>> + usb_pipe = usb_rcvctrlpipe(dev, 0);
>> }
>>
>> - ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
>> - HID_REQ_SET_REPORT,
>> - USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
>> - ((report_type + 1) << 8) | report_id,
>> - interface->desc.bInterfaceNumber, buf, count,
>> + ret = usb_control_msg(dev, usb_pipe, reqtype,
>> + usb_dir | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
>> + ((rtype + 1) << 8) | reportnum,
>> + interface->desc.bInterfaceNumber, buf, len,
>> USB_CTRL_SET_TIMEOUT);
>>
>> - /* Count also the Report ID, in case of an Output report. */
>> - if (ret > 0 && report_type == HID_OUTPUT_REPORT)
>> + /* Count also the Report ID. */
>> + if (ret > 0 && reqtype == HID_REQ_SET_REPORT)
>> ret++;
>>
>> return ret;
>> @@ -1047,7 +1063,7 @@ static void sixaxis_state_worker(struct work_struct *work)
>> buf[10] |= sc->led_state[2] << 3;
>> buf[10] |= sc->led_state[3] << 4;
>>
>> - hid_output_raw_report(sc->hdev, buf, sizeof(buf), HID_OUTPUT_REPORT);
>> + hid_hw_output_report(sc->hdev, buf, sizeof(buf));
>> }
>>
>> static void dualshock4_state_worker(struct work_struct *work)
>> @@ -1254,6 +1270,7 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
>> unsigned long quirks = id->driver_data;
>> struct sony_sc *sc;
>> unsigned int connect_mask = HID_CONNECT_DEFAULT;
>> + struct hid_ll_driver *ll_driver;
>>
>> sc = devm_kzalloc(&hdev->dev, sizeof(*sc), GFP_KERNEL);
>> if (sc == NULL) {
>> @@ -1285,13 +1302,20 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
>> }
>>
>> if (sc->quirks & SIXAXIS_CONTROLLER_USB) {
>> - hdev->hid_output_raw_report = sixaxis_usb_output_raw_report;
>> + ll_driver = devm_kzalloc(&hdev->dev, sizeof(*ll_driver),
>> + GFP_KERNEL);
>> + if (ll_driver == NULL)
>> + return -ENOMEM;
>> + sc->prev_ll_driver = hdev->ll_driver;
>> + *ll_driver = *hdev->ll_driver;
>> + hdev->ll_driver = ll_driver;
>> + ll_driver->output_report = sixaxis_usb_output_report;
>> + ll_driver->raw_request = sixaxis_usb_raw_request;
>> ret = sixaxis_set_operational_usb(hdev);
>> INIT_WORK(&sc->state_worker, sixaxis_state_worker);
>> - }
>> - else if (sc->quirks & SIXAXIS_CONTROLLER_BT)
>> + } else if (sc->quirks & SIXAXIS_CONTROLLER_BT) {
>> ret = sixaxis_set_operational_bt(hdev);
>> - else if (sc->quirks & DUALSHOCK4_CONTROLLER_USB) {
>> + } else if (sc->quirks & DUALSHOCK4_CONTROLLER_USB) {
>> /* Report 5 (31 bytes) is used to send data to the controller via USB */
>> ret = sony_set_output_report(sc, 0x05, 248);
>> if (ret < 0)
>> @@ -1339,6 +1363,8 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
>> err_close:
>> hid_hw_close(hdev);
>> err_stop:
>> + if (sc->prev_ll_driver)
>> + hdev->ll_driver = sc->prev_ll_driver;
>> if (sc->quirks & SONY_LED_SUPPORT)
>> sony_leds_remove(hdev);
>> if (sc->quirks & SONY_BATTERY_SUPPORT)
>> @@ -1351,6 +1377,9 @@ static void sony_remove(struct hid_device *hdev)
>> {
>> struct sony_sc *sc = hid_get_drvdata(hdev);
>>
>> + if (sc->prev_ll_driver)
>> + hdev->ll_driver = sc->prev_ll_driver;
>> +
>> if (sc->quirks & SONY_LED_SUPPORT)
>> sony_leds_remove(hdev);
>>
>> --
>> 1.8.3.1
>>
^ permalink raw reply
* Re: [PATCH 3/4] input: sirfsoc-onkey - use dev_get_drvdata instead of platform_get_drvdata
From: Dmitry Torokhov @ 2014-02-13 16:39 UTC (permalink / raw)
To: Barry Song
Cc: linux-input@vger.kernel.org, DL-SHA-WorkGroupLinux, Xianglong Du,
Barry Song
In-Reply-To: <CAGsJ_4ze3k2_ZQxkrRNiptqMVMm5taRTbFr0wmO9q5Q0kJLoNw@mail.gmail.com>
On Thu, Feb 13, 2014 at 03:47:33PM +0800, Barry Song wrote:
> 2014-02-13 15:24 GMT+08:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> > On Thu, Feb 13, 2014 at 02:40:23PM +0800, Barry Song wrote:
> >> From: Xianglong Du <Xianglong.Du@csr.com>
> >>
> >> In resume entry, use dev_get_drvdata() instead of to_platform_device(dev) +
> >> platform_get_drvdata(pdev).
> >>
> >> Signed-off-by: Xianglong Du <Xianglong.Du@csr.com>
> >> Signed-off-by: Barry Song <Baohua.Song@csr.com>
> >> ---
> >> drivers/input/misc/sirfsoc-onkey.c | 3 +--
> >> 1 files changed, 1 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/input/misc/sirfsoc-onkey.c b/drivers/input/misc/sirfsoc-onkey.c
> >> index 097c10a..8e45bf11 100644
> >> --- a/drivers/input/misc/sirfsoc-onkey.c
> >> +++ b/drivers/input/misc/sirfsoc-onkey.c
> >> @@ -157,8 +157,7 @@ static int sirfsoc_pwrc_remove(struct platform_device *pdev)
> >> #ifdef CONFIG_PM_SLEEP
> >> static int sirfsoc_pwrc_resume(struct device *dev)
> >> {
> >> - struct platform_device *pdev = to_platform_device(dev);
> >> - struct sirfsoc_pwrc_drvdata *pwrcdrv = platform_get_drvdata(pdev);
> >> + struct sirfsoc_pwrc_drvdata *pwrcdrv = dev_get_drvdata(dev);
> >
> > I believe we should use accessors matching the object type. Currently
> > dev_get_drvdata and platform_get_drvdata return the same object, but
> > they do not have to.
> >
> > IOW I prefer the original code.
>
> i did see many commits in kernel which did same jobs with this one. e.g:
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=a1216394e620d0dfbb03c712ae3210e7b77c9e11
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=bab8b563ef08455440badca7fe79b2c700bd1b75
> ...
>
> in my understand, the changed context is a general pm_ops to general
> "driver" and not a legacy suspend/resume in "platform_driver" bound
> with pdev, so in the context, we are caring about "device" more than
> "pdev".
It is more about who owns the data - generic device or platform device?
>
> how do you think if i do a more change in probe() with this by:
>
> - platform_set_drvdata(pdev, pwrcdrv);
> + dev_set_drvdata(&pdev->dev, pwrcdrv);
>
> would this make everything look fine?
Yes, it would, since we would now know that it is generic driver layer
that has ownership of this data item.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 01/15] Input: synaptics-rmi4 - fix checkpatch.pl, sparse and GCC warnings
From: Christopher Heiny @ 2014-02-13 18:56 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Courtney Cavin, linux-input
In-Reply-To: <20140213063611.GB15260@core.coreip.homeip.net>
On 02/12/2014 10:36 PM, Dmitry Torokhov wrote:
> On Wed, Feb 05, 2014 at 05:36:09PM -0800, Christopher Heiny wrote:
>> >On 02/05/2014 05:09 PM, Dmitry Torokhov wrote:
>>> > >On Tue, Feb 04, 2014 at 03:08:12PM -0800, Christopher Heiny wrote:
>>>>> > >>>On 01/23/2014 04:00 PM, Courtney Cavin wrote:
>>>>>>> > >>>> >Cc: Christopher Heiny<cheiny@synaptics.com>
>>>>>>> > >>>> >Cc: Dmitry Torokhov<dmitry.torokhov@gmail.com>
>>>>>>> > >>>> >Signed-off-by: Courtney Cavin<courtney.cavin@sonymobile.com>
>>>>>>> > >>>> >---
>>>>>>> > >>>> > drivers/input/rmi4/rmi_bus.c | 4 ++--
>>>>>>> > >>>> > drivers/input/rmi4/rmi_bus.h | 2 +-
>>>>>>> > >>>> > drivers/input/rmi4/rmi_driver.c | 17 ++++++++++++-----
>>>>>>> > >>>> > drivers/input/rmi4/rmi_f11.c | 4 +++-
>>>>>>> > >>>> > 4 files changed, 18 insertions(+), 9 deletions(-)
>>>>>>> > >>>> >
>>>>>>> > >>>> >diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c
>>>>>>> > >>>> >index 96a76e7..8a939f3 100644
>>>>>>> > >>>> >--- a/drivers/input/rmi4/rmi_bus.c
>>>>>>> > >>>> >+++ b/drivers/input/rmi4/rmi_bus.c
>>>>>>> > >>>> >@@ -37,7 +37,7 @@ static void rmi_release_device(struct device *dev)
>>>>>>> > >>>> > kfree(rmi_dev);
>>>>>>> > >>>> > }
>>>>>>> > >>>> >
>>>>>>> > >>>> >-struct device_type rmi_device_type = {
>>>>>>> > >>>> >+static struct device_type rmi_device_type = {
>>>>>>> > >>>> > .name = "rmi_sensor",
>>>>>>> > >>>> > .release = rmi_release_device,
>>>>>>> > >>>> > };
>>>>> > >>>
>>>>> > >>>This struct is used by diagnostic modules to identify sensor
>>>>> > >>>devices, so it cannot be static.
>>> > >
>>> > >Then we need to declare it somewhere or provide an accessor function.
>> >
>> >Currently it's in a header not included in the patches. We'll move
>> >it to rmi_bus.h.
> Hmm, we do have rmi_is_physical_device() to identify whether it is a
> sensor or a function, so I believe we should mark all structures static
> to avoid anyone poking at them.
I was poking around in the dependent code late last night and came to
the same conclusion. I'll send a micropatch later today to get it out
of the way.
--
Christopher Heiny
Senior Staff Firmware Engineer
Synaptics Incorporated
^ permalink raw reply
* Re: [PATCH 01/15] Input: synaptics-rmi4 - fix checkpatch.pl, sparse and GCC warnings
From: Dmitry Torokhov @ 2014-02-13 19:10 UTC (permalink / raw)
To: Christopher Heiny; +Cc: Courtney Cavin, linux-input
In-Reply-To: <52FD1559.2040101@synaptics.com>
On Thursday, February 13, 2014 10:56:25 AM Christopher Heiny wrote:
> On 02/12/2014 10:36 PM, Dmitry Torokhov wrote:
> > On Wed, Feb 05, 2014 at 05:36:09PM -0800, Christopher Heiny wrote:
> >> >On 02/05/2014 05:09 PM, Dmitry Torokhov wrote:
> >>> > >On Tue, Feb 04, 2014 at 03:08:12PM -0800, Christopher Heiny wrote:
> >>>>> > >>>On 01/23/2014 04:00 PM, Courtney Cavin wrote:
> >>>>>>> > >>>> >Cc: Christopher Heiny<cheiny@synaptics.com>
> >>>>>>> > >>>> >Cc: Dmitry Torokhov<dmitry.torokhov@gmail.com>
> >>>>>>> > >>>> >Signed-off-by: Courtney Cavin<courtney.cavin@sonymobile.com>
> >>>>>>> > >>>> >---
> >>>>>>> > >>>> >
> >>>>>>> > >>>> > drivers/input/rmi4/rmi_bus.c | 4 ++--
> >>>>>>> > >>>> > drivers/input/rmi4/rmi_bus.h | 2 +-
> >>>>>>> > >>>> > drivers/input/rmi4/rmi_driver.c | 17 ++++++++++++-----
> >>>>>>> > >>>> > drivers/input/rmi4/rmi_f11.c | 4 +++-
> >>>>>>> > >>>> > 4 files changed, 18 insertions(+), 9 deletions(-)
> >>>>>>> > >>>> >
> >>>>>>> > >>>> >diff --git a/drivers/input/rmi4/rmi_bus.c
> >>>>>>> > >>>> >b/drivers/input/rmi4/rmi_bus.c
> >>>>>>> > >>>> >index 96a76e7..8a939f3 100644
> >>>>>>> > >>>> >--- a/drivers/input/rmi4/rmi_bus.c
> >>>>>>> > >>>> >+++ b/drivers/input/rmi4/rmi_bus.c
> >>>>>>> > >>>> >@@ -37,7 +37,7 @@ static void rmi_release_device(struct
> >>>>>>> > >>>> >device *dev)
> >>>>>>> > >>>> >
> >>>>>>> > >>>> > kfree(rmi_dev);
> >>>>>>> > >>>> >
> >>>>>>> > >>>> > }
> >>>>>>> > >>>> >
> >>>>>>> > >>>> >-struct device_type rmi_device_type = {
> >>>>>>> > >>>> >+static struct device_type rmi_device_type = {
> >>>>>>> > >>>> >
> >>>>>>> > >>>> > .name = "rmi_sensor",
> >>>>>>> > >>>> > .release = rmi_release_device,
> >>>>>>> > >>>> >
> >>>>>>> > >>>> > };
> >>>>> > >>>
> >>>>> > >>>This struct is used by diagnostic modules to identify sensor
> >>>>> > >>>devices, so it cannot be static.
> >>> > >
> >>> > >Then we need to declare it somewhere or provide an accessor function.
> >> >
> >> >Currently it's in a header not included in the patches. We'll move
> >> >it to rmi_bus.h.
> >
> > Hmm, we do have rmi_is_physical_device() to identify whether it is a
> > sensor or a function, so I believe we should mark all structures static
> > to avoid anyone poking at them.
>
> I was poking around in the dependent code late last night and came to
> the same conclusion. I'll send a micropatch later today to get it out
> of the way.
No need, I untangled relevant bits from the one Courtney sent.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 01/11] Input: synaptics-rmi4 - do not kfree() managed memory in F01
From: Christopher Heiny @ 2014-02-13 19:11 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Andrew Duggan, Vincent Huang, Vivian Ly, Daniel Rosenberg,
Linus Walleij, Benjamin Tissoires, Courtney Cavin, Linux Input,
Linux Kernel
In-Reply-To: <1392269277-16391-1-git-send-email-dmitry.torokhov@gmail.com>
On 02/12/2014 09:27 PM, Dmitry Torokhov wrote:
> Data that is allocated with devm_kzalloc() should not be freed with
> kfree(). In fact, we should rely on the fact that memory is managed and let
> devres core free it for us.
>
> Reported-by: Courtney Cavin <courtney.cavin@sonymobile.com>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
> ---
> drivers/input/rmi4/rmi_f01.c | 23 +++++++++--------------
> 1 file changed, 9 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
> index 381ad60..92b90d1 100644
> --- a/drivers/input/rmi4/rmi_f01.c
> +++ b/drivers/input/rmi4/rmi_f01.c
> @@ -272,7 +272,7 @@ static int rmi_f01_initialize(struct rmi_function *fn)
> if (error < 0) {
> dev_err(&fn->dev,
> "Failed to read F01 control interrupt enable register.\n");
> - goto error_exit;
> + return error;
> }
>
> ctrl_base_addr += data->num_of_irq_regs;
> @@ -307,14 +307,14 @@ static int rmi_f01_initialize(struct rmi_function *fn)
> data->device_control.doze_interval);
> if (error < 0) {
> dev_err(&fn->dev, "Failed to configure F01 doze interval register.\n");
> - goto error_exit;
> + return error;
> }
> } else {
> error = rmi_read(rmi_dev, data->doze_interval_addr,
> &data->device_control.doze_interval);
> if (error < 0) {
> dev_err(&fn->dev, "Failed to read F01 doze interval register.\n");
> - goto error_exit;
> + return error;
> }
> }
>
> @@ -328,14 +328,14 @@ static int rmi_f01_initialize(struct rmi_function *fn)
> data->device_control.wakeup_threshold);
> if (error < 0) {
> dev_err(&fn->dev, "Failed to configure F01 wakeup threshold register.\n");
> - goto error_exit;
> + return error;
> }
> } else {
> error = rmi_read(rmi_dev, data->wakeup_threshold_addr,
> &data->device_control.wakeup_threshold);
> if (error < 0) {
> dev_err(&fn->dev, "Failed to read F01 wakeup threshold register.\n");
> - goto error_exit;
> + return error;
> }
> }
> }
> @@ -351,14 +351,14 @@ static int rmi_f01_initialize(struct rmi_function *fn)
> data->device_control.doze_holdoff);
> if (error < 0) {
> dev_err(&fn->dev, "Failed to configure F01 doze holdoff register.\n");
> - goto error_exit;
> + return error;
> }
> } else {
> error = rmi_read(rmi_dev, data->doze_holdoff_addr,
> &data->device_control.doze_holdoff);
> if (error < 0) {
> dev_err(&fn->dev, "Failed to read F01 doze holdoff register.\n");
> - goto error_exit;
> + return error;
> }
> }
> }
> @@ -367,22 +367,17 @@ static int rmi_f01_initialize(struct rmi_function *fn)
> &data->device_status, sizeof(data->device_status));
> if (error < 0) {
> dev_err(&fn->dev, "Failed to read device status.\n");
> - goto error_exit;
> + return error;
> }
>
> if (RMI_F01_STATUS_UNCONFIGURED(data->device_status)) {
> dev_err(&fn->dev,
> "Device was reset during configuration process, status: %#02x!\n",
> RMI_F01_STATUS_CODE(data->device_status));
> - error = -EINVAL;
> - goto error_exit;
> + return -EINVAL;
> }
>
> return 0;
> -
> - error_exit:
> - kfree(data);
> - return error;
> }
>
> static int rmi_f01_config(struct rmi_function *fn)
>
--
Christopher Heiny
Senior Staff Firmware Engineer
Synaptics Incorporated
^ permalink raw reply
* Re: [PATCH 02/11] Input: synaptics-rmi4 - remove unused rmi_f01_remove()
From: Christopher Heiny @ 2014-02-13 19:11 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Andrew Duggan, Vincent Huang, Vivian Ly, Daniel Rosenberg,
Linus Walleij, Benjamin Tissoires, Courtney Cavin, Linux Input,
Linux Kernel
In-Reply-To: <1392269277-16391-2-git-send-email-dmitry.torokhov@gmail.com>
On 02/12/2014 09:27 PM, Dmitry Torokhov wrote:
> It is an empty stub and is not needed.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
> ---
> drivers/input/rmi4/rmi_f01.c | 6 ------
> 1 file changed, 6 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
> index 92b90d1..897d8ac 100644
> --- a/drivers/input/rmi4/rmi_f01.c
> +++ b/drivers/input/rmi4/rmi_f01.c
> @@ -451,11 +451,6 @@ static int rmi_f01_probe(struct rmi_function *fn)
> return 0;
> }
>
> -static void rmi_f01_remove(struct rmi_function *fn)
> -{
> - /* Placeholder for now. */
> -}
> -
> #ifdef CONFIG_PM_SLEEP
> static int rmi_f01_suspend(struct device *dev)
> {
> @@ -554,7 +549,6 @@ static struct rmi_function_handler rmi_f01_handler = {
> },
> .func = 0x01,
> .probe = rmi_f01_probe,
> - .remove = rmi_f01_remove,
> .config = rmi_f01_config,
> .attention = rmi_f01_attention,
> };
>
--
Christopher Heiny
Senior Staff Firmware Engineer
Synaptics Incorporated
^ permalink raw reply
* Re: [PATCH 01/15] Input: synaptics-rmi4 - fix checkpatch.pl, sparse and GCC warnings
From: Dmitry Torokhov @ 2014-02-13 19:12 UTC (permalink / raw)
To: Christopher Heiny; +Cc: Courtney Cavin, linux-input
In-Reply-To: <2278620.abrl2TfnmQ@dtor-d630.eng.vmware.com>
On Thu, Feb 13, 2014 at 11:10:29AM -0800, Dmitry Torokhov wrote:
> On Thursday, February 13, 2014 10:56:25 AM Christopher Heiny wrote:
> > On 02/12/2014 10:36 PM, Dmitry Torokhov wrote:
> > > On Wed, Feb 05, 2014 at 05:36:09PM -0800, Christopher Heiny wrote:
> > >> >On 02/05/2014 05:09 PM, Dmitry Torokhov wrote:
> > >>> > >On Tue, Feb 04, 2014 at 03:08:12PM -0800, Christopher Heiny wrote:
> > >>>>> > >>>On 01/23/2014 04:00 PM, Courtney Cavin wrote:
> > >>>>>>> > >>>> >Cc: Christopher Heiny<cheiny@synaptics.com>
> > >>>>>>> > >>>> >Cc: Dmitry Torokhov<dmitry.torokhov@gmail.com>
> > >>>>>>> > >>>> >Signed-off-by: Courtney Cavin<courtney.cavin@sonymobile.com>
> > >>>>>>> > >>>> >---
> > >>>>>>> > >>>> >
> > >>>>>>> > >>>> > drivers/input/rmi4/rmi_bus.c | 4 ++--
> > >>>>>>> > >>>> > drivers/input/rmi4/rmi_bus.h | 2 +-
> > >>>>>>> > >>>> > drivers/input/rmi4/rmi_driver.c | 17 ++++++++++++-----
> > >>>>>>> > >>>> > drivers/input/rmi4/rmi_f11.c | 4 +++-
> > >>>>>>> > >>>> > 4 files changed, 18 insertions(+), 9 deletions(-)
> > >>>>>>> > >>>> >
> > >>>>>>> > >>>> >diff --git a/drivers/input/rmi4/rmi_bus.c
> > >>>>>>> > >>>> >b/drivers/input/rmi4/rmi_bus.c
> > >>>>>>> > >>>> >index 96a76e7..8a939f3 100644
> > >>>>>>> > >>>> >--- a/drivers/input/rmi4/rmi_bus.c
> > >>>>>>> > >>>> >+++ b/drivers/input/rmi4/rmi_bus.c
> > >>>>>>> > >>>> >@@ -37,7 +37,7 @@ static void rmi_release_device(struct
> > >>>>>>> > >>>> >device *dev)
> > >>>>>>> > >>>> >
> > >>>>>>> > >>>> > kfree(rmi_dev);
> > >>>>>>> > >>>> >
> > >>>>>>> > >>>> > }
> > >>>>>>> > >>>> >
> > >>>>>>> > >>>> >-struct device_type rmi_device_type = {
> > >>>>>>> > >>>> >+static struct device_type rmi_device_type = {
> > >>>>>>> > >>>> >
> > >>>>>>> > >>>> > .name = "rmi_sensor",
> > >>>>>>> > >>>> > .release = rmi_release_device,
> > >>>>>>> > >>>> >
> > >>>>>>> > >>>> > };
> > >>>>> > >>>
> > >>>>> > >>>This struct is used by diagnostic modules to identify sensor
> > >>>>> > >>>devices, so it cannot be static.
> > >>> > >
> > >>> > >Then we need to declare it somewhere or provide an accessor function.
> > >> >
> > >> >Currently it's in a header not included in the patches. We'll move
> > >> >it to rmi_bus.h.
> > >
> > > Hmm, we do have rmi_is_physical_device() to identify whether it is a
> > > sensor or a function, so I believe we should mark all structures static
> > > to avoid anyone poking at them.
> >
> > I was poking around in the dependent code late last night and came to
> > the same conclusion. I'll send a micropatch later today to get it out
> > of the way.
>
> No need, I untangled relevant bits from the one Courtney sent.
>
> Thanks.
Here it is by the way.
Input: synaptics-rmi4 - make device types module-private
From: Courtney Cavin <courtney.cavin@sonymobile.com>
Nobody outside of RMI bus core should be accessing rmi_device_type,
rmi_function_type or rmi_physical_driver structures so let's limit their
scope.
We provide rmi_is_physical_device() and rmi_is_function_device() to allow
determining whether one is dealing with sensor or function device.
Signed-off-by: Courtney Cavin <courtney.cavin@sonymobile.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/rmi4/rmi_bus.c | 4 ++--
drivers/input/rmi4/rmi_driver.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c
index 7efe7ed..6e0454a 100644
--- a/drivers/input/rmi4/rmi_bus.c
+++ b/drivers/input/rmi4/rmi_bus.c
@@ -38,7 +38,7 @@ static void rmi_release_device(struct device *dev)
kfree(rmi_dev);
}
-struct device_type rmi_device_type = {
+static struct device_type rmi_device_type = {
.name = "rmi_sensor",
.release = rmi_release_device,
};
@@ -154,7 +154,7 @@ static void rmi_release_function(struct device *dev)
kfree(fn);
}
-struct device_type rmi_function_type = {
+static struct device_type rmi_function_type = {
.name = "rmi_function",
.release = rmi_release_function,
};
diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index 788343a..4406a7f 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c
@@ -946,7 +946,7 @@ err_free_mem:
return retval < 0 ? retval : 0;
}
-struct rmi_driver rmi_physical_driver = {
+static struct rmi_driver rmi_physical_driver = {
.driver = {
.owner = THIS_MODULE,
.name = "rmi_physical",
--
Dmitry
^ permalink raw reply related
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