[PATCH v3 01/11] iio: light: cm32181: Switch to new style i2c-driver probe function

[PATCH v3 01/11] iio: light: cm32181: Switch to new style i2c-driver probe function

[Hans de Goede]

Switch to the new style i2c-driver probe_new probe function and drop the
unnecessary i2c_device_id table (we do not have any old style board files
using this).

This is a preparation patch for adding ACPI binding support.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v3:
- This is a new patch in v3 of this patch-set
---
 drivers/iio/light/cm32181.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
index 5f4fb5674fa0..cc57190a24cb 100644
--- a/drivers/iio/light/cm32181.c
+++ b/drivers/iio/light/cm32181.c
@@ -294,8 +294,7 @@ static const struct iio_info cm32181_info = {
 	.attrs			= &cm32181_attribute_group,
 };
 
-static int cm32181_probe(struct i2c_client *client,
-			const struct i2c_device_id *id)
+static int cm32181_probe(struct i2c_client *client)
 {
 	struct cm32181_chip *cm32181;
 	struct iio_dev *indio_dev;
@@ -316,7 +315,7 @@ static int cm32181_probe(struct i2c_client *client,
 	indio_dev->channels = cm32181_channels;
 	indio_dev->num_channels = ARRAY_SIZE(cm32181_channels);
 	indio_dev->info = &cm32181_info;
-	indio_dev->name = id->name;
+	indio_dev->name = dev_name(&client->dev);
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
 	ret = cm32181_reg_init(cm32181);
@@ -338,13 +337,6 @@ static int cm32181_probe(struct i2c_client *client,
 	return 0;
 }
 
-static const struct i2c_device_id cm32181_id[] = {
-	{ "cm32181", 0 },
-	{ }
-};
-
-MODULE_DEVICE_TABLE(i2c, cm32181_id);
-
 static const struct of_device_id cm32181_of_match[] = {
 	{ .compatible = "capella,cm32181" },
 	{ }
@@ -356,8 +348,7 @@ static struct i2c_driver cm32181_driver = {
 		.name	= "cm32181",
 		.of_match_table = of_match_ptr(cm32181_of_match),
 	},
-	.id_table       = cm32181_id,
-	.probe		= cm32181_probe,
+	.probe_new	= cm32181_probe,
 };
 
 module_i2c_driver(cm32181_driver);
-- 
2.26.0

[PATCH v3 02/11] iio: light: cm32181: Add support for ACPI enumeration

[Hans de Goede]

Add support for ACPI enumeration, this has been tested on a HP
HP Pavilion x2 Detachable 10 (Bay Trail model).

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v3:
- This is a new patch in v3 of this patch-set
---
 drivers/iio/light/cm32181.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
index cc57190a24cb..5ca8bbf6667c 100644
--- a/drivers/iio/light/cm32181.c
+++ b/drivers/iio/light/cm32181.c
@@ -343,9 +343,18 @@ static const struct of_device_id cm32181_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, cm32181_of_match);
 
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id cm32181_acpi_match[] = {
+	{ "CPLM3218", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, cm32181_acpi_match);
+#endif
+
 static struct i2c_driver cm32181_driver = {
 	.driver = {
 		.name	= "cm32181",
+		.acpi_match_table = ACPI_PTR(cm32181_acpi_match),
 		.of_match_table = of_match_ptr(cm32181_of_match),
 	},
 	.probe_new	= cm32181_probe,
-- 
2.26.0

[PATCH v3 03/11] iio: light: cm32181: Add some extra register defines

[Hans de Goede]

These come from a newer version of cm32181.c, which is floating around
the net, with a copyright of:

 * Copyright (C) 2014 Capella Microsystems Inc.
 * Author: Kevin Tsai <ktsai@capellamicro.com>
 *
 * This program is free software; you can redistribute it and/or modify it
 * under the terms of the GNU General Public License version 2, as published
 * by the Free Software Foundation.

Note that this removes the bogus CM32181_CMD_ALS_ENABLE define, there
is no enable bit, only a disable bit and enabled is the absence of
being disabled.

This is a preparation patch for adding support for the older
CM3218 model of the light sensor.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/iio/light/cm32181.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
index 5ca8bbf6667c..6fc0a753c499 100644
--- a/drivers/iio/light/cm32181.c
+++ b/drivers/iio/light/cm32181.c
@@ -18,6 +18,9 @@
 
 /* Registers Address */
 #define CM32181_REG_ADDR_CMD		0x00
+#define CM32181_REG_ADDR_WH		0x01
+#define CM32181_REG_ADDR_WL		0x02
+#define CM32181_REG_ADDR_TEST		0x03
 #define CM32181_REG_ADDR_ALS		0x04
 #define CM32181_REG_ADDR_STATUS		0x06
 #define CM32181_REG_ADDR_ID		0x07
@@ -26,9 +29,13 @@
 #define CM32181_CONF_REG_NUM		0x01
 
 /* CMD register */
-#define CM32181_CMD_ALS_ENABLE		0x00
-#define CM32181_CMD_ALS_DISABLE		0x01
-#define CM32181_CMD_ALS_INT_EN		0x02
+#define CM32181_CMD_ALS_DISABLE		BIT(0)
+#define CM32181_CMD_ALS_INT_EN		BIT(1)
+#define CM32181_CMD_ALS_THRES_WINDOW	BIT(2)
+
+#define CM32181_CMD_ALS_PERS_SHIFT	4
+#define CM32181_CMD_ALS_PERS_MASK	(0x03 << CM32181_CMD_ALS_PERS_SHIFT)
+#define CM32181_CMD_ALS_PERS_DEFAULT	(0x01 << CM32181_CMD_ALS_PERS_SHIFT)
 
 #define CM32181_CMD_ALS_IT_SHIFT	6
 #define CM32181_CMD_ALS_IT_MASK		(0x0F << CM32181_CMD_ALS_IT_SHIFT)
@@ -82,7 +89,7 @@ static int cm32181_reg_init(struct cm32181_chip *cm32181)
 		return -ENODEV;
 
 	/* Default Values */
-	cm32181->conf_regs[CM32181_REG_ADDR_CMD] = CM32181_CMD_ALS_ENABLE |
+	cm32181->conf_regs[CM32181_REG_ADDR_CMD] =
 			CM32181_CMD_ALS_IT_DEFAULT | CM32181_CMD_ALS_SM_DEFAULT;
 	cm32181->calibscale = CM32181_CALIBSCALE_DEFAULT;
 
-- 
2.26.0

[PATCH v3 04/11] iio: light: cm32181: Add support for the CM3218

[Hans de Goede]

Add support for the CM3218 which is an older version of the
CM32181.

This is based on a newer version of cm32181.c, with a copyright of:

 * Copyright (C) 2014 Capella Microsystems Inc.
 * Author: Kevin Tsai <ktsai@capellamicro.com>
 *
 * This program is free software; you can redistribute it and/or modify it
 * under the terms of the GNU General Public License version 2, as published
 * by the Free Software Foundation.

Which is floating around on the net in various places, but the changes
from this newer version never made it upstream.

This was tested on an Asus T100TA and an Asus T100CHI, which both come
with the CM3218 variant of the light sensor.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/iio/light/cm32181.c | 48 +++++++++++++++++++++++++++----------
 1 file changed, 36 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
index 6fc0a753c499..065bc7a11f84 100644
--- a/drivers/iio/light/cm32181.c
+++ b/drivers/iio/light/cm32181.c
@@ -55,15 +55,24 @@ 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,
-	800000};
+/* CM3218 Family */
+static const int cm3218_als_it_bits[] = { 0, 1, 2, 3 };
+static const int cm3218_als_it_values[] = { 100000, 200000, 400000, 800000 };
+
+/* CM32181 Family */
+static const int cm32181_als_it_bits[] = { 12, 8, 0, 1, 2, 3 };
+static const int cm32181_als_it_values[] = {
+	25000, 50000, 100000, 200000, 400000, 800000
+};
 
 struct cm32181_chip {
 	struct i2c_client *client;
 	struct mutex lock;
 	u16 conf_regs[CM32181_CONF_REG_NUM];
 	int calibscale;
+	int num_als_it;
+	const int *als_it_bits;
+	const int *als_it_values;
 };
 
 /**
@@ -85,8 +94,21 @@ static int cm32181_reg_init(struct cm32181_chip *cm32181)
 		return ret;
 
 	/* check device ID */
-	if ((ret & 0xFF) != 0x81)
+	switch (ret & 0xFF) {
+	case 0x18: /* CM3218 */
+		cm32181->num_als_it = ARRAY_SIZE(cm3218_als_it_bits);
+		cm32181->als_it_bits = cm3218_als_it_bits;
+		cm32181->als_it_values = cm3218_als_it_values;
+		break;
+	case 0x81: /* CM32181 */
+	case 0x82: /* CM32182, fully compat. with CM32181 */
+		cm32181->num_als_it = ARRAY_SIZE(cm32181_als_it_bits);
+		cm32181->als_it_bits = cm32181_als_it_bits;
+		cm32181->als_it_values = cm32181_als_it_values;
+		break;
+	default:
 		return -ENODEV;
+	}
 
 	/* Default Values */
 	cm32181->conf_regs[CM32181_REG_ADDR_CMD] =
@@ -121,9 +143,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 < cm32181->num_als_it; i++) {
+		if (als_it == cm32181->als_it_bits[i]) {
+			*val2 = cm32181->als_it_values[i];
 			return IIO_VAL_INT_PLUS_MICRO;
 		}
 	}
@@ -146,14 +168,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 = cm32181->num_als_it;
 	for (i = 0; i < n; i++)
-		if (val <= als_it_value[i])
+		if (val <= cm32181->als_it_values[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);
@@ -265,11 +287,12 @@ static int cm32181_write_raw(struct iio_dev *indio_dev,
 static ssize_t cm32181_get_it_available(struct device *dev,
 			struct device_attribute *attr, char *buf)
 {
+	struct cm32181_chip *cm32181 = iio_priv(dev_to_iio_dev(dev));
 	int i, n, len;
 
-	n = ARRAY_SIZE(als_it_value);
+	n = cm32181->num_als_it;
 	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_values[i]);
 	return len + sprintf(buf + len, "\n");
 }
 
@@ -345,6 +368,7 @@ static int cm32181_probe(struct i2c_client *client)
 }
 
 static const struct of_device_id cm32181_of_match[] = {
+	{ .compatible = "capella,cm3218" },
 	{ .compatible = "capella,cm32181" },
 	{ }
 };
-- 
2.26.0

Re: [PATCH v3 04/11] iio: light: cm32181: Add support for the CM3218

[Jonathan Cameron]

On Tue, 28 Apr 2020 19:29:16 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> Add support for the CM3218 which is an older version of the
> CM32181.
> 
> This is based on a newer version of cm32181.c, with a copyright of:
> 
>  * Copyright (C) 2014 Capella Microsystems Inc.
>  * Author: Kevin Tsai <ktsai@capellamicro.com>
>  *
>  * This program is free software; you can redistribute it and/or modify it
>  * under the terms of the GNU General Public License version 2, as published
>  * by the Free Software Foundation.
> 
> Which is floating around on the net in various places, but the changes
> from this newer version never made it upstream.
> 
> This was tested on an Asus T100TA and an Asus T100CHI, which both come
> with the CM3218 variant of the light sensor.
> 
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

The need to also store the name for the different sensors makes
the case for picking between 'chip_info' structures in here stronger.
So I'd do that instead of setting multiple elements in your
switch statement... (See inline)

> ---
>  drivers/iio/light/cm32181.c | 48 +++++++++++++++++++++++++++----------
>  1 file changed, 36 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
> index 6fc0a753c499..065bc7a11f84 100644
> --- a/drivers/iio/light/cm32181.c
> +++ b/drivers/iio/light/cm32181.c
> @@ -55,15 +55,24 @@ 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,
> -	800000};
> +/* CM3218 Family */
> +static const int cm3218_als_it_bits[] = { 0, 1, 2, 3 };
> +static const int cm3218_als_it_values[] = { 100000, 200000, 400000, 800000 };
> +
> +/* CM32181 Family */
> +static const int cm32181_als_it_bits[] = { 12, 8, 0, 1, 2, 3 };
> +static const int cm32181_als_it_values[] = {
> +	25000, 50000, 100000, 200000, 400000, 800000
> +};
>  
>  struct cm32181_chip {
>  	struct i2c_client *client;
>  	struct mutex lock;
>  	u16 conf_regs[CM32181_CONF_REG_NUM];
>  	int calibscale;
> +	int num_als_it;
> +	const int *als_it_bits;
> +	const int *als_it_values;
These are constant for each type of chip and come as a set.
Better to just have a cm32181_chip_info structure with all 3 in it
(and the name as mentioned earlier).  That way your switch below
just becomes a matter of setting a single pointer for each case.

>  };
>  
>  /**
> @@ -85,8 +94,21 @@ static int cm32181_reg_init(struct cm32181_chip *cm32181)
>  		return ret;
>  
>  	/* check device ID */
> -	if ((ret & 0xFF) != 0x81)
> +	switch (ret & 0xFF) {
> +	case 0x18: /* CM3218 */

I'd ideally like to see a sanity check that we have the part expected.
So the compatible matches what we actually get.

If it doesn't but the part is still one we support print a warning.

> +		cm32181->num_als_it = ARRAY_SIZE(cm3218_als_it_bits);
> +		cm32181->als_it_bits = cm3218_als_it_bits;
> +		cm32181->als_it_values = cm3218_als_it_values;
> +		break;
> +	case 0x81: /* CM32181 */
> +	case 0x82: /* CM32182, fully compat. with CM32181 */
> +		cm32181->num_als_it = ARRAY_SIZE(cm32181_als_it_bits);
> +		cm32181->als_it_bits = cm32181_als_it_bits;
> +		cm32181->als_it_values = cm32181_als_it_values;
> +		break;
> +	default:
>  		return -ENODEV;
> +	}
>  
>  	/* Default Values */
>  	cm32181->conf_regs[CM32181_REG_ADDR_CMD] =
> @@ -121,9 +143,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 < cm32181->num_als_it; i++) {
> +		if (als_it == cm32181->als_it_bits[i]) {
> +			*val2 = cm32181->als_it_values[i];
>  			return IIO_VAL_INT_PLUS_MICRO;
>  		}
>  	}
> @@ -146,14 +168,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 = cm32181->num_als_it;
>  	for (i = 0; i < n; i++)
> -		if (val <= als_it_value[i])
> +		if (val <= cm32181->als_it_values[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);
> @@ -265,11 +287,12 @@ static int cm32181_write_raw(struct iio_dev *indio_dev,
>  static ssize_t cm32181_get_it_available(struct device *dev,
>  			struct device_attribute *attr, char *buf)
>  {
> +	struct cm32181_chip *cm32181 = iio_priv(dev_to_iio_dev(dev));
>  	int i, n, len;
>  
> -	n = ARRAY_SIZE(als_it_value);
> +	n = cm32181->num_als_it;
>  	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_values[i]);
>  	return len + sprintf(buf + len, "\n");
>  }
>  
> @@ -345,6 +368,7 @@ static int cm32181_probe(struct i2c_client *client)
>  }
>  
>  static const struct of_device_id cm32181_of_match[] = {
> +	{ .compatible = "capella,cm3218" },
>  	{ .compatible = "capella,cm32181" },
>  	{ }
>  };

Re: [PATCH v3 04/11] iio: light: cm32181: Add support for the CM3218

[Hans de Goede]

Hi,

On 5/3/20 12:59 PM, Jonathan Cameron wrote:
> On Tue, 28 Apr 2020 19:29:16 +0200
> Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> Add support for the CM3218 which is an older version of the
>> CM32181.
>>
>> This is based on a newer version of cm32181.c, with a copyright of:
>>
>>   * Copyright (C) 2014 Capella Microsystems Inc.
>>   * Author: Kevin Tsai <ktsai@capellamicro.com>
>>   *
>>   * This program is free software; you can redistribute it and/or modify it
>>   * under the terms of the GNU General Public License version 2, as published
>>   * by the Free Software Foundation.
>>
>> Which is floating around on the net in various places, but the changes
>> from this newer version never made it upstream.
>>
>> This was tested on an Asus T100TA and an Asus T100CHI, which both come
>> with the CM3218 variant of the light sensor.
>>
>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> The need to also store the name for the different sensors makes
> the case for picking between 'chip_info' structures in here stronger.
> So I'd do that instead of setting multiple elements in your
> switch statement... (See inline)
> 
>> ---
>>   drivers/iio/light/cm32181.c | 48 +++++++++++++++++++++++++++----------
>>   1 file changed, 36 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
>> index 6fc0a753c499..065bc7a11f84 100644
>> --- a/drivers/iio/light/cm32181.c
>> +++ b/drivers/iio/light/cm32181.c
>> @@ -55,15 +55,24 @@ 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,
>> -	800000};
>> +/* CM3218 Family */
>> +static const int cm3218_als_it_bits[] = { 0, 1, 2, 3 };
>> +static const int cm3218_als_it_values[] = { 100000, 200000, 400000, 800000 };
>> +
>> +/* CM32181 Family */
>> +static const int cm32181_als_it_bits[] = { 12, 8, 0, 1, 2, 3 };
>> +static const int cm32181_als_it_values[] = {
>> +	25000, 50000, 100000, 200000, 400000, 800000
>> +};
>>   
>>   struct cm32181_chip {
>>   	struct i2c_client *client;
>>   	struct mutex lock;
>>   	u16 conf_regs[CM32181_CONF_REG_NUM];
>>   	int calibscale;
>> +	int num_als_it;
>> +	const int *als_it_bits;
>> +	const int *als_it_values;
> These are constant for each type of chip and come as a set.
> Better to just have a cm32181_chip_info structure with all 3 in it
> (and the name as mentioned earlier).  That way your switch below
> just becomes a matter of setting a single pointer for each case.

Ok I will add a chip_info structure for v4 off the patch-set.


> 
>>   };
>>   
>>   /**
>> @@ -85,8 +94,21 @@ static int cm32181_reg_init(struct cm32181_chip *cm32181)
>>   		return ret;
>>   
>>   	/* check device ID */
>> -	if ((ret & 0xFF) != 0x81)
>> +	switch (ret & 0xFF) {
>> +	case 0x18: /* CM3218 */
> 
> I'd ideally like to see a sanity check that we have the part expected.
> So the compatible matches what we actually get.

Erm, so far I've only seen the CM3218 on X86 + ACPI devices which
use an ACPI id of CPLM3218 for both sensor models, so at least
on ACPI there is nothing to check.

Regards,

Hans



> 
> If it doesn't but the part is still one we support print a warning.
> 
>> +		cm32181->num_als_it = ARRAY_SIZE(cm3218_als_it_bits);
>> +		cm32181->als_it_bits = cm3218_als_it_bits;
>> +		cm32181->als_it_values = cm3218_als_it_values;
>> +		break;
>> +	case 0x81: /* CM32181 */
>> +	case 0x82: /* CM32182, fully compat. with CM32181 */
>> +		cm32181->num_als_it = ARRAY_SIZE(cm32181_als_it_bits);
>> +		cm32181->als_it_bits = cm32181_als_it_bits;
>> +		cm32181->als_it_values = cm32181_als_it_values;
>> +		break;
>> +	default:
>>   		return -ENODEV;
>> +	}
>>   
>>   	/* Default Values */
>>   	cm32181->conf_regs[CM32181_REG_ADDR_CMD] =
>> @@ -121,9 +143,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 < cm32181->num_als_it; i++) {
>> +		if (als_it == cm32181->als_it_bits[i]) {
>> +			*val2 = cm32181->als_it_values[i];
>>   			return IIO_VAL_INT_PLUS_MICRO;
>>   		}
>>   	}
>> @@ -146,14 +168,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 = cm32181->num_als_it;
>>   	for (i = 0; i < n; i++)
>> -		if (val <= als_it_value[i])
>> +		if (val <= cm32181->als_it_values[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);
>> @@ -265,11 +287,12 @@ static int cm32181_write_raw(struct iio_dev *indio_dev,
>>   static ssize_t cm32181_get_it_available(struct device *dev,
>>   			struct device_attribute *attr, char *buf)
>>   {
>> +	struct cm32181_chip *cm32181 = iio_priv(dev_to_iio_dev(dev));
>>   	int i, n, len;
>>   
>> -	n = ARRAY_SIZE(als_it_value);
>> +	n = cm32181->num_als_it;
>>   	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_values[i]);
>>   	return len + sprintf(buf + len, "\n");
>>   }
>>   
>> @@ -345,6 +368,7 @@ static int cm32181_probe(struct i2c_client *client)
>>   }
>>   
>>   static const struct of_device_id cm32181_of_match[] = {
>> +	{ .compatible = "capella,cm3218" },
>>   	{ .compatible = "capella,cm32181" },
>>   	{ }
>>   };
> 

Re: [PATCH v3 04/11] iio: light: cm32181: Add support for the CM3218

[Jonathan Cameron]

On Mon, 4 May 2020 11:49:59 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 5/3/20 12:59 PM, Jonathan Cameron wrote:
> > On Tue, 28 Apr 2020 19:29:16 +0200
> > Hans de Goede <hdegoede@redhat.com> wrote:
> >   
> >> Add support for the CM3218 which is an older version of the
> >> CM32181.
> >>
> >> This is based on a newer version of cm32181.c, with a copyright of:
> >>
> >>   * Copyright (C) 2014 Capella Microsystems Inc.
> >>   * Author: Kevin Tsai <ktsai@capellamicro.com>
> >>   *
> >>   * This program is free software; you can redistribute it and/or modify it
> >>   * under the terms of the GNU General Public License version 2, as published
> >>   * by the Free Software Foundation.
> >>
> >> Which is floating around on the net in various places, but the changes
> >> from this newer version never made it upstream.
> >>
> >> This was tested on an Asus T100TA and an Asus T100CHI, which both come
> >> with the CM3218 variant of the light sensor.
> >>
> >> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>  
> > 
> > The need to also store the name for the different sensors makes
> > the case for picking between 'chip_info' structures in here stronger.
> > So I'd do that instead of setting multiple elements in your
> > switch statement... (See inline)
> >   
> >> ---
> >>   drivers/iio/light/cm32181.c | 48 +++++++++++++++++++++++++++----------
> >>   1 file changed, 36 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
> >> index 6fc0a753c499..065bc7a11f84 100644
> >> --- a/drivers/iio/light/cm32181.c
> >> +++ b/drivers/iio/light/cm32181.c
> >> @@ -55,15 +55,24 @@ 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,
> >> -	800000};
> >> +/* CM3218 Family */
> >> +static const int cm3218_als_it_bits[] = { 0, 1, 2, 3 };
> >> +static const int cm3218_als_it_values[] = { 100000, 200000, 400000, 800000 };
> >> +
> >> +/* CM32181 Family */
> >> +static const int cm32181_als_it_bits[] = { 12, 8, 0, 1, 2, 3 };
> >> +static const int cm32181_als_it_values[] = {
> >> +	25000, 50000, 100000, 200000, 400000, 800000
> >> +};
> >>   
> >>   struct cm32181_chip {
> >>   	struct i2c_client *client;
> >>   	struct mutex lock;
> >>   	u16 conf_regs[CM32181_CONF_REG_NUM];
> >>   	int calibscale;
> >> +	int num_als_it;
> >> +	const int *als_it_bits;
> >> +	const int *als_it_values;  
> > These are constant for each type of chip and come as a set.
> > Better to just have a cm32181_chip_info structure with all 3 in it
> > (and the name as mentioned earlier).  That way your switch below
> > just becomes a matter of setting a single pointer for each case.  
> 
> Ok I will add a chip_info structure for v4 off the patch-set.
> 
> 
> >   
> >>   };
> >>   
> >>   /**
> >> @@ -85,8 +94,21 @@ static int cm32181_reg_init(struct cm32181_chip *cm32181)
> >>   		return ret;
> >>   
> >>   	/* check device ID */
> >> -	if ((ret & 0xFF) != 0x81)
> >> +	switch (ret & 0xFF) {
> >> +	case 0x18: /* CM3218 */  
> > 
> > I'd ideally like to see a sanity check that we have the part expected.
> > So the compatible matches what we actually get.  
> 
> Erm, so far I've only seen the CM3218 on X86 + ACPI devices which
> use an ACPI id of CPLM3218 for both sensor models, so at least
> on ACPI there is nothing to check.

Groan.  Never mind then.

J 

> 
> Regards,
> 
> Hans
> 
> 
> 
> > 
> > If it doesn't but the part is still one we support print a warning.
> >   
> >> +		cm32181->num_als_it = ARRAY_SIZE(cm3218_als_it_bits);
> >> +		cm32181->als_it_bits = cm3218_als_it_bits;
> >> +		cm32181->als_it_values = cm3218_als_it_values;
> >> +		break;
> >> +	case 0x81: /* CM32181 */
> >> +	case 0x82: /* CM32182, fully compat. with CM32181 */
> >> +		cm32181->num_als_it = ARRAY_SIZE(cm32181_als_it_bits);
> >> +		cm32181->als_it_bits = cm32181_als_it_bits;
> >> +		cm32181->als_it_values = cm32181_als_it_values;
> >> +		break;
> >> +	default:
> >>   		return -ENODEV;
> >> +	}
> >>   
> >>   	/* Default Values */
> >>   	cm32181->conf_regs[CM32181_REG_ADDR_CMD] =
> >> @@ -121,9 +143,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 < cm32181->num_als_it; i++) {
> >> +		if (als_it == cm32181->als_it_bits[i]) {
> >> +			*val2 = cm32181->als_it_values[i];
> >>   			return IIO_VAL_INT_PLUS_MICRO;
> >>   		}
> >>   	}
> >> @@ -146,14 +168,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 = cm32181->num_als_it;
> >>   	for (i = 0; i < n; i++)
> >> -		if (val <= als_it_value[i])
> >> +		if (val <= cm32181->als_it_values[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);
> >> @@ -265,11 +287,12 @@ static int cm32181_write_raw(struct iio_dev *indio_dev,
> >>   static ssize_t cm32181_get_it_available(struct device *dev,
> >>   			struct device_attribute *attr, char *buf)
> >>   {
> >> +	struct cm32181_chip *cm32181 = iio_priv(dev_to_iio_dev(dev));
> >>   	int i, n, len;
> >>   
> >> -	n = ARRAY_SIZE(als_it_value);
> >> +	n = cm32181->num_als_it;
> >>   	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_values[i]);
> >>   	return len + sprintf(buf + len, "\n");
> >>   }
> >>   
> >> @@ -345,6 +368,7 @@ static int cm32181_probe(struct i2c_client *client)
> >>   }
> >>   
> >>   static const struct of_device_id cm32181_of_match[] = {
> >> +	{ .compatible = "capella,cm3218" },
> >>   	{ .compatible = "capella,cm32181" },
> >>   	{ }
> >>   };  
> >   
> 

[PATCH v3 05/11] iio: light: cm32181: Clean up the probe function a bit

[Hans de Goede]

3 small cleanups to cm32181_probe():

1. Do not log an error when we fail to allocate memory (as a general
rule drivers do not log errors for this as the kernel will already
have complained loudly that it could not alloc the mem).

2. Remove the i2c_set_clientdata() call, we never use i2c_get_clientdata()
or dev_get_drvdata() anywhere.

3. Add a dev helper variable and use it in various places instead of
&client->dev.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v3:
- This is a new patch in v3 of this patch-set
---
 drivers/iio/light/cm32181.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
index 065bc7a11f84..8fe49610fc26 100644
--- a/drivers/iio/light/cm32181.c
+++ b/drivers/iio/light/cm32181.c
@@ -326,41 +326,35 @@ static const struct iio_info cm32181_info = {
 
 static int cm32181_probe(struct i2c_client *client)
 {
+	struct device *dev = &client->dev;
 	struct cm32181_chip *cm32181;
 	struct iio_dev *indio_dev;
 	int ret;
 
-	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*cm32181));
-	if (!indio_dev) {
-		dev_err(&client->dev, "devm_iio_device_alloc failed\n");
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*cm32181));
+	if (!indio_dev)
 		return -ENOMEM;
-	}
 
 	cm32181 = iio_priv(indio_dev);
-	i2c_set_clientdata(client, indio_dev);
 	cm32181->client = client;
 
 	mutex_init(&cm32181->lock);
-	indio_dev->dev.parent = &client->dev;
+	indio_dev->dev.parent = dev;
 	indio_dev->channels = cm32181_channels;
 	indio_dev->num_channels = ARRAY_SIZE(cm32181_channels);
 	indio_dev->info = &cm32181_info;
-	indio_dev->name = dev_name(&client->dev);
+	indio_dev->name = dev_name(dev);
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
 	ret = cm32181_reg_init(cm32181);
 	if (ret) {
-		dev_err(&client->dev,
-			"%s: register init failed\n",
-			__func__);
+		dev_err(dev, "%s: register init failed\n", __func__);
 		return ret;
 	}
 
-	ret = devm_iio_device_register(&client->dev, indio_dev);
+	ret = devm_iio_device_register(dev, indio_dev);
 	if (ret) {
-		dev_err(&client->dev,
-			"%s: regist device failed\n",
-			__func__);
+		dev_err(dev, "%s: regist device failed\n", __func__);
 		return ret;
 	}
 
-- 
2.26.0

Re: [PATCH v3 05/11] iio: light: cm32181: Clean up the probe function a bit

[Jonathan Cameron]

On Tue, 28 Apr 2020 19:29:17 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> 3 small cleanups to cm32181_probe():
> 
> 1. Do not log an error when we fail to allocate memory (as a general
> rule drivers do not log errors for this as the kernel will already
> have complained loudly that it could not alloc the mem).
> 
> 2. Remove the i2c_set_clientdata() call, we never use i2c_get_clientdata()
> or dev_get_drvdata() anywhere.
> 
> 3. Add a dev helper variable and use it in various places instead of
> &client->dev.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
looks fine to me.

J

> ---
> Changes in v3:
> - This is a new patch in v3 of this patch-set
> ---
>  drivers/iio/light/cm32181.c | 22 ++++++++--------------
>  1 file changed, 8 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
> index 065bc7a11f84..8fe49610fc26 100644
> --- a/drivers/iio/light/cm32181.c
> +++ b/drivers/iio/light/cm32181.c
> @@ -326,41 +326,35 @@ static const struct iio_info cm32181_info = {
>  
>  static int cm32181_probe(struct i2c_client *client)
>  {
> +	struct device *dev = &client->dev;
>  	struct cm32181_chip *cm32181;
>  	struct iio_dev *indio_dev;
>  	int ret;
>  
> -	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*cm32181));
> -	if (!indio_dev) {
> -		dev_err(&client->dev, "devm_iio_device_alloc failed\n");
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*cm32181));
> +	if (!indio_dev)
>  		return -ENOMEM;
> -	}
>  
>  	cm32181 = iio_priv(indio_dev);
> -	i2c_set_clientdata(client, indio_dev);
>  	cm32181->client = client;
>  
>  	mutex_init(&cm32181->lock);
> -	indio_dev->dev.parent = &client->dev;
> +	indio_dev->dev.parent = dev;
>  	indio_dev->channels = cm32181_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(cm32181_channels);
>  	indio_dev->info = &cm32181_info;
> -	indio_dev->name = dev_name(&client->dev);
> +	indio_dev->name = dev_name(dev);
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  
>  	ret = cm32181_reg_init(cm32181);
>  	if (ret) {
> -		dev_err(&client->dev,
> -			"%s: register init failed\n",
> -			__func__);
> +		dev_err(dev, "%s: register init failed\n", __func__);
>  		return ret;
>  	}
>  
> -	ret = devm_iio_device_register(&client->dev, indio_dev);
> +	ret = devm_iio_device_register(dev, indio_dev);
>  	if (ret) {
> -		dev_err(&client->dev,
> -			"%s: regist device failed\n",
> -			__func__);
> +		dev_err(dev, "%s: regist device failed\n", __func__);
>  		return ret;
>  	}
>  

[PATCH v3 06/11] iio: light: cm32181: Handle CM3218 ACPI devices with 2 I2C resources

[Hans de Goede]

Some ACPI systems list 2 I2C resources for the CM3218 sensor. On these
systems the first I2cSerialBus ACPI-resource points to the SMBus Alert
Response Address (ARA, 0x0c) and the second I2cSerialBus ACPI-resource
points to the actual CM3218 sensor address:

 Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
 {
     Name (SBUF, ResourceTemplate ()
     {
         I2cSerialBusV2 (0x000C, ControllerInitiated, 0x00061A80,
             AddressingMode7Bit, "\\_SB.I2C3",
             0x00, ResourceConsumer, , Exclusive,
             )
         I2cSerialBusV2 (0x0048, ControllerInitiated, 0x00061A80,
             AddressingMode7Bit, "\\_SB.I2C3",
             0x00, ResourceConsumer, , Exclusive,
             )
         Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive, ,, )
         {
             0x00000033,
         }
     })
     Return (SBUF) /* \_SB_.I2C3.ALSD._CRS.SBUF */
 }

Detect this and take the following step to deal with it:

1. When a SMBus Alert capable sensor has an Alert asserted, it will
   not respond on its actual I2C address. Read a byte from the ARA
   to clear any pending Alerts.

2. Create a "dummy" client for the actual I2C address and
   use that client to communicate with the sensor.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v3:
- Create and use a dummy client instead of relying on i2c-multi-instantiate
  to create 2 separate clients for the 2 I2C resources

Changes in v2
- s/i2c_client-s/I2C clients/ in added comment
---
 drivers/iio/light/cm32181.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
index 8fe49610fc26..c23a5c3a86a3 100644
--- a/drivers/iio/light/cm32181.c
+++ b/drivers/iio/light/cm32181.c
@@ -51,6 +51,8 @@
 #define CM32181_CALIBSCALE_RESOLUTION	1000
 #define MLUX_PER_LUX			1000
 
+#define SMBUS_ALERT_RESPONSE_ADDRESS	0x0c
+
 static const u8 cm32181_reg[CM32181_CONF_REG_NUM] = {
 	CM32181_REG_ADDR_CMD,
 };
@@ -335,6 +337,26 @@ static int cm32181_probe(struct i2c_client *client)
 	if (!indio_dev)
 		return -ENOMEM;
 
+	/*
+	 * Some ACPI systems list 2 I2C resources for the CM3218 sensor, the
+	 * SMBus Alert Response Address (ARA, 0x0c) and the actual I2C address.
+	 * Detect this and take the following step to deal with it:
+	 * 1. When a SMBus Alert capable sensor has an Alert asserted, it will
+	 *    not respond on its actual I2C address. Read a byte from the ARA
+	 *    to clear any pending Alerts.
+	 * 2. Create a "dummy" client for the actual I2C address and
+	 *    use that client to communicate with the sensor.
+	 */
+	if (ACPI_HANDLE(dev) && client->addr == SMBUS_ALERT_RESPONSE_ADDRESS) {
+		struct i2c_board_info board_info = { .type = "dummy" };
+
+		i2c_smbus_read_byte(client);
+
+		client = i2c_acpi_new_device(dev, 1, &board_info);
+		if (IS_ERR(client))
+			return PTR_ERR(client);
+	}
+
 	cm32181 = iio_priv(indio_dev);
 	cm32181->client = client;
 
-- 
2.26.0

Re: [PATCH v3 06/11] iio: light: cm32181: Handle CM3218 ACPI devices with 2 I2C resources

[Jonathan Cameron]

On Tue, 28 Apr 2020 19:29:18 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> Some ACPI systems list 2 I2C resources for the CM3218 sensor. On these
> systems the first I2cSerialBus ACPI-resource points to the SMBus Alert
> Response Address (ARA, 0x0c) and the second I2cSerialBus ACPI-resource
> points to the actual CM3218 sensor address:
> 
>  Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
>  {
>      Name (SBUF, ResourceTemplate ()
>      {

I have a vague recollection that we had case of this where they could
come in either order.  Could that happen here?

My mind may be playing tricks on me of course and that may never
happen...

Did I ever mention how much the lack of spec for some of these corner
cases annoys me?

J

>          I2cSerialBusV2 (0x000C, ControllerInitiated, 0x00061A80,
>              AddressingMode7Bit, "\\_SB.I2C3",
>              0x00, ResourceConsumer, , Exclusive,
>              )
>          I2cSerialBusV2 (0x0048, ControllerInitiated, 0x00061A80,
>              AddressingMode7Bit, "\\_SB.I2C3",
>              0x00, ResourceConsumer, , Exclusive,
>              )
>          Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive, ,, )
>          {
>              0x00000033,
>          }
>      })
>      Return (SBUF) /* \_SB_.I2C3.ALSD._CRS.SBUF */
>  }
> 
> Detect this and take the following step to deal with it:
> 
> 1. When a SMBus Alert capable sensor has an Alert asserted, it will
>    not respond on its actual I2C address. Read a byte from the ARA
>    to clear any pending Alerts.
> 
> 2. Create a "dummy" client for the actual I2C address and
>    use that client to communicate with the sensor.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v3:
> - Create and use a dummy client instead of relying on i2c-multi-instantiate
>   to create 2 separate clients for the 2 I2C resources
> 
> Changes in v2
> - s/i2c_client-s/I2C clients/ in added comment
> ---
>  drivers/iio/light/cm32181.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
> index 8fe49610fc26..c23a5c3a86a3 100644
> --- a/drivers/iio/light/cm32181.c
> +++ b/drivers/iio/light/cm32181.c
> @@ -51,6 +51,8 @@
>  #define CM32181_CALIBSCALE_RESOLUTION	1000
>  #define MLUX_PER_LUX			1000
>  
> +#define SMBUS_ALERT_RESPONSE_ADDRESS	0x0c
> +
>  static const u8 cm32181_reg[CM32181_CONF_REG_NUM] = {
>  	CM32181_REG_ADDR_CMD,
>  };
> @@ -335,6 +337,26 @@ static int cm32181_probe(struct i2c_client *client)
>  	if (!indio_dev)
>  		return -ENOMEM;
>  
> +	/*
> +	 * Some ACPI systems list 2 I2C resources for the CM3218 sensor, the
> +	 * SMBus Alert Response Address (ARA, 0x0c) and the actual I2C address.
> +	 * Detect this and take the following step to deal with it:
> +	 * 1. When a SMBus Alert capable sensor has an Alert asserted, it will
> +	 *    not respond on its actual I2C address. Read a byte from the ARA
> +	 *    to clear any pending Alerts.
> +	 * 2. Create a "dummy" client for the actual I2C address and
> +	 *    use that client to communicate with the sensor.
> +	 */
> +	if (ACPI_HANDLE(dev) && client->addr == SMBUS_ALERT_RESPONSE_ADDRESS) {
> +		struct i2c_board_info board_info = { .type = "dummy" };
> +
> +		i2c_smbus_read_byte(client);
> +
> +		client = i2c_acpi_new_device(dev, 1, &board_info);
> +		if (IS_ERR(client))
> +			return PTR_ERR(client);
> +	}
> +
>  	cm32181 = iio_priv(indio_dev);
>  	cm32181->client = client;
>  

Re: [PATCH v3 06/11] iio: light: cm32181: Handle CM3218 ACPI devices with 2 I2C resources

[Hans de Goede]

Hi,

On 5/3/20 1:04 PM, Jonathan Cameron wrote:
> On Tue, 28 Apr 2020 19:29:18 +0200
> Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> Some ACPI systems list 2 I2C resources for the CM3218 sensor. On these
>> systems the first I2cSerialBus ACPI-resource points to the SMBus Alert
>> Response Address (ARA, 0x0c) and the second I2cSerialBus ACPI-resource
>> points to the actual CM3218 sensor address:
>>
>>   Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
>>   {
>>       Name (SBUF, ResourceTemplate ()
>>       {
> 
> I have a vague recollection that we had case of this where they could
> come in either order.  Could that happen here?

Not to the best of my knowledge. I've only seen this
construct on a couple of Asus models and they are all consistent.

> My mind may be playing tricks on me of course and that may never
> happen...
> 
> Did I ever mention how much the lack of spec for some of these corner
> cases annoys me?

Yes I think you have mentioned before. Unfortunately there
is nothing we can do about this, but I hear you.

Regards,

Hans


> 
> J
> 
>>           I2cSerialBusV2 (0x000C, ControllerInitiated, 0x00061A80,
>>               AddressingMode7Bit, "\\_SB.I2C3",
>>               0x00, ResourceConsumer, , Exclusive,
>>               )
>>           I2cSerialBusV2 (0x0048, ControllerInitiated, 0x00061A80,
>>               AddressingMode7Bit, "\\_SB.I2C3",
>>               0x00, ResourceConsumer, , Exclusive,
>>               )
>>           Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive, ,, )
>>           {
>>               0x00000033,
>>           }
>>       })
>>       Return (SBUF) /* \_SB_.I2C3.ALSD._CRS.SBUF */
>>   }
>>
>> Detect this and take the following step to deal with it:
>>
>> 1. When a SMBus Alert capable sensor has an Alert asserted, it will
>>     not respond on its actual I2C address. Read a byte from the ARA
>>     to clear any pending Alerts.
>>
>> 2. Create a "dummy" client for the actual I2C address and
>>     use that client to communicate with the sensor.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v3:
>> - Create and use a dummy client instead of relying on i2c-multi-instantiate
>>    to create 2 separate clients for the 2 I2C resources
>>
>> Changes in v2
>> - s/i2c_client-s/I2C clients/ in added comment
>> ---
>>   drivers/iio/light/cm32181.c | 22 ++++++++++++++++++++++
>>   1 file changed, 22 insertions(+)
>>
>> diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
>> index 8fe49610fc26..c23a5c3a86a3 100644
>> --- a/drivers/iio/light/cm32181.c
>> +++ b/drivers/iio/light/cm32181.c
>> @@ -51,6 +51,8 @@
>>   #define CM32181_CALIBSCALE_RESOLUTION	1000
>>   #define MLUX_PER_LUX			1000
>>   
>> +#define SMBUS_ALERT_RESPONSE_ADDRESS	0x0c
>> +
>>   static const u8 cm32181_reg[CM32181_CONF_REG_NUM] = {
>>   	CM32181_REG_ADDR_CMD,
>>   };
>> @@ -335,6 +337,26 @@ static int cm32181_probe(struct i2c_client *client)
>>   	if (!indio_dev)
>>   		return -ENOMEM;
>>   
>> +	/*
>> +	 * Some ACPI systems list 2 I2C resources for the CM3218 sensor, the
>> +	 * SMBus Alert Response Address (ARA, 0x0c) and the actual I2C address.
>> +	 * Detect this and take the following step to deal with it:
>> +	 * 1. When a SMBus Alert capable sensor has an Alert asserted, it will
>> +	 *    not respond on its actual I2C address. Read a byte from the ARA
>> +	 *    to clear any pending Alerts.
>> +	 * 2. Create a "dummy" client for the actual I2C address and
>> +	 *    use that client to communicate with the sensor.
>> +	 */
>> +	if (ACPI_HANDLE(dev) && client->addr == SMBUS_ALERT_RESPONSE_ADDRESS) {
>> +		struct i2c_board_info board_info = { .type = "dummy" };
>> +
>> +		i2c_smbus_read_byte(client);
>> +
>> +		client = i2c_acpi_new_device(dev, 1, &board_info);
>> +		if (IS_ERR(client))
>> +			return PTR_ERR(client);
>> +	}
>> +
>>   	cm32181 = iio_priv(indio_dev);
>>   	cm32181->client = client;
>>   
> 

[PATCH v3 07/11] iio: light: cm32181: Change reg_init to use a bitmap of which registers to init

[Hans de Goede]

This is a preparation patch for reading some ACPI tables which give
init values for multiple registers.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Use unsigned long for init_regs_bitmap
- Use for_each_set_bit()
---
 drivers/iio/light/cm32181.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
index c23a5c3a86a3..bcea669d20cb 100644
--- a/drivers/iio/light/cm32181.c
+++ b/drivers/iio/light/cm32181.c
@@ -26,7 +26,7 @@
 #define CM32181_REG_ADDR_ID		0x07
 
 /* Number of Configurable Registers */
-#define CM32181_CONF_REG_NUM		0x01
+#define CM32181_CONF_REG_NUM		4
 
 /* CMD register */
 #define CM32181_CMD_ALS_DISABLE		BIT(0)
@@ -53,10 +53,6 @@
 
 #define SMBUS_ALERT_RESPONSE_ADDRESS	0x0c
 
-static const u8 cm32181_reg[CM32181_CONF_REG_NUM] = {
-	CM32181_REG_ADDR_CMD,
-};
-
 /* CM3218 Family */
 static const int cm3218_als_it_bits[] = { 0, 1, 2, 3 };
 static const int cm3218_als_it_values[] = { 100000, 200000, 400000, 800000 };
@@ -71,6 +67,7 @@ struct cm32181_chip {
 	struct i2c_client *client;
 	struct mutex lock;
 	u16 conf_regs[CM32181_CONF_REG_NUM];
+	unsigned long init_regs_bitmap;
 	int calibscale;
 	int num_als_it;
 	const int *als_it_bits;
@@ -115,12 +112,13 @@ static int cm32181_reg_init(struct cm32181_chip *cm32181)
 	/* Default Values */
 	cm32181->conf_regs[CM32181_REG_ADDR_CMD] =
 			CM32181_CMD_ALS_IT_DEFAULT | CM32181_CMD_ALS_SM_DEFAULT;
+	cm32181->init_regs_bitmap = BIT(CM32181_REG_ADDR_CMD);
 	cm32181->calibscale = CM32181_CALIBSCALE_DEFAULT;
 
 	/* Initialize registers*/
-	for (i = 0; i < CM32181_CONF_REG_NUM; i++) {
-		ret = i2c_smbus_write_word_data(client, cm32181_reg[i],
-			cm32181->conf_regs[i]);
+	for_each_set_bit(i, &cm32181->init_regs_bitmap, CM32181_CONF_REG_NUM) {
+		ret = i2c_smbus_write_word_data(client, i,
+						cm32181->conf_regs[i]);
 		if (ret < 0)
 			return ret;
 	}
-- 
2.26.0

[PATCH v3 08/11] iio: light: cm32181: Use units of 1/100000th for calibscale and lux_per_bit

[Hans de Goede]

Use units of 1/100000th for calibscale and lux_per_bit. The similar
cm3232 driver already uses 1/100000th as unit for calibscale.

This allows for higher-accuracy and makes it easier to add support
for getting device-specific calibscale and lux_per_bit values from
a device's ACPI tables, as the values in the ACPI tables also use
1/100000th units.

This units change means that our intermediate values in cm32181_get_lux()
may get quite big, change the type of the lux variable to a u64 to
deal with this.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/iio/light/cm32181.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
index bcea669d20cb..1ad7d4582d1c 100644
--- a/drivers/iio/light/cm32181.c
+++ b/drivers/iio/light/cm32181.c
@@ -45,11 +45,11 @@
 #define CM32181_CMD_ALS_SM_MASK		(0x03 << CM32181_CMD_ALS_SM_SHIFT)
 #define CM32181_CMD_ALS_SM_DEFAULT	(0x01 << CM32181_CMD_ALS_SM_SHIFT)
 
-#define CM32181_MLUX_PER_BIT		5	/* ALS_SM=01 IT=800ms */
-#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_LUX_PER_BIT		500	/* ALS_SM=01 IT=800ms */
+#define CM32181_LUX_PER_BIT_RESOLUTION	100000
+#define CM32181_LUX_PER_BIT_BASE_IT	800000	/* Based on IT=800ms */
+#define CM32181_CALIBSCALE_DEFAULT	100000
+#define CM32181_CALIBSCALE_RESOLUTION	100000
 
 #define SMBUS_ALERT_RESPONSE_ADDRESS	0x0c
 
@@ -204,15 +204,15 @@ static int cm32181_get_lux(struct cm32181_chip *cm32181)
 	struct i2c_client *client = cm32181->client;
 	int ret;
 	int als_it;
-	unsigned long lux;
+	u64 lux;
 
 	ret = cm32181_read_als_it(cm32181, &als_it);
 	if (ret < 0)
 		return -EINVAL;
 
-	lux = CM32181_MLUX_PER_BIT;
-	lux *= CM32181_MLUX_PER_BIT_BASE_IT;
-	lux /= als_it;
+	lux = CM32181_LUX_PER_BIT;
+	lux *= CM32181_LUX_PER_BIT_BASE_IT;
+	lux = div_u64(lux, als_it);
 
 	ret = i2c_smbus_read_word_data(client, CM32181_REG_ADDR_ALS);
 	if (ret < 0)
@@ -220,8 +220,8 @@ static int cm32181_get_lux(struct cm32181_chip *cm32181)
 
 	lux *= ret;
 	lux *= cm32181->calibscale;
-	lux /= CM32181_CALIBSCALE_RESOLUTION;
-	lux /= MLUX_PER_LUX;
+	lux = div_u64(lux, CM32181_CALIBSCALE_RESOLUTION);
+	lux = div_u64(lux, CM32181_LUX_PER_BIT_RESOLUTION);
 
 	if (lux > 0xFFFF)
 		lux = 0xFFFF;
-- 
2.26.0

[PATCH v3 09/11] iio: light: cm32181: Make lux_per_bit and lux_per_bit_base_it runtime settings

[Hans de Goede]

Make lux_per_bit and lux_per_bit_base_it settings stored in struct
cm32181_chip instead of a hardcoded (defined) values.

This is a preparation patch for reading some ACPI tables which specify
a device specific lux_per_bit value.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/iio/light/cm32181.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
index 1ad7d4582d1c..cae2264e4336 100644
--- a/drivers/iio/light/cm32181.c
+++ b/drivers/iio/light/cm32181.c
@@ -69,6 +69,8 @@ struct cm32181_chip {
 	u16 conf_regs[CM32181_CONF_REG_NUM];
 	unsigned long init_regs_bitmap;
 	int calibscale;
+	int lux_per_bit;
+	int lux_per_bit_base_it;
 	int num_als_it;
 	const int *als_it_bits;
 	const int *als_it_values;
@@ -114,6 +116,8 @@ static int cm32181_reg_init(struct cm32181_chip *cm32181)
 			CM32181_CMD_ALS_IT_DEFAULT | CM32181_CMD_ALS_SM_DEFAULT;
 	cm32181->init_regs_bitmap = BIT(CM32181_REG_ADDR_CMD);
 	cm32181->calibscale = CM32181_CALIBSCALE_DEFAULT;
+	cm32181->lux_per_bit = CM32181_LUX_PER_BIT;
+	cm32181->lux_per_bit_base_it = CM32181_LUX_PER_BIT_BASE_IT;
 
 	/* Initialize registers*/
 	for_each_set_bit(i, &cm32181->init_regs_bitmap, CM32181_CONF_REG_NUM) {
@@ -210,8 +214,8 @@ static int cm32181_get_lux(struct cm32181_chip *cm32181)
 	if (ret < 0)
 		return -EINVAL;
 
-	lux = CM32181_LUX_PER_BIT;
-	lux *= CM32181_LUX_PER_BIT_BASE_IT;
+	lux = cm32181->lux_per_bit;
+	lux *= cm32181->lux_per_bit_base_it;
 	lux = div_u64(lux, als_it);
 
 	ret = i2c_smbus_read_word_data(client, CM32181_REG_ADDR_ALS);
-- 
2.26.0

[PATCH v3 10/11] iio: light: cm32181: Add support for parsing CPM0 and CPM1 ACPI tables

[Hans de Goede]

On ACPI based systems the CPLM3218 ACPI device node describing the
CM3218[1] sensor typically will have some extra tables with register
init values for initializing the sensor and calibration info.

This is based on a newer version of cm32181.c, with a copyright of:

 * Copyright (C) 2014 Capella Microsystems Inc.
 * Author: Kevin Tsai <ktsai@capellamicro.com>
 *
 * This program is free software; you can redistribute it and/or modify it
 * under the terms of the GNU General Public License version 2, as published
 * by the Free Software Foundation.

Which is floating around on the net in various places, but the changes
from this newer version never made it upstream.

This was tested on the following models: Acer Switch 10 SW5-012 (CM32181)
Asus T100TA (CM3218), Asus T100CHI (CM3218) and HP X2 10-n000nd (CM32181).

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Factor out the parsing into a separate helper function
---
 drivers/iio/light/cm32181.c | 101 ++++++++++++++++++++++++++++++++++++
 1 file changed, 101 insertions(+)

diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
index cae2264e4336..d8b128286527 100644
--- a/drivers/iio/light/cm32181.c
+++ b/drivers/iio/light/cm32181.c
@@ -4,6 +4,7 @@
  * Author: Kevin Tsai <ktsai@capellamicro.com>
  */
 
+#include <linux/acpi.h>
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/i2c.h>
@@ -53,6 +54,15 @@
 
 #define SMBUS_ALERT_RESPONSE_ADDRESS	0x0c
 
+/* CPM0 Index 0: device-id (3218 or 32181), 1: Unknown, 2: init_regs_bitmap */
+#define CPM0_REGS_BITMAP		2
+#define CPM0_HEADER_SIZE		3
+
+/* CPM1 Index 0: lux_per_bit, 1: calibscale, 2: resolution (100000) */
+#define CPM1_LUX_PER_BIT		0
+#define CPM1_CALIBSCALE			1
+#define CPM1_SIZE			3
+
 /* CM3218 Family */
 static const int cm3218_als_it_bits[] = { 0, 1, 2, 3 };
 static const int cm3218_als_it_values[] = { 100000, 200000, 400000, 800000 };
@@ -65,6 +75,7 @@ static const int cm32181_als_it_values[] = {
 
 struct cm32181_chip {
 	struct i2c_client *client;
+	struct device *dev;
 	struct mutex lock;
 	u16 conf_regs[CM32181_CONF_REG_NUM];
 	unsigned long init_regs_bitmap;
@@ -76,6 +87,92 @@ struct cm32181_chip {
 	const int *als_it_values;
 };
 
+static int cm32181_read_als_it(struct cm32181_chip *cm32181, int *val2);
+
+#ifdef CONFIG_ACPI
+/**
+ * cm32181_acpi_get_cpm() - Get CPM object from ACPI
+ * @client	pointer of struct i2c_client.
+ * @obj_name	pointer of ACPI object name.
+ * @count	maximum size of return array.
+ * @vals	pointer of array for return elements.
+ *
+ * Convert ACPI CPM table to array.
+ *
+ * Return: -ENODEV for fail.  Otherwise is number of elements.
+ */
+static int cm32181_acpi_get_cpm(struct device *dev, char *obj_name,
+				u64 *values, int count)
+{
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+	union acpi_object *cpm, *elem;
+	acpi_handle handle;
+	acpi_status status;
+	int i;
+
+	handle = ACPI_HANDLE(dev);
+	if (!handle)
+		return -ENODEV;
+
+	status = acpi_evaluate_object(handle, obj_name, NULL, &buffer);
+	if (ACPI_FAILURE(status)) {
+		dev_err(dev, "object %s not found\n", obj_name);
+		return -ENODEV;
+	}
+
+	cpm = buffer.pointer;
+	if (cpm->package.count > count)
+		dev_warn(dev, "%s table contains %d values, only using first %d values\n",
+			 obj_name, cpm->package.count, count);
+
+	count = min_t(int, cpm->package.count, count);
+	for (i = 0; i < count; i++) {
+		elem = &(cpm->package.elements[i]);
+		values[i] = elem->integer.value;
+	}
+
+	kfree(buffer.pointer);
+
+	return count;
+}
+
+static void cm32181_acpi_parse_cpm_tables(struct cm32181_chip *cm32181)
+{
+	u64 vals[CPM0_HEADER_SIZE + CM32181_CONF_REG_NUM];
+	struct device *dev = cm32181->dev;
+	int i, count;
+
+	count = cm32181_acpi_get_cpm(dev, "CPM0", vals, ARRAY_SIZE(vals));
+	if (count <= CPM0_HEADER_SIZE)
+		return;
+
+	count -= CPM0_HEADER_SIZE;
+
+	cm32181->init_regs_bitmap = vals[CPM0_REGS_BITMAP];
+	cm32181->init_regs_bitmap &= GENMASK(count - 1, 0);
+	for_each_set_bit(i, &cm32181->init_regs_bitmap, count)
+		cm32181->conf_regs[i] =	vals[CPM0_HEADER_SIZE + i];
+
+	count = cm32181_acpi_get_cpm(dev, "CPM1", vals, ARRAY_SIZE(vals));
+	if (count != CPM1_SIZE)
+		return;
+
+	cm32181->lux_per_bit = vals[CPM1_LUX_PER_BIT];
+
+	/* Check for uncalibrated devices */
+	if (vals[CPM1_CALIBSCALE] == CM32181_CALIBSCALE_DEFAULT)
+		return;
+
+	cm32181->calibscale = vals[CPM1_CALIBSCALE];
+	/* CPM1 lux_per_bit is for the current it value */
+	cm32181_read_als_it(cm32181, &cm32181->lux_per_bit_base_it);
+}
+#else
+static void cm32181_acpi_parse_cpm_tables(struct cm32181_chip *cm32181)
+{
+}
+#endif /* CONFIG_ACPI */
+
 /**
  * cm32181_reg_init() - Initialize CM32181 registers
  * @cm32181:	pointer of struct cm32181.
@@ -119,6 +216,9 @@ static int cm32181_reg_init(struct cm32181_chip *cm32181)
 	cm32181->lux_per_bit = CM32181_LUX_PER_BIT;
 	cm32181->lux_per_bit_base_it = CM32181_LUX_PER_BIT_BASE_IT;
 
+	if (ACPI_HANDLE(cm32181->dev))
+		cm32181_acpi_parse_cpm_tables(cm32181);
+
 	/* Initialize registers*/
 	for_each_set_bit(i, &cm32181->init_regs_bitmap, CM32181_CONF_REG_NUM) {
 		ret = i2c_smbus_write_word_data(client, i,
@@ -361,6 +461,7 @@ static int cm32181_probe(struct i2c_client *client)
 
 	cm32181 = iio_priv(indio_dev);
 	cm32181->client = client;
+	cm32181->dev = dev;
 
 	mutex_init(&cm32181->lock);
 	indio_dev->dev.parent = dev;
-- 
2.26.0

Re: [PATCH v3 10/11] iio: light: cm32181: Add support for parsing CPM0 and CPM1 ACPI tables

[Jonathan Cameron]

On Tue, 28 Apr 2020 19:29:22 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> On ACPI based systems the CPLM3218 ACPI device node describing the
> CM3218[1] sensor typically will have some extra tables with register
> init values for initializing the sensor and calibration info.
> 
> This is based on a newer version of cm32181.c, with a copyright of:
> 
>  * Copyright (C) 2014 Capella Microsystems Inc.
>  * Author: Kevin Tsai <ktsai@capellamicro.com>
>  *
>  * This program is free software; you can redistribute it and/or modify it
>  * under the terms of the GNU General Public License version 2, as published
>  * by the Free Software Foundation.
> 
> Which is floating around on the net in various places, but the changes
> from this newer version never made it upstream.
> 
> This was tested on the following models: Acer Switch 10 SW5-012 (CM32181)
> Asus T100TA (CM3218), Asus T100CHI (CM3218) and HP X2 10-n000nd (CM32181).

I assume it's far too much to hope this CPM0 / CPM1 stuff is actually defined
in a spec anywhere?  

There are standard way of adding vendor specific data blobs to ACPI and this
isn't one of them (unless I'm missing something).  People need to beat
up vendors earlier about this stuff.

Grumble over...

Code looks fine to me, but I'd like an ACPI review ideally.

Thanks,

Jonathan

> 
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> - Factor out the parsing into a separate helper function
> ---
>  drivers/iio/light/cm32181.c | 101 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 101 insertions(+)
> 
> diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
> index cae2264e4336..d8b128286527 100644
> --- a/drivers/iio/light/cm32181.c
> +++ b/drivers/iio/light/cm32181.c
> @@ -4,6 +4,7 @@
>   * Author: Kevin Tsai <ktsai@capellamicro.com>
>   */
>  
> +#include <linux/acpi.h>
>  #include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/i2c.h>
> @@ -53,6 +54,15 @@
>  
>  #define SMBUS_ALERT_RESPONSE_ADDRESS	0x0c
>  
> +/* CPM0 Index 0: device-id (3218 or 32181), 1: Unknown, 2: init_regs_bitmap */
> +#define CPM0_REGS_BITMAP		2
> +#define CPM0_HEADER_SIZE		3
> +
> +/* CPM1 Index 0: lux_per_bit, 1: calibscale, 2: resolution (100000) */
> +#define CPM1_LUX_PER_BIT		0
> +#define CPM1_CALIBSCALE			1
> +#define CPM1_SIZE			3
> +
>  /* CM3218 Family */
>  static const int cm3218_als_it_bits[] = { 0, 1, 2, 3 };
>  static const int cm3218_als_it_values[] = { 100000, 200000, 400000, 800000 };
> @@ -65,6 +75,7 @@ static const int cm32181_als_it_values[] = {
>  
>  struct cm32181_chip {
>  	struct i2c_client *client;
> +	struct device *dev;
>  	struct mutex lock;
>  	u16 conf_regs[CM32181_CONF_REG_NUM];
>  	unsigned long init_regs_bitmap;
> @@ -76,6 +87,92 @@ struct cm32181_chip {
>  	const int *als_it_values;
>  };
>  
> +static int cm32181_read_als_it(struct cm32181_chip *cm32181, int *val2);
> +
> +#ifdef CONFIG_ACPI
> +/**
> + * cm32181_acpi_get_cpm() - Get CPM object from ACPI
> + * @client	pointer of struct i2c_client.
> + * @obj_name	pointer of ACPI object name.
> + * @count	maximum size of return array.
> + * @vals	pointer of array for return elements.
> + *
> + * Convert ACPI CPM table to array.
> + *
> + * Return: -ENODEV for fail.  Otherwise is number of elements.
> + */
> +static int cm32181_acpi_get_cpm(struct device *dev, char *obj_name,
> +				u64 *values, int count)
> +{
> +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> +	union acpi_object *cpm, *elem;
> +	acpi_handle handle;
> +	acpi_status status;
> +	int i;
> +
> +	handle = ACPI_HANDLE(dev);
> +	if (!handle)
> +		return -ENODEV;
> +
> +	status = acpi_evaluate_object(handle, obj_name, NULL, &buffer);
> +	if (ACPI_FAILURE(status)) {
> +		dev_err(dev, "object %s not found\n", obj_name);
> +		return -ENODEV;
> +	}
> +
> +	cpm = buffer.pointer;
> +	if (cpm->package.count > count)
> +		dev_warn(dev, "%s table contains %d values, only using first %d values\n",
> +			 obj_name, cpm->package.count, count);
> +
> +	count = min_t(int, cpm->package.count, count);
> +	for (i = 0; i < count; i++) {
> +		elem = &(cpm->package.elements[i]);
> +		values[i] = elem->integer.value;
> +	}
> +
> +	kfree(buffer.pointer);
> +
> +	return count;
> +}
> +
> +static void cm32181_acpi_parse_cpm_tables(struct cm32181_chip *cm32181)
> +{
> +	u64 vals[CPM0_HEADER_SIZE + CM32181_CONF_REG_NUM];
> +	struct device *dev = cm32181->dev;
> +	int i, count;
> +
> +	count = cm32181_acpi_get_cpm(dev, "CPM0", vals, ARRAY_SIZE(vals));
> +	if (count <= CPM0_HEADER_SIZE)
> +		return;
> +
> +	count -= CPM0_HEADER_SIZE;
> +
> +	cm32181->init_regs_bitmap = vals[CPM0_REGS_BITMAP];
> +	cm32181->init_regs_bitmap &= GENMASK(count - 1, 0);
> +	for_each_set_bit(i, &cm32181->init_regs_bitmap, count)
> +		cm32181->conf_regs[i] =	vals[CPM0_HEADER_SIZE + i];
> +
> +	count = cm32181_acpi_get_cpm(dev, "CPM1", vals, ARRAY_SIZE(vals));
> +	if (count != CPM1_SIZE)
> +		return;
> +
> +	cm32181->lux_per_bit = vals[CPM1_LUX_PER_BIT];
> +
> +	/* Check for uncalibrated devices */
> +	if (vals[CPM1_CALIBSCALE] == CM32181_CALIBSCALE_DEFAULT)
> +		return;
> +
> +	cm32181->calibscale = vals[CPM1_CALIBSCALE];
> +	/* CPM1 lux_per_bit is for the current it value */
> +	cm32181_read_als_it(cm32181, &cm32181->lux_per_bit_base_it);
> +}
> +#else
> +static void cm32181_acpi_parse_cpm_tables(struct cm32181_chip *cm32181)
> +{
> +}
> +#endif /* CONFIG_ACPI */
> +
>  /**
>   * cm32181_reg_init() - Initialize CM32181 registers
>   * @cm32181:	pointer of struct cm32181.
> @@ -119,6 +216,9 @@ static int cm32181_reg_init(struct cm32181_chip *cm32181)
>  	cm32181->lux_per_bit = CM32181_LUX_PER_BIT;
>  	cm32181->lux_per_bit_base_it = CM32181_LUX_PER_BIT_BASE_IT;
>  
> +	if (ACPI_HANDLE(cm32181->dev))
> +		cm32181_acpi_parse_cpm_tables(cm32181);
> +
>  	/* Initialize registers*/
>  	for_each_set_bit(i, &cm32181->init_regs_bitmap, CM32181_CONF_REG_NUM) {
>  		ret = i2c_smbus_write_word_data(client, i,
> @@ -361,6 +461,7 @@ static int cm32181_probe(struct i2c_client *client)
>  
>  	cm32181 = iio_priv(indio_dev);
>  	cm32181->client = client;
> +	cm32181->dev = dev;
>  
>  	mutex_init(&cm32181->lock);
>  	indio_dev->dev.parent = dev;

Re: [PATCH v3 10/11] iio: light: cm32181: Add support for parsing CPM0 and CPM1 ACPI tables

[Andy Shevchenko]

On Sun, May 3, 2020 at 2:22 PM Jonathan Cameron <jic23@kernel.org> wrote:
> On Tue, 28 Apr 2020 19:29:22 +0200
> Hans de Goede <hdegoede@redhat.com> wrote:

...

> > This was tested on the following models: Acer Switch 10 SW5-012 (CM32181)
> > Asus T100TA (CM3218), Asus T100CHI (CM3218) and HP X2 10-n000nd (CM32181).
>
> I assume it's far too much to hope this CPM0 / CPM1 stuff is actually defined
> in a spec anywhere?
>
> There are standard way of adding vendor specific data blobs to ACPI and this
> isn't one of them (unless I'm missing something).  People need to beat
> up vendors earlier about this stuff.
>
> Grumble over...
>
> Code looks fine to me, but I'd like an ACPI review ideally.

ACPI didn't cover embedded world and has the following issues
a) where it should be strict (like how many I2CSerialBus() resources
can be given and for what type of devices, etc), it doesn't
b) they need to provides better validation tools, but they didn't
c) it's still windows oriented :-(

Above is custom extension on how to add device properties (and note,
we have now _DSD() and still we have some M$ way of thinking how to
use them).

Since the above approach is in the wild, I'm afraid we have not many
possibilities here (each of them with own problems):
1/ shout at vendors to use ACPI properly and simple don't by broken
hardware (rather firmware)
2/ try to support custom changes (may lead to several approaches for
the same thing)
3/ create a lot of board files (something in between 1/ and 2/)

As a result:
1/ is obviously a best one, but I think it's an utopia.
2/ in practice we don't have many deviations (luckily OEMs are quite
lazy to modify reference BIOSes and often reuse existing approaches)
3/ may not work, because on cheap laptops the means of distinguishing
them (like DMI strings) may also been broken.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 10/11] iio: light: cm32181: Add support for parsing CPM0 and CPM1 ACPI tables

[Jonathan Cameron]

On Sun, 3 May 2020 19:25:20 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sun, May 3, 2020 at 2:22 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > On Tue, 28 Apr 2020 19:29:22 +0200
> > Hans de Goede <hdegoede@redhat.com> wrote:  
> 
> ...
> 
> > > This was tested on the following models: Acer Switch 10 SW5-012 (CM32181)
> > > Asus T100TA (CM3218), Asus T100CHI (CM3218) and HP X2 10-n000nd (CM32181).  
> >
> > I assume it's far too much to hope this CPM0 / CPM1 stuff is actually defined
> > in a spec anywhere?
> >
> > There are standard way of adding vendor specific data blobs to ACPI and this
> > isn't one of them (unless I'm missing something).  People need to beat
> > up vendors earlier about this stuff.
> >
> > Grumble over...
> >
> > Code looks fine to me, but I'd like an ACPI review ideally.  
> 
> ACPI didn't cover embedded world and has the following issues
> a) where it should be strict (like how many I2CSerialBus() resources
> can be given and for what type of devices, etc), it doesn't
> b) they need to provides better validation tools, but they didn't
> c) it's still windows oriented :-(
> 
> Above is custom extension on how to add device properties (and note,
> we have now _DSD() and still we have some M$ way of thinking how to
> use them).
> 
> Since the above approach is in the wild, I'm afraid we have not many
> possibilities here (each of them with own problems):
> 1/ shout at vendors to use ACPI properly and simple don't by broken
> hardware (rather firmware)
> 2/ try to support custom changes (may lead to several approaches for
> the same thing)
> 3/ create a lot of board files (something in between 1/ and 2/)
> 
> As a result:
> 1/ is obviously a best one, but I think it's an utopia.

Let's keep the "shout" bit where possible :)  Makes us feel better anyway.

> 2/ in practice we don't have many deviations (luckily OEMs are quite
> lazy to modify reference BIOSes and often reuse existing approaches)
> 3/ may not work, because on cheap laptops the means of distinguishing
> them (like DMI strings) may also been broken.
> 

The UEFI forum are finally making steps in the right direction on
how they develop their specs (sort of) so I guess interested companies
should rock up and see if they can get some of this stuff fixed.
(those that can attend meetings anyway - but that's a different issue).

Spec meetings are fun and everyone loves the EDK2 source code :)

J

Re: [PATCH v3 10/11] iio: light: cm32181: Add support for parsing CPM0 and CPM1 ACPI tables

[Hans de Goede]

Hi,

On 5/3/20 1:22 PM, Jonathan Cameron wrote:
> On Tue, 28 Apr 2020 19:29:22 +0200
> Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> On ACPI based systems the CPLM3218 ACPI device node describing the
>> CM3218[1] sensor typically will have some extra tables with register
>> init values for initializing the sensor and calibration info.
>>
>> This is based on a newer version of cm32181.c, with a copyright of:
>>
>>   * Copyright (C) 2014 Capella Microsystems Inc.
>>   * Author: Kevin Tsai <ktsai@capellamicro.com>
>>   *
>>   * This program is free software; you can redistribute it and/or modify it
>>   * under the terms of the GNU General Public License version 2, as published
>>   * by the Free Software Foundation.
>>
>> Which is floating around on the net in various places, but the changes
>> from this newer version never made it upstream.
>>
>> This was tested on the following models: Acer Switch 10 SW5-012 (CM32181)
>> Asus T100TA (CM3218), Asus T100CHI (CM3218) and HP X2 10-n000nd (CM32181).
> 
> I assume it's far too much to hope this CPM0 / CPM1 stuff is actually defined
> in a spec anywhere?
> 
> There are standard way of adding vendor specific data blobs to ACPI and this
> isn't one of them (unless I'm missing something).  People need to beat
> up vendors earlier about this stuff.

To be fair, most devices with these sensors are quite old, the T100TA
was released in 2013. I have adding support for these on my todo list
for quite some time now (years really), but I never got around to it until
now.

Surprisingly enough, the recent released Chuwi Hi 13 device is also using
a CM32181 sensor though, so even though these are old they are still
being used.

Anyways what I'm trying to say is that the CPM0 / CPM1 objects predate
specs for doing this in a more standardized ways by many years.

Also the specs to do this in a standardized way are mostly being driven
from the ARM side of things and x86 hardware/firmware developers are
used to just doing there own things for decades now, so I would not
expect them to pick this up anytime soon, sorry.

Regards,

Hans



>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v2:
>> - Factor out the parsing into a separate helper function
>> ---
>>   drivers/iio/light/cm32181.c | 101 ++++++++++++++++++++++++++++++++++++
>>   1 file changed, 101 insertions(+)
>>
>> diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
>> index cae2264e4336..d8b128286527 100644
>> --- a/drivers/iio/light/cm32181.c
>> +++ b/drivers/iio/light/cm32181.c
>> @@ -4,6 +4,7 @@
>>    * Author: Kevin Tsai <ktsai@capellamicro.com>
>>    */
>>   
>> +#include <linux/acpi.h>
>>   #include <linux/delay.h>
>>   #include <linux/err.h>
>>   #include <linux/i2c.h>
>> @@ -53,6 +54,15 @@
>>   
>>   #define SMBUS_ALERT_RESPONSE_ADDRESS	0x0c
>>   
>> +/* CPM0 Index 0: device-id (3218 or 32181), 1: Unknown, 2: init_regs_bitmap */
>> +#define CPM0_REGS_BITMAP		2
>> +#define CPM0_HEADER_SIZE		3
>> +
>> +/* CPM1 Index 0: lux_per_bit, 1: calibscale, 2: resolution (100000) */
>> +#define CPM1_LUX_PER_BIT		0
>> +#define CPM1_CALIBSCALE			1
>> +#define CPM1_SIZE			3
>> +
>>   /* CM3218 Family */
>>   static const int cm3218_als_it_bits[] = { 0, 1, 2, 3 };
>>   static const int cm3218_als_it_values[] = { 100000, 200000, 400000, 800000 };
>> @@ -65,6 +75,7 @@ static const int cm32181_als_it_values[] = {
>>   
>>   struct cm32181_chip {
>>   	struct i2c_client *client;
>> +	struct device *dev;
>>   	struct mutex lock;
>>   	u16 conf_regs[CM32181_CONF_REG_NUM];
>>   	unsigned long init_regs_bitmap;
>> @@ -76,6 +87,92 @@ struct cm32181_chip {
>>   	const int *als_it_values;
>>   };
>>   
>> +static int cm32181_read_als_it(struct cm32181_chip *cm32181, int *val2);
>> +
>> +#ifdef CONFIG_ACPI
>> +/**
>> + * cm32181_acpi_get_cpm() - Get CPM object from ACPI
>> + * @client	pointer of struct i2c_client.
>> + * @obj_name	pointer of ACPI object name.
>> + * @count	maximum size of return array.
>> + * @vals	pointer of array for return elements.
>> + *
>> + * Convert ACPI CPM table to array.
>> + *
>> + * Return: -ENODEV for fail.  Otherwise is number of elements.
>> + */
>> +static int cm32181_acpi_get_cpm(struct device *dev, char *obj_name,
>> +				u64 *values, int count)
>> +{
>> +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
>> +	union acpi_object *cpm, *elem;
>> +	acpi_handle handle;
>> +	acpi_status status;
>> +	int i;
>> +
>> +	handle = ACPI_HANDLE(dev);
>> +	if (!handle)
>> +		return -ENODEV;
>> +
>> +	status = acpi_evaluate_object(handle, obj_name, NULL, &buffer);
>> +	if (ACPI_FAILURE(status)) {
>> +		dev_err(dev, "object %s not found\n", obj_name);
>> +		return -ENODEV;
>> +	}
>> +
>> +	cpm = buffer.pointer;
>> +	if (cpm->package.count > count)
>> +		dev_warn(dev, "%s table contains %d values, only using first %d values\n",
>> +			 obj_name, cpm->package.count, count);
>> +
>> +	count = min_t(int, cpm->package.count, count);
>> +	for (i = 0; i < count; i++) {
>> +		elem = &(cpm->package.elements[i]);
>> +		values[i] = elem->integer.value;
>> +	}
>> +
>> +	kfree(buffer.pointer);
>> +
>> +	return count;
>> +}
>> +
>> +static void cm32181_acpi_parse_cpm_tables(struct cm32181_chip *cm32181)
>> +{
>> +	u64 vals[CPM0_HEADER_SIZE + CM32181_CONF_REG_NUM];
>> +	struct device *dev = cm32181->dev;
>> +	int i, count;
>> +
>> +	count = cm32181_acpi_get_cpm(dev, "CPM0", vals, ARRAY_SIZE(vals));
>> +	if (count <= CPM0_HEADER_SIZE)
>> +		return;
>> +
>> +	count -= CPM0_HEADER_SIZE;
>> +
>> +	cm32181->init_regs_bitmap = vals[CPM0_REGS_BITMAP];
>> +	cm32181->init_regs_bitmap &= GENMASK(count - 1, 0);
>> +	for_each_set_bit(i, &cm32181->init_regs_bitmap, count)
>> +		cm32181->conf_regs[i] =	vals[CPM0_HEADER_SIZE + i];
>> +
>> +	count = cm32181_acpi_get_cpm(dev, "CPM1", vals, ARRAY_SIZE(vals));
>> +	if (count != CPM1_SIZE)
>> +		return;
>> +
>> +	cm32181->lux_per_bit = vals[CPM1_LUX_PER_BIT];
>> +
>> +	/* Check for uncalibrated devices */
>> +	if (vals[CPM1_CALIBSCALE] == CM32181_CALIBSCALE_DEFAULT)
>> +		return;
>> +
>> +	cm32181->calibscale = vals[CPM1_CALIBSCALE];
>> +	/* CPM1 lux_per_bit is for the current it value */
>> +	cm32181_read_als_it(cm32181, &cm32181->lux_per_bit_base_it);
>> +}
>> +#else
>> +static void cm32181_acpi_parse_cpm_tables(struct cm32181_chip *cm32181)
>> +{
>> +}
>> +#endif /* CONFIG_ACPI */
>> +
>>   /**
>>    * cm32181_reg_init() - Initialize CM32181 registers
>>    * @cm32181:	pointer of struct cm32181.
>> @@ -119,6 +216,9 @@ static int cm32181_reg_init(struct cm32181_chip *cm32181)
>>   	cm32181->lux_per_bit = CM32181_LUX_PER_BIT;
>>   	cm32181->lux_per_bit_base_it = CM32181_LUX_PER_BIT_BASE_IT;
>>   
>> +	if (ACPI_HANDLE(cm32181->dev))
>> +		cm32181_acpi_parse_cpm_tables(cm32181);
>> +
>>   	/* Initialize registers*/
>>   	for_each_set_bit(i, &cm32181->init_regs_bitmap, CM32181_CONF_REG_NUM) {
>>   		ret = i2c_smbus_write_word_data(client, i,
>> @@ -361,6 +461,7 @@ static int cm32181_probe(struct i2c_client *client)
>>   
>>   	cm32181 = iio_priv(indio_dev);
>>   	cm32181->client = client;
>> +	cm32181->dev = dev;
>>   
>>   	mutex_init(&cm32181->lock);
>>   	indio_dev->dev.parent = dev;
> 

[PATCH v3 11/11] iio: light: cm32181: Fix integartion time typo

[Hans de Goede]

Fix integartion time typo and while at it improve the comment with
the typo a bit in general.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/iio/light/cm32181.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
index d8b128286527..8e294069c7da 100644
--- a/drivers/iio/light/cm32181.c
+++ b/drivers/iio/light/cm32181.c
@@ -235,7 +235,7 @@ static int cm32181_reg_init(struct cm32181_chip *cm32181)
  *  @cm32181:	pointer of struct cm32181
  *  @val2:	pointer of int to load the als_it value.
  *
- *  Report the current integartion time by millisecond.
+ *  Report the current integration time in milliseconds.
  *
  *  Return: IIO_VAL_INT_PLUS_MICRO for success, otherwise -EINVAL.
  */
-- 
2.26.0

Re: [PATCH v3 01/11] iio: light: cm32181: Switch to new style i2c-driver probe function

[Jonathan Cameron]

On Tue, 28 Apr 2020 19:29:13 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> Switch to the new style i2c-driver probe_new probe function and drop the
> unnecessary i2c_device_id table (we do not have any old style board files
> using this).
> 
> This is a preparation patch for adding ACPI binding support.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v3:
> - This is a new patch in v3 of this patch-set
> ---
>  drivers/iio/light/cm32181.c | 15 +++------------
>  1 file changed, 3 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
> index 5f4fb5674fa0..cc57190a24cb 100644
> --- a/drivers/iio/light/cm32181.c
> +++ b/drivers/iio/light/cm32181.c
> @@ -294,8 +294,7 @@ static const struct iio_info cm32181_info = {
>  	.attrs			= &cm32181_attribute_group,
>  };
>  
> -static int cm32181_probe(struct i2c_client *client,
> -			const struct i2c_device_id *id)
> +static int cm32181_probe(struct i2c_client *client)
>  {
>  	struct cm32181_chip *cm32181;
>  	struct iio_dev *indio_dev;
> @@ -316,7 +315,7 @@ static int cm32181_probe(struct i2c_client *client,
>  	indio_dev->channels = cm32181_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(cm32181_channels);
>  	indio_dev->info = &cm32181_info;
> -	indio_dev->name = id->name;
> +	indio_dev->name = dev_name(&client->dev);

ABI breakage.  The name needs to be unaffected by this patch and I'm 
fairly sure it just gained the vendor prefix.

So to drop that table, you need to provide the 'clean' part number
somewhere else.  Seeing as driver currently only supports one number,
you could just provide it directly here. However, as you are
going to add support for another part number later, you'll need
to do something more clever when you introduce that. 

I'll make this suggestion in that patch, but I think you should add
a chip_info structure for each of the supported chips rather than using
a switch to put a number of different elements in place.  The name
would then go in there.

Jonathan


>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  
>  	ret = cm32181_reg_init(cm32181);
> @@ -338,13 +337,6 @@ static int cm32181_probe(struct i2c_client *client,
>  	return 0;
>  }
>  
> -static const struct i2c_device_id cm32181_id[] = {
> -	{ "cm32181", 0 },
> -	{ }
> -};
> -
> -MODULE_DEVICE_TABLE(i2c, cm32181_id);
> -
>  static const struct of_device_id cm32181_of_match[] = {
>  	{ .compatible = "capella,cm32181" },
>  	{ }
> @@ -356,8 +348,7 @@ static struct i2c_driver cm32181_driver = {
>  		.name	= "cm32181",
>  		.of_match_table = of_match_ptr(cm32181_of_match),
>  	},
> -	.id_table       = cm32181_id,
> -	.probe		= cm32181_probe,
> +	.probe_new	= cm32181_probe,
>  };
>  
>  module_i2c_driver(cm32181_driver);

Re: [PATCH v3 01/11] iio: light: cm32181: Switch to new style i2c-driver probe function

[Hans de Goede]

Hi,

On 5/3/20 12:54 PM, Jonathan Cameron wrote:
> On Tue, 28 Apr 2020 19:29:13 +0200
> Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> Switch to the new style i2c-driver probe_new probe function and drop the
>> unnecessary i2c_device_id table (we do not have any old style board files
>> using this).
>>
>> This is a preparation patch for adding ACPI binding support.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v3:
>> - This is a new patch in v3 of this patch-set
>> ---
>>   drivers/iio/light/cm32181.c | 15 +++------------
>>   1 file changed, 3 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
>> index 5f4fb5674fa0..cc57190a24cb 100644
>> --- a/drivers/iio/light/cm32181.c
>> +++ b/drivers/iio/light/cm32181.c
>> @@ -294,8 +294,7 @@ static const struct iio_info cm32181_info = {
>>   	.attrs			= &cm32181_attribute_group,
>>   };
>>   
>> -static int cm32181_probe(struct i2c_client *client,
>> -			const struct i2c_device_id *id)
>> +static int cm32181_probe(struct i2c_client *client)
>>   {
>>   	struct cm32181_chip *cm32181;
>>   	struct iio_dev *indio_dev;
>> @@ -316,7 +315,7 @@ static int cm32181_probe(struct i2c_client *client,
>>   	indio_dev->channels = cm32181_channels;
>>   	indio_dev->num_channels = ARRAY_SIZE(cm32181_channels);
>>   	indio_dev->info = &cm32181_info;
>> -	indio_dev->name = id->name;
>> +	indio_dev->name = dev_name(&client->dev);
> 
> ABI breakage.  The name needs to be unaffected by this patch and I'm
> fairly sure it just gained the vendor prefix.
> 
> So to drop that table, you need to provide the 'clean' part number
> somewhere else.  Seeing as driver currently only supports one number,
> you could just provide it directly here. However, as you are
> going to add support for another part number later, you'll need
> to do something more clever when you introduce that.

Ok, I will fix this for the next version.

> I'll make this suggestion in that patch, but I think you should add
> a chip_info structure for each of the supported chips rather than using
> a switch to put a number of different elements in place.  The name
> would then go in there.

Ok, I will add a chip_info structure for the next version (version 4).

Regards,

Hans