[PATCH 0/4] staging:iio:ad2s1210: Driver cleanup

[PATCH 0/4] staging:iio:ad2s1210: Driver cleanup

[Rodrigo Siqueira]

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

[PATCH 1/4] staging:iio:ad2s1210: Remove end of line with '['

[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 <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

[PATCH 2/4] staging:iio:ad2s1210: Remove unused #define directive

[Rodrigo Siqueira]

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

[PATCH 3/4] staging:iio:ad2s1210: Remove old_data from ad2s1210_state

[Rodrigo Siqueira]

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

[PATCH 4/4] staging:iio:ad2s1210: Add comments/documentation

[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 <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

Re: [PATCH 1/4] staging:iio:ad2s1210: Remove end of line with '['

[Jonathan Cameron]

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

Re: [PATCH 2/4] staging:iio:ad2s1210: Remove unused #define directive

[Jonathan Cameron]

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

Re: [PATCH 3/4] staging:iio:ad2s1210: Remove old_data from ad2s1210_state

[Jonathan Cameron]

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

Re: [PATCH 4/4] staging:iio:ad2s1210: Add comments/documentation

[Jonathan Cameron]

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

Re: [PATCH 4/4] staging:iio:ad2s1210: Add comments/documentation

[Rodrigo Siqueira]

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