Re: [PATCH 08/10] iio: cros_ec_sensors_ring: add ChromeOS EC Sensors Ring

[Gwendal Grignou <gwendal@chromium.org>]

On Mon, Jul 18, 2016 at 9:27 AM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 18/07/16 08:02, Enric Balletbo i Serra wrote:
>> Add support for handling sensor events FIFO produced by the sensor
>> hub. A single device with a buffer will collect all samples produced
>> by the sensors managed by the CrosEC sensor hub.
> So you are defining a different device to support the buffer?
> That's 'unusual'...


>>
>> Signed-off-by: Guenter Roeck <groeck@chromium.org>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>> ---
>> +     CHANNEL_SENSOR_FLAG,
> Ah, I'm beginning to understand.
>
> I think you really need to demux these into the appropriate devices individual
> buffers... If these are coming in fairly randomly (guessing so?) then you'll
> want a top level mfd pushing data to each of the child devices (representing
> the different possible sensors).

Yes, indeed. I did not find a good solution to demux the hardware FIFO
into each sensor. As you said, I should have written a MFD driver
between the sensor device driver and the cros ec device driver.
But at the same time, that MFD driver, while not being an IIO driver,
would have to know the iio_dev of each sensor and call them with
iio_push_to_buffers(). Is it acceptable to call iio function from
outside the IIO subsystem?

>
> Simply pushing it into a buffer with metadata doesn't fit with the IIO ABI
> at all.
>
> Note that there is no obligation to have any triggers involved at all. Here I'd
> suggest you don't as it'll just make life difficult for little gain.
>

The way chrome OS works today is one process (chrome) is gathering
accelerometer sensor data on a time basis (using sysfs trigger), while
another "process" (android) is listening to all sensors events as they
come. I see that IIO has support for serveral buffer per iio_dev, but
I did not see this feature used. Do you suggest to creating virtual
sensors - not tied to physical hardware - to be able to set different
trigger, or calll iio_update_buffers() directly with new buffers?

>> +
>> +static s64 cros_ec_get_time_ns(void)
>> +{
>> +     struct timespec ts;
>> +
>> +     get_monotonic_boottime(&ts);
>> +     return timespec_to_ns(&ts);
> Use the core IIO timestamp helper? (now supports all the different clocks...)

>> +}
> See comments earlier.  There is no need to have metadata here that userspace
> will have to unwind, just demux the data appropriately and push to the
> correct iio devices (one per 'scan' - set of samples acquired at approximately
> the same time - every time)
>> +
>> +#define CROS_EC_RING_AXIS(_axis)             \
>> +{                                            \
>> +     .type = IIO_ACCEL,                      \
>> +     .modified = 1,                          \
>> +     .channel2 = IIO_MOD_##_axis,            \
>> +     .scan_index = CHANNEL_##_axis,          \
>> +     .scan_type = {                          \
>> +             .sign = 's',                    \
>> +             .realbits = 16,                 \
>> +             .storagebits = 16,              \
>> +     },                                      \
>> +     .extend_name = "ring",                  \
> For obvious reasons given earlier comments I don't like this!
I got your point. We will defer upstreaming this driver until we have
a better solution for sensors behind a muxed FIFO.

Gwendal.

>> +}