Re: [PATCHv2 3/3] IIO: hid-sensor-magn-3d: Add in support for True/Magnetic North HID usages

[Reyad Attiyat]

Hey Jonathan,

I will make the changes you suggested thanks for reviewing this. Any
conclusions on the naming, did you want me to change the sysfs name to
"in_rot_from_north_true/magnetic"?

On Mon, Jun 9, 2014 at 2:55 PM, Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> 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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/