| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 23 Jan 2017 10:18:02 -0800
From: Gwendal Grignou <gwendal@...omium.org>
To: Enric Balletbo Serra <eballetbo@...il.com>
Cc: Jonathan Cameron <jic23@...nel.org>,
Enric Balletbo i Serra <enric.balletbo@...labora.com>,
linux-kernel <linux-kernel@...r.kernel.org>,
linux-iio@...r.kernel.org, Lars-Peter Clausen <lars@...afoo.de>,
Gwendal Grignou <gwendal@...omium.org>,
Guenter Roeck <groeck@...omium.org>
Subject: Re: [PATCHv2 2/2] iio: cros_ec_light_prox: add ChromeOS EC Light and
Proximity Sensors
On Fri, Jan 20, 2017 at 6:23 AM, Enric Balletbo Serra
<eballetbo@...il.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@...nel.org>:
>> On 11/01/17 15:51, Enric Balletbo i Serra wrote:
>>> From: Gwendal Grignou <gwendal@...omium.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@...omium.org>
>>> Signed-off-by: Guenter Roeck <groeck@...omium.org>
>>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@...labora.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
Powered by blists - more mailing lists