[i2c] [PATCH 3/5] powerpc: Document device nodes for I2C devices.

[David Brownell]

On Friday 18 May 2007, Scott Wood wrote:
> David Brownell wrote:
> >>>>+   Recommended properties :
> >>>>+
> >>>>+    - compatible : The name of the Linux device driver that
> >>>>+      handles this device.  If unspecified, the name of the
> >>>>+      node will be used.
> >>>
> >>>NO WAY
> >>
> >>Sorry, that was left in there from a while ago and I missed it.  It
> >>should be defined the same way as any other compatible property
> > 
> > 
> > So long as one outcome is that it's possible to say "use the rtc-ds1307
> > driver, passing chip type of ds1339".
> 
> It's not, unless you want the kernel to keep a table to map OF types to 
> Linux drivers.

I don't follow your argument here.  There *IS* no OF type
model for I2C at this time.

So in a trivial sense, it already has such a null table...

I honestly can't see anything particularly wrong with letting
the first set of I2C type IDs be defined by Linux.  It's not so
different from having let Solaris define the IDs at the core of
OpenBoot.  And in fact, the documentation file in the $SUBJECT
patch recommends defining nodes for busses that don't specifically
fit into the existing OF specs ... that describes I2C exactly!


> > You need to remember that I2C, unlike PCI or USB, doesn't have any
> > internal device typing framework. So any type system layered on top
> > of it will inherently capture OS-specific information.
> 
> It's not *internal*, but there can still be published bindings for a 
> given device tree scheme that standardize names.

Which you haven't proposed -- either the standardized names or
how to publish those bindings.  Right?


> It's much easier to  
> let drivers specify multiple names in order to match multiple device 
> binding schemes and multiple supported types than to get 
> non-Linux-specific bindings to agree on things like "ds1339 chips use 
> the ds1307 driver".

You've had well over six months to make a proposal of that type,
during which we *know* you've been aware enough of the pending
Linux patches to make a concrete proposal.  (Your initial OF
conversion was in November of last year, ISTR...)

That all suggests, much to the contrary, that it's easier to let
other people pioneer a working framework, and only then start to
make concrete suggestions.

... however, your suggestions here haven't yet risen to the
level of being concrete suggestsions, much less patches which
demonstrate this idea.


> The main problem would be conflicts where the same name means different 
> things in different bindings, but that seems much less likely than the 
> same thing having different names in different bindings.

Presumably you're aware that in this case there are trivial
disambiguation schemes available, such as a "Linux-" prefix.


> >>(and the 
> >>i2c code in Linux should be fixed to allow drivers to specify multiple
> >>match names).
> > 
> > I'd have to disagree with that notion on the basis that it's
> > not even half-formed!
> 
> How so?  The drivers would pass a list of match strings, much like a 
> list of PCI or OF matches.  If the driver doesn't specify one, it 
> defaults to the current behavior of using the driver name.  What aspect 
> of it needs more forming?

So where is the definition of those "match strings"?  Remembering
that the strings are not provided by chip vendors, and that the
only currently meaningful strings are those defined by Linux.
I'd say that there's nothing for Linux to care about in terms of
compatibility.

And where is the complete patchset making that work for Linux?
Including not just changes in i2c driver binding, but also modpost
and modalias support to provide equivalent hotplug/coldplug support?

That's "half formed" in the sense that it's not even progressed to
the point of a reviewable patch for the parts that _seem_ obvious.
And those parts aren't even a third -- the easy third, in terms of
concept and deployment! -- of the updates I highlighted above.


> > I suggest first making sure that the OF node can fully populate
> > the current i2c_board_info struct.  Later, if there's interest,
> > it can generalize to support other platforms.
> 
> This is what would be required for OF to be able to populate 
> i2c_board_info correctly.  OF device trees are not Linux-specific.

You may be assuming some context I can't see.  To review, that
struct has the following fields:

        char            driver_name[KOBJ_NAME_LEN];
        char            type[I2C_NAME_SIZE];
        unsigned short  flags;
        unsigned short  addr;
        void            *platform_data;
        int             irq;

Of those fields, you described how to set up only "irq" and "addr".
And not "flags", which is needed to distinguish e.g. 7 vs 10 bit
addressing and say if SMBus PEC is required.  And it's not at all
clear how the other fields would be set up.

"driver_name" was initially going to be "compatible" -- not
clear to me that couldn't be made to work, given some updates.

I believe "platform_data" would never come from OF, but the
board init logic might want to use OF to cross-check its own
records.


> If such a change to the i2c layer is deemed unacceptable, then we can do 
> some sort of table-driven fixup in arch/powerpc -- but that'd be much 
> uglier, and I'd prefer that we didn't have to go down that route.

I too would rather not see powerpc "do its own thing" here; though
it's worth noting that the OF glue doesn't seem to be generic yet,
despite multiple platforms using OF.

 
> >>>>+    - interrupts : <a b> where a is the interrupt number and b is a
> >>>
> >>>I2C doesn't do interrupts,
> >>
> >>...but some I2C devices do.
> > 
> > 
> > And accordingly, an OF device tree needs to be able to say which
> > interrupt is triggered by the device.  An RTC with an alarm (like
> > a ds1339 or rv5c387) is a perfectly reasonable thing to have on a
> > powerpc board, and expect the OS to take full advantage of.
> 
> Sure; I think his point was just that any device can have an interrupts 
> property, and it doesn't need to be explicitly mentioned for i2c because 
> i2c doesn't do anything special with it.

OK, I see.  Then for the benefit of folk not well versed in OF, it
might be worth mentioning that in something like a "common properties"
section.

Also, in the sense that this document is addressed to Linux folk, it
would be good to identify the mapping from OF to i2c_board_info.

- Dave