Re: [PATCH v2 3/4] dt-bindings: drm/bridge: ti-sn65dsi83: Add vcc supply bindings

[Maxime Ripard <maxime@cerno.tech>]

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

Hi

On Mon, Oct 18, 2021 at 08:48:52PM +0300, Laurent Pinchart wrote:
> Hi Maxime,
> 
> On Mon, Oct 18, 2021 at 05:20:13PM +0200, Maxime Ripard wrote:
> > On Sat, Oct 16, 2021 at 05:34:59AM +0300, Laurent Pinchart wrote:
> > > On Thu, Oct 14, 2021 at 09:41:10AM +0200, Maxime Ripard wrote:
> > > > On Wed, Oct 13, 2021 at 12:37:47PM +0300, Laurent Pinchart wrote:
> > > > > On Wed, Oct 13, 2021 at 09:47:22AM +0200, Maxime Ripard wrote:
> > > > > > On Tue, Oct 12, 2021 at 08:48:42AM +0200, Alexander Stein wrote:
> > > > > > > Add a VCC regulator which needs to be enabled before the EN pin is
> > > > > > > released.
> > > > > > > 
> > > > > > > Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> > > > > > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > > > > > > ---
> > > > > > >  .../devicetree/bindings/display/bridge/ti,sn65dsi83.yaml     | 5 +++++
> > > > > > >  1 file changed, 5 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
> > > > > > > index a5779bf17849..49ace6f312d5 100644
> > > > > > > --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
> > > > > > > +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml
> > > > > > > @@ -32,6 +32,9 @@ properties:
> > > > > > >      maxItems: 1
> > > > > > >      description: GPIO specifier for bridge_en pin (active high).
> > > > > > >  
> > > > > > > +  vcc-supply:
> > > > > > > +    description: A 1.8V power supply (see regulator/regulator.yaml).
> > > > > > > +
> > > > > > >    ports:
> > > > > > >      $ref: /schemas/graph.yaml#/properties/ports
> > > > > > >  
> > > > > > > @@ -93,6 +96,7 @@ properties:
> > > > > > >  required:
> > > > > > >    - compatible
> > > > > > >    - reg
> > > > > > > +  - vcc-supply
> > > > > > 
> > > > > > This isn't a backward-compatible change. All the previous users of that
> > > > > > binding will now require a vcc-supply property even though it was
> > > > > > working fine for them before.
> > > > > > 
> > > > > > You handle that nicely in the code, but you can't make that new property
> > > > > > required.
> > > > > 
> > > > > We can't make it required in the driver, but can't we make it required
> > > > > in the bindings ? This indicates that all new DTs need to set the
> > > > > property. We also need to mass-patch the in-tree DTs to avoid validation
> > > > > failures, but apart from that, I don't see any issue.
> > > > 
> > > > I guess we'd need to clarify what the schemas are here for.
> > > > 
> > > > We've been using them for two things: define the bindings, and make
> > > > sure that the users of a binding actually follow it.
> > > > 
> > > > The second part makes it very tempting to also cram "and make sure they
> > > > follow our best practices" in there. We never had the discussion about
> > > > whether that's ok or not, and I think the schemas syntax falls a bit
> > > > short there since I don't think we can make the difference between a
> > > > warning and an error that would make it work.
> > > > 
> > > > However, if we're talking about the binding itself, then no, you can't
> > > > introduce a new property.
> > > 
> > > I assume you mean "a new required property" here.
> > > 
> > > > Since it was acceptable in the past, it still needs to be acceptable
> > > > going forward.
> > > 
> > > I think that's a matter of definition. The way I see it (but I could be
> > > mistaken), we're essentially versioning DT bindings without actually
> > > saying so, with the latest version being the visible one, and drivers
> > > having to preserve backward compatibility with new versions. We could
> > > also see it in different ways of course.
> > 
> > I disagree. A binding is essentially a contract on how the OS is
> > supposed to interpret whatever comes from the DT. If we do what you
> > suggest, then we lose all documentation of older versions we still need
> > to support at the OS level. And relying on all developers to look
> > through the entire history to figure it out is a sure way to screw
> > things up :)
> > 
> > The use of deprecated indicates that we actually want to document the
> > old versions.
> > 
> > > What's important is in my opinion to make sure that new DTs will do
> > > the right thing, and if we don't make this property required, we can't
> > > check that. DT authors wouldn't know if the property is optional due
> > > to backward compatibility only but highly recommended, or really
> > > optional.
> > 
> > Add a comment saying that this should really be added, but we can't
> > because it was missing it was in the original binding?
> 
> That will not help validating that new DTs are compliant with the last
> version of the bindings.
> 
> We have one tool, and two needs. The tool should be extended to cover
> both, but today it can only support one. Which of these two is the most
> important:
> 
> - Documentating old behaviour, to helper driver authors on other
>   operating systems implement backward compatibility without having to
>   look at the history ?
> 
> - Validating all new device trees to ensure they implement the latest
>   recommended version of the bindings ?
> 
> I think the second one is much more frequent, and is also where most of
> the issues will arise.

I understand the drive for the latter, but we shouldn't be dropping the
former in the process, which has been what we've been doing for the last
decade or so.

Maxime

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