Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1582815 > unrolled thread
| Started by | Bartosz Golaszewski <bgolaszewski@baylibre.com> |
|---|---|
| First post | 2017-02-16 19:20 +0100 |
| Last post | 2017-02-21 10:30 +0100 |
| Articles | 7 — 2 participants |
Back to article view | Back to linux.kernel
[PATCH 0/4] ARM: dts: da850-evm: vpif DT changes Bartosz Golaszewski <bgolaszewski@baylibre.com> - 2017-02-16 19:20 +0100
[PATCH 3/4] ARM: dts: da850-evm: add the output port to the vpif node Bartosz Golaszewski <bgolaszewski@baylibre.com> - 2017-02-16 19:20 +0100
[PATCH 4/4] ARM: dts: da850-evm: add the UI expander node Bartosz Golaszewski <bgolaszewski@baylibre.com> - 2017-02-16 19:20 +0100
Re: [PATCH 4/4] ARM: dts: da850-evm: add the UI expander node Sekhar Nori <nsekhar@ti.com> - 2017-02-20 10:40 +0100
Re: [PATCH 4/4] ARM: dts: da850-evm: add the UI expander node Bartosz Golaszewski <bgolaszewski@baylibre.com> - 2017-02-20 16:40 +0100
Re: [PATCH 4/4] ARM: dts: da850-evm: add the UI expander node Sekhar Nori <nsekhar@ti.com> - 2017-02-21 06:10 +0100
Re: [PATCH 4/4] ARM: dts: da850-evm: add the UI expander node Bartosz Golaszewski <bgolaszewski@baylibre.com> - 2017-02-21 10:30 +0100
| From | Bartosz Golaszewski <bgolaszewski@baylibre.com> |
|---|---|
| Date | 2017-02-16 19:20 +0100 |
| Subject | [PATCH 0/4] ARM: dts: da850-evm: vpif DT changes |
| Message-ID | <tbyOS-1GL-9@gated-at.bofh.it> |
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
[toc] | [next] | [standalone]
| From | Bartosz Golaszewski <bgolaszewski@baylibre.com> |
|---|---|
| Date | 2017-02-16 19:20 +0100 |
| Subject | [PATCH 3/4] ARM: dts: da850-evm: add the output port to the vpif node |
| Message-ID | <tbyOS-1GL-25@gated-at.bofh.it> |
| In reply to | #1582815 |
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
[toc] | [prev] | [next] | [standalone]
| From | Bartosz Golaszewski <bgolaszewski@baylibre.com> |
|---|---|
| Date | 2017-02-16 19:20 +0100 |
| Subject | [PATCH 4/4] ARM: dts: da850-evm: add the UI expander node |
| Message-ID | <tbyOT-1GL-47@gated-at.bofh.it> |
| In reply to | #1582815 |
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
[toc] | [prev] | [next] | [standalone]
| From | Sekhar Nori <nsekhar@ti.com> |
|---|---|
| Date | 2017-02-20 10:40 +0100 |
| Subject | Re: [PATCH 4/4] ARM: dts: da850-evm: add the UI expander node |
| Message-ID | <tcSBQ-2Sv-29@gated-at.bofh.it> |
| In reply to | #1582824 |
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
[toc] | [prev] | [next] | [standalone]
| From | Bartosz Golaszewski <bgolaszewski@baylibre.com> |
|---|---|
| Date | 2017-02-20 16:40 +0100 |
| Subject | Re: [PATCH 4/4] ARM: dts: da850-evm: add the UI expander node |
| Message-ID | <tcYee-6r1-33@gated-at.bofh.it> |
| In reply to | #1584442 |
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
[toc] | [prev] | [next] | [standalone]
| From | Sekhar Nori <nsekhar@ti.com> |
|---|---|
| Date | 2017-02-21 06:10 +0100 |
| Subject | Re: [PATCH 4/4] ARM: dts: da850-evm: add the UI expander node |
| Message-ID | <tdaS5-6kB-7@gated-at.bofh.it> |
| In reply to | #1584684 |
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
[toc] | [prev] | [next] | [standalone]
| From | Bartosz Golaszewski <bgolaszewski@baylibre.com> |
|---|---|
| Date | 2017-02-21 10:30 +0100 |
| Subject | Re: [PATCH 4/4] ARM: dts: da850-evm: add the UI expander node |
| Message-ID | <tdeVH-B8-9@gated-at.bofh.it> |
| In reply to | #1585043 |
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
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web