[PATCH 00/11] Move ad7746 out of staging

[PATCH 00/11] Move ad7746 out of staging

[Hernán Gonzalez]

This patch series aims to move the cdc ad7746 driver out of staging. I have some
design questions though so I would introduce them here, along with a short
description of each patch.

*PATCH 0001 - Adjust arguments to match open parenthesis.
There were a couple CHECKS that still remained, so I got rid of them.

*PATCH 0002 - Fix multiple line dereference
In this case, I opted for avoiding the multiple line derefence and having a 80+
characters line instead as I consider that it improves readability. I may be
wrong though, so this patch could just be discarded.

*PATCH 0003 - Reorder includes alphabetically

*PATCH 0004 - Reorder variable declarations in an inverser-pyramid way

*PATCH 0005 - Remove unused defines
There were a few too many #defines that were not used at all, so I just removed
them. I guess if someone plans on extending the drivers functionality they can
be added again, but they were just wasting space as they were. Again, I could be
wrong with this decision so this patch could just be discarded.

*PATCH 0006 - Add dt-bindings
This patch adds dt bindings by populating the old pdata struct. It supports both
platform_data and dt-bindings but uses only one depending on CONFIG_OF. I chose
this way to avoid modifying too much the code, and introduce no errors (or as
few as I could), keeping the same functionality and maintaining support of the
platform_data.

*PATCH 0007 - Add remove()
I added a remove function so I could test that the driver probed properly when
compiled as a module with the new dt-bindings.

*PATCH 0008 - Add comments to clarify some of the calculations
I had to go through most of the datasheet to understand some of the math in the
code, so I added comments where I saw fit. (Comments on the comments are
welcome).

*PATCH 0009 - Add devicetree bindings documentation
Add documentation on the devicetree bindings, explaining the properties of it
and describing a short example.

*PATCH 0010 - Rename sysfs attrs to comply with the ABI
Comments are welcome on this one.
I shortened the names of the sysfs attrs to comply with the ABI:
<type>[Y]_calibbias_calibration -> <type>[Y]_calibbias
<type>[Y]_calibscale_calibration -> <type>[Y]_calibscale

The device supports 2 ways of calibrating the gain (from the datasheet):
'The gain can be changed by executing a capacitance gain
calibration mode, for which an external full-scale capacitance
needs to be connected to the capacitance input, or by writing a
user value to the capacitive gain register.'

The same for the offset calibration:
'One method of adjusting the offset is to connect a zero-scale
capacitance to the input and execute the capacitance offset
calibration mode. The calibration sets the midpoint of the
±4.096 pF range (that is, Output Code 0x800000) to that
zero-scale input.
Another method would be to calculate and write the offset cali-
bration register value, the LSB is value 31.25 aF (4.096 pF/2^17 ).'

The driver only supports the first way in both cases, as it only writes the
register that starts the calibration mode and doesn't allow the user to write
anything on other registers.

What I understand from the ABI is not so different when explaining calibbias and
calibscale:
'Description:
Hardware applied calibration {offset,scale factor} (assumed to fix production
inaccuracies).'

Maybe I'm missing something and the renaming is not good. I would be really
grateful if someone could shed some light on this for me.

*PATCH 0011 - Move cdc ad7746 driver out of staging to mainline iio
Move the files, modify the proper Kconfigs and the documentation.

That'd be all. Any feedback is welcome. I hope this gets out of staging :)

Cheers,

Hernán

Hernán Gonzalez (11):
  staging: iio: ad7746: Adjust arguments to match open parenthesis
  staging: iio: ad7746: Fix multiple line dereference
  staging: iio: ad7746: Reorder includes alphabetically
  staging: iio: ad7746: Reorder variable declarations
  staging: iio: ad7746: Remove unused defines
  staging: iio: ad7746: Add dt-bindings
  staging: iio: ad7746: Add remove()
  staging: iio: ad7746: Add comments
  staging: iio: ad7746: Add devicetree bindings documentation
  staging: iio: ad7746: Rename sysfs attrs to comply with the ABI
  Move cdc ad7746 driver out of staging to mainline iio

 .../devicetree/bindings/iio/cdc/ad7746.txt         |  32 ++++
 drivers/iio/Kconfig                                |   1 +
 drivers/iio/cdc/Kconfig                            |  16 ++
 drivers/{staging => }/iio/cdc/ad7746.c             | 168 +++++++++++++++------
 drivers/staging/iio/cdc/Kconfig                    |  10 --
 .../staging => include/linux}/iio/cdc/ad7746.h     |   9 --
 6 files changed, 168 insertions(+), 68 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/cdc/ad7746.txt
 create mode 100644 drivers/iio/cdc/Kconfig
 rename drivers/{staging => }/iio/cdc/ad7746.c (84%)
 rename {drivers/staging => include/linux}/iio/cdc/ad7746.h (70%)

-- 
2.7.4

[PATCH 01/11] staging: iio: ad7746: Adjust arguments to match open parenthesis

[Hernán Gonzalez]

Clear two more checkpatch.pl CHECKS.

Signed-off-by: Hernán Gonzalez <hernan@vanguardiasur.com.ar>
---
 drivers/staging/iio/cdc/ad7746.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
index 4882dbc..02e3164 100644
--- a/drivers/staging/iio/cdc/ad7746.c
+++ b/drivers/staging/iio/cdc/ad7746.c
@@ -463,7 +463,8 @@ static int ad7746_write_raw(struct iio_dev *indio_dev,
 			goto out;
 		}
 		ret = i2c_smbus_write_word_data(chip->client,
-				AD7746_REG_CAP_OFFH, swab16(val));
+						AD7746_REG_CAP_OFFH,
+						swab16(val));
 		if (ret < 0)
 			goto out;
 
@@ -556,7 +557,8 @@ static int ad7746_read_raw(struct iio_dev *indio_dev,
 		/* Now read the actual register */
 
 		ret = i2c_smbus_read_i2c_block_data(chip->client,
-			chan->address >> 8, 3, &chip->data.d8[1]);
+						    chan->address >> 8, 3,
+						    &chip->data.d8[1]);
 
 		if (ret < 0)
 			goto out;
-- 
2.7.4

Re: [PATCH 01/11] staging: iio: ad7746: Adjust arguments to match open parenthesis

[Jonathan Cameron]

On Wed, 21 Mar 2018 11:28:49 -0300
Hernán Gonzalez <hernan@vanguardiasur.com.ar> wrote:

> Clear two more checkpatch.pl CHECKS.
> 
> Signed-off-by: Hernán Gonzalez <hernan@vanguardiasur.com.ar>

Hi Hernán,

Technically the comment below isn't about this patch, but there
seems little point in fixing alignment when the function
called should be changed anyway (requiring the alignment to change
again!)

Otherwise patch looks good.

Thanks,

Jonathan
> ---
>  drivers/staging/iio/cdc/ad7746.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
> index 4882dbc..02e3164 100644
> --- a/drivers/staging/iio/cdc/ad7746.c
> +++ b/drivers/staging/iio/cdc/ad7746.c
> @@ -463,7 +463,8 @@ static int ad7746_write_raw(struct iio_dev *indio_dev,
>  			goto out;
>  		}
>  		ret = i2c_smbus_write_word_data(chip->client,
> -				AD7746_REG_CAP_OFFH, swab16(val));
> +						AD7746_REG_CAP_OFFH,
> +						swab16(val));
Take a look at i2c_smbus_write_word_swapped.

I thought we had long ago gotten rid of all the places this was
being done by hand, but apparently not!

>  		if (ret < 0)
>  			goto out;
>  
> @@ -556,7 +557,8 @@ static int ad7746_read_raw(struct iio_dev *indio_dev,
>  		/* Now read the actual register */
>  
>  		ret = i2c_smbus_read_i2c_block_data(chip->client,
> -			chan->address >> 8, 3, &chip->data.d8[1]);
> +						    chan->address >> 8, 3,
> +						    &chip->data.d8[1]);
>  
>  		if (ret < 0)
>  			goto out;

[PATCH 02/11] staging: iio: ad7746: Fix multiple line dereference

[Hernán Gonzalez]

Clear checkpatch.pl WARNING about multiple line derefence but creates a
new one of line over 80 characters. In my opinion, it improves
readability.

Signed-off-by: Hernán Gonzalez <hernan@vanguardiasur.com.ar>
---
 drivers/staging/iio/cdc/ad7746.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
index 02e3164..557ed4d 100644
--- a/drivers/staging/iio/cdc/ad7746.c
+++ b/drivers/staging/iio/cdc/ad7746.c
@@ -410,8 +410,7 @@ static struct attribute *ad7746_attributes[] = {
 	&iio_dev_attr_in_capacitance1_calibbias_calibration.dev_attr.attr,
 	&iio_dev_attr_in_voltage0_calibscale_calibration.dev_attr.attr,
 	&iio_const_attr_in_voltage_sampling_frequency_available.dev_attr.attr,
-	&iio_const_attr_in_capacitance_sampling_frequency_available.
-	dev_attr.attr,
+	&iio_const_attr_in_capacitance_sampling_frequency_available.dev_attr.attr,
 	NULL,
 };
 
-- 
2.7.4

[PATCH 03/11] staging: iio: ad7746: Reorder includes alphabetically

[Hernán Gonzalez]

Signed-off-by: Hernán Gonzalez <hernan@vanguardiasur.com.ar>
---
 drivers/staging/iio/cdc/ad7746.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
index 557ed4d..86919e8 100644
--- a/drivers/staging/iio/cdc/ad7746.c
+++ b/drivers/staging/iio/cdc/ad7746.c
@@ -6,15 +6,15 @@
  * Licensed under the GPL-2.
  */
 
-#include <linux/interrupt.h>
+#include <linux/delay.h>
 #include <linux/device.h>
-#include <linux/kernel.h>
-#include <linux/slab.h>
-#include <linux/sysfs.h>
 #include <linux/i2c.h>
-#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/slab.h>
 #include <linux/stat.h>
+#include <linux/sysfs.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
-- 
2.7.4

[PATCH 04/11] staging: iio: ad7746: Reorder variable declarations

[Hernán Gonzalez]

Reorder some variable declarations in an inverse-pyramid scheme.

Signed-off-by: Hernán Gonzalez <hernan@vanguardiasur.com.ar>
---
 drivers/staging/iio/cdc/ad7746.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
index 86919e8..57623db 100644
--- a/drivers/staging/iio/cdc/ad7746.c
+++ b/drivers/staging/iio/cdc/ad7746.c
@@ -220,8 +220,8 @@ static int ad7746_select_channel(struct iio_dev *indio_dev,
 				 struct iio_chan_spec const *chan)
 {
 	struct ad7746_chip_info *chip = iio_priv(indio_dev);
-	int ret, delay, idx;
 	u8 vt_setup, cap_setup;
+	int ret, delay, idx;
 
 	switch (chan->type) {
 	case IIO_CAPACITANCE:
@@ -289,8 +289,8 @@ static inline ssize_t ad7746_start_calib(struct device *dev,
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct ad7746_chip_info *chip = iio_priv(indio_dev);
-	bool doit;
 	int ret, timeout = 10;
+	bool doit;
 
 	ret = strtobool(buf, &doit);
 	if (ret < 0)
@@ -681,8 +681,8 @@ static int ad7746_probe(struct i2c_client *client,
 	struct ad7746_platform_data *pdata = client->dev.platform_data;
 	struct ad7746_chip_info *chip;
 	struct iio_dev *indio_dev;
-	int ret = 0;
 	unsigned char regval = 0;
+	int ret = 0;
 
 	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
 	if (!indio_dev)
-- 
2.7.4

Re: [PATCH 04/11] staging: iio: ad7746: Reorder variable declarations

[Jonathan Cameron]

On Wed, 21 Mar 2018 11:28:52 -0300
Hernán Gonzalez <hernan@vanguardiasur.com.ar> wrote:

> Reorder some variable declarations in an inverse-pyramid scheme.
> 
> Signed-off-by: Hernán Gonzalez <hernan@vanguardiasur.com.ar>
I'm not sure the first few cases here actually add much in
the way of readablity.

This 'rule' is very 'vague' deliberately as sometimes it makes sense
and sometimes it really doesn't.

One nice convention is to have ret on it's own line when it is the
value returned by the function (it doesn't logically group with anything
else).  Having that one last is also nice to see (so I like your final
change more than the others!)

Jonathan
> ---
>  drivers/staging/iio/cdc/ad7746.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
> index 86919e8..57623db 100644
> --- a/drivers/staging/iio/cdc/ad7746.c
> +++ b/drivers/staging/iio/cdc/ad7746.c
> @@ -220,8 +220,8 @@ static int ad7746_select_channel(struct iio_dev *indio_dev,
>  				 struct iio_chan_spec const *chan)
>  {
>  	struct ad7746_chip_info *chip = iio_priv(indio_dev);
> -	int ret, delay, idx;
>  	u8 vt_setup, cap_setup;
> +	int ret, delay, idx;
>  
>  	switch (chan->type) {
>  	case IIO_CAPACITANCE:
> @@ -289,8 +289,8 @@ static inline ssize_t ad7746_start_calib(struct device *dev,
>  {
>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>  	struct ad7746_chip_info *chip = iio_priv(indio_dev);
> -	bool doit;
>  	int ret, timeout = 10;
> +	bool doit;
>  
>  	ret = strtobool(buf, &doit);
>  	if (ret < 0)
> @@ -681,8 +681,8 @@ static int ad7746_probe(struct i2c_client *client,
>  	struct ad7746_platform_data *pdata = client->dev.platform_data;
>  	struct ad7746_chip_info *chip;
>  	struct iio_dev *indio_dev;
> -	int ret = 0;
>  	unsigned char regval = 0;
> +	int ret = 0;
>  
>  	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
>  	if (!indio_dev)

Re: [PATCH 04/11] staging: iio: ad7746: Reorder variable declarations

[Jonathan Cameron]

On Wed, 21 Mar 2018 11:28:52 -0300
Hern=E1n Gonzalez <hernan@vanguardiasur.com.ar> wrote:

> Reorder some variable declarations in an inverse-pyramid scheme.
>=20
> Signed-off-by: Hern=E1n Gonzalez <hernan@vanguardiasur.com.ar>
I'm not sure the first few cases here actually add much in
the way of readablity.

This 'rule' is very 'vague' deliberately as sometimes it makes sense
and sometimes it really doesn't.

One nice convention is to have ret on it's own line when it is the
value returned by the function (it doesn't logically group with anything
else).  Having that one last is also nice to see (so I like your final
change more than the others!)

Jonathan
> ---
>  drivers/staging/iio/cdc/ad7746.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>=20
> diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/a=
d7746.c
> index 86919e8..57623db 100644
> --- a/drivers/staging/iio/cdc/ad7746.c
> +++ b/drivers/staging/iio/cdc/ad7746.c
> @@ -220,8 +220,8 @@ static int ad7746_select_channel(struct iio_dev *indi=
o_dev,
>  				 struct iio_chan_spec const *chan)
>  {
>  	struct ad7746_chip_info *chip =3D iio_priv(indio_dev);
> -	int ret, delay, idx;
>  	u8 vt_setup, cap_setup;
> +	int ret, delay, idx;
> =20
>  	switch (chan->type) {
>  	case IIO_CAPACITANCE:
> @@ -289,8 +289,8 @@ static inline ssize_t ad7746_start_calib(struct devic=
e *dev,
>  {
>  	struct iio_dev *indio_dev =3D dev_to_iio_dev(dev);
>  	struct ad7746_chip_info *chip =3D iio_priv(indio_dev);
> -	bool doit;
>  	int ret, timeout =3D 10;
> +	bool doit;
> =20
>  	ret =3D strtobool(buf, &doit);
>  	if (ret < 0)
> @@ -681,8 +681,8 @@ static int ad7746_probe(struct i2c_client *client,
>  	struct ad7746_platform_data *pdata =3D client->dev.platform_data;
>  	struct ad7746_chip_info *chip;
>  	struct iio_dev *indio_dev;
> -	int ret =3D 0;
>  	unsigned char regval =3D 0;
> +	int ret =3D 0;
> =20
>  	indio_dev =3D devm_iio_device_alloc(&client->dev, sizeof(*chip));
>  	if (!indio_dev)

[PATCH 05/11] staging: iio: ad7746: Remove unused defines

[Hernán Gonzalez]

Signed-off-by: Hernán Gonzalez <hernan@vanguardiasur.com.ar>
---
 drivers/staging/iio/cdc/ad7746.c | 16 ----------------
 drivers/staging/iio/cdc/ad7746.h |  5 -----
 2 files changed, 21 deletions(-)

diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
index 57623db..cba8cd1 100644
--- a/drivers/staging/iio/cdc/ad7746.c
+++ b/drivers/staging/iio/cdc/ad7746.c
@@ -25,7 +25,6 @@
  * AD7746 Register Definition
  */
 
-#define AD7746_REG_STATUS		0
 #define AD7746_REG_CAP_DATA_HIGH	1
 #define AD7746_REG_VT_DATA_HIGH		4
 #define AD7746_REG_CAP_SETUP		7
@@ -38,17 +37,10 @@
 #define AD7746_REG_CAP_GAINH		15
 #define AD7746_REG_VOLT_GAINH		17
 
-/* Status Register Bit Designations (AD7746_REG_STATUS) */
-#define AD7746_STATUS_EXCERR		BIT(3)
-#define AD7746_STATUS_RDY		BIT(2)
-#define AD7746_STATUS_RDYVT		BIT(1)
-#define AD7746_STATUS_RDYCAP		BIT(0)
-
 /* Capacitive Channel Setup Register Bit Designations (AD7746_REG_CAP_SETUP) */
 #define AD7746_CAPSETUP_CAPEN		BIT(7)
 #define AD7746_CAPSETUP_CIN2		BIT(6) /* AD7746 only */
 #define AD7746_CAPSETUP_CAPDIFF		BIT(5)
-#define AD7746_CAPSETUP_CACHOP		BIT(0)
 
 /* Voltage/Temperature Setup Register Bit Designations (AD7746_REG_VT_SETUP) */
 #define AD7746_VTSETUP_VTEN		(1 << 7)
@@ -56,13 +48,8 @@
 #define AD7746_VTSETUP_VTMD_EXT_TEMP	(1 << 5)
 #define AD7746_VTSETUP_VTMD_VDD_MON	(2 << 5)
 #define AD7746_VTSETUP_VTMD_EXT_VIN	(3 << 5)
-#define AD7746_VTSETUP_EXTREF		BIT(4)
-#define AD7746_VTSETUP_VTSHORT		BIT(1)
-#define AD7746_VTSETUP_VTCHOP		BIT(0)
 
 /* Excitation Setup Register Bit Designations (AD7746_REG_EXC_SETUP) */
-#define AD7746_EXCSETUP_CLKCTRL		BIT(7)
-#define AD7746_EXCSETUP_EXCON		BIT(6)
 #define AD7746_EXCSETUP_EXCB		BIT(5)
 #define AD7746_EXCSETUP_NEXCB		BIT(4)
 #define AD7746_EXCSETUP_EXCA		BIT(3)
@@ -74,10 +61,7 @@
 #define AD7746_CONF_CAPFS_SHIFT		3
 #define AD7746_CONF_VTFS_MASK		GENMASK(7, 6)
 #define AD7746_CONF_CAPFS_MASK		GENMASK(5, 3)
-#define AD7746_CONF_MODE_IDLE		(0 << 0)
-#define AD7746_CONF_MODE_CONT_CONV	(1 << 0)
 #define AD7746_CONF_MODE_SINGLE_CONV	(2 << 0)
-#define AD7746_CONF_MODE_PWRDN		(3 << 0)
 #define AD7746_CONF_MODE_OFFS_CAL	(5 << 0)
 #define AD7746_CONF_MODE_GAIN_CAL	(6 << 0)
 
diff --git a/drivers/staging/iio/cdc/ad7746.h b/drivers/staging/iio/cdc/ad7746.h
index ea8572d..2fbcee8 100644
--- a/drivers/staging/iio/cdc/ad7746.h
+++ b/drivers/staging/iio/cdc/ad7746.h
@@ -13,11 +13,6 @@
  * TODO: struct ad7746_platform_data needs to go into include/linux/iio
  */
 
-#define AD7466_EXCLVL_0		0 /* +-VDD/8 */
-#define AD7466_EXCLVL_1		1 /* +-VDD/4 */
-#define AD7466_EXCLVL_2		2 /* +-VDD * 3/8 */
-#define AD7466_EXCLVL_3		3 /* +-VDD/2 */
-
 struct ad7746_platform_data {
 	unsigned char exclvl;	/*Excitation Voltage Level */
 	bool exca_en;		/* enables EXCA pin as the excitation output */
-- 
2.7.4

Re: [PATCH 05/11] staging: iio: ad7746: Remove unused defines

[Jonathan Cameron]

On Wed, 21 Mar 2018 11:28:53 -0300
Hernán Gonzalez <hernan@vanguardiasur.com.ar> wrote:

> Signed-off-by: Hernán Gonzalez <hernan@vanguardiasur.com.ar>
Hmm. This sort of patch is always a trade off between
clearing out unused code and the fact that the defines
sometimes provide useful information even when not
actually used.

> ---
>  drivers/staging/iio/cdc/ad7746.c | 16 ----------------
>  drivers/staging/iio/cdc/ad7746.h |  5 -----
>  2 files changed, 21 deletions(-)
> 
> diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
> index 57623db..cba8cd1 100644
> --- a/drivers/staging/iio/cdc/ad7746.c
> +++ b/drivers/staging/iio/cdc/ad7746.c
> @@ -25,7 +25,6 @@
>   * AD7746 Register Definition
>   */
>  
> -#define AD7746_REG_STATUS		0
>  #define AD7746_REG_CAP_DATA_HIGH	1
>  #define AD7746_REG_VT_DATA_HIGH		4
>  #define AD7746_REG_CAP_SETUP		7
> @@ -38,17 +37,10 @@
>  #define AD7746_REG_CAP_GAINH		15
>  #define AD7746_REG_VOLT_GAINH		17
>  
> -/* Status Register Bit Designations (AD7746_REG_STATUS) */
> -#define AD7746_STATUS_EXCERR		BIT(3)
> -#define AD7746_STATUS_RDY		BIT(2)
> -#define AD7746_STATUS_RDYVT		BIT(1)
> -#define AD7746_STATUS_RDYCAP		BIT(0)
> -
Hmm. My gut feeling is the driver really should be reading this
register... Ah well.  Perhaps it can go in the meantime.

>  /* Capacitive Channel Setup Register Bit Designations (AD7746_REG_CAP_SETUP) */
>  #define AD7746_CAPSETUP_CAPEN		BIT(7)
>  #define AD7746_CAPSETUP_CIN2		BIT(6) /* AD7746 only */
>  #define AD7746_CAPSETUP_CAPDIFF		BIT(5)
> -#define AD7746_CAPSETUP_CACHOP		BIT(0)
Don't remove definitions of 'parts' of a register.
It is odd to only have some parts described.

>  
>  /* Voltage/Temperature Setup Register Bit Designations (AD7746_REG_VT_SETUP) */
>  #define AD7746_VTSETUP_VTEN		(1 << 7)
> @@ -56,13 +48,8 @@
>  #define AD7746_VTSETUP_VTMD_EXT_TEMP	(1 << 5)
>  #define AD7746_VTSETUP_VTMD_VDD_MON	(2 << 5)
>  #define AD7746_VTSETUP_VTMD_EXT_VIN	(3 << 5)
> -#define AD7746_VTSETUP_EXTREF		BIT(4)
> -#define AD7746_VTSETUP_VTSHORT		BIT(1)
> -#define AD7746_VTSETUP_VTCHOP		BIT(0)
Same comment, keep these as odd to not know if the rest of the register
is used etc...
>  
>  /* Excitation Setup Register Bit Designations (AD7746_REG_EXC_SETUP) */
> -#define AD7746_EXCSETUP_CLKCTRL		BIT(7)
> -#define AD7746_EXCSETUP_EXCON		BIT(6)
>  #define AD7746_EXCSETUP_EXCB		BIT(5)
>  #define AD7746_EXCSETUP_NEXCB		BIT(4)
>  #define AD7746_EXCSETUP_EXCA		BIT(3)
> @@ -74,10 +61,7 @@
>  #define AD7746_CONF_CAPFS_SHIFT		3
>  #define AD7746_CONF_VTFS_MASK		GENMASK(7, 6)
>  #define AD7746_CONF_CAPFS_MASK		GENMASK(5, 3)
> -#define AD7746_CONF_MODE_IDLE		(0 << 0)
> -#define AD7746_CONF_MODE_CONT_CONV	(1 << 0)
>  #define AD7746_CONF_MODE_SINGLE_CONV	(2 << 0)
> -#define AD7746_CONF_MODE_PWRDN		(3 << 0)
This is really nasty.  Some particular values may not be used
in the driver (and they should be - for example we should power
down on remove).  Don't remove their definitions.
>  #define AD7746_CONF_MODE_OFFS_CAL	(5 << 0)
>  #define AD7746_CONF_MODE_GAIN_CAL	(6 << 0)
>  
> diff --git a/drivers/staging/iio/cdc/ad7746.h b/drivers/staging/iio/cdc/ad7746.h
> index ea8572d..2fbcee8 100644
> --- a/drivers/staging/iio/cdc/ad7746.h
> +++ b/drivers/staging/iio/cdc/ad7746.h
> @@ -13,11 +13,6 @@
>   * TODO: struct ad7746_platform_data needs to go into include/linux/iio
>   */
>  
> -#define AD7466_EXCLVL_0		0 /* +-VDD/8 */
> -#define AD7466_EXCLVL_1		1 /* +-VDD/4 */
> -#define AD7466_EXCLVL_2		2 /* +-VDD * 3/8 */
> -#define AD7466_EXCLVL_3		3 /* +-VDD/2 */
This is used...  If you wanted to use the platform data you would
need these to fill it.

Jonathan
> -
>  struct ad7746_platform_data {
>  	unsigned char exclvl;	/*Excitation Voltage Level */
>  	bool exca_en;		/* enables EXCA pin as the excitation output */

Re: [PATCH 05/11] staging: iio: ad7746: Remove unused defines

[Jonathan Cameron]

On Wed, 21 Mar 2018 11:28:53 -0300
Hern=E1n Gonzalez <hernan@vanguardiasur.com.ar> wrote:

> Signed-off-by: Hern=E1n Gonzalez <hernan@vanguardiasur.com.ar>
Hmm. This sort of patch is always a trade off between
clearing out unused code and the fact that the defines
sometimes provide useful information even when not
actually used.

> ---
>  drivers/staging/iio/cdc/ad7746.c | 16 ----------------
>  drivers/staging/iio/cdc/ad7746.h |  5 -----
>  2 files changed, 21 deletions(-)
>=20
> diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/a=
d7746.c
> index 57623db..cba8cd1 100644
> --- a/drivers/staging/iio/cdc/ad7746.c
> +++ b/drivers/staging/iio/cdc/ad7746.c
> @@ -25,7 +25,6 @@
>   * AD7746 Register Definition
>   */
> =20
> -#define AD7746_REG_STATUS		0
>  #define AD7746_REG_CAP_DATA_HIGH	1
>  #define AD7746_REG_VT_DATA_HIGH		4
>  #define AD7746_REG_CAP_SETUP		7
> @@ -38,17 +37,10 @@
>  #define AD7746_REG_CAP_GAINH		15
>  #define AD7746_REG_VOLT_GAINH		17
> =20
> -/* Status Register Bit Designations (AD7746_REG_STATUS) */
> -#define AD7746_STATUS_EXCERR		BIT(3)
> -#define AD7746_STATUS_RDY		BIT(2)
> -#define AD7746_STATUS_RDYVT		BIT(1)
> -#define AD7746_STATUS_RDYCAP		BIT(0)
> -
Hmm. My gut feeling is the driver really should be reading this
register... Ah well.  Perhaps it can go in the meantime.

>  /* Capacitive Channel Setup Register Bit Designations (AD7746_REG_CAP_SE=
TUP) */
>  #define AD7746_CAPSETUP_CAPEN		BIT(7)
>  #define AD7746_CAPSETUP_CIN2		BIT(6) /* AD7746 only */
>  #define AD7746_CAPSETUP_CAPDIFF		BIT(5)
> -#define AD7746_CAPSETUP_CACHOP		BIT(0)
Don't remove definitions of 'parts' of a register.
It is odd to only have some parts described.

> =20
>  /* Voltage/Temperature Setup Register Bit Designations (AD7746_REG_VT_SE=
TUP) */
>  #define AD7746_VTSETUP_VTEN		(1 << 7)
> @@ -56,13 +48,8 @@
>  #define AD7746_VTSETUP_VTMD_EXT_TEMP	(1 << 5)
>  #define AD7746_VTSETUP_VTMD_VDD_MON	(2 << 5)
>  #define AD7746_VTSETUP_VTMD_EXT_VIN	(3 << 5)
> -#define AD7746_VTSETUP_EXTREF		BIT(4)
> -#define AD7746_VTSETUP_VTSHORT		BIT(1)
> -#define AD7746_VTSETUP_VTCHOP		BIT(0)
Same comment, keep these as odd to not know if the rest of the register
is used etc...
> =20
>  /* Excitation Setup Register Bit Designations (AD7746_REG_EXC_SETUP) */
> -#define AD7746_EXCSETUP_CLKCTRL		BIT(7)
> -#define AD7746_EXCSETUP_EXCON		BIT(6)
>  #define AD7746_EXCSETUP_EXCB		BIT(5)
>  #define AD7746_EXCSETUP_NEXCB		BIT(4)
>  #define AD7746_EXCSETUP_EXCA		BIT(3)
> @@ -74,10 +61,7 @@
>  #define AD7746_CONF_CAPFS_SHIFT		3
>  #define AD7746_CONF_VTFS_MASK		GENMASK(7, 6)
>  #define AD7746_CONF_CAPFS_MASK		GENMASK(5, 3)
> -#define AD7746_CONF_MODE_IDLE		(0 << 0)
> -#define AD7746_CONF_MODE_CONT_CONV	(1 << 0)
>  #define AD7746_CONF_MODE_SINGLE_CONV	(2 << 0)
> -#define AD7746_CONF_MODE_PWRDN		(3 << 0)
This is really nasty.  Some particular values may not be used
in the driver (and they should be - for example we should power
down on remove).  Don't remove their definitions.
>  #define AD7746_CONF_MODE_OFFS_CAL	(5 << 0)
>  #define AD7746_CONF_MODE_GAIN_CAL	(6 << 0)
> =20
> diff --git a/drivers/staging/iio/cdc/ad7746.h b/drivers/staging/iio/cdc/a=
d7746.h
> index ea8572d..2fbcee8 100644
> --- a/drivers/staging/iio/cdc/ad7746.h
> +++ b/drivers/staging/iio/cdc/ad7746.h
> @@ -13,11 +13,6 @@
>   * TODO: struct ad7746_platform_data needs to go into include/linux/iio
>   */
> =20
> -#define AD7466_EXCLVL_0		0 /* +-VDD/8 */
> -#define AD7466_EXCLVL_1		1 /* +-VDD/4 */
> -#define AD7466_EXCLVL_2		2 /* +-VDD * 3/8 */
> -#define AD7466_EXCLVL_3		3 /* +-VDD/2 */
This is used...  If you wanted to use the platform data you would
need these to fill it.

Jonathan
> -
>  struct ad7746_platform_data {
>  	unsigned char exclvl;	/*Excitation Voltage Level */
>  	bool exca_en;		/* enables EXCA pin as the excitation output */

[PATCH 06/11] staging: iio: ad7746: Add dt-bindings

[Hernán Gonzalez]

This patch adds dt bindings by populating a pdata struct in order to
modify as little as possible the existing code. It supports both
platform_data and dt-bindings but uses only one depending on
CONFIG_OF's value.

Signed-off-by: Hernán Gonzalez <hernan@vanguardiasur.com.ar>
---
 drivers/staging/iio/cdc/ad7746.c | 55 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 54 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
index cba8cd1..815573c 100644
--- a/drivers/staging/iio/cdc/ad7746.c
+++ b/drivers/staging/iio/cdc/ad7746.c
@@ -658,6 +658,44 @@ static const struct iio_info ad7746_info = {
 /*
  * device probe and remove
  */
+#ifdef CONFIG_OF
+static struct ad7746_platform_data *ad7746_parse_dt(struct device *dev)
+{
+	struct device_node *np = dev->of_node;
+	struct ad7746_platform_data *pdata;
+	unsigned int tmp;
+	int ret;
+
+	/* The default excitation outputs are not inverted, it should be stated
+	 * in the dt if needed.
+	 */
+
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return NULL;
+
+	tmp = 0;
+	ret = of_property_read_u32(np, "adi,exclvl", &tmp);
+	if (ret || tmp > 3) {
+		dev_warn(dev, "Wrong exclvl value, using default\n");
+		pdata->exclvl = 3; /* default value */
+	} else {
+		pdata->exclvl = tmp;
+	}
+
+	pdata->exca_inv_en = of_property_read_bool(np, "adi,nexca_en");
+	pdata->excb_inv_en = of_property_read_bool(np, "adi,nexcb_en");
+	pdata->exca_en = !pdata->exca_inv_en;
+	pdata->excb_en = !pdata->excb_inv_en;
+
+	return pdata;
+}
+#else
+static struct ad7746_platform_data *ad7746_parse_dt(struct device *dev)
+{
+	return NULL;
+}
+#endif
 
 static int ad7746_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
@@ -668,6 +706,11 @@ static int ad7746_probe(struct i2c_client *client,
 	unsigned char regval = 0;
 	int ret = 0;
 
+	if (client->dev.of_node)
+		pdata = ad7746_parse_dt(&client->dev);
+	else
+		pdata = client->dev.platform_data;
+
 	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
 	if (!indio_dev)
 		return -ENOMEM;
@@ -731,12 +774,22 @@ static const struct i2c_device_id ad7746_id[] = {
 	{ "ad7747", 7747 },
 	{}
 };
-
 MODULE_DEVICE_TABLE(i2c, ad7746_id);
 
+#ifdef CONFIG_OF
+static const struct of_device_id ad7746_of_match[] = {
+	{ .compatible = "adi,ad7745" },
+	{ .compatible = "adi,ad7746" },
+	{ .compatible = "adi,ad7747" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ad7746_of_match);
+#endif
+
 static struct i2c_driver ad7746_driver = {
 	.driver = {
 		.name = KBUILD_MODNAME,
+		.of_match_table = of_match_ptr(ad7746_of_match),
 	},
 	.probe = ad7746_probe,
 	.id_table = ad7746_id,
-- 
2.7.4

Re: [PATCH 06/11] staging: iio: ad7746: Add dt-bindings

[Jonathan Cameron]

On Wed, 21 Mar 2018 11:28:54 -0300
Hernán Gonzalez <hernan@vanguardiasur.com.ar> wrote:

> This patch adds dt bindings by populating a pdata struct in order to
> modify as little as possible the existing code. It supports both
> platform_data and dt-bindings but uses only one depending on
> CONFIG_OF's value.
> 
> Signed-off-by: Hernán Gonzalez <hernan@vanguardiasur.com.ar>
> ---
>  drivers/staging/iio/cdc/ad7746.c | 55 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 54 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
> index cba8cd1..815573c 100644
> --- a/drivers/staging/iio/cdc/ad7746.c
> +++ b/drivers/staging/iio/cdc/ad7746.c
> @@ -658,6 +658,44 @@ static const struct iio_info ad7746_info = {
>  /*
>   * device probe and remove
>   */
> +#ifdef CONFIG_OF
> +static struct ad7746_platform_data *ad7746_parse_dt(struct device *dev)
> +{
> +	struct device_node *np = dev->of_node;
> +	struct ad7746_platform_data *pdata;
> +	unsigned int tmp;
> +	int ret;
> +
> +	/* The default excitation outputs are not inverted, it should be stated
> +	 * in the dt if needed.
> +	 */
> +
> +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return NULL;
> +
> +	tmp = 0;
> +	ret = of_property_read_u32(np, "adi,exclvl", &tmp);
It's not an optional read so why would temp be unset in any
paths where it is used?

> +	if (ret || tmp > 3) {
> +		dev_warn(dev, "Wrong exclvl value, using default\n");
> +		pdata->exclvl = 3; /* default value */
> +	} else {
> +		pdata->exclvl = tmp;
> +	}
> +
> +	pdata->exca_inv_en = of_property_read_bool(np, "adi,nexca_en");
> +	pdata->excb_inv_en = of_property_read_bool(np, "adi,nexcb_en");
> +	pdata->exca_en = !pdata->exca_inv_en;
> +	pdata->excb_en = !pdata->excb_inv_en;
> +
> +	return pdata;
> +}
> +#else
> +static struct ad7746_platform_data *ad7746_parse_dt(struct device *dev)
> +{
> +	return NULL;
> +}
> +#endif
>  
>  static int ad7746_probe(struct i2c_client *client,
>  			const struct i2c_device_id *id)
> @@ -668,6 +706,11 @@ static int ad7746_probe(struct i2c_client *client,
>  	unsigned char regval = 0;
>  	int ret = 0;
>  
> +	if (client->dev.of_node)
> +		pdata = ad7746_parse_dt(&client->dev);
> +	else
> +		pdata = client->dev.platform_data;
> +
>  	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
>  	if (!indio_dev)
>  		return -ENOMEM;
> @@ -731,12 +774,22 @@ static const struct i2c_device_id ad7746_id[] = {
>  	{ "ad7747", 7747 },
>  	{}
>  };
> -
>  MODULE_DEVICE_TABLE(i2c, ad7746_id);
>  
> +#ifdef CONFIG_OF
> +static const struct of_device_id ad7746_of_match[] = {
> +	{ .compatible = "adi,ad7745" },
> +	{ .compatible = "adi,ad7746" },
> +	{ .compatible = "adi,ad7747" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, ad7746_of_match);
> +#endif
> +
>  static struct i2c_driver ad7746_driver = {
>  	.driver = {
>  		.name = KBUILD_MODNAME,
> +		.of_match_table = of_match_ptr(ad7746_of_match),
>  	},
>  	.probe = ad7746_probe,
>  	.id_table = ad7746_id,

[PATCH 07/11] staging: iio: ad7746: Add remove()

[Hernán Gonzalez]

This allows the driver to be probed and removed as a module.

Signed-off-by: Hernán Gonzalez <hernan@vanguardiasur.com.ar>
---
 drivers/staging/iio/cdc/ad7746.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
index 815573c..8abba71 100644
--- a/drivers/staging/iio/cdc/ad7746.c
+++ b/drivers/staging/iio/cdc/ad7746.c
@@ -768,6 +768,15 @@ static int ad7746_probe(struct i2c_client *client,
 	return 0;
 }
 
+static int ad7746_remove(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+
+	iio_device_unregister(indio_dev);
+
+	return 0;
+}
+
 static const struct i2c_device_id ad7746_id[] = {
 	{ "ad7745", 7745 },
 	{ "ad7746", 7746 },
@@ -792,6 +801,7 @@ static struct i2c_driver ad7746_driver = {
 		.of_match_table = of_match_ptr(ad7746_of_match),
 	},
 	.probe = ad7746_probe,
+	.remove = ad7746_remove,
 	.id_table = ad7746_id,
 };
 module_i2c_driver(ad7746_driver);
-- 
2.7.4

Re: [PATCH 07/11] staging: iio: ad7746: Add remove()

[Jonathan Cameron]

On Wed, 21 Mar 2018 11:28:55 -0300
Hernán Gonzalez <hernan@vanguardiasur.com.ar> wrote:

> This allows the driver to be probed and removed as a module.
> 
> Signed-off-by: Hernán Gonzalez <hernan@vanguardiasur.com.ar>
> ---
>  drivers/staging/iio/cdc/ad7746.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
> index 815573c..8abba71 100644
> --- a/drivers/staging/iio/cdc/ad7746.c
> +++ b/drivers/staging/iio/cdc/ad7746.c
> @@ -768,6 +768,15 @@ static int ad7746_probe(struct i2c_client *client,
>  	return 0;
>  }
>  
> +static int ad7746_remove(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +
> +	iio_device_unregister(indio_dev);
Remove is not required to allow a driver to be removed..
It's required to clean up anything that needs cleaning up
when the remove happens.

If all you have is the device unregister then it should be
possible to use.
devm_iio_device_register and rely on managed cleanup to
automatically call the unregister code for you.

Hence there would be no need to have a remove function.

Jonathan
> +
> +	return 0;
> +}
> +
>  static const struct i2c_device_id ad7746_id[] = {
>  	{ "ad7745", 7745 },
>  	{ "ad7746", 7746 },
> @@ -792,6 +801,7 @@ static struct i2c_driver ad7746_driver = {
>  		.of_match_table = of_match_ptr(ad7746_of_match),
>  	},
>  	.probe = ad7746_probe,
> +	.remove = ad7746_remove,
>  	.id_table = ad7746_id,
>  };
>  module_i2c_driver(ad7746_driver);

Re: [PATCH 07/11] staging: iio: ad7746: Add remove()

[Jonathan Cameron]

On Wed, 21 Mar 2018 11:28:55 -0300
Hern=E1n Gonzalez <hernan@vanguardiasur.com.ar> wrote:

> This allows the driver to be probed and removed as a module.
>=20
> Signed-off-by: Hern=E1n Gonzalez <hernan@vanguardiasur.com.ar>
> ---
>  drivers/staging/iio/cdc/ad7746.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>=20
> diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/a=
d7746.c
> index 815573c..8abba71 100644
> --- a/drivers/staging/iio/cdc/ad7746.c
> +++ b/drivers/staging/iio/cdc/ad7746.c
> @@ -768,6 +768,15 @@ static int ad7746_probe(struct i2c_client *client,
>  	return 0;
>  }
> =20
> +static int ad7746_remove(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev =3D i2c_get_clientdata(client);
> +
> +	iio_device_unregister(indio_dev);
Remove is not required to allow a driver to be removed..
It's required to clean up anything that needs cleaning up
when the remove happens.

If all you have is the device unregister then it should be
possible to use.
devm_iio_device_register and rely on managed cleanup to
automatically call the unregister code for you.

Hence there would be no need to have a remove function.

Jonathan
> +
> +	return 0;
> +}
> +
>  static const struct i2c_device_id ad7746_id[] =3D {
>  	{ "ad7745", 7745 },
>  	{ "ad7746", 7746 },
> @@ -792,6 +801,7 @@ static struct i2c_driver ad7746_driver =3D {
>  		.of_match_table =3D of_match_ptr(ad7746_of_match),
>  	},
>  	.probe =3D ad7746_probe,
> +	.remove =3D ad7746_remove,
>  	.id_table =3D ad7746_id,
>  };
>  module_i2c_driver(ad7746_driver);

[PATCH 08/11] staging: iio: ad7746: Add comments

[Hernán Gonzalez]

Add comments to clarify some of the calculations made, specially when
reading or writing values.

Signed-off-by: Hernán Gonzalez <hernan@vanguardiasur.com.ar>
---
 drivers/staging/iio/cdc/ad7746.c | 34 ++++++++++++++++++++++++++++------
 1 file changed, 28 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
index 8abba71..b6b99e2 100644
--- a/drivers/staging/iio/cdc/ad7746.c
+++ b/drivers/staging/iio/cdc/ad7746.c
@@ -420,6 +420,7 @@ static int ad7746_write_raw(struct iio_dev *indio_dev,
 			goto out;
 		}
 
+		/* 2^16 in micro */
 		val = (val2 * 1024) / 15625;
 
 		switch (chan->type) {
@@ -546,6 +547,13 @@ static int ad7746_read_raw(struct iio_dev *indio_dev,
 		if (ret < 0)
 			goto out;
 
+		/*
+		 * Either for Capacitance, Voltage or Temperature,
+		 * the 0x000000 code represents negative full scale,
+		 * the 0x800000 code represents zero scale, and
+		 * the 0xFFFFFF code represents positive full scale.
+		 */
+
 		*val = (be32_to_cpu(chip->data.d32) & 0xFFFFFF) - 0x800000;
 
 		switch (chan->type) {
@@ -557,7 +565,13 @@ static int ad7746_read_raw(struct iio_dev *indio_dev,
 			*val = (*val * 125) / 256;
 			break;
 		case IIO_VOLTAGE:
-			if (chan->channel == 1) /* supply_raw*/
+
+			/*
+			 * The voltage from the VDD pin is internally
+			 *  attenuated by 6.
+			 */
+
+			if (chan->channel == 1) /* supply_raw */
 				*val = *val * 6;
 			break;
 		default:
@@ -598,21 +612,28 @@ static int ad7746_read_raw(struct iio_dev *indio_dev,
 		ret = IIO_VAL_INT;
 		break;
 	case IIO_CHAN_INFO_OFFSET:
+
+		/*
+		 * CAPDAC Scale = 21pF_typ / 127
+		 * CIN Scale = 8.192pF / 2^24
+		 * Offset Scale = CAPDAC Scale / CIN Scale = 338646
+		 */
+
 		*val = AD7746_CAPDAC_DACP(chip->capdac[chan->channel]
-			[chan->differential]) * 338646;
+					  [chan->differential]) * 338646;
 
 		ret = IIO_VAL_INT;
 		break;
 	case IIO_CHAN_INFO_SCALE:
 		switch (chan->type) {
 		case IIO_CAPACITANCE:
-			/* 8.192pf / 2^24 */
+			/* CIN Scale: 8.192pf / 2^24 */
 			*val =  0;
 			*val2 = 488;
 			ret = IIO_VAL_INT_PLUS_NANO;
 			break;
 		case IIO_VOLTAGE:
-			/* 1170mV / 2^23 */
+			/* VIN Scale: 1170mV / 2^23 */
 			*val = 1170;
 			*val2 = 23;
 			ret = IIO_VAL_FRACTIONAL_LOG2;
@@ -666,7 +687,8 @@ static struct ad7746_platform_data *ad7746_parse_dt(struct device *dev)
 	unsigned int tmp;
 	int ret;
 
-	/* The default excitation outputs are not inverted, it should be stated
+	/*
+	 * The default excitation outputs are not inverted, it should be stated
 	 * in the dt if needed.
 	 */
 
@@ -678,7 +700,7 @@ static struct ad7746_platform_data *ad7746_parse_dt(struct device *dev)
 	ret = of_property_read_u32(np, "adi,exclvl", &tmp);
 	if (ret || tmp > 3) {
 		dev_warn(dev, "Wrong exclvl value, using default\n");
-		pdata->exclvl = 3; /* default value */
+		pdata->exclvl = 3;
 	} else {
 		pdata->exclvl = tmp;
 	}
-- 
2.7.4

Re: [PATCH 08/11] staging: iio: ad7746: Add comments

[Jonathan Cameron]

On Wed, 21 Mar 2018 11:28:56 -0300
Hernán Gonzalez <hernan@vanguardiasur.com.ar> wrote:

> Add comments to clarify some of the calculations made, specially when
> reading or writing values.
> 
> Signed-off-by: Hernán Gonzalez <hernan@vanguardiasur.com.ar>
> ---
>  drivers/staging/iio/cdc/ad7746.c | 34 ++++++++++++++++++++++++++++------
>  1 file changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
> index 8abba71..b6b99e2 100644
> --- a/drivers/staging/iio/cdc/ad7746.c
> +++ b/drivers/staging/iio/cdc/ad7746.c
> @@ -420,6 +420,7 @@ static int ad7746_write_raw(struct iio_dev *indio_dev,
>  			goto out;
>  		}
>  
> +		/* 2^16 in micro */
>  		val = (val2 * 1024) / 15625;
>  
>  		switch (chan->type) {
> @@ -546,6 +547,13 @@ static int ad7746_read_raw(struct iio_dev *indio_dev,
>  		if (ret < 0)
>  			goto out;
>  
> +		/*
> +		 * Either for Capacitance, Voltage or Temperature,
> +		 * the 0x000000 code represents negative full scale,
> +		 * the 0x800000 code represents zero scale, and
> +		 * the 0xFFFFFF code represents positive full scale.
> +		 */
> +
>  		*val = (be32_to_cpu(chip->data.d32) & 0xFFFFFF) - 0x800000;
This should be left for userspace to deal with.  That is it should be
exported as an offset for the various channels rather than applied here.

>  
>  		switch (chan->type) {
> @@ -557,7 +565,13 @@ static int ad7746_read_raw(struct iio_dev *indio_dev,
>  			*val = (*val * 125) / 256;
>  			break;
>  		case IIO_VOLTAGE:
> -			if (chan->channel == 1) /* supply_raw*/
> +
> +			/*
> +			 * The voltage from the VDD pin is internally
> +			 *  attenuated by 6.
Extra space before attenuated.
> +			 */
> +
> +			if (chan->channel == 1) /* supply_raw */
>  				*val = *val * 6;
>  			break;
>  		default:
> @@ -598,21 +612,28 @@ static int ad7746_read_raw(struct iio_dev *indio_dev,
>  		ret = IIO_VAL_INT;
>  		break;
>  	case IIO_CHAN_INFO_OFFSET:
> +
> +		/*
> +		 * CAPDAC Scale = 21pF_typ / 127
> +		 * CIN Scale = 8.192pF / 2^24
> +		 * Offset Scale = CAPDAC Scale / CIN Scale = 338646
> +		 */
> +
>  		*val = AD7746_CAPDAC_DACP(chip->capdac[chan->channel]
> -			[chan->differential]) * 338646;
> +					  [chan->differential]) * 338646;
This change should have been in the alignment fixing patch, not here.

>  
>  		ret = IIO_VAL_INT;
>  		break;
>  	case IIO_CHAN_INFO_SCALE:
>  		switch (chan->type) {
>  		case IIO_CAPACITANCE:
> -			/* 8.192pf / 2^24 */
> +			/* CIN Scale: 8.192pf / 2^24 */
>  			*val =  0;
>  			*val2 = 488;
>  			ret = IIO_VAL_INT_PLUS_NANO;
>  			break;
>  		case IIO_VOLTAGE:
> -			/* 1170mV / 2^23 */
> +			/* VIN Scale: 1170mV / 2^23 */
>  			*val = 1170;
>  			*val2 = 23;
>  			ret = IIO_VAL_FRACTIONAL_LOG2;
> @@ -666,7 +687,8 @@ static struct ad7746_platform_data *ad7746_parse_dt(struct device *dev)
>  	unsigned int tmp;
>  	int ret;
>  
> -	/* The default excitation outputs are not inverted, it should be stated
> +	/*
> +	 * The default excitation outputs are not inverted, it should be stated
>  	 * in the dt if needed.
>  	 */
>  
> @@ -678,7 +700,7 @@ static struct ad7746_platform_data *ad7746_parse_dt(struct device *dev)
>  	ret = of_property_read_u32(np, "adi,exclvl", &tmp);
>  	if (ret || tmp > 3) {
>  		dev_warn(dev, "Wrong exclvl value, using default\n");
> -		pdata->exclvl = 3; /* default value */
> +		pdata->exclvl = 3;
>  	} else {
>  		pdata->exclvl = tmp;
>  	}

[PATCH 09/11] staging: iio: ad7746: Add devicetree bindings documentation

[Hernán Gonzalez]

Signed-off-by: Hernán Gonzalez <hernan@vanguardiasur.com.ar>
---
 .../devicetree/bindings/staging/iio/cdc/ad7746.txt | 32 ++++++++++++++++++++++
 1 file changed, 32 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/staging/iio/cdc/ad7746.txt

diff --git a/Documentation/devicetree/bindings/staging/iio/cdc/ad7746.txt b/Documentation/devicetree/bindings/staging/iio/cdc/ad7746.txt
new file mode 100644
index 0000000..13d0056
--- /dev/null
+++ b/Documentation/devicetree/bindings/staging/iio/cdc/ad7746.txt
@@ -0,0 +1,32 @@
+Analog Devices AD7746/5/7 capacitive sensor driver
+
+Required properties:
+	- compatible: Should be one of
+		* "adi,ad7745": When using the AD7745 device
+		* "adi,ad7746": When using the AD7746 device
+		* "adi,ad7747": When using the AD7747 device
+	- reg: The 7-bits long I2c address of the device
+
+Optional properties:
+	- adi,exclvl: 0 = +-VDD/8
+                1 = +-VDD/4
+                2 = +-VDD * 3/8
+                3 = +-VDD/2 (Default)
+	- adi,nexca_en: Invert excitation output A.
+	- adi,nexcb_en:	Invert excitation output B.
+
+Example:
+Here exclvl would be 1 (VDD/4), Excitation pin A would be inverted and
+Excitation pin B would NOT be inverted.
+
+i2c2 {
+
+      < . . . >
+
+      ad7746: ad7746@60 {
+              compatible = "ad7746";
+              reg = <0x60>;
+              adi,exclvl = <1>;
+              adi,nexca_en;
+      };
+};
-- 
2.7.4

Re: [PATCH 09/11] staging: iio: ad7746: Add devicetree bindings documentation

[Jonathan Cameron]

On Wed, 21 Mar 2018 11:28:57 -0300
Hernán Gonzalez <hernan@vanguardiasur.com.ar> wrote:

> Signed-off-by: Hernán Gonzalez <hernan@vanguardiasur.com.ar>
Device tree bindings must be cc'd to the device tree binding maintainers
and the devicetree binding list.

Comments inline but don't forget to cc them on v2.

Jonathan

> ---
>  .../devicetree/bindings/staging/iio/cdc/ad7746.txt | 32 ++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/staging/iio/cdc/ad7746.txt
> 
> diff --git a/Documentation/devicetree/bindings/staging/iio/cdc/ad7746.txt b/Documentation/devicetree/bindings/staging/iio/cdc/ad7746.txt
> new file mode 100644
> index 0000000..13d0056
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/staging/iio/cdc/ad7746.txt
> @@ -0,0 +1,32 @@
> +Analog Devices AD7746/5/7 capacitive sensor driver
> +
> +Required properties:
> +	- compatible: Should be one of
> +		* "adi,ad7745": When using the AD7745 device
> +		* "adi,ad7746": When using the AD7746 device
> +		* "adi,ad7747": When using the AD7747 device
The when using bit is kind of obvious.  Probably no need to state.

> +	- reg: The 7-bits long I2c address of the device
> +
> +Optional properties:
> +	- adi,exclvl: 0 = +-VDD/8
> +                1 = +-VDD/4
> +                2 = +-VDD * 3/8
> +                3 = +-VDD/2 (Default)
That needs a comment to explain what exactly it is rather than
just documenting the enum values.

> +	- adi,nexca_en: Invert excitation output A.
> +	- adi,nexcb_en:	Invert excitation output B.
> +
> +Example:
> +Here exclvl would be 1 (VDD/4), Excitation pin A would be inverted and
> +Excitation pin B would NOT be inverted.
> +
> +i2c2 {
> +
> +      < . . . >
> +
> +      ad7746: ad7746@60 {
> +              compatible = "ad7746";
> +              reg = <0x60>;
> +              adi,exclvl = <1>;
> +              adi,nexca_en;
> +      };
> +};

[PATCH 10/11] staging: iio: ad7746: Rename sysfs attrs to comply with the ABI

[Hernán Gonzalez]

Signed-off-by: Hernán Gonzalez <hernan@vanguardiasur.com.ar>
---
 drivers/staging/iio/cdc/ad7746.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
index b6b99e2..c1f76fc 100644
--- a/drivers/staging/iio/cdc/ad7746.c
+++ b/drivers/staging/iio/cdc/ad7746.c
@@ -336,16 +336,16 @@ static ssize_t ad7746_start_gain_calib(struct device *dev,
 				  AD7746_CONF_MODE_GAIN_CAL);
 }
 
-static IIO_DEVICE_ATTR(in_capacitance0_calibbias_calibration,
-		       0200, NULL, ad7746_start_offset_calib, CIN1);
-static IIO_DEVICE_ATTR(in_capacitance1_calibbias_calibration,
-		       0200, NULL, ad7746_start_offset_calib, CIN2);
-static IIO_DEVICE_ATTR(in_capacitance0_calibscale_calibration,
-		       0200, NULL, ad7746_start_gain_calib, CIN1);
-static IIO_DEVICE_ATTR(in_capacitance1_calibscale_calibration,
-		       0200, NULL, ad7746_start_gain_calib, CIN2);
-static IIO_DEVICE_ATTR(in_voltage0_calibscale_calibration,
-		       0200, NULL, ad7746_start_gain_calib, VIN);
+static IIO_DEVICE_ATTR(in_capacitance0_calibbias, 0200, NULL,
+		       ad7746_start_offset_calib, CIN1);
+static IIO_DEVICE_ATTR(in_capacitance1_calibbias, 0200, NULL,
+		       ad7746_start_offset_calib, CIN2);
+static IIO_DEVICE_ATTR(in_capacitance0_calibscale, 0200, NULL,
+		       ad7746_start_gain_calib, CIN1);
+static IIO_DEVICE_ATTR(in_capacitance1_calibscale, 0200, NULL,
+		       ad7746_start_gain_calib, CIN2);
+static IIO_DEVICE_ATTR(in_voltage0_calibscale, 0200, NULL,
+		       ad7746_start_gain_calib, VIN);
 
 static int ad7746_store_cap_filter_rate_setup(struct ad7746_chip_info *chip,
 					      int val)
@@ -388,11 +388,11 @@ static IIO_CONST_ATTR(in_capacitance_sampling_frequency_available,
 		       "91 84 50 26 16 13 11 9");
 
 static struct attribute *ad7746_attributes[] = {
-	&iio_dev_attr_in_capacitance0_calibbias_calibration.dev_attr.attr,
-	&iio_dev_attr_in_capacitance0_calibscale_calibration.dev_attr.attr,
-	&iio_dev_attr_in_capacitance1_calibscale_calibration.dev_attr.attr,
-	&iio_dev_attr_in_capacitance1_calibbias_calibration.dev_attr.attr,
-	&iio_dev_attr_in_voltage0_calibscale_calibration.dev_attr.attr,
+	&iio_dev_attr_in_capacitance0_calibbias.dev_attr.attr,
+	&iio_dev_attr_in_capacitance0_calibscale.dev_attr.attr,
+	&iio_dev_attr_in_capacitance1_calibscale.dev_attr.attr,
+	&iio_dev_attr_in_capacitance1_calibbias.dev_attr.attr,
+	&iio_dev_attr_in_voltage0_calibscale.dev_attr.attr,
 	&iio_const_attr_in_voltage_sampling_frequency_available.dev_attr.attr,
 	&iio_const_attr_in_capacitance_sampling_frequency_available.dev_attr.attr,
 	NULL,
-- 
2.7.4

Re: [PATCH 10/11] staging: iio: ad7746: Rename sysfs attrs to comply with the ABI

[Jonathan Cameron]

On Wed, 21 Mar 2018 11:28:58 -0300
Hernán Gonzalez <hernan@vanguardiasur.com.ar> wrote:

> Signed-off-by: Hernán Gonzalez <hernan@vanguardiasur.com.ar>
Please look at what these do.  It is not what those abi elements are
documented as doing.

Also if it were you would need to support them via read_raw and write_raw
and put them in the relevant info mask elements.

Jonathan
> ---
>  drivers/staging/iio/cdc/ad7746.c | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
> index b6b99e2..c1f76fc 100644
> --- a/drivers/staging/iio/cdc/ad7746.c
> +++ b/drivers/staging/iio/cdc/ad7746.c
> @@ -336,16 +336,16 @@ static ssize_t ad7746_start_gain_calib(struct device *dev,
>  				  AD7746_CONF_MODE_GAIN_CAL);
>  }
>  
> -static IIO_DEVICE_ATTR(in_capacitance0_calibbias_calibration,
> -		       0200, NULL, ad7746_start_offset_calib, CIN1);
> -static IIO_DEVICE_ATTR(in_capacitance1_calibbias_calibration,
> -		       0200, NULL, ad7746_start_offset_calib, CIN2);
> -static IIO_DEVICE_ATTR(in_capacitance0_calibscale_calibration,
> -		       0200, NULL, ad7746_start_gain_calib, CIN1);
> -static IIO_DEVICE_ATTR(in_capacitance1_calibscale_calibration,
> -		       0200, NULL, ad7746_start_gain_calib, CIN2);
> -static IIO_DEVICE_ATTR(in_voltage0_calibscale_calibration,
> -		       0200, NULL, ad7746_start_gain_calib, VIN);
> +static IIO_DEVICE_ATTR(in_capacitance0_calibbias, 0200, NULL,
> +		       ad7746_start_offset_calib, CIN1);
> +static IIO_DEVICE_ATTR(in_capacitance1_calibbias, 0200, NULL,
> +		       ad7746_start_offset_calib, CIN2);
> +static IIO_DEVICE_ATTR(in_capacitance0_calibscale, 0200, NULL,
> +		       ad7746_start_gain_calib, CIN1);
> +static IIO_DEVICE_ATTR(in_capacitance1_calibscale, 0200, NULL,
> +		       ad7746_start_gain_calib, CIN2);
> +static IIO_DEVICE_ATTR(in_voltage0_calibscale, 0200, NULL,
> +		       ad7746_start_gain_calib, VIN);
>  
>  static int ad7746_store_cap_filter_rate_setup(struct ad7746_chip_info *chip,
>  					      int val)
> @@ -388,11 +388,11 @@ static IIO_CONST_ATTR(in_capacitance_sampling_frequency_available,
>  		       "91 84 50 26 16 13 11 9");
>  
>  static struct attribute *ad7746_attributes[] = {
> -	&iio_dev_attr_in_capacitance0_calibbias_calibration.dev_attr.attr,
> -	&iio_dev_attr_in_capacitance0_calibscale_calibration.dev_attr.attr,
> -	&iio_dev_attr_in_capacitance1_calibscale_calibration.dev_attr.attr,
> -	&iio_dev_attr_in_capacitance1_calibbias_calibration.dev_attr.attr,
> -	&iio_dev_attr_in_voltage0_calibscale_calibration.dev_attr.attr,
> +	&iio_dev_attr_in_capacitance0_calibbias.dev_attr.attr,
> +	&iio_dev_attr_in_capacitance0_calibscale.dev_attr.attr,
> +	&iio_dev_attr_in_capacitance1_calibscale.dev_attr.attr,
> +	&iio_dev_attr_in_capacitance1_calibbias.dev_attr.attr,
> +	&iio_dev_attr_in_voltage0_calibscale.dev_attr.attr,
>  	&iio_const_attr_in_voltage_sampling_frequency_available.dev_attr.attr,
>  	&iio_const_attr_in_capacitance_sampling_frequency_available.dev_attr.attr,
>  	NULL,

Re: [PATCH 10/11] staging: iio: ad7746: Rename sysfs attrs to comply with the ABI

[Jonathan Cameron]

On Wed, 21 Mar 2018 11:28:58 -0300
Hern=E1n Gonzalez <hernan@vanguardiasur.com.ar> wrote:

> Signed-off-by: Hern=E1n Gonzalez <hernan@vanguardiasur.com.ar>
Please look at what these do.  It is not what those abi elements are
documented as doing.

Also if it were you would need to support them via read_raw and write_raw
and put them in the relevant info mask elements.

Jonathan
> ---
>  drivers/staging/iio/cdc/ad7746.c | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
>=20
> diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/a=
d7746.c
> index b6b99e2..c1f76fc 100644
> --- a/drivers/staging/iio/cdc/ad7746.c
> +++ b/drivers/staging/iio/cdc/ad7746.c
> @@ -336,16 +336,16 @@ static ssize_t ad7746_start_gain_calib(struct devic=
e *dev,
>  				  AD7746_CONF_MODE_GAIN_CAL);
>  }
> =20
> -static IIO_DEVICE_ATTR(in_capacitance0_calibbias_calibration,
> -		       0200, NULL, ad7746_start_offset_calib, CIN1);
> -static IIO_DEVICE_ATTR(in_capacitance1_calibbias_calibration,
> -		       0200, NULL, ad7746_start_offset_calib, CIN2);
> -static IIO_DEVICE_ATTR(in_capacitance0_calibscale_calibration,
> -		       0200, NULL, ad7746_start_gain_calib, CIN1);
> -static IIO_DEVICE_ATTR(in_capacitance1_calibscale_calibration,
> -		       0200, NULL, ad7746_start_gain_calib, CIN2);
> -static IIO_DEVICE_ATTR(in_voltage0_calibscale_calibration,
> -		       0200, NULL, ad7746_start_gain_calib, VIN);
> +static IIO_DEVICE_ATTR(in_capacitance0_calibbias, 0200, NULL,
> +		       ad7746_start_offset_calib, CIN1);
> +static IIO_DEVICE_ATTR(in_capacitance1_calibbias, 0200, NULL,
> +		       ad7746_start_offset_calib, CIN2);
> +static IIO_DEVICE_ATTR(in_capacitance0_calibscale, 0200, NULL,
> +		       ad7746_start_gain_calib, CIN1);
> +static IIO_DEVICE_ATTR(in_capacitance1_calibscale, 0200, NULL,
> +		       ad7746_start_gain_calib, CIN2);
> +static IIO_DEVICE_ATTR(in_voltage0_calibscale, 0200, NULL,
> +		       ad7746_start_gain_calib, VIN);
> =20
>  static int ad7746_store_cap_filter_rate_setup(struct ad7746_chip_info *c=
hip,
>  					      int val)
> @@ -388,11 +388,11 @@ static IIO_CONST_ATTR(in_capacitance_sampling_frequ=
ency_available,
>  		       "91 84 50 26 16 13 11 9");
> =20
>  static struct attribute *ad7746_attributes[] =3D {
> -	&iio_dev_attr_in_capacitance0_calibbias_calibration.dev_attr.attr,
> -	&iio_dev_attr_in_capacitance0_calibscale_calibration.dev_attr.attr,
> -	&iio_dev_attr_in_capacitance1_calibscale_calibration.dev_attr.attr,
> -	&iio_dev_attr_in_capacitance1_calibbias_calibration.dev_attr.attr,
> -	&iio_dev_attr_in_voltage0_calibscale_calibration.dev_attr.attr,
> +	&iio_dev_attr_in_capacitance0_calibbias.dev_attr.attr,
> +	&iio_dev_attr_in_capacitance0_calibscale.dev_attr.attr,
> +	&iio_dev_attr_in_capacitance1_calibscale.dev_attr.attr,
> +	&iio_dev_attr_in_capacitance1_calibbias.dev_attr.attr,
> +	&iio_dev_attr_in_voltage0_calibscale.dev_attr.attr,
>  	&iio_const_attr_in_voltage_sampling_frequency_available.dev_attr.attr,
>  	&iio_const_attr_in_capacitance_sampling_frequency_available.dev_attr.at=
tr,
>  	NULL,

[PATCH 11/11] Move cdc ad7746 driver out of staging to mainline iio

[Hernán Gonzalez]

Also modify the proper Kconfigs and move documentation.

Signed-off-by: Hernán Gonzalez <hernan@vanguardiasur.com.ar>
---
 .../devicetree/bindings/{staging => }/iio/cdc/ad7746.txt |  0
 drivers/iio/Kconfig                                      |  1 +
 drivers/iio/cdc/Kconfig                                  | 16 ++++++++++++++++
 drivers/{staging => }/iio/cdc/ad7746.c                   |  2 +-
 drivers/staging/iio/cdc/Kconfig                          | 10 ----------
 {drivers/staging => include/linux}/iio/cdc/ad7746.h      |  4 ----
 6 files changed, 18 insertions(+), 15 deletions(-)
 rename Documentation/devicetree/bindings/{staging => }/iio/cdc/ad7746.txt (100%)
 create mode 100644 drivers/iio/cdc/Kconfig
 rename drivers/{staging => }/iio/cdc/ad7746.c (99%)
 rename {drivers/staging => include/linux}/iio/cdc/ad7746.h (88%)

diff --git a/Documentation/devicetree/bindings/staging/iio/cdc/ad7746.txt b/Documentation/devicetree/bindings/iio/cdc/ad7746.txt
similarity index 100%
rename from Documentation/devicetree/bindings/staging/iio/cdc/ad7746.txt
rename to Documentation/devicetree/bindings/iio/cdc/ad7746.txt
diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
index b3c8c6e..d1c309b 100644
--- a/drivers/iio/Kconfig
+++ b/drivers/iio/Kconfig
@@ -71,6 +71,7 @@ config IIO_TRIGGERED_EVENT
 source "drivers/iio/accel/Kconfig"
 source "drivers/iio/adc/Kconfig"
 source "drivers/iio/amplifiers/Kconfig"
+source "drivers/iio/cdc/Kconfig"
 source "drivers/iio/chemical/Kconfig"
 source "drivers/iio/common/Kconfig"
 source "drivers/iio/counter/Kconfig"
diff --git a/drivers/iio/cdc/Kconfig b/drivers/iio/cdc/Kconfig
new file mode 100644
index 0000000..d3a8600
--- /dev/null
+++ b/drivers/iio/cdc/Kconfig
@@ -0,0 +1,16 @@
+#
+# CDC drivers
+#
+menu "Capacitance to digital converters"
+
+config AD7746
+	tristate "Analog Devices AD7745, AD7746 AD7747 capacitive sensor driver"
+	depends on I2C
+	help
+	  Say yes here to build support for Analog Devices capacitive sensors.
+	  (AD7745, AD7746, AD7747) Provides direct access via sysfs.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ad7746.
+
+endmenu
diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/iio/cdc/ad7746.c
similarity index 99%
rename from drivers/staging/iio/cdc/ad7746.c
rename to drivers/iio/cdc/ad7746.c
index c1f76fc..23c9f61 100644
--- a/drivers/staging/iio/cdc/ad7746.c
+++ b/drivers/iio/cdc/ad7746.c
@@ -18,8 +18,8 @@
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
+#include <linux/iio/cdc/ad7746.h>
 
-#include "ad7746.h"
 
 /*
  * AD7746 Register Definition
diff --git a/drivers/staging/iio/cdc/Kconfig b/drivers/staging/iio/cdc/Kconfig
index 80211df..a170ab3 100644
--- a/drivers/staging/iio/cdc/Kconfig
+++ b/drivers/staging/iio/cdc/Kconfig
@@ -23,14 +23,4 @@ config AD7152
 	  To compile this driver as a module, choose M here: the
 	  module will be called ad7152.
 
-config AD7746
-	tristate "Analog Devices AD7745, AD7746 AD7747 capacitive sensor driver"
-	depends on I2C
-	help
-	  Say yes here to build support for Analog Devices capacitive sensors.
-	  (AD7745, AD7746, AD7747) Provides direct access via sysfs.
-
-	  To compile this driver as a module, choose M here: the
-	  module will be called ad7746.
-
 endmenu
diff --git a/drivers/staging/iio/cdc/ad7746.h b/include/linux/iio/cdc/ad7746.h
similarity index 88%
rename from drivers/staging/iio/cdc/ad7746.h
rename to include/linux/iio/cdc/ad7746.h
index 2fbcee8..46ff25e 100644
--- a/drivers/staging/iio/cdc/ad7746.h
+++ b/include/linux/iio/cdc/ad7746.h
@@ -9,10 +9,6 @@
 #ifndef IIO_CDC_AD7746_H_
 #define IIO_CDC_AD7746_H_
 
-/*
- * TODO: struct ad7746_platform_data needs to go into include/linux/iio
- */
-
 struct ad7746_platform_data {
 	unsigned char exclvl;	/*Excitation Voltage Level */
 	bool exca_en;		/* enables EXCA pin as the excitation output */
-- 
2.7.4

Re: [PATCH 11/11] Move cdc ad7746 driver out of staging to mainline iio

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1020 bytes --]

Hi Hernán,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on iio/togreg]
[also build test ERROR on v4.16-rc6 next-20180322]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Hern-n-Gonzalez/Move-ad7746-out-of-staging/20180323-163331
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: x86_64-randconfig-u0-03231537 (attached as .config)
compiler: gcc-5 (Debian 5.5.0-3) 5.4.1 20171010
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

>> make[5]: *** No rule to make target 'drivers/staging/iio/cdc/ad7746.c', needed by 'drivers/staging/iio/cdc/ad7746.o'.
   make[5]: Target '__build' not remade because of errors.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30461 bytes --]

Re: [PATCH 11/11] Move cdc ad7746 driver out of staging to mainline iio

[Jonathan Cameron]

On Fri, 23 Mar 2018 18:21:58 +0800
kbuild test robot <lkp@intel.com> wrote:

> Hi Hernán,
> 
> Thank you for the patch! Yet something to improve:
I guess you have figured this out already, but you missed updating the make
files in your patch.

Jonathan
> 
> [auto build test ERROR on iio/togreg]
> [also build test ERROR on v4.16-rc6 next-20180322]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Hern-n-Gonzalez/Move-ad7746-out-of-staging/20180323-163331
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
> config: x86_64-randconfig-u0-03231537 (attached as .config)
> compiler: gcc-5 (Debian 5.5.0-3) 5.4.1 20171010
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=x86_64 
> 
> All errors (new ones prefixed by >>):
> 
> >> make[5]: *** No rule to make target 'drivers/staging/iio/cdc/ad7746.c', needed by 'drivers/staging/iio/cdc/ad7746.o'.  
>    make[5]: Target '__build' not remade because of errors.
> 
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
> 

Re: [PATCH 11/11] Move cdc ad7746 driver out of staging to mainline iio

[Jonathan Cameron]

On Wed, 21 Mar 2018 11:28:59 -0300
Hernán Gonzalez <hernan@vanguardiasur.com.ar> wrote:

> Also modify the proper Kconfigs and move documentation.
> 
> Signed-off-by: Hernán Gonzalez <hernan@vanguardiasur.com.ar>

Please disable git move detection for this patch in v2.
This only applies when moving drivers out of staging (and possibly only
me who asks for it then ;).  The point is to allow full review of the
driver we are actually moving.  That is hard to do if we don't have
the code in the email.

Anyhow, good work on the series in general. I look forward to V2.

Jonathan
> ---
>  .../devicetree/bindings/{staging => }/iio/cdc/ad7746.txt |  0
>  drivers/iio/Kconfig                                      |  1 +
>  drivers/iio/cdc/Kconfig                                  | 16 ++++++++++++++++
>  drivers/{staging => }/iio/cdc/ad7746.c                   |  2 +-
>  drivers/staging/iio/cdc/Kconfig                          | 10 ----------
>  {drivers/staging => include/linux}/iio/cdc/ad7746.h      |  4 ----
>  6 files changed, 18 insertions(+), 15 deletions(-)
>  rename Documentation/devicetree/bindings/{staging => }/iio/cdc/ad7746.txt (100%)
>  create mode 100644 drivers/iio/cdc/Kconfig
>  rename drivers/{staging => }/iio/cdc/ad7746.c (99%)
>  rename {drivers/staging => include/linux}/iio/cdc/ad7746.h (88%)
> 
> diff --git a/Documentation/devicetree/bindings/staging/iio/cdc/ad7746.txt b/Documentation/devicetree/bindings/iio/cdc/ad7746.txt
> similarity index 100%
> rename from Documentation/devicetree/bindings/staging/iio/cdc/ad7746.txt
> rename to Documentation/devicetree/bindings/iio/cdc/ad7746.txt
> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
> index b3c8c6e..d1c309b 100644
> --- a/drivers/iio/Kconfig
> +++ b/drivers/iio/Kconfig
> @@ -71,6 +71,7 @@ config IIO_TRIGGERED_EVENT
>  source "drivers/iio/accel/Kconfig"
>  source "drivers/iio/adc/Kconfig"
>  source "drivers/iio/amplifiers/Kconfig"
> +source "drivers/iio/cdc/Kconfig"
>  source "drivers/iio/chemical/Kconfig"
>  source "drivers/iio/common/Kconfig"
>  source "drivers/iio/counter/Kconfig"
> diff --git a/drivers/iio/cdc/Kconfig b/drivers/iio/cdc/Kconfig
> new file mode 100644
> index 0000000..d3a8600
> --- /dev/null
> +++ b/drivers/iio/cdc/Kconfig
> @@ -0,0 +1,16 @@
> +#
> +# CDC drivers
> +#
> +menu "Capacitance to digital converters"
> +
> +config AD7746
> +	tristate "Analog Devices AD7745, AD7746 AD7747 capacitive sensor driver"
> +	depends on I2C
> +	help
> +	  Say yes here to build support for Analog Devices capacitive sensors.
> +	  (AD7745, AD7746, AD7747) Provides direct access via sysfs.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called ad7746.
> +
> +endmenu
> diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/iio/cdc/ad7746.c
> similarity index 99%
> rename from drivers/staging/iio/cdc/ad7746.c
> rename to drivers/iio/cdc/ad7746.c
> index c1f76fc..23c9f61 100644
> --- a/drivers/staging/iio/cdc/ad7746.c
> +++ b/drivers/iio/cdc/ad7746.c
> @@ -18,8 +18,8 @@
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> +#include <linux/iio/cdc/ad7746.h>
>  
> -#include "ad7746.h"
>  
>  /*
>   * AD7746 Register Definition
> diff --git a/drivers/staging/iio/cdc/Kconfig b/drivers/staging/iio/cdc/Kconfig
> index 80211df..a170ab3 100644
> --- a/drivers/staging/iio/cdc/Kconfig
> +++ b/drivers/staging/iio/cdc/Kconfig
> @@ -23,14 +23,4 @@ config AD7152
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called ad7152.
>  
> -config AD7746
> -	tristate "Analog Devices AD7745, AD7746 AD7747 capacitive sensor driver"
> -	depends on I2C
> -	help
> -	  Say yes here to build support for Analog Devices capacitive sensors.
> -	  (AD7745, AD7746, AD7747) Provides direct access via sysfs.
> -
> -	  To compile this driver as a module, choose M here: the
> -	  module will be called ad7746.
> -
>  endmenu
> diff --git a/drivers/staging/iio/cdc/ad7746.h b/include/linux/iio/cdc/ad7746.h
> similarity index 88%
> rename from drivers/staging/iio/cdc/ad7746.h
> rename to include/linux/iio/cdc/ad7746.h
> index 2fbcee8..46ff25e 100644
> --- a/drivers/staging/iio/cdc/ad7746.h
> +++ b/include/linux/iio/cdc/ad7746.h
> @@ -9,10 +9,6 @@
>  #ifndef IIO_CDC_AD7746_H_
>  #define IIO_CDC_AD7746_H_
>  
> -/*
> - * TODO: struct ad7746_platform_data needs to go into include/linux/iio
> - */
> -
>  struct ad7746_platform_data {
>  	unsigned char exclvl;	/*Excitation Voltage Level */
>  	bool exca_en;		/* enables EXCA pin as the excitation output */

Re: [PATCH 00/11] Move ad7746 out of staging

[Jonathan Cameron]

On Wed, 21 Mar 2018 11:28:48 -0300
Hernán Gonzalez <hernan@vanguardiasur.com.ar> wrote:

> This patch series aims to move the cdc ad7746 driver out of staging. I have some
> design questions though so I would introduce them here, along with a short
> description of each patch.
> 
> *PATCH 0001 - Adjust arguments to match open parenthesis.
> There were a couple CHECKS that still remained, so I got rid of them.
Don't bother describing straight forward patches in the cover letter.
Adds noise to the interesting message.
> 
> *PATCH 0002 - Fix multiple line dereference
> In this case, I opted for avoiding the multiple line derefence and having a 80+
> characters line instead as I consider that it improves readability. I may be
> wrong though, so this patch could just be discarded.
> 
> *PATCH 0003 - Reorder includes alphabetically
> 
> *PATCH 0004 - Reorder variable declarations in an inverser-pyramid way
> 
> *PATCH 0005 - Remove unused defines
> There were a few too many #defines that were not used at all, so I just removed
> them. I guess if someone plans on extending the drivers functionality they can
> be added again, but they were just wasting space as they were. Again, I could be
> wrong with this decision so this patch could just be discarded.
> 
> *PATCH 0006 - Add dt-bindings
> This patch adds dt bindings by populating the old pdata struct. It supports both
> platform_data and dt-bindings but uses only one depending on CONFIG_OF. I chose
> this way to avoid modifying too much the code, and introduce no errors (or as
> few as I could), keeping the same functionality and maintaining support of the
> platform_data.
> 
> *PATCH 0007 - Add remove()
> I added a remove function so I could test that the driver probed properly when
> compiled as a module with the new dt-bindings.
> 
> *PATCH 0008 - Add comments to clarify some of the calculations
> I had to go through most of the datasheet to understand some of the math in the
> code, so I added comments where I saw fit. (Comments on the comments are
> welcome).
> 
> *PATCH 0009 - Add devicetree bindings documentation
> Add documentation on the devicetree bindings, explaining the properties of it
> and describing a short example.
> 
> *PATCH 0010 - Rename sysfs attrs to comply with the ABI
> Comments are welcome on this one.
> I shortened the names of the sysfs attrs to comply with the ABI:
> <type>[Y]_calibbias_calibration -> <type>[Y]_calibbias
> <type>[Y]_calibscale_calibration -> <type>[Y]_calibscale
Hmm. so this one is interesting (note I didn't read the cover letter
and most people don't so better to have put this discussion in that patches
own description.

> 
> The device supports 2 ways of calibrating the gain (from the datasheet):
> 'The gain can be changed by executing a capacitance gain
> calibration mode, for which an external full-scale capacitance
> needs to be connected to the capacitance input, or by writing a
> user value to the capacitive gain register.'
So the second case is valid for the calibscale ABI, the second not.
> 
> The same for the offset calibration:
> 'One method of adjusting the offset is to connect a zero-scale
> capacitance to the input and execute the capacitance offset
> calibration mode. The calibration sets the midpoint of the
> ±4.096 pF range (that is, Output Code 0x800000) to that
> zero-scale input.
> Another method would be to calculate and write the offset cali-
> bration register value, the LSB is value 31.25 aF (4.096 pF/2^17 ).'
> 
> The driver only supports the first way in both cases, as it only writes the
> register that starts the calibration mode and doesn't allow the user to write
> anything on other registers.
> 
> What I understand from the ABI is not so different when explaining calibbias and
> calibscale:
> 'Description:
> Hardware applied calibration {offset,scale factor} (assumed to fix production
> inaccuracies).'
> 
> Maybe I'm missing something and the renaming is not good. I would be really
> grateful if someone could shed some light on this for me.
You are correct - it isn't good.  We can't 'bend' the ABI like this as
userspace would have no idea what to do with it.

This needs new ABI defining to cover the use case.  Please have a go and
make sure to provide documentation of the new ABI
/Documentation/ABI/testing/sysfs-bus-iio-ad7746 (for now - we can move
to the shared docs if it makes sense later).

> 
> *PATCH 0011 - Move cdc ad7746 driver out of staging to mainline iio
> Move the files, modify the proper Kconfigs and the documentation.
> 
> That'd be all. Any feedback is welcome. I hope this gets out of staging :)
> 
> Cheers,
> 
> Hernán
> 
> Hernán Gonzalez (11):
>   staging: iio: ad7746: Adjust arguments to match open parenthesis
>   staging: iio: ad7746: Fix multiple line dereference
>   staging: iio: ad7746: Reorder includes alphabetically
>   staging: iio: ad7746: Reorder variable declarations
>   staging: iio: ad7746: Remove unused defines
>   staging: iio: ad7746: Add dt-bindings
>   staging: iio: ad7746: Add remove()
>   staging: iio: ad7746: Add comments
>   staging: iio: ad7746: Add devicetree bindings documentation
>   staging: iio: ad7746: Rename sysfs attrs to comply with the ABI
>   Move cdc ad7746 driver out of staging to mainline iio
> 
>  .../devicetree/bindings/iio/cdc/ad7746.txt         |  32 ++++
>  drivers/iio/Kconfig                                |   1 +
>  drivers/iio/cdc/Kconfig                            |  16 ++
>  drivers/{staging => }/iio/cdc/ad7746.c             | 168 +++++++++++++++------
>  drivers/staging/iio/cdc/Kconfig                    |  10 --
>  .../staging => include/linux}/iio/cdc/ad7746.h     |   9 --
>  6 files changed, 168 insertions(+), 68 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/iio/cdc/ad7746.txt
>  create mode 100644 drivers/iio/cdc/Kconfig
>  rename drivers/{staging => }/iio/cdc/ad7746.c (84%)
>  rename {drivers/staging => include/linux}/iio/cdc/ad7746.h (70%)
> 

Re: [PATCH 00/11] Move ad7746 out of staging

[Jonathan Cameron]

On Wed, 21 Mar 2018 11:28:48 -0300
Hern=E1n Gonzalez <hernan@vanguardiasur.com.ar> wrote:

> This patch series aims to move the cdc ad7746 driver out of staging. I ha=
ve some
> design questions though so I would introduce them here, along with a short
> description of each patch.
>=20
> *PATCH 0001 - Adjust arguments to match open parenthesis.
> There were a couple CHECKS that still remained, so I got rid of them.
Don't bother describing straight forward patches in the cover letter.
Adds noise to the interesting message.
>=20
> *PATCH 0002 - Fix multiple line dereference
> In this case, I opted for avoiding the multiple line derefence and having=
 a 80+
> characters line instead as I consider that it improves readability. I may=
 be
> wrong though, so this patch could just be discarded.
>=20
> *PATCH 0003 - Reorder includes alphabetically
>=20
> *PATCH 0004 - Reorder variable declarations in an inverser-pyramid way
>=20
> *PATCH 0005 - Remove unused defines
> There were a few too many #defines that were not used at all, so I just r=
emoved
> them. I guess if someone plans on extending the drivers functionality the=
y can
> be added again, but they were just wasting space as they were. Again, I c=
ould be
> wrong with this decision so this patch could just be discarded.
>=20
> *PATCH 0006 - Add dt-bindings
> This patch adds dt bindings by populating the old pdata struct. It suppor=
ts both
> platform_data and dt-bindings but uses only one depending on CONFIG_OF. I=
 chose
> this way to avoid modifying too much the code, and introduce no errors (o=
r as
> few as I could), keeping the same functionality and maintaining support o=
f the
> platform_data.
>=20
> *PATCH 0007 - Add remove()
> I added a remove function so I could test that the driver probed properly=
 when
> compiled as a module with the new dt-bindings.
>=20
> *PATCH 0008 - Add comments to clarify some of the calculations
> I had to go through most of the datasheet to understand some of the math =
in the
> code, so I added comments where I saw fit. (Comments on the comments are
> welcome).
>=20
> *PATCH 0009 - Add devicetree bindings documentation
> Add documentation on the devicetree bindings, explaining the properties o=
f it
> and describing a short example.
>=20
> *PATCH 0010 - Rename sysfs attrs to comply with the ABI
> Comments are welcome on this one.
> I shortened the names of the sysfs attrs to comply with the ABI:
> <type>[Y]_calibbias_calibration -> <type>[Y]_calibbias
> <type>[Y]_calibscale_calibration -> <type>[Y]_calibscale
Hmm. so this one is interesting (note I didn't read the cover letter
and most people don't so better to have put this discussion in that patches
own description.

>=20
> The device supports 2 ways of calibrating the gain (from the datasheet):
> 'The gain can be changed by executing a capacitance gain
> calibration mode, for which an external full-scale capacitance
> needs to be connected to the capacitance input, or by writing a
> user value to the capacitive gain register.'
So the second case is valid for the calibscale ABI, the second not.
>=20
> The same for the offset calibration:
> 'One method of adjusting the offset is to connect a zero-scale
> capacitance to the input and execute the capacitance offset
> calibration mode. The calibration sets the midpoint of the
> =B14.096 pF range (that is, Output Code 0x800000) to that
> zero-scale input.
> Another method would be to calculate and write the offset cali-
> bration register value, the LSB is value 31.25 aF (4.096 pF/2^17 ).'
>=20
> The driver only supports the first way in both cases, as it only writes t=
he
> register that starts the calibration mode and doesn't allow the user to w=
rite
> anything on other registers.
>=20
> What I understand from the ABI is not so different when explaining calibb=
ias and
> calibscale:
> 'Description:
> Hardware applied calibration {offset,scale factor} (assumed to fix produc=
tion
> inaccuracies).'
>=20
> Maybe I'm missing something and the renaming is not good. I would be real=
ly
> grateful if someone could shed some light on this for me.
You are correct - it isn't good.  We can't 'bend' the ABI like this as
userspace would have no idea what to do with it.

This needs new ABI defining to cover the use case.  Please have a go and
make sure to provide documentation of the new ABI
/Documentation/ABI/testing/sysfs-bus-iio-ad7746 (for now - we can move
to the shared docs if it makes sense later).

>=20
> *PATCH 0011 - Move cdc ad7746 driver out of staging to mainline iio
> Move the files, modify the proper Kconfigs and the documentation.
>=20
> That'd be all. Any feedback is welcome. I hope this gets out of staging :)
>=20
> Cheers,
>=20
> Hern=E1n
>=20
> Hern=E1n Gonzalez (11):
>   staging: iio: ad7746: Adjust arguments to match open parenthesis
>   staging: iio: ad7746: Fix multiple line dereference
>   staging: iio: ad7746: Reorder includes alphabetically
>   staging: iio: ad7746: Reorder variable declarations
>   staging: iio: ad7746: Remove unused defines
>   staging: iio: ad7746: Add dt-bindings
>   staging: iio: ad7746: Add remove()
>   staging: iio: ad7746: Add comments
>   staging: iio: ad7746: Add devicetree bindings documentation
>   staging: iio: ad7746: Rename sysfs attrs to comply with the ABI
>   Move cdc ad7746 driver out of staging to mainline iio
>=20
>  .../devicetree/bindings/iio/cdc/ad7746.txt         |  32 ++++
>  drivers/iio/Kconfig                                |   1 +
>  drivers/iio/cdc/Kconfig                            |  16 ++
>  drivers/{staging =3D> }/iio/cdc/ad7746.c             | 168 +++++++++++++=
++------
>  drivers/staging/iio/cdc/Kconfig                    |  10 --
>  .../staging =3D> include/linux}/iio/cdc/ad7746.h     |   9 --
>  6 files changed, 168 insertions(+), 68 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/iio/cdc/ad7746.txt
>  create mode 100644 drivers/iio/cdc/Kconfig
>  rename drivers/{staging =3D> }/iio/cdc/ad7746.c (84%)
>  rename {drivers/staging =3D> include/linux}/iio/cdc/ad7746.h (70%)
>=20