[Patch 0/5] iio: adc: xilinx: Fix some minor issues

[Patch 0/5] iio: adc: xilinx: Fix some minor issues

[Thomas Betker]

This patch series fixes some issues found by tests on our Zynq-7020 
board; the issues usually result in incorrect voltage readings:

[1/5] iio: adc: xilinx: Fix register addresses
[2/5] iio: adc: xilinx: Fix "vccaux" channel .address
[3/5] iio: adc: xilinx: Fix VREFP scale
[4/5] iio: adc: xilinx: Fix VREFN sign
[5/5] iio: adc: xilinx: Set .datasheet_name instead of .extend_name

About [1]: The register addresses in question are currently not used by 
the driver, but we do use them for extended tests. (I might provide a 
patch to support lowest/highest values another time; see also [5].)

About [3], [4]: The transfer functions for VREFP and VREFN are 
	vrefp = unsigned(code) * 3.0 / 4096
	vrefn = signed(code) * 1.0 / 4096
I know it's not consistent, but that's what UG480 says ... VREFP = 1.25 
definitely needs the factor 3.0 to get reasonable values (a factor 1.0 
will never get you more than 1.0V). As for VREFN = 0V, we did actually 
see negative values on our boards.

About [5]: This may be a matter of debate on the IIO list, but I didn't 
get the impression that extensions are intended to be used that way.
    .extend_name = "vccint" means that the sysfs node names are 
in_voltage0_vccint_raw/_scale, which is a bit unexpected. I would 
rather see them called just in_voltage0_*, and reserve extensions for 
things like in_voltage0_lowest_* and in_voltage0_highest_* (MIN_VCCINT, 
MAX_VCCINT) -- that's what we do in our project.
    Note that .extend_name = "vccint", "vccaux", ... also has the side 
effect of creating the sysfs nodes vccint_sampling_frequency, 
vccaux_sampling_frequency, ... [due to .info_mask_shared_by_all = 
BIT(IIO_CHAN_INFO_SAMP_FREQ)], which is probably not desired.

Best regards,
Thomas Betker

[PATCH 1/5] iio: adc: xilinx: Fix register addresses

[Thomas Betker]

From: Thomas Betker <thomas.betker@rohde-schwarz.com>

Define the register addresses for MIN_VCCPINT, MIN_VCCPAUX, MIN_VCCO_DDR
correctly.

Signed-off-by: Thomas Betker <thomas.betker@rohde-schwarz.com>
---
 drivers/iio/adc/xilinx-xadc.h |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/xilinx-xadc.h b/drivers/iio/adc/xilinx-xadc.h
index c7487e8..54adc50 100644
--- a/drivers/iio/adc/xilinx-xadc.h
+++ b/drivers/iio/adc/xilinx-xadc.h
@@ -145,9 +145,9 @@ static inline int xadc_write_adc_reg(struct xadc *xadc, unsigned int reg,
 #define XADC_REG_MAX_VCCPINT	0x28
 #define XADC_REG_MAX_VCCPAUX	0x29
 #define XADC_REG_MAX_VCCO_DDR	0x2a
-#define XADC_REG_MIN_VCCPINT	0x2b
-#define XADC_REG_MIN_VCCPAUX	0x2c
-#define XADC_REG_MIN_VCCO_DDR	0x2d
+#define XADC_REG_MIN_VCCPINT	0x2c
+#define XADC_REG_MIN_VCCPAUX	0x2d
+#define XADC_REG_MIN_VCCO_DDR	0x2e
 
 #define XADC_REG_CONF0		0x40
 #define XADC_REG_CONF1		0x41
-- 
1.7.9.5

Re: [PATCH 1/5] iio: adc: xilinx: Fix register addresses

[Jonathan Cameron]

On 15/04/15 20:11, Thomas Betker wrote:
> From: Thomas Betker <thomas.betker@rohde-schwarz.com>
> 
> Define the register addresses for MIN_VCCPINT, MIN_VCCPAUX, MIN_VCCO_DDR
> correctly.
> 
> Signed-off-by: Thomas Betker <thomas.betker@rohde-schwarz.com>
Applied to the fixes-togreg branch of iio.git, marked for stable.

Thanks,

Jonathan
> ---
>  drivers/iio/adc/xilinx-xadc.h |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/xilinx-xadc.h b/drivers/iio/adc/xilinx-xadc.h
> index c7487e8..54adc50 100644
> --- a/drivers/iio/adc/xilinx-xadc.h
> +++ b/drivers/iio/adc/xilinx-xadc.h
> @@ -145,9 +145,9 @@ static inline int xadc_write_adc_reg(struct xadc *xadc, unsigned int reg,
>  #define XADC_REG_MAX_VCCPINT	0x28
>  #define XADC_REG_MAX_VCCPAUX	0x29
>  #define XADC_REG_MAX_VCCO_DDR	0x2a
> -#define XADC_REG_MIN_VCCPINT	0x2b
> -#define XADC_REG_MIN_VCCPAUX	0x2c
> -#define XADC_REG_MIN_VCCO_DDR	0x2d
> +#define XADC_REG_MIN_VCCPINT	0x2c
> +#define XADC_REG_MIN_VCCPAUX	0x2d
> +#define XADC_REG_MIN_VCCO_DDR	0x2e
>  
>  #define XADC_REG_CONF0		0x40
>  #define XADC_REG_CONF1		0x41
> 

[PATCH 2/5] iio: adc: xilinx: Fix "vccaux" channel .address

[Thomas Betker]

From: Thomas Betker <thomas.betker@rohde-schwarz.com>

For the "vccaux" channel, read the VCCAUX register, not VCCINT.

Signed-off-by: Thomas Betker <thomas.betker@rohde-schwarz.com>
---
 drivers/iio/adc/xilinx-xadc-core.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
index a221f73..0ad7b50 100644
--- a/drivers/iio/adc/xilinx-xadc-core.c
+++ b/drivers/iio/adc/xilinx-xadc-core.c
@@ -1008,7 +1008,7 @@ static const struct iio_event_spec xadc_voltage_events[] = {
 static const struct iio_chan_spec xadc_channels[] = {
 	XADC_CHAN_TEMP(0, 8, XADC_REG_TEMP),
 	XADC_CHAN_VOLTAGE(0, 9, XADC_REG_VCCINT, "vccint", true),
-	XADC_CHAN_VOLTAGE(1, 10, XADC_REG_VCCINT, "vccaux", true),
+	XADC_CHAN_VOLTAGE(1, 10, XADC_REG_VCCAUX, "vccaux", true),
 	XADC_CHAN_VOLTAGE(2, 14, XADC_REG_VCCBRAM, "vccbram", true),
 	XADC_CHAN_VOLTAGE(3, 5, XADC_REG_VCCPINT, "vccpint", true),
 	XADC_CHAN_VOLTAGE(4, 6, XADC_REG_VCCPAUX, "vccpaux", true),
-- 
1.7.9.5

Re: [PATCH 2/5] iio: adc: xilinx: Fix "vccaux" channel .address

[Jonathan Cameron]

On 15/04/15 20:11, Thomas Betker wrote:
> From: Thomas Betker <thomas.betker@rohde-schwarz.com>
> 
> For the "vccaux" channel, read the VCCAUX register, not VCCINT.
> 
> Signed-off-by: Thomas Betker <thomas.betker@rohde-schwarz.com>
Applied to the fixes-togreg branch of iio.git. Marked for stable
Thanks
> ---
>  drivers/iio/adc/xilinx-xadc-core.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
> index a221f73..0ad7b50 100644
> --- a/drivers/iio/adc/xilinx-xadc-core.c
> +++ b/drivers/iio/adc/xilinx-xadc-core.c
> @@ -1008,7 +1008,7 @@ static const struct iio_event_spec xadc_voltage_events[] = {
>  static const struct iio_chan_spec xadc_channels[] = {
>  	XADC_CHAN_TEMP(0, 8, XADC_REG_TEMP),
>  	XADC_CHAN_VOLTAGE(0, 9, XADC_REG_VCCINT, "vccint", true),
> -	XADC_CHAN_VOLTAGE(1, 10, XADC_REG_VCCINT, "vccaux", true),
> +	XADC_CHAN_VOLTAGE(1, 10, XADC_REG_VCCAUX, "vccaux", true),
>  	XADC_CHAN_VOLTAGE(2, 14, XADC_REG_VCCBRAM, "vccbram", true),
>  	XADC_CHAN_VOLTAGE(3, 5, XADC_REG_VCCPINT, "vccpint", true),
>  	XADC_CHAN_VOLTAGE(4, 6, XADC_REG_VCCPAUX, "vccpaux", true),
> 

[PATCH 3/5] iio: adc: xilinx: Fix VREFP scale

[Thomas Betker]

From: Thomas Betker <thomas.betker@rohde-schwarz.com>

The scaling factor for VREFP is 3.0/4096, not 1.0/4096; fix this to get
correct readings.

Signed-off-by: Thomas Betker <thomas.betker@rohde-schwarz.com>
---
 drivers/iio/adc/xilinx-xadc-core.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
index 0ad7b50..6fa629b 100644
--- a/drivers/iio/adc/xilinx-xadc-core.c
+++ b/drivers/iio/adc/xilinx-xadc-core.c
@@ -856,6 +856,7 @@ static int xadc_read_raw(struct iio_dev *indio_dev,
 			switch (chan->address) {
 			case XADC_REG_VCCINT:
 			case XADC_REG_VCCAUX:
+			case XADC_REG_VREFP:
 			case XADC_REG_VCCBRAM:
 			case XADC_REG_VCCPINT:
 			case XADC_REG_VCCPAUX:
-- 
1.7.9.5

Re: [PATCH 3/5] iio: adc: xilinx: Fix VREFP scale

[Jonathan Cameron]

On 15/04/15 20:11, Thomas Betker wrote:
> From: Thomas Betker <thomas.betker@rohde-schwarz.com>
> 
> The scaling factor for VREFP is 3.0/4096, not 1.0/4096; fix this to get
> correct readings.
> 
> Signed-off-by: Thomas Betker <thomas.betker@rohde-schwarz.com>
Applied to the fixes-togreg branch of iio.git.  Marked for stable.

Thanks,
> ---
>  drivers/iio/adc/xilinx-xadc-core.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
> index 0ad7b50..6fa629b 100644
> --- a/drivers/iio/adc/xilinx-xadc-core.c
> +++ b/drivers/iio/adc/xilinx-xadc-core.c
> @@ -856,6 +856,7 @@ static int xadc_read_raw(struct iio_dev *indio_dev,
>  			switch (chan->address) {
>  			case XADC_REG_VCCINT:
>  			case XADC_REG_VCCAUX:
> +			case XADC_REG_VREFP:
>  			case XADC_REG_VCCBRAM:
>  			case XADC_REG_VCCPINT:
>  			case XADC_REG_VCCPAUX:
> 

Re: [PATCH 3/5] iio: adc: xilinx: Fix VREFP scale

[Hartmut Knaack]

Thomas Betker schrieb am 15.04.2015 um 21:11:
> From: Thomas Betker <thomas.betker@rohde-schwarz.com>
> 
> The scaling factor for VREFP is 3.0/4096, not 1.0/4096; fix this to get
> correct readings.
> 

Hi Thomas,
I just had a look at the datasheet [1], table 3-1 on page 37, and get the feeling
that this scaling factor also applies to the VREFN channel. What is your opinion?

Thanks,
Hartmut

[1] http://www.xilinx.com/support/documentation/user_guides/ug480_7Series_XADC.pdf

> Signed-off-by: Thomas Betker <thomas.betker@rohde-schwarz.com>
> ---
>  drivers/iio/adc/xilinx-xadc-core.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
> index 0ad7b50..6fa629b 100644
> --- a/drivers/iio/adc/xilinx-xadc-core.c
> +++ b/drivers/iio/adc/xilinx-xadc-core.c
> @@ -856,6 +856,7 @@ static int xadc_read_raw(struct iio_dev *indio_dev,
>  			switch (chan->address) {
>  			case XADC_REG_VCCINT:
>  			case XADC_REG_VCCAUX:
> +			case XADC_REG_VREFP:
>  			case XADC_REG_VCCBRAM:
>  			case XADC_REG_VCCPINT:
>  			case XADC_REG_VCCPAUX:
> 

Re: [PATCH 3/5] iio: adc: xilinx: Fix VREFP scale

[Thomas.Betker]

Hello Hartmut:

> > The scaling factor for VREFP is 3.0/4096, not 1.0/4096; fix this to 
get
> > correct readings.
> 
> I just had a look at the datasheet [1], table 3-1 on page 37, and 
> get the feeling
> that this scaling factor also applies to the VREFN channel. What is 
> your opinion?
> 
> [1] http://www.xilinx.com/support/documentation/user_guides/
> ug480_7Series_XADC.pdf

The table provides the following references:
- VREFP: Figure 2-10, transfer function 3*unsigned(code)/4096
- VREFN: Figure 2-3, transfer function 1*unsigned(code)/4096

The table also says about VREFN: "The supply sensor is also used to 
measure VREFN, thus 1 LSB = 3V/4096." I agree that this is not consistent 
(by Figure 2-3, 1 LSB = 1V/4096), but I didn't have the means to check 
which version is true as VREFN is close to 0V either way.

The situation is simpler for VREFP as UG480 is unambiguous here, and by 
measuring the actual voltage we can tell 1.25V and 0.41V easily apart, 
even when taking some measurement errors into account. In other words, we 
have to use the factor 3 to get correct results for VREFP (thus the 
patch), while either factor returns plausible results for VREFN.

I would suggest opening a webcase with Xilinx for clarification, and 
adding another patch for VREFN if the factor 3 is confirmed. Can you do 
this? If not, I can take care of it, but it may take a while as I am 
pretty busy at the moment.

Best regards,
Thomas

Re: [PATCH 3/5] iio: adc: xilinx: Fix VREFP scale

[Hartmut Knaack]

Thomas.Betker@rohde-schwarz.com schrieb am 30.06.2015 um 12:56:
> Hello Hartmut:
> 
>>> The scaling factor for VREFP is 3.0/4096, not 1.0/4096; fix this to 
> get
>>> correct readings.
>>
>> I just had a look at the datasheet [1], table 3-1 on page 37, and 
>> get the feeling
>> that this scaling factor also applies to the VREFN channel. What is 
>> your opinion?
>>
>> [1] http://www.xilinx.com/support/documentation/user_guides/
>> ug480_7Series_XADC.pdf
> 
> The table provides the following references:
> - VREFP: Figure 2-10, transfer function 3*unsigned(code)/4096
> - VREFN: Figure 2-3, transfer function 1*unsigned(code)/4096
> 
> The table also says about VREFN: "The supply sensor is also used to 
> measure VREFN, thus 1 LSB = 3V/4096." I agree that this is not consistent 
> (by Figure 2-3, 1 LSB = 1V/4096), but I didn't have the means to check 
> which version is true as VREFN is close to 0V either way.
> 
> The situation is simpler for VREFP as UG480 is unambiguous here, and by 
> measuring the actual voltage we can tell 1.25V and 0.41V easily apart, 
> even when taking some measurement errors into account. In other words, we 
> have to use the factor 3 to get correct results for VREFP (thus the 
> patch), while either factor returns plausible results for VREFN.
> 
> I would suggest opening a webcase with Xilinx for clarification, and 
> adding another patch for VREFN if the factor 3 is confirmed. Can you do 
> this? If not, I can take care of it, but it may take a while as I am 
> pretty busy at the moment.
> 

Go ahead, whenever you find the time. I don't have a Xilinx device, just
reviewing the IIO patches and got a bit curious when reading through the
data sheet.

> Best regards,
> Thomas
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 3/5] iio: adc: xilinx: Fix VREFP scale

[Thomas.Betker]

Hello Hartmut:

> > I would suggest opening a webcase with Xilinx for clarification, and 
> > adding another patch for VREFN if the factor 3 is confirmed. Can you 
do 
> > this? If not, I can take care of it, but it may take a while as I am 
> > pretty busy at the moment.
> > 
> 
> Go ahead, whenever you find the time. I don't have a Xilinx device, just
> reviewing the IIO patches and got a bit curious when reading through the
> data sheet.

Okay. I have opened a service request with Xilinx; let's see what happens 
... (:-)

Best regards,
Thomas

[PATCH 4/5] iio: adc: xilinx: Fix VREFN sign

[Thomas Betker]

From: Thomas Betker <thomas.betker@rohde-schwarz.com>

The VREFN channel is bipolar, not unipolar. Small negative values do
occur (e.g., -1mV), and unsigned conversion maps them incorrectly to
large positive values (about +1V), so fix this.

Signed-off-by: Thomas Betker <thomas.betker@rohde-schwarz.com>
---
 drivers/iio/adc/xilinx-xadc-core.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
index 6fa629b..ce93bd8 100644
--- a/drivers/iio/adc/xilinx-xadc-core.c
+++ b/drivers/iio/adc/xilinx-xadc-core.c
@@ -997,7 +997,7 @@ static const struct iio_event_spec xadc_voltage_events[] = {
 	.num_event_specs = (_alarm) ? ARRAY_SIZE(xadc_voltage_events) : 0, \
 	.scan_index = (_scan_index), \
 	.scan_type = { \
-		.sign = 'u', \
+		.sign = ((_addr) == XADC_REG_VREFN) ? 's' : 'u', \
 		.realbits = 12, \
 		.storagebits = 16, \
 		.shift = 4, \
-- 
1.7.9.5

Re: [PATCH 4/5] iio: adc: xilinx: Fix VREFN sign

[Jonathan Cameron]

On 15/04/15 20:11, Thomas Betker wrote:
> From: Thomas Betker <thomas.betker@rohde-schwarz.com>
> 
> The VREFN channel is bipolar, not unipolar. Small negative values do
> occur (e.g., -1mV), and unsigned conversion maps them incorrectly to
> large positive values (about +1V), so fix this.
> 
> Signed-off-by: Thomas Betker <thomas.betker@rohde-schwarz.com>
Applied to the fixes-togreg branch of iio.git. Marked for stable.

Thanks,

Jonathan
> ---
>  drivers/iio/adc/xilinx-xadc-core.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
> index 6fa629b..ce93bd8 100644
> --- a/drivers/iio/adc/xilinx-xadc-core.c
> +++ b/drivers/iio/adc/xilinx-xadc-core.c
> @@ -997,7 +997,7 @@ static const struct iio_event_spec xadc_voltage_events[] = {
>  	.num_event_specs = (_alarm) ? ARRAY_SIZE(xadc_voltage_events) : 0, \
>  	.scan_index = (_scan_index), \
>  	.scan_type = { \
> -		.sign = 'u', \
> +		.sign = ((_addr) == XADC_REG_VREFN) ? 's' : 'u', \
>  		.realbits = 12, \
>  		.storagebits = 16, \
>  		.shift = 4, \
> 

[PATCH 5/5] iio: adc: xilinx: Set .datasheet_name instead of .extend_name

[Thomas Betker]

From: Thomas Betker <thomas.betker@rohde-schwarz.com>

"vccint", "vccaux", "vccbram" etc. are the datasheet names of the
XADC channels, not extensions; make them so.

Signed-off-by: Thomas Betker <thomas.betker@rohde-schwarz.com>
---
 drivers/iio/adc/xilinx-xadc-core.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
index ce93bd8..69db509 100644
--- a/drivers/iio/adc/xilinx-xadc-core.c
+++ b/drivers/iio/adc/xilinx-xadc-core.c
@@ -985,7 +985,7 @@ static const struct iio_event_spec xadc_voltage_events[] = {
 	}, \
 }
 
-#define XADC_CHAN_VOLTAGE(_chan, _scan_index, _addr, _ext, _alarm) { \
+#define XADC_CHAN_VOLTAGE(_chan, _scan_index, _addr, _name, _alarm) { \
 	.type = IIO_VOLTAGE, \
 	.indexed = 1, \
 	.channel = (_chan), \
@@ -1003,7 +1003,7 @@ static const struct iio_event_spec xadc_voltage_events[] = {
 		.shift = 4, \
 		.endianness = IIO_CPU, \
 	}, \
-	.extend_name = _ext, \
+	.datasheet_name = _name, \
 }
 
 static const struct iio_chan_spec xadc_channels[] = {
-- 
1.7.9.5

Re: [Patch 0/5] iio: adc: xilinx: Fix some minor issues

[Jonathan Cameron]

On 15/04/15 20:11, Thomas Betker wrote:
> This patch series fixes some issues found by tests on our Zynq-7020 
> board; the issues usually result in incorrect voltage readings:
> 
> [1/5] iio: adc: xilinx: Fix register addresses
> [2/5] iio: adc: xilinx: Fix "vccaux" channel .address
> [3/5] iio: adc: xilinx: Fix VREFP scale
> [4/5] iio: adc: xilinx: Fix VREFN sign
> [5/5] iio: adc: xilinx: Set .datasheet_name instead of .extend_name
> 
> About [1]: The register addresses in question are currently not used by 
> the driver, but we do use them for extended tests. (I might provide a 
> patch to support lowest/highest values another time; see also [5].)
> 
> About [3], [4]: The transfer functions for VREFP and VREFN are 
> 	vrefp = unsigned(code) * 3.0 / 4096
> 	vrefn = signed(code) * 1.0 / 4096
> I know it's not consistent, but that's what UG480 says ... VREFP = 1.25 
> definitely needs the factor 3.0 to get reasonable values (a factor 1.0 
> will never get you more than 1.0V). As for VREFN = 0V, we did actually 
> see negative values on our boards.
> 
> About [5]: This may be a matter of debate on the IIO list, but I didn't 
> get the impression that extensions are intended to be used that way.
>     .extend_name = "vccint" means that the sysfs node names are 
> in_voltage0_vccint_raw/_scale, which is a bit unexpected. I would 
> rather see them called just in_voltage0_*, and reserve extensions for 
> things like in_voltage0_lowest_* and in_voltage0_highest_* (MIN_VCCINT, 
> MAX_VCCINT) -- that's what we do in our project.
It was always intended for labelling of channels with well defined
purposes.  That is ones that are hard wired to something rather than
simply exposed for general purpose use.  I'm guessing vccint is
the internal supply voltage, in which case that was pretty much
the intended use.

The datasheet name is only really there for binding consumers to individual
channels.  The thought being that you'd probably be sat there with a circuit
diagram in front of you specifying what is connected to which precise pin
of the ADC...

So I think the original approach is more in keeping with the intent.

The lowest/highest versions are interesting. Right now, the extended name
is the only way to specify them, but somehow it feels like we ought
to have something better.... We could treat them as a filter or simply
another aspect of the channel (like _scale etc), but that's also a little
ugly. Will think about this and see if anyone else has a better suggestion!

>     Note that .extend_name = "vccint", "vccaux", ... also has the side 
> effect of creating the sysfs nodes vccint_sampling_frequency, 
> vccaux_sampling_frequency, ... [due to .info_mask_shared_by_all = 
> BIT(IIO_CHAN_INFO_SAMP_FREQ)], which is probably not desired.
This last one sounds like a bug and definitely isn't the intended result!
The extended name should be ignored for shared_by values.
> 
> Best regards,
> Thomas Betker
> 

Re: [Patch 0/5] iio: adc: xilinx: Fix some minor issues

[Lars-Peter Clausen]

On 04/15/2015 10:54 PM, Jonathan Cameron wrote:
[...]
>>      Note that .extend_name = "vccint", "vccaux", ... also has the side
>> effect of creating the sysfs nodes vccint_sampling_frequency,
>> vccaux_sampling_frequency, ... [due to .info_mask_shared_by_all =
>> BIT(IIO_CHAN_INFO_SAMP_FREQ)], which is probably not desired.
> This last one sounds like a bug and definitely isn't the intended result!
> The extended name should be ignored for shared_by values.

It was fixed a while ago, see 
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=77bfa8baa0c230eb3d8acccd7d341f406a32cdf4

Re: [Patch 0/5] iio: adc: xilinx: Fix some minor issues

[Thomas.Betker]

Hello Lars-Peter:

> >>      Note that .extend_name = "vccint", "vccaux", ... also has the 
side
> >> effect of creating the sysfs nodes vccint_sampling_frequency,
> >> vccaux_sampling_frequency, ... [due to .info_mask_shared_by_all =
> >> BIT(IIO_CHAN_INFO_SAMP_FREQ)], which is probably not desired.
> > This last one sounds like a bug and definitely isn't the intended 
result!
> > The extended name should be ignored for shared_by values.
> 
> It was fixed a while ago, see 
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/
> commit/?id=77bfa8baa0c230eb3d8acccd7d341f406a32cdf4

You're right; my fault. I am using 3.14 (longterm), which doesn't contain 
this fix. And although I did read the mainline code for reference, I 
didn't check for this particular fix. Sorry.

Best regards,
Thomas Betker

Re: [Patch 0/5] iio: adc: xilinx: Fix some minor issues

[Thomas.Betker]

Hello Jonathan:

> > About [5]: This may be a matter of debate on the IIO list, but I 
didn't 
> > get the impression that extensions are intended to be used that way.
> >     .extend_name = "vccint" means that the sysfs node names are 
> > in_voltage0_vccint_raw/_scale, which is a bit unexpected. I would 
> > rather see them called just in_voltage0_*, and reserve extensions for 
> > things like in_voltage0_lowest_* and in_voltage0_highest_* 
(MIN_VCCINT, 
> > MAX_VCCINT) -- that's what we do in our project.
> 
> It was always intended for labelling of channels with well defined
> purposes.  That is ones that are hard wired to something rather than
> simply exposed for general purpose use.  I'm guessing vccint is
> the internal supply voltage, in which case that was pretty much
> the intended use.
> 
> The datasheet name is only really there for binding consumers to 
individual
> channels.  The thought being that you'd probably be sat there with a 
circuit
> diagram in front of you specifying what is connected to which precise 
pin
> of the ADC...
> 
> So I think the original approach is more in keeping with the intent.

Okay, got it. It means that our application needs to remember that it's 
sometimes voltage%d_%s instead of just voltage%d, but we can do that.

> The lowest/highest versions are interesting. Right now, the extended 
name
> is the only way to specify them, but somehow it feels like we ought
> to have something better.... We could treat them as a filter or simply
> another aspect of the channel (like _scale etc), but that's also a 
little
> ugly. Will think about this and see if anyone else has a better 
suggestion!

With hwmon, we would have inX_input, inX_lowest, inX_highest (in fact, we 
had, as Xilinx XADC used to be hwmon -- just to explain where I'm coming 
from). So it made sense to me to use .extended_name = "lowest" to have 
in_voltageX_lowest_raw/_scale in addition to in_voltageX_raw/_scale (same 
index X). I understand that I may have read too much into the IIO code 
here, though, and if there's a better way to do this, I will gladly use 
it.

Best regards,
Thomas Betker

Re: [Patch 0/5] iio: adc: xilinx: Fix some minor issues

[Jonathan Cameron]

On 16/04/15 08:53, Thomas.Betker@rohde-schwarz.com wrote:
> Hello Jonathan:
> 
>>> About [5]: This may be a matter of debate on the IIO list, but I 
> didn't 
>>> get the impression that extensions are intended to be used that way.
>>>     .extend_name = "vccint" means that the sysfs node names are 
>>> in_voltage0_vccint_raw/_scale, which is a bit unexpected. I would 
>>> rather see them called just in_voltage0_*, and reserve extensions for 
>>> things like in_voltage0_lowest_* and in_voltage0_highest_* 
> (MIN_VCCINT, 
>>> MAX_VCCINT) -- that's what we do in our project.
>>
>> It was always intended for labelling of channels with well defined
>> purposes.  That is ones that are hard wired to something rather than
>> simply exposed for general purpose use.  I'm guessing vccint is
>> the internal supply voltage, in which case that was pretty much
>> the intended use.
>>
>> The datasheet name is only really there for binding consumers to 
> individual
>> channels.  The thought being that you'd probably be sat there with a 
> circuit
>> diagram in front of you specifying what is connected to which precise 
> pin
>> of the ADC...
>>
>> So I think the original approach is more in keeping with the intent.
> 
> Okay, got it. It means that our application needs to remember that it's 
> sometimes voltage%d_%s instead of just voltage%d, but we can do that.
> 
>> The lowest/highest versions are interesting. Right now, the extended 
> name
>> is the only way to specify them, but somehow it feels like we ought
>> to have something better.... We could treat them as a filter or simply
>> another aspect of the channel (like _scale etc), but that's also a 
> little
>> ugly. Will think about this and see if anyone else has a better 
> suggestion!
> 
> With hwmon, we would have inX_input, inX_lowest, inX_highest (in fact, we 
> had, as Xilinx XADC used to be hwmon -- just to explain where I'm coming 
> from). So it made sense to me to use .extended_name = "lowest" to have 
> in_voltageX_lowest_raw/_scale in addition to in_voltageX_raw/_scale (same 
> index X). I understand that I may have read too much into the IIO code 
> here, though, and if there's a better way to do this, I will gladly use 
> it.
Hmm, we could as new info_mask elements (like _raw, _scale etc).
The nasty corner case is that we might also read them as part of a buffer
read.  To do that they will need to be separate 'channels' in their own
right. 

I'm not entirely sure right now if you can distinguish channels purely on
the basis of extended name (and different scan index values), but that
might work, though would be a bit ugly.  There is certainly no fundamental
reason you can't do this..

Hmm. Lars, any thoughts?


> 
> Best regards,
> Thomas Betker
> 

Re: [Patch 0/5] iio: adc: xilinx: Fix some minor issues

[Thomas.Betker]

Hello Jonathan:

> > With hwmon, we would have inX_input, inX_lowest, inX_highest (in fact, 
we 
> > had, as Xilinx XADC used to be hwmon -- just to explain where I'm 
coming 
> > from). So it made sense to me to use .extended_name = "lowest" to have 

> > in_voltageX_lowest_raw/_scale in addition to in_voltageX_raw/_scale 
(same 
> > index X). I understand that I may have read too much into the IIO code 

> > here, though, and if there's a better way to do this, I will gladly 
use 
> > it.
> Hmm, we could as new info_mask elements (like _raw, _scale etc).
> The nasty corner case is that we might also read them as part of a 
buffer
> read.  To do that they will need to be separate 'channels' in their own
> right. 
> 
> I'm not entirely sure right now if you can distinguish channels purely 
on
> the basis of extended name (and different scan index values), but that
> might work, though would be a bit ugly.  There is certainly no 
fundamental
> reason you can't do this..

Oh my, it seems I have been a bit unclear here. In my implementation, 
lowest/highest *are* actually separate channels, with their own _raw and 
_scale. The Xilinx XADC controller has separate registers such as 
MIN_VCCINT and MAX_VCCINT in addition to VCCINT, and it was fairly 
straightforward to extend the existing channel set this way.

So lowest/highest is orthogonal to raw/scale (which made it tempting to 
use .extend_name). lowest/highest use the same .channel index as the 
standard channel (so we have a group in_temp0_raw/_scale, 
in_temp0_lowest_raw/_scale, in_temp0_highest_raw/_scale), but their own 
.scan_index (as that has to be unique).

Perhaps we should just use .extend_name = "vccint", "vccint_lowest", 
"vccint_highest"? (And NULL, "lowest", "highest" for channels which do not 
have a hard-wired purpose.)

Best regards,
Thomas Betker

Re: [Patch 0/5] iio: adc: xilinx: Fix some minor issues

[Jonathan Cameron]

On 20 April 2015 09:34:10 BST, Thomas.Betker@rohde-schwarz.com wrote:
>Hello Jonathan:
>
>> > With hwmon, we would have inX_input, inX_lowest, inX_highest (in
>fact, 
>we 
>> > had, as Xilinx XADC used to be hwmon -- just to explain where I'm 
>coming 
>> > from). So it made sense to me to use .extended_name = "lowest" to
>have 
>
>> > in_voltageX_lowest_raw/_scale in addition to in_voltageX_raw/_scale
>
>(same 
>> > index X). I understand that I may have read too much into the IIO
>code 
>
>> > here, though, and if there's a better way to do this, I will gladly
>
>use 
>> > it.
>> Hmm, we could as new info_mask elements (like _raw, _scale etc).
>> The nasty corner case is that we might also read them as part of a 
>buffer
>> read.  To do that they will need to be separate 'channels' in their
>own
>> right. 
>> 
>> I'm not entirely sure right now if you can distinguish channels
>purely 
>on
>> the basis of extended name (and different scan index values), but
>that
>> might work, though would be a bit ugly.  There is certainly no 
>fundamental
>> reason you can't do this..
>
>Oh my, it seems I have been a bit unclear here. In my implementation, 
>lowest/highest *are* actually separate channels, with their own _raw
>and 
>_scale. The Xilinx XADC controller has separate registers such as 
>MIN_VCCINT and MAX_VCCINT in addition to VCCINT, and it was fairly 
>straightforward to extend the existing channel set this way.
>
>So lowest/highest is orthogonal to raw/scale (which made it tempting to
>
>use .extend_name). lowest/highest use the same .channel index as the 
>standard channel (so we have a group in_temp0_raw/_scale, 
>in_temp0_lowest_raw/_scale, in_temp0_highest_raw/_scale), but their own
>
>.scan_index (as that has to be unique).
>
>Perhaps we should just use .extend_name = "vccint", "vccint_lowest", 
>"vccint_highest"? (And NULL, "lowest", "highest" for channels which do
>not 
>have a hard-wired purpose.)
Yes. That will do nicely. 
>
>Best regards,
>Thomas Betker

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.