Re: [patch 3/3] mpc8349emitx.dts: Add ds1339 RTC

[Segher Boessenkool <segher@kernel.crashing.org>]

>>  Scott> #size-cells is zero on i2c, so it should just be reg = <68>.
>>
>>  Scott> You'll probably need to add #address-cells and #size-cells to 
>> the
>>  Scott> controller node, as well.
>
> Uh.. yes.. i2c interfaces should really always have #a and #s.

More generally, every node that defines a bus needs it (unless the
defaults of 2 resp. 1 are correct for this bus, but even then you
might want it because it makes things more explicit).

>>  		i2c@3100 {
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>>  			device_type = "i2c";
>
> Hrm... we probably want an "i2c" device_type class, but I don't think
> we've actually defined one, which is a problem

By defining new device_type's, or new semantics for device_type,
you open the door to (accidentally) becoming incompatible with
"real" OF.

And you don't need to: "real" OF has a mechanism for specifying
the "generic device class" already, if you use the "generic names"
recommended practice (and you do, for both this node and the rtc
node): it's the generic name itself!

>> +			rtc@68 {
>> +				device_type = "rtc";
>> +				compatible = "dallas,ds1339";
>> +				reg = <68>;
>> +			};
>
> I think we want to think a bit more carefully about how to do bindings
> for RTC devices.  No "rtc" device_type is defined, but again we might
> want to.

Actually, "device_type" = "rtc" _is_ defined (in the "device support
extensions" recommended practice), and there is no useful way a flat
device tree can implement it (it merely defines get-time and set-time
methods).

> The fact that NVRAM+RTC chips are so common is a bit of an issue from
> the point of view of defining a device class binding - a device can't
> have type "rtc" and "nvram".

You should match OS drivers on "compatible" only anyway.

Those cases where OS drivers don't nicely 1-1 match device nodes are a
bit of a headache; for RTC/NVRAM devices, these problems are nicely
side-stepped by handling this from platform code.


Segher