Hello, On Fri Mar 1, 2024 at 4:11 PM CET, Rob Herring wrote: > On Thu, Feb 29, 2024 at 07:10:49PM +0100, Théo Lebrun wrote: > > Add EyeQ5 bindings to the existing Nomadik I2C dt-bindings. Add the > > EyeQ5-specific property behind a conditional. Add an example for this > > compatible. > > > > Signed-off-by: Théo Lebrun <theo.lebrun@xxxxxxxxxxx> > > --- > > .../devicetree/bindings/i2c/st,nomadik-i2c.yaml | 48 ++++++++++++++++++++-- > > 1 file changed, 44 insertions(+), 4 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml b/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml > > index 16024415a4a7..2d9d5b276762 100644 > > --- a/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml > > +++ b/Documentation/devicetree/bindings/i2c/st,nomadik-i2c.yaml > > @@ -14,9 +14,6 @@ description: The Nomadik I2C host controller began its life in the ST > > maintainers: > > - Linus Walleij <linus.walleij@xxxxxxxxxx> > > > > -allOf: > > - - $ref: /schemas/i2c/i2c-controller.yaml# > > - > > # Need a custom select here or 'arm,primecell' will match on lots of nodes > > select: > > properties: > > @@ -24,6 +21,7 @@ select: > > contains: > > enum: > > - st,nomadik-i2c > > + - mobileye,eyeq5-i2c > > required: > > - compatible > > > > @@ -39,6 +37,10 @@ properties: > > - const: stericsson,db8500-i2c > > - const: st,nomadik-i2c > > - const: arm,primecell > > + # The variant found on Mobileye EyeQ5 > > Kind of obvious from the compatible string, but maybe you are keeping > the existing style... I indeed kept the existing style. Ping me if you want this removed! > > + - items: > > + - const: mobileye,eyeq5-i2c > > + - const: arm,primecell > > > > reg: > > maxItems: 1 > > @@ -55,7 +57,7 @@ properties: > > - items: > > - const: mclk > > - const: apb_pclk > > - # Clock name in DB8500 > > + # Clock name in DB8500 or EyeQ5 > > - items: > > - const: i2cclk > > - const: apb_pclk > > @@ -70,6 +72,16 @@ properties: > > minimum: 1 > > maximum: 400000 > > > > + mobileye,olb: > > + $ref: /schemas/types.yaml#/definitions/phandle-array > > + items: > > + - items: > > + - description: Phandle to OLB system controller node. > > + - description: Platform-wide controller ID (integer starting from zero). > > Rather than a made up ID, just store the shift value you ultimately > need. Issue with storing the shift value is that you also need to know what value to put in that field. It's an enum mapping the I2C speed which isn't found in DT. > These properties are fragile because they break if anything that's not > defined in DT changes whether that's register offset, bit offset, > bitfield size or values. Or also if there are additional fields to > access. My take is that it is better to have it all either in DT or in driver. Having a mix of both is a mess when debugging. If something breaks it is a driver bug; such bugs get fixed in driver code. That way DT doesn't know about it and doesn't have to be changed. Putting shifts in DT is an abstraction that ends up being unhelpful. You split hardware knowledge half in DT (register offset and/or mask), half in driver (value to put in the field). We'd rather have it all in driver code. Next hardware revision will be more complex with potentially fields split across registers. A shift value wouldn't cut it. A new compatible + made up ID allows accomodating for that. That way we have homogeneity across compatibles. Have a nice day, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
