Linux Input/HID development
 help / color / mirror / Atom feed
* Hello
From: Noemi Alvarez @ 2014-07-29  8:23 UTC (permalink / raw)



I want to keep up with you with hope for friendship if you are interested.
If you don't mind i will like you to write me back. I am waiting to read
from you, because i have something important and urgent to discuss with
you. I will also send some of my beautiful photos to you.


^ permalink raw reply

* Re: [v2] input: drv260x: Add TI drv260x haptics driver
From: Michal Malý @ 2014-07-29 12:08 UTC (permalink / raw)
  To: Murphy, Dan
  Cc: simon@mungewell.org, linux-input@vger.kernel.org,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	Elias Vanderstuyft
In-Reply-To: <00FC9A978A94B7418C33AFAE8A35ED49DB1A79@DFLE09.ent.ti.com>

On Monday 28 of July 2014 21:21:26 Murphy, Dan wrote:
> Dmitry
> 
> On 07/28/2014 12:59 PM, simon@mungewell.org wrote:
> >>> The initial driver supports the devices
> >>> real time playback mode.  But the device
> >>> has additional wave patterns in ROM.
> >> 
> >> As it presented the device appears to be a memoryless device, however
> >> you present it to the rest of the system as if it can support playback
> >> of multiple effects simultaneously, which is incorrect.
> >> 
> >> My guess that you need to engage the memoryless input library to schedule
> >> handling multiple effects for your device, including ramping up, ramping
> >> down, stopping playback when effects runs out, etc.
> > 
> > Hi Dan,
> > Elias and Michal (cc'ed) are working on a kernel/userland library to
> > handle sending multiple force feedback signals to 'simple' devices,
> > perhaps you should engage with them.
> > 
> > Simon

Hi Dan,

since I spent some time trying to improve the memoryless library I can 
hopefully provide you with some hints.

If I'm reading this right, your device cannot play more than one effect at once 
but it can operate eihter in RTP mode where the user controls the operation of 
the motors directly on in an automated mode where the device follows some pre-
programmed waveform. These two modes are mutually exclusive.

If my understanding of the problem is correct, you will struggle both with 
memoryless library and your own imlementation. The memoryless library is quite 
simplistic and it doesn't have any sort of "passthrough" mode so you would not 
be able to do anything besides RTP that way.

On the other hand if you decide to write your own implementation of effect 
handling you will end up duplicating a lot of code that already exists in the 
memoryless library and has been proven to work.

A possible solution for the problem would be to separate the functions that 
control effect timing from the memoryless library and make them globally 
accessible. I believe that this would simplify any customized haptic 
implementation considerably and it might be a decent interim solution before 
the project Simon mentioned is ready.

If there is anybody interested in such a solution I can probably whip up a 
patch for it quite quickly.

Regards,
Michal

^ permalink raw reply

* Re: [PATCH 0/7] Second batch of cleanups for cros_ec
From: Andreas Färber @ 2014-07-29 12:27 UTC (permalink / raw)
  To: Javier Martinez Canillas
  Cc: Lee Jones, Wolfram Sang, Dmitry Torokhov, Doug Anderson,
	Simon Glass, Bill Richardson, Andrew Bresticker, Derek Basehore,
	Todd Broch, Olof Johansson, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
	linux-input-u79uwXL29TY76Z2rM5mHXA,
	linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1406549967-21291-1-git-send-email-javier.martinez-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>

Hi Javier,

Am 28.07.2014 14:19, schrieb Javier Martinez Canillas:
> The patches in this series are authored by different people
> (all on cc) and consist of the following:
> 
> Andrew Bresticker (3):
>   mfd: cros_ec: stop calling ->cmd_xfer() directly
>   mfd: cros_ec: move locking into cros_ec_cmd_xfer
>   mfd: cros_ec: wait for completion of commands that return IN_PROGRESS
> 
> Derek Basehore (1):
>   i2c: i2c-cros-ec-tunnel: Set retries to 3
> 
> Doug Anderson (1):
>   mfd: cros_ec: Delay for 50ms when we see EC_CMD_REBOOT_EC
> 
> Todd Broch (2):
>   mfd: cros_ec: Instantiate sub-devices from device tree
>   Input: cros_ec_keyb: Optimize ghosting algorithm.

When you submit other people's patches, I believe you still need to sign
them off as submitter. In particular when you rebased them.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

^ permalink raw reply

* Re: [PATCH 6/7] mfd: cros_ec: Instantiate sub-devices from device tree
From: Andreas Färber @ 2014-07-29 12:34 UTC (permalink / raw)
  To: Javier Martinez Canillas, Lee Jones
  Cc: Wolfram Sang, Dmitry Torokhov, Doug Anderson, Simon Glass,
	Bill Richardson, Andrew Bresticker, Derek Basehore, Todd Broch,
	Olof Johansson, linux-i2c, linux-input, linux-samsung-soc
In-Reply-To: <1406549967-21291-7-git-send-email-javier.martinez@collabora.co.uk>

Hi Javier,

Am 28.07.2014 14:19, schrieb Javier Martinez Canillas:
> From: Todd Broch <tbroch@chromium.org>
> 
> If the EC device tree node has sub-nodes, try to instantiate them as
> MFD sub-devices.  We can configure the EC features provided by the board.
> 
> Signed-off-by: Todd Broch <tbroch@chromium.org>

This is provoking the blunt question: What sub-devices is it good for?
I.e., do you have a matching DT patchset that adds such devices?

In particular I'm wondering whether that would help with the tps65090 on
Spring? It's a "power-regulator" sub-node of cros-ec in 3.8.

Thanks,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

^ permalink raw reply

* [PATCH] hid: huion: Fix sparse warnings
From: Nikolai Kondrashov @ 2014-07-29 12:50 UTC (permalink / raw)
  To: Jiri Kosina
  Cc: linux-input, DIGImend-devel, Benjamin Tissoires,
	Nikolai Kondrashov
In-Reply-To: <alpine.LNX.2.00.1407291121530.16390@pobox.suse.cz>

Fix sparse warnings in hid-huion.c by using correct buffer type for
retrieved string descriptor.

The warnings in question were:

    drivers/hid/hid-huion.c:144:44: sparse: cast to restricted __le16
    drivers/hid/hid-huion.c:145:44: sparse: cast to restricted __le16
    drivers/hid/hid-huion.c:146:51: sparse: cast to restricted __le16
    drivers/hid/hid-huion.c:147:30: sparse: cast to restricted __le16

Signed-off-by: Nikolai Kondrashov <spbnick@gmail.com>
---

Hi Jiri, Benjamin,

Thanks for the review. There is one minor problem, though. I didn't run a
sparse check and only just got a message with "sparse" warnings. This patch
fixes them.

If you wouldn't like adding a tiny patch like this, I can send another version
of the original patches. Otherwise, please feel free to just fixup my "Switch
to generating report descriptor" commit with this one.

I'll try to remember to run sparse next time before submitting the patches.

Thank you.

Nick

 drivers/hid/hid-huion.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/hid-huion.c b/drivers/hid/hid-huion.c
index 438b54e..60f44cd 100644
--- a/drivers/hid/hid-huion.c
+++ b/drivers/hid/hid-huion.c
@@ -115,7 +115,7 @@ static int huion_tablet_enable(struct hid_device *hdev)
 	int rc;
 	struct usb_device *usb_dev = hid_to_usb_dev(hdev);
 	struct huion_drvdata *drvdata = hid_get_drvdata(hdev);
-	u16 buf[6];
+	__le16 buf[6];
 
 	/*
 	 * Read string descriptor containing tablet parameters. The specific
-- 
2.0.1


^ permalink raw reply related

* Re: [PATCH 0/7] Second batch of cleanups for cros_ec
From: Javier Martinez Canillas @ 2014-07-29 12:58 UTC (permalink / raw)
  To: Andreas Färber
  Cc: Lee Jones, Wolfram Sang, Dmitry Torokhov, Doug Anderson,
	Simon Glass, Bill Richardson, Andrew Bresticker, Derek Basehore,
	Todd Broch, Olof Johansson, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
	linux-input-u79uwXL29TY76Z2rM5mHXA,
	linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <53D79328.8060609-l3A5Bk7waGM@public.gmane.org>

Hi Andreas,

On 07/29/2014 02:27 PM, Andreas Färber wrote:
> Hi Javier,
> 
> Am 28.07.2014 14:19, schrieb Javier Martinez Canillas:
>> The patches in this series are authored by different people
>> (all on cc) and consist of the following:
>> 
>> Andrew Bresticker (3):
>>   mfd: cros_ec: stop calling ->cmd_xfer() directly
>>   mfd: cros_ec: move locking into cros_ec_cmd_xfer
>>   mfd: cros_ec: wait for completion of commands that return IN_PROGRESS
>> 
>> Derek Basehore (1):
>>   i2c: i2c-cros-ec-tunnel: Set retries to 3
>> 
>> Doug Anderson (1):
>>   mfd: cros_ec: Delay for 50ms when we see EC_CMD_REBOOT_EC
>> 
>> Todd Broch (2):
>>   mfd: cros_ec: Instantiate sub-devices from device tree
>>   Input: cros_ec_keyb: Optimize ghosting algorithm.
> 
> When you submit other people's patches, I believe you still need to sign
> them off as submitter. In particular when you rebased them.
> 

You are right. I thought that given that I only rebased the patches and removed
changes for files that are not yet in mainline I shouldn't add my s-o-b.

But looking at the Developer's Certificate of Origin 1.1 rule (b) it seems that
I should had added my s-o-b as well.

> Regards,
> Andreas
> 

Thanks a lot and best regards,
Javier

^ permalink raw reply

* Re: [PATCH] hid: huion: Fix sparse warnings
From: Jiri Kosina @ 2014-07-29 13:06 UTC (permalink / raw)
  To: Nikolai Kondrashov; +Cc: linux-input, DIGImend-devel, Benjamin Tissoires
In-Reply-To: <1406638206-6108-1-git-send-email-spbnick@gmail.com>

On Tue, 29 Jul 2014, Nikolai Kondrashov wrote:

> Fix sparse warnings in hid-huion.c by using correct buffer type for
> retrieved string descriptor.
> 
> The warnings in question were:
> 
>     drivers/hid/hid-huion.c:144:44: sparse: cast to restricted __le16
>     drivers/hid/hid-huion.c:145:44: sparse: cast to restricted __le16
>     drivers/hid/hid-huion.c:146:51: sparse: cast to restricted __le16
>     drivers/hid/hid-huion.c:147:30: sparse: cast to restricted __le16

Interestingly enough, my sparse doesn't seem to report this problem 
(running sparse is part of my machinery before pushing anything out).

> Signed-off-by: Nikolai Kondrashov <spbnick@gmail.com>
> ---
> 
> Hi Jiri, Benjamin,
> 
> Thanks for the review. There is one minor problem, though. I didn't run a
> sparse check and only just got a message with "sparse" warnings. This patch
> fixes them.
> 
> If you wouldn't like adding a tiny patch like this, I can send another version
> of the original patches. Otherwise, please feel free to just fixup my "Switch
> to generating report descriptor" commit with this one.

Yup, I'll be applying this on top of your previous one.

Thanks.

-- 
Jiri Kosina
SUSE Labs

^ permalink raw reply

* Re: [PATCH 7/7] Input: cros_ec_keyb: Optimize ghosting algorithm.
From: Andreas Färber @ 2014-07-29 13:08 UTC (permalink / raw)
  To: Javier Martinez Canillas, Lee Jones
  Cc: Wolfram Sang, Dmitry Torokhov, Doug Anderson, Simon Glass,
	Bill Richardson, Andrew Bresticker, Derek Basehore, Todd Broch,
	Olof Johansson, linux-i2c, linux-input, linux-samsung-soc
In-Reply-To: <1406549967-21291-8-git-send-email-javier.martinez@collabora.co.uk>

Am 28.07.2014 14:19, schrieb Javier Martinez Canillas:
> From: Todd Broch <tbroch@chromium.org>
> 
> Previous algorithm was a bit conservative and complicating with
> respect to identifying key ghosting.  This CL uses the bitops hamming
> weight function (hweight8) to count the number of matching rows for
> colM & colN.  If that number is > 1 ghosting is present.
> 
> Additionally it removes NULL keys and our one virtual keypress
> KEY_BATTERY from consideration as these inputs are never physical
> keypresses.
> 
> Signed-off-by: Todd Broch <tbroch@chromium.org>
> Reviewed-by: Vincent Palatin <vpalatin@chromium.org>
> Reviewed-by: Luigi Semenzato <semenzato@chromium.org>

This seems to fix my Ctrl+O problems on Spring reported a while ago,

Tested-by: Andreas Färber <afaerber@suse.de>

Thanks,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

^ permalink raw reply

* Re: [PATCH] hid: huion: Fix sparse warnings
From: Nikolai Kondrashov @ 2014-07-29 13:24 UTC (permalink / raw)
  To: Jiri Kosina; +Cc: linux-input, DIGImend-devel, Benjamin Tissoires
In-Reply-To: <alpine.LNX.2.00.1407291504370.16390@pobox.suse.cz>

On 07/29/2014 04:06 PM, Jiri Kosina wrote:
> On Tue, 29 Jul 2014, Nikolai Kondrashov wrote:
>
>> Fix sparse warnings in hid-huion.c by using correct buffer type for
>> retrieved string descriptor.
>>
>> The warnings in question were:
>>
>>      drivers/hid/hid-huion.c:144:44: sparse: cast to restricted __le16
>>      drivers/hid/hid-huion.c:145:44: sparse: cast to restricted __le16
>>      drivers/hid/hid-huion.c:146:51: sparse: cast to restricted __le16
>>      drivers/hid/hid-huion.c:147:30: sparse: cast to restricted __le16
>
> Interestingly enough, my sparse doesn't seem to report this problem
> (running sparse is part of my machinery before pushing anything out).

The above only appears with CF=-D__CHECK_ENDIAN__.

>> If you wouldn't like adding a tiny patch like this, I can send another version
>> of the original patches. Otherwise, please feel free to just fixup my "Switch
>> to generating report descriptor" commit with this one.
>
> Yup, I'll be applying this on top of your previous one.

Thanks!

Nick

^ permalink raw reply

* Re: [PATCH 6/7] mfd: cros_ec: Instantiate sub-devices from device tree
From: Javier Martinez Canillas @ 2014-07-29 13:28 UTC (permalink / raw)
  To: Andreas Färber, Lee Jones
  Cc: Wolfram Sang, Dmitry Torokhov, Doug Anderson, Simon Glass,
	Bill Richardson, Andrew Bresticker, Derek Basehore, Todd Broch,
	Olof Johansson, linux-i2c, linux-input, linux-samsung-soc
In-Reply-To: <53D794E1.1090003@suse.de>

Hello Andreas,

On 07/29/2014 02:34 PM, Andreas Färber wrote:
> Hi Javier,
> 
> Am 28.07.2014 14:19, schrieb Javier Martinez Canillas:
>> From: Todd Broch <tbroch@chromium.org>
>> 
>> If the EC device tree node has sub-nodes, try to instantiate them as
>> MFD sub-devices.  We can configure the EC features provided by the board.
>> 
>> Signed-off-by: Todd Broch <tbroch@chromium.org>
> 
> This is provoking the blunt question: What sub-devices is it good for?
> I.e., do you have a matching DT patchset that adds such devices?
> 

For Peach Pit and Pi, the matching DTS changes is [0]. But answering your
question it is to instantiate the subdevices that can be child nodes of either
cros-ec-spi or cros-ec-i2c. So for the devices that are directly connected to
the EC Cortex-M through i2c.

> In particular I'm wondering whether that would help with the tps65090 on
> Spring? It's a "power-regulator" sub-node of cros-ec in 3.8.
> 

Spring is a little more complicated since the EC in Spring don't have the full
EC_CMD_I2C_PASSTHRU. So the downstream Chrome OS 3.8 kernel has a forked
tps65090 driver (drivers/regulator/cros_ec-tps65090.c) that talks directly with
the cros_ec MFD driver, you can get more info from [1] in the "About Spring"
section.

> Thanks,
> Andreas
> 

Best regards,
Javier

[0]:
https://git.kernel.org/cgit/linux/kernel/git/kgene/linux-samsung.git/commit/?h=for-next&id=8060098bbb564d27a287057a93d4fe3bfd266290
[1]: http://code.google.com/p/chromium/issues/detail?id=391797

^ permalink raw reply

* Re: [v2] input: drv260x: Add TI drv260x haptics driver
From: Murphy, Dan @ 2014-07-29 13:41 UTC (permalink / raw)
  To: Michal Malý
  Cc: simon@mungewell.org, linux-input@vger.kernel.org,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
	Elias Vanderstuyft
In-Reply-To: <2696156.zc98nP9bBH@sigyn>

Michal

Thanks for the feedback

On 07/29/2014 07:09 AM, Michal Malý wrote:
> On Monday 28 of July 2014 21:21:26 Murphy, Dan wrote:
>> Dmitry
>>
>> On 07/28/2014 12:59 PM, simon@mungewell.org wrote:
>>>>> The initial driver supports the devices
>>>>> real time playback mode.  But the device
>>>>> has additional wave patterns in ROM.
>>>>
>>>> As it presented the device appears to be a memoryless device, however
>>>> you present it to the rest of the system as if it can support playback
>>>> of multiple effects simultaneously, which is incorrect.
>>>>
>>>> My guess that you need to engage the memoryless input library to schedule
>>>> handling multiple effects for your device, including ramping up, ramping
>>>> down, stopping playback when effects runs out, etc.
>>>
>>> Hi Dan,
>>> Elias and Michal (cc'ed) are working on a kernel/userland library to
>>> handle sending multiple force feedback signals to 'simple' devices,
>>> perhaps you should engage with them.
>>>
>>> Simon
> 
> Hi Dan,
> 
> since I spent some time trying to improve the memoryless library I can 
> hopefully provide you with some hints.
> 
> If I'm reading this right, your device cannot play more than one effect at once 
> but it can operate eihter in RTP mode where the user controls the operation of 
> the motors directly on in an automated mode where the device follows some pre-
> programmed waveform. These two modes are mutually exclusive.

Actually there is one other mode here, that is the ability to program in a
waveform to be played.  So that makes 3 modes I have to think about supporting.

I know there are products out there that would like to and do customize their feedback
based on certain user actions.

For the custom mode I could use the FF_CUSTOM id but not sure
how that fits into the Periodic structure.  If I can get two out of 3 that would be
great.

> 
> If my understanding of the problem is correct, you will struggle both with 
> memoryless library and your own imlementation. The memoryless library is quite 
> simplistic and it doesn't have any sort of "passthrough" mode so you would not 
> be able to do anything besides RTP that way.
> 
> On the other hand if you decide to write your own implementation of effect 
> handling you will end up duplicating a lot of code that already exists in the 
> memoryless library and has been proven to work.
> 
> A possible solution for the problem would be to separate the functions that 
> control effect timing from the memoryless library and make them globally 
> accessible. I believe that this would simplify any customized haptic 
> implementation considerably and it might be a decent interim solution before 
> the project Simon mentioned is ready.

How far are you on finishing the project?
If it is only a little bit out I can just keep with the memless RTP mode
as a start and then implement the other functions once the solution is accepted.

Dan

> 
> If there is anybody interested in such a solution I can probably whip up a 
> patch for it quite quickly.
> 
> Regards,
> Michal
> 


-- 
------------------
Dan Murphy
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [v2] input: drv260x: Add TI drv260x haptics driver
From: Murphy, Dan @ 2014-07-29 13:50 UTC (permalink / raw)
  To: Mark Rutland
  Cc: linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
	devicetree@vger.kernel.org, dmitry.torokhov@gmail.com
In-Reply-To: <20140729093701.GJ2576@leverpostej>

Mark

Thanks for the feedback

On 07/29/2014 04:37 AM, Mark Rutland wrote:
> On Mon, Jul 28, 2014 at 05:53:23PM +0100, Dan Murphy wrote:
>> Add the TI drv260x haptics/vibrator driver.
>> This device uses the input force feedback
>> to produce a wave form to driver an
>> ERM or LRA actuator device.
>>
>> The initial driver supports the devices
>> real time playback mode.  But the device
>> has additional wave patterns in ROM.
>>
>> This functionality will be added in
>> future patchsets.
>>
>> Product data sheet is located here:
>> http://www.ti.com/product/drv2605
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>>
>> v2 - Fixed binding doc and patch headline - https://patchwork.kernel.org/patch/4619641/
>>
>>  .../devicetree/bindings/input/ti,drv260x.txt       |   44 ++
>>  drivers/input/misc/Kconfig                         |    9 +
>>  drivers/input/misc/Makefile                        |    1 +
>>  drivers/input/misc/drv260x.c                       |  537 ++++++++++++++++++++
>>  include/dt-bindings/input/ti-drv260x.h             |   30 ++
>>  include/linux/input/drv260x.h                      |  181 +++++++
>>  6 files changed, 802 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/input/ti,drv260x.txt
>>  create mode 100644 drivers/input/misc/drv260x.c
>>  create mode 100644 include/dt-bindings/input/ti-drv260x.h
>>  create mode 100644 include/linux/input/drv260x.h
>>
>> diff --git a/Documentation/devicetree/bindings/input/ti,drv260x.txt b/Documentation/devicetree/bindings/input/ti,drv260x.txt
>> new file mode 100644
>> index 0000000..930429b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/input/ti,drv260x.txt
>> @@ -0,0 +1,44 @@
>> +Texas Instruments - drv260x Haptics driver family
>> +
>> +The drv260x family serial control bus communicates through I2C protocols
>> +
>> +Required properties:
>> +       - compatible - One of:
>> +               "ti,drv2604" - DRV2604
>> +               "ti,drv2605" - DRV2605
>> +               "ti,drv2605l" - DRV2605L
>> +       - reg -  I2C slave address
>> +       - mode - Power up mode of the chip (defined in include/dt-bindings/input/ti-drv260x.h)
>> +               DRV260X_RTP_MODE - Real time playback mode
>> +               DRV260X_LRA_MODE - Linear Resonance Actuator mode (Piezoelectric)
>> +               DRV260X_ERM_MODE - Eccentric Rotating Mass mode (Rotary vibrator)
> 
> What exactly are these, and why does the kernel not decide?

The LRA and ERM are vibrator types.  These are specific to the product and the kernel cannot decide.
The kernel driver needs to be told what type of vibrator is connected.  The driver then
sets the voltages and other settings to drive the vibrator type safely. All vibrators are dumb and do not
have IDs identifying themselves so there is no self discovery here.

RTP is not a vibrator type but just a mode you can place the device into.  I can probably remove that
and have the user space or driver take care of that setting.

> 
>> +       - library_sel - Library to use at power up (defined in include/dt-bindings/input/ti-drv260x.h)
> 
> Please s/_/-/ in all property names.
> 
>> +               DRV260X_LIB_A - Pre-programmed Library
>> +               DRV260X_LIB_B - Pre-programmed Library
>> +               DRV260X_LIB_C - Pre-programmed Library
>> +               DRV260X_LIB_D - Pre-programmed Library
>> +               DRV260X_LIB_E - Pre-programmed Library
>> +               DRV260X_LIB_F - Pre-programmed Library
> 
> What exactly are these, and why does the kernel not decide?
> 
>> +
>> +Optional properties:
>> +       - enable-gpio - gpio pin to enable/disable the device.
>> +       - vib_rated_voltage - The rated voltage of the actuator.  If this is not
>> +                                                 set then the value will be defaulted to 0x90 in the
>> +                                                 driver.
> 
> What units is this in?

Good catch I was thinking this when I was writing the documentation but forgot to add
it.  I will fix the formatting here and below as well.

> 
> Don't mention the driver, just say "if not 0x90" (but with a better
> description of what 0x90 actually means).
> 

OK

>> +       - vib_overdrive_voltage - The overdrive voltage of the actuator.
>> +                                                       If this is not set then the value
>> +                                                       will be defaulted to 0x90 in the driver.
> 
> Similarly on both points.
> 
> [...]
> 

Same as above as well

>> +static int drv260x_probe(struct i2c_client *client,
>> +                          const struct i2c_device_id *id)
>> +{
>> +       struct drv260x_data *haptics;
>> +       struct device_node *np = client->dev.of_node;
>> +       struct drv260x_platform_data *pdata = client->dev.platform_data;
>> +       struct ff_device *ff;
>> +       int ret;
>> +       int voltage;
>> +
>> +       haptics = devm_kzalloc(&client->dev, sizeof(*haptics), GFP_KERNEL);
>> +       if (!haptics)
>> +               return -ENOMEM;
>> +
>> +       haptics->rated_voltage = DRV260X_DEF_OD_CLAMP_VOLT;
>> +       haptics->rated_voltage = DRV260X_DEF_RATED_VOLT;
>> +
>> +       if (np) {
>> +               ret = of_property_read_u32(np, "mode", &haptics->mode);
>> +               if (ret < 0) {
>> +                       dev_err(&client->dev,
>> +                               "%s: No entry for mode\n", __func__);
>> +
>> +                       return ret;
>> +               }
> 
> No sanity checking on the actual value?
> 
> As far as I can see, haptics->mode is an int, not a u32.
> 

Will fix this and all points below.

>> +               ret = of_property_read_u32(np, "library_sel",
>> +                                       &haptics->library);
>> +               if (ret < 0) {
>> +                       dev_err(&client->dev,
>> +                               "%s: No entry for library selection\n", __func__);
>> +
>> +                       return ret;
>> +               }
> 
> Sanity checking?
> 
> haptics->library is not a u32.
> 
>> +               ret = of_property_read_u32(np, "vib_rated_voltage",
>> +                                       &voltage);
>> +               if (!ret)
>> +                       haptics->rated_voltage = drv260x_calculate_voltage(voltage);
>> +
>> +
>> +               ret = of_property_read_u32(np, "vib_overdrive_voltage",
>> +                                       &voltage);
>> +               if (!ret)
>> +                       haptics->overdrive_voltage = drv260x_calculate_voltage(voltage);
>> +
> 
> And again, on both points.
> 
>> +       } else if (pdata) {
>> +               haptics->mode = pdata->mode;
>> +               haptics->library = pdata->library_selection;
>> +               if (pdata->vib_overdrive_voltage)
>> +                       haptics->overdrive_voltage = drv260x_calculate_voltage(pdata->vib_overdrive_voltage);
>> +               if (pdata->vib_rated_voltage)
>> +                       haptics->rated_voltage = drv260x_calculate_voltage(pdata->vib_rated_voltage);
>> +       } else {
>> +               dev_err(&client->dev, "Platform data not set\n");
>> +               return -ENODEV;
>> +       }
>> +
>> +       haptics->regulator = regulator_get(&client->dev, "vbat");
> 
> This wasn't in the binding.

Yep will add it.


Dan

> 
> Thanks,
> Mark.
> 


-- 
------------------
Dan Murphy

^ permalink raw reply

* Re: [v2] input: drv260x: Add TI drv260x haptics driver
From: Mark Rutland @ 2014-07-29 14:39 UTC (permalink / raw)
  To: Murphy, Dan
  Cc: linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
In-Reply-To: <00FC9A978A94B7418C33AFAE8A35ED49DB2AAD-l8PMxShYob2IQmiDNMet8wC/G2K4zDHf@public.gmane.org>

On Tue, Jul 29, 2014 at 02:50:48PM +0100, Murphy, Dan wrote:
> Mark
> 
> Thanks for the feedback
> 
> On 07/29/2014 04:37 AM, Mark Rutland wrote:
> > On Mon, Jul 28, 2014 at 05:53:23PM +0100, Dan Murphy wrote:
> >> Add the TI drv260x haptics/vibrator driver.
> >> This device uses the input force feedback
> >> to produce a wave form to driver an
> >> ERM or LRA actuator device.
> >>
> >> The initial driver supports the devices
> >> real time playback mode.  But the device
> >> has additional wave patterns in ROM.
> >>
> >> This functionality will be added in
> >> future patchsets.
> >>
> >> Product data sheet is located here:
> >> http://www.ti.com/product/drv2605
> >>
> >> Signed-off-by: Dan Murphy <dmurphy-l0cyMroinI0@public.gmane.org>
> >> ---
> >>
> >> v2 - Fixed binding doc and patch headline - https://patchwork.kernel.org/patch/4619641/
> >>
> >>  .../devicetree/bindings/input/ti,drv260x.txt       |   44 ++
> >>  drivers/input/misc/Kconfig                         |    9 +
> >>  drivers/input/misc/Makefile                        |    1 +
> >>  drivers/input/misc/drv260x.c                       |  537 ++++++++++++++++++++
> >>  include/dt-bindings/input/ti-drv260x.h             |   30 ++
> >>  include/linux/input/drv260x.h                      |  181 +++++++
> >>  6 files changed, 802 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/input/ti,drv260x.txt
> >>  create mode 100644 drivers/input/misc/drv260x.c
> >>  create mode 100644 include/dt-bindings/input/ti-drv260x.h
> >>  create mode 100644 include/linux/input/drv260x.h
> >>
> >> diff --git a/Documentation/devicetree/bindings/input/ti,drv260x.txt b/Documentation/devicetree/bindings/input/ti,drv260x.txt
> >> new file mode 100644
> >> index 0000000..930429b
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/input/ti,drv260x.txt
> >> @@ -0,0 +1,44 @@
> >> +Texas Instruments - drv260x Haptics driver family
> >> +
> >> +The drv260x family serial control bus communicates through I2C protocols
> >> +
> >> +Required properties:
> >> +       - compatible - One of:
> >> +               "ti,drv2604" - DRV2604
> >> +               "ti,drv2605" - DRV2605
> >> +               "ti,drv2605l" - DRV2605L
> >> +       - reg -  I2C slave address
> >> +       - mode - Power up mode of the chip (defined in include/dt-bindings/input/ti-drv260x.h)
> >> +               DRV260X_RTP_MODE - Real time playback mode
> >> +               DRV260X_LRA_MODE - Linear Resonance Actuator mode (Piezoelectric)
> >> +               DRV260X_ERM_MODE - Eccentric Rotating Mass mode (Rotary vibrator)
> > 
> > What exactly are these, and why does the kernel not decide?
> 
> The LRA and ERM are vibrator types.  These are specific to the product and the kernel cannot decide.
> The kernel driver needs to be told what type of vibrator is connected.  The driver then
> sets the voltages and other settings to drive the vibrator type safely. All vibrators are dumb and do not
> have IDs identifying themselves so there is no self discovery here.

I see. That makes sense, thanks for the description.

It might make more sense to call this "vibrator-type", rather than
"mode", but otherwise this should be ok.

In the past I've asked for strings rather than magic numbers, but I'll
leave that choice to you.

> RTP is not a vibrator type but just a mode you can place the device into.  I can probably remove that
> and have the user space or driver take care of that setting.

Ok, that sounds good to me.

> 
> > 
> >> +       - library_sel - Library to use at power up (defined in include/dt-bindings/input/ti-drv260x.h)
> > 
> > Please s/_/-/ in all property names.
> > 
> >> +               DRV260X_LIB_A - Pre-programmed Library
> >> +               DRV260X_LIB_B - Pre-programmed Library
> >> +               DRV260X_LIB_C - Pre-programmed Library
> >> +               DRV260X_LIB_D - Pre-programmed Library
> >> +               DRV260X_LIB_E - Pre-programmed Library
> >> +               DRV260X_LIB_F - Pre-programmed Library
> > 
> > What exactly are these, and why does the kernel not decide?
> > 
> >> +
> >> +Optional properties:
> >> +       - enable-gpio - gpio pin to enable/disable the device.
> >> +       - vib_rated_voltage - The rated voltage of the actuator.  If this is not
> >> +                                                 set then the value will be defaulted to 0x90 in the
> >> +                                                 driver.
> > 
> > What units is this in?
> 
> Good catch I was thinking this when I was writing the documentation but forgot to add
> it.  I will fix the formatting here and below as well.

Cheers.

Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: Trouble with an HP stylus
From: Jiri Kosina @ 2014-07-29 14:41 UTC (permalink / raw)
  To: Éric Brunet; +Cc: linux-input
In-Reply-To: <alpine.LNX.2.00.1407291147190.16390@pobox.suse.cz>

On Tue, 29 Jul 2014, Jiri Kosina wrote:

> Does the patch below fix the problem for you please?
> 
> 
> 
> 
> diff --git a/drivers/hid/usbhid/hid-quirks.c b/drivers/hid/usbhid/hid-quirks.c
> index 31e6727..d48574d 100644
> --- a/drivers/hid/usbhid/hid-quirks.c
> +++ b/drivers/hid/usbhid/hid-quirks.c
> @@ -97,6 +97,7 @@ static const struct hid_blacklist {
>  	{ USB_VENDOR_ID_SYMBOL, USB_DEVICE_ID_SYMBOL_SCANNER_2, HID_QUIRK_NOGET },
>  	{ USB_VENDOR_ID_TPV, USB_DEVICE_ID_TPV_OPTICAL_TOUCHSCREEN, HID_QUIRK_NOGET },
>  	{ USB_VENDOR_ID_TURBOX, USB_DEVICE_ID_TURBOX_KEYBOARD, HID_QUIRK_NOGET },
> +	{ 0x03EB, 0x840B, HID_QUIRK_MULTI_INPUT },
>  	{ USB_VENDOR_ID_UCLOGIC, USB_DEVICE_ID_UCLOGIC_TABLET_PF1209, HID_QUIRK_MULTI_INPUT },
>  	{ USB_VENDOR_ID_UCLOGIC, USB_DEVICE_ID_UCLOGIC_TABLET_WP4030U, HID_QUIRK_MULTI_INPUT },
>  	{ USB_VENDOR_ID_UCLOGIC, USB_DEVICE_ID_UCLOGIC_TABLET_KNA5, HID_QUIRK_MULTI_INPUT },

Alternatively, if you don't want to recompile your kernel, you can achieve 
the equivalent result by modprobing the 'usbhid' module with 
'quirks=0x03EB:0x840B:0x40' module parameter.

-- 
Jiri Kosina
SUSE Labs

^ permalink raw reply

* Re: Trouble with an HP stylus
From: Benjamin Tissoires @ 2014-07-29 14:45 UTC (permalink / raw)
  To: Jiri Kosina; +Cc: Éric Brunet, linux-input
In-Reply-To: <alpine.LNX.2.00.1407291147190.16390@pobox.suse.cz>

Hi Jiri, Eric,


On Tue, Jul 29, 2014 at 5:50 AM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Thu, 24 Jul 2014, Éric Brunet wrote:
>
>> I have a HP EliteBook Revolve 810 (which is a hybrid laptop/tablet) with
>> the HP active pen that goes with it. I have installed fedora 20 (kernel
>> 3.15.6-200.fc20.x86_64) on this, and the pen does not work out of the box.
>> Playing with evtest, it appears that the kernel reports the X coordinate in
>> ABS_X and in ABS_Y, while the Y coordinate goes into ABS_Z and ABS_RX; here
>> are a few lines of output:
>>
>> Event: time 1406153996.865044, -------------- SYN_REPORT ------------
>> Event: time 1406153996.873047, type 3 (EV_ABS), code 0 (ABS_X), value 1087
>> Event: time 1406153996.873047, type 3 (EV_ABS), code 1 (ABS_Y), value 1087
>> Event: time 1406153996.873047, type 3 (EV_ABS), code 2 (ABS_Z), value 1599
>> Event: time 1406153996.873047, type 3 (EV_ABS), code 3 (ABS_RX), value 1599
>>
>> Of course, XOrg's evdev driver is confused and the pointer is stuck on the
>> diagonal X=Y.
>>
>> I guess this hardware needs a new quirk to usbhid, unless there is already
>> a way to correct it ?
>>
>> Here is the content of /sys/class/input/event8/device/uevent:
>>
>> PRODUCT=3/3eb/840b/111
>> NAME="Atmel Atmel maXTouch Digitizer Pen"
>> PHYS="usb-0000:00:1a.0-1.1/input0"
>> UNIQ=""
>> PROP=0
>> EV=1b
>> KEY=c01 1 0 0 0 0
>> ABS=100000f
>> MSC=10
>> MODALIAS=input:b0003v03EBp840Be0111-
>> e0,1,3,4,k100,140,14A,14B,ra0,1,2,3,18,m4,lsfw
>>
>> I hope I gave enough information on this. I don't read the mailing list,
>> please CC any answer.
>
> Does the patch below fix the problem for you please?
>

I have already seen this Atmel panel, and the problem was that they used twice X then twice Y to report coordinates:

0x05, 0x01,                    //     Usage Page (Generic Desktop)    590
0x55, 0x0e,                    //     Unit Exponent (-2)              592
0x65, 0x11,                    //     Unit (Centimeter,SILinear)      594
0x35, 0x00,                    //     Physical Minimum (0)            596
0x75, 0x10,                    //     Report Size (16)                598
0x95, 0x02,                    //     Report Count (2)                600
0x46, 0x00, 0x0a,              //     Physical Maximum (2560)         602
0x26, 0xff, 0x0f,              //     Logical Maximum (4095)          605
0x09, 0x30,                    //     Usage (X)                       608
0x81, 0x02,                    //     Input (Data,Var,Abs)            610
0x46, 0xa0, 0x05,              //     Physical Maximum (1440)         612
0x26, 0xff, 0x0f,              //     Logical Maximum (4095)          615
0x09, 0x31,                    //     Usage (Y)                       618
0x81, 0x02,                    //     Input (Data,Var,Abs)            620

I have no ideas why they did that, but this is a pretty bad idea. I thought at the time that it was just a prototype and that they would fix it for the release, but I was wrong.

I think, a better patch would simply consist in that:

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 2619f7f..b70d4c3 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -603,7 +603,7 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
                        if (field->flags & HID_MAIN_ITEM_RELATIVE)
                                map_rel(usage->hid & 0xf);
                        else
-                               map_abs(usage->hid & 0xf);
+                               map_abs_clear(usage->hid & 0xf);
                        break;
 
                case HID_GD_HATSWITCH:

-----

I have the report descriptors and some traces of it, so I could definitively make some more tests, but I will have to first finish the wacom bits I scheduled for today.

Eric, if you can give a shot to my patch (you can try Jiri's too, but I don't think it will work), that would simplify my process, and I will just have to reformat/submit it properly.

Cheers,
Benjamin
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related

* [PATCH 00/12] HID: Convert UHID to new HID transport-layer
From: David Herrmann @ 2014-07-29 15:14 UTC (permalink / raw)
  To: linux-input; +Cc: Jiri Kosina, Benjamin Tissoires, David Herrmann

Hi

When Benjamin cleanup up the transport layer, he left UHID mostly unconverted
due to ABI issues. I promised to take a look, so here're the patches.

Most of the patches are fairly straightforward. Comments welcome. I tested an
old uhid-sample.c build and it still works, so I think I didn't break
backwards-compatibility.

Thanks
David

David Herrmann (12):
  HID: uhid: simplify report-cb shutdown
  HID: uhid: forward create_req to create2_req
  HID: uhid: avoid dangling pointers in uhid context
  HID: uhid: avoid magic-numbers when setting strings
  HID: uhid: turn report_id into u32
  HID: uhid: invert report_done and make non-atomic
  HID: uhid: add ABI compatible UHID_GET_REPORT replacing UHID_FEATURE
  HID: uhid: keep legacy definitions at the bottom of uhid.h
  HID: uhid: rename uhid_raw_request to uhid_hid_raw_request
  HID: uhid: implement SET_REPORT
  HID: uhid: report to user-space whether reports are numbered
  HID: uhid: update documentation

 Documentation/hid/uhid.txt | 179 ++++++++++----------
 drivers/hid/uhid.c         | 394 +++++++++++++++++++++++++--------------------
 include/uapi/linux/uhid.h  | 120 +++++++++++---
 3 files changed, 408 insertions(+), 285 deletions(-)

-- 
2.0.3


^ permalink raw reply

* [PATCH 01/12] HID: uhid: simplify report-cb shutdown
From: David Herrmann @ 2014-07-29 15:14 UTC (permalink / raw)
  To: linux-input; +Cc: Jiri Kosina, Benjamin Tissoires, David Herrmann
In-Reply-To: <1406646866-999-1-git-send-email-dh.herrmann@gmail.com>

The report-query is blocking, so when user-space destroys a device we have
to wake up any blocking kernel context that is currently in the report-cb.
We used some broken correlation between @report_done and @running so far.
Replace it by a much more obvious use.

We now wake up the report-cb if either @report_done or @running is set.
wake_up() and wait_event() serve as implicit barriers (as they always do)
so no need to use smp_rmb/wmb directly.

Note that @report_done is never reset by anyone but the report-cb, thus
it cannot flip twice while we wait for it. And whenever we set @running,
we afterwards synchronously remove the HID device. Therefore, we wait for
all report-cbs to finish before we return. This way, @running can never
flip to true while we wait for it.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/hid/uhid.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index 0cb92e3..16af4d3 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -172,13 +172,9 @@ static int uhid_hid_get_raw(struct hid_device *hid, unsigned char rnum,
 	spin_unlock_irqrestore(&uhid->qlock, flags);
 
 	ret = wait_event_interruptible_timeout(uhid->report_wait,
-				atomic_read(&uhid->report_done), 5 * HZ);
+			atomic_read(&uhid->report_done) || !uhid->running,
+			5 * HZ);
 
-	/*
-	 * Make sure "uhid->running" is cleared on shutdown before
-	 * "uhid->report_done" is set.
-	 */
-	smp_rmb();
 	if (!ret || !uhid->running) {
 		ret = -EIO;
 	} else if (ret < 0) {
@@ -493,10 +489,7 @@ static int uhid_dev_destroy(struct uhid_device *uhid)
 	if (!uhid->running)
 		return -EINVAL;
 
-	/* clear "running" before setting "report_done" */
 	uhid->running = false;
-	smp_wmb();
-	atomic_set(&uhid->report_done, 1);
 	wake_up_interruptible(&uhid->report_wait);
 
 	hid_destroy_device(uhid->hid);
-- 
2.0.3


^ permalink raw reply related

* [PATCH 02/12] HID: uhid: forward create_req to create2_req
From: David Herrmann @ 2014-07-29 15:14 UTC (permalink / raw)
  To: linux-input; +Cc: Jiri Kosina, Benjamin Tissoires, David Herrmann
In-Reply-To: <1406646866-999-1-git-send-email-dh.herrmann@gmail.com>

Instead of hard-coding the uhid_dev_create() function twice, copy any
create_req into a create2_req structure and forward it.

We allocate uhid_create_req on the stack here, but that should be fine.
Unlike uhid_create2_req it is fairly small (<1KB) and it's only used
temporarily to swap entries. uhid_dev_create2() doesn't access it.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/hid/uhid.c | 90 +++++++++++++++---------------------------------------
 1 file changed, 25 insertions(+), 65 deletions(-)

diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index 16af4d3..c05b544 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -359,71 +359,6 @@ static int uhid_event_from_user(const char __user *buffer, size_t len,
 }
 #endif
 
-static int uhid_dev_create(struct uhid_device *uhid,
-			   const struct uhid_event *ev)
-{
-	struct hid_device *hid;
-	int ret;
-
-	if (uhid->running)
-		return -EALREADY;
-
-	uhid->rd_size = ev->u.create.rd_size;
-	if (uhid->rd_size <= 0 || uhid->rd_size > HID_MAX_DESCRIPTOR_SIZE)
-		return -EINVAL;
-
-	uhid->rd_data = kmalloc(uhid->rd_size, GFP_KERNEL);
-	if (!uhid->rd_data)
-		return -ENOMEM;
-
-	if (copy_from_user(uhid->rd_data, ev->u.create.rd_data,
-			   uhid->rd_size)) {
-		ret = -EFAULT;
-		goto err_free;
-	}
-
-	hid = hid_allocate_device();
-	if (IS_ERR(hid)) {
-		ret = PTR_ERR(hid);
-		goto err_free;
-	}
-
-	strncpy(hid->name, ev->u.create.name, 127);
-	hid->name[127] = 0;
-	strncpy(hid->phys, ev->u.create.phys, 63);
-	hid->phys[63] = 0;
-	strncpy(hid->uniq, ev->u.create.uniq, 63);
-	hid->uniq[63] = 0;
-
-	hid->ll_driver = &uhid_hid_driver;
-	hid->bus = ev->u.create.bus;
-	hid->vendor = ev->u.create.vendor;
-	hid->product = ev->u.create.product;
-	hid->version = ev->u.create.version;
-	hid->country = ev->u.create.country;
-	hid->driver_data = uhid;
-	hid->dev.parent = uhid_misc.this_device;
-
-	uhid->hid = hid;
-	uhid->running = true;
-
-	ret = hid_add_device(hid);
-	if (ret) {
-		hid_err(hid, "Cannot register HID device\n");
-		goto err_hid;
-	}
-
-	return 0;
-
-err_hid:
-	hid_destroy_device(hid);
-	uhid->hid = NULL;
-	uhid->running = false;
-err_free:
-	kfree(uhid->rd_data);
-	return ret;
-}
-
 static int uhid_dev_create2(struct uhid_device *uhid,
 			    const struct uhid_event *ev)
 {
@@ -484,6 +419,31 @@ err_free:
 	return ret;
 }
 
+static int uhid_dev_create(struct uhid_device *uhid,
+			   struct uhid_event *ev)
+{
+	struct uhid_create_req orig;
+
+	orig = ev->u.create;
+
+	if (orig.rd_size <= 0 || orig.rd_size > HID_MAX_DESCRIPTOR_SIZE)
+		return -EINVAL;
+	if (copy_from_user(&ev->u.create2.rd_data, orig.rd_data, orig.rd_size))
+		return -EFAULT;
+
+	memcpy(ev->u.create2.name, orig.name, sizeof(orig.name));
+	memcpy(ev->u.create2.phys, orig.phys, sizeof(orig.phys));
+	memcpy(ev->u.create2.uniq, orig.uniq, sizeof(orig.uniq));
+	ev->u.create2.rd_size = orig.rd_size;
+	ev->u.create2.bus = orig.bus;
+	ev->u.create2.vendor = orig.vendor;
+	ev->u.create2.product = orig.product;
+	ev->u.create2.version = orig.version;
+	ev->u.create2.country = orig.country;
+
+	return uhid_dev_create2(uhid, ev);
+}
+
 static int uhid_dev_destroy(struct uhid_device *uhid)
 {
 	if (!uhid->running)
-- 
2.0.3


^ permalink raw reply related

* [PATCH 03/12] HID: uhid: avoid dangling pointers in uhid context
From: David Herrmann @ 2014-07-29 15:14 UTC (permalink / raw)
  To: linux-input; +Cc: Jiri Kosina, Benjamin Tissoires, David Herrmann
In-Reply-To: <1406646866-999-1-git-send-email-dh.herrmann@gmail.com>

Avoid keeping uhid->rd_data and uhid->rd_size set in case
uhid_dev_create2() fails. This is non-critical as we never flip
uhid->running and thus never enter uhid_dev_destroy(). However, it's much
nicer for debugging if pointers are only set if they point to valid data.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/hid/uhid.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index c05b544..bf13746 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -363,20 +363,24 @@ static int uhid_dev_create2(struct uhid_device *uhid,
 			    const struct uhid_event *ev)
 {
 	struct hid_device *hid;
+	size_t rd_size;
+	void *rd_data;
 	int ret;
 
 	if (uhid->running)
 		return -EALREADY;
 
-	uhid->rd_size = ev->u.create2.rd_size;
-	if (uhid->rd_size <= 0 || uhid->rd_size > HID_MAX_DESCRIPTOR_SIZE)
+	rd_size = ev->u.create2.rd_size;
+	if (rd_size <= 0 || rd_size > HID_MAX_DESCRIPTOR_SIZE)
 		return -EINVAL;
 
-	uhid->rd_data = kmemdup(ev->u.create2.rd_data, uhid->rd_size,
-				GFP_KERNEL);
-	if (!uhid->rd_data)
+	rd_data = kmemdup(ev->u.create2.rd_data, rd_size, GFP_KERNEL);
+	if (!rd_data)
 		return -ENOMEM;
 
+	uhid->rd_size = rd_size;
+	uhid->rd_data = rd_data;
+
 	hid = hid_allocate_device();
 	if (IS_ERR(hid)) {
 		ret = PTR_ERR(hid);
@@ -416,6 +420,8 @@ err_hid:
 	uhid->running = false;
 err_free:
 	kfree(uhid->rd_data);
+	uhid->rd_data = NULL;
+	uhid->rd_size = 0;
 	return ret;
 }
 
-- 
2.0.3


^ permalink raw reply related

* [PATCH 04/12] HID: uhid: avoid magic-numbers when setting strings
From: David Herrmann @ 2014-07-29 15:14 UTC (permalink / raw)
  To: linux-input; +Cc: Jiri Kosina, Benjamin Tissoires, David Herrmann
In-Reply-To: <1406646866-999-1-git-send-email-dh.herrmann@gmail.com>

Avoid hard-coding the target buffer sizes and use sizeof() instead. This
also makes us future-proof to buffer-extensions later on.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/hid/uhid.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index bf13746..5dee8bd 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -363,7 +363,7 @@ static int uhid_dev_create2(struct uhid_device *uhid,
 			    const struct uhid_event *ev)
 {
 	struct hid_device *hid;
-	size_t rd_size;
+	size_t rd_size, len;
 	void *rd_data;
 	int ret;
 
@@ -387,12 +387,12 @@ static int uhid_dev_create2(struct uhid_device *uhid,
 		goto err_free;
 	}
 
-	strncpy(hid->name, ev->u.create2.name, 127);
-	hid->name[127] = 0;
-	strncpy(hid->phys, ev->u.create2.phys, 63);
-	hid->phys[63] = 0;
-	strncpy(hid->uniq, ev->u.create2.uniq, 63);
-	hid->uniq[63] = 0;
+	len = min(sizeof(hid->name), sizeof(ev->u.create2.name)) - 1;
+	strncpy(hid->name, ev->u.create2.name, len);
+	len = min(sizeof(hid->phys), sizeof(ev->u.create2.phys)) - 1;
+	strncpy(hid->phys, ev->u.create2.phys, len);
+	len = min(sizeof(hid->uniq), sizeof(ev->u.create2.uniq)) - 1;
+	strncpy(hid->uniq, ev->u.create2.uniq, len);
 
 	hid->ll_driver = &uhid_hid_driver;
 	hid->bus = ev->u.create2.bus;
-- 
2.0.3


^ permalink raw reply related

* [PATCH 05/12] HID: uhid: turn report_id into u32
From: David Herrmann @ 2014-07-29 15:14 UTC (permalink / raw)
  To: linux-input; +Cc: Jiri Kosina, Benjamin Tissoires, David Herrmann
In-Reply-To: <1406646866-999-1-git-send-email-dh.herrmann@gmail.com>

All accesses to @report_id are protected by @qlock. No need to use an
atomic.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/hid/uhid.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index 5dee8bd..db4e119 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -44,10 +44,11 @@ struct uhid_device {
 	__u8 tail;
 	struct uhid_event *outq[UHID_BUFSIZE];
 
+	/* blocking GET_REPORT support; state changes protected by qlock */
 	struct mutex report_lock;
 	wait_queue_head_t report_wait;
 	atomic_t report_done;
-	atomic_t report_id;
+	u32 report_id;
 	struct uhid_event report_buf;
 };
 
@@ -163,7 +164,7 @@ static int uhid_hid_get_raw(struct hid_device *hid, unsigned char rnum,
 
 	spin_lock_irqsave(&uhid->qlock, flags);
 	ev->type = UHID_FEATURE;
-	ev->u.feature.id = atomic_inc_return(&uhid->report_id);
+	ev->u.feature.id = ++uhid->report_id;
 	ev->u.feature.rnum = rnum;
 	ev->u.feature.rtype = report_type;
 
@@ -497,7 +498,7 @@ static int uhid_dev_feature_answer(struct uhid_device *uhid,
 	spin_lock_irqsave(&uhid->qlock, flags);
 
 	/* id for old report; drop it silently */
-	if (atomic_read(&uhid->report_id) != ev->u.feature_answer.id)
+	if (uhid->report_id != ev->u.feature_answer.id)
 		goto unlock;
 	if (atomic_read(&uhid->report_done))
 		goto unlock;
-- 
2.0.3


^ permalink raw reply related

* [PATCH 06/12] HID: uhid: invert report_done and make non-atomic
From: David Herrmann @ 2014-07-29 15:14 UTC (permalink / raw)
  To: linux-input; +Cc: Jiri Kosina, Benjamin Tissoires, David Herrmann
In-Reply-To: <1406646866-999-1-git-send-email-dh.herrmann@gmail.com>

All accesses to @report_done are protected by qlock (or report-contexts).
No need to use an atomic.

While at it, invert the logic and call it "report_running". This is
similar to the uhid->running field and easier to read.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/hid/uhid.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index db4e119..2d2025a 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -47,7 +47,7 @@ struct uhid_device {
 	/* blocking GET_REPORT support; state changes protected by qlock */
 	struct mutex report_lock;
 	wait_queue_head_t report_wait;
-	atomic_t report_done;
+	bool report_running;
 	u32 report_id;
 	struct uhid_event report_buf;
 };
@@ -168,13 +168,13 @@ static int uhid_hid_get_raw(struct hid_device *hid, unsigned char rnum,
 	ev->u.feature.rnum = rnum;
 	ev->u.feature.rtype = report_type;
 
-	atomic_set(&uhid->report_done, 0);
+	uhid->report_running = true;
 	uhid_queue(uhid, ev);
 	spin_unlock_irqrestore(&uhid->qlock, flags);
 
 	ret = wait_event_interruptible_timeout(uhid->report_wait,
-			atomic_read(&uhid->report_done) || !uhid->running,
-			5 * HZ);
+				!uhid->report_running || !uhid->running,
+				5 * HZ);
 
 	if (!ret || !uhid->running) {
 		ret = -EIO;
@@ -196,7 +196,7 @@ static int uhid_hid_get_raw(struct hid_device *hid, unsigned char rnum,
 		spin_unlock_irqrestore(&uhid->qlock, flags);
 	}
 
-	atomic_set(&uhid->report_done, 1);
+	uhid->report_running = false;
 
 unlock:
 	mutex_unlock(&uhid->report_lock);
@@ -500,11 +500,11 @@ static int uhid_dev_feature_answer(struct uhid_device *uhid,
 	/* id for old report; drop it silently */
 	if (uhid->report_id != ev->u.feature_answer.id)
 		goto unlock;
-	if (atomic_read(&uhid->report_done))
+	if (!uhid->report_running)
 		goto unlock;
 
 	memcpy(&uhid->report_buf, ev, sizeof(*ev));
-	atomic_set(&uhid->report_done, 1);
+	uhid->report_running = false;
 	wake_up_interruptible(&uhid->report_wait);
 
 unlock:
@@ -526,7 +526,6 @@ static int uhid_char_open(struct inode *inode, struct file *file)
 	init_waitqueue_head(&uhid->waitq);
 	init_waitqueue_head(&uhid->report_wait);
 	uhid->running = false;
-	atomic_set(&uhid->report_done, 1);
 
 	file->private_data = uhid;
 	nonseekable_open(inode, file);
-- 
2.0.3


^ permalink raw reply related

* [PATCH 07/12] HID: uhid: add ABI compatible UHID_GET_REPORT replacing UHID_FEATURE
From: David Herrmann @ 2014-07-29 15:14 UTC (permalink / raw)
  To: linux-input; +Cc: Jiri Kosina, Benjamin Tissoires, David Herrmann
In-Reply-To: <1406646866-999-1-git-send-email-dh.herrmann@gmail.com>

The old hdev->hid_get_raw_report() was broken by design. It was never
clear what kind of HW request it should trigger. Benjamin fixed that with
the core HID cleanup, though we never really adjusted uhid.

Unfortunately, our old UHID_FEATURE command was modelled around the broken
hid_get_raw_report(). We converted it silently to the new GET_REPORT and
nothing broke. Make this explicit by renaming UHID_FEATURE to
UHID_GET_REPORT and UHID_FEATURE_ANSWER to UHID_GET_REPORT_REPLY.

Note that this is 100% ABI compatible to UHID_FEATURE. This is just a
rename. But we have to keep the old definitions around to not break API.

>From now on, UHID_GET_REPORT must trigger a GET_REPORT request on the
user-space hardware layer. All the ambiguity due to the weird "feature"
name should be gone now.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/hid/uhid.c        | 28 ++++++++++++++--------------
 include/uapi/linux/uhid.h | 23 +++++++++++++++++++++--
 2 files changed, 35 insertions(+), 16 deletions(-)

diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index 2d2025a..8f5e46b 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -124,8 +124,8 @@ static int uhid_hid_parse(struct hid_device *hid)
 	return hid_parse_report(hid, uhid->rd_data, uhid->rd_size);
 }
 
-static int uhid_hid_get_raw(struct hid_device *hid, unsigned char rnum,
-			    __u8 *buf, size_t count, unsigned char rtype)
+static int uhid_hid_get_report(struct hid_device *hid, unsigned char rnum,
+			       __u8 *buf, size_t count, unsigned char rtype)
 {
 	struct uhid_device *uhid = hid->driver_data;
 	__u8 report_type;
@@ -133,7 +133,7 @@ static int uhid_hid_get_raw(struct hid_device *hid, unsigned char rnum,
 	unsigned long flags;
 	int ret;
 	size_t uninitialized_var(len);
-	struct uhid_feature_answer_req *req;
+	struct uhid_get_report_reply_req *req;
 
 	if (!uhid->running)
 		return -EIO;
@@ -163,10 +163,10 @@ static int uhid_hid_get_raw(struct hid_device *hid, unsigned char rnum,
 	}
 
 	spin_lock_irqsave(&uhid->qlock, flags);
-	ev->type = UHID_FEATURE;
-	ev->u.feature.id = ++uhid->report_id;
-	ev->u.feature.rnum = rnum;
-	ev->u.feature.rtype = report_type;
+	ev->type = UHID_GET_REPORT;
+	ev->u.get_report.id = ++uhid->report_id;
+	ev->u.get_report.rnum = rnum;
+	ev->u.get_report.rtype = report_type;
 
 	uhid->report_running = true;
 	uhid_queue(uhid, ev);
@@ -182,7 +182,7 @@ static int uhid_hid_get_raw(struct hid_device *hid, unsigned char rnum,
 		ret = -ERESTARTSYS;
 	} else {
 		spin_lock_irqsave(&uhid->qlock, flags);
-		req = &uhid->report_buf.u.feature_answer;
+		req = &uhid->report_buf.u.get_report_reply;
 
 		if (req->err) {
 			ret = -EIO;
@@ -253,7 +253,7 @@ static int uhid_raw_request(struct hid_device *hid, unsigned char reportnum,
 {
 	switch (reqtype) {
 	case HID_REQ_GET_REPORT:
-		return uhid_hid_get_raw(hid, reportnum, buf, len, rtype);
+		return uhid_hid_get_report(hid, reportnum, buf, len, rtype);
 	case HID_REQ_SET_REPORT:
 		/* TODO: implement proper SET_REPORT functionality */
 		return -ENOSYS;
@@ -487,8 +487,8 @@ static int uhid_dev_input2(struct uhid_device *uhid, struct uhid_event *ev)
 	return 0;
 }
 
-static int uhid_dev_feature_answer(struct uhid_device *uhid,
-				   struct uhid_event *ev)
+static int uhid_dev_get_report_reply(struct uhid_device *uhid,
+				     struct uhid_event *ev)
 {
 	unsigned long flags;
 
@@ -498,7 +498,7 @@ static int uhid_dev_feature_answer(struct uhid_device *uhid,
 	spin_lock_irqsave(&uhid->qlock, flags);
 
 	/* id for old report; drop it silently */
-	if (uhid->report_id != ev->u.feature_answer.id)
+	if (uhid->report_id != ev->u.get_report_reply.id)
 		goto unlock;
 	if (!uhid->report_running)
 		goto unlock;
@@ -634,8 +634,8 @@ static ssize_t uhid_char_write(struct file *file, const char __user *buffer,
 	case UHID_INPUT2:
 		ret = uhid_dev_input2(uhid, &uhid->input_buf);
 		break;
-	case UHID_FEATURE_ANSWER:
-		ret = uhid_dev_feature_answer(uhid, &uhid->input_buf);
+	case UHID_GET_REPORT_REPLY:
+		ret = uhid_dev_get_report_reply(uhid, &uhid->input_buf);
 		break;
 	default:
 		ret = -EOPNOTSUPP;
diff --git a/include/uapi/linux/uhid.h b/include/uapi/linux/uhid.h
index 1e3b09c..0a08f2b 100644
--- a/include/uapi/linux/uhid.h
+++ b/include/uapi/linux/uhid.h
@@ -33,8 +33,10 @@ enum uhid_event_type {
 	UHID_OUTPUT,
 	UHID_OUTPUT_EV,			/* obsolete! */
 	UHID_INPUT,
-	UHID_FEATURE,
-	UHID_FEATURE_ANSWER,
+	UHID_FEATURE,			/* obsolete! use UHID_GET_REPORT */
+	UHID_GET_REPORT = UHID_FEATURE,
+	UHID_FEATURE_ANSWER,		/* obsolete! use UHID_GET_REPORT_REPLY */
+	UHID_GET_REPORT_REPLY = UHID_FEATURE_ANSWER,
 	UHID_CREATE2,
 	UHID_INPUT2,
 };
@@ -98,12 +100,20 @@ struct uhid_output_ev_req {
 	__s32 value;
 } __attribute__((__packed__));
 
+/* Obsolete! Kernel uses ABI compatible UHID_GET_REPORT. */
 struct uhid_feature_req {
 	__u32 id;
 	__u8 rnum;
 	__u8 rtype;
 } __attribute__((__packed__));
 
+struct uhid_get_report_req {
+	__u32 id;
+	__u8 rnum;
+	__u8 rtype;
+} __attribute__((__packed__));
+
+/* Obsolete! Use ABI compatible UHID_GET_REPORT_REPLY. */
 struct uhid_feature_answer_req {
 	__u32 id;
 	__u16 err;
@@ -111,6 +121,13 @@ struct uhid_feature_answer_req {
 	__u8 data[UHID_DATA_MAX];
 } __attribute__((__packed__));
 
+struct uhid_get_report_reply_req {
+	__u32 id;
+	__u16 err;
+	__u16 size;
+	__u8 data[UHID_DATA_MAX];
+} __attribute__((__packed__));
+
 struct uhid_event {
 	__u32 type;
 
@@ -120,7 +137,9 @@ struct uhid_event {
 		struct uhid_output_req output;
 		struct uhid_output_ev_req output_ev;
 		struct uhid_feature_req feature;
+		struct uhid_get_report_req get_report;
 		struct uhid_feature_answer_req feature_answer;
+		struct uhid_get_report_reply_req get_report_reply;
 		struct uhid_create2_req create2;
 		struct uhid_input2_req input2;
 	} u;
-- 
2.0.3


^ permalink raw reply related

* [PATCH 08/12] HID: uhid: keep legacy definitions at the bottom of uhid.h
From: David Herrmann @ 2014-07-29 15:14 UTC (permalink / raw)
  To: linux-input; +Cc: Jiri Kosina, Benjamin Tissoires, David Herrmann
In-Reply-To: <1406646866-999-1-git-send-email-dh.herrmann@gmail.com>

Instead of inlining the legacy definitions into the main part of uhid.h,
keep them at the bottom now. This way, the API is much easier to read and
legacy requests can be looked up at a separate place.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 include/uapi/linux/uhid.h | 101 ++++++++++++++++++++++++++++------------------
 1 file changed, 62 insertions(+), 39 deletions(-)

diff --git a/include/uapi/linux/uhid.h b/include/uapi/linux/uhid.h
index 0a08f2b..116536e 100644
--- a/include/uapi/linux/uhid.h
+++ b/include/uapi/linux/uhid.h
@@ -24,37 +24,21 @@
 #include <linux/hid.h>
 
 enum uhid_event_type {
-	UHID_CREATE,
+	__UHID_LEGACY_CREATE,
 	UHID_DESTROY,
 	UHID_START,
 	UHID_STOP,
 	UHID_OPEN,
 	UHID_CLOSE,
 	UHID_OUTPUT,
-	UHID_OUTPUT_EV,			/* obsolete! */
-	UHID_INPUT,
-	UHID_FEATURE,			/* obsolete! use UHID_GET_REPORT */
-	UHID_GET_REPORT = UHID_FEATURE,
-	UHID_FEATURE_ANSWER,		/* obsolete! use UHID_GET_REPORT_REPLY */
-	UHID_GET_REPORT_REPLY = UHID_FEATURE_ANSWER,
+	__UHID_LEGACY_OUTPUT_EV,
+	__UHID_LEGACY_INPUT,
+	UHID_GET_REPORT,
+	UHID_GET_REPORT_REPLY,
 	UHID_CREATE2,
 	UHID_INPUT2,
 };
 
-struct uhid_create_req {
-	__u8 name[128];
-	__u8 phys[64];
-	__u8 uniq[64];
-	__u8 __user *rd_data;
-	__u16 rd_size;
-
-	__u16 bus;
-	__u32 vendor;
-	__u32 product;
-	__u32 version;
-	__u32 country;
-} __attribute__((__packed__));
-
 struct uhid_create2_req {
 	__u8 name[128];
 	__u8 phys[64];
@@ -76,24 +60,67 @@ enum uhid_report_type {
 	UHID_INPUT_REPORT,
 };
 
-struct uhid_input_req {
+struct uhid_input2_req {
+	__u16 size;
+	__u8 data[UHID_DATA_MAX];
+} __attribute__((__packed__));
+
+struct uhid_output_req {
 	__u8 data[UHID_DATA_MAX];
 	__u16 size;
+	__u8 rtype;
 } __attribute__((__packed__));
 
-struct uhid_input2_req {
+struct uhid_get_report_req {
+	__u32 id;
+	__u8 rnum;
+	__u8 rtype;
+} __attribute__((__packed__));
+
+struct uhid_get_report_reply_req {
+	__u32 id;
+	__u16 err;
 	__u16 size;
 	__u8 data[UHID_DATA_MAX];
 } __attribute__((__packed__));
 
-struct uhid_output_req {
+/*
+ * Compat Layer
+ * All these commands and requests are obsolete. You should avoid using them in
+ * new code. We support them for backwards-compatibility, but you might not get
+ * access to new feature in case you use them.
+ */
+
+enum uhid_legacy_event_type {
+	UHID_CREATE			= __UHID_LEGACY_CREATE,
+	UHID_OUTPUT_EV			= __UHID_LEGACY_OUTPUT_EV,
+	UHID_INPUT			= __UHID_LEGACY_INPUT,
+	UHID_FEATURE			= UHID_GET_REPORT,
+	UHID_FEATURE_ANSWER		= UHID_GET_REPORT_REPLY,
+};
+
+/* Obsolete! Use UHID_CREATE2. */
+struct uhid_create_req {
+	__u8 name[128];
+	__u8 phys[64];
+	__u8 uniq[64];
+	__u8 __user *rd_data;
+	__u16 rd_size;
+
+	__u16 bus;
+	__u32 vendor;
+	__u32 product;
+	__u32 version;
+	__u32 country;
+} __attribute__((__packed__));
+
+/* Obsolete! Use UHID_INPUT2. */
+struct uhid_input_req {
 	__u8 data[UHID_DATA_MAX];
 	__u16 size;
-	__u8 rtype;
 } __attribute__((__packed__));
 
-/* Obsolete! Newer kernels will no longer send these events but instead convert
- * it into raw output reports via UHID_OUTPUT. */
+/* Obsolete! Kernel uses UHID_OUTPUT exclusively now. */
 struct uhid_output_ev_req {
 	__u16 type;
 	__u16 code;
@@ -107,12 +134,6 @@ struct uhid_feature_req {
 	__u8 rtype;
 } __attribute__((__packed__));
 
-struct uhid_get_report_req {
-	__u32 id;
-	__u8 rnum;
-	__u8 rtype;
-} __attribute__((__packed__));
-
 /* Obsolete! Use ABI compatible UHID_GET_REPORT_REPLY. */
 struct uhid_feature_answer_req {
 	__u32 id;
@@ -121,12 +142,14 @@ struct uhid_feature_answer_req {
 	__u8 data[UHID_DATA_MAX];
 } __attribute__((__packed__));
 
-struct uhid_get_report_reply_req {
-	__u32 id;
-	__u16 err;
-	__u16 size;
-	__u8 data[UHID_DATA_MAX];
-} __attribute__((__packed__));
+/*
+ * UHID Events
+ * All UHID events from and to the kernel are encoded as "struct uhid_event".
+ * The "type" field contains a UHID_* type identifier. All payload depends on
+ * that type and can be accessed via ev->u.XYZ accordingly.
+ * If user-space writes short events, they're extended with 0s by the kernel. If
+ * the kernel writes short events, user-space shall extend them with 0s.
+ */
 
 struct uhid_event {
 	__u32 type;
-- 
2.0.3


^ permalink raw reply related

* [PATCH 09/12] HID: uhid: rename uhid_raw_request to uhid_hid_raw_request
From: David Herrmann @ 2014-07-29 15:14 UTC (permalink / raw)
  To: linux-input; +Cc: Jiri Kosina, Benjamin Tissoires, David Herrmann
In-Reply-To: <1406646866-999-1-git-send-email-dh.herrmann@gmail.com>

We use strict prefixed in uhid.c:
  uhid_char_*: implement char-dev callbacks
  uhid_dev_*: implement uhid device management and runtime
  uhid_hid_*: implement hid-dev callbacks

uhid_raw_request is an hid callback, so rename it to uhid_hid_raw_request.

While at it, move it closer to it's extracted helpers and keep the same
order as in "struct hid_driver".

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/hid/uhid.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index 8f5e46b..8bf613e 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -203,6 +203,21 @@ unlock:
 	return ret ? ret : len;
 }
 
+static int uhid_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 uhid_hid_get_report(hid, reportnum, buf, len, rtype);
+	case HID_REQ_SET_REPORT:
+		/* TODO: implement proper SET_REPORT functionality */
+		return -ENOSYS;
+	default:
+		return -EIO;
+	}
+}
+
 static int uhid_hid_output_raw(struct hid_device *hid, __u8 *buf, size_t count,
 			       unsigned char report_type)
 {
@@ -247,29 +262,14 @@ 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_report(hid, reportnum, buf, len, rtype);
-	case HID_REQ_SET_REPORT:
-		/* TODO: implement proper SET_REPORT functionality */
-		return -ENOSYS;
-	default:
-		return -EIO;
-	}
-}
-
 static struct hid_ll_driver uhid_hid_driver = {
 	.start = uhid_hid_start,
 	.stop = uhid_hid_stop,
 	.open = uhid_hid_open,
 	.close = uhid_hid_close,
 	.parse = uhid_hid_parse,
+	.raw_request = uhid_hid_raw_request,
 	.output_report = uhid_hid_output_report,
-	.raw_request = uhid_raw_request,
 };
 
 #ifdef CONFIG_COMPAT
-- 
2.0.3


^ permalink raw reply related


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox