staging:iio:ad2s1210: Driver cleanup
Subsystems (4)
cover staging:iio:ad2s1210: Driver cleanup
This patchset removes some unused #define directive and variables. Additionally, it fixes two checkpatch problems. Finally, the last patch adds struct documentation and comments to some part of the code. All of the changes have the intention to improve the readability of new updates.
Rodrigo Siqueira (4):
staging:iio:ad2s1210: Remove end of line with '[' staging:iio:ad2s1210: Remove unused #define directive staging:iio:ad2s1210: Remove old_data from ad2s1210_state staging:iio:ad2s1210: Add comments/documentation
drivers/staging/iio/resolver/ad2s1210.c | 52 +++++++++++++++++++++++---------- drivers/staging/iio/resolver/ad2s1210.h | 9 +++++- 2 files changed, 44 insertions(+), 17 deletions(-)
Signature · 6 lines
2.16.2 _______________________________________________ devel mailing list [email protected] http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
PATCH 1/4 staging:iio:ad2s1210: Remove end of line with '['
This patch fixes the checkpatch.pl check:
iio/resolver/ad2s1210.c:202: CHECK: Lines should not end with a '['
--- drivers/staging/iio/resolver/ad2s1210.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
drivers/staging/iio/resolver/ad2s1210.c +4 -3
@@ -165,9 +165,10 @@ int ad2s1210_update_frequency_control_word(struct ad2s1210_state *st)static unsigned char ad2s1210_read_resolution_pin (struct ad2s1210_state *st) { -return ad2s1210_resolution_value[ - (gpio_get_value (st->pdata->res[0 ]) <<1 ) | -gpio_get_value (st->pdata->res[1 ])]; +int resolution = (gpio_get_value (st->pdata->res[0 ]) <<1 ) | +gpio_get_value (st->pdata->res[1 ]); + +return ad2s1210_resolution_value[resolution]; }static const int ad2s1210_res_pins[4 ][2 ] = {
Signature · 1 line
2.16.2
Jonathan Cameron
maintainerRe: [PATCH 1/4] staging:iio:ad2s1210: Remove end of line with '['
Quoted 5 lines · On Fri, 9 Mar 2018 20:45:50 -0300 Rodrigo Siqueira…
This patch fixes the checkpatch.pl check:
iio/resolver/ad2s1210.c:202: CHECK: Lines should not end with a '['Signed-off-by Rodrigo Siqueira <[email protected]>
Applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to play with it.
Thanks,
Jonathan
Quoted 12 lines
--- drivers/staging/iio/resolver/ad2s1210.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)drivers/staging/iio/resolver/ad2s1210.c +0
-0@@ -165,9 +165,10 @@ int ad2s1210_update_frequency_control_word(struct ad2s1210_state *st)static unsigned char ad2s1210_read_resolution_pin (struct ad2s1210_state *st) {
drivers/staging/iio/resolver/ad2s1210.c int ad2s1210_update_frequency_control_word(struct ad2s1210_state *st) · line 174
Quoted 10 lines
drivers/staging/iio/resolver/ad2s1210.c +4
-3-return ad2s1210_resolution_value[ - (gpio_get_value (st->pdata->res[0 ]) <<1 ) | -gpio_get_value (st->pdata->res[1 ])]; +int resolution = (gpio_get_value (st->pdata->res[0 ]) <<1 ) | +gpio_get_value (st->pdata->res[1 ]); + +return ad2s1210_resolution_value[resolution]; }static const int ad2s1210_res_pins[4 ][2 ] = {
_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
PATCH 2/4 staging:iio:ad2s1210: Remove unused #define directive
In reply to Rodrigo SiqueiraThis patch removes some #define directives not used in the code.
--- drivers/staging/iio/resolver/ad2s1210.c | 10 ---------- 1 file changed, 10 deletions(-)
drivers/staging/iio/resolver/ad2s1210.c +0 -10
@@ -35,8 +35,6 @@#define AD2S1210_SET_RES1 0x02#define AD2S1210_SET_RES0 0x01 -#define AD2S1210_SET_ENRESOLUTION (AD2S1210_SET_ENRES1 | \ - AD2S1210_SET_ENRES0)#define AD2S1210_SET_RESOLUTION (AD2S1210_SET_RES1 | AD2S1210_SET_RES0)#define AD2S1210_REG_POSITION 0x80
@@ -53,10 +51,6 @@#define AD2S1210_REG_SOFT_RESET 0xF0#define AD2S1210_REG_FAULT 0xFF -/* pin SAMPLE, A0, A1, RES0, RES1, is controlled by driver */ -#define AD2S1210_SAA 3 -#define AD2S1210_PN (AD2S1210_SAA + AD2S1210_RES) -#define AD2S1210_MIN_CLKIN 6144000#define AD2S1210_MAX_CLKIN 10240000#define AD2S1210_MIN_EXCIT 2000
@@ -64,10 +58,6 @@#define AD2S1210_MIN_FCW 0x4#define AD2S1210_MAX_FCW 0x50 -/* default input clock on serial interface */ -#define AD2S1210_DEF_CLKIN 8192000 -/* clock period in nano second */ -#define AD2S1210_DEF_TCK (1000000000 / AD2S1210_DEF_CLKIN)#define AD2S1210_DEF_EXCIT 10000enum ad2s1210_mode {
Signature · 6 lines
2.16.2 _______________________________________________ devel mailing list [email protected] http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/4] staging:iio:ad2s1210: Remove unused #define directive
Quoted 3 lines · On Fri, 9 Mar 2018 20:46:05 -0300 Rodrigo Siqueira…
This patch removes some #define directives not used in the code.
Signed-off-by Rodrigo Siqueira <[email protected]>
Applied.
One comment inline.
Quoted 29 lines
--- drivers/staging/iio/resolver/ad2s1210.c | 10 ---------- 1 file changed, 10 deletions(-)drivers/staging/iio/resolver/ad2s1210.c +0
-6@@ -35,8 +35,6 @@#define AD2S1210_SET_RES1 0x02#define AD2S1210_SET_RES0 0x01 -#define AD2S1210_SET_ENRESOLUTION (AD2S1210_SET_ENRES1 | \ - AD2S1210_SET_ENRES0)#define AD2S1210_SET_RESOLUTION (AD2S1210_SET_RES1 | AD2S1210_SET_RES0)#define AD2S1210_REG_POSITION 0x80@@ -53,10 +51,6 @@#define AD2S1210_REG_SOFT_RESET 0xF0#define AD2S1210_REG_FAULT 0xFF -/* pin SAMPLE, A0, A1, RES0, RES1, is controlled by driver */ -#define AD2S1210_SAA 3 -#define AD2S1210_PN (AD2S1210_SAA + AD2S1210_RES) -#define AD2S1210_MIN_CLKIN 6144000#define AD2S1210_MAX_CLKIN 10240000#define AD2S1210_MIN_EXCIT 2000
drivers/staging/iio/resolver/ad2s1210.c line 67
Quoted 4 lines
drivers/staging/iio/resolver/ad2s1210.c +0
-1@@ -64,10 +58,6 @@#define AD2S1210_MIN_FCW 0x4#define AD2S1210_MAX_FCW 0x50 -/* default input clock on serial interface */
I'm not sure what this has to do with the serial interface. It's the clk input from an external clock source.
drivers/staging/iio/resolver/ad2s1210.c line 63
Quoted 6 lines
drivers/staging/iio/resolver/ad2s1210.c +0
-3-#define AD2S1210_DEF_CLKIN 8192000 -/* clock period in nano second */ -#define AD2S1210_DEF_TCK (1000000000 / AD2S1210_DEF_CLKIN)#define AD2S1210_DEF_EXCIT 10000enum ad2s1210_mode {
_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
PATCH 3/4 staging:iio:ad2s1210: Remove old_data from ad2s1210_state
In reply to Rodrigo SiqueiraThe variable old_data is a bool type, which only receives the value 'true' in the function ad2s1210_config_write and ad2s1210_config_read. There is no other use for this variable. This patch removes old_data from the ad2s1210_state and from all the function that use it.
--- drivers/staging/iio/resolver/ad2s1210.c | 3 --- 1 file changed, 3 deletions(-)
drivers/staging/iio/resolver/ad2s1210.c +0 -3
@@ -76,7 +76,6 @@ struct ad2s1210_state {unsigned int fclkin;unsigned int fexcit;bool hysteresis; -bool old_data;u8 resolution;enum ad2s1210_mode mode;u8 rx[2 ]____cacheline_aligned ;
@@ -107,7 +106,6 @@ static int ad2s1210_config_write(struct ad2s1210_state *st, u8 data) ret =spi_write (st->sdev, st->tx,1 );if (ret <0 )return ret; - st->old_data = true;return 0 ; }
@@ -129,7 +127,6 @@ static int ad2s1210_config_read(struct ad2s1210_state *st, ret =spi_sync_transfer (st->sdev, &xfer,1 );if (ret <0 )return ret; - st->old_data = true;return st->rx[1 ]; }
Signature · 6 lines
2.16.2 _______________________________________________ devel mailing list [email protected] http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/4] staging:iio:ad2s1210: Remove old_data from ad2s1210_state
Quoted 6 lines · On Fri, 9 Mar 2018 20:46:25 -0300 Rodrigo Siqueira…
The variable
old_datais a bool type, which only receives the value 'true' in the functionad2s1210_config_writeandad2s1210_config_read. There is no other use for this variable. This patch removesold_datafrom thead2s1210_stateand from all the function that use it.Signed-off-by Rodrigo Siqueira <[email protected]>
Applied.
Thanks,
Jonathan
Quoted 25 lines
--- drivers/staging/iio/resolver/ad2s1210.c | 3 --- 1 file changed, 3 deletions(-)drivers/staging/iio/resolver/ad2s1210.c +0
-2@@ -76,7 +76,6 @@ struct ad2s1210_state {unsigned int fclkin;unsigned int fexcit;bool hysteresis; -bool old_data;u8 resolution;enum ad2s1210_mode mode;u8 rx[2 ]____cacheline_aligned ;@@ -107,7 +106,6 @@ static int ad2s1210_config_write(struct ad2s1210_state *st, u8 data) ret =spi_write (st->sdev, st->tx,1 );if (ret <0 )return ret; - st->old_data = true;return 0 ; }
drivers/staging/iio/resolver/ad2s1210.c static int ad2s1210_config_read(struct ad2s1210_state *st, · line 132
Quoted 7 lines
drivers/staging/iio/resolver/ad2s1210.c +0
-1@@ -129,7 +127,6 @@ static int ad2s1210_config_read(struct ad2s1210_state *st, ret =spi_sync_transfer (st->sdev, &xfer,1 );if (ret <0 )return ret; - st->old_data = true;return st->rx[1 ]; }
_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
PATCH 4/4 staging:iio:ad2s1210: Add comments/documentation
In reply to Rodrigo SiqueiraThe original code of AD2S1210 does not have documentation for structs and register configurations; this difficult the code comprehension. This patch adds structs documentation, briefly comments some register settings and acronyms, and adds little explanations of some calculation found in the code.
--- drivers/staging/iio/resolver/ad2s1210.c | 32 ++++++++++++++++++++++++++++++++ drivers/staging/iio/resolver/ad2s1210.h | 9 ++++++++- 2 files changed, 40 insertions(+), 1 deletion(-)
drivers/staging/iio/resolver/ad2s1210.c +32 -0
@@ -24,8 +24,10 @@#define DRV_NAME "ad2s1210" +/* The default value of the control register on power-up */ #define AD2S1210_DEF_CONTROL 0x7E +/* Control Register Bit */ #define AD2S1210_MSB_IS_HIGH 0x80#define AD2S1210_MSB_IS_LOW 0x7F#define AD2S1210_PHASE_LOCK_RANGE_44 0x20
@@ -39,14 +41,23 @@#define AD2S1210_REG_POSITION 0x80#define AD2S1210_REG_VELOCITY 0x82 + +/* Loss of Signal (LOS) register address */ #define AD2S1210_REG_LOS_THRD 0x88 + +/* Degradation of Signal (DOS) register address */ #define AD2S1210_REG_DOS_OVR_THRD 0x89#define AD2S1210_REG_DOS_MIS_THRD 0x8A#define AD2S1210_REG_DOS_RST_MAX_THRD 0x8B#define AD2S1210_REG_DOS_RST_MIN_THRD 0x8C + +/* Loss of Tracking (LOT) register address */ #define AD2S1210_REG_LOT_HIGH_THRD 0x8D#define AD2S1210_REG_LOT_LOW_THRD 0x8E + +/* Excitation Frequency (EXCIT) register address */ #define AD2S1210_REG_EXCIT_FREQ 0x91 +#define AD2S1210_REG_CONTROL 0x92#define AD2S1210_REG_SOFT_RESET 0xF0#define AD2S1210_REG_FAULT 0xFF
@@ -69,6 +80,20 @@ enum ad2s1210_mode {static const unsigned int ad2s1210_resolution_value[] = {10 ,12 ,14 ,16 }; +/** +* struct ad2s1210_state - device instance specific state. +* @pdata: chip model specific constants, gpioin, etc +* @lock: lock to ensure state is consistent +* @sdev: the SPI device for this driver instance +* @fclkin: frequency of clock input +* @fexcit: excitation frequency +* @hysteresis: cache of whether hysteresis is enabled +* @old_data: cache of SPI communication after operation +* @resolution: chip resolution could be 10/12/14/16-bit +* @mode: indicates the operating mode +* @rx: receive buffer +* @tx: transmit buffer +*/ struct ad2s1210_state {const struct ad2s1210_platform_data *pdata;struct mutex lock;
@@ -82,6 +107,7 @@ struct ad2s1210_state {u8 tx[2 ]____cacheline_aligned ; }; +/* Maps A0 and A1 inputs to the respective mode. */ static const int ad2s1210_mode_vals[4 ][2 ] = { [MOD_POS ] = {0 ,0 }, [MOD_VEL ] = {0 ,1 },
@@ -137,6 +163,11 @@ int ad2s1210_update_frequency_control_word(struct ad2s1210_state *st)int ret;unsigned char fcw; +/* +* The fcw stands for frequency control word, which can be obtained +* from: +* fcw = (Excitation Frequency * 2^15) / fclkin +*/ fcw = (unsigned char )(st->fexcit * (1 <<15 ) / st->fclkin);if (fcw <AD2S1210_MIN_FCW || fcw >AD2S1210_MAX_FCW ) {dev_err (&st->sdev->dev,"ad2s1210: FCW out of range\n" );
@@ -158,6 +189,7 @@ static unsigned char ad2s1210_read_resolution_pin(struct ad2s1210_state *st)return ad2s1210_resolution_value[resolution]; } +/* Maps RES0 and RES1 inputs to the respective mode. */ static const int ad2s1210_res_pins[4 ][2 ] = { {0 ,0 }, {0 ,1 }, {1 ,0 }, {1 ,1 } };
drivers/staging/iio/resolver/ad2s1210.h +8 -1
@@ -1,5 +1,5 @@/* -* ad2s1210.h plaform data for the ADI Resolver to Digital Converters: +* ad2s1210.h platform data for the ADI Resolver to Digital Converters: * AD2S1210 * * Copyright (c) 2010-2010 Analog Devices Inc.
@@ -11,6 +11,13 @@ #ifndef _AD2S1210_H #define _AD2S1210_H +/** + * struct ad2s1210_platform_data - chip model + * @sample: sample input used to clearing the fault register + * @a: array of inputs (A0 and A1) + * @res: array of resolution inputs (RES0 and RES1) + * @gpioin: control the read operation + */ struct ad2s1210_platform_data { unsigned int sample; unsigned int a[2];
Signature · 1 line
2.16.2
Jonathan Cameron
maintainerRe: [PATCH 4/4] staging:iio:ad2s1210: Add comments/documentation
Quoted 7 lines · On Fri, 9 Mar 2018 20:46:40 -0300 Rodrigo Siqueira…
The original code of AD2S1210 does not have documentation for structs and register configurations; this difficult the code comprehension. This patch adds structs documentation, briefly comments some register settings and acronyms, and adds little explanations of some calculation found in the code.
Signed-off-by Rodrigo Siqueira <[email protected]>
Various comments inline.
Only a few of them are about you actual patch - mostly more general.
I'd look at renaming all those defines to be more consistent. There is no association between bits of a register and the register at the moment which will make the code rather error prone.
Note this is going to be a difficult driver to get out of staging. There is quite a bit to do and we don't currently have anyone who has test hardware as far as I know. So brave move ;)
Thanks,
Jonathan
Quoted 10 lines
--- drivers/staging/iio/resolver/ad2s1210.c | 32 ++++++++++++++++++++++++++++++++ drivers/staging/iio/resolver/ad2s1210.h | 9 ++++++++- 2 files changed, 40 insertions(+), 1 deletion(-)drivers/staging/iio/resolver/ad2s1210.c +0
-0
drivers/staging/iio/resolver/ad2s1210.c line 30
Quoted 7 lines
drivers/staging/iio/resolver/ad2s1210.c +2
-0@@ -24,8 +24,10 @@#define DRV_NAME "ad2s1210" +/* The default value of the control register on power-up */ #define AD2S1210_DEF_CONTROL 0x7E +/* Control Register Bit */
I would change the defines to make this explicit.
This is a truely odd bit of naming anyway.
#define AD2S1210_ADDRESS 0x80
#define AD2S1210_DATA 0x00
and perhaps a
#define AD2S1210_DATA_MASK 0x7F
would make sense?
drivers/staging/iio/resolver/ad2s1210.c line 48
Quoted 12 lines
drivers/staging/iio/resolver/ad2s1210.c +4
-0#define AD2S1210_MSB_IS_HIGH 0x80#define AD2S1210_MSB_IS_LOW 0x7F#define AD2S1210_PHASE_LOCK_RANGE_44 0x20@@ -39,14 +41,23 @@#define AD2S1210_REG_POSITION 0x80#define AD2S1210_REG_VELOCITY 0x82 + +/* Loss of Signal (LOS) register address */ #define AD2S1210_REG_LOS_THRD 0x88 + +/* Degradation of Signal (DOS) register address */
addresses
drivers/staging/iio/resolver/ad2s1210.c line 54
Quoted 6 lines
drivers/staging/iio/resolver/ad2s1210.c +2
-0#define AD2S1210_REG_DOS_OVR_THRD 0x89#define AD2S1210_REG_DOS_MIS_THRD 0x8A#define AD2S1210_REG_DOS_RST_MAX_THRD 0x8B#define AD2S1210_REG_DOS_RST_MIN_THRD 0x8C + +/* Loss of Tracking (LOT) register address */
addresses
Quoted 10 lines
drivers/staging/iio/resolver/ad2s1210.c +3
-0#define AD2S1210_REG_LOT_HIGH_THRD 0x8D#define AD2S1210_REG_LOT_LOW_THRD 0x8E + +/* Excitation Frequency (EXCIT) register address */ #define AD2S1210_REG_EXCIT_FREQ 0x91 +#define AD2S1210_REG_CONTROL 0x92#define AD2S1210_REG_SOFT_RESET 0xF0#define AD2S1210_REG_FAULT 0xFF
drivers/staging/iio/resolver/ad2s1210.c enum ad2s1210_mode { · line 85
Quoted 6 lines
drivers/staging/iio/resolver/ad2s1210.c +3
-0@@ -69,6 +80,20 @@ enum ad2s1210_mode {static const unsigned int ad2s1210_resolution_value[] = {10 ,12 ,14 ,16 }; +/** +* struct ad2s1210_state - device instance specific state. +* @pdata: chip model specific constants, gpioin, etc
Except they aren't anything to do with the chip model. This is about how it is wired not what it is.
drivers/staging/iio/resolver/ad2s1210.c enum ad2s1210_mode { · line 91
Quoted 6 lines
drivers/staging/iio/resolver/ad2s1210.c +6
-0+* @lock: lock to ensure state is consistent +* @sdev: the SPI device for this driver instance +* @fclkin: frequency of clock input +* @fexcit: excitation frequency +* @hysteresis: cache of whether hysteresis is enabled +* @old_data: cache of SPI communication after operation
Umm. You got rid of this one in the earlier patch didn't you?
Quoted 2 lines
drivers/staging/iio/resolver/ad2s1210.c +1
-0+* @resolution: chip resolution could be 10/12/14/16-bit From reading the datasheet quickly I suspect there is a 'best possible'
resolution given a particular set of controls. I'm not sure we want to expose this to userspace at all.
Quoted 1 line
+ * @mode: indicates the operating mode
Where operating mode is what? Comment would be more useful if it listed them.
Quoted 15 lines
+ * @rx: receive buffer
+ * @tx: transmit buffer
+ */
structad2s1210_state{drivers/staging/iio/resolver/ad2s1210.c +1
-0const struct ad2s1210_platform_data *pdata;struct mutex lock;@@ -82,6 +107,7 @@ struct ad2s1210_state {u8 tx[2 ]____cacheline_aligned ; }; +/* Maps A0 and A1 inputs to the respective mode. */ static const int ad2s1210_mode_vals[4 ][2 ] = { [MOD_POS ] = {0 ,0 }, [MOD_VEL ] = {0 ,1 },
drivers/staging/iio/resolver/ad2s1210.c int ad2s1210_update_frequency_control_word(struct ad2s1210_state *st) · line 170
Quoted 8 lines
drivers/staging/iio/resolver/ad2s1210.c +5
-0@@ -137,6 +163,11 @@ int ad2s1210_update_frequency_control_word(struct ad2s1210_state *st)int ret;unsigned char fcw; +/* +* The fcw stands for frequency control word, which can be obtained +* from: +* fcw = (Excitation Frequency * 2^15) / fclkin +*/
Whilst we are here - userspace being responsible for writing a hardware frequency input needs to change. Makes no sense.
Quoted 16 lines
drivers/staging/iio/resolver/ad2s1210.c +1
-0fcw = (unsigned char )(st->fexcit * (1 <<15 ) / st->fclkin);if (fcw <AD2S1210_MIN_FCW || fcw >AD2S1210_MAX_FCW ) {dev_err (&st->sdev->dev,"ad2s1210: FCW out of range\n" );@@ -158,6 +189,7 @@ static unsigned char ad2s1210_read_resolution_pin(struct ad2s1210_state *st)return ad2s1210_resolution_value[resolution]; } +/* Maps RES0 and RES1 inputs to the respective mode. */ static const int ad2s1210_res_pins[4 ][2 ] = { {0 ,0 }, {0 ,1 }, {1 ,0 }, {1 ,1 } };drivers/staging/iio/resolver/ad2s1210.h +0
-0
drivers/staging/iio/resolver/ad2s1210.h line 3
Quoted 4 lines
drivers/staging/iio/resolver/ad2s1210.h +1
-1@@ -1,5 +1,5 @@/* -* ad2s1210.h plaform data for the ADI Resolver to Digital Converters: +* ad2s1210.h platform data for the ADI Resolver to Digital Converters: * AD2S1210
Hmm. I would suggest that, seeing as we don't have any in kernel users we should probably just drop the platform data in favour of a devicetree binding.
Fair enough to document it as an intermediate step however.
drivers/staging/iio/resolver/ad2s1210.h line 16
Quoted 9 lines
drivers/staging/iio/resolver/ad2s1210.h +3
-0* * Copyright (c) 2010-2010 Analog Devices Inc. @@ -11,6 +11,13 @@#ifndef _AD2S1210_H#define _AD2S1210_H +/** +* struct ad2s1210_platform_data - chip model +* @sample: sample input used to clearing the fault register
This hasn't been a good means of proving a gpio for some time. These all want converting over to the current gpio handling best practice.
drivers/staging/iio/resolver/ad2s1210.h line 19
Quoted 3 lines
drivers/staging/iio/resolver/ad2s1210.h +3
-0+* @a: array of inputs (A0 and A1) +* @res: array of resolution inputs (RES0 and RES1) +* @gpioin: control the read operation
In what way? I think this is actually a hack to allow for the fact that the above pins may not be controllable by the driver. Not sure though as I haven't chased through the code fully.
drivers/staging/iio/resolver/ad2s1210.h line 23
Quoted 4 lines
drivers/staging/iio/resolver/ad2s1210.h +1
-0+*/ struct ad2s1210_platform_data {unsigned int sample;unsigned int a[2 ];
_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/4] staging:iio:ad2s1210: Add comments/documentation
Quoted 14 lines · Jonathan Cameron
Quoted 7 lines · On Fri, 9 Mar 2018 20:46:40 -0300 Rodrigo Siqueira…
The original code of AD2S1210 does not have documentation for structs and register configurations; this difficult the code comprehension. This patch adds structs documentation, briefly comments some register settings and acronyms, and adds little explanations of some calculation found in the code.
Signed-off-by Rodrigo Siqueira <[email protected]>Various comments inline.
Only a few of them are about you actual patch - mostly more general.
Quoted 7 lines
I'd look at renaming all those defines to be more consistent. There is no association between bits of a register and the register at the moment which will make the code rather error prone.
Note this is going to be a difficult driver to get out of staging. There is quite a bit to do and we don't currently have anyone who has test hardware as far as I know. So brave move ;)
Hi Jonathan,
After careful reading your email, I believe that is a better idea to divide this kind of work in other patches. So, instead of trying to document the module at once, I will do it step by step in the future patches series; I take note of all your comments. I will put an effort in this module because I think that is an excellent opportunity to learn the IIO subsystem. Finally, I will try to contact Analog Devices; maybe someone can test the module for me.
Thanks for all the reviews and comments, I learned a lot with all your explanations :)
Quoted 164 lines
Thanks,
Jonathan
Quoted 10 lines
--- drivers/staging/iio/resolver/ad2s1210.c | 32 ++++++++++++++++++++++++++++++++ drivers/staging/iio/resolver/ad2s1210.h | 9 ++++++++- 2 files changed, 40 insertions(+), 1 deletion(-)drivers/staging/iio/resolver/ad2s1210.c +0
-0drivers/staging/iio/resolver/ad2s1210.c line 30
Quoted 7 lines
drivers/staging/iio/resolver/ad2s1210.c +2
-0@@ -24,8 +24,10 @@#define DRV_NAME "ad2s1210" +/* The default value of the control register on power-up */ #define AD2S1210_DEF_CONTROL 0x7E +/* Control Register Bit */ I would change the defines to make this explicit.
This is a truely odd bit of naming anyway.
#defineAD2S1210_ADDRESS0x80
#defineAD2S1210_DATA0x00
and perhaps a
#defineAD2S1210_DATA_MASK0x7Fwould make sense?
drivers/staging/iio/resolver/ad2s1210.c line 48
Quoted 12 lines
drivers/staging/iio/resolver/ad2s1210.c +4
-0#define AD2S1210_MSB_IS_HIGH 0x80#define AD2S1210_MSB_IS_LOW 0x7F#define AD2S1210_PHASE_LOCK_RANGE_44 0x20@@ -39,14 +41,23 @@#define AD2S1210_REG_POSITION 0x80#define AD2S1210_REG_VELOCITY 0x82 + +/* Loss of Signal (LOS) register address */ #define AD2S1210_REG_LOS_THRD 0x88 + +/* Degradation of Signal (DOS) register address */ addresses
drivers/staging/iio/resolver/ad2s1210.c line 54
Quoted 6 lines
drivers/staging/iio/resolver/ad2s1210.c +2
-0#define AD2S1210_REG_DOS_OVR_THRD 0x89#define AD2S1210_REG_DOS_MIS_THRD 0x8A#define AD2S1210_REG_DOS_RST_MAX_THRD 0x8B#define AD2S1210_REG_DOS_RST_MIN_THRD 0x8C + +/* Loss of Tracking (LOT) register address */ addresses
Quoted 10 lines
drivers/staging/iio/resolver/ad2s1210.c +3
-0#define AD2S1210_REG_LOT_HIGH_THRD 0x8D#define AD2S1210_REG_LOT_LOW_THRD 0x8E + +/* Excitation Frequency (EXCIT) register address */ #define AD2S1210_REG_EXCIT_FREQ 0x91 +#define AD2S1210_REG_CONTROL 0x92#define AD2S1210_REG_SOFT_RESET 0xF0#define AD2S1210_REG_FAULT 0xFFdrivers/staging/iio/resolver/ad2s1210.c enum ad2s1210_mode { · line 85
Quoted 6 lines
drivers/staging/iio/resolver/ad2s1210.c +3
-0@@ -69,6 +80,20 @@ enum ad2s1210_mode {static const unsigned int ad2s1210_resolution_value[] = {10 ,12 ,14 ,16 }; +/** +* struct ad2s1210_state - device instance specific state. +* @pdata: chip model specific constants, gpioin, etc Except they aren't anything to do with the chip model. This is about how it is wired not what it is.
drivers/staging/iio/resolver/ad2s1210.c enum ad2s1210_mode { · line 91
Quoted 6 lines
drivers/staging/iio/resolver/ad2s1210.c +6
-0+* @lock: lock to ensure state is consistent +* @sdev: the SPI device for this driver instance +* @fclkin: frequency of clock input +* @fexcit: excitation frequency +* @hysteresis: cache of whether hysteresis is enabled +* @old_data: cache of SPI communication after operation Umm. You got rid of this one in the earlier patch didn't you?
drivers/staging/iio/resolver/ad2s1210.c enum ad2s1210_mode { · line 92
Quoted 1 line
drivers/staging/iio/resolver/ad2s1210.c +1
-0+* @resolution: chip resolution could be 10/12/14/16-bit From reading the datasheet quickly I suspect there is a 'best possible' resolution given a particular set of controls. I'm not sure we want to expose this to userspace at all.
drivers/staging/iio/resolver/ad2s1210.c enum ad2s1210_mode { · line 93
Quoted 1 line
drivers/staging/iio/resolver/ad2s1210.c +1
-0+* @mode: indicates the operating mode Where operating mode is what? Comment would be more useful if it listed them.
Quoted 15 lines
drivers/staging/iio/resolver/ad2s1210.c +4
-0+* @rx: receive buffer +* @tx: transmit buffer +*/ struct ad2s1210_state {const struct ad2s1210_platform_data *pdata;struct mutex lock;@@ -82,6 +107,7 @@ struct ad2s1210_state {u8 tx[2 ]____cacheline_aligned ; }; +/* Maps A0 and A1 inputs to the respective mode. */ static const int ad2s1210_mode_vals[4 ][2 ] = { [MOD_POS ] = {0 ,0 }, [MOD_VEL ] = {0 ,1 },drivers/staging/iio/resolver/ad2s1210.c int ad2s1210_update_frequency_control_word(struct ad2s1210_state *st) · line 170
Quoted 8 lines
drivers/staging/iio/resolver/ad2s1210.c +5
-0@@ -137,6 +163,11 @@ int ad2s1210_update_frequency_control_word(struct ad2s1210_state *st)int ret;unsigned char fcw; +/* +* The fcw stands for frequency control word, which can be obtained +* from: +* fcw = (Excitation Frequency * 2^15) / fclkin +*/ Whilst we are here - userspace being responsible for writing a hardware frequency input needs to change. Makes no sense.
Quoted 16 lines
drivers/staging/iio/resolver/ad2s1210.c +1
-0fcw = (unsigned char )(st->fexcit * (1 <<15 ) / st->fclkin);if (fcw <AD2S1210_MIN_FCW || fcw >AD2S1210_MAX_FCW ) {dev_err (&st->sdev->dev,"ad2s1210: FCW out of range\n" );@@ -158,6 +189,7 @@ static unsigned char ad2s1210_read_resolution_pin(struct ad2s1210_state *st)return ad2s1210_resolution_value[resolution]; } +/* Maps RES0 and RES1 inputs to the respective mode. */ static const int ad2s1210_res_pins[4 ][2 ] = { {0 ,0 }, {0 ,1 }, {1 ,0 }, {1 ,1 } };drivers/staging/iio/resolver/ad2s1210.h +0
-0drivers/staging/iio/resolver/ad2s1210.h line 3
Quoted 4 lines
drivers/staging/iio/resolver/ad2s1210.h +1
-1@@ -1,5 +1,5 @@/* -* ad2s1210.h plaform data for the ADI Resolver to Digital Converters: +* ad2s1210.h platform data for the ADI Resolver to Digital Converters: * AD2S1210 Hmm. I would suggest that, seeing as we don't have any in kernel users we should probably just drop the platform data in favour of a devicetree binding.
Fair enough to document it as an intermediate step however.
drivers/staging/iio/resolver/ad2s1210.h line 16
Quoted 9 lines
drivers/staging/iio/resolver/ad2s1210.h +3
-0* * Copyright (c) 2010-2010 Analog Devices Inc. @@ -11,6 +11,13 @@#ifndef _AD2S1210_H#define _AD2S1210_H +/** +* struct ad2s1210_platform_data - chip model +* @sample: sample input used to clearing the fault register This hasn't been a good means of proving a gpio for some time. These all want converting over to the current gpio handling best practice.
drivers/staging/iio/resolver/ad2s1210.h line 19
Quoted 3 lines
drivers/staging/iio/resolver/ad2s1210.h +3
-0+* @a: array of inputs (A0 and A1) +* @res: array of resolution inputs (RES0 and RES1) +* @gpioin: control the read operation In what way? I think this is actually a hack to allow for the fact that the above pins may not be controllable by the driver. Not sure though as I haven't chased through the code fully.
Quoted 4 lines
drivers/staging/iio/resolver/ad2s1210.h +1
-0+*/ struct ad2s1210_platform_data {unsigned int sample;unsigned int a[2 ];