Re: [PATCH 1/1] iio: ak8975: Add Ak8975 magnetometer sensor

[Jonathan Cameron <jic23@cam.ac.uk>]

Before I dive blindly in please note I have only taken a quick look as away from
a computer with a decent size screen until tomorrow evening. I'll take a closer
look at the driver then.
On 09/02/10 23:41, Alan Cox wrote:
> On Thu, 2 Sep 2010 15:16:20 -0700
> Andrew Chew <AChew@nvidia.com> wrote:
> 
>>>> +static IIO_DEVICE_ATTR(mode, S_IRUGO | S_IWUSR, show_mode, 
>>> store_mode, 0);
>>>> +static IIO_DEVICE_ATTR(magn_x_calibscale, S_IRUGO, 
>>> show_calibscale, NULL, 0);
>>>> +static IIO_DEVICE_ATTR(magn_y_calibscale, S_IRUGO, 
>>> show_calibscale, NULL, 1);
>>>> +static IIO_DEVICE_ATTR(magn_z_calibscale, S_IRUGO, 
>>> show_calibscale, NULL, 2);
>>>> +static IIO_DEV_ATTR_MAGN_X(show_raw, AK8975_REG_HXL);
>>>> +static IIO_DEV_ATTR_MAGN_Y(show_raw, AK8975_REG_HYL);
>>>> +static IIO_DEV_ATTR_MAGN_Z(show_raw, AK8975_REG_HZL);
>>>
>>> This seems odd as an interface as it's raw when the maths to provide
>>> non-raw (and thus abstract and easy for user space) data is trivial
>>> enough to do in kernel
>>
>> IIO guys want to do normalization maths above the kernel-level magnetometer IIO layer. 
>>This interface came before me, so I'm just following current conventions.
> 
> That will make a generic IIO <-> input bridge very hard to do right. I
> can see why IIO wants to do that but you need both if so and this also
> needs discussion.

Note that, though there are few drivers using it (just the tsl2563 iirc)
we do allow the hwmon style _input syntax for values in the standard units
for the device type.  If you are going to use the raw option then,
assuming linear conversion, at the very least magn_scale should be provided
to tell userspace how to do the conversion.  If the bridge to input
is done in userspace then it is trivial to apply this before passing
into uinput.  For background info, the bridge approach currently
relies on implementation of the 'buffered' iio interface which uses
chrdevs rather than the hwmon like 'simple' interface provided here.
It would be a bit of a nightmare to bridge from sysfs (if it could
even be done?)  Typically all drivers start with the sysfs interface
and the buffered one is only added if needed by someone.

Note calibscale is meant for internally applied values.  That is ones applied
actually on the device (looking at what you have here, my choice of naming
wasn't ideal, anyone have a better naming suggestion).  The value used to do raw to
standard unit conversion in userspace is magn_x_scale or equivalent.
I think based on the comment in your code that you also need to provide
magn_x_offset.  Looks like our documentation could do to be a little clearer.
Rather tediously these are both computed floating point values here.  Perhaps
we will need to think further on that interface (this is the first time
we have hit this particular situation)

The raw option is principally there because we share the attributes with the
buffer access (there performance dictates doing as little as possible to the
data on the way through.) Admittedly our overheads are way too high at the
moment for other reasons but the intent is there.  If you are intending to
add this interface later then keeping to the _raw attributes makes for a
more consistent choice (though as long as the scale and bias are there for
the buffer you could use the processed value...)

Thanks,

Jonathan