On 26/09/2018 4:09 PM, Song Qiang wrote:
On Wed, Sep 26, 2018 at 10:30:34AM +0800, Phil Reid wrote:
On 26/09/2018 9:49 AM, Song Qiang wrote:
On Tue, Sep 25, 2018 at 10:36:54PM +0800, Phil Reid wrote:
On 25/09/2018 9:30 PM, Jonathan Cameron wrote:
+static irqreturn_t rm3100_trigger_handler(int irq, void *p)
+{
+ struct iio_poll_func *pf = p;
+ struct iio_dev *indio_dev = pf->indio_dev;
+ struct rm3100_data *data = iio_priv(indio_dev);
+ struct regmap *regmap = data->regmap;
+ u8 buffer[9];
+ int ret;
+ int i;
+
+ mutex_lock(&data->lock);
+ ret = rm3100_wait_measurement(data);
+ if (ret < 0) {
+ mutex_unlock(&data->lock);
+ goto done;
+ }
+
+ ret = regmap_bulk_read(regmap, RM3100_REG_MX2, buffer, sizeof(buffer));
+ mutex_unlock(&data->lock);
+ if (ret < 0)
+ goto done;
+
+ /* Convert XXXYYYZZZxxx to XXXxYYYxZZZx. x for padding. */
+ for (i = 0; i < 3; i++)
+ memcpy(data->buffer + i * 4, buffer + i * 3, 3);
Firstly X doesn't need copying.
Secondly the copy of Y actually overwrites the value of Z
XXXYYYZZZxxx
XXXxYYYZZxxx
XXXxYYYxYZZx
I think...
+
+ iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
+ iio_get_time_ns(indio_dev));
memcpy target is a different buffer so should be ok.
But that raises the question of does it need to be?
'buffer' could be 12 bytes long and just shuffle Z then Y.
Do the unused bytes need to be zeroed? or does libiio mask them anyway?
--
Regards
Phil Reid
Hi Phil,
This is interesting, last patch I submitted uses three transactions and
shuffles X, Y and Z respectively. You said it should be better to use one
transactions, I thought it makes point, and one transaction may reduce
IO pressure of the i2c bus. :)
And that's not necessary for unused bytes to be zero. I'm not familiar
with libiio, actually just been studying it, can't say anything about
it.
yours,
Song Qiang
G'day Song,
yes the one transaction suggestion was to reduce pressure on the bus.
I think also with 3 calls you can up up with other devices taking over
the i2c / spi bus in between.
We've got a devkit for this part, but haven't got to wiring it up to our system
as yet.
We're looking at using the i2c interface which could push things at max
samplerate, so yes I'm
keen to see bus pressure reduced as much as possible.
I was thinking something like the following:
u8 buffer[12];
regmap_bulk_read(regmap, RM3100_REG_MX2, buffer, 9);
buffer[10] = buffer[8]; // or memcpy or some other fancy shuffle code.
buffer[9] = buffer[7];
buffer[8] = buffer[6];
buffer[6] = buffer[5];
buffer[5] = buffer[4];
buffer[4] = buffer[3];
iio_push_to_buffers_with_timestamp(indio_dev, buffer,
iio_get_time_ns(indio_dev));
but I'm unsure if this would be needed:
buffer[7] = 0
buffer[3] = 0
What you've got does the job I think.
I haven't dug into the datasheet in great detail, and my iio knownledge is
limited.
Are you sure the RM3100_CHANNEL scantype endianness is IIO_LE.
rm3100_read_mag looks to be doing big endian conversion and the datasheet
agrees with that.
--
Regards
Phil Reid
Hi Phil,
You're absolutely right!
This should be big endian, I think I probably just want something there
when I was writing this code, planned to change it later, but apparently
I've forgotten it...
AFAIK, filling places we do not need with 0 is not needed, we just
extract valid data from valid bit field(24 here).
Both one transaction and three transactions way have their point, but
this is a OS, probably the spiltted one is better, I need some real
thinking about this...
The one transaction is better.
Reduces i2c traffic by 6 bytes and ensure the measurements are tightly
coupled in time for the 3 axis.
I could have use the same buffer to read from the sensor and send it to
userspace like this:
int i = 0;
ret = regmap_bulk_read(regmap, RM3100_REG_MX2, 9);
if(ret)
...
/* Convert XXXYYYZZZxxx to XXXxYYYxZZZx. */
for (i = 0; i < 2; i++)
memcpy(buffer + (2 - i) * 4, buffer + (2 - i) * 3), 3);
This code snippet will use the same buffer, actually that's what I was
using the first time. Jonathan must thinks so, from what he commented,
he assumed I was using the same buffer, also what you want.
But I changed this due to Peter's comment, maybe not a big deal, he
suggests to use sizeof(buffer), this makes me use an additional size 9
buffer. I thought this doesn't matter too much, just some additional
space from the stack, but now I think maybe less memory using would be
better...
After all, this length 9 seems like never shouldn't be changed...
yes that does the job.
--
Regards
Phil Reid
