| Message ID | 20161122194551.3420-3-khilman@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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>; > + status = "disabled"; > + }; > + > + vpif_capture: video-capture@0x00217000 { Again, @217000. But it seems odd to have two device nodes with the same address. Is enabling these mutually exclusive? > + compatible = "ti,da850-vpif-capture"; > + reg = <0x00217000 0x1000>; Ditto on the leading 0's. > + interrupts = <92>; > + status = "disabled"; > + > + /* VPIF capture: input channels */ > + port { > + vpif_ch0: endpoint@0 { > + reg = <0>; > + bus-width = <8>; > + }; > + > + vpif_ch1: endpoint@1 { > + reg = <1>; > + bus-width = <8>; > + data-shift = <8>; > + }; > + }; > + }; > }; > + > aemif: aemif@68000000 { > compatible = "ti,da850-aemif"; > #address-cells = <2>; >
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@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@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. >> + compatible = "ti,da850-vpif-capture"; >> + reg = <0x00217000 0x1000>; > > Ditto on the leading 0's. > Thanks for the review, Kevin
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@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@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. It should look something like this, I think: vpif: video@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@0 { reg = <0>; bus-width = <8>; }; vpif_ch1: endpoint@1 { reg = <1>; bus-width = <8>; data-shift = <8>; }; }; }; vpif_display: video-display { compatible = "ti,da850-vpif-display" port { vpif_ch2: endpoint@2 { reg = <2>; bus-width = <8>; data-shift = <16>; }; vpif_ch3: endpoint@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. 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. Thanks, Sekhar
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@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@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@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@0 { > reg = <0>; > bus-width = <8>; > }; > > vpif_ch1: endpoint@1 { > reg = <1>; > bus-width = <8>; > data-shift = <8>; > }; > }; > }; > > vpif_display: video-display { > compatible = "ti,da850-vpif-display" > port { > vpif_ch2: endpoint@2 { > reg = <2>; > bus-width = <8>; > data-shift = <16>; > }; > > vpif_ch3: endpoint@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
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@0x00217000 { + compatible = "ti,da850-vpif"; + reg = <0x00217000 0x1000>; + status = "disabled"; + }; + + vpif_capture: video-capture@0x00217000 { + compatible = "ti,da850-vpif-capture"; + reg = <0x00217000 0x1000>; + interrupts = <92>; + status = "disabled"; + + /* VPIF capture: input channels */ + port { + vpif_ch0: endpoint@0 { + reg = <0>; + bus-width = <8>; + }; + + vpif_ch1: endpoint@1 { + reg = <1>; + bus-width = <8>; + data-shift = <8>; + }; + }; + }; }; + aemif: aemif@68000000 { compatible = "ti,da850-aemif"; #address-cells = <2>;
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(+)