* Re: [PATCH] input synaptics-rmi4: rmi_f01.c storage fix
From: Christopher Heiny @ 2014-02-12 3:03 UTC (permalink / raw)
To: Courtney Cavin
Cc: Dmitry Torokhov, Linux Input, Andrew Duggan, Vincent Huang,
Vivian Ly, Daniel Rosenberg, Jean Delvare, Joerie de Gram,
Linus Walleij, Benjamin Tissoires, David Herrmann, Jiri Kosina
In-Reply-To: <20140212012606.GY1706@sonymobile.com>
On 02/11/2014 05:26 PM, Courtney Cavin wrote:
> On Wed, Feb 12, 2014 at 12:13:00AM +0100, Christopher Heiny wrote:
>> Correctly free driver related data when initialization fails.
>>
>> Trivial: Clarify a diagnostic message.
>>
>> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> Cc: Linux Walleij <linus.walleij@linaro.org>
>> Cc: David Herrmann <dh.herrmann@gmail.com>
>> Cc: Jiri Kosina <jkosina@suse.cz>
>>
>> ---
>>
>> drivers/input/rmi4/rmi_f01.c | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
>> index 381ad60..e4a6df9 100644
>> --- a/drivers/input/rmi4/rmi_f01.c
>> +++ b/drivers/input/rmi4/rmi_f01.c
>> @@ -149,7 +149,7 @@ static int rmi_f01_alloc_memory(struct rmi_function *fn,
>>
>> f01 = devm_kzalloc(&fn->dev, sizeof(struct f01_data), GFP_KERNEL);
>> if (!f01) {
>> - dev_err(&fn->dev, "Failed to allocate fn_01_data.\n");
>> + dev_err(&fn->dev, "Failed to allocate f01_data.\n");
>
> Just remove this printout, as it won't help any user in the case of OOM.
> Additionally, there's already plenty of (more useful) information
> printed out if kmalloc fails.
Good point. There's similar messages in a number of places, so we'll do
a single patch to clean them all up at once.
>
>> return -ENOMEM;
>> }
>>
>> @@ -158,6 +158,7 @@ static int rmi_f01_alloc_memory(struct rmi_function *fn,
>> GFP_KERNEL);
>> if (!f01->device_control.interrupt_enable) {
>> dev_err(&fn->dev, "Failed to allocate interrupt enable.\n");
>> + devm_kfree(&fn->dev, f01);
>
> Unnecessary, devres_release_all() will release this, from:
> - really_probe() -> rmi_function_probe() -> rmi_f01_probe()
> - driver_probe_device()
> - __driver_attach()
> - driver_attach()
> - bus_add_driver()
> - driver_register()
> - __rmi_register_function_handler()
As mentioned before, we've received a lot of conflicting advice on
devm_k*. Thanks very much for the clarification - it makes more sense now.
>> return -ENOMEM;
>> }
>> fn->data = f01;
>> @@ -381,7 +382,8 @@ static int rmi_f01_initialize(struct rmi_function *fn)
>> return 0;
>>
>> error_exit:
>> - kfree(data);
>> + devm_kfree(&fn->dev, data->device_control.interrupt_enable);
>> + devm_kfree(&fn->dev, data);
>
> Same as above for these two.
>
>> return error;
>> }
>>
>
> Generally devm_ release functions are unnecessary to call, as all
> resources will get released on driver detach, whether abnormal or not.
>
> -Courtney
>
--
Christopher Heiny
Senior Staff Firmware Engineer
Synaptics Incorporated
^ permalink raw reply
* Re: [PATCH] input synaptics-rmi4: Use put_device() and device_type.release() to free storage.
From: Christopher Heiny @ 2014-02-12 3:17 UTC (permalink / raw)
To: Courtney Cavin
Cc: Dmitry Torokhov, Linux Input, Andrew Duggan, Vincent Huang,
Vivian Ly, Daniel Rosenberg, Jean Delvare, Joerie de Gram,
Linus Walleij, Benjamin Tissoires, David Herrmann, Jiri Kosina
In-Reply-To: <20140212024940.GA1706@sonymobile.com>
On 02/11/2014 06:49 PM, Courtney Cavin wrote:
> On Wed, Feb 12, 2014 at 03:17:57AM +0100, Christopher Heiny wrote:
>> On 02/11/2014 05:59 PM, Courtney Cavin wrote:
>>> On Wed, Feb 12, 2014 at 12:13:30AM +0100, Christopher Heiny wrote:
>>>> For rmi_sensor and rmi_function device_types, use put_device() and
>>>> the assocated device_type.release() function to clean up related
>>>> structures and storage in the correct and safe order.
>>>>
>>>> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
>>>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>>> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>>>> Cc: Linux Walleij <linus.walleij@linaro.org>
>>>> Cc: David Herrmann <dh.herrmann@gmail.com>
>>>> Cc: Jiri Kosina <jkosina@suse.cz>
>>>> Cc: Courtney Cavin <courtney.cavin@sonymobile.com>
>>>
>>> I'm not a huge fan of you taking my patches, re-formatting them and
>>> sending them as your own. More out of principle then actually caring
>>> about ownership. You at least cc'd me on this one....
>>
>> Sorry - no slight was intended at all! I wasn't sure what the protocol
>> was for picking up an idea from someone else's patch and building on
>> that idea, so I just went with the CC. I definitely prefer to attribute
>> sources correctly - if you could clarify what should be done (beyond the
>> CC) to acknowledge the author of the original patch, I'd appreciate it.
>
> Sure. In short, follow Documentation/SubmittingPatches , esp. section
> 12) Sign your work.
>
> Generally the patch should read something like the following:
>
> From: Original Author <original.author@example.org>
>
> *BLURB*
>
> Signed-off-by: Original Author <original.author@example.org>
> [additional.author@example.org: changed x and y]
> Signed-off-by: Additional Author <additional.author@example.org>
>
> Assuming the original author actually signed-off the patch in the first
> place, of course. The square bracket part is optional, but can be
> helpful for reviewers.
>
> I'm somewhat surprised that you are not aware of this procedure, as this
> is how Dmitry has replied to some of your patches in the past.'
Thanks very much.
I was actually aware of that, but thought the work was sufficiently
different from your original patch that applying your Signed-off-by: to
it wouldn't be appropriate (I dislike being signed off on things I don't
necessarily agree with as much as lack of attribution). I'll be less
paranoid about that in the future.
^ permalink raw reply
* Re: [PATCH] input synaptics-rmi4: Use put_device() and device_type.release() to free storage.
From: Courtney Cavin @ 2014-02-12 4:54 UTC (permalink / raw)
To: Christopher Heiny
Cc: Dmitry Torokhov, Linux Input, Andrew Duggan, Vincent Huang,
Vivian Ly, Daniel Rosenberg, Jean Delvare, Joerie de Gram,
Linus Walleij, Benjamin Tissoires, David Herrmann, Jiri Kosina
In-Reply-To: <52FAE7E7.4090905@synaptics.com>
On Wed, Feb 12, 2014 at 04:17:59AM +0100, Christopher Heiny wrote:
> On 02/11/2014 06:49 PM, Courtney Cavin wrote:
> > On Wed, Feb 12, 2014 at 03:17:57AM +0100, Christopher Heiny wrote:
> >> On 02/11/2014 05:59 PM, Courtney Cavin wrote:
> >>> On Wed, Feb 12, 2014 at 12:13:30AM +0100, Christopher Heiny wrote:
> >>>> For rmi_sensor and rmi_function device_types, use put_device() and
> >>>> the assocated device_type.release() function to clean up related
> >>>> structures and storage in the correct and safe order.
> >>>>
> >>>> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
> >>>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> >>>> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> >>>> Cc: Linux Walleij <linus.walleij@linaro.org>
> >>>> Cc: David Herrmann <dh.herrmann@gmail.com>
> >>>> Cc: Jiri Kosina <jkosina@suse.cz>
> >>>> Cc: Courtney Cavin <courtney.cavin@sonymobile.com>
> >>>
> >>> I'm not a huge fan of you taking my patches, re-formatting them and
> >>> sending them as your own. More out of principle then actually caring
> >>> about ownership. You at least cc'd me on this one....
> >>
> >> Sorry - no slight was intended at all! I wasn't sure what the protocol
> >> was for picking up an idea from someone else's patch and building on
> >> that idea, so I just went with the CC. I definitely prefer to attribute
> >> sources correctly - if you could clarify what should be done (beyond the
> >> CC) to acknowledge the author of the original patch, I'd appreciate it.
> >
> > Sure. In short, follow Documentation/SubmittingPatches , esp. section
> > 12) Sign your work.
> >
> > Generally the patch should read something like the following:
> >
> > From: Original Author <original.author@example.org>
> >
> > *BLURB*
> >
> > Signed-off-by: Original Author <original.author@example.org>
> > [additional.author@example.org: changed x and y]
> > Signed-off-by: Additional Author <additional.author@example.org>
> >
> > Assuming the original author actually signed-off the patch in the first
> > place, of course. The square bracket part is optional, but can be
> > helpful for reviewers.
> >
> > I'm somewhat surprised that you are not aware of this procedure, as this
> > is how Dmitry has replied to some of your patches in the past.'
>
> Thanks very much.
>
> I was actually aware of that, but thought the work was sufficiently
> different from your original patch that applying your Signed-off-by: to
> it wouldn't be appropriate (I dislike being signed off on things I don't
> necessarily agree with as much as lack of attribution). I'll be less
> paranoid about that in the future.
I don't see how they were different enough, when clearly these two
patches attempt to fix the same bugs, using the same methods with
slightly modified flow. Perhaps the patches may be small enough
to be interpreted either way, but at the very least reported-by (since
this is a bug-fix) or suggested-by is more appropriate than Cc. This is
a public list, so I'm sure someone will tell you when you are wrong, if
nothing else.
Along the same topic, I guess I should also mention that it is typically
frowned upon to takeover someone else's patches without giving them
due-time to fix any outstanding review comments.
In both of these cases, you neither asked for me to submit the patches
separately, outside of my DT-series, nor to make any specific changes.
I was under the impression that you were still participating in the
discussion for that series.
While it is apparent that we have differing views on how this particular
driver development should proceed, and we should definitely discuss
them, please do not think that I'm not willing to apply my patches
individually to what's in tree now.
My main concern here is that I cannot actually properly test this driver
without DT, non-gpio irq, and regulator support. Likewise, pre-3.7 is
ancient, and would require back-porting hundreds of changes.
-Courtney
^ permalink raw reply
* Re: [PATCH] input synaptics-rmi4: rmi_f01.c storage fix
From: Dmitry Torokhov @ 2014-02-12 6:40 UTC (permalink / raw)
To: Christopher Heiny
Cc: Linux Input, Andrew Duggan, Vincent Huang, Vivian Ly,
Daniel Rosenberg, Jean Delvare, Joerie de Gram, Linus Walleij,
Benjamin Tissoires, David Herrmann, Jiri Kosina, Courtney Cavin
In-Reply-To: <1392160380-8221-1-git-send-email-cheiny@synaptics.com>
Hi Chris,
On Tue, Feb 11, 2014 at 03:13:00PM -0800, Christopher Heiny wrote:
> Correctly free driver related data when initialization fails.
>
> Trivial: Clarify a diagnostic message.
>
> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Cc: Linux Walleij <linus.walleij@linaro.org>
> Cc: David Herrmann <dh.herrmann@gmail.com>
> Cc: Jiri Kosina <jkosina@suse.cz>
>
> ---
>
> drivers/input/rmi4/rmi_f01.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
> index 381ad60..e4a6df9 100644
> --- a/drivers/input/rmi4/rmi_f01.c
> +++ b/drivers/input/rmi4/rmi_f01.c
> @@ -149,7 +149,7 @@ static int rmi_f01_alloc_memory(struct rmi_function *fn,
>
> f01 = devm_kzalloc(&fn->dev, sizeof(struct f01_data), GFP_KERNEL);
> if (!f01) {
> - dev_err(&fn->dev, "Failed to allocate fn_01_data.\n");
> + dev_err(&fn->dev, "Failed to allocate f01_data.\n");
> return -ENOMEM;
> }
>
> @@ -158,6 +158,7 @@ static int rmi_f01_alloc_memory(struct rmi_function *fn,
> GFP_KERNEL);
> if (!f01->device_control.interrupt_enable) {
> dev_err(&fn->dev, "Failed to allocate interrupt enable.\n");
> + devm_kfree(&fn->dev, f01);
As Courtney mentioned if you are calling devm_kfree() you are most
likely doing something wrong.
How about the patch below? Please check the XXX comment, I have some
concerns about lts vs doze_holdoff check mismatch in probe() and
config().
Thanks.
--
Dmitry
Input: synaptics-rmi4 - F01 initialization cleanup
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
- rename data to f01 where appropriate;
- switch to using rmi_read()/rmi_write() for single-byte data;
- allocate interrupt mask together with the main structure;
- do not kfree() memory allocated with devm;
- do not write config data in probe(), we have config() for that;
- drop unneeded rmi_f01_remove().
Reported-by: Courtney Cavin <courtney.cavin@sonymobile.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/rmi4/rmi_f01.c | 397 ++++++++++++++++++------------------------
1 file changed, 172 insertions(+), 225 deletions(-)
diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
index 381ad60..8f7840e 100644
--- a/drivers/input/rmi4/rmi_f01.c
+++ b/drivers/input/rmi4/rmi_f01.c
@@ -123,47 +123,21 @@ struct f01_device_control {
struct f01_data {
struct f01_basic_properties properties;
-
struct f01_device_control device_control;
- struct mutex control_mutex;
-
- u8 device_status;
u16 interrupt_enable_addr;
u16 doze_interval_addr;
u16 wakeup_threshold_addr;
u16 doze_holdoff_addr;
- int irq_count;
- int num_of_irq_regs;
#ifdef CONFIG_PM_SLEEP
bool suspended;
bool old_nosleep;
#endif
-};
-
-static int rmi_f01_alloc_memory(struct rmi_function *fn,
- int num_of_irq_regs)
-{
- struct f01_data *f01;
- f01 = devm_kzalloc(&fn->dev, sizeof(struct f01_data), GFP_KERNEL);
- if (!f01) {
- dev_err(&fn->dev, "Failed to allocate fn_01_data.\n");
- return -ENOMEM;
- }
-
- f01->device_control.interrupt_enable = devm_kzalloc(&fn->dev,
- sizeof(u8) * (num_of_irq_regs),
- GFP_KERNEL);
- if (!f01->device_control.interrupt_enable) {
- dev_err(&fn->dev, "Failed to allocate interrupt enable.\n");
- return -ENOMEM;
- }
- fn->data = f01;
-
- return 0;
-}
+ unsigned int num_of_irq_regs;
+ u8 interrupt_enable[];
+};
static int rmi_f01_read_properties(struct rmi_device *rmi_dev,
u16 query_base_addr,
@@ -174,8 +148,9 @@ static int rmi_f01_read_properties(struct rmi_device *rmi_dev,
error = rmi_read_block(rmi_dev, query_base_addr,
basic_query, sizeof(basic_query));
- if (error < 0) {
- dev_err(&rmi_dev->dev, "Failed to read device query registers.\n");
+ if (error) {
+ dev_err(&rmi_dev->dev,
+ "Failed to read device query registers: %d\n", error);
return error;
}
@@ -204,38 +179,51 @@ static int rmi_f01_read_properties(struct rmi_device *rmi_dev,
return 0;
}
-static int rmi_f01_initialize(struct rmi_function *fn)
+static int rmi_f01_probe(struct rmi_function *fn)
{
- u8 temp;
- int error;
- u16 ctrl_base_addr;
struct rmi_device *rmi_dev = fn->rmi_dev;
struct rmi_driver_data *driver_data = dev_get_drvdata(&rmi_dev->dev);
- struct f01_data *data = fn->data;
- struct rmi_device_platform_data *pdata = to_rmi_platform_data(rmi_dev);
+ const struct rmi_device_platform_data *pdata =
+ to_rmi_platform_data(rmi_dev);
+ struct f01_data *f01;
+ size_t f01_size;
+ int error;
+ u16 ctrl_base_addr;
+ u8 device_status;
+ u8 temp;
+
+ f01_size = sizeof(struct f01_data) +
+ sizeof(u8) * driver_data->num_of_irq_regs;
+ f01 = devm_kzalloc(&fn->dev, f01_size, GFP_KERNEL);
+ if (!f01) {
+ dev_err(&fn->dev, "Failed to allocate fn01_data.\n");
+ return -ENOMEM;
+ }
- mutex_init(&data->control_mutex);
+ f01->num_of_irq_regs = driver_data->num_of_irq_regs;
+ f01->device_control.interrupt_enable = f01->interrupt_enable;
/*
* Set the configured bit and (optionally) other important stuff
* in the device control register.
*/
ctrl_base_addr = fn->fd.control_base_addr;
- error = rmi_read_block(rmi_dev, fn->fd.control_base_addr,
- &data->device_control.ctrl0,
- sizeof(data->device_control.ctrl0));
- if (error < 0) {
- dev_err(&fn->dev, "Failed to read F01 control.\n");
+
+ error = rmi_read(rmi_dev, fn->fd.control_base_addr,
+ &f01->device_control.ctrl0);
+ if (error) {
+ dev_err(&fn->dev, "Failed to read F01 control: %d\n", error);
return error;
}
+
switch (pdata->power_management.nosleep) {
case RMI_F01_NOSLEEP_DEFAULT:
break;
case RMI_F01_NOSLEEP_OFF:
- data->device_control.ctrl0 &= ~RMI_F01_CRTL0_NOSLEEP_BIT;
+ f01->device_control.ctrl0 &= ~RMI_F01_CRTL0_NOSLEEP_BIT;
break;
case RMI_F01_NOSLEEP_ON:
- data->device_control.ctrl0 |= RMI_F01_CRTL0_NOSLEEP_BIT;
+ f01->device_control.ctrl0 |= RMI_F01_CRTL0_NOSLEEP_BIT;
break;
}
@@ -244,250 +232,213 @@ static int rmi_f01_initialize(struct rmi_function *fn)
* reboot without power cycle. If so, clear it so the sensor
* is certain to function.
*/
- if ((data->device_control.ctrl0 & RMI_F01_CTRL0_SLEEP_MODE_MASK) !=
+ if ((f01->device_control.ctrl0 & RMI_F01_CTRL0_SLEEP_MODE_MASK) !=
RMI_SLEEP_MODE_NORMAL) {
dev_warn(&fn->dev,
"WARNING: Non-zero sleep mode found. Clearing...\n");
- data->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
+ f01->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
}
- data->device_control.ctrl0 |= RMI_F01_CRTL0_CONFIGURED_BIT;
+ f01->device_control.ctrl0 |= RMI_F01_CRTL0_CONFIGURED_BIT;
- error = rmi_write_block(rmi_dev, fn->fd.control_base_addr,
- &data->device_control.ctrl0,
- sizeof(data->device_control.ctrl0));
- if (error < 0) {
- dev_err(&fn->dev, "Failed to write F01 control.\n");
+ error = rmi_write(rmi_dev, fn->fd.control_base_addr,
+ f01->device_control.ctrl0);
+ if (error) {
+ dev_err(&fn->dev, "Failed to write F01 control: %d\n", error);
return error;
}
- data->irq_count = driver_data->irq_count;
- data->num_of_irq_regs = driver_data->num_of_irq_regs;
- ctrl_base_addr += sizeof(u8);
+ f01->num_of_irq_regs = driver_data->num_of_irq_regs;
+ ctrl_base_addr += sizeof(f01->device_control.ctrl0);
- data->interrupt_enable_addr = ctrl_base_addr;
+ f01->interrupt_enable_addr = ctrl_base_addr;
error = rmi_read_block(rmi_dev, ctrl_base_addr,
- data->device_control.interrupt_enable,
- sizeof(u8) * (data->num_of_irq_regs));
- if (error < 0) {
+ f01->device_control.interrupt_enable,
+ sizeof(u8) * (f01->num_of_irq_regs));
+ if (error) {
dev_err(&fn->dev,
- "Failed to read F01 control interrupt enable register.\n");
- goto error_exit;
+ "Failed to read F01 control interrupt enable register: %d\n",
+ error);
+ return error;
}
- ctrl_base_addr += data->num_of_irq_regs;
+ ctrl_base_addr += f01->num_of_irq_regs;
/* dummy read in order to clear irqs */
error = rmi_read(rmi_dev, fn->fd.data_base_addr + 1, &temp);
- if (error < 0) {
+ if (error) {
dev_err(&fn->dev, "Failed to read Interrupt Status.\n");
return error;
}
error = rmi_f01_read_properties(rmi_dev, fn->fd.query_base_addr,
- &data->properties);
- if (error < 0) {
+ &f01->properties);
+ if (error) {
dev_err(&fn->dev, "Failed to read F01 properties.\n");
return error;
}
+
dev_info(&fn->dev, "found RMI device, manufacturer: %s, product: %s\n",
- data->properties.manufacturer_id == 1 ?
- "Synaptics" : "unknown",
- data->properties.product_id);
+ f01->properties.manufacturer_id == 1 ? "Synaptics" : "unknown",
+ f01->properties.product_id);
/* read control register */
- if (data->properties.has_adjustable_doze) {
- data->doze_interval_addr = ctrl_base_addr;
+ if (f01->properties.has_adjustable_doze) {
+ f01->doze_interval_addr = ctrl_base_addr;
ctrl_base_addr++;
if (pdata->power_management.doze_interval) {
- data->device_control.doze_interval =
+ f01->device_control.doze_interval =
pdata->power_management.doze_interval;
- error = rmi_write(rmi_dev, data->doze_interval_addr,
- data->device_control.doze_interval);
- if (error < 0) {
- dev_err(&fn->dev, "Failed to configure F01 doze interval register.\n");
- goto error_exit;
- }
} 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;
+ error = rmi_read(rmi_dev, f01->doze_interval_addr,
+ &f01->device_control.doze_interval);
+ if (error) {
+ dev_err(&fn->dev,
+ "Failed to read F01 doze interval register: %d\n",
+ error);
+ return error;
}
}
- data->wakeup_threshold_addr = ctrl_base_addr;
+ f01->wakeup_threshold_addr = ctrl_base_addr;
ctrl_base_addr++;
if (pdata->power_management.wakeup_threshold) {
- data->device_control.wakeup_threshold =
+ f01->device_control.wakeup_threshold =
pdata->power_management.wakeup_threshold;
- error = rmi_write(rmi_dev, data->wakeup_threshold_addr,
- data->device_control.wakeup_threshold);
- if (error < 0) {
- dev_err(&fn->dev, "Failed to configure F01 wakeup threshold register.\n");
- goto error_exit;
- }
} 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;
+ error = rmi_read(rmi_dev, f01->wakeup_threshold_addr,
+ &f01->device_control.wakeup_threshold);
+ if (error) {
+ dev_err(&fn->dev,
+ "Failed to read F01 wakeup threshold register: %d\n",
+ error);
+ return error;
}
}
}
- if (data->properties.has_adjustable_doze_holdoff) {
- data->doze_holdoff_addr = ctrl_base_addr;
+ if (f01->properties.has_adjustable_doze_holdoff) {
+ f01->doze_holdoff_addr = ctrl_base_addr;
ctrl_base_addr++;
if (pdata->power_management.doze_holdoff) {
- data->device_control.doze_holdoff =
+ f01->device_control.doze_holdoff =
pdata->power_management.doze_holdoff;
- error = rmi_write(rmi_dev, data->doze_holdoff_addr,
- data->device_control.doze_holdoff);
- if (error < 0) {
- dev_err(&fn->dev, "Failed to configure F01 doze holdoff register.\n");
- goto error_exit;
- }
} 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;
+ error = rmi_read(rmi_dev, f01->doze_holdoff_addr,
+ &f01->device_control.doze_holdoff);
+ if (error) {
+ dev_err(&fn->dev,
+ "Failed to read F01 doze holdoff register: %d\n",
+ error);
+ return error;
}
}
}
- error = rmi_read_block(rmi_dev, fn->fd.data_base_addr,
- &data->device_status, sizeof(data->device_status));
- if (error < 0) {
- dev_err(&fn->dev, "Failed to read device status.\n");
- goto error_exit;
+ error = rmi_read(rmi_dev, fn->fd.data_base_addr, &device_status);
+ if (error) {
+ dev_err(&fn->dev,
+ "Failed to read device status: %d\n", error);
+ return error;
}
- if (RMI_F01_STATUS_UNCONFIGURED(data->device_status)) {
+ if (RMI_F01_STATUS_UNCONFIGURED(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;
+ RMI_F01_STATUS_CODE(device_status));
+ return -EINVAL;
}
- return 0;
+ fn->data = f01;
- error_exit:
- kfree(data);
- return error;
+ return 0;
}
static int rmi_f01_config(struct rmi_function *fn)
{
- struct f01_data *data = fn->data;
- int retval;
-
- retval = rmi_write_block(fn->rmi_dev, fn->fd.control_base_addr,
- &data->device_control.ctrl0,
- sizeof(data->device_control.ctrl0));
- if (retval < 0) {
- dev_err(&fn->dev, "Failed to write device_control.reg.\n");
- return retval;
- }
+ struct f01_data *f01 = fn->data;
+ int error;
- retval = rmi_write_block(fn->rmi_dev, data->interrupt_enable_addr,
- data->device_control.interrupt_enable,
- sizeof(u8) * data->num_of_irq_regs);
+ error = rmi_write(fn->rmi_dev, fn->fd.control_base_addr,
+ f01->device_control.ctrl0);
+ if (error) {
+ dev_err(&fn->dev,
+ "Failed to write device_control reg: %d\n", error);
+ return error;
+ }
- if (retval < 0) {
- dev_err(&fn->dev, "Failed to write interrupt enable.\n");
- return retval;
+ error = rmi_write_block(fn->rmi_dev, f01->interrupt_enable_addr,
+ f01->interrupt_enable,
+ sizeof(u8) * f01->num_of_irq_regs);
+ if (error) {
+ dev_err(&fn->dev,
+ "Failed to write interrupt enable: %d\n", error);
+ return error;
}
- if (data->properties.has_lts) {
- retval = rmi_write_block(fn->rmi_dev, data->doze_interval_addr,
- &data->device_control.doze_interval,
- sizeof(u8));
- if (retval < 0) {
- dev_err(&fn->dev, "Failed to write doze interval.\n");
- return retval;
+
+ /* XXX: why we check has_lts here but has_adjustable_doze in probe? */
+ if (f01->properties.has_lts) {
+ error = rmi_write(fn->rmi_dev, f01->doze_interval_addr,
+ f01->device_control.doze_interval);
+ if (error) {
+ dev_err(&fn->dev,
+ "Failed to write doze interval: %d\n", error);
+ return error;
}
}
- if (data->properties.has_adjustable_doze) {
- retval = rmi_write_block(fn->rmi_dev,
- data->wakeup_threshold_addr,
- &data->device_control.wakeup_threshold,
- sizeof(u8));
- if (retval < 0) {
- dev_err(&fn->dev, "Failed to write wakeup threshold.\n");
- return retval;
+ if (f01->properties.has_adjustable_doze) {
+ error = rmi_write(fn->rmi_dev, f01->wakeup_threshold_addr,
+ f01->device_control.wakeup_threshold);
+ if (error) {
+ dev_err(&fn->dev,
+ "Failed to write wakeup threshold: %d\n",
+ error);
+ return error;
}
}
- if (data->properties.has_adjustable_doze_holdoff) {
- retval = rmi_write_block(fn->rmi_dev, data->doze_holdoff_addr,
- &data->device_control.doze_holdoff,
- sizeof(u8));
- if (retval < 0) {
- dev_err(&fn->dev, "Failed to write doze holdoff.\n");
- return retval;
+ if (f01->properties.has_adjustable_doze_holdoff) {
+ error = rmi_write(fn->rmi_dev, f01->doze_holdoff_addr,
+ f01->device_control.doze_holdoff);
+ if (error) {
+ dev_err(&fn->dev,
+ "Failed to write doze holdoff: %d\n", error);
+ return error;
}
}
- return 0;
-}
-
-static int rmi_f01_probe(struct rmi_function *fn)
-{
- struct rmi_driver_data *driver_data =
- dev_get_drvdata(&fn->rmi_dev->dev);
- int error;
-
- error = rmi_f01_alloc_memory(fn, driver_data->num_of_irq_regs);
- if (error)
- return error;
-
- error = rmi_f01_initialize(fn);
- if (error)
- return error;
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)
{
struct rmi_function *fn = to_rmi_function(dev);
struct rmi_device *rmi_dev = fn->rmi_dev;
- struct f01_data *data = fn->data;
+ struct f01_data *f01 = fn->data;
int error;
- data->old_nosleep = data->device_control.ctrl0 &
- RMI_F01_CRTL0_NOSLEEP_BIT;
- data->device_control.ctrl0 &= ~RMI_F01_CRTL0_NOSLEEP_BIT;
+ f01->old_nosleep =
+ f01->device_control.ctrl0 & RMI_F01_CRTL0_NOSLEEP_BIT;
- data->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
- data->device_control.ctrl0 |= RMI_SLEEP_MODE_SENSOR_SLEEP;
+ f01->device_control.ctrl0 &= ~RMI_F01_CRTL0_NOSLEEP_BIT;
- error = rmi_write_block(rmi_dev,
- fn->fd.control_base_addr,
- &data->device_control.ctrl0,
- sizeof(data->device_control.ctrl0));
- if (error < 0) {
- dev_err(&fn->dev, "Failed to write sleep mode. Code: %d.\n",
- error);
- if (data->old_nosleep)
- data->device_control.ctrl0 |=
- RMI_F01_CRTL0_NOSLEEP_BIT;
- data->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
- data->device_control.ctrl0 |= RMI_SLEEP_MODE_NORMAL;
+ f01->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
+ f01->device_control.ctrl0 |= RMI_SLEEP_MODE_SENSOR_SLEEP;
+
+ error = rmi_write(rmi_dev, fn->fd.control_base_addr,
+ f01->device_control.ctrl0);
+ if (error) {
+ dev_err(&fn->dev,
+ "Failed to write sleep mode: %d.\n", error);
+ if (f01->old_nosleep)
+ f01->device_control.ctrl0 |= RMI_F01_CRTL0_NOSLEEP_BIT;
+ f01->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
+ f01->device_control.ctrl0 |= RMI_SLEEP_MODE_NORMAL;
return error;
}
@@ -498,22 +449,20 @@ static int rmi_f01_resume(struct device *dev)
{
struct rmi_function *fn = to_rmi_function(dev);
struct rmi_device *rmi_dev = fn->rmi_dev;
- struct f01_data *data = fn->data;
+ struct f01_data *f01 = fn->data;
int error;
- if (data->old_nosleep)
- data->device_control.ctrl0 |= RMI_F01_CRTL0_NOSLEEP_BIT;
+ if (f01->old_nosleep)
+ f01->device_control.ctrl0 |= RMI_F01_CRTL0_NOSLEEP_BIT;
- data->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
- data->device_control.ctrl0 |= RMI_SLEEP_MODE_NORMAL;
+ f01->device_control.ctrl0 &= ~RMI_F01_CTRL0_SLEEP_MODE_MASK;
+ f01->device_control.ctrl0 |= RMI_SLEEP_MODE_NORMAL;
- error = rmi_write_block(rmi_dev, fn->fd.control_base_addr,
- &data->device_control.ctrl0,
- sizeof(data->device_control.ctrl0));
- if (error < 0) {
+ error = rmi_write(rmi_dev, fn->fd.control_base_addr,
+ f01->device_control.ctrl0);
+ if (error) {
dev_err(&fn->dev,
- "Failed to restore normal operation. Code: %d.\n",
- error);
+ "Failed to restore normal operation: %d.\n", error);
return error;
}
@@ -527,22 +476,21 @@ static int rmi_f01_attention(struct rmi_function *fn,
unsigned long *irq_bits)
{
struct rmi_device *rmi_dev = fn->rmi_dev;
- struct f01_data *data = fn->data;
- int retval;
-
- retval = rmi_read_block(rmi_dev, fn->fd.data_base_addr,
- &data->device_status, sizeof(data->device_status));
- if (retval < 0) {
- dev_err(&fn->dev, "Failed to read device status, code: %d.\n",
- retval);
- return retval;
+ int error;
+ u8 device_status;
+
+ error = rmi_read(rmi_dev, fn->fd.data_base_addr, &device_status);
+ if (error) {
+ dev_err(&fn->dev,
+ "Failed to read device status: %d.\n", error);
+ return error;
}
- if (RMI_F01_STATUS_UNCONFIGURED(data->device_status)) {
+ if (RMI_F01_STATUS_UNCONFIGURED(device_status)) {
dev_warn(&fn->dev, "Device reset detected.\n");
- retval = rmi_dev->driver->reset_handler(rmi_dev);
- if (retval < 0)
- return retval;
+ error = rmi_dev->driver->reset_handler(rmi_dev);
+ if (error < 0)
+ return error;
}
return 0;
}
@@ -559,7 +507,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,
};
^ permalink raw reply related
* Re: [PATCH] input synaptics-rmi4: Use put_device() and device_type.release() to free storage.
From: Dmitry Torokhov @ 2014-02-12 6:43 UTC (permalink / raw)
To: Courtney Cavin
Cc: Christopher Heiny, Linux Input, Andrew Duggan, Vincent Huang,
Vivian Ly, Daniel Rosenberg, Jean Delvare, Joerie de Gram,
Linus Walleij, Benjamin Tissoires, David Herrmann, Jiri Kosina
In-Reply-To: <20140212045452.GB1706@sonymobile.com>
On Tue, Feb 11, 2014 at 08:54:53PM -0800, Courtney Cavin wrote:
> On Wed, Feb 12, 2014 at 04:17:59AM +0100, Christopher Heiny wrote:
> > On 02/11/2014 06:49 PM, Courtney Cavin wrote:
> > > On Wed, Feb 12, 2014 at 03:17:57AM +0100, Christopher Heiny wrote:
> > >> On 02/11/2014 05:59 PM, Courtney Cavin wrote:
> > >>> On Wed, Feb 12, 2014 at 12:13:30AM +0100, Christopher Heiny wrote:
> > >>>> For rmi_sensor and rmi_function device_types, use put_device() and
> > >>>> the assocated device_type.release() function to clean up related
> > >>>> structures and storage in the correct and safe order.
> > >>>>
> > >>>> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
> > >>>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > >>>> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > >>>> Cc: Linux Walleij <linus.walleij@linaro.org>
> > >>>> Cc: David Herrmann <dh.herrmann@gmail.com>
> > >>>> Cc: Jiri Kosina <jkosina@suse.cz>
> > >>>> Cc: Courtney Cavin <courtney.cavin@sonymobile.com>
> > >>>
> > >>> I'm not a huge fan of you taking my patches, re-formatting them and
> > >>> sending them as your own. More out of principle then actually caring
> > >>> about ownership. You at least cc'd me on this one....
> > >>
> > >> Sorry - no slight was intended at all! I wasn't sure what the protocol
> > >> was for picking up an idea from someone else's patch and building on
> > >> that idea, so I just went with the CC. I definitely prefer to attribute
> > >> sources correctly - if you could clarify what should be done (beyond the
> > >> CC) to acknowledge the author of the original patch, I'd appreciate it.
> > >
> > > Sure. In short, follow Documentation/SubmittingPatches , esp. section
> > > 12) Sign your work.
> > >
> > > Generally the patch should read something like the following:
> > >
> > > From: Original Author <original.author@example.org>
> > >
> > > *BLURB*
> > >
> > > Signed-off-by: Original Author <original.author@example.org>
> > > [additional.author@example.org: changed x and y]
> > > Signed-off-by: Additional Author <additional.author@example.org>
> > >
> > > Assuming the original author actually signed-off the patch in the first
> > > place, of course. The square bracket part is optional, but can be
> > > helpful for reviewers.
> > >
> > > I'm somewhat surprised that you are not aware of this procedure, as this
> > > is how Dmitry has replied to some of your patches in the past.'
> >
> > Thanks very much.
> >
> > I was actually aware of that, but thought the work was sufficiently
> > different from your original patch that applying your Signed-off-by: to
> > it wouldn't be appropriate (I dislike being signed off on things I don't
> > necessarily agree with as much as lack of attribution). I'll be less
> > paranoid about that in the future.
>
> I don't see how they were different enough, when clearly these two
> patches attempt to fix the same bugs, using the same methods with
> slightly modified flow. Perhaps the patches may be small enough
> to be interpreted either way, but at the very least reported-by (since
> this is a bug-fix) or suggested-by is more appropriate than Cc. This is
> a public list, so I'm sure someone will tell you when you are wrong, if
> nothing else.
>
> Along the same topic, I guess I should also mention that it is typically
> frowned upon to takeover someone else's patches without giving them
> due-time to fix any outstanding review comments.
>
> In both of these cases, you neither asked for me to submit the patches
> separately, outside of my DT-series, nor to make any specific changes.
> I was under the impression that you were still participating in the
> discussion for that series.
>
> While it is apparent that we have differing views on how this particular
> driver development should proceed, and we should definitely discuss
> them, please do not think that I'm not willing to apply my patches
> individually to what's in tree now.
>
> My main concern here is that I cannot actually properly test this driver
> without DT, non-gpio irq, and regulator support. Likewise, pre-3.7 is
> ancient, and would require back-porting hundreds of changes.
I can rebase to something more recent; I just did not want to cause
additional work for Chris. Once he finishes pushing his code I was going
to rebase anyway.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] input synaptics-rmi4: Use put_device() and device_type.release() to free storage.
From: Dmitry Torokhov @ 2014-02-12 6:49 UTC (permalink / raw)
To: Christopher Heiny
Cc: Linux Input, Andrew Duggan, Vincent Huang, Vivian Ly,
Daniel Rosenberg, Jean Delvare, Joerie de Gram, Linus Walleij,
Benjamin Tissoires, David Herrmann, Jiri Kosina, Courtney Cavin
In-Reply-To: <1392160410-8293-1-git-send-email-cheiny@synaptics.com>
On Tue, Feb 11, 2014 at 03:13:30PM -0800, Christopher Heiny wrote:
> For rmi_sensor and rmi_function device_types, use put_device() and
> the assocated device_type.release() function to clean up related
> structures and storage in the correct and safe order.
>
> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Cc: Linux Walleij <linus.walleij@linaro.org>
> Cc: David Herrmann <dh.herrmann@gmail.com>
> Cc: Jiri Kosina <jkosina@suse.cz>
> Cc: Courtney Cavin <courtney.cavin@sonymobile.com>
>
> ---
>
> drivers/input/rmi4/rmi_bus.c | 65 +++++++++++++++--------------------------
> drivers/input/rmi4/rmi_driver.c | 11 ++-----
> 2 files changed, 25 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c
> index 96a76e7..1b9ad80 100644
> --- a/drivers/input/rmi4/rmi_bus.c
> +++ b/drivers/input/rmi4/rmi_bus.c
> @@ -30,23 +30,6 @@ static struct dentry *rmi_debugfs_root;
> * purpose. For example F11 is a 2D touch sensor while F01 is a generic
> * function present in every RMI device.
> */
> -
> -static void rmi_release_device(struct device *dev)
> -{
> - struct rmi_device *rmi_dev = to_rmi_device(dev);
> - kfree(rmi_dev);
> -}
> -
> -struct device_type rmi_device_type = {
> - .name = "rmi_sensor",
> - .release = rmi_release_device,
> -};
> -
> -bool rmi_is_physical_device(struct device *dev)
> -{
> - return dev->type == &rmi_device_type;
> -}
> -
> #ifdef CONFIG_RMI4_DEBUG
>
> static void rmi_physical_setup_debugfs(struct rmi_device *rmi_dev)
> @@ -94,8 +77,7 @@ int rmi_register_transport_device(struct rmi_transport_dev *xport)
> return -EINVAL;
> }
>
> - rmi_dev = devm_kzalloc(xport->dev,
> - sizeof(struct rmi_device), GFP_KERNEL);
> + rmi_dev = kzalloc(sizeof(struct rmi_device), GFP_KERNEL);
> if (!rmi_dev)
> return -ENOMEM;
>
> @@ -112,8 +94,10 @@ int rmi_register_transport_device(struct rmi_transport_dev *xport)
> rmi_physical_setup_debugfs(rmi_dev);
>
> error = device_register(&rmi_dev->dev);
> - if (error)
> + if (error) {
> + put_device(&rmi_dev->dev);
> return error;
> + }
>
> dev_dbg(xport->dev, "%s: Registered %s as %s.\n", __func__,
> pdata->sensor_name, dev_name(&rmi_dev->dev));
> @@ -131,7 +115,6 @@ void rmi_unregister_transport_device(struct rmi_transport_dev *xport)
> {
> struct rmi_device *rmi_dev = xport->rmi_dev;
>
> - rmi_physical_teardown_debugfs(rmi_dev);
> device_unregister(&rmi_dev->dev);
> }
> EXPORT_SYMBOL(rmi_unregister_transport_device);
> @@ -139,21 +122,6 @@ EXPORT_SYMBOL(rmi_unregister_transport_device);
>
> /* Function specific stuff */
>
> -static void rmi_release_function(struct device *dev)
> -{
> - struct rmi_function *fn = to_rmi_function(dev);
> - kfree(fn);
> -}
> -
> -struct device_type rmi_function_type = {
> - .name = "rmi_function",
> - .release = rmi_release_function,
> -};
> -
> -bool rmi_is_function_device(struct device *dev)
> -{
> - return dev->type == &rmi_function_type;
> -}
>
> #ifdef CONFIG_RMI4_DEBUG
>
> @@ -185,6 +153,23 @@ static void rmi_function_teardown_debugfs(struct rmi_function *fn)
> }
>
> #endif
> +static void rmi_release_function(struct device *dev)
> +{
> + struct rmi_function *fn = to_rmi_function(dev);
> + rmi_function_teardown_debugfs(fn);
This is wrong, rmi_release_function should finish cleaning up resources,
however unregistration part should happen much earlier. If someone takes
reference to the device in debugfs the device may never go away as noone
will kick the user out.
Please put the calls to rmi_function_teardown_debugfs() back where they
originally were.
> + kfree(fn->irq_mask);
> + kfree(fn);
> +}
> +
> +struct device_type rmi_function_type = {
> + .name = "rmi_function",
> + .release = rmi_release_function,
> +};
> +
> +bool rmi_is_function_device(struct device *dev)
> +{
> + return dev->type == &rmi_function_type;
> +}
>
> static int rmi_function_match(struct device *dev, struct device_driver *drv)
> {
> @@ -240,21 +225,17 @@ int rmi_register_function(struct rmi_function *fn)
> dev_err(&rmi_dev->dev,
> "Failed device_register function device %s\n",
> dev_name(&fn->dev));
> - goto error_exit;
> + put_device(&fn->dev);
> + return error;
> }
>
> dev_dbg(&rmi_dev->dev, "Registered F%02X.\n", fn->fd.function_number);
>
> return 0;
> -
> -error_exit:
> - rmi_function_teardown_debugfs(fn);
> - return error;
> }
>
> void rmi_unregister_function(struct rmi_function *fn)
> {
> - rmi_function_teardown_debugfs(fn);
> device_unregister(&fn->dev);
> }
>
> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
> index 34f19e9..43575a1 100644
> --- a/drivers/input/rmi4/rmi_driver.c
> +++ b/drivers/input/rmi4/rmi_driver.c
> @@ -674,8 +674,7 @@ static int rmi_create_function(struct rmi_device *rmi_dev,
> if (!fn->irq_mask) {
> dev_err(dev, "%s: Failed to create irq_mask for F%02X.\n",
> __func__, pdt->function_number);
> - error = -ENOMEM;
> - goto err_free_mem;
> + return -ENOMEM;
> }
>
> for (i = 0; i < fn->num_of_irqs; i++)
> @@ -683,7 +682,7 @@ static int rmi_create_function(struct rmi_device *rmi_dev,
>
> error = rmi_register_function(fn);
> if (error)
> - goto err_free_irq_mask;
> + return error;
>
> if (pdt->function_number == 0x01)
> data->f01_container = fn;
> @@ -691,12 +690,6 @@ static int rmi_create_function(struct rmi_device *rmi_dev,
> list_add_tail(&fn->node, &data->function_list);
>
> return RMI_SCAN_CONTINUE;
> -
> -err_free_irq_mask:
> - kfree(fn->irq_mask);
> -err_free_mem:
> - kfree(fn);
> - return error;
Unless you create rmi_allocate_function() and do device initialization
there (so that device and kobject are fully initialized and proper ktype
is assigned so that ->release() is set up) you still need to free memory
here if failure happens before calling rmi_register_function().
Thanks.
--
Dmitry
^ permalink raw reply
* Cash Awaiting Pick Up..
From: 2014 Heritage Foundation Board @ 2014-02-12 9:13 UTC (permalink / raw)
This is to re-notify you that you have $500,000.00 waiting for pick-up at Money Gram, Contact
Mrs Hillary Florence via email : heritdept@xtra.co.nz for claims.
^ permalink raw reply
* Re: [PATCH v2 0/9] HID: spring cleanup v2
From: David Herrmann @ 2014-02-12 10:13 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Jiri Kosina, Frank Praznik,
open list:HID CORE LAYER, linux-kernel
In-Reply-To: <1391636004-29354-1-git-send-email-benjamin.tissoires@redhat.com>
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 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 01/14] HID: uHID: remove duplicated code
From: David Herrmann @ 2014-02-12 10:17 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Jiri Kosina, open list:HID CORE LAYER,
linux-kernel
In-Reply-To: <1392055139-19631-2-git-send-email-benjamin.tissoires@redhat.com>
Hi
On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> uhid_hid_output_report() can be implemented through a simple call
> to uhid_hid_output_raw().
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
> drivers/hid/uhid.c | 22 +---------------------
> 1 file changed, 1 insertion(+), 21 deletions(-)
>
> diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
> index 12439e1..b6de903 100644
> --- a/drivers/hid/uhid.c
> +++ b/drivers/hid/uhid.c
> @@ -247,27 +247,7 @@ static int uhid_hid_output_raw(struct hid_device *hid, __u8 *buf, size_t count,
> static int uhid_hid_output_report(struct hid_device *hid, __u8 *buf,
> size_t count)
> {
> - struct uhid_device *uhid = hid->driver_data;
> - unsigned long flags;
> - struct uhid_event *ev;
> -
> - if (count < 1 || count > UHID_DATA_MAX)
> - return -EINVAL;
> -
> - ev = kzalloc(sizeof(*ev), GFP_KERNEL);
> - if (!ev)
> - return -ENOMEM;
> -
> - ev->type = UHID_OUTPUT;
> - ev->u.output.size = count;
> - ev->u.output.rtype = UHID_OUTPUT_REPORT;
> - memcpy(ev->u.output.data, buf, count);
> -
> - spin_lock_irqsave(&uhid->qlock, flags);
> - uhid_queue(uhid, ev);
> - spin_unlock_irqrestore(&uhid->qlock, flags);
> -
> - return count;
> + return uhid_hid_output_raw(hid, buf, count, HID_OUTPUT_REPORT);
Took me a while to see that we have:
HID_OUTPUT_REPORT
UHID_OUTPUT_REPORT
.. gnah! I should have named the uhid stuff better.. anyhow, patch looks good:
Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
Thanks
David
> }
>
> static struct hid_ll_driver uhid_hid_driver = {
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [PATCH 02/14] HID: uHID: implement .raw_request
From: David Herrmann @ 2014-02-12 10:18 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Jiri Kosina, open list:HID CORE LAYER,
linux-kernel
In-Reply-To: <1392055139-19631-3-git-send-email-benjamin.tissoires@redhat.com>
Hi
On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> uHID is missing a SET_REPORT protocol implementation, but as
> .hid_get_raw_report() as been removed from struct hid_device,
> there were no means to access GET_REPORT in uhid.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
> drivers/hid/uhid.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
> index b6de903..60acee4 100644
> --- a/drivers/hid/uhid.c
> +++ b/drivers/hid/uhid.c
> @@ -250,6 +250,21 @@ static int uhid_hid_output_report(struct hid_device *hid, __u8 *buf,
> return uhid_hid_output_raw(hid, buf, count, HID_OUTPUT_REPORT);
> }
>
> +static int uhid_raw_request(struct hid_device *hid, unsigned char reportnum,
> + __u8 *buf, size_t len, unsigned char rtype,
> + int reqtype)
> +{
> + switch (reqtype) {
> + case HID_REQ_GET_REPORT:
> + return uhid_hid_get_raw(hid, reportnum, buf, len, rtype);
> + case HID_REQ_SET_REPORT:
> + /* TODO: implement proper SET_REPORT functionality */
> + return -ENOSYS;
Looks good. I will try to implement it today and send a patch.
Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
Thanks
David
> + default:
> + return -EIO;
> + }
> +}
> +
> static struct hid_ll_driver uhid_hid_driver = {
> .start = uhid_hid_start,
> .stop = uhid_hid_stop,
> @@ -257,6 +272,7 @@ static struct hid_ll_driver uhid_hid_driver = {
> .close = uhid_hid_close,
> .parse = uhid_hid_parse,
> .output_report = uhid_hid_output_report,
> + .raw_request = uhid_raw_request,
> };
>
> #ifdef CONFIG_COMPAT
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [PATCH 03/14] HID: core: implement generic .request()
From: David Herrmann @ 2014-02-12 10:25 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Jiri Kosina, open list:HID CORE LAYER,
linux-kernel
In-Reply-To: <1392055139-19631-4-git-send-email-benjamin.tissoires@redhat.com>
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;
I always have to read that shifting code twice to get it.. Maybe add a
comment explaining the different entries here.
> +}
> +
> /*
> * 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);
> +
> + 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 04/14] HID: i2c-hid: implement ll_driver transport-layer callbacks
From: David Herrmann @ 2014-02-12 10:29 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Jiri Kosina, open list:HID CORE LAYER,
linux-kernel
In-Reply-To: <1392055139-19631-5-git-send-email-benjamin.tissoires@redhat.com>
Hi
On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> Add output_report and raw_request to i2c-hid.
> The current implementation of i2c_hid_output_raw_report decides
> by itself if it should use a direct send of the output report
> or use the data register (SET_REPORT). Split that by reimplement
> the logic in __i2c_hid_output_raw_report() which will be dropped
> soon.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
> drivers/hid/i2c-hid/i2c-hid.c | 69 +++++++++++++++++++++++++++++++++++++------
> 1 file changed, 60 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index f57de7f..07a054a 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -257,12 +257,21 @@ static int i2c_hid_get_report(struct i2c_client *client, u8 reportType,
> return 0;
> }
>
> -static int i2c_hid_set_report(struct i2c_client *client, u8 reportType,
> - u8 reportID, unsigned char *buf, size_t data_len)
> +/**
> + * i2c_hid_set_or_send_report: forward an incoming report to the device
> + * @client: the i2c_client of the device
> + * @reportType: 0x03 for HID_FEATURE_REPORT ; 0x02 for HID_OUTPUT_REPORT
> + * @reportID: the report ID
> + * @buf: the actual data to transfer, without the report ID
> + * @len: size of buf
> + * @use_data: true: use SET_REPORT HID command, false: send plain OUTPUT report
> + */
> +static int i2c_hid_set_or_send_report(struct i2c_client *client, u8 reportType,
> + u8 reportID, unsigned char *buf, size_t data_len, bool use_data)
> {
> struct i2c_hid *ihid = i2c_get_clientdata(client);
> u8 *args = ihid->argsbuf;
> - const struct i2c_hid_cmd * hidcmd = &hid_set_report_cmd;
> + const struct i2c_hid_cmd *hidcmd;
> int ret;
> u16 dataRegister = le16_to_cpu(ihid->hdesc.wDataRegister);
> u16 outputRegister = le16_to_cpu(ihid->hdesc.wOutputRegister);
> @@ -279,6 +288,9 @@ static int i2c_hid_set_report(struct i2c_client *client, u8 reportType,
>
> i2c_hid_dbg(ihid, "%s\n", __func__);
>
> + if (!use_data && maxOutputLength == 0)
> + return -ENOSYS;
> +
> if (reportID >= 0x0F) {
> args[index++] = reportID;
> reportID = 0x0F;
> @@ -288,9 +300,10 @@ static int i2c_hid_set_report(struct i2c_client *client, u8 reportType,
> * use the data register for feature reports or if the device does not
> * support the output register
> */
> - if (reportType == 0x03 || maxOutputLength == 0) {
> + if (use_data) {
> args[index++] = dataRegister & 0xFF;
> args[index++] = dataRegister >> 8;
> + hidcmd = &hid_set_report_cmd;
> } else {
> args[index++] = outputRegister & 0xFF;
> args[index++] = outputRegister >> 8;
> @@ -559,7 +572,7 @@ static int i2c_hid_get_raw_report(struct hid_device *hid,
> }
>
> static int i2c_hid_output_raw_report(struct hid_device *hid, __u8 *buf,
> - size_t count, unsigned char report_type)
> + size_t count, unsigned char report_type, bool use_data)
> {
> struct i2c_client *client = hid->driver_data;
> int report_id = buf[0];
> @@ -573,9 +586,9 @@ static int i2c_hid_output_raw_report(struct hid_device *hid, __u8 *buf,
> count--;
> }
>
> - ret = i2c_hid_set_report(client,
> + ret = i2c_hid_set_or_send_report(client,
> report_type == HID_FEATURE_REPORT ? 0x03 : 0x02,
> - report_id, buf, count);
> + report_id, buf, count, use_data);
>
> if (report_id && ret >= 0)
> ret++; /* add report_id to the number of transfered bytes */
> @@ -583,6 +596,42 @@ static int i2c_hid_output_raw_report(struct hid_device *hid, __u8 *buf,
> return ret;
> }
>
> +static int __i2c_hid_output_raw_report(struct hid_device *hid, __u8 *buf,
> + size_t count, unsigned char report_type)
> +{
> + struct i2c_client *client = hid->driver_data;
> + struct i2c_hid *ihid = i2c_get_clientdata(client);
> + bool data = true; /* SET_REPORT */
> +
> + if (report_type == HID_OUTPUT_REPORT)
> + data = le16_to_cpu(ihid->hdesc.wMaxOutputLength) == 0;
Yepp, thanks for splitting that logic out of the report-handling,
makes it much easier to follow.
Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
Thanks
David
> +
> + return i2c_hid_output_raw_report(hid, buf, count, report_type, data);
> +}
> +
> +static int i2c_hid_output_report(struct hid_device *hid, __u8 *buf,
> + size_t count)
> +{
> + return i2c_hid_output_raw_report(hid, buf, count, HID_OUTPUT_REPORT,
> + false);
> +}
> +
> +static int i2c_hid_raw_request(struct hid_device *hid, unsigned char reportnum,
> + __u8 *buf, size_t len, unsigned char rtype,
> + int reqtype)
> +{
> + switch (reqtype) {
> + case HID_REQ_GET_REPORT:
> + return i2c_hid_get_raw_report(hid, reportnum, buf, len, rtype);
> + case HID_REQ_SET_REPORT:
> + if (buf[0] != reportnum)
> + return -EINVAL;
> + return i2c_hid_output_raw_report(hid, buf, len, rtype, true);
> + default:
> + return -EIO;
> + }
> +}
> +
> static void i2c_hid_request(struct hid_device *hid, struct hid_report *rep,
> int reqtype)
> {
> @@ -606,7 +655,7 @@ static void i2c_hid_request(struct hid_device *hid, struct hid_report *rep,
> break;
> case HID_REQ_SET_REPORT:
> hid_output_report(rep, buf);
> - i2c_hid_output_raw_report(hid, buf, len, rep->type);
> + i2c_hid_output_raw_report(hid, buf, len, rep->type, true);
> break;
> }
>
> @@ -769,6 +818,8 @@ static struct hid_ll_driver i2c_hid_ll_driver = {
> .close = i2c_hid_close,
> .power = i2c_hid_power,
> .request = i2c_hid_request,
> + .output_report = i2c_hid_output_report,
> + .raw_request = i2c_hid_raw_request,
> };
>
> static int i2c_hid_init_irq(struct i2c_client *client)
> @@ -1003,7 +1054,7 @@ static int i2c_hid_probe(struct i2c_client *client,
>
> hid->driver_data = client;
> hid->ll_driver = &i2c_hid_ll_driver;
> - hid->hid_output_raw_report = i2c_hid_output_raw_report;
> + hid->hid_output_raw_report = __i2c_hid_output_raw_report;
> hid->dev.parent = &client->dev;
> ACPI_COMPANION_SET(&hid->dev, ACPI_COMPANION(&client->dev));
> hid->bus = BUS_I2C;
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [PATCH 05/14] HID: i2c-hid: use generic .request() implementation
From: David Herrmann @ 2014-02-12 10:30 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Jiri Kosina, open list:HID CORE LAYER,
linux-kernel
In-Reply-To: <1392055139-19631-6-git-send-email-benjamin.tissoires@redhat.com>
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,
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 06/14] HID: usbhid: change return error of usbhid_output_report
From: David Herrmann @ 2014-02-12 10:31 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Jiri Kosina, open list:HID CORE LAYER,
linux-kernel
In-Reply-To: <1392055139-19631-7-git-send-email-benjamin.tissoires@redhat.com>
Hi
On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> If there is no urbout when sending a output report, ENOSYS (Function
> not implemented) is a better error than EIO (I/O error).
Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
Thanks
David
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
> drivers/hid/usbhid/hid-core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index b9a770f..0d1d875 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -922,7 +922,7 @@ static int usbhid_output_report(struct hid_device *hid, __u8 *buf, size_t count)
> int actual_length, skipped_report_id = 0, ret;
>
> if (!usbhid->urbout)
> - return -EIO;
> + return -ENOSYS;
>
> if (buf[0] == 0x0) {
> /* Don't send the Report ID */
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [PATCH 07/14] HID: input: hid-input remove hid_output_raw_report call
From: David Herrmann @ 2014-02-12 10:35 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Jiri Kosina, open list:HID CORE LAYER,
linux-kernel
In-Reply-To: <1392055139-19631-8-git-send-email-benjamin.tissoires@redhat.com>
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?
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.
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 08/14] HID: logitech-dj: remove hid_output_raw_report call
From: David Herrmann @ 2014-02-12 10:36 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Jiri Kosina, open list:HID CORE LAYER,
linux-kernel
In-Reply-To: <1392055139-19631-9-git-send-email-benjamin.tissoires@redhat.com>
Hi
On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> hid-input do not use anymore hid_output_raw_report() to set the LEDs.
> Use the correct implementation now and make them working again.
Looks good.
Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
Thanks
David
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
> drivers/hid/hid-logitech-dj.c | 21 ++++++---------------
> 1 file changed, 6 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
> index 980ede5..486dbde 100644
> --- a/drivers/hid/hid-logitech-dj.c
> +++ b/drivers/hid/hid-logitech-dj.c
> @@ -193,9 +193,6 @@ static const u8 hid_reportid_size_map[NUMBER_OF_HID_REPORTS] = {
>
> static struct hid_ll_driver logi_dj_ll_driver;
>
> -static int logi_dj_output_hidraw_report(struct hid_device *hid, u8 * buf,
> - size_t count,
> - unsigned char report_type);
> static int logi_dj_recv_query_paired_devices(struct dj_receiver_dev *djrcv_dev);
>
> static void logi_dj_recv_destroy_djhid_device(struct dj_receiver_dev *djrcv_dev,
> @@ -262,7 +259,6 @@ static void logi_dj_recv_add_djhid_device(struct dj_receiver_dev *djrcv_dev,
> }
>
> dj_hiddev->ll_driver = &logi_dj_ll_driver;
> - dj_hiddev->hid_output_raw_report = logi_dj_output_hidraw_report;
>
> dj_hiddev->dev.parent = &djrcv_hdev->dev;
> dj_hiddev->bus = BUS_USB;
> @@ -544,9 +540,10 @@ static void logi_dj_ll_close(struct hid_device *hid)
> dbg_hid("%s:%s\n", __func__, hid->phys);
> }
>
> -static int logi_dj_output_hidraw_report(struct hid_device *hid, u8 * buf,
> - size_t count,
> - unsigned char report_type)
> +static int logi_dj_ll_raw_request(struct hid_device *hid,
> + unsigned char reportnum, __u8 *buf,
> + size_t count, unsigned char report_type,
> + int reqtype)
> {
> struct dj_device *djdev = hid->driver_data;
> struct dj_receiver_dev *djrcv_dev = djdev->dj_receiver_dev;
> @@ -567,15 +564,8 @@ static int logi_dj_output_hidraw_report(struct hid_device *hid, u8 * buf,
> out_buf[1] = djdev->device_index;
> memcpy(out_buf + 2, buf, count);
>
> - /*
> - * hid-generic calls us with hid_output_raw_report(), but the LEDs
> - * are set through a SET_REPORT command. It works for USB-HID devices
> - * because usbhid either calls a SET_REPORT or directly send the output
> - * report depending if the device presents an urbout.
> - * Let be simple, send a SET_REPORT request.
> - */
> ret = hid_hw_raw_request(djrcv_dev->hdev, out_buf[0], out_buf,
> - DJREPORT_SHORT_LENGTH, report_type, HID_REQ_SET_REPORT);
> + DJREPORT_SHORT_LENGTH, report_type, reqtype);
>
> kfree(out_buf);
> return ret;
> @@ -662,6 +652,7 @@ static struct hid_ll_driver logi_dj_ll_driver = {
> .stop = logi_dj_ll_stop,
> .open = logi_dj_ll_open,
> .close = logi_dj_ll_close,
> + .raw_request = logi_dj_ll_raw_request,
> };
>
>
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [PATCH 10/14] HID: wiimote: replace hid_output_raw_report with hid_hw_output_report for output requests
From: David Herrmann @ 2014-02-12 10:39 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Jiri Kosina, open list:HID CORE LAYER,
linux-kernel
In-Reply-To: <1392055139-19631-11-git-send-email-benjamin.tissoires@redhat.com>
Hi
On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> For BT transport layer,
> ret = hid_output_raw_report(A, B, C, HID_OUTPUT_REPORT);
> is equivalent to
> ret = hid_hw_output_report(A, B, C);
>
> So use the new API where available
Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
Thanks
David
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
> drivers/hid/hid-wiimote-core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
> index d7dc6c5b..d003914 100644
> --- a/drivers/hid/hid-wiimote-core.c
> +++ b/drivers/hid/hid-wiimote-core.c
> @@ -28,14 +28,14 @@ static int wiimote_hid_send(struct hid_device *hdev, __u8 *buffer,
> __u8 *buf;
> int ret;
>
> - if (!hdev->hid_output_raw_report)
> + if (!hdev->ll_driver->output_report)
> return -ENODEV;
>
> buf = kmemdup(buffer, count, GFP_KERNEL);
> if (!buf)
> return -ENOMEM;
>
> - ret = hid_output_raw_report(hdev, buf, count, HID_OUTPUT_REPORT);
> + ret = hid_hw_output_report(hdev, buf, count);
>
> kfree(buf);
> return ret;
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [PATCH 11/14] HID: sony: remove hid_output_raw_report calls
From: David Herrmann @ 2014-02-12 10:47 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Jiri Kosina, open list:HID CORE LAYER,
linux-kernel
In-Reply-To: <1392055139-19631-12-git-send-email-benjamin.tissoires@redhat.com>
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
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.
Why can't we introduce QUIRK flags that make HID core drop the
report_id for outgoing messages?
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 12/14] HID: hidraw: replace hid_output_raw_report() calls by appropriates ones
From: David Herrmann @ 2014-02-12 10:49 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Jiri Kosina, open list:HID CORE LAYER,
linux-kernel
In-Reply-To: <1392055139-19631-13-git-send-email-benjamin.tissoires@redhat.com>
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.
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 13/14] HID: remove hid_output_raw_report
From: David Herrmann @ 2014-02-12 10:50 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Jiri Kosina, open list:HID CORE LAYER,
linux-kernel
In-Reply-To: <1392055139-19631-14-git-send-email-benjamin.tissoires@redhat.com>
Hi
On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> hid_output_raw_report() is not used anymore, delete it.
yippieh!
Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
Thanks
David
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
> drivers/hid/i2c-hid/i2c-hid.c | 14 --------------
> drivers/hid/uhid.c | 1 -
> drivers/hid/usbhid/hid-core.c | 12 ------------
> include/linux/hid.h | 19 -------------------
> net/bluetooth/hidp/core.c | 14 --------------
> 5 files changed, 60 deletions(-)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index 925fb8d..d3b8d7a 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -596,19 +596,6 @@ static int i2c_hid_output_raw_report(struct hid_device *hid, __u8 *buf,
> return ret;
> }
>
> -static int __i2c_hid_output_raw_report(struct hid_device *hid, __u8 *buf,
> - size_t count, unsigned char report_type)
> -{
> - struct i2c_client *client = hid->driver_data;
> - struct i2c_hid *ihid = i2c_get_clientdata(client);
> - bool data = true; /* SET_REPORT */
> -
> - if (report_type == HID_OUTPUT_REPORT)
> - data = le16_to_cpu(ihid->hdesc.wMaxOutputLength) == 0;
> -
> - return i2c_hid_output_raw_report(hid, buf, count, report_type, data);
> -}
> -
> static int i2c_hid_output_report(struct hid_device *hid, __u8 *buf,
> size_t count)
> {
> @@ -1023,7 +1010,6 @@ static int i2c_hid_probe(struct i2c_client *client,
>
> hid->driver_data = client;
> hid->ll_driver = &i2c_hid_ll_driver;
> - hid->hid_output_raw_report = __i2c_hid_output_raw_report;
> hid->dev.parent = &client->dev;
> ACPI_COMPANION_SET(&hid->dev, ACPI_COMPANION(&client->dev));
> hid->bus = BUS_I2C;
> diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
> index 60acee4..7ed79be 100644
> --- a/drivers/hid/uhid.c
> +++ b/drivers/hid/uhid.c
> @@ -400,7 +400,6 @@ static int uhid_dev_create(struct uhid_device *uhid,
> hid->uniq[63] = 0;
>
> hid->ll_driver = &uhid_hid_driver;
> - hid->hid_output_raw_report = uhid_hid_output_raw;
> hid->bus = ev->u.create.bus;
> hid->vendor = ev->u.create.vendor;
> hid->product = ev->u.create.product;
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index 0d1d875..02b3256 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -945,17 +945,6 @@ static int usbhid_output_report(struct hid_device *hid, __u8 *buf, size_t count)
> return ret;
> }
>
> -static int usbhid_output_raw_report(struct hid_device *hid, __u8 *buf,
> - size_t count, unsigned char report_type)
> -{
> - struct usbhid_device *usbhid = hid->driver_data;
> -
> - if (usbhid->urbout && report_type != HID_FEATURE_REPORT)
> - return usbhid_output_report(hid, buf, count);
> -
> - return usbhid_set_raw_report(hid, buf[0], buf, count, report_type);
> -}
> -
> static void usbhid_restart_queues(struct usbhid_device *usbhid)
> {
> if (usbhid->urbout && !test_bit(HID_OUT_RUNNING, &usbhid->iofl))
> @@ -1289,7 +1278,6 @@ static int usbhid_probe(struct usb_interface *intf, const struct usb_device_id *
>
> usb_set_intfdata(intf, hid);
> hid->ll_driver = &usb_hid_driver;
> - hid->hid_output_raw_report = usbhid_output_raw_report;
> hid->ff_init = hid_pidff_init;
> #ifdef CONFIG_USB_HIDDEV
> hid->hiddev_connect = hiddev_connect;
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 09fbbd7..fa07639 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -508,9 +508,6 @@ struct hid_device { /* device report descriptor */
> struct hid_usage *, __s32);
> void (*hiddev_report_event) (struct hid_device *, struct hid_report *);
>
> - /* handler for raw output data, used by hidraw */
> - int (*hid_output_raw_report) (struct hid_device *, __u8 *, size_t, unsigned char);
> -
> /* debugging support via debugfs */
> unsigned short debug;
> struct dentry *debug_dir;
> @@ -1015,22 +1012,6 @@ static inline int hid_hw_output_report(struct hid_device *hdev, __u8 *buf,
> }
>
> /**
> - * hid_output_raw_report - send an output or a feature report to the device
> - *
> - * @hdev: hid device
> - * @buf: raw data to transfer
> - * @len: length of buf
> - * @report_type: HID_FEATURE_REPORT or HID_OUTPUT_REPORT
> - *
> - * @return: count of data transfered, negative if error
> - */
> -static inline int hid_output_raw_report(struct hid_device *hdev, __u8 *buf,
> - size_t len, unsigned char report_type)
> -{
> - return hdev->hid_output_raw_report(hdev, buf, len, report_type);
> -}
> -
> -/**
> * hid_hw_idle - send idle request to device
> *
> * @hdev: hid device
> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
> index 77c4bad..89da18d 100644
> --- a/net/bluetooth/hidp/core.c
> +++ b/net/bluetooth/hidp/core.c
> @@ -382,18 +382,6 @@ static int hidp_output_report(struct hid_device *hid, __u8 *data, size_t count)
> data, count);
> }
>
> -static int hidp_output_raw_report(struct hid_device *hid, unsigned char *data,
> - size_t count, unsigned char report_type)
> -{
> - if (report_type == HID_OUTPUT_REPORT) {
> - return hidp_output_report(hid, data, count);
> - } else if (report_type != HID_FEATURE_REPORT) {
> - return -EINVAL;
> - }
> -
> - return hidp_set_raw_report(hid, data[0], data, count, report_type);
> -}
> -
> static int hidp_raw_request(struct hid_device *hid, unsigned char reportnum,
> __u8 *buf, size_t len, unsigned char rtype,
> int reqtype)
> @@ -773,8 +761,6 @@ static int hidp_setup_hid(struct hidp_session *session,
> hid->dev.parent = &session->conn->hcon->dev;
> hid->ll_driver = &hidp_hid_driver;
>
> - hid->hid_output_raw_report = hidp_output_raw_report;
> -
> /* True if device is blacklisted in drivers/hid/hid-core.c */
> if (hid_ignore(hid)) {
> hid_destroy_device(session->hid);
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [PATCH 14/14] HID: core: check parameters when sending/receiving data from the device
From: David Herrmann @ 2014-02-12 10:51 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Jiri Kosina, open list:HID CORE LAYER,
linux-kernel
In-Reply-To: <1392055139-19631-15-git-send-email-benjamin.tissoires@redhat.com>
Hi
On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> It is better to check them soon enough before triggering any kernel panic.
Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
Thanks
David
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
> drivers/hid/i2c-hid/i2c-hid.c | 2 +-
> include/linux/hid.h | 6 ++++++
> 2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index d3b8d7a..b50860d 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -277,7 +277,7 @@ static int i2c_hid_set_or_send_report(struct i2c_client *client, u8 reportType,
> u16 outputRegister = le16_to_cpu(ihid->hdesc.wOutputRegister);
> u16 maxOutputLength = le16_to_cpu(ihid->hdesc.wMaxOutputLength);
>
> - /* hidraw already checked that data_len < HID_MAX_BUFFER_SIZE */
> + /* hid_hw_* already checked that data_len < HID_MAX_BUFFER_SIZE */
> u16 size = 2 /* size */ +
> (reportID ? 1 : 0) /* reportID */ +
> data_len /* buf */;
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index fa07639..f801506 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -986,6 +986,9 @@ static inline int hid_hw_raw_request(struct hid_device *hdev,
> unsigned char reportnum, __u8 *buf,
> size_t len, unsigned char rtype, int reqtype)
> {
> + if (len < 1 || len > HID_MAX_BUFFER_SIZE || !buf)
> + return -EINVAL;
> +
> if (hdev->ll_driver->raw_request)
> return hdev->ll_driver->raw_request(hdev, reportnum, buf, len,
> rtype, reqtype);
> @@ -1005,6 +1008,9 @@ static inline int hid_hw_raw_request(struct hid_device *hdev,
> static inline int hid_hw_output_report(struct hid_device *hdev, __u8 *buf,
> size_t len)
> {
> + if (len < 1 || len > HID_MAX_BUFFER_SIZE || !buf)
> + return -EINVAL;
> +
> if (hdev->ll_driver->output_report)
> return hdev->ll_driver->output_report(hdev, buf, len);
>
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [PATCH v3 0/6] HID: sony: Add full Bluetooth support for the Dualshock 4
From: David Herrmann @ 2014-02-12 12:14 UTC (permalink / raw)
To: Frank Praznik; +Cc: open list:HID CORE LAYER, Jiri Kosina
In-Reply-To: <1391648629-3077-1-git-send-email-frank.praznik@oh.rr.com>
Hi
On Thu, Feb 6, 2014 at 2:03 AM, Frank Praznik <frank.praznik@oh.rr.com> wrote:
> v3 of this patch series rebases the code against Benjamin Tissoires' HID
> transport layer changes to use the safe inline hid_hw_* functions which
> eliminates the need to check the function pointers manually.
>
> It adds an explicit Bluetooth initialization function instead of the controller
> input report state being set as a side effect of initializing the LED system.
>
> It also uses a new DUALSHOCK4_CONTROLLER macro to simplify conditional cases
> where the connection type is irrelevant.
The series looks good to me.
Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
Thanks
David
^ permalink raw reply
* [PATCH V2] ONKEY: da9052: Use correct register bit for key status
From: Anthony Olech @ 2014-02-12 14:11 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Paul Gortmaker, linux-input, linux-kernel, David Dajun Chen
The wrong register bit of the DA9052/3 PMIC registers was
used to determine the status on the ONKEY.
Also a failure in reading the status register will no longer
result in the work queue being rescheduled as that would result
in a (potentially) endless retry.
Signed-off-by: Anthony Olech <anthony.olech.opensource@diasemi.com>
Acked-by: David Dajun Chen <david.chen@diasemi.com>
---
This patch is relative to linux-next repository tag next-20140212
The bug that this patch fixes affects only the DA9052 ONKEY driver.
The problem was detected whilst running a scripted set of functional
regression tests whilst investigating a different problem.
This patch has been test compiled on an amd64 server for both x86
and arm targets.
This patch has been spot verified using an SMDK6410 platform
fly-wired to a Dialog da9053 EVB.
drivers/input/misc/da9052_onkey.c | 29 ++++++++++++++++-------------
1 file changed, 16 insertions(+), 13 deletions(-)
diff --git a/drivers/input/misc/da9052_onkey.c b/drivers/input/misc/da9052_onkey.c
index 1f695f2..184c8f2 100644
--- a/drivers/input/misc/da9052_onkey.c
+++ b/drivers/input/misc/da9052_onkey.c
@@ -27,29 +27,32 @@ struct da9052_onkey {
static void da9052_onkey_query(struct da9052_onkey *onkey)
{
- int key_stat;
+ int ret;
- key_stat = da9052_reg_read(onkey->da9052, DA9052_EVENT_B_REG);
- if (key_stat < 0) {
+ ret = da9052_reg_read(onkey->da9052, DA9052_STATUS_A_REG);
+ if (ret < 0) {
dev_err(onkey->da9052->dev,
- "Failed to read onkey event %d\n", key_stat);
+ "Failed to read onkey event err=%d\n", ret);
} else {
/*
* Since interrupt for deassertion of ONKEY pin is not
* generated, onkey event state determines the onkey
* button state.
*/
- key_stat &= DA9052_EVENTB_ENONKEY;
- input_report_key(onkey->input, KEY_POWER, key_stat);
+ bool pressed = !(ret & DA9052_STATUSA_NONKEY);
+
+ input_report_key(onkey->input, KEY_POWER, pressed);
input_sync(onkey->input);
- }
- /*
- * Interrupt is generated only when the ONKEY pin is asserted.
- * Hence the deassertion of the pin is simulated through work queue.
- */
- if (key_stat)
- schedule_delayed_work(&onkey->work, msecs_to_jiffies(50));
+ /*
+ * Interrupt is generated only when the ONKEY pin
+ * is asserted. Hence the deassertion of the pin
+ * is simulated through work queue.
+ */
+ if (pressed)
+ schedule_delayed_work(&onkey->work,
+ msecs_to_jiffies(50));
+ }
}
static void da9052_onkey_work(struct work_struct *work)
--
end-of-patch for ONKEY: da9052: Use correct register bit for key status V2
^ permalink raw reply related
* Re: [PATCH] input synaptics-rmi4: Use put_device() and device_type.release() to free storage.
From: Courtney Cavin @ 2014-02-12 17:09 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Christopher Heiny, Linux Input, Andrew Duggan, Vincent Huang,
Vivian Ly, Daniel Rosenberg, Jean Delvare, Joerie de Gram,
Linus Walleij, Benjamin Tissoires, David Herrmann, Jiri Kosina
In-Reply-To: <20140212064342.GB15855@core.coreip.homeip.net>
On Wed, Feb 12, 2014 at 07:43:42AM +0100, Dmitry Torokhov wrote:
> On Tue, Feb 11, 2014 at 08:54:53PM -0800, Courtney Cavin wrote:
> > On Wed, Feb 12, 2014 at 04:17:59AM +0100, Christopher Heiny wrote:
> > > On 02/11/2014 06:49 PM, Courtney Cavin wrote:
> > > > On Wed, Feb 12, 2014 at 03:17:57AM +0100, Christopher Heiny wrote:
> > > >> On 02/11/2014 05:59 PM, Courtney Cavin wrote:
> > > >>> On Wed, Feb 12, 2014 at 12:13:30AM +0100, Christopher Heiny wrote:
> > > >>>> For rmi_sensor and rmi_function device_types, use put_device() and
> > > >>>> the assocated device_type.release() function to clean up related
> > > >>>> structures and storage in the correct and safe order.
> > > >>>>
> > > >>>> Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
> > > >>>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > >>>> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > >>>> Cc: Linux Walleij <linus.walleij@linaro.org>
> > > >>>> Cc: David Herrmann <dh.herrmann@gmail.com>
> > > >>>> Cc: Jiri Kosina <jkosina@suse.cz>
> > > >>>> Cc: Courtney Cavin <courtney.cavin@sonymobile.com>
> > > >>>
> > > >>> I'm not a huge fan of you taking my patches, re-formatting them and
> > > >>> sending them as your own. More out of principle then actually caring
> > > >>> about ownership. You at least cc'd me on this one....
> > > >>
> > > >> Sorry - no slight was intended at all! I wasn't sure what the protocol
> > > >> was for picking up an idea from someone else's patch and building on
> > > >> that idea, so I just went with the CC. I definitely prefer to attribute
> > > >> sources correctly - if you could clarify what should be done (beyond the
> > > >> CC) to acknowledge the author of the original patch, I'd appreciate it.
> > > >
> > > > Sure. In short, follow Documentation/SubmittingPatches , esp. section
> > > > 12) Sign your work.
> > > >
> > > > Generally the patch should read something like the following:
> > > >
> > > > From: Original Author <original.author@example.org>
> > > >
> > > > *BLURB*
> > > >
> > > > Signed-off-by: Original Author <original.author@example.org>
> > > > [additional.author@example.org: changed x and y]
> > > > Signed-off-by: Additional Author <additional.author@example.org>
> > > >
> > > > Assuming the original author actually signed-off the patch in the first
> > > > place, of course. The square bracket part is optional, but can be
> > > > helpful for reviewers.
> > > >
> > > > I'm somewhat surprised that you are not aware of this procedure, as this
> > > > is how Dmitry has replied to some of your patches in the past.'
> > >
> > > Thanks very much.
> > >
> > > I was actually aware of that, but thought the work was sufficiently
> > > different from your original patch that applying your Signed-off-by: to
> > > it wouldn't be appropriate (I dislike being signed off on things I don't
> > > necessarily agree with as much as lack of attribution). I'll be less
> > > paranoid about that in the future.
> >
> > I don't see how they were different enough, when clearly these two
> > patches attempt to fix the same bugs, using the same methods with
> > slightly modified flow. Perhaps the patches may be small enough
> > to be interpreted either way, but at the very least reported-by (since
> > this is a bug-fix) or suggested-by is more appropriate than Cc. This is
> > a public list, so I'm sure someone will tell you when you are wrong, if
> > nothing else.
> >
> > Along the same topic, I guess I should also mention that it is typically
> > frowned upon to takeover someone else's patches without giving them
> > due-time to fix any outstanding review comments.
> >
> > In both of these cases, you neither asked for me to submit the patches
> > separately, outside of my DT-series, nor to make any specific changes.
> > I was under the impression that you were still participating in the
> > discussion for that series.
> >
> > While it is apparent that we have differing views on how this particular
> > driver development should proceed, and we should definitely discuss
> > them, please do not think that I'm not willing to apply my patches
> > individually to what's in tree now.
> >
> > My main concern here is that I cannot actually properly test this driver
> > without DT, non-gpio irq, and regulator support. Likewise, pre-3.7 is
> > ancient, and would require back-porting hundreds of changes.
>
> I can rebase to something more recent; I just did not want to cause
> additional work for Chris. Once he finishes pushing his code I was going
> to rebase anyway.
That would be great! Thanks.
-Courtney
^ permalink raw reply
* Re: [PATCH] input synaptics-rmi4: rmi_f01.c storage fix
From: Courtney Cavin @ 2014-02-12 21:48 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Christopher Heiny, Linux Input, Andrew Duggan, Vincent Huang,
Vivian Ly, Daniel Rosenberg, Jean Delvare, Joerie de Gram,
Linus Walleij, Benjamin Tissoires, David Herrmann, Jiri Kosina
In-Reply-To: <20140212064049.GA15855@core.coreip.homeip.net>
On Wed, Feb 12, 2014 at 07:40:49AM +0100, Dmitry Torokhov wrote:
> Hi Chris,
>
> On Tue, Feb 11, 2014 at 03:13:00PM -0800, Christopher Heiny wrote:
> > Correctly free driver related data when initialization fails.
> >
> > Trivial: Clarify a diagnostic message.
> >
> > Signed-off-by: Christopher Heiny <cheiny@synaptics.com>
> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > Cc: Linux Walleij <linus.walleij@linaro.org>
> > Cc: David Herrmann <dh.herrmann@gmail.com>
> > Cc: Jiri Kosina <jkosina@suse.cz>
> >
> > ---
> >
> > drivers/input/rmi4/rmi_f01.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
> > index 381ad60..e4a6df9 100644
> > --- a/drivers/input/rmi4/rmi_f01.c
> > +++ b/drivers/input/rmi4/rmi_f01.c
> > @@ -149,7 +149,7 @@ static int rmi_f01_alloc_memory(struct rmi_function *fn,
> >
> > f01 = devm_kzalloc(&fn->dev, sizeof(struct f01_data), GFP_KERNEL);
> > if (!f01) {
> > - dev_err(&fn->dev, "Failed to allocate fn_01_data.\n");
> > + dev_err(&fn->dev, "Failed to allocate f01_data.\n");
> > return -ENOMEM;
> > }
> >
> > @@ -158,6 +158,7 @@ static int rmi_f01_alloc_memory(struct rmi_function *fn,
> > GFP_KERNEL);
> > if (!f01->device_control.interrupt_enable) {
> > dev_err(&fn->dev, "Failed to allocate interrupt enable.\n");
> > + devm_kfree(&fn->dev, f01);
>
> As Courtney mentioned if you are calling devm_kfree() you are most
> likely doing something wrong.
>
> How about the patch below? Please check the XXX comment, I have some
> concerns about lts vs doze_holdoff check mismatch in probe() and
> config().
>
> Thanks.
>
> --
> Dmitry
>
> Input: synaptics-rmi4 - F01 initialization cleanup
>
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
> - rename data to f01 where appropriate;
> - switch to using rmi_read()/rmi_write() for single-byte data;
> - allocate interrupt mask together with the main structure;
> - do not kfree() memory allocated with devm;
> - do not write config data in probe(), we have config() for that;
> - drop unneeded rmi_f01_remove().
These seem like unrelated changes and make this patch hard to read, I
would prefer if we could separate these out. Perhaps like so?
[1] bug-fix
- do not kfree() memory allocated with devm
[2] simplify probe/remove logic
- allocate interrupt mask together with the main structure
- do not write config data in probe(), we have config() for that
- drop unneeded rmi_f01_remove()
[3] non-behavioral changes/cleanup
- switch to using rmi_read()/rmi_write() for single-byte data
- rename data to f01 where appropriate
Disregarding that, and the nitpick below, it looks good to me.
>
> Reported-by: Courtney Cavin <courtney.cavin@sonymobile.com>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> drivers/input/rmi4/rmi_f01.c | 397 ++++++++++++++++++------------------------
> 1 file changed, 172 insertions(+), 225 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_f01.c b/drivers/input/rmi4/rmi_f01.c
> index 381ad60..8f7840e 100644
> --- a/drivers/input/rmi4/rmi_f01.c
> +++ b/drivers/input/rmi4/rmi_f01.c
[...]
> -static int rmi_f01_initialize(struct rmi_function *fn)
> +static int rmi_f01_probe(struct rmi_function *fn)
> {
> - u8 temp;
> - int error;
> - u16 ctrl_base_addr;
> struct rmi_device *rmi_dev = fn->rmi_dev;
> struct rmi_driver_data *driver_data = dev_get_drvdata(&rmi_dev->dev);
> - struct f01_data *data = fn->data;
> - struct rmi_device_platform_data *pdata = to_rmi_platform_data(rmi_dev);
> + const struct rmi_device_platform_data *pdata =
> + to_rmi_platform_data(rmi_dev);
> + struct f01_data *f01;
> + size_t f01_size;
> + int error;
> + u16 ctrl_base_addr;
> + u8 device_status;
> + u8 temp;
> +
> + f01_size = sizeof(struct f01_data) +
> + sizeof(u8) * driver_data->num_of_irq_regs;
> + f01 = devm_kzalloc(&fn->dev, f01_size, GFP_KERNEL);
> + if (!f01) {
> + dev_err(&fn->dev, "Failed to allocate fn01_data.\n");
Nitpick: Can we drop this printout in the process? It's much less
useful than the error and backtrace coming from kmalloc on failure anyway.
> + return -ENOMEM;
> + }
[...]
> + /* XXX: why we check has_lts here but has_adjustable_doze in probe? */
Hrm. This register is poorly documented in the spec. All of these bits
are reserved. Chris, is there a newer version of the spec which
documents these bits?
-Courtney
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox