Re: [PATCH v2 01/11] dt-bindings: i2c: nomadik: add mobileye,eyeq5-i2c bindings and example

[Théo Lebrun]

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