lkml.org 
[lkml]   [2014]   [Jun]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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@gmail.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;
    >



    \
     
     \ /
      Last update: 2014-06-09 22:21    [W:5.631 / U:0.232 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site