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

[Jonathan Cameron]

On June 10, 2014 6:33:25 PM GMT+01:00, Reyad Attiyat <reyad.attiyat@xxxxxxxxx> wrote:
>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"?
Yes. I think that is the cleanest option from those we have considered!
>
>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-iio" in
>the body of a message to majordomo@xxxxxxxxxxxxxxx
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.
--
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/