2020-05-07 01:46:06

by Gwendal Grignou

[permalink] [raw]
Subject: [PATCH v3 0/3] iio: cros_ec: Add support for RGB light sensor

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


2020-05-07 02:51:37

by Gwendal Grignou

[permalink] [raw]
Subject: [PATCH v2 1/3] iio: Add in_illumincance vectors in different color spaces

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 <[email protected]>
---
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: [email protected]
+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: [email protected]
+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

2020-05-07 02:51:41

by Gwendal Grignou

[permalink] [raw]
Subject: [PATCH v2 2/3] iio: cros_ec: Allow enabling/disabling calibration mode

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 <[email protected]>
---
.../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

2020-05-08 14:58:34

by Jonathan Cameron

[permalink] [raw]
Subject: Re: [PATCH v3 0/3] iio: cros_ec: Add support for RGB light sensor

On Wed, 6 May 2020 16:03:21 -0700
Gwendal Grignou <[email protected]> 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(-)
>


2020-05-08 15:18:51

by Jonathan Cameron

[permalink] [raw]
Subject: Re: [PATCH v2 1/3] iio: Add in_illumincance vectors in different color spaces

On Wed, 6 May 2020 16:03:22 -0700
Gwendal Grignou <[email protected]> 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 <[email protected]>
> ---
> 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: [email protected]
> +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: [email protected]
> +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


2020-05-12 04:12:53

by Gwendal Grignou

[permalink] [raw]
Subject: Re: [PATCH v2 1/3] iio: Add in_illumincance vectors in different color spaces

On Fri, May 8, 2020 at 8:16 AM Jonathan Cameron
<[email protected]> wrote:
>
> On Wed, 6 May 2020 16:03:22 -0700
> Gwendal Grignou <[email protected]> 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 <[email protected]>
> > ---
> > 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: [email protected]
> > +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: [email protected]
> > +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
>
>

2020-05-16 15:53:52

by Jonathan Cameron

[permalink] [raw]
Subject: Re: [PATCH v2 1/3] iio: Add in_illumincance vectors in different color spaces

On Mon, 11 May 2020 21:10:40 -0700
Gwendal Grignou <[email protected]> wrote:

> On Fri, May 8, 2020 at 8:16 AM Jonathan Cameron
> <[email protected]> wrote:
> >
> > On Wed, 6 May 2020 16:03:22 -0700
> > Gwendal Grignou <[email protected]> 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 <[email protected]>
> > > ---
> > > 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: [email protected]
> > > +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: [email protected]
> > > +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
> >
> >