Re: [HID : improvement] Allow drivers to replace report descriptors

[Jiri Kosina <jkosina@suse.cz>]

On Sun, 4 Jan 2009, Marcin Tolysz wrote:

> [HID : improvement] Allow drivers to replace report descriptors completely
>    Some devices present themselves as a HID device, however if we pass
> their device descriptor to HID subsystem they
>   might be bogus or broken. The idea behind this patch is to allow a
> device driver to decide how descriptor should look at the end.

Hi Marcin,

sorry for late response, I have been buried by other things.

>   Once we will be able to fix broken descriptors we could look into
> other bits of system i.e. completing support for other HID
>   extensions and then improving descriptors to support that new
> extensions.  And descriptors are static, one we have one
>   it act as a "firmware" for a kernel to tell how device's events
> should be interpreted.

In an ideal world this shouldn't be needed, because in ideal world there 
are vendors who create devices that are compliant with the standard ... oh 
well.

Could you please separate the patches, so that we have one that introduces 
that possibility to perform the complete report descriptor replacement, 
and the second one which uses this framework to implement better support 
for Sony PS3 controller? Thanks.

> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 40df3e1..9c04e23 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -643,7 +643,9 @@ int hid_parse_report(struct hid_device *device, __u8 *start,
>  	struct hid_parser *parser;
>  	struct hid_item item;
>  	__u8 *end;
> -	int ret;
> +	int ret,n;

Please add a space here.

> +	unsigned int possibly_new_size=size;

Some spaces would also be nice here. And the variable name isn't really 
descriptive; new_size or nsize would work as well.

> -	if (device->driver->report_fixup)
> -		device->driver->report_fixup(device, start, size);
> +	if (device->driver->report_fixup){

Please add a space before the opening bracket.

> +	if (sc->quirks & PS3_SIXAXIS_REMAP ){
> +		/*The goal is to replace it, with rdesc supplied from user-space*/
> +		memcpy (rdesc , six_rep  , sizeof (six_rep));
> +		*rsize=sizeof (six_rep);
> +	}
>  }

I don't see a possibility to supply a replacement report descriptor from 
userspace, which contradicts the comment? (and BTW some spaces in the 
comment would also be nice, to be compatible with the rest of the kernel).

Also, please keep the proper formatting/spacing.

> +static int sony_set_operational_bluetooth(struct hid_device *hdev)
> +{
> +	/*TODO: Add bluetooth fix for dualshock/sixaxis */
> +	return 0;
> +}
> +

Are you planning to implement this for the final version of the patch?

> @@ -81,7 +97,7 @@ static int sony_probe(struct hid_device *hdev, const
> struct hid_device_id *id)
> 
>  	sc = kzalloc(sizeof(*sc), GFP_KERNEL);
>  	if (sc == NULL) {
> -		dev_err(&hdev->dev, "can't alloc apple descriptor\n");
> +		dev_err(&hdev->dev, "can't alloc sony descriptor\n");

Ah, good catch, there is indeed a typo in sony_probe(). Could you please 
send this to me as a separate fix?

> -
> -	ret = sony_set_operational(hdev);
> -	if (ret)
> +
> +    if(quirks & PS3_SIXAXIS_OPERATIONAL){
> +    	switch (hdev->bus) {
> +			case BUS_USB:
> +				ret = sony_set_operational(hdev);
> +				break;
> +			case BUS_BLUETOOTH:
> +				ret = sony_set_operational_bluetooth(hdev);
> +				break;
> +			//default:

What is the purpose of this //default? (also please don't use this comment 
style in kernel source).

> +
> +/*
> +* Comments show connection between a descriptor and a report i.e. a position.
> +*/
> +
> +
> +__u8  six_rep[] ={
> +	0x05 , 0x01, //   Generic Desktop Controls
> +	0x09 , 0x04, //   Usage  Joystick
> +
> +	0xA1 , 0x01, //  Collection   Application
> +	0xA1 , 0x02, //  Collection   Logical
> +	0x85 , 0x01, //  Report ID  1

I'd prefer also the standard kernel commenting style here, please.

Also, the patch was whitespace-damaged, so please fix that up for your 
following submissions.

Thanks!

-- 
Jiri Kosina
SUSE Labs