* [PATCH 0/4] staging:iio:ad2s1210: Driver cleanup
@ 2018-03-09 23:45 Rodrigo Siqueira
2018-03-09 23:45 ` [PATCH 1/4] staging:iio:ad2s1210: Remove end of line with '[' Rodrigo Siqueira
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Rodrigo Siqueira @ 2018-03-09 23:45 UTC (permalink / raw)
To: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman,
Graff Yang
Cc: linux-iio, devel, daniel.baluta, linux-kernel
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(-)
--
2.16.2
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 1/4] staging:iio:ad2s1210: Remove end of line with '[' 2018-03-09 23:45 [PATCH 0/4] staging:iio:ad2s1210: Driver cleanup Rodrigo Siqueira @ 2018-03-09 23:45 ` Rodrigo Siqueira 2018-03-10 16:41 ` Jonathan Cameron 2018-03-09 23:46 ` [PATCH 2/4] staging:iio:ad2s1210: Remove unused #define directive Rodrigo Siqueira ` (2 subsequent siblings) 3 siblings, 1 reply; 10+ messages in thread From: Rodrigo Siqueira @ 2018-03-09 23:45 UTC (permalink / raw) To: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman, Graff Yang Cc: linux-iio, devel, daniel.baluta, linux-kernel This patch fixes the checkpatch.pl check: iio/resolver/ad2s1210.c:202: CHECK: Lines should not end with a '[' Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> --- drivers/staging/iio/resolver/ad2s1210.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c index f8baab061eba..0cdcd71e285b 100644 --- a/drivers/staging/iio/resolver/ad2s1210.c +++ b/drivers/staging/iio/resolver/ad2s1210.c @@ -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] = { -- 2.16.2 _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/4] staging:iio:ad2s1210: Remove end of line with '[' 2018-03-09 23:45 ` [PATCH 1/4] staging:iio:ad2s1210: Remove end of line with '[' Rodrigo Siqueira @ 2018-03-10 16:41 ` Jonathan Cameron 0 siblings, 0 replies; 10+ messages in thread From: Jonathan Cameron @ 2018-03-10 16:41 UTC (permalink / raw) To: Rodrigo Siqueira Cc: devel, Graff Yang, Lars-Peter Clausen, Michael Hennerich, linux-iio, Greg Kroah-Hartman, linux-kernel, Peter Meerwald-Stadler, Hartmut Knaack, daniel.baluta On Fri, 9 Mar 2018 20:45:50 -0300 Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> wrote: > This patch fixes the checkpatch.pl check: > > iio/resolver/ad2s1210.c:202: CHECK: Lines should not end with a '[' > > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> Applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to play with it. Thanks, Jonathan > --- > drivers/staging/iio/resolver/ad2s1210.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c > index f8baab061eba..0cdcd71e285b 100644 > --- a/drivers/staging/iio/resolver/ad2s1210.c > +++ b/drivers/staging/iio/resolver/ad2s1210.c > @@ -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] = { _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/4] staging:iio:ad2s1210: Remove unused #define directive 2018-03-09 23:45 [PATCH 0/4] staging:iio:ad2s1210: Driver cleanup Rodrigo Siqueira 2018-03-09 23:45 ` [PATCH 1/4] staging:iio:ad2s1210: Remove end of line with '[' Rodrigo Siqueira @ 2018-03-09 23:46 ` Rodrigo Siqueira 2018-03-10 16:45 ` Jonathan Cameron 2018-03-09 23:46 ` [PATCH 3/4] staging:iio:ad2s1210: Remove old_data from ad2s1210_state Rodrigo Siqueira 2018-03-09 23:46 ` [PATCH 4/4] staging:iio:ad2s1210: Add comments/documentation Rodrigo Siqueira 3 siblings, 1 reply; 10+ messages in thread From: Rodrigo Siqueira @ 2018-03-09 23:46 UTC (permalink / raw) To: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman, Graff Yang Cc: linux-iio, devel, daniel.baluta, linux-kernel This patch removes some #define directives not used in the code. Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> --- drivers/staging/iio/resolver/ad2s1210.c | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c index 0cdcd71e285b..79cb56670fc2 100644 --- a/drivers/staging/iio/resolver/ad2s1210.c +++ b/drivers/staging/iio/resolver/ad2s1210.c @@ -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 10000 enum ad2s1210_mode { -- 2.16.2 _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/4] staging:iio:ad2s1210: Remove unused #define directive 2018-03-09 23:46 ` [PATCH 2/4] staging:iio:ad2s1210: Remove unused #define directive Rodrigo Siqueira @ 2018-03-10 16:45 ` Jonathan Cameron 0 siblings, 0 replies; 10+ messages in thread From: Jonathan Cameron @ 2018-03-10 16:45 UTC (permalink / raw) To: Rodrigo Siqueira Cc: devel, Graff Yang, Lars-Peter Clausen, Michael Hennerich, linux-iio, Greg Kroah-Hartman, linux-kernel, Peter Meerwald-Stadler, Hartmut Knaack, daniel.baluta On Fri, 9 Mar 2018 20:46:05 -0300 Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> wrote: > This patch removes some #define directives not used in the code. > > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> Applied. One comment inline. > --- > drivers/staging/iio/resolver/ad2s1210.c | 10 ---------- > 1 file changed, 10 deletions(-) > > diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c > index 0cdcd71e285b..79cb56670fc2 100644 > --- a/drivers/staging/iio/resolver/ad2s1210.c > +++ b/drivers/staging/iio/resolver/ad2s1210.c > @@ -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 */ I'm not sure what this has to do with the serial interface. It's the clk input from an external clock source. > -#define AD2S1210_DEF_CLKIN 8192000 > -/* clock period in nano second */ > -#define AD2S1210_DEF_TCK (1000000000 / AD2S1210_DEF_CLKIN) > #define AD2S1210_DEF_EXCIT 10000 > > enum ad2s1210_mode { _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/4] staging:iio:ad2s1210: Remove old_data from ad2s1210_state 2018-03-09 23:45 [PATCH 0/4] staging:iio:ad2s1210: Driver cleanup Rodrigo Siqueira 2018-03-09 23:45 ` [PATCH 1/4] staging:iio:ad2s1210: Remove end of line with '[' Rodrigo Siqueira 2018-03-09 23:46 ` [PATCH 2/4] staging:iio:ad2s1210: Remove unused #define directive Rodrigo Siqueira @ 2018-03-09 23:46 ` Rodrigo Siqueira 2018-03-10 17:21 ` Jonathan Cameron 2018-03-09 23:46 ` [PATCH 4/4] staging:iio:ad2s1210: Add comments/documentation Rodrigo Siqueira 3 siblings, 1 reply; 10+ messages in thread From: Rodrigo Siqueira @ 2018-03-09 23:46 UTC (permalink / raw) To: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman, Graff Yang Cc: linux-iio, devel, daniel.baluta, linux-kernel The 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. Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> --- drivers/staging/iio/resolver/ad2s1210.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c index 79cb56670fc2..ac13b99bd9cb 100644 --- a/drivers/staging/iio/resolver/ad2s1210.c +++ b/drivers/staging/iio/resolver/ad2s1210.c @@ -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]; } -- 2.16.2 _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 3/4] staging:iio:ad2s1210: Remove old_data from ad2s1210_state 2018-03-09 23:46 ` [PATCH 3/4] staging:iio:ad2s1210: Remove old_data from ad2s1210_state Rodrigo Siqueira @ 2018-03-10 17:21 ` Jonathan Cameron 0 siblings, 0 replies; 10+ messages in thread From: Jonathan Cameron @ 2018-03-10 17:21 UTC (permalink / raw) To: Rodrigo Siqueira Cc: devel, Graff Yang, Lars-Peter Clausen, Michael Hennerich, linux-iio, Greg Kroah-Hartman, linux-kernel, Peter Meerwald-Stadler, Hartmut Knaack, daniel.baluta On Fri, 9 Mar 2018 20:46:25 -0300 Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> wrote: > The 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. > > Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> Applied. Thanks, Jonathan > --- > drivers/staging/iio/resolver/ad2s1210.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c > index 79cb56670fc2..ac13b99bd9cb 100644 > --- a/drivers/staging/iio/resolver/ad2s1210.c > +++ b/drivers/staging/iio/resolver/ad2s1210.c > @@ -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]; > } _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 4/4] staging:iio:ad2s1210: Add comments/documentation 2018-03-09 23:45 [PATCH 0/4] staging:iio:ad2s1210: Driver cleanup Rodrigo Siqueira ` (2 preceding siblings ...) 2018-03-09 23:46 ` [PATCH 3/4] staging:iio:ad2s1210: Remove old_data from ad2s1210_state Rodrigo Siqueira @ 2018-03-09 23:46 ` Rodrigo Siqueira 2018-03-10 17:52 ` Jonathan Cameron 3 siblings, 1 reply; 10+ messages in thread From: Rodrigo Siqueira @ 2018-03-09 23:46 UTC (permalink / raw) To: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman, Graff Yang Cc: linux-iio, devel, daniel.baluta, linux-kernel 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 <rodrigosiqueiramelo@gmail.com> --- drivers/staging/iio/resolver/ad2s1210.c | 32 ++++++++++++++++++++++++++++++++ drivers/staging/iio/resolver/ad2s1210.h | 9 ++++++++- 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c index ac13b99bd9cb..9bb8fd782f5a 100644 --- a/drivers/staging/iio/resolver/ad2s1210.c +++ b/drivers/staging/iio/resolver/ad2s1210.c @@ -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} }; diff --git a/drivers/staging/iio/resolver/ad2s1210.h b/drivers/staging/iio/resolver/ad2s1210.h index e9b2147701fc..cbe21bca7638 100644 --- a/drivers/staging/iio/resolver/ad2s1210.h +++ b/drivers/staging/iio/resolver/ad2s1210.h @@ -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]; -- 2.16.2 _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 4/4] staging:iio:ad2s1210: Add comments/documentation 2018-03-09 23:46 ` [PATCH 4/4] staging:iio:ad2s1210: Add comments/documentation Rodrigo Siqueira @ 2018-03-10 17:52 ` Jonathan Cameron 2018-03-12 12:25 ` Rodrigo Siqueira 0 siblings, 1 reply; 10+ messages in thread From: Jonathan Cameron @ 2018-03-10 17:52 UTC (permalink / raw) To: Rodrigo Siqueira Cc: devel, Graff Yang, Lars-Peter Clausen, Michael Hennerich, linux-iio, Greg Kroah-Hartman, linux-kernel, Peter Meerwald-Stadler, Hartmut Knaack, daniel.baluta On Fri, 9 Mar 2018 20:46:40 -0300 Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> wrote: > 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 <rodrigosiqueiramelo@gmail.com> 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 > --- > drivers/staging/iio/resolver/ad2s1210.c | 32 ++++++++++++++++++++++++++++++++ > drivers/staging/iio/resolver/ad2s1210.h | 9 ++++++++- > 2 files changed, 40 insertions(+), 1 deletion(-) > > diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c > index ac13b99bd9cb..9bb8fd782f5a 100644 > --- a/drivers/staging/iio/resolver/ad2s1210.c > +++ b/drivers/staging/iio/resolver/ad2s1210.c > @@ -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? > #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 > #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 > #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 Except they aren't anything to do with the chip model. This is about how it is wired not what it is. > + * @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? > + * @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. > + * @mode: indicates the operating mode Where operating mode is what? Comment would be more useful if it listed them. > + * @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 > + */ Whilst we are here - userspace being responsible for writing a hardware frequency input needs to change. Makes no sense. > 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} > }; > diff --git a/drivers/staging/iio/resolver/ad2s1210.h b/drivers/staging/iio/resolver/ad2s1210.h > index e9b2147701fc..cbe21bca7638 100644 > --- a/drivers/staging/iio/resolver/ad2s1210.h > +++ b/drivers/staging/iio/resolver/ad2s1210.h > @@ -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. > * > * 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. > + * @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. > + */ > struct ad2s1210_platform_data { > unsigned int sample; > unsigned int a[2]; _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 4/4] staging:iio:ad2s1210: Add comments/documentation 2018-03-10 17:52 ` Jonathan Cameron @ 2018-03-12 12:25 ` Rodrigo Siqueira 0 siblings, 0 replies; 10+ messages in thread From: Rodrigo Siqueira @ 2018-03-12 12:25 UTC (permalink / raw) To: Jonathan Cameron Cc: devel, Graff Yang, Lars-Peter Clausen, Michael Hennerich, linux-iio, Greg Kroah-Hartman, linux-kernel, Peter Meerwald-Stadler, Hartmut Knaack, daniel.baluta On 03/10, Jonathan Cameron wrote: > On Fri, 9 Mar 2018 20:46:40 -0300 > Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> wrote: > > > 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 <rodrigosiqueiramelo@gmail.com> > 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 ;) 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 :) > Thanks, > > Jonathan > > > --- > > drivers/staging/iio/resolver/ad2s1210.c | 32 ++++++++++++++++++++++++++++++++ > > drivers/staging/iio/resolver/ad2s1210.h | 9 ++++++++- > > 2 files changed, 40 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c > > index ac13b99bd9cb..9bb8fd782f5a 100644 > > --- a/drivers/staging/iio/resolver/ad2s1210.c > > +++ b/drivers/staging/iio/resolver/ad2s1210.c > > @@ -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? > > > > #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 > > > #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 > > > #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 > Except they aren't anything to do with the chip model. This is about > how it is wired not what it is. > > > + * @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? > > > + * @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. > > > + * @mode: indicates the operating mode > Where operating mode is what? Comment would be more useful if it > listed them. > > > + * @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 > > + */ > Whilst we are here - userspace being responsible for writing a hardware > frequency input needs to change. Makes no sense. > > > 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} > > }; > > diff --git a/drivers/staging/iio/resolver/ad2s1210.h b/drivers/staging/iio/resolver/ad2s1210.h > > index e9b2147701fc..cbe21bca7638 100644 > > --- a/drivers/staging/iio/resolver/ad2s1210.h > > +++ b/drivers/staging/iio/resolver/ad2s1210.h > > @@ -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. > > * > > * 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. > > > + * @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. > > > + */ > > struct ad2s1210_platform_data { > > unsigned int sample; > > unsigned int a[2]; > _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-03-12 12:25 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-03-09 23:45 [PATCH 0/4] staging:iio:ad2s1210: Driver cleanup Rodrigo Siqueira 2018-03-09 23:45 ` [PATCH 1/4] staging:iio:ad2s1210: Remove end of line with '[' Rodrigo Siqueira 2018-03-10 16:41 ` Jonathan Cameron 2018-03-09 23:46 ` [PATCH 2/4] staging:iio:ad2s1210: Remove unused #define directive Rodrigo Siqueira 2018-03-10 16:45 ` Jonathan Cameron 2018-03-09 23:46 ` [PATCH 3/4] staging:iio:ad2s1210: Remove old_data from ad2s1210_state Rodrigo Siqueira 2018-03-10 17:21 ` Jonathan Cameron 2018-03-09 23:46 ` [PATCH 4/4] staging:iio:ad2s1210: Add comments/documentation Rodrigo Siqueira 2018-03-10 17:52 ` Jonathan Cameron 2018-03-12 12:25 ` Rodrigo Siqueira
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).