[PATCH 2/4] ARM: dts: davinci: da850: add VPIF

[khilman@baylibre.com (Kevin Hilman)]

Sekhar Nori <nsekhar@ti.com> writes:

> On Wednesday 23 November 2016 11:13 AM, Kevin Hilman wrote:
>> David Lechner <david@lechnology.com> writes:
>> 
>>> On 11/22/2016 01:45 PM, Kevin Hilman wrote:
>>>> Add VPIF and VPIF capture nodes to da850.  VPIF capture has two input
>>>> channels describe using the standard DT ports and enpoints.
>>>>
>>>> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
>>>> ---
>>>>  arch/arm/boot/dts/da850.dtsi | 28 ++++++++++++++++++++++++++++
>>>>  1 file changed, 28 insertions(+)
>>>>
>>>> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
>>>> index 6205917b4f59..e05e2bb834e8 100644
>>>> --- a/arch/arm/boot/dts/da850.dtsi
>>>> +++ b/arch/arm/boot/dts/da850.dtsi
>>>> @@ -453,7 +453,35 @@
>>>>  			interrupts = <52>;
>>>>  			status = "disabled";
>>>>  		};
>>>> +
>>>> +		vpif: video at 0x00217000 {
>>>
>>> Should be @217000
>>>
>>>> +			compatible = "ti,da850-vpif";
>>>> +			reg = <0x00217000 0x1000>;
>>>
>>> Could omit leading 0's to be consistent with existing entries.
>>>
>>> 	reg = <0x217000 0x1000>;
>> 
>> Ugh, yeah. I hate that convention, but better to be consistent, I guess.
>> 
>>>> +			status = "disabled";
>>>> +		};
>>>> +
>>>> +		vpif_capture: video-capture at 0x00217000 {
>>>
>>> Again, @217000. But it seems odd to have two device nodes with the
>>> same address. Is enabling these mutually exclusive?
>> 
>> They're not mutually exclusive because the vpif is the one that actually
>> maps the register range (since it's shared between vpif_display and
>> vpif_capture) so I guess I should just drop the reg property from the
>> vpif_capture node.
>
> Reading the documentation, VPIF is presented as a single device, with
> two channels dedicated to display and two for capture. Most of the
> channel registers are independent, but there are are some like interrupt
> enable which are common for all channels. So I believe we cannot use
> simple-mfd. But I believe VPIF display and capture should be subdevices
> of a single VPIF device.

OK, I can give it a try that way.

> It should look something like this, I think:
>
> 		vpif: video at 217000 {
> 			compatible = "ti,da850-vpif";
> 			reg = <0x217000 0x1000>;
> 			interrupts = <92>;
> 			status = "disabled";
>
> 			vpif_capture: video-capture {
> 				compatible = "ti,da850-vpif-capture"
> 				port {
> 					vpif_ch0: endpoint at 0 {
> 						  reg = <0>;
> 						  bus-width = <8>;
> 					};
> 	
> 					vpif_ch1: endpoint at 1 {
> 						  reg = <1>;
> 						  bus-width = <8>;
> 						  data-shift = <8>;
> 					};
> 				};
> 			};
>
> 			vpif_display: video-display {
> 				compatible = "ti,da850-vpif-display"
> 				port {
> 					vpif_ch2: endpoint at 2 {
> 						  reg = <2>;
> 						  bus-width = <8>;
> 						  data-shift = <16>;
> 					};
>
> 					vpif_ch3: endpoint at 3 {
> 						  reg = <3>;
> 						  bus-width = <8>;
> 						  data-shift = <24>;
> 					};
> 				};
> 			};
> 		};

> The interrupt too, seems to be common interrupt for both display and
> capture. So, it should not be under the capture node.

Possibly.  That will require reworking of the way the driver works
today, since the vpif driver doesn't request interrupts, but the capture
and display drivers both register interrupts, one per channel.  I'll
have a closer look.

> BTW, I am sure
> what exactly data-shift is used for. It does not seem to be used in the
> driver patches too. I just extrapolated the values based on the pattern
> I saw.

For video out, the way I read the spec, and based on the schematics,
there appears to be a separate 16-bit parallel bus, so the data-shift on
the video-display endpoints should probably be zero and 16 like for
catpure.  Anyways, I'll get to that when I get to the display part.

Kevin