Hi, On 5/3/20 12:59 PM, Jonathan Cameron wrote:
On Tue, 28 Apr 2020 19:29:16 +0200 Hans de Goede <hdegoede@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxx> * * 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@xxxxxxxxx> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>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" }, { } };
