[PATCH v6 1/4] i2c-core-acpi : Add i2c_acpi_set_connection

[PATCH v6 1/4] i2c-core-acpi : Add i2c_acpi_set_connection

[Marc CAPDEVILLE]

Add a methode to allow clients to change their connection address on
same adapter.

On ACPI enumerated device, when a device support smbus alert protocol,
there are two acpi serial bus connection description. The order in which
connection is given is not well defined and devices may be enumerated
with the wrong address (the first one). So let the driver detect the
unsupported address and request i2c acpi core to choose the second one
at probing time.

Signed-off-by: Marc CAPDEVILLE <m.capdeville@no-log.org>
---
 drivers/i2c/i2c-core-acpi.c | 50 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/i2c.h         | 10 +++++++++
 2 files changed, 60 insertions(+)

diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index a9126b3cda61..47bc0da12055 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -421,6 +421,56 @@ struct i2c_client *i2c_acpi_new_device(struct device *dev, int index,
 }
 EXPORT_SYMBOL_GPL(i2c_acpi_new_device);
 
+int i2c_acpi_set_connection(struct i2c_client *client, int index)
+{
+	struct i2c_acpi_lookup lookup;
+	struct i2c_adapter *adapter;
+	struct acpi_device *adev;
+	struct i2c_board_info info;
+	LIST_HEAD(resource_list);
+	int ret;
+
+	if (!client)
+		return -ENODEV;
+
+	adev = ACPI_COMPANION(&client->dev);
+	if (!adev)
+		return -ENODEV;
+
+	memset(&info,  0, sizeof(info));
+	memset(&lookup, 0, sizeof(lookup));
+	lookup.info = &info;
+	lookup.device_handle = acpi_device_handle(adev);
+	lookup.index = index;
+
+	ret = acpi_dev_get_resources(adev, &resource_list,
+				     i2c_acpi_fill_info, &lookup);
+	acpi_dev_free_resource_list(&resource_list);
+
+	adapter = i2c_acpi_find_adapter_by_handle(lookup.adapter_handle);
+
+	if (ret < 0 || !info.addr)
+		return -EINVAL;
+
+	/* Only accept connection on same adapter */
+	if (adapter != client->adapter)
+		return -EINVAL;
+
+	ret = i2c_check_addr_validity(info.addr, info.flags);
+	if (ret) {
+		dev_err(&client->dev, "Invalid %d-bit I2C address 0x%02hx\n",
+			info.flags & I2C_CLIENT_TEN ? 10 : 7, info.addr);
+		return -EINVAL;
+	}
+
+	/* Set new address and flags */
+	client->addr = info.addr;
+	client->flags = info.flags;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(i2c_acpi_set_connection);
+
 #ifdef CONFIG_ACPI_I2C_OPREGION
 static int acpi_gsb_i2c_read_bytes(struct i2c_client *client,
 		u8 cmd, u8 *data, u8 data_len)
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 0f774406fad0..618b453901da 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -595,6 +595,8 @@ struct i2c_adapter {
 	const struct i2c_adapter_quirks *quirks;
 
 	struct irq_domain *host_notify_domain;
+
+	struct i2c_client *smbus_ara;	/* ARA for SMBUS if present */
 };
 #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev)
 
@@ -839,16 +841,24 @@ static inline const struct of_device_id
 u32 i2c_acpi_find_bus_speed(struct device *dev);
 struct i2c_client *i2c_acpi_new_device(struct device *dev, int index,
 				       struct i2c_board_info *info);
+
+int i2c_acpi_set_connection(struct i2c_client *client, int index);
 #else
 static inline u32 i2c_acpi_find_bus_speed(struct device *dev)
 {
 	return 0;
 }
+
 static inline struct i2c_client *i2c_acpi_new_device(struct device *dev,
 					int index, struct i2c_board_info *info)
 {
 	return NULL;
 }
+
+static inline int i2c_acpi_set_connection(struct i2c_client *client, int index)
+{
+	return -EINVAL;
+}
 #endif /* CONFIG_ACPI */
 
 #endif /* _LINUX_I2C_H */
-- 
2.11.0

[PATCH v6 2/4] i2c-smbus : Add client discovered ARA support

[Marc CAPDEVILLE]

This is from rfc by Alan Cox : https://patchwork.ozlabs.org/patch/381773

The idea is as follows (extract from above rfc) :
- If an adapter knows about its ARA and smbus alerts then the adapter
  creates its own interrupt handler as before

- If a client knows it needs smbus alerts it calls
  i2c_require_smbus_alert(). This ensures that there is an ARA handler
  registered and tells the client whether the adapter is handling it
  anyway or not.

- When the client learns that an ARA event has occurred it calls
  i2c_smbus_alert_event() which uses the existing ARA mechanism to kick
  things off.

Suggested-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Marc CAPDEVILLE <m.capdeville@no-log.org>
---
 drivers/i2c/i2c-smbus.c   | 83 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/i2c-smbus.h | 15 +++++++++
 2 files changed, 98 insertions(+)

diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
index 5a1dd7f13bac..e97fbafd10ef 100644
--- a/drivers/i2c/i2c-smbus.c
+++ b/drivers/i2c/i2c-smbus.c
@@ -161,6 +161,9 @@ static int smbalert_probe(struct i2c_client *ara,
 	}
 
 	i2c_set_clientdata(ara, alert);
+
+	ara->adapter->smbus_ara = ara;
+
 	dev_info(&adapter->dev, "supports SMBALERT#\n");
 
 	return 0;
@@ -172,6 +175,9 @@ static int smbalert_remove(struct i2c_client *ara)
 	struct i2c_smbus_alert *alert = i2c_get_clientdata(ara);
 
 	cancel_work_sync(&alert->alert);
+
+	ara->adapter->smbus_ara = NULL;
+
 	return 0;
 }
 
@@ -210,6 +216,83 @@ int i2c_handle_smbus_alert(struct i2c_client *ara)
 }
 EXPORT_SYMBOL_GPL(i2c_handle_smbus_alert);
 
+/*
+ * i2c_require_smbus_alert - Client discovered SMBus alert
+ * @c: client requiring ARA
+ *
+ * When a client needs an ARA it calls this method. If the bus adapter
+ * supports ARA and already knows how to do so then it will already have
+ * configured for ARA and this is a no-op. If not then we set up an ARA
+ * on the adapter.
+ *
+ * We *cannot* simply register a new IRQ handler for this because we might
+ * have multiple GPIO interrupts to devices all of which trigger an ARA.
+ *
+ * Return:
+ *	0 if ara support already registered
+ *	1 if call register a new smbus_alert device
+ *	<0 on error
+ */
+int i2c_require_smbus_alert(struct i2c_client *client)
+{
+	struct i2c_adapter *adapter = client->adapter;
+	struct i2c_smbus_alert_setup setup;
+	struct i2c_client *ara;
+
+	/* ARA is already known and handled by the adapter (ideal case)
+	 * or another client has specified ARA is needed
+	 */
+	if (adapter->smbus_ara)
+		return 0;
+
+	/* Client driven, do not set up a new IRQ handler */
+	setup.irq = 0;
+
+	ara = i2c_setup_smbus_alert(adapter, &setup);
+	if (!ara)
+		return -ENODEV;
+
+	return 1;
+}
+EXPORT_SYMBOL_GPL(i2c_require_smbus_alert);
+
+/*
+ * i2c_smbus_alert_event
+ * @client: the client who known of a probable ara event
+ * Context: can't sleep
+ *
+ * Helper function to be called from an I2C device driver's interrupt
+ * handler. It will schedule the alert work, in turn calling the
+ * corresponding I2C device driver's alert function.
+ *
+ * It is assumed that client is an i2c client who previously call
+ * i2c_require_smbus_alert().
+ */
+int i2c_smbus_alert_event(struct i2c_client *client)
+{
+	struct i2c_adapter *adapter;
+	struct i2c_client *ara;
+	struct i2c_smbus_alert *alert;
+
+	if (!client)
+		return -EINVAL;
+
+	adapter = client->adapter;
+	if (!adapter)
+		return -EINVAL;
+
+	ara = adapter->smbus_ara;
+	if (!ara)
+		return -EINVAL;
+
+	alert = i2c_get_clientdata(ara);
+	if (!alert)
+		return -EINVAL;
+
+	return schedule_work(&alert->alert);
+}
+EXPORT_SYMBOL_GPL(i2c_smbus_alert_event);
+
 module_i2c_driver(smbalert_driver);
 
 MODULE_AUTHOR("Jean Delvare <jdelvare@suse.de>");
diff --git a/include/linux/i2c-smbus.h b/include/linux/i2c-smbus.h
index fb0e040b1abb..49f362fa6ac5 100644
--- a/include/linux/i2c-smbus.h
+++ b/include/linux/i2c-smbus.h
@@ -58,4 +58,19 @@ static inline int of_i2c_setup_smbus_alert(struct i2c_adapter *adap)
 }
 #endif
 
+#if IS_ENABLED(CONFIG_I2C_SMBUS)
+int i2c_require_smbus_alert(struct i2c_client *client);
+int i2c_smbus_alert_event(struct i2c_client *client);
+#else
+static inline int i2c_require_smbus_alert(struct i2c_client *client)
+{
+	return NULL;
+}
+
+static inline int i2c_smbus_alert_event(struct i2c_client *client)
+{
+	return NULL;
+}
+#endif
+
 #endif /* _LINUX_I2C_SMBUS_H */
-- 
2.11.0

[PATCH v6 3/4] iio : Add cm3218 smbus ara and acpi support

[Marc CAPDEVILLE]

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 two 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 change address of the device for
the one in the second acpi serial bus connection.
This free the ara address so we can register with the smbus_alert
driver.

If an interrupt resource is given, and a smbus_alert device was
found on the adapter, we request a treaded interrupt to
call i2c_smbus_alert_event to handle the alert.

In somme case, the treaded interrupt is not schedule before the driver
try to initialize chip registers. So if first registers access fail, we
call i2c_smbus_alert_event() to acknowledge initial alert, then retry
register access.

Signed-off-by: Marc CAPDEVILLE <m.capdeville@no-log.org>
---
 drivers/iio/light/cm32181.c | 120 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 115 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
index aebf7dd071af..96c08755e6e3 100644
--- a/drivers/iio/light/cm32181.c
+++ b/drivers/iio/light/cm32181.c
@@ -18,6 +18,9 @@
 #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>
 
 /* Registers Address */
 #define CM32181_REG_ADDR_CMD		0x00
@@ -47,6 +50,11 @@
 #define CM32181_CALIBSCALE_RESOLUTION	1000
 #define MLUX_PER_LUX			1000
 
+#define CM32181_ID			0x81
+#define CM3218_ID			0x18
+
+#define SMBUS_ARA_ADDR			0x0c
+
 static const u8 cm32181_reg[CM32181_CONF_REG_NUM] = {
 	CM32181_REG_ADDR_CMD,
 };
@@ -57,6 +65,7 @@ static const int als_it_value[] = {25000, 50000, 100000, 200000, 400000,
 
 struct cm32181_chip {
 	struct i2c_client *client;
+	int chip_id;
 	struct mutex lock;
 	u16 conf_regs[CM32181_CONF_REG_NUM];
 	int calibscale;
@@ -77,11 +86,22 @@ static int cm32181_reg_init(struct cm32181_chip *cm32181)
 	s32 ret;
 
 	ret = i2c_smbus_read_word_data(client, CM32181_REG_ADDR_ID);
-	if (ret < 0)
-		return ret;
+	if (ret < 0) {
+		if (cm32181->chip_id == CM3218_ID) {
+			/*
+			 * On cm3218, smbus alert trigger after probing,
+			 * so poll for first alert here, then retry.
+			 */
+			i2c_smbus_alert_event(client);
+			ret = i2c_smbus_read_word_data(client,
+						       CM32181_REG_ADDR_ID);
+		} else {
+			return ret;
+		}
+	}
 
 	/* check device ID */
-	if ((ret & 0xFF) != 0x81)
+	if ((ret & 0xFF) != cm32181->chip_id)
 		return -ENODEV;
 
 	/* Default Values */
@@ -297,12 +317,36 @@ static const struct iio_info cm32181_info = {
 	.attrs			= &cm32181_attribute_group,
 };
 
+static void cm3218_alert(struct i2c_client *client,
+			 enum i2c_alert_protocol type,
+			 unsigned int data)
+{
+	/*
+	 * nothing to do for now.
+	 * This is just here to acknownledge the cm3218 alert.
+	 */
+}
+
+static irqreturn_t cm32181_irq(int irq, void *d)
+{
+	struct cm32181_chip *cm32181 = d;
+
+	if (cm32181->chip_id == CM3218_ID) {
+		/* This is cm3218 chip irq, so handle the smbus alert */
+		i2c_smbus_alert_event(cm32181->client);
+	}
+
+	return IRQ_HANDLED;
+}
+
 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;
+	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) {
@@ -322,6 +366,61 @@ 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(client->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 (ACPI_COMPANION(&client->dev) &&
+		    client->addr == SMBUS_ARA_ADDR) {
+			/*
+			 * In case this device as been enumerated by acpi
+			 * with the reserved smbus ARA address (first acpi
+			 * connection), request change of address to the second
+			 * connection.
+			 */
+			ret = i2c_acpi_set_connection(client, 1);
+			if (ret)
+				return ret;
+		}
+
+		/* cm3218 chip require an ara device on his adapter */
+		ret = i2c_require_smbus_alert(client);
+		if (ret < 0)
+			return ret;
+
+		/*
+		 * If irq is given, request a threaded irq handler to manage
+		 * smbus alert.
+		 */
+		if (client->irq > 0) {
+			ret = devm_request_threaded_irq(&client->dev,
+							client->irq,
+							NULL, cm32181_irq,
+							IRQF_ONESHOT,
+							"cm32181", cm32181);
+			if (ret)
+				return ret;
+		}
+	}
+
 	ret = cm32181_reg_init(cm32181);
 	if (ret) {
 		dev_err(&client->dev,
@@ -342,25 +441,36 @@ static int cm32181_probe(struct i2c_client *client,
 }
 
 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,
+	.alert		= cm3218_alert,
 };
 
 module_i2c_driver(cm32181_driver);
-- 
2.11.0

[PATCH v6 4/4] iio : cm32181 : cosmetic cleanup

[Marc CAPDEVILLE]

Somme cosmetic cleanup suggested by Peter Meerwald-Stadler.

Macro name :
   MLUX_PER_LUX => CM32181_MLUX_PER_LUX

Constante name :
   als_it_bits => cm32181_als_it_bits
   als_it_value => cm32181_als_it_value

Comment :
   Registers Address => Register Addresses

Suggested-by:  Peter Meerwald-Stadler <pmeerw@pmeerw.net>
Signed-off-by: Marc CAPDEVILLE <m.capdeville@no-log.org>
---
 drivers/iio/light/cm32181.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
index 96c08755e6e3..3e6b244d5cd1 100644
--- a/drivers/iio/light/cm32181.c
+++ b/drivers/iio/light/cm32181.c
@@ -22,7 +22,7 @@
 #include <linux/acpi.h>
 #include <linux/of_device.h>
 
-/* Registers Address */
+/* Register Addresses */
 #define CM32181_REG_ADDR_CMD		0x00
 #define CM32181_REG_ADDR_ALS		0x04
 #define CM32181_REG_ADDR_STATUS		0x06
@@ -48,7 +48,7 @@
 #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
@@ -59,8 +59,8 @@ 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 {
@@ -137,9 +137,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;
 		}
 	}
@@ -162,14 +162,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);
@@ -215,7 +215,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;
@@ -283,9 +283,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");
 }
 
-- 
2.11.0

Re: [PATCH v6 2/4] i2c-smbus : Add client discovered ARA support

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2338 bytes --]

Hi Marc,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on wsa/i2c/for-next]
[also build test WARNING on v4.15-rc5 next-20171222]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Marc-CAPDEVILLE/i2c-core-acpi-Add-i2c_acpi_set_connection/20171226-083729
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
config: ia64-defconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=ia64 

All warnings (new ones prefixed by >>):

   In file included from include/uapi/linux/posix_types.h:5:0,
                    from include/uapi/linux/types.h:14,
                    from include/linux/compiler.h:164,
                    from include/linux/ioport.h:13,
                    from include/linux/acpi.h:25,
                    from drivers//i2c/i2c-core-base.c:24:
   include/linux/i2c-smbus.h: In function 'i2c_require_smbus_alert':
>> include/linux/stddef.h:8:14: warning: return makes integer from pointer without a cast [-Wint-conversion]
    #define NULL ((void *)0)
                 ^
>> include/linux/i2c-smbus.h:67:9: note: in expansion of macro 'NULL'
     return NULL;
            ^~~~
   include/linux/i2c-smbus.h: In function 'i2c_smbus_alert_event':
>> include/linux/stddef.h:8:14: warning: return makes integer from pointer without a cast [-Wint-conversion]
    #define NULL ((void *)0)
                 ^
   include/linux/i2c-smbus.h:72:9: note: in expansion of macro 'NULL'
     return NULL;
            ^~~~

vim +/NULL +67 include/linux/i2c-smbus.h

    60	
    61	#if IS_ENABLED(CONFIG_I2C_SMBUS)
    62	int i2c_require_smbus_alert(struct i2c_client *client);
    63	int i2c_smbus_alert_event(struct i2c_client *client);
    64	#else
    65	static inline int i2c_require_smbus_alert(struct i2c_client *client)
    66	{
  > 67		return NULL;
    68	}
    69	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 18935 bytes --]

Re: [PATCH v6 3/4] iio : Add cm3218 smbus ara and acpi support

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 4861 bytes --]

Hi Marc,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on wsa/i2c/for-next]
[also build test ERROR on v4.15-rc5 next-20171222]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Marc-CAPDEVILLE/i2c-core-acpi-Add-i2c_acpi_set_connection/20171226-083729
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
config: i386-randconfig-c0-12261310 (attached as .config)
compiler: gcc-7 (Debian 7.2.0-12) 7.2.1 20171025
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/iio/light/cm32181.o: In function `cm32181_irq':
>> drivers/iio/light/cm32181.c:336: undefined reference to `i2c_smbus_alert_event'
   drivers/iio/light/cm32181.o: In function `cm32181_probe':
>> drivers/iio/light/cm32181.c:405: undefined reference to `i2c_require_smbus_alert'
   drivers/iio/light/cm32181.o: In function `cm32181_reg_init':
   drivers/iio/light/cm32181.c:95: undefined reference to `i2c_smbus_alert_event'

vim +336 drivers/iio/light/cm32181.c

   329	
   330	static irqreturn_t cm32181_irq(int irq, void *d)
   331	{
   332		struct cm32181_chip *cm32181 = d;
   333	
   334		if (cm32181->chip_id == CM3218_ID) {
   335			/* This is cm3218 chip irq, so handle the smbus alert */
 > 336			i2c_smbus_alert_event(cm32181->client);
   337		}
   338	
   339		return IRQ_HANDLED;
   340	}
   341	
   342	static int cm32181_probe(struct i2c_client *client,
   343				const struct i2c_device_id *id)
   344	{
   345		struct cm32181_chip *cm32181;
   346		struct iio_dev *indio_dev;
   347		int ret;
   348		const struct of_device_id *of_id;
   349		const struct acpi_device_id *acpi_id;
   350	
   351		indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*cm32181));
   352		if (!indio_dev) {
   353			dev_err(&client->dev, "devm_iio_device_alloc failed\n");
   354			return -ENOMEM;
   355		}
   356	
   357		cm32181 = iio_priv(indio_dev);
   358		i2c_set_clientdata(client, indio_dev);
   359		cm32181->client = client;
   360	
   361		mutex_init(&cm32181->lock);
   362		indio_dev->dev.parent = &client->dev;
   363		indio_dev->channels = cm32181_channels;
   364		indio_dev->num_channels = ARRAY_SIZE(cm32181_channels);
   365		indio_dev->info = &cm32181_info;
   366		indio_dev->name = id->name;
   367		indio_dev->modes = INDIO_DIRECT_MODE;
   368	
   369		/* Lookup for chip ID from platform, acpi or of device table */
   370		if (id) {
   371			cm32181->chip_id = id->driver_data;
   372		} else if (ACPI_COMPANION(&client->dev)) {
   373			acpi_id = acpi_match_device(client->dev.driver->acpi_match_table,
   374						    &client->dev);
   375			if (!acpi_id)
   376				return -ENODEV;
   377	
   378			cm32181->chip_id = (kernel_ulong_t)acpi_id->driver_data;
   379		} else if (client->dev.of_node) {
   380			of_id = of_match_device(client->dev.driver->of_match_table,
   381						&client->dev);
   382			if (!of_id)
   383				return -ENODEV;
   384	
   385			cm32181->chip_id = (kernel_ulong_t)of_id->data;
   386		} else {
   387			return -ENODEV;
   388		}
   389	
   390		if (cm32181->chip_id == CM3218_ID) {
   391			if (ACPI_COMPANION(&client->dev) &&
   392			    client->addr == SMBUS_ARA_ADDR) {
   393				/*
   394				 * In case this device as been enumerated by acpi
   395				 * with the reserved smbus ARA address (first acpi
   396				 * connection), request change of address to the second
   397				 * connection.
   398				 */
   399				ret = i2c_acpi_set_connection(client, 1);
   400				if (ret)
   401					return ret;
   402			}
   403	
   404			/* cm3218 chip require an ara device on his adapter */
 > 405			ret = i2c_require_smbus_alert(client);
   406			if (ret < 0)
   407				return ret;
   408	
   409			/*
   410			 * If irq is given, request a threaded irq handler to manage
   411			 * smbus alert.
   412			 */
   413			if (client->irq > 0) {
   414				ret = devm_request_threaded_irq(&client->dev,
   415								client->irq,
   416								NULL, cm32181_irq,
   417								IRQF_ONESHOT,
   418								"cm32181", cm32181);
   419				if (ret)
   420					return ret;
   421			}
   422		}
   423	
   424		ret = cm32181_reg_init(cm32181);
   425		if (ret) {
   426			dev_err(&client->dev,
   427				"%s: register init failed\n",
   428				__func__);
   429			return ret;
   430		}
   431	
   432		ret = devm_iio_device_register(&client->dev, indio_dev);
   433		if (ret) {
   434			dev_err(&client->dev,
   435				"%s: regist device failed\n",
   436				__func__);
   437			return ret;
   438		}
   439	
   440		return 0;
   441	}
   442	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 21676 bytes --]

Re: [PATCH v6 1/4] i2c-core-acpi : Add i2c_acpi_set_connection

[Rafael J. Wysocki]

On Monday, December 25, 2017 4:57:20 PM CET Marc CAPDEVILLE wrote:
> Add a methode to allow clients to change their connection address on
> same adapter.
> 
> On ACPI enumerated device, when a device support smbus alert protocol,
> there are two acpi serial bus connection description. The order in which
> connection is given is not well defined and devices may be enumerated
> with the wrong address (the first one). So let the driver detect the
> unsupported address and request i2c acpi core to choose the second one
> at probing time.

By convention, the ordering of resources in _CRS is meaningful, but it is
a contract between AML and an OS driver written to work with it.

> Signed-off-by: Marc CAPDEVILLE <m.capdeville@no-log.org>
> ---
>  drivers/i2c/i2c-core-acpi.c | 50 +++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/i2c.h         | 10 +++++++++
>  2 files changed, 60 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
> index a9126b3cda61..47bc0da12055 100644
> --- a/drivers/i2c/i2c-core-acpi.c
> +++ b/drivers/i2c/i2c-core-acpi.c
> @@ -421,6 +421,56 @@ struct i2c_client *i2c_acpi_new_device(struct device *dev, int index,
>  }
>  EXPORT_SYMBOL_GPL(i2c_acpi_new_device);
>  

Please add a kerneldoc description of this function.

It is exported and needs to be documented properly.

> +int i2c_acpi_set_connection(struct i2c_client *client, int index)
> +{
> +	struct i2c_acpi_lookup lookup;
> +	struct i2c_adapter *adapter;
> +	struct acpi_device *adev;
> +	struct i2c_board_info info;
> +	LIST_HEAD(resource_list);
> +	int ret;
> +
> +	if (!client)
> +		return -ENODEV;
> +
> +	adev = ACPI_COMPANION(&client->dev);
> +	if (!adev)
> +		return -ENODEV;
> +
> +	memset(&info,  0, sizeof(info));
> +	memset(&lookup, 0, sizeof(lookup));
> +	lookup.info = &info;
> +	lookup.device_handle = acpi_device_handle(adev);
> +	lookup.index = index;
> +
> +	ret = acpi_dev_get_resources(adev, &resource_list,
> +				     i2c_acpi_fill_info, &lookup);
> +	acpi_dev_free_resource_list(&resource_list);
> +
> +	adapter = i2c_acpi_find_adapter_by_handle(lookup.adapter_handle);
> +
> +	if (ret < 0 || !info.addr)
> +		return -EINVAL;

Why do you make this check after calling i2c_acpi_find_adapter_by_handle()?

> +
> +	/* Only accept connection on same adapter */
> +	if (adapter != client->adapter)
> +		return -EINVAL;
> +
> +	ret = i2c_check_addr_validity(info.addr, info.flags);
> +	if (ret) {
> +		dev_err(&client->dev, "Invalid %d-bit I2C address 0x%02hx\n",
> +			info.flags & I2C_CLIENT_TEN ? 10 : 7, info.addr);
> +		return -EINVAL;
> +	}
> +
> +	/* Set new address and flags */
> +	client->addr = info.addr;
> +	client->flags = info.flags;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(i2c_acpi_set_connection);
> +
>  #ifdef CONFIG_ACPI_I2C_OPREGION
>  static int acpi_gsb_i2c_read_bytes(struct i2c_client *client,
>  		u8 cmd, u8 *data, u8 data_len)
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 0f774406fad0..618b453901da 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -595,6 +595,8 @@ struct i2c_adapter {
>  	const struct i2c_adapter_quirks *quirks;
>  
>  	struct irq_domain *host_notify_domain;
> +
> +	struct i2c_client *smbus_ara;	/* ARA for SMBUS if present */

Not used in this patch and how is it related to the other changes here?

>  };
>  #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev)
>  
> @@ -839,16 +841,24 @@ static inline const struct of_device_id
>  u32 i2c_acpi_find_bus_speed(struct device *dev);
>  struct i2c_client *i2c_acpi_new_device(struct device *dev, int index,
>  				       struct i2c_board_info *info);
> +
> +int i2c_acpi_set_connection(struct i2c_client *client, int index);
>  #else
>  static inline u32 i2c_acpi_find_bus_speed(struct device *dev)
>  {
>  	return 0;
>  }
> +
>  static inline struct i2c_client *i2c_acpi_new_device(struct device *dev,
>  					int index, struct i2c_board_info *info)
>  {
>  	return NULL;
>  }
> +
> +static inline int i2c_acpi_set_connection(struct i2c_client *client, int index)
> +{
> +	return -EINVAL;
> +}
>  #endif /* CONFIG_ACPI */
>  
>  #endif /* _LINUX_I2C_H */
> 

Re: [PATCH v6 3/4] iio : Add cm3218 smbus ara and acpi support

[Rafael J. Wysocki]

On Monday, December 25, 2017 4:57:22 PM CET Marc CAPDEVILLE wrote:
> 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 two 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 change address of the device for
> the one in the second acpi serial bus connection.
> This free the ara address so we can register with the smbus_alert
> driver.
> 
> If an interrupt resource is given, and a smbus_alert device was
> found on the adapter, we request a treaded interrupt to
> call i2c_smbus_alert_event to handle the alert.
> 
> In somme case, the treaded interrupt is not schedule before the driver
> try to initialize chip registers. So if first registers access fail, we
> call i2c_smbus_alert_event() to acknowledge initial alert, then retry
> register access.
> 
> Signed-off-by: Marc CAPDEVILLE <m.capdeville-n+LsquliYkMdnm+yROfE0A@public.gmane.org>
> ---
>  drivers/iio/light/cm32181.c | 120 ++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 115 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
> index aebf7dd071af..96c08755e6e3 100644
> --- a/drivers/iio/light/cm32181.c
> +++ b/drivers/iio/light/cm32181.c
> @@ -18,6 +18,9 @@
>  #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>
>  
>  /* Registers Address */
>  #define CM32181_REG_ADDR_CMD		0x00
> @@ -47,6 +50,11 @@
>  #define CM32181_CALIBSCALE_RESOLUTION	1000
>  #define MLUX_PER_LUX			1000
>  
> +#define CM32181_ID			0x81
> +#define CM3218_ID			0x18
> +
> +#define SMBUS_ARA_ADDR			0x0c
> +
>  static const u8 cm32181_reg[CM32181_CONF_REG_NUM] = {
>  	CM32181_REG_ADDR_CMD,
>  };
> @@ -57,6 +65,7 @@ static const int als_it_value[] = {25000, 50000, 100000, 200000, 400000,
>  
>  struct cm32181_chip {
>  	struct i2c_client *client;
> +	int chip_id;
>  	struct mutex lock;
>  	u16 conf_regs[CM32181_CONF_REG_NUM];
>  	int calibscale;
> @@ -77,11 +86,22 @@ static int cm32181_reg_init(struct cm32181_chip *cm32181)
>  	s32 ret;
>  
>  	ret = i2c_smbus_read_word_data(client, CM32181_REG_ADDR_ID);
> -	if (ret < 0)
> -		return ret;
> +	if (ret < 0) {
> +		if (cm32181->chip_id == CM3218_ID) {
> +			/*
> +			 * On cm3218, smbus alert trigger after probing,
> +			 * so poll for first alert here, then retry.
> +			 */
> +			i2c_smbus_alert_event(client);
> +			ret = i2c_smbus_read_word_data(client,
> +						       CM32181_REG_ADDR_ID);
> +		} else {
> +			return ret;
> +		}
> +	}
>  
>  	/* check device ID */
> -	if ((ret & 0xFF) != 0x81)
> +	if ((ret & 0xFF) != cm32181->chip_id)
>  		return -ENODEV;
>  
>  	/* Default Values */
> @@ -297,12 +317,36 @@ static const struct iio_info cm32181_info = {
>  	.attrs			= &cm32181_attribute_group,
>  };
>  
> +static void cm3218_alert(struct i2c_client *client,
> +			 enum i2c_alert_protocol type,
> +			 unsigned int data)
> +{
> +	/*
> +	 * nothing to do for now.
> +	 * This is just here to acknownledge the cm3218 alert.
> +	 */
> +}
> +
> +static irqreturn_t cm32181_irq(int irq, void *d)
> +{
> +	struct cm32181_chip *cm32181 = d;
> +
> +	if (cm32181->chip_id == CM3218_ID) {
> +		/* This is cm3218 chip irq, so handle the smbus alert */
> +		i2c_smbus_alert_event(cm32181->client);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
>  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;
> +	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) {
> @@ -322,6 +366,61 @@ 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(client->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 (ACPI_COMPANION(&client->dev) &&
> +		    client->addr == SMBUS_ARA_ADDR) {
> +			/*
> +			 * In case this device as been enumerated by acpi
> +			 * with the reserved smbus ARA address (first acpi
> +			 * connection), request change of address to the second
> +			 * connection.
> +			 */
> +			ret = i2c_acpi_set_connection(client, 1);
> +			if (ret)
> +				return ret;

This looks super-fragile to me.

What about making the enumeration aware of the SMBUS_ARA thing and avoid
using resources corresponding to that at all?

BTW, in comments and similar always spell ACPI in capitals.

Thanks,
Rafael

Re: [PATCH v6 3/4] iio : Add cm3218 smbus ara and acpi support

[Jonathan Cameron]

On Mon, 25 Dec 2017 16:57:22 +0100
Marc CAPDEVILLE <m.capdeville@no-log.org> wrote:

> On asus T100, Capella cm3218 chip is implemented as ambiant light

ambient

> 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 two 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

if

> device is enumerated via acpi, we change address of the device for
> the one in the second acpi serial bus connection.
> This free the ara address so we can register with the smbus_alert
> driver.
> 
> If an interrupt resource is given, and a smbus_alert device was
> found on the adapter, we request a treaded interrupt to

threaded

> call i2c_smbus_alert_event to handle the alert.
> 
> In somme case, the treaded interrupt is not schedule before the driver

threaded

> try to initialize chip registers. So if first registers access fail, we
> call i2c_smbus_alert_event() to acknowledge initial alert, then retry
> register access.
> 
> Signed-off-by: Marc CAPDEVILLE <m.capdeville@no-log.org>

The main thing that bothers me about this is it is putting a non trivial
burden on each individual driver.  Perhaps we let it go like this
for now, and see how common this is.  It would be possible (I think)
to do it in a fashion invisible to the client drivers, but I appreciate
that would be more complex than this. The multiple devices sharing a bus
with individual interrupt lines would need to be handled.

Minor comments inline.

Jonathan

> ---
>  drivers/iio/light/cm32181.c | 120 ++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 115 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
> index aebf7dd071af..96c08755e6e3 100644
> --- a/drivers/iio/light/cm32181.c
> +++ b/drivers/iio/light/cm32181.c
> @@ -18,6 +18,9 @@
>  #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>
>  
>  /* Registers Address */
>  #define CM32181_REG_ADDR_CMD		0x00
> @@ -47,6 +50,11 @@
>  #define CM32181_CALIBSCALE_RESOLUTION	1000
>  #define MLUX_PER_LUX			1000
>  
> +#define CM32181_ID			0x81
> +#define CM3218_ID			0x18
> +
> +#define SMBUS_ARA_ADDR			0x0c

This isn't driver specific - to my mind belongs in the smbus header.

> +
>  static const u8 cm32181_reg[CM32181_CONF_REG_NUM] = {
>  	CM32181_REG_ADDR_CMD,
>  };
> @@ -57,6 +65,7 @@ static const int als_it_value[] = {25000, 50000, 100000, 200000, 400000,
>  
>  struct cm32181_chip {
>  	struct i2c_client *client;
> +	int chip_id;
>  	struct mutex lock;
>  	u16 conf_regs[CM32181_CONF_REG_NUM];
>  	int calibscale;
> @@ -77,11 +86,22 @@ static int cm32181_reg_init(struct cm32181_chip *cm32181)
>  	s32 ret;
>  
>  	ret = i2c_smbus_read_word_data(client, CM32181_REG_ADDR_ID);
> -	if (ret < 0)
> -		return ret;
> +	if (ret < 0) {
> +		if (cm32181->chip_id == CM3218_ID) {
> +			/*
> +			 * On cm3218, smbus alert trigger after probing,
> +			 * so poll for first alert here, then retry.

Is it a level interrupt?  I'd have thought we could leave this out and let
the interrupt handler pick it up if so.
The check on the ID is rather paranoid anyway so I wouldn't worry about
it failing here.

Or are we blocked from finishing configuring the device until this is done?

> +			 */
> +			i2c_smbus_alert_event(client);
> +			ret = i2c_smbus_read_word_data(client,
> +						       CM32181_REG_ADDR_ID);
> +		} else {
> +			return ret;
> +		}
> +	}
>  
>  	/* check device ID */
> -	if ((ret & 0xFF) != 0x81)
> +	if ((ret & 0xFF) != cm32181->chip_id)
>  		return -ENODEV;
>  
>  	/* Default Values */
> @@ -297,12 +317,36 @@ static const struct iio_info cm32181_info = {
>  	.attrs			= &cm32181_attribute_group,
>  };
>  
> +static void cm3218_alert(struct i2c_client *client,
> +			 enum i2c_alert_protocol type,
> +			 unsigned int data)
> +{
> +	/*
> +	 * nothing to do for now.
> +	 * This is just here to acknownledge the cm3218 alert.
> +	 */
> +}
> +
> +static irqreturn_t cm32181_irq(int irq, void *d)
> +{
> +	struct cm32181_chip *cm32181 = d;
> +
> +	if (cm32181->chip_id == CM3218_ID) {

I'd move the check out and only register this handler if the chip supports
ARA in the first place.  That then makes this handler
generic to any ARA device. Not sure there is a generic handler available,
but it would make sense to add one if there isn't in the smbus code.

> +		/* This is cm3218 chip irq, so handle the smbus alert */
> +		i2c_smbus_alert_event(cm32181->client);
> +	}
The 32181 also has interrupt support - just looks like it isn't
ARA.

I guess support for that can be added here at some later date.
> +
> +	return IRQ_HANDLED;
> +}
> +
>  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;
> +	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) {
> @@ -322,6 +366,61 @@ 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(client->dev.driver->of_match_table,
> +					&client->dev);
> +		if (!of_id)
> +			return -ENODEV;
> +
> +		cm32181->chip_id = (kernel_ulong_t)of_id->data;

of_device_get_match_data perhaps?

I thought something similar had been proposed for acpi as well to
avoid this boiler plate, but can't find it right now so maybe that
never went anywhere.


> +	} else {
> +		return -ENODEV;
> +	}
> +
> +	if (cm32181->chip_id == CM3218_ID) {
> +		if (ACPI_COMPANION(&client->dev) &&
> +		    client->addr == SMBUS_ARA_ADDR) {
> +			/*
> +			 * In case this device as been enumerated by acpi
> +			 * with the reserved smbus ARA address (first acpi
> +			 * connection), request change of address to the second
> +			 * connection.
> +			 */
> +			ret = i2c_acpi_set_connection(client, 1);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		/* cm3218 chip require an ara device on his adapter */
> +		ret = i2c_require_smbus_alert(client);
> +		if (ret < 0)
> +			return ret;
> +
> +		/*
> +		 * If irq is given, request a threaded irq handler to manage
> +		 * smbus alert.
> +		 */
> +		if (client->irq > 0) {

If we have a chip that needs ara and no IRQ isn't it a probe failure case?
The device won't work as I understand it.

> +			ret = devm_request_threaded_irq(&client->dev,
> +							client->irq,
> +							NULL, cm32181_irq,
> +							IRQF_ONESHOT,
> +							"cm32181", cm32181);
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +
>  	ret = cm32181_reg_init(cm32181);
>  	if (ret) {
>  		dev_err(&client->dev,
> @@ -342,25 +441,36 @@ static int cm32181_probe(struct i2c_client *client,
>  }
>  
>  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,
> +	.alert		= cm3218_alert,

It's a bit ugly - I'm not sure we wouldn't be better registering
two separate i2c drivers so as not to specify the alert for devices
that don't handle it.

>  };
>  
>  module_i2c_driver(cm32181_driver);

Re: [PATCH v6 3/4] iio : Add cm3218 smbus ara and acpi support

[Jonathan Cameron]

On Thu, 28 Dec 2017 02:19:55 +0100
"Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:

> On Monday, December 25, 2017 4:57:22 PM CET Marc CAPDEVILLE wrote:
> > 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 two 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 change address of the device for
> > the one in the second acpi serial bus connection.
> > This free the ara address so we can register with the smbus_alert
> > driver.
> > 
> > If an interrupt resource is given, and a smbus_alert device was
> > found on the adapter, we request a treaded interrupt to
> > call i2c_smbus_alert_event to handle the alert.
> > 
> > In somme case, the treaded interrupt is not schedule before the driver
> > try to initialize chip registers. So if first registers access fail, we
> > call i2c_smbus_alert_event() to acknowledge initial alert, then retry
> > register access.
> > 
> > Signed-off-by: Marc CAPDEVILLE <m.capdeville@no-log.org>
> > ---
> >  drivers/iio/light/cm32181.c | 120 ++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 115 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
> > index aebf7dd071af..96c08755e6e3 100644
> > --- a/drivers/iio/light/cm32181.c
> > +++ b/drivers/iio/light/cm32181.c
> > @@ -18,6 +18,9 @@
> >  #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>
> >  
> >  /* Registers Address */
> >  #define CM32181_REG_ADDR_CMD		0x00
> > @@ -47,6 +50,11 @@
> >  #define CM32181_CALIBSCALE_RESOLUTION	1000
> >  #define MLUX_PER_LUX			1000
> >  
> > +#define CM32181_ID			0x81
> > +#define CM3218_ID			0x18
> > +
> > +#define SMBUS_ARA_ADDR			0x0c
> > +
> >  static const u8 cm32181_reg[CM32181_CONF_REG_NUM] = {
> >  	CM32181_REG_ADDR_CMD,
> >  };
> > @@ -57,6 +65,7 @@ static const int als_it_value[] = {25000, 50000, 100000, 200000, 400000,
> >  
> >  struct cm32181_chip {
> >  	struct i2c_client *client;
> > +	int chip_id;
> >  	struct mutex lock;
> >  	u16 conf_regs[CM32181_CONF_REG_NUM];
> >  	int calibscale;
> > @@ -77,11 +86,22 @@ static int cm32181_reg_init(struct cm32181_chip *cm32181)
> >  	s32 ret;
> >  
> >  	ret = i2c_smbus_read_word_data(client, CM32181_REG_ADDR_ID);
> > -	if (ret < 0)
> > -		return ret;
> > +	if (ret < 0) {
> > +		if (cm32181->chip_id == CM3218_ID) {
> > +			/*
> > +			 * On cm3218, smbus alert trigger after probing,
> > +			 * so poll for first alert here, then retry.
> > +			 */
> > +			i2c_smbus_alert_event(client);
> > +			ret = i2c_smbus_read_word_data(client,
> > +						       CM32181_REG_ADDR_ID);
> > +		} else {
> > +			return ret;
> > +		}
> > +	}
> >  
> >  	/* check device ID */
> > -	if ((ret & 0xFF) != 0x81)
> > +	if ((ret & 0xFF) != cm32181->chip_id)
> >  		return -ENODEV;
> >  
> >  	/* Default Values */
> > @@ -297,12 +317,36 @@ static const struct iio_info cm32181_info = {
> >  	.attrs			= &cm32181_attribute_group,
> >  };
> >  
> > +static void cm3218_alert(struct i2c_client *client,
> > +			 enum i2c_alert_protocol type,
> > +			 unsigned int data)
> > +{
> > +	/*
> > +	 * nothing to do for now.
> > +	 * This is just here to acknownledge the cm3218 alert.
> > +	 */
> > +}
> > +
> > +static irqreturn_t cm32181_irq(int irq, void *d)
> > +{
> > +	struct cm32181_chip *cm32181 = d;
> > +
> > +	if (cm32181->chip_id == CM3218_ID) {
> > +		/* This is cm3218 chip irq, so handle the smbus alert */
> > +		i2c_smbus_alert_event(cm32181->client);
> > +	}
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> >  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;
> > +	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) {
> > @@ -322,6 +366,61 @@ 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(client->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 (ACPI_COMPANION(&client->dev) &&
> > +		    client->addr == SMBUS_ARA_ADDR) {
> > +			/*
> > +			 * In case this device as been enumerated by acpi
> > +			 * with the reserved smbus ARA address (first acpi
> > +			 * connection), request change of address to the second
> > +			 * connection.
> > +			 */
> > +			ret = i2c_acpi_set_connection(client, 1);
> > +			if (ret)
> > +				return ret;  
> 
> This looks super-fragile to me.
> 
> What about making the enumeration aware of the SMBUS_ARA thing and avoid
> using resources corresponding to that at all?

I agree, if it can be done reasonably cleanly that would be preferable.

> 
> BTW, in comments and similar always spell ACPI in capitals.
> 
> Thanks,
> Rafael
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 4/4] iio : cm32181 : cosmetic cleanup

[Jonathan Cameron]

On Mon, 25 Dec 2017 16:57:23 +0100
Marc CAPDEVILLE <m.capdeville@no-log.org> wrote:

> Somme cosmetic cleanup suggested by Peter Meerwald-Stadler.
> 
> Macro name :
>    MLUX_PER_LUX => CM32181_MLUX_PER_LUX
> 
> Constante name :
>    als_it_bits => cm32181_als_it_bits
>    als_it_value => cm32181_als_it_value
> 
> Comment :
>    Registers Address => Register Addresses
> 
> Suggested-by:  Peter Meerwald-Stadler <pmeerw@pmeerw.net>
> Signed-off-by: Marc CAPDEVILLE <m.capdeville@no-log.org>
This is all fine. For my reference please add

Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/iio/light/cm32181.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
> index 96c08755e6e3..3e6b244d5cd1 100644
> --- a/drivers/iio/light/cm32181.c
> +++ b/drivers/iio/light/cm32181.c
> @@ -22,7 +22,7 @@
>  #include <linux/acpi.h>
>  #include <linux/of_device.h>
>  
> -/* Registers Address */
> +/* Register Addresses */
>  #define CM32181_REG_ADDR_CMD		0x00
>  #define CM32181_REG_ADDR_ALS		0x04
>  #define CM32181_REG_ADDR_STATUS		0x06
> @@ -48,7 +48,7 @@
>  #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
> @@ -59,8 +59,8 @@ 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 {
> @@ -137,9 +137,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;
>  		}
>  	}
> @@ -162,14 +162,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);
> @@ -215,7 +215,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;
> @@ -283,9 +283,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");
>  }
>  

Re: [PATCH v6 2/4] i2c-smbus : Add client discovered ARA support

[Jonathan Cameron]

On Mon, 25 Dec 2017 16:57:21 +0100
Marc CAPDEVILLE <m.capdeville-n+LsquliYkMdnm+yROfE0A@public.gmane.org> wrote:

> This is from rfc by Alan Cox : https://patchwork.ozlabs.org/patch/381773
> 
> The idea is as follows (extract from above rfc) :
> - If an adapter knows about its ARA and smbus alerts then the adapter
>   creates its own interrupt handler as before
> 
> - If a client knows it needs smbus alerts it calls
>   i2c_require_smbus_alert(). This ensures that there is an ARA handler
>   registered and tells the client whether the adapter is handling it
>   anyway or not.
> 
> - When the client learns that an ARA event has occurred it calls
>   i2c_smbus_alert_event() which uses the existing ARA mechanism to kick
>   things off.

A few minor things inline.  I'm not sure the balance between what is
done in driver and what is in the core is quite right yet.

Jonathan

> 
> Suggested-by: Alan Cox <alan-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> Signed-off-by: Marc CAPDEVILLE <m.capdeville-n+LsquliYkMdnm+yROfE0A@public.gmane.org>
> ---
>  drivers/i2c/i2c-smbus.c   | 83 +++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/i2c-smbus.h | 15 +++++++++
>  2 files changed, 98 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
> index 5a1dd7f13bac..e97fbafd10ef 100644
> --- a/drivers/i2c/i2c-smbus.c
> +++ b/drivers/i2c/i2c-smbus.c
> @@ -161,6 +161,9 @@ static int smbalert_probe(struct i2c_client *ara,
>  	}
>  
>  	i2c_set_clientdata(ara, alert);
> +
> +	ara->adapter->smbus_ara = ara;
> +
>  	dev_info(&adapter->dev, "supports SMBALERT#\n");
>  
>  	return 0;
> @@ -172,6 +175,9 @@ static int smbalert_remove(struct i2c_client *ara)
>  	struct i2c_smbus_alert *alert = i2c_get_clientdata(ara);
>  
>  	cancel_work_sync(&alert->alert);
> +
> +	ara->adapter->smbus_ara = NULL;
> +
>  	return 0;
>  }
>  
> @@ -210,6 +216,83 @@ int i2c_handle_smbus_alert(struct i2c_client *ara)
>  }
>  EXPORT_SYMBOL_GPL(i2c_handle_smbus_alert);
>  
> +/*
/** It looks like kernel doc to me.
> + * i2c_require_smbus_alert - Client discovered SMBus alert
> + * @c: client requiring ARA
> + *
> + * When a client needs an ARA it calls this method. If the bus adapter
> + * supports ARA and already knows how to do so then it will already have
> + * configured for ARA and this is a no-op. If not then we set up an ARA
> + * on the adapter.
> + *
> + * We *cannot* simply register a new IRQ handler for this because we might
> + * have multiple GPIO interrupts to devices all of which trigger an ARA.
I'm a little confused by this comment.  Do you mean:

1. Same gpio irq is provided as as shared irq to a number of devices?
(In this case we are just aiming not to register the same IRQ multiple times
as each driver may have already done it?)  If it is this case, I think we
should be pushing the irq register and checking logic to the smbus core which
can then verify if it is the same interrupt or not? (could be case 2 below
in which case we need to register another handler).

2. Different gpio irqs provided to a number of devices on same bus with
   each acting as ARA interrupt?  (this is hideous so I hope not - but
   I would be surprised if we never see a board doing this)

> + *
> + * Return:
> + *	0 if ara support already registered
> + *	1 if call register a new smbus_alert device
> + *	<0 on error
> + */
> +int i2c_require_smbus_alert(struct i2c_client *client)
> +{
> +	struct i2c_adapter *adapter = client->adapter;
> +	struct i2c_smbus_alert_setup setup;
> +	struct i2c_client *ara;
> +
> +	/* ARA is already known and handled by the adapter (ideal case)

/*
 * ARA

> +	 * or another client has specified ARA is needed
> +	 */
> +	if (adapter->smbus_ara)
> +		return 0;
> +
> +	/* Client driven, do not set up a new IRQ handler */
> +	setup.irq = 0;
> +
> +	ara = i2c_setup_smbus_alert(adapter, &setup);
> +	if (!ara)
> +		return -ENODEV;
> +
> +	return 1;
> +}
> +EXPORT_SYMBOL_GPL(i2c_require_smbus_alert);
> +
> +/*

Fix this up (needs a short description) and make it kernel-doc /**

> + * i2c_smbus_alert_event
> + * @client: the client who known of a probable ara event
> + * Context: can't sleep
> + *
> + * Helper function to be called from an I2C device driver's interrupt
> + * handler. It will schedule the alert work, in turn calling the
> + * corresponding I2C device driver's alert function.
> + *
> + * It is assumed that client is an i2c client who previously call
> + * i2c_require_smbus_alert().
> + */
> +int i2c_smbus_alert_event(struct i2c_client *client)
> +{
> +	struct i2c_adapter *adapter;
> +	struct i2c_client *ara;
> +	struct i2c_smbus_alert *alert;
> +
> +	if (!client)
> +		return -EINVAL;
> +
> +	adapter = client->adapter;
> +	if (!adapter)
> +		return -EINVAL;
> +
> +	ara = adapter->smbus_ara;
> +	if (!ara)
> +		return -EINVAL;
> +
> +	alert = i2c_get_clientdata(ara);
> +	if (!alert)
> +		return -EINVAL;
> +
> +	return schedule_work(&alert->alert);
> +}
> +EXPORT_SYMBOL_GPL(i2c_smbus_alert_event);
> +
>  module_i2c_driver(smbalert_driver);
>  
>  MODULE_AUTHOR("Jean Delvare <jdelvare-l3A5Bk7waGM@public.gmane.org>");
> diff --git a/include/linux/i2c-smbus.h b/include/linux/i2c-smbus.h
> index fb0e040b1abb..49f362fa6ac5 100644
> --- a/include/linux/i2c-smbus.h
> +++ b/include/linux/i2c-smbus.h
> @@ -58,4 +58,19 @@ static inline int of_i2c_setup_smbus_alert(struct i2c_adapter *adap)
>  }
>  #endif
>  
> +#if IS_ENABLED(CONFIG_I2C_SMBUS)
> +int i2c_require_smbus_alert(struct i2c_client *client);
> +int i2c_smbus_alert_event(struct i2c_client *client);
> +#else
> +static inline int i2c_require_smbus_alert(struct i2c_client *client)
> +{
> +	return NULL;
> +}
> +
> +static inline int i2c_smbus_alert_event(struct i2c_client *client)
> +{
> +	return NULL;
> +}
> +#endif
> +
>  #endif /* _LINUX_I2C_SMBUS_H */