[PATCH] HID: i2c-hid: add ACPI support

[PATCH] HID: i2c-hid: add ACPI support

[Mika Westerberg]

The HID over I2C protocol specification states that when the device is
enumerated from ACPI the HID descriptor address can be obtained by
executing "_DSM" for the device with function 1. Enable this.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/hid/i2c-hid/i2c-hid.c |   73 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 71 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 9ef22244..b2eebb6 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -34,6 +34,7 @@
 #include <linux/kernel.h>
 #include <linux/hid.h>
 #include <linux/mutex.h>
+#include <linux/acpi.h>
 
 #include <linux/i2c/i2c-hid.h>
 
@@ -810,6 +811,70 @@ static int __devinit i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
 	return 0;
 }
 
+#ifdef CONFIG_ACPI
+static struct i2c_hid_platform_data *
+i2c_hid_acpi_pdata(struct i2c_client *client)
+{
+	static u8 i2c_hid_guid[] = {
+		0xF7, 0xF6, 0xDF, 0x3C, 0x67, 0x42, 0x55, 0x45,
+		0xAD, 0x05, 0xB3, 0x0A, 0x3D, 0x89, 0x38, 0xDE,
+	};
+	struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
+	struct i2c_hid_platform_data *pdata = NULL;
+	union acpi_object params[4], *obj;
+	struct acpi_object_list input;
+	struct acpi_device *adev;
+	acpi_handle handle;
+
+	handle = ACPI_HANDLE(&client->dev);
+	if (!handle || acpi_bus_get_device(handle, &adev))
+		return NULL;
+
+	input.count = ARRAY_SIZE(params);
+	input.pointer = params;
+
+	params[0].type = ACPI_TYPE_BUFFER;
+	params[0].buffer.length = sizeof(i2c_hid_guid);
+	params[0].buffer.pointer = i2c_hid_guid;
+	params[1].type = ACPI_TYPE_INTEGER;
+	params[1].integer.value = 1;
+	params[2].type = ACPI_TYPE_INTEGER;
+	params[2].integer.value = 1; /* HID function */
+	params[3].type = ACPI_TYPE_INTEGER;
+	params[3].integer.value = 0;
+
+	if (ACPI_FAILURE(acpi_evaluate_object(handle, "_DSM", &input, &buf)))
+		return NULL;
+
+	obj = (union acpi_object *)buf.pointer;
+	if (obj->type != ACPI_TYPE_INTEGER)
+		goto fail;
+
+	pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		goto fail;
+
+	pdata->hid_descriptor_address = obj->integer.value;
+
+fail:
+	kfree(buf.pointer);
+	return pdata;
+}
+
+static struct acpi_device_id i2c_hid_acpi_match[] = {
+	{"ACPI0C50", 0 },
+	{"PNP0C50", 0 },
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, i2c_hid_acpi_match);
+#else
+static inline struct i2c_hid_platform_data *
+i2c_hid_acpi_pdata(struct i2c_client *client)
+{
+	return NULL;
+}
+#endif
+
 static int __devinit i2c_hid_probe(struct i2c_client *client,
 		const struct i2c_device_id *dev_id)
 {
@@ -822,8 +887,11 @@ static int __devinit i2c_hid_probe(struct i2c_client *client,
 	dbg_hid("HID probe called for i2c 0x%02x\n", client->addr);
 
 	if (!platform_data) {
-		dev_err(&client->dev, "HID register address not provided\n");
-		return -EINVAL;
+		platform_data = i2c_hid_acpi_pdata(client);
+		if (!platform_data) {
+			dev_err(&client->dev, "HID register address not provided\n");
+			return -EINVAL;
+		}
 	}
 
 	if (!client->irq) {
@@ -964,6 +1032,7 @@ static struct i2c_driver i2c_hid_driver = {
 		.name	= "i2c_hid",
 		.owner	= THIS_MODULE,
 		.pm	= &i2c_hid_pm,
+		.acpi_match_table = ACPI_PTR(i2c_hid_acpi_match),
 	},
 
 	.probe		= i2c_hid_probe,
-- 
1.7.10.4

Re: [PATCH] HID: i2c-hid: add ACPI support

[Benjamin Tissoires]

Hi Mika,

On Tue, Jan 8, 2013 at 2:05 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> The HID over I2C protocol specification states that when the device is
> enumerated from ACPI the HID descriptor address can be obtained by
> executing "_DSM" for the device with function 1. Enable this.

Hehe, funny thing, I worked on the very same patch last Friday. I was
not able to send it upstream as I still don't have an ACPI 5 enabled
device...
So thanks for submitting (and testing) this!

Before the review, I just have a quick question. All the HID/i2c
devices I saw so far present a reset line (through a GPIO). Does the
shipped device you have present this line, and if yes, how this meld
with the code (i.e. should we take this into account).

Please note that I can only compare this to my patch, based on the
DSDT example Microsoft gave in its spec. So sorry if I'm asking
useless things, but I like to understand... :)
Here are a few comments:

>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/hid/i2c-hid/i2c-hid.c |   73 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 71 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index 9ef22244..b2eebb6 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -34,6 +34,7 @@
>  #include <linux/kernel.h>
>  #include <linux/hid.h>
>  #include <linux/mutex.h>
> +#include <linux/acpi.h>
>
>  #include <linux/i2c/i2c-hid.h>
>
> @@ -810,6 +811,70 @@ static int __devinit i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
>         return 0;
>  }
>
> +#ifdef CONFIG_ACPI
> +static struct i2c_hid_platform_data *
> +i2c_hid_acpi_pdata(struct i2c_client *client)
> +{
> +       static u8 i2c_hid_guid[] = {
> +               0xF7, 0xF6, 0xDF, 0x3C, 0x67, 0x42, 0x55, 0x45,
> +               0xAD, 0x05, 0xB3, 0x0A, 0x3D, 0x89, 0x38, 0xDE,
> +       };
> +       struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
> +       struct i2c_hid_platform_data *pdata = NULL;
> +       union acpi_object params[4], *obj;
> +       struct acpi_object_list input;
> +       struct acpi_device *adev;
> +       acpi_handle handle;
> +
> +       handle = ACPI_HANDLE(&client->dev);
> +       if (!handle || acpi_bus_get_device(handle, &adev))
> +               return NULL;
> +
> +       input.count = ARRAY_SIZE(params);
> +       input.pointer = params;
> +
> +       params[0].type = ACPI_TYPE_BUFFER;
> +       params[0].buffer.length = sizeof(i2c_hid_guid);
> +       params[0].buffer.pointer = i2c_hid_guid;
> +       params[1].type = ACPI_TYPE_INTEGER;
> +       params[1].integer.value = 1;

Can you confirm that any value here is ok (this is what I read from
the DSDT example Microsoft gave, Arg1 is not used).

> +       params[2].type = ACPI_TYPE_INTEGER;
> +       params[2].integer.value = 1; /* HID function */
> +       params[3].type = ACPI_TYPE_INTEGER;
> +       params[3].integer.value = 0;

Again, the DSDT example showed that no Arg3 was used... But again, I
never wrote ACPI driver, so I may be wrong.

> +
> +       if (ACPI_FAILURE(acpi_evaluate_object(handle, "_DSM", &input, &buf)))
> +               return NULL;

As you are returning NULL here, the error that will be raised is
-EINVAL. I think it should be -ENODEV in this case. I have no strong
opinion on this, but this can be achieved by returning the error from
the function, and the returned i2c_hid_platform_data as an argument.

Or maybe just an error message will be sufficient.

> +
> +       obj = (union acpi_object *)buf.pointer;
> +       if (obj->type != ACPI_TYPE_INTEGER)
> +               goto fail;

Again, I would like to know what happened here in case of a failure.
So an error message would be good.

> +
> +       pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
> +       if (!pdata)
> +               goto fail;

idem (and returning -ENOMEM would be better IMHO).

> +
> +       pdata->hid_descriptor_address = obj->integer.value;
> +
> +fail:
> +       kfree(buf.pointer);
> +       return pdata;
> +}
> +
> +static struct acpi_device_id i2c_hid_acpi_match[] = {
> +       {"ACPI0C50", 0 },

I never saw this _CID/_HID. Just out of curiosity, is this a
standardized _CID or is it a _HID specific to a device?

> +       {"PNP0C50", 0 },
> +       { },
> +};
> +MODULE_DEVICE_TABLE(acpi, i2c_hid_acpi_match);
> +#else
> +static inline struct i2c_hid_platform_data *
> +i2c_hid_acpi_pdata(struct i2c_client *client)
> +{
> +       return NULL;
> +}
> +#endif
> +
>  static int __devinit i2c_hid_probe(struct i2c_client *client,
>                 const struct i2c_device_id *dev_id)
>  {
> @@ -822,8 +887,11 @@ static int __devinit i2c_hid_probe(struct i2c_client *client,
>         dbg_hid("HID probe called for i2c 0x%02x\n", client->addr);
>
>         if (!platform_data) {
> -               dev_err(&client->dev, "HID register address not provided\n");
> -               return -EINVAL;
> +               platform_data = i2c_hid_acpi_pdata(client);
> +               if (!platform_data) {
> +                       dev_err(&client->dev, "HID register address not provided\n");

You may have a line with more than 80 characters here (it's difficult
to guess thanks to my gmail client converting tabs into spaces...
grrr)

> +                       return -EINVAL;

Always returning -EINVAL does not seem to be the exact error code if
i2c_hid_acpi_pdata fails.

> +               }
>         }
>
>         if (!client->irq) {
> @@ -964,6 +1032,7 @@ static struct i2c_driver i2c_hid_driver = {
>                 .name   = "i2c_hid",
>                 .owner  = THIS_MODULE,
>                 .pm     = &i2c_hid_pm,
> +               .acpi_match_table = ACPI_PTR(i2c_hid_acpi_match),

This is something I was wondering when looking at your documentation.
If CONFIG_ACPI is not defined, then 'i2c_hid_acpi_match' does not exists.
Is there some magic within ACPI_PTR if CONFIG_ACPI is not defined, or
should we put the #ifdef around?

Anyway thanks for this job!

Cheers,
Benjamin

>         },
>
>         .probe          = i2c_hid_probe,
> --
> 1.7.10.4
>

Re: [PATCH] HID: i2c-hid: add ACPI support

[Mika Westerberg]

On Tue, Jan 08, 2013 at 02:51:53PM +0100, Benjamin Tissoires wrote:
> Hi Mika,
> 
> On Tue, Jan 8, 2013 at 2:05 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > The HID over I2C protocol specification states that when the device is
> > enumerated from ACPI the HID descriptor address can be obtained by
> > executing "_DSM" for the device with function 1. Enable this.
> 
> Hehe, funny thing, I worked on the very same patch last Friday. I was
> not able to send it upstream as I still don't have an ACPI 5 enabled
> device...
> So thanks for submitting (and testing) this!
> 
> Before the review, I just have a quick question. All the HID/i2c
> devices I saw so far present a reset line (through a GPIO). Does the
> shipped device you have present this line, and if yes, how this meld
> with the code (i.e. should we take this into account).

Yes, there is either one or two GPIO lines. But there are also devices that
don't have any GPIO lines.

We probably should take this into account. I'm not too familiar with HID
drivers but what if we set the hid->dev.acpi_node and let the actual HID
driver to deal with the GPIO?

> Please note that I can only compare this to my patch, based on the
> DSDT example Microsoft gave in its spec. So sorry if I'm asking
> useless things, but I like to understand... :)
> Here are a few comments:
> 
> >
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> >  drivers/hid/i2c-hid/i2c-hid.c |   73 +++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 71 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> > index 9ef22244..b2eebb6 100644
> > --- a/drivers/hid/i2c-hid/i2c-hid.c
> > +++ b/drivers/hid/i2c-hid/i2c-hid.c
> > @@ -34,6 +34,7 @@
> >  #include <linux/kernel.h>
> >  #include <linux/hid.h>
> >  #include <linux/mutex.h>
> > +#include <linux/acpi.h>
> >
> >  #include <linux/i2c/i2c-hid.h>
> >
> > @@ -810,6 +811,70 @@ static int __devinit i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
> >         return 0;
> >  }
> >
> > +#ifdef CONFIG_ACPI
> > +static struct i2c_hid_platform_data *
> > +i2c_hid_acpi_pdata(struct i2c_client *client)
> > +{
> > +       static u8 i2c_hid_guid[] = {
> > +               0xF7, 0xF6, 0xDF, 0x3C, 0x67, 0x42, 0x55, 0x45,
> > +               0xAD, 0x05, 0xB3, 0x0A, 0x3D, 0x89, 0x38, 0xDE,
> > +       };
> > +       struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
> > +       struct i2c_hid_platform_data *pdata = NULL;
> > +       union acpi_object params[4], *obj;
> > +       struct acpi_object_list input;
> > +       struct acpi_device *adev;
> > +       acpi_handle handle;
> > +
> > +       handle = ACPI_HANDLE(&client->dev);
> > +       if (!handle || acpi_bus_get_device(handle, &adev))
> > +               return NULL;
> > +
> > +       input.count = ARRAY_SIZE(params);
> > +       input.pointer = params;
> > +
> > +       params[0].type = ACPI_TYPE_BUFFER;
> > +       params[0].buffer.length = sizeof(i2c_hid_guid);
> > +       params[0].buffer.pointer = i2c_hid_guid;
> > +       params[1].type = ACPI_TYPE_INTEGER;
> > +       params[1].integer.value = 1;
> 
> Can you confirm that any value here is ok (this is what I read from
> the DSDT example Microsoft gave, Arg1 is not used).

It is being used in the Microsoft example for querying the supported
functions so I decided to stick with the same when calling the func 1.

All the DSDTs I've seen where the _DSM is implemented, this is not checked
at all.

> > +       params[2].type = ACPI_TYPE_INTEGER;
> > +       params[2].integer.value = 1; /* HID function */
> > +       params[3].type = ACPI_TYPE_INTEGER;
> > +       params[3].integer.value = 0;
> 
> Again, the DSDT example showed that no Arg3 was used... But again, I
> never wrote ACPI driver, so I may be wrong.

It is not being used, right. But the _DSM function wants 4 parameters so we
must pass something.

> > +
> > +       if (ACPI_FAILURE(acpi_evaluate_object(handle, "_DSM", &input, &buf)))
> > +               return NULL;
> 
> As you are returning NULL here, the error that will be raised is
> -EINVAL. I think it should be -ENODEV in this case. I have no strong
> opinion on this, but this can be achieved by returning the error from
> the function, and the returned i2c_hid_platform_data as an argument.

Good idea.

> 
> Or maybe just an error message will be sufficient.
> 
> > +
> > +       obj = (union acpi_object *)buf.pointer;
> > +       if (obj->type != ACPI_TYPE_INTEGER)
> > +               goto fail;
> 
> Again, I would like to know what happened here in case of a failure.
> So an error message would be good.

OK.

> > +
> > +       pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
> > +       if (!pdata)
> > +               goto fail;
> 
> idem (and returning -ENOMEM would be better IMHO).

OK.

> 
> > +
> > +       pdata->hid_descriptor_address = obj->integer.value;
> > +
> > +fail:
> > +       kfree(buf.pointer);
> > +       return pdata;
> > +}
> > +
> > +static struct acpi_device_id i2c_hid_acpi_match[] = {
> > +       {"ACPI0C50", 0 },
> 
> I never saw this _CID/_HID. Just out of curiosity, is this a
> standardized _CID or is it a _HID specific to a device?

In the Microsoft spec you can have either PNPxxxx or ACPIxxxx.

> > +       {"PNP0C50", 0 },
> > +       { },
> > +};
> > +MODULE_DEVICE_TABLE(acpi, i2c_hid_acpi_match);
> > +#else
> > +static inline struct i2c_hid_platform_data *
> > +i2c_hid_acpi_pdata(struct i2c_client *client)
> > +{
> > +       return NULL;
> > +}
> > +#endif
> > +
> >  static int __devinit i2c_hid_probe(struct i2c_client *client,
> >                 const struct i2c_device_id *dev_id)
> >  {
> > @@ -822,8 +887,11 @@ static int __devinit i2c_hid_probe(struct i2c_client *client,
> >         dbg_hid("HID probe called for i2c 0x%02x\n", client->addr);
> >
> >         if (!platform_data) {
> > -               dev_err(&client->dev, "HID register address not provided\n");
> > -               return -EINVAL;
> > +               platform_data = i2c_hid_acpi_pdata(client);
> > +               if (!platform_data) {
> > +                       dev_err(&client->dev, "HID register address not provided\n");
> 
> You may have a line with more than 80 characters here (it's difficult
> to guess thanks to my gmail client converting tabs into spaces...
> grrr)

Shouldn't be as checkpatch.pl didn't complain but I'll re-check.

> > +                       return -EINVAL;
> 
> Always returning -EINVAL does not seem to be the exact error code if
> i2c_hid_acpi_pdata fails.

OK. I'll return whatever error code is returned from i2c_hid_acpi_pdata().

> > +               }
> >         }
> >
> >         if (!client->irq) {
> > @@ -964,6 +1032,7 @@ static struct i2c_driver i2c_hid_driver = {
> >                 .name   = "i2c_hid",
> >                 .owner  = THIS_MODULE,
> >                 .pm     = &i2c_hid_pm,
> > +               .acpi_match_table = ACPI_PTR(i2c_hid_acpi_match),
> 
> This is something I was wondering when looking at your documentation.
> If CONFIG_ACPI is not defined, then 'i2c_hid_acpi_match' does not exists.
> Is there some magic within ACPI_PTR if CONFIG_ACPI is not defined, or
> should we put the #ifdef around?

if !CONFIG_ACPI then ACPI_PTR(whatever) becomes NULL so it is safe to set
here.

> Anyway thanks for this job!

No problem and thanks for your review.

Re: [PATCH] HID: i2c-hid: add ACPI support

[Benjamin Tissoires]

On Tue, Jan 8, 2013 at 7:09 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Tue, Jan 08, 2013 at 02:51:53PM +0100, Benjamin Tissoires wrote:
>> Hi Mika,
>>
>> On Tue, Jan 8, 2013 at 2:05 PM, Mika Westerberg
>> <mika.westerberg@linux.intel.com> wrote:
>> > The HID over I2C protocol specification states that when the device is
>> > enumerated from ACPI the HID descriptor address can be obtained by
>> > executing "_DSM" for the device with function 1. Enable this.
>>
>> Hehe, funny thing, I worked on the very same patch last Friday. I was
>> not able to send it upstream as I still don't have an ACPI 5 enabled
>> device...
>> So thanks for submitting (and testing) this!
>>
>> Before the review, I just have a quick question. All the HID/i2c
>> devices I saw so far present a reset line (through a GPIO). Does the
>> shipped device you have present this line, and if yes, how this meld
>> with the code (i.e. should we take this into account).
>
> Yes, there is either one or two GPIO lines. But there are also devices that
> don't have any GPIO lines.

Ouch. But if they don't have any GPIO, how can they manage to send
information? If they are using polling, then this will require some
more work in i2c-hid :(

>
> We probably should take this into account. I'm not too familiar with HID
> drivers but what if we set the hid->dev.acpi_node and let the actual HID
> driver to deal with the GPIO?

Well, HID is a communication layer between the device and the kernel
in order to make it agnostic of the transport layer (usb, bt or i2c).
Even if some drivers do some transport tuning, I don't think it's a
good idea to ask HID drivers to do transport handling such as setting
up GPIOs.

The closest thing which is already in the kernel tree is the handling
of device specific quirks in usbhid. We may be forced to do such a
thing if the DSDT is not explicit enough to guess the right behavior
(how to trigger the reset line, etc..)

Also, I missed one point in my previous review:

>
>> Please note that I can only compare this to my patch, based on the
>> DSDT example Microsoft gave in its spec. So sorry if I'm asking
>> useless things, but I like to understand... :)
>> Here are a few comments:
>>
>> >
>> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>> > ---
>> >  drivers/hid/i2c-hid/i2c-hid.c |   73 +++++++++++++++++++++++++++++++++++++++--
>> >  1 file changed, 71 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
>> > index 9ef22244..b2eebb6 100644
 (...snipped...)

>> > +
>> > +       pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);

Here, I don't think mixing devm_* and regular allocations is a good thing.
I know it's a pain, but at the time I wrote the driver, the input
layer was not devm-ized, so I was not able to use devm allocs. Now it
should be feasible, but I didn't found the time to do it.
So I'm afraid, this allocation must use a regular kwalloc and it
should be freed somewhere later, until we devm-ize the whole module.

>> > +       if (!pdata)
>> > +               goto fail;
>>

(...snipped...)
>> >         if (!platform_data) {
>> > -               dev_err(&client->dev, "HID register address not provided\n");
>> > -               return -EINVAL;
>> > +               platform_data = i2c_hid_acpi_pdata(client);
>> > +               if (!platform_data) {
>> > +                       dev_err(&client->dev, "HID register address not provided\n");
>>
>> You may have a line with more than 80 characters here (it's difficult
>> to guess thanks to my gmail client converting tabs into spaces...
>> grrr)
>
> Shouldn't be as checkpatch.pl didn't complain but I'll re-check.

It's strange if it didn't complained: I count 61 chars for the
dev_err() message + 3 tabs * 8 chars => 85 chars in total...

Cheers,
Benjamin

>
>> > +                       return -EINVAL;
>>
>> Always returning -EINVAL does not seem to be the exact error code if
>> i2c_hid_acpi_pdata fails.
>
> OK. I'll return whatever error code is returned from i2c_hid_acpi_pdata().
>
>> > +               }
>> >         }
>> >
>> >         if (!client->irq) {
>> > @@ -964,6 +1032,7 @@ static struct i2c_driver i2c_hid_driver = {
>> >                 .name   = "i2c_hid",
>> >                 .owner  = THIS_MODULE,
>> >                 .pm     = &i2c_hid_pm,
>> > +               .acpi_match_table = ACPI_PTR(i2c_hid_acpi_match),
>>
>> This is something I was wondering when looking at your documentation.
>> If CONFIG_ACPI is not defined, then 'i2c_hid_acpi_match' does not exists.
>> Is there some magic within ACPI_PTR if CONFIG_ACPI is not defined, or
>> should we put the #ifdef around?
>
> if !CONFIG_ACPI then ACPI_PTR(whatever) becomes NULL so it is safe to set
> here.
>
>> Anyway thanks for this job!
>
> No problem and thanks for your review.

Re: [PATCH] HID: i2c-hid: add ACPI support

[Mika Westerberg]

On Tue, Jan 08, 2013 at 10:55:59PM +0100, Benjamin Tissoires wrote:
> On Tue, Jan 8, 2013 at 7:09 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Tue, Jan 08, 2013 at 02:51:53PM +0100, Benjamin Tissoires wrote:
> >> Hi Mika,
> >>
> >> On Tue, Jan 8, 2013 at 2:05 PM, Mika Westerberg
> >> <mika.westerberg@linux.intel.com> wrote:
> >> > The HID over I2C protocol specification states that when the device is
> >> > enumerated from ACPI the HID descriptor address can be obtained by
> >> > executing "_DSM" for the device with function 1. Enable this.
> >>
> >> Hehe, funny thing, I worked on the very same patch last Friday. I was
> >> not able to send it upstream as I still don't have an ACPI 5 enabled
> >> device...
> >> So thanks for submitting (and testing) this!
> >>
> >> Before the review, I just have a quick question. All the HID/i2c
> >> devices I saw so far present a reset line (through a GPIO). Does the
> >> shipped device you have present this line, and if yes, how this meld
> >> with the code (i.e. should we take this into account).
> >
> > Yes, there is either one or two GPIO lines. But there are also devices that
> > don't have any GPIO lines.
> 
> Ouch. But if they don't have any GPIO, how can they manage to send
> information? If they are using polling, then this will require some
> more work in i2c-hid :(

Well they don't have GPIO resources because their interrupt is directly
routed to the ioapic and exposed as an Interrupt resource.

> >
> > We probably should take this into account. I'm not too familiar with HID
> > drivers but what if we set the hid->dev.acpi_node and let the actual HID
> > driver to deal with the GPIO?
> 
> Well, HID is a communication layer between the device and the kernel
> in order to make it agnostic of the transport layer (usb, bt or i2c).
> Even if some drivers do some transport tuning, I don't think it's a
> good idea to ask HID drivers to do transport handling such as setting
> up GPIOs.

If they have reset GPIO or something like that, how else we should we
handle this if not in the driver? The i2c-hid core doesn't know for what
purpose a given GPIO line is.

i2c-hid core can handle the GPIO interrupt if client->irq is not set (by
converting the GPIO into interrupt number and passing it to the hid
driver). I didn't implement that because we have the client->irq already
set so I can't test this.

> The closest thing which is already in the kernel tree is the handling
> of device specific quirks in usbhid. We may be forced to do such a
> thing if the DSDT is not explicit enough to guess the right behavior
> (how to trigger the reset line, etc..)
> 
> Also, I missed one point in my previous review:
> 
> >
> >> Please note that I can only compare this to my patch, based on the
> >> DSDT example Microsoft gave in its spec. So sorry if I'm asking
> >> useless things, but I like to understand... :)
> >> Here are a few comments:
> >>
> >> >
> >> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> >> > ---
> >> >  drivers/hid/i2c-hid/i2c-hid.c |   73 +++++++++++++++++++++++++++++++++++++++--
> >> >  1 file changed, 71 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> >> > index 9ef22244..b2eebb6 100644
>  (...snipped...)
> 
> >> > +
> >> > +       pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
> 
> Here, I don't think mixing devm_* and regular allocations is a good thing.
> I know it's a pain, but at the time I wrote the driver, the input
> layer was not devm-ized, so I was not able to use devm allocs. Now it
> should be feasible, but I didn't found the time to do it.
> So I'm afraid, this allocation must use a regular kwalloc and it
> should be freed somewhere later, until we devm-ize the whole module.

Good point.

I was thinking to embed the platform data in the i2c_hid structure so that
it gets allocated at the same time as ihid and then we can handle setting
the platform data like:

	if (!platform_data) {
		ret = i2c_hid_acpi_pdata(client, &ihid->pdata);
		/* handle error */
	} else {
		ihid->pdata = *platform_data;	
	}

Does that sound feasible to you?

> >> > +       if (!pdata)
> >> > +               goto fail;
> >>
> 
> (...snipped...)
> >> >         if (!platform_data) {
> >> > -               dev_err(&client->dev, "HID register address not provided\n");
> >> > -               return -EINVAL;
> >> > +               platform_data = i2c_hid_acpi_pdata(client);
> >> > +               if (!platform_data) {
> >> > +                       dev_err(&client->dev, "HID register address not provided\n");
> >>
> >> You may have a line with more than 80 characters here (it's difficult
> >> to guess thanks to my gmail client converting tabs into spaces...
> >> grrr)
> >
> > Shouldn't be as checkpatch.pl didn't complain but I'll re-check.
> 
> It's strange if it didn't complained: I count 61 chars for the
> dev_err() message + 3 tabs * 8 chars => 85 chars in total...

You are right, it is over 85 chars.

I've learned that checkpatch.pl doesn't complain if the line is longer than
80 chars, if it contains quoted string.

I'll fix it up.

Re: [PATCH] HID: i2c-hid: add ACPI support

[Benjamin Tissoires]

On Wed, Jan 9, 2013 at 10:28 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Tue, Jan 08, 2013 at 10:55:59PM +0100, Benjamin Tissoires wrote:
>> On Tue, Jan 8, 2013 at 7:09 PM, Mika Westerberg
>> <mika.westerberg@linux.intel.com> wrote:
>> > On Tue, Jan 08, 2013 at 02:51:53PM +0100, Benjamin Tissoires wrote:
>> >> Hi Mika,
>> >>
>> >> On Tue, Jan 8, 2013 at 2:05 PM, Mika Westerberg
>> >> <mika.westerberg@linux.intel.com> wrote:
>> >> > The HID over I2C protocol specification states that when the device is
>> >> > enumerated from ACPI the HID descriptor address can be obtained by
>> >> > executing "_DSM" for the device with function 1. Enable this.
>> >>
>> >> Hehe, funny thing, I worked on the very same patch last Friday. I was
>> >> not able to send it upstream as I still don't have an ACPI 5 enabled
>> >> device...
>> >> So thanks for submitting (and testing) this!
>> >>
>> >> Before the review, I just have a quick question. All the HID/i2c
>> >> devices I saw so far present a reset line (through a GPIO). Does the
>> >> shipped device you have present this line, and if yes, how this meld
>> >> with the code (i.e. should we take this into account).
>> >
>> > Yes, there is either one or two GPIO lines. But there are also devices that
>> > don't have any GPIO lines.
>>
>> Ouch. But if they don't have any GPIO, how can they manage to send
>> information? If they are using polling, then this will require some
>> more work in i2c-hid :(
>
> Well they don't have GPIO resources because their interrupt is directly
> routed to the ioapic and exposed as an Interrupt resource.
>
>> >
>> > We probably should take this into account. I'm not too familiar with HID
>> > drivers but what if we set the hid->dev.acpi_node and let the actual HID
>> > driver to deal with the GPIO?
>>
>> Well, HID is a communication layer between the device and the kernel
>> in order to make it agnostic of the transport layer (usb, bt or i2c).
>> Even if some drivers do some transport tuning, I don't think it's a
>> good idea to ask HID drivers to do transport handling such as setting
>> up GPIOs.
>
> If they have reset GPIO or something like that, how else we should we
> handle this if not in the driver? The i2c-hid core doesn't know for what
> purpose a given GPIO line is.

But the hid protocol aims at reducing the number of drivers. The idea
is to built one driver to rule them all... :)
Some stats:
- hid-multitouch (the driver that drives multitouch screens) has a
list of ~80 registered products, most of them are USB (few are
Bluetooth). Bonus point, this list is not exhaustive because there is
an auto-detection mechanism in place which forwards unknown
mt-touchscreen to hid-multitouch. Now with i2c-hid, hid-multitouch can
also handle any Win8 certified i2c touchscreen, so I guess it will add
a lot of new devices to this list.
- hid-core registered ~240 exception -> it means that on all existing
input devices that follow the hid protocol, only 240 need special
handling.

So my point is that some of the hid drivers may know the attached
GPIOs, but the generic hid drivers (hid-multitouch, hid-generic and
hid-core then) won't.

Anyway, there may be adjustments at the beginning, but I still think
we could add a common place for i2c quirks in i2c-hid, at least at the
beginning.
The best solution would be to be able to deduce the behavior of those
GPIOs thanks to the ACPI description. Problem for me: I can't have
access to the document referred in HID over i2c specification (Windows
ACPI Design Guide for SOC Platform) while working at Red Hat...

>
> i2c-hid core can handle the GPIO interrupt if client->irq is not set (by
> converting the GPIO into interrupt number and passing it to the hid
> driver). I didn't implement that because we have the client->irq already
> set so I can't test this.

But this part of the code is already in ACPI i2c enumeration, right?
So why you want to rewrite it in i2c-hid?

>
>> The closest thing which is already in the kernel tree is the handling
>> of device specific quirks in usbhid. We may be forced to do such a
>> thing if the DSDT is not explicit enough to guess the right behavior
>> (how to trigger the reset line, etc..)
>>
>> Also, I missed one point in my previous review:
>>
>> >
>> >> Please note that I can only compare this to my patch, based on the
>> >> DSDT example Microsoft gave in its spec. So sorry if I'm asking
>> >> useless things, but I like to understand... :)
>> >> Here are a few comments:
>> >>
>> >> >
>> >> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>> >> > ---
>> >> >  drivers/hid/i2c-hid/i2c-hid.c |   73 +++++++++++++++++++++++++++++++++++++++--
>> >> >  1 file changed, 71 insertions(+), 2 deletions(-)
>> >> >
>> >> > diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
>> >> > index 9ef22244..b2eebb6 100644
>>  (...snipped...)
>>
>> >> > +
>> >> > +       pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
>>
>> Here, I don't think mixing devm_* and regular allocations is a good thing.
>> I know it's a pain, but at the time I wrote the driver, the input
>> layer was not devm-ized, so I was not able to use devm allocs. Now it
>> should be feasible, but I didn't found the time to do it.
>> So I'm afraid, this allocation must use a regular kwalloc and it
>> should be freed somewhere later, until we devm-ize the whole module.
>
> Good point.
>
> I was thinking to embed the platform data in the i2c_hid structure so that
> it gets allocated at the same time as ihid and then we can handle setting
> the platform data like:
>
>         if (!platform_data) {
>                 ret = i2c_hid_acpi_pdata(client, &ihid->pdata);
>                 /* handle error */
>         } else {
>                 ihid->pdata = *platform_data;
>         }
>
> Does that sound feasible to you?

yep, go for it!

Cheers,
Benjamin

>
>> >> > +       if (!pdata)
>> >> > +               goto fail;
>> >>
>>
>> (...snipped...)
>> >> >         if (!platform_data) {
>> >> > -               dev_err(&client->dev, "HID register address not provided\n");
>> >> > -               return -EINVAL;
>> >> > +               platform_data = i2c_hid_acpi_pdata(client);
>> >> > +               if (!platform_data) {
>> >> > +                       dev_err(&client->dev, "HID register address not provided\n");
>> >>
>> >> You may have a line with more than 80 characters here (it's difficult
>> >> to guess thanks to my gmail client converting tabs into spaces...
>> >> grrr)
>> >
>> > Shouldn't be as checkpatch.pl didn't complain but I'll re-check.
>>
>> It's strange if it didn't complained: I count 61 chars for the
>> dev_err() message + 3 tabs * 8 chars => 85 chars in total...
>
> You are right, it is over 85 chars.
>
> I've learned that checkpatch.pl doesn't complain if the line is longer than
> 80 chars, if it contains quoted string.
>
> I'll fix it up.

Re: [PATCH] HID: i2c-hid: add ACPI support

[Mika Westerberg]

On Wed, Jan 09, 2013 at 11:38:24AM +0100, Benjamin Tissoires wrote:
> >
> > If they have reset GPIO or something like that, how else we should we
> > handle this if not in the driver? The i2c-hid core doesn't know for what
> > purpose a given GPIO line is.
> 
> But the hid protocol aims at reducing the number of drivers. The idea
> is to built one driver to rule them all... :)
> Some stats:
> - hid-multitouch (the driver that drives multitouch screens) has a
> list of ~80 registered products, most of them are USB (few are
> Bluetooth). Bonus point, this list is not exhaustive because there is
> an auto-detection mechanism in place which forwards unknown
> mt-touchscreen to hid-multitouch. Now with i2c-hid, hid-multitouch can
> also handle any Win8 certified i2c touchscreen, so I guess it will add
> a lot of new devices to this list.
> - hid-core registered ~240 exception -> it means that on all existing
> input devices that follow the hid protocol, only 240 need special
> handling.

OK, got it :)

> So my point is that some of the hid drivers may know the attached
> GPIOs, but the generic hid drivers (hid-multitouch, hid-generic and
> hid-core then) won't.

OK.

> Anyway, there may be adjustments at the beginning, but I still think
> we could add a common place for i2c quirks in i2c-hid, at least at the
> beginning.

Sure.

I noticed that the current i2c-hid code doesn't handle the GPIOs at all
right now (even if not enumerated form ACPI) so we need to invent something
once real hardware starts to be available. I have few devices here but they
are unusable (except the enumeration parts) as they need some FW which I
don't have yet.

> The best solution would be to be able to deduce the behavior of those
> GPIOs thanks to the ACPI description. Problem for me: I can't have
> access to the document referred in HID over i2c specification (Windows
> ACPI Design Guide for SOC Platform) while working at Red Hat...
> 
> >
> > i2c-hid core can handle the GPIO interrupt if client->irq is not set (by
> > converting the GPIO into interrupt number and passing it to the hid
> > driver). I didn't implement that because we have the client->irq already
> > set so I can't test this.
> 
> But this part of the code is already in ACPI i2c enumeration, right?
> So why you want to rewrite it in i2c-hid?

No, the ACPI I2C enumeration only handles Interrupt and IRQ resources. It
doesn't handle the GpioInt resources and this needs to be done in i2c-hid.

> >
> >> The closest thing which is already in the kernel tree is the handling
> >> of device specific quirks in usbhid. We may be forced to do such a
> >> thing if the DSDT is not explicit enough to guess the right behavior
> >> (how to trigger the reset line, etc..)
> >>
> >> Also, I missed one point in my previous review:
> >>
> >> >
> >> >> Please note that I can only compare this to my patch, based on the
> >> >> DSDT example Microsoft gave in its spec. So sorry if I'm asking
> >> >> useless things, but I like to understand... :)
> >> >> Here are a few comments:
> >> >>
> >> >> >
> >> >> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> >> >> > ---
> >> >> >  drivers/hid/i2c-hid/i2c-hid.c |   73 +++++++++++++++++++++++++++++++++++++++--
> >> >> >  1 file changed, 71 insertions(+), 2 deletions(-)
> >> >> >
> >> >> > diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> >> >> > index 9ef22244..b2eebb6 100644
> >>  (...snipped...)
> >>
> >> >> > +
> >> >> > +       pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
> >>
> >> Here, I don't think mixing devm_* and regular allocations is a good thing.
> >> I know it's a pain, but at the time I wrote the driver, the input
> >> layer was not devm-ized, so I was not able to use devm allocs. Now it
> >> should be feasible, but I didn't found the time to do it.
> >> So I'm afraid, this allocation must use a regular kwalloc and it
> >> should be freed somewhere later, until we devm-ize the whole module.
> >
> > Good point.
> >
> > I was thinking to embed the platform data in the i2c_hid structure so that
> > it gets allocated at the same time as ihid and then we can handle setting
> > the platform data like:
> >
> >         if (!platform_data) {
> >                 ret = i2c_hid_acpi_pdata(client, &ihid->pdata);
> >                 /* handle error */
> >         } else {
> >                 ihid->pdata = *platform_data;
> >         }
> >
> > Does that sound feasible to you?
> 
> yep, go for it!

Thanks. I'll prepare next version and send it out for review soon.