Re: [PATCH 1/2] media: dt-bindings: Add Intersil ISL7998x DT bindings

[Jacopo Mondi <jacopo@jmondi.org>]

[-- Attachment #1: Type: text/plain, Size: 6223 bytes --]

HI Marek,

On Wed, May 29, 2019 at 01:09:47PM +0200, Marek Vasut wrote:
> On 5/29/19 1:04 PM, Ian Arkver wrote:
> > Hi,
> >
> > On 29/05/2019 11:41, Marek Vasut wrote:
> >> On 5/29/19 8:28 AM, Jacopo Mondi wrote:
> >>
> >> [...]
> >>
> >>>>>>> [1]
> >>>>>>> https://www.renesas.com/eu/en/products/audio-video/video-decoders-encoders/video-decoders/device/ISL79987.html
> >>>>>>>
> >>>>>>>
> >>>>>>>> +Required Properties:
> >>>>>>>> +- compatible: value should be "isil,isl79987"
> >>>>>
> >>>>> And here you might want to have 2 different compatibles for 79987 and
> >>>>> 79988.
> >>>>
> >>>> The 79988 is not supported yet, do we want to have it in the binding
> >>>> doc?
> >>>>
> >>>
> >>> I got mislead by the isl7998x naming scheme you used...
> >>>
> >>> I would say that's up to you, the two chips seems very similar,
> >>> and it might make sense to provide bindings that support both. At the
> >>> same time, as long as the here defined bindings does not prevent
> >>> future expansions to include the ISL79988, its support could be safely
> >>> post-poned. In that case please s/isl7998x/isl79987/ in this document
> >>> and do not mention BT565 in the description.
> >>
> >> Right
> >>
> >>>> [...]
> >>>>
> >>>>>>> I see from the example you only support one output port? How do you
> >>>>>>> model the input ones.
> >>>>>>
> >>>>>> I don't . Do we model analog inputs now somehow ?
> >>>>>
> >>>>> I really think so, please see:
> >>>>> Documentation/devicetree/bindings/display/connector/analog-tv-connector.txt
> >>>>>
> >>>>>
> >>>>> And as an example of a board device tree using connectors to model
> >>>>> analog input see how the cvbs input on Salvator-X is described:
> >>>>>
> >>>>>     cvbs-in {
> >>>>>         compatible = "composite-video-connector";
> >>>>>         label = "CVBS IN";
> >>>>>
> >>>>>         port {
> >>>>>             cvbs_con: endpoint {
> >>>>>                 remote-endpoint = <&adv7482_ain7>;
> >>>>>             };
> >>>>>         };
> >>>>>     };
> >>>>>
> >>>>> I think you should provide 4 input ports, where to connect input from
> >>>>> the analog connectors, and derive the number of enabled inputs from
> >>>>> the number of endpoints connected to an active remote.
> >>>>
> >>>> Deriving the number of active physical inputs from some existing
> >>>> binding
> >>>> makes sense.
> >>>>
> >>>> However unlike the adv7482, the isl79987 does not support remapping the
> >>>> physical inputs to ADCs in the chip. It does support some remapping of
> >>>> physical inputs to MIPI CSI2 channels, but that's probably not very
> >>>> useful.
> >>>>
> >>>
> >>> I understand, but I will now use against you the argument you have
> >>> correctly pointed out here below that DT should describe hardware, and
> >>> the hardware has indeed 4 input ports..
> >>
> >> My question here is whether it makes sense to describe the ports even if
> >> they cannot be muxed to different ADC. Does it ?
> >
> > Each input port can be either differential CVBS or single ended with a
> > 2:1 input select mux. It would be nice to be able to describe this.
>
> Where do you see that ?
>
> > You cannot remap the inputs to different ADCs, but you can remap the
> > ADCs to different VC IDs using the
> > ISL7998x_REG_P5_LI_ENGINE_VC_ASSIGNMENT register. Describing each input
> > would proivde somewhere to specify the vc-id.
>
> I think Jacopo mentioned above the input muxing and the MIPI CSI2 VC
> muxing are two separate things. But I have to wonder, do we have a way
> of muxing the VCs in the DT or via the media controller yet ?

I'm not sure I get what you mean with "input muxing", do you mean
controlling which input is directed to which ADC ? I don't have the
chip manual, but according to what you and Ian said this is not
possible.

Selecting which input video stream is directed to which CSI-2 virtual
channel in the output CSI-2 stream is not possible in mainline. There
are patches in development that would allow you to do so, but their
design is not fully finalized yet. They would, in any case, require
you to have one sink pad for each input port, and while you could
register as many pads as it is specified in your custom property,
you would loose the notion of which input is connected to which port
ie. when a single input (isil,num-inputs=1) is connected to an input
port which is not the first one (anyway, quickly looking at 2/2 it
seems to me you only register a single source pad for this device).

The DT bindings layout is an orthogonal problem though. My opinion is
that as the chip has 4 available input connections it should have 4
input ports in the bindings definition, and for each one you would
register a sink pad. Only the actually connected ones should be present
in the DTS, so that the driver knows which input port is active and,
once something that allows you to control VC will land in mainline,
it will let you tell something like "I want the video stream
received on the input port@2 sent in virtual channel x in the output
CSI-2 stream", but again this is quite an orthogonal issue. Sure thing
is that with the current design and implementation, which afaict does
not provides any sink pad, this is frowned upon (but you can then say
'who cares, since it's not here yet' :)

Or what Ian said, if you can model a connector that provides 2 single
ended inputs, each connected to an input port and muxed internally by
the chip, you could only do that if you have numbered input ports I
guess. Nobody requires you support any of these, but at least bindings
should be defined in a future proof way, to avoid later changes to the
ABI.

Anyway, you had my opinion, multiple times already, and as I'm not in
any position to judge if something is acceptable for merge or not,
I'll now get quiet hoping that this prolonged email exchange drags in
opinion from more experienced people to tell you which direction you
should take and if what you have here is fine already or not :)

Cheers
   j

>
> --
> Best regards,
> Marek Vasut

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]