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

[Hans de Goede <hdegoede@redhat.com>]

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