| Message ID | 20211121171545.27402-3-j@jannau.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Apple Arm patform device tree and bindings fixes | expand |
Hi, On Sun, Nov 21, 2021, at 18:15, Janne Grunau wrote: > The intention was to have a SoC-specific and base compatible string > to allow forward compatibility and SoC specific quirks, > > Fixes: df7c4a8c1b47 ("dt-bindings: i2c: Add Apple I2C controller bindings") > Signed-off-by: Janne Grunau <j@jannau.net> > Cc: Mark Kettenis <kettenis@openbsd.org> > --- Yeah, this should've been "apple,t8103-i2c", "apple,i2c" all along :/ Given that we have no i2c nodes in the dts yet and that this binding was only added for -rc1 I think it's fine to just drop "apple,t8103-i2c" here instead of marking it as deprecated and keeping it around forever if Mark Kettenis also agrees. > Documentation/devicetree/bindings/i2c/apple,i2c.yaml | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/Documentation/devicetree/bindings/i2c/apple,i2c.yaml > b/Documentation/devicetree/bindings/i2c/apple,i2c.yaml > index 22fc8483256f..f1cb96c08212 100644 > --- a/Documentation/devicetree/bindings/i2c/apple,i2c.yaml > +++ b/Documentation/devicetree/bindings/i2c/apple,i2c.yaml > @@ -20,9 +20,10 @@ allOf: > > properties: > compatible: > - enum: > - - apple,t8103-i2c > - - apple,i2c > + items: > + - enum: > + - apple,t8103-i2c > + - const: apple,i2c Nit: the enum makes sense once we add t6000-i2c but right now properties: compatible: items: - const: apple,t8103-i2c - const: apple,i2c also works and look a bit less weird. Either way, Reviewed-by: Sven Peter <sven@svenpeter.dev> Sven
On 2021-11-21 21:22:47 +0100, Sven Peter wrote: > Hi, > > On Sun, Nov 21, 2021, at 18:15, Janne Grunau wrote: > > The intention was to have a SoC-specific and base compatible string > > to allow forward compatibility and SoC specific quirks, > > > > Fixes: df7c4a8c1b47 ("dt-bindings: i2c: Add Apple I2C controller bindings") > > Signed-off-by: Janne Grunau <j@jannau.net> > > Cc: Mark Kettenis <kettenis@openbsd.org> > > --- > > Yeah, this should've been "apple,t8103-i2c", "apple,i2c" all along :/ > Given that we have no i2c nodes in the dts yet and that this binding was > only added for -rc1 I think it's fine to just drop "apple,t8103-i2c" > here instead of marking it as deprecated and keeping it around forever > if Mark Kettenis also agrees. > > > Documentation/devicetree/bindings/i2c/apple,i2c.yaml | 9 +++++---- > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/i2c/apple,i2c.yaml > > b/Documentation/devicetree/bindings/i2c/apple,i2c.yaml > > index 22fc8483256f..f1cb96c08212 100644 > > --- a/Documentation/devicetree/bindings/i2c/apple,i2c.yaml > > +++ b/Documentation/devicetree/bindings/i2c/apple,i2c.yaml > > @@ -20,9 +20,10 @@ allOf: > > > > properties: > > compatible: > > - enum: > > - - apple,t8103-i2c > > - - apple,i2c > > + items: > > + - enum: > > + - apple,t8103-i2c > > + - const: apple,i2c > > Nit: the enum makes sense once we add t6000-i2c but right now > > properties: > compatible: > items: > - const: apple,t8103-i2c > - const: apple,i2c > > also works and look a bit less weird. I split it from change which in addition added "apple,t6000-i2c" as second enum value. I have no strong preference but the weirdness will hopefully vanish soon. Janne
On Mon, Nov 22, 2021, at 18:59, Janne Grunau wrote: > On 2021-11-21 21:22:47 +0100, Sven Peter wrote: >> Hi, >> >> On Sun, Nov 21, 2021, at 18:15, Janne Grunau wrote: >> > The intention was to have a SoC-specific and base compatible string >> > to allow forward compatibility and SoC specific quirks, >> > >> > Fixes: df7c4a8c1b47 ("dt-bindings: i2c: Add Apple I2C controller bindings") >> > Signed-off-by: Janne Grunau <j@jannau.net> >> > Cc: Mark Kettenis <kettenis@openbsd.org> >> > --- >> >> Yeah, this should've been "apple,t8103-i2c", "apple,i2c" all along :/ >> Given that we have no i2c nodes in the dts yet and that this binding was >> only added for -rc1 I think it's fine to just drop "apple,t8103-i2c" >> here instead of marking it as deprecated and keeping it around forever >> if Mark Kettenis also agrees. >> >> > Documentation/devicetree/bindings/i2c/apple,i2c.yaml | 9 +++++---- >> > 1 file changed, 5 insertions(+), 4 deletions(-) >> > >> > diff --git a/Documentation/devicetree/bindings/i2c/apple,i2c.yaml >> > b/Documentation/devicetree/bindings/i2c/apple,i2c.yaml >> > index 22fc8483256f..f1cb96c08212 100644 >> > --- a/Documentation/devicetree/bindings/i2c/apple,i2c.yaml >> > +++ b/Documentation/devicetree/bindings/i2c/apple,i2c.yaml >> > @@ -20,9 +20,10 @@ allOf: >> > >> > properties: >> > compatible: >> > - enum: >> > - - apple,t8103-i2c >> > - - apple,i2c >> > + items: >> > + - enum: >> > + - apple,t8103-i2c >> > + - const: apple,i2c >> >> Nit: the enum makes sense once we add t6000-i2c but right now >> >> properties: >> compatible: >> items: >> - const: apple,t8103-i2c >> - const: apple,i2c >> >> also works and look a bit less weird. > > I split it from change which in addition added "apple,t6000-i2c" as > second enum value. I have no strong preference but the weirdness will > hopefully vanish soon. I'm slightly in favor of not introducing the enum. You'd have to fix the indentation if you wanted to introduce it anyway [1] (checkpatch there also seems to complain about the commit hash for the Fixes tag but that looks like a spurious warning to me since df7c4a8c1b47 is in Linus' tree [2]): yamllint warnings/errors: ./Documentation/devicetree/bindings/i2c/apple,i2c.yaml:25:9: [warning] wrong indentation: expected 10 but found 8 (indentation) Best, Sven [1] http://patchwork.ozlabs.org/project/devicetree-bindings/patch/20211121171545.27402-3-j@jannau.net/ [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=df7c4a8c1b47
diff --git a/Documentation/devicetree/bindings/i2c/apple,i2c.yaml b/Documentation/devicetree/bindings/i2c/apple,i2c.yaml index 22fc8483256f..f1cb96c08212 100644 --- a/Documentation/devicetree/bindings/i2c/apple,i2c.yaml +++ b/Documentation/devicetree/bindings/i2c/apple,i2c.yaml @@ -20,9 +20,10 @@ allOf: properties: compatible: - enum: - - apple,t8103-i2c - - apple,i2c + items: + - enum: + - apple,t8103-i2c + - const: apple,i2c reg: maxItems: 1 @@ -51,7 +52,7 @@ unevaluatedProperties: false examples: - | i2c@35010000 { - compatible = "apple,t8103-i2c"; + compatible = "apple,t8103-i2c", "apple,i2c"; reg = <0x35010000 0x4000>; interrupt-parent = <&aic>; interrupts = <0 627 4>;
The intention was to have a SoC-specific and base compatible string to allow forward compatibility and SoC specific quirks, Fixes: df7c4a8c1b47 ("dt-bindings: i2c: Add Apple I2C controller bindings") Signed-off-by: Janne Grunau <j@jannau.net> Cc: Mark Kettenis <kettenis@openbsd.org> --- Documentation/devicetree/bindings/i2c/apple,i2c.yaml | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)