Re: [PATCHv2 2/2] iio: cros_ec_light_prox: add ChromeOS EC Light and Proximity Sensors

[Gwendal Grignou]

On Fri, Jan 20, 2017 at 6:23 AM, Enric Balletbo Serra
<eballetbo@xxxxxxxxx> 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@xxxxxxxxxx>:
>> On 11/01/17 15:51, 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>
>> 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