* [PATCH V1 1/1] iio: Added Capella cm3232 ambient light sensor driver.
@ 2015-01-01 0:10 ` Kevin Tsai
0 siblings, 0 replies; 21+ messages in thread
From: Kevin Tsai @ 2015-01-01 0:10 UTC (permalink / raw)
To: Kevin Tsai, Wolfram Sang, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald, Grant Likely, Andrew Morton,
David S. Miller, Greg Kroah-Hartman, Joe Perches,
Mauro Carvalho Chehab, Antti Palosaari, Daniel Baluta,
Archana Patni
Cc: linux-i2c, devicetree, linux-iio, linux-kernel
CM3232 is an advanced ambient light sensor with I2C protocol interface.
The I2C slave address is internally hardwired as 0x10 (7-bit). Writing
to configure register is byte mode, but reading ALS register requests to
use word mode for 16-bit resolution.
Signed-off-by: Kevin Tsai <ktsai@capellamicro.com>
---
.../devicetree/bindings/i2c/trivial-devices.txt | 1 +
MAINTAINERS | 6 +
drivers/iio/light/Kconfig | 11 +
drivers/iio/light/Makefile | 1 +
drivers/iio/light/cm3232.c | 403 +++++++++++++++++++++
5 files changed, 422 insertions(+)
create mode 100644 drivers/iio/light/cm3232.c
diff --git a/Documentation/devicetree/bindings/i2c/trivial-devices.txt b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
index 9f4e382..572a7c4 100644
--- a/Documentation/devicetree/bindings/i2c/trivial-devices.txt
+++ b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
@@ -34,6 +34,7 @@ atmel,24c512 i2c serial eeprom (24cxx)
atmel,24c1024 i2c serial eeprom (24cxx)
atmel,at97sc3204t i2c trusted platform module (TPM)
capella,cm32181 CM32181: Ambient Light Sensor
+capella,cm3232 CM3232: Ambient Light Sensor
catalyst,24c32 i2c serial eeprom
cirrus,cs42l51 Cirrus Logic CS42L51 audio codec
dallas,ds1307 64 x 8, Serial, I2C Real-Time Clock
diff --git a/MAINTAINERS b/MAINTAINERS
index ddb9ac8..06a613a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2378,6 +2378,12 @@ F: security/capability.c
F: security/commoncap.c
F: kernel/capability.c
+CAPELLA MICROSYSTEMS LIGHT SENSOR DRIVER
+M: Kevin Tsai <ktsai@capellamicro.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@gmail.com>
L: linux-wpan@vger.kernel.org
diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 5bea821..d2318e2 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -48,6 +48,17 @@ config CM32181
To compile this driver as a module, choose M here:
the module will be called cm32181.
+config CM3232
+ depends on I2C
+ tristate "CM3232 driver"
+ help
+ Say Y here if you use cm3232.
+ This option enables ambient light sensor using
+ Capella Microsystems cm3232 device driver.
+
+ To compile this driver as a module, choose M here:
+ the module will be called cm3232.
+
config CM36651
depends on I2C
tristate "CM36651 driver"
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index 47877a3..f2c8d55 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_ADJD_S311) += adjd_s311.o
obj-$(CONFIG_AL3320A) += al3320a.o
obj-$(CONFIG_APDS9300) += apds9300.o
obj-$(CONFIG_CM32181) += cm32181.o
+obj-$(CONFIG_CM3232) += cm3232.o
obj-$(CONFIG_CM36651) += cm36651.o
obj-$(CONFIG_GP2AP020A00F) += gp2ap020a00f.o
obj-$(CONFIG_HID_SENSOR_ALS) += hid-sensor-als.o
diff --git a/drivers/iio/light/cm3232.c b/drivers/iio/light/cm3232.c
new file mode 100644
index 0000000..fd98eeb
--- /dev/null
+++ b/drivers/iio/light/cm3232.c
@@ -0,0 +1,403 @@
+/*
+ * 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.
+ *
+ */
+
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/mutex.h>
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/regulator/consumer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/events.h>
+#include <linux/init.h>
+
+/* Registers Address */
+#define CM3232_REG_ADDR_CMD 0x00
+#define CM3232_REG_ADDR_ALS 0x50
+#define CM3232_REG_ADDR_ID 0x53
+
+/* CMD register */
+#define CM3232_CMD_ALS_DISABLE BIT(0)
+#define CM3232_CMD_ALS_HS BIT(1)
+
+#define CM3232_CMD_ALS_IT_SHIFT 2
+#define CM3232_CMD_ALS_IT_MASK (0x07 << CM3232_CMD_ALS_IT_SHIFT)
+#define CM3232_CMD_ALS_IT_DEFAULT (0x01 << CM3232_CMD_ALS_IT_SHIFT)
+
+#define CM3232_CMD_ALS_RESET BIT(6)
+
+#define CM3232_CMD_DEFAULT CM3232_CMD_ALS_IT_DEFAULT
+
+#define CM3232_CALIBSCALE_DEFAULT 100000
+#define CM3232_CALIBSCALE_RESOLUTION 100000
+#define CM3232_MLUX_PER_LUX 1000
+
+#define CM3232_MLUX_PER_BIT_DEFAULT 64
+#define CM3232_MLUX_PER_BIT_BASE_IT 100000
+static const int CM3232_als_it_bits[] = { 0, 1, 2, 3, 4, 5};
+static const int CM3232_als_it_values[] = {
+ 100000, 200000, 400000, 800000, 1600000, 3200000};
+
+struct cm3232_als_info {
+ u32 id;
+ int calibscale;
+ int mlux_per_bit;
+ int mlux_per_bit_base_it;
+ const int *als_it_bits;
+ const int *als_it_values;
+ const int num_als_it;
+ int als_raw;
+};
+
+static struct cm3232_als_info cm3232_als_info_default = {
+ .id = 3232,
+ .calibscale = CM3232_CALIBSCALE_DEFAULT,
+ .mlux_per_bit = CM3232_MLUX_PER_BIT_DEFAULT,
+ .mlux_per_bit_base_it = CM3232_MLUX_PER_BIT_BASE_IT,
+ .als_it_bits = CM3232_als_it_bits,
+ .als_it_values = CM3232_als_it_values,
+ .num_als_it = ARRAY_SIZE(CM3232_als_it_bits),
+};
+
+struct cm3232_chip {
+ struct i2c_client *client;
+ struct mutex lock;
+ struct cm3232_als_info *als_info;
+ u8 regs_cmd;
+};
+
+static int cm3232_get_lux(struct cm3232_chip *chip);
+static int cm3232_read_als_it(struct cm3232_chip *chip, int *val2);
+
+/**
+ * cm3232_reg_init() - Initialize CM3232 registers
+ * @chip: pointer of struct cm3232.
+ *
+ * Initialize CM3232 ambient light sensor register to default values.
+ *
+ Return: 0 for success; otherwise for error code.
+ */
+static int cm3232_reg_init(struct cm3232_chip *chip)
+{
+ struct i2c_client *client = chip->client;
+ struct cm3232_als_info *als_info;
+ s32 ret;
+
+ /* Identify device */
+ ret = i2c_smbus_read_word_data(client, CM3232_REG_ADDR_ID);
+ if (ret < 0)
+ return ret;
+ if ((ret & 0xFF) != 0x32)
+ return -ENODEV;
+
+ /* Disable and reset device */
+ chip->regs_cmd = CM3232_CMD_ALS_DISABLE | CM3232_CMD_ALS_RESET;
+ ret = i2c_smbus_write_byte_data(client, CM3232_REG_ADDR_CMD,
+ chip->regs_cmd);
+ if (ret < 0)
+ return ret;
+
+ /* Initialization */
+ als_info = chip->als_info = &cm3232_als_info_default;
+ chip->regs_cmd = CM3232_CMD_DEFAULT;
+
+ /* Configure register */
+ ret = i2c_smbus_write_byte_data(client, CM3232_REG_ADDR_CMD,
+ chip->regs_cmd);
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+/**
+ * cm3232_read_als_it() - Get sensor integration time (ms)
+ * @chip: pointer of struct cm3232
+ * @val2: pointer of int to load the als_it value.
+ *
+ * Report the current integration time in milliseconds.
+ *
+ * Return: IIO_VAL_INT_PLUS_MICRO for success, otherwise -EINVAL.
+ */
+static int cm3232_read_als_it(struct cm3232_chip *chip, int *val2)
+{
+ struct cm3232_als_info *als_info = chip->als_info;
+ u16 als_it;
+ int i;
+
+ als_it = chip->regs_cmd;
+ als_it &= CM3232_CMD_ALS_IT_MASK;
+ als_it >>= CM3232_CMD_ALS_IT_SHIFT;
+ for (i = 0; i < als_info->num_als_it; i++) {
+ if (als_it == als_info->als_it_bits[i]) {
+ *val2 = als_info->als_it_values[i];
+ return IIO_VAL_INT_PLUS_MICRO;
+ }
+ }
+
+ return -EINVAL;
+}
+
+/**
+ * cm3232_write_als_it() - Write sensor integration time
+ * @chip: pointer of struct cm3232.
+ * @val: integration time in milliseconds.
+ *
+ * Convert integration time (ms) to sensor value.
+ *
+ * Return: i2c_smbus_write_word_data command return value.
+ */
+static int cm3232_write_als_it(struct cm3232_chip *chip, int val)
+{
+ struct i2c_client *client = chip->client;
+ struct cm3232_als_info *als_info = chip->als_info;
+ u16 als_it;
+ int ret, i;
+
+ for (i = 0; i < als_info->num_als_it; i++)
+ if (val <= als_info->als_it_values[i])
+ break;
+ if (i >= als_info->num_als_it)
+ i = als_info->num_als_it - 1;
+
+ als_it = als_info->als_it_bits[i];
+ als_it <<= CM3232_CMD_ALS_IT_SHIFT;
+
+ mutex_lock(&chip->lock);
+ chip->regs_cmd &= ~CM3232_CMD_ALS_IT_MASK;
+ chip->regs_cmd |= als_it;
+ ret = i2c_smbus_write_byte_data(client, CM3232_REG_ADDR_CMD,
+ chip->regs_cmd);
+ mutex_unlock(&chip->lock);
+
+ return ret;
+}
+
+/**
+ * cm3232_get_lux() - report current lux value
+ * @chip: pointer of struct cm3232.
+ *
+ * Convert sensor raw data to lux. It depends on integration
+ * time and calibscale variable.
+ *
+ * Return: Positive value is lux, otherwise is error code.
+ */
+static int cm3232_get_lux(struct cm3232_chip *chip)
+{
+ struct i2c_client *client = chip->client;
+ struct cm3232_als_info *als_info = chip->als_info;
+ int ret;
+ int als_it;
+ u64 tmp;
+
+ /* Calculate mlux per bit based on als_it */
+ ret = cm3232_read_als_it(chip, &als_it);
+ if (ret < 0)
+ return -EINVAL;
+ tmp = (__force u64)als_info->mlux_per_bit;
+ tmp *= als_info->mlux_per_bit_base_it;
+ tmp = div_u64 (tmp, als_it);
+
+ /* Get als_raw */
+ als_info->als_raw = i2c_smbus_read_word_data(
+ client,
+ CM3232_REG_ADDR_ALS);
+ if (als_info->als_raw < 0)
+ return als_info->als_raw;
+
+ tmp *= als_info->als_raw;
+ tmp *= als_info->calibscale;
+ tmp = div_u64(tmp, CM3232_CALIBSCALE_RESOLUTION);
+ tmp = div_u64(tmp, CM3232_MLUX_PER_LUX);
+
+ if (tmp > 0xFFFF)
+ tmp = 0xFFFF;
+
+ return (int)tmp;
+}
+
+static int cm3232_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct cm3232_chip *chip = iio_priv(indio_dev);
+ struct cm3232_als_info *als_info = chip->als_info;
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_PROCESSED:
+ ret = cm3232_get_lux(chip);
+ if (ret < 0)
+ return ret;
+ *val = ret;
+ return IIO_VAL_INT;
+ case IIO_CHAN_INFO_CALIBSCALE:
+ *val = als_info->calibscale;
+ return IIO_VAL_INT;
+ case IIO_CHAN_INFO_INT_TIME:
+ *val = 0;
+ ret = cm3232_read_als_it(chip, val2);
+ return ret;
+ }
+
+ return -EINVAL;
+}
+
+static int cm3232_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int val, int val2, long mask)
+{
+ struct cm3232_chip *chip = iio_priv(indio_dev);
+ struct cm3232_als_info *als_info = chip->als_info;
+ long ms;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_CALIBSCALE:
+ als_info->calibscale = val;
+ return val;
+ case IIO_CHAN_INFO_INT_TIME:
+ ms = val * 1000000 + val2;
+ return cm3232_write_als_it(chip, (int)ms);
+ }
+
+ return -EINVAL;
+}
+
+/**
+ * cm3232_get_it_available() - Get available ALS IT value
+ * @dev: pointer of struct device.
+ * @attr: pointer of struct device_attribute.
+ * @buf: pointer of return string buffer.
+ *
+ * Display the available integration time in milliseconds.
+ *
+ * Return: string length.
+ */
+static ssize_t cm3232_get_it_available(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct cm3232_chip *chip = iio_priv(dev_to_iio_dev(dev));
+ struct cm3232_als_info *als_info = chip->als_info;
+ int i, len;
+
+ for (i = 0, len = 0; i < als_info->num_als_it; i++)
+ len += scnprintf(buf + len, PAGE_SIZE - len, "%u.%06u ",
+ als_info->als_it_values[i]/1000000,
+ als_info->als_it_values[i]%1000000);
+ return len + scnprintf(buf + len, PAGE_SIZE - len, "\n");
+}
+
+static const struct iio_chan_spec cm3232_channels[] = {
+ {
+ .type = IIO_LIGHT,
+ .info_mask_separate =
+ BIT(IIO_CHAN_INFO_PROCESSED) |
+ BIT(IIO_CHAN_INFO_CALIBSCALE) |
+ BIT(IIO_CHAN_INFO_INT_TIME),
+ }
+};
+
+static IIO_DEVICE_ATTR(in_illuminance_integration_time_available,
+ S_IRUGO, cm3232_get_it_available, NULL, 0);
+
+static struct attribute *cm3232_attributes[] = {
+ &iio_dev_attr_in_illuminance_integration_time_available.dev_attr.attr,
+ NULL,
+};
+
+static const struct attribute_group cm3232_attribute_group = {
+ .attrs = cm3232_attributes
+};
+
+static const struct iio_info cm3232_info = {
+ .driver_module = THIS_MODULE,
+ .read_raw = &cm3232_read_raw,
+ .write_raw = &cm3232_write_raw,
+ .attrs = &cm3232_attribute_group,
+};
+
+static int cm3232_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct cm3232_chip *chip;
+ struct iio_dev *indio_dev;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
+ if (!indio_dev) {
+ dev_err(&client->dev, "devm_iio_device_alloc failed\n");
+ return -ENOMEM;
+ }
+
+ chip = iio_priv(indio_dev);
+ i2c_set_clientdata(client, indio_dev);
+ chip->client = client;
+
+ mutex_init(&chip->lock);
+ indio_dev->dev.parent = &client->dev;
+ indio_dev->channels = cm3232_channels;
+ indio_dev->num_channels = ARRAY_SIZE(cm3232_channels);
+ indio_dev->info = &cm3232_info;
+ if (id && id->name)
+ indio_dev->name = id->name;
+ else
+ indio_dev->name = (char *)dev_name(&client->dev);
+ indio_dev->modes = INDIO_DIRECT_MODE;
+
+ ret = cm3232_reg_init(chip);
+ if (ret) {
+ dev_err(&client->dev,
+ "%s: register init failed\n",
+ __func__);
+ return ret;
+ }
+
+ ret = iio_device_register(indio_dev);
+ return 0;
+}
+
+static int cm3232_remove(struct i2c_client *client)
+{
+ struct iio_dev *indio_dev = i2c_get_clientdata(client);
+
+ iio_device_unregister(indio_dev);
+ return 0;
+}
+
+static const struct i2c_device_id cm3232_id[] = {
+ { "cm3232", 0},
+ { }
+};
+
+MODULE_DEVICE_TABLE(i2c, cm3232_id);
+
+static const struct of_device_id cm3232_of_match[] = {
+ { .compatible = "capella,cm3232" },
+ { }
+};
+
+static struct i2c_driver cm3232_driver = {
+ .driver = {
+ .name = "cm3232",
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(cm3232_of_match),
+ },
+ .id_table = cm3232_id,
+ .probe = cm3232_probe,
+ .remove = cm3232_remove,
+};
+
+module_i2c_driver(cm3232_driver);
+
+MODULE_AUTHOR("Kevin Tsai <ktsai@capellamicro.com>");
+MODULE_DESCRIPTION("CM3232 ambient light sensor driver");
+MODULE_LICENSE("GPL");
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH V1 1/1] iio: Added Capella cm3232 ambient light sensor driver.
@ 2015-01-01 0:10 ` Kevin Tsai
0 siblings, 0 replies; 21+ messages in thread
From: Kevin Tsai @ 2015-01-01 0:10 UTC (permalink / raw)
To: Kevin Tsai, Wolfram Sang, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald, Grant Likely, Andrew Morton,
David S. Miller, Greg Kroah-Hartman, Joe Perches,
Mauro Carvalho Chehab, Antti Palosaari, Daniel Baluta,
Archana Patni
Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
CM3232 is an advanced ambient light sensor with I2C protocol interface.
The I2C slave address is internally hardwired as 0x10 (7-bit). Writing
to configure register is byte mode, but reading ALS register requests to
use word mode for 16-bit resolution.
Signed-off-by: Kevin Tsai <ktsai-GubuWUlQtMwciDkP5Hr2oA@public.gmane.org>
---
.../devicetree/bindings/i2c/trivial-devices.txt | 1 +
MAINTAINERS | 6 +
drivers/iio/light/Kconfig | 11 +
drivers/iio/light/Makefile | 1 +
drivers/iio/light/cm3232.c | 403 +++++++++++++++++++++
5 files changed, 422 insertions(+)
create mode 100644 drivers/iio/light/cm3232.c
diff --git a/Documentation/devicetree/bindings/i2c/trivial-devices.txt b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
index 9f4e382..572a7c4 100644
--- a/Documentation/devicetree/bindings/i2c/trivial-devices.txt
+++ b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
@@ -34,6 +34,7 @@ atmel,24c512 i2c serial eeprom (24cxx)
atmel,24c1024 i2c serial eeprom (24cxx)
atmel,at97sc3204t i2c trusted platform module (TPM)
capella,cm32181 CM32181: Ambient Light Sensor
+capella,cm3232 CM3232: Ambient Light Sensor
catalyst,24c32 i2c serial eeprom
cirrus,cs42l51 Cirrus Logic CS42L51 audio codec
dallas,ds1307 64 x 8, Serial, I2C Real-Time Clock
diff --git a/MAINTAINERS b/MAINTAINERS
index ddb9ac8..06a613a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2378,6 +2378,12 @@ F: security/capability.c
F: security/commoncap.c
F: kernel/capability.c
+CAPELLA MICROSYSTEMS LIGHT SENSOR DRIVER
+M: Kevin Tsai <ktsai-GubuWUlQtMwciDkP5Hr2oA@public.gmane.org>
+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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
L: linux-wpan-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 5bea821..d2318e2 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -48,6 +48,17 @@ config CM32181
To compile this driver as a module, choose M here:
the module will be called cm32181.
+config CM3232
+ depends on I2C
+ tristate "CM3232 driver"
+ help
+ Say Y here if you use cm3232.
+ This option enables ambient light sensor using
+ Capella Microsystems cm3232 device driver.
+
+ To compile this driver as a module, choose M here:
+ the module will be called cm3232.
+
config CM36651
depends on I2C
tristate "CM36651 driver"
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index 47877a3..f2c8d55 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_ADJD_S311) += adjd_s311.o
obj-$(CONFIG_AL3320A) += al3320a.o
obj-$(CONFIG_APDS9300) += apds9300.o
obj-$(CONFIG_CM32181) += cm32181.o
+obj-$(CONFIG_CM3232) += cm3232.o
obj-$(CONFIG_CM36651) += cm36651.o
obj-$(CONFIG_GP2AP020A00F) += gp2ap020a00f.o
obj-$(CONFIG_HID_SENSOR_ALS) += hid-sensor-als.o
diff --git a/drivers/iio/light/cm3232.c b/drivers/iio/light/cm3232.c
new file mode 100644
index 0000000..fd98eeb
--- /dev/null
+++ b/drivers/iio/light/cm3232.c
@@ -0,0 +1,403 @@
+/*
+ * Copyright (C) 2014 Capella Microsystems Inc.
+ * Author: Kevin Tsai <ktsai-GubuWUlQtMwciDkP5Hr2oA@public.gmane.org>
+ *
+ * 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.
+ *
+ */
+
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/mutex.h>
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/regulator/consumer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/events.h>
+#include <linux/init.h>
+
+/* Registers Address */
+#define CM3232_REG_ADDR_CMD 0x00
+#define CM3232_REG_ADDR_ALS 0x50
+#define CM3232_REG_ADDR_ID 0x53
+
+/* CMD register */
+#define CM3232_CMD_ALS_DISABLE BIT(0)
+#define CM3232_CMD_ALS_HS BIT(1)
+
+#define CM3232_CMD_ALS_IT_SHIFT 2
+#define CM3232_CMD_ALS_IT_MASK (0x07 << CM3232_CMD_ALS_IT_SHIFT)
+#define CM3232_CMD_ALS_IT_DEFAULT (0x01 << CM3232_CMD_ALS_IT_SHIFT)
+
+#define CM3232_CMD_ALS_RESET BIT(6)
+
+#define CM3232_CMD_DEFAULT CM3232_CMD_ALS_IT_DEFAULT
+
+#define CM3232_CALIBSCALE_DEFAULT 100000
+#define CM3232_CALIBSCALE_RESOLUTION 100000
+#define CM3232_MLUX_PER_LUX 1000
+
+#define CM3232_MLUX_PER_BIT_DEFAULT 64
+#define CM3232_MLUX_PER_BIT_BASE_IT 100000
+static const int CM3232_als_it_bits[] = { 0, 1, 2, 3, 4, 5};
+static const int CM3232_als_it_values[] = {
+ 100000, 200000, 400000, 800000, 1600000, 3200000};
+
+struct cm3232_als_info {
+ u32 id;
+ int calibscale;
+ int mlux_per_bit;
+ int mlux_per_bit_base_it;
+ const int *als_it_bits;
+ const int *als_it_values;
+ const int num_als_it;
+ int als_raw;
+};
+
+static struct cm3232_als_info cm3232_als_info_default = {
+ .id = 3232,
+ .calibscale = CM3232_CALIBSCALE_DEFAULT,
+ .mlux_per_bit = CM3232_MLUX_PER_BIT_DEFAULT,
+ .mlux_per_bit_base_it = CM3232_MLUX_PER_BIT_BASE_IT,
+ .als_it_bits = CM3232_als_it_bits,
+ .als_it_values = CM3232_als_it_values,
+ .num_als_it = ARRAY_SIZE(CM3232_als_it_bits),
+};
+
+struct cm3232_chip {
+ struct i2c_client *client;
+ struct mutex lock;
+ struct cm3232_als_info *als_info;
+ u8 regs_cmd;
+};
+
+static int cm3232_get_lux(struct cm3232_chip *chip);
+static int cm3232_read_als_it(struct cm3232_chip *chip, int *val2);
+
+/**
+ * cm3232_reg_init() - Initialize CM3232 registers
+ * @chip: pointer of struct cm3232.
+ *
+ * Initialize CM3232 ambient light sensor register to default values.
+ *
+ Return: 0 for success; otherwise for error code.
+ */
+static int cm3232_reg_init(struct cm3232_chip *chip)
+{
+ struct i2c_client *client = chip->client;
+ struct cm3232_als_info *als_info;
+ s32 ret;
+
+ /* Identify device */
+ ret = i2c_smbus_read_word_data(client, CM3232_REG_ADDR_ID);
+ if (ret < 0)
+ return ret;
+ if ((ret & 0xFF) != 0x32)
+ return -ENODEV;
+
+ /* Disable and reset device */
+ chip->regs_cmd = CM3232_CMD_ALS_DISABLE | CM3232_CMD_ALS_RESET;
+ ret = i2c_smbus_write_byte_data(client, CM3232_REG_ADDR_CMD,
+ chip->regs_cmd);
+ if (ret < 0)
+ return ret;
+
+ /* Initialization */
+ als_info = chip->als_info = &cm3232_als_info_default;
+ chip->regs_cmd = CM3232_CMD_DEFAULT;
+
+ /* Configure register */
+ ret = i2c_smbus_write_byte_data(client, CM3232_REG_ADDR_CMD,
+ chip->regs_cmd);
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+/**
+ * cm3232_read_als_it() - Get sensor integration time (ms)
+ * @chip: pointer of struct cm3232
+ * @val2: pointer of int to load the als_it value.
+ *
+ * Report the current integration time in milliseconds.
+ *
+ * Return: IIO_VAL_INT_PLUS_MICRO for success, otherwise -EINVAL.
+ */
+static int cm3232_read_als_it(struct cm3232_chip *chip, int *val2)
+{
+ struct cm3232_als_info *als_info = chip->als_info;
+ u16 als_it;
+ int i;
+
+ als_it = chip->regs_cmd;
+ als_it &= CM3232_CMD_ALS_IT_MASK;
+ als_it >>= CM3232_CMD_ALS_IT_SHIFT;
+ for (i = 0; i < als_info->num_als_it; i++) {
+ if (als_it == als_info->als_it_bits[i]) {
+ *val2 = als_info->als_it_values[i];
+ return IIO_VAL_INT_PLUS_MICRO;
+ }
+ }
+
+ return -EINVAL;
+}
+
+/**
+ * cm3232_write_als_it() - Write sensor integration time
+ * @chip: pointer of struct cm3232.
+ * @val: integration time in milliseconds.
+ *
+ * Convert integration time (ms) to sensor value.
+ *
+ * Return: i2c_smbus_write_word_data command return value.
+ */
+static int cm3232_write_als_it(struct cm3232_chip *chip, int val)
+{
+ struct i2c_client *client = chip->client;
+ struct cm3232_als_info *als_info = chip->als_info;
+ u16 als_it;
+ int ret, i;
+
+ for (i = 0; i < als_info->num_als_it; i++)
+ if (val <= als_info->als_it_values[i])
+ break;
+ if (i >= als_info->num_als_it)
+ i = als_info->num_als_it - 1;
+
+ als_it = als_info->als_it_bits[i];
+ als_it <<= CM3232_CMD_ALS_IT_SHIFT;
+
+ mutex_lock(&chip->lock);
+ chip->regs_cmd &= ~CM3232_CMD_ALS_IT_MASK;
+ chip->regs_cmd |= als_it;
+ ret = i2c_smbus_write_byte_data(client, CM3232_REG_ADDR_CMD,
+ chip->regs_cmd);
+ mutex_unlock(&chip->lock);
+
+ return ret;
+}
+
+/**
+ * cm3232_get_lux() - report current lux value
+ * @chip: pointer of struct cm3232.
+ *
+ * Convert sensor raw data to lux. It depends on integration
+ * time and calibscale variable.
+ *
+ * Return: Positive value is lux, otherwise is error code.
+ */
+static int cm3232_get_lux(struct cm3232_chip *chip)
+{
+ struct i2c_client *client = chip->client;
+ struct cm3232_als_info *als_info = chip->als_info;
+ int ret;
+ int als_it;
+ u64 tmp;
+
+ /* Calculate mlux per bit based on als_it */
+ ret = cm3232_read_als_it(chip, &als_it);
+ if (ret < 0)
+ return -EINVAL;
+ tmp = (__force u64)als_info->mlux_per_bit;
+ tmp *= als_info->mlux_per_bit_base_it;
+ tmp = div_u64 (tmp, als_it);
+
+ /* Get als_raw */
+ als_info->als_raw = i2c_smbus_read_word_data(
+ client,
+ CM3232_REG_ADDR_ALS);
+ if (als_info->als_raw < 0)
+ return als_info->als_raw;
+
+ tmp *= als_info->als_raw;
+ tmp *= als_info->calibscale;
+ tmp = div_u64(tmp, CM3232_CALIBSCALE_RESOLUTION);
+ tmp = div_u64(tmp, CM3232_MLUX_PER_LUX);
+
+ if (tmp > 0xFFFF)
+ tmp = 0xFFFF;
+
+ return (int)tmp;
+}
+
+static int cm3232_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct cm3232_chip *chip = iio_priv(indio_dev);
+ struct cm3232_als_info *als_info = chip->als_info;
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_PROCESSED:
+ ret = cm3232_get_lux(chip);
+ if (ret < 0)
+ return ret;
+ *val = ret;
+ return IIO_VAL_INT;
+ case IIO_CHAN_INFO_CALIBSCALE:
+ *val = als_info->calibscale;
+ return IIO_VAL_INT;
+ case IIO_CHAN_INFO_INT_TIME:
+ *val = 0;
+ ret = cm3232_read_als_it(chip, val2);
+ return ret;
+ }
+
+ return -EINVAL;
+}
+
+static int cm3232_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int val, int val2, long mask)
+{
+ struct cm3232_chip *chip = iio_priv(indio_dev);
+ struct cm3232_als_info *als_info = chip->als_info;
+ long ms;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_CALIBSCALE:
+ als_info->calibscale = val;
+ return val;
+ case IIO_CHAN_INFO_INT_TIME:
+ ms = val * 1000000 + val2;
+ return cm3232_write_als_it(chip, (int)ms);
+ }
+
+ return -EINVAL;
+}
+
+/**
+ * cm3232_get_it_available() - Get available ALS IT value
+ * @dev: pointer of struct device.
+ * @attr: pointer of struct device_attribute.
+ * @buf: pointer of return string buffer.
+ *
+ * Display the available integration time in milliseconds.
+ *
+ * Return: string length.
+ */
+static ssize_t cm3232_get_it_available(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct cm3232_chip *chip = iio_priv(dev_to_iio_dev(dev));
+ struct cm3232_als_info *als_info = chip->als_info;
+ int i, len;
+
+ for (i = 0, len = 0; i < als_info->num_als_it; i++)
+ len += scnprintf(buf + len, PAGE_SIZE - len, "%u.%06u ",
+ als_info->als_it_values[i]/1000000,
+ als_info->als_it_values[i]%1000000);
+ return len + scnprintf(buf + len, PAGE_SIZE - len, "\n");
+}
+
+static const struct iio_chan_spec cm3232_channels[] = {
+ {
+ .type = IIO_LIGHT,
+ .info_mask_separate =
+ BIT(IIO_CHAN_INFO_PROCESSED) |
+ BIT(IIO_CHAN_INFO_CALIBSCALE) |
+ BIT(IIO_CHAN_INFO_INT_TIME),
+ }
+};
+
+static IIO_DEVICE_ATTR(in_illuminance_integration_time_available,
+ S_IRUGO, cm3232_get_it_available, NULL, 0);
+
+static struct attribute *cm3232_attributes[] = {
+ &iio_dev_attr_in_illuminance_integration_time_available.dev_attr.attr,
+ NULL,
+};
+
+static const struct attribute_group cm3232_attribute_group = {
+ .attrs = cm3232_attributes
+};
+
+static const struct iio_info cm3232_info = {
+ .driver_module = THIS_MODULE,
+ .read_raw = &cm3232_read_raw,
+ .write_raw = &cm3232_write_raw,
+ .attrs = &cm3232_attribute_group,
+};
+
+static int cm3232_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct cm3232_chip *chip;
+ struct iio_dev *indio_dev;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
+ if (!indio_dev) {
+ dev_err(&client->dev, "devm_iio_device_alloc failed\n");
+ return -ENOMEM;
+ }
+
+ chip = iio_priv(indio_dev);
+ i2c_set_clientdata(client, indio_dev);
+ chip->client = client;
+
+ mutex_init(&chip->lock);
+ indio_dev->dev.parent = &client->dev;
+ indio_dev->channels = cm3232_channels;
+ indio_dev->num_channels = ARRAY_SIZE(cm3232_channels);
+ indio_dev->info = &cm3232_info;
+ if (id && id->name)
+ indio_dev->name = id->name;
+ else
+ indio_dev->name = (char *)dev_name(&client->dev);
+ indio_dev->modes = INDIO_DIRECT_MODE;
+
+ ret = cm3232_reg_init(chip);
+ if (ret) {
+ dev_err(&client->dev,
+ "%s: register init failed\n",
+ __func__);
+ return ret;
+ }
+
+ ret = iio_device_register(indio_dev);
+ return 0;
+}
+
+static int cm3232_remove(struct i2c_client *client)
+{
+ struct iio_dev *indio_dev = i2c_get_clientdata(client);
+
+ iio_device_unregister(indio_dev);
+ return 0;
+}
+
+static const struct i2c_device_id cm3232_id[] = {
+ { "cm3232", 0},
+ { }
+};
+
+MODULE_DEVICE_TABLE(i2c, cm3232_id);
+
+static const struct of_device_id cm3232_of_match[] = {
+ { .compatible = "capella,cm3232" },
+ { }
+};
+
+static struct i2c_driver cm3232_driver = {
+ .driver = {
+ .name = "cm3232",
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(cm3232_of_match),
+ },
+ .id_table = cm3232_id,
+ .probe = cm3232_probe,
+ .remove = cm3232_remove,
+};
+
+module_i2c_driver(cm3232_driver);
+
+MODULE_AUTHOR("Kevin Tsai <ktsai-GubuWUlQtMwciDkP5Hr2oA@public.gmane.org>");
+MODULE_DESCRIPTION("CM3232 ambient light sensor driver");
+MODULE_LICENSE("GPL");
--
1.8.3.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH V1 1/1] iio: Added Capella cm3232 ambient light sensor driver.
2015-01-01 0:10 ` Kevin Tsai
(?)
@ 2015-01-01 3:38 ` Jeremiah Mahler
-1 siblings, 0 replies; 21+ messages in thread
From: Jeremiah Mahler @ 2015-01-01 3:38 UTC (permalink / raw)
To: Kevin Tsai
Cc: Wolfram Sang, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald, Grant Likely, Andrew Morton,
David S. Miller, Greg Kroah-Hartman, Joe Perches,
Mauro Carvalho Chehab, Antti Palosaari, Daniel Baluta,
Archana Patni, linux-i2c, devicetree, linux-iio, linux-kernel
Kevin,
On Wed, Dec 31, 2014 at 04:10:30PM -0800, Kevin Tsai wrote:
> CM3232 is an advanced ambient light sensor with I2C protocol interface.
> The I2C slave address is internally hardwired as 0x10 (7-bit). Writing
> to configure register is byte mode, but reading ALS register requests to
> use word mode for 16-bit resolution.
>
> Signed-off-by: Kevin Tsai <ktsai@capellamicro.com>
> ---
> .../devicetree/bindings/i2c/trivial-devices.txt | 1 +
> MAINTAINERS | 6 +
> drivers/iio/light/Kconfig | 11 +
[...]
> +static int cm3232_get_lux(struct cm3232_chip *chip);
> +static int cm3232_read_als_it(struct cm3232_chip *chip, int *val2);
> +
> +/**
> + * cm3232_reg_init() - Initialize CM3232 registers
> + * @chip: pointer of struct cm3232.
> + *
> + * Initialize CM3232 ambient light sensor register to default values.
> + *
> + Return: 0 for success; otherwise for error code.
Is a '*' missing?
> + */
> +static int cm3232_reg_init(struct cm3232_chip *chip)
> +{
> + struct i2c_client *client = chip->client;
> + struct cm3232_als_info *als_info;
> + s32 ret;
> +
> + /* Identify device */
[...]
> +
> + /* Calculate mlux per bit based on als_it */
> + ret = cm3232_read_als_it(chip, &als_it);
> + if (ret < 0)
> + return -EINVAL;
> + tmp = (__force u64)als_info->mlux_per_bit;
> + tmp *= als_info->mlux_per_bit_base_it;
> + tmp = div_u64 (tmp, als_it);
^
no space after function
> +
> + /* Get als_raw */
> + als_info->als_raw = i2c_smbus_read_word_data(
> + client,
> + CM3232_REG_ADDR_ALS);
> + if (als_info->als_raw < 0)
> + return als_info->als_raw;
> +
> + tmp *= als_info->als_raw;
> + tmp *= als_info->calibscale;
> + tmp = div_u64(tmp, CM3232_CALIBSCALE_RESOLUTION);
> + tmp = div_u64(tmp, CM3232_MLUX_PER_LUX);
[...]
It builds clean and there are no checkpatch or sparse errors.
Overall it looks good.
--
- Jeremiah Mahler
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V1 1/1] iio: Added Capella cm3232 ambient light sensor driver.
2015-01-01 0:10 ` Kevin Tsai
(?)
(?)
@ 2015-01-01 11:02 ` Peter Meerwald
2015-01-02 4:23 ` Kevin Tsai
-1 siblings, 1 reply; 21+ messages in thread
From: Peter Meerwald @ 2015-01-01 11:02 UTC (permalink / raw)
To: Kevin Tsai
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Daniel Baluta, Archana Patni, linux-iio
On Wed, 31 Dec 2014, Kevin Tsai wrote:
> CM3232 is an advanced ambient light sensor with I2C protocol interface.
> The I2C slave address is internally hardwired as 0x10 (7-bit). Writing
> to configure register is byte mode, but reading ALS register requests to
> use word mode for 16-bit resolution.
comments below
> Signed-off-by: Kevin Tsai <ktsai@capellamicro.com>
> ---
> .../devicetree/bindings/i2c/trivial-devices.txt | 1 +
> MAINTAINERS | 6 +
> drivers/iio/light/Kconfig | 11 +
> drivers/iio/light/Makefile | 1 +
> drivers/iio/light/cm3232.c | 403 +++++++++++++++++++++
> 5 files changed, 422 insertions(+)
> create mode 100644 drivers/iio/light/cm3232.c
>
> diff --git a/Documentation/devicetree/bindings/i2c/trivial-devices.txt b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
> index 9f4e382..572a7c4 100644
> --- a/Documentation/devicetree/bindings/i2c/trivial-devices.txt
> +++ b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
> @@ -34,6 +34,7 @@ atmel,24c512 i2c serial eeprom (24cxx)
> atmel,24c1024 i2c serial eeprom (24cxx)
> atmel,at97sc3204t i2c trusted platform module (TPM)
> capella,cm32181 CM32181: Ambient Light Sensor
> +capella,cm3232 CM3232: Ambient Light Sensor
> catalyst,24c32 i2c serial eeprom
> cirrus,cs42l51 Cirrus Logic CS42L51 audio codec
> dallas,ds1307 64 x 8, Serial, I2C Real-Time Clock
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ddb9ac8..06a613a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2378,6 +2378,12 @@ F: security/capability.c
> F: security/commoncap.c
> F: kernel/capability.c
>
> +CAPELLA MICROSYSTEMS LIGHT SENSOR DRIVER
> +M: Kevin Tsai <ktsai@capellamicro.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@gmail.com>
> L: linux-wpan@vger.kernel.org
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index 5bea821..d2318e2 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -48,6 +48,17 @@ config CM32181
> To compile this driver as a module, choose M here:
> the module will be called cm32181.
>
> +config CM3232
> + depends on I2C
> + tristate "CM3232 driver"
> + help
> + Say Y here if you use cm3232.
> + This option enables ambient light sensor using
> + Capella Microsystems cm3232 device driver.
> +
> + To compile this driver as a module, choose M here:
> + the module will be called cm3232.
> +
> config CM36651
> depends on I2C
> tristate "CM36651 driver"
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index 47877a3..f2c8d55 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_ADJD_S311) += adjd_s311.o
> obj-$(CONFIG_AL3320A) += al3320a.o
> obj-$(CONFIG_APDS9300) += apds9300.o
> obj-$(CONFIG_CM32181) += cm32181.o
> +obj-$(CONFIG_CM3232) += cm3232.o
> obj-$(CONFIG_CM36651) += cm36651.o
> obj-$(CONFIG_GP2AP020A00F) += gp2ap020a00f.o
> obj-$(CONFIG_HID_SENSOR_ALS) += hid-sensor-als.o
> diff --git a/drivers/iio/light/cm3232.c b/drivers/iio/light/cm3232.c
> new file mode 100644
> index 0000000..fd98eeb
> --- /dev/null
> +++ b/drivers/iio/light/cm3232.c
> @@ -0,0 +1,403 @@
> +/*
> + * 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.
> + *
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/mutex.h>
> +#include <linux/module.h>
> +#include <linux/interrupt.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/events.h>
> +#include <linux/init.h>
> +
> +/* Registers Address */
> +#define CM3232_REG_ADDR_CMD 0x00
> +#define CM3232_REG_ADDR_ALS 0x50
> +#define CM3232_REG_ADDR_ID 0x53
> +
> +/* CMD register */
> +#define CM3232_CMD_ALS_DISABLE BIT(0)
> +#define CM3232_CMD_ALS_HS BIT(1)
whitespace issue, replace tab with space
ALS_HS is not used
> +
> +#define CM3232_CMD_ALS_IT_SHIFT 2
> +#define CM3232_CMD_ALS_IT_MASK (0x07 << CM3232_CMD_ALS_IT_SHIFT)
> +#define CM3232_CMD_ALS_IT_DEFAULT (0x01 << CM3232_CMD_ALS_IT_SHIFT)
> +
> +#define CM3232_CMD_ALS_RESET BIT(6)
> +
> +#define CM3232_CMD_DEFAULT CM3232_CMD_ALS_IT_DEFAULT
> +
> +#define CM3232_CALIBSCALE_DEFAULT 100000
> +#define CM3232_CALIBSCALE_RESOLUTION 100000
> +#define CM3232_MLUX_PER_LUX 1000
> +
> +#define CM3232_MLUX_PER_BIT_DEFAULT 64
> +#define CM3232_MLUX_PER_BIT_BASE_IT 100000
> +static const int CM3232_als_it_bits[] = { 0, 1, 2, 3, 4, 5};
cm3232_als_it_bits and remove space before 0
> +static const int CM3232_als_it_values[] = {
> + 100000, 200000, 400000, 800000, 1600000, 3200000};
cm3232_als_it_values
> +
> +struct cm3232_als_info {
that may pay off lateron, currently there is just one chip...
> + u32 id;
> + int calibscale;
> + int mlux_per_bit;
> + int mlux_per_bit_base_it;
> + const int *als_it_bits;
> + const int *als_it_values;
> + const int num_als_it;
num_als_it could be unsigned probably
> + int als_raw;
> +};
> +
> +static struct cm3232_als_info cm3232_als_info_default = {
> + .id = 3232,
> + .calibscale = CM3232_CALIBSCALE_DEFAULT,
> + .mlux_per_bit = CM3232_MLUX_PER_BIT_DEFAULT,
> + .mlux_per_bit_base_it = CM3232_MLUX_PER_BIT_BASE_IT,
> + .als_it_bits = CM3232_als_it_bits,
> + .als_it_values = CM3232_als_it_values,
> + .num_als_it = ARRAY_SIZE(CM3232_als_it_bits),
> +};
> +
> +struct cm3232_chip {
> + struct i2c_client *client;
> + struct mutex lock;
> + struct cm3232_als_info *als_info;
> + u8 regs_cmd;
> +};
> +
> +static int cm3232_get_lux(struct cm3232_chip *chip);
> +static int cm3232_read_als_it(struct cm3232_chip *chip, int *val2);
> +
> +/**
> + * cm3232_reg_init() - Initialize CM3232 registers
> + * @chip: pointer of struct cm3232.
> + *
> + * Initialize CM3232 ambient light sensor register to default values.
checks for and initializes; registers
> + *
> + Return: 0 for success; otherwise for error code.
> + */
> +static int cm3232_reg_init(struct cm3232_chip *chip)
> +{
> + struct i2c_client *client = chip->client;
> + struct cm3232_als_info *als_info;
> + s32 ret;
> +
> + /* Identify device */
> + ret = i2c_smbus_read_word_data(client, CM3232_REG_ADDR_ID);
> + if (ret < 0)
> + return ret;
> + if ((ret & 0xFF) != 0x32)
this could/should use the .id field of _als_info
> + return -ENODEV;
> +
> + /* Disable and reset device */
> + chip->regs_cmd = CM3232_CMD_ALS_DISABLE | CM3232_CMD_ALS_RESET;
> + ret = i2c_smbus_write_byte_data(client, CM3232_REG_ADDR_CMD,
> + chip->regs_cmd);
> + if (ret < 0)
> + return ret;
> +
> + /* Initialization */
> + als_info = chip->als_info = &cm3232_als_info_default;
> + chip->regs_cmd = CM3232_CMD_DEFAULT;
> +
> + /* Configure register */
> + ret = i2c_smbus_write_byte_data(client, CM3232_REG_ADDR_CMD,
> + chip->regs_cmd);
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +/**
> + * cm3232_read_als_it() - Get sensor integration time (ms)
> + * @chip: pointer of struct cm3232
> + * @val2: pointer of int to load the als_it value.
> + *
> + * Report the current integration time in milliseconds.
> + *
> + * Return: IIO_VAL_INT_PLUS_MICRO for success, otherwise -EINVAL.
> + */
> +static int cm3232_read_als_it(struct cm3232_chip *chip, int *val2)
> +{
> + struct cm3232_als_info *als_info = chip->als_info;
> + u16 als_it;
> + int i;
> +
> + als_it = chip->regs_cmd;
> + als_it &= CM3232_CMD_ALS_IT_MASK;
> + als_it >>= CM3232_CMD_ALS_IT_SHIFT;
there is no i2c read here?!
what is regs_cmd? the name seems to be rather inappropriate
> + for (i = 0; i < als_info->num_als_it; i++) {
> + if (als_it == als_info->als_it_bits[i]) {
> + *val2 = als_info->als_it_values[i];
> + return IIO_VAL_INT_PLUS_MICRO;
> + }
> + }
> +
> + return -EINVAL;
> +}
> +
> +/**
> + * cm3232_write_als_it() - Write sensor integration time
> + * @chip: pointer of struct cm3232.
> + * @val: integration time in milliseconds.
> + *
> + * Convert integration time (ms) to sensor value.
> + *
> + * Return: i2c_smbus_write_word_data command return value.
write_byte_data
> + */
> +static int cm3232_write_als_it(struct cm3232_chip *chip, int val)
> +{
> + struct i2c_client *client = chip->client;
> + struct cm3232_als_info *als_info = chip->als_info;
> + u16 als_it;
> + int ret, i;
> +
> + for (i = 0; i < als_info->num_als_it; i++)
> + if (val <= als_info->als_it_values[i])
> + break;
> + if (i >= als_info->num_als_it)
> + i = als_info->num_als_it - 1;
> +
> + als_it = als_info->als_it_bits[i];
> + als_it <<= CM3232_CMD_ALS_IT_SHIFT;
> +
> + mutex_lock(&chip->lock);
I don't see what the mutex is trying to lock -- why?
> + chip->regs_cmd &= ~CM3232_CMD_ALS_IT_MASK;
> + chip->regs_cmd |= als_it;
> + ret = i2c_smbus_write_byte_data(client, CM3232_REG_ADDR_CMD,
> + chip->regs_cmd);
> + mutex_unlock(&chip->lock);
> +
> + return ret;
> +}
> +
> +/**
> + * cm3232_get_lux() - report current lux value
> + * @chip: pointer of struct cm3232.
> + *
> + * Convert sensor raw data to lux. It depends on integration
> + * time and calibscale variable.
> + *
> + * Return: Positive value is lux, otherwise is error code.
otherwise error code
> + */
> +static int cm3232_get_lux(struct cm3232_chip *chip)
> +{
> + struct i2c_client *client = chip->client;
> + struct cm3232_als_info *als_info = chip->als_info;
> + int ret;
> + int als_it;
> + u64 tmp;
> +
> + /* Calculate mlux per bit based on als_it */
> + ret = cm3232_read_als_it(chip, &als_it);
> + if (ret < 0)
> + return -EINVAL;
> + tmp = (__force u64)als_info->mlux_per_bit;
> + tmp *= als_info->mlux_per_bit_base_it;
> + tmp = div_u64 (tmp, als_it);
> +
> + /* Get als_raw */
interesting comment :)
> + als_info->als_raw = i2c_smbus_read_word_data(
> + client,
> + CM3232_REG_ADDR_ALS);
> + if (als_info->als_raw < 0)
> + return als_info->als_raw;
> +
> + tmp *= als_info->als_raw;
> + tmp *= als_info->calibscale;
> + tmp = div_u64(tmp, CM3232_CALIBSCALE_RESOLUTION);
> + tmp = div_u64(tmp, CM3232_MLUX_PER_LUX);
> +
> + if (tmp > 0xFFFF)
> + tmp = 0xFFFF;
> +
> + return (int)tmp;
> +}
> +
> +static int cm3232_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct cm3232_chip *chip = iio_priv(indio_dev);
> + struct cm3232_als_info *als_info = chip->als_info;
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_PROCESSED:
> + ret = cm3232_get_lux(chip);
> + if (ret < 0)
> + return ret;
> + *val = ret;
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_CALIBSCALE:
> + *val = als_info->calibscale;
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_INT_TIME:
> + *val = 0;
> + ret = cm3232_read_als_it(chip, val2);
> + return ret;
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int cm3232_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + struct cm3232_chip *chip = iio_priv(indio_dev);
> + struct cm3232_als_info *als_info = chip->als_info;
> + long ms;
ms is cast into int later
what is ms -- milliseconds or microseconds? the latter I guess; ms is
usually millisecs
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_CALIBSCALE:
> + als_info->calibscale = val;
> + return val;
> + case IIO_CHAN_INFO_INT_TIME:
> + ms = val * 1000000 + val2;
> + return cm3232_write_als_it(chip, (int)ms);
> + }
> +
> + return -EINVAL;
> +}
> +
> +/**
> + * cm3232_get_it_available() - Get available ALS IT value
> + * @dev: pointer of struct device.
> + * @attr: pointer of struct device_attribute.
> + * @buf: pointer of return string buffer.
> + *
> + * Display the available integration time in milliseconds.
> + *
> + * Return: string length.
> + */
> +static ssize_t cm3232_get_it_available(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct cm3232_chip *chip = iio_priv(dev_to_iio_dev(dev));
> + struct cm3232_als_info *als_info = chip->als_info;
> + int i, len;
> +
> + for (i = 0, len = 0; i < als_info->num_als_it; i++)
> + len += scnprintf(buf + len, PAGE_SIZE - len, "%u.%06u ",
> + als_info->als_it_values[i]/1000000,
> + als_info->als_it_values[i]%1000000);
> + return len + scnprintf(buf + len, PAGE_SIZE - len, "\n");
> +}
> +
> +static const struct iio_chan_spec cm3232_channels[] = {
> + {
> + .type = IIO_LIGHT,
> + .info_mask_separate =
> + BIT(IIO_CHAN_INFO_PROCESSED) |
> + BIT(IIO_CHAN_INFO_CALIBSCALE) |
> + BIT(IIO_CHAN_INFO_INT_TIME),
> + }
> +};
> +
> +static IIO_DEVICE_ATTR(in_illuminance_integration_time_available,
> + S_IRUGO, cm3232_get_it_available, NULL, 0);
> +
> +static struct attribute *cm3232_attributes[] = {
> + &iio_dev_attr_in_illuminance_integration_time_available.dev_attr.attr,
> + NULL,
> +};
> +
> +static const struct attribute_group cm3232_attribute_group = {
> + .attrs = cm3232_attributes
> +};
> +
> +static const struct iio_info cm3232_info = {
> + .driver_module = THIS_MODULE,
> + .read_raw = &cm3232_read_raw,
> + .write_raw = &cm3232_write_raw,
> + .attrs = &cm3232_attribute_group,
> +};
> +
> +static int cm3232_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct cm3232_chip *chip;
> + struct iio_dev *indio_dev;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
> + if (!indio_dev) {
> + dev_err(&client->dev, "devm_iio_device_alloc failed\n");
> + return -ENOMEM;
> + }
> +
> + chip = iio_priv(indio_dev);
> + i2c_set_clientdata(client, indio_dev);
> + chip->client = client;
> +
> + mutex_init(&chip->lock);
> + indio_dev->dev.parent = &client->dev;
> + indio_dev->channels = cm3232_channels;
> + indio_dev->num_channels = ARRAY_SIZE(cm3232_channels);
> + indio_dev->info = &cm3232_info;
> + if (id && id->name)
> + indio_dev->name = id->name;
> + else
> + indio_dev->name = (char *)dev_name(&client->dev);
> + indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + ret = cm3232_reg_init(chip);
> + if (ret) {
> + dev_err(&client->dev,
> + "%s: register init failed\n",
> + __func__);
> + return ret;
> + }
> +
> + ret = iio_device_register(indio_dev);
ret not checked
> + return 0;
> +}
> +
> +static int cm3232_remove(struct i2c_client *client)
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +
> + iio_device_unregister(indio_dev);
maybe disable device?
> + return 0;
> +}
> +
> +static const struct i2c_device_id cm3232_id[] = {
> + { "cm3232", 0},
> + { }
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, cm3232_id);
> +
> +static const struct of_device_id cm3232_of_match[] = {
> + { .compatible = "capella,cm3232" },
> + { }
> +};
> +
> +static struct i2c_driver cm3232_driver = {
> + .driver = {
> + .name = "cm3232",
> + .owner = THIS_MODULE,
> + .of_match_table = of_match_ptr(cm3232_of_match),
> + },
> + .id_table = cm3232_id,
> + .probe = cm3232_probe,
> + .remove = cm3232_remove,
> +};
> +
> +module_i2c_driver(cm3232_driver);
> +
> +MODULE_AUTHOR("Kevin Tsai <ktsai@capellamicro.com>");
> +MODULE_DESCRIPTION("CM3232 ambient light sensor driver");
> +MODULE_LICENSE("GPL");
>
--
Peter Meerwald
+43-664-2444418 (mobile)
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V1 1/1] iio: Added Capella cm3232 ambient light sensor driver.
2015-01-01 11:02 ` Peter Meerwald
@ 2015-01-02 4:23 ` Kevin Tsai
0 siblings, 0 replies; 21+ messages in thread
From: Kevin Tsai @ 2015-01-02 4:23 UTC (permalink / raw)
To: Peter Meerwald
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Daniel Baluta, Archana Patni, linux-iio
[-- Attachment #1: Type: text/plain, Size: 19392 bytes --]
Hi Peter,
Thanks for your comments. I will update and resubmit it next week.
About your question:> + als_it = chip->regs_cmd;
> + als_it &= CM3232_CMD_ALS_IT_MASK;
> + als_it >>= CM3232_CMD_ALS_IT_SHIFT;
there is no i2c read here?!
KT: Configure register is "Write only" register. I use "chip->regs_cmd" to store the value.
Thanks.
Kevin Tsai01/01/15
From: Peter Meerwald <pmeerw@pmeerw.net>
To: Kevin Tsai <ktsai@capellamicro.com>
Cc: Jonathan Cameron <jic23@kernel.org>; Hartmut Knaack <knaack.h@gmx.de>; Lars-Peter Clausen <lars@metafoo.de>; Daniel Baluta <daniel.baluta@intel.com>; Archana Patni <archana.patni@linux.intel.com>; linux-iio@vger.kernel.org
Sent: Thursday, January 1, 2015 3:02 AM
Subject: Re: [PATCH V1 1/1] iio: Added Capella cm3232 ambient light sensor driver.
On Wed, 31 Dec 2014, Kevin Tsai wrote:
> CM3232 is an advanced ambient light sensor with I2C protocol interface.
> The I2C slave address is internally hardwired as 0x10 (7-bit). Writing
> to configure register is byte mode, but reading ALS register requests to
> use word mode for 16-bit resolution.
comments below
> Signed-off-by: Kevin Tsai <ktsai@capellamicro.com>
> ---
> .../devicetree/bindings/i2c/trivial-devices.txt | 1 +
> MAINTAINERS | 6 +
> drivers/iio/light/Kconfig | 11 +
> drivers/iio/light/Makefile | 1 +
> drivers/iio/light/cm3232.c | 403 +++++++++++++++++++++
> 5 files changed, 422 insertions(+)
> create mode 100644 drivers/iio/light/cm3232.c
>
> diff --git a/Documentation/devicetree/bindings/i2c/trivial-devices.txt b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
> index 9f4e382..572a7c4 100644
> --- a/Documentation/devicetree/bindings/i2c/trivial-devices.txt
> +++ b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
> @@ -34,6 +34,7 @@ atmel,24c512 i2c serial eeprom (24cxx)
> atmel,24c1024 i2c serial eeprom (24cxx)
> atmel,at97sc3204t i2c trusted platform module (TPM)
> capella,cm32181 CM32181: Ambient Light Sensor
> +capella,cm3232 CM3232: Ambient Light Sensor
> catalyst,24c32 i2c serial eeprom
> cirrus,cs42l51 Cirrus Logic CS42L51 audio codec
> dallas,ds1307 64 x 8, Serial, I2C Real-Time Clock
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ddb9ac8..06a613a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2378,6 +2378,12 @@ F: security/capability.c
> F: security/commoncap.c
> F: kernel/capability.c
>
> +CAPELLA MICROSYSTEMS LIGHT SENSOR DRIVER
> +M: Kevin Tsai <ktsai@capellamicro.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@gmail.com>
> L: linux-wpan@vger.kernel.org
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index 5bea821..d2318e2 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -48,6 +48,17 @@ config CM32181
> To compile this driver as a module, choose M here:
> the module will be called cm32181.
>
> +config CM3232
> + depends on I2C
> + tristate "CM3232 driver"
> + help
> + Say Y here if you use cm3232.
> + This option enables ambient light sensor using
> + Capella Microsystems cm3232 device driver.
> +
> + To compile this driver as a module, choose M here:
> + the module will be called cm3232.
> +
> config CM36651
> depends on I2C
> tristate "CM36651 driver"
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index 47877a3..f2c8d55 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_ADJD_S311) += adjd_s311.o
> obj-$(CONFIG_AL3320A) += al3320a.o
> obj-$(CONFIG_APDS9300) += apds9300.o
> obj-$(CONFIG_CM32181) += cm32181.o
> +obj-$(CONFIG_CM3232) += cm3232.o
> obj-$(CONFIG_CM36651) += cm36651.o
> obj-$(CONFIG_GP2AP020A00F) += gp2ap020a00f.o
> obj-$(CONFIG_HID_SENSOR_ALS) += hid-sensor-als.o
> diff --git a/drivers/iio/light/cm3232.c b/drivers/iio/light/cm3232.c
> new file mode 100644
> index 0000000..fd98eeb
> --- /dev/null
> +++ b/drivers/iio/light/cm3232.c
> @@ -0,0 +1,403 @@
> +/*
> + * 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.
> + *
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/mutex.h>
> +#include <linux/module.h>
> +#include <linux/interrupt.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/events.h>
> +#include <linux/init.h>
> +
> +/* Registers Address */
> +#define CM3232_REG_ADDR_CMD 0x00
> +#define CM3232_REG_ADDR_ALS 0x50
> +#define CM3232_REG_ADDR_ID 0x53
> +
> +/* CMD register */
> +#define CM3232_CMD_ALS_DISABLE BIT(0)
> +#define CM3232_CMD_ALS_HS BIT(1)
whitespace issue, replace tab with space
ALS_HS is not used
> +
> +#define CM3232_CMD_ALS_IT_SHIFT 2
> +#define CM3232_CMD_ALS_IT_MASK (0x07 << CM3232_CMD_ALS_IT_SHIFT)
> +#define CM3232_CMD_ALS_IT_DEFAULT (0x01 << CM3232_CMD_ALS_IT_SHIFT)
> +
> +#define CM3232_CMD_ALS_RESET BIT(6)
> +
> +#define CM3232_CMD_DEFAULT CM3232_CMD_ALS_IT_DEFAULT
> +
> +#define CM3232_CALIBSCALE_DEFAULT 100000
> +#define CM3232_CALIBSCALE_RESOLUTION 100000
> +#define CM3232_MLUX_PER_LUX 1000
> +
> +#define CM3232_MLUX_PER_BIT_DEFAULT 64
> +#define CM3232_MLUX_PER_BIT_BASE_IT 100000
> +static const int CM3232_als_it_bits[] = { 0, 1, 2, 3, 4, 5};
cm3232_als_it_bits and remove space before 0
> +static const int CM3232_als_it_values[] = {
> + 100000, 200000, 400000, 800000, 1600000, 3200000};
cm3232_als_it_values
> +
> +struct cm3232_als_info {
that may pay off lateron, currently there is just one chip...
> + u32 id;
> + int calibscale;
> + int mlux_per_bit;
> + int mlux_per_bit_base_it;
> + const int *als_it_bits;
> + const int *als_it_values;
> + const int num_als_it;
num_als_it could be unsigned probably
> + int als_raw;
> +};
> +
> +static struct cm3232_als_info cm3232_als_info_default = {
> + .id = 3232,
> + .calibscale = CM3232_CALIBSCALE_DEFAULT,
> + .mlux_per_bit = CM3232_MLUX_PER_BIT_DEFAULT,
> + .mlux_per_bit_base_it = CM3232_MLUX_PER_BIT_BASE_IT,
> + .als_it_bits = CM3232_als_it_bits,
> + .als_it_values = CM3232_als_it_values,
> + .num_als_it = ARRAY_SIZE(CM3232_als_it_bits),
> +};
> +
> +struct cm3232_chip {
> + struct i2c_client *client;
> + struct mutex lock;
> + struct cm3232_als_info *als_info;
> + u8 regs_cmd;
> +};
> +
> +static int cm3232_get_lux(struct cm3232_chip *chip);
> +static int cm3232_read_als_it(struct cm3232_chip *chip, int *val2);
> +
> +/**
> + * cm3232_reg_init() - Initialize CM3232 registers
> + * @chip: pointer of struct cm3232.
> + *
> + * Initialize CM3232 ambient light sensor register to default values.
checks for and initializes; registers
> + *
> + Return: 0 for success; otherwise for error code.
> + */
> +static int cm3232_reg_init(struct cm3232_chip *chip)
> +{
> + struct i2c_client *client = chip->client;
> + struct cm3232_als_info *als_info;
> + s32 ret;
> +
> + /* Identify device */
> + ret = i2c_smbus_read_word_data(client, CM3232_REG_ADDR_ID);
> + if (ret < 0)
> + return ret;
> + if ((ret & 0xFF) != 0x32)
this could/should use the .id field of _als_info
> + return -ENODEV;
> +
> + /* Disable and reset device */
> + chip->regs_cmd = CM3232_CMD_ALS_DISABLE | CM3232_CMD_ALS_RESET;
> + ret = i2c_smbus_write_byte_data(client, CM3232_REG_ADDR_CMD,
> + chip->regs_cmd);
> + if (ret < 0)
> + return ret;
> +
> + /* Initialization */
> + als_info = chip->als_info = &cm3232_als_info_default;
> + chip->regs_cmd = CM3232_CMD_DEFAULT;
> +
> + /* Configure register */
> + ret = i2c_smbus_write_byte_data(client, CM3232_REG_ADDR_CMD,
> + chip->regs_cmd);
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +/**
> + * cm3232_read_als_it() - Get sensor integration time (ms)
> + * @chip: pointer of struct cm3232
> + * @val2: pointer of int to load the als_it value.
> + *
> + * Report the current integration time in milliseconds.
> + *
> + * Return: IIO_VAL_INT_PLUS_MICRO for success, otherwise -EINVAL.
> + */
> +static int cm3232_read_als_it(struct cm3232_chip *chip, int *val2)
> +{
> + struct cm3232_als_info *als_info = chip->als_info;
> + u16 als_it;
> + int i;
> +
> + als_it = chip->regs_cmd;
> + als_it &= CM3232_CMD_ALS_IT_MASK;
> + als_it >>= CM3232_CMD_ALS_IT_SHIFT;
there is no i2c read here?!
what is regs_cmd? the name seems to be rather inappropriate
> + for (i = 0; i < als_info->num_als_it; i++) {
> + if (als_it == als_info->als_it_bits[i]) {
> + *val2 = als_info->als_it_values[i];
> + return IIO_VAL_INT_PLUS_MICRO;
> + }
> + }
> +
> + return -EINVAL;
> +}
> +
> +/**
> + * cm3232_write_als_it() - Write sensor integration time
> + * @chip: pointer of struct cm3232.
> + * @val: integration time in milliseconds.
> + *
> + * Convert integration time (ms) to sensor value.
> + *
> + * Return: i2c_smbus_write_word_data command return value.
write_byte_data
> + */
> +static int cm3232_write_als_it(struct cm3232_chip *chip, int val)
> +{
> + struct i2c_client *client = chip->client;
> + struct cm3232_als_info *als_info = chip->als_info;
> + u16 als_it;
> + int ret, i;
> +
> + for (i = 0; i < als_info->num_als_it; i++)
> + if (val <= als_info->als_it_values[i])
> + break;
> + if (i >= als_info->num_als_it)
> + i = als_info->num_als_it - 1;
> +
> + als_it = als_info->als_it_bits[i];
> + als_it <<= CM3232_CMD_ALS_IT_SHIFT;
> +
> + mutex_lock(&chip->lock);
I don't see what the mutex is trying to lock -- why?
> + chip->regs_cmd &= ~CM3232_CMD_ALS_IT_MASK;
> + chip->regs_cmd |= als_it;
> + ret = i2c_smbus_write_byte_data(client, CM3232_REG_ADDR_CMD,
> + chip->regs_cmd);
> + mutex_unlock(&chip->lock);
> +
> + return ret;
> +}
> +
> +/**
> + * cm3232_get_lux() - report current lux value
> + * @chip: pointer of struct cm3232.
> + *
> + * Convert sensor raw data to lux. It depends on integration
> + * time and calibscale variable.
> + *
> + * Return: Positive value is lux, otherwise is error code.
otherwise error code
> + */
> +static int cm3232_get_lux(struct cm3232_chip *chip)
> +{
> + struct i2c_client *client = chip->client;
> + struct cm3232_als_info *als_info = chip->als_info;
> + int ret;
> + int als_it;
> + u64 tmp;
> +
> + /* Calculate mlux per bit based on als_it */
> + ret = cm3232_read_als_it(chip, &als_it);
> + if (ret < 0)
> + return -EINVAL;
> + tmp = (__force u64)als_info->mlux_per_bit;
> + tmp *= als_info->mlux_per_bit_base_it;
> + tmp = div_u64 (tmp, als_it);
> +
> + /* Get als_raw */
interesting comment :)
> + als_info->als_raw = i2c_smbus_read_word_data(
> + client,
> + CM3232_REG_ADDR_ALS);
> + if (als_info->als_raw < 0)
> + return als_info->als_raw;
> +
> + tmp *= als_info->als_raw;
> + tmp *= als_info->calibscale;
> + tmp = div_u64(tmp, CM3232_CALIBSCALE_RESOLUTION);
> + tmp = div_u64(tmp, CM3232_MLUX_PER_LUX);
> +
> + if (tmp > 0xFFFF)
> + tmp = 0xFFFF;
> +
> + return (int)tmp;
> +}
> +
> +static int cm3232_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct cm3232_chip *chip = iio_priv(indio_dev);
> + struct cm3232_als_info *als_info = chip->als_info;
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_PROCESSED:
> + ret = cm3232_get_lux(chip);
> + if (ret < 0)
> + return ret;
> + *val = ret;
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_CALIBSCALE:
> + *val = als_info->calibscale;
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_INT_TIME:
> + *val = 0;
> + ret = cm3232_read_als_it(chip, val2);
> + return ret;
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int cm3232_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + struct cm3232_chip *chip = iio_priv(indio_dev);
> + struct cm3232_als_info *als_info = chip->als_info;
> + long ms;
ms is cast into int later
what is ms -- milliseconds or microseconds? the latter I guess; ms is
usually millisecs
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_CALIBSCALE:
> + als_info->calibscale = val;
> + return val;
> + case IIO_CHAN_INFO_INT_TIME:
> + ms = val * 1000000 + val2;
> + return cm3232_write_als_it(chip, (int)ms);
> + }
> +
> + return -EINVAL;
> +}
> +
> +/**
> + * cm3232_get_it_available() - Get available ALS IT value
> + * @dev: pointer of struct device.
> + * @attr: pointer of struct device_attribute.
> + * @buf: pointer of return string buffer.
> + *
> + * Display the available integration time in milliseconds.
> + *
> + * Return: string length.
> + */
> +static ssize_t cm3232_get_it_available(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct cm3232_chip *chip = iio_priv(dev_to_iio_dev(dev));
> + struct cm3232_als_info *als_info = chip->als_info;
> + int i, len;
> +
> + for (i = 0, len = 0; i < als_info->num_als_it; i++)
> + len += scnprintf(buf + len, PAGE_SIZE - len, "%u.%06u ",
> + als_info->als_it_values[i]/1000000,
> + als_info->als_it_values[i]%1000000);
> + return len + scnprintf(buf + len, PAGE_SIZE - len, "\n");
> +}
> +
> +static const struct iio_chan_spec cm3232_channels[] = {
> + {
> + .type = IIO_LIGHT,
> + .info_mask_separate =
> + BIT(IIO_CHAN_INFO_PROCESSED) |
> + BIT(IIO_CHAN_INFO_CALIBSCALE) |
> + BIT(IIO_CHAN_INFO_INT_TIME),
> + }
> +};
> +
> +static IIO_DEVICE_ATTR(in_illuminance_integration_time_available,
> + S_IRUGO, cm3232_get_it_available, NULL, 0);
> +
> +static struct attribute *cm3232_attributes[] = {
> + &iio_dev_attr_in_illuminance_integration_time_available.dev_attr.attr,
> + NULL,
> +};
> +
> +static const struct attribute_group cm3232_attribute_group = {
> + .attrs = cm3232_attributes
> +};
> +
> +static const struct iio_info cm3232_info = {
> + .driver_module = THIS_MODULE,
> + .read_raw = &cm3232_read_raw,
> + .write_raw = &cm3232_write_raw,
> + .attrs = &cm3232_attribute_group,
> +};
> +
> +static int cm3232_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct cm3232_chip *chip;
> + struct iio_dev *indio_dev;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
> + if (!indio_dev) {
> + dev_err(&client->dev, "devm_iio_device_alloc failed\n");
> + return -ENOMEM;
> + }
> +
> + chip = iio_priv(indio_dev);
> + i2c_set_clientdata(client, indio_dev);
> + chip->client = client;
> +
> + mutex_init(&chip->lock);
> + indio_dev->dev.parent = &client->dev;
> + indio_dev->channels = cm3232_channels;
> + indio_dev->num_channels = ARRAY_SIZE(cm3232_channels);
> + indio_dev->info = &cm3232_info;
> + if (id && id->name)
> + indio_dev->name = id->name;
> + else
> + indio_dev->name = (char *)dev_name(&client->dev);
> + indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + ret = cm3232_reg_init(chip);
> + if (ret) {
> + dev_err(&client->dev,
> + "%s: register init failed\n",
> + __func__);
> + return ret;
> + }
> +
> + ret = iio_device_register(indio_dev);
ret not checked
> + return 0;
> +}
> +
> +static int cm3232_remove(struct i2c_client *client)
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +
> + iio_device_unregister(indio_dev);
maybe disable device?
> + return 0;
> +}
> +
> +static const struct i2c_device_id cm3232_id[] = {
> + { "cm3232", 0},
> + { }
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, cm3232_id);
> +
> +static const struct of_device_id cm3232_of_match[] = {
> + { .compatible = "capella,cm3232" },
> + { }
> +};
> +
> +static struct i2c_driver cm3232_driver = {
> + .driver = {
> + .name = "cm3232",
> + .owner = THIS_MODULE,
> + .of_match_table = of_match_ptr(cm3232_of_match),
> + },
> + .id_table = cm3232_id,
> + .probe = cm3232_probe,
> + .remove = cm3232_remove,
> +};
> +
> +module_i2c_driver(cm3232_driver);
> +
> +MODULE_AUTHOR("Kevin Tsai <ktsai@capellamicro.com>");
> +MODULE_DESCRIPTION("CM3232 ambient light sensor driver");
> +MODULE_LICENSE("GPL");
>
--
Peter Meerwald
+43-664-2444418 (mobile)
[-- Attachment #2: Type: text/html, Size: 31392 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V1 1/1] iio: Added Capella cm3232 ambient light sensor driver.
2015-01-01 0:10 ` Kevin Tsai
@ 2015-01-05 10:51 ` Daniel Baluta
-1 siblings, 0 replies; 21+ messages in thread
From: Daniel Baluta @ 2015-01-05 10:51 UTC (permalink / raw)
To: Kevin Tsai
Cc: Wolfram Sang, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald, Grant Likely, Andrew Morton,
David S. Miller, Greg Kroah-Hartman, Joe Perches,
Mauro Carvalho Chehab, Antti Palosaari, Daniel Baluta,
Archana Patni, linux-i2c, devicetree, linux-iio,
Linux Kernel Mailing List
On Thu, Jan 1, 2015 at 2:10 AM, Kevin Tsai <ktsai@capellamicro.com> wrote:
> CM3232 is an advanced ambient light sensor with I2C protocol interface.
> The I2C slave address is internally hardwired as 0x10 (7-bit). Writing
> to configure register is byte mode, but reading ALS register requests to
> use word mode for 16-bit resolution.
This looks good to me. Few comments inline.
>
> Signed-off-by: Kevin Tsai <ktsai@capellamicro.com>
> ---
> .../devicetree/bindings/i2c/trivial-devices.txt | 1 +
> MAINTAINERS | 6 +
> drivers/iio/light/Kconfig | 11 +
> drivers/iio/light/Makefile | 1 +
> drivers/iio/light/cm3232.c | 403 +++++++++++++++++++++
> 5 files changed, 422 insertions(+)
> create mode 100644 drivers/iio/light/cm3232.c
>
> diff --git a/Documentation/devicetree/bindings/i2c/trivial-devices.txt b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
> index 9f4e382..572a7c4 100644
> --- a/Documentation/devicetree/bindings/i2c/trivial-devices.txt
> +++ b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
> @@ -34,6 +34,7 @@ atmel,24c512 i2c serial eeprom (24cxx)
> atmel,24c1024 i2c serial eeprom (24cxx)
> atmel,at97sc3204t i2c trusted platform module (TPM)
> capella,cm32181 CM32181: Ambient Light Sensor
> +capella,cm3232 CM3232: Ambient Light Sensor
> catalyst,24c32 i2c serial eeprom
> cirrus,cs42l51 Cirrus Logic CS42L51 audio codec
> dallas,ds1307 64 x 8, Serial, I2C Real-Time Clock
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ddb9ac8..06a613a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2378,6 +2378,12 @@ F: security/capability.c
> F: security/commoncap.c
> F: kernel/capability.c
>
> +CAPELLA MICROSYSTEMS LIGHT SENSOR DRIVER
> +M: Kevin Tsai <ktsai@capellamicro.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@gmail.com>
> L: linux-wpan@vger.kernel.org
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index 5bea821..d2318e2 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -48,6 +48,17 @@ config CM32181
> To compile this driver as a module, choose M here:
> the module will be called cm32181.
>
> +config CM3232
> + depends on I2C
> + tristate "CM3232 driver"
Better name this: "CM3232 ambient light sensor".
> + help
> + Say Y here if you use cm3232.
> + This option enables ambient light sensor using
> + Capella Microsystems cm3232 device driver.
> +
> + To compile this driver as a module, choose M here:
> + the module will be called cm3232.
> +
> config CM36651
> depends on I2C
> tristate "CM36651 driver"
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index 47877a3..f2c8d55 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_ADJD_S311) += adjd_s311.o
> obj-$(CONFIG_AL3320A) += al3320a.o
> obj-$(CONFIG_APDS9300) += apds9300.o
> obj-$(CONFIG_CM32181) += cm32181.o
> +obj-$(CONFIG_CM3232) += cm3232.o
> obj-$(CONFIG_CM36651) += cm36651.o
> obj-$(CONFIG_GP2AP020A00F) += gp2ap020a00f.o
> obj-$(CONFIG_HID_SENSOR_ALS) += hid-sensor-als.o
> diff --git a/drivers/iio/light/cm3232.c b/drivers/iio/light/cm3232.c
> new file mode 100644
> index 0000000..fd98eeb
> --- /dev/null
> +++ b/drivers/iio/light/cm3232.c
> @@ -0,0 +1,403 @@
> +/*
> + * 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.
> + *
Usually, here is the place to mention the I2C slave address.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/mutex.h>
> +#include <linux/module.h>
> +#include <linux/interrupt.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/events.h>
> +#include <linux/init.h>
> +
> +/* Registers Address */
> +#define CM3232_REG_ADDR_CMD 0x00
> +#define CM3232_REG_ADDR_ALS 0x50
> +#define CM3232_REG_ADDR_ID 0x53
> +
> +/* CMD register */
> +#define CM3232_CMD_ALS_DISABLE BIT(0)
> +#define CM3232_CMD_ALS_HS BIT(1)
> +
> +#define CM3232_CMD_ALS_IT_SHIFT 2
> +#define CM3232_CMD_ALS_IT_MASK (0x07 << CM3232_CMD_ALS_IT_SHIFT)
> +#define CM3232_CMD_ALS_IT_DEFAULT (0x01 << CM3232_CMD_ALS_IT_SHIFT)
> +
> +#define CM3232_CMD_ALS_RESET BIT(6)
> +
> +#define CM3232_CMD_DEFAULT CM3232_CMD_ALS_IT_DEFAULT
> +
> +#define CM3232_CALIBSCALE_DEFAULT 100000
> +#define CM3232_CALIBSCALE_RESOLUTION 100000
> +#define CM3232_MLUX_PER_LUX 1000
> +
> +#define CM3232_MLUX_PER_BIT_DEFAULT 64
> +#define CM3232_MLUX_PER_BIT_BASE_IT 100000
> +static const int CM3232_als_it_bits[] = { 0, 1, 2, 3, 4, 5};
> +static const int CM3232_als_it_values[] = {
> + 100000, 200000, 400000, 800000, 1600000, 3200000};
> +
> +struct cm3232_als_info {
> + u32 id;
> + int calibscale;
> + int mlux_per_bit;
> + int mlux_per_bit_base_it;
> + const int *als_it_bits;
> + const int *als_it_values;
> + const int num_als_it;
> + int als_raw;
> +};
> +
> +static struct cm3232_als_info cm3232_als_info_default = {
> + .id = 3232,
> + .calibscale = CM3232_CALIBSCALE_DEFAULT,
> + .mlux_per_bit = CM3232_MLUX_PER_BIT_DEFAULT,
> + .mlux_per_bit_base_it = CM3232_MLUX_PER_BIT_BASE_IT,
> + .als_it_bits = CM3232_als_it_bits,
> + .als_it_values = CM3232_als_it_values,
> + .num_als_it = ARRAY_SIZE(CM3232_als_it_bits),
> +};
> +
> +struct cm3232_chip {
> + struct i2c_client *client;
> + struct mutex lock;
> + struct cm3232_als_info *als_info;
> + u8 regs_cmd;
> +};
> +
> +static int cm3232_get_lux(struct cm3232_chip *chip);
> +static int cm3232_read_als_it(struct cm3232_chip *chip, int *val2);
> +
> +/**
> + * cm3232_reg_init() - Initialize CM3232 registers
> + * @chip: pointer of struct cm3232.
> + *
> + * Initialize CM3232 ambient light sensor register to default values.
> + *
> + Return: 0 for success; otherwise for error code.
> + */
> +static int cm3232_reg_init(struct cm3232_chip *chip)
> +{
> + struct i2c_client *client = chip->client;
> + struct cm3232_als_info *als_info;
> + s32 ret;
> +
> + /* Identify device */
> + ret = i2c_smbus_read_word_data(client, CM3232_REG_ADDR_ID);
> + if (ret < 0)
> + return ret;
> + if ((ret & 0xFF) != 0x32)
> + return -ENODEV;
> +
> + /* Disable and reset device */
> + chip->regs_cmd = CM3232_CMD_ALS_DISABLE | CM3232_CMD_ALS_RESET;
> + ret = i2c_smbus_write_byte_data(client, CM3232_REG_ADDR_CMD,
> + chip->regs_cmd);
> + if (ret < 0)
> + return ret;
> +
> + /* Initialization */
> + als_info = chip->als_info = &cm3232_als_info_default;
> + chip->regs_cmd = CM3232_CMD_DEFAULT;
> +
> + /* Configure register */
> + ret = i2c_smbus_write_byte_data(client, CM3232_REG_ADDR_CMD,
> + chip->regs_cmd);
You could directly return i2c_smbus_write_byte_data(..).
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +/**
> + * cm3232_read_als_it() - Get sensor integration time (ms)
> + * @chip: pointer of struct cm3232
> + * @val2: pointer of int to load the als_it value.
> + *
> + * Report the current integration time in milliseconds.
> + *
> + * Return: IIO_VAL_INT_PLUS_MICRO for success, otherwise -EINVAL.
> + */
> +static int cm3232_read_als_it(struct cm3232_chip *chip, int *val2)
Wouldn't make more sense to also add val here?
> +{
> + struct cm3232_als_info *als_info = chip->als_info;
> + u16 als_it;
> + int i;
> +
> + als_it = chip->regs_cmd;
> + als_it &= CM3232_CMD_ALS_IT_MASK;
> + als_it >>= CM3232_CMD_ALS_IT_SHIFT;
> + for (i = 0; i < als_info->num_als_it; i++) {
> + if (als_it == als_info->als_it_bits[i]) {
.. then make, *val = 0.
> + *val2 = als_info->als_it_values[i];
> + return IIO_VAL_INT_PLUS_MICRO;
> + }
> + }
> +
> + return -EINVAL;
> +}
> +
> +/**
> + * cm3232_write_als_it() - Write sensor integration time
> + * @chip: pointer of struct cm3232.
> + * @val: integration time in milliseconds.
> + *
> + * Convert integration time (ms) to sensor value.
> + *
> + * Return: i2c_smbus_write_word_data command return value.
> + */
> +static int cm3232_write_als_it(struct cm3232_chip *chip, int val)
> +{
> + struct i2c_client *client = chip->client;
> + struct cm3232_als_info *als_info = chip->als_info;
> + u16 als_it;
> + int ret, i;
> +
> + for (i = 0; i < als_info->num_als_it; i++)
> + if (val <= als_info->als_it_values[i])
> + break;
> + if (i >= als_info->num_als_it)
> + i = als_info->num_als_it - 1;
> +
> + als_it = als_info->als_it_bits[i];
> + als_it <<= CM3232_CMD_ALS_IT_SHIFT;
> +
> + mutex_lock(&chip->lock);
> + chip->regs_cmd &= ~CM3232_CMD_ALS_IT_MASK;
> + chip->regs_cmd |= als_it;
> + ret = i2c_smbus_write_byte_data(client, CM3232_REG_ADDR_CMD,
> + chip->regs_cmd);
> + mutex_unlock(&chip->lock);
> +
> + return ret;
> +}
> +
> +/**
> + * cm3232_get_lux() - report current lux value
> + * @chip: pointer of struct cm3232.
> + *
> + * Convert sensor raw data to lux. It depends on integration
> + * time and calibscale variable.
> + *
> + * Return: Positive value is lux, otherwise is error code.
> + */
> +static int cm3232_get_lux(struct cm3232_chip *chip)
> +{
> + struct i2c_client *client = chip->client;
> + struct cm3232_als_info *als_info = chip->als_info;
> + int ret;
> + int als_it;
> + u64 tmp;
> +
> + /* Calculate mlux per bit based on als_it */
> + ret = cm3232_read_als_it(chip, &als_it);
> + if (ret < 0)
> + return -EINVAL;
> + tmp = (__force u64)als_info->mlux_per_bit;
> + tmp *= als_info->mlux_per_bit_base_it;
> + tmp = div_u64 (tmp, als_it);
> +
> + /* Get als_raw */
> + als_info->als_raw = i2c_smbus_read_word_data(
> + client,
> + CM3232_REG_ADDR_ALS);
> + if (als_info->als_raw < 0)
> + return als_info->als_raw;
> +
> + tmp *= als_info->als_raw;
> + tmp *= als_info->calibscale;
> + tmp = div_u64(tmp, CM3232_CALIBSCALE_RESOLUTION);
> + tmp = div_u64(tmp, CM3232_MLUX_PER_LUX);
> +
> + if (tmp > 0xFFFF)
> + tmp = 0xFFFF;
> +
> + return (int)tmp;
> +}
> +
> +static int cm3232_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct cm3232_chip *chip = iio_priv(indio_dev);
> + struct cm3232_als_info *als_info = chip->als_info;
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_PROCESSED:
> + ret = cm3232_get_lux(chip);
> + if (ret < 0)
> + return ret;
> + *val = ret;
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_CALIBSCALE:
> + *val = als_info->calibscale;
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_INT_TIME:
> + *val = 0;
> + ret = cm3232_read_als_it(chip, val2);
> + return ret;
Here you could just return cm3232_read_als_it(...)
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int cm3232_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + struct cm3232_chip *chip = iio_priv(indio_dev);
> + struct cm3232_als_info *als_info = chip->als_info;
> + long ms;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_CALIBSCALE:
> + als_info->calibscale = val;
> + return val;
> + case IIO_CHAN_INFO_INT_TIME:
> + ms = val * 1000000 + val2;
> + return cm3232_write_als_it(chip, (int)ms);
> + }
> +
> + return -EINVAL;
> +}
> +
> +/**
> + * cm3232_get_it_available() - Get available ALS IT value
> + * @dev: pointer of struct device.
> + * @attr: pointer of struct device_attribute.
> + * @buf: pointer of return string buffer.
> + *
> + * Display the available integration time in milliseconds.
> + *
> + * Return: string length.
> + */
> +static ssize_t cm3232_get_it_available(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct cm3232_chip *chip = iio_priv(dev_to_iio_dev(dev));
> + struct cm3232_als_info *als_info = chip->als_info;
> + int i, len;
> +
> + for (i = 0, len = 0; i < als_info->num_als_it; i++)
> + len += scnprintf(buf + len, PAGE_SIZE - len, "%u.%06u ",
> + als_info->als_it_values[i]/1000000,
> + als_info->als_it_values[i]%1000000);
> + return len + scnprintf(buf + len, PAGE_SIZE - len, "\n");
> +}
> +
> +static const struct iio_chan_spec cm3232_channels[] = {
> + {
> + .type = IIO_LIGHT,
> + .info_mask_separate =
> + BIT(IIO_CHAN_INFO_PROCESSED) |
> + BIT(IIO_CHAN_INFO_CALIBSCALE) |
> + BIT(IIO_CHAN_INFO_INT_TIME),
> + }
> +};
> +
> +static IIO_DEVICE_ATTR(in_illuminance_integration_time_available,
> + S_IRUGO, cm3232_get_it_available, NULL, 0);
> +
> +static struct attribute *cm3232_attributes[] = {
> + &iio_dev_attr_in_illuminance_integration_time_available.dev_attr.attr,
> + NULL,
> +};
> +
> +static const struct attribute_group cm3232_attribute_group = {
> + .attrs = cm3232_attributes
> +};
> +
> +static const struct iio_info cm3232_info = {
> + .driver_module = THIS_MODULE,
> + .read_raw = &cm3232_read_raw,
> + .write_raw = &cm3232_write_raw,
> + .attrs = &cm3232_attribute_group,
> +};
> +
> +static int cm3232_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct cm3232_chip *chip;
> + struct iio_dev *indio_dev;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
> + if (!indio_dev) {
> + dev_err(&client->dev, "devm_iio_device_alloc failed\n");
There is no need for this error message. devm_iio_device_alloc will print
an error message in case of failure.
> + return -ENOMEM;
> + }
> +
> + chip = iio_priv(indio_dev);
> + i2c_set_clientdata(client, indio_dev);
> + chip->client = client;
> +
> + mutex_init(&chip->lock);
> + indio_dev->dev.parent = &client->dev;
> + indio_dev->channels = cm3232_channels;
> + indio_dev->num_channels = ARRAY_SIZE(cm3232_channels);
> + indio_dev->info = &cm3232_info;
> + if (id && id->name)
> + indio_dev->name = id->name;
> + else
> + indio_dev->name = (char *)dev_name(&client->dev);
> + indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + ret = cm3232_reg_init(chip);
> + if (ret) {
> + dev_err(&client->dev,
> + "%s: register init failed\n",
> + __func__);
> + return ret;
> + }
> +
> + ret = iio_device_register(indio_dev);
> + return 0;
Directly return iio_device_register()
> +}
> +
> +static int cm3232_remove(struct i2c_client *client)
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +
> + iio_device_unregister(indio_dev);
> + return 0;
> +}
> +
> +static const struct i2c_device_id cm3232_id[] = {
> + { "cm3232", 0},
No need for space before ".
> + { }
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, cm3232_id);
> +
> +static const struct of_device_id cm3232_of_match[] = {
> + { .compatible = "capella,cm3232" },
Same here. No need for space before "."
> + { }
> +};
> +
> +static struct i2c_driver cm3232_driver = {
> + .driver = {
> + .name = "cm3232",
> + .owner = THIS_MODULE,
> + .of_match_table = of_match_ptr(cm3232_of_match),
> + },
> + .id_table = cm3232_id,
> + .probe = cm3232_probe,
> + .remove = cm3232_remove,
> +};
> +
> +module_i2c_driver(cm3232_driver);
> +
> +MODULE_AUTHOR("Kevin Tsai <ktsai@capellamicro.com>");
> +MODULE_DESCRIPTION("CM3232 ambient light sensor driver");
> +MODULE_LICENSE("GPL");
> --
> 1.8.3.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V1 1/1] iio: Added Capella cm3232 ambient light sensor driver.
@ 2015-01-05 10:51 ` Daniel Baluta
0 siblings, 0 replies; 21+ messages in thread
From: Daniel Baluta @ 2015-01-05 10:51 UTC (permalink / raw)
To: Kevin Tsai
Cc: Wolfram Sang, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald, Grant Likely, Andrew Morton,
David S. Miller, Greg Kroah-Hartman, Joe Perches,
Mauro Carvalho Chehab, Antti Palosaari, Daniel Baluta,
Archana Patni, linux-i2c, devicetree, linux-iio
On Thu, Jan 1, 2015 at 2:10 AM, Kevin Tsai <ktsai@capellamicro.com> wrote:
> CM3232 is an advanced ambient light sensor with I2C protocol interface.
> The I2C slave address is internally hardwired as 0x10 (7-bit). Writing
> to configure register is byte mode, but reading ALS register requests to
> use word mode for 16-bit resolution.
This looks good to me. Few comments inline.
>
> Signed-off-by: Kevin Tsai <ktsai@capellamicro.com>
> ---
> .../devicetree/bindings/i2c/trivial-devices.txt | 1 +
> MAINTAINERS | 6 +
> drivers/iio/light/Kconfig | 11 +
> drivers/iio/light/Makefile | 1 +
> drivers/iio/light/cm3232.c | 403 +++++++++++++++++++++
> 5 files changed, 422 insertions(+)
> create mode 100644 drivers/iio/light/cm3232.c
>
> diff --git a/Documentation/devicetree/bindings/i2c/trivial-devices.txt b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
> index 9f4e382..572a7c4 100644
> --- a/Documentation/devicetree/bindings/i2c/trivial-devices.txt
> +++ b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
> @@ -34,6 +34,7 @@ atmel,24c512 i2c serial eeprom (24cxx)
> atmel,24c1024 i2c serial eeprom (24cxx)
> atmel,at97sc3204t i2c trusted platform module (TPM)
> capella,cm32181 CM32181: Ambient Light Sensor
> +capella,cm3232 CM3232: Ambient Light Sensor
> catalyst,24c32 i2c serial eeprom
> cirrus,cs42l51 Cirrus Logic CS42L51 audio codec
> dallas,ds1307 64 x 8, Serial, I2C Real-Time Clock
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ddb9ac8..06a613a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2378,6 +2378,12 @@ F: security/capability.c
> F: security/commoncap.c
> F: kernel/capability.c
>
> +CAPELLA MICROSYSTEMS LIGHT SENSOR DRIVER
> +M: Kevin Tsai <ktsai@capellamicro.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@gmail.com>
> L: linux-wpan@vger.kernel.org
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index 5bea821..d2318e2 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -48,6 +48,17 @@ config CM32181
> To compile this driver as a module, choose M here:
> the module will be called cm32181.
>
> +config CM3232
> + depends on I2C
> + tristate "CM3232 driver"
Better name this: "CM3232 ambient light sensor".
> + help
> + Say Y here if you use cm3232.
> + This option enables ambient light sensor using
> + Capella Microsystems cm3232 device driver.
> +
> + To compile this driver as a module, choose M here:
> + the module will be called cm3232.
> +
> config CM36651
> depends on I2C
> tristate "CM36651 driver"
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index 47877a3..f2c8d55 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_ADJD_S311) += adjd_s311.o
> obj-$(CONFIG_AL3320A) += al3320a.o
> obj-$(CONFIG_APDS9300) += apds9300.o
> obj-$(CONFIG_CM32181) += cm32181.o
> +obj-$(CONFIG_CM3232) += cm3232.o
> obj-$(CONFIG_CM36651) += cm36651.o
> obj-$(CONFIG_GP2AP020A00F) += gp2ap020a00f.o
> obj-$(CONFIG_HID_SENSOR_ALS) += hid-sensor-als.o
> diff --git a/drivers/iio/light/cm3232.c b/drivers/iio/light/cm3232.c
> new file mode 100644
> index 0000000..fd98eeb
> --- /dev/null
> +++ b/drivers/iio/light/cm3232.c
> @@ -0,0 +1,403 @@
> +/*
> + * 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.
> + *
Usually, here is the place to mention the I2C slave address.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/mutex.h>
> +#include <linux/module.h>
> +#include <linux/interrupt.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/events.h>
> +#include <linux/init.h>
> +
> +/* Registers Address */
> +#define CM3232_REG_ADDR_CMD 0x00
> +#define CM3232_REG_ADDR_ALS 0x50
> +#define CM3232_REG_ADDR_ID 0x53
> +
> +/* CMD register */
> +#define CM3232_CMD_ALS_DISABLE BIT(0)
> +#define CM3232_CMD_ALS_HS BIT(1)
> +
> +#define CM3232_CMD_ALS_IT_SHIFT 2
> +#define CM3232_CMD_ALS_IT_MASK (0x07 << CM3232_CMD_ALS_IT_SHIFT)
> +#define CM3232_CMD_ALS_IT_DEFAULT (0x01 << CM3232_CMD_ALS_IT_SHIFT)
> +
> +#define CM3232_CMD_ALS_RESET BIT(6)
> +
> +#define CM3232_CMD_DEFAULT CM3232_CMD_ALS_IT_DEFAULT
> +
> +#define CM3232_CALIBSCALE_DEFAULT 100000
> +#define CM3232_CALIBSCALE_RESOLUTION 100000
> +#define CM3232_MLUX_PER_LUX 1000
> +
> +#define CM3232_MLUX_PER_BIT_DEFAULT 64
> +#define CM3232_MLUX_PER_BIT_BASE_IT 100000
> +static const int CM3232_als_it_bits[] = { 0, 1, 2, 3, 4, 5};
> +static const int CM3232_als_it_values[] = {
> + 100000, 200000, 400000, 800000, 1600000, 3200000};
> +
> +struct cm3232_als_info {
> + u32 id;
> + int calibscale;
> + int mlux_per_bit;
> + int mlux_per_bit_base_it;
> + const int *als_it_bits;
> + const int *als_it_values;
> + const int num_als_it;
> + int als_raw;
> +};
> +
> +static struct cm3232_als_info cm3232_als_info_default = {
> + .id = 3232,
> + .calibscale = CM3232_CALIBSCALE_DEFAULT,
> + .mlux_per_bit = CM3232_MLUX_PER_BIT_DEFAULT,
> + .mlux_per_bit_base_it = CM3232_MLUX_PER_BIT_BASE_IT,
> + .als_it_bits = CM3232_als_it_bits,
> + .als_it_values = CM3232_als_it_values,
> + .num_als_it = ARRAY_SIZE(CM3232_als_it_bits),
> +};
> +
> +struct cm3232_chip {
> + struct i2c_client *client;
> + struct mutex lock;
> + struct cm3232_als_info *als_info;
> + u8 regs_cmd;
> +};
> +
> +static int cm3232_get_lux(struct cm3232_chip *chip);
> +static int cm3232_read_als_it(struct cm3232_chip *chip, int *val2);
> +
> +/**
> + * cm3232_reg_init() - Initialize CM3232 registers
> + * @chip: pointer of struct cm3232.
> + *
> + * Initialize CM3232 ambient light sensor register to default values.
> + *
> + Return: 0 for success; otherwise for error code.
> + */
> +static int cm3232_reg_init(struct cm3232_chip *chip)
> +{
> + struct i2c_client *client = chip->client;
> + struct cm3232_als_info *als_info;
> + s32 ret;
> +
> + /* Identify device */
> + ret = i2c_smbus_read_word_data(client, CM3232_REG_ADDR_ID);
> + if (ret < 0)
> + return ret;
> + if ((ret & 0xFF) != 0x32)
> + return -ENODEV;
> +
> + /* Disable and reset device */
> + chip->regs_cmd = CM3232_CMD_ALS_DISABLE | CM3232_CMD_ALS_RESET;
> + ret = i2c_smbus_write_byte_data(client, CM3232_REG_ADDR_CMD,
> + chip->regs_cmd);
> + if (ret < 0)
> + return ret;
> +
> + /* Initialization */
> + als_info = chip->als_info = &cm3232_als_info_default;
> + chip->regs_cmd = CM3232_CMD_DEFAULT;
> +
> + /* Configure register */
> + ret = i2c_smbus_write_byte_data(client, CM3232_REG_ADDR_CMD,
> + chip->regs_cmd);
You could directly return i2c_smbus_write_byte_data(..).
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +/**
> + * cm3232_read_als_it() - Get sensor integration time (ms)
> + * @chip: pointer of struct cm3232
> + * @val2: pointer of int to load the als_it value.
> + *
> + * Report the current integration time in milliseconds.
> + *
> + * Return: IIO_VAL_INT_PLUS_MICRO for success, otherwise -EINVAL.
> + */
> +static int cm3232_read_als_it(struct cm3232_chip *chip, int *val2)
Wouldn't make more sense to also add val here?
> +{
> + struct cm3232_als_info *als_info = chip->als_info;
> + u16 als_it;
> + int i;
> +
> + als_it = chip->regs_cmd;
> + als_it &= CM3232_CMD_ALS_IT_MASK;
> + als_it >>= CM3232_CMD_ALS_IT_SHIFT;
> + for (i = 0; i < als_info->num_als_it; i++) {
> + if (als_it == als_info->als_it_bits[i]) {
.. then make, *val = 0.
> + *val2 = als_info->als_it_values[i];
> + return IIO_VAL_INT_PLUS_MICRO;
> + }
> + }
> +
> + return -EINVAL;
> +}
> +
> +/**
> + * cm3232_write_als_it() - Write sensor integration time
> + * @chip: pointer of struct cm3232.
> + * @val: integration time in milliseconds.
> + *
> + * Convert integration time (ms) to sensor value.
> + *
> + * Return: i2c_smbus_write_word_data command return value.
> + */
> +static int cm3232_write_als_it(struct cm3232_chip *chip, int val)
> +{
> + struct i2c_client *client = chip->client;
> + struct cm3232_als_info *als_info = chip->als_info;
> + u16 als_it;
> + int ret, i;
> +
> + for (i = 0; i < als_info->num_als_it; i++)
> + if (val <= als_info->als_it_values[i])
> + break;
> + if (i >= als_info->num_als_it)
> + i = als_info->num_als_it - 1;
> +
> + als_it = als_info->als_it_bits[i];
> + als_it <<= CM3232_CMD_ALS_IT_SHIFT;
> +
> + mutex_lock(&chip->lock);
> + chip->regs_cmd &= ~CM3232_CMD_ALS_IT_MASK;
> + chip->regs_cmd |= als_it;
> + ret = i2c_smbus_write_byte_data(client, CM3232_REG_ADDR_CMD,
> + chip->regs_cmd);
> + mutex_unlock(&chip->lock);
> +
> + return ret;
> +}
> +
> +/**
> + * cm3232_get_lux() - report current lux value
> + * @chip: pointer of struct cm3232.
> + *
> + * Convert sensor raw data to lux. It depends on integration
> + * time and calibscale variable.
> + *
> + * Return: Positive value is lux, otherwise is error code.
> + */
> +static int cm3232_get_lux(struct cm3232_chip *chip)
> +{
> + struct i2c_client *client = chip->client;
> + struct cm3232_als_info *als_info = chip->als_info;
> + int ret;
> + int als_it;
> + u64 tmp;
> +
> + /* Calculate mlux per bit based on als_it */
> + ret = cm3232_read_als_it(chip, &als_it);
> + if (ret < 0)
> + return -EINVAL;
> + tmp = (__force u64)als_info->mlux_per_bit;
> + tmp *= als_info->mlux_per_bit_base_it;
> + tmp = div_u64 (tmp, als_it);
> +
> + /* Get als_raw */
> + als_info->als_raw = i2c_smbus_read_word_data(
> + client,
> + CM3232_REG_ADDR_ALS);
> + if (als_info->als_raw < 0)
> + return als_info->als_raw;
> +
> + tmp *= als_info->als_raw;
> + tmp *= als_info->calibscale;
> + tmp = div_u64(tmp, CM3232_CALIBSCALE_RESOLUTION);
> + tmp = div_u64(tmp, CM3232_MLUX_PER_LUX);
> +
> + if (tmp > 0xFFFF)
> + tmp = 0xFFFF;
> +
> + return (int)tmp;
> +}
> +
> +static int cm3232_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct cm3232_chip *chip = iio_priv(indio_dev);
> + struct cm3232_als_info *als_info = chip->als_info;
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_PROCESSED:
> + ret = cm3232_get_lux(chip);
> + if (ret < 0)
> + return ret;
> + *val = ret;
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_CALIBSCALE:
> + *val = als_info->calibscale;
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_INT_TIME:
> + *val = 0;
> + ret = cm3232_read_als_it(chip, val2);
> + return ret;
Here you could just return cm3232_read_als_it(...)
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int cm3232_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + struct cm3232_chip *chip = iio_priv(indio_dev);
> + struct cm3232_als_info *als_info = chip->als_info;
> + long ms;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_CALIBSCALE:
> + als_info->calibscale = val;
> + return val;
> + case IIO_CHAN_INFO_INT_TIME:
> + ms = val * 1000000 + val2;
> + return cm3232_write_als_it(chip, (int)ms);
> + }
> +
> + return -EINVAL;
> +}
> +
> +/**
> + * cm3232_get_it_available() - Get available ALS IT value
> + * @dev: pointer of struct device.
> + * @attr: pointer of struct device_attribute.
> + * @buf: pointer of return string buffer.
> + *
> + * Display the available integration time in milliseconds.
> + *
> + * Return: string length.
> + */
> +static ssize_t cm3232_get_it_available(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct cm3232_chip *chip = iio_priv(dev_to_iio_dev(dev));
> + struct cm3232_als_info *als_info = chip->als_info;
> + int i, len;
> +
> + for (i = 0, len = 0; i < als_info->num_als_it; i++)
> + len += scnprintf(buf + len, PAGE_SIZE - len, "%u.%06u ",
> + als_info->als_it_values[i]/1000000,
> + als_info->als_it_values[i]%1000000);
> + return len + scnprintf(buf + len, PAGE_SIZE - len, "\n");
> +}
> +
> +static const struct iio_chan_spec cm3232_channels[] = {
> + {
> + .type = IIO_LIGHT,
> + .info_mask_separate =
> + BIT(IIO_CHAN_INFO_PROCESSED) |
> + BIT(IIO_CHAN_INFO_CALIBSCALE) |
> + BIT(IIO_CHAN_INFO_INT_TIME),
> + }
> +};
> +
> +static IIO_DEVICE_ATTR(in_illuminance_integration_time_available,
> + S_IRUGO, cm3232_get_it_available, NULL, 0);
> +
> +static struct attribute *cm3232_attributes[] = {
> + &iio_dev_attr_in_illuminance_integration_time_available.dev_attr.attr,
> + NULL,
> +};
> +
> +static const struct attribute_group cm3232_attribute_group = {
> + .attrs = cm3232_attributes
> +};
> +
> +static const struct iio_info cm3232_info = {
> + .driver_module = THIS_MODULE,
> + .read_raw = &cm3232_read_raw,
> + .write_raw = &cm3232_write_raw,
> + .attrs = &cm3232_attribute_group,
> +};
> +
> +static int cm3232_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct cm3232_chip *chip;
> + struct iio_dev *indio_dev;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
> + if (!indio_dev) {
> + dev_err(&client->dev, "devm_iio_device_alloc failed\n");
There is no need for this error message. devm_iio_device_alloc will print
an error message in case of failure.
> + return -ENOMEM;
> + }
> +
> + chip = iio_priv(indio_dev);
> + i2c_set_clientdata(client, indio_dev);
> + chip->client = client;
> +
> + mutex_init(&chip->lock);
> + indio_dev->dev.parent = &client->dev;
> + indio_dev->channels = cm3232_channels;
> + indio_dev->num_channels = ARRAY_SIZE(cm3232_channels);
> + indio_dev->info = &cm3232_info;
> + if (id && id->name)
> + indio_dev->name = id->name;
> + else
> + indio_dev->name = (char *)dev_name(&client->dev);
> + indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + ret = cm3232_reg_init(chip);
> + if (ret) {
> + dev_err(&client->dev,
> + "%s: register init failed\n",
> + __func__);
> + return ret;
> + }
> +
> + ret = iio_device_register(indio_dev);
> + return 0;
Directly return iio_device_register()
> +}
> +
> +static int cm3232_remove(struct i2c_client *client)
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +
> + iio_device_unregister(indio_dev);
> + return 0;
> +}
> +
> +static const struct i2c_device_id cm3232_id[] = {
> + { "cm3232", 0},
No need for space before ".
> + { }
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, cm3232_id);
> +
> +static const struct of_device_id cm3232_of_match[] = {
> + { .compatible = "capella,cm3232" },
Same here. No need for space before "."
> + { }
> +};
> +
> +static struct i2c_driver cm3232_driver = {
> + .driver = {
> + .name = "cm3232",
> + .owner = THIS_MODULE,
> + .of_match_table = of_match_ptr(cm3232_of_match),
> + },
> + .id_table = cm3232_id,
> + .probe = cm3232_probe,
> + .remove = cm3232_remove,
> +};
> +
> +module_i2c_driver(cm3232_driver);
> +
> +MODULE_AUTHOR("Kevin Tsai <ktsai@capellamicro.com>");
> +MODULE_DESCRIPTION("CM3232 ambient light sensor driver");
> +MODULE_LICENSE("GPL");
> --
> 1.8.3.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V1 1/1] iio: Added Capella cm3232 ambient light sensor driver.
@ 2015-01-05 13:09 ` Joe Perches
0 siblings, 0 replies; 21+ messages in thread
From: Joe Perches @ 2015-01-05 13:09 UTC (permalink / raw)
To: Daniel Baluta
Cc: Kevin Tsai, Wolfram Sang, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald, Grant Likely, Andrew Morton,
David S. Miller, Greg Kroah-Hartman, Mauro Carvalho Chehab,
Antti Palosaari, Archana Patni, linux-i2c, devicetree, linux-iio,
Linux Kernel Mailing List
On Mon, 2015-01-05 at 12:51 +0200, Daniel Baluta wrote:
> On Thu, Jan 1, 2015 at 2:10 AM, Kevin Tsai <ktsai@capellamicro.com> wrote:
> > CM3232 is an advanced ambient light sensor with I2C protocol interface.
> > The I2C slave address is internally hardwired as 0x10 (7-bit). Writing
> > to configure register is byte mode, but reading ALS register requests to
> > use word mode for 16-bit resolution.
>
> This looks good to me. Few comments inline.
[]
> > diff --git a/drivers/iio/light/cm3232.c b/drivers/iio/light/cm3232.c
[]
> > +/**
> > + * cm3232_reg_init() - Initialize CM3232 registers
> > + * @chip: pointer of struct cm3232.
> > + *
> > + * Initialize CM3232 ambient light sensor register to default values.
> > + *
> > + Return: 0 for success; otherwise for error code.
> > + */
> > +static int cm3232_reg_init(struct cm3232_chip *chip)
> > +{
> > + struct i2c_client *client = chip->client;
> > + struct cm3232_als_info *als_info;
> > + s32 ret;
> > +
> > + /* Identify device */
> > + ret = i2c_smbus_read_word_data(client, CM3232_REG_ADDR_ID);
> > + if (ret < 0)
> > + return ret;
> > + if ((ret & 0xFF) != 0x32)
> > + return -ENODEV;
> > +
> > + /* Disable and reset device */
> > + chip->regs_cmd = CM3232_CMD_ALS_DISABLE | CM3232_CMD_ALS_RESET;
> > + ret = i2c_smbus_write_byte_data(client, CM3232_REG_ADDR_CMD,
> > + chip->regs_cmd);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /* Initialization */
> > + als_info = chip->als_info = &cm3232_als_info_default;
> > + chip->regs_cmd = CM3232_CMD_DEFAULT;
> > +
> > + /* Configure register */
> > + ret = i2c_smbus_write_byte_data(client, CM3232_REG_ADDR_CMD,
> > + chip->regs_cmd);
>
> You could directly return i2c_smbus_write_byte_data(..).
Sometimes it's better to return a specific value
for the error instead of depending on correctness
of all the indirect functions in the call chain.
In this case, all the smbus_xfer functions must
return 0 on success. Do they?
Inspecting the codebase for correctness or not
depending on that correctness is sometimes better
to avoid doing.
> > + if (ret < 0)
> > + return ret;
> > +
> > + return 0;
> > +}
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V1 1/1] iio: Added Capella cm3232 ambient light sensor driver.
@ 2015-01-05 13:09 ` Joe Perches
0 siblings, 0 replies; 21+ messages in thread
From: Joe Perches @ 2015-01-05 13:09 UTC (permalink / raw)
To: Daniel Baluta
Cc: Kevin Tsai, Wolfram Sang, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald, Grant Likely, Andrew Morton,
David S. Miller, Greg Kroah-Hartman, Mauro Carvalho Chehab,
Antti Palosaari, Archana Patni, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA, Linux Kernel Mailing List
On Mon, 2015-01-05 at 12:51 +0200, Daniel Baluta wrote:
> On Thu, Jan 1, 2015 at 2:10 AM, Kevin Tsai <ktsai-GubuWUlQtMwciDkP5Hr2oA@public.gmane.org> wrote:
> > CM3232 is an advanced ambient light sensor with I2C protocol interface.
> > The I2C slave address is internally hardwired as 0x10 (7-bit). Writing
> > to configure register is byte mode, but reading ALS register requests to
> > use word mode for 16-bit resolution.
>
> This looks good to me. Few comments inline.
[]
> > diff --git a/drivers/iio/light/cm3232.c b/drivers/iio/light/cm3232.c
[]
> > +/**
> > + * cm3232_reg_init() - Initialize CM3232 registers
> > + * @chip: pointer of struct cm3232.
> > + *
> > + * Initialize CM3232 ambient light sensor register to default values.
> > + *
> > + Return: 0 for success; otherwise for error code.
> > + */
> > +static int cm3232_reg_init(struct cm3232_chip *chip)
> > +{
> > + struct i2c_client *client = chip->client;
> > + struct cm3232_als_info *als_info;
> > + s32 ret;
> > +
> > + /* Identify device */
> > + ret = i2c_smbus_read_word_data(client, CM3232_REG_ADDR_ID);
> > + if (ret < 0)
> > + return ret;
> > + if ((ret & 0xFF) != 0x32)
> > + return -ENODEV;
> > +
> > + /* Disable and reset device */
> > + chip->regs_cmd = CM3232_CMD_ALS_DISABLE | CM3232_CMD_ALS_RESET;
> > + ret = i2c_smbus_write_byte_data(client, CM3232_REG_ADDR_CMD,
> > + chip->regs_cmd);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /* Initialization */
> > + als_info = chip->als_info = &cm3232_als_info_default;
> > + chip->regs_cmd = CM3232_CMD_DEFAULT;
> > +
> > + /* Configure register */
> > + ret = i2c_smbus_write_byte_data(client, CM3232_REG_ADDR_CMD,
> > + chip->regs_cmd);
>
> You could directly return i2c_smbus_write_byte_data(..).
Sometimes it's better to return a specific value
for the error instead of depending on correctness
of all the indirect functions in the call chain.
In this case, all the smbus_xfer functions must
return 0 on success. Do they?
Inspecting the codebase for correctness or not
depending on that correctness is sometimes better
to avoid doing.
> > + if (ret < 0)
> > + return ret;
> > +
> > + return 0;
> > +}
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V1 1/1] iio: Added Capella cm3232 ambient light sensor driver.
@ 2015-01-05 13:09 ` Joe Perches
0 siblings, 0 replies; 21+ messages in thread
From: Joe Perches @ 2015-01-05 13:09 UTC (permalink / raw)
To: Daniel Baluta
Cc: Kevin Tsai, Wolfram Sang, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald, Grant Likely, Andrew Morton,
David S. Miller, Greg Kroah-Hartman, Mauro Carvalho Chehab,
Antti Palosaari, Archana Patni, linux-i2c, devicetree, linux-iio,
Linux Kernel Mailing List
On Mon, 2015-01-05 at 12:51 +0200, Daniel Baluta wrote:
> On Thu, Jan 1, 2015 at 2:10 AM, Kevin Tsai <ktsai@capellamicro.com> wrote:
> > CM3232 is an advanced ambient light sensor with I2C protocol interface.
> > The I2C slave address is internally hardwired as 0x10 (7-bit). Writing
> > to configure register is byte mode, but reading ALS register requests to
> > use word mode for 16-bit resolution.
>
> This looks good to me. Few comments inline.
[]
> > diff --git a/drivers/iio/light/cm3232.c b/drivers/iio/light/cm3232.c
[]
> > +/**
> > + * cm3232_reg_init() - Initialize CM3232 registers
> > + * @chip: pointer of struct cm3232.
> > + *
> > + * Initialize CM3232 ambient light sensor register to default values.
> > + *
> > + Return: 0 for success; otherwise for error code.
> > + */
> > +static int cm3232_reg_init(struct cm3232_chip *chip)
> > +{
> > + struct i2c_client *client = chip->client;
> > + struct cm3232_als_info *als_info;
> > + s32 ret;
> > +
> > + /* Identify device */
> > + ret = i2c_smbus_read_word_data(client, CM3232_REG_ADDR_ID);
> > + if (ret < 0)
> > + return ret;
> > + if ((ret & 0xFF) != 0x32)
> > + return -ENODEV;
> > +
> > + /* Disable and reset device */
> > + chip->regs_cmd = CM3232_CMD_ALS_DISABLE | CM3232_CMD_ALS_RESET;
> > + ret = i2c_smbus_write_byte_data(client, CM3232_REG_ADDR_CMD,
> > + chip->regs_cmd);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /* Initialization */
> > + als_info = chip->als_info = &cm3232_als_info_default;
> > + chip->regs_cmd = CM3232_CMD_DEFAULT;
> > +
> > + /* Configure register */
> > + ret = i2c_smbus_write_byte_data(client, CM3232_REG_ADDR_CMD,
> > + chip->regs_cmd);
>
> You could directly return i2c_smbus_write_byte_data(..).
Sometimes it's better to return a specific value
for the error instead of depending on correctness
of all the indirect functions in the call chain.
In this case, all the smbus_xfer functions must
return 0 on success. Do they?
Inspecting the codebase for correctness or not
depending on that correctness is sometimes better
to avoid doing.
> > + if (ret < 0)
> > + return ret;
> > +
> > + return 0;
> > +}
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V1 1/1] iio: Added Capella cm3232 ambient light sensor driver.
@ 2015-01-05 14:20 ` Daniel Baluta
0 siblings, 0 replies; 21+ messages in thread
From: Daniel Baluta @ 2015-01-05 14:20 UTC (permalink / raw)
To: Joe Perches
Cc: Daniel Baluta, Kevin Tsai, Wolfram Sang, Rob Herring, Pawel Moll,
Mark Rutland, Ian Campbell, Kumar Gala, Jonathan Cameron,
Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald, Grant Likely,
Andrew Morton, David S. Miller, Greg Kroah-Hartman,
Mauro Carvalho Chehab, Antti Palosaari, Archana Patni, linux-i2c,
devicetree, linux-iio, Linux Kernel Mailing List
On Mon, Jan 5, 2015 at 3:09 PM, Joe Perches <joe@perches.com> wrote:
> On Mon, 2015-01-05 at 12:51 +0200, Daniel Baluta wrote:
>> On Thu, Jan 1, 2015 at 2:10 AM, Kevin Tsai <ktsai@capellamicro.com> wrote:
>> > CM3232 is an advanced ambient light sensor with I2C protocol interface.
>> > The I2C slave address is internally hardwired as 0x10 (7-bit). Writing
>> > to configure register is byte mode, but reading ALS register requests to
>> > use word mode for 16-bit resolution.
>>
>> This looks good to me. Few comments inline.
> []
>> > diff --git a/drivers/iio/light/cm3232.c b/drivers/iio/light/cm3232.c
> []
>> > +/**
>> > + * cm3232_reg_init() - Initialize CM3232 registers
>> > + * @chip: pointer of struct cm3232.
>> > + *
>> > + * Initialize CM3232 ambient light sensor register to default values.
>> > + *
>> > + Return: 0 for success; otherwise for error code.
>> > + */
>> > +static int cm3232_reg_init(struct cm3232_chip *chip)
>> > +{
>> > + struct i2c_client *client = chip->client;
>> > + struct cm3232_als_info *als_info;
>> > + s32 ret;
>> > +
>> > + /* Identify device */
>> > + ret = i2c_smbus_read_word_data(client, CM3232_REG_ADDR_ID);
>> > + if (ret < 0)
>> > + return ret;
>> > + if ((ret & 0xFF) != 0x32)
>> > + return -ENODEV;
>> > +
>> > + /* Disable and reset device */
>> > + chip->regs_cmd = CM3232_CMD_ALS_DISABLE | CM3232_CMD_ALS_RESET;
>> > + ret = i2c_smbus_write_byte_data(client, CM3232_REG_ADDR_CMD,
>> > + chip->regs_cmd);
>> > + if (ret < 0)
>> > + return ret;
>> > +
>> > + /* Initialization */
>> > + als_info = chip->als_info = &cm3232_als_info_default;
>> > + chip->regs_cmd = CM3232_CMD_DEFAULT;
>> > +
>> > + /* Configure register */
>> > + ret = i2c_smbus_write_byte_data(client, CM3232_REG_ADDR_CMD,
>> > + chip->regs_cmd);
>>
>> You could directly return i2c_smbus_write_byte_data(..).
>
> Sometimes it's better to return a specific value
> for the error instead of depending on correctness
> of all the indirect functions in the call chain.
>
> In this case, all the smbus_xfer functions must
> return 0 on success. Do they?
Yes.
http://lxr.free-electrons.com/source/drivers/i2c/i2c-core.c#L2845
>
> Inspecting the codebase for correctness or not
> depending on that correctness is sometimes better
> to avoid doing.
You have a valid point. Anyhow, AFAIK the code from IIO
prefers a direct return.
Also, I guess kbuild test robot will complain about it, like here:
http://marc.info/?l=linux-kernel&m=141963536230419&w=2
thanks,
Daniel.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V1 1/1] iio: Added Capella cm3232 ambient light sensor driver.
@ 2015-01-05 14:20 ` Daniel Baluta
0 siblings, 0 replies; 21+ messages in thread
From: Daniel Baluta @ 2015-01-05 14:20 UTC (permalink / raw)
To: Joe Perches
Cc: Daniel Baluta, Kevin Tsai, Wolfram Sang, Rob Herring, Pawel Moll,
Mark Rutland, Ian Campbell, Kumar Gala, Jonathan Cameron,
Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald, Grant Likely,
Andrew Morton, David S. Miller, Greg Kroah-Hartman,
Mauro Carvalho Chehab, Antti Palosaari, Archana Patni,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA, Linux
On Mon, Jan 5, 2015 at 3:09 PM, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
> On Mon, 2015-01-05 at 12:51 +0200, Daniel Baluta wrote:
>> On Thu, Jan 1, 2015 at 2:10 AM, Kevin Tsai <ktsai-GubuWUlQtMwciDkP5Hr2oA@public.gmane.org> wrote:
>> > CM3232 is an advanced ambient light sensor with I2C protocol interface.
>> > The I2C slave address is internally hardwired as 0x10 (7-bit). Writing
>> > to configure register is byte mode, but reading ALS register requests to
>> > use word mode for 16-bit resolution.
>>
>> This looks good to me. Few comments inline.
> []
>> > diff --git a/drivers/iio/light/cm3232.c b/drivers/iio/light/cm3232.c
> []
>> > +/**
>> > + * cm3232_reg_init() - Initialize CM3232 registers
>> > + * @chip: pointer of struct cm3232.
>> > + *
>> > + * Initialize CM3232 ambient light sensor register to default values.
>> > + *
>> > + Return: 0 for success; otherwise for error code.
>> > + */
>> > +static int cm3232_reg_init(struct cm3232_chip *chip)
>> > +{
>> > + struct i2c_client *client = chip->client;
>> > + struct cm3232_als_info *als_info;
>> > + s32 ret;
>> > +
>> > + /* Identify device */
>> > + ret = i2c_smbus_read_word_data(client, CM3232_REG_ADDR_ID);
>> > + if (ret < 0)
>> > + return ret;
>> > + if ((ret & 0xFF) != 0x32)
>> > + return -ENODEV;
>> > +
>> > + /* Disable and reset device */
>> > + chip->regs_cmd = CM3232_CMD_ALS_DISABLE | CM3232_CMD_ALS_RESET;
>> > + ret = i2c_smbus_write_byte_data(client, CM3232_REG_ADDR_CMD,
>> > + chip->regs_cmd);
>> > + if (ret < 0)
>> > + return ret;
>> > +
>> > + /* Initialization */
>> > + als_info = chip->als_info = &cm3232_als_info_default;
>> > + chip->regs_cmd = CM3232_CMD_DEFAULT;
>> > +
>> > + /* Configure register */
>> > + ret = i2c_smbus_write_byte_data(client, CM3232_REG_ADDR_CMD,
>> > + chip->regs_cmd);
>>
>> You could directly return i2c_smbus_write_byte_data(..).
>
> Sometimes it's better to return a specific value
> for the error instead of depending on correctness
> of all the indirect functions in the call chain.
>
> In this case, all the smbus_xfer functions must
> return 0 on success. Do they?
Yes.
http://lxr.free-electrons.com/source/drivers/i2c/i2c-core.c#L2845
>
> Inspecting the codebase for correctness or not
> depending on that correctness is sometimes better
> to avoid doing.
You have a valid point. Anyhow, AFAIK the code from IIO
prefers a direct return.
Also, I guess kbuild test robot will complain about it, like here:
http://marc.info/?l=linux-kernel&m=141963536230419&w=2
thanks,
Daniel.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V1 1/1] iio: Added Capella cm3232 ambient light sensor driver.
2015-01-05 14:20 ` Daniel Baluta
(?)
@ 2015-01-05 16:42 ` Joe Perches
-1 siblings, 0 replies; 21+ messages in thread
From: Joe Perches @ 2015-01-05 16:42 UTC (permalink / raw)
To: Daniel Baluta
Cc: Kevin Tsai, Wolfram Sang, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald, Grant Likely, Andrew Morton,
David S. Miller, Greg Kroah-Hartman, Mauro Carvalho Chehab,
Antti Palosaari, Archana Patni, linux-i2c, devicetree, linux-iio,
Linux Kernel Mailing List
On Mon, 2015-01-05 at 16:20 +0200, Daniel Baluta wrote:
> On Mon, Jan 5, 2015 at 3:09 PM, Joe Perches <joe@perches.com> wrote:
> > On Mon, 2015-01-05 at 12:51 +0200, Daniel Baluta wrote:
> >> On Thu, Jan 1, 2015 at 2:10 AM, Kevin Tsai <ktsai@capellamicro.com> wrote:
> >> > CM3232 is an advanced ambient light sensor with I2C protocol interface.
> >> > The I2C slave address is internally hardwired as 0x10 (7-bit). Writing
> >> > to configure register is byte mode, but reading ALS register requests to
> >> > use word mode for 16-bit resolution.
[]
> >> You could directly return i2c_smbus_write_byte_data(..).
> >
> > Sometimes it's better to return a specific value
> > for the error instead of depending on correctness
> > of all the indirect functions in the call chain.
> >
> > In this case, all the smbus_xfer functions must
> > return 0 on success. Do they?
>
> Yes.
>
> http://lxr.free-electrons.com/source/drivers/i2c/i2c-core.c#L2845
This doesn't show that adapter->algo->smbus_xfer()
returns 0, you have to look at the code for that
indirectly called function.
> Also, I guess kbuild test robot will complain about it, like here:
>
> http://marc.info/?l=linux-kernel&m=141963536230419&w=2
Is the kbuild test robot now submitting patches for all
possible coccinelle simplifications? I don't think so.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V1 1/1] iio: Added Capella cm3232 ambient light sensor driver.
@ 2015-01-05 16:42 ` Joe Perches
0 siblings, 0 replies; 21+ messages in thread
From: Joe Perches @ 2015-01-05 16:42 UTC (permalink / raw)
To: Daniel Baluta
Cc: Kevin Tsai, Wolfram Sang, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald, Grant Likely, Andrew Morton,
David S. Miller, Greg Kroah-Hartman, Mauro Carvalho Chehab,
Antti Palosaari, Archana Patni, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA, Linux Kernel Mailing List
On Mon, 2015-01-05 at 16:20 +0200, Daniel Baluta wrote:
> On Mon, Jan 5, 2015 at 3:09 PM, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
> > On Mon, 2015-01-05 at 12:51 +0200, Daniel Baluta wrote:
> >> On Thu, Jan 1, 2015 at 2:10 AM, Kevin Tsai <ktsai-GubuWUlQtMwciDkP5Hr2oA@public.gmane.org> wrote:
> >> > CM3232 is an advanced ambient light sensor with I2C protocol interface.
> >> > The I2C slave address is internally hardwired as 0x10 (7-bit). Writing
> >> > to configure register is byte mode, but reading ALS register requests to
> >> > use word mode for 16-bit resolution.
[]
> >> You could directly return i2c_smbus_write_byte_data(..).
> >
> > Sometimes it's better to return a specific value
> > for the error instead of depending on correctness
> > of all the indirect functions in the call chain.
> >
> > In this case, all the smbus_xfer functions must
> > return 0 on success. Do they?
>
> Yes.
>
> http://lxr.free-electrons.com/source/drivers/i2c/i2c-core.c#L2845
This doesn't show that adapter->algo->smbus_xfer()
returns 0, you have to look at the code for that
indirectly called function.
> Also, I guess kbuild test robot will complain about it, like here:
>
> http://marc.info/?l=linux-kernel&m=141963536230419&w=2
Is the kbuild test robot now submitting patches for all
possible coccinelle simplifications? I don't think so.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V1 1/1] iio: Added Capella cm3232 ambient light sensor driver.
@ 2015-01-05 16:42 ` Joe Perches
0 siblings, 0 replies; 21+ messages in thread
From: Joe Perches @ 2015-01-05 16:42 UTC (permalink / raw)
To: Daniel Baluta
Cc: Kevin Tsai, Wolfram Sang, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald, Grant Likely, Andrew Morton,
David S. Miller, Greg Kroah-Hartman, Mauro Carvalho Chehab,
Antti Palosaari, Archana Patni, linux-i2c, devicetree, linux-iio,
Linux Kernel Mailing List
On Mon, 2015-01-05 at 16:20 +0200, Daniel Baluta wrote:
> On Mon, Jan 5, 2015 at 3:09 PM, Joe Perches <joe@perches.com> wrote:
> > On Mon, 2015-01-05 at 12:51 +0200, Daniel Baluta wrote:
> >> On Thu, Jan 1, 2015 at 2:10 AM, Kevin Tsai <ktsai@capellamicro.com> wrote:
> >> > CM3232 is an advanced ambient light sensor with I2C protocol interface.
> >> > The I2C slave address is internally hardwired as 0x10 (7-bit). Writing
> >> > to configure register is byte mode, but reading ALS register requests to
> >> > use word mode for 16-bit resolution.
[]
> >> You could directly return i2c_smbus_write_byte_data(..).
> >
> > Sometimes it's better to return a specific value
> > for the error instead of depending on correctness
> > of all the indirect functions in the call chain.
> >
> > In this case, all the smbus_xfer functions must
> > return 0 on success. Do they?
>
> Yes.
>
> http://lxr.free-electrons.com/source/drivers/i2c/i2c-core.c#L2845
This doesn't show that adapter->algo->smbus_xfer()
returns 0, you have to look at the code for that
indirectly called function.
> Also, I guess kbuild test robot will complain about it, like here:
>
> http://marc.info/?l=linux-kernel&m=141963536230419&w=2
Is the kbuild test robot now submitting patches for all
possible coccinelle simplifications? I don't think so.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V1 1/1] iio: Added Capella cm3232 ambient light sensor driver.
@ 2015-01-05 17:50 ` Daniel Baluta
0 siblings, 0 replies; 21+ messages in thread
From: Daniel Baluta @ 2015-01-05 17:50 UTC (permalink / raw)
To: Joe Perches
Cc: Daniel Baluta, Kevin Tsai, Wolfram Sang, Rob Herring, Pawel Moll,
Mark Rutland, Ian Campbell, Kumar Gala, Jonathan Cameron,
Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald, Grant Likely,
Andrew Morton, David S. Miller, Greg Kroah-Hartman,
Mauro Carvalho Chehab, Antti Palosaari, Archana Patni, linux-i2c,
devicetree, linux-iio, Linux Kernel Mailing List
On Mon, Jan 5, 2015 at 6:42 PM, Joe Perches <joe@perches.com> wrote:
> On Mon, 2015-01-05 at 16:20 +0200, Daniel Baluta wrote:
>> On Mon, Jan 5, 2015 at 3:09 PM, Joe Perches <joe@perches.com> wrote:
>> > On Mon, 2015-01-05 at 12:51 +0200, Daniel Baluta wrote:
>> >> On Thu, Jan 1, 2015 at 2:10 AM, Kevin Tsai <ktsai@capellamicro.com> wrote:
>> >> > CM3232 is an advanced ambient light sensor with I2C protocol interface.
>> >> > The I2C slave address is internally hardwired as 0x10 (7-bit). Writing
>> >> > to configure register is byte mode, but reading ALS register requests to
>> >> > use word mode for 16-bit resolution.
> []
>> >> You could directly return i2c_smbus_write_byte_data(..).
>> >
>> > Sometimes it's better to return a specific value
>> > for the error instead of depending on correctness
>> > of all the indirect functions in the call chain.
>> >
>> > In this case, all the smbus_xfer functions must
>> > return 0 on success. Do they?
>>
>> Yes.
>>
>> http://lxr.free-electrons.com/source/drivers/i2c/i2c-core.c#L2845
>
> This doesn't show that adapter->algo->smbus_xfer()
> returns 0, you have to look at the code for that
> indirectly called function.
I based my answer on the comment at the top of the function:
2845 * This executes an SMBus protocol operation, and returns a negative
2846 * errno code else zero on success.
>
>> Also, I guess kbuild test robot will complain about it, like here:
>>
>> http://marc.info/?l=linux-kernel&m=141963536230419&w=2
>
> Is the kbuild test robot now submitting patches for all
> possible coccinelle simplifications? I don't think so.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V1 1/1] iio: Added Capella cm3232 ambient light sensor driver.
@ 2015-01-05 17:50 ` Daniel Baluta
0 siblings, 0 replies; 21+ messages in thread
From: Daniel Baluta @ 2015-01-05 17:50 UTC (permalink / raw)
To: Joe Perches
Cc: Daniel Baluta, Kevin Tsai, Wolfram Sang, Rob Herring, Pawel Moll,
Mark Rutland, Ian Campbell, Kumar Gala, Jonathan Cameron,
Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald, Grant Likely,
Andrew Morton, David S. Miller, Greg Kroah-Hartman,
Mauro Carvalho Chehab, Antti Palosaari, Archana Patni,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA, Linux
On Mon, Jan 5, 2015 at 6:42 PM, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
> On Mon, 2015-01-05 at 16:20 +0200, Daniel Baluta wrote:
>> On Mon, Jan 5, 2015 at 3:09 PM, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
>> > On Mon, 2015-01-05 at 12:51 +0200, Daniel Baluta wrote:
>> >> On Thu, Jan 1, 2015 at 2:10 AM, Kevin Tsai <ktsai-GubuWUlQtMwciDkP5Hr2oA@public.gmane.org> wrote:
>> >> > CM3232 is an advanced ambient light sensor with I2C protocol interface.
>> >> > The I2C slave address is internally hardwired as 0x10 (7-bit). Writing
>> >> > to configure register is byte mode, but reading ALS register requests to
>> >> > use word mode for 16-bit resolution.
> []
>> >> You could directly return i2c_smbus_write_byte_data(..).
>> >
>> > Sometimes it's better to return a specific value
>> > for the error instead of depending on correctness
>> > of all the indirect functions in the call chain.
>> >
>> > In this case, all the smbus_xfer functions must
>> > return 0 on success. Do they?
>>
>> Yes.
>>
>> http://lxr.free-electrons.com/source/drivers/i2c/i2c-core.c#L2845
>
> This doesn't show that adapter->algo->smbus_xfer()
> returns 0, you have to look at the code for that
> indirectly called function.
I based my answer on the comment at the top of the function:
2845 * This executes an SMBus protocol operation, and returns a negative
2846 * errno code else zero on success.
>
>> Also, I guess kbuild test robot will complain about it, like here:
>>
>> http://marc.info/?l=linux-kernel&m=141963536230419&w=2
>
> Is the kbuild test robot now submitting patches for all
> possible coccinelle simplifications? I don't think so.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V1 1/1] iio: Added Capella cm3232 ambient light sensor driver.
2015-01-05 17:50 ` Daniel Baluta
@ 2015-01-05 17:56 ` Joe Perches
-1 siblings, 0 replies; 21+ messages in thread
From: Joe Perches @ 2015-01-05 17:56 UTC (permalink / raw)
To: Daniel Baluta
Cc: Kevin Tsai, Wolfram Sang, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald, Grant Likely, Andrew Morton,
David S. Miller, Greg Kroah-Hartman, Mauro Carvalho Chehab,
Antti Palosaari, Archana Patni, linux-i2c, devicetree, linux-iio,
Linux Kernel Mailing List
On Mon, 2015-01-05 at 19:50 +0200, Daniel Baluta wrote:
> On Mon, Jan 5, 2015 at 6:42 PM, Joe Perches <joe@perches.com> wrote:
> > On Mon, 2015-01-05 at 16:20 +0200, Daniel Baluta wrote:
> >> On Mon, Jan 5, 2015 at 3:09 PM, Joe Perches <joe@perches.com> wrote:
> >> > On Mon, 2015-01-05 at 12:51 +0200, Daniel Baluta wrote:
> >> >> On Thu, Jan 1, 2015 at 2:10 AM, Kevin Tsai <ktsai@capellamicro.com> wrote:
> >> >> > CM3232 is an advanced ambient light sensor with I2C protocol interface.
> >> >> > The I2C slave address is internally hardwired as 0x10 (7-bit). Writing
> >> >> > to configure register is byte mode, but reading ALS register requests to
> >> >> > use word mode for 16-bit resolution.
> > []
> >> >> You could directly return i2c_smbus_write_byte_data(..).
> >> >
> >> > Sometimes it's better to return a specific value
> >> > for the error instead of depending on correctness
> >> > of all the indirect functions in the call chain.
> >> >
> >> > In this case, all the smbus_xfer functions must
> >> > return 0 on success. Do they?
> >>
> >> Yes.
> >>
> >> http://lxr.free-electrons.com/source/drivers/i2c/i2c-core.c#L2845
> >
> > This doesn't show that adapter->algo->smbus_xfer()
> > returns 0, you have to look at the code for that
> > indirectly called function.
>
> I based my answer on the comment at the top of the function:
>
> 2845 * This executes an SMBus protocol operation, and returns a negative
> 2846 * errno code else zero on success.
Sure, but comments and code often differ and the
implementation of any of those smbus_xfer functions
could return a positive value like the byte value or
the number of bytes written instead of 0.
For correctness, you'd have to inspect them all.
If some new future smbus_xfer function was written
incorrectly, the return value from this function could
now be positive.
cheers, Joe
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V1 1/1] iio: Added Capella cm3232 ambient light sensor driver.
@ 2015-01-05 17:56 ` Joe Perches
0 siblings, 0 replies; 21+ messages in thread
From: Joe Perches @ 2015-01-05 17:56 UTC (permalink / raw)
To: Daniel Baluta
Cc: Kevin Tsai, Wolfram Sang, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald, Grant Likely, Andrew Morton,
David S. Miller, Greg Kroah-Hartman, Mauro Carvalho Chehab,
Antti Palosaari, Archana Patni, linux-i2c, devicetree, linux-iio,
Linux Kernel Mailing List
On Mon, 2015-01-05 at 19:50 +0200, Daniel Baluta wrote:
> On Mon, Jan 5, 2015 at 6:42 PM, Joe Perches <joe@perches.com> wrote:
> > On Mon, 2015-01-05 at 16:20 +0200, Daniel Baluta wrote:
> >> On Mon, Jan 5, 2015 at 3:09 PM, Joe Perches <joe@perches.com> wrote:
> >> > On Mon, 2015-01-05 at 12:51 +0200, Daniel Baluta wrote:
> >> >> On Thu, Jan 1, 2015 at 2:10 AM, Kevin Tsai <ktsai@capellamicro.com> wrote:
> >> >> > CM3232 is an advanced ambient light sensor with I2C protocol interface.
> >> >> > The I2C slave address is internally hardwired as 0x10 (7-bit). Writing
> >> >> > to configure register is byte mode, but reading ALS register requests to
> >> >> > use word mode for 16-bit resolution.
> > []
> >> >> You could directly return i2c_smbus_write_byte_data(..).
> >> >
> >> > Sometimes it's better to return a specific value
> >> > for the error instead of depending on correctness
> >> > of all the indirect functions in the call chain.
> >> >
> >> > In this case, all the smbus_xfer functions must
> >> > return 0 on success. Do they?
> >>
> >> Yes.
> >>
> >> http://lxr.free-electrons.com/source/drivers/i2c/i2c-core.c#L2845
> >
> > This doesn't show that adapter->algo->smbus_xfer()
> > returns 0, you have to look at the code for that
> > indirectly called function.
>
> I based my answer on the comment at the top of the function:
>
> 2845 * This executes an SMBus protocol operation, and returns a negative
> 2846 * errno code else zero on success.
Sure, but comments and code often differ and the
implementation of any of those smbus_xfer functions
could return a positive value like the byte value or
the number of bytes written instead of 0.
For correctness, you'd have to inspect them all.
If some new future smbus_xfer function was written
incorrectly, the return value from this function could
now be positive.
cheers, Joe
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V1 1/1] iio: Added Capella cm3232 ambient light sensor driver.
@ 2015-01-10 14:06 ` Jonathan Cameron
0 siblings, 0 replies; 21+ messages in thread
From: Jonathan Cameron @ 2015-01-10 14:06 UTC (permalink / raw)
To: Joe Perches, Daniel Baluta
Cc: Kevin Tsai, Wolfram Sang, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald, Grant Likely, Andrew Morton, David S. Miller,
Greg Kroah-Hartman, Mauro Carvalho Chehab, Antti Palosaari,
Archana Patni, linux-i2c, devicetree, linux-iio,
Linux Kernel Mailing List
On 05/01/15 17:56, Joe Perches wrote:
> On Mon, 2015-01-05 at 19:50 +0200, Daniel Baluta wrote:
>> On Mon, Jan 5, 2015 at 6:42 PM, Joe Perches <joe@perches.com> wrote:
>>> On Mon, 2015-01-05 at 16:20 +0200, Daniel Baluta wrote:
>>>> On Mon, Jan 5, 2015 at 3:09 PM, Joe Perches <joe@perches.com> wrote:
>>>>> On Mon, 2015-01-05 at 12:51 +0200, Daniel Baluta wrote:
>>>>>> On Thu, Jan 1, 2015 at 2:10 AM, Kevin Tsai <ktsai@capellamicro.com> wrote:
>>>>>>> CM3232 is an advanced ambient light sensor with I2C protocol interface.
>>>>>>> The I2C slave address is internally hardwired as 0x10 (7-bit). Writing
>>>>>>> to configure register is byte mode, but reading ALS register requests to
>>>>>>> use word mode for 16-bit resolution.
>>> []
>>>>>> You could directly return i2c_smbus_write_byte_data(..).
>>>>>
>>>>> Sometimes it's better to return a specific value
>>>>> for the error instead of depending on correctness
>>>>> of all the indirect functions in the call chain.
>>>>>
>>>>> In this case, all the smbus_xfer functions must
>>>>> return 0 on success. Do they?
>>>>
>>>> Yes.
>>>>
>>>> http://lxr.free-electrons.com/source/drivers/i2c/i2c-core.c#L2845
>>>
>>> This doesn't show that adapter->algo->smbus_xfer()
>>> returns 0, you have to look at the code for that
>>> indirectly called function.
>>
>> I based my answer on the comment at the top of the function:
>>
>> 2845 * This executes an SMBus protocol operation, and returns a negative
>> 2846 * errno code else zero on success.
>
> Sure, but comments and code often differ and the
> implementation of any of those smbus_xfer functions
> could return a positive value like the byte value or
> the number of bytes written instead of 0.
>
> For correctness, you'd have to inspect them all.
>
> If some new future smbus_xfer function was written
> incorrectly, the return value from this function could
> now be positive.
>
That would however, clearly be a bug and likely to cause all
sorts of issues elsewhere given the documentation requires that
it does return 0 on success and people will have been
relying on it.
Jonathan
> cheers, Joe
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH V1 1/1] iio: Added Capella cm3232 ambient light sensor driver.
@ 2015-01-10 14:06 ` Jonathan Cameron
0 siblings, 0 replies; 21+ messages in thread
From: Jonathan Cameron @ 2015-01-10 14:06 UTC (permalink / raw)
To: Joe Perches, Daniel Baluta
Cc: Kevin Tsai, Wolfram Sang, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald, Grant Likely, Andrew Morton, David S. Miller,
Greg Kroah-Hartman, Mauro Carvalho Chehab, Antti Palosaari,
Archana Patni, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA, Linux Kernel Mailing List
On 05/01/15 17:56, Joe Perches wrote:
> On Mon, 2015-01-05 at 19:50 +0200, Daniel Baluta wrote:
>> On Mon, Jan 5, 2015 at 6:42 PM, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
>>> On Mon, 2015-01-05 at 16:20 +0200, Daniel Baluta wrote:
>>>> On Mon, Jan 5, 2015 at 3:09 PM, Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
>>>>> On Mon, 2015-01-05 at 12:51 +0200, Daniel Baluta wrote:
>>>>>> On Thu, Jan 1, 2015 at 2:10 AM, Kevin Tsai <ktsai-GubuWUlQtMwciDkP5Hr2oA@public.gmane.org> wrote:
>>>>>>> CM3232 is an advanced ambient light sensor with I2C protocol interface.
>>>>>>> The I2C slave address is internally hardwired as 0x10 (7-bit). Writing
>>>>>>> to configure register is byte mode, but reading ALS register requests to
>>>>>>> use word mode for 16-bit resolution.
>>> []
>>>>>> You could directly return i2c_smbus_write_byte_data(..).
>>>>>
>>>>> Sometimes it's better to return a specific value
>>>>> for the error instead of depending on correctness
>>>>> of all the indirect functions in the call chain.
>>>>>
>>>>> In this case, all the smbus_xfer functions must
>>>>> return 0 on success. Do they?
>>>>
>>>> Yes.
>>>>
>>>> http://lxr.free-electrons.com/source/drivers/i2c/i2c-core.c#L2845
>>>
>>> This doesn't show that adapter->algo->smbus_xfer()
>>> returns 0, you have to look at the code for that
>>> indirectly called function.
>>
>> I based my answer on the comment at the top of the function:
>>
>> 2845 * This executes an SMBus protocol operation, and returns a negative
>> 2846 * errno code else zero on success.
>
> Sure, but comments and code often differ and the
> implementation of any of those smbus_xfer functions
> could return a positive value like the byte value or
> the number of bytes written instead of 0.
>
> For correctness, you'd have to inspect them all.
>
> If some new future smbus_xfer function was written
> incorrectly, the return value from this function could
> now be positive.
>
That would however, clearly be a bug and likely to cause all
sorts of issues elsewhere given the documentation requires that
it does return 0 on success and people will have been
relying on it.
Jonathan
> cheers, Joe
>
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2015-01-10 14:06 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-01 0:10 [PATCH V1 1/1] iio: Added Capella cm3232 ambient light sensor driver Kevin Tsai
2015-01-01 0:10 ` Kevin Tsai
2015-01-01 3:38 ` Jeremiah Mahler
2015-01-01 11:02 ` Peter Meerwald
2015-01-02 4:23 ` Kevin Tsai
2015-01-05 10:51 ` Daniel Baluta
2015-01-05 10:51 ` Daniel Baluta
2015-01-05 13:09 ` Joe Perches
2015-01-05 13:09 ` Joe Perches
2015-01-05 13:09 ` Joe Perches
2015-01-05 14:20 ` Daniel Baluta
2015-01-05 14:20 ` Daniel Baluta
2015-01-05 16:42 ` Joe Perches
2015-01-05 16:42 ` Joe Perches
2015-01-05 16:42 ` Joe Perches
2015-01-05 17:50 ` Daniel Baluta
2015-01-05 17:50 ` Daniel Baluta
2015-01-05 17:56 ` Joe Perches
2015-01-05 17:56 ` Joe Perches
2015-01-10 14:06 ` Jonathan Cameron
2015-01-10 14:06 ` Jonathan Cameron
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.