[PATCH v3] iio:kxcjk-1013: Add support for SMO8500 device

[PATCH v3] iio:kxcjk-1013: Add support for SMO8500 device

[Bastien Nocera]

The Onda v975w tablet contains an accelerometer that's advertised over
ACPI as SMO8500. This device is however a KXCJ9 accelerometer as
can be seen in the Windows driver's INF file, and from the etching on
the chipset ("KXCJ9 41566 0414").

This patch also removes the attempt to get the IRQ for the "data ready"
signal, as it does not seem to be supported by this device on this
platform.

Signed-off-by: Bastien Nocera <hadess@hadess.net>

---

Changes from v1:
- Rebase after driver changes
- Stop checking for IRQ when not supported

Changes from v2:
- Fixed the space before the opening bracket
- Added Sob line
- Add version to the patch subject line ;)

---
 drivers/iio/accel/kxcjk-1013.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
index 98909a9..3e966c2 100644
--- a/drivers/iio/accel/kxcjk-1013.c
+++ b/drivers/iio/accel/kxcjk-1013.c
@@ -108,6 +108,7 @@ struct kxcjk1013_data {
 	bool motion_trigger_on;
 	int64_t timestamp;
 	enum kx_chipset chipset;
+	bool is_smo8500_device;
 };
 
 enum kxcjk1013_axis {
@@ -1129,12 +1130,15 @@ static irqreturn_t kxcjk1013_data_rdy_trig_poll(int irq, void *private)
 }
 
 static const char *kxcjk1013_match_acpi_device(struct device *dev,
-					       enum kx_chipset *chipset)
+					       enum kx_chipset *chipset,
+					       bool *is_smo8500_device)
 {
 	const struct acpi_device_id *id;
 	id = acpi_match_device(dev->driver->acpi_match_table, dev);
 	if (!id)
 		return NULL;
+	if (strcmp(id->id, "SMO8500") == 0)
+		*is_smo8500_device = true;
 	*chipset = (enum kx_chipset)id->driver_data;
 
 	return dev_name(dev);
@@ -1149,6 +1153,8 @@ static int kxcjk1013_gpio_probe(struct i2c_client *client,
 
 	if (!client)
 		return -EINVAL;
+	if (data->is_smo8500_device)
+		return -ENOTSUPP;
 
 	dev = &client->dev;
 
@@ -1198,7 +1204,8 @@ static int kxcjk1013_probe(struct i2c_client *client,
 		name = id->name;
 	} else if (ACPI_HANDLE(&client->dev)) {
 		name = kxcjk1013_match_acpi_device(&client->dev,
-						   &data->chipset);
+						   &data->chipset,
+						   &data->is_smo8500_device);
 	} else
 		return -ENODEV;
 
@@ -1397,6 +1404,7 @@ static const struct acpi_device_id kx_acpi_match[] = {
 	{"KXCJ1013", KXCJK1013},
 	{"KXCJ1008", KXCJ91008},
 	{"KXTJ1009", KXTJ21009},
+	{"SMO8500",  KXCJK1013},
 	{ },
 };
 MODULE_DEVICE_TABLE(acpi, kx_acpi_match);
@@ -1405,6 +1413,7 @@ static const struct i2c_device_id kxcjk1013_id[] = {
 	{"kxcjk1013", KXCJK1013},
 	{"kxcj91008", KXCJ91008},
 	{"kxtj21009", KXTJ21009},
+	{"SMO8500",   KXCJK1013},
 	{}
 };
 
-- 
2.1.0

Re: [PATCH v3] iio:kxcjk-1013: Add support for SMO8500 device

[Jonathan Cameron]

On 03/11/14 23:20, Bastien Nocera wrote:
> The Onda v975w tablet contains an accelerometer that's advertised over
> ACPI as SMO8500. This device is however a KXCJ9 accelerometer as
> can be seen in the Windows driver's INF file, and from the etching on
> the chipset ("KXCJ9 41566 0414").
> 
> This patch also removes the attempt to get the IRQ for the "data ready"
> signal, as it does not seem to be supported by this device on this
> platform.
> 
> Signed-off-by: Bastien Nocera <hadess@hadess.net>
Looks fine to me, but I'd ideally like an ack / review from Srinivas as it's his
driver... (and/or Daniel)
> 
> ---
> 
> Changes from v1:
> - Rebase after driver changes
> - Stop checking for IRQ when not supported
> 
> Changes from v2:
> - Fixed the space before the opening bracket
> - Added Sob line
> - Add version to the patch subject line ;)
> 
> ---
>  drivers/iio/accel/kxcjk-1013.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
> index 98909a9..3e966c2 100644
> --- a/drivers/iio/accel/kxcjk-1013.c
> +++ b/drivers/iio/accel/kxcjk-1013.c
> @@ -108,6 +108,7 @@ struct kxcjk1013_data {
>  	bool motion_trigger_on;
>  	int64_t timestamp;
>  	enum kx_chipset chipset;
> +	bool is_smo8500_device;
>  };
>  
>  enum kxcjk1013_axis {
> @@ -1129,12 +1130,15 @@ static irqreturn_t kxcjk1013_data_rdy_trig_poll(int irq, void *private)
>  }
>  
>  static const char *kxcjk1013_match_acpi_device(struct device *dev,
> -					       enum kx_chipset *chipset)
> +					       enum kx_chipset *chipset,
> +					       bool *is_smo8500_device)
>  {
>  	const struct acpi_device_id *id;
>  	id = acpi_match_device(dev->driver->acpi_match_table, dev);
>  	if (!id)
>  		return NULL;
> +	if (strcmp(id->id, "SMO8500") == 0)
> +		*is_smo8500_device = true;
>  	*chipset = (enum kx_chipset)id->driver_data;
>  
>  	return dev_name(dev);
> @@ -1149,6 +1153,8 @@ static int kxcjk1013_gpio_probe(struct i2c_client *client,
>  
>  	if (!client)
>  		return -EINVAL;
> +	if (data->is_smo8500_device)
> +		return -ENOTSUPP;
>  
>  	dev = &client->dev;
>  
> @@ -1198,7 +1204,8 @@ static int kxcjk1013_probe(struct i2c_client *client,
>  		name = id->name;
>  	} else if (ACPI_HANDLE(&client->dev)) {
>  		name = kxcjk1013_match_acpi_device(&client->dev,
> -						   &data->chipset);
> +						   &data->chipset,
> +						   &data->is_smo8500_device);
>  	} else
>  		return -ENODEV;
>  
> @@ -1397,6 +1404,7 @@ static const struct acpi_device_id kx_acpi_match[] = {
>  	{"KXCJ1013", KXCJK1013},
>  	{"KXCJ1008", KXCJ91008},
>  	{"KXTJ1009", KXTJ21009},
> +	{"SMO8500",  KXCJK1013},
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(acpi, kx_acpi_match);
> @@ -1405,6 +1413,7 @@ static const struct i2c_device_id kxcjk1013_id[] = {
>  	{"kxcjk1013", KXCJK1013},
>  	{"kxcj91008", KXCJ91008},
>  	{"kxtj21009", KXTJ21009},
> +	{"SMO8500",   KXCJK1013},
>  	{}
>  };
>  
> 

Re: [PATCH v3] iio:kxcjk-1013: Add support for SMO8500 device

[Daniel Baluta]

On Wed, Nov 5, 2014 at 4:38 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 03/11/14 23:20, Bastien Nocera wrote:
>> The Onda v975w tablet contains an accelerometer that's advertised over
>> ACPI as SMO8500. This device is however a KXCJ9 accelerometer as
>> can be seen in the Windows driver's INF file, and from the etching on
>> the chipset ("KXCJ9 41566 0414").
>>
>> This patch also removes the attempt to get the IRQ for the "data ready"
>> signal, as it does not seem to be supported by this device on this
>> platform.

I don't understand exactly why you remove the attempt to get IRQ? If this is
not supported on your device than kxcjk1013_gpio_probe should return
a negative value. Then, all IRQ handling is done only if client->irq >= 0 and
you should be safe.

>>
>> Signed-off-by: Bastien Nocera <hadess@hadess.net>
> Looks fine to me, but I'd ideally like an ack / review from Srinivas as it's his
> driver... (and/or Daniel)
>>
>> ---

Do not add scissor line by hand.

>>
>> Changes from v1:
>> - Rebase after driver changes
>> - Stop checking for IRQ when not supported
>>
>> Changes from v2:
>> - Fixed the space before the opening bracket
>> - Added Sob line
>> - Add version to the patch subject line ;)
>>
>> ---

Add change history here instead.

>>  drivers/iio/accel/kxcjk-1013.c | 13 +++++++++++--
>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
>> index 98909a9..3e966c2 100644
>> --- a/drivers/iio/accel/kxcjk-1013.c
>> +++ b/drivers/iio/accel/kxcjk-1013.c
>> @@ -108,6 +108,7 @@ struct kxcjk1013_data {
>>       bool motion_trigger_on;
>>       int64_t timestamp;
>>       enum kx_chipset chipset;
>> +     bool is_smo8500_device;
>>  };
>>
>>  enum kxcjk1013_axis {
>> @@ -1129,12 +1130,15 @@ static irqreturn_t kxcjk1013_data_rdy_trig_poll(int irq, void *private)
>>  }
>>
>>  static const char *kxcjk1013_match_acpi_device(struct device *dev,
>> -                                            enum kx_chipset *chipset)
>> +                                            enum kx_chipset *chipset,
>> +                                            bool *is_smo8500_device)
>>  {
>>       const struct acpi_device_id *id;
>>       id = acpi_match_device(dev->driver->acpi_match_table, dev);
>>       if (!id)
>>               return NULL;
>> +     if (strcmp(id->id, "SMO8500") == 0)
>> +             *is_smo8500_device = true;
>>       *chipset = (enum kx_chipset)id->driver_data;
>>
>>       return dev_name(dev);
>> @@ -1149,6 +1153,8 @@ static int kxcjk1013_gpio_probe(struct i2c_client *client,
>>
>>       if (!client)
>>               return -EINVAL;
>> +     if (data->is_smo8500_device)
>> +             return -ENOTSUPP;
>>
>>       dev = &client->dev;
>>
>> @@ -1198,7 +1204,8 @@ static int kxcjk1013_probe(struct i2c_client *client,
>>               name = id->name;
>>       } else if (ACPI_HANDLE(&client->dev)) {
>>               name = kxcjk1013_match_acpi_device(&client->dev,
>> -                                                &data->chipset);
>> +                                                &data->chipset,
>> +                                                &data->is_smo8500_device);
>>       } else
>>               return -ENODEV;
>>
>> @@ -1397,6 +1404,7 @@ static const struct acpi_device_id kx_acpi_match[] = {
>>       {"KXCJ1013", KXCJK1013},
>>       {"KXCJ1008", KXCJ91008},
>>       {"KXTJ1009", KXTJ21009},
>> +     {"SMO8500",  KXCJK1013},

Commit message says that your accel is KXCJ9, but here you use the index
for KXCJK1013. Which one is true?

>>       { },
>>  };
>>  MODULE_DEVICE_TABLE(acpi, kx_acpi_match);
>> @@ -1405,6 +1413,7 @@ static const struct i2c_device_id kxcjk1013_id[] = {
>>       {"kxcjk1013", KXCJK1013},
>>       {"kxcj91008", KXCJ91008},
>>       {"kxtj21009", KXTJ21009},
>> +     {"SMO8500",   KXCJK1013},

Ditto.

>>       {}
>>  };

I have  recently set up an IRC channel for linux-iio,
I am going to send an official  announce email
as soon as possible (not sure if this channel is
needed though :D).

You can join it now at:
* server: irc.oftc.net
* channel: #linux-iio

There is also an web interface available:

http://webchat.oftc.net/

thanks,
Daniel.

Re: [PATCH v3] iio:kxcjk-1013: Add support for SMO8500 device

[Bastien Nocera]

On Wed, 2014-11-05 at 17:22 +0200, Daniel Baluta wrote:
> On Wed, Nov 5, 2014 at 4:38 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> > On 03/11/14 23:20, Bastien Nocera wrote:
> >> The Onda v975w tablet contains an accelerometer that's advertised over
> >> ACPI as SMO8500. This device is however a KXCJ9 accelerometer as
> >> can be seen in the Windows driver's INF file, and from the etching on
> >> the chipset ("KXCJ9 41566 0414").
> >>
> >> This patch also removes the attempt to get the IRQ for the "data ready"
> >> signal, as it does not seem to be supported by this device on this
> >> platform.
> 
> I don't understand exactly why you remove the attempt to get IRQ? If this is
> not supported on your device than kxcjk1013_gpio_probe should return
> a negative value.

Yep, and I see a warning in dmesg when the device is actually working
correctly:
                dev_err(dev, "acpi gpio get index failed\n");

I don't want to see that warning. Either you can downgrade that error to
a debug message, in which case it might silently fail when it should
have warned, or you special case so that this code isn't run.

>  Then, all IRQ handling is done only if client->irq >= 0 and
> you should be safe.
> 
> >>
> >> Signed-off-by: Bastien Nocera <hadess@hadess.net>
> > Looks fine to me, but I'd ideally like an ack / review from Srinivas as it's his
> > driver... (and/or Daniel)
> >>
> >> ---
> 
> Do not add scissor line by hand.
> 
> >>
> >> Changes from v1:
> >> - Rebase after driver changes
> >> - Stop checking for IRQ when not supported
> >>
> >> Changes from v2:
> >> - Fixed the space before the opening bracket
> >> - Added Sob line
> >> - Add version to the patch subject line ;)
> >>
> >> ---
> 
> Add change history here instead.

Sure. Need me to send it again for that?

> >>  drivers/iio/accel/kxcjk-1013.c | 13 +++++++++++--
> >>  1 file changed, 11 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
> >> index 98909a9..3e966c2 100644
> >> --- a/drivers/iio/accel/kxcjk-1013.c
> >> +++ b/drivers/iio/accel/kxcjk-1013.c
> >> @@ -108,6 +108,7 @@ struct kxcjk1013_data {
> >>       bool motion_trigger_on;
> >>       int64_t timestamp;
> >>       enum kx_chipset chipset;
> >> +     bool is_smo8500_device;
> >>  };
> >>
> >>  enum kxcjk1013_axis {
> >> @@ -1129,12 +1130,15 @@ static irqreturn_t kxcjk1013_data_rdy_trig_poll(int irq, void *private)
> >>  }
> >>
> >>  static const char *kxcjk1013_match_acpi_device(struct device *dev,
> >> -                                            enum kx_chipset *chipset)
> >> +                                            enum kx_chipset *chipset,
> >> +                                            bool *is_smo8500_device)
> >>  {
> >>       const struct acpi_device_id *id;
> >>       id = acpi_match_device(dev->driver->acpi_match_table, dev);
> >>       if (!id)
> >>               return NULL;
> >> +     if (strcmp(id->id, "SMO8500") == 0)
> >> +             *is_smo8500_device = true;
> >>       *chipset = (enum kx_chipset)id->driver_data;
> >>
> >>       return dev_name(dev);
> >> @@ -1149,6 +1153,8 @@ static int kxcjk1013_gpio_probe(struct i2c_client *client,
> >>
> >>       if (!client)
> >>               return -EINVAL;
> >> +     if (data->is_smo8500_device)
> >> +             return -ENOTSUPP;
> >>
> >>       dev = &client->dev;
> >>
> >> @@ -1198,7 +1204,8 @@ static int kxcjk1013_probe(struct i2c_client *client,
> >>               name = id->name;
> >>       } else if (ACPI_HANDLE(&client->dev)) {
> >>               name = kxcjk1013_match_acpi_device(&client->dev,
> >> -                                                &data->chipset);
> >> +                                                &data->chipset,
> >> +                                                &data->is_smo8500_device);
> >>       } else
> >>               return -ENODEV;
> >>
> >> @@ -1397,6 +1404,7 @@ static const struct acpi_device_id kx_acpi_match[] = {
> >>       {"KXCJ1013", KXCJK1013},
> >>       {"KXCJ1008", KXCJ91008},
> >>       {"KXTJ1009", KXTJ21009},
> >> +     {"SMO8500",  KXCJK1013},
> 
> Commit message says that your accel is KXCJ9, but here you use the index
> for KXCJK1013. Which one is true?
> 
> >>       { },
> >>  };
> >>  MODULE_DEVICE_TABLE(acpi, kx_acpi_match);
> >> @@ -1405,6 +1413,7 @@ static const struct i2c_device_id kxcjk1013_id[] = {
> >>       {"kxcjk1013", KXCJK1013},
> >>       {"kxcj91008", KXCJ91008},
> >>       {"kxtj21009", KXTJ21009},
> >> +     {"SMO8500",   KXCJK1013},
> 
> Ditto.
> 
> >>       {}
> >>  };
> 
> I have  recently set up an IRC channel for linux-iio,
> I am going to send an official  announce email
> as soon as possible (not sure if this channel is
> needed though :D).
> 
> You can join it now at:
> * server: irc.oftc.net
> * channel: #linux-iio
> 
> There is also an web interface available:
> 
> http://webchat.oftc.net/
> 
> thanks,
> Daniel.

Re: [PATCH v3] iio:kxcjk-1013: Add support for SMO8500 device

[Daniel Baluta]

On Wed, Nov 5, 2014 at 5:28 PM, Bastien Nocera <hadess@hadess.net> wrote:
> On Wed, 2014-11-05 at 17:22 +0200, Daniel Baluta wrote:
>> On Wed, Nov 5, 2014 at 4:38 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>> > On 03/11/14 23:20, Bastien Nocera wrote:
>> >> The Onda v975w tablet contains an accelerometer that's advertised over
>> >> ACPI as SMO8500. This device is however a KXCJ9 accelerometer as
>> >> can be seen in the Windows driver's INF file, and from the etching on
>> >> the chipset ("KXCJ9 41566 0414").
>> >>
>> >> This patch also removes the attempt to get the IRQ for the "data ready"
>> >> signal, as it does not seem to be supported by this device on this
>> >> platform.
>>
>> I don't understand exactly why you remove the attempt to get IRQ? If this is
>> not supported on your device than kxcjk1013_gpio_probe should return
>> a negative value.
>
> Yep, and I see a warning in dmesg when the device is actually working
> correctly:
>                 dev_err(dev, "acpi gpio get index failed\n");

You just add is_smo8500_device member to avoid seeing an error
message. This is not a good idea.

>
> I don't want to see that warning. Either you can downgrade that error to
> a debug message, in which case it might silently fail when it should
> have warned, or you special case so that this code isn't run.

We should work on this instead. We can make the error message more
clear to really let users know what happened. I would like to hear
Srinivas opinion on this.

>
>>  Then, all IRQ handling is done only if client->irq >= 0 and
>> you should be safe.
>>
>> >>
>> >> Signed-off-by: Bastien Nocera <hadess@hadess.net>
>> > Looks fine to me, but I'd ideally like an ack / review from Srinivas as it's his
>> > driver... (and/or Daniel)
>> >>
>> >> ---
>>
>> Do not add scissor line by hand.
>>
>> >>
>> >> Changes from v1:
>> >> - Rebase after driver changes
>> >> - Stop checking for IRQ when not supported
>> >>
>> >> Changes from v2:
>> >> - Fixed the space before the opening bracket
>> >> - Added Sob line
>> >> - Add version to the patch subject line ;)
>> >>
>> >> ---
>>
>> Add change history here instead.
>
> Sure. Need me to send it again for that?

No need for the moment. Just for future patches.

>
>> >>  drivers/iio/accel/kxcjk-1013.c | 13 +++++++++++--
>> >>  1 file changed, 11 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
>> >> index 98909a9..3e966c2 100644
>> >> --- a/drivers/iio/accel/kxcjk-1013.c
>> >> +++ b/drivers/iio/accel/kxcjk-1013.c
>> >> @@ -108,6 +108,7 @@ struct kxcjk1013_data {
>> >>       bool motion_trigger_on;
>> >>       int64_t timestamp;
>> >>       enum kx_chipset chipset;
>> >> +     bool is_smo8500_device;
>> >>  };
>> >>
>> >>  enum kxcjk1013_axis {
>> >> @@ -1129,12 +1130,15 @@ static irqreturn_t kxcjk1013_data_rdy_trig_poll(int irq, void *private)
>> >>  }
>> >>
>> >>  static const char *kxcjk1013_match_acpi_device(struct device *dev,
>> >> -                                            enum kx_chipset *chipset)
>> >> +                                            enum kx_chipset *chipset,
>> >> +                                            bool *is_smo8500_device)
>> >>  {
>> >>       const struct acpi_device_id *id;
>> >>       id = acpi_match_device(dev->driver->acpi_match_table, dev);
>> >>       if (!id)
>> >>               return NULL;
>> >> +     if (strcmp(id->id, "SMO8500") == 0)
>> >> +             *is_smo8500_device = true;
>> >>       *chipset = (enum kx_chipset)id->driver_data;
>> >>
>> >>       return dev_name(dev);
>> >> @@ -1149,6 +1153,8 @@ static int kxcjk1013_gpio_probe(struct i2c_client *client,
>> >>
>> >>       if (!client)
>> >>               return -EINVAL;
>> >> +     if (data->is_smo8500_device)
>> >> +             return -ENOTSUPP;
>> >>
>> >>       dev = &client->dev;
>> >>
>> >> @@ -1198,7 +1204,8 @@ static int kxcjk1013_probe(struct i2c_client *client,
>> >>               name = id->name;
>> >>       } else if (ACPI_HANDLE(&client->dev)) {
>> >>               name = kxcjk1013_match_acpi_device(&client->dev,
>> >> -                                                &data->chipset);
>> >> +                                                &data->chipset,
>> >> +                                                &data->is_smo8500_device);
>> >>       } else
>> >>               return -ENODEV;
>> >>
>> >> @@ -1397,6 +1404,7 @@ static const struct acpi_device_id kx_acpi_match[] = {
>> >>       {"KXCJ1013", KXCJK1013},
>> >>       {"KXCJ1008", KXCJ91008},
>> >>       {"KXTJ1009", KXTJ21009},
>> >> +     {"SMO8500",  KXCJK1013},
>>
>> Commit message says that your accel is KXCJ9, but here you use the index
>> for KXCJK1013. Which one is true?

What about this?

Re: [PATCH v3] iio:kxcjk-1013: Add support for SMO8500 device

[Bastien Nocera]

On Wed, 2014-11-05 at 17:22 +0200, Daniel Baluta wrote:
> >> @@ -1397,6 +1404,7 @@ static const struct acpi_device_id
> kx_acpi_match[] = {
> >>       {"KXCJ1013", KXCJK1013},
> >>       {"KXCJ1008", KXCJ91008},
> >>       {"KXTJ1009", KXTJ21009},
> >> +     {"SMO8500",  KXCJK1013},
> 
> Commit message says that your accel is KXCJ9, but here you use the
> index
> for KXCJK1013. Which one is true?
> 
> >>       { },
> >>  };
> >>  MODULE_DEVICE_TABLE(acpi, kx_acpi_match);
> >> @@ -1405,6 +1413,7 @@ static const struct i2c_device_id
> kxcjk1013_id[] = {
> >>       {"kxcjk1013", KXCJK1013},
> >>       {"kxcj91008", KXCJ91008},
> >>       {"kxtj21009", KXTJ21009},
> >> +     {"SMO8500",   KXCJK1013},
> 
> Ditto.

Missed this. Will respin a patch after testing, though the differences
in code between the 2 devices are very minimal it seems (only some PM
functionality).