Re: [PATCH 02/13] dt-bindings: i2c: nomadik: add mobileye,eyeq5-i2c bindings and example

[Théo Lebrun]

Hello,

On Sat Feb 17, 2024 at 9:25 AM CET, Krzysztof Kozlowski wrote:
> On 16/02/2024 11:40, Théo Lebrun wrote:
> > On Fri Feb 16, 2024 at 11:33 AM CET, Krzysztof Kozlowski wrote:
> >> On 16/02/2024 11:18, Théo Lebrun wrote:
> >>>
> >>>>> +        mobileye,id:
> >>>>> +          $ref: /schemas/types.yaml#/definitions/uint32
> >>>>> +          description: Platform-wide controller ID (integer starting from zero).
> >>>>
> >>>> instance indexes are a NAK. You can use i2cN aliases if you must.
> >>>>
> >>>> Why do you need it? To access OLB? If so, add cell args to the OLB 
> >>>> phandle instead.
> >>>
> >>> Why we do what we do: I2C controller must write a 2 bit value depending
> >>> on the bus speed. All I2C controllers write into the same register.
> >>
> >> Which register?  Your devices do not share IO address space.
> > 
> > mobileye,olb is a prop with a phandle to a syscon. That syscon contains
> > the register we are interested in.
>
> So exactly what Rob said... I don't understand why you have chosen to go
> with alias.

I had misunderstood Rob's original message. Now that I've done some
tests to use cells I get what was meant. I'd have a follow-up question.
What should the cells contain? I see two options:

 - phandle + I2C controller global index (from 0 thru 4). Then Linux
   (or other) driver know how to map that index to register + mask
   combo. ie:

      i2c2: i2c@500000 {
         compatible = "mobileye,eyeq5-i2c", "arm,primecell";
         reg = <0 0x500000 0x0 0x1000>;
         /* ... */
         mobileye,olb = <&olb 2>;
      };

 - phandle + register offset + mask. ie:

      i2c2: i2c@500000 {
         compatible = "mobileye,eyeq5-i2c", "arm,primecell";
         reg = <0 0x500000 0x0 0x1000>;
         /* ... */
         mobileye,olb = <&olb 0xB8 0x300>; /* phandle + offset + mask */
      };

I would have guessed the second approach was frown upon as DT aren't
meant to contain iomem offsets. However I'm seeing quite a few drivers
using this approach, and no driver doing the first approach. Maybe my
instinct isn't leading me the right way.

See those bindings that use the second approach. They were found because
their drivers use the syscon_regmap_lookup_by_phandle_args() function
call. I've added the file creation date to highlight recent bindings
(that hopefully are closer to the right way).
 - phy/starfive,jh7110-pcie-phy.yaml    2023-06-29T15:51:12+08:00
 - usb/starfive,jh7110-usb.yaml         2023-05-18T19:27:48+08:00
 - net/starfive,jh7110-dwmac.yaml       2023-04-17T18:02:49+08:00
 - phy/qcom,sc8280xp-qmp-pcie-phy.yaml  2022-11-05T15:59:34+01:00
 - sound/snps,designware-i2s.yaml       2022-07-01T20:22:49+01:00
 - pinctrl/canaan,k210-fpioa.yaml       2020-12-13T22:50:44+09:00
 - media/ti,cal.yaml                    2019-11-12T15:53:47+01:00

I know looking at existing drivers/bindings isn't the right way, but I
have no other frame of reference. That's why I'm asking for guidance on
this one.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

------------------------------------------------------------------------