Message-ID: <5396111B.6090109@kernel.org>
Date:	Mon, 09 Jun 2014 20:55:07 +0100
From:	Jonathan Cameron <jic23@...nel.org>
To:	Reyad Attiyat <reyad.attiyat@...il.com>,
	linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org,
	srinivas.pandruvada@...ux.intel.com, linux-input@...r.kernel.org,
	jkosina@...e.cz
Subject: Re: [PATCHv2 3/3] IIO: hid-sensor-magn-3d: Add in support for True/Magnetic
 North HID usages

On 03/06/14 00:14, Reyad Attiyat wrote:
> Updated magn_3d_channel enum for all possible north channels
>
> Added functions to setup iio_chan_spec array depending on which hid usage reports are found
>
> Renamed magn_val to iio_val to differentiate the index being used
>
> Updated magn_3d_state struct to hold pointer array (magn_val_addr[]) which points to iio_val[]
>
> Updated magn_3d_parse_report to scan for all compass usages and create iio channels for each
>
> Signed-off-by: Reyad Attiyat <reyad.attiyat@...il.com>
Hi Reyad,

I've taken a rather quick look at this.  Will probably want to take
one more close look at a newer version.

Various bits and bobs inline - mostly fine!
Jonathan
> ---
>   drivers/iio/magnetometer/hid-sensor-magn-3d.c | 271 +++++++++++++++++---------
>   1 file changed, 177 insertions(+), 94 deletions(-)
>
> diff --git a/drivers/iio/magnetometer/hid-sensor-magn-3d.c b/drivers/iio/magnetometer/hid-sensor-magn-3d.c
> index 6d162b7..32f4d90 100644
> --- a/drivers/iio/magnetometer/hid-sensor-magn-3d.c
> +++ b/drivers/iio/magnetometer/hid-sensor-magn-3d.c
> @@ -34,63 +34,54 @@ enum magn_3d_channel {
>   	CHANNEL_SCAN_INDEX_X,
>   	CHANNEL_SCAN_INDEX_Y,
>   	CHANNEL_SCAN_INDEX_Z,
> +	CHANNEL_SCAN_INDEX_NORTH_MAGN,
> +	CHANNEL_SCAN_INDEX_NORTH_TRUE,
> +	CHANNEL_SCAN_INDEX_NORTH_TILT_COMP,
> +	CHANNEL_SCAN_INDEX_NORTH_TRUE_TILT_COMP,
>   	MAGN_3D_CHANNEL_MAX,
>   };
>
> +#define IIO_CHANNEL_MAX MAGN_3D_CHANNEL_MAX
Please don't define a generic sounding name for a local
constant.  Why not use MAGN_3D_CHANNEL_MAX everywhere?
> +
>   struct magn_3d_state {
>   	struct hid_sensor_hub_callbacks callbacks;
>   	struct hid_sensor_common common_attributes;
>   	struct hid_sensor_hub_attribute_info magn[MAGN_3D_CHANNEL_MAX];
> -	u32 magn_val[MAGN_3D_CHANNEL_MAX];
> -};
> +	u32 *magn_val_addr[MAGN_3D_CHANNEL_MAX];
>
> -static const u32 magn_3d_addresses[MAGN_3D_CHANNEL_MAX] = {
> -	HID_USAGE_SENSOR_ORIENT_MAGN_FLUX_X_AXIS,
> -	HID_USAGE_SENSOR_ORIENT_MAGN_FLUX_Y_AXIS,
> -	HID_USAGE_SENSOR_ORIENT_MAGN_FLUX_Z_AXIS
> +	u32 iio_val[IIO_CHANNEL_MAX];
> +	int num_iio_channels;
>   };
>
> -/* Channel definitions */
> -static const struct iio_chan_spec magn_3d_channels[] = {
> -	{
> -		.type = IIO_MAGN,
> -		.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_MAGN,
> -		.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_MAGN,
> -		.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,
> +/* Find index into magn_3d_state magn[] and magn_val_addr[] from HID Usage  */
> +static int magn_3d_usage_id_to_chan_index(unsigned usage_id){
> +	int offset = -1;
I'd personally prefer offset = -EINVAL and to have it assigned as
a default element in the switch statement rather that here.
> +
> +	switch (usage_id) {
> +	case HID_USAGE_SENSOR_ORIENT_MAGN_FLUX_X_AXIS:
> +		offset = CHANNEL_SCAN_INDEX_X;
> +		break;
> +	case HID_USAGE_SENSOR_ORIENT_MAGN_FLUX_Y_AXIS:
> +		offset = CHANNEL_SCAN_INDEX_Y;
> +		break;
> +	case HID_USAGE_SENSOR_ORIENT_MAGN_FLUX_Z_AXIS:
> +		offset = CHANNEL_SCAN_INDEX_Z;
> +		break;
> +	case HID_USAGE_SENSOR_ORIENT_COMP_MAGN_NORTH:
> +		offset = CHANNEL_SCAN_INDEX_NORTH_TILT_COMP;
> +		break;
> +	case HID_USAGE_SENSOR_ORIENT_COMP_TRUE_NORTH:
> +		offset = CHANNEL_SCAN_INDEX_NORTH_TRUE_TILT_COMP;
> +		break;
> +	case HID_USAGE_SENSOR_ORIENT_MAGN_NORTH:
> +		offset = CHANNEL_SCAN_INDEX_NORTH_MAGN;
> +		break;
> +	case HID_USAGE_SENSOR_ORIENT_TRUE_NORTH:
> +		offset = CHANNEL_SCAN_INDEX_NORTH_TRUE;
> +		break;
>   	}
> -};
>
> -/* Adjust channel real bits based on report descriptor */
> -static void magn_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;
> +	return offset;
>   }
>
>   /* Channel read_raw handler */
> @@ -101,21 +92,31 @@ static int magn_3d_read_raw(struct iio_dev *indio_dev,
>   {
>   	struct magn_3d_state *magn_state = iio_priv(indio_dev);
>   	int report_id = -1;
> -	u32 address;
> +	unsigned usage_id;
> +	int chan_index = -1;
>   	int ret;
>   	int ret_type;
>
> +	dev_dbg(&indio_dev->dev, "magn_3d_read_raw\n");
> +
>   	*val = 0;
>   	*val2 = 0;
>   	switch (mask) {
>   	case 0:
> +		/* We store the HID usage ID of the iio channel
> +		 * in its address field
> +		 */
> +		usage_id = chan->address;
> +		chan_index = magn_3d_usage_id_to_chan_index(usage_id);

> +		if(chan_index < 0)
> +			return -EINVAL;
> +
>   		report_id =
> -			magn_state->magn[chan->scan_index].report_id;
> -		address = magn_3d_addresses[chan->scan_index];
> +			magn_state->magn[chan_index].report_id;
>   		if (report_id >= 0)
>   			*val = sensor_hub_input_attr_get_raw_value(
>   				magn_state->common_attributes.hsdev,
> -				HID_USAGE_SENSOR_COMPASS_3D, address,
> +				HID_USAGE_SENSOR_COMPASS_3D, usage_id,
>   				report_id);
>   		else {
>   			*val = 0;
> @@ -202,12 +203,13 @@ static int magn_3d_proc_event(struct hid_sensor_hub_device *hsdev,
>   				magn_state->common_attributes.data_ready);
>   	if (magn_state->common_attributes.data_ready)
>   		hid_sensor_push_data(indio_dev,
> -				magn_state->magn_val,
> -				sizeof(magn_state->magn_val));
> +				&(magn_state->iio_val),
> +				sizeof(magn_state->iio_val));
>
>   	return 0;
>   }
>
> +
>   /* Capture samples in local storage */
>   static int magn_3d_capture_sample(struct hid_sensor_hub_device *hsdev,
>   				unsigned usage_id,
> @@ -217,63 +219,143 @@ static int magn_3d_capture_sample(struct hid_sensor_hub_device *hsdev,
>   	struct iio_dev *indio_dev = platform_get_drvdata(priv);
>   	struct magn_3d_state *magn_state = iio_priv(indio_dev);
>   	int offset;
> +	u32 *magn_val;
>   	int ret = -EINVAL;
>
> -	switch (usage_id) {
> +	offset = magn_3d_usage_id_to_chan_index(usage_id);
> +	if(offset < 0)
> +		return ret;
> +
> +	magn_val = magn_state->magn_val_addr[offset];
> +	if(!magn_val)
> +		return ret;
> +
> +	*(magn_val) =  *(u32 *)raw_data;
> +
> +	return 0;
> +}
> +
> +/* Setup the iio_chan_spec for HID Usage ID */
> +static int magn_3d_setup_new_iio_chan(struct hid_sensor_hub_device *hsdev,
> +				unsigned usage_id,
> +				unsigned attr_usage_id,
> +				struct iio_chan_spec *iio_chans,
> +				struct magn_3d_state *st)
> +{
> +	int ret = -1;
This is kind of backwards to normal practice for this sort of function.
Better to maintain ret in the correct state at all times.  Hence
start with it being 0 and set to negative when there is an error rather
than the other way around.
> +	int iio_index;
> +	int magn_index;
> +	struct iio_chan_spec *channel;
> +
> +	/* Setup magn_3d_state for new channel */
> +	magn_index = magn_3d_usage_id_to_chan_index(attr_usage_id);
> +	if(magn_index < 0 || magn_index >= MAGN_3D_CHANNEL_MAX){
> +		return -1;
> +	}
> +
> +	/* Check if usage attribute exists in the sensor hub device */
> +	ret = sensor_hub_input_get_attribute_info(hsdev,
> +		HID_INPUT_REPORT,
> +		usage_id,
> +		attr_usage_id,
> +		&(st->magn[magn_index]));
> +	if(ret != 0){
> +		/* Usage not found. Nothing to setup.*/
> +		return 0;
> +	}
> +
> +	iio_index = st->num_iio_channels;
> +	if(iio_index < 0 || iio_index >= IIO_CHANNEL_MAX){
I'd be tempted to allocate space dynamically but I guess this works.

> +		return -2;
> +	}
> +
> +	/* Check if this usage hasn't been setup */
> +	if(st->magn_val_addr[magn_index] != NULL){
Space before that bracket here and elsewhere.
Don't return your own error codes. Use standard ones.  I guess thees
are left from debugging.
> +		return -3;
> +	}
> +	st->magn_val_addr[magn_index] = &(st->iio_val[iio_index]);
> +
> +	/* Setup IIO Channel spec */
> +	channel = &(iio_chans[iio_index]);
Just pass this is in as a pointer in the first place?
That is a pointer to the current channel location, that may or may
not be filled by this function.
> +
> +	channel->type = IIO_MAGN;
> +	channel->address = attr_usage_id;
> +	channel->modified = 1;
> +
> +	switch (attr_usage_id){
> +	case HID_USAGE_SENSOR_ORIENT_COMP_MAGN_NORTH:
> +		channel->channel2 = IIO_MOD_NORTH_MAGN_TILT_COMP;
> +		break;
> +	case HID_USAGE_SENSOR_ORIENT_COMP_TRUE_NORTH:
> +		channel->channel2 = IIO_MOD_NORTH_TRUE_TILT_COMP;
> +		break;
> +	case HID_USAGE_SENSOR_ORIENT_MAGN_NORTH:
> +		channel->channel2 = IIO_MOD_NORTH_MAGN;
> +		break;
> +	case HID_USAGE_SENSOR_ORIENT_TRUE_NORTH:
> +		channel->channel2 = IIO_MOD_NORTH_TRUE;
> +		break;
>   	case HID_USAGE_SENSOR_ORIENT_MAGN_FLUX_X_AXIS:
> +		channel->channel2 = IIO_MOD_X;
> +		break;
>   	case HID_USAGE_SENSOR_ORIENT_MAGN_FLUX_Y_AXIS:
> +		channel->channel2 = IIO_MOD_Y;
> +		break;
>   	case HID_USAGE_SENSOR_ORIENT_MAGN_FLUX_Z_AXIS:
> -		offset = usage_id - HID_USAGE_SENSOR_ORIENT_MAGN_FLUX_X_AXIS;
> -		magn_state->magn_val[CHANNEL_SCAN_INDEX_X + offset] =
> -						*(u32 *)raw_data;
> -		ret = 0;
> -	break;
> -	default:
> +		channel->channel2 = IIO_MOD_Z;
>   		break;
> +	default:
> +		return -4;
huh?  why -4. -EINVAL or -ENOSUP or something similar perhaps.
>   	}
>
> +	channel->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);
> +
> +	channel->scan_type.sign = 's';
> +	/* Real storage bits will change based on the report desc. */
> +	channel->scan_type.realbits = st->magn[magn_index].size * 8;
> +	/* Maximum size of a sample to capture is u32 */
> +	channel->scan_type.storagebits = sizeof(u32) * 8;
> +
I'd keen num_iio_channels outside this function and increment it if
this function does not return an error.
> +	(st->num_iio_channels)++;
Don't do this. ret should have been valid throughout... if not something
odd is going on with your use of ret.

Hmm. I see it is in the original code :( feel free to clean this up.
> +	ret = 0;
> +
>   	return ret;
>   }
>
> -/* Parse report which is specific to an usage id*/
> +/* Read the HID reports and setup IIO Channels */
>   static int magn_3d_parse_report(struct platform_device *pdev,
>   				struct hid_sensor_hub_device *hsdev,
> -				struct iio_chan_spec *channels,
> +				struct iio_chan_spec *iio_chans,
>   				unsigned usage_id,
>   				struct magn_3d_state *st)
>   {
> -	int ret;
> +	int ret = 0;
>   	int i;
>
> -	for (i = 0; i <= CHANNEL_SCAN_INDEX_Z; ++i) {
> -		ret = sensor_hub_input_get_attribute_info(hsdev,
> -				HID_INPUT_REPORT,
> -				usage_id,
> -				HID_USAGE_SENSOR_ORIENT_MAGN_FLUX_X_AXIS + i,
> -				&st->magn[CHANNEL_SCAN_INDEX_X + i]);
> -		if (ret < 0)
> -			break;
> -		magn_3d_adjust_channel_bit_mask(channels,
> -				CHANNEL_SCAN_INDEX_X + i,
> -				st->magn[CHANNEL_SCAN_INDEX_X + i].size);
> -	}
> -	dev_dbg(&pdev->dev, "magn_3d %x:%x, %x:%x, %x:%x\n",
> -			st->magn[0].index,
> -			st->magn[0].report_id,
> -			st->magn[1].index, st->magn[1].report_id,
> -			st->magn[2].index, st->magn[2].report_id);
> -
> -	/* Set Sensitivity field ids, when there is no individual modifier */
> -	if (st->common_attributes.sensitivity.index < 0) {
> -		sensor_hub_input_get_attribute_info(hsdev,
> -			HID_FEATURE_REPORT, usage_id,
> -			HID_USAGE_SENSOR_DATA_MOD_CHANGE_SENSITIVITY_ABS |
> -			HID_USAGE_SENSOR_DATA_ORIENTATION,
> -			&st->common_attributes.sensitivity);
> -		dev_dbg(&pdev->dev, "Sensitivity index:report %d:%d\n",
> -			st->common_attributes.sensitivity.index,
> -			st->common_attributes.sensitivity.report_id);
> -	}
> +	dev_dbg(&pdev->dev, "magn_3d_parse_reports Usage ID: %x\n", usage_id);
> +	for(i = HID_USAGE_SENSOR_ORIENT_MAGN_FLUX_X_AXIS;
> +	    i <= HID_USAGE_SENSOR_ORIENT_MAGN_FLUX_Z_AXIS && ret == 0;
> +		i++)
> +			ret = magn_3d_setup_new_iio_chan(hsdev,
> +					usage_id,
> +					i,
> +					iio_chans,
> +					st);
> +
> +	dev_dbg(&pdev->dev, "magn_3d_parse_reports Found %d\n magnetic axis. Returned: %d", st->num_iio_channels, ret);
> +	for(i = HID_USAGE_SENSOR_ORIENT_COMP_MAGN_NORTH;
> +	    i <= HID_USAGE_SENSOR_ORIENT_TRUE_NORTH && ret == 0;
> +		i++)
> +			ret = magn_3d_setup_new_iio_chan(hsdev,
> +					usage_id,
> +					i,
> +					iio_chans,
> +					st);
As stated above, I'd use whether this succeeds for a given channel to
increment the location in iio_chans and then pass in only the 'next' channel
location on each pass.  Moving the bounds checks out here as well would be
make for a cleaner magn_3d_setup_new_iio_chan function.

> +	dev_dbg(&pdev->dev, "magn_3d_parse_reports Found %d\n magnetic channels. Returned: %d", st->num_iio_channels, ret);
>
>   	return ret;
>   }
> @@ -307,10 +389,11 @@ static int hid_magn_3d_probe(struct platform_device *pdev)
>   		return ret;
>   	}
>
> -	channels = kmemdup(magn_3d_channels, sizeof(magn_3d_channels),
> -			   GFP_KERNEL);
> +	channels = kcalloc(MAGN_3D_CHANNEL_MAX,
> +			sizeof(struct iio_chan_spec),
> +			GFP_KERNEL);
>   	if (!channels) {
> -		dev_err(&pdev->dev, "failed to duplicate channels\n");
> +		dev_err(&pdev->dev, "failed to allocate memory for iio channel\n");
>   		return -ENOMEM;
>   	}
>
> @@ -322,7 +405,7 @@ static int hid_magn_3d_probe(struct platform_device *pdev)
>   	}
>
>   	indio_dev->channels = channels;
> -	indio_dev->num_channels = ARRAY_SIZE(magn_3d_channels);
> +	indio_dev->num_channels = magn_state->num_iio_channels;
With a bit of rearranging it strikes me that you can fill num_channels
directly rather than having another copy of it....
>   	indio_dev->dev.parent = &pdev->dev;
>   	indio_dev->info = &magn_3d_info;
>   	indio_dev->name = name;
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists