iio:kxcjk-1013: Add support for SMO8500 device

PATCH Superseded Bastien Nocera 3 participants 6 messages Nov 3, 2014 – Nov 5, 2014 IIO

Share & tools

Bastien Nocera

author

2014-11-03 23:20 UTC #0 lore raw

PATCH v3 1/1 iio:kxcjk-1013: Add support for SMO8500 device

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 <[email protected]>

---

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(-)

drivers/iio/accel/kxcjk-1013.c +11 -2

index 98909a9..3e966c2 100644
@@ -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},
 	{}
 };

Signature · 1 line

2.1.0

Jonathan Cameron

maintainer

2014-11-05 14:38 UTC #1 lore raw

Quoted 10 lines · 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 <[email protected]>

Looks fine to me, but I'd ideally like an ack / review from Srinivas as it's his driver... (and/or Daniel)

Quoted 81 lines

---

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(-)

drivers/iio/accel/kxcjk-1013.c +11 -2

index 98909a9..3e966c2 100644
@@ -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},
 	{}
 };

Daniel Baluta

2014-11-05 15:22 UTC #2 lore raw

Quoted 9 lines · Jonathan Cameron

Quoted 8 lines · 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.

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.

Quoted 6 lines

Quoted 2 lines
Signed-off-by Bastien Nocera <[email protected]>
Looks fine to me, but I'd ideally like an ack / review from Srinivas as it's his driver... (and/or Daniel)
Quoted 2 lines
---

Do not add scissor line by hand.

Quoted 11 lines

Quoted 11 lines
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.

Quoted 51 lines

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

drivers/iio/accel/kxcjk-1013.c +9 -2

index 98909a9..3e966c2 100644
@@ -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;

Quoted 5 lines

drivers/iio/accel/kxcjk-1013.c static const struct acpi_device_id kx_acpi_match[] = { · line 1407
Quoted 5 lines
drivers/iio/accel/kxcjk-1013.c +1 -0
@@ -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?

Quoted 8 lines

drivers/iio/accel/kxcjk-1013.c static const struct i2c_device_id kxcjk1013_id[] = { · line 1416

Quoted 8 lines

drivers/iio/accel/kxcjk-1013.c +1 -0

      { },
 };
 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.

Quoted 2 lines

drivers/iio/accel/kxcjk-1013.c static const struct i2c_device_id kxcjk1013_id[] = { · line 1418
Quoted 2 lines
drivers/iio/accel/kxcjk-1013.c +0 -0
      {}
 };

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.

Bastien Nocera

author

2014-11-05 15:28 UTC #3 lore raw

Quoted 7 lines · Daniel Baluta

Quoted 6 lines · Jonathan Cameron
Quoted 5 lines · 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").

Quoted 7 lines

Quoted 3 lines
Quoted 3 lines
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.

Quoted 17 lines

Then, all IRQ handling is done only if client->irq >= 0 and
you should be safe.
Quoted 6 lines
Quoted 2 lines
Signed-off-by Bastien Nocera <[email protected]>
Looks fine to me, but I'd ideally like an ack / review from Srinivas as it's his driver... (and/or Daniel)
Quoted 2 lines
---
Do not add scissor line by hand.
Quoted 5 lines
Quoted 5 lines
Changes from v1:
- Rebase after driver changes
- Stop checking for IRQ when not supported

Quoted 8 lines

Quoted 6 lines
Quoted 6 lines
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?

Quoted 89 lines

Quoted 51 lines

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

drivers/iio/accel/kxcjk-1013.c +9 -2

index 98909a9..3e966c2 100644
@@ -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;

Quoted 5 lines

drivers/iio/accel/kxcjk-1013.c static const struct acpi_device_id kx_acpi_match[] = { · line 1407
Quoted 5 lines
drivers/iio/accel/kxcjk-1013.c +1 -0
@@ -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?

Quoted 8 lines

drivers/iio/accel/kxcjk-1013.c static const struct i2c_device_id kxcjk1013_id[] = { · line 1416

Quoted 8 lines

drivers/iio/accel/kxcjk-1013.c +1 -0

      { },
 };
 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.

Quoted 2 lines

drivers/iio/accel/kxcjk-1013.c static const struct i2c_device_id kxcjk1013_id[] = { · line 1418
Quoted 2 lines
drivers/iio/accel/kxcjk-1013.c +0 -0
      {}
 };

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.

Daniel Baluta

2014-11-05 15:46 UTC #4 lore raw

Quoted 12 lines · Bastien Nocera

Quoted 11 lines · Daniel Baluta
Quoted 9 lines · Jonathan Cameron
Quoted 8 lines · 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.

Quoted 7 lines

Quoted 3 lines
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.

Quoted 4 lines

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.

Quoted 23 lines

Quoted 23 lines
Then, all IRQ handling is done only if client->irq >= 0 and
you should be safe.
Quoted 6 lines
Quoted 2 lines
Signed-off-by Bastien Nocera <[email protected]>
Looks fine to me, but I'd ideally like an ack / review from Srinivas as it's his driver... (and/or Daniel)
Quoted 2 lines
---
Do not add scissor line by hand.
Quoted 10 lines
Quoted 10 lines
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 ;)

Quoted 5 lines

Quoted 3 lines
Quoted 1 line
Quoted 1 line
---
Add change history here instead.
Sure. Need me to send it again for that?

No need for the moment. Just for future patches.

Quoted 52 lines

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

drivers/iio/accel/kxcjk-1013.c +9 -2

index 98909a9..3e966c2 100644
@@ -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;

Quoted 8 lines

Quoted 8 lines
Quoted 5 lines
drivers/iio/accel/kxcjk-1013.c static const struct acpi_device_id kx_acpi_match[] = { · line 1407
Quoted 5 lines
drivers/iio/accel/kxcjk-1013.c +1 -0
@@ -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?

Bastien Nocera

author

2014-11-05 15:48 UTC #5 lore raw

In reply to Daniel Baluta

Quoted 16 lines · Daniel Baluta

Quoted 1 line
Review static const struct acpi_device_id
Quoted 1 line
(quoted diff) +0 -0
@@ -1397,6 +1404,7 @@ static const struct acpi_device_id
kx_acpi_match[] = {
Quoted 4 lines
drivers/iio/accel/kxcjk-1013.c static const struct acpi_device_id kx_acpi_match[] = { · line 1407
Quoted 4 lines
drivers/iio/accel/kxcjk-1013.c +1 -0
      {"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?
Quoted 4 lines
drivers/iio/accel/kxcjk-1013.c static const struct i2c_device_id
Quoted 4 lines
drivers/iio/accel/kxcjk-1013.c +0 -0
      { },
 };
 MODULE_DEVICE_TABLE(acpi, kx_acpi_match);
@@ -1405,6 +1413,7 @@ static const struct i2c_device_id
kxcjk1013_id[] = {

Quoted 6 lines

Quoted 4 lines
drivers/iio/accel/kxcjk-1013.c static const struct i2c_device_id kxcjk1013_id[] = { · line 1416
Quoted 4 lines
drivers/iio/accel/kxcjk-1013.c +1 -0
      {"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).