ARM: zynq: Add #io-channel-cells to (x)adc node for iio-hwmon

PATCH In review Moritz Fischer 4 participants 13 messages Mar 8, 2017 – Apr 7, 2017

Subsystems (2)

Device tree bindings Arm/Zynq architecture

lore

Share & tools

lore

Moritz Fischer

author

2017-03-08 20:11 UTC #0 lore raw

PATCH v2 1/1 ARM: zynq: Add #io-channel-cells to (x)adc node for iio-hwmon

Fix

OF: /iio_hwmon: could not get #io-channel-cells for /amba/adc at f8007100
OF: /iio_hwmon: could not get #io-channel-cells for /amba/adc at f8007100
OF: /iio_hwmon: could not get #io-channel-cells for /amba/adc at f8007100

by adding the #io-channel-cells property.

Signed-off-by Moritz Fischer <[email protected]>

Cc Michal Simek <[email protected]>

Cc S?ren Brinkmann <[email protected]>

Cc Julia Cartwright <[email protected]>

Cc linux-kernel at vger.kernel.org

Cc devicetree at vger.kernel.org

---

Changes from v1:
- fix messed up commit message

---
 arch/arm/boot/dts/zynq-7000.dtsi | 1 +
 1 file changed, 1 insertion(+)

arch/arm/boot/dts/zynq-7000.dtsi +1 -0

index f3ac9bf..98233a8 100644
@@ -72,6 +72,7 @@
 			interrupts = <0 7 4>;
 			interrupt-parent = <&intc>;
 			clocks = <&clkc 12>;
+			#io-channel-cells = <1>;
 		};
 
 		can0: can at e0008000 {

Signature · 1 line

2.7.4

Michal Simek

2017-03-16 16:16 UTC #1 lore raw

Hi,

Quoted 31 lines · Moritz Fischer

Fix
OF: /iio_hwmon: could not get #io-channel-cells for /amba/adc at f8007100
OF: /iio_hwmon: could not get #io-channel-cells for /amba/adc at f8007100
OF: /iio_hwmon: could not get #io-channel-cells for /amba/adc at f8007100
by adding the #io-channel-cells property.
Signed-off-by Moritz Fischer <[email protected]>
Cc Michal Simek <[email protected]>
Cc S?ren Brinkmann <[email protected]>
Cc Julia Cartwright <[email protected]>
Cc linux-kernel at vger.kernel.org
Cc devicetree at vger.kernel.org
---
Changes from v1:
- fix messed up commit message
---
 arch/arm/boot/dts/zynq-7000.dtsi | 1 +
 1 file changed, 1 insertion(+)
arch/arm/boot/dts/zynq-7000.dtsi +0 -0
index f3ac9bf..98233a8 100644

arch/arm/boot/dts/zynq-7000.dtsi line 78

Quoted 7 lines

arch/arm/boot/dts/zynq-7000.dtsi +1 -0

@@ -72,6 +72,7 @@
 			interrupts = <0 7 4>;
 			interrupt-parent = <&intc>;
 			clocks = <&clkc 12>;
+			#io-channel-cells = <1>;
 		};
 
 		can0: can at e0008000 {

I think it will be good to the next step too.
It means also add iio-hwmon node too.

What do you think?

Thanks,
Michal

Moritz Fischer

2017-03-16 16:39 UTC #2 lore raw

Quoted 39 lines · Michal Simek

Hi,
Quoted 36 lines · Moritz Fischer
Fix
OF: /iio_hwmon: could not get #io-channel-cells for /amba/adc at f8007100
OF: /iio_hwmon: could not get #io-channel-cells for /amba/adc at f8007100
OF: /iio_hwmon: could not get #io-channel-cells for /amba/adc at f8007100
by adding the #io-channel-cells property.
Signed-off-by Moritz Fischer <[email protected]>
Cc Michal Simek <[email protected]>
Cc S?ren Brinkmann <[email protected]>
Cc Julia Cartwright <[email protected]>
Cc linux-kernel at vger.kernel.org
Cc devicetree at vger.kernel.org
---
Changes from v1:
- fix messed up commit message
---
 arch/arm/boot/dts/zynq-7000.dtsi | 1 +
 1 file changed, 1 insertion(+)
arch/arm/boot/dts/zynq-7000.dtsi +1 -0
index f3ac9bf..98233a8 100644
@@ -72,6 +72,7 @@
                      interrupts = <0 7 4>;
                      interrupt-parent = <&intc>;
                      clocks = <&clkc 12>;
+                     #io-channel-cells = <1>;
              };
 

Quoted 7 lines

arch/arm/boot/dts/zynq-7000.dtsi line 78
Quoted 2 lines
arch/arm/boot/dts/zynq-7000.dtsi +0 -0
              can0: can at e0008000 {
I think it will be good to the next step too.
It means also add iio-hwmon node too.
What do you think?

I hadn't put it in there since dts is supposed to describe hw, but obviously putting the actual hwmon in there makes it more useful.

I can resubmit with the hwmon node in there.

Cheers,
Moritz

Michal Simek

2017-03-16 16:45 UTC #3 lore raw

Quoted 46 lines · Moritz Fischer

Quoted 45 lines · Michal Simek
Hi,
Quoted 31 lines · Moritz Fischer
Fix
OF: /iio_hwmon: could not get #io-channel-cells for /amba/adc at f8007100
OF: /iio_hwmon: could not get #io-channel-cells for /amba/adc at f8007100
OF: /iio_hwmon: could not get #io-channel-cells for /amba/adc at f8007100
by adding the #io-channel-cells property.
Signed-off-by Moritz Fischer <[email protected]>
Cc Michal Simek <[email protected]>
Cc S?ren Brinkmann <[email protected]>
Cc Julia Cartwright <[email protected]>
Cc linux-kernel at vger.kernel.org
Cc devicetree at vger.kernel.org
---
Changes from v1:
- fix messed up commit message
---
 arch/arm/boot/dts/zynq-7000.dtsi | 1 +
 1 file changed, 1 insertion(+)
arch/arm/boot/dts/zynq-7000.dtsi +0 -0
index f3ac9bf..98233a8 100644
arch/arm/boot/dts/zynq-7000.dtsi line 78
Quoted 7 lines
arch/arm/boot/dts/zynq-7000.dtsi +1 -0
@@ -72,6 +72,7 @@
                      interrupts = <0 7 4>;
                      interrupt-parent = <&intc>;
                      clocks = <&clkc 12>;
+                     #io-channel-cells = <1>;
              };
 
              can0: can at e0008000 {
I think it will be good to the next step too.
It means also add iio-hwmon node too.

Quoted 4 lines

Quoted 1 line
What do you think?
I hadn't put it in there since dts is supposed to describe hw, but obviously putting the actual hwmon in there makes it more useful.

I had one discussion about this with Grant in past and it is common mistake. It is simplification of purpose of dts.

Quoted 2 lines

I can resubmit with the hwmon node in there.

If you grep kernel tree you will see that others are using it too. Also there is accepted binding for that that's why I can't see big problem with it.

Thanks,
Michal

Lars-Peter Clausen

2017-03-16 16:51 UTC #4 lore raw

Quoted 47 lines · Michal Simek

Quoted 46 lines · Moritz Fischer
Quoted 45 lines · Michal Simek
Hi,
Quoted 31 lines · Moritz Fischer
Fix
OF: /iio_hwmon: could not get #io-channel-cells for /amba/adc at f8007100
OF: /iio_hwmon: could not get #io-channel-cells for /amba/adc at f8007100
OF: /iio_hwmon: could not get #io-channel-cells for /amba/adc at f8007100
by adding the #io-channel-cells property.
Signed-off-by Moritz Fischer <[email protected]>
Cc Michal Simek <[email protected]>
Cc S?ren Brinkmann <[email protected]>
Cc Julia Cartwright <[email protected]>
Cc linux-kernel at vger.kernel.org
Cc devicetree at vger.kernel.org
---
Changes from v1:
- fix messed up commit message
---
 arch/arm/boot/dts/zynq-7000.dtsi | 1 +
 1 file changed, 1 insertion(+)
arch/arm/boot/dts/zynq-7000.dtsi +0 -0
index f3ac9bf..98233a8 100644
arch/arm/boot/dts/zynq-7000.dtsi line 78
Quoted 7 lines
arch/arm/boot/dts/zynq-7000.dtsi +1 -0
@@ -72,6 +72,7 @@
                      interrupts = <0 7 4>;
                      interrupt-parent = <&intc>;
                      clocks = <&clkc 12>;
+                     #io-channel-cells = <1>;
              };
 
              can0: can at e0008000 {
I think it will be good to the next step too.
It means also add iio-hwmon node too.

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
What do you think?
I hadn't put it in there since dts is supposed to describe hw, but obviously putting the actual hwmon in there makes it more useful.
I had one discussion about this with Grant in past and it is common mistake. It is simplification of purpose of dts.

If the iio-hwmon binding had gone through review it would have been rejected.

Quoted 6 lines

Quoted 2 lines
I can resubmit with the hwmon node in there.
If you grep kernel tree you will see that others are using it too. Also there is accepted binding for that that's why I can't see big problem with it.

Since this is an application specific binding I wouldn't put it in the generic DT include file. It's a bit like adding a gpio-key binding for each of the GPIOs just in case somebody wants to use it.

Moritz Fischer

author

2017-03-16 17:54 UTC #5 lore raw

Hi Lars,

Quoted 62 lines · Lars-Peter Clausen

Quoted 47 lines · Michal Simek
Quoted 46 lines · Moritz Fischer
Quoted 45 lines · Michal Simek
Hi,
Quoted 31 lines · Moritz Fischer
Fix
OF: /iio_hwmon: could not get #io-channel-cells for /amba/adc at f8007100
OF: /iio_hwmon: could not get #io-channel-cells for /amba/adc at f8007100
OF: /iio_hwmon: could not get #io-channel-cells for /amba/adc at f8007100
by adding the #io-channel-cells property.
Signed-off-by Moritz Fischer <[email protected]>
Cc Michal Simek <[email protected]>
Cc S?ren Brinkmann <[email protected]>
Cc Julia Cartwright <[email protected]>
Cc linux-kernel at vger.kernel.org
Cc devicetree at vger.kernel.org
---
Changes from v1:
- fix messed up commit message
---
 arch/arm/boot/dts/zynq-7000.dtsi | 1 +
 1 file changed, 1 insertion(+)
arch/arm/boot/dts/zynq-7000.dtsi +0 -0
index f3ac9bf..98233a8 100644
arch/arm/boot/dts/zynq-7000.dtsi line 78
Quoted 7 lines
arch/arm/boot/dts/zynq-7000.dtsi +1 -0
@@ -72,6 +72,7 @@
                      interrupts = <0 7 4>;
                      interrupt-parent = <&intc>;
                      clocks = <&clkc 12>;
+                     #io-channel-cells = <1>;
              };
 
              can0: can at e0008000 {
I think it will be good to the next step too.
It means also add iio-hwmon node too.
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
What do you think?
I hadn't put it in there since dts is supposed to describe hw, but obviously putting the actual hwmon in there makes it more useful.
I had one discussion about this with Grant in past and it is common mistake. It is simplification of purpose of dts.
If the iio-hwmon binding had gone through review it would have been rejected.
Quoted 3 lines
Quoted 2 lines
I can resubmit with the hwmon node in there.

Quoted 7 lines

Quoted 3 lines
If you grep kernel tree you will see that others are using it too. Also there is accepted binding for that that's why I can't see big problem with it.
Since this is an application specific binding I wouldn't put it in the generic DT include file. It's a bit like adding a gpio-key binding for each of the GPIOs just in case somebody wants to use it.

So is your suggestion to move the whole thing from .dtsi -> .dts?

Thanks,

Moritz

Lars-Peter Clausen

2017-03-16 21:23 UTC #6 lore raw

Quoted 69 lines · Moritz Fischer

Hi Lars,
Quoted 66 lines · Lars-Peter Clausen
Quoted 47 lines · Michal Simek
Quoted 46 lines · Moritz Fischer
Quoted 45 lines · Michal Simek
Hi,
Quoted 31 lines · Moritz Fischer
Fix
OF: /iio_hwmon: could not get #io-channel-cells for /amba/adc at f8007100
OF: /iio_hwmon: could not get #io-channel-cells for /amba/adc at f8007100
OF: /iio_hwmon: could not get #io-channel-cells for /amba/adc at f8007100
by adding the #io-channel-cells property.
Signed-off-by Moritz Fischer <[email protected]>
Cc Michal Simek <[email protected]>
Cc S?ren Brinkmann <[email protected]>
Cc Julia Cartwright <[email protected]>
Cc linux-kernel at vger.kernel.org
Cc devicetree at vger.kernel.org
---
Changes from v1:
- fix messed up commit message
---
 arch/arm/boot/dts/zynq-7000.dtsi | 1 +
 1 file changed, 1 insertion(+)
arch/arm/boot/dts/zynq-7000.dtsi +0 -0
index f3ac9bf..98233a8 100644
arch/arm/boot/dts/zynq-7000.dtsi line 78
Quoted 7 lines
arch/arm/boot/dts/zynq-7000.dtsi +1 -0
@@ -72,6 +72,7 @@
                      interrupts = <0 7 4>;
                      interrupt-parent = <&intc>;
                      clocks = <&clkc 12>;
+                     #io-channel-cells = <1>;
              };
 
              can0: can at e0008000 {
I think it will be good to the next step too.
It means also add iio-hwmon node too.
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
What do you think?
I hadn't put it in there since dts is supposed to describe hw, but obviously putting the actual hwmon in there makes it more useful.
I had one discussion about this with Grant in past and it is common mistake. It is simplification of purpose of dts.
If the iio-hwmon binding had gone through review it would have been rejected.
Quoted 6 lines
Quoted 2 lines
I can resubmit with the hwmon node in there.
If you grep kernel tree you will see that others are using it too. Also there is accepted binding for that that's why I can't see big problem with it.

Quoted 5 lines

Quoted 3 lines
Since this is an application specific binding I wouldn't put it in the generic DT include file. It's a bit like adding a gpio-key binding for each of the GPIOs just in case somebody wants to use it.
So is your suggestion to move the whole thing from .dtsi -> .dts?

The #io-channels-cells can go into the dtsi, that's a sensible thing to do. The number of cells is part of the binding specification for the XADC and wont change.

The hwmon bridge on the other hand I wouldn't put into any default devicetree. In my opinion is should only used if and only if there are legacy applications on a specific system that need to use the hwmon interface.

Michal Simek

2017-03-16 18:06 UTC #7 lore raw

In reply to Lars-Peter Clausen

Quoted 50 lines · Lars-Peter Clausen

Quoted 49 lines · Michal Simek
Quoted 48 lines · Moritz Fischer
Quoted 46 lines · Michal Simek
Hi,
Quoted 31 lines · Moritz Fischer
Fix
OF: /iio_hwmon: could not get #io-channel-cells for /amba/adc at f8007100
OF: /iio_hwmon: could not get #io-channel-cells for /amba/adc at f8007100
OF: /iio_hwmon: could not get #io-channel-cells for /amba/adc at f8007100
by adding the #io-channel-cells property.
Signed-off-by Moritz Fischer <[email protected]>
Cc Michal Simek <[email protected]>
Cc S?ren Brinkmann <[email protected]>
Cc Julia Cartwright <[email protected]>
Cc linux-kernel at vger.kernel.org
Cc devicetree at vger.kernel.org
---
Changes from v1:
- fix messed up commit message
---
 arch/arm/boot/dts/zynq-7000.dtsi | 1 +
 1 file changed, 1 insertion(+)
arch/arm/boot/dts/zynq-7000.dtsi +0 -0
index f3ac9bf..98233a8 100644
arch/arm/boot/dts/zynq-7000.dtsi line 78
Quoted 7 lines
arch/arm/boot/dts/zynq-7000.dtsi +1 -0
@@ -72,6 +72,7 @@
                      interrupts = <0 7 4>;
                      interrupt-parent = <&intc>;
                      clocks = <&clkc 12>;
+                     #io-channel-cells = <1>;
              };
 
              can0: can at e0008000 {
I think it will be good to the next step too.
It means also add iio-hwmon node too.
What do you think?

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
I hadn't put it in there since dts is supposed to describe hw, but obviously putting the actual hwmon in there makes it more useful.
I had one discussion about this with Grant in past and it is common mistake. It is simplification of purpose of dts.
If the iio-hwmon binding had gone through review it would have been rejected.

Isn't it a time to deprecate it?

Quoted 11 lines

Quoted 7 lines
Quoted 3 lines
I can resubmit with the hwmon node in there.
If you grep kernel tree you will see that others are using it too. Also there is accepted binding for that that's why I can't see big problem with it.
Since this is an application specific binding I wouldn't put it in the generic DT include file. It's a bit like adding a gpio-key binding for each of the GPIOs just in case somebody wants to use it.

psci is system specific too.

IIRC this driver for zynq was written by ADI or with ADI help that's why you know much better than I what's the correct configuration.

This targets PS IP which should be present in the hw all the time. Not sure if for all configuration but I expect at least the part of it is there all them time.

If binding is incorrect then please remove it with removing from all dts/dtsi files which have this. The same is for of probe function in the driver itself.
If this is not done then this is just +1 case.

If you still insist that we shouldn't do it then please at least extend commit message and put there example how to wire it on zynq.

Thanks,
Michal

Lars-Peter Clausen

2017-03-16 21:20 UTC #8 lore raw

Quoted 54 lines · Michal Simek

Quoted 53 lines · Lars-Peter Clausen
Quoted 52 lines · Michal Simek
Quoted 50 lines · Moritz Fischer
Quoted 39 lines · Michal Simek
Hi,
Quoted 36 lines · Moritz Fischer
Fix
OF: /iio_hwmon: could not get #io-channel-cells for /amba/adc at f8007100
OF: /iio_hwmon: could not get #io-channel-cells for /amba/adc at f8007100
OF: /iio_hwmon: could not get #io-channel-cells for /amba/adc at f8007100
by adding the #io-channel-cells property.
Signed-off-by Moritz Fischer <[email protected]>
Cc Michal Simek <[email protected]>
Cc S?ren Brinkmann <[email protected]>
Cc Julia Cartwright <[email protected]>
Cc linux-kernel at vger.kernel.org
Cc devicetree at vger.kernel.org
---
Changes from v1:
- fix messed up commit message
---
 arch/arm/boot/dts/zynq-7000.dtsi | 1 +
 1 file changed, 1 insertion(+)
arch/arm/boot/dts/zynq-7000.dtsi +1 -0
index f3ac9bf..98233a8 100644
@@ -72,6 +72,7 @@
                      interrupts = <0 7 4>;
                      interrupt-parent = <&intc>;
                      clocks = <&clkc 12>;
+                     #io-channel-cells = <1>;
              };
 
Quoted 7 lines
arch/arm/boot/dts/zynq-7000.dtsi line 78
Quoted 2 lines
arch/arm/boot/dts/zynq-7000.dtsi +0 -0
              can0: can at e0008000 {
I think it will be good to the next step too.
It means also add iio-hwmon node too.
What do you think?
I hadn't put it in there since dts is supposed to describe hw, but obviously putting the actual hwmon in there makes it more useful.

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
I had one discussion about this with Grant in past and it is common mistake. It is simplification of purpose of dts.
If the iio-hwmon binding had gone through review it would have been rejected.
Isn't it a time to deprecate it?

Well, it's ABI now and has to stay forever. Deprecating it makes only sense if there is a replacement, which there is not. The iio-hwmon bridge has its usecases it's just instantiating it via devicetree which is not so nice.

Quoted 22 lines

Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
I can resubmit with the hwmon node in there.
Quoted 7 lines
Quoted 3 lines
If you grep kernel tree you will see that others are using it too. Also there is accepted binding for that that's why I can't see big problem with it.
Since this is an application specific binding I wouldn't put it in the generic DT include file. It's a bit like adding a gpio-key binding for each of the GPIOs just in case somebody wants to use it.
psci is system specific too.
IIRC this driver for zynq was written by ADI or with ADI help that's why you know much better than I what's the correct configuration.
This targets PS IP which should be present in the hw all the time. Not sure if for all configuration but I expect at least the part of it is there all them time.

Quoted 7 lines

If binding is incorrect then please remove it with removing from all dts/dtsi files which have this. The same is for of probe function in the driver itself.
If this is not done then this is just +1 case.
If you still insist that we shouldn't do it then please at least extend commit message and put there example how to wire it on zynq.

There is a IIO driver for the XADC, this driver has a userspace interface that exposes the measurements provided by the hardware. Using the hwmon bridge will expose the same information just through a hwmon interface.

One reason for using the iio-hwmon bridge is because you have a legacy application that expects the a hwmon interface. But new applications that want to access the XADC should really use the IIO interface if possible.

In my opinion instantiating the hwmon bridge by default will only cause confusion. There are now redundant interfaces and users will wonder what is the difference between the two. Is it the same data, is it different data? Which is the preferred interface? Which one is 'better'?

Michal Simek

2017-03-17 06:46 UTC #9 lore raw

Quoted 101 lines · Lars-Peter Clausen

Quoted 54 lines · Michal Simek
Quoted 53 lines · Lars-Peter Clausen
Quoted 52 lines · Michal Simek
Quoted 50 lines · Moritz Fischer
Quoted 39 lines · Michal Simek
Hi,
Quoted 36 lines · Moritz Fischer
Fix
OF: /iio_hwmon: could not get #io-channel-cells for /amba/adc at f8007100
OF: /iio_hwmon: could not get #io-channel-cells for /amba/adc at f8007100
OF: /iio_hwmon: could not get #io-channel-cells for /amba/adc at f8007100
by adding the #io-channel-cells property.
Signed-off-by Moritz Fischer <[email protected]>
Cc Michal Simek <[email protected]>
Cc S?ren Brinkmann <[email protected]>
Cc Julia Cartwright <[email protected]>
Cc linux-kernel at vger.kernel.org
Cc devicetree at vger.kernel.org
---
Changes from v1:
- fix messed up commit message
---
 arch/arm/boot/dts/zynq-7000.dtsi | 1 +
 1 file changed, 1 insertion(+)
arch/arm/boot/dts/zynq-7000.dtsi +1 -0
index f3ac9bf..98233a8 100644
@@ -72,6 +72,7 @@
                      interrupts = <0 7 4>;
                      interrupt-parent = <&intc>;
                      clocks = <&clkc 12>;
+                     #io-channel-cells = <1>;
              };
 
Quoted 7 lines
arch/arm/boot/dts/zynq-7000.dtsi line 78
Quoted 2 lines
arch/arm/boot/dts/zynq-7000.dtsi +0 -0
              can0: can at e0008000 {
I think it will be good to the next step too.
It means also add iio-hwmon node too.
What do you think?
I hadn't put it in there since dts is supposed to describe hw, but obviously putting the actual hwmon in there makes it more useful.
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
I had one discussion about this with Grant in past and it is common mistake. It is simplification of purpose of dts.
If the iio-hwmon binding had gone through review it would have been rejected.
Isn't it a time to deprecate it?
Well, it's ABI now and has to stay forever. Deprecating it makes only sense if there is a replacement, which there is not. The iio-hwmon bridge has its usecases it's just instantiating it via devicetree which is not so nice.
Quoted 22 lines
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
I can resubmit with the hwmon node in there.
Quoted 7 lines
Quoted 3 lines
If you grep kernel tree you will see that others are using it too. Also there is accepted binding for that that's why I can't see big problem with it.
Since this is an application specific binding I wouldn't put it in the generic DT include file. It's a bit like adding a gpio-key binding for each of the GPIOs just in case somebody wants to use it.
psci is system specific too.
IIRC this driver for zynq was written by ADI or with ADI help that's why you know much better than I what's the correct configuration.
This targets PS IP which should be present in the hw all the time. Not sure if for all configuration but I expect at least the part of it is there all them time.
Quoted 7 lines
If binding is incorrect then please remove it with removing from all dts/dtsi files which have this. The same is for of probe function in the driver itself.
If this is not done then this is just +1 case.
If you still insist that we shouldn't do it then please at least extend commit message and put there example how to wire it on zynq.
There is a IIO driver for the XADC, this driver has a userspace interface that exposes the measurements provided by the hardware. Using the hwmon bridge will expose the same information just through a hwmon interface.

Quoted 8 lines

One reason for using the iio-hwmon bridge is because you have a legacy application that expects the a hwmon interface. But new applications that want to access the XADC should really use the IIO interface if possible.
In my opinion instantiating the hwmon bridge by default will only cause confusion. There are now redundant interfaces and users will wonder what is the difference between the two. Is it the same data, is it different data? Which is the preferred interface? Which one is 'better'?

IMHO this should be covered by documentation. One paragraph in iio-hwmon binding can have answers for this and it will be very clear what people should use.

Thanks,
Michal

Michal Simek

2017-03-20 15:33 UTC #10 lore raw

Quoted 106 lines · Michal Simek

Quoted 105 lines · Lars-Peter Clausen
Quoted 54 lines · Michal Simek
Quoted 53 lines · Lars-Peter Clausen
Quoted 52 lines · Michal Simek
Quoted 50 lines · Moritz Fischer
Quoted 39 lines · Michal Simek
Hi,
Quoted 36 lines · Moritz Fischer
Fix
OF: /iio_hwmon: could not get #io-channel-cells for /amba/adc at f8007100
OF: /iio_hwmon: could not get #io-channel-cells for /amba/adc at f8007100
OF: /iio_hwmon: could not get #io-channel-cells for /amba/adc at f8007100
by adding the #io-channel-cells property.
Signed-off-by Moritz Fischer <[email protected]>
Cc Michal Simek <[email protected]>
Cc S?ren Brinkmann <[email protected]>
Cc Julia Cartwright <[email protected]>
Cc linux-kernel at vger.kernel.org
Cc devicetree at vger.kernel.org
---
Changes from v1:
- fix messed up commit message
---
 arch/arm/boot/dts/zynq-7000.dtsi | 1 +
 1 file changed, 1 insertion(+)
arch/arm/boot/dts/zynq-7000.dtsi +1 -0
index f3ac9bf..98233a8 100644
@@ -72,6 +72,7 @@
                      interrupts = <0 7 4>;
                      interrupt-parent = <&intc>;
                      clocks = <&clkc 12>;
+                     #io-channel-cells = <1>;
              };
 
Quoted 7 lines
arch/arm/boot/dts/zynq-7000.dtsi line 78
Quoted 2 lines
arch/arm/boot/dts/zynq-7000.dtsi +0 -0
              can0: can at e0008000 {
I think it will be good to the next step too.
It means also add iio-hwmon node too.
What do you think?
I hadn't put it in there since dts is supposed to describe hw, but obviously putting the actual hwmon in there makes it more useful.
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
I had one discussion about this with Grant in past and it is common mistake. It is simplification of purpose of dts.
If the iio-hwmon binding had gone through review it would have been rejected.
Isn't it a time to deprecate it?
Well, it's ABI now and has to stay forever. Deprecating it makes only sense if there is a replacement, which there is not. The iio-hwmon bridge has its usecases it's just instantiating it via devicetree which is not so nice.
Quoted 22 lines
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
I can resubmit with the hwmon node in there.
Quoted 7 lines
Quoted 3 lines
If you grep kernel tree you will see that others are using it too. Also there is accepted binding for that that's why I can't see big problem with it.
Since this is an application specific binding I wouldn't put it in the generic DT include file. It's a bit like adding a gpio-key binding for each of the GPIOs just in case somebody wants to use it.
psci is system specific too.
IIRC this driver for zynq was written by ADI or with ADI help that's why you know much better than I what's the correct configuration.
This targets PS IP which should be present in the hw all the time. Not sure if for all configuration but I expect at least the part of it is there all them time.
Quoted 7 lines
If binding is incorrect then please remove it with removing from all dts/dtsi files which have this. The same is for of probe function in the driver itself.
If this is not done then this is just +1 case.
If you still insist that we shouldn't do it then please at least extend commit message and put there example how to wire it on zynq.
There is a IIO driver for the XADC, this driver has a userspace interface that exposes the measurements provided by the hardware. Using the hwmon bridge will expose the same information just through a hwmon interface.
One reason for using the iio-hwmon bridge is because you have a legacy application that expects the a hwmon interface. But new applications that want to access the XADC should really use the IIO interface if possible.

Quoted 8 lines

Quoted 4 lines
In my opinion instantiating the hwmon bridge by default will only cause confusion. There are now redundant interfaces and users will wonder what is the difference between the two. Is it the same data, is it different data? Which is the preferred interface? Which one is 'better'?
IMHO this should be covered by documentation. One paragraph in iio-hwmon binding can have answers for this and it will be very clear what people should use.

Any comment on this one?

Thanks,
Michal

Lars-Peter Clausen

2017-03-20 15:34 UTC #11 lore raw

Quoted 112 lines · Michal Simek

Quoted 111 lines · Michal Simek
Quoted 109 lines · Lars-Peter Clausen
Quoted 54 lines · Michal Simek
Quoted 53 lines · Lars-Peter Clausen
Quoted 52 lines · Michal Simek
Quoted 50 lines · Moritz Fischer
Quoted 39 lines · Michal Simek
Hi,
Quoted 36 lines · Moritz Fischer
Fix
OF: /iio_hwmon: could not get #io-channel-cells for /amba/adc at f8007100
OF: /iio_hwmon: could not get #io-channel-cells for /amba/adc at f8007100
OF: /iio_hwmon: could not get #io-channel-cells for /amba/adc at f8007100
by adding the #io-channel-cells property.
Signed-off-by Moritz Fischer <[email protected]>
Cc Michal Simek <[email protected]>
Cc S?ren Brinkmann <[email protected]>
Cc Julia Cartwright <[email protected]>
Cc linux-kernel at vger.kernel.org
Cc devicetree at vger.kernel.org
---
Changes from v1:
- fix messed up commit message
---
 arch/arm/boot/dts/zynq-7000.dtsi | 1 +
 1 file changed, 1 insertion(+)
arch/arm/boot/dts/zynq-7000.dtsi +1 -0
index f3ac9bf..98233a8 100644
@@ -72,6 +72,7 @@
                      interrupts = <0 7 4>;
                      interrupt-parent = <&intc>;
                      clocks = <&clkc 12>;
+                     #io-channel-cells = <1>;
              };
 
Quoted 7 lines
arch/arm/boot/dts/zynq-7000.dtsi line 78
Quoted 2 lines
arch/arm/boot/dts/zynq-7000.dtsi +0 -0
              can0: can at e0008000 {
I think it will be good to the next step too.
It means also add iio-hwmon node too.
What do you think?
I hadn't put it in there since dts is supposed to describe hw, but obviously putting the actual hwmon in there makes it more useful.
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
I had one discussion about this with Grant in past and it is common mistake. It is simplification of purpose of dts.
If the iio-hwmon binding had gone through review it would have been rejected.
Isn't it a time to deprecate it?
Well, it's ABI now and has to stay forever. Deprecating it makes only sense if there is a replacement, which there is not. The iio-hwmon bridge has its usecases it's just instantiating it via devicetree which is not so nice.
Quoted 22 lines
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
I can resubmit with the hwmon node in there.
Quoted 7 lines
Quoted 3 lines
If you grep kernel tree you will see that others are using it too. Also there is accepted binding for that that's why I can't see big problem with it.
Since this is an application specific binding I wouldn't put it in the generic DT include file. It's a bit like adding a gpio-key binding for each of the GPIOs just in case somebody wants to use it.
psci is system specific too.
IIRC this driver for zynq was written by ADI or with ADI help that's why you know much better than I what's the correct configuration.
This targets PS IP which should be present in the hw all the time. Not sure if for all configuration but I expect at least the part of it is there all them time.
Quoted 7 lines
If binding is incorrect then please remove it with removing from all dts/dtsi files which have this. The same is for of probe function in the driver itself.
If this is not done then this is just +1 case.
If you still insist that we shouldn't do it then please at least extend commit message and put there example how to wire it on zynq.
There is a IIO driver for the XADC, this driver has a userspace interface that exposes the measurements provided by the hardware. Using the hwmon bridge will expose the same information just through a hwmon interface.
One reason for using the iio-hwmon bridge is because you have a legacy application that expects the a hwmon interface. But new applications that want to access the XADC should really use the IIO interface if possible.
In my opinion instantiating the hwmon bridge by default will only cause confusion. There are now redundant interfaces and users will wonder what is the difference between the two. Is it the same data, is it different data? Which is the preferred interface? Which one is 'better'?

Quoted 5 lines

Quoted 3 lines
IMHO this should be covered by documentation. One paragraph in iio-hwmon binding can have answers for this and it will be very clear what people should use.
Any comment on this one?

Send a patch? :)

Michal Simek

2017-04-07 07:11 UTC #12 lore raw

Quoted 113 lines · Lars-Peter Clausen

Quoted 112 lines · Michal Simek
Quoted 111 lines · Michal Simek
Quoted 109 lines · Lars-Peter Clausen
Quoted 54 lines · Michal Simek
Quoted 53 lines · Lars-Peter Clausen
Quoted 52 lines · Michal Simek
Quoted 50 lines · Moritz Fischer
Quoted 39 lines · Michal Simek
Hi,
Quoted 36 lines · Moritz Fischer
Fix
OF: /iio_hwmon: could not get #io-channel-cells for /amba/adc at f8007100
OF: /iio_hwmon: could not get #io-channel-cells for /amba/adc at f8007100
OF: /iio_hwmon: could not get #io-channel-cells for /amba/adc at f8007100
by adding the #io-channel-cells property.
Signed-off-by Moritz Fischer <[email protected]>
Cc Michal Simek <[email protected]>
Cc S?ren Brinkmann <[email protected]>
Cc Julia Cartwright <[email protected]>
Cc linux-kernel at vger.kernel.org
Cc devicetree at vger.kernel.org
---
Changes from v1:
- fix messed up commit message
---
 arch/arm/boot/dts/zynq-7000.dtsi | 1 +
 1 file changed, 1 insertion(+)
arch/arm/boot/dts/zynq-7000.dtsi +1 -0
index f3ac9bf..98233a8 100644
@@ -72,6 +72,7 @@
                      interrupts = <0 7 4>;
                      interrupt-parent = <&intc>;
                      clocks = <&clkc 12>;
+                     #io-channel-cells = <1>;
              };
 
Quoted 7 lines
arch/arm/boot/dts/zynq-7000.dtsi line 78
Quoted 2 lines
arch/arm/boot/dts/zynq-7000.dtsi +0 -0
              can0: can at e0008000 {
I think it will be good to the next step too.
It means also add iio-hwmon node too.
What do you think?
I hadn't put it in there since dts is supposed to describe hw, but obviously putting the actual hwmon in there makes it more useful.
Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
I had one discussion about this with Grant in past and it is common mistake. It is simplification of purpose of dts.
If the iio-hwmon binding had gone through review it would have been rejected.
Isn't it a time to deprecate it?
Well, it's ABI now and has to stay forever. Deprecating it makes only sense if there is a replacement, which there is not. The iio-hwmon bridge has its usecases it's just instantiating it via devicetree which is not so nice.
Quoted 22 lines
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
I can resubmit with the hwmon node in there.
Quoted 7 lines
Quoted 3 lines
If you grep kernel tree you will see that others are using it too. Also there is accepted binding for that that's why I can't see big problem with it.
Since this is an application specific binding I wouldn't put it in the generic DT include file. It's a bit like adding a gpio-key binding for each of the GPIOs just in case somebody wants to use it.
psci is system specific too.
IIRC this driver for zynq was written by ADI or with ADI help that's why you know much better than I what's the correct configuration.
This targets PS IP which should be present in the hw all the time. Not sure if for all configuration but I expect at least the part of it is there all them time.
Quoted 7 lines
If binding is incorrect then please remove it with removing from all dts/dtsi files which have this. The same is for of probe function in the driver itself.
If this is not done then this is just +1 case.
If you still insist that we shouldn't do it then please at least extend commit message and put there example how to wire it on zynq.
There is a IIO driver for the XADC, this driver has a userspace interface that exposes the measurements provided by the hardware. Using the hwmon bridge will expose the same information just through a hwmon interface.
One reason for using the iio-hwmon bridge is because you have a legacy application that expects the a hwmon interface. But new applications that want to access the XADC should really use the IIO interface if possible.
In my opinion instantiating the hwmon bridge by default will only cause confusion. There are now redundant interfaces and users will wonder what is the difference between the two. Is it the same data, is it different data? Which is the preferred interface? Which one is 'better'?

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
IMHO this should be covered by documentation. One paragraph in iio-hwmon binding can have answers for this and it will be very clear what people should use.
Any comment on this one?
Send a patch? :)

I don't feel that I know iio enough to be good candidate to describe this properly.

Thanks,
Michal