On 03/02/2016 12:36 PM, Peter Meerwald-Stadler wrote:
On Wed, 2 Mar 2016, Gregor Boirie wrote:
This will be used together with an external trigger (e.g hrtimer based
software trigger).
Samples of current acquisition round are cached in RAM in order to allow
simultaneous userspace access to buffer content and synchronous
"read_raw" API.
comments below
Signed-off-by: Gregor Boirie <gregor.boirie@xxxxxxxxxx>
---
drivers/iio/magnetometer/Kconfig | 2 +
drivers/iio/magnetometer/ak8975.c | 206 ++++++++++++++++++++++++++++++++------
2 files changed, 178 insertions(+), 30 deletions(-)
[snip...]
+
+ /* Clamp to valid range. */
+ *axis = clamp_t(s16, le16_to_cpu((s16)ret), -def->range, def->range);
this looks wrong; i2c_smbus_read_word_data() reads two bytes (on chip
in little endian) and converts them to a word (in cpu endianness), so
le16_to_cpu() in not necessary
so NAK on patch 3/6
oops... silly mistake !
+ return 0;
+}
+
+/*
+ * Retrieve raw flux value for one of the x, y, or z axis.
+ */
+static int ak8975_read_axis(struct iio_dev *indio_dev, struct ak8975_data *data,
+ int index, int *val)
+{
+ int ret;
+ s16 *axis = &data->buff[index];
+
+ mutex_lock(&data->lock);
+
+ if (!iio_buffer_enabled(indio_dev)) {
most drivers return -EBUSY when buffering is enabled and a single read is
performed
From a previous discussion here:
http://www.spinics.net/lists/linux-iio/msg22948.html
From my understanding I have 2 options here:
1) simply return EBUSY when buffering is enabled as you suggest ;
2) lock for exclusive bus access and serialize accesses between trigger
handler
and read_raw.
I can see no reason to prevent from concurrent access. I would prefere
getting
rid of this overly complex "caching" stuff and implement point 2).
Unless somebody states otherwise, I will follow your guidance here.
[snip...]
- *val2 = data->raw_to_gauss[chan->address];
+ *val2 = data->raw_to_gauss[chan->scan_index];
read_raw() is about reading without buffering, but scan_index relates to
buffering -- probably a reason to keep .address
ok.
[snip...]
+static int ak8975_fetch_all(const struct i2c_client *client,
+ struct ak8975_data *data)
+{
+ int ret;
+ s16 x, y, z;
I'd rather not use the per-device cache/buffer; if buffer mode is enable,
read_raw() simply returns -EBUSY
why not use have a local variable here and save the copying?
+
+ ret = ak8975_start_read_axis(data, client);
+ if (ret)
+ return ret;
+
+ /*
+ * For each axis, read the flux value from the appropriate register
+ * (the register is specified in the iio device attributes).
+ * Use a temporary storage to preserve 3D coordinate consistency.
+ */
it is rather inefficient to set up three separate i2c transactions; can't
the data be transferred in one go?
Agreed.
+ ret = ak8975_fetch_axis(client, data->def, 0, &x);
+ if (ret)
fetch axis does
+ return ret;
+ ret = ak8975_fetch_axis(client, data->def, 1, &y);
+ if (ret)
+ return ret;
+ ret = ak8975_fetch_axis(client, data->def, 2, &z);
+ if (ret)
+ return ret;
+
+ data->buff[0] = x;
+ data->buff[1] = y;
+ data->buff[2] = z;
+
+ return 0;
+}
+
+static int ak8975_buffer_postenable(struct iio_dev *indio_dev)
+{
+ struct ak8975_data *data = iio_priv(indio_dev);
+ int ret;
+
+ /*
+ * Update channels so that a client getting samples through read_raw
+ * retrieves valid data when buffering has just been enabled but first
+ * sampling round is not yet completed.
+ */
this function is not needed if -EBUSY is returned in read_raw when
buffering is enabled
see above.
[snip...]
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
