Re: [PATCH 3/3] iio: proximity: Add a ChromeOS EC MKBP proximity driver

[Gwendal Grignou]

On Mon, Jan 25, 2021 at 3:44 PM Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
>
> Quoting Gwendal Grignou (2021-01-25 14:28:46)
> > On Mon, Jan 25, 2021 at 10:52 AM Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
> > >
> > > Quoting Gwendal Grignou (2021-01-24 13:41:44)
> > > > On Sun, Jan 24, 2021 at 9:38 AM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> > > > >
> > > > > On Fri, 22 Jan 2021 14:54:43 -0800
> > > > > Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
> > > > > > +     if (event_type == EC_MKBP_EVENT_SWITCH) {
> > > > > > +             data = container_of(nb, struct cros_ec_proximity_data, notifier);
> > > > > > +             indio_dev = data->indio_dev;
> > > > > > +
> > > > > > +             mutex_lock(&data->lock);
> > > > > > +             if (data->enabled) {
> > > > > > +                     timestamp = iio_get_time_ns(indio_dev);
> > > > For Android, given the timestamp must be time it happens, not reported
> > > > [https://source.android.com/devices/sensors/sensors-hal2] """The
> > > > timestamp must be accurate and correspond to the time at which the
> > > > event physically happened, not the time it was reported.""", consider
> > > > using ec_dev->last_event_time and apply a delta if the iio clock base
> > > > is different from CLOCK_BOOTTIME.
> > >
> > > Ah alright. Is there a reason why cros_ec_get_time_ns() is using
> > > boottime instead of plain ktime_get(), i.e. CLOCK_MONOTONIC? Otherwise I
> > > suppose some sort of cros_ec API should be exposed to convert the
> > > last_event_time to whatever clock base is desired. Does that exist?
> > CLOCK_BOOTTIME was chosen to be Android compliant, as it includes
> > suspend time and match elapsedRealtime() [see
> > https://developer.android.com/reference/android/os/SystemClock#elapsedRealtime()]
> > Chromebook set iio clock reference for all sensor to CLOCK_BOOTTIME
> > (see https://chromium.googlesource.com/chromiumos/platform2/+/HEAD/mems_setup/configuration.cc#127).
> > In case the iio device clock_id is not CLOCK_BOOTTIME/"bootime", we
> > need to add a delta, like in cros_ec_sensors_push_data()
> > [https://elixir.bootlin.com/linux/latest/source/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c#L210]
>
> The delta may help but what if the clock is adjusted in the time between
> the event is timestamped and this driver reading the timestamp? That
> could lead to some odd behavior where the timestamp is in the future
> because we don't know what sort of adjustment was made, e.g. the
> realtime clock being moved back in time.
>
> I'd rather have a way to request that cros_ec core timestamp the event
> with some clock base that this driver desires,
ec_dev->last_event_time is collected outside of the IIO stack, and can
be used by multiple drivers.

> instead of having to do
> an offset after the fact. For now I'll use ec_dev->last_event_time
> because this is only used on chromeos and that should work until
> userspace is changed, but in the future I think we'll need to have a way
> for this IIO device to be notified when the clock base changes in
> iio_device_set_clock() and then have this driver call into cros_ec to
> request that such a timestamp be made when this event is seen. Or even
> better have a way to request that cros_ec timestamp the event itself on
> the EC side, but I don't know if that's possible.
One way would be use the EC sensor stack that collect such timestamp,
but that would be more firmware changes.

On second thought, to keep it simple and consistent with other IIO
drivers, I suggest to keep using iio_get_time_ns() when the sensor
clock is not CLOCK_BOOTTIME, ec_dev->last_event_time when it is.
>
> > > > > > +static const struct of_device_id cros_ec_proximity_of_match[] = {
> > > > > > +     { .compatible = "google,cros-ec-proximity" },
> > > > > > +     {}
> > > > > > +};
> > > > > > +MODULE_DEVICE_TABLE(of, cros_ec_proximity_of_match);
> > > > > > +#endif
> > > > > > +
> > > > > > +static struct platform_driver cros_ec_proximity_driver = {
> > > > > > +     .driver = {
> > > > > > +             .name = "cros-ec-proximity",
> > > > > > +             .of_match_table = of_match_ptr(cros_ec_proximity_of_match),
> > > > Add a ACPI match table to match.
> > >
> > > I don't have an ACPI system in hand. What should the ACPI table look
> > > like? Can ACPI use the of_match_table logic?
> > AFAIK, ACPI uses .acpi_match_table, see
> > drivers/iio/magnetometer/ak8975.c for a simple example.
>
> Ok. I'm leaning towards punting on this. I don't have an ACPI system to
> test and I don't know what the ACPI match table should have in it. If
> you can tell me what to put in the acpi_match_table then I can add it.