Moving ad2s1200 out of staging

Moving ad2s1200 out of staging

[David Julian Veenstra]

Dear IIO Community,

I have looked at the ad2s1200 driver. To clean up the code, I propose
the following:

1. Replace Licence with SPDX identifier
2. Reorder variable declarations to prefer reverse
    Christmas tree ordering.
3. Add newlines (e.g. before return statement)
4. Add documentation to the device specific struct
5. Sort headers alphabetically
6. Remove indexed property for channels as only a single channel is needed
    per type

If I understand the manual correctly, the unit of the angular velocity is
rps (not entirely sure). So to conform the IIO ABI, a scaling factor
needs to
be added (IIO_CHAN_INFO_SCALE). In addition it also misses either a
MOD_X, MOD_Y, or MOD_Z.

The documentation in Documentation/ABI/testing/sysfs-bus-iio does not
define what the unit should be for an angle. If the velocity is defined
as radians
per second, it would make sense for the angle to be defined in radians.
Is there
any agreement of what the unit should be for angles?

Besides missing features, I did not find anything else wrong with the
driver.

Best regards,
David Veenstra

Re: Moving ad2s1200 out of staging

[Jonathan Cameron]

On Mon, 12 Mar 2018 16:36:50 +0100
David Julian Veenstra <davidjulianveenstra@gmail.com> wrote:

> Dear IIO Community,
> 
> I have looked at the ad2s1200 driver. To clean up the code, I propose
> the following:
> 
> 1. Replace Licence with SPDX identifier
This ha proved controversial and certainly isn't necessary to move
a driver out of staging.

> 2. Reorder variable declarations to prefer reverse
>     Christmas tree ordering.
> 3. Add newlines (e.g. before return statement)
> 4. Add documentation to the device specific struct
> 5. Sort headers alphabetically
> 6. Remove indexed property for channels as only a single channel is needed
>     per type
> 
> If I understand the manual correctly, the unit of the angular velocity is
> rps (not entirely sure). So to conform the IIO ABI, a scaling factor
> needs to
> be added (IIO_CHAN_INFO_SCALE). In addition it also misses either a
> MOD_X, MOD_Y, or MOD_Z.
Nature of resolvers is they don't have any concept of an axis like that
so I wouldn't expect a modifier.

> 
> The documentation in Documentation/ABI/testing/sysfs-bus-iio does not
> define what the unit should be for an angle. If the velocity is defined
> as radians
> per second, it would make sense for the angle to be defined in radians.
> Is there
> any agreement of what the unit should be for angles?
There are other angle measurements already in place be it not simple axial
rotation ones so we need to match them.
See in_incli_x_raw for example.

> 
> Besides missing features, I did not find anything else wrong with the
> driver.
Cool - run through the changes you suggest and then propose a move.
At that point you'll get a thorough review to see if there is anything
much else that needs doing.

I can see some more issues so I'll give a few hints:
1) How much is platform data used these days?
2) Does the platform data there conform to how we do things these days?

> 
> Best regards,
> David Veenstra
> --
> 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

Re: Moving ad2s1200 out of staging

[David Julian Veenstra]

On 03/12/2018 06:05 PM, Jonathan Cameron wrote:
> On Mon, 12 Mar 2018 16:36:50 +0100
> David Julian Veenstra <davidjulianveenstra@gmail.com> wrote:
>
>> Dear IIO Community,
>>
>> I have looked at the ad2s1200 driver. To clean up the code, I propose
>> the following:
>>
>> 1. Replace Licence with SPDX identifier
> This ha proved controversial and certainly isn't necessary to move
> a driver out of staging.
>
>> 2. Reorder variable declarations to prefer reverse
>>     Christmas tree ordering.
>> 3. Add newlines (e.g. before return statement)
>> 4. Add documentation to the device specific struct
>> 5. Sort headers alphabetically
>> 6. Remove indexed property for channels as only a single channel is needed
>>     per type
>>
>> If I understand the manual correctly, the unit of the angular velocity is
>> rps (not entirely sure). So to conform the IIO ABI, a scaling factor
>> needs to
>> be added (IIO_CHAN_INFO_SCALE). In addition it also misses either a
>> MOD_X, MOD_Y, or MOD_Z.
> Nature of resolvers is they don't have any concept of an axis like that
> so I wouldn't expect a modifier.
>
>> The documentation in Documentation/ABI/testing/sysfs-bus-iio does not
>> define what the unit should be for an angle. If the velocity is defined
>> as radians
>> per second, it would make sense for the angle to be defined in radians.
>> Is there
>> any agreement of what the unit should be for angles?
> There are other angle measurements already in place be it not simple axial
> rotation ones so we need to match them.
> See in_incli_x_raw for example.
>
>> Besides missing features, I did not find anything else wrong with the
>> driver.
> Cool - run through the changes you suggest and then propose a move.
> At that point you'll get a thorough review to see if there is anything
> much else that needs doing.
>
> I can see some more issues so I'll give a few hints:
> 1) How much is platform data used these days?
> 2) Does the platform data there conform to how we do things these days?
>
>> Best regards,
>> David Veenstra
>> --
>> 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
Dear Jonathan Cameron,

Thanks for the feedback and the hints. I'll look into the platform data.

Best regards,
David Veenstra