Re: [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.

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

sensors to be listed in alphabetical order

consistently start comments with uppercase letter

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

+
+/* 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 */

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

what are the magic constants 0x04, 0x08?

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

use i2c_smbus_read_byte_data() instead?

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

use i2c_smbus_read_word_data() instead?

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

use i2c_smbus_write_byte_data() instead?

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

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

use a #define for 0x01 to describe function

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

val is not used?

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

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

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

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

tmp must be initialized with the command?

+	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

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

typo: toe; rephase

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

_read_output() does not return data in 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;

rising and falling?

+	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

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

iio_device_free() not needed since using devm_iio_device_alloc()

+
+	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 [off-list ref]");
+MODULE_DESCRIPTION("CM36651 proximity/ambient light sensor driver");
+MODULE_LICENSE("GPL v2");

-- 

Peter Meerwald
+43-664-2444418 (mobile)