linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] ARM: dts: da850-evm: vpif DT changes
@ 2017-02-16 18:15 Bartosz Golaszewski
  2017-02-16 18:15 ` [PATCH 1/4] ARM: dts: da850-evm: fix whitespace errors Bartosz Golaszewski
                   ` (3 more replies)
  0 siblings, 4 replies; 14+ messages in thread
From: Bartosz Golaszewski @ 2017-02-16 18:15 UTC (permalink / raw)
  To: Sekhar Nori, David Lechner, Kevin Hilman, Michael Turquette,
	Patrick Titiano, Laurent Pinchart, Rob Herring, Mark Rutland,
	Russell King
  Cc: linux-arm-kernel, linux-kernel, devicetree, Bartosz Golaszewski

This series adds necessary changes to make vpif work on the da850-evm
board.

The first patch only contains whitespace error fixes.

The second patch add a pinctrl node for vpif display pins.

The third patch extends the vpif node with an output port.

The last patch adds the UI expander node and GPIO hogs that are needed
to select video capture functionality.

The last patch might not be the correct approach - if we wanted to
select camera input we'd need to have separate dts files in the
future.

In board file mode this is done at GPIO expander setup. We can't use
pdata quirks neither as they are called before the expander is probed.

Any ideas are welcome.

Bartosz Golaszewski (4):
  ARM: dts: da850-evm: fix whitespace errors
  ARM: dts: da850: add vpif video display pins
  ARM: dts: da850-evm: add the output port to the vpif node
  ARM: dts: da850-evm: add the UI expander node

 arch/arm/boot/dts/da850-evm.dts | 52 ++++++++++++++++++++++++++++++++++-------
 arch/arm/boot/dts/da850.dtsi    | 25 +++++++++++++++++---
 2 files changed, 65 insertions(+), 12 deletions(-)

-- 
2.9.3

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/4] ARM: dts: da850-evm: fix whitespace errors
  2017-02-16 18:15 [PATCH 0/4] ARM: dts: da850-evm: vpif DT changes Bartosz Golaszewski
@ 2017-02-16 18:15 ` Bartosz Golaszewski
  2017-02-16 18:15 ` [PATCH 2/4] ARM: dts: da850: add vpif video display pins Bartosz Golaszewski
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 14+ messages in thread
From: Bartosz Golaszewski @ 2017-02-16 18:15 UTC (permalink / raw)
  To: Sekhar Nori, David Lechner, Kevin Hilman, Michael Turquette,
	Patrick Titiano, Laurent Pinchart, Rob Herring, Mark Rutland,
	Russell King
  Cc: linux-arm-kernel, linux-kernel, devicetree, Bartosz Golaszewski

The da850-evm dts file contains whitespace errors in the vpif node.

This patch fixes them.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 arch/arm/boot/dts/da850-evm.dts | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/arm/boot/dts/da850-evm.dts b/arch/arm/boot/dts/da850-evm.dts
index 8c5671c..dece981 100644
--- a/arch/arm/boot/dts/da850-evm.dts
+++ b/arch/arm/boot/dts/da850-evm.dts
@@ -302,14 +302,14 @@
 	/* VPIF capture port */
 	port {
 		vpif_ch0: endpoint@0 {
-			  reg = <0>;
-			  bus-width = <8>;
+			reg = <0>;
+			bus-width = <8>;
 		};
 
 		vpif_ch1: endpoint@1 {
-			  reg = <1>;
-			  bus-width = <8>;
-			  data-shift = <8>;
+			reg = <1>;
+			bus-width = <8>;
+			data-shift = <8>;
 		};
 	};
 };
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/4] ARM: dts: da850: add vpif video display pins
  2017-02-16 18:15 [PATCH 0/4] ARM: dts: da850-evm: vpif DT changes Bartosz Golaszewski
  2017-02-16 18:15 ` [PATCH 1/4] ARM: dts: da850-evm: fix whitespace errors Bartosz Golaszewski
@ 2017-02-16 18:15 ` Bartosz Golaszewski
  2017-02-20 10:29   ` Sekhar Nori
  2017-02-16 18:15 ` [PATCH 3/4] ARM: dts: da850-evm: add the output port to the vpif node Bartosz Golaszewski
  2017-02-16 18:15 ` [PATCH 4/4] ARM: dts: da850-evm: add the UI expander node Bartosz Golaszewski
  3 siblings, 1 reply; 14+ messages in thread
From: Bartosz Golaszewski @ 2017-02-16 18:15 UTC (permalink / raw)
  To: Sekhar Nori, David Lechner, Kevin Hilman, Michael Turquette,
	Patrick Titiano, Laurent Pinchart, Rob Herring, Mark Rutland,
	Russell King
  Cc: linux-arm-kernel, linux-kernel, devicetree, Bartosz Golaszewski

Add a new pinctrl sub-node for vpif display pins. Move VP_CLKIN3 and
VP_CLKIN2 to the display node where they actually belong (vide section
35.2.2 of the da850 datasheet).

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 arch/arm/boot/dts/da850-evm.dts |  2 +-
 arch/arm/boot/dts/da850.dtsi    | 17 +++++++++++++++--
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/da850-evm.dts b/arch/arm/boot/dts/da850-evm.dts
index dece981..14f6f8ea 100644
--- a/arch/arm/boot/dts/da850-evm.dts
+++ b/arch/arm/boot/dts/da850-evm.dts
@@ -296,7 +296,7 @@
 
 &vpif {
 	pinctrl-names = "default";
-	pinctrl-0 = <&vpif_capture_pins>;
+	pinctrl-0 = <&vpif_capture_pins>, <&vpif_display_pins>;
 	status = "okay";
 
 	/* VPIF capture port */
diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index 92d633d..5150331 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -216,8 +216,21 @@
 					0x3c 0x11111111 0xffffffff
 					/* VP_DIN[8..9] */
 					0x40 0x00000011 0x000000ff
-					/* VP_CLKIN3, VP_CLKIN2 */
-					0x4c 0x00010100 0x000f0f00
+				>;
+			};
+			vpif_display_pins: vpif_display_pins {
+				pinctrl-single,bits = <
+					/* VP_DOUT[2..7] */
+					0x40 0x11111100 0xffffff00
+					/* VP_DOUT[10..15,0..1] */
+					0x44 0x11111111 0xffffffff
+					/*  VP_DOUT[8..9] */
+					0x48 0x00000011 0x000000ff
+					/*
+					 * VP_CLKOUT3, VP_CLKIN3,
+					 * VP_CLKOUT2, VP_CLKIN2
+					 */
+					0x4c 0x00111100 0x00ffff00
 				>;
 			};
 		};
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 3/4] ARM: dts: da850-evm: add the output port to the vpif node
  2017-02-16 18:15 [PATCH 0/4] ARM: dts: da850-evm: vpif DT changes Bartosz Golaszewski
  2017-02-16 18:15 ` [PATCH 1/4] ARM: dts: da850-evm: fix whitespace errors Bartosz Golaszewski
  2017-02-16 18:15 ` [PATCH 2/4] ARM: dts: da850: add vpif video display pins Bartosz Golaszewski
@ 2017-02-16 18:15 ` Bartosz Golaszewski
  2017-02-16 18:15 ` [PATCH 4/4] ARM: dts: da850-evm: add the UI expander node Bartosz Golaszewski
  3 siblings, 0 replies; 14+ messages in thread
From: Bartosz Golaszewski @ 2017-02-16 18:15 UTC (permalink / raw)
  To: Sekhar Nori, David Lechner, Kevin Hilman, Michael Turquette,
	Patrick Titiano, Laurent Pinchart, Rob Herring, Mark Rutland,
	Russell King
  Cc: linux-arm-kernel, linux-kernel, devicetree, Bartosz Golaszewski

Extend the vpif node with an output port with a single channel.

NOTE: this is still just hardware description - the actual driver
is registered using pdata-quirks.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 arch/arm/boot/dts/da850-evm.dts | 13 ++++++++++---
 arch/arm/boot/dts/da850.dtsi    |  8 +++++++-
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/da850-evm.dts b/arch/arm/boot/dts/da850-evm.dts
index 14f6f8ea..b549861 100644
--- a/arch/arm/boot/dts/da850-evm.dts
+++ b/arch/arm/boot/dts/da850-evm.dts
@@ -300,16 +300,23 @@
 	status = "okay";
 
 	/* VPIF capture port */
-	port {
-		vpif_ch0: endpoint@0 {
+	port@0 {
+		vpif_input_ch0: endpoint@0 {
 			reg = <0>;
 			bus-width = <8>;
 		};
 
-		vpif_ch1: endpoint@1 {
+		vpif_input_ch1: endpoint@1 {
 			reg = <1>;
 			bus-width = <8>;
 			data-shift = <8>;
 		};
 	};
+
+	/* VPIF display port */
+	port@1 {
+		vpif_output_ch0: endpoint {
+			bus-width = <8>;
+		};
+	};
 };
diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index 5150331..c708155 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -358,7 +358,13 @@
 			status = "disabled";
 
 			/* VPIF capture port */
-			port {
+			port@0 {
+				#address-cells = <1>;
+				#size-cells = <0>;
+			};
+
+			/* VPIF display port */
+			port@1 {
 				#address-cells = <1>;
 				#size-cells = <0>;
 			};
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 4/4] ARM: dts: da850-evm: add the UI expander node
  2017-02-16 18:15 [PATCH 0/4] ARM: dts: da850-evm: vpif DT changes Bartosz Golaszewski
                   ` (2 preceding siblings ...)
  2017-02-16 18:15 ` [PATCH 3/4] ARM: dts: da850-evm: add the output port to the vpif node Bartosz Golaszewski
@ 2017-02-16 18:15 ` Bartosz Golaszewski
  2017-02-20  9:36   ` Sekhar Nori
  3 siblings, 1 reply; 14+ messages in thread
From: Bartosz Golaszewski @ 2017-02-16 18:15 UTC (permalink / raw)
  To: Sekhar Nori, David Lechner, Kevin Hilman, Michael Turquette,
	Patrick Titiano, Laurent Pinchart, Rob Herring, Mark Rutland,
	Russell King
  Cc: linux-arm-kernel, linux-kernel, devicetree, Bartosz Golaszewski

If we're using the UI board and want vpif capture, we need to select
the video capture functionality by driving the sel_c pin low on the
tca6416 expander and sel_a & sel_b pins high. Do it statically by
hogging relevant GPIOs in the device tree.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 arch/arm/boot/dts/da850-evm.dts | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/arch/arm/boot/dts/da850-evm.dts b/arch/arm/boot/dts/da850-evm.dts
index b549861..a90c764 100644
--- a/arch/arm/boot/dts/da850-evm.dts
+++ b/arch/arm/boot/dts/da850-evm.dts
@@ -9,6 +9,7 @@
  */
 /dts-v1/;
 #include "da850.dtsi"
+#include <dt-bindings/gpio/gpio.h>
 
 / {
 	compatible = "ti,da850-evm", "ti,da850";
@@ -78,7 +79,33 @@
 				DRVDD-supply = <&vbat>;
 				DVDD-supply = <&vbat>;
 			};
+			ui_expander: tca6416@20 {
+				compatible = "ti,tca6416";
+				reg = <0x20>;
+				gpio-controller;
+				#gpio-cells = <2>;
 
+				sel_a {
+					gpio-hog;
+					gpios = <7 GPIO_ACTIVE_HIGH>;
+					output-high;
+					line-name = "sel_a";
+				};
+
+				sel_b {
+					gpio-hog;
+					gpios = <6 GPIO_ACTIVE_HIGH>;
+					output-high;
+					line-name = "sel_b";
+				};
+
+				sel_c {
+					gpio-hog;
+					gpios = <5 GPIO_ACTIVE_HIGH>;
+					output-low;
+					line-name = "sel_c";
+				};
+			};
 		};
 		wdt: wdt@21000 {
 			status = "okay";
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 4/4] ARM: dts: da850-evm: add the UI expander node
  2017-02-16 18:15 ` [PATCH 4/4] ARM: dts: da850-evm: add the UI expander node Bartosz Golaszewski
@ 2017-02-20  9:36   ` Sekhar Nori
  2017-02-20 15:38     ` Bartosz Golaszewski
  0 siblings, 1 reply; 14+ messages in thread
From: Sekhar Nori @ 2017-02-20  9:36 UTC (permalink / raw)
  To: Bartosz Golaszewski, David Lechner, Kevin Hilman,
	Michael Turquette, Patrick Titiano, Laurent Pinchart,
	Rob Herring, Mark Rutland, Russell King
  Cc: devicetree, linux-kernel, linux-arm-kernel

On Thursday 16 February 2017 11:45 PM, Bartosz Golaszewski wrote:
> If we're using the UI board and want vpif capture, we need to select
> the video capture functionality by driving the sel_c pin low on the
> tca6416 expander and sel_a & sel_b pins high. Do it statically by
> hogging relevant GPIOs in the device tree.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  arch/arm/boot/dts/da850-evm.dts | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/da850-evm.dts b/arch/arm/boot/dts/da850-evm.dts
> index b549861..a90c764 100644
> --- a/arch/arm/boot/dts/da850-evm.dts
> +++ b/arch/arm/boot/dts/da850-evm.dts
> @@ -9,6 +9,7 @@
>   */
>  /dts-v1/;
>  #include "da850.dtsi"
> +#include <dt-bindings/gpio/gpio.h>
>  
>  / {
>  	compatible = "ti,da850-evm", "ti,da850";
> @@ -78,7 +79,33 @@
>  				DRVDD-supply = <&vbat>;
>  				DVDD-supply = <&vbat>;
>  			};

> +			ui_expander: tca6416@20 {

This should be called:

			tca6416: gpio@20 {

in keeping with ePAPR 1.1 generic node names recommendation.

> +				compatible = "ti,tca6416";
> +				reg = <0x20>;
> +				gpio-controller;
> +				#gpio-cells = <2>;
>  
> +				sel_a {
> +					gpio-hog;
> +					gpios = <7 GPIO_ACTIVE_HIGH>;
> +					output-high;
> +					line-name = "sel_a";
> +				};
> +
> +				sel_b {
> +					gpio-hog;
> +					gpios = <6 GPIO_ACTIVE_HIGH>;
> +					output-high;
> +					line-name = "sel_b";
> +				};
> +
> +				sel_c {
> +					gpio-hog;
> +					gpios = <5 GPIO_ACTIVE_HIGH>;
> +					output-low;
> +					line-name = "sel_c";

I think this is better handled by using an enable-gpios property in vpif
capture device-tree node. So in the vpif capture node you would have:

	enable-gpios =  <&tca6416 7 GPIO_ACTIVE_HIGH
			 &tca6416 6 GPIO_ACTIVE_HIGH
			 &tca6416 5 GPIO_ACTIVE_LOW>;

and in the vpif capture driver, you would request each of these gpios
using: devm_gpiod_get_array_optional(.., .., GPIOD_OUT_HIGH);

Thanks,
Sekhar

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/4] ARM: dts: da850: add vpif video display pins
  2017-02-16 18:15 ` [PATCH 2/4] ARM: dts: da850: add vpif video display pins Bartosz Golaszewski
@ 2017-02-20 10:29   ` Sekhar Nori
  2017-02-20 15:42     ` Bartosz Golaszewski
  0 siblings, 1 reply; 14+ messages in thread
From: Sekhar Nori @ 2017-02-20 10:29 UTC (permalink / raw)
  To: Bartosz Golaszewski, David Lechner, Kevin Hilman,
	Michael Turquette, Patrick Titiano, Laurent Pinchart,
	Rob Herring, Mark Rutland, Russell King
  Cc: devicetree, linux-kernel, linux-arm-kernel

On Thursday 16 February 2017 11:45 PM, Bartosz Golaszewski wrote:
> Add a new pinctrl sub-node for vpif display pins. Move VP_CLKIN3 and
> VP_CLKIN2 to the display node where they actually belong (vide section
> 35.2.2 of the da850 datasheet).

You mean 36.2.2. Also, its in the technical reference manual (TRM). The
datahseet is another document.

> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  arch/arm/boot/dts/da850-evm.dts |  2 +-
>  arch/arm/boot/dts/da850.dtsi    | 17 +++++++++++++++--

Can you separate out the board and SoC specific changes? The board
changes could be done in patch 3/4 where other video output parts are added.

Thanks,
Sekhar

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 4/4] ARM: dts: da850-evm: add the UI expander node
  2017-02-20  9:36   ` Sekhar Nori
@ 2017-02-20 15:38     ` Bartosz Golaszewski
  2017-02-21  5:03       ` Sekhar Nori
  0 siblings, 1 reply; 14+ messages in thread
From: Bartosz Golaszewski @ 2017-02-20 15:38 UTC (permalink / raw)
  To: Sekhar Nori
  Cc: David Lechner, Kevin Hilman, Michael Turquette, Patrick Titiano,
	Laurent Pinchart, Rob Herring, Mark Rutland, Russell King,
	linux-devicetree, LKML, arm-soc

2017-02-20 10:36 GMT+01:00 Sekhar Nori <nsekhar@ti.com>:
> On Thursday 16 February 2017 11:45 PM, Bartosz Golaszewski wrote:
>> If we're using the UI board and want vpif capture, we need to select
>> the video capture functionality by driving the sel_c pin low on the
>> tca6416 expander and sel_a & sel_b pins high. Do it statically by
>> hogging relevant GPIOs in the device tree.
>>
>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>> ---

[snip]

>>
>> +                             sel_a {
>> +                                     gpio-hog;
>> +                                     gpios = <7 GPIO_ACTIVE_HIGH>;
>> +                                     output-high;
>> +                                     line-name = "sel_a";
>> +                             };
>> +
>> +                             sel_b {
>> +                                     gpio-hog;
>> +                                     gpios = <6 GPIO_ACTIVE_HIGH>;
>> +                                     output-high;
>> +                                     line-name = "sel_b";
>> +                             };
>> +
>> +                             sel_c {
>> +                                     gpio-hog;
>> +                                     gpios = <5 GPIO_ACTIVE_HIGH>;
>> +                                     output-low;
>> +                                     line-name = "sel_c";
>
> I think this is better handled by using an enable-gpios property in vpif
> capture device-tree node. So in the vpif capture node you would have:
>
>         enable-gpios =  <&tca6416 7 GPIO_ACTIVE_HIGH
>                          &tca6416 6 GPIO_ACTIVE_HIGH
>                          &tca6416 5 GPIO_ACTIVE_LOW>;
>
> and in the vpif capture driver, you would request each of these gpios
> using: devm_gpiod_get_array_optional(.., .., GPIOD_OUT_HIGH);
>

I'm not sure about this one - the result is the same (function still
defined statically in the DT) while now it requires changes to the
vpif driver too.

Is there any other reason we'd prefer this approach?

Thanks,
Bartosz

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/4] ARM: dts: da850: add vpif video display pins
  2017-02-20 10:29   ` Sekhar Nori
@ 2017-02-20 15:42     ` Bartosz Golaszewski
  2017-02-21  4:49       ` Sekhar Nori
  0 siblings, 1 reply; 14+ messages in thread
From: Bartosz Golaszewski @ 2017-02-20 15:42 UTC (permalink / raw)
  To: Sekhar Nori
  Cc: David Lechner, Kevin Hilman, Michael Turquette, Patrick Titiano,
	Laurent Pinchart, Rob Herring, Mark Rutland, Russell King,
	linux-devicetree, LKML, arm-soc

2017-02-20 11:29 GMT+01:00 Sekhar Nori <nsekhar@ti.com>:
> On Thursday 16 February 2017 11:45 PM, Bartosz Golaszewski wrote:
>> Add a new pinctrl sub-node for vpif display pins. Move VP_CLKIN3 and
>> VP_CLKIN2 to the display node where they actually belong (vide section
>> 35.2.2 of the da850 datasheet).
>
> You mean 36.2.2. Also, its in the technical reference manual (TRM). The
> datahseet is another document.
>

I'm looking at the revision from September 2016 and it's 35.2.2: VPIF
-> Architecture -> signal descriptions.

Thanks,
Bartosz

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/4] ARM: dts: da850: add vpif video display pins
  2017-02-20 15:42     ` Bartosz Golaszewski
@ 2017-02-21  4:49       ` Sekhar Nori
  2017-02-21  9:23         ` Bartosz Golaszewski
  0 siblings, 1 reply; 14+ messages in thread
From: Sekhar Nori @ 2017-02-21  4:49 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: David Lechner, Kevin Hilman, Michael Turquette, Patrick Titiano,
	Laurent Pinchart, Rob Herring, Mark Rutland, Russell King,
	linux-devicetree, LKML, arm-soc

On Monday 20 February 2017 09:12 PM, Bartosz Golaszewski wrote:
> 2017-02-20 11:29 GMT+01:00 Sekhar Nori <nsekhar@ti.com>:
>> On Thursday 16 February 2017 11:45 PM, Bartosz Golaszewski wrote:
>>> Add a new pinctrl sub-node for vpif display pins. Move VP_CLKIN3 and
>>> VP_CLKIN2 to the display node where they actually belong (vide section
>>> 35.2.2 of the da850 datasheet).
>>
>> You mean 36.2.2. Also, its in the technical reference manual (TRM). The
>> datahseet is another document.
>>
> 
> I'm looking at the revision from September 2016 and it's 35.2.2: VPIF
> -> Architecture -> signal descriptions.

Is this the document ?

http://www.ti.com/lit/ug/spruh77c/spruh77c.pdf

In this VPIF is chapter 36.

Thanks,
Sekhar

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 4/4] ARM: dts: da850-evm: add the UI expander node
  2017-02-20 15:38     ` Bartosz Golaszewski
@ 2017-02-21  5:03       ` Sekhar Nori
  2017-02-21  9:22         ` Bartosz Golaszewski
  0 siblings, 1 reply; 14+ messages in thread
From: Sekhar Nori @ 2017-02-21  5:03 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: David Lechner, Kevin Hilman, Michael Turquette, Patrick Titiano,
	Laurent Pinchart, Rob Herring, Mark Rutland, Russell King,
	linux-devicetree, LKML, arm-soc

On Monday 20 February 2017 09:08 PM, Bartosz Golaszewski wrote:
> 2017-02-20 10:36 GMT+01:00 Sekhar Nori <nsekhar@ti.com>:
>> On Thursday 16 February 2017 11:45 PM, Bartosz Golaszewski wrote:
>>> If we're using the UI board and want vpif capture, we need to select
>>> the video capture functionality by driving the sel_c pin low on the
>>> tca6416 expander and sel_a & sel_b pins high. Do it statically by
>>> hogging relevant GPIOs in the device tree.
>>>
>>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>>> ---
> 
> [snip]
> 
>>>
>>> +                             sel_a {
>>> +                                     gpio-hog;
>>> +                                     gpios = <7 GPIO_ACTIVE_HIGH>;
>>> +                                     output-high;
>>> +                                     line-name = "sel_a";
>>> +                             };
>>> +
>>> +                             sel_b {
>>> +                                     gpio-hog;
>>> +                                     gpios = <6 GPIO_ACTIVE_HIGH>;
>>> +                                     output-high;
>>> +                                     line-name = "sel_b";
>>> +                             };
>>> +
>>> +                             sel_c {
>>> +                                     gpio-hog;
>>> +                                     gpios = <5 GPIO_ACTIVE_HIGH>;
>>> +                                     output-low;
>>> +                                     line-name = "sel_c";
>>
>> I think this is better handled by using an enable-gpios property in vpif
>> capture device-tree node. So in the vpif capture node you would have:
>>
>>         enable-gpios =  <&tca6416 7 GPIO_ACTIVE_HIGH
>>                          &tca6416 6 GPIO_ACTIVE_HIGH
>>                          &tca6416 5 GPIO_ACTIVE_LOW>;
>>
>> and in the vpif capture driver, you would request each of these gpios
>> using: devm_gpiod_get_array_optional(.., .., GPIOD_OUT_HIGH);
>>
> 
> I'm not sure about this one - the result is the same (function still
> defined statically in the DT) while now it requires changes to the
> vpif driver too.
> 
> Is there any other reason we'd prefer this approach?

The GPIO hog functionality can race with driver probe. Based on probe
order, you may have a situation where VPIF probes before tca6416 so we
have an erroneous situation where probe is successful, but hardware is
not really available.

Using enable-gpios lets you handle probe deferral so VPIF capture probe
completes only when hardware is ready. So if for some reason tca6416
driver or hardware is misbehaving, VPIF will know about it through some
error value rather than just assuming that everything went well.

So, yes, in the "all goes well" scenario, there is not much difference
in the two approaches. But the difference will be apparent when
something goes wrong.

Probe order will also influence the shutdown and suspend order. So
kernel will automatically make sure that shutdown happens in reverse
probe order. This may or may not matter in this case. But in  general,
it will be nice to make sure VPIF shuts down before tca6416 does so that
hardware is available for VPIF to cleanly shutdown (and not disconnected
behind its back because tca6416 decided to put all its lines to off as
part of its shutdown).

I think GPIO hog should only be used for pins which are really "system
level". IOW, not related to any driver functionality.

Thanks,
Sekhar

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 4/4] ARM: dts: da850-evm: add the UI expander node
  2017-02-21  5:03       ` Sekhar Nori
@ 2017-02-21  9:22         ` Bartosz Golaszewski
  0 siblings, 0 replies; 14+ messages in thread
From: Bartosz Golaszewski @ 2017-02-21  9:22 UTC (permalink / raw)
  To: Sekhar Nori
  Cc: David Lechner, Kevin Hilman, Michael Turquette, Patrick Titiano,
	Laurent Pinchart, Rob Herring, Mark Rutland, Russell King,
	linux-devicetree, LKML, arm-soc

2017-02-21 6:03 GMT+01:00 Sekhar Nori <nsekhar@ti.com>:
> On Monday 20 February 2017 09:08 PM, Bartosz Golaszewski wrote:
>> 2017-02-20 10:36 GMT+01:00 Sekhar Nori <nsekhar@ti.com>:
>>> On Thursday 16 February 2017 11:45 PM, Bartosz Golaszewski wrote:
>>>> If we're using the UI board and want vpif capture, we need to select
>>>> the video capture functionality by driving the sel_c pin low on the
>>>> tca6416 expander and sel_a & sel_b pins high. Do it statically by
>>>> hogging relevant GPIOs in the device tree.
>>>>
>>>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>>>> ---
>>
>> [snip]
>>
>>>>
>>>> +                             sel_a {
>>>> +                                     gpio-hog;
>>>> +                                     gpios = <7 GPIO_ACTIVE_HIGH>;
>>>> +                                     output-high;
>>>> +                                     line-name = "sel_a";
>>>> +                             };
>>>> +
>>>> +                             sel_b {
>>>> +                                     gpio-hog;
>>>> +                                     gpios = <6 GPIO_ACTIVE_HIGH>;
>>>> +                                     output-high;
>>>> +                                     line-name = "sel_b";
>>>> +                             };
>>>> +
>>>> +                             sel_c {
>>>> +                                     gpio-hog;
>>>> +                                     gpios = <5 GPIO_ACTIVE_HIGH>;
>>>> +                                     output-low;
>>>> +                                     line-name = "sel_c";
>>>
>>> I think this is better handled by using an enable-gpios property in vpif
>>> capture device-tree node. So in the vpif capture node you would have:
>>>
>>>         enable-gpios =  <&tca6416 7 GPIO_ACTIVE_HIGH
>>>                          &tca6416 6 GPIO_ACTIVE_HIGH
>>>                          &tca6416 5 GPIO_ACTIVE_LOW>;
>>>
>>> and in the vpif capture driver, you would request each of these gpios
>>> using: devm_gpiod_get_array_optional(.., .., GPIOD_OUT_HIGH);
>>>
>>
>> I'm not sure about this one - the result is the same (function still
>> defined statically in the DT) while now it requires changes to the
>> vpif driver too.
>>
>> Is there any other reason we'd prefer this approach?
>
> The GPIO hog functionality can race with driver probe. Based on probe
> order, you may have a situation where VPIF probes before tca6416 so we
> have an erroneous situation where probe is successful, but hardware is
> not really available.
>
> Using enable-gpios lets you handle probe deferral so VPIF capture probe
> completes only when hardware is ready. So if for some reason tca6416
> driver or hardware is misbehaving, VPIF will know about it through some
> error value rather than just assuming that everything went well.
>
> So, yes, in the "all goes well" scenario, there is not much difference
> in the two approaches. But the difference will be apparent when
> something goes wrong.
>
> Probe order will also influence the shutdown and suspend order. So
> kernel will automatically make sure that shutdown happens in reverse
> probe order. This may or may not matter in this case. But in  general,
> it will be nice to make sure VPIF shuts down before tca6416 does so that
> hardware is available for VPIF to cleanly shutdown (and not disconnected
> behind its back because tca6416 decided to put all its lines to off as
> part of its shutdown).
>
> I think GPIO hog should only be used for pins which are really "system
> level". IOW, not related to any driver functionality.
>
> Thanks,
> Sekhar

Ok, I'll extend the driver then.

Thanks,
Bartosz

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/4] ARM: dts: da850: add vpif video display pins
  2017-02-21  4:49       ` Sekhar Nori
@ 2017-02-21  9:23         ` Bartosz Golaszewski
  2017-02-22  5:59           ` Sekhar Nori
  0 siblings, 1 reply; 14+ messages in thread
From: Bartosz Golaszewski @ 2017-02-21  9:23 UTC (permalink / raw)
  To: Sekhar Nori
  Cc: David Lechner, Kevin Hilman, Michael Turquette, Patrick Titiano,
	Laurent Pinchart, Rob Herring, Mark Rutland, Russell King,
	linux-devicetree, LKML, arm-soc

2017-02-21 5:49 GMT+01:00 Sekhar Nori <nsekhar@ti.com>:
> On Monday 20 February 2017 09:12 PM, Bartosz Golaszewski wrote:
>> 2017-02-20 11:29 GMT+01:00 Sekhar Nori <nsekhar@ti.com>:
>>> On Thursday 16 February 2017 11:45 PM, Bartosz Golaszewski wrote:
>>>> Add a new pinctrl sub-node for vpif display pins. Move VP_CLKIN3 and
>>>> VP_CLKIN2 to the display node where they actually belong (vide section
>>>> 35.2.2 of the da850 datasheet).
>>>
>>> You mean 36.2.2. Also, its in the technical reference manual (TRM). The
>>> datahseet is another document.
>>>
>>
>> I'm looking at the revision from September 2016 and it's 35.2.2: VPIF
>> -> Architecture -> signal descriptions.
>
> Is this the document ?
>
> http://www.ti.com/lit/ug/spruh77c/spruh77c.pdf
>
> In this VPIF is chapter 36.
>
> Thanks,
> Sekhar
>

-ETOOMANYPDFS I was looking at spruh82c.pdf.

You're right of course.

Thanks,
Bartosz

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/4] ARM: dts: da850: add vpif video display pins
  2017-02-21  9:23         ` Bartosz Golaszewski
@ 2017-02-22  5:59           ` Sekhar Nori
  0 siblings, 0 replies; 14+ messages in thread
From: Sekhar Nori @ 2017-02-22  5:59 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: David Lechner, Kevin Hilman, Michael Turquette, Patrick Titiano,
	Laurent Pinchart, Rob Herring, Mark Rutland, Russell King,
	linux-devicetree, LKML, arm-soc

On Tuesday 21 February 2017 02:53 PM, Bartosz Golaszewski wrote:
> 2017-02-21 5:49 GMT+01:00 Sekhar Nori <nsekhar@ti.com>:
>> On Monday 20 February 2017 09:12 PM, Bartosz Golaszewski wrote:
>>> 2017-02-20 11:29 GMT+01:00 Sekhar Nori <nsekhar@ti.com>:
>>>> On Thursday 16 February 2017 11:45 PM, Bartosz Golaszewski wrote:
>>>>> Add a new pinctrl sub-node for vpif display pins. Move VP_CLKIN3 and
>>>>> VP_CLKIN2 to the display node where they actually belong (vide section
>>>>> 35.2.2 of the da850 datasheet).
>>>>
>>>> You mean 36.2.2. Also, its in the technical reference manual (TRM). The
>>>> datahseet is another document.
>>>>
>>>
>>> I'm looking at the revision from September 2016 and it's 35.2.2: VPIF
>>> -> Architecture -> signal descriptions.
>>
>> Is this the document ?
>>
>> http://www.ti.com/lit/ug/spruh77c/spruh77c.pdf
>>
>> In this VPIF is chapter 36.
>>
>> Thanks,
>> Sekhar
>>
> 
> -ETOOMANYPDFS I was looking at spruh82c.pdf.
> 
> You're right of course.

I see. I guess its better to call it OMAP-L138 technical reference
manual (instead of da850) and also provide a link.

Thanks,
Sekhar

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2017-02-22  6:03 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-16 18:15 [PATCH 0/4] ARM: dts: da850-evm: vpif DT changes Bartosz Golaszewski
2017-02-16 18:15 ` [PATCH 1/4] ARM: dts: da850-evm: fix whitespace errors Bartosz Golaszewski
2017-02-16 18:15 ` [PATCH 2/4] ARM: dts: da850: add vpif video display pins Bartosz Golaszewski
2017-02-20 10:29   ` Sekhar Nori
2017-02-20 15:42     ` Bartosz Golaszewski
2017-02-21  4:49       ` Sekhar Nori
2017-02-21  9:23         ` Bartosz Golaszewski
2017-02-22  5:59           ` Sekhar Nori
2017-02-16 18:15 ` [PATCH 3/4] ARM: dts: da850-evm: add the output port to the vpif node Bartosz Golaszewski
2017-02-16 18:15 ` [PATCH 4/4] ARM: dts: da850-evm: add the UI expander node Bartosz Golaszewski
2017-02-20  9:36   ` Sekhar Nori
2017-02-20 15:38     ` Bartosz Golaszewski
2017-02-21  5:03       ` Sekhar Nori
2017-02-21  9:22         ` Bartosz Golaszewski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).