[PATCH 0/4] ARM: dts: da850-evm: vpif DT changes

[PATCH 0/4] ARM: dts: da850-evm: vpif DT changes

[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

[PATCH 1/4] ARM: dts: da850-evm: fix whitespace errors

[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

[PATCH 2/4] ARM: dts: da850: add vpif video display pins

[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

Re: [PATCH 2/4] ARM: dts: da850: add vpif video display pins

[Sekhar Nori]

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-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> ---
>  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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] ARM: dts: da850: add vpif video display pins

[Bartosz Golaszewski]

2017-02-20 11:29 GMT+01:00 Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org>:
> 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] ARM: dts: da850: add vpif video display pins

[Sekhar Nori]

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

Re: [PATCH 2/4] ARM: dts: da850: add vpif video display pins

[Bartosz Golaszewski]

2017-02-21 5:49 GMT+01:00 Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org>:
> On Monday 20 February 2017 09:12 PM, Bartosz Golaszewski wrote:
>> 2017-02-20 11:29 GMT+01:00 Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org>:
>>> 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] ARM: dts: da850: add vpif video display pins

[Sekhar Nori]

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

[PATCH 3/4] ARM: dts: da850-evm: add the output port to the vpif node

[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

[PATCH 4/4] ARM: dts: da850-evm: add the UI expander node

[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-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] ARM: dts: da850-evm: add the UI expander node

[Sekhar Nori]

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

Re: [PATCH 4/4] ARM: dts: da850-evm: add the UI expander node

[Bartosz Golaszewski]

2017-02-20 10:36 GMT+01:00 Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org>:
> 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-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>> ---

[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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] ARM: dts: da850-evm: add the UI expander node

[Sekhar Nori]

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

Re: [PATCH 4/4] ARM: dts: da850-evm: add the UI expander node

[Bartosz Golaszewski]

2017-02-21 6:03 GMT+01:00 Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org>:
> On Monday 20 February 2017 09:08 PM, Bartosz Golaszewski wrote:
>> 2017-02-20 10:36 GMT+01:00 Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org>:
>>> 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-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>>>> ---
>>
>> [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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html