Re: [PATCH v4 6/6] dt-bindings: media: rcar-vin: Clarify optional props

["Niklas Söderlund" <niklas.soderlund@ragnatech.se>]

Hi Jacopo,

Thanks for not giving up on this work :-)

On 2018-07-03 21:52:38 +0200, Jacopo Mondi wrote:
> Hi Laurent, Niklas,
> 
> On Mon, Jul 02, 2018 at 10:19:25AM +0300, Laurent Pinchart wrote:
> > Hi Niklas,
> >
> > On Wednesday, 27 June 2018 08:24:31 EEST Niklas Söderlund wrote:
> > > On 2018-06-13 10:54:55 +0200, Jacopo Mondi wrote:
> > > > On Tue, Jun 12, 2018 at 06:45:53PM +0300, Sakari Ailus wrote:
> > > >> On Tue, Jun 12, 2018 at 04:26:06PM +0200, Jacopo Mondi wrote:
> > > >>> Add a note to the R-Car VIN interface bindings to clarify that all
> > > >>> properties listed as generic properties in video-interfaces.txt can
> > > >>> be included in port@0 endpoint, but if not explicitly listed in the
> > > >>> interface bindings documentation, they do not modify it behaviour.
> > > >>>
> > > >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > >>> ---
> > > >>>
> > > >>>  Documentation/devicetree/bindings/media/rcar_vin.txt | 6 ++++++
> > > >>>  1 file changed, 6 insertions(+)
> > > >>>
> > > >>> diff --git a/Documentation/devicetree/bindings/media/rcar_vin.txt
> > > >>> b/Documentation/devicetree/bindings/media/rcar_vin.txt index
> > > >>> 8130849..03544c7 100644
> > > >>> --- a/Documentation/devicetree/bindings/media/rcar_vin.txt
> > > >>> +++ b/Documentation/devicetree/bindings/media/rcar_vin.txt
> > > >>> @@ -55,6 +55,12 @@ from local SoC CSI-2 receivers (port1) depending on
> > > >>> SoC.
> > > >>>
> > > >>>        instances that are connected to external pins should have port
> > > >>>        0.
> > > >>>
> > > >>>        - Optional properties for endpoint nodes of port@0:
> > > >>> +
> > > >>> +        All properties described in [1] and which apply to the
> > > >>> selected
> > > >>> +        media bus type could be optionally listed here to better
> > > >>> describe
> > > >>> +        the current hardware configuration, but only the following
> > > >>> ones do
> > > >>> +        actually modify the VIN interface behaviour:
> > > >>> +
> > >
> > > I'm not sure the description have to be as explicit to that the
> > > properties in 'video-interfaces.txt' are not currently used by the
> > > driver. I find it not relevant which ones are used or not, what is
> > > important for me is that all properties in 'video-interfaces.txt' which
> > > can be used to describe the specific bus are valid for the DT
> > > description.
> >
> > I agree with you. The driver is irrelevant in this context. What matters is
> > which properties are applicable to the bus. For instance, if the VIN parallel
> > input supports configurable polarities for the h/v sync signals, hsync-active
> > and vsync-active should be listed in the bindings. On the other hand, if the
> > polarities are fixed, then the properties are not needed.
> >
> 
> Fine, I'll surrender then, you're too many to fight against :p
> 
> Joking aside, I see your points, and I'll resend dropping this last
> bit and documenting which properties applies to the interface.
> 
> I can list:
> - vsync-active (added by this series)
> - hsync-active (added by this series)
> - data-enable-active (added by this series)
> - field-active-even (to be added)
> 
> I haven't find a way to configure the plck-sample value in the VINs
> registers. Should we skip it?

IIRC my findings agree with yours, no register to control plck-sample in 
the VIN register set.

> 
> I also think 'bus-width' and 'data-shift' are not directly configurable in
> registers but depends on SoC, the selected input interface, and the
> input image format (see tables at 26.3.1 datasheet revision 1.00), so
> it makes sense list them as acceptable properties.
> 
> Do I have your ack on this so I can re-spin?

I think I understand your plan and I think it sounds ok, please try it 
:-)

> 
> > > On a side note, in rcar_vin.txt we have this section describing the Gen2
> > > bindings:
> > >
> > >   The per-board settings Gen2 platforms:
> > >    - port sub-node describing a single endpoint connected to the vin
> > >      as described in video-interfaces.txt[1]. Only the first one will
> > >      be considered as each vin interface has one input port.
> > >
> > > This whole series only deals with documenting the Gen3 optional
> > > properties and not the Gen2. Maybe with parallel input support for Gen3
> > > patches on there way to making it upstream this series should be
> > > extended to in a good way merge the port@0 optional properties for both
> > > generations of hardware?
> >
> > That would be nice too :-)
> 
> I see..
> 
> Well, I would keep the Gen2/Gen3 sections separate, as they have a
> different layout of port nodes (Gen2 has a single 'port' node, while Gen3 could
> have several 'port' nodes enclosed in a 'ports' one).
> 
> But I could indeed reference the optional endpoint properties of
> 'port@0' of Gen3 bindings in Gen2 ones. Or the other way around. Do
> you have any preference?

My preference would be to list the properties in the Gen2 section and in 
the Gen3 port@0 section refer to the Gen2 optional properties.

> 
> Thanks
>    j
> 
> 
> >
> > > >> I don't think this should be needed. You should only have properties
> > > >> that describe the hardware configuration in a given system.
> > > >
> > > > There has been quite some debate on this, and please bear with me
> > > > here for re-proposing it: I started by removing properties in some DT
> > > > files for older Renesas board which listed endpoint properties not
> > > > documented in the VIN's bindings and not parsed by the VIN driver [1]
> > > > Niklas (but Simon and Geert seems to agree here) opposed to that
> > > > patch, as those properties where described in 'video-interfaces.txt' and
> > > > even if not parsed by the current driver implementation, they actually
> > > > describe hardware. I rebated that only properties listed in the device
> > > > bindings documentation should actually be used, and having properties
> > > > not parsed by the driver confuses users, which may expect changing
> > > > them modifies the interface configuration, which does not happens at
> > > > the moment.
> > > >
> > > > This came out as a middle ground from a discussion with Niklas. As
> > > > stated in the cover letter if this patch makes someone uncomfortable, feel
> > > > free to drop it not to hold back the rest of the series which has been
> > > > well received instead.
> > >
> > > What I don't agree with and sparked this debate from my side was the
> > > deletion of properties in dts files which correctly does describe the
> > > bus but which are not currently parsed by the driver. To me that is
> > > decreasing the value of the dts. If on the other hand the goal is to
> > > deprecate a property from the video-interfaces.txt by slowly removing
> > > them from dts where the driver don't use them I'm all for it. But I
> > > don't think this is the case here right?
> >
> > I think you're right, I don't think that's the case.
> >
> > We should not remove properties from the dts files when they correctly
> > describe the hardware and have an added-value. On the other hand, if a
> > property correctly describes the hardware, but is constrained to a single
> > value due to hardware limitations, then we can omit it.
> >
> > > > [1] https://www.spinics.net/lists/arm-kernel/msg656302.html
> > > >
> > > >>> - hsync-active: see [1] for description. Default is active high.
> > > >>> - vsync-active: see [1] for description. Default is active high.
> > > >>> - data-enable-active: polarity of CLKENB signal, see [1] for
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
> >
> >
> >



-- 
Regards,
Niklas Söderlund