* [PATCH v4 0/4] Add support for rotation from north to the hid-sensor-magn-3d driver
@ 2014-06-30 2:58 Reyad Attiyat
2014-06-30 2:58 ` [PATCH v4 1/4] iio: Documentation: Add documentation for rotation from north sensor usage attributes Reyad Attiyat
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Reyad Attiyat @ 2014-06-30 2:58 UTC (permalink / raw)
To: linux-iio, linux-kernel, jic23, srinivas.pandruvada; +Cc: Reyad Attiyat
This version of patches tries to make smaller changes to the magn-3d driver. It
scans for usages present in the magn_3d_addresses array. It then allocates
memory for the IIO channel structs and value arrays that are used in the IIO subsystem.
It sets-up each IIO channel struct by copying the values from the static array of
IIO configurations. There is also an array used to store each channel IIO value
pointer for each HID usage attribute.
Reyad Attiyat (4):
iio: Documentation: Add documentation for rotation from north sensor
usage attributes
iio: types: Added support for rotation from north usage attributes
iio: hid-sensor-magn-3d: Scan for usage attributes before setting up
iio channels
iio: hid-sensor-magn-3d: Add support for rotation from north
Documentation/ABI/testing/sysfs-bus-iio | 82 +++++++++++++
drivers/iio/industrialio-core.c | 4 +
drivers/iio/magnetometer/hid-sensor-magn-3d.c | 167 ++++++++++++++++++++------
include/linux/iio/types.h | 4 +
4 files changed, 220 insertions(+), 37 deletions(-)
--
1.9.3
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v4 1/4] iio: Documentation: Add documentation for rotation from north sensor usage attributes
2014-06-30 2:58 [PATCH v4 0/4] Add support for rotation from north to the hid-sensor-magn-3d driver Reyad Attiyat
@ 2014-06-30 2:58 ` Reyad Attiyat
2014-07-07 16:45 ` Jonathan Cameron
2014-06-30 2:58 ` [PATCH v4 2/4] iio: types: Added support for rotation from north " Reyad Attiyat
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Reyad Attiyat @ 2014-06-30 2:58 UTC (permalink / raw)
To: linux-iio, linux-kernel, jic23, srinivas.pandruvada; +Cc: Reyad Attiyat
Added documentation for the sysfs attributes supported by the rotation from north
sensor.
Signed-off-by: Reyad Attiyat <reyad.attiyat@gmail.com>
---
Documentation/ABI/testing/sysfs-bus-iio | 82 +++++++++++++++++++++++++++++++++
1 file changed, 82 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index a9757dc..e88833d 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -260,6 +260,10 @@ What: /sys/bus/iio/devices/iio:deviceX/in_magn_scale
What: /sys/bus/iio/devices/iio:deviceX/in_magn_x_scale
What: /sys/bus/iio/devices/iio:deviceX/in_magn_y_scale
What: /sys/bus/iio/devices/iio:deviceX/in_magn_z_scale
+What: /sys/bus/iio/devices/iio:deviceX/in_rot_from_north_magnetic_scale
+What: /sys/bus/iio/devices/iio:deviceX/in_rot_from_north_true_scale
+What: /sys/bus/iio/devices/iio:deviceX/in_rot_from_north_magnetic_tilt_comp_scale
+What: /sys/bus/iio/devices/iio:deviceX/in_rot_from_north_true_tilt_comp_scale
What: /sys/bus/iio/devices/iio:deviceX/in_pressureY_scale
What: /sys/bus/iio/devices/iio:deviceX/in_pressure_scale
KernelVersion: 2.6.35
@@ -447,6 +451,14 @@ What: /sys/.../iio:deviceX/events/in_magn_y_thresh_rising_en
What: /sys/.../iio:deviceX/events/in_magn_y_thresh_falling_en
What: /sys/.../iio:deviceX/events/in_magn_z_thresh_rising_en
What: /sys/.../iio:deviceX/events/in_magn_z_thresh_falling_en
+What: /sys/.../iio:deviceX/events/in_rot_from_north_magnetic_thresh_rising_en
+What: /sys/.../iio:deviceX/events/in_rot_from_north_magnetic_thresh_falling_en
+What: /sys/.../iio:deviceX/events/in_rot_from_north_true_thresh_rising_en
+What: /sys/.../iio:deviceX/events/in_rot_from_north_true_thresh_falling_en
+What: /sys/.../iio:deviceX/events/in_rot_from_north_magnetic_tilt_comp_thresh_rising_en
+What: /sys/.../iio:deviceX/events/in_rot_from_north_magnetic_tilt_comp_thresh_falling_en
+What: /sys/.../iio:deviceX/events/in_rot_from_north_true_tilt_comp_thresh_rising_en
+What: /sys/.../iio:deviceX/events/in_rot_from_north_true_tilt_comp_thresh_falling_en
What: /sys/.../iio:deviceX/events/in_voltageY_supply_thresh_rising_en
What: /sys/.../iio:deviceX/events/in_voltageY_supply_thresh_falling_en
What: /sys/.../iio:deviceX/events/in_voltageY_thresh_rising_en
@@ -492,6 +504,14 @@ What: /sys/.../iio:deviceX/events/in_magn_y_roc_rising_en
What: /sys/.../iio:deviceX/events/in_magn_y_roc_falling_en
What: /sys/.../iio:deviceX/events/in_magn_z_roc_rising_en
What: /sys/.../iio:deviceX/events/in_magn_z_roc_falling_en
+What: /sys/.../iio:deviceX/events/in_rot_from_north_magnetic_roc_rising_en
+What: /sys/.../iio:deviceX/events/in_rot_from_north_magnetic_roc_falling_en
+What: /sys/.../iio:deviceX/events/in_rot_from_north_true_roc_rising_en
+What: /sys/.../iio:deviceX/events/in_rot_from_north_true_roc_falling_en
+What: /sys/.../iio:deviceX/events/in_rot_from_north_magnetic_tilt_comp_roc_rising_en
+What: /sys/.../iio:deviceX/events/in_rot_from_north_magnetic_tilt_comp_roc_falling_en
+What: /sys/.../iio:deviceX/events/in_rot_from_north_true_tilt_comp_roc_rising_en
+What: /sys/.../iio:deviceX/events/in_rot_from_north_true_tilt_comp_roc_falling_en
What: /sys/.../iio:deviceX/events/in_voltageY_supply_roc_rising_en
What: /sys/.../iio:deviceX/events/in_voltageY_supply_roc_falling_en
What: /sys/.../iio:deviceX/events/in_voltageY_roc_rising_en
@@ -538,6 +558,14 @@ What: /sys/.../events/in_magn_y_raw_thresh_rising_value
What: /sys/.../events/in_magn_y_raw_thresh_falling_value
What: /sys/.../events/in_magn_z_raw_thresh_rising_value
What: /sys/.../events/in_magn_z_raw_thresh_falling_value
+What: /sys/.../events/in_rot_from_north_magnetic_raw_thresh_rising_value
+What: /sys/.../events/in_rot_from_north_magnetic_raw_thresh_falling_value
+What: /sys/.../events/in_rot_from_north_true_raw_thresh_rising_value
+What: /sys/.../events/in_rot_from_north_true_raw_thresh_falling_value
+What: /sys/.../events/in_rot_from_north_magnetic_tilt_comp_raw_thresh_rising_value
+What: /sys/.../events/in_rot_from_north_magnetic_tilt_comp_raw_thresh_falling_value
+What: /sys/.../events/in_rot_from_north_true_tilt_comp_raw_thresh_rising_value
+What: /sys/.../events/in_rot_from_north_true_tilt_comp_raw_thresh_falling_value
What: /sys/.../events/in_voltageY_supply_raw_thresh_rising_value
What: /sys/.../events/in_voltageY_supply_raw_thresh_falling_value
What: /sys/.../events/in_voltageY_raw_thresh_rising_value
@@ -588,6 +616,18 @@ What: /sys/.../events/in_magn_y_thresh_either_hysteresis
What: /sys/.../events/in_magn_z_thresh_rising_hysteresis
What: /sys/.../events/in_magn_z_thresh_falling_hysteresis
What: /sys/.../events/in_magn_z_thresh_either_hysteresis
+What: /sys/.../events/in_rot_from_north_magnetic_thresh_rising_hysteresis
+What: /sys/.../events/in_rot_from_north_magnetic_thresh_falling_hysteresis
+What: /sys/.../events/in_rot_from_north_magnetic_thresh_either_hysteresis
+What: /sys/.../events/in_rot_from_north_true_thresh_rising_hysteresis
+What: /sys/.../events/in_rot_from_north_true_thresh_falling_hysteresis
+What: /sys/.../events/in_rot_from_north_true_thresh_either_hysteresis
+What: /sys/.../events/in_rot_from_north_magnetic_tilt_comp_thresh_rising_hysteresis
+What: /sys/.../events/in_rot_from_north_magnetic_tilt_comp_thresh_falling_hysteresis
+What: /sys/.../events/in_rot_from_north_magnetic_tilt_comp_thresh_either_hysteresis
+What: /sys/.../events/in_rot_from_north_true_tilt_comp_thresh_rising_hysteresis
+What: /sys/.../events/in_rot_from_north_true_tilt_comp_thresh_falling_hysteresis
+What: /sys/.../events/in_rot_from_north_true_tilt_comp_thresh_either_hysteresis
What: /sys/.../events/in_voltageY_thresh_rising_hysteresis
What: /sys/.../events/in_voltageY_thresh_falling_hysteresis
What: /sys/.../events/in_voltageY_thresh_either_hysteresis
@@ -635,6 +675,14 @@ What: /sys/.../events/in_magn_y_raw_roc_rising_value
What: /sys/.../events/in_magn_y_raw_roc_falling_value
What: /sys/.../events/in_magn_z_raw_roc_rising_value
What: /sys/.../events/in_magn_z_raw_roc_falling_value
+What: /sys/.../events/in_rot_from_north_magnetic_raw_roc_rising_value
+What: /sys/.../events/in_rot_from_north_magnetic_raw_roc_falling_value
+What: /sys/.../events/in_rot_from_north_true_raw_roc_rising_value
+What: /sys/.../events/in_rot_from_north_true_raw_roc_falling_value
+What: /sys/.../events/in_rot_from_north_magnetic_tilt_comp_raw_roc_rising_value
+What: /sys/.../events/in_rot_from_north_magnetic_tilt_comp_raw_roc_falling_value
+What: /sys/.../events/in_rot_from_north_true_tilt_comp_raw_roc_rising_value
+What: /sys/.../events/in_rot_from_north_true_tilt_comp_raw_roc_falling_value
What: /sys/.../events/in_voltageY_supply_raw_roc_rising_value
What: /sys/.../events/in_voltageY_supply_raw_roc_falling_value
What: /sys/.../events/in_voltageY_raw_roc_rising_value
@@ -690,6 +738,22 @@ What: /sys/.../events/in_magn_z_thresh_rising_period
What: /sys/.../events/in_magn_z_thresh_falling_period
What: /sys/.../events/in_magn_z_roc_rising_period
What: /sys/.../events/in_magn_z_roc_falling_period
+What: /sys/.../events/in_rot_from_north_magnetic_thresh_rising_period
+What: /sys/.../events/in_rot_from_north_magnetic_thresh_falling_period
+What: /sys/.../events/in_rot_from_north_magnetic_roc_rising_period
+What: /sys/.../events/in_rot_from_north_magnetic_roc_falling_period
+What: /sys/.../events/in_rot_from_north_true_thresh_rising_period
+What: /sys/.../events/in_rot_from_north_true_thresh_falling_period
+What: /sys/.../events/in_rot_from_north_true_roc_rising_period
+What: /sys/.../events/in_rot_from_north_true_roc_falling_period
+What: /sys/.../events/in_rot_from_north_magnetic_tilt_comp_thresh_rising_period
+What: /sys/.../events/in_rot_from_north_magnetic_tilt_comp_thresh_falling_period
+What: /sys/.../events/in_rot_from_north_magnetic_tilt_comp_roc_rising_period
+What: /sys/.../events/in_rot_from_north_magnetic_tilt_comp_roc_falling_period
+What: /sys/.../events/in_rot_from_north_true_tilt_comp_thresh_rising_period
+What: /sys/.../events/in_rot_from_north_true_tilt_comp_thresh_falling_period
+What: /sys/.../events/in_rot_from_north_true_tilt_comp_roc_rising_period
+What: /sys/.../events/in_rot_from_north_true_tilt_comp_roc_falling_period
What: /sys/.../events/in_voltageY_supply_thresh_rising_period
What: /sys/.../events/in_voltageY_supply_thresh_falling_period
What: /sys/.../events/in_voltageY_supply_roc_rising_period
@@ -787,6 +851,10 @@ What: /sys/.../iio:deviceX/scan_elements/in_anglvel_z_en
What: /sys/.../iio:deviceX/scan_elements/in_magn_x_en
What: /sys/.../iio:deviceX/scan_elements/in_magn_y_en
What: /sys/.../iio:deviceX/scan_elements/in_magn_z_en
+What: /sys/.../iio:deviceX/scan_elements/in_rot_from_north_magnetic_en
+What: /sys/.../iio:deviceX/scan_elements/in_rot_from_north_true_en
+What: /sys/.../iio:deviceX/scan_elements/in_rot_from_north_magnetic_tilt_comp_en
+What: /sys/.../iio:deviceX/scan_elements/in_rot_from_north_true_tilt_comp_en
What: /sys/.../iio:deviceX/scan_elements/in_timestamp_en
What: /sys/.../iio:deviceX/scan_elements/in_voltageY_supply_en
What: /sys/.../iio:deviceX/scan_elements/in_voltageY_en
@@ -853,6 +921,10 @@ What: /sys/.../iio:deviceX/scan_elements/in_anglvel_z_index
What: /sys/.../iio:deviceX/scan_elements/in_magn_x_index
What: /sys/.../iio:deviceX/scan_elements/in_magn_y_index
What: /sys/.../iio:deviceX/scan_elements/in_magn_z_index
+What: /sys/.../iio:deviceX/scan_elements/in_rot_from_north_magnetic_index
+What: /sys/.../iio:deviceX/scan_elements/in_rot_from_north_true_index
+What: /sys/.../iio:deviceX/scan_elements/in_rot_from_north_magnetic_tilt_comp_index
+What: /sys/.../iio:deviceX/scan_elements/in_rot_from_north_true_tilt_comp_index
What: /sys/.../iio:deviceX/scan_elements/in_incli_x_index
What: /sys/.../iio:deviceX/scan_elements/in_incli_y_index
What: /sys/.../iio:deviceX/scan_elements/in_timestamp_index
@@ -933,3 +1005,13 @@ Description:
x y z w. Here x, y, and z component represents the axis about
which a rotation will occur and w component represents the
amount of rotation.
+
+What: /sys/bus/iio/devices/iio:deviceX/in_rot_from_north_magnetic_tilt_comp_raw
+What: /sys/bus/iio/devices/iio:deviceX/in_rot_from_north_true_tilt_comp_raw
+What: /sys/bus/iio/devices/iio:deviceX/in_rot_from_north_magnetic_raw
+What: /sys/bus/iio/devices/iio:deviceX/in_rot_from_north_true_raw
+KernelVersion: 3.15
+Contact: linux-iio@vger.kernel.org
+Description:
+ Raw value of rotation from true/magnetic north measured with
+ or without compensation from tilt sensors.
--
1.9.3
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v4 2/4] iio: types: Added support for rotation from north usage attributes
2014-06-30 2:58 [PATCH v4 0/4] Add support for rotation from north to the hid-sensor-magn-3d driver Reyad Attiyat
2014-06-30 2:58 ` [PATCH v4 1/4] iio: Documentation: Add documentation for rotation from north sensor usage attributes Reyad Attiyat
@ 2014-06-30 2:58 ` Reyad Attiyat
2014-06-30 2:58 ` [PATCH v4 3/4] iio: hid-sensor-magn-3d: Scan for usage attributes before setting up iio channels Reyad Attiyat
2014-06-30 2:58 ` [PATCH v4 4/4] iio: hid-sensor-magn-3d: Add support for rotation from north Reyad Attiyat
3 siblings, 0 replies; 11+ messages in thread
From: Reyad Attiyat @ 2014-06-30 2:58 UTC (permalink / raw)
To: linux-iio, linux-kernel, jic23, srinivas.pandruvada; +Cc: Reyad Attiyat
Added the rotation from north usage attributes to the iio modifier enum and to the iio modifier names array.
Signed-off-by: Reyad Attiyat <reyad.attiyat@gmail.com>
---
drivers/iio/industrialio-core.c | 4 ++++
include/linux/iio/types.h | 4 ++++
2 files changed, 8 insertions(+)
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 4b1f375..af3e76d 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -87,6 +87,10 @@ static const char * const iio_modifier_names[] = {
[IIO_MOD_QUATERNION] = "quaternion",
[IIO_MOD_TEMP_AMBIENT] = "ambient",
[IIO_MOD_TEMP_OBJECT] = "object",
+ [IIO_MOD_NORTH_MAGN] = "from_north_magnetic",
+ [IIO_MOD_NORTH_TRUE] = "from_north_true",
+ [IIO_MOD_NORTH_MAGN_TILT_COMP] = "from_north_magnetic_tilt_comp",
+ [IIO_MOD_NORTH_TRUE_TILT_COMP] = "from_north_true_tilt_comp",
};
/* relies on pairs of these shared then separate */
diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
index d480631..d09c40d 100644
--- a/include/linux/iio/types.h
+++ b/include/linux/iio/types.h
@@ -56,6 +56,10 @@ enum iio_modifier {
IIO_MOD_QUATERNION,
IIO_MOD_TEMP_AMBIENT,
IIO_MOD_TEMP_OBJECT,
+ IIO_MOD_NORTH_MAGN,
+ IIO_MOD_NORTH_TRUE,
+ IIO_MOD_NORTH_MAGN_TILT_COMP,
+ IIO_MOD_NORTH_TRUE_TILT_COMP
};
enum iio_event_type {
--
1.9.3
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v4 3/4] iio: hid-sensor-magn-3d: Scan for usage attributes before setting up iio channels
2014-06-30 2:58 [PATCH v4 0/4] Add support for rotation from north to the hid-sensor-magn-3d driver Reyad Attiyat
2014-06-30 2:58 ` [PATCH v4 1/4] iio: Documentation: Add documentation for rotation from north sensor usage attributes Reyad Attiyat
2014-06-30 2:58 ` [PATCH v4 2/4] iio: types: Added support for rotation from north " Reyad Attiyat
@ 2014-06-30 2:58 ` Reyad Attiyat
2014-07-07 16:44 ` Jonathan Cameron
2014-06-30 2:58 ` [PATCH v4 4/4] iio: hid-sensor-magn-3d: Add support for rotation from north Reyad Attiyat
3 siblings, 1 reply; 11+ messages in thread
From: Reyad Attiyat @ 2014-06-30 2:58 UTC (permalink / raw)
To: linux-iio, linux-kernel, jic23, srinivas.pandruvada; +Cc: Reyad Attiyat
Scan for and count the HID usage attributes supported by the driver.
This allows for the driver to only setup the IIO channels for the
sensor usages present in the HID USB reports.
Signed-off-by: Reyad Attiyat <reyad.attiyat@gmail.com>
---
drivers/iio/magnetometer/hid-sensor-magn-3d.c | 111 +++++++++++++++++---------
1 file changed, 75 insertions(+), 36 deletions(-)
diff --git a/drivers/iio/magnetometer/hid-sensor-magn-3d.c b/drivers/iio/magnetometer/hid-sensor-magn-3d.c
index 41cf29e..070d20e 100644
--- a/drivers/iio/magnetometer/hid-sensor-magn-3d.c
+++ b/drivers/iio/magnetometer/hid-sensor-magn-3d.c
@@ -42,7 +42,8 @@ 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];
+ u32 *iio_val;
int scale_pre_decml;
int scale_post_decml;
int scale_precision;
@@ -221,8 +222,8 @@ static int magn_3d_proc_event(struct hid_sensor_hub_device *hsdev,
dev_dbg(&indio_dev->dev, "magn_3d_proc_event\n");
if (atomic_read(&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;
}
@@ -236,52 +237,99 @@ 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;
- int ret = -EINVAL;
+ int ret = 0;
+ u32 *iio_val = NULL;
switch (usage_id) {
case HID_USAGE_SENSOR_ORIENT_MAGN_FLUX_X_AXIS:
case HID_USAGE_SENSOR_ORIENT_MAGN_FLUX_Y_AXIS:
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;
+ iio_val = magn_state->magn_val_addr[CHANNEL_SCAN_INDEX_X + offset];
+
break;
default:
- break;
+ return -EINVAL;
}
+ if(iio_val)
+ *iio_val = *(u32 *)raw_data;
+ else
+ ret = -EINVAL;
+
return ret;
}
/* Parse report which is specific to an usage id*/
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 **channels,
+ int *chan_count,
unsigned usage_id,
struct magn_3d_state *st)
{
- int ret;
+ int ret = 0;
int i;
+ int attr_count = 0;
+
+ /* Scan for each usage attribute supported */
+ for (i = 0; i < MAGN_3D_CHANNEL_MAX; i++) {
+ u32 address = magn_3d_addresses[i];
- for (i = 0; i <= CHANNEL_SCAN_INDEX_Z; ++i) {
+ /* Check if usage attribute exists in the sensor hub device */
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);
+ HID_INPUT_REPORT,
+ usage_id,
+ address,
+ &(st->magn[i]));
+ if (!ret)
+ attr_count++;
}
- dev_dbg(&pdev->dev, "magn_3d %x:%x, %x:%x, %x:%x\n",
+
+ dev_dbg(&pdev->dev, "magn_3d Found %d usage attributes\n",
+ attr_count);
+ dev_dbg(&pdev->dev, "magn_3d X: %x:%x Y: %x:%x Z: %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);
+ if (attr_count > 0)
+ ret = 0;
+ else
+ return -EINVAL;
+
+ /* Setup IIO channel array */
+ *channels = devm_kcalloc(&pdev->dev, attr_count,
+ sizeof(struct iio_chan_spec),
+ GFP_KERNEL);
+ if (!*channels) {
+ dev_err(&pdev->dev, "failed to allocate space for iio channels\n");
+ return -ENOMEM;
+ }
+
+ st->iio_val = devm_kcalloc(&pdev->dev, attr_count,
+ sizeof(u32),
+ GFP_KERNEL);
+ if (!st->iio_val) {
+ dev_err(&pdev->dev, "failed to allocate space for iio values array\n");
+ return -ENOMEM;
+ }
+
+ for (i = 0, *chan_count = 0;
+ i < MAGN_3D_CHANNEL_MAX && *chan_count < attr_count;
+ i++)
+ {
+ if (st->magn[i].index >= 0) {
+ /* Setup IIO channel struct */
+ *channels[*chan_count] = magn_3d_channels[i];
+
+ st->magn_val_addr[i] = &(st->iio_val[*chan_count]);
+ magn_3d_adjust_channel_bit_mask(*channels, *chan_count, st->magn[i].size);
+ (*chan_count)++;
+ }
+ }
+
st->scale_precision = hid_sensor_format_scale(
HID_USAGE_SENSOR_COMPASS_3D,
&st->magn[CHANNEL_SCAN_INDEX_X],
@@ -311,6 +359,7 @@ static int hid_magn_3d_probe(struct platform_device *pdev)
struct magn_3d_state *magn_state;
struct hid_sensor_hub_device *hsdev = pdev->dev.platform_data;
struct iio_chan_spec *channels;
+ int chan_count = 0;
indio_dev = devm_iio_device_alloc(&pdev->dev,
sizeof(struct magn_3d_state));
@@ -331,22 +380,15 @@ static int hid_magn_3d_probe(struct platform_device *pdev)
return ret;
}
- channels = kmemdup(magn_3d_channels, sizeof(magn_3d_channels),
- GFP_KERNEL);
- if (!channels) {
- dev_err(&pdev->dev, "failed to duplicate channels\n");
- return -ENOMEM;
- }
-
- ret = magn_3d_parse_report(pdev, hsdev, channels,
- HID_USAGE_SENSOR_COMPASS_3D, magn_state);
+ ret = magn_3d_parse_report(pdev, hsdev, &channels,
+ &chan_count, HID_USAGE_SENSOR_COMPASS_3D, magn_state);
if (ret) {
dev_err(&pdev->dev, "failed to setup attributes\n");
- goto error_free_dev_mem;
+ return ret;
}
indio_dev->channels = channels;
- indio_dev->num_channels = ARRAY_SIZE(magn_3d_channels);
+ indio_dev->num_channels = chan_count;
indio_dev->dev.parent = &pdev->dev;
indio_dev->info = &magn_3d_info;
indio_dev->name = name;
@@ -356,7 +398,7 @@ static int hid_magn_3d_probe(struct platform_device *pdev)
NULL, NULL);
if (ret) {
dev_err(&pdev->dev, "failed to initialize trigger buffer\n");
- goto error_free_dev_mem;
+ return ret;
}
atomic_set(&magn_state->common_attributes.data_ready, 0);
ret = hid_sensor_setup_trigger(indio_dev, name,
@@ -390,8 +432,6 @@ error_remove_trigger:
hid_sensor_remove_trigger(&magn_state->common_attributes);
error_unreg_buffer_funcs:
iio_triggered_buffer_cleanup(indio_dev);
-error_free_dev_mem:
- kfree(indio_dev->channels);
return ret;
}
@@ -406,7 +446,6 @@ static int hid_magn_3d_remove(struct platform_device *pdev)
iio_device_unregister(indio_dev);
hid_sensor_remove_trigger(&magn_state->common_attributes);
iio_triggered_buffer_cleanup(indio_dev);
- kfree(indio_dev->channels);
return 0;
}
--
1.9.3
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v4 4/4] iio: hid-sensor-magn-3d: Add support for rotation from north
2014-06-30 2:58 [PATCH v4 0/4] Add support for rotation from north to the hid-sensor-magn-3d driver Reyad Attiyat
` (2 preceding siblings ...)
2014-06-30 2:58 ` [PATCH v4 3/4] iio: hid-sensor-magn-3d: Scan for usage attributes before setting up iio channels Reyad Attiyat
@ 2014-06-30 2:58 ` Reyad Attiyat
3 siblings, 0 replies; 11+ messages in thread
From: Reyad Attiyat @ 2014-06-30 2:58 UTC (permalink / raw)
To: linux-iio, linux-kernel, jic23, srinivas.pandruvada; +Cc: Reyad Attiyat
Add the HID usage attribute ID's and IIO channel info for rotation
from north support.
Signed-off-by: Reyad Attiyat <reyad.attiyat@gmail.com>
---
drivers/iio/magnetometer/hid-sensor-magn-3d.c | 58 ++++++++++++++++++++++++++-
1 file changed, 57 insertions(+), 1 deletion(-)
diff --git a/drivers/iio/magnetometer/hid-sensor-magn-3d.c b/drivers/iio/magnetometer/hid-sensor-magn-3d.c
index 070d20e..69f8ac9 100644
--- a/drivers/iio/magnetometer/hid-sensor-magn-3d.c
+++ b/drivers/iio/magnetometer/hid-sensor-magn-3d.c
@@ -35,6 +35,10 @@ enum magn_3d_channel {
CHANNEL_SCAN_INDEX_X,
CHANNEL_SCAN_INDEX_Y,
CHANNEL_SCAN_INDEX_Z,
+ CHANNEL_SCAN_INDEX_NORTH_MAGN_TILT_COMP,
+ CHANNEL_SCAN_INDEX_NORTH_TRUE_TILT_COMP,
+ CHANNEL_SCAN_INDEX_NORTH_MAGN,
+ CHANNEL_SCAN_INDEX_NORTH_TRUE,
MAGN_3D_CHANNEL_MAX,
};
@@ -53,7 +57,11 @@ struct magn_3d_state {
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
+ HID_USAGE_SENSOR_ORIENT_MAGN_FLUX_Z_AXIS,
+ HID_USAGE_SENSOR_ORIENT_COMP_MAGN_NORTH,
+ HID_USAGE_SENSOR_ORIENT_COMP_TRUE_NORTH,
+ HID_USAGE_SENSOR_ORIENT_MAGN_NORTH,
+ HID_USAGE_SENSOR_ORIENT_TRUE_NORTH,
};
/* Channel definitions */
@@ -88,6 +96,46 @@ static const struct iio_chan_spec magn_3d_channels[] = {
BIT(IIO_CHAN_INFO_SAMP_FREQ) |
BIT(IIO_CHAN_INFO_HYSTERESIS),
.scan_index = CHANNEL_SCAN_INDEX_Z,
+ }, {
+ .type = IIO_ROT,
+ .modified = 1,
+ .channel2 = IIO_MOD_NORTH_MAGN_TILT_COMP,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+ .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_NORTH_MAGN_TILT_COMP,
+ }, {
+ .type = IIO_ROT,
+ .modified = 1,
+ .channel2 = IIO_MOD_NORTH_TRUE_TILT_COMP,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+ .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_NORTH_TRUE_TILT_COMP,
+ }, {
+ .type = IIO_ROT,
+ .modified = 1,
+ .channel2 = IIO_MOD_NORTH_MAGN,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+ .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_NORTH_TRUE,
+ }, {
+ .type = IIO_ROT,
+ .modified = 1,
+ .channel2 = IIO_MOD_NORTH_TRUE,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+ .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_NORTH_TRUE,
}
};
@@ -248,6 +296,14 @@ static int magn_3d_capture_sample(struct hid_sensor_hub_device *hsdev,
iio_val = magn_state->magn_val_addr[CHANNEL_SCAN_INDEX_X + offset];
break;
+ case HID_USAGE_SENSOR_ORIENT_COMP_MAGN_NORTH:
+ case HID_USAGE_SENSOR_ORIENT_COMP_TRUE_NORTH:
+ case HID_USAGE_SENSOR_ORIENT_MAGN_NORTH:
+ case HID_USAGE_SENSOR_ORIENT_TRUE_NORTH:
+ offset = usage_id - HID_USAGE_SENSOR_ORIENT_COMP_MAGN_NORTH;
+ iio_val = magn_state->magn_val_addr[CHANNEL_SCAN_INDEX_NORTH_MAGN_TILT_COMP + offset];
+
+ break;
default:
return -EINVAL;
}
--
1.9.3
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v4 3/4] iio: hid-sensor-magn-3d: Scan for usage attributes before setting up iio channels
2014-06-30 2:58 ` [PATCH v4 3/4] iio: hid-sensor-magn-3d: Scan for usage attributes before setting up iio channels Reyad Attiyat
@ 2014-07-07 16:44 ` Jonathan Cameron
2014-07-07 16:49 ` Srinivas Pandruvada
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Jonathan Cameron @ 2014-07-07 16:44 UTC (permalink / raw)
To: Reyad Attiyat, linux-iio, linux-kernel, srinivas.pandruvada
On 30/06/14 03:58, Reyad Attiyat wrote:
> Scan for and count the HID usage attributes supported by the driver.
> This allows for the driver to only setup the IIO channels for the
> sensor usages present in the HID USB reports.
>
> Signed-off-by: Reyad Attiyat <reyad.attiyat@gmail.com>
> ---
There should be an explanation here of what has changed from one version to the
next.
The patches should have been run through checkpatch.pl (at least one place below
indicates that didn't happen).
Mostly little bits left. Now I would definitely like an ack or reviewed-by
from Srinivas for this one if at all possible.
Any tested-bys, particularly with parts that don't have the new channel
types, would also be good.
Jonathan
> drivers/iio/magnetometer/hid-sensor-magn-3d.c | 111 +++++++++++++++++---------
> 1 file changed, 75 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/iio/magnetometer/hid-sensor-magn-3d.c b/drivers/iio/magnetometer/hid-sensor-magn-3d.c
> index 41cf29e..070d20e 100644
> --- a/drivers/iio/magnetometer/hid-sensor-magn-3d.c
> +++ b/drivers/iio/magnetometer/hid-sensor-magn-3d.c
> @@ -42,7 +42,8 @@ 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];
> + u32 *iio_val;
> int scale_pre_decml;
> int scale_post_decml;
> int scale_precision;
> @@ -221,8 +222,8 @@ static int magn_3d_proc_event(struct hid_sensor_hub_device *hsdev,
> dev_dbg(&indio_dev->dev, "magn_3d_proc_event\n");
> if (atomic_read(&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;
> }
> @@ -236,52 +237,99 @@ 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;
> - int ret = -EINVAL;
> + int ret = 0;
> + u32 *iio_val = NULL;
This has me a little confused. You have an iio_val in your state
structure and in this function. I can't actually find anywhere where
the elements of the one in the state structure are ever assigned to
anything.
>
> switch (usage_id) {
> case HID_USAGE_SENSOR_ORIENT_MAGN_FLUX_X_AXIS:
> case HID_USAGE_SENSOR_ORIENT_MAGN_FLUX_Y_AXIS:
> 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;
> + iio_val = magn_state->magn_val_addr[CHANNEL_SCAN_INDEX_X + offset];
> +
> break;
> default:
> - break;
> + return -EINVAL;
> }
>
> + if(iio_val)
white space after if please.
> + *iio_val = *(u32 *)raw_data;
> + else
> + ret = -EINVAL;
> +
> return ret;
> }
>
> /* Parse report which is specific to an usage id*/
> 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 **channels,
> + int *chan_count,
> unsigned usage_id,
> struct magn_3d_state *st)
> {
> - int ret;
> + int ret = 0;
> int i;
> + int attr_count = 0;
> +
> + /* Scan for each usage attribute supported */
> + for (i = 0; i < MAGN_3D_CHANNEL_MAX; i++) {
> + u32 address = magn_3d_addresses[i];
>
> - for (i = 0; i <= CHANNEL_SCAN_INDEX_Z; ++i) {
> + /* Check if usage attribute exists in the sensor hub device */
> 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);
I would have preferred you kept the indenting the same as before. That would
make it more obvious what changed.
> + HID_INPUT_REPORT,
> + usage_id,
> + address,
> + &(st->magn[i]));
> + if (!ret)
> + attr_count++;
> }
> - dev_dbg(&pdev->dev, "magn_3d %x:%x, %x:%x, %x:%x\n",
> +
> + dev_dbg(&pdev->dev, "magn_3d Found %d usage attributes\n",
> + attr_count);
> + dev_dbg(&pdev->dev, "magn_3d X: %x:%x Y: %x:%x Z: %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);
>
> + if (attr_count > 0)
> + ret = 0;
This would suggest to me that you want to rename the ret above (used
to indicate if a channel is there) as something more specific so you
don't end up appearing to squash and error here...
> + else
> + return -EINVAL;
Again your spacing is messed up. Please fix.
> +
> + /* Setup IIO channel array */
> + *channels = devm_kcalloc(&pdev->dev, attr_count,
> +
The resulting indenting here is a complete mess. Please fix.
sizeof(struct iio_chan_spec),
> + GFP_KERNEL);
> + if (!*channels) {
> + dev_err(&pdev->dev, "failed to allocate space for iio channels\n");
> + return -ENOMEM;
> + }
> +
> + st->iio_val = devm_kcalloc(&pdev->dev, attr_count,
> + sizeof(u32),
> + GFP_KERNEL);
> + if (!st->iio_val) {
> + dev_err(&pdev->dev, "failed to allocate space for iio values array\n");
> + return -ENOMEM;
> + }
> +
> + for (i = 0, *chan_count = 0;
> + i < MAGN_3D_CHANNEL_MAX && *chan_count < attr_count;
> + i++)
> + {
> + if (st->magn[i].index >= 0) {
> + /* Setup IIO channel struct */
> + *channels[*chan_count] = magn_3d_channels[i];
> +
> + st->magn_val_addr[i] = &(st->iio_val[*chan_count]);
> + magn_3d_adjust_channel_bit_mask(*channels, *chan_count, st->magn[i].size);
The above should be wrapped appropriately.
> + (*chan_count)++;
> + }
> + }
> +
> st->scale_precision = hid_sensor_format_scale(
> HID_USAGE_SENSOR_COMPASS_3D,
> &st->magn[CHANNEL_SCAN_INDEX_X],
> @@ -311,6 +359,7 @@ static int hid_magn_3d_probe(struct platform_device *pdev)
> struct magn_3d_state *magn_state;
> struct hid_sensor_hub_device *hsdev = pdev->dev.platform_data;
> struct iio_chan_spec *channels;
> + int chan_count = 0;
>
> indio_dev = devm_iio_device_alloc(&pdev->dev,
> sizeof(struct magn_3d_state));
> @@ -331,22 +380,15 @@ static int hid_magn_3d_probe(struct platform_device *pdev)
> return ret;
> }
>
> - channels = kmemdup(magn_3d_channels, sizeof(magn_3d_channels),
> - GFP_KERNEL);
> - if (!channels) {
> - dev_err(&pdev->dev, "failed to duplicate channels\n");
> - return -ENOMEM;
> - }
> -
> - ret = magn_3d_parse_report(pdev, hsdev, channels,
> - HID_USAGE_SENSOR_COMPASS_3D, magn_state);
> + ret = magn_3d_parse_report(pdev, hsdev, &channels,
> + &chan_count, HID_USAGE_SENSOR_COMPASS_3D, magn_state);
> if (ret) {
> dev_err(&pdev->dev, "failed to setup attributes\n");
> - goto error_free_dev_mem;
> + return ret;
> }
>
> indio_dev->channels = channels;
> - indio_dev->num_channels = ARRAY_SIZE(magn_3d_channels);
> + indio_dev->num_channels = chan_count;
> indio_dev->dev.parent = &pdev->dev;
> indio_dev->info = &magn_3d_info;
> indio_dev->name = name;
> @@ -356,7 +398,7 @@ static int hid_magn_3d_probe(struct platform_device *pdev)
> NULL, NULL);
> if (ret) {
> dev_err(&pdev->dev, "failed to initialize trigger buffer\n");
> - goto error_free_dev_mem;
> + return ret;
> }
> atomic_set(&magn_state->common_attributes.data_ready, 0);
> ret = hid_sensor_setup_trigger(indio_dev, name,
> @@ -390,8 +432,6 @@ error_remove_trigger:
> hid_sensor_remove_trigger(&magn_state->common_attributes);
> error_unreg_buffer_funcs:
> iio_triggered_buffer_cleanup(indio_dev);
> -error_free_dev_mem:
> - kfree(indio_dev->channels);
> return ret;
> }
>
> @@ -406,7 +446,6 @@ static int hid_magn_3d_remove(struct platform_device *pdev)
> iio_device_unregister(indio_dev);
> hid_sensor_remove_trigger(&magn_state->common_attributes);
> iio_triggered_buffer_cleanup(indio_dev);
> - kfree(indio_dev->channels);
>
> return 0;
> }
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 1/4] iio: Documentation: Add documentation for rotation from north sensor usage attributes
2014-06-30 2:58 ` [PATCH v4 1/4] iio: Documentation: Add documentation for rotation from north sensor usage attributes Reyad Attiyat
@ 2014-07-07 16:45 ` Jonathan Cameron
0 siblings, 0 replies; 11+ messages in thread
From: Jonathan Cameron @ 2014-07-07 16:45 UTC (permalink / raw)
To: Reyad Attiyat, linux-iio, linux-kernel, srinivas.pandruvada
On 30/06/14 03:58, Reyad Attiyat wrote:
> Added documentation for the sysfs attributes supported by the rotation from north
> sensor.
>
> Signed-off-by: Reyad Attiyat <reyad.attiyat@gmail.com>
Looks good, will pick up once I'm happy with patch 3.
> ---
> Documentation/ABI/testing/sysfs-bus-iio | 82 +++++++++++++++++++++++++++++++++
> 1 file changed, 82 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index a9757dc..e88833d 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -260,6 +260,10 @@ What: /sys/bus/iio/devices/iio:deviceX/in_magn_scale
> What: /sys/bus/iio/devices/iio:deviceX/in_magn_x_scale
> What: /sys/bus/iio/devices/iio:deviceX/in_magn_y_scale
> What: /sys/bus/iio/devices/iio:deviceX/in_magn_z_scale
> +What: /sys/bus/iio/devices/iio:deviceX/in_rot_from_north_magnetic_scale
> +What: /sys/bus/iio/devices/iio:deviceX/in_rot_from_north_true_scale
> +What: /sys/bus/iio/devices/iio:deviceX/in_rot_from_north_magnetic_tilt_comp_scale
> +What: /sys/bus/iio/devices/iio:deviceX/in_rot_from_north_true_tilt_comp_scale
> What: /sys/bus/iio/devices/iio:deviceX/in_pressureY_scale
> What: /sys/bus/iio/devices/iio:deviceX/in_pressure_scale
> KernelVersion: 2.6.35
> @@ -447,6 +451,14 @@ What: /sys/.../iio:deviceX/events/in_magn_y_thresh_rising_en
> What: /sys/.../iio:deviceX/events/in_magn_y_thresh_falling_en
> What: /sys/.../iio:deviceX/events/in_magn_z_thresh_rising_en
> What: /sys/.../iio:deviceX/events/in_magn_z_thresh_falling_en
> +What: /sys/.../iio:deviceX/events/in_rot_from_north_magnetic_thresh_rising_en
> +What: /sys/.../iio:deviceX/events/in_rot_from_north_magnetic_thresh_falling_en
> +What: /sys/.../iio:deviceX/events/in_rot_from_north_true_thresh_rising_en
> +What: /sys/.../iio:deviceX/events/in_rot_from_north_true_thresh_falling_en
> +What: /sys/.../iio:deviceX/events/in_rot_from_north_magnetic_tilt_comp_thresh_rising_en
> +What: /sys/.../iio:deviceX/events/in_rot_from_north_magnetic_tilt_comp_thresh_falling_en
> +What: /sys/.../iio:deviceX/events/in_rot_from_north_true_tilt_comp_thresh_rising_en
> +What: /sys/.../iio:deviceX/events/in_rot_from_north_true_tilt_comp_thresh_falling_en
> What: /sys/.../iio:deviceX/events/in_voltageY_supply_thresh_rising_en
> What: /sys/.../iio:deviceX/events/in_voltageY_supply_thresh_falling_en
> What: /sys/.../iio:deviceX/events/in_voltageY_thresh_rising_en
> @@ -492,6 +504,14 @@ What: /sys/.../iio:deviceX/events/in_magn_y_roc_rising_en
> What: /sys/.../iio:deviceX/events/in_magn_y_roc_falling_en
> What: /sys/.../iio:deviceX/events/in_magn_z_roc_rising_en
> What: /sys/.../iio:deviceX/events/in_magn_z_roc_falling_en
> +What: /sys/.../iio:deviceX/events/in_rot_from_north_magnetic_roc_rising_en
> +What: /sys/.../iio:deviceX/events/in_rot_from_north_magnetic_roc_falling_en
> +What: /sys/.../iio:deviceX/events/in_rot_from_north_true_roc_rising_en
> +What: /sys/.../iio:deviceX/events/in_rot_from_north_true_roc_falling_en
> +What: /sys/.../iio:deviceX/events/in_rot_from_north_magnetic_tilt_comp_roc_rising_en
> +What: /sys/.../iio:deviceX/events/in_rot_from_north_magnetic_tilt_comp_roc_falling_en
> +What: /sys/.../iio:deviceX/events/in_rot_from_north_true_tilt_comp_roc_rising_en
> +What: /sys/.../iio:deviceX/events/in_rot_from_north_true_tilt_comp_roc_falling_en
> What: /sys/.../iio:deviceX/events/in_voltageY_supply_roc_rising_en
> What: /sys/.../iio:deviceX/events/in_voltageY_supply_roc_falling_en
> What: /sys/.../iio:deviceX/events/in_voltageY_roc_rising_en
> @@ -538,6 +558,14 @@ What: /sys/.../events/in_magn_y_raw_thresh_rising_value
> What: /sys/.../events/in_magn_y_raw_thresh_falling_value
> What: /sys/.../events/in_magn_z_raw_thresh_rising_value
> What: /sys/.../events/in_magn_z_raw_thresh_falling_value
> +What: /sys/.../events/in_rot_from_north_magnetic_raw_thresh_rising_value
> +What: /sys/.../events/in_rot_from_north_magnetic_raw_thresh_falling_value
> +What: /sys/.../events/in_rot_from_north_true_raw_thresh_rising_value
> +What: /sys/.../events/in_rot_from_north_true_raw_thresh_falling_value
> +What: /sys/.../events/in_rot_from_north_magnetic_tilt_comp_raw_thresh_rising_value
> +What: /sys/.../events/in_rot_from_north_magnetic_tilt_comp_raw_thresh_falling_value
> +What: /sys/.../events/in_rot_from_north_true_tilt_comp_raw_thresh_rising_value
> +What: /sys/.../events/in_rot_from_north_true_tilt_comp_raw_thresh_falling_value
> What: /sys/.../events/in_voltageY_supply_raw_thresh_rising_value
> What: /sys/.../events/in_voltageY_supply_raw_thresh_falling_value
> What: /sys/.../events/in_voltageY_raw_thresh_rising_value
> @@ -588,6 +616,18 @@ What: /sys/.../events/in_magn_y_thresh_either_hysteresis
> What: /sys/.../events/in_magn_z_thresh_rising_hysteresis
> What: /sys/.../events/in_magn_z_thresh_falling_hysteresis
> What: /sys/.../events/in_magn_z_thresh_either_hysteresis
> +What: /sys/.../events/in_rot_from_north_magnetic_thresh_rising_hysteresis
> +What: /sys/.../events/in_rot_from_north_magnetic_thresh_falling_hysteresis
> +What: /sys/.../events/in_rot_from_north_magnetic_thresh_either_hysteresis
> +What: /sys/.../events/in_rot_from_north_true_thresh_rising_hysteresis
> +What: /sys/.../events/in_rot_from_north_true_thresh_falling_hysteresis
> +What: /sys/.../events/in_rot_from_north_true_thresh_either_hysteresis
> +What: /sys/.../events/in_rot_from_north_magnetic_tilt_comp_thresh_rising_hysteresis
> +What: /sys/.../events/in_rot_from_north_magnetic_tilt_comp_thresh_falling_hysteresis
> +What: /sys/.../events/in_rot_from_north_magnetic_tilt_comp_thresh_either_hysteresis
> +What: /sys/.../events/in_rot_from_north_true_tilt_comp_thresh_rising_hysteresis
> +What: /sys/.../events/in_rot_from_north_true_tilt_comp_thresh_falling_hysteresis
> +What: /sys/.../events/in_rot_from_north_true_tilt_comp_thresh_either_hysteresis
> What: /sys/.../events/in_voltageY_thresh_rising_hysteresis
> What: /sys/.../events/in_voltageY_thresh_falling_hysteresis
> What: /sys/.../events/in_voltageY_thresh_either_hysteresis
> @@ -635,6 +675,14 @@ What: /sys/.../events/in_magn_y_raw_roc_rising_value
> What: /sys/.../events/in_magn_y_raw_roc_falling_value
> What: /sys/.../events/in_magn_z_raw_roc_rising_value
> What: /sys/.../events/in_magn_z_raw_roc_falling_value
> +What: /sys/.../events/in_rot_from_north_magnetic_raw_roc_rising_value
> +What: /sys/.../events/in_rot_from_north_magnetic_raw_roc_falling_value
> +What: /sys/.../events/in_rot_from_north_true_raw_roc_rising_value
> +What: /sys/.../events/in_rot_from_north_true_raw_roc_falling_value
> +What: /sys/.../events/in_rot_from_north_magnetic_tilt_comp_raw_roc_rising_value
> +What: /sys/.../events/in_rot_from_north_magnetic_tilt_comp_raw_roc_falling_value
> +What: /sys/.../events/in_rot_from_north_true_tilt_comp_raw_roc_rising_value
> +What: /sys/.../events/in_rot_from_north_true_tilt_comp_raw_roc_falling_value
> What: /sys/.../events/in_voltageY_supply_raw_roc_rising_value
> What: /sys/.../events/in_voltageY_supply_raw_roc_falling_value
> What: /sys/.../events/in_voltageY_raw_roc_rising_value
> @@ -690,6 +738,22 @@ What: /sys/.../events/in_magn_z_thresh_rising_period
> What: /sys/.../events/in_magn_z_thresh_falling_period
> What: /sys/.../events/in_magn_z_roc_rising_period
> What: /sys/.../events/in_magn_z_roc_falling_period
> +What: /sys/.../events/in_rot_from_north_magnetic_thresh_rising_period
> +What: /sys/.../events/in_rot_from_north_magnetic_thresh_falling_period
> +What: /sys/.../events/in_rot_from_north_magnetic_roc_rising_period
> +What: /sys/.../events/in_rot_from_north_magnetic_roc_falling_period
> +What: /sys/.../events/in_rot_from_north_true_thresh_rising_period
> +What: /sys/.../events/in_rot_from_north_true_thresh_falling_period
> +What: /sys/.../events/in_rot_from_north_true_roc_rising_period
> +What: /sys/.../events/in_rot_from_north_true_roc_falling_period
> +What: /sys/.../events/in_rot_from_north_magnetic_tilt_comp_thresh_rising_period
> +What: /sys/.../events/in_rot_from_north_magnetic_tilt_comp_thresh_falling_period
> +What: /sys/.../events/in_rot_from_north_magnetic_tilt_comp_roc_rising_period
> +What: /sys/.../events/in_rot_from_north_magnetic_tilt_comp_roc_falling_period
> +What: /sys/.../events/in_rot_from_north_true_tilt_comp_thresh_rising_period
> +What: /sys/.../events/in_rot_from_north_true_tilt_comp_thresh_falling_period
> +What: /sys/.../events/in_rot_from_north_true_tilt_comp_roc_rising_period
> +What: /sys/.../events/in_rot_from_north_true_tilt_comp_roc_falling_period
> What: /sys/.../events/in_voltageY_supply_thresh_rising_period
> What: /sys/.../events/in_voltageY_supply_thresh_falling_period
> What: /sys/.../events/in_voltageY_supply_roc_rising_period
> @@ -787,6 +851,10 @@ What: /sys/.../iio:deviceX/scan_elements/in_anglvel_z_en
> What: /sys/.../iio:deviceX/scan_elements/in_magn_x_en
> What: /sys/.../iio:deviceX/scan_elements/in_magn_y_en
> What: /sys/.../iio:deviceX/scan_elements/in_magn_z_en
> +What: /sys/.../iio:deviceX/scan_elements/in_rot_from_north_magnetic_en
> +What: /sys/.../iio:deviceX/scan_elements/in_rot_from_north_true_en
> +What: /sys/.../iio:deviceX/scan_elements/in_rot_from_north_magnetic_tilt_comp_en
> +What: /sys/.../iio:deviceX/scan_elements/in_rot_from_north_true_tilt_comp_en
> What: /sys/.../iio:deviceX/scan_elements/in_timestamp_en
> What: /sys/.../iio:deviceX/scan_elements/in_voltageY_supply_en
> What: /sys/.../iio:deviceX/scan_elements/in_voltageY_en
> @@ -853,6 +921,10 @@ What: /sys/.../iio:deviceX/scan_elements/in_anglvel_z_index
> What: /sys/.../iio:deviceX/scan_elements/in_magn_x_index
> What: /sys/.../iio:deviceX/scan_elements/in_magn_y_index
> What: /sys/.../iio:deviceX/scan_elements/in_magn_z_index
> +What: /sys/.../iio:deviceX/scan_elements/in_rot_from_north_magnetic_index
> +What: /sys/.../iio:deviceX/scan_elements/in_rot_from_north_true_index
> +What: /sys/.../iio:deviceX/scan_elements/in_rot_from_north_magnetic_tilt_comp_index
> +What: /sys/.../iio:deviceX/scan_elements/in_rot_from_north_true_tilt_comp_index
> What: /sys/.../iio:deviceX/scan_elements/in_incli_x_index
> What: /sys/.../iio:deviceX/scan_elements/in_incli_y_index
> What: /sys/.../iio:deviceX/scan_elements/in_timestamp_index
> @@ -933,3 +1005,13 @@ Description:
> x y z w. Here x, y, and z component represents the axis about
> which a rotation will occur and w component represents the
> amount of rotation.
> +
> +What: /sys/bus/iio/devices/iio:deviceX/in_rot_from_north_magnetic_tilt_comp_raw
> +What: /sys/bus/iio/devices/iio:deviceX/in_rot_from_north_true_tilt_comp_raw
> +What: /sys/bus/iio/devices/iio:deviceX/in_rot_from_north_magnetic_raw
> +What: /sys/bus/iio/devices/iio:deviceX/in_rot_from_north_true_raw
> +KernelVersion: 3.15
> +Contact: linux-iio@vger.kernel.org
> +Description:
> + Raw value of rotation from true/magnetic north measured with
> + or without compensation from tilt sensors.
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 3/4] iio: hid-sensor-magn-3d: Scan for usage attributes before setting up iio channels
2014-07-07 16:44 ` Jonathan Cameron
@ 2014-07-07 16:49 ` Srinivas Pandruvada
2014-07-07 19:58 ` Srinivas Pandruvada
2014-07-08 15:59 ` Reyad Attiyat
2 siblings, 0 replies; 11+ messages in thread
From: Srinivas Pandruvada @ 2014-07-07 16:49 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: Reyad Attiyat, linux-iio, linux-kernel
On 07/07/2014 09:44 AM, Jonathan Cameron wrote:
> On 30/06/14 03:58, Reyad Attiyat wrote:
>> Scan for and count the HID usage attributes supported by the driver.
>> This allows for the driver to only setup the IIO channels for the
>> sensor usages present in the HID USB reports.
>>
>> Signed-off-by: Reyad Attiyat <reyad.attiyat@gmail.com>
>> ---
> There should be an explanation here of what has changed from one version
> to the
> next.
>
> The patches should have been run through checkpatch.pl (at least one
> place below
> indicates that didn't happen).
>
> Mostly little bits left. Now I would definitely like an ack or reviewed-by
> from Srinivas for this one if at all possible.
I will provide by end of this week. I have applied to my tree for tests.
Thanks,
Srinivas
> Any tested-bys, particularly with parts that don't have the new channel
> types, would also be good.
>
> Jonathan
>> drivers/iio/magnetometer/hid-sensor-magn-3d.c | 111
>> +++++++++++++++++---------
>> 1 file changed, 75 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/iio/magnetometer/hid-sensor-magn-3d.c
>> b/drivers/iio/magnetometer/hid-sensor-magn-3d.c
>> index 41cf29e..070d20e 100644
>> --- a/drivers/iio/magnetometer/hid-sensor-magn-3d.c
>> +++ b/drivers/iio/magnetometer/hid-sensor-magn-3d.c
>> @@ -42,7 +42,8 @@ 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];
>> + u32 *iio_val;
>> int scale_pre_decml;
>> int scale_post_decml;
>> int scale_precision;
>> @@ -221,8 +222,8 @@ static int magn_3d_proc_event(struct
>> hid_sensor_hub_device *hsdev,
>> dev_dbg(&indio_dev->dev, "magn_3d_proc_event\n");
>> if (atomic_read(&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;
>> }
>> @@ -236,52 +237,99 @@ 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;
>> - int ret = -EINVAL;
>> + int ret = 0;
>> + u32 *iio_val = NULL;
> This has me a little confused. You have an iio_val in your state
> structure and in this function. I can't actually find anywhere where
> the elements of the one in the state structure are ever assigned to
> anything.
>
>>
>> switch (usage_id) {
>> case HID_USAGE_SENSOR_ORIENT_MAGN_FLUX_X_AXIS:
>> case HID_USAGE_SENSOR_ORIENT_MAGN_FLUX_Y_AXIS:
>> 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;
>> + iio_val = magn_state->magn_val_addr[CHANNEL_SCAN_INDEX_X +
>> offset];
>> +
>> break;
>> default:
>> - break;
>> + return -EINVAL;
>> }
>>
>> + if(iio_val)
> white space after if please.
>> + *iio_val = *(u32 *)raw_data;
>> + else
>> + ret = -EINVAL;
>> +
>> return ret;
>> }
>>
>> /* Parse report which is specific to an usage id*/
>> 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 **channels,
>> + int *chan_count,
>> unsigned usage_id,
>> struct magn_3d_state *st)
>> {
>> - int ret;
>> + int ret = 0;
>> int i;
>> + int attr_count = 0;
>> +
>> + /* Scan for each usage attribute supported */
>> + for (i = 0; i < MAGN_3D_CHANNEL_MAX; i++) {
>> + u32 address = magn_3d_addresses[i];
>>
>> - for (i = 0; i <= CHANNEL_SCAN_INDEX_Z; ++i) {
>> + /* Check if usage attribute exists in the sensor hub device */
>> 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);
> I would have preferred you kept the indenting the same as before. That
> would
> make it more obvious what changed.
>> + HID_INPUT_REPORT,
>> + usage_id,
>> + address,
>> + &(st->magn[i]));
>> + if (!ret)
>> + attr_count++;
>> }
>> - dev_dbg(&pdev->dev, "magn_3d %x:%x, %x:%x, %x:%x\n",
>> +
>> + dev_dbg(&pdev->dev, "magn_3d Found %d usage attributes\n",
>> + attr_count);
>> + dev_dbg(&pdev->dev, "magn_3d X: %x:%x Y: %x:%x Z: %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);
>>
>> + if (attr_count > 0)
>> + ret = 0;
> This would suggest to me that you want to rename the ret above (used
> to indicate if a channel is there) as something more specific so you
> don't end up appearing to squash and error here...
>> + else
>> + return -EINVAL;
> Again your spacing is messed up. Please fix.
>> +
>> + /* Setup IIO channel array */
>> + *channels = devm_kcalloc(&pdev->dev, attr_count,
>> +
> The resulting indenting here is a complete mess. Please fix.
> sizeof(struct iio_chan_spec),
>> + GFP_KERNEL);
>> + if (!*channels) {
>> + dev_err(&pdev->dev, "failed to allocate space for iio
>> channels\n");
>> + return -ENOMEM;
>> + }
>> +
>> + st->iio_val = devm_kcalloc(&pdev->dev, attr_count,
>> + sizeof(u32),
>> + GFP_KERNEL);
>> + if (!st->iio_val) {
>> + dev_err(&pdev->dev, "failed to allocate space for iio values
>> array\n");
>> + return -ENOMEM;
>> + }
>> +
>> + for (i = 0, *chan_count = 0;
>> + i < MAGN_3D_CHANNEL_MAX && *chan_count < attr_count;
>> + i++)
>> + {
>> + if (st->magn[i].index >= 0) {
>> + /* Setup IIO channel struct */
>> + *channels[*chan_count] = magn_3d_channels[i];
>> +
>> + st->magn_val_addr[i] = &(st->iio_val[*chan_count]);
>> + magn_3d_adjust_channel_bit_mask(*channels, *chan_count,
>> st->magn[i].size);
> The above should be wrapped appropriately.
>> + (*chan_count)++;
>> + }
>> + }
>> +
>> st->scale_precision = hid_sensor_format_scale(
>> HID_USAGE_SENSOR_COMPASS_3D,
>> &st->magn[CHANNEL_SCAN_INDEX_X],
>> @@ -311,6 +359,7 @@ static int hid_magn_3d_probe(struct
>> platform_device *pdev)
>> struct magn_3d_state *magn_state;
>> struct hid_sensor_hub_device *hsdev = pdev->dev.platform_data;
>> struct iio_chan_spec *channels;
>> + int chan_count = 0;
>>
>> indio_dev = devm_iio_device_alloc(&pdev->dev,
>> sizeof(struct magn_3d_state));
>> @@ -331,22 +380,15 @@ static int hid_magn_3d_probe(struct
>> platform_device *pdev)
>> return ret;
>> }
>>
>> - channels = kmemdup(magn_3d_channels, sizeof(magn_3d_channels),
>> - GFP_KERNEL);
>> - if (!channels) {
>> - dev_err(&pdev->dev, "failed to duplicate channels\n");
>> - return -ENOMEM;
>> - }
>> -
>> - ret = magn_3d_parse_report(pdev, hsdev, channels,
>> - HID_USAGE_SENSOR_COMPASS_3D, magn_state);
>> + ret = magn_3d_parse_report(pdev, hsdev, &channels,
>> + &chan_count, HID_USAGE_SENSOR_COMPASS_3D, magn_state);
>> if (ret) {
>> dev_err(&pdev->dev, "failed to setup attributes\n");
>> - goto error_free_dev_mem;
>> + return ret;
>> }
>>
>> indio_dev->channels = channels;
>> - indio_dev->num_channels = ARRAY_SIZE(magn_3d_channels);
>> + indio_dev->num_channels = chan_count;
>> indio_dev->dev.parent = &pdev->dev;
>> indio_dev->info = &magn_3d_info;
>> indio_dev->name = name;
>> @@ -356,7 +398,7 @@ static int hid_magn_3d_probe(struct
>> platform_device *pdev)
>> NULL, NULL);
>> if (ret) {
>> dev_err(&pdev->dev, "failed to initialize trigger buffer\n");
>> - goto error_free_dev_mem;
>> + return ret;
>> }
>> atomic_set(&magn_state->common_attributes.data_ready, 0);
>> ret = hid_sensor_setup_trigger(indio_dev, name,
>> @@ -390,8 +432,6 @@ error_remove_trigger:
>> hid_sensor_remove_trigger(&magn_state->common_attributes);
>> error_unreg_buffer_funcs:
>> iio_triggered_buffer_cleanup(indio_dev);
>> -error_free_dev_mem:
>> - kfree(indio_dev->channels);
>> return ret;
>> }
>>
>> @@ -406,7 +446,6 @@ static int hid_magn_3d_remove(struct
>> platform_device *pdev)
>> iio_device_unregister(indio_dev);
>> hid_sensor_remove_trigger(&magn_state->common_attributes);
>> iio_triggered_buffer_cleanup(indio_dev);
>> - kfree(indio_dev->channels);
>>
>> return 0;
>> }
>>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 3/4] iio: hid-sensor-magn-3d: Scan for usage attributes before setting up iio channels
2014-07-07 16:44 ` Jonathan Cameron
2014-07-07 16:49 ` Srinivas Pandruvada
@ 2014-07-07 19:58 ` Srinivas Pandruvada
2014-07-08 16:02 ` Reyad Attiyat
2014-07-08 15:59 ` Reyad Attiyat
2 siblings, 1 reply; 11+ messages in thread
From: Srinivas Pandruvada @ 2014-07-07 19:58 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: Reyad Attiyat, linux-iio, linux-kernel
Hi Reyad,
I see panic in the probe function. Can you review your logic?
It is possible that platforms don't have all attributes, so looks
like the probe is returnning with error.
On 07/07/2014 09:44 AM, Jonathan Cameron wrote:
> On 30/06/14 03:58, Reyad Attiyat wrote:
>> Scan for and count the HID usage attributes supported by the driver.
>> This allows for the driver to only setup the IIO channels for the
>> sensor usages present in the HID USB reports.
>>
>> Signed-off-by: Reyad Attiyat <reyad.attiyat@gmail.com>
>> ---
> There should be an explanation here of what has changed from one version
> to the
> next.
>
> The patches should have been run through checkpatch.pl (at least one
> place below
> indicates that didn't happen).
>
> Mostly little bits left. Now I would definitely like an ack or reviewed-by
> from Srinivas for this one if at all possible.
[ 7.653643] BUG: unable to handle kernel NULL pointer dereference at
0000000000000031
[ 7.653648] IP: [<ffffffff81242cee>] sysfs_remove_link+0xe/0x30
[ 7.653650] PGD 13d0fd067 PUD 147cbd067 PMD 0
[ 7.653652] Oops: 0000 [#1] SMP
[ 7.653676] Modules linked in: hid_sensor_magn_3d(+)
hid_sensor_rotation hid_sensor_gyro_3d hid_sensor_trigger
industrialio_triggered_buffer kfifo_buf industrialio asix(+) usb_storage
hid_sensor_iio_common usbnet usbhid mii joydev usbserial(+) hid_rmi(+)
intel_rapl x86_pkg_temp_thermal uvcvideo intel_powerclamp coretemp
kvm_intel videobuf2_vmalloc videobuf2_memops videobuf2_core iwlwifi kvm
v4l2_common videodev hid_multitouch hid_sensor_hub ppdev btusb
crct10dif_pclmul cfg80211 crc32_pclmul ghash_clmulni_intel aesni_intel
aes_x86_64 lrw gf128mul glue_helper ablk_helper cryptd mei_me(+) mei
serio_raw lpc_ich(+) i915(+) snd_hda_codec_realtek snd_hda_codec_generic
snd_hda_intel snd_hda_controller snd_hda_codec snd_hwdep snd_pcm
snd_seq_midi drm_kms_helper snd_seq_midi_event snd_rawmidi snd_seq drm
snd_seq_device snd_timer snd soundcore i2c_algo_bit mac_hid nfsd i2c_hid
hid auth_rpcgss nfs_acl rfcomm nfs bnep bluetooth lockd winbond_cir
sunrpc rc_core parport_pc dw_dmac dw_dmac_core video
i2c_designware_platform spi_pxa2xx_platform i2c_designware_core
binfmt_misc 8250_dw fscache lp nls_iso8859_1 parport ahci libahci
sdhci_acpi sdhci
[ 7.653691] CPU: 1 PID: 372 Comm: systemd-udevd Not tainted
3.16.0-rc4+ #27
[ 7.653692] Hardware name: Intel Corporation Shark Bay Client
platform/Harris Beach SDS, BIOS HSWLPTU1.86C.0133.R00.1309172123 09/17/2013
[ 7.653693] task: ffff880148584b60 ti: ffff880148914000 task.ti:
ffff880148914000
[ 7.653696] RIP: 0010:[<ffffffff81242cee>] [<ffffffff81242cee>]
sysfs_remove_link+0xe/0x30
[ 7.653697] RSP: 0018:ffff880148917c30 EFLAGS: 00010202
[ 7.653698] RAX: ffffffffa08a1028 RBX: ffff880090bef810 RCX:
0000000000001043
[ 7.653698] RDX: 0000000000001042 RSI: 0000000000000000 RDI:
0000000000000001
[ 7.653699] RBP: ffff880148917c30 R08: 00000000000171c0 R09:
ffff88014f2571c0
[ 7.653700] R10: ffffea0002ac20c0 R11: ffffffff814a48a9 R12:
00000000fffffff0
[ 7.653701] R13: ffffffffa08a1028 R14: 0000000000000025 R15:
0000000000000001
[ 7.653702] FS: 00007fd70e437880(0000) GS:ffff88014f240000(0000)
knlGS:0000000000000000
[ 7.653703] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 7.653704] CR2: 0000000000000031 CR3: 000000013d05e000 CR4:
00000000001407e0
[ 7.653705] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
0000000000000000
[ 7.653706] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7:
0000000000000400
[ 7.653706] Stack:
[ 7.653708] ffff880148917c48 ffffffff814a0626 ffff880090bef810
ffff880148917c78
[ 7.653710] ffffffff814a0c2e ffff880090bef810 ffffffffa08a1028
ffff880090bef870
[ 7.653712] 0000000000000000 ffff880148917ca0 ffffffff814a1053
0000000000000000
[ 7.653712] Call Trace:
[ 7.653716] [<ffffffff814a0626>] driver_sysfs_remove+0x26/0x40
[ 7.653719] [<ffffffff814a0c2e>] driver_probe_device+0x8e/0x3e0
[ 7.653720] [<ffffffff814a1053>] __driver_attach+0x93/0xa0
[ 7.653722] [<ffffffff814a0fc0>] ? __device_attach+0x40/0x40
[ 7.653725] [<ffffffff8149ec93>] bus_for_each_dev+0x63/0xa0
[ 7.653727] [<ffffffff814a06ce>] driver_attach+0x1e/0x20
[ 7.653728] [<ffffffff814a02e0>] bus_add_driver+0x180/0x250
[ 7.653731] [<ffffffffa005b000>] ? 0xffffffffa005afff
[ 7.653733] [<ffffffff814a1834>] driver_register+0x64/0xf0
[ 7.653735] [<ffffffff814a2dca>] __platform_driver_register+0x4a/0x50
[ 7.653737] [<ffffffffa005b017>]
hid_magn_3d_platform_driver_init+0x17/0x1000 [hid_sensor_magn_3d]
[ 7.653741] [<ffffffff8100212c>] do_one_initcall+0xbc/0x1f0
[ 7.653744] [<ffffffff811b239d>] ? kfree+0xfd/0x140
[ 7.653747] [<ffffffff811996f2>] ? __vunmap+0xb2/0x100
[ 7.653750] [<ffffffff810eaf0c>] load_module+0x1cec/0x2700
[ 7.653752] [<ffffffff810e6e10>] ? store_uevent+0x40/0x40
[ 7.653755] [<ffffffff810e79f1>] ?
copy_module_from_fd.isra.46+0x121/0x180
[ 7.653757] [<ffffffff810eba96>] SyS_finit_module+0x86/0xb0
[ 7.653761] [<ffffffff8173f73f>] tracesys+0xe1/0xe6
[ 7.653779] Code: 48 8b 35 8e 16 d5 00 48 8b 57 10 e8 0d de ff ff 5d
c3 66 2e 0f 1f 84 00 00 00 00 00 90 0f 1f 44 00 00 55 48 85 ff 48 89 e5
74 12 <48> 8b 7f 30 31 d2 e8 47 dd ff ff 5d c3 0f 1f 44 00 00 48 8b 3d
[ 7.653781] RIP [<ffffffff81242cee>] sysfs_remove_link+0xe/0x30
[ 7.653782] RSP <ffff880148917c30>
[ 7.653782] CR2: 0000000000000031
[ 7.653785] ---[ end trace 05dd86b8f35f8800 ]---
Other changes, as suggested by Jontahan regarding format, one more
on iio_val in the structure magn_3d_state.
> Any tested-bys, particularly with parts that don't have the new channel
> types, would also be good.
>
> Jonathan
>> drivers/iio/magnetometer/hid-sensor-magn-3d.c | 111
>> +++++++++++++++++---------
>> 1 file changed, 75 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/iio/magnetometer/hid-sensor-magn-3d.c
>> b/drivers/iio/magnetometer/hid-sensor-magn-3d.c
>> index 41cf29e..070d20e 100644
>> --- a/drivers/iio/magnetometer/hid-sensor-magn-3d.c
>> +++ b/drivers/iio/magnetometer/hid-sensor-magn-3d.c
>> @@ -42,7 +42,8 @@ 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];
>> + u32 *iio_val;
I prefer iio_vals, as this stores all the values not a single value.
Thanks,
Srinivas
>> int scale_pre_decml;
>> int scale_post_decml;
>> int scale_precision;
>> @@ -221,8 +222,8 @@ static int magn_3d_proc_event(struct
>> hid_sensor_hub_device *hsdev,
>> dev_dbg(&indio_dev->dev, "magn_3d_proc_event\n");
>> if (atomic_read(&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;
>> }
>> @@ -236,52 +237,99 @@ 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;
>> - int ret = -EINVAL;
>> + int ret = 0;
>> + u32 *iio_val = NULL;
> This has me a little confused. You have an iio_val in your state
> structure and in this function. I can't actually find anywhere where
> the elements of the one in the state structure are ever assigned to
> anything.
>
>>
>> switch (usage_id) {
>> case HID_USAGE_SENSOR_ORIENT_MAGN_FLUX_X_AXIS:
>> case HID_USAGE_SENSOR_ORIENT_MAGN_FLUX_Y_AXIS:
>> 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;
>> + iio_val = magn_state->magn_val_addr[CHANNEL_SCAN_INDEX_X +
>> offset];
>> +
>> break;
>> default:
>> - break;
>> + return -EINVAL;
>> }
>>
>> + if(iio_val)
> white space after if please.
>> + *iio_val = *(u32 *)raw_data;
>> + else
>> + ret = -EINVAL;
>> +
>> return ret;
>> }
>>
>> /* Parse report which is specific to an usage id*/
>> 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 **channels,
>> + int *chan_count,
>> unsigned usage_id,
>> struct magn_3d_state *st)
>> {
>> - int ret;
>> + int ret = 0;
>> int i;
>> + int attr_count = 0;
>> +
>> + /* Scan for each usage attribute supported */
>> + for (i = 0; i < MAGN_3D_CHANNEL_MAX; i++) {
>> + u32 address = magn_3d_addresses[i];
>>
>> - for (i = 0; i <= CHANNEL_SCAN_INDEX_Z; ++i) {
>> + /* Check if usage attribute exists in the sensor hub device */
>> 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);
> I would have preferred you kept the indenting the same as before. That
> would
> make it more obvious what changed.
>> + HID_INPUT_REPORT,
>> + usage_id,
>> + address,
>> + &(st->magn[i]));
>> + if (!ret)
>> + attr_count++;
>> }
>> - dev_dbg(&pdev->dev, "magn_3d %x:%x, %x:%x, %x:%x\n",
>> +
>> + dev_dbg(&pdev->dev, "magn_3d Found %d usage attributes\n",
>> + attr_count);
>> + dev_dbg(&pdev->dev, "magn_3d X: %x:%x Y: %x:%x Z: %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);
>>
>> + if (attr_count > 0)
>> + ret = 0;
> This would suggest to me that you want to rename the ret above (used
> to indicate if a channel is there) as something more specific so you
> don't end up appearing to squash and error here...
>> + else
>> + return -EINVAL;
> Again your spacing is messed up. Please fix.
>> +
>> + /* Setup IIO channel array */
>> + *channels = devm_kcalloc(&pdev->dev, attr_count,
>> +
> The resulting indenting here is a complete mess. Please fix.
> sizeof(struct iio_chan_spec),
>> + GFP_KERNEL);
>> + if (!*channels) {
>> + dev_err(&pdev->dev, "failed to allocate space for iio
>> channels\n");
>> + return -ENOMEM;
>> + }
>> +
>> + st->iio_val = devm_kcalloc(&pdev->dev, attr_count,
>> + sizeof(u32),
>> + GFP_KERNEL);
>> + if (!st->iio_val) {
>> + dev_err(&pdev->dev, "failed to allocate space for iio values
>> array\n");
>> + return -ENOMEM;
>> + }
>> +
>> + for (i = 0, *chan_count = 0;
>> + i < MAGN_3D_CHANNEL_MAX && *chan_count < attr_count;
>> + i++)
>> + {
>> + if (st->magn[i].index >= 0) {
>> + /* Setup IIO channel struct */
>> + *channels[*chan_count] = magn_3d_channels[i];
>> +
>> + st->magn_val_addr[i] = &(st->iio_val[*chan_count]);
>> + magn_3d_adjust_channel_bit_mask(*channels, *chan_count,
>> st->magn[i].size);
> The above should be wrapped appropriately.
>> + (*chan_count)++;
>> + }
>> + }
>> +
>> st->scale_precision = hid_sensor_format_scale(
>> HID_USAGE_SENSOR_COMPASS_3D,
>> &st->magn[CHANNEL_SCAN_INDEX_X],
>> @@ -311,6 +359,7 @@ static int hid_magn_3d_probe(struct
>> platform_device *pdev)
>> struct magn_3d_state *magn_state;
>> struct hid_sensor_hub_device *hsdev = pdev->dev.platform_data;
>> struct iio_chan_spec *channels;
>> + int chan_count = 0;
>>
>> indio_dev = devm_iio_device_alloc(&pdev->dev,
>> sizeof(struct magn_3d_state));
>> @@ -331,22 +380,15 @@ static int hid_magn_3d_probe(struct
>> platform_device *pdev)
>> return ret;
>> }
>>
>> - channels = kmemdup(magn_3d_channels, sizeof(magn_3d_channels),
>> - GFP_KERNEL);
>> - if (!channels) {
>> - dev_err(&pdev->dev, "failed to duplicate channels\n");
>> - return -ENOMEM;
>> - }
>> -
>> - ret = magn_3d_parse_report(pdev, hsdev, channels,
>> - HID_USAGE_SENSOR_COMPASS_3D, magn_state);
>> + ret = magn_3d_parse_report(pdev, hsdev, &channels,
>> + &chan_count, HID_USAGE_SENSOR_COMPASS_3D, magn_state);
>> if (ret) {
>> dev_err(&pdev->dev, "failed to setup attributes\n");
>> - goto error_free_dev_mem;
>> + return ret;
>> }
>>
>> indio_dev->channels = channels;
>> - indio_dev->num_channels = ARRAY_SIZE(magn_3d_channels);
>> + indio_dev->num_channels = chan_count;
>> indio_dev->dev.parent = &pdev->dev;
>> indio_dev->info = &magn_3d_info;
>> indio_dev->name = name;
>> @@ -356,7 +398,7 @@ static int hid_magn_3d_probe(struct
>> platform_device *pdev)
>> NULL, NULL);
>> if (ret) {
>> dev_err(&pdev->dev, "failed to initialize trigger buffer\n");
>> - goto error_free_dev_mem;
>> + return ret;
>> }
>> atomic_set(&magn_state->common_attributes.data_ready, 0);
>> ret = hid_sensor_setup_trigger(indio_dev, name,
>> @@ -390,8 +432,6 @@ error_remove_trigger:
>> hid_sensor_remove_trigger(&magn_state->common_attributes);
>> error_unreg_buffer_funcs:
>> iio_triggered_buffer_cleanup(indio_dev);
>> -error_free_dev_mem:
>> - kfree(indio_dev->channels);
>> return ret;
>> }
>>
>> @@ -406,7 +446,6 @@ static int hid_magn_3d_remove(struct
>> platform_device *pdev)
>> iio_device_unregister(indio_dev);
>> hid_sensor_remove_trigger(&magn_state->common_attributes);
>> iio_triggered_buffer_cleanup(indio_dev);
>> - kfree(indio_dev->channels);
>>
>> return 0;
>> }
>>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 3/4] iio: hid-sensor-magn-3d: Scan for usage attributes before setting up iio channels
2014-07-07 16:44 ` Jonathan Cameron
2014-07-07 16:49 ` Srinivas Pandruvada
2014-07-07 19:58 ` Srinivas Pandruvada
@ 2014-07-08 15:59 ` Reyad Attiyat
2 siblings, 0 replies; 11+ messages in thread
From: Reyad Attiyat @ 2014-07-08 15:59 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: linux-iio, linux-kernel, Srinivas Pandruvada
Hey Jonathan,
Thanks for the review. I'll be sure to fix up all the formatting you
mentioned and run it through the checkpatch.pl script.
> There should be an explanation here of what has changed from one version to
> the
> next.
Will include updates in future patch notes.
> The patches should have been run through checkpatch.pl (at least one place
> below
> indicates that didn't happen).
>
> Mostly little bits left. Now I would definitely like an ack or reviewed-by
> from Srinivas for this one if at all possible.
> Any tested-bys, particularly with parts that don't have the new channel
> types, would also be good.
>
> Jonathan
>
>> drivers/iio/magnetometer/hid-sensor-magn-3d.c | 111
>> +++++++++++++++++---------
>> 1 file changed, 75 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/iio/magnetometer/hid-sensor-magn-3d.c
>> b/drivers/iio/magnetometer/hid-sensor-magn-3d.c
>> index 41cf29e..070d20e 100644
>> --- a/drivers/iio/magnetometer/hid-sensor-magn-3d.c
>> +++ b/drivers/iio/magnetometer/hid-sensor-magn-3d.c
>> @@ -42,7 +42,8 @@ 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];
>> + u32 *iio_val;
>> int scale_pre_decml;
>> int scale_post_decml;
>> int scale_precision;
>> @@ -221,8 +222,8 @@ static int magn_3d_proc_event(struct
>> hid_sensor_hub_device *hsdev,
>> dev_dbg(&indio_dev->dev, "magn_3d_proc_event\n");
>> if (atomic_read(&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;
>> }
>> @@ -236,52 +237,99 @@ 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;
>> - int ret = -EINVAL;
>> + int ret = 0;
>> + u32 *iio_val = NULL;
>
> This has me a little confused. You have an iio_val in your state
> structure and in this function. I can't actually find anywhere where
> the elements of the one in the state structure are ever assigned to
> anything.
>
>
I assign the pointer location of iio_vals to:
u32 *magn_val_addr[MAGN_3D_CHANNEL_MAX];
I also added this array just for the locations. The assignment occurs
in the parse_report
function, after iio_vals is allocated, like this st->magn_val_addr[i]
= &(st->iio_val[*chan_count]).
I'll add a comment to explain the usage better.
>>
>> switch (usage_id) {
>> case HID_USAGE_SENSOR_ORIENT_MAGN_FLUX_X_AXIS:
>> case HID_USAGE_SENSOR_ORIENT_MAGN_FLUX_Y_AXIS:
>> 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;
>> + iio_val = magn_state->magn_val_addr[CHANNEL_SCAN_INDEX_X +
>> offset];
>> +
>> break;
>> default:
>> - break;
>> + return -EINVAL;
>> }
>>
>> + if(iio_val)
>
> white space after if please.
>
>> + *iio_val = *(u32 *)raw_data;
>> + else
>> + ret = -EINVAL;
>> +
>> return ret;
>> }
>>
>> /* Parse report which is specific to an usage id*/
>> 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 **channels,
>> + int *chan_count,
>> unsigned usage_id,
>> struct magn_3d_state *st)
>> {
>> - int ret;
>> + int ret = 0;
>> int i;
>> + int attr_count = 0;
>> +
>> + /* Scan for each usage attribute supported */
>> + for (i = 0; i < MAGN_3D_CHANNEL_MAX; i++) {
>> + u32 address = magn_3d_addresses[i];
>>
>> - for (i = 0; i <= CHANNEL_SCAN_INDEX_Z; ++i) {
>> + /* Check if usage attribute exists in the sensor hub
>> device */
>> 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);
>
> I would have preferred you kept the indenting the same as before. That would
> make it more obvious what changed.
>
>> + HID_INPUT_REPORT,
>> + usage_id,
>> + address,
>> + &(st->magn[i]));
>> + if (!ret)
>> + attr_count++;
>> }
>> - dev_dbg(&pdev->dev, "magn_3d %x:%x, %x:%x, %x:%x\n",
>> +
>> + dev_dbg(&pdev->dev, "magn_3d Found %d usage attributes\n",
>> + attr_count);
>> + dev_dbg(&pdev->dev, "magn_3d X: %x:%x Y: %x:%x Z: %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);
>>
>> + if (attr_count > 0)
>> + ret = 0;
>
> This would suggest to me that you want to rename the ret above (used
> to indicate if a channel is there) as something more specific so you
> don't end up appearing to squash and error here...
>
The ret value above needs to be zeroed out as it is used to break out
of the for loop when scanning for usage attributes.
I will use a different variable name perhaps to distinguish the two.
>> + else
>> + return -EINVAL;
>
> Again your spacing is messed up. Please fix.
>
>> +
>> + /* Setup IIO channel array */
>> + *channels = devm_kcalloc(&pdev->dev, attr_count,
>> +
>
> The resulting indenting here is a complete mess. Please fix.
>
>
> sizeof(struct iio_chan_spec),
>>
>> +
>> GFP_KERNEL);
>> + if (!*channels) {
>> + dev_err(&pdev->dev, "failed to allocate space for iio
>> channels\n");
>> + return -ENOMEM;
>> + }
>> +
>> + st->iio_val = devm_kcalloc(&pdev->dev, attr_count,
>> +
>> sizeof(u32),
>> +
>> GFP_KERNEL);
>> + if (!st->iio_val) {
>> + dev_err(&pdev->dev, "failed to allocate space for iio
>> values array\n");
>> + return -ENOMEM;
>> + }
>> +
>> + for (i = 0, *chan_count = 0;
>> + i < MAGN_3D_CHANNEL_MAX && *chan_count < attr_count;
>> + i++)
>> + {
>> + if (st->magn[i].index >= 0) {
>> + /* Setup IIO channel struct */
>> + *channels[*chan_count] = magn_3d_channels[i];
>> +
>> + st->magn_val_addr[i] =
>> &(st->iio_val[*chan_count]);
>> + magn_3d_adjust_channel_bit_mask(*channels,
>> *chan_count, st->magn[i].size);
>
> The above should be wrapped appropriately.
>
>> + (*chan_count)++;
>> + }
>> + }
>> +
>> st->scale_precision = hid_sensor_format_scale(
>> HID_USAGE_SENSOR_COMPASS_3D,
>> &st->magn[CHANNEL_SCAN_INDEX_X],
>> @@ -311,6 +359,7 @@ static int hid_magn_3d_probe(struct platform_device
>> *pdev)
>> struct magn_3d_state *magn_state;
>> struct hid_sensor_hub_device *hsdev = pdev->dev.platform_data;
>> struct iio_chan_spec *channels;
>> + int chan_count = 0;
>>
>> indio_dev = devm_iio_device_alloc(&pdev->dev,
>> sizeof(struct magn_3d_state));
>> @@ -331,22 +380,15 @@ static int hid_magn_3d_probe(struct platform_device
>> *pdev)
>> return ret;
>> }
>>
>> - channels = kmemdup(magn_3d_channels, sizeof(magn_3d_channels),
>> - GFP_KERNEL);
>> - if (!channels) {
>> - dev_err(&pdev->dev, "failed to duplicate channels\n");
>> - return -ENOMEM;
>> - }
>> -
>> - ret = magn_3d_parse_report(pdev, hsdev, channels,
>> - HID_USAGE_SENSOR_COMPASS_3D, magn_state);
>> + ret = magn_3d_parse_report(pdev, hsdev, &channels,
>> + &chan_count, HID_USAGE_SENSOR_COMPASS_3D,
>> magn_state);
>> if (ret) {
>> dev_err(&pdev->dev, "failed to setup attributes\n");
>> - goto error_free_dev_mem;
>> + return ret;
>> }
>>
>> indio_dev->channels = channels;
>> - indio_dev->num_channels = ARRAY_SIZE(magn_3d_channels);
>> + indio_dev->num_channels = chan_count;
>> indio_dev->dev.parent = &pdev->dev;
>> indio_dev->info = &magn_3d_info;
>> indio_dev->name = name;
>> @@ -356,7 +398,7 @@ static int hid_magn_3d_probe(struct platform_device
>> *pdev)
>> NULL, NULL);
>> if (ret) {
>> dev_err(&pdev->dev, "failed to initialize trigger
>> buffer\n");
>> - goto error_free_dev_mem;
>> + return ret;
>> }
>> atomic_set(&magn_state->common_attributes.data_ready, 0);
>> ret = hid_sensor_setup_trigger(indio_dev, name,
>> @@ -390,8 +432,6 @@ error_remove_trigger:
>> hid_sensor_remove_trigger(&magn_state->common_attributes);
>> error_unreg_buffer_funcs:
>> iio_triggered_buffer_cleanup(indio_dev);
>> -error_free_dev_mem:
>> - kfree(indio_dev->channels);
>> return ret;
>> }
>>
>> @@ -406,7 +446,6 @@ static int hid_magn_3d_remove(struct platform_device
>> *pdev)
>> iio_device_unregister(indio_dev);
>> hid_sensor_remove_trigger(&magn_state->common_attributes);
>> iio_triggered_buffer_cleanup(indio_dev);
>> - kfree(indio_dev->channels);
>>
>> return 0;
>> }
>>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 3/4] iio: hid-sensor-magn-3d: Scan for usage attributes before setting up iio channels
2014-07-07 19:58 ` Srinivas Pandruvada
@ 2014-07-08 16:02 ` Reyad Attiyat
0 siblings, 0 replies; 11+ messages in thread
From: Reyad Attiyat @ 2014-07-08 16:02 UTC (permalink / raw)
To: Srinivas Pandruvada; +Cc: Jonathan Cameron, linux-iio, linux-kernel
Hello Srinivas,
Thanks for testing this patch. I only have one PC with a
hid-sensor-hub and mine doesn't have the axis usage attribute only
north. I'll look into this.
On Mon, Jul 7, 2014 at 2:58 PM, Srinivas Pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
> Hi Reyad,
>
> I see panic in the probe function. Can you review your logic?
> It is possible that platforms don't have all attributes, so looks
> like the probe is returnning with error.
>
>
> On 07/07/2014 09:44 AM, Jonathan Cameron wrote:
>>
>> On 30/06/14 03:58, Reyad Attiyat wrote:
>>>
>>> Scan for and count the HID usage attributes supported by the driver.
>>> This allows for the driver to only setup the IIO channels for the
>>> sensor usages present in the HID USB reports.
>>>
>>> Signed-off-by: Reyad Attiyat <reyad.attiyat@gmail.com>
>>> ---
>>
>> There should be an explanation here of what has changed from one version
>> to the
>> next.
>>
>> The patches should have been run through checkpatch.pl (at least one
>> place below
>> indicates that didn't happen).
>>
>> Mostly little bits left. Now I would definitely like an ack or
>> reviewed-by
>> from Srinivas for this one if at all possible.
>
>
> [ 7.653643] BUG: unable to handle kernel NULL pointer dereference at
> 0000000000000031
> [ 7.653648] IP: [<ffffffff81242cee>] sysfs_remove_link+0xe/0x30
> [ 7.653650] PGD 13d0fd067 PUD 147cbd067 PMD 0
> [ 7.653652] Oops: 0000 [#1] SMP
> [ 7.653676] Modules linked in: hid_sensor_magn_3d(+) hid_sensor_rotation
> hid_sensor_gyro_3d hid_sensor_trigger industrialio_triggered_buffer
> kfifo_buf industrialio asix(+) usb_storage hid_sensor_iio_common usbnet
> usbhid mii joydev usbserial(+) hid_rmi(+) intel_rapl x86_pkg_temp_thermal
> uvcvideo intel_powerclamp coretemp kvm_intel videobuf2_vmalloc
> videobuf2_memops videobuf2_core iwlwifi kvm v4l2_common videodev
> hid_multitouch hid_sensor_hub ppdev btusb crct10dif_pclmul cfg80211
> crc32_pclmul ghash_clmulni_intel aesni_intel aes_x86_64 lrw gf128mul
> glue_helper ablk_helper cryptd mei_me(+) mei serio_raw lpc_ich(+) i915(+)
> snd_hda_codec_realtek snd_hda_codec_generic snd_hda_intel snd_hda_controller
> snd_hda_codec snd_hwdep snd_pcm snd_seq_midi drm_kms_helper
> snd_seq_midi_event snd_rawmidi snd_seq drm snd_seq_device snd_timer snd
> soundcore i2c_algo_bit mac_hid nfsd i2c_hid hid auth_rpcgss nfs_acl rfcomm
> nfs bnep bluetooth lockd winbond_cir sunrpc rc_core parport_pc dw_dmac
> dw_dmac_core video i2c_designware_platform spi_pxa2xx_platform
> i2c_designware_core binfmt_misc 8250_dw fscache lp nls_iso8859_1 parport
> ahci libahci sdhci_acpi sdhci
> [ 7.653691] CPU: 1 PID: 372 Comm: systemd-udevd Not tainted 3.16.0-rc4+
> #27
> [ 7.653692] Hardware name: Intel Corporation Shark Bay Client
> platform/Harris Beach SDS, BIOS HSWLPTU1.86C.0133.R00.1309172123 09/17/2013
> [ 7.653693] task: ffff880148584b60 ti: ffff880148914000 task.ti:
> ffff880148914000
> [ 7.653696] RIP: 0010:[<ffffffff81242cee>] [<ffffffff81242cee>]
> sysfs_remove_link+0xe/0x30
> [ 7.653697] RSP: 0018:ffff880148917c30 EFLAGS: 00010202
> [ 7.653698] RAX: ffffffffa08a1028 RBX: ffff880090bef810 RCX:
> 0000000000001043
> [ 7.653698] RDX: 0000000000001042 RSI: 0000000000000000 RDI:
> 0000000000000001
> [ 7.653699] RBP: ffff880148917c30 R08: 00000000000171c0 R09:
> ffff88014f2571c0
> [ 7.653700] R10: ffffea0002ac20c0 R11: ffffffff814a48a9 R12:
> 00000000fffffff0
> [ 7.653701] R13: ffffffffa08a1028 R14: 0000000000000025 R15:
> 0000000000000001
> [ 7.653702] FS: 00007fd70e437880(0000) GS:ffff88014f240000(0000)
> knlGS:0000000000000000
> [ 7.653703] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 7.653704] CR2: 0000000000000031 CR3: 000000013d05e000 CR4:
> 00000000001407e0
> [ 7.653705] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
> 0000000000000000
> [ 7.653706] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7:
> 0000000000000400
> [ 7.653706] Stack:
> [ 7.653708] ffff880148917c48 ffffffff814a0626 ffff880090bef810
> ffff880148917c78
> [ 7.653710] ffffffff814a0c2e ffff880090bef810 ffffffffa08a1028
> ffff880090bef870
> [ 7.653712] 0000000000000000 ffff880148917ca0 ffffffff814a1053
> 0000000000000000
> [ 7.653712] Call Trace:
> [ 7.653716] [<ffffffff814a0626>] driver_sysfs_remove+0x26/0x40
> [ 7.653719] [<ffffffff814a0c2e>] driver_probe_device+0x8e/0x3e0
> [ 7.653720] [<ffffffff814a1053>] __driver_attach+0x93/0xa0
> [ 7.653722] [<ffffffff814a0fc0>] ? __device_attach+0x40/0x40
> [ 7.653725] [<ffffffff8149ec93>] bus_for_each_dev+0x63/0xa0
> [ 7.653727] [<ffffffff814a06ce>] driver_attach+0x1e/0x20
> [ 7.653728] [<ffffffff814a02e0>] bus_add_driver+0x180/0x250
> [ 7.653731] [<ffffffffa005b000>] ? 0xffffffffa005afff
> [ 7.653733] [<ffffffff814a1834>] driver_register+0x64/0xf0
> [ 7.653735] [<ffffffff814a2dca>] __platform_driver_register+0x4a/0x50
> [ 7.653737] [<ffffffffa005b017>]
> hid_magn_3d_platform_driver_init+0x17/0x1000 [hid_sensor_magn_3d]
> [ 7.653741] [<ffffffff8100212c>] do_one_initcall+0xbc/0x1f0
> [ 7.653744] [<ffffffff811b239d>] ? kfree+0xfd/0x140
> [ 7.653747] [<ffffffff811996f2>] ? __vunmap+0xb2/0x100
> [ 7.653750] [<ffffffff810eaf0c>] load_module+0x1cec/0x2700
> [ 7.653752] [<ffffffff810e6e10>] ? store_uevent+0x40/0x40
> [ 7.653755] [<ffffffff810e79f1>] ?
> copy_module_from_fd.isra.46+0x121/0x180
> [ 7.653757] [<ffffffff810eba96>] SyS_finit_module+0x86/0xb0
> [ 7.653761] [<ffffffff8173f73f>] tracesys+0xe1/0xe6
> [ 7.653779] Code: 48 8b 35 8e 16 d5 00 48 8b 57 10 e8 0d de ff ff 5d c3
> 66 2e 0f 1f 84 00 00 00 00 00 90 0f 1f 44 00 00 55 48 85 ff 48 89 e5 74 12
> <48> 8b 7f 30 31 d2 e8 47 dd ff ff 5d c3 0f 1f 44 00 00 48 8b 3d
> [ 7.653781] RIP [<ffffffff81242cee>] sysfs_remove_link+0xe/0x30
> [ 7.653782] RSP <ffff880148917c30>
> [ 7.653782] CR2: 0000000000000031
> [ 7.653785] ---[ end trace 05dd86b8f35f8800 ]---
>
>
> Other changes, as suggested by Jontahan regarding format, one more
> on iio_val in the structure magn_3d_state.
>
>
>
>> Any tested-bys, particularly with parts that don't have the new channel
>> types, would also be good.
>>
>> Jonathan
>>>
>>> drivers/iio/magnetometer/hid-sensor-magn-3d.c | 111
>>> +++++++++++++++++---------
>>> 1 file changed, 75 insertions(+), 36 deletions(-)
>>>
>>> diff --git a/drivers/iio/magnetometer/hid-sensor-magn-3d.c
>>> b/drivers/iio/magnetometer/hid-sensor-magn-3d.c
>>> index 41cf29e..070d20e 100644
>>> --- a/drivers/iio/magnetometer/hid-sensor-magn-3d.c
>>> +++ b/drivers/iio/magnetometer/hid-sensor-magn-3d.c
>>> @@ -42,7 +42,8 @@ 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];
>>> + u32 *iio_val;
>
> I prefer iio_vals, as this stores all the values not a single value.
>
> Thanks,
> Srinivas
>
>>> int scale_pre_decml;
>>> int scale_post_decml;
>>> int scale_precision;
>>> @@ -221,8 +222,8 @@ static int magn_3d_proc_event(struct
>>> hid_sensor_hub_device *hsdev,
>>> dev_dbg(&indio_dev->dev, "magn_3d_proc_event\n");
>>> if (atomic_read(&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;
>>> }
>>> @@ -236,52 +237,99 @@ 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;
>>> - int ret = -EINVAL;
>>> + int ret = 0;
>>> + u32 *iio_val = NULL;
>>
>> This has me a little confused. You have an iio_val in your state
>> structure and in this function. I can't actually find anywhere where
>> the elements of the one in the state structure are ever assigned to
>> anything.
>>
>>>
>>> switch (usage_id) {
>>> case HID_USAGE_SENSOR_ORIENT_MAGN_FLUX_X_AXIS:
>>> case HID_USAGE_SENSOR_ORIENT_MAGN_FLUX_Y_AXIS:
>>> 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;
>>> + iio_val = magn_state->magn_val_addr[CHANNEL_SCAN_INDEX_X +
>>> offset];
>>> +
>>> break;
>>> default:
>>> - break;
>>> + return -EINVAL;
>>> }
>>>
>>> + if(iio_val)
>>
>> white space after if please.
>>>
>>> + *iio_val = *(u32 *)raw_data;
>>> + else
>>> + ret = -EINVAL;
>>> +
>>> return ret;
>>> }
>>>
>>> /* Parse report which is specific to an usage id*/
>>> 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 **channels,
>>> + int *chan_count,
>>> unsigned usage_id,
>>> struct magn_3d_state *st)
>>> {
>>> - int ret;
>>> + int ret = 0;
>>> int i;
>>> + int attr_count = 0;
>>> +
>>> + /* Scan for each usage attribute supported */
>>> + for (i = 0; i < MAGN_3D_CHANNEL_MAX; i++) {
>>> + u32 address = magn_3d_addresses[i];
>>>
>>> - for (i = 0; i <= CHANNEL_SCAN_INDEX_Z; ++i) {
>>> + /* Check if usage attribute exists in the sensor hub device */
>>> 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);
>>
>> I would have preferred you kept the indenting the same as before. That
>> would
>> make it more obvious what changed.
>>>
>>> + HID_INPUT_REPORT,
>>> + usage_id,
>>> + address,
>>> + &(st->magn[i]));
>>> + if (!ret)
>>> + attr_count++;
>>> }
>>> - dev_dbg(&pdev->dev, "magn_3d %x:%x, %x:%x, %x:%x\n",
>>> +
>>> + dev_dbg(&pdev->dev, "magn_3d Found %d usage attributes\n",
>>> + attr_count);
>>> + dev_dbg(&pdev->dev, "magn_3d X: %x:%x Y: %x:%x Z: %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);
>>>
>>> + if (attr_count > 0)
>>> + ret = 0;
>>
>> This would suggest to me that you want to rename the ret above (used
>> to indicate if a channel is there) as something more specific so you
>> don't end up appearing to squash and error here...
>>>
>>> + else
>>> + return -EINVAL;
>>
>> Again your spacing is messed up. Please fix.
>>>
>>> +
>>> + /* Setup IIO channel array */
>>> + *channels = devm_kcalloc(&pdev->dev, attr_count,
>>> +
>>
>> The resulting indenting here is a complete mess. Please fix.
>> sizeof(struct iio_chan_spec),
>>>
>>> + GFP_KERNEL);
>>> + if (!*channels) {
>>> + dev_err(&pdev->dev, "failed to allocate space for iio
>>> channels\n");
>>> + return -ENOMEM;
>>> + }
>>> +
>>> + st->iio_val = devm_kcalloc(&pdev->dev, attr_count,
>>> + sizeof(u32),
>>> + GFP_KERNEL);
>>> + if (!st->iio_val) {
>>> + dev_err(&pdev->dev, "failed to allocate space for iio values
>>> array\n");
>>> + return -ENOMEM;
>>> + }
>>> +
>>> + for (i = 0, *chan_count = 0;
>>> + i < MAGN_3D_CHANNEL_MAX && *chan_count < attr_count;
>>> + i++)
>>> + {
>>> + if (st->magn[i].index >= 0) {
>>> + /* Setup IIO channel struct */
>>> + *channels[*chan_count] = magn_3d_channels[i];
>>> +
>>> + st->magn_val_addr[i] = &(st->iio_val[*chan_count]);
>>> + magn_3d_adjust_channel_bit_mask(*channels, *chan_count,
>>> st->magn[i].size);
>>
>> The above should be wrapped appropriately.
>>>
>>> + (*chan_count)++;
>>> + }
>>> + }
>>> +
>>> st->scale_precision = hid_sensor_format_scale(
>>> HID_USAGE_SENSOR_COMPASS_3D,
>>> &st->magn[CHANNEL_SCAN_INDEX_X],
>>> @@ -311,6 +359,7 @@ static int hid_magn_3d_probe(struct
>>> platform_device *pdev)
>>> struct magn_3d_state *magn_state;
>>> struct hid_sensor_hub_device *hsdev = pdev->dev.platform_data;
>>> struct iio_chan_spec *channels;
>>> + int chan_count = 0;
>>>
>>> indio_dev = devm_iio_device_alloc(&pdev->dev,
>>> sizeof(struct magn_3d_state));
>>> @@ -331,22 +380,15 @@ static int hid_magn_3d_probe(struct
>>> platform_device *pdev)
>>> return ret;
>>> }
>>>
>>> - channels = kmemdup(magn_3d_channels, sizeof(magn_3d_channels),
>>> - GFP_KERNEL);
>>> - if (!channels) {
>>> - dev_err(&pdev->dev, "failed to duplicate channels\n");
>>> - return -ENOMEM;
>>> - }
>>> -
>>> - ret = magn_3d_parse_report(pdev, hsdev, channels,
>>> - HID_USAGE_SENSOR_COMPASS_3D, magn_state);
>>> + ret = magn_3d_parse_report(pdev, hsdev, &channels,
>>> + &chan_count, HID_USAGE_SENSOR_COMPASS_3D, magn_state);
>>> if (ret) {
>>> dev_err(&pdev->dev, "failed to setup attributes\n");
>>> - goto error_free_dev_mem;
>>> + return ret;
>>> }
>>>
>>> indio_dev->channels = channels;
>>> - indio_dev->num_channels = ARRAY_SIZE(magn_3d_channels);
>>> + indio_dev->num_channels = chan_count;
>>> indio_dev->dev.parent = &pdev->dev;
>>> indio_dev->info = &magn_3d_info;
>>> indio_dev->name = name;
>>> @@ -356,7 +398,7 @@ static int hid_magn_3d_probe(struct
>>> platform_device *pdev)
>>> NULL, NULL);
>>> if (ret) {
>>> dev_err(&pdev->dev, "failed to initialize trigger buffer\n");
>>> - goto error_free_dev_mem;
>>> + return ret;
>>> }
>>> atomic_set(&magn_state->common_attributes.data_ready, 0);
>>> ret = hid_sensor_setup_trigger(indio_dev, name,
>>> @@ -390,8 +432,6 @@ error_remove_trigger:
>>> hid_sensor_remove_trigger(&magn_state->common_attributes);
>>> error_unreg_buffer_funcs:
>>> iio_triggered_buffer_cleanup(indio_dev);
>>> -error_free_dev_mem:
>>> - kfree(indio_dev->channels);
>>> return ret;
>>> }
>>>
>>> @@ -406,7 +446,6 @@ static int hid_magn_3d_remove(struct
>>> platform_device *pdev)
>>> iio_device_unregister(indio_dev);
>>> hid_sensor_remove_trigger(&magn_state->common_attributes);
>>> iio_triggered_buffer_cleanup(indio_dev);
>>> - kfree(indio_dev->channels);
>>>
>>> return 0;
>>> }
>>>
>>
>>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2014-07-08 16:03 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-06-30 2:58 [PATCH v4 0/4] Add support for rotation from north to the hid-sensor-magn-3d driver Reyad Attiyat
2014-06-30 2:58 ` [PATCH v4 1/4] iio: Documentation: Add documentation for rotation from north sensor usage attributes Reyad Attiyat
2014-07-07 16:45 ` Jonathan Cameron
2014-06-30 2:58 ` [PATCH v4 2/4] iio: types: Added support for rotation from north " Reyad Attiyat
2014-06-30 2:58 ` [PATCH v4 3/4] iio: hid-sensor-magn-3d: Scan for usage attributes before setting up iio channels Reyad Attiyat
2014-07-07 16:44 ` Jonathan Cameron
2014-07-07 16:49 ` Srinivas Pandruvada
2014-07-07 19:58 ` Srinivas Pandruvada
2014-07-08 16:02 ` Reyad Attiyat
2014-07-08 15:59 ` Reyad Attiyat
2014-06-30 2:58 ` [PATCH v4 4/4] iio: hid-sensor-magn-3d: Add support for rotation from north Reyad Attiyat
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).