Re: [PATCH v2 7/9] iio: hid-sensors: Added Inclinometer 3D

[Jonathan Cameron <jic23@kernel.org>]

On 10/24/13 12:06, Jonathan Cameron wrote:
> On 10/23/13 20:11, Srinivas Pandruvada wrote:
>> Added usage id processing for Inclinometer 3D. This uses IIO
>> interfaces for triggered buffer to present data to user
>> mode.This uses HID sensor framework for registering callback
>> events from the sensor hub.
>>
>> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> Looks good.  One little comment inline... I'd probably not have mentioned that
> but as there is going to be another version fo the series, might as well tidy
> it up ;)

Just noticed another point to fix.  In read raw you have case 0:
We long ago defined IIO_CHAN_INFO_RAW to be 0 so please use the enum
rather than 0.

I'm guessing this might want some tidy up patches for any other remaining uses as well.

> 
> Thanks,
> 
>> +/* Channel definitions */
>> +static const struct iio_chan_spec incl_3d_channels[] = {
>> +	{
>> +		.type = IIO_INCLI,
>> +		.modified = 1,
>> +		.channel2 = IIO_MOD_X,
>> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OFFSET) |
>> +		BIT(IIO_CHAN_INFO_SCALE) |
>> +		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
>> +		BIT(IIO_CHAN_INFO_HYSTERESIS),
>> +		.scan_index = CHANNEL_SCAN_INDEX_X,
>> +	}, {
>> +		.type = IIO_INCLI,
>> +		.modified = 1,
>> +		.channel2 = IIO_MOD_Y,
>> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OFFSET) |
>> +		BIT(IIO_CHAN_INFO_SCALE) |
>> +		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
>> +		BIT(IIO_CHAN_INFO_HYSTERESIS),
>> +		.scan_index = CHANNEL_SCAN_INDEX_Y,
>> +	}, {
>> +		.type = IIO_INCLI,
>> +		.modified = 1,
>> +		.channel2 = IIO_MOD_Z,
>> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OFFSET) |
>> +		BIT(IIO_CHAN_INFO_SCALE) |
>> +		BIT(IIO_CHAN_INFO_SAMP_FREQ) |
>> +		BIT(IIO_CHAN_INFO_HYSTERESIS),
>> +		.scan_index = CHANNEL_SCAN_INDEX_Z,
>> +	}
>> +};
>> +
>> +/* Adjust channel real bits based on report 
> This would be cleaner if it just took the channel in question?
> so
> static void incl_3d_adjust_channel_bit_mask(struct iio_chan_spec *chan,
> 					    int size)
> {
> 	chan->scan_type.sign = 's';
> 	/* Real storage bits will change based on the report desc. */
> 	chan->scan_type.realbits = size * 8;
> 	/* Maximum size of a sample to capture is u32 */
> 	chan->scan_type.storagebits = sizeof(u32) * 8;
> }
> 
> then call as
> 
> 	incl_3d_adjust_channel_bit_mask(&channels[CHANNEL_SCAN_INDEX_X],
> 				st->incl[CHANNEL_SCAN_INDEX_X].size);
> 
>> +static void incl_3d_adjust_channel_bit_mask(struct iio_chan_spec *channels,
>> +						int channel, int size)
>> +{
>> +	channels[channel].scan_type.sign = 's';
>> +	/* Real storage bits will change based on the report desc. */
>> +	channels[channel].scan_type.realbits = size * 8;
>> +	/* Maximum size of a sample to capture is u32 */
>> +	channels[channel].scan_type.storagebits = sizeof(u32) * 8;
>> +}
>> +
> ...
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>