| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Sat, 23 May 2015 12:15:42 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Kevin Tsai <ktsai@...ellamicro.com>,
Rob Herring <robh+dt@...nel.org>,
Pawel Moll <pawel.moll@....com>,
Mark Rutland <mark.rutland@....com>,
Ian Campbell <ijc+devicetree@...lion.org.uk>,
Kumar Gala <galak@...eaurora.org>,
Hartmut Knaack <knaack.h@....de>,
Lars-Peter Clausen <lars@...afoo.de>,
Peter Meerwald <pmeerw@...erw.net>,
Andrew Morton <akpm@...ux-foundation.org>,
Greg KH <gregkh@...uxfoundation.org>,
"David S. Miller" <davem@...emloft.net>,
Mauro Carvalho Chehab <mchehab@....samsung.com>,
Arnd Bergmann <arnd@...db.de>, Joe Perches <joe@...ches.com>,
Jingoo Han <jingoohan1@...il.com>,
Daniel Baluta <daniel.baluta@...el.com>,
Roberta Dobrescu <roberta.dobrescu@...il.com>,
Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@...ux.intel.com>
CC: devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-iio@...r.kernel.org
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 <ktsai@...ellamicro.com>
> ---
> .../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 <ktsai@...ellamicro.com>
> -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 <varkabhadram@...il.com>
> L: linux-wpan@...r.kernel.org
> @@ -10610,6 +10604,12 @@ L: netdev@...r.kernel.org
> S: Maintained
> F: drivers/net/ethernet/via/via-velocity.*
>
> +VISHAY CAPELLA LIGHT SENSOR DRIVER
> +M: Kevin Tsai <ktsai@...ellamicro.com>
> +S: Maintained
> +F: drivers/iio/light/cm*
> +F: Documentation/devicetree/bindings/i2c/trivial-devices.txt
> +
> VIVID VIRTUAL VIDEO DRIVER
> M: Hans Verkuil <hverkuil@...all.nl>
> L: linux-media@...r.kernel.org
> 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[] = {
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists