Re: [PATCH v2 10/18] platform: chrome: sensorhub: Add FIFO support

From: Jonathan Cameron <jic23@kernel.org>
To: Gwendal Grignou <gwendal@chromium.org>
Cc: Brian Norris <briannorris@chromium.org>,
	Hartmut Knaack <knaack.h@gmx.de>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Peter Meerwald-Stadler <pmeerw@pmeerw.net>,
	Lee Jones <lee.jones@linaro.org>,
	Benson Leung <bleung@chromium.org>,
	Enric Balletbo i Serra <enric.balletbo@collabora.com>,
	Doug Anderson <dianders@chromium.org>,
	Guenter Roeck <groeck@chromium.org>,
	Fabien Lahoudere <fabien.lahoudere@collabora.com>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	linux-iio <linux-iio@vger.kernel.org>
Subject: Re: [PATCH v2 10/18] platform: chrome: sensorhub: Add FIFO support
Date: Tue, 22 Oct 2019 11:42:43 +0100	[thread overview]
Message-ID: <20191022114243.1eaf19c1@archlinux> (raw)
In-Reply-To: <CAPUE2uuaFQYXKhn+b6tW6tXWQECeix3pt3+KnMbyXgvx-Qoa1Q@mail.gmail.com>

...
> > > + * @sensor_hub : Sensor Hub object
> > > + */
> > > +int cros_ec_sensorhub_ring_add(struct cros_ec_sensorhub *sensorhub)
> > > +{
> > > +     struct cros_ec_dev *ec = sensorhub->ec;
> > > +     int ret;
> > > +
> > > +     /* Retrieve FIFO information */
> > > +     sensorhub->msg->version = 2;
> > > +     sensorhub->params->cmd = MOTIONSENSE_CMD_FIFO_INFO;
> > > +     sensorhub->msg->outsize = 1;
> > > +     sensorhub->msg->insize =
> > > +             sizeof(struct ec_response_motion_sense_fifo_info) +
> > > +             sizeof(u16) * CROS_EC_SENSOR_MAX;
> > > +
> > > +     ret = cros_ec_cmd_xfer_status(ec->ec_dev, sensorhub->msg);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +
> > > +     /*
> > > +      * Allocate the full fifo.
> > > +      * We need to copy the whole FIFO to set timestamps properly *
> > > +      */
> > > +     sensorhub->fifo_size = sensorhub->resp->fifo_info.size;
> > > +     sensorhub->ring = devm_kcalloc(sensorhub->dev, sensorhub->fifo_size,
> > > +                     sizeof(*sensorhub->ring), GFP_KERNEL);
> > > +     if (!sensorhub->ring)
> > > +             return -ENOMEM;
> > > +
> > > +     sensorhub->fifo_timestamp[CROS_EC_SENSOR_LAST_TS] =
> > > +             cros_ec_get_time_ns();  
> >
> > Hmm. Is the IIO standard timestamp selection attribute being exposed?
> > If so this is going to be confusing as we aren't obeying the selection
> > of clock from that..  
> 
> You're right, I did not find an elegant solution.
> cros_ec_get_time_ns() is an inline version of ktime_get_boottime_ns
> aka iio_get_time_ns( with indio_dev->clock_id set to  CLOCK_BOOTTIME).
> But I can not call iio_get_time_ns() here, as I don't have a
> indio_dev. Besides, sensors could have a different
> current_timestamp_clock attribute. I will convert in the callback
> routine (cros_ec_sensors_push_data() by adding an offset
> (iio_get_time_ns(indio_dev) - cros_ec_get_time_ns())
> >  

If you can special case that (perhaps adding some functionality to IIO)
to not do anything if both are CLOCK_BOOTTIME that would be great.

Thanks,

Jonathan