Re: [RFC PATCH v1 3/3] iio:imu:mpu6050: enhance mounting matrix support

[Jonathan Cameron <jic23@kernel.org>]

On 16/04/16 11:18, Jonathan Cameron wrote:
> On 11/04/16 22:44, Gregor Boirie wrote:
>> On Mon, Apr 11, 2016 at 02:12:12PM -0500, Rob Herring wrote:
>>> On Fri, Apr 08, 2016 at 04:12:05PM +0200, Gregor Boirie wrote:
>>>> Add a new rotation matrix sysfs attribute compliant with IIO core
>>>> mounting matrix API.
>>>> Matrix is retrieved from "in_anglvel_mount_matrix" and
>>>> "in_accel_mount_matrix" sysfs attributes. It is declared into mpu6050 DTS
>>>> entry as a "mount-matrix" property.
>>>
>>> We have a common DT property, but can't have common sysfs interface?
>> Agreed. Original driver was structured that way. In fact, a single matrix is
>> enought for this sensor: AFAIK, reference frames are the same for gyro and
>> accelero.
>> I suppose that having a DTS and sysfs attribute per type of sample /
>> "sub-device" ensure better userspace API consistency across drivers.
>> Jonathan ?
> If shared by all channels in device, could just be mount_matrix
> (as per normal info_mask_shared_by_all elements on which we drop all reference
> to the channel name).
> 
> However, we do have to allow for the per channel type version (and at least in theory
> the per channel entry)  There are are weird devices combining multiple parallel accelerometers
> in a package.  Possible for some reason that these could be other than parallel in some future
> device.
> So we end up with a sort of pyramid with any of the following being possible (and needing
> a sysfs entry if they occur)  Taking a few examples...
> 
> mount_matrix
> in_mount_matrix
> out_mount_matrix
> in_accel_mount_matrix
> in_magn_mount_matrix
> in_accel_x_mount_matrix
> in_accel_y_mount_matrix
> in_accel_x1_mount_matrix
> in_accel_x2_mount_matrix 
> etc where convention is to use the element highest up that covers the device in question.
> 
> It gets awkward if you have say an additional general purpose ADC channel on the device
> as then clearly the mount matrix has nothing to do with it, but convention says we still
> have to drop down to say in_accel_mount_matrix rather than using the mount_matrix form
> that implies that it applies to the in_voltage0 channel.  Common sense and ABIs never
> combine that well ;)
> 
>>
>>>
>>> BTW, sysfs interfaces also have to be documented.
>> Ok
> Yes - update Documentation/sysfs/testing/sysfs-bus-iio with an appropriate entry please.
> Btw - the above pyramid structure is why that document is huge (and often missing
> entries that we all assumed were there for particular combinations of the above!)
You have documented it in patch 1.  I'd forgotten about that.
:)
>>
>>>
>>>>
>>>> Old interface is kept for backward userspace compatibility and may be
>>>> retrieved from legacy platform_data mechanism only.
>>>>
>>>> Signed-off-by: Gregor Boirie <gregor.boirie@parrot.com>
>>>> ---
>>>>  .../devicetree/bindings/iio/imu/inv_mpu6050.txt    | 13 +++++
>>>>  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c         | 56 ++++++++++++++++++++--
>>>>  drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h          |  4 +-
>>>>  include/linux/platform_data/invensense_mpu6050.h   |  5 +-
>>>>  4 files changed, 72 insertions(+), 6 deletions(-)
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>