* [PATCHv2 1/2] iio: Documentation: Add proximity unit
@ 2017-01-11 15:51 Enric Balletbo i Serra
2017-01-11 15:51 ` [PATCHv2 2/2] iio: cros_ec_light_prox: add ChromeOS EC Light and Proximity Sensors Enric Balletbo i Serra
2017-01-14 12:58 ` [PATCHv2 1/2] iio: Documentation: Add proximity unit Jonathan Cameron
0 siblings, 2 replies; 9+ messages in thread
From: Enric Balletbo i Serra @ 2017-01-11 15:51 UTC (permalink / raw)
To: linux-kernel, linux-iio; +Cc: jic23, lars
To follow iio guidelines Where possible we stick to the raw SI unit, so
specify meters for proximity.
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
Documentation/ABI/testing/sysfs-bus-iio | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index b8f220f..1bd8ed1 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -1245,7 +1245,8 @@ Description:
reflectivity of infrared or ultrasound emitted.
Often these sensors are unit less and as such conversion
to SI units is not possible. Higher proximity measurements
- indicate closer objects, and vice versa.
+ indicate closer objects, and vice versa. Units after
+ application of scale and offset are meters.
What: /sys/.../iio:deviceX/in_illuminance_input
What: /sys/.../iio:deviceX/in_illuminance_raw
--
2.9.3
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCHv2 2/2] iio: cros_ec_light_prox: add ChromeOS EC Light and Proximity Sensors
2017-01-11 15:51 [PATCHv2 1/2] iio: Documentation: Add proximity unit Enric Balletbo i Serra
@ 2017-01-11 15:51 ` Enric Balletbo i Serra
2017-01-14 13:06 ` Jonathan Cameron
2017-01-14 12:58 ` [PATCHv2 1/2] iio: Documentation: Add proximity unit Jonathan Cameron
1 sibling, 1 reply; 9+ messages in thread
From: Enric Balletbo i Serra @ 2017-01-11 15:51 UTC (permalink / raw)
To: linux-kernel, linux-iio; +Cc: jic23, lars, Gwendal Grignou, Guenter Roeck
From: Gwendal Grignou <gwendal@chromium.org>
Handle Light and Proximity sensors presented by the ChromeOS EC Sensor hub.
Creates an IIO device for each functions.
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Signed-off-by: Guenter Roeck <groeck@chromium.org>
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
drivers/iio/common/cros_ec_sensors/Kconfig | 8 +
drivers/iio/common/cros_ec_sensors/Makefile | 1 +
.../common/cros_ec_sensors/cros_ec_light_prox.c | 287 +++++++++++++++++++++
3 files changed, 296 insertions(+)
create mode 100644 drivers/iio/common/cros_ec_sensors/cros_ec_light_prox.c
diff --git a/drivers/iio/common/cros_ec_sensors/Kconfig b/drivers/iio/common/cros_ec_sensors/Kconfig
index 135f682..26360d4 100644
--- a/drivers/iio/common/cros_ec_sensors/Kconfig
+++ b/drivers/iio/common/cros_ec_sensors/Kconfig
@@ -20,3 +20,11 @@ config IIO_CROS_EC_SENSORS
Accelerometers, Gyroscope and Magnetometer that are
presented by the ChromeOS EC Sensor hub.
Creates an IIO device for each functions.
+
+config IIO_CROS_EC_LIGHT_PROX
+ tristate "ChromeOS EC Light and Proximity Sensors"
+ depends on IIO_CROS_EC_SENSORS_CORE
+ help
+ Module to handle Light and Proximity sensors
+ presented by the ChromeOS EC Sensor hub.
+ Creates an IIO device for each functions.
diff --git a/drivers/iio/common/cros_ec_sensors/Makefile b/drivers/iio/common/cros_ec_sensors/Makefile
index ec716ff..7aaf2a2 100644
--- a/drivers/iio/common/cros_ec_sensors/Makefile
+++ b/drivers/iio/common/cros_ec_sensors/Makefile
@@ -4,3 +4,4 @@
obj-$(CONFIG_IIO_CROS_EC_SENSORS_CORE) += cros_ec_sensors_core.o
obj-$(CONFIG_IIO_CROS_EC_SENSORS) += cros_ec_sensors.o
+obj-$(CONFIG_IIO_CROS_EC_LIGHT_PROX) += cros_ec_light_prox.o
diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_light_prox.c b/drivers/iio/common/cros_ec_sensors/cros_ec_light_prox.c
new file mode 100644
index 0000000..62faf58
--- /dev/null
+++ b/drivers/iio/common/cros_ec_sensors/cros_ec_light_prox.c
@@ -0,0 +1,287 @@
+/*
+ * cros_ec_light_prox - Driver for light and prox sensors behing CrosEC.
+ *
+ * Copyright (C) 2017 Google, Inc
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/kfifo_buf.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/kernel.h>
+#include <linux/mfd/cros_ec.h>
+#include <linux/mfd/cros_ec_commands.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/sysfs.h>
+
+#include "cros_ec_sensors_core.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.
+ */
+#define CROS_EC_LIGHT_PROX_MAX_CHANNELS (1 + 1)
+
+/* 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];
+};
+
+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 = IIO_VAL_INT;
+ int idx = chan->scan_index;
+
+ mutex_lock(&st->core.cmd_lock);
+
+ switch (mask) {
+ case IIO_CHAN_INFO_PROCESSED:
+ if (cros_ec_sensors_read_cmd(indio_dev, 1 << idx,
+ (s16 *)&data) < 0) {
+ ret = -EIO;
+ break;
+ }
+
+ switch (chan->type) {
+ /*
+ * The data coming from the sensor is pre-processed, represents
+ * the ambient light illuminance reading expressed in lux.
+ */
+ case IIO_LIGHT:
+ *val = data;
+ ret = IIO_VAL_INT;
+ break;
+ /*
+ * The data coming from the sensor is pre-processed, represents
+ * the distance reading expressed in cm. Convert to m.
+ */
+ case IIO_PROXIMITY:
+ *val = 0;
+ *val2 = data * 10000;
+ ret = IIO_VAL_INT_PLUS_MICRO;
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+ break;
+ case IIO_CHAN_INFO_CALIBBIAS:
+ st->core.param.cmd = MOTIONSENSE_CMD_SENSOR_OFFSET;
+ st->core.param.sensor_offset.flags = 0;
+
+ if (cros_ec_motion_send_host_cmd(&st->core, 0)) {
+ ret = -EIO;
+ break;
+ }
+
+ /* Save values */
+ st->core.calib[0] = st->core.resp->sensor_offset.offset[0];
+
+ *val = st->core.calib[idx];
+ break;
+ case IIO_CHAN_INFO_CALIBSCALE:
+ /*
+ * RANGE is used for calibration
+ * scale is a number x.y, where x is coded on 16bits,
+ * y coded on 16 bits, between 0 and 9999.
+ */
+ st->core.param.cmd = MOTIONSENSE_CMD_SENSOR_RANGE;
+ st->core.param.sensor_range.data = EC_MOTION_SENSE_NO_VALUE;
+
+ if (cros_ec_motion_send_host_cmd(&st->core, 0)) {
+ ret = -EIO;
+ break;
+ }
+
+ val64 = st->core.resp->sensor_range.ret;
+ *val = val64 >> 16;
+ *val2 = (val64 & 0xffff) * 100;
+ ret = IIO_VAL_INT_PLUS_MICRO;
+ break;
+ default:
+ ret = cros_ec_sensors_core_read(&st->core, chan, val, val2,
+ mask);
+ break;
+ }
+
+ mutex_unlock(&st->core.cmd_lock);
+
+ return ret;
+}
+
+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 = 0;
+ int idx = chan->scan_index;
+
+ mutex_lock(&st->core.cmd_lock);
+
+ switch (mask) {
+ case IIO_CHAN_INFO_CALIBBIAS:
+ st->core.calib[idx] = 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];
+ st->core.param.sensor_offset.temp =
+ EC_MOTION_SENSE_INVALID_CALIB_TEMP;
+ if (cros_ec_motion_send_host_cmd(&st->core, 0))
+ ret = -EIO;
+ break;
+ case IIO_CHAN_INFO_CALIBSCALE:
+ st->core.param.cmd = MOTIONSENSE_CMD_SENSOR_RANGE;
+ st->core.param.sensor_range.data = (val << 16) | (val2 / 100);
+ if (cros_ec_motion_send_host_cmd(&st->core, 0))
+ ret = -EIO;
+ break;
+ default:
+ ret = cros_ec_sensors_core_write(&st->core, chan, val, val2,
+ mask);
+ break;
+ }
+
+ mutex_unlock(&st->core.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,
+ .driver_module = THIS_MODULE,
+};
+
+static int cros_ec_light_prox_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent);
+ struct cros_ec_device *ec_device;
+ struct iio_dev *indio_dev;
+ struct cros_ec_light_prox_state *state;
+ struct iio_chan_spec *channel;
+ int ret;
+
+ if (!ec_dev || !ec_dev->ec_dev) {
+ dev_warn(dev, "No CROS EC device found.\n");
+ return -EINVAL;
+ }
+ ec_device = ec_dev->ec_dev;
+
+ indio_dev = devm_iio_device_alloc(dev, sizeof(*state));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ ret = cros_ec_sensors_core_init(pdev, indio_dev, true);
+ if (ret)
+ return ret;
+
+ indio_dev->info = &cros_ec_light_prox_info;
+ 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_separate =
+// BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_PROCESSED) |
+ BIT(IIO_CHAN_INFO_CALIBBIAS) |
+ BIT(IIO_CHAN_INFO_CALIBSCALE);
+ channel->info_mask_shared_by_all =
+ BIT(IIO_CHAN_INFO_SCALE) |
+ BIT(IIO_CHAN_INFO_SAMP_FREQ) |
+ BIT(IIO_CHAN_INFO_FREQUENCY);
+ 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';
+
+ state->core.calib[0] = 0;
+
+ /* Sensor specific */
+ switch (state->core.type) {
+ case MOTIONSENSE_TYPE_LIGHT:
+ channel->type = IIO_LIGHT;
+ break;
+ case MOTIONSENSE_TYPE_PROX:
+ channel->type = IIO_PROXIMITY;
+ break;
+ default:
+ dev_warn(dev, "Unknown motion sensor\n");
+ return -EINVAL;
+ }
+
+ /* Timestamp */
+ channel++;
+ channel->type = IIO_TIMESTAMP;
+ channel->channel = -1;
+ channel->scan_index = 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;
+
+ ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
+ cros_ec_sensors_capture, NULL);
+ if (ret)
+ return ret;
+
+ return devm_iio_device_register(dev, indio_dev);
+}
+
+static const struct platform_device_id cros_ec_light_prox_ids[] = {
+ {
+ .name = "cros-ec-prox",
+ },
+ {
+ .name = "cros-ec-light",
+ },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(platform, cros_ec_light_prox_ids);
+
+static struct platform_driver cros_ec_light_prox_platform_driver = {
+ .driver = {
+ .name = "cros-ec-light-prox",
+ },
+ .probe = cros_ec_light_prox_probe,
+ .id_table = cros_ec_light_prox_ids,
+};
+module_platform_driver(cros_ec_light_prox_platform_driver);
+
+MODULE_DESCRIPTION("ChromeOS EC light/proximity sensors driver");
+MODULE_LICENSE("GPL v2");
--
2.9.3
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCHv2 1/2] iio: Documentation: Add proximity unit
2017-01-11 15:51 [PATCHv2 1/2] iio: Documentation: Add proximity unit Enric Balletbo i Serra
2017-01-11 15:51 ` [PATCHv2 2/2] iio: cros_ec_light_prox: add ChromeOS EC Light and Proximity Sensors Enric Balletbo i Serra
@ 2017-01-14 12:58 ` Jonathan Cameron
1 sibling, 0 replies; 9+ messages in thread
From: Jonathan Cameron @ 2017-01-14 12:58 UTC (permalink / raw)
To: Enric Balletbo i Serra, linux-kernel, linux-iio; +Cc: lars, Matt Ranostay
On 11/01/17 15:51, Enric Balletbo i Serra wrote:
> To follow iio guidelines Where possible we stick to the raw SI unit, so
> specify meters for proximity.
>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Please remember to add a change log to later versions of patch sets.
Saves reviewers who typically have the memory of goldfish (well I do
anyway) having to read old emails to check what they found last time!
This patch is fine on it's own. Effectively allows us to drop the
custom attribute description in sysfs-bus-iio-proximit-as3935 as well.
(Matt cc'd)
Applied to the togreg branch of iio.git.
Thanks,
Jonathan
> ---
> Documentation/ABI/testing/sysfs-bus-iio | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index b8f220f..1bd8ed1 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -1245,7 +1245,8 @@ Description:
> reflectivity of infrared or ultrasound emitted.
> Often these sensors are unit less and as such conversion
> to SI units is not possible. Higher proximity measurements
> - indicate closer objects, and vice versa.
> + indicate closer objects, and vice versa. Units after
> + application of scale and offset are meters.
>
> What: /sys/.../iio:deviceX/in_illuminance_input
> What: /sys/.../iio:deviceX/in_illuminance_raw
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCHv2 2/2] iio: cros_ec_light_prox: add ChromeOS EC Light and Proximity Sensors
2017-01-11 15:51 ` [PATCHv2 2/2] iio: cros_ec_light_prox: add ChromeOS EC Light and Proximity Sensors Enric Balletbo i Serra
@ 2017-01-14 13:06 ` Jonathan Cameron
2017-01-20 14:23 ` Enric Balletbo Serra
0 siblings, 1 reply; 9+ messages in thread
From: Jonathan Cameron @ 2017-01-14 13:06 UTC (permalink / raw)
To: Enric Balletbo i Serra, linux-kernel, linux-iio
Cc: lars, Gwendal Grignou, Guenter Roeck
On 11/01/17 15:51, Enric Balletbo i Serra wrote:
> From: Gwendal Grignou <gwendal@chromium.org>
>
> Handle Light and Proximity sensors presented by the ChromeOS EC Sensor hub.
> Creates an IIO device for each functions.
>
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> Signed-off-by: Guenter Roeck <groeck@chromium.org>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Hi.
Looks like you were cleaning up the interface and left a few bits behind...
Please tidy those up and repost.
Thanks,
Jonathan
> ---
> drivers/iio/common/cros_ec_sensors/Kconfig | 8 +
> drivers/iio/common/cros_ec_sensors/Makefile | 1 +
> .../common/cros_ec_sensors/cros_ec_light_prox.c | 287 +++++++++++++++++++++
I missed this before. I'd actually like to have this in the proximity
directory rather than here. The reason is to keep the drivers grouped
by function is preferred to grouping by what implements them.
> 3 files changed, 296 insertions(+)
> create mode 100644 drivers/iio/common/cros_ec_sensors/cros_ec_light_prox.c
>
> diff --git a/drivers/iio/common/cros_ec_sensors/Kconfig b/drivers/iio/common/cros_ec_sensors/Kconfig
> index 135f682..26360d4 100644
> --- a/drivers/iio/common/cros_ec_sensors/Kconfig
> +++ b/drivers/iio/common/cros_ec_sensors/Kconfig
> @@ -20,3 +20,11 @@ config IIO_CROS_EC_SENSORS
> Accelerometers, Gyroscope and Magnetometer that are
> presented by the ChromeOS EC Sensor hub.
> Creates an IIO device for each functions.
> +
> +config IIO_CROS_EC_LIGHT_PROX
> + tristate "ChromeOS EC Light and Proximity Sensors"
> + depends on IIO_CROS_EC_SENSORS_CORE
> + help
> + Module to handle Light and Proximity sensors
> + presented by the ChromeOS EC Sensor hub.
> + Creates an IIO device for each functions.
> diff --git a/drivers/iio/common/cros_ec_sensors/Makefile b/drivers/iio/common/cros_ec_sensors/Makefile
> index ec716ff..7aaf2a2 100644
> --- a/drivers/iio/common/cros_ec_sensors/Makefile
> +++ b/drivers/iio/common/cros_ec_sensors/Makefile
> @@ -4,3 +4,4 @@
>
> obj-$(CONFIG_IIO_CROS_EC_SENSORS_CORE) += cros_ec_sensors_core.o
> obj-$(CONFIG_IIO_CROS_EC_SENSORS) += cros_ec_sensors.o
> +obj-$(CONFIG_IIO_CROS_EC_LIGHT_PROX) += cros_ec_light_prox.o
> diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_light_prox.c b/drivers/iio/common/cros_ec_sensors/cros_ec_light_prox.c
> new file mode 100644
> index 0000000..62faf58
> --- /dev/null
> +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_light_prox.c
> @@ -0,0 +1,287 @@
> +/*
> + * cros_ec_light_prox - Driver for light and prox sensors behing CrosEC.
> + *
> + * Copyright (C) 2017 Google, Inc
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/kfifo_buf.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/cros_ec.h>
> +#include <linux/mfd/cros_ec_commands.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/sysfs.h>
> +
> +#include "cros_ec_sensors_core.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.
> + */
> +#define CROS_EC_LIGHT_PROX_MAX_CHANNELS (1 + 1)
> +
> +/* 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];
> +};
> +
> +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 = IIO_VAL_INT;
> + int idx = chan->scan_index;
> +
> + mutex_lock(&st->core.cmd_lock);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_PROCESSED:
> + if (cros_ec_sensors_read_cmd(indio_dev, 1 << idx,
> + (s16 *)&data) < 0) {
> + ret = -EIO;
> + break;
> + }
> +
> + switch (chan->type) {
> + /*
> + * The data coming from the sensor is pre-processed, represents
> + * the ambient light illuminance reading expressed in lux.
> + */
> + case IIO_LIGHT:
> + *val = data;
> + ret = IIO_VAL_INT;
> + break;
> + /*
> + * The data coming from the sensor is pre-processed, represents
> + * the distance reading expressed in cm. Convert to m.
> + */
> + case IIO_PROXIMITY:
Out of curiousity, what kind of proximity sensor is this? I'm suprised it
has any real world units as I'd assumed we were dealing with a light
intensity based sensor and reflection.
> + *val = 0;
> + *val2 = data * 10000;
> + ret = IIO_VAL_INT_PLUS_MICRO;
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> + break;
> + case IIO_CHAN_INFO_CALIBBIAS:
> + st->core.param.cmd = MOTIONSENSE_CMD_SENSOR_OFFSET;
> + st->core.param.sensor_offset.flags = 0;
> +
> + if (cros_ec_motion_send_host_cmd(&st->core, 0)) {
> + ret = -EIO;
> + break;
> + }
> +
> + /* Save values */
> + st->core.calib[0] = st->core.resp->sensor_offset.offset[0];
> +
> + *val = st->core.calib[idx];
> + break;
> + case IIO_CHAN_INFO_CALIBSCALE:
> + /*
> + * RANGE is used for calibration
> + * scale is a number x.y, where x is coded on 16bits,
> + * y coded on 16 bits, between 0 and 9999.
> + */
> + st->core.param.cmd = MOTIONSENSE_CMD_SENSOR_RANGE;
> + st->core.param.sensor_range.data = EC_MOTION_SENSE_NO_VALUE;
> +
> + if (cros_ec_motion_send_host_cmd(&st->core, 0)) {
> + ret = -EIO;
> + break;
> + }
> +
> + val64 = st->core.resp->sensor_range.ret;
> + *val = val64 >> 16;
> + *val2 = (val64 & 0xffff) * 100;
> + ret = IIO_VAL_INT_PLUS_MICRO;
> + break;
> + default:
> + ret = cros_ec_sensors_core_read(&st->core, chan, val, val2,
> + mask);
> + break;
> + }
> +
> + mutex_unlock(&st->core.cmd_lock);
> +
> + return ret;
> +}
> +
> +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 = 0;
> + int idx = chan->scan_index;
> +
> + mutex_lock(&st->core.cmd_lock);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_CALIBBIAS:
> + st->core.calib[idx] = 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];
> + st->core.param.sensor_offset.temp =
> + EC_MOTION_SENSE_INVALID_CALIB_TEMP;
This needs some explanation of what is going on. This presumably is
ultimately passing an offset to be applied by the sensor hub?
> + if (cros_ec_motion_send_host_cmd(&st->core, 0))
Command of 0? This wants an explanation.
> + ret = -EIO;
> + break;
> + case IIO_CHAN_INFO_CALIBSCALE:
> + st->core.param.cmd = MOTIONSENSE_CMD_SENSOR_RANGE;
> + st->core.param.sensor_range.data = (val << 16) | (val2 / 100);
> + if (cros_ec_motion_send_host_cmd(&st->core, 0))
> + ret = -EIO;
> + break;
> + default:
> + ret = cros_ec_sensors_core_write(&st->core, chan, val, val2,
> + mask);
> + break;
> + }
> +
> + mutex_unlock(&st->core.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,
> + .driver_module = THIS_MODULE,
> +};
> +
> +static int cros_ec_light_prox_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent);
> + struct cros_ec_device *ec_device;
> + struct iio_dev *indio_dev;
> + struct cros_ec_light_prox_state *state;
> + struct iio_chan_spec *channel;
> + int ret;
> +
> + if (!ec_dev || !ec_dev->ec_dev) {
> + dev_warn(dev, "No CROS EC device found.\n");
> + return -EINVAL;
> + }
> + ec_device = ec_dev->ec_dev;
> +
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*state));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + ret = cros_ec_sensors_core_init(pdev, indio_dev, true);
> + if (ret)
> + return ret;
> +
> + indio_dev->info = &cros_ec_light_prox_info;
> + 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_separate =
> +// BIT(IIO_CHAN_INFO_RAW) |
What's this doing here?
> + BIT(IIO_CHAN_INFO_PROCESSED) |
> + BIT(IIO_CHAN_INFO_CALIBBIAS) |
> + BIT(IIO_CHAN_INFO_CALIBSCALE);
> + channel->info_mask_shared_by_all =
> + BIT(IIO_CHAN_INFO_SCALE) |
Providing processed and 'scale' is unusual... Even more interesting
is you don't seem to provide it or indeed the next two...
Please check out this whole area.
> + BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> + BIT(IIO_CHAN_INFO_FREQUENCY);
> + 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';
> +
> + state->core.calib[0] = 0;
> +
> + /* Sensor specific */
> + switch (state->core.type) {
> + case MOTIONSENSE_TYPE_LIGHT:
> + channel->type = IIO_LIGHT;
> + break;
> + case MOTIONSENSE_TYPE_PROX:
> + channel->type = IIO_PROXIMITY;
> + break;
> + default:
> + dev_warn(dev, "Unknown motion sensor\n");
> + return -EINVAL;
> + }
> +
> + /* Timestamp */
> + channel++;
> + channel->type = IIO_TIMESTAMP;
> + channel->channel = -1;
> + channel->scan_index = 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;
> +
> + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> + cros_ec_sensors_capture, NULL);
> + if (ret)
> + return ret;
> +
> + return devm_iio_device_register(dev, indio_dev);
> +}
> +
> +static const struct platform_device_id cros_ec_light_prox_ids[] = {
> + {
> + .name = "cros-ec-prox",
> + },
> + {
> + .name = "cros-ec-light",
> + },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(platform, cros_ec_light_prox_ids);
> +
> +static struct platform_driver cros_ec_light_prox_platform_driver = {
> + .driver = {
> + .name = "cros-ec-light-prox",
> + },
> + .probe = cros_ec_light_prox_probe,
> + .id_table = cros_ec_light_prox_ids,
> +};
> +module_platform_driver(cros_ec_light_prox_platform_driver);
> +
> +MODULE_DESCRIPTION("ChromeOS EC light/proximity sensors driver");
> +MODULE_LICENSE("GPL v2");
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCHv2 2/2] iio: cros_ec_light_prox: add ChromeOS EC Light and Proximity Sensors
2017-01-14 13:06 ` Jonathan Cameron
@ 2017-01-20 14:23 ` Enric Balletbo Serra
2017-01-23 18:18 ` Gwendal Grignou
0 siblings, 1 reply; 9+ messages in thread
From: Enric Balletbo Serra @ 2017-01-20 14:23 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Enric Balletbo i Serra, linux-kernel, linux-iio,
Lars-Peter Clausen, Gwendal Grignou, Guenter Roeck
Hi Jonathan,
Thanks for the review, I am preparing v3. Some answers to your questions below.
2017-01-14 14:06 GMT+01:00 Jonathan Cameron <jic23@kernel.org>:
> On 11/01/17 15:51, Enric Balletbo i Serra wrote:
>> From: Gwendal Grignou <gwendal@chromium.org>
>>
>> Handle Light and Proximity sensors presented by the ChromeOS EC Sensor hub.
>> Creates an IIO device for each functions.
>>
>> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
>> Signed-off-by: Guenter Roeck <groeck@chromium.org>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> Hi.
>
> Looks like you were cleaning up the interface and left a few bits behind...
> Please tidy those up and repost.
>
> Thanks,
>
> Jonathan
>> ---
>> drivers/iio/common/cros_ec_sensors/Kconfig | 8 +
>> drivers/iio/common/cros_ec_sensors/Makefile | 1 +
>> .../common/cros_ec_sensors/cros_ec_light_prox.c | 287 +++++++++++++++++++++
> I missed this before. I'd actually like to have this in the proximity
> directory rather than here. The reason is to keep the drivers grouped
> by function is preferred to grouping by what implements them.
Ok, I'll move this.
>> 3 files changed, 296 insertions(+)
>> create mode 100644 drivers/iio/common/cros_ec_sensors/cros_ec_light_prox.c
>>
>> diff --git a/drivers/iio/common/cros_ec_sensors/Kconfig b/drivers/iio/common/cros_ec_sensors/Kconfig
>> index 135f682..26360d4 100644
>> --- a/drivers/iio/common/cros_ec_sensors/Kconfig
>> +++ b/drivers/iio/common/cros_ec_sensors/Kconfig
>> @@ -20,3 +20,11 @@ config IIO_CROS_EC_SENSORS
>> Accelerometers, Gyroscope and Magnetometer that are
>> presented by the ChromeOS EC Sensor hub.
>> Creates an IIO device for each functions.
>> +
>> +config IIO_CROS_EC_LIGHT_PROX
>> + tristate "ChromeOS EC Light and Proximity Sensors"
>> + depends on IIO_CROS_EC_SENSORS_CORE
>> + help
>> + Module to handle Light and Proximity sensors
>> + presented by the ChromeOS EC Sensor hub.
>> + Creates an IIO device for each functions.
>> diff --git a/drivers/iio/common/cros_ec_sensors/Makefile b/drivers/iio/common/cros_ec_sensors/Makefile
>> index ec716ff..7aaf2a2 100644
>> --- a/drivers/iio/common/cros_ec_sensors/Makefile
>> +++ b/drivers/iio/common/cros_ec_sensors/Makefile
>> @@ -4,3 +4,4 @@
>>
>> obj-$(CONFIG_IIO_CROS_EC_SENSORS_CORE) += cros_ec_sensors_core.o
>> obj-$(CONFIG_IIO_CROS_EC_SENSORS) += cros_ec_sensors.o
>> +obj-$(CONFIG_IIO_CROS_EC_LIGHT_PROX) += cros_ec_light_prox.o
>> diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_light_prox.c b/drivers/iio/common/cros_ec_sensors/cros_ec_light_prox.c
>> new file mode 100644
>> index 0000000..62faf58
>> --- /dev/null
>> +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_light_prox.c
>> @@ -0,0 +1,287 @@
>> +/*
>> + * cros_ec_light_prox - Driver for light and prox sensors behing CrosEC.
>> + *
>> + * Copyright (C) 2017 Google, Inc
>> + *
>> + * This software is licensed under the terms of the GNU General Public
>> + * License version 2, as published by the Free Software Foundation, and
>> + * may be copied, distributed, and modified under those terms.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/delay.h>
>> +#include <linux/device.h>
>> +#include <linux/iio/buffer.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/kfifo_buf.h>
>> +#include <linux/iio/trigger.h>
>> +#include <linux/iio/triggered_buffer.h>
>> +#include <linux/iio/trigger_consumer.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mfd/cros_ec.h>
>> +#include <linux/mfd/cros_ec_commands.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/sysfs.h>
>> +
>> +#include "cros_ec_sensors_core.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.
>> + */
>> +#define CROS_EC_LIGHT_PROX_MAX_CHANNELS (1 + 1)
>> +
>> +/* 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];
>> +};
>> +
>> +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 = IIO_VAL_INT;
>> + int idx = chan->scan_index;
>> +
>> + mutex_lock(&st->core.cmd_lock);
>> +
>> + switch (mask) {
>> + case IIO_CHAN_INFO_PROCESSED:
>> + if (cros_ec_sensors_read_cmd(indio_dev, 1 << idx,
>> + (s16 *)&data) < 0) {
>> + ret = -EIO;
>> + break;
>> + }
>> +
>> + switch (chan->type) {
>> + /*
>> + * The data coming from the sensor is pre-processed, represents
>> + * the ambient light illuminance reading expressed in lux.
>> + */
>> + case IIO_LIGHT:
>> + *val = data;
>> + ret = IIO_VAL_INT;
>> + break;
>> + /*
>> + * The data coming from the sensor is pre-processed, represents
>> + * the distance reading expressed in cm. Convert to m.
>> + */
>> + case IIO_PROXIMITY:
> Out of curiousity, what kind of proximity sensor is this? I'm suprised it
> has any real world units as I'd assumed we were dealing with a light
> intensity based sensor and reflection.
The CrosEC acts as a sensor hub, it can have different sensors
attached that differs betweens Chromebooks models. The sensor hub
captures the data from sensors and does some conversion and then
exposes the result through it's interface. E.g For light and proximity
sensors it can have attached a si144x [1] Proximity/Ambient Light
Sensor Modules.
[1] http://www.silabs.com/products/sensors/infraredsensors/Pages/Si114x.aspx
>> + *val = 0;
>> + *val2 = data * 10000;
>> + ret = IIO_VAL_INT_PLUS_MICRO;
>> + break;
>> + default:
>> + ret = -EINVAL;
>> + break;
>> + }
>> + break;
>> + case IIO_CHAN_INFO_CALIBBIAS:
>> + st->core.param.cmd = MOTIONSENSE_CMD_SENSOR_OFFSET;
>> + st->core.param.sensor_offset.flags = 0;
>> +
>> + if (cros_ec_motion_send_host_cmd(&st->core, 0)) {
>> + ret = -EIO;
>> + break;
>> + }
>> +
>> + /* Save values */
>> + st->core.calib[0] = st->core.resp->sensor_offset.offset[0];
>> +
>> + *val = st->core.calib[idx];
>> + break;
>> + case IIO_CHAN_INFO_CALIBSCALE:
>> + /*
>> + * RANGE is used for calibration
>> + * scale is a number x.y, where x is coded on 16bits,
>> + * y coded on 16 bits, between 0 and 9999.
>> + */
>> + st->core.param.cmd = MOTIONSENSE_CMD_SENSOR_RANGE;
>> + st->core.param.sensor_range.data = EC_MOTION_SENSE_NO_VALUE;
>> +
>> + if (cros_ec_motion_send_host_cmd(&st->core, 0)) {
>> + ret = -EIO;
>> + break;
>> + }
>> +
>> + val64 = st->core.resp->sensor_range.ret;
>> + *val = val64 >> 16;
>> + *val2 = (val64 & 0xffff) * 100;
>> + ret = IIO_VAL_INT_PLUS_MICRO;
>> + break;
>> + default:
>> + ret = cros_ec_sensors_core_read(&st->core, chan, val, val2,
>> + mask);
>> + break;
>> + }
>> +
>> + mutex_unlock(&st->core.cmd_lock);
>> +
>> + return ret;
>> +}
>> +
>> +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 = 0;
>> + int idx = chan->scan_index;
>> +
>> + mutex_lock(&st->core.cmd_lock);
>> +
>> + switch (mask) {
>> + case IIO_CHAN_INFO_CALIBBIAS:
>> + st->core.calib[idx] = 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];
>> + st->core.param.sensor_offset.temp =
>> + EC_MOTION_SENSE_INVALID_CALIB_TEMP;
> This needs some explanation of what is going on. This presumably is
> ultimately passing an offset to be applied by the sensor hub?
This sets the calibration information in the sensor hub (EC) for the
specified sensor, the MOTION_SENSE_SET_OFFSET is to be able to write
the values, in offset is stored the calibration value and temp is to
store the temperature at calibration which is unknown/invalid.
>> + if (cros_ec_motion_send_host_cmd(&st->core, 0))
> Command of 0? This wants an explanation.
The second parameter of cros_ec_motion_send_host_cmd is an optional
length for the response, when 0 it assumes a 'default' response,
otherwise it request a response to the command with the specified
length.
>> + ret = -EIO;
>> + break;
>> + case IIO_CHAN_INFO_CALIBSCALE:
>> + st->core.param.cmd = MOTIONSENSE_CMD_SENSOR_RANGE;
>> + st->core.param.sensor_range.data = (val << 16) | (val2 / 100);
>> + if (cros_ec_motion_send_host_cmd(&st->core, 0))
>> + ret = -EIO;
>> + break;
>> + default:
>> + ret = cros_ec_sensors_core_write(&st->core, chan, val, val2,
>> + mask);
>> + break;
>> + }
>> +
>> + mutex_unlock(&st->core.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,
>> + .driver_module = THIS_MODULE,
>> +};
>> +
>> +static int cros_ec_light_prox_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct cros_ec_dev *ec_dev = dev_get_drvdata(dev->parent);
>> + struct cros_ec_device *ec_device;
>> + struct iio_dev *indio_dev;
>> + struct cros_ec_light_prox_state *state;
>> + struct iio_chan_spec *channel;
>> + int ret;
>> +
>> + if (!ec_dev || !ec_dev->ec_dev) {
>> + dev_warn(dev, "No CROS EC device found.\n");
>> + return -EINVAL;
>> + }
>> + ec_device = ec_dev->ec_dev;
>> +
>> + indio_dev = devm_iio_device_alloc(dev, sizeof(*state));
>> + if (!indio_dev)
>> + return -ENOMEM;
>> +
>> + ret = cros_ec_sensors_core_init(pdev, indio_dev, true);
>> + if (ret)
>> + return ret;
>> +
>> + indio_dev->info = &cros_ec_light_prox_info;
>> + 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_separate =
>> +// BIT(IIO_CHAN_INFO_RAW) |
> What's this doing here?
Sorry, removed.
>> + BIT(IIO_CHAN_INFO_PROCESSED) |
>> + BIT(IIO_CHAN_INFO_CALIBBIAS) |
>> + BIT(IIO_CHAN_INFO_CALIBSCALE);
>> + channel->info_mask_shared_by_all =
>> + BIT(IIO_CHAN_INFO_SCALE) |
> Providing processed and 'scale' is unusual... Even more interesting
> is you don't seem to provide it or indeed the next two...
>
> Please check out this whole area.
Yes, I get rid of scale from here.
>> + BIT(IIO_CHAN_INFO_SAMP_FREQ) |
>> + BIT(IIO_CHAN_INFO_FREQUENCY);
These are from cros_ec_sensors_core
>> + 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';
>> +
>> + state->core.calib[0] = 0;
>> +
>> + /* Sensor specific */
>> + switch (state->core.type) {
>> + case MOTIONSENSE_TYPE_LIGHT:
>> + channel->type = IIO_LIGHT;
>> + break;
>> + case MOTIONSENSE_TYPE_PROX:
>> + channel->type = IIO_PROXIMITY;
>> + break;
>> + default:
>> + dev_warn(dev, "Unknown motion sensor\n");
>> + return -EINVAL;
>> + }
>> +
>> + /* Timestamp */
>> + channel++;
>> + channel->type = IIO_TIMESTAMP;
>> + channel->channel = -1;
>> + channel->scan_index = 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;
>> +
>> + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
>> + cros_ec_sensors_capture, NULL);
>> + if (ret)
>> + return ret;
>> +
>> + return devm_iio_device_register(dev, indio_dev);
>> +}
>> +
>> +static const struct platform_device_id cros_ec_light_prox_ids[] = {
>> + {
>> + .name = "cros-ec-prox",
>> + },
>> + {
>> + .name = "cros-ec-light",
>> + },
>> + { /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(platform, cros_ec_light_prox_ids);
>> +
>> +static struct platform_driver cros_ec_light_prox_platform_driver = {
>> + .driver = {
>> + .name = "cros-ec-light-prox",
>> + },
>> + .probe = cros_ec_light_prox_probe,
>> + .id_table = cros_ec_light_prox_ids,
>> +};
>> +module_platform_driver(cros_ec_light_prox_platform_driver);
>> +
>> +MODULE_DESCRIPTION("ChromeOS EC light/proximity sensors driver");
>> +MODULE_LICENSE("GPL v2");
>>
>
I'm preparing v3, thanks,
Enric
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCHv2 2/2] iio: cros_ec_light_prox: add ChromeOS EC Light and Proximity Sensors
2017-01-20 14:23 ` Enric Balletbo Serra
@ 2017-01-23 18:18 ` Gwendal Grignou
2017-01-27 10:42 ` Enric Balletbo Serra
2017-01-28 12:07 ` Jonathan Cameron
0 siblings, 2 replies; 9+ messages in thread
From: Gwendal Grignou @ 2017-01-23 18:18 UTC (permalink / raw)
To: Enric Balletbo Serra
Cc: Jonathan Cameron, Enric Balletbo i Serra, linux-kernel,
linux-iio, Lars-Peter Clausen, Gwendal Grignou, Guenter Roeck
On Fri, Jan 20, 2017 at 6:23 AM, Enric Balletbo Serra
<eballetbo@gmail.com> wrote:
> Hi Jonathan,
>
> Thanks for the review, I am preparing v3. Some answers to your questions below.
>
> 2017-01-14 14:06 GMT+01:00 Jonathan Cameron <jic23@kernel.org>:
>> On 11/01/17 15:51, Enric Balletbo i Serra wrote:
>>> From: Gwendal Grignou <gwendal@chromium.org>
>>>
>>> Handle Light and Proximity sensors presented by the ChromeOS EC Sensor hub.
>>> Creates an IIO device for each functions.
>>>
>>> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
>>> Signed-off-by: Guenter Roeck <groeck@chromium.org>
>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>> Hi.
>>
>> Looks like you were cleaning up the interface and left a few bits behind...
>> Please tidy those up and repost.
>>
>> Thanks,
>>
>> Jonathan
>>> ---
>>> drivers/iio/common/cros_ec_sensors/Kconfig | 8 +
>>> drivers/iio/common/cros_ec_sensors/Makefile | 1 +
>>> .../common/cros_ec_sensors/cros_ec_light_prox.c | 287 +++++++++++++++++++++
>> I missed this before. I'd actually like to have this in the proximity
>> directory rather than here. The reason is to keep the drivers grouped
>> by function is preferred to grouping by what implements them.
>
> Ok, I'll move this.
>
>>> 3 files changed, 296 insertions(+)
>>> create mode 100644 drivers/iio/common/cros_ec_sensors/cros_ec_light_prox.c
...
>>> +
>>> + switch (mask) {
>>> + case IIO_CHAN_INFO_PROCESSED:
>>> + if (cros_ec_sensors_read_cmd(indio_dev, 1 << idx,
>>> + (s16 *)&data) < 0) {
>>> + ret = -EIO;
>>> + break;
>>> + }
>>> +
>>> + switch (chan->type) {
>>> + /*
>>> + * The data coming from the sensor is pre-processed, represents
>>> + * the ambient light illuminance reading expressed in lux.
>>> + */
>>> + case IIO_LIGHT:
>>> + *val = data;
>>> + ret = IIO_VAL_INT;
>>> + break;
>>> + /*
>>> + * The data coming from the sensor is pre-processed, represents
>>> + * the distance reading expressed in cm. Convert to m.
>>> + */
>>> + case IIO_PROXIMITY:
>> Out of curiousity, what kind of proximity sensor is this? I'm suprised it
>> has any real world units as I'd assumed we were dealing with a light
>> intensity based sensor and reflection.
>
> The CrosEC acts as a sensor hub, it can have different sensors
> attached that differs betweens Chromebooks models. The sensor hub
> captures the data from sensors and does some conversion and then
> exposes the result through it's interface. E.g For light and proximity
> sensors it can have attached a si144x [1] Proximity/Ambient Light
> Sensor Modules.
>
> [1] http://www.silabs.com/products/sensors/infraredsensors/Pages/Si114x.aspx
>
The embedded controller (EC) does the conversion from the reflection.
SI114x has 2 mode, light sensor and proximity sensor: in the later
case, ambient light is ignored and reflection of a led is measured.
The transformation from reflection to distance is an approximation.
>>> + *val = 0;
>>> + *val2 = data * 10000;
>>> + ret = IIO_VAL_INT_PLUS_MICRO;
>>> + break;
>>> + default:
>>> + ret = -EINVAL;
...
>>> + state->core.loc = state->core.resp->info.location;
>>> + channel = state->channels;
>>> +
>>> + /* Common part */
>>> + channel->info_mask_separate =
>>> +// BIT(IIO_CHAN_INFO_RAW) |
>> What's this doing here?
>
> Sorry, removed.
>
>>> + BIT(IIO_CHAN_INFO_PROCESSED) |
>>> + BIT(IIO_CHAN_INFO_CALIBBIAS) |
>>> + BIT(IIO_CHAN_INFO_CALIBSCALE);
>>> + channel->info_mask_shared_by_all =
>>> + BIT(IIO_CHAN_INFO_SCALE) |
>> Providing processed and 'scale' is unusual... Even more interesting
>> is you don't seem to provide it or indeed the next two...
I see your point.
Data from the sensor is massaged by the EC with input from calibbias
and calibscale. Given this is not a heavy processing, it would be more
logical to expose the illimunance/proximity as IIO_CHAN_INFO_RAW
instead of IIO_CHAN_INFO_PROCESSED.
An earlier version of this driver was returning 1 to
IIO_CHAN_INFO_SCALE, that's why IIO_CHAN_INFO_SCALE was present. We
may bring it back if a sensor needs it.
>>
>> Please check out this whole area.
>
> Yes, I get rid of scale from here.
>
>>> + BIT(IIO_CHAN_INFO_SAMP_FREQ) |
>>> + BIT(IIO_CHAN_INFO_FREQUENCY);
>
> These are from cros_ec_sensors_core
>
>>> + channel->scan_type.realbits = CROS_EC_SENSOR_BITS;
>>> + channel->scan_type.storagebits = CROS_EC_SENSOR_BITS;
>>> + channel->scan_type.shift = 0;
...
>>> +
>>> +MODULE_DESCRIPTION("ChromeOS EC light/proximity sensors driver");
>>> +MODULE_LICENSE("GPL v2");
>>>
>>
>
> I'm preparing v3, thanks,
> Enric
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCHv2 2/2] iio: cros_ec_light_prox: add ChromeOS EC Light and Proximity Sensors
2017-01-23 18:18 ` Gwendal Grignou
@ 2017-01-27 10:42 ` Enric Balletbo Serra
2017-01-28 12:10 ` Jonathan Cameron
2017-01-28 12:07 ` Jonathan Cameron
1 sibling, 1 reply; 9+ messages in thread
From: Enric Balletbo Serra @ 2017-01-27 10:42 UTC (permalink / raw)
To: Gwendal Grignou
Cc: Jonathan Cameron, Enric Balletbo i Serra, linux-kernel,
linux-iio, Lars-Peter Clausen, Guenter Roeck
Hi,
2017-01-23 19:18 GMT+01:00 Gwendal Grignou <gwendal@chromium.org>:
> On Fri, Jan 20, 2017 at 6:23 AM, Enric Balletbo Serra
> <eballetbo@gmail.com> wrote:
>> Hi Jonathan,
>>
>> Thanks for the review, I am preparing v3. Some answers to your questions below.
>>
>> 2017-01-14 14:06 GMT+01:00 Jonathan Cameron <jic23@kernel.org>:
>>> On 11/01/17 15:51, Enric Balletbo i Serra wrote:
>>>> From: Gwendal Grignou <gwendal@chromium.org>
>>>>
>>>> Handle Light and Proximity sensors presented by the ChromeOS EC Sensor hub.
>>>> Creates an IIO device for each functions.
>>>>
>>>> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
>>>> Signed-off-by: Guenter Roeck <groeck@chromium.org>
>>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>>> Hi.
>>>
>>> Looks like you were cleaning up the interface and left a few bits behind...
>>> Please tidy those up and repost.
>>>
>>> Thanks,
>>>
>>> Jonathan
>>>> ---
>>>> drivers/iio/common/cros_ec_sensors/Kconfig | 8 +
>>>> drivers/iio/common/cros_ec_sensors/Makefile | 1 +
>>>> .../common/cros_ec_sensors/cros_ec_light_prox.c | 287 +++++++++++++++++++++
>>> I missed this before. I'd actually like to have this in the proximity
>>> directory rather than here. The reason is to keep the drivers grouped
>>> by function is preferred to grouping by what implements them.
>>
>> Ok, I'll move this.
>>
>>>> 3 files changed, 296 insertions(+)
>>>> create mode 100644 drivers/iio/common/cros_ec_sensors/cros_ec_light_prox.c
> ...
>>>> +
>>>> + switch (mask) {
>>>> + case IIO_CHAN_INFO_PROCESSED:
>>>> + if (cros_ec_sensors_read_cmd(indio_dev, 1 << idx,
>>>> + (s16 *)&data) < 0) {
>>>> + ret = -EIO;
>>>> + break;
>>>> + }
>>>> +
>>>> + switch (chan->type) {
>>>> + /*
>>>> + * The data coming from the sensor is pre-processed, represents
>>>> + * the ambient light illuminance reading expressed in lux.
>>>> + */
>>>> + case IIO_LIGHT:
>>>> + *val = data;
>>>> + ret = IIO_VAL_INT;
>>>> + break;
>>>> + /*
>>>> + * The data coming from the sensor is pre-processed, represents
>>>> + * the distance reading expressed in cm. Convert to m.
>>>> + */
>>>> + case IIO_PROXIMITY:
>>> Out of curiousity, what kind of proximity sensor is this? I'm suprised it
>>> has any real world units as I'd assumed we were dealing with a light
>>> intensity based sensor and reflection.
>>
>> The CrosEC acts as a sensor hub, it can have different sensors
>> attached that differs betweens Chromebooks models. The sensor hub
>> captures the data from sensors and does some conversion and then
>> exposes the result through it's interface. E.g For light and proximity
>> sensors it can have attached a si144x [1] Proximity/Ambient Light
>> Sensor Modules.
>>
>> [1] http://www.silabs.com/products/sensors/infraredsensors/Pages/Si114x.aspx
>>
> The embedded controller (EC) does the conversion from the reflection.
> SI114x has 2 mode, light sensor and proximity sensor: in the later
> case, ambient light is ignored and reflection of a led is measured.
> The transformation from reflection to distance is an approximation.
>
>>>> + *val = 0;
>>>> + *val2 = data * 10000;
>>>> + ret = IIO_VAL_INT_PLUS_MICRO;
>>>> + break;
>>>> + default:
>>>> + ret = -EINVAL;
> ...
>>>> + state->core.loc = state->core.resp->info.location;
>>>> + channel = state->channels;
>>>> +
>>>> + /* Common part */
>>>> + channel->info_mask_separate =
>>>> +// BIT(IIO_CHAN_INFO_RAW) |
>>> What's this doing here?
>>
>> Sorry, removed.
>>
>>>> + BIT(IIO_CHAN_INFO_PROCESSED) |
>>>> + BIT(IIO_CHAN_INFO_CALIBBIAS) |
>>>> + BIT(IIO_CHAN_INFO_CALIBSCALE);
>>>> + channel->info_mask_shared_by_all =
>>>> + BIT(IIO_CHAN_INFO_SCALE) |
>>> Providing processed and 'scale' is unusual... Even more interesting
>>> is you don't seem to provide it or indeed the next two...
> I see your point.
> Data from the sensor is massaged by the EC with input from calibbias
> and calibscale. Given this is not a heavy processing, it would be more
> logical to expose the illimunance/proximity as IIO_CHAN_INFO_RAW
> instead of IIO_CHAN_INFO_PROCESSED.
>
I see, I did not know about the internals of the EC for this sensor (I
will look at the EC code), but seems I was wrong saying that the EC
was returning processed value in known SI units. As Gwendal says in
the comment above, if the value is an approximation and not really
processed value that returns exactly the distance, what it really
indicates is if the object is more or less closer. It's a userspace
decision assume the values are valid meters or not. So I agree on use
RAW instead of PROCESSED. I assume is the same for light.
Jonathan, sounds good I use the RAW info for the next version again?
What do you think?
Also, as seems the EC massages the data from a si144x and there is
drivers/iio/light/si1145.c, which is a similar, makes more sense put
the cros_ec_light_prox in the iio/light directory instead of
iio/proximity?
> An earlier version of this driver was returning 1 to
> IIO_CHAN_INFO_SCALE, that's why IIO_CHAN_INFO_SCALE was present. We
> may bring it back if a sensor needs it.
>>>
>>> Please check out this whole area.
>>
>> Yes, I get rid of scale from here.
>>
>>>> + BIT(IIO_CHAN_INFO_SAMP_FREQ) |
>>>> + BIT(IIO_CHAN_INFO_FREQUENCY);
>>
>> These are from cros_ec_sensors_core
>>
>>>> + channel->scan_type.realbits = CROS_EC_SENSOR_BITS;
>>>> + channel->scan_type.storagebits = CROS_EC_SENSOR_BITS;
>>>> + channel->scan_type.shift = 0;
> ...
>>>> +
>>>> +MODULE_DESCRIPTION("ChromeOS EC light/proximity sensors driver");
>>>> +MODULE_LICENSE("GPL v2");
>>>>
>>>
>>
>> I'm preparing v3, thanks,
>> Enric
Thanks,
Enric
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCHv2 2/2] iio: cros_ec_light_prox: add ChromeOS EC Light and Proximity Sensors
2017-01-23 18:18 ` Gwendal Grignou
2017-01-27 10:42 ` Enric Balletbo Serra
@ 2017-01-28 12:07 ` Jonathan Cameron
1 sibling, 0 replies; 9+ messages in thread
From: Jonathan Cameron @ 2017-01-28 12:07 UTC (permalink / raw)
To: Gwendal Grignou, Enric Balletbo Serra
Cc: Enric Balletbo i Serra, linux-kernel, linux-iio,
Lars-Peter Clausen, Guenter Roeck
On 23/01/17 18:18, Gwendal Grignou wrote:
> On Fri, Jan 20, 2017 at 6:23 AM, Enric Balletbo Serra
> <eballetbo@gmail.com> wrote:
>> Hi Jonathan,
>>
>> Thanks for the review, I am preparing v3. Some answers to your questions below.
>>
>> 2017-01-14 14:06 GMT+01:00 Jonathan Cameron <jic23@kernel.org>:
>>> On 11/01/17 15:51, Enric Balletbo i Serra wrote:
>>>> From: Gwendal Grignou <gwendal@chromium.org>
>>>>
>>>> Handle Light and Proximity sensors presented by the ChromeOS EC Sensor hub.
>>>> Creates an IIO device for each functions.
>>>>
>>>> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
>>>> Signed-off-by: Guenter Roeck <groeck@chromium.org>
>>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>>> Hi.
>>>
>>> Looks like you were cleaning up the interface and left a few bits behind...
>>> Please tidy those up and repost.
>>>
>>> Thanks,
>>>
>>> Jonathan
>>>> ---
>>>> drivers/iio/common/cros_ec_sensors/Kconfig | 8 +
>>>> drivers/iio/common/cros_ec_sensors/Makefile | 1 +
>>>> .../common/cros_ec_sensors/cros_ec_light_prox.c | 287 +++++++++++++++++++++
>>> I missed this before. I'd actually like to have this in the proximity
>>> directory rather than here. The reason is to keep the drivers grouped
>>> by function is preferred to grouping by what implements them.
>>
>> Ok, I'll move this.
>>
>>>> 3 files changed, 296 insertions(+)
>>>> create mode 100644 drivers/iio/common/cros_ec_sensors/cros_ec_light_prox.c
> ...
>>>> +
>>>> + switch (mask) {
>>>> + case IIO_CHAN_INFO_PROCESSED:
>>>> + if (cros_ec_sensors_read_cmd(indio_dev, 1 << idx,
>>>> + (s16 *)&data) < 0) {
>>>> + ret = -EIO;
>>>> + break;
>>>> + }
>>>> +
>>>> + switch (chan->type) {
>>>> + /*
>>>> + * The data coming from the sensor is pre-processed, represents
>>>> + * the ambient light illuminance reading expressed in lux.
>>>> + */
>>>> + case IIO_LIGHT:
>>>> + *val = data;
>>>> + ret = IIO_VAL_INT;
>>>> + break;
>>>> + /*
>>>> + * The data coming from the sensor is pre-processed, represents
>>>> + * the distance reading expressed in cm. Convert to m.
>>>> + */
>>>> + case IIO_PROXIMITY:
>>> Out of curiousity, what kind of proximity sensor is this? I'm suprised it
>>> has any real world units as I'd assumed we were dealing with a light
>>> intensity based sensor and reflection.
>>
>> The CrosEC acts as a sensor hub, it can have different sensors
>> attached that differs betweens Chromebooks models. The sensor hub
>> captures the data from sensors and does some conversion and then
>> exposes the result through it's interface. E.g For light and proximity
>> sensors it can have attached a si144x [1] Proximity/Ambient Light
>> Sensor Modules.
>>
>> [1] http://www.silabs.com/products/sensors/infraredsensors/Pages/Si114x.aspx
>>
> The embedded controller (EC) does the conversion from the reflection.
> SI114x has 2 mode, light sensor and proximity sensor: in the later
> case, ambient light is ignored and reflection of a led is measured.
> The transformation from reflection to distance is an approximation.
Very approximate given the reflective material is unknown. Still that's why
they call them proximity sensors rather than distance sensors ;)
>
>>>> + *val = 0;
>>>> + *val2 = data * 10000;
>>>> + ret = IIO_VAL_INT_PLUS_MICRO;
>>>> + break;
>>>> + default:
>>>> + ret = -EINVAL;
> ...
>>>> + state->core.loc = state->core.resp->info.location;
>>>> + channel = state->channels;
>>>> +
>>>> + /* Common part */
>>>> + channel->info_mask_separate =
>>>> +// BIT(IIO_CHAN_INFO_RAW) |
>>> What's this doing here?
>>
>> Sorry, removed.
>>
>>>> + BIT(IIO_CHAN_INFO_PROCESSED) |
>>>> + BIT(IIO_CHAN_INFO_CALIBBIAS) |
>>>> + BIT(IIO_CHAN_INFO_CALIBSCALE);
>>>> + channel->info_mask_shared_by_all =
>>>> + BIT(IIO_CHAN_INFO_SCALE) |
>>> Providing processed and 'scale' is unusual... Even more interesting
>>> is you don't seem to provide it or indeed the next two...
> I see your point.
> Data from the sensor is massaged by the EC with input from calibbias
> and calibscale. Given this is not a heavy processing, it would be more
> logical to expose the illimunance/proximity as IIO_CHAN_INFO_RAW
> instead of IIO_CHAN_INFO_PROCESSED.
>
> An earlier version of this driver was returning 1 to
> IIO_CHAN_INFO_SCALE, that's why IIO_CHAN_INFO_SCALE was present. We
> may bring it back if a sensor needs it.
Yes. That makes sense.
>>>
>>> Please check out this whole area.
>>
>> Yes, I get rid of scale from here.
>>
>>>> + BIT(IIO_CHAN_INFO_SAMP_FREQ) |
>>>> + BIT(IIO_CHAN_INFO_FREQUENCY);
>>
>> These are from cros_ec_sensors_core
>>
>>>> + channel->scan_type.realbits = CROS_EC_SENSOR_BITS;
>>>> + channel->scan_type.storagebits = CROS_EC_SENSOR_BITS;
>>>> + channel->scan_type.shift = 0;
> ...
>>>> +
>>>> +MODULE_DESCRIPTION("ChromeOS EC light/proximity sensors driver");
>>>> +MODULE_LICENSE("GPL v2");
>>>>
>>>
>>
>> I'm preparing v3, thanks,
>> Enric
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCHv2 2/2] iio: cros_ec_light_prox: add ChromeOS EC Light and Proximity Sensors
2017-01-27 10:42 ` Enric Balletbo Serra
@ 2017-01-28 12:10 ` Jonathan Cameron
0 siblings, 0 replies; 9+ messages in thread
From: Jonathan Cameron @ 2017-01-28 12:10 UTC (permalink / raw)
To: Enric Balletbo Serra, Gwendal Grignou
Cc: Enric Balletbo i Serra, linux-kernel, linux-iio,
Lars-Peter Clausen, Guenter Roeck
On 27/01/17 10:42, Enric Balletbo Serra wrote:
> Hi,
>
> 2017-01-23 19:18 GMT+01:00 Gwendal Grignou <gwendal@chromium.org>:
>> On Fri, Jan 20, 2017 at 6:23 AM, Enric Balletbo Serra
>> <eballetbo@gmail.com> wrote:
>>> Hi Jonathan,
>>>
>>> Thanks for the review, I am preparing v3. Some answers to your questions below.
>>>
>>> 2017-01-14 14:06 GMT+01:00 Jonathan Cameron <jic23@kernel.org>:
>>>> On 11/01/17 15:51, Enric Balletbo i Serra wrote:
>>>>> From: Gwendal Grignou <gwendal@chromium.org>
>>>>>
>>>>> Handle Light and Proximity sensors presented by the ChromeOS EC Sensor hub.
>>>>> Creates an IIO device for each functions.
>>>>>
>>>>> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
>>>>> Signed-off-by: Guenter Roeck <groeck@chromium.org>
>>>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>>>> Hi.
>>>>
>>>> Looks like you were cleaning up the interface and left a few bits behind...
>>>> Please tidy those up and repost.
>>>>
>>>> Thanks,
>>>>
>>>> Jonathan
>>>>> ---
>>>>> drivers/iio/common/cros_ec_sensors/Kconfig | 8 +
>>>>> drivers/iio/common/cros_ec_sensors/Makefile | 1 +
>>>>> .../common/cros_ec_sensors/cros_ec_light_prox.c | 287 +++++++++++++++++++++
>>>> I missed this before. I'd actually like to have this in the proximity
>>>> directory rather than here. The reason is to keep the drivers grouped
>>>> by function is preferred to grouping by what implements them.
>>>
>>> Ok, I'll move this.
>>>
>>>>> 3 files changed, 296 insertions(+)
>>>>> create mode 100644 drivers/iio/common/cros_ec_sensors/cros_ec_light_prox.c
>> ...
>>>>> +
>>>>> + switch (mask) {
>>>>> + case IIO_CHAN_INFO_PROCESSED:
>>>>> + if (cros_ec_sensors_read_cmd(indio_dev, 1 << idx,
>>>>> + (s16 *)&data) < 0) {
>>>>> + ret = -EIO;
>>>>> + break;
>>>>> + }
>>>>> +
>>>>> + switch (chan->type) {
>>>>> + /*
>>>>> + * The data coming from the sensor is pre-processed, represents
>>>>> + * the ambient light illuminance reading expressed in lux.
>>>>> + */
>>>>> + case IIO_LIGHT:
>>>>> + *val = data;
>>>>> + ret = IIO_VAL_INT;
>>>>> + break;
>>>>> + /*
>>>>> + * The data coming from the sensor is pre-processed, represents
>>>>> + * the distance reading expressed in cm. Convert to m.
>>>>> + */
>>>>> + case IIO_PROXIMITY:
>>>> Out of curiousity, what kind of proximity sensor is this? I'm suprised it
>>>> has any real world units as I'd assumed we were dealing with a light
>>>> intensity based sensor and reflection.
>>>
>>> The CrosEC acts as a sensor hub, it can have different sensors
>>> attached that differs betweens Chromebooks models. The sensor hub
>>> captures the data from sensors and does some conversion and then
>>> exposes the result through it's interface. E.g For light and proximity
>>> sensors it can have attached a si144x [1] Proximity/Ambient Light
>>> Sensor Modules.
>>>
>>> [1] http://www.silabs.com/products/sensors/infraredsensors/Pages/Si114x.aspx
>>>
>> The embedded controller (EC) does the conversion from the reflection.
>> SI114x has 2 mode, light sensor and proximity sensor: in the later
>> case, ambient light is ignored and reflection of a led is measured.
>> The transformation from reflection to distance is an approximation.
>>
>>>>> + *val = 0;
>>>>> + *val2 = data * 10000;
>>>>> + ret = IIO_VAL_INT_PLUS_MICRO;
>>>>> + break;
>>>>> + default:
>>>>> + ret = -EINVAL;
>> ...
>>>>> + state->core.loc = state->core.resp->info.location;
>>>>> + channel = state->channels;
>>>>> +
>>>>> + /* Common part */
>>>>> + channel->info_mask_separate =
>>>>> +// BIT(IIO_CHAN_INFO_RAW) |
>>>> What's this doing here?
>>>
>>> Sorry, removed.
>>>
>>>>> + BIT(IIO_CHAN_INFO_PROCESSED) |
>>>>> + BIT(IIO_CHAN_INFO_CALIBBIAS) |
>>>>> + BIT(IIO_CHAN_INFO_CALIBSCALE);
>>>>> + channel->info_mask_shared_by_all =
>>>>> + BIT(IIO_CHAN_INFO_SCALE) |
>>>> Providing processed and 'scale' is unusual... Even more interesting
>>>> is you don't seem to provide it or indeed the next two...
>> I see your point.
>> Data from the sensor is massaged by the EC with input from calibbias
>> and calibscale. Given this is not a heavy processing, it would be more
>> logical to expose the illimunance/proximity as IIO_CHAN_INFO_RAW
>> instead of IIO_CHAN_INFO_PROCESSED.
>>
>
> I see, I did not know about the internals of the EC for this sensor (I
> will look at the EC code), but seems I was wrong saying that the EC
> was returning processed value in known SI units. As Gwendal says in
> the comment above, if the value is an approximation and not really
> processed value that returns exactly the distance, what it really
> indicates is if the object is more or less closer. It's a userspace
> decision assume the values are valid meters or not. So I agree on use
> RAW instead of PROCESSED. I assume is the same for light.
>
> Jonathan, sounds good I use the RAW info for the next version again?
> What do you think?
For the proximity it's normal to provide raw with no known scale.
For light the conversion shouldn't have an unknowns so either should be
processed (if the transform is non linear and hence pushed into the driver,
or done on the hardware), or raw with scale and offset as necessary to
describe a linear conversion.
>
> Also, as seems the EC massages the data from a si144x and there is
> drivers/iio/light/si1145.c, which is a similar, makes more sense put
> the cros_ec_light_prox in the iio/light directory instead of
> iio/proximity?
Good point. We have tended to put the dual light / proximity sensors in
there rather than proximity. Never been formalized, but might as well
keep to the convention.
>
>> An earlier version of this driver was returning 1 to
>> IIO_CHAN_INFO_SCALE, that's why IIO_CHAN_INFO_SCALE was present. We
>> may bring it back if a sensor needs it.
>>>>
>>>> Please check out this whole area.
>>>
>>> Yes, I get rid of scale from here.
>>>
>>>>> + BIT(IIO_CHAN_INFO_SAMP_FREQ) |
>>>>> + BIT(IIO_CHAN_INFO_FREQUENCY);
>>>
>>> These are from cros_ec_sensors_core
>>>
>>>>> + channel->scan_type.realbits = CROS_EC_SENSOR_BITS;
>>>>> + channel->scan_type.storagebits = CROS_EC_SENSOR_BITS;
>>>>> + channel->scan_type.shift = 0;
>> ...
>>>>> +
>>>>> +MODULE_DESCRIPTION("ChromeOS EC light/proximity sensors driver");
>>>>> +MODULE_LICENSE("GPL v2");
>>>>>
>>>>
>>>
>>> I'm preparing v3, thanks,
>>> Enric
>
> Thanks,
> Enric
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-01-28 12:14 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-11 15:51 [PATCHv2 1/2] iio: Documentation: Add proximity unit Enric Balletbo i Serra
2017-01-11 15:51 ` [PATCHv2 2/2] iio: cros_ec_light_prox: add ChromeOS EC Light and Proximity Sensors Enric Balletbo i Serra
2017-01-14 13:06 ` Jonathan Cameron
2017-01-20 14:23 ` Enric Balletbo Serra
2017-01-23 18:18 ` Gwendal Grignou
2017-01-27 10:42 ` Enric Balletbo Serra
2017-01-28 12:10 ` Jonathan Cameron
2017-01-28 12:07 ` Jonathan Cameron
2017-01-14 12:58 ` [PATCHv2 1/2] iio: Documentation: Add proximity unit 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).