* [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
* 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
* [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
* 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 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 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
* 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
* [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
end of thread, other threads:[~2014-07-08 16:02 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).