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@xxxxxxxxx>
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-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
