linux-iio.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/3] iio: cros_ec: Add support for RGB light sensor
@ 2020-05-06 23:03 Gwendal Grignou
  2020-05-06 23:03 ` [PATCH v2 1/3] iio: Add in_illumincance vectors in different color spaces Gwendal Grignou
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Gwendal Grignou @ 2020-05-06 23:03 UTC (permalink / raw)
  To: enric.balletbo, jic23
  Cc: bleung, groeck, linux-kernel, linux-iio, Gwendal Grignou

Add support for color light sensor presented by the Chromebook Embedded
Controller (EC).
Instead of just presenting lux measurement (clear channel), a color light
sensor is able to report color temperature measurement.

The EC, using factory settings, can transform the raw measurement into
the CIE 1931 XYZ color space (XYZ) and take adavantage of color sensor
autocalibration to provide the most accurate measurements.

Gwendal Grignou (3):
  iio: Add in_illumincance vectors in different color spaces
  iio: cros_ec: Allow enabling/disabling calibration mode
  iio: cros_ec_light: Add support for RGB sensor

 Documentation/ABI/testing/sysfs-bus-iio       |  27 +
 .../cros_ec_sensors/cros_ec_sensors_core.c    |   3 +-
 drivers/iio/light/cros_ec_light_prox.c        | 469 +++++++++++++++---
 drivers/platform/chrome/cros_ec_sensorhub.c   |   3 +
 .../linux/iio/common/cros_ec_sensors_core.h   |   1 -
 .../linux/platform_data/cros_ec_commands.h    |  14 +-
 6 files changed, 441 insertions(+), 76 deletions(-)

-- 
2.26.2.526.g744177e7f7-goog


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 1/3] iio: Add in_illumincance vectors in different color spaces
  2020-05-06 23:03 [PATCH v3 0/3] iio: cros_ec: Add support for RGB light sensor Gwendal Grignou
@ 2020-05-06 23:03 ` Gwendal Grignou
  2020-05-08 15:16   ` Jonathan Cameron
  2020-05-06 23:03 ` [PATCH v2 2/3] iio: cros_ec: Allow enabling/disabling calibration mode Gwendal Grignou
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 10+ messages in thread
From: Gwendal Grignou @ 2020-05-06 23:03 UTC (permalink / raw)
  To: enric.balletbo, jic23
  Cc: bleung, groeck, linux-kernel, linux-iio, Gwendal Grignou

Define 2 spaces for defining color coming from color sensors:
RGB and XYZ: Both are in lux.
RGB is the raw output from sensors (Red, Green and Blue channels), in
addition to the existing clear channel (C).
The RGBC vector goes through a matrix transformation to produce the XYZ
vector. Y is illumincance, and XY caries the chromaticity information.
The matrix is model specific, as the color sensor can be behing a glass
that can filter some wavelengths.

Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
---
New in v2.

 Documentation/ABI/testing/sysfs-bus-iio | 27 +++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index d3e53a6d8331b..256db6e63a25e 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -1309,6 +1309,33 @@ Description:
 		Illuminance measurement, units after application of scale
 		and offset are lux.
 
+What:		/sys/.../iio:deviceX/in_illuminance_red_raw
+What:		/sys/.../iio:deviceX/in_illuminance_green_raw
+What:		/sys/.../iio:deviceX/in_illuminance_blue_raw
+KernelVersion:	5.7
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Illuminance measuremed in red, green or blue channels, units
+		after application of scale and offset are lux.
+
+What:		/sys/.../iio:deviceX/in_illuminance_x_raw
+What:		/sys/.../iio:deviceX/in_illuminance_y_raw
+What:		/sys/.../iio:deviceX/in_illuminance_z_raw
+KernelVersion:	5.7
+Contact:	linux-iio@vger.kernel.org
+Description:
+		lluminance measured in the CIE 1931 color space (XYZ).
+		in_illuminance_y_raw is a measure of the brightness, and is
+		identical in_illuminance_raw.
+		in_illuminance_x_raw and in_illuminance_z_raw carry chromacity
+		information.
+		in_illuminance_x,y,z_raw are be obtained from the sensor color
+		channels using color matching functions that may be device
+		specific.
+		Units after application of scale and offset are lux.
+		The measurments can be used to represent colors in the CIE
+		xyY color space
+
 What:		/sys/.../iio:deviceX/in_intensityY_raw
 What:		/sys/.../iio:deviceX/in_intensityY_ir_raw
 What:		/sys/.../iio:deviceX/in_intensityY_both_raw
-- 
2.26.2.526.g744177e7f7-goog


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 2/3] iio: cros_ec: Allow enabling/disabling calibration mode
  2020-05-06 23:03 [PATCH v3 0/3] iio: cros_ec: Add support for RGB light sensor Gwendal Grignou
  2020-05-06 23:03 ` [PATCH v2 1/3] iio: Add in_illumincance vectors in different color spaces Gwendal Grignou
@ 2020-05-06 23:03 ` Gwendal Grignou
  2020-05-06 23:03 ` [PATCH v2 3/3] iio: cros_ec_light: Add support for RGB sensor Gwendal Grignou
  2020-05-08 14:56 ` [PATCH v3 0/3] iio: cros_ec: Add support for RGB light sensor Jonathan Cameron
  3 siblings, 0 replies; 10+ messages in thread
From: Gwendal Grignou @ 2020-05-06 23:03 UTC (permalink / raw)
  To: enric.balletbo, jic23
  Cc: bleung, groeck, linux-kernel, linux-iio, Gwendal Grignou

calibration was a one-shot event sent to the sensor to calibrate itself.
It is used on Bosch sensors (BMI160, BMA254).
For TCS3400 light sensor, we need to stay in calibration mode to run
tests.
Accept boolean true and false (not just true) to enter/exit calibration.

Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
---
 .../common/cros_ec_sensors/cros_ec_sensors_core.c    |  3 +--
 include/linux/platform_data/cros_ec_commands.h       | 12 +++++++++---
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
index c831915ca7e56..3d8b25ee9d80c 100644
--- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
+++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
@@ -411,11 +411,10 @@ static ssize_t cros_ec_sensors_calibrate(struct iio_dev *indio_dev,
 	ret = strtobool(buf, &calibrate);
 	if (ret < 0)
 		return ret;
-	if (!calibrate)
-		return -EINVAL;
 
 	mutex_lock(&st->cmd_lock);
 	st->param.cmd = MOTIONSENSE_CMD_PERFORM_CALIB;
+	st->param.perform_calib.enable = calibrate;
 	ret = cros_ec_motion_send_host_cmd(st, 0);
 	if (ret != 0) {
 		dev_warn(&indio_dev->dev, "Unable to calibrate sensor\n");
diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h
index 451885c697cc3..395c9b2b05c66 100644
--- a/include/linux/platform_data/cros_ec_commands.h
+++ b/include/linux/platform_data/cros_ec_commands.h
@@ -2517,13 +2517,19 @@ struct ec_params_motion_sense {
 
 		/*
 		 * Used for MOTIONSENSE_CMD_INFO, MOTIONSENSE_CMD_DATA
-		 * and MOTIONSENSE_CMD_PERFORM_CALIB.
 		 */
 		struct __ec_todo_unpacked {
 			uint8_t sensor_num;
-		} info, info_3, data, fifo_flush, perform_calib,
-				list_activities;
+		} info, info_3, data, fifo_flush, list_activities;
 
+		/*
+		 * Used for MOTIONSENSE_CMD_PERFORM_CALIB:
+		 * Allow entering/exiting the calibration mode.
+		 */
+		struct __ec_todo_unpacked {
+			uint8_t sensor_num;
+			uint8_t enable;
+		} perform_calib;
 		/*
 		 * Used for MOTIONSENSE_CMD_EC_RATE, MOTIONSENSE_CMD_SENSOR_ODR
 		 * and MOTIONSENSE_CMD_SENSOR_RANGE.
-- 
2.26.2.526.g744177e7f7-goog


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 3/3] iio: cros_ec_light: Add support for RGB sensor
  2020-05-06 23:03 [PATCH v3 0/3] iio: cros_ec: Add support for RGB light sensor Gwendal Grignou
  2020-05-06 23:03 ` [PATCH v2 1/3] iio: Add in_illumincance vectors in different color spaces Gwendal Grignou
  2020-05-06 23:03 ` [PATCH v2 2/3] iio: cros_ec: Allow enabling/disabling calibration mode Gwendal Grignou
@ 2020-05-06 23:03 ` Gwendal Grignou
  2020-05-08 15:23   ` Jonathan Cameron
  2020-05-11  6:31   ` kbuild test robot
  2020-05-08 14:56 ` [PATCH v3 0/3] iio: cros_ec: Add support for RGB light sensor Jonathan Cameron
  3 siblings, 2 replies; 10+ messages in thread
From: Gwendal Grignou @ 2020-05-06 23:03 UTC (permalink / raw)
  To: enric.balletbo, jic23
  Cc: bleung, groeck, linux-kernel, linux-iio, Gwendal Grignou

Add support for color sensors behind EC like TCS3400.
The color data can be presented in Red Green Blue color space (RGB) or
the CIE 1931 XYZ color space (XYZ).
In XYZ mode, the sensor is configured for auto calibrating its channels
and is the "normal" mode.
The driver tells the EC to switch between the 2 modes by using the
calibration command.
When the sensor is in calibration mode, only clear and RGB channels are
available. In normal mode, only clear and XYZ are.
When RGB channels are enabled, the sensor switches to calibration mode
when the buffer is enabled.

When reading trhough sysfs command, set calibration mode and then read
the channel(s). A command will be issue for each read, so the channels
may come from different sensor sample.
When using the buffer, after setting the mask, when the buffer is
enabled, the calibration will be set based on the channel mask.

libiio tools can be used to gather sensor information:
iio_readdev -s 10 cros-ec-light \
illuminance_clear illuminance_x illuminance_y illuminance_z

To match IIO ABI, the clear illuminance channel has been renamed
in_illuminance_clear_raw from in_illuminance_input.

Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
---
 drivers/iio/light/cros_ec_light_prox.c        | 469 +++++++++++++++---
 drivers/platform/chrome/cros_ec_sensorhub.c   |   3 +
 .../linux/iio/common/cros_ec_sensors_core.h   |   1 -
 .../linux/platform_data/cros_ec_commands.h    |   2 +
 4 files changed, 404 insertions(+), 71 deletions(-)

diff --git a/drivers/iio/light/cros_ec_light_prox.c b/drivers/iio/light/cros_ec_light_prox.c
index 2198b50909ed0..83bd3057b334c 100644
--- a/drivers/iio/light/cros_ec_light_prox.c
+++ b/drivers/iio/light/cros_ec_light_prox.c
@@ -17,82 +17,188 @@
 #include <linux/module.h>
 #include <linux/platform_data/cros_ec_commands.h>
 #include <linux/platform_data/cros_ec_proto.h>
+#include <linux/platform_data/cros_ec_sensorhub.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 
 /*
- * We only represent one entry for light or proximity. EC is merging different
- * light sensors to return the what the eye would see. For proximity, we
- * currently support only one light source.
+ * We may present up to 7 channels:
+ *
+ * +-----+-----+-----+-----+-----+-----+-----+
+ * |Clear|  X  |  Y  |  Z  | RED |BLUE |GREEN|
+ * |Prox |     |     |     |     |     |     |
+ * +-----+-----+-----+-----+-----+-----+-----+
+ *
+ * Prox[imity] is presented by proximity sensors.
+ * The clear channel is supported by single and color light sensors.
+ * Color light sensor either reports color information in the RGB space or
+ * the CIE 1931 XYZ (XYZ) color space.
  */
-#define CROS_EC_LIGHT_PROX_MAX_CHANNELS (1 + 1)
+#define CROS_EC_LIGHT_CLEAR_OR_PROXIMITY_MASK GENMASK(0, 0)
+#define CROS_EC_LIGHT_XYZ_SPACE_MASK GENMASK(3, 1)
+#define CROS_EC_LIGHT_RGB_SPACE_MASK (6, 4)
+
+/*
+ * We always represent one entry for light or proximity, and all
+ * samples can be timestamped.
+ */
+#define CROS_EC_LIGHT_PROX_MIN_CHANNELS (1 + 1)
+
+static const unsigned long cros_ec_light_prox_bitmasks[] = {
+	CROS_EC_LIGHT_CLEAR_OR_PROXIMITY_MASK,
+	CROS_EC_LIGHT_XYZ_SPACE_MASK,
+	CROS_EC_LIGHT_XYZ_SPACE_MASK | CROS_EC_LIGHT_CLEAR_OR_PROXIMITY_MASK,
+	CROS_EC_LIGHT_RGB_SPACE_MASK,
+	CROS_EC_LIGHT_RGB_SPACE_MASK | CROS_EC_LIGHT_CLEAR_OR_PROXIMITY_MASK,
+	0,
+};
+
+#define CROS_EC_LIGHT_IDX_TO_CHAN(_idx) (((_idx) - 1) % CROS_EC_SENSOR_MAX_AXIS)
 
 /* State data for ec_sensors iio driver. */
 struct cros_ec_light_prox_state {
 	/* Shared by all sensors */
 	struct cros_ec_sensors_core_state core;
 
-	struct iio_chan_spec channels[CROS_EC_LIGHT_PROX_MAX_CHANNELS];
+	/* Calibration information for the color channels. */
+	struct calib_data rgb_calib[CROS_EC_SENSOR_MAX_AXIS];
 };
 
+static void cros_ec_light_channel_common(struct iio_chan_spec *channel)
+{
+	channel->info_mask_shared_by_all =
+		BIT(IIO_CHAN_INFO_SAMP_FREQ);
+	channel->info_mask_shared_by_all_available =
+		BIT(IIO_CHAN_INFO_SAMP_FREQ);
+	channel->scan_type.realbits = CROS_EC_SENSOR_BITS;
+	channel->scan_type.storagebits = CROS_EC_SENSOR_BITS;
+	channel->ext_info = cros_ec_sensors_ext_info;
+	channel->scan_type.sign = 'u';
+}
+
+static int
+cros_ec_light_extra_send_host_cmd(struct cros_ec_sensors_core_state *state,
+				  int increment, u16 opt_length)
+{
+	u8 save_sensor_num = state->param.info.sensor_num;
+	int ret;
+
+	state->param.info.sensor_num += increment;
+	ret = cros_ec_motion_send_host_cmd(state, opt_length);
+	state->param.info.sensor_num = save_sensor_num;
+	return ret;
+}
+
+static int cros_ec_light_prox_read_data(struct iio_dev *indio_dev,
+					struct iio_chan_spec const *chan,
+					int *val)
+{
+	struct cros_ec_light_prox_state *st = iio_priv(indio_dev);
+	int ret;
+	int idx = chan->scan_index;
+	s16 data;
+
+	/*
+	 * The data coming from the light sensor is
+	 * pre-processed and represents the ambient light
+	 * illuminance reading expressed in lux.
+	 */
+	if (idx == 0) {
+		ret = cros_ec_sensors_read_cmd(indio_dev, 1, &data);
+		if (ret < 0)
+			return ret;
+		*val = data;
+	} else {
+		ret = cros_ec_light_extra_send_host_cmd(
+				&st->core, 1, sizeof(st->core.resp->data));
+		if (ret)
+			return ret;
+		*val = st->core.resp->data.data[CROS_EC_LIGHT_IDX_TO_CHAN(idx)];
+	}
+	return IIO_VAL_INT;
+}
+
+static int cros_ec_light_read_color_scale(struct cros_ec_light_prox_state *st,
+					  int idx, int *val, int *val2)
+{
+	int ret, i;
+	u16 scale;
+
+	st->core.param.cmd = MOTIONSENSE_CMD_SENSOR_SCALE;
+	st->core.param.sensor_scale.flags = 0;
+	if (idx == 0)
+		ret = cros_ec_motion_send_host_cmd(&st->core, 0);
+	else
+		ret = cros_ec_light_extra_send_host_cmd(&st->core, 1, 0);
+	if (ret)
+		return ret;
+
+	if (idx == 0) {
+		scale = st->core.resp->sensor_scale.scale[0];
+		st->core.calib[0].scale = scale;
+	} else {
+		for (i = CROS_EC_SENSOR_X; i < CROS_EC_SENSOR_MAX_AXIS; i++)
+			st->rgb_calib[i].scale =
+				st->core.resp->sensor_scale.scale[i];
+		scale = st->rgb_calib[CROS_EC_LIGHT_IDX_TO_CHAN(idx)].scale;
+	}
+	/*
+	 * scale is a number x.y, where x is coded on 1 bit,
+	 * y coded on 15 bits, between 0 and 9999.
+	 */
+	*val = scale >> 15;
+	*val2 = ((scale & 0x7FFF) * 1000000LL) /
+		MOTION_SENSE_DEFAULT_SCALE;
+	return IIO_VAL_INT_PLUS_MICRO;
+}
+
 static int cros_ec_light_prox_read(struct iio_dev *indio_dev,
 				   struct iio_chan_spec const *chan,
 				   int *val, int *val2, long mask)
 {
 	struct cros_ec_light_prox_state *st = iio_priv(indio_dev);
-	u16 data = 0;
-	s64 val64;
-	int ret;
+	int i, ret;
 	int idx = chan->scan_index;
+	s64 val64;
 
 	mutex_lock(&st->core.cmd_lock);
-
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		if (chan->type == IIO_PROXIMITY) {
-			ret = cros_ec_sensors_read_cmd(indio_dev, 1 << idx,
-						     (s16 *)&data);
-			if (ret)
-				break;
-			*val = data;
-			ret = IIO_VAL_INT;
-		} else {
-			ret = -EINVAL;
-		}
-		break;
-	case IIO_CHAN_INFO_PROCESSED:
-		if (chan->type == IIO_LIGHT) {
-			ret = cros_ec_sensors_read_cmd(indio_dev, 1 << idx,
-						     (s16 *)&data);
-			if (ret)
-				break;
-			/*
-			 * The data coming from the light sensor is
-			 * pre-processed and represents the ambient light
-			 * illuminance reading expressed in lux.
-			 */
-			*val = data;
-			ret = IIO_VAL_INT;
-		} else {
-			ret = -EINVAL;
-		}
+		ret = cros_ec_light_prox_read_data(indio_dev, chan, val);
 		break;
 	case IIO_CHAN_INFO_CALIBBIAS:
 		st->core.param.cmd = MOTIONSENSE_CMD_SENSOR_OFFSET;
 		st->core.param.sensor_offset.flags = 0;
 
-		ret = cros_ec_motion_send_host_cmd(&st->core, 0);
+		if (idx == 0)
+			ret = cros_ec_motion_send_host_cmd(&st->core, 0);
+		else
+			ret = cros_ec_light_extra_send_host_cmd(
+					&st->core, 1, 0);
 		if (ret)
 			break;
-
-		/* Save values */
-		st->core.calib[0].offset =
-			st->core.resp->sensor_offset.offset[0];
-
-		*val = st->core.calib[idx].offset;
+		if (idx == 0) {
+			*val = st->core.calib[0].offset =
+				st->core.resp->sensor_offset.offset[0];
+		} else {
+			for (i = CROS_EC_SENSOR_X; i < CROS_EC_SENSOR_MAX_AXIS;
+			     i++)
+				st->rgb_calib[i].offset =
+					st->core.resp->sensor_offset.offset[i];
+			i = CROS_EC_LIGHT_IDX_TO_CHAN(idx);
+			*val = st->rgb_calib[i].offset;
+		}
 		ret = IIO_VAL_INT;
 		break;
 	case IIO_CHAN_INFO_CALIBSCALE:
+		if (indio_dev->num_channels > CROS_EC_LIGHT_PROX_MIN_CHANNELS) {
+			ret = cros_ec_light_read_color_scale(st, idx, val,
+							     val2);
+			break;
+		}
+		/* RANGE is used for calibration in 1 channel sensors. */
+		fallthrough;
+	case IIO_CHAN_INFO_SCALE:
 		/*
 		 * RANGE is used for calibration
 		 * scale is a number x.y, where x is coded on 16 bits,
@@ -121,29 +227,85 @@ static int cros_ec_light_prox_read(struct iio_dev *indio_dev,
 	return ret;
 }
 
+static int cros_ec_light_write_color_scale(struct cros_ec_light_prox_state *st,
+					   int idx, int val, int val2)
+{
+	int i;
+	u16 scale;
+
+	if (val >= 2) {
+		/*
+		 * The user space is sending values already
+		 * multiplied by MOTION_SENSE_DEFAULT_SCALE.
+		 */
+		scale = val;
+	} else {
+		u64 val64 = val2 * MOTION_SENSE_DEFAULT_SCALE;
+
+		do_div(val64, 1000000);
+		scale = (val << 15) | val64;
+	}
+
+	st->core.param.cmd = MOTIONSENSE_CMD_SENSOR_SCALE;
+	st->core.param.sensor_offset.flags = MOTION_SENSE_SET_OFFSET;
+	st->core.param.sensor_offset.temp = EC_MOTION_SENSE_INVALID_CALIB_TEMP;
+	if (idx == 0) {
+		st->core.calib[0].scale = scale;
+		st->core.param.sensor_scale.scale[0] = scale;
+		return cros_ec_motion_send_host_cmd(&st->core, 0);
+	}
+
+	st->rgb_calib[CROS_EC_LIGHT_IDX_TO_CHAN(idx)].scale = scale;
+	for (i = CROS_EC_SENSOR_X; i < CROS_EC_SENSOR_MAX_AXIS; i++)
+		st->core.param.sensor_scale.scale[i] = st->rgb_calib[i].scale;
+	return cros_ec_light_extra_send_host_cmd(&st->core, 1, 0);
+}
+
 static int cros_ec_light_prox_write(struct iio_dev *indio_dev,
 			       struct iio_chan_spec const *chan,
 			       int val, int val2, long mask)
 {
 	struct cros_ec_light_prox_state *st = iio_priv(indio_dev);
-	int ret;
+	int ret, i;
 	int idx = chan->scan_index;
 
 	mutex_lock(&st->core.cmd_lock);
 
 	switch (mask) {
 	case IIO_CHAN_INFO_CALIBBIAS:
-		st->core.calib[idx].offset = val;
 		/* Send to EC for each axis, even if not complete */
 		st->core.param.cmd = MOTIONSENSE_CMD_SENSOR_OFFSET;
 		st->core.param.sensor_offset.flags = MOTION_SENSE_SET_OFFSET;
-		st->core.param.sensor_offset.offset[0] =
-			st->core.calib[0].offset;
 		st->core.param.sensor_offset.temp =
 					EC_MOTION_SENSE_INVALID_CALIB_TEMP;
-		ret = cros_ec_motion_send_host_cmd(&st->core, 0);
+		if (idx == 0) {
+			st->core.calib[0].offset = val;
+			st->core.param.sensor_offset.offset[0] = val;
+			ret = cros_ec_motion_send_host_cmd(&st->core, 0);
+		} else {
+			i = CROS_EC_LIGHT_IDX_TO_CHAN(idx);
+			st->rgb_calib[i].offset = val;
+			for (i = CROS_EC_SENSOR_X;
+			     i < CROS_EC_SENSOR_MAX_AXIS;
+			     i++)
+				st->core.param.sensor_offset.offset[i] =
+					st->rgb_calib[i].offset;
+			ret = cros_ec_light_extra_send_host_cmd(
+					&st->core, 1, 0);
+		}
 		break;
 	case IIO_CHAN_INFO_CALIBSCALE:
+		if (indio_dev->num_channels > CROS_EC_LIGHT_PROX_MIN_CHANNELS) {
+			ret = cros_ec_light_write_color_scale(st, idx,
+							      val, val2);
+			break;
+		}
+		/*
+		 * For sensors with only one channel, _RANGE is used
+		 * instead of _SCALE.
+		 */
+		fallthrough;
+	case IIO_CHAN_INFO_SCALE:
 		st->core.param.cmd = MOTIONSENSE_CMD_SENSOR_RANGE;
 		st->core.param.sensor_range.data = (val << 16) | (val2 / 100);
 		ret = cros_ec_motion_send_host_cmd(&st->core, 0);
@@ -159,27 +321,154 @@ static int cros_ec_light_prox_write(struct iio_dev *indio_dev,
 	return ret;
 }
 
+static int cros_ec_light_push_data(struct iio_dev *indio_dev,
+				   s16 *data,
+				   s64 timestamp)
+{
+	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
+	unsigned long scan_mask;
+
+	if (!indio_dev->active_scan_mask)
+		return 0;
+
+	scan_mask = *indio_dev->active_scan_mask;
+	if ((scan_mask & ~CROS_EC_LIGHT_CLEAR_OR_PROXIMITY_MASK) == 0)
+		/*
+		 * Only one channel at most is used.
+		 * Use regular push function.
+		 */
+		return cros_ec_sensors_push_data(indio_dev, data, timestamp);
+
+	if (scan_mask & CROS_EC_LIGHT_CLEAR_OR_PROXIMITY_MASK)
+		/*
+		 * Save clear channel, will be used when RGB data arrives.
+		 */
+		st->samples[0] = data[0];
+
+	return 0;
+}
+
+static int cros_ec_light_push_data_rgb(struct iio_dev *indio_dev,
+				       s16 *data,
+				       s64 timestamp)
+{
+	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
+	s16 *out;
+	unsigned long scan_mask;
+	unsigned int i;
+
+	if (!indio_dev->active_scan_mask)
+		return 0;
+
+	scan_mask = *indio_dev->active_scan_mask;
+	/*
+	 * Send all data needed.
+	 */
+	out = (s16 *)st->samples;
+	for_each_set_bit(i,
+			 indio_dev->active_scan_mask,
+			 indio_dev->masklength) {
+		if (i > 0)
+			*out = data[CROS_EC_LIGHT_IDX_TO_CHAN(i)];
+		out++;
+	}
+	iio_push_to_buffers_with_timestamp(indio_dev, st->samples, timestamp);
+	return 0;
+}
+
+static irqreturn_t cros_ec_light_capture(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
+	int ret, i, idx = 0;
+	s16 data;
+	const unsigned long scan_mask = *indio_dev->active_scan_mask;
+
+	mutex_lock(&st->cmd_lock);
+
+	/* Clear capture data. */
+	memset(st->samples, 0, indio_dev->scan_bytes);
+
+	/* Read first channel. */
+	ret = cros_ec_sensors_read_cmd(indio_dev, 1, &data);
+	if (ret < 0) {
+		mutex_unlock(&st->cmd_lock);
+		goto done;
+	}
+	if (scan_mask & CROS_EC_LIGHT_CLEAR_OR_PROXIMITY_MASK)
+		((s16 *)st->samples)[idx++] = data;
+
+	/* Read remaining channels. */
+	if ((scan_mask & CROS_EC_LIGHT_XYZ_SPACE_MASK) ||
+	    (scan_mask & CROS_EC_LIGHT_RGB_SPACE_MASK)) {
+		ret = cros_ec_light_extra_send_host_cmd(
+				st, 1, sizeof(st->resp->data));
+		if (ret < 0) {
+			mutex_unlock(&st->cmd_lock);
+			goto done;
+		}
+		for (i = 0; i < CROS_EC_SENSOR_MAX_AXIS; i++)
+			((s16 *)st->samples)[idx++] = st->resp->data.data[i];
+	}
+	mutex_unlock(&st->cmd_lock);
+
+	iio_push_to_buffers_with_timestamp(indio_dev, st->samples,
+					   iio_get_time_ns(indio_dev));
+
+done:
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+static int cros_ec_light_prox_update_scan_mode(struct iio_dev *indio_dev,
+					       const unsigned long *scan_mask)
+{
+	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
+	int ret;
+	bool enable_raw_mode;
+
+	if (*scan_mask & CROS_EC_LIGHT_XYZ_SPACE_MASK)
+		enable_raw_mode = false;
+	else if (*scan_mask & CROS_EC_LIGHT_RGB_SPACE_MASK)
+		enable_raw_mode = true;
+	else
+		/* Just clear channel or proxmity, nothing to do. */
+		return 0;
+
+	mutex_lock(&st->cmd_lock);
+	st->param.cmd = MOTIONSENSE_CMD_PERFORM_CALIB;
+	st->param.perform_calib.enable = enable_raw_mode;
+	ret = cros_ec_motion_send_host_cmd(st, 0);
+	mutex_unlock(&st->cmd_lock);
+
+	return ret;
+}
+
 static const struct iio_info cros_ec_light_prox_info = {
 	.read_raw = &cros_ec_light_prox_read,
 	.write_raw = &cros_ec_light_prox_write,
 	.read_avail = &cros_ec_sensors_core_read_avail,
+	.update_scan_mode = &cros_ec_light_prox_update_scan_mode,
 };
 
 static int cros_ec_light_prox_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
+	struct cros_ec_sensorhub *sensor_hub = dev_get_drvdata(dev->parent);
 	struct iio_dev *indio_dev;
 	struct cros_ec_light_prox_state *state;
 	struct iio_chan_spec *channel;
-	int ret;
+	int ret, i, num_channels = CROS_EC_LIGHT_PROX_MIN_CHANNELS;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*state));
 	if (!indio_dev)
 		return -ENOMEM;
 
 	ret = cros_ec_sensors_core_init(pdev, indio_dev, true,
-					cros_ec_sensors_capture,
-					cros_ec_sensors_push_data);
+					cros_ec_light_capture,
+					cros_ec_light_push_data);
 	if (ret)
 		return ret;
 
@@ -189,28 +478,40 @@ static int cros_ec_light_prox_probe(struct platform_device *pdev)
 	state = iio_priv(indio_dev);
 	state->core.type = state->core.resp->info.type;
 	state->core.loc = state->core.resp->info.location;
-	channel = state->channels;
 
-	/* Common part */
-	channel->info_mask_shared_by_all =
-		BIT(IIO_CHAN_INFO_SAMP_FREQ);
-	channel->info_mask_shared_by_all_available =
-		BIT(IIO_CHAN_INFO_SAMP_FREQ);
-	channel->scan_type.realbits = CROS_EC_SENSOR_BITS;
-	channel->scan_type.storagebits = CROS_EC_SENSOR_BITS;
-	channel->scan_type.shift = 0;
-	channel->scan_index = 0;
-	channel->ext_info = cros_ec_sensors_ext_info;
-	channel->scan_type.sign = 'u';
+	/* Check if we need more sensors for RGB (or XYZ). */
+	state->core.param.cmd = MOTIONSENSE_CMD_INFO;
+	if (cros_ec_light_extra_send_host_cmd(&state->core, 1, 0) == 0 &&
+	    state->core.resp->info.type == MOTIONSENSE_TYPE_LIGHT_RGB)
+		num_channels += 2 * CROS_EC_SENSOR_MAX_AXIS;
+
+	channel = devm_kcalloc(dev, num_channels, sizeof(*channel), 0);
+	if (!channel)
+		return -ENOMEM;
+
+	indio_dev->channels = channel;
+	indio_dev->num_channels = num_channels;
+	indio_dev->available_scan_masks = cros_ec_light_prox_bitmasks;
 
+	cros_ec_light_channel_common(channel);
 	/* Sensor specific */
 	switch (state->core.type) {
 	case MOTIONSENSE_TYPE_LIGHT:
 		channel->type = IIO_LIGHT;
+		if (num_channels > CROS_EC_LIGHT_PROX_MIN_CHANNELS) {
+			/*
+			 * To set a global scale, as CALIB_SCALE for RGB sensor
+			 * is limited between 0 and 2.
+			 */
+			channel->info_mask_shared_by_all |=
+				BIT(IIO_CHAN_INFO_SCALE);
+		}
 		channel->info_mask_separate =
-			BIT(IIO_CHAN_INFO_PROCESSED) |
+			BIT(IIO_CHAN_INFO_RAW) |
 			BIT(IIO_CHAN_INFO_CALIBBIAS) |
 			BIT(IIO_CHAN_INFO_CALIBSCALE);
+		channel->modified = 1;
+		channel->channel2 = IIO_MOD_LIGHT_CLEAR;
 		break;
 	case MOTIONSENSE_TYPE_PROX:
 		channel->type = IIO_PROXIMITY;
@@ -223,20 +524,48 @@ static int cros_ec_light_prox_probe(struct platform_device *pdev)
 		dev_warn(dev, "Unknown motion sensor\n");
 		return -EINVAL;
 	}
+	channel++;
+
+	if (num_channels > CROS_EC_LIGHT_PROX_MIN_CHANNELS) {
+		u8 sensor_num = state->core.param.info.sensor_num;
+		int idx;
+
+		for (i = CROS_EC_SENSOR_X, idx = 1; i < CROS_EC_SENSOR_MAX_AXIS;
+				i++, channel++, idx++) {
+			cros_ec_light_channel_common(channel);
+			channel->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
+			channel->scan_index = idx;
+			channel->modified = 1;
+			channel->channel2 = IIO_MOD_X + i;
+			channel->type = IIO_LIGHT;
+		}
+		for (i = CROS_EC_SENSOR_X; i < CROS_EC_SENSOR_MAX_AXIS;
+				i++, channel++, idx++) {
+			cros_ec_light_channel_common(channel);
+			channel->info_mask_separate =
+				BIT(IIO_CHAN_INFO_RAW) |
+				BIT(IIO_CHAN_INFO_CALIBBIAS) |
+				BIT(IIO_CHAN_INFO_CALIBSCALE);
+			channel->scan_index = idx;
+			channel->modified = 1;
+			channel->channel2 = IIO_MOD_LIGHT_RED + i;
+			channel->type = IIO_LIGHT;
+		}
+		cros_ec_sensorhub_register_push_data(
+				sensor_hub,
+				sensor_num + 1,
+				indio_dev,
+				cros_ec_light_push_data_rgb);
+	}
 
 	/* Timestamp */
-	channel++;
 	channel->type = IIO_TIMESTAMP;
 	channel->channel = -1;
-	channel->scan_index = 1;
+	channel->scan_index = num_channels - 1;
 	channel->scan_type.sign = 's';
 	channel->scan_type.realbits = 64;
 	channel->scan_type.storagebits = 64;
 
-	indio_dev->channels = state->channels;
-
-	indio_dev->num_channels = CROS_EC_LIGHT_PROX_MAX_CHANNELS;
-
 	state->core.read_ec_sensors_data = cros_ec_sensors_read_cmd;
 
 	return devm_iio_device_register(dev, indio_dev);
diff --git a/drivers/platform/chrome/cros_ec_sensorhub.c b/drivers/platform/chrome/cros_ec_sensorhub.c
index b7f2c00db5e1e..f85191ab2ee34 100644
--- a/drivers/platform/chrome/cros_ec_sensorhub.c
+++ b/drivers/platform/chrome/cros_ec_sensorhub.c
@@ -103,6 +103,9 @@ static int cros_ec_sensorhub_register(struct device *dev,
 		case MOTIONSENSE_TYPE_LIGHT:
 			name = "cros-ec-light";
 			break;
+		case MOTIONSENSE_TYPE_LIGHT_RGB:
+			/* Processed with cros-ec-light. */
+			continue;
 		case MOTIONSENSE_TYPE_ACTIVITY:
 			name = "cros-ec-activity";
 			break;
diff --git a/include/linux/iio/common/cros_ec_sensors_core.h b/include/linux/iio/common/cros_ec_sensors_core.h
index 7bc961defa87e..c31766c64bf94 100644
--- a/include/linux/iio/common/cros_ec_sensors_core.h
+++ b/include/linux/iio/common/cros_ec_sensors_core.h
@@ -26,7 +26,6 @@ enum {
 
 /*
  * 4 16 bit channels are allowed.
- * Good enough for current sensors, they use up to 3 16 bit vectors.
  */
 #define CROS_EC_SAMPLE_SIZE  (sizeof(s64) * 2)
 
diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h
index 395c9b2b05c66..e8b51e112c191 100644
--- a/include/linux/platform_data/cros_ec_commands.h
+++ b/include/linux/platform_data/cros_ec_commands.h
@@ -2342,6 +2342,7 @@ enum motionsensor_type {
 	MOTIONSENSE_TYPE_ACTIVITY = 5,
 	MOTIONSENSE_TYPE_BARO = 6,
 	MOTIONSENSE_TYPE_SYNC = 7,
+	MOTIONSENSE_TYPE_LIGHT_RGB = 8,
 	MOTIONSENSE_TYPE_MAX,
 };
 
@@ -2375,6 +2376,7 @@ enum motionsensor_chip {
 	MOTIONSENSE_CHIP_LSM6DS3 = 17,
 	MOTIONSENSE_CHIP_LSM6DSO = 18,
 	MOTIONSENSE_CHIP_LNG2DM = 19,
+	MOTIONSENSE_CHIP_TCS3400 = 20,
 	MOTIONSENSE_CHIP_MAX,
 };
 
-- 
2.26.2.526.g744177e7f7-goog


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 0/3] iio: cros_ec: Add support for RGB light sensor
  2020-05-06 23:03 [PATCH v3 0/3] iio: cros_ec: Add support for RGB light sensor Gwendal Grignou
                   ` (2 preceding siblings ...)
  2020-05-06 23:03 ` [PATCH v2 3/3] iio: cros_ec_light: Add support for RGB sensor Gwendal Grignou
@ 2020-05-08 14:56 ` Jonathan Cameron
  3 siblings, 0 replies; 10+ messages in thread
From: Jonathan Cameron @ 2020-05-08 14:56 UTC (permalink / raw)
  To: Gwendal Grignou
  Cc: enric.balletbo, jic23, bleung, groeck, linux-kernel, linux-iio

On Wed, 6 May 2020 16:03:21 -0700
Gwendal Grignou <gwendal@chromium.org> wrote:

> Add support for color light sensor presented by the Chromebook Embedded
> Controller (EC).
> Instead of just presenting lux measurement (clear channel), a color light
> sensor is able to report color temperature measurement.
> 
> The EC, using factory settings, can transform the raw measurement into
> the CIE 1931 XYZ color space (XYZ) and take adavantage of color sensor
> autocalibration to provide the most accurate measurements.

v3 of series with v2 patches?

Also my earlier comment about colour channels cannot be illuminance
still stands. It is a term that "only" applies to light measurements with
a particular frequency / sensitivity curve.

The colour channels should all be in_intensity_xxx_raw.

If you want to do the computation in driver to derive the illuminance
that would be great, otherwise we shouldn't have any illuminance channels.

Jonathan


> 
> Gwendal Grignou (3):
>   iio: Add in_illumincance vectors in different color spaces
>   iio: cros_ec: Allow enabling/disabling calibration mode
>   iio: cros_ec_light: Add support for RGB sensor
> 
>  Documentation/ABI/testing/sysfs-bus-iio       |  27 +
>  .../cros_ec_sensors/cros_ec_sensors_core.c    |   3 +-
>  drivers/iio/light/cros_ec_light_prox.c        | 469 +++++++++++++++---
>  drivers/platform/chrome/cros_ec_sensorhub.c   |   3 +
>  .../linux/iio/common/cros_ec_sensors_core.h   |   1 -
>  .../linux/platform_data/cros_ec_commands.h    |  14 +-
>  6 files changed, 441 insertions(+), 76 deletions(-)
> 



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/3] iio: Add in_illumincance vectors in different color spaces
  2020-05-06 23:03 ` [PATCH v2 1/3] iio: Add in_illumincance vectors in different color spaces Gwendal Grignou
@ 2020-05-08 15:16   ` Jonathan Cameron
  2020-05-12  4:10     ` Gwendal Grignou
  0 siblings, 1 reply; 10+ messages in thread
From: Jonathan Cameron @ 2020-05-08 15:16 UTC (permalink / raw)
  To: Gwendal Grignou
  Cc: enric.balletbo, jic23, bleung, groeck, linux-kernel, linux-iio

On Wed, 6 May 2020 16:03:22 -0700
Gwendal Grignou <gwendal@chromium.org> wrote:

Illuminance in the title.  Plus I'm still arguing these
aren't illuminance values.  

The Y value is illuminance but X and Z definitely aren't.
RGB needs to stick to intensity - like the other existing
RGB sensors.

Gah.  XYZ and IIO is a mess. 

I suppose we could introduce a new type and have
in_illumiance_raw
in_chromacity_x_raw
in_chromacity_z_raw but chances of anyone understanding what we
are on about without reading wikipedia is low...

Sigh.  Unless someone else chips in, I'm inclined to be lazy and rely
on documentation to let in_illuminance_x,y,z be defined as being
cie xyz color space measurements.

It seems slighlty preferable to defining another type for these,
though I suspect I'll regret this comment when some adds
cie lab which was always my favourite colour space :)



> Define 2 spaces for defining color coming from color sensors:
> RGB and XYZ: Both are in lux.
> RGB is the raw output from sensors (Red, Green and Blue channels), in
> addition to the existing clear channel (C).

> The RGBC vector goes through a matrix transformation to produce the XYZ
> vector. Y is illumincance, and XY caries the chromaticity information.
> The matrix is model specific, as the color sensor can be behing a glass
> that can filter some wavelengths.
> 
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> ---
> New in v2.
> 
>  Documentation/ABI/testing/sysfs-bus-iio | 27 +++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index d3e53a6d8331b..256db6e63a25e 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -1309,6 +1309,33 @@ Description:
>  		Illuminance measurement, units after application of scale
>  		and offset are lux.
>  
> +What:		/sys/.../iio:deviceX/in_illuminance_red_raw
> +What:		/sys/.../iio:deviceX/in_illuminance_green_raw
> +What:		/sys/.../iio:deviceX/in_illuminance_blue_raw
> +KernelVersion:	5.7
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Illuminance measuremed in red, green or blue channels, units
> +		after application of scale and offset are lux.

No they aren't.  Units are some magic intensity at some magic wavelength.

> +
> +What:		/sys/.../iio:deviceX/in_illuminance_x_raw
> +What:		/sys/.../iio:deviceX/in_illuminance_y_raw
> +What:		/sys/.../iio:deviceX/in_illuminance_z_raw
> +KernelVersion:	5.7
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		lluminance measured in the CIE 1931 color space (XYZ).
> +		in_illuminance_y_raw is a measure of the brightness, and is
> +		identical in_illuminance_raw.

That is fair enough.

> +		in_illuminance_x_raw and in_illuminance_z_raw carry chromacity
> +		information.
> +		in_illuminance_x,y,z_raw are be obtained from the sensor color
> +		channels using color matching functions that may be device
> +		specific.
> +		Units after application of scale and offset are lux.

True for Y, not for X and Z which don't have 'units' as such.

> +		The measurments can be used to represent colors in the CIE
> +		xyY color space

XYZ

> +
>  What:		/sys/.../iio:deviceX/in_intensityY_raw
>  What:		/sys/.../iio:deviceX/in_intensityY_ir_raw
>  What:		/sys/.../iio:deviceX/in_intensityY_both_raw



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 3/3] iio: cros_ec_light: Add support for RGB sensor
  2020-05-06 23:03 ` [PATCH v2 3/3] iio: cros_ec_light: Add support for RGB sensor Gwendal Grignou
@ 2020-05-08 15:23   ` Jonathan Cameron
  2020-05-11  6:31   ` kbuild test robot
  1 sibling, 0 replies; 10+ messages in thread
From: Jonathan Cameron @ 2020-05-08 15:23 UTC (permalink / raw)
  To: Gwendal Grignou
  Cc: enric.balletbo, jic23, bleung, groeck, linux-kernel, linux-iio

On Wed, 6 May 2020 16:03:24 -0700
Gwendal Grignou <gwendal@chromium.org> wrote:

> Add support for color sensors behind EC like TCS3400.
> The color data can be presented in Red Green Blue color space (RGB) or
> the CIE 1931 XYZ color space (XYZ).
> In XYZ mode, the sensor is configured for auto calibrating its channels
> and is the "normal" mode.
> The driver tells the EC to switch between the 2 modes by using the
> calibration command.
> When the sensor is in calibration mode, only clear and RGB channels are
> available. In normal mode, only clear and XYZ are.
> When RGB channels are enabled, the sensor switches to calibration mode
> when the buffer is enabled.
> 
> When reading trhough sysfs command, set calibration mode and then read
> the channel(s). A command will be issue for each read, so the channels
> may come from different sensor sample.
> When using the buffer, after setting the mask, when the buffer is
> enabled, the calibration will be set based on the channel mask.
> 
> libiio tools can be used to gather sensor information:
> iio_readdev -s 10 cros-ec-light \
> illuminance_clear illuminance_x illuminance_y illuminance_z
> 
> To match IIO ABI, the clear illuminance channel has been renamed
> in_illuminance_clear_raw from in_illuminance_input.

This seems like a misunderstanding...
If that channel is measuring normal illuminance value via an
appropriate sensor with filters to approximate the human eye
(which is what good ambient light sensors do) then it
should stay as in_illuminance_input / raw (depending only on whether
we have a scale to apply in userspace).

If it is a random clear signal as you tend to get on rgbc sensors
which is not intended to be used on its own and hence does not have
appropriate filters, then it is not an illuminance sensor but
is just measuring some sort of 'intensity'.

From the earliest days of IIO we've dealt with those by using
in_intensity_clear_raw

That has no defined units because they are meaningless without
a very careful definition of 'clear'.

Earlier in the series I've suggested we 'might' abuse
the illuminance measure for XYZ as it's well defined. RGBC are
not so should stay as intensity channels.

> 
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> ---
>  drivers/iio/light/cros_ec_light_prox.c        | 469 +++++++++++++++---
>  drivers/platform/chrome/cros_ec_sensorhub.c   |   3 +
>  .../linux/iio/common/cros_ec_sensors_core.h   |   1 -
>  .../linux/platform_data/cros_ec_commands.h    |   2 +
>  4 files changed, 404 insertions(+), 71 deletions(-)
> 
> diff --git a/drivers/iio/light/cros_ec_light_prox.c b/drivers/iio/light/cros_ec_light_prox.c
> index 2198b50909ed0..83bd3057b334c 100644
> --- a/drivers/iio/light/cros_ec_light_prox.c
> +++ b/drivers/iio/light/cros_ec_light_prox.c
> @@ -17,82 +17,188 @@
>  #include <linux/module.h>
>  #include <linux/platform_data/cros_ec_commands.h>
>  #include <linux/platform_data/cros_ec_proto.h>
> +#include <linux/platform_data/cros_ec_sensorhub.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  
>  /*
> - * We only represent one entry for light or proximity. EC is merging different
> - * light sensors to return the what the eye would see. For proximity, we
> - * currently support only one light source.
> + * We may present up to 7 channels:
> + *
> + * +-----+-----+-----+-----+-----+-----+-----+
> + * |Clear|  X  |  Y  |  Z  | RED |BLUE |GREEN|
> + * |Prox |     |     |     |     |     |     |
> + * +-----+-----+-----+-----+-----+-----+-----+
> + *
> + * Prox[imity] is presented by proximity sensors.
> + * The clear channel is supported by single and color light sensors.
> + * Color light sensor either reports color information in the RGB space or
> + * the CIE 1931 XYZ (XYZ) color space.
>   */
> -#define CROS_EC_LIGHT_PROX_MAX_CHANNELS (1 + 1)
> +#define CROS_EC_LIGHT_CLEAR_OR_PROXIMITY_MASK GENMASK(0, 0)
> +#define CROS_EC_LIGHT_XYZ_SPACE_MASK GENMASK(3, 1)
> +#define CROS_EC_LIGHT_RGB_SPACE_MASK (6, 4)
> +
> +/*
> + * We always represent one entry for light or proximity, and all
> + * samples can be timestamped.
> + */
> +#define CROS_EC_LIGHT_PROX_MIN_CHANNELS (1 + 1)
> +
> +static const unsigned long cros_ec_light_prox_bitmasks[] = {
> +	CROS_EC_LIGHT_CLEAR_OR_PROXIMITY_MASK,
> +	CROS_EC_LIGHT_XYZ_SPACE_MASK,
> +	CROS_EC_LIGHT_XYZ_SPACE_MASK | CROS_EC_LIGHT_CLEAR_OR_PROXIMITY_MASK,
> +	CROS_EC_LIGHT_RGB_SPACE_MASK,
> +	CROS_EC_LIGHT_RGB_SPACE_MASK | CROS_EC_LIGHT_CLEAR_OR_PROXIMITY_MASK,
> +	0,
> +};
> +
> +#define CROS_EC_LIGHT_IDX_TO_CHAN(_idx) (((_idx) - 1) % CROS_EC_SENSOR_MAX_AXIS)
>  
>  /* State data for ec_sensors iio driver. */
>  struct cros_ec_light_prox_state {
>  	/* Shared by all sensors */
>  	struct cros_ec_sensors_core_state core;
>  
> -	struct iio_chan_spec channels[CROS_EC_LIGHT_PROX_MAX_CHANNELS];
> +	/* Calibration information for the color channels. */
> +	struct calib_data rgb_calib[CROS_EC_SENSOR_MAX_AXIS];
>  };
>  
> +static void cros_ec_light_channel_common(struct iio_chan_spec *channel)
> +{
> +	channel->info_mask_shared_by_all =
> +		BIT(IIO_CHAN_INFO_SAMP_FREQ);
> +	channel->info_mask_shared_by_all_available =
> +		BIT(IIO_CHAN_INFO_SAMP_FREQ);
> +	channel->scan_type.realbits = CROS_EC_SENSOR_BITS;
> +	channel->scan_type.storagebits = CROS_EC_SENSOR_BITS;
> +	channel->ext_info = cros_ec_sensors_ext_info;
> +	channel->scan_type.sign = 'u';
> +}
> +
> +static int
> +cros_ec_light_extra_send_host_cmd(struct cros_ec_sensors_core_state *state,
> +				  int increment, u16 opt_length)
> +{
> +	u8 save_sensor_num = state->param.info.sensor_num;
> +	int ret;
> +
> +	state->param.info.sensor_num += increment;
> +	ret = cros_ec_motion_send_host_cmd(state, opt_length);
> +	state->param.info.sensor_num = save_sensor_num;
> +	return ret;
> +}
> +
> +static int cros_ec_light_prox_read_data(struct iio_dev *indio_dev,
> +					struct iio_chan_spec const *chan,
> +					int *val)
> +{
> +	struct cros_ec_light_prox_state *st = iio_priv(indio_dev);
> +	int ret;
> +	int idx = chan->scan_index;
> +	s16 data;
> +
> +	/*
> +	 * The data coming from the light sensor is
> +	 * pre-processed and represents the ambient light
> +	 * illuminance reading expressed in lux.
> +	 */
> +	if (idx == 0) {
> +		ret = cros_ec_sensors_read_cmd(indio_dev, 1, &data);
> +		if (ret < 0)
> +			return ret;
> +		*val = data;
> +	} else {
> +		ret = cros_ec_light_extra_send_host_cmd(
> +				&st->core, 1, sizeof(st->core.resp->data));
> +		if (ret)
> +			return ret;
> +		*val = st->core.resp->data.data[CROS_EC_LIGHT_IDX_TO_CHAN(idx)];
> +	}
> +	return IIO_VAL_INT;
> +}
> +
> +static int cros_ec_light_read_color_scale(struct cros_ec_light_prox_state *st,
> +					  int idx, int *val, int *val2)
> +{
> +	int ret, i;
> +	u16 scale;
> +
> +	st->core.param.cmd = MOTIONSENSE_CMD_SENSOR_SCALE;
> +	st->core.param.sensor_scale.flags = 0;
> +	if (idx == 0)
> +		ret = cros_ec_motion_send_host_cmd(&st->core, 0);
> +	else
> +		ret = cros_ec_light_extra_send_host_cmd(&st->core, 1, 0);
> +	if (ret)
> +		return ret;
> +
> +	if (idx == 0) {
> +		scale = st->core.resp->sensor_scale.scale[0];
> +		st->core.calib[0].scale = scale;
> +	} else {
> +		for (i = CROS_EC_SENSOR_X; i < CROS_EC_SENSOR_MAX_AXIS; i++)
> +			st->rgb_calib[i].scale =
> +				st->core.resp->sensor_scale.scale[i];
> +		scale = st->rgb_calib[CROS_EC_LIGHT_IDX_TO_CHAN(idx)].scale;
> +	}
> +	/*
> +	 * scale is a number x.y, where x is coded on 1 bit,
> +	 * y coded on 15 bits, between 0 and 9999.
> +	 */
> +	*val = scale >> 15;
> +	*val2 = ((scale & 0x7FFF) * 1000000LL) /
> +		MOTION_SENSE_DEFAULT_SCALE;
> +	return IIO_VAL_INT_PLUS_MICRO;
> +}
> +
>  static int cros_ec_light_prox_read(struct iio_dev *indio_dev,
>  				   struct iio_chan_spec const *chan,
>  				   int *val, int *val2, long mask)
>  {
>  	struct cros_ec_light_prox_state *st = iio_priv(indio_dev);
> -	u16 data = 0;
> -	s64 val64;
> -	int ret;
> +	int i, ret;
>  	int idx = chan->scan_index;
> +	s64 val64;
>  
>  	mutex_lock(&st->core.cmd_lock);
> -
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> -		if (chan->type == IIO_PROXIMITY) {
> -			ret = cros_ec_sensors_read_cmd(indio_dev, 1 << idx,
> -						     (s16 *)&data);
> -			if (ret)
> -				break;
> -			*val = data;
> -			ret = IIO_VAL_INT;
> -		} else {
> -			ret = -EINVAL;
> -		}
> -		break;
> -	case IIO_CHAN_INFO_PROCESSED:
> -		if (chan->type == IIO_LIGHT) {
> -			ret = cros_ec_sensors_read_cmd(indio_dev, 1 << idx,
> -						     (s16 *)&data);
> -			if (ret)
> -				break;
> -			/*
> -			 * The data coming from the light sensor is
> -			 * pre-processed and represents the ambient light
> -			 * illuminance reading expressed in lux.
> -			 */
> -			*val = data;
> -			ret = IIO_VAL_INT;
> -		} else {
> -			ret = -EINVAL;
> -		}
> +		ret = cros_ec_light_prox_read_data(indio_dev, chan, val);
>  		break;
>  	case IIO_CHAN_INFO_CALIBBIAS:
>  		st->core.param.cmd = MOTIONSENSE_CMD_SENSOR_OFFSET;
>  		st->core.param.sensor_offset.flags = 0;
>  
> -		ret = cros_ec_motion_send_host_cmd(&st->core, 0);
> +		if (idx == 0)
> +			ret = cros_ec_motion_send_host_cmd(&st->core, 0);
> +		else
> +			ret = cros_ec_light_extra_send_host_cmd(
> +					&st->core, 1, 0);
>  		if (ret)
>  			break;
> -
> -		/* Save values */
> -		st->core.calib[0].offset =
> -			st->core.resp->sensor_offset.offset[0];
> -
> -		*val = st->core.calib[idx].offset;
> +		if (idx == 0) {
> +			*val = st->core.calib[0].offset =
> +				st->core.resp->sensor_offset.offset[0];
> +		} else {
> +			for (i = CROS_EC_SENSOR_X; i < CROS_EC_SENSOR_MAX_AXIS;
> +			     i++)
> +				st->rgb_calib[i].offset =
> +					st->core.resp->sensor_offset.offset[i];
> +			i = CROS_EC_LIGHT_IDX_TO_CHAN(idx);
> +			*val = st->rgb_calib[i].offset;
> +		}
>  		ret = IIO_VAL_INT;
>  		break;
>  	case IIO_CHAN_INFO_CALIBSCALE:
> +		if (indio_dev->num_channels > CROS_EC_LIGHT_PROX_MIN_CHANNELS) {
> +			ret = cros_ec_light_read_color_scale(st, idx, val,
> +							     val2);
> +			break;
> +		}
> +		/* RANGE is used for calibration in 1 channel sensors. */
> +		fallthrough;
> +	case IIO_CHAN_INFO_SCALE:
>  		/*
>  		 * RANGE is used for calibration
>  		 * scale is a number x.y, where x is coded on 16 bits,
> @@ -121,29 +227,85 @@ static int cros_ec_light_prox_read(struct iio_dev *indio_dev,
>  	return ret;
>  }
>  
> +static int cros_ec_light_write_color_scale(struct cros_ec_light_prox_state *st,
> +					   int idx, int val, int val2)
> +{
> +	int i;
> +	u16 scale;
> +
> +	if (val >= 2) {
> +		/*
> +		 * The user space is sending values already
> +		 * multiplied by MOTION_SENSE_DEFAULT_SCALE.
> +		 */
> +		scale = val;
> +	} else {
> +		u64 val64 = val2 * MOTION_SENSE_DEFAULT_SCALE;
> +
> +		do_div(val64, 1000000);
> +		scale = (val << 15) | val64;
> +	}
> +
> +	st->core.param.cmd = MOTIONSENSE_CMD_SENSOR_SCALE;
> +	st->core.param.sensor_offset.flags = MOTION_SENSE_SET_OFFSET;
> +	st->core.param.sensor_offset.temp = EC_MOTION_SENSE_INVALID_CALIB_TEMP;
> +	if (idx == 0) {
> +		st->core.calib[0].scale = scale;
> +		st->core.param.sensor_scale.scale[0] = scale;
> +		return cros_ec_motion_send_host_cmd(&st->core, 0);
> +	}
> +
> +	st->rgb_calib[CROS_EC_LIGHT_IDX_TO_CHAN(idx)].scale = scale;
> +	for (i = CROS_EC_SENSOR_X; i < CROS_EC_SENSOR_MAX_AXIS; i++)
> +		st->core.param.sensor_scale.scale[i] = st->rgb_calib[i].scale;
> +	return cros_ec_light_extra_send_host_cmd(&st->core, 1, 0);
> +}
> +
>  static int cros_ec_light_prox_write(struct iio_dev *indio_dev,
>  			       struct iio_chan_spec const *chan,
>  			       int val, int val2, long mask)
>  {
>  	struct cros_ec_light_prox_state *st = iio_priv(indio_dev);
> -	int ret;
> +	int ret, i;
>  	int idx = chan->scan_index;
>  
>  	mutex_lock(&st->core.cmd_lock);
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_CALIBBIAS:
> -		st->core.calib[idx].offset = val;
>  		/* Send to EC for each axis, even if not complete */
>  		st->core.param.cmd = MOTIONSENSE_CMD_SENSOR_OFFSET;
>  		st->core.param.sensor_offset.flags = MOTION_SENSE_SET_OFFSET;
> -		st->core.param.sensor_offset.offset[0] =
> -			st->core.calib[0].offset;
>  		st->core.param.sensor_offset.temp =
>  					EC_MOTION_SENSE_INVALID_CALIB_TEMP;
> -		ret = cros_ec_motion_send_host_cmd(&st->core, 0);
> +		if (idx == 0) {
> +			st->core.calib[0].offset = val;
> +			st->core.param.sensor_offset.offset[0] = val;
> +			ret = cros_ec_motion_send_host_cmd(&st->core, 0);
> +		} else {
> +			i = CROS_EC_LIGHT_IDX_TO_CHAN(idx);
> +			st->rgb_calib[i].offset = val;
> +			for (i = CROS_EC_SENSOR_X;
> +			     i < CROS_EC_SENSOR_MAX_AXIS;
> +			     i++)
> +				st->core.param.sensor_offset.offset[i] =
> +					st->rgb_calib[i].offset;
> +			ret = cros_ec_light_extra_send_host_cmd(
> +					&st->core, 1, 0);
> +		}
>  		break;
>  	case IIO_CHAN_INFO_CALIBSCALE:
> +		if (indio_dev->num_channels > CROS_EC_LIGHT_PROX_MIN_CHANNELS) {
> +			ret = cros_ec_light_write_color_scale(st, idx,
> +							      val, val2);
> +			break;
> +		}
> +		/*
> +		 * For sensors with only one channel, _RANGE is used
> +		 * instead of _SCALE.
> +		 */
> +		fallthrough;
> +	case IIO_CHAN_INFO_SCALE:
>  		st->core.param.cmd = MOTIONSENSE_CMD_SENSOR_RANGE;
>  		st->core.param.sensor_range.data = (val << 16) | (val2 / 100);
>  		ret = cros_ec_motion_send_host_cmd(&st->core, 0);
> @@ -159,27 +321,154 @@ static int cros_ec_light_prox_write(struct iio_dev *indio_dev,
>  	return ret;
>  }
>  
> +static int cros_ec_light_push_data(struct iio_dev *indio_dev,
> +				   s16 *data,
> +				   s64 timestamp)
> +{
> +	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
> +	unsigned long scan_mask;
> +
> +	if (!indio_dev->active_scan_mask)
> +		return 0;
> +
> +	scan_mask = *indio_dev->active_scan_mask;
> +	if ((scan_mask & ~CROS_EC_LIGHT_CLEAR_OR_PROXIMITY_MASK) == 0)
> +		/*
> +		 * Only one channel at most is used.
> +		 * Use regular push function.
> +		 */
> +		return cros_ec_sensors_push_data(indio_dev, data, timestamp);
> +
> +	if (scan_mask & CROS_EC_LIGHT_CLEAR_OR_PROXIMITY_MASK)
> +		/*
> +		 * Save clear channel, will be used when RGB data arrives.
> +		 */
> +		st->samples[0] = data[0];
> +
> +	return 0;
> +}
> +
> +static int cros_ec_light_push_data_rgb(struct iio_dev *indio_dev,
> +				       s16 *data,
> +				       s64 timestamp)
> +{
> +	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
> +	s16 *out;
> +	unsigned long scan_mask;
> +	unsigned int i;
> +
> +	if (!indio_dev->active_scan_mask)
> +		return 0;
> +
> +	scan_mask = *indio_dev->active_scan_mask;
> +	/*
> +	 * Send all data needed.
> +	 */
> +	out = (s16 *)st->samples;
> +	for_each_set_bit(i,
> +			 indio_dev->active_scan_mask,
> +			 indio_dev->masklength) {
> +		if (i > 0)
> +			*out = data[CROS_EC_LIGHT_IDX_TO_CHAN(i)];
> +		out++;
> +	}
> +	iio_push_to_buffers_with_timestamp(indio_dev, st->samples, timestamp);
> +	return 0;
> +}
> +
> +static irqreturn_t cros_ec_light_capture(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
> +	int ret, i, idx = 0;
> +	s16 data;
> +	const unsigned long scan_mask = *indio_dev->active_scan_mask;
> +
> +	mutex_lock(&st->cmd_lock);
> +
> +	/* Clear capture data. */
> +	memset(st->samples, 0, indio_dev->scan_bytes);
> +
> +	/* Read first channel. */
> +	ret = cros_ec_sensors_read_cmd(indio_dev, 1, &data);
> +	if (ret < 0) {
> +		mutex_unlock(&st->cmd_lock);
> +		goto done;
> +	}
> +	if (scan_mask & CROS_EC_LIGHT_CLEAR_OR_PROXIMITY_MASK)
> +		((s16 *)st->samples)[idx++] = data;
> +
> +	/* Read remaining channels. */
> +	if ((scan_mask & CROS_EC_LIGHT_XYZ_SPACE_MASK) ||
> +	    (scan_mask & CROS_EC_LIGHT_RGB_SPACE_MASK)) {
> +		ret = cros_ec_light_extra_send_host_cmd(
> +				st, 1, sizeof(st->resp->data));
> +		if (ret < 0) {
> +			mutex_unlock(&st->cmd_lock);
> +			goto done;
> +		}
> +		for (i = 0; i < CROS_EC_SENSOR_MAX_AXIS; i++)
> +			((s16 *)st->samples)[idx++] = st->resp->data.data[i];
> +	}
> +	mutex_unlock(&st->cmd_lock);
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, st->samples,
> +					   iio_get_time_ns(indio_dev));
> +
> +done:
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int cros_ec_light_prox_update_scan_mode(struct iio_dev *indio_dev,
> +					       const unsigned long *scan_mask)
> +{
> +	struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
> +	int ret;
> +	bool enable_raw_mode;
> +
> +	if (*scan_mask & CROS_EC_LIGHT_XYZ_SPACE_MASK)
> +		enable_raw_mode = false;
> +	else if (*scan_mask & CROS_EC_LIGHT_RGB_SPACE_MASK)
> +		enable_raw_mode = true;
> +	else
> +		/* Just clear channel or proxmity, nothing to do. */
> +		return 0;
> +
> +	mutex_lock(&st->cmd_lock);
> +	st->param.cmd = MOTIONSENSE_CMD_PERFORM_CALIB;
> +	st->param.perform_calib.enable = enable_raw_mode;
> +	ret = cros_ec_motion_send_host_cmd(st, 0);
> +	mutex_unlock(&st->cmd_lock);
> +
> +	return ret;
> +}
> +
>  static const struct iio_info cros_ec_light_prox_info = {
>  	.read_raw = &cros_ec_light_prox_read,
>  	.write_raw = &cros_ec_light_prox_write,
>  	.read_avail = &cros_ec_sensors_core_read_avail,
> +	.update_scan_mode = &cros_ec_light_prox_update_scan_mode,
>  };
>  
>  static int cros_ec_light_prox_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> +	struct cros_ec_sensorhub *sensor_hub = dev_get_drvdata(dev->parent);
>  	struct iio_dev *indio_dev;
>  	struct cros_ec_light_prox_state *state;
>  	struct iio_chan_spec *channel;
> -	int ret;
> +	int ret, i, num_channels = CROS_EC_LIGHT_PROX_MIN_CHANNELS;
>  
>  	indio_dev = devm_iio_device_alloc(dev, sizeof(*state));
>  	if (!indio_dev)
>  		return -ENOMEM;
>  
>  	ret = cros_ec_sensors_core_init(pdev, indio_dev, true,
> -					cros_ec_sensors_capture,
> -					cros_ec_sensors_push_data);
> +					cros_ec_light_capture,
> +					cros_ec_light_push_data);
>  	if (ret)
>  		return ret;
>  
> @@ -189,28 +478,40 @@ static int cros_ec_light_prox_probe(struct platform_device *pdev)
>  	state = iio_priv(indio_dev);
>  	state->core.type = state->core.resp->info.type;
>  	state->core.loc = state->core.resp->info.location;
> -	channel = state->channels;
>  
> -	/* Common part */
> -	channel->info_mask_shared_by_all =
> -		BIT(IIO_CHAN_INFO_SAMP_FREQ);
> -	channel->info_mask_shared_by_all_available =
> -		BIT(IIO_CHAN_INFO_SAMP_FREQ);
> -	channel->scan_type.realbits = CROS_EC_SENSOR_BITS;
> -	channel->scan_type.storagebits = CROS_EC_SENSOR_BITS;
> -	channel->scan_type.shift = 0;
> -	channel->scan_index = 0;
> -	channel->ext_info = cros_ec_sensors_ext_info;
> -	channel->scan_type.sign = 'u';
> +	/* Check if we need more sensors for RGB (or XYZ). */
> +	state->core.param.cmd = MOTIONSENSE_CMD_INFO;
> +	if (cros_ec_light_extra_send_host_cmd(&state->core, 1, 0) == 0 &&
> +	    state->core.resp->info.type == MOTIONSENSE_TYPE_LIGHT_RGB)
> +		num_channels += 2 * CROS_EC_SENSOR_MAX_AXIS;
> +
> +	channel = devm_kcalloc(dev, num_channels, sizeof(*channel), 0);
> +	if (!channel)
> +		return -ENOMEM;
> +
> +	indio_dev->channels = channel;
> +	indio_dev->num_channels = num_channels;
> +	indio_dev->available_scan_masks = cros_ec_light_prox_bitmasks;
>  
> +	cros_ec_light_channel_common(channel);
>  	/* Sensor specific */
>  	switch (state->core.type) {
>  	case MOTIONSENSE_TYPE_LIGHT:
>  		channel->type = IIO_LIGHT;
> +		if (num_channels > CROS_EC_LIGHT_PROX_MIN_CHANNELS) {
> +			/*
> +			 * To set a global scale, as CALIB_SCALE for RGB sensor
> +			 * is limited between 0 and 2.
> +			 */
> +			channel->info_mask_shared_by_all |=
> +				BIT(IIO_CHAN_INFO_SCALE);
> +		}
>  		channel->info_mask_separate =
> -			BIT(IIO_CHAN_INFO_PROCESSED) |
> +			BIT(IIO_CHAN_INFO_RAW) |
>  			BIT(IIO_CHAN_INFO_CALIBBIAS) |
>  			BIT(IIO_CHAN_INFO_CALIBSCALE);
> +		channel->modified = 1;
> +		channel->channel2 = IIO_MOD_LIGHT_CLEAR;
>  		break;
>  	case MOTIONSENSE_TYPE_PROX:
>  		channel->type = IIO_PROXIMITY;
> @@ -223,20 +524,48 @@ static int cros_ec_light_prox_probe(struct platform_device *pdev)
>  		dev_warn(dev, "Unknown motion sensor\n");
>  		return -EINVAL;
>  	}
> +	channel++;
> +
> +	if (num_channels > CROS_EC_LIGHT_PROX_MIN_CHANNELS) {
> +		u8 sensor_num = state->core.param.info.sensor_num;
> +		int idx;
> +
> +		for (i = CROS_EC_SENSOR_X, idx = 1; i < CROS_EC_SENSOR_MAX_AXIS;
> +				i++, channel++, idx++) {
> +			cros_ec_light_channel_common(channel);
> +			channel->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> +			channel->scan_index = idx;
> +			channel->modified = 1;
> +			channel->channel2 = IIO_MOD_X + i;
> +			channel->type = IIO_LIGHT;
> +		}
> +		for (i = CROS_EC_SENSOR_X; i < CROS_EC_SENSOR_MAX_AXIS;
> +				i++, channel++, idx++) {
> +			cros_ec_light_channel_common(channel);
> +			channel->info_mask_separate =
> +				BIT(IIO_CHAN_INFO_RAW) |
> +				BIT(IIO_CHAN_INFO_CALIBBIAS) |
> +				BIT(IIO_CHAN_INFO_CALIBSCALE);
> +			channel->scan_index = idx;
> +			channel->modified = 1;
> +			channel->channel2 = IIO_MOD_LIGHT_RED + i;
> +			channel->type = IIO_LIGHT;
> +		}
> +		cros_ec_sensorhub_register_push_data(
> +				sensor_hub,
> +				sensor_num + 1,
> +				indio_dev,
> +				cros_ec_light_push_data_rgb);
> +	}
>  
>  	/* Timestamp */
> -	channel++;
>  	channel->type = IIO_TIMESTAMP;
>  	channel->channel = -1;
> -	channel->scan_index = 1;
> +	channel->scan_index = num_channels - 1;
>  	channel->scan_type.sign = 's';
>  	channel->scan_type.realbits = 64;
>  	channel->scan_type.storagebits = 64;
>  
> -	indio_dev->channels = state->channels;
> -
> -	indio_dev->num_channels = CROS_EC_LIGHT_PROX_MAX_CHANNELS;
> -
>  	state->core.read_ec_sensors_data = cros_ec_sensors_read_cmd;
>  
>  	return devm_iio_device_register(dev, indio_dev);
> diff --git a/drivers/platform/chrome/cros_ec_sensorhub.c b/drivers/platform/chrome/cros_ec_sensorhub.c
> index b7f2c00db5e1e..f85191ab2ee34 100644
> --- a/drivers/platform/chrome/cros_ec_sensorhub.c
> +++ b/drivers/platform/chrome/cros_ec_sensorhub.c
> @@ -103,6 +103,9 @@ static int cros_ec_sensorhub_register(struct device *dev,
>  		case MOTIONSENSE_TYPE_LIGHT:
>  			name = "cros-ec-light";
>  			break;
> +		case MOTIONSENSE_TYPE_LIGHT_RGB:
> +			/* Processed with cros-ec-light. */
> +			continue;
>  		case MOTIONSENSE_TYPE_ACTIVITY:
>  			name = "cros-ec-activity";
>  			break;
> diff --git a/include/linux/iio/common/cros_ec_sensors_core.h b/include/linux/iio/common/cros_ec_sensors_core.h
> index 7bc961defa87e..c31766c64bf94 100644
> --- a/include/linux/iio/common/cros_ec_sensors_core.h
> +++ b/include/linux/iio/common/cros_ec_sensors_core.h
> @@ -26,7 +26,6 @@ enum {
>  
>  /*
>   * 4 16 bit channels are allowed.
> - * Good enough for current sensors, they use up to 3 16 bit vectors.
>   */
>  #define CROS_EC_SAMPLE_SIZE  (sizeof(s64) * 2)
>  
> diff --git a/include/linux/platform_data/cros_ec_commands.h b/include/linux/platform_data/cros_ec_commands.h
> index 395c9b2b05c66..e8b51e112c191 100644
> --- a/include/linux/platform_data/cros_ec_commands.h
> +++ b/include/linux/platform_data/cros_ec_commands.h
> @@ -2342,6 +2342,7 @@ enum motionsensor_type {
>  	MOTIONSENSE_TYPE_ACTIVITY = 5,
>  	MOTIONSENSE_TYPE_BARO = 6,
>  	MOTIONSENSE_TYPE_SYNC = 7,
> +	MOTIONSENSE_TYPE_LIGHT_RGB = 8,
>  	MOTIONSENSE_TYPE_MAX,
>  };
>  
> @@ -2375,6 +2376,7 @@ enum motionsensor_chip {
>  	MOTIONSENSE_CHIP_LSM6DS3 = 17,
>  	MOTIONSENSE_CHIP_LSM6DSO = 18,
>  	MOTIONSENSE_CHIP_LNG2DM = 19,
> +	MOTIONSENSE_CHIP_TCS3400 = 20,
>  	MOTIONSENSE_CHIP_MAX,
>  };
>  



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 3/3] iio: cros_ec_light: Add support for RGB sensor
  2020-05-06 23:03 ` [PATCH v2 3/3] iio: cros_ec_light: Add support for RGB sensor Gwendal Grignou
  2020-05-08 15:23   ` Jonathan Cameron
@ 2020-05-11  6:31   ` kbuild test robot
  1 sibling, 0 replies; 10+ messages in thread
From: kbuild test robot @ 2020-05-11  6:31 UTC (permalink / raw)
  To: Gwendal Grignou, enric.balletbo, jic23
  Cc: kbuild-all, bleung, groeck, linux-kernel, linux-iio, Gwendal Grignou

[-- Attachment #1: Type: text/plain, Size: 10588 bytes --]

Hi Gwendal,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on iio/togreg]
[also build test WARNING on chrome-platform-linux/for-next linus/master v5.7-rc5 next-20200508]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Gwendal-Grignou/iio-cros_ec-Add-support-for-RGB-light-sensor/20200507-074251
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: i386-randconfig-a003-20200511 (attached as .config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/iio/light/cros_ec_light_prox.c:39:40: warning: left-hand operand of comma expression has no effect [-Wunused-value]
    #define CROS_EC_LIGHT_RGB_SPACE_MASK (6, 4)
                                           ^
   drivers/iio/light/cros_ec_light_prox.c:51:2: note: in expansion of macro 'CROS_EC_LIGHT_RGB_SPACE_MASK'
     CROS_EC_LIGHT_RGB_SPACE_MASK,
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/iio/light/cros_ec_light_prox.c:39:38: error: initializer element is not constant
    #define CROS_EC_LIGHT_RGB_SPACE_MASK (6, 4)
                                         ^
   drivers/iio/light/cros_ec_light_prox.c:51:2: note: in expansion of macro 'CROS_EC_LIGHT_RGB_SPACE_MASK'
     CROS_EC_LIGHT_RGB_SPACE_MASK,
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/iio/light/cros_ec_light_prox.c:39:38: note: (near initialization for 'cros_ec_light_prox_bitmasks[3]')
    #define CROS_EC_LIGHT_RGB_SPACE_MASK (6, 4)
                                         ^
   drivers/iio/light/cros_ec_light_prox.c:51:2: note: in expansion of macro 'CROS_EC_LIGHT_RGB_SPACE_MASK'
     CROS_EC_LIGHT_RGB_SPACE_MASK,
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/iio/light/cros_ec_light_prox.c:39:40: warning: left-hand operand of comma expression has no effect [-Wunused-value]
    #define CROS_EC_LIGHT_RGB_SPACE_MASK (6, 4)
                                           ^
   drivers/iio/light/cros_ec_light_prox.c:52:2: note: in expansion of macro 'CROS_EC_LIGHT_RGB_SPACE_MASK'
     CROS_EC_LIGHT_RGB_SPACE_MASK | CROS_EC_LIGHT_CLEAR_OR_PROXIMITY_MASK,
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/iio/light/cros_ec_light_prox.c:39:38: error: initializer element is not constant
    #define CROS_EC_LIGHT_RGB_SPACE_MASK (6, 4)
                                         ^
   drivers/iio/light/cros_ec_light_prox.c:52:2: note: in expansion of macro 'CROS_EC_LIGHT_RGB_SPACE_MASK'
     CROS_EC_LIGHT_RGB_SPACE_MASK | CROS_EC_LIGHT_CLEAR_OR_PROXIMITY_MASK,
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/iio/light/cros_ec_light_prox.c:39:38: note: (near initialization for 'cros_ec_light_prox_bitmasks[4]')
    #define CROS_EC_LIGHT_RGB_SPACE_MASK (6, 4)
                                         ^
   drivers/iio/light/cros_ec_light_prox.c:52:2: note: in expansion of macro 'CROS_EC_LIGHT_RGB_SPACE_MASK'
     CROS_EC_LIGHT_RGB_SPACE_MASK | CROS_EC_LIGHT_CLEAR_OR_PROXIMITY_MASK,
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   In file included from include/linux/dev_printk.h:14:0,
                    from include/linux/device.h:15,
                    from drivers/iio/light/cros_ec_light_prox.c:8:
   drivers/iio/light/cros_ec_light_prox.c: In function 'cros_ec_light_capture':
   drivers/iio/light/cros_ec_light_prox.c:39:40: warning: left-hand operand of comma expression has no effect [-Wunused-value]
    #define CROS_EC_LIGHT_RGB_SPACE_MASK (6, 4)
                                           ^
   include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var'
    #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                                       ^~~~
>> drivers/iio/light/cros_ec_light_prox.c:403:2: note: in expansion of macro 'if'
     if ((scan_mask & CROS_EC_LIGHT_XYZ_SPACE_MASK) ||
     ^~
   drivers/iio/light/cros_ec_light_prox.c:404:19: note: in expansion of macro 'CROS_EC_LIGHT_RGB_SPACE_MASK'
         (scan_mask & CROS_EC_LIGHT_RGB_SPACE_MASK)) {
                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/iio/light/cros_ec_light_prox.c:39:40: warning: left-hand operand of comma expression has no effect [-Wunused-value]
    #define CROS_EC_LIGHT_RGB_SPACE_MASK (6, 4)
                                           ^
   include/linux/compiler.h:58:61: note: in definition of macro '__trace_if_var'
    #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                                                ^~~~
>> drivers/iio/light/cros_ec_light_prox.c:403:2: note: in expansion of macro 'if'
     if ((scan_mask & CROS_EC_LIGHT_XYZ_SPACE_MASK) ||
     ^~
   drivers/iio/light/cros_ec_light_prox.c:404:19: note: in expansion of macro 'CROS_EC_LIGHT_RGB_SPACE_MASK'
         (scan_mask & CROS_EC_LIGHT_RGB_SPACE_MASK)) {
                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/iio/light/cros_ec_light_prox.c:39:40: warning: left-hand operand of comma expression has no effect [-Wunused-value]
    #define CROS_EC_LIGHT_RGB_SPACE_MASK (6, 4)
                                           ^
   include/linux/compiler.h:69:3: note: in definition of macro '__trace_if_value'
     (cond) ?     \
      ^~~~
   include/linux/compiler.h:56:28: note: in expansion of macro '__trace_if_var'
    #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
                               ^~~~~~~~~~~~~~
>> drivers/iio/light/cros_ec_light_prox.c:403:2: note: in expansion of macro 'if'
     if ((scan_mask & CROS_EC_LIGHT_XYZ_SPACE_MASK) ||
     ^~
   drivers/iio/light/cros_ec_light_prox.c:404:19: note: in expansion of macro 'CROS_EC_LIGHT_RGB_SPACE_MASK'
         (scan_mask & CROS_EC_LIGHT_RGB_SPACE_MASK)) {
                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/iio/light/cros_ec_light_prox.c: In function 'cros_ec_light_prox_update_scan_mode':
   drivers/iio/light/cros_ec_light_prox.c:39:40: warning: left-hand operand of comma expression has no effect [-Wunused-value]
    #define CROS_EC_LIGHT_RGB_SPACE_MASK (6, 4)
                                           ^
   include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var'
    #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                                       ^~~~
   drivers/iio/light/cros_ec_light_prox.c:434:7: note: in expansion of macro 'if'
     else if (*scan_mask & CROS_EC_LIGHT_RGB_SPACE_MASK)
          ^~
   drivers/iio/light/cros_ec_light_prox.c:434:24: note: in expansion of macro 'CROS_EC_LIGHT_RGB_SPACE_MASK'
     else if (*scan_mask & CROS_EC_LIGHT_RGB_SPACE_MASK)
                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/iio/light/cros_ec_light_prox.c:39:40: warning: left-hand operand of comma expression has no effect [-Wunused-value]
    #define CROS_EC_LIGHT_RGB_SPACE_MASK (6, 4)
                                           ^
   include/linux/compiler.h:58:61: note: in definition of macro '__trace_if_var'
    #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                                                ^~~~
   drivers/iio/light/cros_ec_light_prox.c:434:7: note: in expansion of macro 'if'
     else if (*scan_mask & CROS_EC_LIGHT_RGB_SPACE_MASK)
          ^~
   drivers/iio/light/cros_ec_light_prox.c:434:24: note: in expansion of macro 'CROS_EC_LIGHT_RGB_SPACE_MASK'
     else if (*scan_mask & CROS_EC_LIGHT_RGB_SPACE_MASK)
                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/iio/light/cros_ec_light_prox.c:39:40: warning: left-hand operand of comma expression has no effect [-Wunused-value]
    #define CROS_EC_LIGHT_RGB_SPACE_MASK (6, 4)
                                           ^
   include/linux/compiler.h:69:3: note: in definition of macro '__trace_if_value'
     (cond) ?     \
      ^~~~
   include/linux/compiler.h:56:28: note: in expansion of macro '__trace_if_var'
    #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
                               ^~~~~~~~~~~~~~
   drivers/iio/light/cros_ec_light_prox.c:434:7: note: in expansion of macro 'if'
     else if (*scan_mask & CROS_EC_LIGHT_RGB_SPACE_MASK)
          ^~
   drivers/iio/light/cros_ec_light_prox.c:434:24: note: in expansion of macro 'CROS_EC_LIGHT_RGB_SPACE_MASK'
     else if (*scan_mask & CROS_EC_LIGHT_RGB_SPACE_MASK)
                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

vim +/if +403 drivers/iio/light/cros_ec_light_prox.c

   378	
   379	static irqreturn_t cros_ec_light_capture(int irq, void *p)
   380	{
   381		struct iio_poll_func *pf = p;
   382		struct iio_dev *indio_dev = pf->indio_dev;
   383		struct cros_ec_sensors_core_state *st = iio_priv(indio_dev);
   384		int ret, i, idx = 0;
   385		s16 data;
   386		const unsigned long scan_mask = *indio_dev->active_scan_mask;
   387	
   388		mutex_lock(&st->cmd_lock);
   389	
   390		/* Clear capture data. */
   391		memset(st->samples, 0, indio_dev->scan_bytes);
   392	
   393		/* Read first channel. */
   394		ret = cros_ec_sensors_read_cmd(indio_dev, 1, &data);
   395		if (ret < 0) {
   396			mutex_unlock(&st->cmd_lock);
   397			goto done;
   398		}
   399		if (scan_mask & CROS_EC_LIGHT_CLEAR_OR_PROXIMITY_MASK)
   400			((s16 *)st->samples)[idx++] = data;
   401	
   402		/* Read remaining channels. */
 > 403		if ((scan_mask & CROS_EC_LIGHT_XYZ_SPACE_MASK) ||
   404		    (scan_mask & CROS_EC_LIGHT_RGB_SPACE_MASK)) {
   405			ret = cros_ec_light_extra_send_host_cmd(
   406					st, 1, sizeof(st->resp->data));
   407			if (ret < 0) {
   408				mutex_unlock(&st->cmd_lock);
   409				goto done;
   410			}
   411			for (i = 0; i < CROS_EC_SENSOR_MAX_AXIS; i++)
   412				((s16 *)st->samples)[idx++] = st->resp->data.data[i];
   413		}
   414		mutex_unlock(&st->cmd_lock);
   415	
   416		iio_push_to_buffers_with_timestamp(indio_dev, st->samples,
   417						   iio_get_time_ns(indio_dev));
   418	
   419	done:
   420		iio_trigger_notify_done(indio_dev->trig);
   421	
   422		return IRQ_HANDLED;
   423	}
   424	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32469 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/3] iio: Add in_illumincance vectors in different color spaces
  2020-05-08 15:16   ` Jonathan Cameron
@ 2020-05-12  4:10     ` Gwendal Grignou
  2020-05-16 15:49       ` Jonathan Cameron
  0 siblings, 1 reply; 10+ messages in thread
From: Gwendal Grignou @ 2020-05-12  4:10 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Enric Balletbo i Serra, Jonathan Cameron, Benson Leung,
	Guenter Roeck, linux-kernel, linux-iio

On Fri, May 8, 2020 at 8:16 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Wed, 6 May 2020 16:03:22 -0700
> Gwendal Grignou <gwendal@chromium.org> wrote:
>
> Illuminance in the title.  Plus I'm still arguing these
> aren't illuminance values.
>
> The Y value is illuminance but X and Z definitely aren't.
> RGB needs to stick to intensity - like the other existing
> RGB sensors.
>
> Gah.  XYZ and IIO is a mess.
>
> I suppose we could introduce a new type and have
> in_illumiance_raw
> in_chromacity_x_raw
> in_chromacity_z_raw but chances of anyone understanding what we
> are on about without reading wikipedia is low...
>
> Sigh.  Unless someone else chips in, I'm inclined to be lazy and rely
> on documentation to let in_illuminance_x,y,z be defined as being
> cie xyz color space measurements.
>
> It seems slighlty preferable to defining another type for these,
> though I suspect I'll regret this comment when some adds
> cie lab which was always my favourite colour space :)
>
>
>
> > Define 2 spaces for defining color coming from color sensors:
> > RGB and XYZ: Both are in lux.
> > RGB is the raw output from sensors (Red, Green and Blue channels), in
> > addition to the existing clear channel (C).
>
> > The RGBC vector goes through a matrix transformation to produce the XYZ
> > vector. Y is illumincance, and XY caries the chromaticity information.
> > The matrix is model specific, as the color sensor can be behing a glass
> > that can filter some wavelengths.
> >
> > Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> > ---
> > New in v2.
> >
> >  Documentation/ABI/testing/sysfs-bus-iio | 27 +++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> > index d3e53a6d8331b..256db6e63a25e 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-iio
> > +++ b/Documentation/ABI/testing/sysfs-bus-iio
> > @@ -1309,6 +1309,33 @@ Description:
> >               Illuminance measurement, units after application of scale
> >               and offset are lux.
> >
> > +What:                /sys/.../iio:deviceX/in_illuminance_red_raw
> > +What:                /sys/.../iio:deviceX/in_illuminance_green_raw
> > +What:                /sys/.../iio:deviceX/in_illuminance_blue_raw
> > +KernelVersion:       5.7
> > +Contact:     linux-iio@vger.kernel.org
> > +Description:
> > +             Illuminance measuremed in red, green or blue channels, units
> > +             after application of scale and offset are lux.
>
> No they aren't.  Units are some magic intensity at some magic wavelength.
>
> > +
> > +What:                /sys/.../iio:deviceX/in_illuminance_x_raw
> > +What:                /sys/.../iio:deviceX/in_illuminance_y_raw
> > +What:                /sys/.../iio:deviceX/in_illuminance_z_raw
> > +KernelVersion:       5.7
> > +Contact:     linux-iio@vger.kernel.org
> > +Description:
> > +             lluminance measured in the CIE 1931 color space (XYZ).
> > +             in_illuminance_y_raw is a measure of the brightness, and is
> > +             identical in_illuminance_raw.
>
> That is fair enough.
>
> > +             in_illuminance_x_raw and in_illuminance_z_raw carry chromacity
> > +             information.
> > +             in_illuminance_x,y,z_raw are be obtained from the sensor color
> > +             channels using color matching functions that may be device
> > +             specific.
> > +             Units after application of scale and offset are lux.
>
> True for Y, not for X and Z which don't have 'units' as such.
Indeed,I have difficulty mapping what comes from the sensor after
adapting to an acceptable universal entity.

The goal of the sensor is to measure the ambient correlated color
temperature (CCT), based on the x and y of the CIE xyY color space.
Given x and y are defined as x = X / (X + Y +Z) and y = X / (X + Y +
Z), X, Y and Z must have the same units.

CCT(x,y) is computed in user space, for example using this
approximation (https://en.wikipedia.org/wiki/Color_temperature#Approximation).

Gwendal.


>
> > +             The measurments can be used to represent colors in the CIE
> > +             xyY color space
>
> XYZ
>
> > +
> >  What:                /sys/.../iio:deviceX/in_intensityY_raw
> >  What:                /sys/.../iio:deviceX/in_intensityY_ir_raw
> >  What:                /sys/.../iio:deviceX/in_intensityY_both_raw
>
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/3] iio: Add in_illumincance vectors in different color spaces
  2020-05-12  4:10     ` Gwendal Grignou
@ 2020-05-16 15:49       ` Jonathan Cameron
  0 siblings, 0 replies; 10+ messages in thread
From: Jonathan Cameron @ 2020-05-16 15:49 UTC (permalink / raw)
  To: Gwendal Grignou
  Cc: Jonathan Cameron, Enric Balletbo i Serra, Benson Leung,
	Guenter Roeck, linux-kernel, linux-iio

On Mon, 11 May 2020 21:10:40 -0700
Gwendal Grignou <gwendal@chromium.org> wrote:

> On Fri, May 8, 2020 at 8:16 AM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > On Wed, 6 May 2020 16:03:22 -0700
> > Gwendal Grignou <gwendal@chromium.org> wrote:
> >
> > Illuminance in the title.  Plus I'm still arguing these
> > aren't illuminance values.
> >
> > The Y value is illuminance but X and Z definitely aren't.
> > RGB needs to stick to intensity - like the other existing
> > RGB sensors.
> >
> > Gah.  XYZ and IIO is a mess.
> >
> > I suppose we could introduce a new type and have
> > in_illumiance_raw
> > in_chromacity_x_raw
> > in_chromacity_z_raw but chances of anyone understanding what we
> > are on about without reading wikipedia is low...
> >
> > Sigh.  Unless someone else chips in, I'm inclined to be lazy and rely
> > on documentation to let in_illuminance_x,y,z be defined as being
> > cie xyz color space measurements.
> >
> > It seems slighlty preferable to defining another type for these,
> > though I suspect I'll regret this comment when some adds
> > cie lab which was always my favourite colour space :)
> >
> >
> >  
> > > Define 2 spaces for defining color coming from color sensors:
> > > RGB and XYZ: Both are in lux.
> > > RGB is the raw output from sensors (Red, Green and Blue channels), in
> > > addition to the existing clear channel (C).  
> >  
> > > The RGBC vector goes through a matrix transformation to produce the XYZ
> > > vector. Y is illumincance, and XY caries the chromaticity information.
> > > The matrix is model specific, as the color sensor can be behing a glass
> > > that can filter some wavelengths.
> > >
> > > Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> > > ---
> > > New in v2.
> > >
> > >  Documentation/ABI/testing/sysfs-bus-iio | 27 +++++++++++++++++++++++++
> > >  1 file changed, 27 insertions(+)
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> > > index d3e53a6d8331b..256db6e63a25e 100644
> > > --- a/Documentation/ABI/testing/sysfs-bus-iio
> > > +++ b/Documentation/ABI/testing/sysfs-bus-iio
> > > @@ -1309,6 +1309,33 @@ Description:
> > >               Illuminance measurement, units after application of scale
> > >               and offset are lux.
> > >
> > > +What:                /sys/.../iio:deviceX/in_illuminance_red_raw
> > > +What:                /sys/.../iio:deviceX/in_illuminance_green_raw
> > > +What:                /sys/.../iio:deviceX/in_illuminance_blue_raw
> > > +KernelVersion:       5.7
> > > +Contact:     linux-iio@vger.kernel.org
> > > +Description:
> > > +             Illuminance measuremed in red, green or blue channels, units
> > > +             after application of scale and offset are lux.  
> >
> > No they aren't.  Units are some magic intensity at some magic wavelength.
> >  
> > > +
> > > +What:                /sys/.../iio:deviceX/in_illuminance_x_raw
> > > +What:                /sys/.../iio:deviceX/in_illuminance_y_raw
> > > +What:                /sys/.../iio:deviceX/in_illuminance_z_raw
> > > +KernelVersion:       5.7
> > > +Contact:     linux-iio@vger.kernel.org
> > > +Description:
> > > +             lluminance measured in the CIE 1931 color space (XYZ).
> > > +             in_illuminance_y_raw is a measure of the brightness, and is
> > > +             identical in_illuminance_raw.  
> >
> > That is fair enough.
> >  
> > > +             in_illuminance_x_raw and in_illuminance_z_raw carry chromacity
> > > +             information.
> > > +             in_illuminance_x,y,z_raw are be obtained from the sensor color
> > > +             channels using color matching functions that may be device
> > > +             specific.
> > > +             Units after application of scale and offset are lux.  
> >
> > True for Y, not for X and Z which don't have 'units' as such.  
> Indeed,I have difficulty mapping what comes from the sensor after
> adapting to an acceptable universal entity.
> 
> The goal of the sensor is to measure the ambient correlated color
> temperature (CCT), based on the x and y of the CIE xyY color space.
> Given x and y are defined as x = X / (X + Y +Z) and y = X / (X + Y +
> Z), X, Y and Z must have the same units.

The issue here is that illuminance is an unusual thing.   To go
for an analogy.  It's like measuring the volume of something at a
particular temperature.  However, it's only defined at that temperature.
You can divide it by other volumes at other temperatures and get all sorts
of interesting quantities and  much like volume the indeed have
the same units, but that unit is not lux (which is unit of illuminance).
The reason being illuminance is like defining volume at 0 degree centigrade.

Illuminance is only defined for that particular set of frequencies
and is not defined otherwise.

If we'd called our light input something other than illuminance 
- say 'in_light_raw' then we could play fast and loose with units
perhaps but we didn't.  We deliberately used intensity to represent
all light measurements other than the one specific one that is illuminance.

So we should stick to intensity really which was chosen specifically
to 'not carry' the weird characteristics that illuminance has. It deliberately
makes no statement about frequency sensitivity etc.

In a past life I did a lot of work machine vision so am familiar with
most of the standard colour spaces and what they were trying to do
when defining them.  Personally always like CieLAB because you can
basically use it to get rid of shadows :)  Maths to compute it
is however fairly crazy.

Jonathan
 
> 
> CCT(x,y) is computed in user space, for example using this
> approximation (https://en.wikipedia.org/wiki/Color_temperature#Approximation).
> 
> Gwendal.
> 
> 
> >  
> > > +             The measurments can be used to represent colors in the CIE
> > > +             xyY color space  
> >
> > XYZ
> >  
> > > +
> > >  What:                /sys/.../iio:deviceX/in_intensityY_raw
> > >  What:                /sys/.../iio:deviceX/in_intensityY_ir_raw
> > >  What:                /sys/.../iio:deviceX/in_intensityY_both_raw  
> >
> >  


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-05-16 15:50 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-06 23:03 [PATCH v3 0/3] iio: cros_ec: Add support for RGB light sensor Gwendal Grignou
2020-05-06 23:03 ` [PATCH v2 1/3] iio: Add in_illumincance vectors in different color spaces Gwendal Grignou
2020-05-08 15:16   ` Jonathan Cameron
2020-05-12  4:10     ` Gwendal Grignou
2020-05-16 15:49       ` Jonathan Cameron
2020-05-06 23:03 ` [PATCH v2 2/3] iio: cros_ec: Allow enabling/disabling calibration mode Gwendal Grignou
2020-05-06 23:03 ` [PATCH v2 3/3] iio: cros_ec_light: Add support for RGB sensor Gwendal Grignou
2020-05-08 15:23   ` Jonathan Cameron
2020-05-11  6:31   ` kbuild test robot
2020-05-08 14:56 ` [PATCH v3 0/3] iio: cros_ec: Add support for RGB light sensor Jonathan Cameron

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).