* [PATCH v2 0/4] ti-sn65dsi83 patches @ 2021-10-12 6:48 Alexander Stein 2021-10-12 6:48 ` [PATCH v2 1/4] dt-bindings: display: bridge: sn65dsi83: Make enable GPIO optional Alexander Stein ` (3 more replies) 0 siblings, 4 replies; 16+ messages in thread From: Alexander Stein @ 2021-10-12 6:48 UTC (permalink / raw) To: David Airlie, Daniel Vetter, Rob Herring, Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman, Jernej Skrabec Cc: Alexander Stein, dri-devel, devicetree Changes in V2 of this set: * Add patch from Laurent for fixing the binding regarding optional GPIO * Reorder patches so bindings are changed beforehand * Add small fixes from Sam's review Alexander Stein (3): drm/bridge: ti-sn65dsi83: Make enable GPIO optional dt-bindings: drm/bridge: ti-sn65dsi83: Add vcc supply bindings drm/bridge: ti-sn65dsi83: Add vcc supply regulator support Laurent Pinchart (1): dt-bindings: display: bridge: sn65dsi83: Make enable GPIO optional .../bindings/display/bridge/ti,sn65dsi83.yaml | 6 +++++- drivers/gpu/drm/bridge/ti-sn65dsi83.c | 18 ++++++++++++++++-- 2 files changed, 21 insertions(+), 3 deletions(-) -- 2.25.1 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 1/4] dt-bindings: display: bridge: sn65dsi83: Make enable GPIO optional 2021-10-12 6:48 [PATCH v2 0/4] ti-sn65dsi83 patches Alexander Stein @ 2021-10-12 6:48 ` Alexander Stein 2021-10-12 6:48 ` [PATCH v2 2/4] drm/bridge: ti-sn65dsi83: " Alexander Stein ` (2 subsequent siblings) 3 siblings, 0 replies; 16+ messages in thread From: Alexander Stein @ 2021-10-12 6:48 UTC (permalink / raw) To: David Airlie, Daniel Vetter, Rob Herring, Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman, Jernej Skrabec Cc: Laurent Pinchart, dri-devel, devicetree, Alexander Stein From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> The SN65DSI8x EN signal may be tied to VCC, or otherwise controlled by means not available to the kernel. Make the GPIO optional. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> --- .../devicetree/bindings/display/bridge/ti,sn65dsi83.yaml | 1 - 1 file changed, 1 deletion(-) diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml index 07b20383cbca..a5779bf17849 100644 --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi83.yaml @@ -93,7 +93,6 @@ properties: required: - compatible - reg - - enable-gpios - ports allOf: -- 2.25.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 2/4] drm/bridge: ti-sn65dsi83: Make enable GPIO optional 2021-10-12 6:48 [PATCH v2 0/4] ti-sn65dsi83 patches Alexander Stein 2021-10-12 6:48 ` [PATCH v2 1/4] dt-bindings: display: bridge: sn65dsi83: Make enable GPIO optional Alexander Stein @ 2021-10-12 6:48 ` Alexander Stein 2021-10-12 6:48 ` [PATCH v2 3/4] dt-bindings: drm/bridge: ti-sn65dsi83: Add vcc supply bindings Alexander Stein 2021-10-12 6:48 ` [PATCH v2 4/4] drm/bridge: ti-sn65dsi83: Add vcc supply regulator support Alexander Stein 3 siblings, 0 replies; 16+ messages in thread From: Alexander Stein @ 2021-10-12 6:48 UTC (permalink / raw) To: David Airlie, Daniel Vetter, Rob Herring, Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman, Jernej Skrabec Cc: Alexander Stein, dri-devel, devicetree, Laurent Pinchart, Sam Ravnborg The enable signal may not be controllable by the kernel. Make it optional. This is a similar to commit bbda1704fc15 ("drm/bridge: ti-sn65dsi86: Make enable GPIO optional") Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Sam Ravnborg <sam@ravnborg.org> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> --- drivers/gpu/drm/bridge/ti-sn65dsi83.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c index a32f70bc68ea..9072342566f3 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c @@ -671,7 +671,8 @@ static int sn65dsi83_probe(struct i2c_client *client, model = id->driver_data; } - ctx->enable_gpio = devm_gpiod_get(ctx->dev, "enable", GPIOD_OUT_LOW); + ctx->enable_gpio = devm_gpiod_get_optional(ctx->dev, "enable", + GPIOD_OUT_LOW); if (IS_ERR(ctx->enable_gpio)) return PTR_ERR(ctx->enable_gpio); -- 2.25.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v2 3/4] dt-bindings: drm/bridge: ti-sn65dsi83: Add vcc supply bindings 2021-10-12 6:48 [PATCH v2 0/4] ti-sn65dsi83 patches Alexander Stein 2021-10-12 6:48 ` [PATCH v2 1/4] dt-bindings: display: bridge: sn65dsi83: Make enable GPIO optional Alexander Stein 2021-10-12 6:48 ` [PATCH v2 2/4] drm/bridge: ti-sn65dsi83: " Alexander Stein @ 2021-10-12 6:48 ` Alexander Stein 2021-10-13 7:47 ` Maxime Ripard 2021-10-12 6:48 ` [PATCH v2 4/4] drm/bridge: ti-sn65dsi83: Add vcc supply regulator support Alexander Stein 3 siblings, 1 reply; 16+ messages in thread From: Alexander Stein @ 2021-10-12 6:48 UTC (permalink / raw) To: David Airlie, Daniel Vetter, Rob Herring, Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman, Jernej Skrabec Cc: Alexander Stein, dri-devel, devicetree, Sam Ravnborg 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 - ports allOf: @@ -134,6 +138,7 @@ examples: reg = <0x2d>; enable-gpios = <&gpio2 1 GPIO_ACTIVE_HIGH>; + vcc-supply = <®_sn65dsi83_1v8>; ports { #address-cells = <1>; -- 2.25.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/4] dt-bindings: drm/bridge: ti-sn65dsi83: Add vcc supply bindings 2021-10-12 6:48 ` [PATCH v2 3/4] dt-bindings: drm/bridge: ti-sn65dsi83: Add vcc supply bindings Alexander Stein @ 2021-10-13 7:47 ` Maxime Ripard 2021-10-13 9:37 ` Laurent Pinchart 0 siblings, 1 reply; 16+ messages in thread From: Maxime Ripard @ 2021-10-13 7:47 UTC (permalink / raw) To: Alexander Stein Cc: David Airlie, Daniel Vetter, Rob Herring, Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman, Jernej Skrabec, dri-devel, devicetree, Sam Ravnborg [-- Attachment #1: Type: text/plain, Size: 1405 bytes --] Hi, 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. Maxime [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/4] dt-bindings: drm/bridge: ti-sn65dsi83: Add vcc supply bindings 2021-10-13 7:47 ` Maxime Ripard @ 2021-10-13 9:37 ` Laurent Pinchart 2021-10-14 7:41 ` Maxime Ripard 0 siblings, 1 reply; 16+ messages in thread From: Laurent Pinchart @ 2021-10-13 9:37 UTC (permalink / raw) To: Maxime Ripard Cc: Alexander Stein, David Airlie, Daniel Vetter, Rob Herring, Andrzej Hajda, Neil Armstrong, Robert Foss, Jonas Karlman, Jernej Skrabec, dri-devel, devicetree, Sam Ravnborg Hi Maxime, 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. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/4] dt-bindings: drm/bridge: ti-sn65dsi83: Add vcc supply bindings 2021-10-13 9:37 ` Laurent Pinchart @ 2021-10-14 7:41 ` Maxime Ripard 2021-10-16 2:34 ` Laurent Pinchart 0 siblings, 1 reply; 16+ messages in thread From: Maxime Ripard @ 2021-10-14 7:41 UTC (permalink / raw) To: Laurent Pinchart Cc: Alexander Stein, David Airlie, Daniel Vetter, Rob Herring, Andrzej Hajda, Neil Armstrong, Robert Foss, Jonas Karlman, Jernej Skrabec, dri-devel, devicetree, Sam Ravnborg [-- Attachment #1: Type: text/plain, Size: 2681 bytes --] On Wed, Oct 13, 2021 at 12:37:47PM +0300, Laurent Pinchart wrote: > Hi Maxime, > > 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. Since it was acceptable in the past, it still needs to be acceptable going forward. Maxime [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/4] dt-bindings: drm/bridge: ti-sn65dsi83: Add vcc supply bindings 2021-10-14 7:41 ` Maxime Ripard @ 2021-10-16 2:34 ` Laurent Pinchart 2021-10-18 15:20 ` Maxime Ripard 0 siblings, 1 reply; 16+ messages in thread From: Laurent Pinchart @ 2021-10-16 2:34 UTC (permalink / raw) To: Maxime Ripard Cc: Alexander Stein, David Airlie, Daniel Vetter, Rob Herring, Andrzej Hajda, Neil Armstrong, Robert Foss, Jonas Karlman, Jernej Skrabec, dri-devel, devicetree, Sam Ravnborg Hi Maxime, 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. 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. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/4] dt-bindings: drm/bridge: ti-sn65dsi83: Add vcc supply bindings 2021-10-16 2:34 ` Laurent Pinchart @ 2021-10-18 15:20 ` Maxime Ripard 2021-10-18 17:48 ` Laurent Pinchart 0 siblings, 1 reply; 16+ messages in thread From: Maxime Ripard @ 2021-10-18 15:20 UTC (permalink / raw) To: Laurent Pinchart Cc: Alexander Stein, David Airlie, Daniel Vetter, Rob Herring, Andrzej Hajda, Neil Armstrong, Robert Foss, Jonas Karlman, Jernej Skrabec, dri-devel, devicetree, Sam Ravnborg [-- Attachment #1: Type: text/plain, Size: 4309 bytes --] 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? Maxime [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/4] dt-bindings: drm/bridge: ti-sn65dsi83: Add vcc supply bindings 2021-10-18 15:20 ` Maxime Ripard @ 2021-10-18 17:48 ` Laurent Pinchart 2021-10-19 7:37 ` Maxime Ripard 0 siblings, 1 reply; 16+ messages in thread From: Laurent Pinchart @ 2021-10-18 17:48 UTC (permalink / raw) To: Maxime Ripard Cc: Alexander Stein, David Airlie, Daniel Vetter, Rob Herring, Andrzej Hajda, Neil Armstrong, Robert Foss, Jonas Karlman, Jernej Skrabec, dri-devel, devicetree, Sam Ravnborg 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. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/4] dt-bindings: drm/bridge: ti-sn65dsi83: Add vcc supply bindings 2021-10-18 17:48 ` Laurent Pinchart @ 2021-10-19 7:37 ` Maxime Ripard 2021-10-19 10:37 ` Laurent Pinchart 0 siblings, 1 reply; 16+ messages in thread From: Maxime Ripard @ 2021-10-19 7:37 UTC (permalink / raw) To: Laurent Pinchart Cc: Alexander Stein, David Airlie, Daniel Vetter, Rob Herring, Andrzej Hajda, Neil Armstrong, Robert Foss, Jonas Karlman, Jernej Skrabec, dri-devel, devicetree, Sam Ravnborg [-- 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 --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/4] dt-bindings: drm/bridge: ti-sn65dsi83: Add vcc supply bindings 2021-10-19 7:37 ` Maxime Ripard @ 2021-10-19 10:37 ` Laurent Pinchart 2021-10-19 19:39 ` Sam Ravnborg 0 siblings, 1 reply; 16+ messages in thread From: Laurent Pinchart @ 2021-10-19 10:37 UTC (permalink / raw) To: Maxime Ripard Cc: Alexander Stein, David Airlie, Daniel Vetter, Rob Herring, Andrzej Hajda, Neil Armstrong, Robert Foss, Jonas Karlman, Jernej Skrabec, dri-devel, devicetree, Sam Ravnborg Hi Maxime, On Tue, Oct 19, 2021 at 09:37:28AM +0200, Maxime Ripard wrote: > On Mon, Oct 18, 2021 at 08:48:52PM +0300, Laurent Pinchart wrote: > > 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. That point is debatable :-) I've repeatedly asked during review of DT bindings for new properties to be made required, based on the above rationale. This is the first time I see a push back. I believe we need to address both of the above problems. In the very short term, we have to pick which of the two we care about most, as we can't have both yet. I have made my personal preference clear, but I'll apply the official decision in further reviews. Maybe Rob could share his point of view ? -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/4] dt-bindings: drm/bridge: ti-sn65dsi83: Add vcc supply bindings 2021-10-19 10:37 ` Laurent Pinchart @ 2021-10-19 19:39 ` Sam Ravnborg 2021-10-27 7:28 ` Laurent Pinchart 0 siblings, 1 reply; 16+ messages in thread From: Sam Ravnborg @ 2021-10-19 19:39 UTC (permalink / raw) To: Laurent Pinchart Cc: Maxime Ripard, Alexander Stein, David Airlie, Daniel Vetter, Rob Herring, Andrzej Hajda, Neil Armstrong, Robert Foss, Jonas Karlman, Jernej Skrabec, dri-devel, devicetree > > > > > > 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. > > That point is debatable :-) I've repeatedly asked during review of DT > bindings for new properties to be made required, based on the above > rationale. This is the first time I see a push back. > > I believe we need to address both of the above problems. In the very > short term, we have to pick which of the two we care about most, as we > can't have both yet. I have made my personal preference clear, but I'll > apply the official decision in further reviews. Maybe Rob could share > his point of view ? The bindings are there to make sure the device trees are OK, and the bindings shall do their best to make sure the device trees are as correct as possible. This will break existing device trees when we realise something is not correct in bindings files. In such a case the ideal workflow would be to: 1) Fix the device tree files so they match the new and more correct bindings 2) Update the bindings with the latest fixes As we have different trees for device trees and for bindings this is a bit difficult at the moment. But the above would be the ideal ways of working IMO. Compare this to updating a header file in the kernel that results in new warnings/errors. The ways of working here is to fix the warnings/errors before adding the change to the header file. (For example when adding a must-check attribute). My take - but then I seldom checks the device tree files as keeping the bindings free of errors was the challenge in the past. Rob does a fantastic jobs to keep the kernel error free here and I assume focus may change to device trees soon. Sam ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/4] dt-bindings: drm/bridge: ti-sn65dsi83: Add vcc supply bindings 2021-10-19 19:39 ` Sam Ravnborg @ 2021-10-27 7:28 ` Laurent Pinchart 0 siblings, 0 replies; 16+ messages in thread From: Laurent Pinchart @ 2021-10-27 7:28 UTC (permalink / raw) To: Rob Herring Cc: Sam Ravnborg, Maxime Ripard, Alexander Stein, David Airlie, Daniel Vetter, Andrzej Hajda, Neil Armstrong, Robert Foss, Jonas Karlman, Jernej Skrabec, dri-devel, devicetree Hi Rob, Could you please share your opinion on this ? On Tue, Oct 19, 2021 at 09:39:15PM +0200, Sam Ravnborg wrote: > > > > > > > > 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. > > > > That point is debatable :-) I've repeatedly asked during review of DT > > bindings for new properties to be made required, based on the above > > rationale. This is the first time I see a push back. > > > > I believe we need to address both of the above problems. In the very > > short term, we have to pick which of the two we care about most, as we > > can't have both yet. I have made my personal preference clear, but I'll > > apply the official decision in further reviews. Maybe Rob could share > > his point of view ? > > The bindings are there to make sure the device trees are OK, and the > bindings shall do their best to make sure the device trees are as > correct as possible. > > This will break existing device trees when we realise something is > not correct in bindings files. > > In such a case the ideal workflow would be to: > 1) Fix the device tree files so they match the new and more correct > bindings > 2) Update the bindings with the latest fixes > > As we have different trees for device trees and for bindings this is a > bit difficult at the moment. But the above would be the ideal ways of > working IMO. > > Compare this to updating a header file in the kernel that results in new > warnings/errors. The ways of working here is to fix the warnings/errors > before adding the change to the header file. (For example when adding a > must-check attribute). > > My take - but then I seldom checks the device tree files as keeping the > bindings free of errors was the challenge in the past. Rob does a > fantastic jobs to keep the kernel error free here and I assume focus may > change to device trees soon. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 4/4] drm/bridge: ti-sn65dsi83: Add vcc supply regulator support 2021-10-12 6:48 [PATCH v2 0/4] ti-sn65dsi83 patches Alexander Stein ` (2 preceding siblings ...) 2021-10-12 6:48 ` [PATCH v2 3/4] dt-bindings: drm/bridge: ti-sn65dsi83: Add vcc supply bindings Alexander Stein @ 2021-10-12 6:48 ` Alexander Stein 2021-10-12 8:43 ` Laurent Pinchart 3 siblings, 1 reply; 16+ messages in thread From: Alexander Stein @ 2021-10-12 6:48 UTC (permalink / raw) To: David Airlie, Daniel Vetter, Rob Herring, Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart, Jonas Karlman, Jernej Skrabec Cc: Alexander Stein, dri-devel, devicetree, Sam Ravnborg VCC needs to be enabled before releasing the enable GPIO. Reviewed-by: Sam Ravnborg <sam@ravnborg.org> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> --- drivers/gpu/drm/bridge/ti-sn65dsi83.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c index 9072342566f3..a6b1fd71dfee 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c @@ -33,6 +33,7 @@ #include <linux/of_device.h> #include <linux/of_graph.h> #include <linux/regmap.h> +#include <linux/regulator/consumer.h> #include <drm/drm_atomic_helper.h> #include <drm/drm_bridge.h> @@ -143,6 +144,7 @@ struct sn65dsi83 { struct mipi_dsi_device *dsi; struct drm_bridge *panel_bridge; struct gpio_desc *enable_gpio; + struct regulator *vcc; int dsi_lanes; bool lvds_dual_link; bool lvds_dual_link_even_odd_swap; @@ -647,6 +649,12 @@ static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum sn65dsi83_model model) ctx->panel_bridge = panel_bridge; + ctx->vcc = devm_regulator_get(dev, "vcc"); + if (IS_ERR(ctx->vcc)) + return dev_err_probe(dev, PTR_ERR(ctx->vcc), + "Failed to get supply 'vcc': %pe\n", + ctx->vcc); + return 0; } @@ -691,7 +699,11 @@ static int sn65dsi83_probe(struct i2c_client *client, ctx->bridge.of_node = dev->of_node; drm_bridge_add(&ctx->bridge); - return 0; + ret = regulator_enable(ctx->vcc); + if (ret) + dev_err(dev, "Failed to enable vcc: %i\n", ret); + + return ret; } static int sn65dsi83_remove(struct i2c_client *client) @@ -702,6 +714,7 @@ static int sn65dsi83_remove(struct i2c_client *client) mipi_dsi_device_unregister(ctx->dsi); drm_bridge_remove(&ctx->bridge); of_node_put(ctx->host_node); + regulator_disable(ctx->vcc); return 0; } -- 2.25.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 4/4] drm/bridge: ti-sn65dsi83: Add vcc supply regulator support 2021-10-12 6:48 ` [PATCH v2 4/4] drm/bridge: ti-sn65dsi83: Add vcc supply regulator support Alexander Stein @ 2021-10-12 8:43 ` Laurent Pinchart 0 siblings, 0 replies; 16+ messages in thread From: Laurent Pinchart @ 2021-10-12 8:43 UTC (permalink / raw) To: Alexander Stein Cc: David Airlie, Daniel Vetter, Rob Herring, Andrzej Hajda, Neil Armstrong, Robert Foss, Jonas Karlman, Jernej Skrabec, dri-devel, devicetree, Sam Ravnborg Hi Alexander, Thank you for the patch. On Tue, Oct 12, 2021 at 08:48:43AM +0200, Alexander Stein wrote: > VCC needs to be enabled before releasing the enable GPIO. > > Reviewed-by: Sam Ravnborg <sam@ravnborg.org> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> > --- > drivers/gpu/drm/bridge/ti-sn65dsi83.c | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > index 9072342566f3..a6b1fd71dfee 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > @@ -33,6 +33,7 @@ > #include <linux/of_device.h> > #include <linux/of_graph.h> > #include <linux/regmap.h> > +#include <linux/regulator/consumer.h> > > #include <drm/drm_atomic_helper.h> > #include <drm/drm_bridge.h> > @@ -143,6 +144,7 @@ struct sn65dsi83 { > struct mipi_dsi_device *dsi; > struct drm_bridge *panel_bridge; > struct gpio_desc *enable_gpio; > + struct regulator *vcc; > int dsi_lanes; > bool lvds_dual_link; > bool lvds_dual_link_even_odd_swap; > @@ -647,6 +649,12 @@ static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum sn65dsi83_model model) > > ctx->panel_bridge = panel_bridge; > > + ctx->vcc = devm_regulator_get(dev, "vcc"); > + if (IS_ERR(ctx->vcc)) > + return dev_err_probe(dev, PTR_ERR(ctx->vcc), > + "Failed to get supply 'vcc': %pe\n", > + ctx->vcc); > + > return 0; > } > > @@ -691,7 +699,11 @@ static int sn65dsi83_probe(struct i2c_client *client, > ctx->bridge.of_node = dev->of_node; > drm_bridge_add(&ctx->bridge); > > - return 0; > + ret = regulator_enable(ctx->vcc); > + if (ret) > + dev_err(dev, "Failed to enable vcc: %i\n", ret); I think this should move to sn65dsi83_atomic_pre_enable() (and similarly for regulator_disable()) as keeping the regulator enabled at all times will cost power. > + > + return ret; > } > > static int sn65dsi83_remove(struct i2c_client *client) > @@ -702,6 +714,7 @@ static int sn65dsi83_remove(struct i2c_client *client) > mipi_dsi_device_unregister(ctx->dsi); > drm_bridge_remove(&ctx->bridge); > of_node_put(ctx->host_node); > + regulator_disable(ctx->vcc); > > return 0; > } -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2021-10-27 7:29 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-10-12 6:48 [PATCH v2 0/4] ti-sn65dsi83 patches Alexander Stein 2021-10-12 6:48 ` [PATCH v2 1/4] dt-bindings: display: bridge: sn65dsi83: Make enable GPIO optional Alexander Stein 2021-10-12 6:48 ` [PATCH v2 2/4] drm/bridge: ti-sn65dsi83: " Alexander Stein 2021-10-12 6:48 ` [PATCH v2 3/4] dt-bindings: drm/bridge: ti-sn65dsi83: Add vcc supply bindings Alexander Stein 2021-10-13 7:47 ` Maxime Ripard 2021-10-13 9:37 ` Laurent Pinchart 2021-10-14 7:41 ` Maxime Ripard 2021-10-16 2:34 ` Laurent Pinchart 2021-10-18 15:20 ` Maxime Ripard 2021-10-18 17:48 ` Laurent Pinchart 2021-10-19 7:37 ` Maxime Ripard 2021-10-19 10:37 ` Laurent Pinchart 2021-10-19 19:39 ` Sam Ravnborg 2021-10-27 7:28 ` Laurent Pinchart 2021-10-12 6:48 ` [PATCH v2 4/4] drm/bridge: ti-sn65dsi83: Add vcc supply regulator support Alexander Stein 2021-10-12 8:43 ` Laurent Pinchart
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.