iio : Add cm3218 smbus ara and acpi support

On asus T100, Capella cm3218 chip is implemented as ambiant light sensor. This chip expose an smbus ARA protocol device on standard address 0x0c. The chip is not functional before all alerts are acknowledged.
On asus T100, this device is enumerated on ACPI bus and the description give tow I2C connection. The first is the connection to the ARA device and the second gives the real address of the device.

So, on device probe,If the i2c address is the ARA address and the device is enumerated via acpi, we lookup for the real address in the ACPI resource list and change it in the client structure. if an interrupt resource is given, and only for cm3218 chip, we declare an smbus_alert device.

Signed-off-by Marc CAPDEVILLE <[email protected]>

---
v4 :
- rework acpi i2c adress lookup due to bad commit being sent.

v3 :
- rework acpi i2c adress lookup
- comment style cleanup
- add prefix cm32181_to constantes and macros

v2 :
- cm3218 support always build
- Cleanup of unneeded #if statement
- Beter identifying chip in platform device, acpi and of_device

 drivers/iio/light/cm32181.c | 202 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 185 insertions(+), 17 deletions(-)

drivers/iio/light/cm32181.c +0 -0

index d6fd0dace74f..f1a7495e851b 100644

drivers/iio/light/cm32181.c +80 -14

 #define CM32181_REG_ADDR_CMD		0x00
 #define CM32181_REG_ADDR_ALS		0x04
 #define CM32181_REG_ADDR_STATUS		0x06

@@ -45,18 +49,25 @@
 #define CM32181_MLUX_PER_BIT_BASE_IT	800000	/* Based on IT=800ms */
 #define	CM32181_CALIBSCALE_DEFAULT	1000
 #define CM32181_CALIBSCALE_RESOLUTION	1000
-#define MLUX_PER_LUX			1000
+#define CM32181_MLUX_PER_LUX			1000
+
+#define CM32181_ID			0x81
+#define CM3218_ID			0x18
+
+#define CM3218_ARA_ADDR			0x0c
 
 static const u8 cm32181_reg[CM32181_CONF_REG_NUM] = {
 	CM32181_REG_ADDR_CMD,
 };
 
-static const int als_it_bits[] = {12, 8, 0, 1, 2, 3};
-static const int als_it_value[] = {25000, 50000, 100000, 200000, 400000,
+static const int cm32181_als_it_bits[] = {12, 8, 0, 1, 2, 3};
+static const int cm32181_als_it_value[] = {25000, 50000, 100000, 200000, 400000,
 	800000};
 
 struct cm32181_chip {
 	struct i2c_client *client;
+	int chip_id;
+	struct i2c_client *ara;
 	struct mutex lock;
 	u16 conf_regs[CM32181_CONF_REG_NUM];
 	int calibscale;

@@ -81,7 +92,7 @@ static int cm32181_reg_init(struct cm32181_chip *cm32181)
 		return ret;
 
 	/* check device ID */
-	if ((ret & 0xFF) != 0x81)
+	if ((ret & 0xFF) != cm32181->chip_id)
 		return -ENODEV;
 
 	/* Default Values */

@@ -103,7 +114,7 @@ static int cm32181_reg_init(struct cm32181_chip *cm32181)
 /**
  *  cm32181_read_als_it() - Get sensor integration time (ms)
  *  @cm32181:	pointer of struct cm32181
- *  @val2:	pointer of int to load the als_it value.
+ *  @val2:	pointer of int to load the cm32181_als_it value.
  *
  *  Report the current integartion time by millisecond.
  *

@@ -117,9 +128,9 @@ static int cm32181_read_als_it(struct cm32181_chip *cm32181, int *val2)
 	als_it = cm32181->conf_regs[CM32181_REG_ADDR_CMD];
 	als_it &= CM32181_CMD_ALS_IT_MASK;
 	als_it >>= CM32181_CMD_ALS_IT_SHIFT;
-	for (i = 0; i < ARRAY_SIZE(als_it_bits); i++) {
-		if (als_it == als_it_bits[i]) {
-			*val2 = als_it_value[i];
+	for (i = 0; i < ARRAY_SIZE(cm32181_als_it_bits); i++) {
+		if (als_it == cm32181_als_it_bits[i]) {
+			*val2 = cm32181_als_it_value[i];
 			return IIO_VAL_INT_PLUS_MICRO;
 		}
 	}

@@ -142,14 +153,14 @@ static int cm32181_write_als_it(struct cm32181_chip *cm32181, int val)
 	u16 als_it;
 	int ret, i, n;
 
-	n = ARRAY_SIZE(als_it_value);
+	n = ARRAY_SIZE(cm32181_als_it_value);
 	for (i = 0; i < n; i++)
-		if (val <= als_it_value[i])
+		if (val <= cm32181_als_it_value[i])
 			break;
 	if (i >= n)
 		i = n - 1;
 
-	als_it = als_it_bits[i];
+	als_it = cm32181_als_it_bits[i];
 	als_it <<= CM32181_CMD_ALS_IT_SHIFT;
 
 	mutex_lock(&cm32181->lock);

@@ -195,7 +206,7 @@ static int cm32181_get_lux(struct cm32181_chip *cm32181)
 	lux *= ret;
 	lux *= cm32181->calibscale;
 	lux /= CM32181_CALIBSCALE_RESOLUTION;
-	lux /= MLUX_PER_LUX;
+	lux /= CM32181_MLUX_PER_LUX;
 
 	if (lux > 0xFFFF)
 		lux = 0xFFFF;

@@ -263,9 +274,9 @@ static ssize_t cm32181_get_it_available(struct device *dev,
 {
 	int i, n, len;
 
-	n = ARRAY_SIZE(als_it_value);
+	n = ARRAY_SIZE(cm32181_als_it_value);
 	for (i = 0, len = 0; i < n; i++)
-		len += sprintf(buf + len, "0.%06u ", als_it_value[i]);
+		len += sprintf(buf + len, "0.%06u ", cm32181_als_it_value[i]);
 	return len + sprintf(buf + len, "\n");
 }
 

@@ -298,12 +309,89 @@ static const struct iio_info cm32181_info = {
 	.attrs			= &cm32181_attribute_group,
 };
 
+#if defined(CONFIG_ACPI)
+/* Filter acpi resources for a real i2c address on same adapter */
+static int cm3218_filter_i2c_address(struct acpi_resource *ares, void *data)
+{
+	struct i2c_client *client = data;
+	struct acpi_resource_i2c_serialbus *sb;
+	acpi_status status;
+	acpi_handle adapter_handle;
+
+	if (ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
+		return 1;
+
+	sb = &ares->data.i2c_serial_bus;
+	if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_I2C)
+		return 1;
+
+	status = acpi_get_handle(ACPI_HANDLE(&client->dev),
+				 sb->resource_source.string_ptr,
+				 &adapter_handle);
+	if (ACPI_FAILURE(status))
+		return status;
+
+	if (adapter_handle != ACPI_HANDLE(&client->adapter->dev))
+		return 1;
+
+	if (sb->slave_address == CM3218_ARA_ADDR)
+		return 1;
+
+	return AE_OK;
+}
+
+/* Get the real i2c address from acpi resources */
+static int cm3218_acpi_get_address(struct i2c_client *client)
+{
+	int ret;
+	struct acpi_device *adev;
+	LIST_HEAD(res_list);
+	struct resource_entry *res_entry;
+	struct acpi_resource *ares;
+	struct acpi_resource_i2c_serialbus *sb;
+
+	adev = ACPI_COMPANION(&client->dev);
+	if (!adev)
+		return -ENODEV;
+
+	ret = acpi_dev_get_resources(adev,
+				     &res_list,
+				     cm3218_filter_i2c_address,
+				     client);
+	if (ret < 0)
+		return ret;
+
+	/* get first resource */
+	res_entry = list_entry(&res_list, struct resource_entry, node);
+	ares = (struct acpi_resource *)res_entry->res;
+	sb = &ares->data.i2c_serial_bus;
+
+	/* set i2c address */
+	client->addr = sb->slave_address;

drivers/iio/light/cm32181.c +90 -2

+{
+	return -ENODEV;
+}
+#endif
+
 static int cm32181_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
 	struct cm32181_chip *cm32181;
 	struct iio_dev *indio_dev;
 	int ret;
+	struct i2c_smbus_alert_setup ara_setup;
+	const struct of_device_id *of_id;
+	const struct acpi_device_id *acpi_id;
 
 	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*cm32181));
 	if (!indio_dev) {

@@ -323,11 +411,65 @@ static int cm32181_probe(struct i2c_client *client,
 	indio_dev->name = id->name;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
+	/* Lookup for chip ID from platform, acpi or of device table */
+	if (id) {
+		cm32181->chip_id = id->driver_data;
+	} else if (ACPI_COMPANION(&client->dev)) {
+		acpi_id = acpi_match_device(client->dev.driver->acpi_match_table,
+					    &client->dev);
+		if (!acpi_id)
+			return -ENODEV;
+
+		cm32181->chip_id = (kernel_ulong_t)acpi_id->driver_data;
+	} else if (client->dev.of_node) {
+		of_id = of_match_device(clients->dev.driver->of_match_table,
+					&client->dev);
+		if (!of_id)
+			return -ENODEV;
+
+		cm32181->chip_id = (kernel_ulong_t)of_id->data;
+	} else {
+		return -ENODEV;
+	}
+
+	if (cm32181->chip_id == CM3218_ID) {
+		if (client->addr == CM3218_ARA_ADDR) {
+			/*
+			 * In case first address is the ARA device
+			 * lookup for a second one in acpi resources if
+			 * this client is enumerated on acpi
+			 */
+			ret = cm3218_acpi_get_address(client);
+			if (ret < 0)
+				return -ENODEV;
+		}
+
+#ifdef CONFIG_I2C_SMBUS
+		if (client->irq > 0) {
+			/* setup smb alert device */
+			ara_setup.irq = client->irq;
+			ara_setup.alert_edge_triggered = 0;
+			cm32181->ara = i2c_setup_smbus_alert(client->adapter,
+							     &ara_setup);
+			if (!cm32181->ara)
+				return -ENODEV;
+		} else {
+			return -ENODEV;
+		}
+#else
+		return -ENODEV;
+#endif
+	}
+
 	ret = cm32181_reg_init(cm32181);
 	if (ret) {
 		dev_err(&client->dev,
 			"%s: register init failed\n",
 			__func__);
+
+		if (cm32181->ara)
+			i2c_unregister_device(cm32181->ara);
+
 		return ret;
 	}
 

@@ -336,32 +478,58 @@ static int cm32181_probe(struct i2c_client *client,
 		dev_err(&client->dev,
 			"%s: regist device failed\n",
 			__func__);
+
+		if (cm32181->ara)
+			i2c_unregister_device(cm32181->ara);
+
 		return ret;
 	}
 
 	return 0;
 }
 
+static int cm32181_remove(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+	struct cm32181_chip *cm32181 = iio_priv(indio_dev);
+
+	if (cm32181->ara)
+		i2c_unregister_device(cm32181->ara);
+
+	return 0;
+};
+
 static const struct i2c_device_id cm32181_id[] = {
-	{ "cm32181", 0 },
+	{ "cm32181", CM32181_ID },
+	{ "cm3218", CM3218_ID },
 	{ }
 };
 
 MODULE_DEVICE_TABLE(i2c, cm32181_id);
 
 static const struct of_device_id cm32181_of_match[] = {
-	{ .compatible = "capella,cm32181" },
+	{ .compatible = "capella,cm32181", (void *)CM32181_ID },
+	{ .compatible = "capella,cm3218",  (void *)CM3218_ID },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, cm32181_of_match);
 
+static const struct acpi_device_id cm32181_acpi_match[] = {
+	{ "CPLM3218", CM3218_ID },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(acpi, cm32181_acpi_match);
+
 static struct i2c_driver cm32181_driver = {
 	.driver = {
 		.name	= "cm32181",
 		.of_match_table = of_match_ptr(cm32181_of_match),
+		.acpi_match_table = ACPI_PTR(cm32181_acpi_match),
 	},
 	.id_table       = cm32181_id,
 	.probe		= cm32181_probe,
+	.remove		= cm32181_remove,
 };
 
 module_i2c_driver(cm32181_driver);

On asus T100, Capella cm3218 chip is implemented as ambiant light sensor. This chip expose an smbus ARA protocol device on standard address 0x0c. The chip is not functional before all alerts are acknowledged.
On asus T100, this device is enumerated on ACPI bus and the description give tow I2C connection. The first is the connection to the ARA device and the second gives the real address of the device.

So, on device probe,If the i2c address is the ARA address and the device is enumerated via acpi, we lookup for the real address in the ACPI resource list and change it in the client structure. if an interrupt resource is given, and only for cm3218 chip, we declare an smbus_alert device.

Signed-off-by Marc CAPDEVILLE <[email protected]>

---
v4 :
- rework acpi i2c adress lookup due to bad commit being sent.

v3 :
- rework acpi i2c adress lookup
- comment style cleanup
- add prefix cm32181_to constantes and macros

v2 :
- cm3218 support always build
- Cleanup of unneeded #if statement
- Beter identifying chip in platform device, acpi and of_device

 drivers/iio/light/cm32181.c | 202

++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 185 insertions(+), 17 deletions(-)

drivers/iio/light/cm32181.c +5 -1

index d6fd0dace74f..f1a7495e851b 100644
@@ -18,8 +18,12 @@
 #include <linux/iio/sysfs.h>
 #include <linux/iio/events.h>
 #include <linux/init.h>
+#include <linux/i2c-smbus.h>
+#include <linux/acpi.h>
+#include <linux/of_device.h>
+#include <linux/resource_ext.h>
 
-/* Registers Address */
+/* Registers Addresses */
 #define CM32181_REG_ADDR_CMD		0x00
 #define CM32181_REG_ADDR_ALS		0x04
 #define CM32181_REG_ADDR_STATUS		0x06

@@ -45,18 +49,25 @@
 #define CM32181_MLUX_PER_BIT_BASE_IT	800000	/* Based

on IT=800ms */ #define CM32181_CALIBSCALE_DEFAULT 1000
#define CM32181_CALIBSCALE_RESOLUTION 1000

drivers/iio/light/cm32181.c +6 -3

-#define MLUX_PER_LUX			1000
+#define CM32181_MLUX_PER_LUX			1000
+
+#define CM32181_ID			0x81
+#define CM3218_ID			0x18
+
+#define CM3218_ARA_ADDR			0x0c
 
 static const u8 cm32181_reg[CM32181_CONF_REG_NUM] = {
 	CM32181_REG_ADDR_CMD,
 };
 
-static const int als_it_bits[] = {12, 8, 0, 1, 2, 3};
-static const int als_it_value[] = {25000, 50000, 100000, 200000,

400000, +static const int cm32181_als_it_bits[] = {12, 8, 0, 1, 2, 3}; +static const int cm32181_als_it_value[] = {25000, 50000, 100000, 200000, 400000, 800000};

struct cm32181_chip {

drivers/iio/light/cm32181.c +4 -2

 	struct i2c_client *client;
+	int chip_id;
+	struct i2c_client *ara;
 	struct mutex lock;
 	u16 conf_regs[CM32181_CONF_REG_NUM];
 	int calibscale;

@@ -81,7 +92,7 @@ static int cm32181_reg_init(struct cm32181_chip
 *cm32181) return ret;
 
 	/* check device ID */
-	if ((ret & 0xFF) != 0x81)
+	if ((ret & 0xFF) != cm32181->chip_id)
 		return -ENODEV;
 
 	/* Default Values */

@@ -103,7 +114,7 @@ static int cm32181_reg_init(struct cm32181_chip
 *cm32181) /**
  *  cm32181_read_als_it() - Get sensor integration time (ms)
  *  @cm32181:	pointer of struct cm32181
- *  @val2:	pointer of int to load the als_it value.
+ *  @val2:	pointer of int to load the cm32181_als_it value.
  *
  *  Report the current integartion time by millisecond.
  *

@@ -117,9 +128,9 @@ static int cm32181_read_als_it(struct

cm32181_chip *cm32181, int *val2) als_it = cm32181->conf_regs[CM32181_REG_ADDR_CMD]; als_it &= CM32181_CMD_ALS_IT_MASK; als_it >>= CM32181_CMD_ALS_IT_SHIFT;

drivers/iio/light/cm32181.c +85 -9

-	for (i = 0; i < ARRAY_SIZE(als_it_bits); i++) {
-		if (als_it == als_it_bits[i]) {
-			*val2 = als_it_value[i];
+	for (i = 0; i < ARRAY_SIZE(cm32181_als_it_bits); i++) {
+		if (als_it == cm32181_als_it_bits[i]) {
+			*val2 = cm32181_als_it_value[i];
 			return IIO_VAL_INT_PLUS_MICRO;
 		}
 	}

@@ -142,14 +153,14 @@ static int cm32181_write_als_it(struct
 cm32181_chip *cm32181, int val) u16 als_it;
 	int ret, i, n;
 
-	n = ARRAY_SIZE(als_it_value);
+	n = ARRAY_SIZE(cm32181_als_it_value);
 	for (i = 0; i < n; i++)
-		if (val <= als_it_value[i])
+		if (val <= cm32181_als_it_value[i])
 			break;
 	if (i >= n)
 		i = n - 1;
 
-	als_it = als_it_bits[i];
+	als_it = cm32181_als_it_bits[i];
 	als_it <<= CM32181_CMD_ALS_IT_SHIFT;
 
 	mutex_lock(&cm32181->lock);

@@ -195,7 +206,7 @@ static int cm32181_get_lux(struct cm32181_chip
 *cm32181) lux *= ret;
 	lux *= cm32181->calibscale;
 	lux /= CM32181_CALIBSCALE_RESOLUTION;
-	lux /= MLUX_PER_LUX;
+	lux /= CM32181_MLUX_PER_LUX;
 
 	if (lux > 0xFFFF)
 		lux = 0xFFFF;

@@ -263,9 +274,9 @@ static ssize_t cm32181_get_it_available(struct
 device *dev, {
 	int i, n, len;
 
-	n = ARRAY_SIZE(als_it_value);
+	n = ARRAY_SIZE(cm32181_als_it_value);
 	for (i = 0, len = 0; i < n; i++)
-		len += sprintf(buf + len, "0.%06u ",
 als_it_value[i]);
+		len += sprintf(buf + len, "0.%06u ",
 cm32181_als_it_value[i]); return len + sprintf(buf + len, "\n");
 }

@@ -298,12 +309,89 @@ static const struct iio_info cm32181_info = {
 	.attrs			= &cm32181_attribute_group,
 };
 
+#if defined(CONFIG_ACPI)
+/* Filter acpi resources for a real i2c address on same adapter */
+static int cm3218_filter_i2c_address(struct acpi_resource *ares,
 void *data) +{
+	struct i2c_client *client = data;
+	struct acpi_resource_i2c_serialbus *sb;
+	acpi_status status;
+	acpi_handle adapter_handle;
+
+	if (ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
+		return 1;
+
+	sb = &ares->data.i2c_serial_bus;
+	if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_I2C)
+		return 1;
+
+	status = acpi_get_handle(ACPI_HANDLE(&client->dev),
+				 sb->resource_source.string_ptr,
+				 &adapter_handle);
+	if (ACPI_FAILURE(status))
+		return status;
+
+	if (adapter_handle != ACPI_HANDLE(&client->adapter->dev))
+		return 1;
+
+	if (sb->slave_address == CM3218_ARA_ADDR)
+		return 1;
+
+	return AE_OK;
+}
+
+/* Get the real i2c address from acpi resources */
+static int cm3218_acpi_get_address(struct i2c_client *client)
+{
+	int ret;
+	struct acpi_device *adev;
+	LIST_HEAD(res_list);
+	struct resource_entry *res_entry;
+	struct acpi_resource *ares;
+	struct acpi_resource_i2c_serialbus *sb;
+
+	adev = ACPI_COMPANION(&client->dev);
+	if (!adev)
+		return -ENODEV;
+
+	ret = acpi_dev_get_resources(adev,
+				     &res_list,
+				     cm3218_filter_i2c_address,
+				     client);
+	if (ret < 0)
+		return ret;
+
+	/* get first resource */
+	res_entry = list_entry(&res_list, struct resource_entry,
 node);
+	ares = (struct acpi_resource *)res_entry->res;
+	sb = &ares->data.i2c_serial_bus;
+
+	/* set i2c address */
+	client->addr = sb->slave_address;
+	client->flags &= ~I2C_CLIENT_TEN;
+	if (sb->access_mode == ACPI_I2C_10BIT_MODE)
+		client->flags |= I2C_CLIENT_TEN;
+
+	acpi_dev_free_resource_list(&res_list);
+
+	return 0;
+}
+#else
+static inline int cm3218_acpi_get_address(struct i2c_client *client)
+{
+	return -ENODEV;
+}
+#endif
+
 static int cm32181_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
 	struct cm32181_chip *cm32181;
 	struct iio_dev *indio_dev;
 	int ret;
+	struct i2c_smbus_alert_setup ara_setup;
+	const struct of_device_id *of_id;
+	const struct acpi_device_id *acpi_id;
 
 	indio_dev = devm_iio_device_alloc(&client->dev,

sizeof(*cm32181)); if (!indio_dev) {

drivers/iio/light/cm32181.c +26 -0

@@ -323,11 +411,65 @@ static int cm32181_probe(struct i2c_client
 *client, indio_dev->name = id->name;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
+	/* Lookup for chip ID from platform, acpi or of device table
 */
+	if (id) {
+		cm32181->chip_id = id->driver_data;
+	} else if (ACPI_COMPANION(&client->dev)) {
+		acpi_id =
 acpi_match_device(client->dev.driver->acpi_match_table,
+					    &client->dev);
+		if (!acpi_id)
+			return -ENODEV;
+
+		cm32181->chip_id =
 (kernel_ulong_t)acpi_id->driver_data;
+	} else if (client->dev.of_node) {
+		of_id =
 of_match_device(clients->dev.driver->of_match_table,
+					&client->dev);
+		if (!of_id)
+			return -ENODEV;
+
+		cm32181->chip_id = (kernel_ulong_t)of_id->data;
+	} else {
+		return -ENODEV;
+	}
+
+	if (cm32181->chip_id == CM3218_ID) {
+		if (client->addr == CM3218_ARA_ADDR) {
+			/*
+			 * In case first address is the ARA device
+			 * lookup for a second one in acpi resources

if

drivers/iio/light/cm32181.c +8 -0

+		}
+
+#ifdef CONFIG_I2C_SMBUS
+		if (client->irq > 0) {
+			/* setup smb alert device */
+			ara_setup.irq = client->irq;
+			ara_setup.alert_edge_triggered = 0;
+			cm32181->ara =

i2c_setup_smbus_alert(client->adapter,
+
&ara_setup);

drivers/iio/light/cm32181.c +42 -2

+			if (!cm32181->ara)
+				return -ENODEV;
+		} else {
+			return -ENODEV;
+		}
+#else
+		return -ENODEV;
+#endif
+	}
+
 	ret = cm32181_reg_init(cm32181);
 	if (ret) {
 		dev_err(&client->dev,
 			"%s: register init failed\n",
 			__func__);
+
+		if (cm32181->ara)
+			i2c_unregister_device(cm32181->ara);
+
 		return ret;
 	}

@@ -336,32 +478,58 @@ static int cm32181_probe(struct i2c_client
 *client, dev_err(&client->dev,
 			"%s: regist device failed\n",
 			__func__);
+
+		if (cm32181->ara)
+			i2c_unregister_device(cm32181->ara);
+
 		return ret;
 	}
 
 	return 0;
 }
 
+static int cm32181_remove(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+	struct cm32181_chip *cm32181 = iio_priv(indio_dev);
+
+	if (cm32181->ara)
+		i2c_unregister_device(cm32181->ara);
+
+	return 0;
+};
+
 static const struct i2c_device_id cm32181_id[] = {
-	{ "cm32181", 0 },
+	{ "cm32181", CM32181_ID },
+	{ "cm3218", CM3218_ID },
 	{ }
 };
 
 MODULE_DEVICE_TABLE(i2c, cm32181_id);
 
 static const struct of_device_id cm32181_of_match[] = {
-	{ .compatible = "capella,cm32181" },
+	{ .compatible = "capella,cm32181", (void *)CM32181_ID },
+	{ .compatible = "capella,cm3218",  (void *)CM3218_ID },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, cm32181_of_match);
 
+static const struct acpi_device_id cm32181_acpi_match[] = {
+	{ "CPLM3218", CM3218_ID },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(acpi, cm32181_acpi_match);
+
 static struct i2c_driver cm32181_driver = {
 	.driver = {
 		.name	= "cm32181",
 		.of_match_table = of_match_ptr(cm32181_of_match),
+		.acpi_match_table = ACPI_PTR(cm32181_acpi_match),
 	},
 	.id_table       = cm32181_id,
 	.probe		= cm32181_probe,
+	.remove		= cm32181_remove,
 };
 
 module_i2c_driver(cm32181_driver);

Quoted 15 lines · On Fri, 27 Oct 2017 18:27:02 +0200 Marc CAPDEVILLE <[email protected]>…

On asus T100, Capella cm3218 chip is implemented as ambiant light sensor. This chip expose an smbus ARA protocol device on standard address 0x0c. The chip is not functional before all alerts are acknowledged.
On asus T100, this device is enumerated on ACPI bus and the description give tow I2C connection. The first is the connection to the ARA device and the second gives the real address of the device.

So, on device probe,If the i2c address is the ARA address and the device is enumerated via acpi, we lookup for the real address in the ACPI resource list and change it in the client structure. if an interrupt resource is given, and only for cm3218 chip, we declare an smbus_alert device.

Quoted 1 line

Signed-off-by Marc CAPDEVILLE <[email protected]>

Wolfram - this needs input from you on how to neatly handle an ACPI registered ARA.

Thanks,

Jonathan

Quoted 269 lines

---
v4 :
   - rework acpi i2c adress lookup due to bad commit being sent.

v3 :
   - rework acpi i2c adress lookup
   - comment style cleanup
   - add prefix cm32181_to constantes and macros

v2 :
   - cm3218 support always build
   - Cleanup of unneeded #if statement
   - Beter identifying chip in platform device, acpi and of_device

 drivers/iio/light/cm32181.c | 202 ++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 185 insertions(+), 17 deletions(-)

drivers/iio/light/cm32181.c +4 -0

index d6fd0dace74f..f1a7495e851b 100644
@@ -18,8 +18,12 @@
  #include <linux/iio/sysfs.h>
  #include <linux/iio/events.h>
  #include <linux/init.h>
+#include <linux/i2c-smbus.h>
+#include <linux/acpi.h>
+#include <linux/of_device.h>
+#include <linux/resource_ext.h>

drivers/iio/light/cm32181.c +1 -1

-/* Registers Address */
+/* Registers Addresses */

 #define CM32181_REG_ADDR_CMD 0x00
 #define CM32181_REG_ADDR_ALS 0x04
 #define CM32181_REG_ADDR_STATUS 0x06

drivers/iio/light/cm32181.c +0 -0

@@ -45,18 +49,25 @@
  #define CM32181_MLUX_PER_BIT_BASE_IT	800000	/* Based

on IT=800ms */ #define CM32181_CALIBSCALE_DEFAULT 100
0
 #define CM32181_CALIBSCALE_RESOLUTION 1000

drivers/iio/light/cm32181.c +6 -1

-#define MLUX_PER_LUX			1000
+#define CM32181_MLUX_PER_LUX			1000
+
+#define CM32181_ID			0x81
+#define CM3218_ID			0x18
+
+#define CM3218_ARA_ADDR			0x0c

 static const u8 cm32181_reg[CM32181_CONF_REG_NUM] = { CM32181_REG_ADDR_CMD,
 };

drivers/iio/light/cm32181.c +0 -2

-static const int als_it_bits[] = {12, 8, 0, 1, 2, 3};
-static const int als_it_value[] = {25000, 50000, 100000, 200000,

400000, +static const int cm32181_als_it_bits[] = {12, 8, 0, 1, 2, 3};
+static const int cm32181_als_it_value[] = {25000, 50000, 100000, 200000, 400000, 800000};

 struct cm32181_chip {
  struct i2c_client *client;

drivers/iio/light/cm32181.c +2 -0

+	int chip_id;
+	struct i2c_client *ara;

  struct mutex lock;
  u16 conf_regs[CM32181_CONF_REG_NUM];
  int calibscale;

drivers/iio/light/cm32181.c +0 -0

@@ -81,7 +92,7 @@ static int cm32181_reg_init(struct cm32181_chip
 *cm32181) return ret;

  /* check device ID */

drivers/iio/light/cm32181.c +1 -1

-	if ((ret & 0xFF) != 0x81)
+	if ((ret & 0xFF) != cm32181->chip_id)

  return -ENODEV;

  /* Default Values */

drivers/iio/light/cm32181.c +1 -1

@@ -103,7 +114,7 @@ static int cm32181_reg_init(struct cm32181_chip
 *cm32181) /**
   *  cm32181_read_als_it() - Get sensor integration time (ms)
   *  @cm32181:	pointer of struct cm32181
- *  @val2:	pointer of int to load the als_it value.
+ *  @val2:	pointer of int to load the cm32181_als_it value.
   *
   *  Report the current integartion time by millisecond.
   *

@@ -117,9 +128,9 @@ static int cm32181_read_als_it(struct

cm32181_chip *cm32181, int *val2) als_it = cm32181->conf_regs[CM32181_REG_ADDR_CMD]; als_it &= CM32181_CMD_ALS_IT_MASK; als_it >>= CM32181_CMD_ALS_IT_SHIFT;

drivers/iio/light/cm32181.c +3 -3

-	for (i = 0; i < ARRAY_SIZE(als_it_bits); i++) {
-		if (als_it == als_it_bits[i]) {
-			*val2 = als_it_value[i];
+	for (i = 0; i < ARRAY_SIZE(cm32181_als_it_bits); i++) {
+		if (als_it == cm32181_als_it_bits[i]) {
+			*val2 = cm32181_als_it_value[i];

  return IIO_VAL_INT_PLUS_MICRO; }
  }

drivers/iio/light/cm32181.c +0 -0

@@ -142,14 +153,14 @@ static int cm32181_write_als_it(struct
 cm32181_chip *cm32181, int val) u16 als_it;
  	int ret, i, n;

drivers/iio/light/cm32181.c +1 -1

-	n = ARRAY_SIZE(als_it_value);
+	n = ARRAY_SIZE(cm32181_als_it_value);

  for (i = 0; i < n; i++)

drivers/iio/light/cm32181.c +1 -1

-		if (val <= als_it_value[i])
+		if (val <= cm32181_als_it_value[i])

  break;
  if (i >= n)
  i = n - 1;

drivers/iio/light/cm32181.c +1 -1

-	als_it = als_it_bits[i];
+	als_it = cm32181_als_it_bits[i];

  als_it <<= CM32181_CMD_ALS_IT_SHIFT;

  mutex_lock(&cm32181->lock);

drivers/iio/light/cm32181.c +1 -1

@@ -195,7 +206,7 @@ static int cm32181_get_lux(struct cm32181_chip
 *cm32181) lux *= ret;
  	lux *= cm32181->calibscale;
  	lux /= CM32181_CALIBSCALE_RESOLUTION;
-	lux /= MLUX_PER_LUX;
+	lux /= CM32181_MLUX_PER_LUX;

  if (lux > 0xFFFF)
  lux = 0xFFFF;

drivers/iio/light/cm32181.c +0 -0

@@ -263,9 +274,9 @@ static ssize_t cm32181_get_it_available(struct
 device *dev, {
  	int i, n, len;

drivers/iio/light/cm32181.c +1 -1

-	n = ARRAY_SIZE(als_it_value);
+	n = ARRAY_SIZE(cm32181_als_it_value);

  for (i = 0, len = 0; i < n; i++)

drivers/iio/light/cm32181.c +0 -1

-		len += sprintf(buf + len, "0.%06u ",

als_it_value[i]);

drivers/iio/light/cm32181.c +1 -0

+		len += sprintf(buf + len, "0.%06u ",

cm32181_als_it_value[i]); return len + sprintf(buf + len, "\n"); }

drivers/iio/light/cm32181.c +0 -0

@@ -298,12 +309,89 @@ static const struct iio_info cm32181_info = {
  	.attrs			= &cm32181_attribute_group,
  };

drivers/iio/light/cm32181.c +3 -0

+#if defined(CONFIG_ACPI)
+/* Filter acpi resources for a real i2c address on same adapter */
+static int cm3218_filter_i2c_address(struct acpi_resource *ares,

void *data) +{

drivers/iio/light/cm32181.c +50 -0

+	struct i2c_client *client = data;
+	struct acpi_resource_i2c_serialbus *sb;
+	acpi_status status;
+	acpi_handle adapter_handle;
+
+	if (ares->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
+		return 1;
+
+	sb = &ares->data.i2c_serial_bus;
+	if (sb->type != ACPI_RESOURCE_SERIAL_TYPE_I2C)
+		return 1;
+
+	status = acpi_get_handle(ACPI_HANDLE(&client->dev),
+				 sb->resource_source.string_ptr,
+				 &adapter_handle);
+	if (ACPI_FAILURE(status))
+		return status;
+
+	if (adapter_handle != ACPI_HANDLE(&client->adapter->dev))
+		return 1;
+
+	if (sb->slave_address == CM3218_ARA_ADDR)
+		return 1;
+
+	return AE_OK;
+}
+
+/* Get the real i2c address from acpi resources */
+static int cm3218_acpi_get_address(struct i2c_client *client)
+{
+	int ret;
+	struct acpi_device *adev;
+	LIST_HEAD(res_list);
+	struct resource_entry *res_entry;
+	struct acpi_resource *ares;
+	struct acpi_resource_i2c_serialbus *sb;
+
+	adev = ACPI_COMPANION(&client->dev);
+	if (!adev)
+		return -ENODEV;
+
+	ret = acpi_dev_get_resources(adev,
+				     &res_list,
+				     cm3218_filter_i2c_address,
+				     client);
+	if (ret < 0)
+		return ret;
+
+	/* get first resource */
+	res_entry = list_entry(&res_list, struct resource_entry,

node);

drivers/iio/light/cm32181.c +15 -0

+	ares = (struct acpi_resource *)res_entry->res;
+	sb = &ares->data.i2c_serial_bus;
+
+	/* set i2c address */
+	client->addr = sb->slave_address;
+	client->flags &= ~I2C_CLIENT_TEN;
+	if (sb->access_mode == ACPI_I2C_10BIT_MODE)
+		client->flags |= I2C_CLIENT_TEN;
+
+	acpi_dev_free_resource_list(&res_list);
+
+	return 0;
+}
+#else
+static inline int cm3218_acpi_get_address(struct i2c_client

*client)

drivers/iio/light/cm32181.c +5 -0

+{
+	return -ENODEV;
+}
+#endif
+

 static int cm32181_probe(struct i2c_client *client, const struct i2c_device_id *id) {
  struct cm32181_chip *cm32181;
  struct iio_dev *indio_dev;
  int ret;

drivers/iio/light/cm32181.c +3 -0

+	struct i2c_smbus_alert_setup ara_setup;
+	const struct of_device_id *of_id;
+	const struct acpi_device_id *acpi_id;

  indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*cm32181)); if (!indio_dev) {

drivers/iio/light/cm32181.c +0 -0

@@ -323,11 +411,65 @@ static int cm32181_probe(struct i2c_client
 *client, indio_dev->name = id->name;
  	indio_dev->modes = INDIO_DIRECT_MODE;

drivers/iio/light/cm32181.c +1 -0

+	/* Lookup for chip ID from platform, acpi or of device

table
*/

drivers/iio/light/cm32181.c +4 -0

+	if (id) {
+		cm32181->chip_id = id->driver_data;
+	} else if (ACPI_COMPANION(&client->dev)) {
+		acpi_id =

acpi_match_device(client->dev.driver->acpi_match_table,

drivers/iio/light/cm32181.c +5 -0

+					    &client->dev);
+		if (!acpi_id)
+			return -ENODEV;
+
+		cm32181->chip_id =

(kernel_ulong_t)acpi_id->driver_data;

drivers/iio/light/cm32181.c +2 -0

+	} else if (client->dev.of_node) {
+		of_id =

of_match_device(clients->dev.driver->of_match_table,

drivers/iio/light/cm32181.c +14 -0

+					&client->dev);
+		if (!of_id)
+			return -ENODEV;
+
+		cm32181->chip_id = (kernel_ulong_t)of_id->data;
+	} else {
+		return -ENODEV;
+	}
+
+	if (cm32181->chip_id == CM3218_ID) {
+		if (client->addr == CM3218_ARA_ADDR) {
+			/*
+			 * In case first address is the ARA device
+			 * lookup for a second one in acpi

resources
if

drivers/iio/light/cm32181.c static int cm32181_probe(struct i2c_client *client, · line 444

Quoted 5 lines

drivers/iio/light/cm32181.c +5 -0

+			 * this client is enumerated on acpi
+			 */
+			ret = cm3218_acpi_get_address(client);
+			if (ret < 0)
+				return -ENODEV;

This bares so little resemblance to existing uses of the ARA infrastructure that I'm not going to take this until Wolfram has time to look at it.

In all other instances the setup_smbus_alert stuff has been done from a bus driver, not a client driver.

Unfortunately ACPI support is thin on the ground so it may be this is the only way we can do it.

There was a relevant patch from Srinivas that google found for me: https://patchwork.ozlabs.org/patch/386410/

This ensures that we probe the device on the valid address not the ARA.

Looks like Srinivas' patch never went anywhere.  However I think it was correct in it's assumption that this sort of support should be in the core.

Quoted 95 lines

drivers/iio/light/cm32181.c +8 -0

+		}
+
+#ifdef CONFIG_I2C_SMBUS
+		if (client->irq > 0) {
+			/* setup smb alert device */
+			ara_setup.irq = client->irq;
+			ara_setup.alert_edge_triggered = 0;
+			cm32181->ara =

i2c_setup_smbus_alert(client->adapter,
+
&ara_setup);

drivers/iio/light/cm32181.c +10 -0

+			if (!cm32181->ara)
+				return -ENODEV;
+		} else {
+			return -ENODEV;
+		}
+#else
+		return -ENODEV;
+#endif
+	}
+

  ret = cm32181_reg_init(cm32181);
  if (ret) {
  dev_err(&client->dev,
  "%s: register init failed\n", __func__);

drivers/iio/light/cm32181.c +4 -0

+
+		if (cm32181->ara)
+			i2c_unregister_device(cm32181->ara);
+

  return ret;
  }

drivers/iio/light/cm32181.c +4 -0

@@ -336,32 +478,58 @@ static int cm32181_probe(struct i2c_client
 *client, dev_err(&client->dev,
  			"%s: regist device failed\n",
  			__func__);
+
+		if (cm32181->ara)
+			i2c_unregister_device(cm32181->ara);
+
  		return ret;
  	}

  return 0;
 }

drivers/iio/light/cm32181.c +11 -0

+static int cm32181_remove(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+	struct cm32181_chip *cm32181 = iio_priv(indio_dev);
+
+	if (cm32181->ara)
+		i2c_unregister_device(cm32181->ara);
+
+	return 0;
+};
+

 static const struct i2c_device_id cm32181_id[] = {

drivers/iio/light/cm32181.c +2 -1

-	{ "cm32181", 0 },
+	{ "cm32181", CM32181_ID },
+	{ "cm3218", CM3218_ID },

  { }
 };

 MODULE_DEVICE_TABLE(i2c, cm32181_id);

 static const struct of_device_id cm32181_of_match[] = {

drivers/iio/light/cm32181.c +2 -1

-	{ .compatible = "capella,cm32181" },
+	{ .compatible = "capella,cm32181", (void *)CM32181_ID },
+	{ .compatible = "capella,cm3218",  (void *)CM3218_ID },

  { }
 };
 MODULE_DEVICE_TABLE(of, cm32181_of_match);

drivers/iio/light/cm32181.c +7 -0

+static const struct acpi_device_id cm32181_acpi_match[] = {
+	{ "CPLM3218", CM3218_ID },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(acpi, cm32181_acpi_match);
+

 static struct i2c_driver cm32181_driver = {
  .driver = {
  .name = "cm32181",
  .of_match_table = of_match_ptr(cm32181_of_match),

drivers/iio/light/cm32181.c +1 -0

+		.acpi_match_table = ACPI_PTR(cm32181_acpi_match),

  },
  .id_table       = cm32181_id,
  .probe = cm32181_probe,

drivers/iio/light/cm32181.c +1 -0

+	.remove		= cm32181_remove,

 };

 module_i2c_driver(cm32181_driver);

Quoted 18 lines · Jonathan Cameron

Quoted 15 lines · On Fri, 27 Oct 2017 18:27:02 +0200 Marc CAPDEVILLE <[email protected]>…

On asus T100, Capella cm3218 chip is implemented as ambiant light sensor. This chip expose an smbus ARA protocol device on standard address 0x0c. The chip is not functional before all alerts are acknowledged.
On asus T100, this device is enumerated on ACPI bus and the description give tow I2C connection. The first is the connection to the ARA device and the second gives the real address of the device.

So, on device probe,If the i2c address is the ARA address and the device is enumerated via acpi, we lookup for the real address in the ACPI resource list and change it in the client structure. if an interrupt resource is given, and only for cm3218 chip, we declare an smbus_alert device.

Quoted 3 lines

Quoted 1 line

Signed-off-by Marc CAPDEVILLE <[email protected]>

Wolfram - this needs input from you on how to neatly handle an ACPI registered ARA.

There was another RFC from Alan cox
https://patchwork.ozlabs.org/patch/381773/

Quoted 14 lines · On Fri, 27 Oct 2017 18:27:02 +0200 Marc CAPDEVILLE <[email protected]>…

On asus T100, Capella cm3218 chip is implemented as ambiant light sensor. This chip expose an smbus ARA protocol device on standard address 0x0c. The chip is not functional before all alerts are acknowledged.
On asus T100, this device is enumerated on ACPI bus and the description give tow I2C connection. The first is the connection to the ARA device and the second gives the real address of the device.

So, on device probe,If the i2c address is the ARA address and the device is enumerated via acpi, we lookup for the real address in the ACPI resource list and change it in the client structure. if an interrupt resource is given, and only for cm3218 chip, we declare an smbus_alert device.

Quoted 1 line

Signed-off-by Marc CAPDEVILLE <[email protected]>

Wolfram - this needs input from you on how to neatly handle an ACPI registered ARA.

Quoted 17 lines · Jonathan Cameron

Quoted 14 lines · On Fri, 27 Oct 2017 18:27:02 +0200 Marc CAPDEVILLE <[email protected]>…

On asus T100, Capella cm3218 chip is implemented as ambiant light sensor. This chip expose an smbus ARA protocol device on standard address 0x0c. The chip is not functional before all alerts are acknowledged.
On asus T100, this device is enumerated on ACPI bus and the description give tow I2C connection. The first is the connection to the ARA device and the second gives the real address of the device.

So, on device probe,If the i2c address is the ARA address and the device is enumerated via acpi, we lookup for the real address in the ACPI resource list and change it in the client structure. if an interrupt resource is given, and only for cm3218 chip, we declare an smbus_alert device.

Quoted 4 lines

Quoted 1 line

Signed-off-by Marc CAPDEVILLE <[email protected]>

Wolfram - this needs input from you on how to neatly handle an ACPI registered ARA.

ACPI is really not my field. Try asking the I2C ACPI maintainers or Benjamin Tissoires <[email protected]> who did work on SMBus interrupts recently.

Quoted 18 lines · On Thu, 2 Nov 2017 16:04:07 +0100 Wolfram Sang <[email protected]> wrote:

Quoted 17 lines · Jonathan Cameron

Quoted 14 lines · On Fri, 27 Oct 2017 18:27:02 +0200 Marc CAPDEVILLE <[email protected]>…

On asus T100, Capella cm3218 chip is implemented as ambiant light sensor. This chip expose an smbus ARA protocol device on standard address 0x0c. The chip is not functional before all alerts are acknowledged.
On asus T100, this device is enumerated on ACPI bus and the description give tow I2C connection. The first is the connection to the ARA device and the second gives the real address of the device.

So, on device probe,If the i2c address is the ARA address and the device is enumerated via acpi, we lookup for the real address in the ACPI resource list and change it in the client structure. if an interrupt resource is given, and only for cm3218 chip, we declare an smbus_alert device.

Quoted 9 lines

Quoted 4 lines

Quoted 1 line

Signed-off-by Marc CAPDEVILLE <[email protected]>

Wolfram - this needs input from you on how to neatly handle an ACPI registered ARA.

ACPI is really not my field. Try asking the I2C ACPI maintainers or Benjamin Tissoires <[email protected]> who did work on SMBus interrupts recently.

Hi Mika, Benjamin,

So we've lost most of the context in this thread, but the basic question is how to handle smbus ARA support with ACPI.

https://patchwork.kernel.org/patch/10030309/

Has the proposal made in this driver which is really not terribly nice (as it registers the ARA device by messing with the address and registering a second device).

As I understood it the ARA device registration should be handled by the i2c master, but there are very few examples.

Phil pointed out that equivalent OF support recently got taken from him..
https://www.spinics.net/lists/devicetree/msg191947.html
https://www.spinics.net/lists/linux-i2c/msg31173.html

Any thoughts on the right way to do this?

+Jarkko

Quoted 41 lines · Jonathan Cameron

Quoted 18 lines · On Thu, 2 Nov 2017 16:04:07 +0100 Wolfram Sang <[email protected]> wrote:

Quoted 17 lines · Jonathan Cameron

Quoted 14 lines · On Fri, 27 Oct 2017 18:27:02 +0200 Marc CAPDEVILLE <[email protected]>…

On asus T100, Capella cm3218 chip is implemented as ambiant light sensor. This chip expose an smbus ARA protocol device on standard address 0x0c. The chip is not functional before all alerts are acknowledged.
On asus T100, this device is enumerated on ACPI bus and the description give tow I2C connection. The first is the connection to the ARA device and the second gives the real address of the device.

So, on device probe,If the i2c address is the ARA address and the device is enumerated via acpi, we lookup for the real address in the ACPI resource list and change it in the client structure. if an interrupt resource is given, and only for cm3218 chip, we declare an smbus_alert device.

Quoted 9 lines

Quoted 4 lines

Quoted 1 line

Signed-off-by Marc CAPDEVILLE <[email protected]>

Wolfram - this needs input from you on how to neatly handle an ACPI registered ARA.

ACPI is really not my field. Try asking the I2C ACPI maintainers or Benjamin Tissoires <[email protected]> who did work on SMBus interrupts recently.

Hi Mika, Benjamin,

So we've lost most of the context in this thread, but the basic question is how to handle smbus ARA support with ACPI.

https://patchwork.kernel.org/patch/10030309/

Has the proposal made in this driver which is really not terribly nice (as it registers the ARA device by messing with the address and registering a second device).

Quoted 8 lines

As I understood it the ARA device registration should be handled by the i2c master, but there are very few examples.

Phil pointed out that equivalent OF support recently got taken from him..
https://www.spinics.net/lists/devicetree/msg191947.html
https://www.spinics.net/lists/linux-i2c/msg31173.html

Any thoughts on the right way to do this?

There does not seem to be any way in ACPI to tell which "connection" is used to describe ARA so that part currently is something each driver needs to handle as they know the device the best. I don't think we have any means to handle it in generic way in I2C core except to provide some helpers that work on top of i2c_setup_smbus_alert() but understand ACPI resources. Say provide function like this:

   int acpi_i2c_setup_smbus_alert(struct i2c_adapter *adapter, int index);

Which then extracts automatically I2cSerialBus connection from "index" and calls i2c_setup_smbus_alert() accordingly.

In the long run we could introduce _DSD property that can be used to name the connection in the same way DT does;

     Name (_CRS, ResourceTemplate () {
         I2cSerialBus () { ... } // ARA
         I2cSerialBus () { ... } // normal device address
     })

     Name (_DSD, Package () {
         ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
         Package () {
             Package () {"smbus_alert", 0} // Where 0 means the first I2cSerialBus
             ...
         }
     })

Quoted 68 lines · Mika Westerberg

+Jarkko

Quoted 41 lines · Jonathan Cameron

Quoted 18 lines · On Thu, 2 Nov 2017 16:04:07 +0100 Wolfram Sang <[email protected]> wrote:

Quoted 17 lines · Jonathan Cameron

Quoted 14 lines · On Fri, 27 Oct 2017 18:27:02 +0200 Marc CAPDEVILLE <[email protected]>…

On asus T100, Capella cm3218 chip is implemented as ambiant light sensor. This chip expose an smbus ARA protocol device on standard address 0x0c. The chip is not functional before all alerts are acknowledged.
On asus T100, this device is enumerated on ACPI bus and the description give tow I2C connection. The first is the connection to the ARA device and the second gives the real address of the device.

So, on device probe,If the i2c address is the ARA address and the device is enumerated via acpi, we lookup for the real address in the ACPI resource list and change it in the client structure. if an interrupt resource is given, and only for cm3218 chip, we declare an smbus_alert device.

Quoted 9 lines

Quoted 4 lines

Quoted 1 line

Signed-off-by Marc CAPDEVILLE <[email protected]>

Wolfram - this needs input from you on how to neatly handle an ACPI registered ARA.

ACPI is really not my field. Try asking the I2C ACPI maintainers or Benjamin Tissoires <[email protected]> who did work on SMBus interrupts recently.

Hi Mika, Benjamin,

So we've lost most of the context in this thread, but the basic question is how to handle smbus ARA support with ACPI.

https://patchwork.kernel.org/patch/10030309/

Has the proposal made in this driver which is really not terribly nice (as it registers the ARA device by messing with the address and registering a second device).

Quoted 8 lines

As I understood it the ARA device registration should be handled by the i2c master, but there are very few examples.

Phil pointed out that equivalent OF support recently got taken from him..
https://www.spinics.net/lists/devicetree/msg191947.html
https://www.spinics.net/lists/linux-i2c/msg31173.html

Any thoughts on the right way to do this?

There does not seem to be any way in ACPI to tell which "connection" is used to describe ARA so that part currently is something each driver needs to handle as they know the device the best. I don't think we have any means to handle it in generic way in I2C core except to provide some helpers that work on top of i2c_setup_smbus_alert() but understand ACPI resources. Say provide function like this:

   int acpi_i2c_setup_smbus_alert(struct i2c_adapter *adapter, int index);

Which then extracts automatically I2cSerialBus connection from "index" and calls i2c_setup_smbus_alert() accordingly.

In the long run we could introduce _DSD property that can be used to name the connection in the same way DT does;

Quoted 13 lines

     Name (_CRS, ResourceTemplate () {
         I2cSerialBus () { ... } // ARA
         I2cSerialBus () { ... } // normal device address
     })

     Name (_DSD, Package () {
         ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
         Package () {
             Package () {"smbus_alert", 0} // Where 0 means the first I2cSerialBus
             ...
         }
     })

I wonder if it's worth involving the smbus_alert driver in this case. The cm3218 driver doesn't appear to be using the alert callback in strcut i2c_driver.

So the smbus_alert driver is not going to notifiy the cm3218 driver. Are there more than one alert/ara capable devices on the bus?

Perhaps a workaround in this case is if that acpi entry is defined the cm3218 driver handles that ara request directly to clear the interrupt.

Quoted 83 lines · On Tue, 21 Nov 2017 09:22:16 +0800 Phil Reid <[email protected]> wrote:

Quoted 68 lines · Mika Westerberg

+Jarkko

Quoted 41 lines · Jonathan Cameron

Quoted 18 lines · On Thu, 2 Nov 2017 16:04:07 +0100 Wolfram Sang <[email protected]> wrote:

Quoted 17 lines · Jonathan Cameron

Quoted 14 lines · On Fri, 27 Oct 2017 18:27:02 +0200 Marc CAPDEVILLE <[email protected]>…

On asus T100, Capella cm3218 chip is implemented as ambiant light sensor. This chip expose an smbus ARA protocol device on standard address 0x0c. The chip is not functional before all alerts are acknowledged.
On asus T100, this device is enumerated on ACPI bus and the description give tow I2C connection. The first is the connection to the ARA device and the second gives the real address of the device.

So, on device probe,If the i2c address is the ARA address and the device is enumerated via acpi, we lookup for the real address in the ACPI resource list and change it in the client structure. if an interrupt resource is given, and only for cm3218 chip, we declare an smbus_alert device.

Quoted 9 lines

Quoted 4 lines

Quoted 1 line

Signed-off-by Marc CAPDEVILLE <[email protected]>

Wolfram - this needs input from you on how to neatly handle an ACPI registered ARA.

ACPI is really not my field. Try asking the I2C ACPI maintainers or Benjamin Tissoires <[email protected]> who did work on SMBus interrupts recently.

Hi Mika, Benjamin,

So we've lost most of the context in this thread, but the basic question is how to handle smbus ARA support with ACPI.

https://patchwork.kernel.org/patch/10030309/

Has the proposal made in this driver which is really not terribly nice (as it registers the ARA device by messing with the address and registering a second device).

Quoted 8 lines

As I understood it the ARA device registration should be handled by the i2c master, but there are very few examples.

Phil pointed out that equivalent OF support recently got taken from him..
https://www.spinics.net/lists/devicetree/msg191947.html
https://www.spinics.net/lists/linux-i2c/msg31173.html

Any thoughts on the right way to do this?

There does not seem to be any way in ACPI to tell which "connection" is used to describe ARA so that part currently is something each driver needs to handle as they know the device the best. I don't think we have any means to handle it in generic way in I2C core except to provide some helpers that work on top of i2c_setup_smbus_alert() but understand ACPI resources. Say provide function like this:

    int acpi_i2c_setup_smbus_alert(struct i2c_adapter *adapter, int index);

Which then extracts automatically I2cSerialBus connection from "index" and calls i2c_setup_smbus_alert() accordingly.

In the long run we could introduce _DSD property that can be used to name the connection in the same way DT does;

Quoted 13 lines

      Name (_CRS, ResourceTemplate () {
          I2cSerialBus () { ... } // ARA
          I2cSerialBus () { ... } // normal device address
      })

      Name (_DSD, Package () {
          ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
          Package () {
              Package () {"smbus_alert", 0} // Where 0 means the first I2cSerialBus
              ...
          }
      })

Quoted 2 lines

I wonder if it's worth involving the smbus_alert driver in this case. The cm3218 driver doesn't appear to be using the alert callback in strcut i2c_driver.

True - though it really should be..

Quoted 2 lines

So the smbus_alert driver is not going to notifiy the cm3218 driver. Are there more than one alert/ara capable devices on the bus?

I'm not keen on taking a work around for one board on the basis that it only has this one device on the bus - we will probably get a different one down the line where this isn't true - then we end up ripping up what has been done so far and starting again.

I don't mind having some ACPI matching code in the driver but it needs to use the ARA infrastructure to actually handle things...

If we then get nice generic ACPI code via some other means in the future we can move the driver over to that.

Sometimes I wonder if some people just write ACPI tables with no though to generalization at all, or that the code running might be shared across different machines. (sometimes that assumption is valid, but not that often... oh well - usual case of if we all work together everyone wins but not worth one company trying to do things right...)

Quoted 3 lines

Perhaps a workaround in this case is if that acpi entry is defined the cm3218 driver handles that ara request directly to clear the interrupt.

Jonathan

+Jarkko

Quoted 41 lines · Jonathan Cameron

Quoted 18 lines · On Thu, 2 Nov 2017 16:04:07 +0100 Wolfram Sang <[email protected]> wrote:

Quoted 17 lines · Jonathan Cameron

Quoted 14 lines · On Fri, 27 Oct 2017 18:27:02 +0200 Marc CAPDEVILLE <[email protected]>…

On asus T100, Capella cm3218 chip is implemented as ambiant light sensor. This chip expose an smbus ARA protocol device on standard address 0x0c. The chip is not functional before all alerts are acknowledged.
On asus T100, this device is enumerated on ACPI bus and the description give tow I2C connection. The first is the connection to the ARA device and the second gives the real address of the device.

So, on device probe,If the i2c address is the ARA address and the device is enumerated via acpi, we lookup for the real address in the ACPI resource list and change it in the client structure. if an interrupt resource is given, and only for cm3218 chip, we declare an smbus_alert device.

Quoted 9 lines

Quoted 4 lines

Quoted 1 line

Signed-off-by Marc CAPDEVILLE <[email protected]>

Wolfram - this needs input from you on how to neatly handle an ACPI registered ARA.

ACPI is really not my field. Try asking the I2C ACPI maintainers or Benjamin Tissoires <[email protected]> who did work on SMBus interrupts recently.

Hi Mika, Benjamin,

So we've lost most of the context in this thread, but the basic question is how to handle smbus ARA support with ACPI.

https://patchwork.kernel.org/patch/10030309/

Has the proposal made in this driver which is really not terribly nice (as it registers the ARA device by messing with the address and registering a second device).

Quoted 8 lines

As I understood it the ARA device registration should be handled by the i2c master, but there are very few examples.

Phil pointed out that equivalent OF support recently got taken from him..
https://www.spinics.net/lists/devicetree/msg191947.html
https://www.spinics.net/lists/linux-i2c/msg31173.html

Any thoughts on the right way to do this?

There does not seem to be any way in ACPI to tell which "connection" is used to describe ARA so that part currently is something each driver needs to handle as they know the device the best. I don't think we have any means to handle it in generic way in I2C core except to provide some helpers that work on top of i2c_setup_smbus_alert() but understand ACPI resources. Say provide function like this:

  int acpi_i2c_setup_smbus_alert(struct i2c_adapter *adapter, int index);

Which then extracts automatically I2cSerialBus connection from "index" and calls i2c_setup_smbus_alert() accordingly.

In the long run we could introduce _DSD property that can be used to name the connection in the same way DT does;

    Name (_CRS, ResourceTemplate () {
        I2cSerialBus () { ... } // ARA
        I2cSerialBus () { ... } // normal device address
    })

    Name (_DSD, Package () {
        ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
        Package () {
            Package () {"smbus_alert", 0} // Where 0 means the first I2cSerialBus
            ...
        }
    })

But it does not help the existing systems.

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 28 lines

There does not seem to be any way in ACPI to tell which "connection" is used to describe ARA so that part currently is something each driver needs to handle as they know the device the best. I don't think we have any means to handle it in generic way in I2C core except to provide some helpers that work on top of i2c_setup_smbus_alert() but understand ACPI resources. Say provide function like this:

  int acpi_i2c_setup_smbus_alert(struct i2c_adapter *adapter, int index);

Which then extracts automatically I2cSerialBus connection from "index" and calls i2c_setup_smbus_alert() accordingly.

In the long run we could introduce _DSD property that can be used to name the connection in the same way DT does;

    Name (_CRS, ResourceTemplate () {
        I2cSerialBus () { ... } // ARA
        I2cSerialBus () { ... } // normal device address
    })

    Name (_DSD, Package () {
        ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
        Package () {
            Package () {"smbus_alert", 0} // Where 0 means the first I2cSerialBus
            ...
        }
    })

Quoted 1 line

But it does not help the existing systems.

I'm curious - how would we go about promoting this piece of common sense?