Re: [PATCH v1 6/6] iio:magnetometer:ak8975: triggered buffer support

[Gregor Boirie <gregor.boirie@parrot.com>]

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@parrot.com>
>> ---
>>   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.