iio: cm36651: Add CM36651 proximity/light sensor driver

This patch add a new driver for Capella CM36651 proximity and RGB sensor.

drivers/iio/light/Kconfig +11 -0

index 0a25ae6..c965aeb 100644
@@ -27,6 +27,17 @@ config APDS9300
 	 To compile this driver as a module, choose M here: the
 	 module will be called apds9300.
 
+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.
+
 config GP2AP020A00F
 	tristate "Sharp GP2AP020A00F Proximity/ALS sensor"
 	depends on I2C

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

new file mode 100644
index 0000000..e1958ac
@@ -0,0 +1,591 @@
+/*
+ * 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>

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

+#define CM36651_CS_CONF2		0x01
+#define CM36651_ALS_WH_M		0x02
+#define CM36651_ALS_WH_L		0x03
+#define CM36651_ALS_WL_M		0x04
+#define CM36651_ALS_WL_L		0x05
+#define CM36651_CS_CONF3		0x06
+#define CM36651_CS_REG_NUM		0x07
+
+/* Proximity sensor */
+#define CM36651_PS_CONF1		0x00
+#define CM36651_PS_THD			0x01
+#define CM36651_PS_CANC		0x02
+#define CM36651_PS_CONF2		0x03
+#define CM36651_PS_REG_NUM		0x04
+

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

+#define CM36651_IT_80			0x00
+#define CM36651_W_IT_160		0x01
+#define CM36651_W_IT_320		0x02
+#define CM36651_W_IT_640		0x03
+#define CM36651_B_IT_160		0x04
+#define CM36651_B_IT_320		0x08
+#define CM36651_B_IT_640		0x0C
+#define CM36651_G_IT_160		0x10
+#define CM36651_G_IT_320		0x20
+#define CM36651_G_IT_640		0x30
+#define CM36651_R_IT_160		0x40
+#define CM36651_R_IT_320		0x80
+#define CM36651_R_IT_640		0xC0
+
+/* PS_CONF1 command code */
+#define CM36651_PS_ENABLE		0x00
+#define CM36651_PS_DISABLE		0x01
+#define CM36651_PS_INT_EN		0x02
+#define CM36651_PS_PERS_2		0x04
+#define CM36651_PS_PERS_3		0x08
+#define CM36651_PS_PERS_4		0x0C
+#define CM36651_PS_IT_2		0x10
+#define CM36651_PS_IT_3		0x20
+#define CM36651_PS_IT_4		0x30
+
+/* PS_THD command code */
+#define CM36651_PS_INITIAL_THD		0x09
+
+/* PS_CANC command code */
+#define CM36651_PS_CANC_DEFAULT	0x00
+
+/* PS_CONF2 command code */
+#define CM36651_PS_HYS_1		0x00
+#define CM36651_PS_HYS_2		0x01
+#define CM36651_PS_SMART_PERS_EN	0x02
+#define CM36651_PS_MS			0x10
+
+#define CM36651_SCAN_MODE_LIGHT	0
+#define CM36651_SCAN_MODE_PROX		1
+
+enum cm36651_operation_mode {
+	CM36651_LIGHT_EN,
+	CM36651_PROXIMITY_EN,
+	CM36651_PROXIMITY_EV_EN,
+};
+
+enum cm36651_light_channel_idx {
+	CM36651_LIGHT_CHANNEL_IDX_RED,
+	CM36651_LIGHT_CHANNEL_IDX_GREEN,
+	CM36651_LIGHT_CHANNEL_IDX_BLUE,
+	CM36651_LIGHT_CHANNEL_IDX_CLEAR,
+};
+
+enum cm36651_command {
+	CM36651_CMD_READ_RAW_LIGHT,
+	CM36651_CMD_READ_RAW_PROXIMITY,

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

+	CM36651_PS_CONF1,
+	CM36651_PS_THD,
+	CM36651_PS_CANC,
+	CM36651_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 cs_ctrl_regs[2];
+	u8 ps_ctrl_regs[4];
+	u16 color[4];
+};
+
+static int cm36651_setup_reg(struct cm36651_data *cm36651)
+{

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

+		ret = i2c_smbus_write_byte_data(client, cm36651_als_reg[i],
+						cm36651->cs_ctrl_regs[i]);
+		if (ret < 0)
+			goto err_setup_reg;
+	}

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

+
+	/* Device turn off */
+	ret = i2c_smbus_write_byte_data(client, CM36651_CS_CONF1,
+						CM36651_ALS_DISABLE);
+	if (ret < 0)
+		goto err_setup_reg;
+
+	ret = i2c_smbus_write_byte_data(cm36651->ps_client,
+				CM36651_PS_CONF1, CM36651_PS_DISABLE);
+	if (ret < 0)
+		goto err_setup_reg;
+
+	return 0;
+
+err_setup_reg:
+	dev_err(&client->dev, "Register setup failed: %d\n", ret);
+	return ret;
+}

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

+		if (val < 0)
+			goto read_err;
+
+		ret = i2c_smbus_write_byte_data(client, CM36651_CS_CONF1,
+							CM36651_ALS_DISABLE);
+		if (ret < 0)
+			goto write_err;
+
+		ret = IIO_VAL_INT;
+		break;
+	case CM36651_SCAN_MODE_PROX:
+		*val = i2c_smbus_read_byte(cm36651->ps_client);
+		if (val < 0)
+			goto read_err;
+
+		ret = i2c_smbus_write_byte_data(cm36651->ps_client,
+					CM36651_PS_CONF1, CM36651_PS_DISABLE);
+		if (ret < 0)
+			goto write_err;
+
+		ret = IIO_VAL_INT;
+		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;
+
+	ret = i2c_smbus_read_byte(cm36651->ps_client);
+	if (ret < 0) {
+		dev_err(&client->dev,
+				"%s: Data read failed: %d\n", __func__, ret);
+		return IRQ_HANDLED;
+	}
+
+	if (ret < CM36651_PS_INITIAL_THD) {
+		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);
+

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

+	int i;
+
+	switch (cmd) {
+	case CM36651_CMD_READ_RAW_LIGHT:
+		ret = i2c_smbus_write_byte_data(client, CM36651_CS_CONF1,
+				cm36651->cs_ctrl_regs[CM36651_CS_CONF1]);
+		break;
+	case CM36651_CMD_READ_RAW_PROXIMITY:
+		ret = i2c_smbus_write_byte_data(cm36651->ps_client,
+		   CM36651_PS_CONF1, cm36651->ps_ctrl_regs[CM36651_PS_CONF1]);

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

+			ret = i2c_smbus_write_byte_data(cm36651->ps_client,
+				cm36651_ps_reg[i], cm36651->ps_ctrl_regs[i]);
+			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 = i2c_smbus_write_byte_data(cm36651->ps_client,
+					CM36651_PS_CONF1, CM36651_PS_DISABLE);

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

+	return ret;
+}
+
+static int cm36651_read_channel(struct cm36651_data *cm36651,

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

+	ret = cm36651_read_output(cm36651, chan, val);
+	if (ret < 0) {
+		dev_err(&client->dev, "CM36651 read output failed\n");
+		return ret;
+	}
+
+	return ret;
+}
+
+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);
+
+	if (mask == IIO_CHAN_INFO_RAW)
+		ret = cm36651_read_channel(cm36651, chan, val);
+
+	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;
+
+	*val = cm36651->ps_ctrl_regs[CM36651_PS_THD];
+
+	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;

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

+	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);
+

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

+	.modified = 1,					\
+	.channel2 = IIO_MOD_LIGHT_##_color,		\
+}
+
+static const struct iio_chan_spec cm36651_channels[] = {
+	{
+		.type = IIO_PROXIMITY,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.scan_type = IIO_ST('u', 8, 8, 0),
+		.scan_index = CM36651_SCAN_MODE_PROX,
+		.event_mask = IIO_EV_BIT(IIO_EV_TYPE_THRESH, IIO_EV_DIR_EITHER)
+	},
+	CM36651_LIGHT_CHANNEL(RED, CM36651_LIGHT_CHANNEL_IDX_RED),
+	CM36651_LIGHT_CHANNEL(GREEN, CM36651_LIGHT_CHANNEL_IDX_GREEN),
+	CM36651_LIGHT_CHANNEL(BLUE, CM36651_LIGHT_CHANNEL_IDX_BLUE),
+	CM36651_LIGHT_CHANNEL(CLEAR, CM36651_LIGHT_CHANNEL_IDX_CLEAR),
+};
+
+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,

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

+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*cm36651));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	cm36651 = iio_priv(indio_dev);

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

+		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;

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

+	}
+
+	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);
+
+	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");

Quoted 2 lines

This patch add a new driver for Capella CM36651 proximity and RGB sensor.

moving in the right direction!
couple of comments inline

Quoted 47 lines

drivers/iio/light/Kconfig +11 -0

index 0a25ae6..c965aeb 100644
@@ -27,6 +27,17 @@ config APDS9300
 	 To compile this driver as a module, choose M here: the
 	 module will be called apds9300.
 
+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.
+
 config GP2AP020A00F
 	tristate "Sharp GP2AP020A00F Proximity/ALS sensor"
 	depends on I2C

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

new file mode 100644
index 0000000..e1958ac
@@ -0,0 +1,591 @@
+/*
+ * 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>
+

drivers/iio/light/cm36651.c line 24

Quoted 5 lines

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

+/* Slave address 0x19 for PS of 7 bit addressing protocol for I2C */
+#define CM36651_I2C_ADDR_PS		0x19
+
+/* Ambient light sensor */
+#define CM36651_CS_CONF1		0x00

what does CS stand for?
maybe ALS?

Quoted 15 lines

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

+#define CM36651_CS_CONF2		0x01
+#define CM36651_ALS_WH_M		0x02
+#define CM36651_ALS_WH_L		0x03
+#define CM36651_ALS_WL_M		0x04
+#define CM36651_ALS_WL_L		0x05
+#define CM36651_CS_CONF3		0x06
+#define CM36651_CS_REG_NUM		0x07
+
+/* Proximity sensor */
+#define CM36651_PS_CONF1		0x00
+#define CM36651_PS_THD		0x01
+#define CM36651_PS_CANC		0x02
+#define CM36651_PS_CONF2		0x03
+#define CM36651_PS_REG_NUM		0x04
+

drivers/iio/light/cm36651.c line 49

Quoted 10 lines

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

+/* CS_CONF1 command code */
+#define CM36651_ALS_ENABLE		0x00
+#define CM36651_ALS_DISABLE		0x01
+#define CM36651_ALS_INT_EN		0x02
+#define CM36651_ALS_THRES		0x04
+
+/* CS_CONF2 command code */
+#define CM36651_CS_CONF2_DEFAULT_BIT	0x08
+
+/* CS_CONF3 channel integration time */

which unit?
this could be made configurable via INT_TIME channel

Quoted 56 lines

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

+#define CM36651_IT_80		0x00
+#define CM36651_W_IT_160		0x01
+#define CM36651_W_IT_320		0x02
+#define CM36651_W_IT_640		0x03
+#define CM36651_B_IT_160		0x04
+#define CM36651_B_IT_320		0x08
+#define CM36651_B_IT_640		0x0C
+#define CM36651_G_IT_160		0x10
+#define CM36651_G_IT_320		0x20
+#define CM36651_G_IT_640		0x30
+#define CM36651_R_IT_160		0x40
+#define CM36651_R_IT_320		0x80
+#define CM36651_R_IT_640		0xC0
+
+/* PS_CONF1 command code */
+#define CM36651_PS_ENABLE		0x00
+#define CM36651_PS_DISABLE		0x01
+#define CM36651_PS_INT_EN		0x02
+#define CM36651_PS_PERS_2		0x04
+#define CM36651_PS_PERS_3		0x08
+#define CM36651_PS_PERS_4		0x0C
+#define CM36651_PS_IT_2		0x10
+#define CM36651_PS_IT_3		0x20
+#define CM36651_PS_IT_4		0x30
+
+/* PS_THD command code */
+#define CM36651_PS_INITIAL_THD	0x09
+
+/* PS_CANC command code */
+#define CM36651_PS_CANC_DEFAULT	0x00
+
+/* PS_CONF2 command code */
+#define CM36651_PS_HYS_1		0x00
+#define CM36651_PS_HYS_2		0x01
+#define CM36651_PS_SMART_PERS_EN	0x02
+#define CM36651_PS_MS		0x10
+
+#define CM36651_SCAN_MODE_LIGHT	0
+#define CM36651_SCAN_MODE_PROX	1
+
+enum cm36651_operation_mode {
+	CM36651_LIGHT_EN,
+	CM36651_PROXIMITY_EN,
+	CM36651_PROXIMITY_EV_EN,
+};
+
+enum cm36651_light_channel_idx {
+	CM36651_LIGHT_CHANNEL_IDX_RED,
+	CM36651_LIGHT_CHANNEL_IDX_GREEN,
+	CM36651_LIGHT_CHANNEL_IDX_BLUE,
+	CM36651_LIGHT_CHANNEL_IDX_CLEAR,
+};
+
+enum cm36651_command {
+	CM36651_CMD_READ_RAW_LIGHT,
+	CM36651_CMD_READ_RAW_PROXIMITY,

drivers/iio/light/cm36651.c line 115

Quoted 10 lines

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

+	CM36651_CMD_PROX_EV_EN,
+	CM36651_CMD_PROX_EV_DIS,
+};
+
+enum cm36651_proximity_event {
+	CM36651_CLOSE_PROXIMITY,
+	CM36651_FAR_PROXIMITY,
+};
+
+static u8 cm36651_als_reg[2] = {

const

drivers/iio/light/cm36651.c line 120

Quoted 5 lines

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

+	CM36651_CS_CONF1,
+	CM36651_CS_CONF2,
+};
+
+static u8 cm36651_ps_reg[4] = {

const

Quoted 20 lines

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

+	CM36651_PS_CONF1,
+	CM36651_PS_THD,
+	CM36651_PS_CANC,
+	CM36651_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 cs_ctrl_regs[2];
+	u8 ps_ctrl_regs[4];
+	u16 color[4];
+};
+
+static int cm36651_setup_reg(struct cm36651_data *cm36651)
+{

drivers/iio/light/cm36651.c line 150

Quoted 10 lines

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

+	struct i2c_client *client = cm36651->client;
+	struct i2c_client *ps_client = cm36651->ps_client;
+	int i, ret;
+
+	/* ALS initialization */
+	cm36651->cs_ctrl_regs[CM36651_CS_CONF1] = CM36651_ALS_ENABLE
+							| CM36651_ALS_THRES;
+	cm36651->cs_ctrl_regs[CM36651_CS_CONF2] = CM36651_CS_CONF2_DEFAULT_BIT;
+
+	for (i = 0; i < 2; i++) {

use the CS_REG_NUM #defined

Quoted 5 lines

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

+		ret = i2c_smbus_write_byte_data(client, cm36651_als_reg[i],
+						cm36651->cs_ctrl_regs[i]);
+		if (ret < 0)
+			goto err_setup_reg;
+	}

drivers/iio/light/cm36651.c line 165

Quoted 10 lines

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

+
+	/* PS initialization */
+	cm36651->ps_ctrl_regs[CM36651_PS_CONF1] = CM36651_PS_ENABLE
+				   | CM36651_PS_PERS_4 | CM36651_PS_IT_4;
+	cm36651->ps_ctrl_regs[CM36651_PS_THD] = CM36651_PS_INITIAL_THD;
+	cm36651->ps_ctrl_regs[CM36651_PS_CANC] = CM36651_PS_CANC_DEFAULT;
+	cm36651->ps_ctrl_regs[CM36651_PS_CONF2] = CM36651_PS_HYS_2
+			      | CM36651_PS_SMART_PERS_EN | CM36651_PS_MS;
+
+	for (i = 0; i < 4; i++) {

use the PS_REG_NUM #defined

drivers/iio/light/cm36651.c line 177

Quoted 12 lines

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

+		ret = i2c_smbus_write_byte_data(ps_client, cm36651_ps_reg[i],
+						cm36651->ps_ctrl_regs[i]);
+		if (ret < 0)
+			goto err_setup_reg;
+	}
+
+	ret = i2c_smbus_read_byte(cm36651->ps_client);
+	if (ret < 0)
+		goto err_setup_reg;
+
+	/* Printing the initial proximity value with no contact */
+	dev_dbg(&client->dev, "Initial proximity value: %d\n", ret);

maybe drop the initial read? not needed when not in debug mode

Quoted 23 lines

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

+
+	/* Device turn off */
+	ret = i2c_smbus_write_byte_data(client, CM36651_CS_CONF1,
+						CM36651_ALS_DISABLE);
+	if (ret < 0)
+		goto err_setup_reg;
+
+	ret = i2c_smbus_write_byte_data(cm36651->ps_client,
+				CM36651_PS_CONF1, CM36651_PS_DISABLE);
+	if (ret < 0)
+		goto err_setup_reg;
+
+	return 0;
+
+err_setup_reg:
+	dev_err(&client->dev, "Register setup failed: %d\n", ret);
+	return ret;
+}
+
+static int cm36651_read_output(struct cm36651_data *cm36651,
+				struct iio_chan_spec const *chan, int *val)
+{
+	struct i2c_client *client = cm36651->client;

drivers/iio/light/cm36651.c line 205

Quoted 5 lines

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

+	int ret = -EINVAL;
+
+	switch (chan->scan_index) {
+	case CM36651_SCAN_MODE_LIGHT:
+		*val = i2c_smbus_read_word_data(client, chan->channel);

the CM36651 seems to be special here how data is returned; there seems to be no dedicated register to pass the measurement?

Quoted 62 lines

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

+		if (val < 0)
+			goto read_err;
+
+		ret = i2c_smbus_write_byte_data(client, CM36651_CS_CONF1,
+							CM36651_ALS_DISABLE);
+		if (ret < 0)
+			goto write_err;
+
+		ret = IIO_VAL_INT;
+		break;
+	case CM36651_SCAN_MODE_PROX:
+		*val = i2c_smbus_read_byte(cm36651->ps_client);
+		if (val < 0)
+			goto read_err;
+
+		ret = i2c_smbus_write_byte_data(cm36651->ps_client,
+					CM36651_PS_CONF1, CM36651_PS_DISABLE);
+		if (ret < 0)
+			goto write_err;
+
+		ret = IIO_VAL_INT;
+		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;
+
+	ret = i2c_smbus_read_byte(cm36651->ps_client);
+	if (ret < 0) {
+		dev_err(&client->dev,
+				"%s: Data read failed: %d\n", __func__, ret);
+		return IRQ_HANDLED;
+	}
+
+	if (ret < CM36651_PS_INITIAL_THD) {
+		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);
+

drivers/iio/light/cm36651.c line 277

Quoted 10 lines

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

+	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_command cmd)
+{
+	struct i2c_client *client = cm36651->client;
+	int ret = 0;

unknown cmd are silently not handled;
probably better to set ret = -EINVAL

Quoted 10 lines

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

+	int i;
+
+	switch (cmd) {
+	case CM36651_CMD_READ_RAW_LIGHT:
+		ret = i2c_smbus_write_byte_data(client, CM36651_CS_CONF1,
+				cm36651->cs_ctrl_regs[CM36651_CS_CONF1]);
+		break;
+	case CM36651_CMD_READ_RAW_PROXIMITY:
+		ret = i2c_smbus_write_byte_data(cm36651->ps_client,
+		   CM36651_PS_CONF1, cm36651->ps_ctrl_regs[CM36651_PS_CONF1]);

drivers/iio/light/cm36651.c line 297

Quoted 10 lines

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

+		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++) {

REG_NUM

Quoted 18 lines

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

+			ret = i2c_smbus_write_byte_data(cm36651->ps_client,
+				cm36651_ps_reg[i], cm36651->ps_ctrl_regs[i]);
+			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 = i2c_smbus_write_byte_data(cm36651->ps_client,
+					CM36651_PS_CONF1, CM36651_PS_DISABLE);

drivers/iio/light/cm36651.c line 325

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 enable event is failed\n");

remove 'is'

Quoted 4 lines

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

+	return ret;
+}
+
+static int cm36651_read_channel(struct cm36651_data *cm36651,

drivers/iio/light/cm36651.c line 339

Quoted 10 lines

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

+				struct iio_chan_spec const *chan, int *val)
+{
+	struct i2c_client *client = cm36651->client;
+	enum cm36651_command cmd = 0;
+	int ret;
+
+	if (chan->scan_index == CM36651_SCAN_MODE_LIGHT)
+		cmd = CM36651_CMD_READ_RAW_LIGHT;
+	else /* CM36651_SCAN_MODE_PROX */
+		cmd = CM36651_CMD_READ_RAW_PROXIMITY;

chan->type could be used to distinguish between LIGHT and PROXIMITY, so no need for scan_index

drivers/iio/light/cm36651.c line 347

Quoted 8 lines

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

+
+	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);

shouldn't this depend on the integration time configured? same for ALS and PS?

Quoted 47 lines

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

+	ret = cm36651_read_output(cm36651, chan, val);
+	if (ret < 0) {
+		dev_err(&client->dev, "CM36651 read output failed\n");
+		return ret;
+	}
+
+	return ret;
+}
+
+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);
+
+	if (mask == IIO_CHAN_INFO_RAW)
+		ret = cm36651_read_channel(cm36651, chan, val);
+
+	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;
+
+	*val = cm36651->ps_ctrl_regs[CM36651_PS_THD];
+
+	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;

drivers/iio/light/cm36651.c line 404

Quoted 10 lines

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

+
+	if (val < 3 || val > 255)
+		return -EINVAL;
+
+	cm36651->ps_ctrl_regs[CM36651_PS_THD] = val;
+	ret = i2c_smbus_write_byte_data(cm36651->ps_client, CM36651_PS_THD,
+					cm36651->ps_ctrl_regs[CM36651_PS_THD]);
+
+	if (ret < 0) {
+		dev_err(&client->dev, "PS register read faied: %d\n", ret);

that's a write; 'failed', maybe more specific: "PS threshold write failed"

drivers/iio/light/cm36651.c line 417

Quoted 13 lines

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

+		return ret;
+	}
+	dev_dbg(&client->dev, "New threshold is 0x%x\n",
+					cm36651->ps_ctrl_regs[CM36651_PS_THD]);
+
+	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_command cmd;

could avoid that local variable or move it down

Quoted 26 lines

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

+	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);
+

drivers/iio/light/cm36651.c line 453

Quoted 10 lines

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

+	mutex_unlock(&cm36651->lock);
+
+	return event_en;
+}
+
+#define CM36651_LIGHT_CHANNEL(_color, _idx) {		\
+	.type = IIO_LIGHT,				\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
+	.scan_type = IIO_ST('u', 16, 16, 0),		\
+	.scan_index = CM36651_SCAN_MODE_LIGHT,		\

no need scan_type and scan_index; driver does not support buffering

drivers/iio/light/cm36651.c line 454

Quoted 1 line

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

+	.channel = _idx,

I'd rather use .address than .channel; there is just one light channel

				\

Quoted 23 lines

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

+	.modified = 1,					\
+	.channel2 = IIO_MOD_LIGHT_##_color,		\
+}
+
+static const struct iio_chan_spec cm36651_channels[] = {
+	{
+		.type = IIO_PROXIMITY,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.scan_type = IIO_ST('u', 8, 8, 0),
+		.scan_index = CM36651_SCAN_MODE_PROX,
+		.event_mask = IIO_EV_BIT(IIO_EV_TYPE_THRESH, IIO_EV_DIR_EITHER)
+	},
+	CM36651_LIGHT_CHANNEL(RED, CM36651_LIGHT_CHANNEL_IDX_RED),
+	CM36651_LIGHT_CHANNEL(GREEN, CM36651_LIGHT_CHANNEL_IDX_GREEN),
+	CM36651_LIGHT_CHANNEL(BLUE, CM36651_LIGHT_CHANNEL_IDX_BLUE),
+	CM36651_LIGHT_CHANNEL(CLEAR, CM36651_LIGHT_CHANNEL_IDX_CLEAR),
+};
+
+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,

drivers/iio/light/cm36651.c line 487

Quoted 10 lines

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

+	.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;

could avoid variable irqflag

drivers/iio/light/cm36651.c line 490

Quoted 3 lines

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

+	int ret;
+
+	dev_dbg(&client->dev, "cm36651 light/proxymity sensor probe\n");

proximity

Quoted 6 lines

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

+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*cm36651));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	cm36651 = iio_priv(indio_dev);

drivers/iio/light/cm36651.c line 506

Quoted 10 lines

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

+
+	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");

regulator vled

Quoted 11 lines

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

+		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;

drivers/iio/light/cm36651.c line 527

Quoted 10 lines

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

+	indio_dev->num_channels = ARRAY_SIZE(cm36651_channels);
+	indio_dev->info = &cm36651_info;
+	indio_dev->name = id->name;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	/* Check if the device is there or not */
+	ret = i2c_smbus_write_byte_data(cm36651->ps_client, CM36651_PS_CONF1,
+							   CM36651_PS_DISABLE);
+	if (ret < 0) {
+		dev_err(&client->dev, "PS register read faied: %d\n", ret);

"failed"
isn't there a better way to check? setup_reg() would fail also if the device is not there?!

regulator doesn't get disabled

drivers/iio/light/cm36651.c line 531

Quoted 4 lines

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

+		return ret;
+	}
+
+	ret = cm36651_setup_reg(cm36651);

ret not checked

drivers/iio/light/cm36651.c line 537

Quoted 6 lines

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

+
+	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__);

regulator not disabled

drivers/iio/light/cm36651.c line 546

Quoted 9 lines

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

+		return ret;
+	}
+	disable_irq(client->irq);
+
+	ret = iio_device_register(indio_dev);
+	if (ret) {
+		regulator_disable(cm36651->vled_reg);
+		free_irq(client->irq, indio_dev);
+		return ret;

move 'return ret' down, save 'return 0'

Quoted 46 lines

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

+	}
+
+	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);
+
+	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");