Re: [PATCH 20/22] [media] tvp5150: Add input port connectors DT bindings

[Marco Felsch <m.felsch@pengutronix.de>]

Hi Mauro,

On 18-08-03 14:30, Mauro Carvalho Chehab wrote:
> Em Fri, 3 Aug 2018 09:29:53 +0200
> Marco Felsch <m.felsch@pengutronix.de> escreveu:
> 
> > Hi Rob,
> > 
> > first of all, thanks for the review. After some discussion with the
> > media guys I have a question about the dt-bindings.
> > 
> > On 18-07-03 17:23, Rob Herring wrote:
> > > On Thu, Jun 28, 2018 at 06:20:52PM +0200, Marco Felsch wrote:  
> > > > The TVP5150/1 decoders support different video input sources to their
> > > > AIP1A/B pins.
> > > > 
> > > > Possible configurations are as follows:
> > > >   - Analog Composite signal connected to AIP1A.
> > > >   - Analog Composite signal connected to AIP1B.
> > > >   - Analog S-Video Y (luminance) and C (chrominance)
> > > >     signals connected to AIP1A and AIP1B respectively.
> > > > 
> > > > This patch extends the device tree bindings documentation to describe
> > > > how the input connectors for these devices should be defined in a DT.
> > > > 
> > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > > ---
> > > >  .../devicetree/bindings/media/i2c/tvp5150.txt | 118 +++++++++++++++++-
> > > >  1 file changed, 113 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/media/i2c/tvp5150.txt b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
> > > > index 8c0fc1a26bf0..feed8c911c5e 100644
> > > > --- a/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
> > > > +++ b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
> > > > @@ -12,11 +12,23 @@ Optional Properties:
> > > >  - pdn-gpios: phandle for the GPIO connected to the PDN pin, if any.
> > > >  - reset-gpios: phandle for the GPIO connected to the RESETB pin, if any.
> > > >  
> > > > -The device node must contain one 'port' child node for its digital output
> > > > -video port, in accordance with the video interface bindings defined in
> > > > -Documentation/devicetree/bindings/media/video-interfaces.txt.
> > > > +The device node must contain one 'port' child node per device input and output
> > > > +port, in accordance with the video interface bindings defined in
> > > > +Documentation/devicetree/bindings/media/video-interfaces.txt. The port nodes
> > > > +are numbered as follows
> > > >  
> > > > -Required Endpoint Properties for parallel synchronization:
> > > > +	  Name		Type		Port
> > > > +	--------------------------------------
> > > > +	  AIP1A		sink		0
> > > > +	  AIP1B		sink		1
> > > > +	  S-VIDEO	sink		2
> > > > +	  Y-OUT		src		3  
> > 
> > Do you think it's correct to have a seperate port for each binding?
> > Since the S-Video port is a combination of AIP1A and AIP1B. After a
> > discussion with Mauro [1] the TVP5150 should have only 3 pads. Since the
> > pads are directly mappped to the dt-ports this will correspond to three
> > ports (2 in, 1 out). Now the svideo connector will be mapped to a second
> > endpoint in each port:
> > 
> > port@0			
> > 	endpoint@0 -----------> Comp0-Con
> > 	endpoint@1 -----+-----> Svideo-Con
> > port@1		|
> > 	endpoint@0 -----|-----> Comp1-Con
> > 	endpoint@1 -----+
> > port@2
> > 	endpoint
> 
> For tvp5150, the model is like the above, so just one port at the
> S-video connector.

Okay, but this won't work. I tested it yesterday. Since the svideo port
is linked to two endpoints and the DTC will throw an error
"ERROR (duplicate_label)". Which make sens, but I covered it to late...

So we have two opportunities: 1) the one you described below or 2) we
map only port@0/port@1 to the svideo connector.

Anyway, should endpoint@1 always refer to the svideo connector or should
it be independent (in case of 1xcomp-con and 1xsvideo-con)?

> 
> Yet, for more complex devices that would allow switching the
> endpoints at the Svideo connector, the model would be:
> 
> port@0			
> 	endpoint@0 (AIP1A) -----------> Comp0-Con
> 	endpoint@1 (AIP1B) -----------> Svideo-Con  port@0 (luminance)
> port@1
> 	endpoint@0 (AIP1A) -----------> Comp1-Con
> 	endpoint@1 (AIP1B) -----------> Svideo-Con  port@1 (chrominance)
> port@2
> 	endpoint   (video bitstream output)
> 
> E. g. the S-Video connector will also have two ports, one for the
> chrominance signal and another one for the luminance one.
> 
> > I don't like that solution that much, since the mapping is now signal
> > based. We also don't map each line of a parallel port.
> > 
> > A quick grep shows that currently each device using a svideo connector
> > seperates them in a own port as I did.
> 
> No. I've no idea about how you did the grep, but this is not how other
> drivers handle it currently.

Sorry for that. My grep only covered the dt related connectors and not the
connectors within the mc framework.

> 
> Right now, on all hardware where connectors are mapped, there is just one
> input port and multiple connectors linked to it. You can see some examples
> here:
> 
> 	https://www.infradead.org/~mchehab/mc-next-gen/au0828_hvr950q.png
> 	https://www.infradead.org/~mchehab/mc-next-gen/cx231xx_hvr930c_hd.png
> 	https://www.infradead.org/~mchehab/mc-next-gen/em28xx_hvr950.png
> 	https://www.infradead.org/~mchehab/mc-next-gen/playtv_usb.png
> 	https://www.infradead.org/~mchehab/mc-next-gen/saa7134-asus-p7131-dual.png
> 	https://www.infradead.org/~mchehab/mc-next-gen/wintv_usb2.png
> 
> The problem with this approach is that it doesn't reflect how the
> hardware is actually wired. On some hardware similar to tvp5150,
> it may be possible, for example, to wire the S-Video both ways,
> e. g., something equivalent to (using tvp5150 terminology):
> 
> 	Luminance   -> AIP1A
> 	Chrominance -> AIP1B
> or
> 	Luminance   -> AIP1B
> 	Chrominance -> AIP1A
> 
> So, just one pad wouldn't allow this kind of config.
> 
> Having three pads is equally wrong, as there's no S-Video port on
> tvp5150. All it has physically are two inputs: AIP1A and AIP1B.
> 
> If you want to see the discussions we had, they are at:
> 	https://linuxtv.org/irc/irclogger_log/media-maint?date=2018-08-02,Thu
> 
> I'm preparing right now a summary of them. will copy you once I
> finish it.

I read the channel and your RFC patch, thanks for the discussion. One
thing I missed is to enable the svideo-link. I think hans wanted to
enable the links seperatly, but what about the "easy" use-case (no
chroma/luma seperation). If the user wants to enable svideo,
this pad should be linked to both tvp5150 input-pads and both links
should be enabled simultaneous. Should this be handeld by a pad flag?

Thanks,
Marco

> 
> > IMHO this is a uncomplicate
> > solution, but don't abstract the HW correctly. What is your opinion
> > about that? Is it correct to have seperate (virtual) port or should I
> > map the svideo connector as shown above?
> > 
> > [1] https://www.spinics.net/lists/devicetree/msg242825.html
> 
> Anyway, I'm writing a summary of our discussions 
> 
> Thanks,
> Mauro
>