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.
> };
>
> /**
> @@ -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" },
> { }
> };
