Moving ad2s1200 out of staging
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
Quoted 6 lines · On Mon, 12 Mar 2018 16:36:50 +0100 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
This ha proved controversial and certainly isn't necessary to move a driver out of staging.
Quoted 8 lines
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
Quoted 5 lines
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 aMOD_X,MOD_Y, orMOD_Z.
Nature of resolvers is they don't have any concept of an axis like that so I wouldn't expect a modifier.
Quoted 7 lines
The documentation in
Documentation/ABI/testing/sysfs-bus-iiodoes 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.
Quoted 3 lines
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?
Quoted 7 lines
Best regards,
David Veenstra
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 40 lines · Jonathan Cameron
Quoted 6 lines · On Mon, 12 Mar 2018 16:36:50 +0100 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
This ha proved controversial and certainly isn't necessary to move a driver out of staging.
Quoted 8 lines
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 typeQuoted 5 lines
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 aMOD_X,MOD_Y, orMOD_Z.Nature of resolvers is they don't have any concept of an axis like that so I wouldn't expect a modifier.
Quoted 6 lines
The documentation in
Documentation/ABI/testing/sysfs-bus-iiodoes 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.
Seein_incli_x_rawfor example.Quoted 2 lines
Besides missing features, I did not find anything else wrong with the driver.
Quoted 14 lines
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?Quoted 6 lines
Best regards,
David Veenstra
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
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