Re: [PATCH V2 1/1] iio: add Capella CM32181 ambient light sensor driver.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Jonathan,

Thanks for your review. I've modifoed it and post the [PATCH V3 1/1]. Please review it again.

By the way, I did always use the "scripts/checkpatch.pl" before I submit it. It looks like script cannot catch the "i = n-1;" case.

Also, please guide me how to use the staging-next tree. I will modify it to "devm_iio_device_register" API.

Thanks.

Kevin Tsai
12/23/13

----- Original Message ----- From: "Jonathan Cameron" <jic23@xxxxxxxxxx> To: "Kevin Tsai" <ktsai@xxxxxxxxxxxxxxxx>; "Grant Likely" <grant.likely@xxxxxxxxxx>; "Rob Herring" <rob.herring@xxxxxxxxxxx>; "Peter Meerwald" <pmeerw@xxxxxxxxxx>; "Lars-Peter Clausen" <lars@xxxxxxxxxx>; "Oleksandr Kravchenko" <o.v.kravchenko@xxxxxxxxxxxxxxx>; "Jacek Anaszewski" <j.anaszewski@xxxxxxxxxxx>
Cc: <linux-iio@xxxxxxxxxxxxxxx>
Sent: Sunday, December 22, 2013 08:00
Subject: Re: [PATCH V2 1/1] iio: add Capella CM32181 ambient light sensor driver.


On 12/18/13 23:10, Kevin Tsai wrote:
Add Capella Microsystem CM32181 Ambient Light Sensor IIO driver.
This driver will convert raw data to lux value under open-air
condition. Change the calibscale based on the cover material.

Signed-off-by: Kevin Tsai <ktsai@xxxxxxxxxxxxxxxx>
Hi Kevin,

The device tree binding still needs documenting - probably in
Documentation/devicetree/bindings/i2c/trivial-devices.txt

That binding needs to be sent to devicetree@xxxxxxxxxxxxxxx.
The easiest option is probably going to be to do a v3 of this
patch with that included and send it to both linux-iio and devicetree
mailing lists.

There are a few more comments inline.  Mainly little things like putting
function documentation into kernel-doc style and slight tweaks to code
ordering / error paths that make things a little bit cleaner.

Probably just a few minutes work to be ready to go.

Jonathan
---
 drivers/iio/light/Kconfig   |  11 ++
 drivers/iio/light/Makefile  |   1 +
drivers/iio/light/cm32181.c | 354 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 366 insertions(+)
 create mode 100644 drivers/iio/light/cm32181.c

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index a022f27..d12b2a0 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -27,6 +27,17 @@ config APDS9300
 To compile this driver as a module, choose M here: the
 module will be called apds9300.

+config CM32181
+ depends on I2C
+ tristate "CM32181 driver"
+ help
+ Say Y here if you use cm32181.
+ This option enables ambient light sensor using
+ Capella cm32181 device driver.
+
+ To compile this driver as a module, choose M here:
+ the module will be called cm32181.
+
 config CM36651
 depends on I2C
 tristate "CM36651 driver"
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index daa327f..60e35ac 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -5,6 +5,7 @@
 # When adding new entries keep the list in alphabetical order
 obj-$(CONFIG_ADJD_S311) += adjd_s311.o
 obj-$(CONFIG_APDS9300) += apds9300.o
+obj-$(CONFIG_CM32181) += cm32181.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/cm32181.c b/drivers/iio/light/cm32181.c
new file mode 100644
index 0000000..305789b
--- /dev/null
+++ b/drivers/iio/light/cm32181.c
@@ -0,0 +1,354 @@
+/*
+ * Copyright (C) 2013 Capella Microsystems Inc.
+ * Author: Kevin Tsai <ktsai@xxxxxxxxxxxxxxxx>
+ *
+ * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2, as published
+ * by the Free Software Foundation.
+ */
+
+#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 CM32181_REG_ADDR_CMD 0x00
+#define CM32181_REG_ADDR_ALS 0x04
+#define CM32181_REG_ADDR_STATUS 0x06
+#define CM32181_REG_ADDR_ID 0x07
+
+/* Number of Configurable Registers */
+#define CM32181_CONF_REG_NUM 0x01
+
+/* CMD register */
+#define CM32181_CMD_ALS_ENABLE 0x00
+#define CM32181_CMD_ALS_DISABLE 0x01
+#define CM32181_CMD_ALS_INT_EN 0x02
+
+#define CM32181_CMD_ALS_IT_SHIFT 6
+#define CM32181_CMD_ALS_IT_MASK (0x0F << CM32181_CMD_ALS_IT_SHIFT)
+#define CM32181_CMD_ALS_IT_DEFAULT (0x00 << CM32181_CMD_ALS_IT_SHIFT)
+
+#define CM32181_CMD_ALS_SM_SHIFT 11
+#define CM32181_CMD_ALS_SM_MASK (0x03 << CM32181_CMD_ALS_SM_SHIFT)
+#define CM32181_CMD_ALS_SM_DEFAULT (0x01 << CM32181_CMD_ALS_SM_SHIFT)
+
+#define CM32181_MLUX_PER_BIT 5 /* ALS_SM=01 IT=800ms */
+#define CM32181_MLUX_PER_BIT_BASE_IT 800000 /* Based on IT=800ms */
+#define CM32181_CALIBSCALE_RESOLUTION 1000
+#define MLUX_PER_LUX 1000
+
+static const u8 cm32181_reg[CM32181_CONF_REG_NUM] = {
+ CM32181_REG_ADDR_CMD,
+};
+
+static const int als_it_bits[] = {12, 8, 0, 1, 2, 3};
+static int als_it_value[] = {25000, 50000, 100000, 200000, 400000, 800000};
+
+struct cm32181_chip {
+ struct i2c_client *client;
+ struct mutex lock;
+ u16 conf_regs[CM32181_CONF_REG_NUM];
+ int calibscale;
+};
+
+static int cm32181_reg_init(struct cm32181_chip *cm32181)
+{
+ struct i2c_client *client = cm32181->client;
+ int i;
+ s32 ret;
+
+ cm32181->conf_regs[CM32181_REG_ADDR_CMD] = CM32181_CMD_ALS_ENABLE |
+ CM32181_CMD_ALS_IT_DEFAULT | CM32181_CMD_ALS_SM_DEFAULT;
+
+ for (i = 0; i < CM32181_CONF_REG_NUM; i++) {
+ ret = i2c_smbus_write_word_data(client, cm32181_reg[i],
+ cm32181->conf_regs[i]);
+ if (ret < 0)
+ return ret;
+ }
+
+ cm32181->calibscale = 1000;
+
+ return 0;
+}
+
+/*
+ *  Get sensor integration time (ms).
+ *  Return: IIO_VAL_INT for success, otherwise -EINVAL.
+ */
+static int cm32181_read_als_it(struct cm32181_chip *cm32181, int *val)
+{
+ u16 als_it;
+ int i;
+
+ als_it = cm32181->conf_regs[CM32181_REG_ADDR_CMD];
+ als_it &= CM32181_CMD_ALS_IT_MASK;
+ als_it >>= CM32181_CMD_ALS_IT_SHIFT;
+ for (i = 0; i < sizeof(als_it_bits)/sizeof(als_it_bits[0]); i++) {
+ if (als_it == als_it_bits[i]) {
+ *val = als_it_value[i];
+ if (*val == 0)
+ return -EINVAL;
+ return IIO_VAL_INT;
+ }
+ }
+
+ return -EINVAL;
+}
+
+/*
+ * Convert integration time (ms) to sensor value.
+ * Return: i2c_smbus_write_word_data command return value.
+ */
+static int cm32181_write_als_it(struct cm32181_chip *cm32181, int val)
+{
+ struct i2c_client *client = cm32181->client;
+ u16 als_it;
+ int ret, i, n;
+
+ n = sizeof(als_it_value)/sizeof(als_it_value[0]);
+ for (i = 0; i < n; i++)
+ if (val <= als_it_value[i])
+ break;
+ if (i >= n)
Should be spaces around that -.
Please make sure to run scripts/checkpatch.pl which I would have
expected to catch this and any other similar cases..
+ i = n-1;
+
+ als_it = als_it_bits[i];
+ als_it <<= CM32181_CMD_ALS_IT_SHIFT;
+
+ mutex_lock(&cm32181->lock);
+ cm32181->conf_regs[CM32181_REG_ADDR_CMD] &=
+ ~CM32181_CMD_ALS_IT_MASK;
+ cm32181->conf_regs[CM32181_REG_ADDR_CMD] |=
+ als_it;
+ ret = i2c_smbus_write_word_data(client, CM32181_REG_ADDR_CMD,
+ cm32181->conf_regs[CM32181_REG_ADDR_CMD]);
+ mutex_unlock(&cm32181->lock);
+
+ return ret;
+}
+
+/*

Ideally any documentation will be in kernel-doc style
Documentation/kernel-doc-nano-HOWTO.txt

+ * Convert sensor data to lux.
+ * Return: Positive value is lux, otherwise is error code.
+ */
+static int cm32181_get_lux(struct cm32181_chip *cm32181)
+{
+ struct i2c_client *client = cm32181->client;
+ int ret;
+ int als_it;
+ unsigned long lux;
+
+ ret = cm32181_read_als_it(cm32181, &als_it);
+ if (ret < 0)
+ return -EINVAL;
+
+ lux = CM32181_MLUX_PER_BIT;
+ lux *= CM32181_MLUX_PER_BIT_BASE_IT;
+ lux /= als_it;
+
+ ret = i2c_smbus_read_word_data(client, CM32181_REG_ADDR_ALS);
+ if (ret < 0)
+ return ret;
+
+ lux *= ret;
+ lux *= cm32181->calibscale;
+ lux /= CM32181_CALIBSCALE_RESOLUTION;
+ lux /= MLUX_PER_LUX;
+
+ if (lux > 0xFFFF)
+ lux = 0xFFFF;
+
+ return (int)lux;
+}
+
+static int cm32181_read_raw(struct iio_dev *indio_dev,
+     struct iio_chan_spec const *chan,
+     int *val, int *val2, long mask)
+{
+ struct cm32181_chip *cm32181 = iio_priv(indio_dev);
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_PROCESSED:
+ ret = cm32181_get_lux(cm32181);
+ if (ret < 0)
+ return ret;
+ *val = ret;
Might as well return from here...
return IIO_VAL_INT;
instead of bothering with the break.
+ ret = IIO_VAL_INT;
+ break;
+ case IIO_CHAN_INFO_CALIBSCALE:
+ *val = cm32181->calibscale;
+ ret = IIO_VAL_INT;
+ break;
+ case IIO_CHAN_INFO_INT_TIME:
+ ret = cm32181_read_als_it(cm32181, val);
+ if (ret < 0)
+ return ret;
+ break;
+ default:
+ ret = -EINVAL;
+ }
+
+ return ret;
+}
+
+static int cm32181_write_raw(struct iio_dev *indio_dev,
+      struct iio_chan_spec const *chan,
+      int val, int val2, long mask)
+{
+ struct cm32181_chip *cm32181 = iio_priv(indio_dev);
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_CALIBSCALE:
+ cm32181->calibscale = val;
+ ret = val;
+ break;
+ case IIO_CHAN_INFO_INT_TIME:
+ ret = cm32181_write_als_it(cm32181, val);
+ if (ret < 0)
+ return ret;
+ break;
+ default:
+ ret = -EINVAL;
+ }
+
+ return ret;
+}
+

Function should have a prefix. Its just possible someone
will one day introduce a generic get_it_available
and hence cause a name clash.

hence
cm32181_get_it_available(...)
+static ssize_t get_it_available(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int i, n, len;
+
+ n = sizeof(als_it_value)/sizeof(als_it_value[0]);
+ for (i = 0, len = 0; i < n; i++)
+ len += sprintf(buf + len, "%d ", als_it_value[i]);
+ return len + sprintf(buf + len, "\n");
+}
+
+static const struct iio_chan_spec cm32181_channels[] = {
+ {
+ .type = IIO_LIGHT,
+ .indexed = 0,
+ .channel = 0,
+ .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, get_it_available, NULL, 0);
+
+static struct attribute *cm32181_attributes[] = {
+ &iio_dev_attr_in_illuminance_integration_time_available.dev_attr.attr,
+ NULL,
+};
+
+static const struct attribute_group cm32181_attribute_group = {
+ .attrs = cm32181_attributes
+};
+
+static const struct iio_info cm32181_info = {
+ .driver_module = THIS_MODULE,
+ .read_raw = &cm32181_read_raw,
+ .write_raw = &cm32181_write_raw,
+ .attrs = &cm32181_attribute_group,
+};
+
+static int cm32181_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct cm32181_chip *cm32181;
+ struct iio_dev *indio_dev;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*cm32181));
+ if (!indio_dev) {
+ dev_err(&client->dev, "devm_iio_device_alloc failed\n");
+ return -ENOMEM;
+ }
+
+ cm32181 = iio_priv(indio_dev);
+ i2c_set_clientdata(client, indio_dev);
+ cm32181->client = client;
+
+ mutex_init(&cm32181->lock);
+ indio_dev->dev.parent = &client->dev;
+ indio_dev->channels = cm32181_channels;
+ indio_dev->num_channels = ARRAY_SIZE(cm32181_channels);
+ indio_dev->info = &cm32181_info;
+ indio_dev->name = id->name;
+ indio_dev->modes = INDIO_DIRECT_MODE;
+
+ ret = cm32181_reg_init(cm32181);
+ if (ret) {
+ dev_err(&client->dev,
+ "%s: register init failed\n",
+ __func__);
return ret;
+ goto error_disable_reg;
+ }
+
+ ret = iio_device_register(indio_dev);
+ if (ret) {
+ dev_err(&client->dev,
+ "%s: regist device failed\n",
+ __func__);
return ret
+ goto error_disable_reg;
+ }
+
+ return 0;
+
Don't bother with this - just return directly when the errors
occur. See above.
+error_disable_reg:
+ return ret;
+}
+

As mentioned before, the staging-next tree includes
devm_iio_device_register and if you use that, then no
remove funciton will be needed.

+static int cm32181_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 cm32181_id[] = {
+ { "cm32181", 0 },
+ { }
+};
+
+MODULE_DEVICE_TABLE(i2c, cm32181_id);
+
+static const struct of_device_id cm32181_of_match[] = {
+ { .compatible = "capella,cm32181" },
+ { }
+};
+
+static struct i2c_driver cm32181_driver = {
+ .driver = {
+ .name = "cm32181",
+ .of_match_table = of_match_ptr(cm32181_of_match),
+ .owner = THIS_MODULE,
+ },
+ .id_table       = cm32181_id,
+ .probe = cm32181_probe,
+ .remove = cm32181_remove,
+};
+
+module_i2c_driver(cm32181_driver);
+
+MODULE_AUTHOR("Kevin Tsai <ktsai@xxxxxxxxxxxxxxxx>");
+MODULE_DESCRIPTION("CM32181 ambient light sensor driver");
+MODULE_LICENSE("GPL");


--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux