Re: [PATCH] iio: cros_ec_light_prox: add ChromeOS EC Light and Proximity Sensors

[Jonathan Cameron]

On 30/11/16 08:57, Enric Balletbo Serra wrote:
> Hi Jonathan,
> 
> Many thanks for the review, I'm preparing v2. One question below.
> 
> 2016-11-27 11:39 GMT+01:00 Jonathan Cameron <jic23@xxxxxxxxxx>:
>> On 25/11/16 12:03, Enric Balletbo i Serra wrote:
>>> From: Gwendal Grignou <gwendal@xxxxxxxxxxxx>
>>>
>>> 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@xxxxxxxxxxxx>
>>> Signed-off-by: Guenter Roeck <groeck@xxxxxxxxxxxx>
>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx>
>> A few comments / questions inline.   We've missed the upcoming merge window
>> now anyway so plenty of time to tidy up loose ends.
>>
>> 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    | 278 +++++++++++++++++++++
>>>  3 files changed, 287 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..4fd0b87 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"
>>> +     select IIO_CROS_EC_SENSORS_CORE
>> As the build bot has pointed out this doesn't work...
>> I'd go with depends on myself as it's simpler than making sure to select
>> the whole tree of things needed.
>>
> 
> I'll fix this in v2.
> 
>>> +     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..40a34de
>>> --- /dev/null
>>> +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_light_prox.c
>>> @@ -0,0 +1,278 @@
>>> +/*
>>> + * cros_ec_light_prox - Driver for light and prox sensors behing CrosEC.
>>> + *
>>> + * Copyright (C) 2016 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_sensors_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_sensors_state *st = iio_priv(indio_dev);
>>> +     u16 data = 0;
>>> +     s64 val64;
>>> +     int ret;
>>> +     int idx = chan->scan_index;
>>> +
>>> +     mutex_lock(&st->core.cmd_lock);
>>> +
>>> +     switch (mask) {
>>> +     case IIO_CHAN_INFO_RAW:
>>> +             ret = cros_ec_sensors_read_cmd(indio_dev, 1 << idx,
>>> +                                            (s16 *)&data);
>> As I read the code, this is already being output in lux or
>>> +             if (ret < 0)
>>> +                     break;
>>> +             *val = data;
>>> +             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 (ret < 0)
>>> +                     break;
>>> +
>>> +             /* Save values */
>>> +             st->core.calib[0] = st->core.resp->sensor_offset.offset[0];
>> Why save the value?  As far as I can tell it's not used anywhere?
>>> +
>>> +             *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;
>>> +
>>> +             ret = cros_ec_motion_send_host_cmd(&st->core, 0);
>>> +             if (ret < 0)
>>> +                     break;
>>> +
>>> +             val64 = st->core.resp->sensor_range.ret;
>>> +             *val = val64 >> 16;
>>> +             *val2 = (val64 & 0xffff) * 100;
>>> +             ret = IIO_VAL_INT_PLUS_MICRO;
>>> +             break;
>>> +     case IIO_CHAN_INFO_SCALE:
>>> +             /*
>>> +              * Light: Result in Lux, using calibration multiplier.
>> Hmm? I'm not following this.  The multiplier set here is 1 so 'using' it
>> is a strong way of putting it ;)
>>
>>> +              * Prox: Result in cm.
>> Ah, base unit should be in the simple SI unit.  Here m.  Just checked and the
>> docs don't actually state a unit for proximity.  Feel free to fix that as well.
>>
>>> +              */
>>> +             *val = 1;
>>> +             ret = IIO_VAL_INT;
>> No need to provide this if the scale is 1.  That means the results
>> are 'processed' so shouldn't be using the _raw form at all.
> 
> Sorry, I'm still confused with the _raw/_processed details. Just to
> make sure I understand the API, _raw should be only used for the raw
> values of the sensor and _processed if the value is already in the
> simple SI unit, right?
Yes. Generally if the value reported by the hardware is not already
in the relevant units and the conversion is linear then we do it
as raw with offset and scale.  If units happen to be right or the
conversion is complex then we just provide the processed form 
> 
> In this case the sensor returns lux for light and cm for proximity
> because the EC (where the sensors are connected) does some
> pre-processing. For light I must not use _raw and directly use
> _processed. For proximity, I must use _processed too, but convert cm
> to m.
> 
> What confused me is that  in acpi-als, the ALS sensor returns lux and
> it is using _raw and _processed (though I guess _raw is used only for
> backward compability)
Yeah, this was wrong in the initial driver, but we didn't pick up
on it until much later.  By then it was ABI so we had to leave
it there for any userspace code already relying on it
(no way of knowing what is out there!)
> 
> To sum up, if a sensor returns a known SI unit I must use always
> _processed, never _raw, right?
Almost.  For historical reasons (was a bad idea) we match units
with hwmon for some types of channel.  Some of these are not
in the basic SI unit, but rather for example in milli units
(mV) for voltage for example. 

Check the docs in Documentation/ABI/testing/sysfs-bus-iio* and
if it isn't specified, feel free to query and we'll fix it.

Where possible we stick to the raw SI unit.  So distances should be
in m.

Docs should state that though so please send a patch adding that if you
would like to.
> 
> 
>>> +             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_sensors_state *st = iio_priv(indio_dev);
>>> +     int ret;
>>> +     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;
>>> +             ret = cros_ec_motion_send_host_cmd(&st->core, 0);
>>> +             break;
>>> +     case IIO_CHAN_INFO_CALIBSCALE:
>>> +             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);
>>> +             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 ec_sensors_info = {
>>> +     .read_raw = &cros_ec_light_prox_read,
>>> +     .write_raw = &cros_ec_light_prox_write,
>>> +     .driver_module = THIS_MODULE,
>>> +};
>>> +
>>> +static int cros_ec_sensors_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_sensors_state *state;
>>> +     struct iio_chan_spec *channel;
>>> +     int ret;
>>> +
>>> +     if (!ec_dev || !ec_dev->ec_dev) {
>>> +             dev_warn(&pdev->dev, "No CROS EC device found.\n");
>>> +             return -EINVAL;
>>> +     }
>>> +     ec_device = ec_dev->ec_dev;
>>> +
>>> +     indio_dev = devm_iio_device_alloc(&pdev->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 = &ec_sensors_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_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->ext_info = cros_ec_sensors_ext_info;
>>> +
>>> +     /* 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(&pdev->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 = iio_triggered_buffer_setup(indio_dev, NULL,
>>> +                                      cros_ec_sensors_capture, NULL);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     ret = iio_device_register(indio_dev);
>>> +     if (ret)
>>> +             iio_triggered_buffer_cleanup(indio_dev);
>>> +
>>> +     return ret;
>>> +}
>>> +
>>> +static int cros_ec_sensors_remove(struct platform_device *pdev)
>>> +{
>>> +     struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>>> +
>>> +     iio_device_unregister(indio_dev);
>>> +     iio_triggered_buffer_cleanup(indio_dev);
>>
>> You could use the devm versions of register and triggered_buffer_setup
>> and drop this remove entirely.
>>
> 
> Ok, done in v2.
> 
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static const struct platform_device_id cros_ec_sensors_ids[] = {
>>> +     {
>>> +             .name = "cros-ec-prox",
>>> +     },
>>> +     {
>>> +             .name = "cros-ec-light",
>>> +     },
>>> +     { /* sentinel */ }
>>> +};
>>> +MODULE_DEVICE_TABLE(platform, cros_ec_sensors_ids);
>>> +
>>> +static struct platform_driver cros_ec_sensors_platform_driver = {
>>> +     .driver = {
>>> +             .name   = "cros-ec-light-prox",
>>> +     },
>>> +     .probe          = cros_ec_sensors_probe,
>>> +     .remove         = cros_ec_sensors_remove,
>>> +     .id_table       = cros_ec_sensors_ids,
>>> +};
>>> +module_platform_driver(cros_ec_sensors_platform_driver);
>>> +
>>> +MODULE_DESCRIPTION("ChromeOS EC light/proximity sensors driver");
>>> +MODULE_LICENSE("GPL v2");
>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>