Re: [RFC 1/1] iio: common: cros_ec_sensors: add extra sensor API

[Gwendal Grignou <gwendal@chromium.org>]

On Mon, May 27, 2019 at 2:18 AM Jonathan Cameron
<jic23@jic23.retrosnub.co.uk> wrote:
>
> On Fri, 24 May 2019 17:28:42 -0700
> Gwendal Grignou <gwendal@chromium.org> wrote:
>
> > On Sun, May 5, 2019 at 8:36 AM Jonathan Cameron
> > <jic23@jic23.retrosnub.co.uk> wrote:
> > >
> > > On Fri,  3 May 2019 12:54:46 +0200
> > > Fabien Lahoudere <fabien.lahoudere@collabora.com> wrote:
> > >
> > > > From: Gwendal Grignou <gwendal@chromium.org>
> > > >
> > > > Version 3 of the EC protocol provides min and max frequencies and fifo
> > > > size for EC sensors.
> > > > The driver allows EC to set parameters for version 3 and set default values
> > > > for earlier versions.
> > > > The sysfs entries are read only and cannot be used to impact on values.
> > > >
> > > > Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> > > > Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.com>
> > > Mostly changes in here will follow from comments on the cover letter
> > > but a few other things inline.
> > >
> > > Thanks,
> > >
> > > Jonathan
> > > > ---
> > > >  .../ABI/testing/sysfs-bus-iio-cros-ec         |  24 ++++
> > > >  .../cros_ec_sensors/cros_ec_sensors_core.c    | 126 +++++++++++++++++-
> > > >  .../linux/iio/common/cros_ec_sensors_core.h   |   7 +
> > > >  include/linux/mfd/cros_ec_commands.h          |  21 +++
> > > >  4 files changed, 177 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-cros-ec b/Documentation/ABI/testing/sysfs-bus-iio-cros-ec
> > > > index 0e95c2ca105c..85d266f4a57e 100644
> > > > --- a/Documentation/ABI/testing/sysfs-bus-iio-cros-ec
> > > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-cros-ec
> > > > @@ -26,3 +26,27 @@ Description:
> > > >               driver and represents the sensor ID as exposed by the EC. This
> > > >               ID is used by the Android sensor service hardware abstraction
> > > >               layer (sensor HAL) through the Android container on ChromeOS.
> > > > +
> > > > +What:                /sys/bus/iio/devices/iio:deviceX/min_frequency
> > > > +Date:                April 2019
> > > > +KernelVersion:       5.0
> > > > +Contact:     linux-iio@vger.kernel.org
> > > > +Description:
> > > > +             This attribute is exposed by CrOS EC sensors. It defines the
> > > > +             minimum frequency used by the sensor.
> > > > +
> > > > +What:                /sys/bus/iio/devices/iio:deviceX/max_frequency
> > > > +Date:                April 2019
> > > > +KernelVersion:       5.0
> > > > +Contact:     linux-iio@vger.kernel.org
> > > > +Description:
> > > > +             This attribute is exposed by CrOS EC sensors. It defines the
> > > > +             maximum frequency used by the sensor.
> > > > +
> > > See reply to the cover letter on these.  Bonus points if you implement them
> > > with the read_avail callback.
> > >
> > > > +What:                /sys/bus/iio/devices/iio:deviceX/max_events
> > > > +Date:                April 2019
> > > > +KernelVersion:       5.0
> > > > +Contact:     linux-iio@vger.kernel.org
> > > > +Description:
> > > > +             This attribute is exposed by CrOS EC sensors. It defines the
> > > > +             maximum number of events in the fifo.
> > >
> > > Why does userspace care?
> > Is use to report the size of the hardware FIFO to Android, to enable
> > batching mode:
> > https://source.android.com/devices/sensors/batching.
> > It is reported in fifoReservedEventCount or fifoMaxEventCount in the
> > sensor_t structure described at
> > https://source.android.com/devices/sensors/hal-interface#sensor_t.
> > Having said that, batching is not terribly useful for tablet or
> > laptop, but is critical for wearable.
>
> Ok. So let me just check, in android terms, events are just readings
> from the sensor?
Yes [https://source.android.com/devices/sensors/hal-interface#sensors_event_t]
> These aren't events in the IIO sense of 'thresholds'
> being passed?
Correct.
>
> If so can we map this to hwfifo_watermark[_available] using the range
> descriptor? We have a bit of legacy around that with  hwfifo_watermark_max
> and hwfifo_watermark_min also defined, but should probably move towards
> the more standard _available form and deprecate the other two.
That make sense: hwfifo_watermark_available will be set to a single
value, which match hwfifo_watermak_max.
Cros EC has a limited support:
- the size can not be changed
- the EC will seldom fill the hwfifo, it fires interrupt to a hard
coded threshold to avoid losing samples
- the hwfifo is shared by all sensors
- sampling_frequency attribute is used to force the EC to generate
interrupt before the hwfifo is full.

Replacing max_events with hwfifo_watermark_max works for me.

Thanks,
Gwendal.
>
> Jonathan
>
>