From: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
To: Jonathan Cameron <jic23@kernel.org>,
"Ooi, Joyce" <joyce.ooi@intel.com>,
Jiri Kosina <jikos@kernel.org>
Cc: Hartmut Knaack <knaack.h@gmx.de>,
Lars-Peter Clausen <lars@metafoo.de>,
Peter Meerwald-Stadler <pmeerw@pmeerw.net>,
linux-input@vger.kernel.org, linux-iio@vger.kernel.org,
linux-kernel@vger.kernel.org,
Kweh Hock Leong <hock.leong.kweh@intel.com>,
Ong Boon Leong <boon.leong.ong@intel.com>,
Lay Kuan Loon <kuan.loon.lay@intel.com>
Subject: Re: [PATCH v2] iio: magnetometer: separate the values of attributes based on their usage type for HID compass sensor
Date: Sat, 19 Nov 2016 08:46:02 -0800 [thread overview]
Message-ID: <1479573962.31577.1.camel@linux.intel.com> (raw)
In-Reply-To: <491ee5bc-441e-119f-46f3-6f79798dc63c@kernel.org>
On Sat, 2016-11-19 at 12:56 +0000, Jonathan Cameron wrote:
> On 16/11/16 09:43, Ooi, Joyce wrote:
> >
> > There are 2 usage types (Magnetic Flux and Heading data field) for
> > HID
> > compass sensor, thus the values of offset, scale, and sensitivity
> > should
> > be separated according to their respective usage type. The changes
> > made
> > are as below:
> > 1. Hysteresis: A struct hid_sensor_common rot_attributes is created
> > in
> > struct magn_3d_state to contain the sensitivity for IIO_ROT.
> > 2. Scale: scale_pre_decml and scale_post_decml are separated for
> > IIO_MAGN
> > and IIO_ROT.
> > 3. Offset: Same as scale, value_offset is separated for IIO_MAGN
> > and
> > IIO_ROT.
> >
> > For sensitivity, HID_USAGE_SENSOR_ORIENT_MAGN_FLUX and
> > HID_USAGE_SENSOR_ORIENT_MAGN_HEADING are used for sensivitity
> > fields based
> > on the HID Sensor Usages specifications. Hence, these changes are
> > added on
> > the sensitivity field.
> >
> > Signed-off-by: Ooi, Joyce <joyce.ooi@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> I think I follow this one and it looks fine to me. However, I would
> like
> an Ack or review from Srinivas on this one.
>
> Thanks,
>
> Jonathan
> >
> > ---
> > changelog v2:
> > * rename struct hid_sensor_common common_attributes to struct
> > hid_sensor_common magn_flux_attributes.
> > * create a common_attributes struct which stores scale_pre_decml,
> > scale_post_decml, scale_precision, and value_offset.
> > * create struct hid_sensor_common magn_flux_attributes and
> > rot_attributes
> > for IIO_MAGN and IIO_ROT respectively.
> >
> > drivers/iio/magnetometer/hid-sensor-magn-3d.c | 147
> > ++++++++++++++++++++------
> > 1 file changed, 112 insertions(+), 35 deletions(-)
> >
> > diff --git a/drivers/iio/magnetometer/hid-sensor-magn-3d.c
> > b/drivers/iio/magnetometer/hid-sensor-magn-3d.c
> > index d8a0c8d..0e791b0 100644
> > --- a/drivers/iio/magnetometer/hid-sensor-magn-3d.c
> > +++ b/drivers/iio/magnetometer/hid-sensor-magn-3d.c
> > @@ -42,9 +42,17 @@ enum magn_3d_channel {
> > MAGN_3D_CHANNEL_MAX,
> > };
> >
> > +struct common_attributes {
> > + int scale_pre_decml;
> > + int scale_post_decml;
> > + int scale_precision;
> > + int value_offset;
> > +};
> > +
> > struct magn_3d_state {
> > struct hid_sensor_hub_callbacks callbacks;
> > - struct hid_sensor_common common_attributes;
> > + struct hid_sensor_common magn_flux_attributes;
> > + struct hid_sensor_common rot_attributes;
> > struct hid_sensor_hub_attribute_info
> > magn[MAGN_3D_CHANNEL_MAX];
> >
> > /* dynamically sized array to hold sensor values */
> > @@ -52,10 +60,8 @@ struct magn_3d_state {
> > /* array of pointers to sensor value */
> > u32 *magn_val_addr[MAGN_3D_CHANNEL_MAX];
> >
> > - int scale_pre_decml;
> > - int scale_post_decml;
> > - int scale_precision;
> > - int value_offset;
> > + struct common_attributes magn_flux_attr;
> > + struct common_attributes rot_attr;
> > };
> >
> > static const u32 magn_3d_addresses[MAGN_3D_CHANNEL_MAX] = {
> > @@ -162,41 +168,74 @@ static int magn_3d_read_raw(struct iio_dev
> > *indio_dev,
> > *val2 = 0;
> > switch (mask) {
> > case 0:
> > - hid_sensor_power_state(&magn_state-
> > >common_attributes, true);
> > + hid_sensor_power_state(&magn_state-
> > >magn_flux_attributes, true);
> > report_id =
> > magn_state->magn[chan->address].report_id;
> > address = magn_3d_addresses[chan->address];
> > if (report_id >= 0)
> > *val =
> > sensor_hub_input_attr_get_raw_value(
> > - magn_state-
> > >common_attributes.hsdev,
> > + magn_state-
> > >magn_flux_attributes.hsdev,
> > HID_USAGE_SENSOR_COMPASS_3D,
> > address,
> > report_id,
> > SENSOR_HUB_SYNC);
> > else {
> > *val = 0;
> > - hid_sensor_power_state(&magn_state-
> > >common_attributes,
> > - false);
> > + hid_sensor_power_state(
> > + &magn_state->magn_flux_attributes,
> > + false);
> > return -EINVAL;
> > }
> > - hid_sensor_power_state(&magn_state-
> > >common_attributes, false);
> > + hid_sensor_power_state(&magn_state-
> > >magn_flux_attributes,
> > + false);
> > ret_type = IIO_VAL_INT;
> > break;
> > case IIO_CHAN_INFO_SCALE:
> > - *val = magn_state->scale_pre_decml;
> > - *val2 = magn_state->scale_post_decml;
> > - ret_type = magn_state->scale_precision;
> > + switch (chan->type) {
> > + case IIO_MAGN:
> > + *val = magn_state-
> > >magn_flux_attr.scale_pre_decml;
> > + *val2 = magn_state-
> > >magn_flux_attr.scale_post_decml;
> > + ret_type = magn_state-
> > >magn_flux_attr.scale_precision;
> > + break;
> > + case IIO_ROT:
> > + *val = magn_state-
> > >rot_attr.scale_pre_decml;
> > + *val2 = magn_state-
> > >rot_attr.scale_post_decml;
> > + ret_type = magn_state-
> > >rot_attr.scale_precision;
> > + break;
> > + default:
> > + ret_type = -EINVAL;
> > + }
> > break;
> > case IIO_CHAN_INFO_OFFSET:
> > - *val = magn_state->value_offset;
> > - ret_type = IIO_VAL_INT;
> > + switch (chan->type) {
> > + case IIO_MAGN:
> > + *val = magn_state-
> > >magn_flux_attr.value_offset;
> > + ret_type = IIO_VAL_INT;
> > + break;
> > + case IIO_ROT:
> > + *val = magn_state->rot_attr.value_offset;
> > + ret_type = IIO_VAL_INT;
> > + break;
> > + default:
> > + ret_type = -EINVAL;
> > + }
> > break;
> > case IIO_CHAN_INFO_SAMP_FREQ:
> > ret_type = hid_sensor_read_samp_freq_value(
> > - &magn_state->common_attributes, val,
> > val2);
> > + &magn_state->magn_flux_attributes, val,
> > val2);
> > break;
> > case IIO_CHAN_INFO_HYSTERESIS:
> > - ret_type = hid_sensor_read_raw_hyst_value(
> > - &magn_state->common_attributes, val,
> > val2);
> > + switch (chan->type) {
> > + case IIO_MAGN:
> > + ret_type = hid_sensor_read_raw_hyst_value(
> > + &magn_state->magn_flux_attributes,
> > val, val2);
> > + break;
> > + case IIO_ROT:
> > + ret_type = hid_sensor_read_raw_hyst_value(
> > + &magn_state->rot_attributes, val,
> > val2);
> > + break;
> > + default:
> > + ret_type = -EINVAL;
> > + }
> > break;
> > default:
> > ret_type = -EINVAL;
> > @@ -219,11 +258,21 @@ static int magn_3d_write_raw(struct iio_dev
> > *indio_dev,
> > switch (mask) {
> > case IIO_CHAN_INFO_SAMP_FREQ:
> > ret = hid_sensor_write_samp_freq_value(
> > - &magn_state->common_attributes,
> > val, val2);
> > + &magn_state->magn_flux_attributes,
> > val, val2);
> > break;
> > case IIO_CHAN_INFO_HYSTERESIS:
> > - ret = hid_sensor_write_raw_hyst_value(
> > - &magn_state->common_attributes,
> > val, val2);
> > + switch (chan->type) {
> > + case IIO_MAGN:
> > + ret = hid_sensor_write_raw_hyst_value(
> > + &magn_state->magn_flux_attributes,
> > val, val2);
> > + break;
> > + case IIO_ROT:
> > + ret = hid_sensor_write_raw_hyst_value(
> > + &magn_state->rot_attributes, val,
> > val2);
> > + break;
> > + default:
> > + ret = -EINVAL;
> > + }
> > break;
> > default:
> > ret = -EINVAL;
> > @@ -254,7 +303,7 @@ static int magn_3d_proc_event(struct
> > hid_sensor_hub_device *hsdev,
> > struct magn_3d_state *magn_state = iio_priv(indio_dev);
> >
> > dev_dbg(&indio_dev->dev, "magn_3d_proc_event\n");
> > - if (atomic_read(&magn_state-
> > >common_attributes.data_ready))
> > + if (atomic_read(&magn_state-
> > >magn_flux_attributes.data_ready))
> > hid_sensor_push_data(indio_dev, magn_state-
> > >iio_vals);
> >
> > return 0;
> > @@ -389,21 +438,48 @@ static int magn_3d_parse_report(struct
> > platform_device *pdev,
> > dev_dbg(&pdev->dev, "magn_3d Setup %d IIO channels\n",
> > *chan_count);
> >
> > - st->scale_precision = hid_sensor_format_scale(
> > + st->magn_flux_attr.scale_precision =
> > hid_sensor_format_scale(
> > HID_USAGE_SENSOR_COMPASS_3D,
> > &st->magn[CHANNEL_SCAN_INDEX_X],
> > - &st->scale_pre_decml, &st-
> > >scale_post_decml);
> > + &st-
> > >magn_flux_attr.scale_pre_decml,
> > + &st-
> > >magn_flux_attr.scale_post_decml);
> > + st->rot_attr.scale_precision
> > + = hid_sensor_format_scale(
> > + HID_USAGE_SENSOR_ORIENT_COMP_MAGN_NORTH,
> > + &st-
> > >magn[CHANNEL_SCAN_INDEX_NORTH_MAGN_TILT_COMP],
> > + &st->rot_attr.scale_pre_decml,
> > + &st->rot_attr.scale_post_decml);
> >
> > /* Set Sensitivity field ids, when there is no individual
> > modifier */
> > - if (st->common_attributes.sensitivity.index < 0) {
> > + if (st->magn_flux_attributes.sensitivity.index < 0) {
> > sensor_hub_input_get_attribute_info(hsdev,
> > HID_FEATURE_REPORT, usage_id,
> > HID_USAGE_SENSOR_DATA_MOD_CHANGE_SENSITIVI
> > TY_ABS |
> > HID_USAGE_SENSOR_DATA_ORIENTATION,
> > - &st->common_attributes.sensitivity);
> > + &st->magn_flux_attributes.sensitivity);
> > + dev_dbg(&pdev->dev, "Sensitivity index:report
> > %d:%d\n",
> > + st-
> > >magn_flux_attributes.sensitivity.index,
> > + st-
> > >magn_flux_attributes.sensitivity.report_id);
> > + }
> > + if (st->magn_flux_attributes.sensitivity.index < 0) {
> > + sensor_hub_input_get_attribute_info(hsdev,
> > + HID_FEATURE_REPORT, usage_id,
> > + HID_USAGE_SENSOR_DATA_MOD_CHANGE_SENSITIVI
> > TY_ABS |
> > + HID_USAGE_SENSOR_ORIENT_MAGN_FLUX,
> > + &st->magn_flux_attributes.sensitivity);
> > + dev_dbg(&pdev->dev, "Sensitivity index:report
> > %d:%d\n",
> > + st-
> > >magn_flux_attributes.sensitivity.index,
> > + st-
> > >magn_flux_attributes.sensitivity.report_id);
> > + }
> > + if (st->rot_attributes.sensitivity.index < 0) {
> > + sensor_hub_input_get_attribute_info(hsdev,
> > + HID_FEATURE_REPORT, usage_id,
> > + HID_USAGE_SENSOR_DATA_MOD_CHANGE_SENSITIVI
> > TY_ABS |
> > + HID_USAGE_SENSOR_ORIENT_COMP_MAGN_NORTH,
> > + &st->rot_attributes.sensitivity);
> > dev_dbg(&pdev->dev, "Sensitivity index:report
> > %d:%d\n",
> > - st->common_attributes.sensitivity.index,
> > - st-
> > >common_attributes.sensitivity.report_id);
> > + st->rot_attributes.sensitivity.index,
> > + st->rot_attributes.sensitivity.report_id);
> > }
> >
> > return 0;
> > @@ -428,16 +504,17 @@ static int hid_magn_3d_probe(struct
> > platform_device *pdev)
> > platform_set_drvdata(pdev, indio_dev);
> >
> > magn_state = iio_priv(indio_dev);
> > - magn_state->common_attributes.hsdev = hsdev;
> > - magn_state->common_attributes.pdev = pdev;
> > + magn_state->magn_flux_attributes.hsdev = hsdev;
> > + magn_state->magn_flux_attributes.pdev = pdev;
> >
> > ret = hid_sensor_parse_common_attributes(hsdev,
> > HID_USAGE_SENSOR_COMPASS_3D,
> > - &magn_state->common_attributes);
> > + &magn_state-
> > >magn_flux_attributes);
> > if (ret) {
> > dev_err(&pdev->dev, "failed to setup common
> > attributes\n");
> > return ret;
> > }
> > + magn_state->rot_attributes = magn_state-
> > >magn_flux_attributes;
> >
> > ret = magn_3d_parse_report(pdev, hsdev,
> > &channels, &chan_count,
> > @@ -460,9 +537,9 @@ static int hid_magn_3d_probe(struct
> > platform_device *pdev)
> > dev_err(&pdev->dev, "failed to initialize trigger
> > buffer\n");
> > return ret;
> > }
> > - atomic_set(&magn_state->common_attributes.data_ready, 0);
> > + atomic_set(&magn_state->magn_flux_attributes.data_ready,
> > 0);
> > ret = hid_sensor_setup_trigger(indio_dev, name,
> > - &magn_state-
> > >common_attributes);
> > + &magn_state-
> > >magn_flux_attributes);
> > if (ret < 0) {
> > dev_err(&pdev->dev, "trigger setup failed\n");
> > goto error_unreg_buffer_funcs;
> > @@ -489,7 +566,7 @@ static int hid_magn_3d_probe(struct
> > platform_device *pdev)
> > error_iio_unreg:
> > iio_device_unregister(indio_dev);
> > error_remove_trigger:
> > - hid_sensor_remove_trigger(&magn_state->common_attributes);
> > + hid_sensor_remove_trigger(&magn_state-
> > >magn_flux_attributes);
> > error_unreg_buffer_funcs:
> > iio_triggered_buffer_cleanup(indio_dev);
> > return ret;
> > @@ -504,7 +581,7 @@ static int hid_magn_3d_remove(struct
> > platform_device *pdev)
> >
> > sensor_hub_remove_callback(hsdev,
> > HID_USAGE_SENSOR_COMPASS_3D);
> > iio_device_unregister(indio_dev);
> > - hid_sensor_remove_trigger(&magn_state->common_attributes);
> > + hid_sensor_remove_trigger(&magn_state-
> > >magn_flux_attributes);
> > iio_triggered_buffer_cleanup(indio_dev);
> >
> > return 0;
> >
>
next prev parent reply other threads:[~2016-11-19 16:46 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-11-16 9:43 [PATCH v2] iio: magnetometer: separate the values of attributes based on their usage type for HID compass sensor Ooi, Joyce
2016-11-19 12:56 ` Jonathan Cameron
2016-11-19 16:46 ` Srinivas Pandruvada [this message]
2016-11-24 20:43 ` Jonathan Cameron
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1479573962.31577.1.camel@linux.intel.com \
--to=srinivas.pandruvada@linux.intel.com \
--cc=boon.leong.ong@intel.com \
--cc=hock.leong.kweh@intel.com \
--cc=jic23@kernel.org \
--cc=jikos@kernel.org \
--cc=joyce.ooi@intel.com \
--cc=knaack.h@gmx.de \
--cc=kuan.loon.lay@intel.com \
--cc=lars@metafoo.de \
--cc=linux-iio@vger.kernel.org \
--cc=linux-input@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=pmeerw@pmeerw.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).