iio:cm36651: Add CM36651 proximity/light sensor driver

PATCH Superseded Beomho Seo 3 participants 6 messages Sep 9, 2013 – Sep 12, 2013 5 messages not in the archive

Subsystems (2)

IIO Capella microsystems light sensor driver

lore

Share & tools

lore

Beomho Seo

author

2013-09-09 06:10 UTC #0 lore raw

PATCH 1/2 iio:cm36651: Add CM36651 proximity/light sensor driver

This patch add a new driver for Capella CM36651 proximity and RGB light sensor.
The driver exposes two channels: light and proximity. It also support detection proximity event.

This driver supports:
- events - rising and falling proximity events.
- reading channels through read_raw callback.

Signed-off-by Beomho Seo <[email protected]>

---
 drivers/iio/light/Kconfig   |   11 +
 drivers/iio/light/cm36651.c |  607
+++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 618 insertions(+)
 create mode 100644 drivers/iio/light/cm36651.c

drivers/iio/light/Kconfig +11 -0

index bf9fa0d..66fb76f 100644
@@ -75,4 +75,15 @@ config VCNL4000
 	 To compile this driver as a module, choose M here: the
 	 module will be called vcnl4000.
 
+config CM36651
+	depends on I2C
+	tristate "CM36651 driver"
+	help
+	  Say Y here if you use cm36651.
+	  This option enables proximity & RGB sensor using
+	  Capella cm36651 device driver.
+
+	  To compile this driver as a module, choose M here:
+	  the module will be called cm36651.
+
 endmenu

drivers/iio/light/cm36651.c new file +6 -0

new file mode 100644
index 0000000..b3e1f0d
@@ -0,0 +1,607 @@
+/*
+ * Copyright (C) 2013 Samsung Electronics Co., Ltd.
+ * Author: Beomho Seo <[email protected]>
+ *
+ * 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/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>
+
+/* slave address 0x19 for PS of 7 bit addressing protocol for I2C */
+#define CM36651_I2C_ADDR_PS 0x19
+
+/* Ambient light sensor */
+#define CS_CONF1 0x00
+#define CS_CONF2 0x01
+#define CS_CONF3 0x06
+
+#define RED 0x00
+#define GREEN 0x01
+#define BLUE 0x02
+#define WHITE 0x03
+
+/* Proximity sensor */
+#define PS_CONF1 0x00
+#define PS_THD 0x01
+#define PS_CANC 0x02
+#define PS_CONF2 0x03
+
+#define ALS_REG_NUM 3
+#define PS_REG_NUM 4
+#define ALS_CHANNEL_NUM 4
+#define INITIAL_THD 0x09
+#define SCAN_MODE_LIGHT 0
+#define SCAN_MODE_PROX 1
+
+enum {
+ CM36651_LIGHT_EN,
+ CM36651_PROXIMITY_EN,
+ CM36651_PROXIMITY_EV_EN,
+};
+
+enum cm36651_cmd {
+ CM36651_CMD_READ_RAW_LIGHT,
+ CM36651_CMD_READ_RAW_PROXIMITY,
+ CM36651_CMD_PROX_EV_EN,
+ CM36651_CMD_PROX_EV_DIS,
+};
+
+enum {
+ CM36651_CLOSE_PROXIMITY,
+ CM36651_FAR_PROXIMITY,
+};
+
+/* register settings */
+static const u8 als_reg_setting[ALS_REG_NUM][2] = {
+ { 0x00, 0x04 }, /* CS_CONF1 */
+ { 0x01, 0x08 }, /* CS_CONF2 */
+ { 0x06, 0x00 } /* CS_CONF3 */
+};
+
+static const u8 ps_reg_setting[PS_REG_NUM][2] = {
+ { 0x00, 0x3C }, /* PS_CONF1 */
+ { 0x01, 0x09 }, /* PS_THD */
+ { 0x02, 0x00 }, /* PS_CANC */
+ { 0x03, 0x13 }, /* PS_CONF2 */
+};
+
+struct cm36651_data {
+ const struct cm36651_platform_data *pdata;
+ struct i2c_client *client;
+ struct i2c_client *ps_client;
+ struct mutex lock;
+ struct regulator *vled_reg;
+ unsigned long flags;
+ u8 thres;
+ u16 color[4];
+};
+
+int cm36651_i2c_read_byte(struct i2c_client *client, u8 *val)
+{
+ int ret = 0;
+ struct i2c_msg msg[1];
+
+ if (!client->adapter)
+ return -ENODEV;
+
+ /* send slave address & command */
+ msg->addr = client->addr;
+ msg->flags = I2C_M_RD;
+ msg->len = 1;
+ msg->buf = val;
+
+ ret = i2c_transfer(client->adapter, msg, 1);
+ if (ret != 1) {
+ dev_err(&client->dev, "i2c transfer error ret: %d\n", ret);
+ ret = -EIO;
+ }
+
+ return ret;
+}
+
+int cm36651_i2c_read_word(struct i2c_client *client, u8 command, u16 *val) +{
+ int ret = 0;
+ struct i2c_msg msg[2];
+ unsigned char data[2] = { 0, };
+
+ if (!client->adapter)
+ return -ENODEV;
+
+ /* send slave address & command */
+ msg[0].addr = client->addr;
+ msg[0].flags = 0;
+ msg[0].len = 1;
+ msg[0].buf = &command;
+
+ /* read word data */
+ msg[1].addr = client->addr;
+ msg[1].flags = I2C_M_RD;
+ msg[1].len = 2;
+ msg[1].buf = data;
+
+ ret = i2c_transfer(client->adapter, msg, 2);
+ if (ret != 2) {
+ dev_err(&client->dev, "i2c transfer error ret: %d\n", ret);
+ ret = -EIO;
+ }
+ *val = le16_to_cpup((__le16 *)data);
+
+ return ret;
+}
+
+int cm36651_i2c_write_byte(struct i2c_client *client, u8 command, u8 val) +{
+ int ret;
+ struct i2c_msg msg[1];
+ unsigned char data[2] = { command, val };
+
+ if (!client->adapter)
+ return -ENODEV;
+
+ /* send slave address & command */
+ msg->addr = client->addr;
+ msg->flags = 0;
+ msg->len = 2;
+ msg->buf = data;
+
+ ret = i2c_transfer(client->adapter, msg, 1);
+ if (ret != 1) {
+ dev_err(&client->dev, "i2c transfer error ret: %d\n", ret);
+ ret = -EIO;
+ }
+
+ return ret;
+}
+
+static int cm36651_setup_reg(struct cm36651_data *cm36651)
+{
+ struct i2c_client *client = cm36651->client;
+ int ret = 0, i = 0;
+ u8 tmp = 0;
+
+ /* ALS initialization */
+ for (i = 0; i < ALS_REG_NUM; i++) {
+ ret = cm36651_i2c_write_byte(client,
+ als_reg_setting[i][0], als_reg_setting[i][1]);
+ if (ret < 0)
+ goto err_setup_reg;
+ }
+
+ /* PS initialization */
+ for (i = 0; i < PS_REG_NUM; i++) {
+ ret = cm36651_i2c_write_byte(cm36651->ps_client,
+ ps_reg_setting[i][0], ps_reg_setting[i][1]);
+ if (ret < 0)
+ goto err_setup_reg;
+ }
+
+ ret = cm36651_i2c_read_byte(cm36651->ps_client, &tmp);
+ if (ret < 0)
+ goto err_setup_reg;
+
+ /* printing the initial proximity value with no contact */
+ dev_dbg(&client->dev, "initial proximity value: %d\n", tmp);
+
+ /* device turn off */
+ ret = cm36651_i2c_write_byte(client, CS_CONF1, 0x01);
+ if (ret < 0)
+ goto err_setup_reg;
+
+ ret = cm36651_i2c_write_byte(cm36651->ps_client, PS_CONF1, 0x01);
+ if (ret < 0)
+ goto err_setup_reg;
+
+ return 0;
+
+err_setup_reg:
+ dev_err(&client->dev, "cm36651_setup_reg() failed: %d\n", ret);
+ return ret;
+}
+
+static int cm36651_read_output(struct cm36651_data *cm36651,
+ int scan_index, int *val)
+{
+ struct i2c_client *client = cm36651->client;
+ int i;
+ int ret = -EINVAL;
+ u8 prox_val;
+
+ switch (scan_index) {
+ case SCAN_MODE_LIGHT:
+ for (i = 0; i < ALS_CHANNEL_NUM; i++) {
+ ret = cm36651_i2c_read_word(client, i,
+ &cm36651->color[i]);
+ if (ret < 0)
+ goto read_err;
+ }
+
+ dev_dbg(&client->dev, "red: %d, green: %d, blue: %d, white: %d\n",
+ cm36651->color[0] + 1, cm36651->color[1] + 1,
+ cm36651->color[2] + 1, cm36651->color[3] + 1);
+
+ ret = cm36651_i2c_write_byte(client, CS_CONF1, 0x01);
+ if (ret < 0)
+ goto write_err;
+
+ break;
+ case SCAN_MODE_PROX:
+ ret = cm36651_i2c_read_byte(cm36651->ps_client, &prox_val);
+ if (ret < 0)
+ goto read_err;
+
+ dev_dbg(&client->dev, "proximity value: %d\n", prox_val);
+
+ ret = cm36651_i2c_write_byte(cm36651->ps_client,
+ PS_CONF1, 0x01);
+ if (ret < 0)
+ goto write_err;
+
+ break;
+ }
+
+ return ret;
+
+read_err:
+ dev_err(&client->dev, "register read failed\n");
+ return ret;
+
+write_err:
+ dev_err(&client->dev, "register write failed\n");
+ return ret;
+}
+
+static irqreturn_t cm36651_irq_handler(int irq, void *data)
+{
+ struct iio_dev *indio_dev = data;
+ struct cm36651_data *cm36651 = iio_priv(indio_dev);
+ struct i2c_client *client = cm36651->client;
+ int ev_dir, val, ret;
+ u64 ev_code;
+ u8 tmp;
+
+ ret = cm36651_i2c_read_byte(cm36651->ps_client, &tmp);
+ if (ret < 0) {
+ dev_err(&client->dev,
+ "%s: data read failed: %d\n", __func__, ret);
+ return IRQ_NONE;
+ }
+
+ if (tmp < ps_reg_setting[1][1]) {
+ ev_dir = IIO_EV_DIR_RISING;
+ val = CM36651_FAR_PROXIMITY;
+ } else {
+ ev_dir = IIO_EV_DIR_FALLING;
+ val = CM36651_CLOSE_PROXIMITY;
+ }
+
+ ev_code = IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY,
+ CM36651_CMD_READ_RAW_PROXIMITY,
+ IIO_EV_TYPE_THRESH, ev_dir);
+
+ iio_push_event(indio_dev, ev_code, iio_get_time_ns());
+
+ return IRQ_HANDLED;
+}
+
+static int cm36651_set_operation_mode(struct cm36651_data *cm36651,
+ enum cm36651_cmd cmd)
+{
+ struct i2c_client *client = cm36651->client;
+ int ret = 0;
+ int i;
+
+ switch (cmd) {
+ case CM36651_CMD_READ_RAW_LIGHT:
+ ret = cm36651_i2c_write_byte(client, CS_CONF1, 0x04);
+ break;
+ case CM36651_CMD_READ_RAW_PROXIMITY:
+ ret = cm36651_i2c_write_byte(cm36651->ps_client,
+ PS_CONF1, 0x3C);
+ break;
+ case CM36651_CMD_PROX_EV_EN:
+ if (test_bit(CM36651_PROXIMITY_EV_EN, &cm36651->flags)) {
+ dev_err(&client->dev,
+ "Already proximity event enable state\n");
+ return ret;
+ }
+ set_bit(CM36651_PROXIMITY_EV_EN, &cm36651->flags);
+ for (i = 0; i < 4; i++) {
+ ret = cm36651_i2c_write_byte(cm36651->ps_client,
+ ps_reg_setting[i][0], ps_reg_setting[i][1]);
+ if (ret < 0)
+ goto write_err;
+ }
+ enable_irq(client->irq);
+ break;
+ case CM36651_CMD_PROX_EV_DIS:
+ if (!test_bit(CM36651_PROXIMITY_EV_EN, &cm36651->flags)) {
+ dev_err(&client->dev,
+ "Already proximity event disable state\n");
+ return ret;
+ }
+ clear_bit(CM36651_PROXIMITY_EV_EN, &cm36651->flags);
+ disable_irq(client->irq);
+ ret = cm36651_i2c_write_byte(cm36651->ps_client,
+ PS_CONF1, 0x01);
+ break;
+ }
+
+ if (ret < 0)
+ dev_err(&client->dev, "write register failed\n");
+
+ return ret;
+
+write_err:
+ dev_err(&client->dev, "proximity event toe enable is failed\n");
+ return ret;
+}
+
+static int cm36651_read_channel(struct cm36651_data *cm36651,
+ struct iio_chan_spec const *chan, int *val)
+{
+ struct i2c_client *client = cm36651->client;
+ enum cm36651_cmd cmd = 0;
+ int ret;
+
+ if (chan->scan_index == SCAN_MODE_LIGHT)
+ cmd = CM36651_CMD_READ_RAW_LIGHT;
+ else /* SCAN_MODE_PROX */
+ cmd = CM36651_CMD_READ_RAW_PROXIMITY;
+
+ ret = cm36651_set_operation_mode(cm36651, cmd);
+ if (ret < 0) {
+ dev_err(&client->dev, "cm36651 set operation mode failed\n");
+ return ret;
+ }
+ /* raw data integration time */
+ msleep(50);
+ ret = cm36651_read_output(cm36651, chan->scan_index, val);
+ if (ret < 0) {
+ dev_err(&client->dev, "cm36651 read output failed\n");
+ return ret;
+ }
+
+ return 0;
+}
+
+static int cm36651_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct cm36651_data *cm36651 = iio_priv(indio_dev);
+ int ret = -EINVAL;
+
+ mutex_lock(&cm36651->lock);
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ ret = cm36651_read_channel(cm36651, chan, val);
+ break;
+ }
+ mutex_unlock(&cm36651->lock);
+
+ return ret;
+}
+
+static int cm36651_read_thresh(struct iio_dev *indio_dev,
+ u64 event_code, int *val)
+{
+ struct cm36651_data *cm36651 = iio_priv(indio_dev);
+ int chan_type = IIO_EVENT_CODE_EXTRACT_CHAN_TYPE(event_code);
+ int event_type = IIO_EVENT_CODE_EXTRACT_TYPE(event_code);
+
+ if (event_type != IIO_EV_TYPE_THRESH || chan_type != IIO_PROXIMITY)
+ return -EINVAL;
+
+ if (!cm36651->thres)
+ *val = ps_reg_setting[1][1]; /* initial threshold value */
+ else
+ *val = cm36651->thres;
+
+ return 0;
+}
+
+static int cm36651_write_thresh(struct iio_dev *indio_dev,
+ u64 event_code, int val)
+{
+ struct cm36651_data *cm36651 = iio_priv(indio_dev);
+ struct i2c_client *client = cm36651->client;
+ int ret;
+
+ cm36651->thres = val;
+ ret = cm36651_i2c_write_byte(cm36651->ps_client,
+ PS_THD, cm36651->thres);
+
+ if (ret < 0) {
+ dev_err(&client->dev, "PS register read faied: %d\n", ret);
+ return ret;
+ }
+ dev_dbg(&client->dev, "new threshold is 0x%x\n", cm36651->thres);
+
+ return 0;
+}
+
+static int cm36651_write_event_config(struct iio_dev *indio_dev,
+ u64 event_code, int state)
+{
+ struct cm36651_data *cm36651 = iio_priv(indio_dev);
+ enum cm36651_cmd cmd;
+ int chan_type = IIO_EVENT_CODE_EXTRACT_CHAN_TYPE(event_code);
+ int ret = -EINVAL;
+
+ mutex_lock(&cm36651->lock);
+
+ if (chan_type == IIO_PROXIMITY) {
+ cmd = state ? CM36651_CMD_PROX_EV_EN : CM36651_CMD_PROX_EV_DIS;
+ ret = cm36651_set_operation_mode(cm36651, cmd);
+ }
+
+ mutex_unlock(&cm36651->lock);
+
+ return ret;
+}
+
+static int cm36651_read_event_config(struct iio_dev *indio_dev, u64 event_code)
+{
+ struct cm36651_data *cm36651 = iio_priv(indio_dev);
+ int chan_type = IIO_EVENT_CODE_EXTRACT_CHAN_TYPE(event_code);
+ int event_en = -EINVAL;
+
+ mutex_lock(&cm36651->lock);
+
+ if (chan_type == IIO_PROXIMITY)
+ event_en = test_bit(CM36651_PROXIMITY_EV_EN, &cm36651->flags);
+
+ mutex_unlock(&cm36651->lock);
+
+ return event_en;
+}
+
+static const struct iio_chan_spec cm36651_channels[] = {
+ {
+ .type = IIO_LIGHT,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+ .scan_type = {
+ .sign = 'u',
+ .realbits = 16,
+ .shift = 0,
+ .storagebits = 16,
+ },
+ .scan_index = SCAN_MODE_LIGHT
+ },
+ {
+ .type = IIO_PROXIMITY,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+ .scan_type = {
+ .sign = 'u',
+ .realbits = 8,
+ .shift = 0,
+ .storagebits = 8,
+ },
+ .scan_index = SCAN_MODE_PROX,
+ .event_mask = IIO_EV_BIT(IIO_EV_TYPE_THRESH, IIO_EV_DIR_EITHER)
+ },
+};
+
+static const struct iio_info cm36651_info = {
+ .driver_module = THIS_MODULE,
+ .read_raw = &cm36651_read_raw,
+ .read_event_value = &cm36651_read_thresh,
+ .write_event_value = &cm36651_write_thresh,
+ .read_event_config = &cm36651_read_event_config,
+ .write_event_config = &cm36651_write_event_config,
+};
+
+static int cm36651_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct cm36651_data *cm36651;
+ struct iio_dev *indio_dev;
+ unsigned long irqflag;
+ int ret;
+
+ dev_dbg(&client->dev, "cm36651 light/proxymity sensor probe\n");
+
+ indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*cm36651));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ cm36651 = iio_priv(indio_dev);
+
+ cm36651->vled_reg = devm_regulator_get(&client->dev, "vled");
+ if (IS_ERR(cm36651->vled_reg)) {
+ dev_err(&client->dev, "get regulator vled failed\n");
+ return PTR_ERR(cm36651->vled_reg);
+ }
+
+ ret = regulator_enable(cm36651->vled_reg);
+ if (ret) {
+ dev_err(&client->dev, "enable regulator failed\n");
+ return ret;
+ }
+
+ i2c_set_clientdata(client, indio_dev);
+
+ cm36651->client = client;
+ cm36651->ps_client = i2c_new_dummy(client->adapter,
+ CM36651_I2C_ADDR_PS);
+ mutex_init(&cm36651->lock);
+ indio_dev->dev.parent = &client->dev;
+ indio_dev->channels = cm36651_channels;
+ indio_dev->num_channels = ARRAY_SIZE(cm36651_channels);
+ indio_dev->info = &cm36651_info;
+ indio_dev->name = id->name;
+ indio_dev->modes = INDIO_DIRECT_MODE;
+
+ ret = cm36651_setup_reg(cm36651);
+
+ irqflag = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT;
+ ret = request_threaded_irq(client->irq, NULL, cm36651_irq_handler,
+ irqflag, "cm36651_proximity", indio_dev);
+ if (ret) {
+ dev_err(&client->dev, "%s: request irq failed\n", __func__);
+ return ret;
+ }
+ disable_irq(client->irq);
+
+ ret = iio_device_register(indio_dev);
+ if (ret) {
+ regulator_disable(cm36651->vled_reg);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int cm36651_remove(struct i2c_client *client)
+{
+ struct iio_dev *indio_dev = i2c_get_clientdata(client);
+ struct cm36651_data *cm36651 = iio_priv(indio_dev);
+
+ iio_device_unregister(indio_dev);
+ regulator_disable(cm36651->vled_reg);
+ free_irq(client->irq, indio_dev);
+ iio_device_free(indio_dev);
+
+ return 0;
+}
+
+static const struct i2c_device_id cm36651_id[] = {
+ { "cm36651", 0 },
+ { }
+};
+
+MODULE_DEVICE_TABLE(i2c, cm36651_id);
+
+static const struct of_device_id cm36651_of_match[] = {
+ { .compatible = "capella,cm36651" },
+ { }
+};
+
+static struct i2c_driver cm36651_driver = {
+ .driver = {
+ .name = "cm36651",
+ .of_match_table = of_match_ptr(cm36651_of_match),
+ .owner = THIS_MODULE,
+ },
+ .probe = cm36651_probe,
+ .remove = cm36651_remove,
+ .id_table = cm36651_id,
+};
+
+module_i2c_driver(cm36651_driver);
+
+MODULE_AUTHOR("Beomho Seo <[email protected]>"); +MODULE_DESCRIPTION("CM36651 proximity/ambient light sensor driver"); +MODULE_LICENSE("GPL v2");

Signature · 10 lines

1.7.9.5

-- 
Beomho Seo, Assistant Engineer
System S/W Lab., S/W Platform Team, Software Center
Samsung Electronics
-- 
Beomho Seo, Assistant Engineer
System S/W Lab., S/W Platform Team, Software Center
Samsung Electronics

A message that is not in the archive (sent off-list or not yet received): [email protected]

Peter Meerwald

2013-09-09 07:57 UTC #2 lore raw

Quoted 5 lines

This patch add a new driver for Capella CM36651 proximity and RGB light sensor.
The driver exposes two channels: light and proximity. It also support detection proximity event.

comments inline;
I do not have the chip, but I don't think that the code will work and intended

Quoted 17 lines

This driver supports:
- events - rising and falling proximity events.
- reading channels through read_raw callback.
Signed-off-by Beomho Seo <[email protected]>
---
 drivers/iio/light/Kconfig   |   11 +
 drivers/iio/light/cm36651.c |  607
+++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 618 insertions(+)
 create mode 100644 drivers/iio/light/cm36651.c
drivers/iio/light/Kconfig +0 -0
index bf9fa0d..66fb76f 100644

drivers/iio/light/Kconfig config VCNL4000 · line 78

Quoted 4 lines

drivers/iio/light/Kconfig +1 -0

@@ -75,4 +75,15 @@ config VCNL4000
 	 To compile this driver as a module, choose M here: the
 	 module will be called vcnl4000.
 
+config CM36651

sensors to be listed in alphabetical order

Quoted 28 lines

drivers/iio/light/Kconfig +10 -0

+	depends on I2C
+	tristate "CM36651 driver"
+	help
+	  Say Y here if you use cm36651.
+	  This option enables proximity & RGB sensor using
+	  Capella cm36651 device driver.
+
+	  To compile this driver as a module, choose M here:
+	  the module will be called cm36651.
+
 endmenu

drivers/iio/light/cm36651.c new file +6 -0

new file mode 100644
index 0000000..b3e1f0d
@@ -0,0 +1,607 @@
+/*
+ * Copyright (C) 2013 Samsung Electronics Co., Ltd.
+ * Author: Beomho Seo <[email protected]>
+ *
+ * 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.
+ */

drivers/iio/light/cm36651.c new file +2 -0

+
+#include <linux/delay.h>

drivers/iio/light/cm36651.c

Quoted 10 lines

drivers/iio/light/cm36651.c new file +10 -0

+#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>
+
+/* slave address 0x19 for PS of 7 bit addressing protocol for I2C */

consistently start comments with uppercase letter

Quoted 1 line

+#define CM36651_I2C_ADDR_PS 0x19

interesting, the chip seems to have two i2c addresses, the other one is 0x18

is this really one chip? an option would be to do two drivers: one for ALS, one for proximity

Quoted 35 lines

drivers/iio/light/cm36651.c new file +35 -0

+
+/* Ambient light sensor */
+#define CS_CONF1	0x00
+#define CS_CONF2	0x01
+#define CS_CONF3	0x06
+
+#define RED		0x00
+#define GREEN		0x01
+#define BLUE		0x02
+#define WHITE		0x03
+
+/* Proximity sensor */
+#define PS_CONF1	0x00
+#define PS_THD		0x01
+#define PS_CANC		0x02
+#define PS_CONF2	0x03
+
+#define ALS_REG_NUM	3
+#define PS_REG_NUM	4
+#define ALS_CHANNEL_NUM	4
+#define INITIAL_THD	0x09
+#define SCAN_MODE_LIGHT	0
+#define SCAN_MODE_PROX	1
+
+enum {
+	CM36651_LIGHT_EN,
+	CM36651_PROXIMITY_EN,
+	CM36651_PROXIMITY_EV_EN,
+};
+
+enum cm36651_cmd {
+	CM36651_CMD_READ_RAW_LIGHT,
+	CM36651_CMD_READ_RAW_PROXIMITY,
+	CM36651_CMD_PROX_EV_EN,
+	CM36651_CMD_PROX_EV_DIS,

drivers/iio/light/cm36651.c

Quoted 10 lines

drivers/iio/light/cm36651.c new file +10 -0

+};
+
+enum {
+	CM36651_CLOSE_PROXIMITY,
+	CM36651_FAR_PROXIMITY,
+};
+
+/* register settings */
+static const u8 als_reg_setting[ALS_REG_NUM][2] = {
+	{ 0x00, 0x04 },	/* CS_CONF1 */

use your #defines instead of a comment, e.g. { CS_CONF1, 0x04 }

what are the magic constants 0x04, 0x08?

Quoted 34 lines

drivers/iio/light/cm36651.c new file +34 -0

+	{ 0x01, 0x08 },	/* CS_CONF2 */
+	{ 0x06, 0x00 }	/* CS_CONF3 */
+};
+
+static const u8 ps_reg_setting[PS_REG_NUM][2] = {
+	{ 0x00, 0x3C },	/* PS_CONF1 */
+	{ 0x01, 0x09 },	/* PS_THD */
+	{ 0x02, 0x00 },	/* PS_CANC */
+	{ 0x03, 0x13 },	/* PS_CONF2 */
+};
+
+struct cm36651_data {
+	const struct cm36651_platform_data *pdata;
+	struct i2c_client *client;
+	struct i2c_client *ps_client;
+	struct mutex lock;
+	struct regulator *vled_reg;
+	unsigned long flags;
+	u8 thres;
+	u16 color[4];
+};
+
+int cm36651_i2c_read_byte(struct i2c_client *client, u8 *val)
+{
+	int ret = 0;
+	struct i2c_msg msg[1];
+
+	if (!client->adapter)
+		return -ENODEV;
+
+	/* send slave address & command */
+	msg->addr = client->addr;
+	msg->flags = I2C_M_RD;
+	msg->len = 1;

drivers/iio/light/cm36651.c

Quoted 10 lines

drivers/iio/light/cm36651.c new file +10 -0

+	msg->buf = val;
+
+	ret = i2c_transfer(client->adapter, msg, 1);
+	if (ret != 1) {
+		dev_err(&client->dev, "i2c transfer error ret: %d\n", ret);
+		ret = -EIO;
+	}
+
+	return ret;
+}

use i2c_smbus_read_byte_data() instead?

Quoted 21 lines

drivers/iio/light/cm36651.c new file +21 -0

+
+int cm36651_i2c_read_word(struct i2c_client *client, u8 command, u16 *val)
+{
+	int ret = 0;
+	struct i2c_msg msg[2];
+	unsigned char data[2] = { 0, };
+
+	if (!client->adapter)
+		return -ENODEV;
+
+	/* send slave address & command */
+	msg[0].addr = client->addr;
+	msg[0].flags = 0;
+	msg[0].len = 1;
+	msg[0].buf = &command;
+
+	/* read word data */
+	msg[1].addr = client->addr;
+	msg[1].flags = I2C_M_RD;
+	msg[1].len = 2;
+	msg[1].buf = data;

drivers/iio/light/cm36651.c

Quoted 10 lines

drivers/iio/light/cm36651.c new file +10 -0

+
+	ret = i2c_transfer(client->adapter, msg, 2);
+	if (ret != 2) {
+		dev_err(&client->dev, "i2c transfer error ret: %d\n", ret);
+		ret = -EIO;
+	}
+	*val = le16_to_cpup((__le16 *)data);
+
+	return ret;
+}

use i2c_smbus_read_word_data() instead?

Quoted 14 lines

drivers/iio/light/cm36651.c new file +14 -0

+
+int cm36651_i2c_write_byte(struct i2c_client *client, u8 command, u8 val)
+{
+	int ret;
+	struct i2c_msg msg[1];
+	unsigned char data[2] = { command, val };
+
+	if (!client->adapter)
+		return -ENODEV;
+
+	/* send slave address & command */
+	msg->addr = client->addr;
+	msg->flags = 0;
+	msg->len = 2;

drivers/iio/light/cm36651.c

Quoted 10 lines

drivers/iio/light/cm36651.c new file +10 -0

+	msg->buf = data;
+
+	ret = i2c_transfer(client->adapter, msg, 1);
+	if (ret != 1) {
+		dev_err(&client->dev, "i2c transfer error ret: %d\n", ret);
+		ret = -EIO;
+	}
+
+	return ret;
+}

use i2c_smbus_write_byte_data() instead?

drivers/iio/light/cm36651.c

Quoted 5 lines

drivers/iio/light/cm36651.c new file +5 -0

+
+static int cm36651_setup_reg(struct cm36651_data *cm36651)
+{
+	struct i2c_client *client = cm36651->client;
+	int ret = 0, i = 0;

no need to initialize ret, tmp and i

Quoted 17 lines

drivers/iio/light/cm36651.c new file +17 -0

+	u8 tmp = 0;
+
+	/* ALS initialization */
+	for (i = 0; i < ALS_REG_NUM; i++) {
+		ret = cm36651_i2c_write_byte(client,
+			als_reg_setting[i][0], als_reg_setting[i][1]);
+		if (ret < 0)
+			goto err_setup_reg;
+	}
+
+	/* PS initialization */
+	for (i = 0; i < PS_REG_NUM; i++) {
+		ret = cm36651_i2c_write_byte(cm36651->ps_client,
+			ps_reg_setting[i][0], ps_reg_setting[i][1]);
+		if (ret < 0)
+			goto err_setup_reg;
+	}

drivers/iio/light/cm36651.c

Quoted 10 lines

drivers/iio/light/cm36651.c new file +10 -0

+
+	ret = cm36651_i2c_read_byte(cm36651->ps_client, &tmp);
+	if (ret < 0)
+		goto err_setup_reg;
+
+	/* printing the initial proximity value with no contact */
+	dev_dbg(&client->dev, "initial proximity value: %d\n", tmp);
+
+	/* device turn off */
+	ret = cm36651_i2c_write_byte(client, CS_CONF1, 0x01);

use a #define for 0x01 to describe function

Quoted 7 lines

drivers/iio/light/cm36651.c new file +7 -0

+	if (ret < 0)
+		goto err_setup_reg;
+
+	ret = cm36651_i2c_write_byte(cm36651->ps_client, PS_CONF1, 0x01);
+	if (ret < 0)
+		goto err_setup_reg;
+

drivers/iio/light/cm36651.c

Quoted 10 lines

drivers/iio/light/cm36651.c new file +10 -0

+	return 0;
+
+err_setup_reg:
+	dev_err(&client->dev, "cm36651_setup_reg() failed: %d\n", ret);
+	return ret;
+}
+
+static int cm36651_read_output(struct cm36651_data *cm36651,
+						int scan_index, int *val)
+{

val is not used?

drivers/iio/light/cm36651.c

Quoted 10 lines

drivers/iio/light/cm36651.c new file +10 -0

+	struct i2c_client *client = cm36651->client;
+	int i;
+	int ret = -EINVAL;
+	u8 prox_val;
+
+	switch (scan_index) {
+	case SCAN_MODE_LIGHT:
+		for (i = 0; i < ALS_CHANNEL_NUM; i++) {
+			ret = cm36651_i2c_read_word(client, i,
+						&cm36651->color[i]);

I think this is wrong: i is 0,1,2,3,4 but CS_CONF1, CS_CONF2 is 0, 1 as well -- so the config registers are read here?

the channel data (red,green,blue,clear) is at addresses 0x02 .. 0x05 (according to
https://github.com/mozilla-b2g/kernel-android-galaxy-s2-ics/blob/master/drivers/sensor/cm36651.c)

drivers/iio/light/cm36651.c

Quoted 9 lines

drivers/iio/light/cm36651.c new file +9 -0

+			if (ret < 0)
+				goto read_err;
+		}
+
+		dev_dbg(&client->dev, "red: %d, green: %d, blue: %d, white: %d\n",
+				cm36651->color[0] + 1, cm36651->color[1] + 1,
+				cm36651->color[2] + 1, cm36651->color[3] + 1);
+
+		ret = cm36651_i2c_write_byte(client, CS_CONF1, 0x01);

what does this do?

drivers/iio/light/cm36651.c

Quoted 10 lines

drivers/iio/light/cm36651.c new file +10 -0

+		if (ret < 0)
+			goto write_err;
+
+		break;
+	case SCAN_MODE_PROX:
+		ret = cm36651_i2c_read_byte(cm36651->ps_client, &prox_val);
+		if (ret < 0)
+			goto read_err;
+
+		dev_dbg(&client->dev, "proximity value: %d\n", prox_val);

prox_val is not returned?

Quoted 19 lines

drivers/iio/light/cm36651.c new file +19 -0

+
+		ret = cm36651_i2c_write_byte(cm36651->ps_client,
+							PS_CONF1, 0x01);
+		if (ret < 0)
+			goto write_err;
+
+		break;
+	}
+
+	return ret;
+
+read_err:
+	dev_err(&client->dev, "register read failed\n");
+	return ret;
+
+write_err:
+	dev_err(&client->dev, "register write failed\n");
+	return ret;
+}

drivers/iio/light/cm36651.c

Quoted 10 lines

drivers/iio/light/cm36651.c new file +10 -0

+
+static irqreturn_t cm36651_irq_handler(int irq, void *data)
+{
+	struct iio_dev *indio_dev = data;
+	struct cm36651_data *cm36651 = iio_priv(indio_dev);
+	struct i2c_client *client = cm36651->client;
+	int ev_dir, val, ret;
+	u64 ev_code;
+	u8 tmp;
+

tmp must be initialized with the command?

drivers/iio/light/cm36651.c

Quoted 5 lines

drivers/iio/light/cm36651.c new file +5 -0

+	ret = cm36651_i2c_read_byte(cm36651->ps_client, &tmp);
+	if (ret < 0) {
+		dev_err(&client->dev,
+				"%s: data read failed: %d\n", __func__, ret);
+		return IRQ_NONE;

should always return IRQ_HANDLED

Quoted 59 lines

drivers/iio/light/cm36651.c new file +59 -0

+	}
+
+	if (tmp < ps_reg_setting[1][1]) {
+		ev_dir = IIO_EV_DIR_RISING;
+		val = CM36651_FAR_PROXIMITY;
+	} else {
+		ev_dir = IIO_EV_DIR_FALLING;
+		val = CM36651_CLOSE_PROXIMITY;
+	}
+
+	ev_code = IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY,
+				CM36651_CMD_READ_RAW_PROXIMITY,
+				IIO_EV_TYPE_THRESH, ev_dir);
+
+	iio_push_event(indio_dev, ev_code, iio_get_time_ns());
+
+	return IRQ_HANDLED;
+}
+
+static int cm36651_set_operation_mode(struct cm36651_data *cm36651,
+						enum cm36651_cmd cmd)
+{
+	struct i2c_client *client = cm36651->client;
+	int ret = 0;
+	int i;
+
+	switch (cmd) {
+	case CM36651_CMD_READ_RAW_LIGHT:
+		ret = cm36651_i2c_write_byte(client, CS_CONF1, 0x04);
+		break;
+	case CM36651_CMD_READ_RAW_PROXIMITY:
+		ret = cm36651_i2c_write_byte(cm36651->ps_client,
+							PS_CONF1, 0x3C);
+		break;
+	case CM36651_CMD_PROX_EV_EN:
+		if (test_bit(CM36651_PROXIMITY_EV_EN, &cm36651->flags)) {
+			dev_err(&client->dev,
+				"Already proximity event enable state\n");
+			return ret;
+		}
+		set_bit(CM36651_PROXIMITY_EV_EN, &cm36651->flags);
+		for (i = 0; i < 4; i++) {
+			ret = cm36651_i2c_write_byte(cm36651->ps_client,
+				ps_reg_setting[i][0], ps_reg_setting[i][1]);
+			if (ret < 0)
+				goto write_err;
+		}
+		enable_irq(client->irq);
+		break;
+	case CM36651_CMD_PROX_EV_DIS:
+		if (!test_bit(CM36651_PROXIMITY_EV_EN, &cm36651->flags)) {
+			dev_err(&client->dev,
+				"Already proximity event disable state\n");
+			return ret;
+		}
+		clear_bit(CM36651_PROXIMITY_EV_EN, &cm36651->flags);
+		disable_irq(client->irq);
+		ret = cm36651_i2c_write_byte(cm36651->ps_client,
+							PS_CONF1, 0x01);

drivers/iio/light/cm36651.c

Quoted 10 lines

drivers/iio/light/cm36651.c new file +10 -0

+		break;
+	}
+
+	if (ret < 0)
+		dev_err(&client->dev, "write register failed\n");
+
+	return ret;
+
+write_err:
+	dev_err(&client->dev, "proximity event toe enable is failed\n");

typo: toe; rephase

Quoted 13 lines

drivers/iio/light/cm36651.c new file +13 -0

+	return ret;
+}
+
+static int cm36651_read_channel(struct cm36651_data *cm36651,
+				struct iio_chan_spec const *chan, int *val)
+{
+	struct i2c_client *client = cm36651->client;
+	enum cm36651_cmd cmd = 0;
+	int ret;
+
+	if (chan->scan_index == SCAN_MODE_LIGHT)
+		cmd = CM36651_CMD_READ_RAW_LIGHT;
+	else /* SCAN_MODE_PROX */

drivers/iio/light/cm36651.c

Quoted 10 lines

drivers/iio/light/cm36651.c new file +10 -0

+		cmd = CM36651_CMD_READ_RAW_PROXIMITY;
+
+	ret = cm36651_set_operation_mode(cm36651, cmd);
+	if (ret < 0) {
+		dev_err(&client->dev, "cm36651 set operation mode failed\n");
+		return ret;
+	}
+	/* raw data integration time */
+	msleep(50);
+	ret = cm36651_read_output(cm36651, chan->scan_index, val);

_read_output() does not return data in val!

Quoted 171 lines

drivers/iio/light/cm36651.c new file +86 -0

+	if (ret < 0) {
+		dev_err(&client->dev, "cm36651 read output failed\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int cm36651_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int *val, int *val2, long mask)
+{
+	struct cm36651_data *cm36651 = iio_priv(indio_dev);
+	int ret = -EINVAL;
+
+	mutex_lock(&cm36651->lock);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = cm36651_read_channel(cm36651, chan, val);
+		break;
+	}
+	mutex_unlock(&cm36651->lock);
+
+	return ret;
+}
+
+static int cm36651_read_thresh(struct iio_dev *indio_dev,
+					u64 event_code, int *val)
+{
+	struct cm36651_data *cm36651 = iio_priv(indio_dev);
+	int chan_type = IIO_EVENT_CODE_EXTRACT_CHAN_TYPE(event_code);
+	int event_type = IIO_EVENT_CODE_EXTRACT_TYPE(event_code);
+
+	if (event_type != IIO_EV_TYPE_THRESH ||	chan_type != IIO_PROXIMITY)
+		return -EINVAL;
+
+	if (!cm36651->thres)
+		*val = ps_reg_setting[1][1]; /* initial threshold value */
+	else
+		*val = cm36651->thres;
+
+	return 0;
+}
+
+static int cm36651_write_thresh(struct iio_dev *indio_dev,
+					u64 event_code, int val)
+{
+	struct cm36651_data *cm36651 = iio_priv(indio_dev);
+	struct i2c_client *client = cm36651->client;
+	int ret;
+
+	cm36651->thres = val;
+	ret = cm36651_i2c_write_byte(cm36651->ps_client,
+					PS_THD, cm36651->thres);
+
+	if (ret < 0) {
+		dev_err(&client->dev, "PS register read faied: %d\n", ret);
+		return ret;
+	}
+	dev_dbg(&client->dev, "new threshold is 0x%x\n", cm36651->thres);
+
+	return 0;
+}
+
+static int cm36651_write_event_config(struct iio_dev *indio_dev,
+					u64 event_code, int state)
+{
+	struct cm36651_data *cm36651 = iio_priv(indio_dev);
+	enum cm36651_cmd cmd;
+	int chan_type = IIO_EVENT_CODE_EXTRACT_CHAN_TYPE(event_code);
+	int ret = -EINVAL;
+
+	mutex_lock(&cm36651->lock);
+
+	if (chan_type == IIO_PROXIMITY) {
+		cmd = state ? CM36651_CMD_PROX_EV_EN : CM36651_CMD_PROX_EV_DIS;
+		ret = cm36651_set_operation_mode(cm36651, cmd);
+	}
+
+	mutex_unlock(&cm36651->lock);
+
+	return ret;
+}
+
+static int cm36651_read_event_config(struct iio_dev *indio_dev, u64

event_code)

drivers/iio/light/cm36651.c new file +84 -0

+{
+	struct cm36651_data *cm36651 = iio_priv(indio_dev);
+	int chan_type = IIO_EVENT_CODE_EXTRACT_CHAN_TYPE(event_code);
+	int event_en = -EINVAL;
+
+	mutex_lock(&cm36651->lock);
+
+	if (chan_type == IIO_PROXIMITY)
+		event_en = test_bit(CM36651_PROXIMITY_EV_EN, &cm36651->flags);
+
+	mutex_unlock(&cm36651->lock);
+
+	return event_en;
+}
+
+static const struct iio_chan_spec cm36651_channels[] = {
+	{
+		.type = IIO_LIGHT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.shift = 0,
+			.storagebits = 16,
+		},
+		.scan_index = SCAN_MODE_LIGHT
+	},
+	{
+		.type = IIO_PROXIMITY,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 8,
+			.shift = 0,
+			.storagebits = 8,
+		},
+		.scan_index = SCAN_MODE_PROX,
+		.event_mask = IIO_EV_BIT(IIO_EV_TYPE_THRESH, IIO_EV_DIR_EITHER)
+	},
+};
+
+static const struct iio_info cm36651_info = {
+	.driver_module		= THIS_MODULE,
+	.read_raw		= &cm36651_read_raw,
+	.read_event_value	= &cm36651_read_thresh,
+	.write_event_value	= &cm36651_write_thresh,
+	.read_event_config	= &cm36651_read_event_config,
+	.write_event_config	= &cm36651_write_event_config,
+};
+
+static int cm36651_probe(struct i2c_client *client,
+			     const struct i2c_device_id *id)
+{
+	struct cm36651_data *cm36651;
+	struct iio_dev *indio_dev;
+	unsigned long irqflag;
+	int ret;
+
+	dev_dbg(&client->dev, "cm36651 light/proxymity sensor probe\n");
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*cm36651));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	cm36651 = iio_priv(indio_dev);
+
+	cm36651->vled_reg = devm_regulator_get(&client->dev, "vled");
+	if (IS_ERR(cm36651->vled_reg)) {
+		dev_err(&client->dev, "get regulator vled failed\n");
+		return PTR_ERR(cm36651->vled_reg);
+	}
+
+	ret = regulator_enable(cm36651->vled_reg);
+	if (ret) {
+		dev_err(&client->dev, "enable regulator failed\n");
+		return ret;
+	}
+
+	i2c_set_clientdata(client, indio_dev);
+
+	cm36651->client = client;
+	cm36651->ps_client = i2c_new_dummy(client->adapter,
+						CM36651_I2C_ADDR_PS);
+	mutex_init(&cm36651->lock);

drivers/iio/light/cm36651.c

Quoted 10 lines

drivers/iio/light/cm36651.c new file +10 -0

+	indio_dev->dev.parent = &client->dev;
+	indio_dev->channels = cm36651_channels;
+	indio_dev->num_channels = ARRAY_SIZE(cm36651_channels);
+	indio_dev->info = &cm36651_info;
+	indio_dev->name = id->name;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	ret = cm36651_setup_reg(cm36651);
+
+	irqflag = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT;

rising and falling?

drivers/iio/light/cm36651.c

Quoted 11 lines

drivers/iio/light/cm36651.c new file +11 -0

+	ret = request_threaded_irq(client->irq, NULL, cm36651_irq_handler,
+				irqflag, "cm36651_proximity", indio_dev);
+	if (ret) {
+		dev_err(&client->dev, "%s: request irq failed\n", __func__);
+		return ret;
+	}
+	disable_irq(client->irq);
+
+	ret = iio_device_register(indio_dev);
+	if (ret) {
+		regulator_disable(cm36651->vled_reg);

irq not freed

Quoted 5 lines

drivers/iio/light/cm36651.c new file +5 -0
+		return ret;
+	}
+
+	return 0;
+}

drivers/iio/light/cm36651.c

Quoted 10 lines

drivers/iio/light/cm36651.c new file +10 -0

+
+static int cm36651_remove(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+	struct cm36651_data *cm36651 = iio_priv(indio_dev);
+
+	iio_device_unregister(indio_dev);
+	regulator_disable(cm36651->vled_reg);
+	free_irq(client->irq, indio_dev);
+	iio_device_free(indio_dev);

iio_device_free() not needed since using devm_iio_device_alloc()

Quoted 33 lines

drivers/iio/light/cm36651.c new file +32 -0

+
+	return 0;
+}
+
+static const struct i2c_device_id cm36651_id[] = {
+	{ "cm36651", 0 },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(i2c, cm36651_id);
+
+static const struct of_device_id cm36651_of_match[] = {
+	{ .compatible = "capella,cm36651" },
+	{ }
+};
+
+static struct i2c_driver cm36651_driver = {
+	.driver = {
+		.name	= "cm36651",
+		.of_match_table = of_match_ptr(cm36651_of_match),
+		.owner	= THIS_MODULE,
+	},
+	.probe		= cm36651_probe,
+	.remove		= cm36651_remove,
+	.id_table	= cm36651_id,
+};
+
+module_i2c_driver(cm36651_driver);
+
+MODULE_AUTHOR("Beomho Seo <[email protected]>");
+MODULE_DESCRIPTION("CM36651 proximity/ambient light sensor driver");
+MODULE_LICENSE("GPL v2");

Signature · 2 lines

Peter Meerwald
+43-664-2444418 (mobile)

A message that is not in the archive (sent off-list or not yet received): alpine.DEB.2.01.1309090925290.26885-jW+XmwGofnusTnJN9+BGXg@public.gmane.org

Beomho Seo

author

2013-09-09 12:14 UTC #4 lore raw

Hello, Mr. Meerwald

Thank you for your advice.

2013년 09월 09일 16:57, Peter Meerwald 쓴 글:

Quoted 10 lines

Quoted 5 lines
This patch add a new driver for Capella CM36651 proximity and RGB light sensor.
The driver exposes two channels: light and proximity. It also support detection proximity event.
comments inline;
I do not have the chip, but I don't think that the code will work and intended

After I have made this driver, I have checked device working on test board.

Quoted 20 lines

Quoted 20 lines
This driver supports:
- events - rising and falling proximity events.
- reading channels through read_raw callback.
Signed-off-by Beomho Seo <[email protected]>
---
 drivers/iio/light/Kconfig   |   11 +
 drivers/iio/light/cm36651.c |  607
+++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 618 insertions(+)
 create mode 100644 drivers/iio/light/cm36651.c
drivers/iio/light/Kconfig +0 -0
index bf9fa0d..66fb76f 100644
@@ -75,4 +75,15 @@ config VCNL4000
 	 To compile this driver as a module, choose M here: the
 	 module will be called vcnl4000.
 

Quoted 4 lines

drivers/iio/light/Kconfig config VCNL4000 · line 78
Quoted 1 line
drivers/iio/light/Kconfig +1 -0
+config CM36651
sensors to be listed in alphabetical order

I have fixed it.

Quoted 31 lines

drivers/iio/light/Kconfig +10 -0

+	depends on I2C
+	tristate "CM36651 driver"
+	help
+	  Say Y here if you use cm36651.
+	  This option enables proximity & RGB sensor using
+	  Capella cm36651 device driver.
+
+	  To compile this driver as a module, choose M here:
+	  the module will be called cm36651.
+
 endmenu

drivers/iio/light/cm36651.c new file +6 -0

new file mode 100644
index 0000000..b3e1f0d
@@ -0,0 +1,607 @@
+/*
+ * Copyright (C) 2013 Samsung Electronics Co., Ltd.
+ * Author: Beomho Seo <[email protected]>
+ *
+ * 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.
+ */

drivers/iio/light/cm36651.c new file +5 -0

+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/mutex.h>
+#include <linux/module.h>

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 7 lines
drivers/iio/light/cm36651.c new file +7 -0
+#include <linux/interrupt.h>
+#include <linux/regulator/consumer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/events.h>
+
+/* slave address 0x19 for PS of 7 bit addressing protocol for I2C */
consistently start comments with uppercase letter

I use uppercase letter starting comments.

Quoted 8 lines

Quoted 1 line
+#define CM36651_I2C_ADDR_PS 0x19
interesting, the chip seems to have two i2c addresses, the other one is 0x18
is this really one chip? an option would be to do two drivers: one for ALS, one for proximity

CM36651 is one chip. I have checked it. CM36651 apply slave address 0x18 for CS and 0x19 for PS of 7 bit addressing protocol for I2C. CM36651 contains are 8-bit command register following each of slave address. All operations can ve contrroled by the command register.

Quoted 48 lines

Quoted 35 lines

drivers/iio/light/cm36651.c new file +35 -0

+
+/* Ambient light sensor */
+#define CS_CONF1	0x00
+#define CS_CONF2	0x01
+#define CS_CONF3	0x06
+
+#define RED		0x00
+#define GREEN		0x01
+#define BLUE		0x02
+#define WHITE		0x03
+
+/* Proximity sensor */
+#define PS_CONF1	0x00
+#define PS_THD		0x01
+#define PS_CANC		0x02
+#define PS_CONF2	0x03
+
+#define ALS_REG_NUM	3
+#define PS_REG_NUM	4
+#define ALS_CHANNEL_NUM	4
+#define INITIAL_THD	0x09
+#define SCAN_MODE_LIGHT	0
+#define SCAN_MODE_PROX	1
+
+enum {
+	CM36651_LIGHT_EN,
+	CM36651_PROXIMITY_EN,
+	CM36651_PROXIMITY_EV_EN,
+};
+
+enum cm36651_cmd {
+	CM36651_CMD_READ_RAW_LIGHT,
+	CM36651_CMD_READ_RAW_PROXIMITY,
+	CM36651_CMD_PROX_EV_EN,
+	CM36651_CMD_PROX_EV_DIS,

drivers/iio/light/cm36651.c

Quoted 10 lines

drivers/iio/light/cm36651.c new file +10 -0

+};
+
+enum {
+	CM36651_CLOSE_PROXIMITY,
+	CM36651_FAR_PROXIMITY,
+};
+
+/* register settings */
+static const u8 als_reg_setting[ALS_REG_NUM][2] = {
+	{ 0x00, 0x04 },	/* CS_CONF1 */

use your #defines instead of a comment, e.g. { CS_CONF1, 0x04 }

Quoted 2 lines

what are the magic constants 0x04, 0x08?

I'll use #defines instedad of a comment next version. And magic constants(e.g. 0x04, 0x08) will be made to #defines also.

Quoted 38 lines

drivers/iio/light/cm36651.c new file +38 -0

+	{ 0x01, 0x08 },	/* CS_CONF2 */
+	{ 0x06, 0x00 }	/* CS_CONF3 */
+};
+
+static const u8 ps_reg_setting[PS_REG_NUM][2] = {
+	{ 0x00, 0x3C },	/* PS_CONF1 */
+	{ 0x01, 0x09 },	/* PS_THD */
+	{ 0x02, 0x00 },	/* PS_CANC */
+	{ 0x03, 0x13 },	/* PS_CONF2 */
+};
+
+struct cm36651_data {
+	const struct cm36651_platform_data *pdata;
+	struct i2c_client *client;
+	struct i2c_client *ps_client;
+	struct mutex lock;
+	struct regulator *vled_reg;
+	unsigned long flags;
+	u8 thres;
+	u16 color[4];
+};
+
+int cm36651_i2c_read_byte(struct i2c_client *client, u8 *val)
+{
+	int ret = 0;
+	struct i2c_msg msg[1];
+
+	if (!client->adapter)
+		return -ENODEV;
+
+	/* send slave address & command */
+	msg->addr = client->addr;
+	msg->flags = I2C_M_RD;
+	msg->len = 1;
+	msg->buf = val;
+
+	ret = i2c_transfer(client->adapter, msg, 1);
+	if (ret != 1) {

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 6 lines
drivers/iio/light/cm36651.c new file +6 -0
+		dev_err(&client->dev, "i2c transfer error ret: %d\n", ret);
+		ret = -EIO;
+	}
+
+	return ret;
+}
use i2c_smbus_read_byte_data() instead?

I'll try use i2c_smbus_* API at next patch.

Quoted 61 lines

Quoted 21 lines

drivers/iio/light/cm36651.c new file +21 -0

+
+int cm36651_i2c_read_word(struct i2c_client *client, u8 command, u16 *val)
+{
+	int ret = 0;
+	struct i2c_msg msg[2];
+	unsigned char data[2] = { 0, };
+
+	if (!client->adapter)
+		return -ENODEV;
+
+	/* send slave address & command */
+	msg[0].addr = client->addr;
+	msg[0].flags = 0;
+	msg[0].len = 1;
+	msg[0].buf = &command;
+
+	/* read word data */
+	msg[1].addr = client->addr;
+	msg[1].flags = I2C_M_RD;
+	msg[1].len = 2;
+	msg[1].buf = data;

drivers/iio/light/cm36651.c

Quoted 10 lines

drivers/iio/light/cm36651.c new file +10 -0

+
+	ret = i2c_transfer(client->adapter, msg, 2);
+	if (ret != 2) {
+		dev_err(&client->dev, "i2c transfer error ret: %d\n", ret);
+		ret = -EIO;
+	}
+	*val = le16_to_cpup((__le16 *)data);
+
+	return ret;
+}

use i2c_smbus_read_word_data() instead?

Quoted 14 lines

drivers/iio/light/cm36651.c new file +14 -0

+
+int cm36651_i2c_write_byte(struct i2c_client *client, u8 command, u8 val)
+{
+	int ret;
+	struct i2c_msg msg[1];
+	unsigned char data[2] = { command, val };
+
+	if (!client->adapter)
+		return -ENODEV;
+
+	/* send slave address & command */
+	msg->addr = client->addr;
+	msg->flags = 0;
+	msg->len = 2;

drivers/iio/light/cm36651.c

Quoted 10 lines

drivers/iio/light/cm36651.c new file +10 -0

+	msg->buf = data;
+
+	ret = i2c_transfer(client->adapter, msg, 1);
+	if (ret != 1) {
+		dev_err(&client->dev, "i2c transfer error ret: %d\n", ret);
+		ret = -EIO;
+	}
+
+	return ret;
+}

use i2c_smbus_write_byte_data() instead?

Quoted 8 lines

drivers/iio/light/cm36651.c
Quoted 5 lines
drivers/iio/light/cm36651.c new file +5 -0
+
+static int cm36651_setup_reg(struct cm36651_data *cm36651)
+{
+	struct i2c_client *client = cm36651->client;
+	int ret = 0, i = 0;
no need to initialize ret, tmp and i

I'll fix on your comment.

Quoted 21 lines

drivers/iio/light/cm36651.c new file +21 -0

+	u8 tmp = 0;
+
+	/* ALS initialization */
+	for (i = 0; i < ALS_REG_NUM; i++) {
+		ret = cm36651_i2c_write_byte(client,
+			als_reg_setting[i][0], als_reg_setting[i][1]);
+		if (ret < 0)
+			goto err_setup_reg;
+	}
+
+	/* PS initialization */
+	for (i = 0; i < PS_REG_NUM; i++) {
+		ret = cm36651_i2c_write_byte(cm36651->ps_client,
+			ps_reg_setting[i][0], ps_reg_setting[i][1]);
+		if (ret < 0)
+			goto err_setup_reg;
+	}
+
+	ret = cm36651_i2c_read_byte(cm36651->ps_client, &tmp);
+	if (ret < 0)
+		goto err_setup_reg;

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 6 lines
drivers/iio/light/cm36651.c new file +6 -0
+
+	/* printing the initial proximity value with no contact */
+	dev_dbg(&client->dev, "initial proximity value: %d\n", tmp);
+
+	/* device turn off */
+	ret = cm36651_i2c_write_byte(client, CS_CONF1, 0x01);
use a #define for 0x01 to describe function

OK. I'll use a #define for 0x01 to describe function.

Quoted 11 lines

drivers/iio/light/cm36651.c new file +11 -0

+	if (ret < 0)
+		goto err_setup_reg;
+
+	ret = cm36651_i2c_write_byte(cm36651->ps_client, PS_CONF1, 0x01);
+	if (ret < 0)
+		goto err_setup_reg;
+
+	return 0;
+
+err_setup_reg:
+	dev_err(&client->dev, "cm36651_setup_reg() failed: %d\n", ret);

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 6 lines
drivers/iio/light/cm36651.c new file +6 -0
+	return ret;
+}
+
+static int cm36651_read_output(struct cm36651_data *cm36651,
+						int scan_index, int *val)
+{
val is not used?

I'll check it, and then fix.

Quoted 14 lines

drivers/iio/light/cm36651.c
Quoted 10 lines
drivers/iio/light/cm36651.c new file +10 -0
+	struct i2c_client *client = cm36651->client;
+	int i;
+	int ret = -EINVAL;
+	u8 prox_val;
+
+	switch (scan_index) {
+	case SCAN_MODE_LIGHT:
+		for (i = 0; i < ALS_CHANNEL_NUM; i++) {
+			ret = cm36651_i2c_read_word(client, i,
+						&cm36651->color[i]);
I think this is wrong: i is 0,1,2,3,4 but CS_CONF1, CS_CONF2 is 0, 1 as well -- so the config registers are read here?

Quoted 4 lines

the channel data (red,green,blue,clear) is at addresses 0x02 .. 0x05 (according to
https://github.com/mozilla-b2g/kernel-android-galaxy-s2-ics/blob/master/drivers/sensor/cm36651.c)

i is 0, 1, 2, 3. Also in above link, you can refer to loghtsensor_lux_show and cm36641_work_func_light function, cm36651_i2c_read_word function need to cm36651_data struct, address, command, and val data. Where, color Macros(RED, GREEN, BLUE, WHITE) are command data. Not address. Address is fixed on CM36651_ALS Macro. And then, ALS_WH_M, ... ALS_WL_L(0x02...0x05) registers like below:

ALS_WH_M 0x02 ALS High Threshold Window setting [15:8]
ALS_WH_L 0x03 ALS High Threshold Whndow setting [7:0]
...
ALS_WL_M 0x05 ALS Low Threshold Window setting [7:0]

Quoted 12 lines

drivers/iio/light/cm36651.c

Quoted 9 lines

drivers/iio/light/cm36651.c new file +9 -0

+			if (ret < 0)
+				goto read_err;
+		}
+
+		dev_dbg(&client->dev, "red: %d, green: %d, blue: %d, white: %d\n",
+				cm36651->color[0] + 1, cm36651->color[1] + 1,
+				cm36651->color[2] + 1, cm36651->color[3] + 1);
+
+		ret = cm36651_i2c_write_byte(client, CS_CONF1, 0x01);

what does this do?

Above code is light sensor disable setting.

Quoted 4 lines

Quoted 4 lines
drivers/iio/light/cm36651.c new file +4 -0
+		if (ret < 0)
+			goto write_err;
+
+		break;

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 6 lines
drivers/iio/light/cm36651.c new file +6 -0
+	case SCAN_MODE_PROX:
+		ret = cm36651_i2c_read_byte(cm36651->ps_client, &prox_val);
+		if (ret < 0)
+			goto read_err;
+
+		dev_dbg(&client->dev, "proximity value: %d\n", prox_val);
prox_val is not returned?

I'll check and fix at next version patch.

Quoted 23 lines

drivers/iio/light/cm36651.c new file +23 -0

+
+		ret = cm36651_i2c_write_byte(cm36651->ps_client,
+							PS_CONF1, 0x01);
+		if (ret < 0)
+			goto write_err;
+
+		break;
+	}
+
+	return ret;
+
+read_err:
+	dev_err(&client->dev, "register read failed\n");
+	return ret;
+
+write_err:
+	dev_err(&client->dev, "register write failed\n");
+	return ret;
+}
+
+static irqreturn_t cm36651_irq_handler(int irq, void *data)
+{
+	struct iio_dev *indio_dev = data;

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 6 lines
drivers/iio/light/cm36651.c new file +6 -0
+	struct cm36651_data *cm36651 = iio_priv(indio_dev);
+	struct i2c_client *client = cm36651->client;
+	int ev_dir, val, ret;
+	u64 ev_code;
+	u8 tmp;
+
tmp must be initialized with the command?

I'll fix it on your advice.

Quoted 8 lines

drivers/iio/light/cm36651.c
Quoted 5 lines
drivers/iio/light/cm36651.c new file +5 -0
+	ret = cm36651_i2c_read_byte(cm36651->ps_client, &tmp);
+	if (ret < 0) {
+		dev_err(&client->dev,
+				"%s: data read failed: %d\n", __func__, ret);
+		return IRQ_NONE;
should always return IRQ_HANDLED

I'll fix it on your advice.

Quoted 63 lines

drivers/iio/light/cm36651.c new file +63 -0

+	}
+
+	if (tmp < ps_reg_setting[1][1]) {
+		ev_dir = IIO_EV_DIR_RISING;
+		val = CM36651_FAR_PROXIMITY;
+	} else {
+		ev_dir = IIO_EV_DIR_FALLING;
+		val = CM36651_CLOSE_PROXIMITY;
+	}
+
+	ev_code = IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY,
+				CM36651_CMD_READ_RAW_PROXIMITY,
+				IIO_EV_TYPE_THRESH, ev_dir);
+
+	iio_push_event(indio_dev, ev_code, iio_get_time_ns());
+
+	return IRQ_HANDLED;
+}
+
+static int cm36651_set_operation_mode(struct cm36651_data *cm36651,
+						enum cm36651_cmd cmd)
+{
+	struct i2c_client *client = cm36651->client;
+	int ret = 0;
+	int i;
+
+	switch (cmd) {
+	case CM36651_CMD_READ_RAW_LIGHT:
+		ret = cm36651_i2c_write_byte(client, CS_CONF1, 0x04);
+		break;
+	case CM36651_CMD_READ_RAW_PROXIMITY:
+		ret = cm36651_i2c_write_byte(cm36651->ps_client,
+							PS_CONF1, 0x3C);
+		break;
+	case CM36651_CMD_PROX_EV_EN:
+		if (test_bit(CM36651_PROXIMITY_EV_EN, &cm36651->flags)) {
+			dev_err(&client->dev,
+				"Already proximity event enable state\n");
+			return ret;
+		}
+		set_bit(CM36651_PROXIMITY_EV_EN, &cm36651->flags);
+		for (i = 0; i < 4; i++) {
+			ret = cm36651_i2c_write_byte(cm36651->ps_client,
+				ps_reg_setting[i][0], ps_reg_setting[i][1]);
+			if (ret < 0)
+				goto write_err;
+		}
+		enable_irq(client->irq);
+		break;
+	case CM36651_CMD_PROX_EV_DIS:
+		if (!test_bit(CM36651_PROXIMITY_EV_EN, &cm36651->flags)) {
+			dev_err(&client->dev,
+				"Already proximity event disable state\n");
+			return ret;
+		}
+		clear_bit(CM36651_PROXIMITY_EV_EN, &cm36651->flags);
+		disable_irq(client->irq);
+		ret = cm36651_i2c_write_byte(cm36651->ps_client,
+							PS_CONF1, 0x01);
+		break;
+	}
+
+	if (ret < 0)

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 6 lines
drivers/iio/light/cm36651.c new file +6 -0
+		dev_err(&client->dev, "write register failed\n");
+
+	return ret;
+
+write_err:
+	dev_err(&client->dev, "proximity event toe enable is failed\n");
typo: toe; rephase

I have fixed on correct typo.

Quoted 17 lines

drivers/iio/light/cm36651.c new file +17 -0

+	return ret;
+}
+
+static int cm36651_read_channel(struct cm36651_data *cm36651,
+				struct iio_chan_spec const *chan, int *val)
+{
+	struct i2c_client *client = cm36651->client;
+	enum cm36651_cmd cmd = 0;
+	int ret;
+
+	if (chan->scan_index == SCAN_MODE_LIGHT)
+		cmd = CM36651_CMD_READ_RAW_LIGHT;
+	else /* SCAN_MODE_PROX */
+		cmd = CM36651_CMD_READ_RAW_PROXIMITY;
+
+	ret = cm36651_set_operation_mode(cm36651, cmd);
+	if (ret < 0) {

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 6 lines
drivers/iio/light/cm36651.c new file +6 -0
+		dev_err(&client->dev, "cm36651 set operation mode failed\n");
+		return ret;
+	}
+	/* raw data integration time */
+	msleep(50);
+	ret = cm36651_read_output(cm36651, chan->scan_index, val);
_read_output() does not return data in val!

OK. I'll fix it.

Quoted 175 lines

drivers/iio/light/cm36651.c new file +86 -0

+	if (ret < 0) {
+		dev_err(&client->dev, "cm36651 read output failed\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int cm36651_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int *val, int *val2, long mask)
+{
+	struct cm36651_data *cm36651 = iio_priv(indio_dev);
+	int ret = -EINVAL;
+
+	mutex_lock(&cm36651->lock);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = cm36651_read_channel(cm36651, chan, val);
+		break;
+	}
+	mutex_unlock(&cm36651->lock);
+
+	return ret;
+}
+
+static int cm36651_read_thresh(struct iio_dev *indio_dev,
+					u64 event_code, int *val)
+{
+	struct cm36651_data *cm36651 = iio_priv(indio_dev);
+	int chan_type = IIO_EVENT_CODE_EXTRACT_CHAN_TYPE(event_code);
+	int event_type = IIO_EVENT_CODE_EXTRACT_TYPE(event_code);
+
+	if (event_type != IIO_EV_TYPE_THRESH ||	chan_type != IIO_PROXIMITY)
+		return -EINVAL;
+
+	if (!cm36651->thres)
+		*val = ps_reg_setting[1][1]; /* initial threshold value */
+	else
+		*val = cm36651->thres;
+
+	return 0;
+}
+
+static int cm36651_write_thresh(struct iio_dev *indio_dev,
+					u64 event_code, int val)
+{
+	struct cm36651_data *cm36651 = iio_priv(indio_dev);
+	struct i2c_client *client = cm36651->client;
+	int ret;
+
+	cm36651->thres = val;
+	ret = cm36651_i2c_write_byte(cm36651->ps_client,
+					PS_THD, cm36651->thres);
+
+	if (ret < 0) {
+		dev_err(&client->dev, "PS register read faied: %d\n", ret);
+		return ret;
+	}
+	dev_dbg(&client->dev, "new threshold is 0x%x\n", cm36651->thres);
+
+	return 0;
+}
+
+static int cm36651_write_event_config(struct iio_dev *indio_dev,
+					u64 event_code, int state)
+{
+	struct cm36651_data *cm36651 = iio_priv(indio_dev);
+	enum cm36651_cmd cmd;
+	int chan_type = IIO_EVENT_CODE_EXTRACT_CHAN_TYPE(event_code);
+	int ret = -EINVAL;
+
+	mutex_lock(&cm36651->lock);
+
+	if (chan_type == IIO_PROXIMITY) {
+		cmd = state ? CM36651_CMD_PROX_EV_EN : CM36651_CMD_PROX_EV_DIS;
+		ret = cm36651_set_operation_mode(cm36651, cmd);
+	}
+
+	mutex_unlock(&cm36651->lock);
+
+	return ret;
+}
+
+static int cm36651_read_event_config(struct iio_dev *indio_dev, u64

event_code)

drivers/iio/light/cm36651.c new file +88 -0

+{
+	struct cm36651_data *cm36651 = iio_priv(indio_dev);
+	int chan_type = IIO_EVENT_CODE_EXTRACT_CHAN_TYPE(event_code);
+	int event_en = -EINVAL;
+
+	mutex_lock(&cm36651->lock);
+
+	if (chan_type == IIO_PROXIMITY)
+		event_en = test_bit(CM36651_PROXIMITY_EV_EN, &cm36651->flags);
+
+	mutex_unlock(&cm36651->lock);
+
+	return event_en;
+}
+
+static const struct iio_chan_spec cm36651_channels[] = {
+	{
+		.type = IIO_LIGHT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.shift = 0,
+			.storagebits = 16,
+		},
+		.scan_index = SCAN_MODE_LIGHT
+	},
+	{
+		.type = IIO_PROXIMITY,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 8,
+			.shift = 0,
+			.storagebits = 8,
+		},
+		.scan_index = SCAN_MODE_PROX,
+		.event_mask = IIO_EV_BIT(IIO_EV_TYPE_THRESH, IIO_EV_DIR_EITHER)
+	},
+};
+
+static const struct iio_info cm36651_info = {
+	.driver_module		= THIS_MODULE,
+	.read_raw		= &cm36651_read_raw,
+	.read_event_value	= &cm36651_read_thresh,
+	.write_event_value	= &cm36651_write_thresh,
+	.read_event_config	= &cm36651_read_event_config,
+	.write_event_config	= &cm36651_write_event_config,
+};
+
+static int cm36651_probe(struct i2c_client *client,
+			     const struct i2c_device_id *id)
+{
+	struct cm36651_data *cm36651;
+	struct iio_dev *indio_dev;
+	unsigned long irqflag;
+	int ret;
+
+	dev_dbg(&client->dev, "cm36651 light/proxymity sensor probe\n");
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*cm36651));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	cm36651 = iio_priv(indio_dev);
+
+	cm36651->vled_reg = devm_regulator_get(&client->dev, "vled");
+	if (IS_ERR(cm36651->vled_reg)) {
+		dev_err(&client->dev, "get regulator vled failed\n");
+		return PTR_ERR(cm36651->vled_reg);
+	}
+
+	ret = regulator_enable(cm36651->vled_reg);
+	if (ret) {
+		dev_err(&client->dev, "enable regulator failed\n");
+		return ret;
+	}
+
+	i2c_set_clientdata(client, indio_dev);
+
+	cm36651->client = client;
+	cm36651->ps_client = i2c_new_dummy(client->adapter,
+						CM36651_I2C_ADDR_PS);
+	mutex_init(&cm36651->lock);
+	indio_dev->dev.parent = &client->dev;
+	indio_dev->channels = cm36651_channels;
+	indio_dev->num_channels = ARRAY_SIZE(cm36651_channels);
+	indio_dev->info = &cm36651_info;

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 6 lines
drivers/iio/light/cm36651.c new file +6 -0
+	indio_dev->name = id->name;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	ret = cm36651_setup_reg(cm36651);
+
+	irqflag = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT;
rising and falling?

There are distance from sensor device. Rising is far from device. on the contrary to this Falling is close from device.

Quoted 5 lines

drivers/iio/light/cm36651.c new file +5 -0

+	ret = request_threaded_irq(client->irq, NULL, cm36651_irq_handler,
+				irqflag, "cm36651_proximity", indio_dev);
+	if (ret) {
+		dev_err(&client->dev, "%s: request irq failed\n", __func__);
+		return ret;

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 6 lines
drivers/iio/light/cm36651.c new file +6 -0
+	}
+	disable_irq(client->irq);
+
+	ret = iio_device_register(indio_dev);
+	if (ret) {
+		regulator_disable(cm36651->vled_reg);
irq not freed

It will be fixed.

Quoted 9 lines

drivers/iio/light/cm36651.c new file +9 -0

+		return ret;
+	}
+
+	return 0;
+}
+
+static int cm36651_remove(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 6 lines
drivers/iio/light/cm36651.c new file +6 -0
+	struct cm36651_data *cm36651 = iio_priv(indio_dev);
+
+	iio_device_unregister(indio_dev);
+	regulator_disable(cm36651->vled_reg);
+	free_irq(client->irq, indio_dev);
+	iio_device_free(indio_dev);
iio_device_free() not needed since using devm_iio_device_alloc()

It will be fixed.

Quoted 25 lines

drivers/iio/light/cm36651.c new file +25 -0

+
+	return 0;
+}
+
+static const struct i2c_device_id cm36651_id[] = {
+	{ "cm36651", 0 },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(i2c, cm36651_id);
+
+static const struct of_device_id cm36651_of_match[] = {
+	{ .compatible = "capella,cm36651" },
+	{ }
+};
+
+static struct i2c_driver cm36651_driver = {
+	.driver = {
+		.name	= "cm36651",
+		.of_match_table = of_match_ptr(cm36651_of_match),
+		.owner	= THIS_MODULE,
+	},
+	.probe		= cm36651_probe,
+	.remove		= cm36651_remove,
+	.id_table	= cm36651_id,

Quoted 9 lines

drivers/iio/light/cm36651.c

Quoted 8 lines

drivers/iio/light/cm36651.c new file +7 -0

+};
+
+module_i2c_driver(cm36651_driver);
+
+MODULE_AUTHOR("Beomho Seo <[email protected]>");
+MODULE_DESCRIPTION("CM36651 proximity/ambient light sensor driver");
+MODULE_LICENSE("GPL v2");

I will fix and send v2 patch soon.

Best regards,
Beomho Seo

Signature · 3 lines

Beomho Seo, Assistant Engineer
System S/W Lab., S/W Platform Team, Software Center
Samsung Electronics

A message that is not in the archive (sent off-list or not yet received): [email protected]

Jaehoon Chung

2013-09-10 02:46 UTC #6 lore raw

Dear Peter,

Quoted 15 lines · Beomho Seo

Hello, Mr. Meerwald
Thank you for your advice.
2013년 09월 09일 16:57, Peter Meerwald 쓴 글:
Quoted 10 lines
Quoted 5 lines
This patch add a new driver for Capella CM36651 proximity and RGB light sensor.
The driver exposes two channels: light and proximity. It also support detection proximity event.
comments inline;
I do not have the chip, but I don't think that the code will work and intended

Why do you think so? I'm not sure whether this driver is working well or not. But i known that Beomho has tested this driver on exynos4 board with this device. If you will mention the more detailed, he will fix them.

Best Regards,
Jaehoon Chung

Quoted 790 lines

After I have made this driver, I have checked device working on test board.

Quoted 20 lines

Quoted 20 lines
This driver supports:
- events - rising and falling proximity events.
- reading channels through read_raw callback.
Signed-off-by Beomho Seo <[email protected]>
---
 drivers/iio/light/Kconfig   |   11 +
 drivers/iio/light/cm36651.c |  607
+++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 618 insertions(+)
 create mode 100644 drivers/iio/light/cm36651.c
drivers/iio/light/Kconfig +0 -0
index bf9fa0d..66fb76f 100644
@@ -75,4 +75,15 @@ config VCNL4000
 	 To compile this driver as a module, choose M here: the
 	 module will be called vcnl4000.
 

Quoted 4 lines

drivers/iio/light/Kconfig config VCNL4000 · line 78
Quoted 1 line
drivers/iio/light/Kconfig +1 -0
+config CM36651
sensors to be listed in alphabetical order

I have fixed it.

Quoted 31 lines

drivers/iio/light/Kconfig +10 -0

+	depends on I2C
+	tristate "CM36651 driver"
+	help
+	  Say Y here if you use cm36651.
+	  This option enables proximity & RGB sensor using
+	  Capella cm36651 device driver.
+
+	  To compile this driver as a module, choose M here:
+	  the module will be called cm36651.
+
 endmenu

drivers/iio/light/cm36651.c new file +6 -0

new file mode 100644
index 0000000..b3e1f0d
@@ -0,0 +1,607 @@
+/*
+ * Copyright (C) 2013 Samsung Electronics Co., Ltd.
+ * Author: Beomho Seo <[email protected]>
+ *
+ * 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.
+ */

drivers/iio/light/cm36651.c new file +5 -0

+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/mutex.h>
+#include <linux/module.h>

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 7 lines
drivers/iio/light/cm36651.c new file +7 -0
+#include <linux/interrupt.h>
+#include <linux/regulator/consumer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/events.h>
+
+/* slave address 0x19 for PS of 7 bit addressing protocol for I2C */
consistently start comments with uppercase letter

I use uppercase letter starting comments.

Quoted 8 lines

Quoted 1 line
+#define CM36651_I2C_ADDR_PS 0x19
interesting, the chip seems to have two i2c addresses, the other one is 0x18
is this really one chip? an option would be to do two drivers: one for ALS, one for proximity

Quoted 48 lines

Quoted 35 lines

drivers/iio/light/cm36651.c new file +35 -0

+
+/* Ambient light sensor */
+#define CS_CONF1	0x00
+#define CS_CONF2	0x01
+#define CS_CONF3	0x06
+
+#define RED		0x00
+#define GREEN		0x01
+#define BLUE		0x02
+#define WHITE		0x03
+
+/* Proximity sensor */
+#define PS_CONF1	0x00
+#define PS_THD		0x01
+#define PS_CANC		0x02
+#define PS_CONF2	0x03
+
+#define ALS_REG_NUM	3
+#define PS_REG_NUM	4
+#define ALS_CHANNEL_NUM	4
+#define INITIAL_THD	0x09
+#define SCAN_MODE_LIGHT	0
+#define SCAN_MODE_PROX	1
+
+enum {
+	CM36651_LIGHT_EN,
+	CM36651_PROXIMITY_EN,
+	CM36651_PROXIMITY_EV_EN,
+};
+
+enum cm36651_cmd {
+	CM36651_CMD_READ_RAW_LIGHT,
+	CM36651_CMD_READ_RAW_PROXIMITY,
+	CM36651_CMD_PROX_EV_EN,
+	CM36651_CMD_PROX_EV_DIS,

drivers/iio/light/cm36651.c

Quoted 10 lines

drivers/iio/light/cm36651.c new file +10 -0

+};
+
+enum {
+	CM36651_CLOSE_PROXIMITY,
+	CM36651_FAR_PROXIMITY,
+};
+
+/* register settings */
+static const u8 als_reg_setting[ALS_REG_NUM][2] = {
+	{ 0x00, 0x04 },	/* CS_CONF1 */

use your #defines instead of a comment, e.g. { CS_CONF1, 0x04 }

Quoted 2 lines

what are the magic constants 0x04, 0x08?

I'll use #defines instedad of a comment next version. And magic constants(e.g. 0x04, 0x08) will be made to #defines also.

Quoted 38 lines

drivers/iio/light/cm36651.c new file +38 -0

+	{ 0x01, 0x08 },	/* CS_CONF2 */
+	{ 0x06, 0x00 }	/* CS_CONF3 */
+};
+
+static const u8 ps_reg_setting[PS_REG_NUM][2] = {
+	{ 0x00, 0x3C },	/* PS_CONF1 */
+	{ 0x01, 0x09 },	/* PS_THD */
+	{ 0x02, 0x00 },	/* PS_CANC */
+	{ 0x03, 0x13 },	/* PS_CONF2 */
+};
+
+struct cm36651_data {
+	const struct cm36651_platform_data *pdata;
+	struct i2c_client *client;
+	struct i2c_client *ps_client;
+	struct mutex lock;
+	struct regulator *vled_reg;
+	unsigned long flags;
+	u8 thres;
+	u16 color[4];
+};
+
+int cm36651_i2c_read_byte(struct i2c_client *client, u8 *val)
+{
+	int ret = 0;
+	struct i2c_msg msg[1];
+
+	if (!client->adapter)
+		return -ENODEV;
+
+	/* send slave address & command */
+	msg->addr = client->addr;
+	msg->flags = I2C_M_RD;
+	msg->len = 1;
+	msg->buf = val;
+
+	ret = i2c_transfer(client->adapter, msg, 1);
+	if (ret != 1) {

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 6 lines
drivers/iio/light/cm36651.c new file +6 -0
+		dev_err(&client->dev, "i2c transfer error ret: %d\n", ret);
+		ret = -EIO;
+	}
+
+	return ret;
+}
use i2c_smbus_read_byte_data() instead?

I'll try use i2c_smbus_* API at next patch.

Quoted 61 lines

Quoted 21 lines

drivers/iio/light/cm36651.c new file +21 -0

+
+int cm36651_i2c_read_word(struct i2c_client *client, u8 command, u16 *val)
+{
+	int ret = 0;
+	struct i2c_msg msg[2];
+	unsigned char data[2] = { 0, };
+
+	if (!client->adapter)
+		return -ENODEV;
+
+	/* send slave address & command */
+	msg[0].addr = client->addr;
+	msg[0].flags = 0;
+	msg[0].len = 1;
+	msg[0].buf = &command;
+
+	/* read word data */
+	msg[1].addr = client->addr;
+	msg[1].flags = I2C_M_RD;
+	msg[1].len = 2;
+	msg[1].buf = data;

drivers/iio/light/cm36651.c

Quoted 10 lines

drivers/iio/light/cm36651.c new file +10 -0

+
+	ret = i2c_transfer(client->adapter, msg, 2);
+	if (ret != 2) {
+		dev_err(&client->dev, "i2c transfer error ret: %d\n", ret);
+		ret = -EIO;
+	}
+	*val = le16_to_cpup((__le16 *)data);
+
+	return ret;
+}

use i2c_smbus_read_word_data() instead?

Quoted 14 lines

drivers/iio/light/cm36651.c new file +14 -0

+
+int cm36651_i2c_write_byte(struct i2c_client *client, u8 command, u8 val)
+{
+	int ret;
+	struct i2c_msg msg[1];
+	unsigned char data[2] = { command, val };
+
+	if (!client->adapter)
+		return -ENODEV;
+
+	/* send slave address & command */
+	msg->addr = client->addr;
+	msg->flags = 0;
+	msg->len = 2;

drivers/iio/light/cm36651.c

Quoted 10 lines

drivers/iio/light/cm36651.c new file +10 -0

+	msg->buf = data;
+
+	ret = i2c_transfer(client->adapter, msg, 1);
+	if (ret != 1) {
+		dev_err(&client->dev, "i2c transfer error ret: %d\n", ret);
+		ret = -EIO;
+	}
+
+	return ret;
+}

use i2c_smbus_write_byte_data() instead?

Quoted 8 lines

drivers/iio/light/cm36651.c
Quoted 5 lines
drivers/iio/light/cm36651.c new file +5 -0
+
+static int cm36651_setup_reg(struct cm36651_data *cm36651)
+{
+	struct i2c_client *client = cm36651->client;
+	int ret = 0, i = 0;
no need to initialize ret, tmp and i

I'll fix on your comment.

Quoted 21 lines

drivers/iio/light/cm36651.c new file +21 -0

+	u8 tmp = 0;
+
+	/* ALS initialization */
+	for (i = 0; i < ALS_REG_NUM; i++) {
+		ret = cm36651_i2c_write_byte(client,
+			als_reg_setting[i][0], als_reg_setting[i][1]);
+		if (ret < 0)
+			goto err_setup_reg;
+	}
+
+	/* PS initialization */
+	for (i = 0; i < PS_REG_NUM; i++) {
+		ret = cm36651_i2c_write_byte(cm36651->ps_client,
+			ps_reg_setting[i][0], ps_reg_setting[i][1]);
+		if (ret < 0)
+			goto err_setup_reg;
+	}
+
+	ret = cm36651_i2c_read_byte(cm36651->ps_client, &tmp);
+	if (ret < 0)
+		goto err_setup_reg;

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 6 lines
drivers/iio/light/cm36651.c new file +6 -0
+
+	/* printing the initial proximity value with no contact */
+	dev_dbg(&client->dev, "initial proximity value: %d\n", tmp);
+
+	/* device turn off */
+	ret = cm36651_i2c_write_byte(client, CS_CONF1, 0x01);
use a #define for 0x01 to describe function

OK. I'll use a #define for 0x01 to describe function.

Quoted 11 lines

drivers/iio/light/cm36651.c new file +11 -0

+	if (ret < 0)
+		goto err_setup_reg;
+
+	ret = cm36651_i2c_write_byte(cm36651->ps_client, PS_CONF1, 0x01);
+	if (ret < 0)
+		goto err_setup_reg;
+
+	return 0;
+
+err_setup_reg:
+	dev_err(&client->dev, "cm36651_setup_reg() failed: %d\n", ret);

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 6 lines
drivers/iio/light/cm36651.c new file +6 -0
+	return ret;
+}
+
+static int cm36651_read_output(struct cm36651_data *cm36651,
+						int scan_index, int *val)
+{
val is not used?

I'll check it, and then fix.

Quoted 14 lines

drivers/iio/light/cm36651.c
Quoted 10 lines
drivers/iio/light/cm36651.c new file +10 -0
+	struct i2c_client *client = cm36651->client;
+	int i;
+	int ret = -EINVAL;
+	u8 prox_val;
+
+	switch (scan_index) {
+	case SCAN_MODE_LIGHT:
+		for (i = 0; i < ALS_CHANNEL_NUM; i++) {
+			ret = cm36651_i2c_read_word(client, i,
+						&cm36651->color[i]);
I think this is wrong: i is 0,1,2,3,4 but CS_CONF1, CS_CONF2 is 0, 1 as well -- so the config registers are read here?

Quoted 4 lines

the channel data (red,green,blue,clear) is at addresses 0x02 .. 0x05 (according to
https://github.com/mozilla-b2g/kernel-android-galaxy-s2-ics/blob/master/drivers/sensor/cm36651.c)

ALS_WH_M 0x02 ALS High Threshold Window setting [15:8]
ALS_WH_L 0x03 ALS High Threshold Whndow setting [7:0]
...
ALS_WL_M 0x05 ALS Low Threshold Window setting [7:0]

Quoted 12 lines

drivers/iio/light/cm36651.c

Quoted 9 lines

drivers/iio/light/cm36651.c new file +9 -0

+			if (ret < 0)
+				goto read_err;
+		}
+
+		dev_dbg(&client->dev, "red: %d, green: %d, blue: %d, white: %d\n",
+				cm36651->color[0] + 1, cm36651->color[1] + 1,
+				cm36651->color[2] + 1, cm36651->color[3] + 1);
+
+		ret = cm36651_i2c_write_byte(client, CS_CONF1, 0x01);

what does this do?

Above code is light sensor disable setting.

Quoted 4 lines

Quoted 4 lines
drivers/iio/light/cm36651.c new file +4 -0
+		if (ret < 0)
+			goto write_err;
+
+		break;

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 6 lines
drivers/iio/light/cm36651.c new file +6 -0
+	case SCAN_MODE_PROX:
+		ret = cm36651_i2c_read_byte(cm36651->ps_client, &prox_val);
+		if (ret < 0)
+			goto read_err;
+
+		dev_dbg(&client->dev, "proximity value: %d\n", prox_val);
prox_val is not returned?

I'll check and fix at next version patch.

Quoted 23 lines

drivers/iio/light/cm36651.c new file +23 -0

+
+		ret = cm36651_i2c_write_byte(cm36651->ps_client,
+							PS_CONF1, 0x01);
+		if (ret < 0)
+			goto write_err;
+
+		break;
+	}
+
+	return ret;
+
+read_err:
+	dev_err(&client->dev, "register read failed\n");
+	return ret;
+
+write_err:
+	dev_err(&client->dev, "register write failed\n");
+	return ret;
+}
+
+static irqreturn_t cm36651_irq_handler(int irq, void *data)
+{
+	struct iio_dev *indio_dev = data;

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 6 lines
drivers/iio/light/cm36651.c new file +6 -0
+	struct cm36651_data *cm36651 = iio_priv(indio_dev);
+	struct i2c_client *client = cm36651->client;
+	int ev_dir, val, ret;
+	u64 ev_code;
+	u8 tmp;
+
tmp must be initialized with the command?

I'll fix it on your advice.

Quoted 8 lines

drivers/iio/light/cm36651.c
Quoted 5 lines
drivers/iio/light/cm36651.c new file +5 -0
+	ret = cm36651_i2c_read_byte(cm36651->ps_client, &tmp);
+	if (ret < 0) {
+		dev_err(&client->dev,
+				"%s: data read failed: %d\n", __func__, ret);
+		return IRQ_NONE;
should always return IRQ_HANDLED

I'll fix it on your advice.

Quoted 63 lines

drivers/iio/light/cm36651.c new file +63 -0

+	}
+
+	if (tmp < ps_reg_setting[1][1]) {
+		ev_dir = IIO_EV_DIR_RISING;
+		val = CM36651_FAR_PROXIMITY;
+	} else {
+		ev_dir = IIO_EV_DIR_FALLING;
+		val = CM36651_CLOSE_PROXIMITY;
+	}
+
+	ev_code = IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY,
+				CM36651_CMD_READ_RAW_PROXIMITY,
+				IIO_EV_TYPE_THRESH, ev_dir);
+
+	iio_push_event(indio_dev, ev_code, iio_get_time_ns());
+
+	return IRQ_HANDLED;
+}
+
+static int cm36651_set_operation_mode(struct cm36651_data *cm36651,
+						enum cm36651_cmd cmd)
+{
+	struct i2c_client *client = cm36651->client;
+	int ret = 0;
+	int i;
+
+	switch (cmd) {
+	case CM36651_CMD_READ_RAW_LIGHT:
+		ret = cm36651_i2c_write_byte(client, CS_CONF1, 0x04);
+		break;
+	case CM36651_CMD_READ_RAW_PROXIMITY:
+		ret = cm36651_i2c_write_byte(cm36651->ps_client,
+							PS_CONF1, 0x3C);
+		break;
+	case CM36651_CMD_PROX_EV_EN:
+		if (test_bit(CM36651_PROXIMITY_EV_EN, &cm36651->flags)) {
+			dev_err(&client->dev,
+				"Already proximity event enable state\n");
+			return ret;
+		}
+		set_bit(CM36651_PROXIMITY_EV_EN, &cm36651->flags);
+		for (i = 0; i < 4; i++) {
+			ret = cm36651_i2c_write_byte(cm36651->ps_client,
+				ps_reg_setting[i][0], ps_reg_setting[i][1]);
+			if (ret < 0)
+				goto write_err;
+		}
+		enable_irq(client->irq);
+		break;
+	case CM36651_CMD_PROX_EV_DIS:
+		if (!test_bit(CM36651_PROXIMITY_EV_EN, &cm36651->flags)) {
+			dev_err(&client->dev,
+				"Already proximity event disable state\n");
+			return ret;
+		}
+		clear_bit(CM36651_PROXIMITY_EV_EN, &cm36651->flags);
+		disable_irq(client->irq);
+		ret = cm36651_i2c_write_byte(cm36651->ps_client,
+							PS_CONF1, 0x01);
+		break;
+	}
+
+	if (ret < 0)

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 6 lines
drivers/iio/light/cm36651.c new file +6 -0
+		dev_err(&client->dev, "write register failed\n");
+
+	return ret;
+
+write_err:
+	dev_err(&client->dev, "proximity event toe enable is failed\n");
typo: toe; rephase

I have fixed on correct typo.

Quoted 17 lines

drivers/iio/light/cm36651.c new file +17 -0

+	return ret;
+}
+
+static int cm36651_read_channel(struct cm36651_data *cm36651,
+				struct iio_chan_spec const *chan, int *val)
+{
+	struct i2c_client *client = cm36651->client;
+	enum cm36651_cmd cmd = 0;
+	int ret;
+
+	if (chan->scan_index == SCAN_MODE_LIGHT)
+		cmd = CM36651_CMD_READ_RAW_LIGHT;
+	else /* SCAN_MODE_PROX */
+		cmd = CM36651_CMD_READ_RAW_PROXIMITY;
+
+	ret = cm36651_set_operation_mode(cm36651, cmd);
+	if (ret < 0) {

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 6 lines
drivers/iio/light/cm36651.c new file +6 -0
+		dev_err(&client->dev, "cm36651 set operation mode failed\n");
+		return ret;
+	}
+	/* raw data integration time */
+	msleep(50);
+	ret = cm36651_read_output(cm36651, chan->scan_index, val);
_read_output() does not return data in val!

OK. I'll fix it.

Quoted 175 lines

drivers/iio/light/cm36651.c new file +86 -0

+	if (ret < 0) {
+		dev_err(&client->dev, "cm36651 read output failed\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int cm36651_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int *val, int *val2, long mask)
+{
+	struct cm36651_data *cm36651 = iio_priv(indio_dev);
+	int ret = -EINVAL;
+
+	mutex_lock(&cm36651->lock);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = cm36651_read_channel(cm36651, chan, val);
+		break;
+	}
+	mutex_unlock(&cm36651->lock);
+
+	return ret;
+}
+
+static int cm36651_read_thresh(struct iio_dev *indio_dev,
+					u64 event_code, int *val)
+{
+	struct cm36651_data *cm36651 = iio_priv(indio_dev);
+	int chan_type = IIO_EVENT_CODE_EXTRACT_CHAN_TYPE(event_code);
+	int event_type = IIO_EVENT_CODE_EXTRACT_TYPE(event_code);
+
+	if (event_type != IIO_EV_TYPE_THRESH ||	chan_type != IIO_PROXIMITY)
+		return -EINVAL;
+
+	if (!cm36651->thres)
+		*val = ps_reg_setting[1][1]; /* initial threshold value */
+	else
+		*val = cm36651->thres;
+
+	return 0;
+}
+
+static int cm36651_write_thresh(struct iio_dev *indio_dev,
+					u64 event_code, int val)
+{
+	struct cm36651_data *cm36651 = iio_priv(indio_dev);
+	struct i2c_client *client = cm36651->client;
+	int ret;
+
+	cm36651->thres = val;
+	ret = cm36651_i2c_write_byte(cm36651->ps_client,
+					PS_THD, cm36651->thres);
+
+	if (ret < 0) {
+		dev_err(&client->dev, "PS register read faied: %d\n", ret);
+		return ret;
+	}
+	dev_dbg(&client->dev, "new threshold is 0x%x\n", cm36651->thres);
+
+	return 0;
+}
+
+static int cm36651_write_event_config(struct iio_dev *indio_dev,
+					u64 event_code, int state)
+{
+	struct cm36651_data *cm36651 = iio_priv(indio_dev);
+	enum cm36651_cmd cmd;
+	int chan_type = IIO_EVENT_CODE_EXTRACT_CHAN_TYPE(event_code);
+	int ret = -EINVAL;
+
+	mutex_lock(&cm36651->lock);
+
+	if (chan_type == IIO_PROXIMITY) {
+		cmd = state ? CM36651_CMD_PROX_EV_EN : CM36651_CMD_PROX_EV_DIS;
+		ret = cm36651_set_operation_mode(cm36651, cmd);
+	}
+
+	mutex_unlock(&cm36651->lock);
+
+	return ret;
+}
+
+static int cm36651_read_event_config(struct iio_dev *indio_dev, u64

event_code)

drivers/iio/light/cm36651.c new file +88 -0

+{
+	struct cm36651_data *cm36651 = iio_priv(indio_dev);
+	int chan_type = IIO_EVENT_CODE_EXTRACT_CHAN_TYPE(event_code);
+	int event_en = -EINVAL;
+
+	mutex_lock(&cm36651->lock);
+
+	if (chan_type == IIO_PROXIMITY)
+		event_en = test_bit(CM36651_PROXIMITY_EV_EN, &cm36651->flags);
+
+	mutex_unlock(&cm36651->lock);
+
+	return event_en;
+}
+
+static const struct iio_chan_spec cm36651_channels[] = {
+	{
+		.type = IIO_LIGHT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.shift = 0,
+			.storagebits = 16,
+		},
+		.scan_index = SCAN_MODE_LIGHT
+	},
+	{
+		.type = IIO_PROXIMITY,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 8,
+			.shift = 0,
+			.storagebits = 8,
+		},
+		.scan_index = SCAN_MODE_PROX,
+		.event_mask = IIO_EV_BIT(IIO_EV_TYPE_THRESH, IIO_EV_DIR_EITHER)
+	},
+};
+
+static const struct iio_info cm36651_info = {
+	.driver_module		= THIS_MODULE,
+	.read_raw		= &cm36651_read_raw,
+	.read_event_value	= &cm36651_read_thresh,
+	.write_event_value	= &cm36651_write_thresh,
+	.read_event_config	= &cm36651_read_event_config,
+	.write_event_config	= &cm36651_write_event_config,
+};
+
+static int cm36651_probe(struct i2c_client *client,
+			     const struct i2c_device_id *id)
+{
+	struct cm36651_data *cm36651;
+	struct iio_dev *indio_dev;
+	unsigned long irqflag;
+	int ret;
+
+	dev_dbg(&client->dev, "cm36651 light/proxymity sensor probe\n");
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*cm36651));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	cm36651 = iio_priv(indio_dev);
+
+	cm36651->vled_reg = devm_regulator_get(&client->dev, "vled");
+	if (IS_ERR(cm36651->vled_reg)) {
+		dev_err(&client->dev, "get regulator vled failed\n");
+		return PTR_ERR(cm36651->vled_reg);
+	}
+
+	ret = regulator_enable(cm36651->vled_reg);
+	if (ret) {
+		dev_err(&client->dev, "enable regulator failed\n");
+		return ret;
+	}
+
+	i2c_set_clientdata(client, indio_dev);
+
+	cm36651->client = client;
+	cm36651->ps_client = i2c_new_dummy(client->adapter,
+						CM36651_I2C_ADDR_PS);
+	mutex_init(&cm36651->lock);
+	indio_dev->dev.parent = &client->dev;
+	indio_dev->channels = cm36651_channels;
+	indio_dev->num_channels = ARRAY_SIZE(cm36651_channels);
+	indio_dev->info = &cm36651_info;

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 6 lines
drivers/iio/light/cm36651.c new file +6 -0
+	indio_dev->name = id->name;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	ret = cm36651_setup_reg(cm36651);
+
+	irqflag = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT;
rising and falling?

There are distance from sensor device. Rising is far from device. on the contrary to this Falling is close from device.

Quoted 5 lines

drivers/iio/light/cm36651.c new file +5 -0

+	ret = request_threaded_irq(client->irq, NULL, cm36651_irq_handler,
+				irqflag, "cm36651_proximity", indio_dev);
+	if (ret) {
+		dev_err(&client->dev, "%s: request irq failed\n", __func__);
+		return ret;

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 6 lines
drivers/iio/light/cm36651.c new file +6 -0
+	}
+	disable_irq(client->irq);
+
+	ret = iio_device_register(indio_dev);
+	if (ret) {
+		regulator_disable(cm36651->vled_reg);
irq not freed

It will be fixed.

Quoted 9 lines

drivers/iio/light/cm36651.c new file +9 -0

+		return ret;
+	}
+
+	return 0;
+}
+
+static int cm36651_remove(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 6 lines
drivers/iio/light/cm36651.c new file +6 -0
+	struct cm36651_data *cm36651 = iio_priv(indio_dev);
+
+	iio_device_unregister(indio_dev);
+	regulator_disable(cm36651->vled_reg);
+	free_irq(client->irq, indio_dev);
+	iio_device_free(indio_dev);
iio_device_free() not needed since using devm_iio_device_alloc()

It will be fixed.

Quoted 25 lines

drivers/iio/light/cm36651.c new file +25 -0

+
+	return 0;
+}
+
+static const struct i2c_device_id cm36651_id[] = {
+	{ "cm36651", 0 },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(i2c, cm36651_id);
+
+static const struct of_device_id cm36651_of_match[] = {
+	{ .compatible = "capella,cm36651" },
+	{ }
+};
+
+static struct i2c_driver cm36651_driver = {
+	.driver = {
+		.name	= "cm36651",
+		.of_match_table = of_match_ptr(cm36651_of_match),
+		.owner	= THIS_MODULE,
+	},
+	.probe		= cm36651_probe,
+	.remove		= cm36651_remove,
+	.id_table	= cm36651_id,

Quoted 9 lines

drivers/iio/light/cm36651.c

Quoted 8 lines

drivers/iio/light/cm36651.c new file +7 -0

+};
+
+module_i2c_driver(cm36651_driver);
+
+MODULE_AUTHOR("Beomho Seo <[email protected]>");
+MODULE_DESCRIPTION("CM36651 proximity/ambient light sensor driver");
+MODULE_LICENSE("GPL v2");

I will fix and send v2 patch soon.

Best regards,
Beomho Seo

A message that is not in the archive (sent off-list or not yet received): [email protected]

Peter Meerwald

2013-09-10 05:44 UTC #8 lore raw

Hello,

Quoted 5 lines

Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
This patch add a new driver for Capella CM36651 proximity and RGB light sensor.
The driver exposes two channels: light and proximity. It also support detection proximity event.

Quoted 7 lines

Quoted 3 lines
Quoted 3 lines
comments inline;
I do not have the chip, but I don't think that the code will work and intended
Why do you think so? I'm not sure whether this driver is working well or not. But i known that Beomho has tested this driver on exynos4 board with this device. If you will mention the more detailed, he will fix them.

sorry, I wasn't very clean;
the driver can certainly be fixed! I'm happy to look at an upcoming revision

regarding testing: _read_output() seems to have not been tested, I don't think that correct/meaningful values are returned (for the reasons indicated)

regards, p.

Quoted 790 lines

Quoted 789 lines

After I have made this driver, I have checked device working on test board.

Quoted 20 lines

Quoted 20 lines
This driver supports:
- events - rising and falling proximity events.
- reading channels through read_raw callback.
Signed-off-by Beomho Seo <[email protected]>
---
 drivers/iio/light/Kconfig   |   11 +
 drivers/iio/light/cm36651.c |  607
+++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 618 insertions(+)
 create mode 100644 drivers/iio/light/cm36651.c
drivers/iio/light/Kconfig +0 -0
index bf9fa0d..66fb76f 100644
@@ -75,4 +75,15 @@ config VCNL4000
 	 To compile this driver as a module, choose M here: the
 	 module will be called vcnl4000.
 

Quoted 4 lines

drivers/iio/light/Kconfig config VCNL4000 · line 78
Quoted 1 line
drivers/iio/light/Kconfig +1 -0
+config CM36651
sensors to be listed in alphabetical order

I have fixed it.

Quoted 31 lines

drivers/iio/light/Kconfig +10 -0

+	depends on I2C
+	tristate "CM36651 driver"
+	help
+	  Say Y here if you use cm36651.
+	  This option enables proximity & RGB sensor using
+	  Capella cm36651 device driver.
+
+	  To compile this driver as a module, choose M here:
+	  the module will be called cm36651.
+
 endmenu

drivers/iio/light/cm36651.c new file +6 -0

new file mode 100644
index 0000000..b3e1f0d
@@ -0,0 +1,607 @@
+/*
+ * Copyright (C) 2013 Samsung Electronics Co., Ltd.
+ * Author: Beomho Seo <[email protected]>
+ *
+ * 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.
+ */

drivers/iio/light/cm36651.c new file +5 -0

+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/mutex.h>
+#include <linux/module.h>

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 7 lines
drivers/iio/light/cm36651.c new file +7 -0
+#include <linux/interrupt.h>
+#include <linux/regulator/consumer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/events.h>
+
+/* slave address 0x19 for PS of 7 bit addressing protocol for I2C */
consistently start comments with uppercase letter

I use uppercase letter starting comments.

Quoted 8 lines

Quoted 1 line
+#define CM36651_I2C_ADDR_PS 0x19
interesting, the chip seems to have two i2c addresses, the other one is 0x18
is this really one chip? an option would be to do two drivers: one for ALS, one for proximity

Quoted 48 lines

Quoted 35 lines

drivers/iio/light/cm36651.c new file +35 -0

+
+/* Ambient light sensor */
+#define CS_CONF1	0x00
+#define CS_CONF2	0x01
+#define CS_CONF3	0x06
+
+#define RED		0x00
+#define GREEN		0x01
+#define BLUE		0x02
+#define WHITE		0x03
+
+/* Proximity sensor */
+#define PS_CONF1	0x00
+#define PS_THD		0x01
+#define PS_CANC		0x02
+#define PS_CONF2	0x03
+
+#define ALS_REG_NUM	3
+#define PS_REG_NUM	4
+#define ALS_CHANNEL_NUM	4
+#define INITIAL_THD	0x09
+#define SCAN_MODE_LIGHT	0
+#define SCAN_MODE_PROX	1
+
+enum {
+	CM36651_LIGHT_EN,
+	CM36651_PROXIMITY_EN,
+	CM36651_PROXIMITY_EV_EN,
+};
+
+enum cm36651_cmd {
+	CM36651_CMD_READ_RAW_LIGHT,
+	CM36651_CMD_READ_RAW_PROXIMITY,
+	CM36651_CMD_PROX_EV_EN,
+	CM36651_CMD_PROX_EV_DIS,

drivers/iio/light/cm36651.c

Quoted 10 lines

drivers/iio/light/cm36651.c new file +10 -0

+};
+
+enum {
+	CM36651_CLOSE_PROXIMITY,
+	CM36651_FAR_PROXIMITY,
+};
+
+/* register settings */
+static const u8 als_reg_setting[ALS_REG_NUM][2] = {
+	{ 0x00, 0x04 },	/* CS_CONF1 */

use your #defines instead of a comment, e.g. { CS_CONF1, 0x04 }

Quoted 2 lines

what are the magic constants 0x04, 0x08?

I'll use #defines instedad of a comment next version. And magic constants(e.g. 0x04, 0x08) will be made to #defines also.

Quoted 38 lines

drivers/iio/light/cm36651.c new file +38 -0

+	{ 0x01, 0x08 },	/* CS_CONF2 */
+	{ 0x06, 0x00 }	/* CS_CONF3 */
+};
+
+static const u8 ps_reg_setting[PS_REG_NUM][2] = {
+	{ 0x00, 0x3C },	/* PS_CONF1 */
+	{ 0x01, 0x09 },	/* PS_THD */
+	{ 0x02, 0x00 },	/* PS_CANC */
+	{ 0x03, 0x13 },	/* PS_CONF2 */
+};
+
+struct cm36651_data {
+	const struct cm36651_platform_data *pdata;
+	struct i2c_client *client;
+	struct i2c_client *ps_client;
+	struct mutex lock;
+	struct regulator *vled_reg;
+	unsigned long flags;
+	u8 thres;
+	u16 color[4];
+};
+
+int cm36651_i2c_read_byte(struct i2c_client *client, u8 *val)
+{
+	int ret = 0;
+	struct i2c_msg msg[1];
+
+	if (!client->adapter)
+		return -ENODEV;
+
+	/* send slave address & command */
+	msg->addr = client->addr;
+	msg->flags = I2C_M_RD;
+	msg->len = 1;
+	msg->buf = val;
+
+	ret = i2c_transfer(client->adapter, msg, 1);
+	if (ret != 1) {

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 6 lines
drivers/iio/light/cm36651.c new file +6 -0
+		dev_err(&client->dev, "i2c transfer error ret: %d\n", ret);
+		ret = -EIO;
+	}
+
+	return ret;
+}
use i2c_smbus_read_byte_data() instead?

I'll try use i2c_smbus_* API at next patch.

Quoted 61 lines

Quoted 21 lines

drivers/iio/light/cm36651.c new file +21 -0

+
+int cm36651_i2c_read_word(struct i2c_client *client, u8 command, u16 *val)
+{
+	int ret = 0;
+	struct i2c_msg msg[2];
+	unsigned char data[2] = { 0, };
+
+	if (!client->adapter)
+		return -ENODEV;
+
+	/* send slave address & command */
+	msg[0].addr = client->addr;
+	msg[0].flags = 0;
+	msg[0].len = 1;
+	msg[0].buf = &command;
+
+	/* read word data */
+	msg[1].addr = client->addr;
+	msg[1].flags = I2C_M_RD;
+	msg[1].len = 2;
+	msg[1].buf = data;

drivers/iio/light/cm36651.c

Quoted 10 lines

drivers/iio/light/cm36651.c new file +10 -0

+
+	ret = i2c_transfer(client->adapter, msg, 2);
+	if (ret != 2) {
+		dev_err(&client->dev, "i2c transfer error ret: %d\n", ret);
+		ret = -EIO;
+	}
+	*val = le16_to_cpup((__le16 *)data);
+
+	return ret;
+}

use i2c_smbus_read_word_data() instead?

Quoted 14 lines

drivers/iio/light/cm36651.c new file +14 -0

+
+int cm36651_i2c_write_byte(struct i2c_client *client, u8 command, u8 val)
+{
+	int ret;
+	struct i2c_msg msg[1];
+	unsigned char data[2] = { command, val };
+
+	if (!client->adapter)
+		return -ENODEV;
+
+	/* send slave address & command */
+	msg->addr = client->addr;
+	msg->flags = 0;
+	msg->len = 2;

drivers/iio/light/cm36651.c

Quoted 10 lines

drivers/iio/light/cm36651.c new file +10 -0

+	msg->buf = data;
+
+	ret = i2c_transfer(client->adapter, msg, 1);
+	if (ret != 1) {
+		dev_err(&client->dev, "i2c transfer error ret: %d\n", ret);
+		ret = -EIO;
+	}
+
+	return ret;
+}

use i2c_smbus_write_byte_data() instead?

Quoted 8 lines

drivers/iio/light/cm36651.c
Quoted 5 lines
drivers/iio/light/cm36651.c new file +5 -0
+
+static int cm36651_setup_reg(struct cm36651_data *cm36651)
+{
+	struct i2c_client *client = cm36651->client;
+	int ret = 0, i = 0;
no need to initialize ret, tmp and i

I'll fix on your comment.

Quoted 21 lines

drivers/iio/light/cm36651.c new file +21 -0

+	u8 tmp = 0;
+
+	/* ALS initialization */
+	for (i = 0; i < ALS_REG_NUM; i++) {
+		ret = cm36651_i2c_write_byte(client,
+			als_reg_setting[i][0], als_reg_setting[i][1]);
+		if (ret < 0)
+			goto err_setup_reg;
+	}
+
+	/* PS initialization */
+	for (i = 0; i < PS_REG_NUM; i++) {
+		ret = cm36651_i2c_write_byte(cm36651->ps_client,
+			ps_reg_setting[i][0], ps_reg_setting[i][1]);
+		if (ret < 0)
+			goto err_setup_reg;
+	}
+
+	ret = cm36651_i2c_read_byte(cm36651->ps_client, &tmp);
+	if (ret < 0)
+		goto err_setup_reg;

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 6 lines
drivers/iio/light/cm36651.c new file +6 -0
+
+	/* printing the initial proximity value with no contact */
+	dev_dbg(&client->dev, "initial proximity value: %d\n", tmp);
+
+	/* device turn off */
+	ret = cm36651_i2c_write_byte(client, CS_CONF1, 0x01);
use a #define for 0x01 to describe function

OK. I'll use a #define for 0x01 to describe function.

Quoted 11 lines

drivers/iio/light/cm36651.c new file +11 -0

+	if (ret < 0)
+		goto err_setup_reg;
+
+	ret = cm36651_i2c_write_byte(cm36651->ps_client, PS_CONF1, 0x01);
+	if (ret < 0)
+		goto err_setup_reg;
+
+	return 0;
+
+err_setup_reg:
+	dev_err(&client->dev, "cm36651_setup_reg() failed: %d\n", ret);

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 6 lines
drivers/iio/light/cm36651.c new file +6 -0
+	return ret;
+}
+
+static int cm36651_read_output(struct cm36651_data *cm36651,
+						int scan_index, int *val)
+{
val is not used?

I'll check it, and then fix.

Quoted 14 lines

drivers/iio/light/cm36651.c
Quoted 10 lines
drivers/iio/light/cm36651.c new file +10 -0
+	struct i2c_client *client = cm36651->client;
+	int i;
+	int ret = -EINVAL;
+	u8 prox_val;
+
+	switch (scan_index) {
+	case SCAN_MODE_LIGHT:
+		for (i = 0; i < ALS_CHANNEL_NUM; i++) {
+			ret = cm36651_i2c_read_word(client, i,
+						&cm36651->color[i]);
I think this is wrong: i is 0,1,2,3,4 but CS_CONF1, CS_CONF2 is 0, 1 as well -- so the config registers are read here?

Quoted 4 lines

the channel data (red,green,blue,clear) is at addresses 0x02 .. 0x05 (according to
https://github.com/mozilla-b2g/kernel-android-galaxy-s2-ics/blob/master/drivers/sensor/cm36651.c)

ALS_WH_M 0x02 ALS High Threshold Window setting [15:8]
ALS_WH_L 0x03 ALS High Threshold Whndow setting [7:0]
...
ALS_WL_M 0x05 ALS Low Threshold Window setting [7:0]

Quoted 12 lines

drivers/iio/light/cm36651.c

Quoted 9 lines

drivers/iio/light/cm36651.c new file +9 -0

+			if (ret < 0)
+				goto read_err;
+		}
+
+		dev_dbg(&client->dev, "red: %d, green: %d, blue: %d, white: %d\n",
+				cm36651->color[0] + 1, cm36651->color[1] + 1,
+				cm36651->color[2] + 1, cm36651->color[3] + 1);
+
+		ret = cm36651_i2c_write_byte(client, CS_CONF1, 0x01);

what does this do?

Above code is light sensor disable setting.

Quoted 4 lines

Quoted 4 lines
drivers/iio/light/cm36651.c new file +4 -0
+		if (ret < 0)
+			goto write_err;
+
+		break;

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 6 lines
drivers/iio/light/cm36651.c new file +6 -0
+	case SCAN_MODE_PROX:
+		ret = cm36651_i2c_read_byte(cm36651->ps_client, &prox_val);
+		if (ret < 0)
+			goto read_err;
+
+		dev_dbg(&client->dev, "proximity value: %d\n", prox_val);
prox_val is not returned?

I'll check and fix at next version patch.

Quoted 23 lines

drivers/iio/light/cm36651.c new file +23 -0

+
+		ret = cm36651_i2c_write_byte(cm36651->ps_client,
+							PS_CONF1, 0x01);
+		if (ret < 0)
+			goto write_err;
+
+		break;
+	}
+
+	return ret;
+
+read_err:
+	dev_err(&client->dev, "register read failed\n");
+	return ret;
+
+write_err:
+	dev_err(&client->dev, "register write failed\n");
+	return ret;
+}
+
+static irqreturn_t cm36651_irq_handler(int irq, void *data)
+{
+	struct iio_dev *indio_dev = data;

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 6 lines
drivers/iio/light/cm36651.c new file +6 -0
+	struct cm36651_data *cm36651 = iio_priv(indio_dev);
+	struct i2c_client *client = cm36651->client;
+	int ev_dir, val, ret;
+	u64 ev_code;
+	u8 tmp;
+
tmp must be initialized with the command?

I'll fix it on your advice.

Quoted 8 lines

drivers/iio/light/cm36651.c
Quoted 5 lines
drivers/iio/light/cm36651.c new file +5 -0
+	ret = cm36651_i2c_read_byte(cm36651->ps_client, &tmp);
+	if (ret < 0) {
+		dev_err(&client->dev,
+				"%s: data read failed: %d\n", __func__, ret);
+		return IRQ_NONE;
should always return IRQ_HANDLED

I'll fix it on your advice.

Quoted 63 lines

drivers/iio/light/cm36651.c new file +63 -0

+	}
+
+	if (tmp < ps_reg_setting[1][1]) {
+		ev_dir = IIO_EV_DIR_RISING;
+		val = CM36651_FAR_PROXIMITY;
+	} else {
+		ev_dir = IIO_EV_DIR_FALLING;
+		val = CM36651_CLOSE_PROXIMITY;
+	}
+
+	ev_code = IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY,
+				CM36651_CMD_READ_RAW_PROXIMITY,
+				IIO_EV_TYPE_THRESH, ev_dir);
+
+	iio_push_event(indio_dev, ev_code, iio_get_time_ns());
+
+	return IRQ_HANDLED;
+}
+
+static int cm36651_set_operation_mode(struct cm36651_data *cm36651,
+						enum cm36651_cmd cmd)
+{
+	struct i2c_client *client = cm36651->client;
+	int ret = 0;
+	int i;
+
+	switch (cmd) {
+	case CM36651_CMD_READ_RAW_LIGHT:
+		ret = cm36651_i2c_write_byte(client, CS_CONF1, 0x04);
+		break;
+	case CM36651_CMD_READ_RAW_PROXIMITY:
+		ret = cm36651_i2c_write_byte(cm36651->ps_client,
+							PS_CONF1, 0x3C);
+		break;
+	case CM36651_CMD_PROX_EV_EN:
+		if (test_bit(CM36651_PROXIMITY_EV_EN, &cm36651->flags)) {
+			dev_err(&client->dev,
+				"Already proximity event enable state\n");
+			return ret;
+		}
+		set_bit(CM36651_PROXIMITY_EV_EN, &cm36651->flags);
+		for (i = 0; i < 4; i++) {
+			ret = cm36651_i2c_write_byte(cm36651->ps_client,
+				ps_reg_setting[i][0], ps_reg_setting[i][1]);
+			if (ret < 0)
+				goto write_err;
+		}
+		enable_irq(client->irq);
+		break;
+	case CM36651_CMD_PROX_EV_DIS:
+		if (!test_bit(CM36651_PROXIMITY_EV_EN, &cm36651->flags)) {
+			dev_err(&client->dev,
+				"Already proximity event disable state\n");
+			return ret;
+		}
+		clear_bit(CM36651_PROXIMITY_EV_EN, &cm36651->flags);
+		disable_irq(client->irq);
+		ret = cm36651_i2c_write_byte(cm36651->ps_client,
+							PS_CONF1, 0x01);
+		break;
+	}
+
+	if (ret < 0)

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 6 lines
drivers/iio/light/cm36651.c new file +6 -0
+		dev_err(&client->dev, "write register failed\n");
+
+	return ret;
+
+write_err:
+	dev_err(&client->dev, "proximity event toe enable is failed\n");
typo: toe; rephase

I have fixed on correct typo.

Quoted 17 lines

drivers/iio/light/cm36651.c new file +17 -0

+	return ret;
+}
+
+static int cm36651_read_channel(struct cm36651_data *cm36651,
+				struct iio_chan_spec const *chan, int *val)
+{
+	struct i2c_client *client = cm36651->client;
+	enum cm36651_cmd cmd = 0;
+	int ret;
+
+	if (chan->scan_index == SCAN_MODE_LIGHT)
+		cmd = CM36651_CMD_READ_RAW_LIGHT;
+	else /* SCAN_MODE_PROX */
+		cmd = CM36651_CMD_READ_RAW_PROXIMITY;
+
+	ret = cm36651_set_operation_mode(cm36651, cmd);
+	if (ret < 0) {

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 6 lines
drivers/iio/light/cm36651.c new file +6 -0
+		dev_err(&client->dev, "cm36651 set operation mode failed\n");
+		return ret;
+	}
+	/* raw data integration time */
+	msleep(50);
+	ret = cm36651_read_output(cm36651, chan->scan_index, val);
_read_output() does not return data in val!

OK. I'll fix it.

Quoted 175 lines

drivers/iio/light/cm36651.c new file +86 -0

+	if (ret < 0) {
+		dev_err(&client->dev, "cm36651 read output failed\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int cm36651_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int *val, int *val2, long mask)
+{
+	struct cm36651_data *cm36651 = iio_priv(indio_dev);
+	int ret = -EINVAL;
+
+	mutex_lock(&cm36651->lock);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = cm36651_read_channel(cm36651, chan, val);
+		break;
+	}
+	mutex_unlock(&cm36651->lock);
+
+	return ret;
+}
+
+static int cm36651_read_thresh(struct iio_dev *indio_dev,
+					u64 event_code, int *val)
+{
+	struct cm36651_data *cm36651 = iio_priv(indio_dev);
+	int chan_type = IIO_EVENT_CODE_EXTRACT_CHAN_TYPE(event_code);
+	int event_type = IIO_EVENT_CODE_EXTRACT_TYPE(event_code);
+
+	if (event_type != IIO_EV_TYPE_THRESH ||	chan_type != IIO_PROXIMITY)
+		return -EINVAL;
+
+	if (!cm36651->thres)
+		*val = ps_reg_setting[1][1]; /* initial threshold value */
+	else
+		*val = cm36651->thres;
+
+	return 0;
+}
+
+static int cm36651_write_thresh(struct iio_dev *indio_dev,
+					u64 event_code, int val)
+{
+	struct cm36651_data *cm36651 = iio_priv(indio_dev);
+	struct i2c_client *client = cm36651->client;
+	int ret;
+
+	cm36651->thres = val;
+	ret = cm36651_i2c_write_byte(cm36651->ps_client,
+					PS_THD, cm36651->thres);
+
+	if (ret < 0) {
+		dev_err(&client->dev, "PS register read faied: %d\n", ret);
+		return ret;
+	}
+	dev_dbg(&client->dev, "new threshold is 0x%x\n", cm36651->thres);
+
+	return 0;
+}
+
+static int cm36651_write_event_config(struct iio_dev *indio_dev,
+					u64 event_code, int state)
+{
+	struct cm36651_data *cm36651 = iio_priv(indio_dev);
+	enum cm36651_cmd cmd;
+	int chan_type = IIO_EVENT_CODE_EXTRACT_CHAN_TYPE(event_code);
+	int ret = -EINVAL;
+
+	mutex_lock(&cm36651->lock);
+
+	if (chan_type == IIO_PROXIMITY) {
+		cmd = state ? CM36651_CMD_PROX_EV_EN : CM36651_CMD_PROX_EV_DIS;
+		ret = cm36651_set_operation_mode(cm36651, cmd);
+	}
+
+	mutex_unlock(&cm36651->lock);
+
+	return ret;
+}
+
+static int cm36651_read_event_config(struct iio_dev *indio_dev, u64

event_code)

drivers/iio/light/cm36651.c new file +88 -0

+{
+	struct cm36651_data *cm36651 = iio_priv(indio_dev);
+	int chan_type = IIO_EVENT_CODE_EXTRACT_CHAN_TYPE(event_code);
+	int event_en = -EINVAL;
+
+	mutex_lock(&cm36651->lock);
+
+	if (chan_type == IIO_PROXIMITY)
+		event_en = test_bit(CM36651_PROXIMITY_EV_EN, &cm36651->flags);
+
+	mutex_unlock(&cm36651->lock);
+
+	return event_en;
+}
+
+static const struct iio_chan_spec cm36651_channels[] = {
+	{
+		.type = IIO_LIGHT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.shift = 0,
+			.storagebits = 16,
+		},
+		.scan_index = SCAN_MODE_LIGHT
+	},
+	{
+		.type = IIO_PROXIMITY,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 8,
+			.shift = 0,
+			.storagebits = 8,
+		},
+		.scan_index = SCAN_MODE_PROX,
+		.event_mask = IIO_EV_BIT(IIO_EV_TYPE_THRESH, IIO_EV_DIR_EITHER)
+	},
+};
+
+static const struct iio_info cm36651_info = {
+	.driver_module		= THIS_MODULE,
+	.read_raw		= &cm36651_read_raw,
+	.read_event_value	= &cm36651_read_thresh,
+	.write_event_value	= &cm36651_write_thresh,
+	.read_event_config	= &cm36651_read_event_config,
+	.write_event_config	= &cm36651_write_event_config,
+};
+
+static int cm36651_probe(struct i2c_client *client,
+			     const struct i2c_device_id *id)
+{
+	struct cm36651_data *cm36651;
+	struct iio_dev *indio_dev;
+	unsigned long irqflag;
+	int ret;
+
+	dev_dbg(&client->dev, "cm36651 light/proxymity sensor probe\n");
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*cm36651));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	cm36651 = iio_priv(indio_dev);
+
+	cm36651->vled_reg = devm_regulator_get(&client->dev, "vled");
+	if (IS_ERR(cm36651->vled_reg)) {
+		dev_err(&client->dev, "get regulator vled failed\n");
+		return PTR_ERR(cm36651->vled_reg);
+	}
+
+	ret = regulator_enable(cm36651->vled_reg);
+	if (ret) {
+		dev_err(&client->dev, "enable regulator failed\n");
+		return ret;
+	}
+
+	i2c_set_clientdata(client, indio_dev);
+
+	cm36651->client = client;
+	cm36651->ps_client = i2c_new_dummy(client->adapter,
+						CM36651_I2C_ADDR_PS);
+	mutex_init(&cm36651->lock);
+	indio_dev->dev.parent = &client->dev;
+	indio_dev->channels = cm36651_channels;
+	indio_dev->num_channels = ARRAY_SIZE(cm36651_channels);
+	indio_dev->info = &cm36651_info;

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 6 lines
drivers/iio/light/cm36651.c new file +6 -0
+	indio_dev->name = id->name;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	ret = cm36651_setup_reg(cm36651);
+
+	irqflag = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT;
rising and falling?

There are distance from sensor device. Rising is far from device. on the contrary to this Falling is close from device.

Quoted 5 lines

drivers/iio/light/cm36651.c new file +5 -0

+	ret = request_threaded_irq(client->irq, NULL, cm36651_irq_handler,
+				irqflag, "cm36651_proximity", indio_dev);
+	if (ret) {
+		dev_err(&client->dev, "%s: request irq failed\n", __func__);
+		return ret;

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 6 lines
drivers/iio/light/cm36651.c new file +6 -0
+	}
+	disable_irq(client->irq);
+
+	ret = iio_device_register(indio_dev);
+	if (ret) {
+		regulator_disable(cm36651->vled_reg);
irq not freed

It will be fixed.

Quoted 9 lines

drivers/iio/light/cm36651.c new file +9 -0

+		return ret;
+	}
+
+	return 0;
+}
+
+static int cm36651_remove(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 6 lines
drivers/iio/light/cm36651.c new file +6 -0
+	struct cm36651_data *cm36651 = iio_priv(indio_dev);
+
+	iio_device_unregister(indio_dev);
+	regulator_disable(cm36651->vled_reg);
+	free_irq(client->irq, indio_dev);
+	iio_device_free(indio_dev);
iio_device_free() not needed since using devm_iio_device_alloc()

It will be fixed.

Quoted 25 lines

drivers/iio/light/cm36651.c new file +25 -0

+
+	return 0;
+}
+
+static const struct i2c_device_id cm36651_id[] = {
+	{ "cm36651", 0 },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(i2c, cm36651_id);
+
+static const struct of_device_id cm36651_of_match[] = {
+	{ .compatible = "capella,cm36651" },
+	{ }
+};
+
+static struct i2c_driver cm36651_driver = {
+	.driver = {
+		.name	= "cm36651",
+		.of_match_table = of_match_ptr(cm36651_of_match),
+		.owner	= THIS_MODULE,
+	},
+	.probe		= cm36651_probe,
+	.remove		= cm36651_remove,
+	.id_table	= cm36651_id,

Quoted 9 lines

drivers/iio/light/cm36651.c

Quoted 8 lines

drivers/iio/light/cm36651.c new file +7 -0

+};
+
+module_i2c_driver(cm36651_driver);
+
+MODULE_AUTHOR("Beomho Seo <[email protected]>");
+MODULE_DESCRIPTION("CM36651 proximity/ambient light sensor driver");
+MODULE_LICENSE("GPL v2");

I will fix and send v2 patch soon.

Best regards,
Beomho Seo

Signature · 2 lines

Peter Meerwald
+43-664-2444418 (mobile)

A message that is not in the archive (sent off-list or not yet received): alpine.DEB.2.01.1309100735160.3847-jW+XmwGofnusTnJN9+BGXg@public.gmane.org

Beomho Seo

author

2013-09-12 01:01 UTC #10 lore raw

2013년 09월 10일 14:44, Peter Meerwald 쓴 글:

Quoted 19 lines

Hello,
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
This patch add a new driver for Capella CM36651 proximity and RGB light sensor.
The driver exposes two channels: light and proximity. It also support detection proximity event.
Quoted 7 lines
Quoted 3 lines
Quoted 3 lines
comments inline;
I do not have the chip, but I don't think that the code will work and intended
Why do you think so? I'm not sure whether this driver is working well or not. But i known that Beomho has tested this driver on exynos4 board with this device. If you will mention the more detailed, he will fix them.
sorry, I wasn't very clean;
the driver can certainly be fixed! I'm happy to look at an upcoming revision

Quoted 6 lines

regarding testing: _read_output() seems to have not been tested, I don't think that correct/meaningful values are returned (for the reasons indicated)
regards, p.

I have understood your advice. I think, Your advice means that read_raw callback function should return IIO_VAL_INT and sensor data. but this driver doesn't return these value. I will revise this driver and send next patch as soon as maybe.

Again, thank you for your advise.

Quoted 791 lines

Quoted 790 lines

Quoted 789 lines

After I have made this driver, I have checked device working on test board.

Quoted 20 lines

Quoted 20 lines
This driver supports:
- events - rising and falling proximity events.
- reading channels through read_raw callback.
Signed-off-by Beomho Seo <[email protected]>
---
 drivers/iio/light/Kconfig   |   11 +
 drivers/iio/light/cm36651.c |  607
+++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 618 insertions(+)
 create mode 100644 drivers/iio/light/cm36651.c
drivers/iio/light/Kconfig +0 -0
index bf9fa0d..66fb76f 100644
@@ -75,4 +75,15 @@ config VCNL4000
 	 To compile this driver as a module, choose M here: the
 	 module will be called vcnl4000.
 

Quoted 4 lines

drivers/iio/light/Kconfig config VCNL4000 · line 78
Quoted 1 line
drivers/iio/light/Kconfig +1 -0
+config CM36651
sensors to be listed in alphabetical order

I have fixed it.

Quoted 31 lines

drivers/iio/light/Kconfig +10 -0

+	depends on I2C
+	tristate "CM36651 driver"
+	help
+	  Say Y here if you use cm36651.
+	  This option enables proximity & RGB sensor using
+	  Capella cm36651 device driver.
+
+	  To compile this driver as a module, choose M here:
+	  the module will be called cm36651.
+
 endmenu

drivers/iio/light/cm36651.c new file +6 -0

new file mode 100644
index 0000000..b3e1f0d
@@ -0,0 +1,607 @@
+/*
+ * Copyright (C) 2013 Samsung Electronics Co., Ltd.
+ * Author: Beomho Seo <[email protected]>
+ *
+ * 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.
+ */

drivers/iio/light/cm36651.c new file +5 -0

+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/mutex.h>
+#include <linux/module.h>

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 7 lines
drivers/iio/light/cm36651.c new file +7 -0
+#include <linux/interrupt.h>
+#include <linux/regulator/consumer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/events.h>
+
+/* slave address 0x19 for PS of 7 bit addressing protocol for I2C */
consistently start comments with uppercase letter

I use uppercase letter starting comments.

Quoted 8 lines

Quoted 1 line
+#define CM36651_I2C_ADDR_PS 0x19
interesting, the chip seems to have two i2c addresses, the other one is 0x18
is this really one chip? an option would be to do two drivers: one for ALS, one for proximity

Quoted 48 lines

Quoted 35 lines

drivers/iio/light/cm36651.c new file +35 -0

+
+/* Ambient light sensor */
+#define CS_CONF1	0x00
+#define CS_CONF2	0x01
+#define CS_CONF3	0x06
+
+#define RED		0x00
+#define GREEN		0x01
+#define BLUE		0x02
+#define WHITE		0x03
+
+/* Proximity sensor */
+#define PS_CONF1	0x00
+#define PS_THD		0x01
+#define PS_CANC		0x02
+#define PS_CONF2	0x03
+
+#define ALS_REG_NUM	3
+#define PS_REG_NUM	4
+#define ALS_CHANNEL_NUM	4
+#define INITIAL_THD	0x09
+#define SCAN_MODE_LIGHT	0
+#define SCAN_MODE_PROX	1
+
+enum {
+	CM36651_LIGHT_EN,
+	CM36651_PROXIMITY_EN,
+	CM36651_PROXIMITY_EV_EN,
+};
+
+enum cm36651_cmd {
+	CM36651_CMD_READ_RAW_LIGHT,
+	CM36651_CMD_READ_RAW_PROXIMITY,
+	CM36651_CMD_PROX_EV_EN,
+	CM36651_CMD_PROX_EV_DIS,

drivers/iio/light/cm36651.c

Quoted 10 lines

drivers/iio/light/cm36651.c new file +10 -0

+};
+
+enum {
+	CM36651_CLOSE_PROXIMITY,
+	CM36651_FAR_PROXIMITY,
+};
+
+/* register settings */
+static const u8 als_reg_setting[ALS_REG_NUM][2] = {
+	{ 0x00, 0x04 },	/* CS_CONF1 */

use your #defines instead of a comment, e.g. { CS_CONF1, 0x04 }

Quoted 2 lines

what are the magic constants 0x04, 0x08?

I'll use #defines instedad of a comment next version. And magic constants(e.g. 0x04, 0x08) will be made to #defines also.

Quoted 38 lines

drivers/iio/light/cm36651.c new file +38 -0

+	{ 0x01, 0x08 },	/* CS_CONF2 */
+	{ 0x06, 0x00 }	/* CS_CONF3 */
+};
+
+static const u8 ps_reg_setting[PS_REG_NUM][2] = {
+	{ 0x00, 0x3C },	/* PS_CONF1 */
+	{ 0x01, 0x09 },	/* PS_THD */
+	{ 0x02, 0x00 },	/* PS_CANC */
+	{ 0x03, 0x13 },	/* PS_CONF2 */
+};
+
+struct cm36651_data {
+	const struct cm36651_platform_data *pdata;
+	struct i2c_client *client;
+	struct i2c_client *ps_client;
+	struct mutex lock;
+	struct regulator *vled_reg;
+	unsigned long flags;
+	u8 thres;
+	u16 color[4];
+};
+
+int cm36651_i2c_read_byte(struct i2c_client *client, u8 *val)
+{
+	int ret = 0;
+	struct i2c_msg msg[1];
+
+	if (!client->adapter)
+		return -ENODEV;
+
+	/* send slave address & command */
+	msg->addr = client->addr;
+	msg->flags = I2C_M_RD;
+	msg->len = 1;
+	msg->buf = val;
+
+	ret = i2c_transfer(client->adapter, msg, 1);
+	if (ret != 1) {

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 6 lines
drivers/iio/light/cm36651.c new file +6 -0
+		dev_err(&client->dev, "i2c transfer error ret: %d\n", ret);
+		ret = -EIO;
+	}
+
+	return ret;
+}
use i2c_smbus_read_byte_data() instead?

I'll try use i2c_smbus_* API at next patch.

Quoted 61 lines

Quoted 21 lines

drivers/iio/light/cm36651.c new file +21 -0

+
+int cm36651_i2c_read_word(struct i2c_client *client, u8 command, u16 *val)
+{
+	int ret = 0;
+	struct i2c_msg msg[2];
+	unsigned char data[2] = { 0, };
+
+	if (!client->adapter)
+		return -ENODEV;
+
+	/* send slave address & command */
+	msg[0].addr = client->addr;
+	msg[0].flags = 0;
+	msg[0].len = 1;
+	msg[0].buf = &command;
+
+	/* read word data */
+	msg[1].addr = client->addr;
+	msg[1].flags = I2C_M_RD;
+	msg[1].len = 2;
+	msg[1].buf = data;

drivers/iio/light/cm36651.c

Quoted 10 lines

drivers/iio/light/cm36651.c new file +10 -0

+
+	ret = i2c_transfer(client->adapter, msg, 2);
+	if (ret != 2) {
+		dev_err(&client->dev, "i2c transfer error ret: %d\n", ret);
+		ret = -EIO;
+	}
+	*val = le16_to_cpup((__le16 *)data);
+
+	return ret;
+}

use i2c_smbus_read_word_data() instead?

Quoted 14 lines

drivers/iio/light/cm36651.c new file +14 -0

+
+int cm36651_i2c_write_byte(struct i2c_client *client, u8 command, u8 val)
+{
+	int ret;
+	struct i2c_msg msg[1];
+	unsigned char data[2] = { command, val };
+
+	if (!client->adapter)
+		return -ENODEV;
+
+	/* send slave address & command */
+	msg->addr = client->addr;
+	msg->flags = 0;
+	msg->len = 2;

drivers/iio/light/cm36651.c

Quoted 10 lines

drivers/iio/light/cm36651.c new file +10 -0

+	msg->buf = data;
+
+	ret = i2c_transfer(client->adapter, msg, 1);
+	if (ret != 1) {
+		dev_err(&client->dev, "i2c transfer error ret: %d\n", ret);
+		ret = -EIO;
+	}
+
+	return ret;
+}

use i2c_smbus_write_byte_data() instead?

Quoted 8 lines

drivers/iio/light/cm36651.c
Quoted 5 lines
drivers/iio/light/cm36651.c new file +5 -0
+
+static int cm36651_setup_reg(struct cm36651_data *cm36651)
+{
+	struct i2c_client *client = cm36651->client;
+	int ret = 0, i = 0;
no need to initialize ret, tmp and i

I'll fix on your comment.

Quoted 21 lines

drivers/iio/light/cm36651.c new file +21 -0

+	u8 tmp = 0;
+
+	/* ALS initialization */
+	for (i = 0; i < ALS_REG_NUM; i++) {
+		ret = cm36651_i2c_write_byte(client,
+			als_reg_setting[i][0], als_reg_setting[i][1]);
+		if (ret < 0)
+			goto err_setup_reg;
+	}
+
+	/* PS initialization */
+	for (i = 0; i < PS_REG_NUM; i++) {
+		ret = cm36651_i2c_write_byte(cm36651->ps_client,
+			ps_reg_setting[i][0], ps_reg_setting[i][1]);
+		if (ret < 0)
+			goto err_setup_reg;
+	}
+
+	ret = cm36651_i2c_read_byte(cm36651->ps_client, &tmp);
+	if (ret < 0)
+		goto err_setup_reg;

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 6 lines
drivers/iio/light/cm36651.c new file +6 -0
+
+	/* printing the initial proximity value with no contact */
+	dev_dbg(&client->dev, "initial proximity value: %d\n", tmp);
+
+	/* device turn off */
+	ret = cm36651_i2c_write_byte(client, CS_CONF1, 0x01);
use a #define for 0x01 to describe function

OK. I'll use a #define for 0x01 to describe function.

Quoted 11 lines

drivers/iio/light/cm36651.c new file +11 -0

+	if (ret < 0)
+		goto err_setup_reg;
+
+	ret = cm36651_i2c_write_byte(cm36651->ps_client, PS_CONF1, 0x01);
+	if (ret < 0)
+		goto err_setup_reg;
+
+	return 0;
+
+err_setup_reg:
+	dev_err(&client->dev, "cm36651_setup_reg() failed: %d\n", ret);

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 6 lines
drivers/iio/light/cm36651.c new file +6 -0
+	return ret;
+}
+
+static int cm36651_read_output(struct cm36651_data *cm36651,
+						int scan_index, int *val)
+{
val is not used?

I'll check it, and then fix.

Quoted 14 lines

drivers/iio/light/cm36651.c
Quoted 10 lines
drivers/iio/light/cm36651.c new file +10 -0
+	struct i2c_client *client = cm36651->client;
+	int i;
+	int ret = -EINVAL;
+	u8 prox_val;
+
+	switch (scan_index) {
+	case SCAN_MODE_LIGHT:
+		for (i = 0; i < ALS_CHANNEL_NUM; i++) {
+			ret = cm36651_i2c_read_word(client, i,
+						&cm36651->color[i]);
I think this is wrong: i is 0,1,2,3,4 but CS_CONF1, CS_CONF2 is 0, 1 as well -- so the config registers are read here?

Quoted 4 lines

the channel data (red,green,blue,clear) is at addresses 0x02 .. 0x05 (according to
https://github.com/mozilla-b2g/kernel-android-galaxy-s2-ics/blob/master/drivers/sensor/cm36651.c)

ALS_WH_M 0x02 ALS High Threshold Window setting [15:8]
ALS_WH_L 0x03 ALS High Threshold Whndow setting [7:0]
...
ALS_WL_M 0x05 ALS Low Threshold Window setting [7:0]

Quoted 12 lines

drivers/iio/light/cm36651.c

Quoted 9 lines

drivers/iio/light/cm36651.c new file +9 -0

+			if (ret < 0)
+				goto read_err;
+		}
+
+		dev_dbg(&client->dev, "red: %d, green: %d, blue: %d, white: %d\n",
+				cm36651->color[0] + 1, cm36651->color[1] + 1,
+				cm36651->color[2] + 1, cm36651->color[3] + 1);
+
+		ret = cm36651_i2c_write_byte(client, CS_CONF1, 0x01);

what does this do?

Above code is light sensor disable setting.

Quoted 4 lines

Quoted 4 lines
drivers/iio/light/cm36651.c new file +4 -0
+		if (ret < 0)
+			goto write_err;
+
+		break;

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 6 lines
drivers/iio/light/cm36651.c new file +6 -0
+	case SCAN_MODE_PROX:
+		ret = cm36651_i2c_read_byte(cm36651->ps_client, &prox_val);
+		if (ret < 0)
+			goto read_err;
+
+		dev_dbg(&client->dev, "proximity value: %d\n", prox_val);
prox_val is not returned?

I'll check and fix at next version patch.

Quoted 23 lines

drivers/iio/light/cm36651.c new file +23 -0

+
+		ret = cm36651_i2c_write_byte(cm36651->ps_client,
+							PS_CONF1, 0x01);
+		if (ret < 0)
+			goto write_err;
+
+		break;
+	}
+
+	return ret;
+
+read_err:
+	dev_err(&client->dev, "register read failed\n");
+	return ret;
+
+write_err:
+	dev_err(&client->dev, "register write failed\n");
+	return ret;
+}
+
+static irqreturn_t cm36651_irq_handler(int irq, void *data)
+{
+	struct iio_dev *indio_dev = data;

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 6 lines
drivers/iio/light/cm36651.c new file +6 -0
+	struct cm36651_data *cm36651 = iio_priv(indio_dev);
+	struct i2c_client *client = cm36651->client;
+	int ev_dir, val, ret;
+	u64 ev_code;
+	u8 tmp;
+
tmp must be initialized with the command?

I'll fix it on your advice.

Quoted 8 lines

drivers/iio/light/cm36651.c
Quoted 5 lines
drivers/iio/light/cm36651.c new file +5 -0
+	ret = cm36651_i2c_read_byte(cm36651->ps_client, &tmp);
+	if (ret < 0) {
+		dev_err(&client->dev,
+				"%s: data read failed: %d\n", __func__, ret);
+		return IRQ_NONE;
should always return IRQ_HANDLED

I'll fix it on your advice.

Quoted 63 lines

drivers/iio/light/cm36651.c new file +63 -0

+	}
+
+	if (tmp < ps_reg_setting[1][1]) {
+		ev_dir = IIO_EV_DIR_RISING;
+		val = CM36651_FAR_PROXIMITY;
+	} else {
+		ev_dir = IIO_EV_DIR_FALLING;
+		val = CM36651_CLOSE_PROXIMITY;
+	}
+
+	ev_code = IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY,
+				CM36651_CMD_READ_RAW_PROXIMITY,
+				IIO_EV_TYPE_THRESH, ev_dir);
+
+	iio_push_event(indio_dev, ev_code, iio_get_time_ns());
+
+	return IRQ_HANDLED;
+}
+
+static int cm36651_set_operation_mode(struct cm36651_data *cm36651,
+						enum cm36651_cmd cmd)
+{
+	struct i2c_client *client = cm36651->client;
+	int ret = 0;
+	int i;
+
+	switch (cmd) {
+	case CM36651_CMD_READ_RAW_LIGHT:
+		ret = cm36651_i2c_write_byte(client, CS_CONF1, 0x04);
+		break;
+	case CM36651_CMD_READ_RAW_PROXIMITY:
+		ret = cm36651_i2c_write_byte(cm36651->ps_client,
+							PS_CONF1, 0x3C);
+		break;
+	case CM36651_CMD_PROX_EV_EN:
+		if (test_bit(CM36651_PROXIMITY_EV_EN, &cm36651->flags)) {
+			dev_err(&client->dev,
+				"Already proximity event enable state\n");
+			return ret;
+		}
+		set_bit(CM36651_PROXIMITY_EV_EN, &cm36651->flags);
+		for (i = 0; i < 4; i++) {
+			ret = cm36651_i2c_write_byte(cm36651->ps_client,
+				ps_reg_setting[i][0], ps_reg_setting[i][1]);
+			if (ret < 0)
+				goto write_err;
+		}
+		enable_irq(client->irq);
+		break;
+	case CM36651_CMD_PROX_EV_DIS:
+		if (!test_bit(CM36651_PROXIMITY_EV_EN, &cm36651->flags)) {
+			dev_err(&client->dev,
+				"Already proximity event disable state\n");
+			return ret;
+		}
+		clear_bit(CM36651_PROXIMITY_EV_EN, &cm36651->flags);
+		disable_irq(client->irq);
+		ret = cm36651_i2c_write_byte(cm36651->ps_client,
+							PS_CONF1, 0x01);
+		break;
+	}
+
+	if (ret < 0)

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 6 lines
drivers/iio/light/cm36651.c new file +6 -0
+		dev_err(&client->dev, "write register failed\n");
+
+	return ret;
+
+write_err:
+	dev_err(&client->dev, "proximity event toe enable is failed\n");
typo: toe; rephase

I have fixed on correct typo.

Quoted 17 lines

drivers/iio/light/cm36651.c new file +17 -0

+	return ret;
+}
+
+static int cm36651_read_channel(struct cm36651_data *cm36651,
+				struct iio_chan_spec const *chan, int *val)
+{
+	struct i2c_client *client = cm36651->client;
+	enum cm36651_cmd cmd = 0;
+	int ret;
+
+	if (chan->scan_index == SCAN_MODE_LIGHT)
+		cmd = CM36651_CMD_READ_RAW_LIGHT;
+	else /* SCAN_MODE_PROX */
+		cmd = CM36651_CMD_READ_RAW_PROXIMITY;
+
+	ret = cm36651_set_operation_mode(cm36651, cmd);
+	if (ret < 0) {

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 6 lines
drivers/iio/light/cm36651.c new file +6 -0
+		dev_err(&client->dev, "cm36651 set operation mode failed\n");
+		return ret;
+	}
+	/* raw data integration time */
+	msleep(50);
+	ret = cm36651_read_output(cm36651, chan->scan_index, val);
_read_output() does not return data in val!

OK. I'll fix it.

Quoted 175 lines

drivers/iio/light/cm36651.c new file +86 -0

+	if (ret < 0) {
+		dev_err(&client->dev, "cm36651 read output failed\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int cm36651_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int *val, int *val2, long mask)
+{
+	struct cm36651_data *cm36651 = iio_priv(indio_dev);
+	int ret = -EINVAL;
+
+	mutex_lock(&cm36651->lock);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = cm36651_read_channel(cm36651, chan, val);
+		break;
+	}
+	mutex_unlock(&cm36651->lock);
+
+	return ret;
+}
+
+static int cm36651_read_thresh(struct iio_dev *indio_dev,
+					u64 event_code, int *val)
+{
+	struct cm36651_data *cm36651 = iio_priv(indio_dev);
+	int chan_type = IIO_EVENT_CODE_EXTRACT_CHAN_TYPE(event_code);
+	int event_type = IIO_EVENT_CODE_EXTRACT_TYPE(event_code);
+
+	if (event_type != IIO_EV_TYPE_THRESH ||	chan_type != IIO_PROXIMITY)
+		return -EINVAL;
+
+	if (!cm36651->thres)
+		*val = ps_reg_setting[1][1]; /* initial threshold value */
+	else
+		*val = cm36651->thres;
+
+	return 0;
+}
+
+static int cm36651_write_thresh(struct iio_dev *indio_dev,
+					u64 event_code, int val)
+{
+	struct cm36651_data *cm36651 = iio_priv(indio_dev);
+	struct i2c_client *client = cm36651->client;
+	int ret;
+
+	cm36651->thres = val;
+	ret = cm36651_i2c_write_byte(cm36651->ps_client,
+					PS_THD, cm36651->thres);
+
+	if (ret < 0) {
+		dev_err(&client->dev, "PS register read faied: %d\n", ret);
+		return ret;
+	}
+	dev_dbg(&client->dev, "new threshold is 0x%x\n", cm36651->thres);
+
+	return 0;
+}
+
+static int cm36651_write_event_config(struct iio_dev *indio_dev,
+					u64 event_code, int state)
+{
+	struct cm36651_data *cm36651 = iio_priv(indio_dev);
+	enum cm36651_cmd cmd;
+	int chan_type = IIO_EVENT_CODE_EXTRACT_CHAN_TYPE(event_code);
+	int ret = -EINVAL;
+
+	mutex_lock(&cm36651->lock);
+
+	if (chan_type == IIO_PROXIMITY) {
+		cmd = state ? CM36651_CMD_PROX_EV_EN : CM36651_CMD_PROX_EV_DIS;
+		ret = cm36651_set_operation_mode(cm36651, cmd);
+	}
+
+	mutex_unlock(&cm36651->lock);
+
+	return ret;
+}
+
+static int cm36651_read_event_config(struct iio_dev *indio_dev, u64

event_code)

drivers/iio/light/cm36651.c new file +88 -0

+{
+	struct cm36651_data *cm36651 = iio_priv(indio_dev);
+	int chan_type = IIO_EVENT_CODE_EXTRACT_CHAN_TYPE(event_code);
+	int event_en = -EINVAL;
+
+	mutex_lock(&cm36651->lock);
+
+	if (chan_type == IIO_PROXIMITY)
+		event_en = test_bit(CM36651_PROXIMITY_EV_EN, &cm36651->flags);
+
+	mutex_unlock(&cm36651->lock);
+
+	return event_en;
+}
+
+static const struct iio_chan_spec cm36651_channels[] = {
+	{
+		.type = IIO_LIGHT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.shift = 0,
+			.storagebits = 16,
+		},
+		.scan_index = SCAN_MODE_LIGHT
+	},
+	{
+		.type = IIO_PROXIMITY,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 8,
+			.shift = 0,
+			.storagebits = 8,
+		},
+		.scan_index = SCAN_MODE_PROX,
+		.event_mask = IIO_EV_BIT(IIO_EV_TYPE_THRESH, IIO_EV_DIR_EITHER)
+	},
+};
+
+static const struct iio_info cm36651_info = {
+	.driver_module		= THIS_MODULE,
+	.read_raw		= &cm36651_read_raw,
+	.read_event_value	= &cm36651_read_thresh,
+	.write_event_value	= &cm36651_write_thresh,
+	.read_event_config	= &cm36651_read_event_config,
+	.write_event_config	= &cm36651_write_event_config,
+};
+
+static int cm36651_probe(struct i2c_client *client,
+			     const struct i2c_device_id *id)
+{
+	struct cm36651_data *cm36651;
+	struct iio_dev *indio_dev;
+	unsigned long irqflag;
+	int ret;
+
+	dev_dbg(&client->dev, "cm36651 light/proxymity sensor probe\n");
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*cm36651));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	cm36651 = iio_priv(indio_dev);
+
+	cm36651->vled_reg = devm_regulator_get(&client->dev, "vled");
+	if (IS_ERR(cm36651->vled_reg)) {
+		dev_err(&client->dev, "get regulator vled failed\n");
+		return PTR_ERR(cm36651->vled_reg);
+	}
+
+	ret = regulator_enable(cm36651->vled_reg);
+	if (ret) {
+		dev_err(&client->dev, "enable regulator failed\n");
+		return ret;
+	}
+
+	i2c_set_clientdata(client, indio_dev);
+
+	cm36651->client = client;
+	cm36651->ps_client = i2c_new_dummy(client->adapter,
+						CM36651_I2C_ADDR_PS);
+	mutex_init(&cm36651->lock);
+	indio_dev->dev.parent = &client->dev;
+	indio_dev->channels = cm36651_channels;
+	indio_dev->num_channels = ARRAY_SIZE(cm36651_channels);
+	indio_dev->info = &cm36651_info;

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 6 lines
drivers/iio/light/cm36651.c new file +6 -0
+	indio_dev->name = id->name;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	ret = cm36651_setup_reg(cm36651);
+
+	irqflag = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT;
rising and falling?

There are distance from sensor device. Rising is far from device. on the contrary to this Falling is close from device.

Quoted 5 lines

drivers/iio/light/cm36651.c new file +5 -0

+	ret = request_threaded_irq(client->irq, NULL, cm36651_irq_handler,
+				irqflag, "cm36651_proximity", indio_dev);
+	if (ret) {
+		dev_err(&client->dev, "%s: request irq failed\n", __func__);
+		return ret;

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 6 lines
drivers/iio/light/cm36651.c new file +6 -0
+	}
+	disable_irq(client->irq);
+
+	ret = iio_device_register(indio_dev);
+	if (ret) {
+		regulator_disable(cm36651->vled_reg);
irq not freed

It will be fixed.

Quoted 9 lines

drivers/iio/light/cm36651.c new file +9 -0

+		return ret;
+	}
+
+	return 0;
+}
+
+static int cm36651_remove(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);

Quoted 9 lines

drivers/iio/light/cm36651.c
Quoted 6 lines
drivers/iio/light/cm36651.c new file +6 -0
+	struct cm36651_data *cm36651 = iio_priv(indio_dev);
+
+	iio_device_unregister(indio_dev);
+	regulator_disable(cm36651->vled_reg);
+	free_irq(client->irq, indio_dev);
+	iio_device_free(indio_dev);
iio_device_free() not needed since using devm_iio_device_alloc()

It will be fixed.

Quoted 25 lines

drivers/iio/light/cm36651.c new file +25 -0

+
+	return 0;
+}
+
+static const struct i2c_device_id cm36651_id[] = {
+	{ "cm36651", 0 },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(i2c, cm36651_id);
+
+static const struct of_device_id cm36651_of_match[] = {
+	{ .compatible = "capella,cm36651" },
+	{ }
+};
+
+static struct i2c_driver cm36651_driver = {
+	.driver = {
+		.name	= "cm36651",
+		.of_match_table = of_match_ptr(cm36651_of_match),
+		.owner	= THIS_MODULE,
+	},
+	.probe		= cm36651_probe,
+	.remove		= cm36651_remove,
+	.id_table	= cm36651_id,

Quoted 9 lines

drivers/iio/light/cm36651.c

Quoted 8 lines

drivers/iio/light/cm36651.c new file +7 -0

+};
+
+module_i2c_driver(cm36651_driver);
+
+MODULE_AUTHOR("Beomho Seo <[email protected]>");
+MODULE_DESCRIPTION("CM36651 proximity/ambient light sensor driver");
+MODULE_LICENSE("GPL v2");

I will fix and send v2 patch soon.

Best regards,
Beomho Seo

Signature · 3 lines

Beomho Seo, Assistant Engineer
System S/W Lab., S/W Platform Team, Software Center
Samsung Electronics