by Jonathan Cameron

[permalink] [raw]

Subject: Re: [PATCH 5/6] iio: light: Updated Vishay Capella cm32181 ambient light sensor driver.

On 22/05/15 05:19, Kevin Tsai wrote:
> - Added Device Tree support.
>
> Signed-off-by: Kevin Tsai <[email protected]>
> ---
> .../devicetree/bindings/iio/light/cm32181.txt | 33 +++++++++++
> MAINTAINERS | 12 ++--
> drivers/iio/light/cm32181.c | 66 ++++++++++------------
> 3 files changed, 70 insertions(+), 41 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/iio/light/cm32181.txt
>
> diff --git a/Documentation/devicetree/bindings/iio/light/cm32181.txt b/Documentation/devicetree/bindings/iio/light/cm32181.txt
> new file mode 100644
> index 0000000..b81a3e3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/light/cm32181.txt
> @@ -0,0 +1,33 @@
> +* Vishay Capella CM32181 Ambient Light sensor
> +
> +Required properties:
> +- compatible: must be "capella,cm32181"
> +- reg: the I2C address of the device
> +
> +Optional properties:
> +- capella,hw_id: hardware device id.
Why? Also listing it just the once would be good :)
I'd actively discourage this. If a new part using a different hw_id
turns up then the compatible string should distinguish it and the driver
be ammended to support it.

> +- capella,reg_cmd: command register initialization.
If you want to actually do this then you need to drop the magic numbers
and explicitly support everything these registers support.
> +- capella,reg_als_wh: high threshold register initialization.
> +- capella,reg_als_wl: low threshold register initialization.
> +- capella,calibscale: calibrated factor with 10^-5 notation.
This one is fair enough though impresively random unit.
Documentation here should indicate that this is dependent on device
packaging (and what else?)
> +- capella,hw_id: hardware device id.
> +- capella,mlux_per_bit: millilux per bit under the default IT.
Should be handled by the calibscale value above I think...

> +- capella,thd_percent: threshold percentage change to trigger.
Not a hardware dependent element to my mind. Should not really
be in the device tree...
> +
> +Example:
> +
> +cm32181@10 {
> + compatible = "capella,cm32181";
> + reg = <0x10>;
> + interrupt-parent = <&gpio1>;
> + interrupts = <17 0>;
> +
> + capella,hw_id = <0x81>;
> + capella,reg_cmd = <0x04>;
> + capella,reg_als_wh = <0xFFFF>;
> + capella,reg_als_wl = <0x0000>;
> + capella,calibscale = <10000>;
> + capella,mlux_per_bit = <5>;
> + capella,thd_percent = <5>;
> +};
> +
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f8e0afb..ee6a8f6 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2431,12 +2431,6 @@ F: security/capability.c
> F: security/commoncap.c
> F: kernel/capability.c
>
> -CAPELLA MICROSYSTEMS LIGHT SENSOR DRIVER
> -M: Kevin Tsai <[email protected]>
> -S: Maintained
> -F: drivers/iio/light/cm*
> -F: Documentation/devicetree/bindings/i2c/trivial-devices.txt
> -
> CC2520 IEEE-802.15.4 RADIO DRIVER
> M: Varka Bhadram <[email protected]>
> L: [email protected]
> @@ -10610,6 +10604,12 @@ L: [email protected]
> S: Maintained
> F: drivers/net/ethernet/via/via-velocity.*
>
> +VISHAY CAPELLA LIGHT SENSOR DRIVER
> +M: Kevin Tsai <[email protected]>
> +S: Maintained
> +F: drivers/iio/light/cm*
> +F: Documentation/devicetree/bindings/i2c/trivial-devices.txt
> +
> VIVID VIRTUAL VIDEO DRIVER
> M: Hans Verkuil <[email protected]>
> L: [email protected]
> diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
> index 54bf0cb..b7abd46 100644
> --- a/drivers/iio/light/cm32181.c
> +++ b/drivers/iio/light/cm32181.c
> @@ -51,6 +51,7 @@
> #define CM32181_HW_ID 0x81
> #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_THD_PERCENT 5 /* 0 for polling mode */
> #define CM32181_CALIBSCALE_DEFAULT 1000
> #define CM32181_CALIBSCALE_RESOLUTION 1000
> #define CM32181_MLUX_PER_LUX 1000
> @@ -80,6 +81,7 @@ struct cm32181_als_info {
> int calibscale;
> int mlux_per_bit;
> int mlux_per_bit_base_it;
> + int thd_percent;
> };
>
> static struct cm32181_als_info cm32181_als_info_default = {
> @@ -90,6 +92,7 @@ static struct cm32181_als_info cm32181_als_info_default = {
> .calibscale = CM32181_CALIBSCALE_DEFAULT,
> .mlux_per_bit = CM32181_MLUX_PER_BIT,
> .mlux_per_bit_base_it = CM32181_MLUX_PER_BIT_BASE_IT,
> + .thd_percent = CM32181_THD_PERCENT,
> };
>
> struct cm32181_chip {
> @@ -99,6 +102,30 @@ struct cm32181_chip {
> u16 conf_regs[CM32181_CONF_REG_NUM];
> };
>
> +#ifdef CONFIG_OF
> +void cm32181_parse_dt(struct cm32181_chip *chip)
> +{
> + struct device_node *dn = chip->client->dev.of_node;
> + struct cm32181_als_info *als_info = chip->als_info;
> + u32 temp_val;
> +
> + if (!of_property_read_u32(dn, "capella,hw_id", &temp_val))
> + als_info->hw_id = (uint8_t)temp_val;
> + if (!of_property_read_u32(dn, "capella,reg_cmd", &temp_val))
> + als_info->reg_cmd = (uint16_t)temp_val;
> + if (!of_property_read_u32(dn, "capella,reg_als_wh", &temp_val))
> + als_info->reg_als_wh = (uint16_t)temp_val;
> + if (!of_property_read_u32(dn, "capella,reg_als_wl", &temp_val))
> + als_info->reg_als_wl = (uint16_t)temp_val;
> + if (!of_property_read_u32(dn, "capella,calibscale", &temp_val))
> + als_info->calibscale = (int)temp_val;
> + if (!of_property_read_u32(dn, "capella,mlux_per_bit", &temp_val))
> + als_info->mlux_per_bit = (int)temp_val;
> + if (!of_property_read_u32(dn, "capella,thd_percent", &temp_val))
> + als_info->thd_percent = (int)temp_val;
> +}
> +#endif
> +
> /**
> * cm32181_reg_init() - Initialize CM32181 registers
> * @chip: pointer of struct cm32181_chip
> @@ -116,6 +143,10 @@ static int cm32181_reg_init(struct cm32181_chip *chip)
>
> chip->als_info = &cm32181_als_info_default;
> als_info = chip->als_info;
> +#ifdef CONFIG_OF
> + if (client->dev.of_node)
> + cm32181_parse_dt(chip);
> +#endif
>
> ret = i2c_smbus_read_word_data(client, CM32181_REG_ADDR_ID);
> if (ret < 0)
> @@ -208,33 +239,6 @@ static int cm32181_write_als_it(struct cm32181_chip *chip, int val, int val2)
> }
> }
> return -EINVAL;
Ah, here's where the dead code from the previous patch gets cleaned up.
Sort this out please.
> -
> -/*
> - struct i2c_client *client = chip->client;
> - u16 als_it;
> - int ret, i, n;
> -
> - n = ARRAY_SIZE(als_it_value);
> - for (i = 0; i < n; i++)
> - if (val <= als_it_value[i])
> - break;
> - if (i >= n)
> - i = n - 1;
> -
> - als_it = als_it_bits[i];
> - als_it <<= CM32181_CMD_ALS_IT_SHIFT;
> -
> - mutex_lock(&chip->lock);
> - chip->conf_regs[CM32181_REG_ADDR_CMD] &=
> - ~CM32181_CMD_ALS_IT_MASK;
> - chip->conf_regs[CM32181_REG_ADDR_CMD] |=
> - als_it;
> - ret = i2c_smbus_write_word_data(client, CM32181_REG_ADDR_CMD,
> - chip->conf_regs[CM32181_REG_ADDR_CMD]);
> - mutex_unlock(&chip->lock);
> -
> - return ret;
> -*/
> }
>
> /**
> @@ -344,14 +348,6 @@ static ssize_t cm32181_get_it_available(struct device *dev,
> cm32181_als_it_scales[i].val,
> cm32181_als_it_scales[i].val2);
> return len + scnprintf(buf + len, PAGE_SIZE - len, "\n");
Hmm. I missed this one. Clean up whereever it came from.
> -/*
> - int i, n, len;
> -
> - n = ARRAY_SIZE(als_it_value);
> - for (i = 0, len = 0; i < n; i++)
> - len += sprintf(buf + len, "0.%06u ", als_it_value[i]);
> - return len + sprintf(buf + len, "\n");
> -*/
> }
>
> static const struct iio_chan_spec cm32181_channels[] = {
>

2015-05-23 11:22:04

by Jonathan Cameron

[permalink] [raw]

Subject: Re: [PATCH 6/6] iio: light: Updated Vishay Capella cm32181 ambient light sensor driver.

On 22/05/15 05:19, Kevin Tsai wrote:
> - Added Interrupt support.
>
> Signed-off-by: Kevin Tsai <[email protected]>
This needs a detailed description. There is interaction in here I think
between auto set thresholds and IIO threshold events. Not entirely
obvious how this intended to work.

Particularly as it seems that any read of the als will result in the thresholds
moving whether or nor a limit has been reached.

Jonathan

> ---
> drivers/iio/light/cm32181.c | 156 +++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 153 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
> index b7abd46..1ae32a0 100644
> --- a/drivers/iio/light/cm32181.c
> +++ b/drivers/iio/light/cm32181.c
> @@ -47,6 +47,10 @@
> /* ALS_WL register */
> #define CM32181_ALS_WL_DEFAULT 0x0000
>
> +/* STATUS register */
> +#define CM32181_STATUS_ALS_IF_L BIT(15)
> +#define CM32181_STATUS_ALS_IF_H BIT(14)
> +
> /* Software parameters */
> #define CM32181_HW_ID 0x81
> #define CM32181_MLUX_PER_BIT 5 /* ALS_SM=01 IT=800ms */
> @@ -122,7 +126,8 @@ void cm32181_parse_dt(struct cm32181_chip *chip)
> if (!of_property_read_u32(dn, "capella,mlux_per_bit", &temp_val))
> als_info->mlux_per_bit = (int)temp_val;
> if (!of_property_read_u32(dn, "capella,thd_percent", &temp_val))
> - als_info->thd_percent = (int)temp_val;
> + if (((int)temp_val >= 0) && ((int)temp_val < 100))
> + als_info->thd_percent = (int)temp_val;
> }
> #endif
>
> @@ -173,6 +178,63 @@ static int cm32181_reg_init(struct cm32181_chip *chip)
> }
>
> /**
> + * cm32181_interrupt() - enable/disable interrupt
> + * @chip: pointer of struct cm32181_chip
> + * @ int_en: truen for enable; false for disable
> + *
> + * Enable/disable interrupt mode
> + *
> + * Return: 0 for success; otherwise for error code.
> + */
> +static int cm32181_interrupt(struct cm32181_chip *cm32181, bool int_en)
> +{
> + int ret;
> +
> + mutex_lock(&cm32181->lock);
> + if (int_en)
> + cm32181->conf_regs[CM32181_REG_ADDR_CMD] |=
> + CM32181_CMD_ALS_INT_EN;
> + else
> + cm32181->conf_regs[CM32181_REG_ADDR_CMD] &=
> + ~CM32181_CMD_ALS_INT_EN;
> +
> + ret = i2c_smbus_write_word_data(cm32181->client,
> + CM32181_REG_ADDR_CMD,
> + cm32181->conf_regs[CM32181_REG_ADDR_CMD]);
> + mutex_unlock(&cm32181->lock);
> + return ret;
> +}
> +
> +static irqreturn_t cm32181_irq_handler(int irq, void *data)
> +{
> + struct iio_dev *indio_dev = data;
> + struct cm32181_chip *cm32181 = iio_priv(indio_dev);
> + struct i2c_client *client = cm32181->client;
> + int ret;
> + u64 ev_code;
> +
> + ret = i2c_smbus_read_word_data(cm32181->client,
> + CM32181_REG_ADDR_STATUS);
> + if (ret < 0) {
> + dev_err(&client->dev,
> + "%s: Data read failed: %d\n", __func__, ret);
> + return IRQ_NONE;
> + }
> +
> + if (!(ret & (CM32181_STATUS_ALS_IF_H | CM32181_STATUS_ALS_IF_L)))
> + return IRQ_NONE;
> +
> + ev_code = IIO_UNMOD_EVENT_CODE(IIO_LIGHT,
> + 0,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_EITHER);
> +
> + iio_push_event(indio_dev, ev_code, iio_get_time_ns());
> +
> + return IRQ_HANDLED;
> +}
> +
> +/**
> * cm32181_read_als_it() - Get sensor integration time
> * @chip: pointer of struct cm32181_chip
> * @val: pointer of int to load the integration (sec)
> @@ -242,6 +304,51 @@ static int cm32181_write_als_it(struct cm32181_chip *chip, int val, int val2)
> }
>
> /**
> + * cm32181_ials_read() - read ALS raw data
typo
> + * @chip: pointer of struct cm32181_chip
> + *
> + * Read the ALS raw data and update the interrupt threshold windows.
> + *
> + * Return: Positive value is ALS raw data, otherwise is error code.
> + */
> +static int cm32181_als_read(struct cm32181_chip *chip)
> +{
> + struct i2c_client *client = chip->client;
> + struct cm32181_als_info *als_info = chip->als_info;
> + u16 als, wh, wl, delta;
> + int ret;
> +
> + ret = i2c_smbus_read_word_data(client, CM32181_REG_ADDR_ALS);
> + if (ret < 0)
> + return ret;
> +
> + als = (u16)ret;
> +
> + if (als_info->thd_percent) {
> + delta = als * als_info->thd_percent / 100;
> + if (delta < 3)
> + delta = 3;
> + wh = (als + delta > 0xFFFF) ? 0xFFFF : (als + delta);
> + wl = (als < delta) ? 0 : (als - delta);
> +
> + ret = i2c_smbus_write_word_data(client,
> + CM32181_REG_ADDR_ALS_WH, wh);
> + if (ret < 0)
> + return ret;
> + chip->conf_regs[CM32181_REG_ADDR_ALS_WH] = wh;
> +
> + ret = i2c_smbus_write_word_data(client,
> + CM32181_REG_ADDR_ALS_WL, wl);
> + if (ret < 0)
> + return ret;
> + chip->conf_regs[CM32181_REG_ADDR_ALS_WL] = wl;
> + ret = als;
> + }
> +
> + return ret;
> +}
> +
> +/**
> * cm32181_get_lux() - report current lux value
> * @chip: pointer of struct cm32181_chip
> *
> @@ -252,7 +359,6 @@ static int cm32181_write_als_it(struct cm32181_chip *chip, int val, int val2)
> */
> static int cm32181_get_lux(struct cm32181_chip *chip)
> {
> - struct i2c_client *client = chip->client;
> struct cm32181_als_info *als_info = chip->als_info;
> int ret;
> int val, val2;
> @@ -268,7 +374,7 @@ static int cm32181_get_lux(struct cm32181_chip *chip)
> lux *= als_info->mlux_per_bit_base_it;
> lux = div_u64(lux, als_it);
>
> - ret = i2c_smbus_read_word_data(client, CM32181_REG_ADDR_ALS);
> + ret = cm32181_als_read(chip);
> if (ret < 0)
> return ret;
>
> @@ -350,6 +456,15 @@ static ssize_t cm32181_get_it_available(struct device *dev,
> return len + scnprintf(buf + len, PAGE_SIZE - len, "\n");
> }
>
> +static const struct iio_event_spec cm32181_event_spec[] = {
> + {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_EITHER,
> + .mask_separate = BIT(IIO_EV_INFO_VALUE) |
> + BIT(IIO_EV_INFO_ENABLE),
> + }
> +};
> +
> static const struct iio_chan_spec cm32181_channels[] = {
> {
> .type = IIO_LIGHT,
> @@ -357,6 +472,8 @@ static const struct iio_chan_spec cm32181_channels[] = {
> BIT(IIO_CHAN_INFO_PROCESSED) |
> BIT(IIO_CHAN_INFO_CALIBSCALE) |
> BIT(IIO_CHAN_INFO_INT_TIME),
> + .event_spec = cm32181_event_spec,
> + .num_event_specs = ARRAY_SIZE(cm32181_event_spec),
> }
> };
>
> @@ -383,6 +500,7 @@ static int cm32181_probe(struct i2c_client *client,
> const struct i2c_device_id *id)
> {
> struct cm32181_chip *chip;
> + struct cm32181_als_info *als_info;
> struct iio_dev *indio_dev;
> int ret;
>
> @@ -412,11 +530,35 @@ static int cm32181_probe(struct i2c_client *client,
> return ret;
> }
>
> + als_info = chip->als_info;
> + if (als_info->thd_percent && client->irq) {
> + ret = request_threaded_irq(client->irq, NULL,
> + cm32181_irq_handler,
> + IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> + indio_dev->name, indio_dev);
> + if (ret < 0) {
> + dev_err(&client->dev, "%s: request irq failed\n",
> + __func__);
> + return ret;
> + }
> +
> + ret = cm32181_interrupt(chip, true);
> + if (ret) {
> + als_info->thd_percent = 0;
> + dev_err(&client->dev,
> + "%s: failed to enable interrupt\n", __func__);
> + }
> + } else {
> + als_info->thd_percent = 0; /* Polling Mode */
> + }
> +
> ret = devm_iio_device_register(&client->dev, indio_dev);
> if (ret) {
> dev_err(&client->dev,
> "%s: regist device failed\n",
> __func__);
use goto and a combined exit path now we have this to unwind.
> + if (als_info->thd_percent)
> + free_irq(client->irq, indio_dev);
> return ret;
> }
>
> @@ -425,9 +567,17 @@ static int cm32181_probe(struct i2c_client *client,
>
> static int cm32181_remove(struct i2c_client *client)
> {
> + struct iio_dev *indio_dev = i2c_get_clientdata(client);
> + struct cm32181_chip *chip = iio_priv(indio_dev);
> + struct cm32181_als_info *als_info = chip->als_info;
> +
> + cm32181_interrupt(chip, false);
> i2c_smbus_write_word_data(client, CM32181_REG_ADDR_CMD,
> CM32181_CMD_ALS_DISABLE);
>
> + if (als_info->thd_percent)
> + free_irq(client->irq, indio_dev);
> +
> return 0;
> }
>
>