| Message ID | 20230606144447.775942-1-alexander.stein@ew.tq-group.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [1/1] dt-bindings: phy: mixel,mipi-dsi-phy: Remove assigned-clock* properties | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 0 errors, 2 warnings, 15 lines checked |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On Tue, Jun 06, 2023 at 04:44:46PM +0200, Alexander Stein wrote: > These properties are allowed anyway and some SoC (e.g. imx8mq) configure > more than just one clock using these properties. What does "allowed anyway" mean? And following from that, why not modify the min/maxItems to suit reality, rather than remove them. Is there enforcement from elsewhere? > Fixes: f9b0593dd4fc6 ("dt-bindings: phy: Convert mixel,mipi-dsi-phy to json-schema") > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> > --- > I can't reproduce the mentioned mis-matches in commit f9b0593dd4fc6 > ("dt-bindings: phy: Convert mixel,mipi-dsi-phy to json-schema"). I suspect that meant that the property was in the dt but not in the binding at the time of the conversion. Cheers, Conor. > > Since commit 62270eeb2b639 ("arm64: dts: imx8mq: Add clock parents for > mipi dphy") imx8mq.dtsi configures several clocks using assigned-clocks* > properties. > > .../devicetree/bindings/phy/mixel,mipi-dsi-phy.yaml | 9 --------- > 1 file changed, 9 deletions(-) > > diff --git a/Documentation/devicetree/bindings/phy/mixel,mipi-dsi-phy.yaml b/Documentation/devicetree/bindings/phy/mixel,mipi-dsi-phy.yaml > index 786cfd71cb7eb..3c28ec50f0979 100644 > --- a/Documentation/devicetree/bindings/phy/mixel,mipi-dsi-phy.yaml > +++ b/Documentation/devicetree/bindings/phy/mixel,mipi-dsi-phy.yaml > @@ -32,15 +32,6 @@ properties: > clock-names: > const: phy_ref > > - assigned-clocks: > - maxItems: 1 > - > - assigned-clock-parents: > - maxItems: 1 > - > - assigned-clock-rates: > - maxItems: 1 > - > "#phy-cells": > const: 0 > > -- > 2.34.1 >
> From: Alexander Stein <alexander.stein@ew.tq-group.com> > > These properties are allowed anyway and some SoC (e.g. imx8mq) configure > more than just one clock using these properties. > > Fixes: f9b0593dd4fc6 ("dt-bindings: phy: Convert mixel,mipi-dsi-phy to json- > schema") > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> > --- > I can't reproduce the mentioned mis-matches in commit f9b0593dd4fc6 > ("dt-bindings: phy: Convert mixel,mipi-dsi-phy to json-schema"). IIUC, this is due to dt-schema change done by commit c3424745f900 ("dtschema: Add assigned-clock* properties by default"). See https://github.com/devicetree-org/dt-schema/commit/c3424745f900e8cf0a0e3acebffeeda83a82f6d4 Regards, Liu Ying
Hi Conor, Am Dienstag, 6. Juni 2023, 20:21:02 CEST schrieb Conor Dooley: > * PGP Signed by an unknown key > > On Tue, Jun 06, 2023 at 04:44:46PM +0200, Alexander Stein wrote: > > These properties are allowed anyway and some SoC (e.g. imx8mq) configure > > more than just one clock using these properties. > > What does "allowed anyway" mean? > And following from that, why not modify the min/maxItems to suit > reality, rather than remove them. Is there enforcement from elsewhere? As Liu pointed out, assigned-clock* were considered a generic property added by default at that time. With that support added there is no need to specify these properties in this bindings again. Despite that you never know in advance how many items you will have to add to assigned-clock* properties, that's totally different to 'clocks', it may even depend on board specific clock setups. > > Fixes: f9b0593dd4fc6 ("dt-bindings: phy: Convert mixel,mipi-dsi-phy to > > json-schema") Signed-off-by: Alexander Stein > > <alexander.stein@ew.tq-group.com> > > --- > > I can't reproduce the mentioned mis-matches in commit f9b0593dd4fc6 > > ("dt-bindings: phy: Convert mixel,mipi-dsi-phy to json-schema"). > > I suspect that meant that the property was in the dt but not in the > binding at the time of the conversion. You are right, given the commit [1]. Which, from today's perspective, is also a rationale for this patch. Best regards, Alexander [1] https://github.com/devicetree-org/dt-schema/commit/ c3424745f900e8cf0a0e3acebffeeda83a82f6d4 > Cheers, > Conor. > > > Since commit 62270eeb2b639 ("arm64: dts: imx8mq: Add clock parents for > > mipi dphy") imx8mq.dtsi configures several clocks using assigned-clocks* > > properties. > > > > .../devicetree/bindings/phy/mixel,mipi-dsi-phy.yaml | 9 --------- > > 1 file changed, 9 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/phy/mixel,mipi-dsi-phy.yaml > > b/Documentation/devicetree/bindings/phy/mixel,mipi-dsi-phy.yaml index > > 786cfd71cb7eb..3c28ec50f0979 100644 > > --- a/Documentation/devicetree/bindings/phy/mixel,mipi-dsi-phy.yaml > > +++ b/Documentation/devicetree/bindings/phy/mixel,mipi-dsi-phy.yaml > > > > @@ -32,15 +32,6 @@ properties: > > clock-names: > > const: phy_ref > > > > - assigned-clocks: > > - maxItems: 1 > > - > > - assigned-clock-parents: > > - maxItems: 1 > > - > > - assigned-clock-rates: > > - maxItems: 1 > > - > > > > "#phy-cells": > > const: 0 > > * Unknown Key > * 0xA08262D2
On Thu, Jun 08, 2023 at 09:31:57AM +0200, Alexander Stein wrote: > Hi Conor, > > Am Dienstag, 6. Juni 2023, 20:21:02 CEST schrieb Conor Dooley: > > * PGP Signed by an unknown key > > > > On Tue, Jun 06, 2023 at 04:44:46PM +0200, Alexander Stein wrote: > > > These properties are allowed anyway and some SoC (e.g. imx8mq) configure > > > more than just one clock using these properties. > > > > What does "allowed anyway" mean? > > And following from that, why not modify the min/maxItems to suit > > reality, rather than remove them. Is there enforcement from elsewhere? > > As Liu pointed out, assigned-clock* were considered a generic property added > by default at that time. With that support added there is no need to specify > these properties in this bindings again. > Despite that you never know in advance how many items you will have to add to > assigned-clock* properties, that's totally different to 'clocks', it may even > depend on board specific clock setups. Sounds grand to me. I think it'd be good in the future to explain *where* the enforcement comes from, rather than saying something like "allowed anyway". Otherwise, Reviewed-by: Conor Dooley <conor.dooley@microchip.com> Cheers, Conor.
Hi Conor, Am Donnerstag, 8. Juni 2023, 09:59:10 CEST schrieb Conor Dooley: > * PGP Signed by an unknown key > > On Thu, Jun 08, 2023 at 09:31:57AM +0200, Alexander Stein wrote: > > Hi Conor, > > > > Am Dienstag, 6. Juni 2023, 20:21:02 CEST schrieb Conor Dooley: > > > > Old Signed by an unknown key > > > > > > On Tue, Jun 06, 2023 at 04:44:46PM +0200, Alexander Stein wrote: > > > > These properties are allowed anyway and some SoC (e.g. imx8mq) > > > > configure > > > > more than just one clock using these properties. > > > > > > What does "allowed anyway" mean? > > > And following from that, why not modify the min/maxItems to suit > > > reality, rather than remove them. Is there enforcement from elsewhere? > > > > As Liu pointed out, assigned-clock* were considered a generic property > > added by default at that time. With that support added there is no need > > to specify these properties in this bindings again. > > Despite that you never know in advance how many items you will have to add > > to assigned-clock* properties, that's totally different to 'clocks', it > > may even depend on board specific clock setups. > > Sounds grand to me. I think it'd be good in the future to explain > *where* the enforcement comes from, rather than saying something like > "allowed anyway". True, I'll send an updated patch with commit message improved referring to updated dt-schema. > Otherwise, > Reviewed-by: Conor Dooley <conor.dooley@microchip.com> Thanks, Alexander > Cheers, > Conor. > > * Unknown Key > * 0xA08262D2
diff --git a/Documentation/devicetree/bindings/phy/mixel,mipi-dsi-phy.yaml b/Documentation/devicetree/bindings/phy/mixel,mipi-dsi-phy.yaml index 786cfd71cb7eb..3c28ec50f0979 100644 --- a/Documentation/devicetree/bindings/phy/mixel,mipi-dsi-phy.yaml +++ b/Documentation/devicetree/bindings/phy/mixel,mipi-dsi-phy.yaml @@ -32,15 +32,6 @@ properties: clock-names: const: phy_ref - assigned-clocks: - maxItems: 1 - - assigned-clock-parents: - maxItems: 1 - - assigned-clock-rates: - maxItems: 1 - "#phy-cells": const: 0
These properties are allowed anyway and some SoC (e.g. imx8mq) configure more than just one clock using these properties. Fixes: f9b0593dd4fc6 ("dt-bindings: phy: Convert mixel,mipi-dsi-phy to json-schema") Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> --- I can't reproduce the mentioned mis-matches in commit f9b0593dd4fc6 ("dt-bindings: phy: Convert mixel,mipi-dsi-phy to json-schema"). Since commit 62270eeb2b639 ("arm64: dts: imx8mq: Add clock parents for mipi dphy") imx8mq.dtsi configures several clocks using assigned-clocks* properties. .../devicetree/bindings/phy/mixel,mipi-dsi-phy.yaml | 9 --------- 1 file changed, 9 deletions(-)