Re: [PATCH 2/2] iio: magnetometer: Add driver support for PNI RM3100

[Phil Reid <preid@electromag.com.au>]

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