| Message ID | 1424773744-15106-3-git-send-email-wens@csie.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Tuesday 24 February 2015 18:29:02 Chen-Yu Tsai wrote: > > + rsb@01f03400 { > + compatible = "allwinner,sun8i-a23-rsb"; > + reg = <0x01f03400 0x400>; > + interrupts = <0 39 4>; > + clocks = <&apb0_gates 3>; > + clock-frequency = <3000000>; > + resets = <&apb0_rst 3>; > + > + axp223: pmic@2d { > + compatible = "x-powers,axp223", "x-powers,axp221"; > + reg = <0x2d>; > + allwinner,rsb-hw-addr = <0x3e3>; > + > + /* ... */ > + }; > + }; The child node cannot have a 'reg' property if the parent does not have #address-cells/#size-cells. You should add these as mandatory properties in the list. I don't really understand the need for having two addresses (runtime and hardware). Could the runtime address be configured at runtime? Alternatively, could you use #address-cells=<2> and put both into 'reg'? Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 24, 2015 at 6:37 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Tuesday 24 February 2015 18:29:02 Chen-Yu Tsai wrote: >> >> + rsb@01f03400 { >> + compatible = "allwinner,sun8i-a23-rsb"; >> + reg = <0x01f03400 0x400>; >> + interrupts = <0 39 4>; >> + clocks = <&apb0_gates 3>; >> + clock-frequency = <3000000>; >> + resets = <&apb0_rst 3>; >> + >> + axp223: pmic@2d { >> + compatible = "x-powers,axp223", "x-powers,axp221"; >> + reg = <0x2d>; >> + allwinner,rsb-hw-addr = <0x3e3>; >> + >> + /* ... */ >> + }; >> + }; > > The child node cannot have a 'reg' property if the parent does not > have #address-cells/#size-cells. You should add these as mandatory > properties in the list. Oops. I'll add them. A few of the other i2c bindings seem to be missing it as well. Probably why I missed it. > I don't really understand the need for having two addresses (runtime > and hardware). Could the runtime address be configured at runtime? You can, though the driver doesn't support this. I don't think the I2C subsystem allows arbitrary device insertion during normal operation, but maybe i2c-dev? I've tried using different addresses for devices so they do get changed during the probe phase, just to be sure that the code works, and it's not just sitting at the address the bootloader used. In any case, the distinction is more like burnt-in or hardwired addresses vs software configurable addresses. > Alternatively, could you use #address-cells=<2> and put both into > 'reg'? I sort of looked into it, and it seems the I2C subsystem won't mind the extra address cell. Maxime also asked whether we could use 2 addresses, possibly with a reg-names property. Using 2 cells is simpler though. I'm open to other options, but I'd like to get some input, especially from the device tree maintainers, before I change it. Regards, ChenYu -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 24 February 2015 22:01:26 Chen-Yu Tsai wrote: > On Tue, Feb 24, 2015 at 6:37 PM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Tuesday 24 February 2015 18:29:02 Chen-Yu Tsai wrote: > >> > >> + rsb@01f03400 { > >> + compatible = "allwinner,sun8i-a23-rsb"; > >> + reg = <0x01f03400 0x400>; > >> + interrupts = <0 39 4>; > >> + clocks = <&apb0_gates 3>; > >> + clock-frequency = <3000000>; > >> + resets = <&apb0_rst 3>; > >> + > >> + axp223: pmic@2d { > >> + compatible = "x-powers,axp223", "x-powers,axp221"; > >> + reg = <0x2d>; > >> + allwinner,rsb-hw-addr = <0x3e3>; > >> + > >> + /* ... */ > >> + }; > >> + }; > > > I don't really understand the need for having two addresses (runtime > > and hardware). Could the runtime address be configured at runtime? > > You can, though the driver doesn't support this. I don't think the > I2C subsystem allows arbitrary device insertion during normal > operation, but maybe i2c-dev? I've tried using different addresses > for devices so they do get changed during the probe phase, just > to be sure that the code works, and it's not just sitting at > the address the bootloader used. > > In any case, the distinction is more like burnt-in or hardwired > addresses vs software configurable addresses. The simplest binding would the probably be to only put the hardware address into the 'reg' property and always assign the logical addresses dynamically. Would that add a lot of complexity or does it have any other downsides? arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 24, 2015 at 10:17 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Tuesday 24 February 2015 22:01:26 Chen-Yu Tsai wrote: >> On Tue, Feb 24, 2015 at 6:37 PM, Arnd Bergmann <arnd@arndb.de> wrote: >> > On Tuesday 24 February 2015 18:29:02 Chen-Yu Tsai wrote: >> >> >> >> + rsb@01f03400 { >> >> + compatible = "allwinner,sun8i-a23-rsb"; >> >> + reg = <0x01f03400 0x400>; >> >> + interrupts = <0 39 4>; >> >> + clocks = <&apb0_gates 3>; >> >> + clock-frequency = <3000000>; >> >> + resets = <&apb0_rst 3>; >> >> + >> >> + axp223: pmic@2d { >> >> + compatible = "x-powers,axp223", "x-powers,axp221"; >> >> + reg = <0x2d>; >> >> + allwinner,rsb-hw-addr = <0x3e3>; >> >> + >> >> + /* ... */ >> >> + }; >> >> + }; >> >> > I don't really understand the need for having two addresses (runtime >> > and hardware). Could the runtime address be configured at runtime? >> >> You can, though the driver doesn't support this. I don't think the >> I2C subsystem allows arbitrary device insertion during normal >> operation, but maybe i2c-dev? I've tried using different addresses >> for devices so they do get changed during the probe phase, just >> to be sure that the code works, and it's not just sitting at >> the address the bootloader used. >> >> In any case, the distinction is more like burnt-in or hardwired >> addresses vs software configurable addresses. > > The simplest binding would the probably be to only put the > hardware address into the 'reg' property and always assign the > logical addresses dynamically. > > Would that add a lot of complexity or does it have any other > downsides? The hardware address is 12 bits wide. Any address higher than 0x3ff will be rejected by the I2C core. The AC100 is at 0xe89. Assigning addresses dynamically means the driver has to keep a lookup table to map the hardware address to the logical address to issue the command to. There's also the issue of dynamically assigned address colliding with unlisted devices, though I think this would only happen in the development / bring up phase of the device. I think the first issue pretty much rules out putting the hardware address into 'reg'. Putting the logical address in the 'reg' property allows the user to poke unlisted devices using i2c-tools, though this is not so useful to the average user. Regards, ChenYu -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 24 February 2015 22:32:57 Chen-Yu Tsai wrote: > On Tue, Feb 24, 2015 at 10:17 PM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Tuesday 24 February 2015 22:01:26 Chen-Yu Tsai wrote: > >> On Tue, Feb 24, 2015 at 6:37 PM, Arnd Bergmann <arnd@arndb.de> wrote: > >> > On Tuesday 24 February 2015 18:29:02 Chen-Yu Tsai wrote: > >> >> > >> >> + rsb@01f03400 { > >> >> + compatible = "allwinner,sun8i-a23-rsb"; > >> >> + reg = <0x01f03400 0x400>; > >> >> + interrupts = <0 39 4>; > >> >> + clocks = <&apb0_gates 3>; > >> >> + clock-frequency = <3000000>; > >> >> + resets = <&apb0_rst 3>; > >> >> + > >> >> + axp223: pmic@2d { > >> >> + compatible = "x-powers,axp223", "x-powers,axp221"; > >> >> + reg = <0x2d>; > >> >> + allwinner,rsb-hw-addr = <0x3e3>; > >> >> + > >> >> + /* ... */ > >> >> + }; > >> >> + }; > >> > >> > I don't really understand the need for having two addresses (runtime > >> > and hardware). Could the runtime address be configured at runtime? > >> > >> You can, though the driver doesn't support this. I don't think the > >> I2C subsystem allows arbitrary device insertion during normal > >> operation, but maybe i2c-dev? I've tried using different addresses > >> for devices so they do get changed during the probe phase, just > >> to be sure that the code works, and it's not just sitting at > >> the address the bootloader used. > >> > >> In any case, the distinction is more like burnt-in or hardwired > >> addresses vs software configurable addresses. > > > > The simplest binding would the probably be to only put the > > hardware address into the 'reg' property and always assign the > > logical addresses dynamically. > > > > Would that add a lot of complexity or does it have any other > > downsides? > > The hardware address is 12 bits wide. Any address higher than > 0x3ff will be rejected by the I2C core. The AC100 is at 0xe89. > > Assigning addresses dynamically means the driver has to keep > a lookup table to map the hardware address to the logical > address to issue the command to. > > There's also the issue of dynamically assigned address colliding > with unlisted devices, though I think this would only happen in > the development / bring up phase of the device. > > I think the first issue pretty much rules out putting the > hardware address into 'reg'. > > Putting the logical address in the 'reg' property allows the > user to poke unlisted devices using i2c-tools, though this > is not so useful to the average user. Ok, fair enough. Your approach seems reasonable then, but it might be helpful to add your explanation to the changelog of the patch that introduces the binding. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/i2c/i2c-sunxi-rsb.txt b/Documentation/devicetree/bindings/i2c/i2c-sunxi-rsb.txt new file mode 100644 index 000000000000..90aa5066873c --- /dev/null +++ b/Documentation/devicetree/bindings/i2c/i2c-sunxi-rsb.txt @@ -0,0 +1,50 @@ + +* Allwinner RSB (Reduced Serial Bus) controller + +Required properties : + + - reg : Offset and length of the register set for the device. + - compatible : Should be "allwinner,sun8i-a23-rsb". + - interrupts : The interrupt line connected to the RSB peripheral. + - clocks : The gate clk connected to the RSB peripheral. + - resets : The reset line connected to the RSB peripheral. + +Optional properties : + + - clock-frequency : Desired RSB bus clock frequency in Hz. If not set + the default frequency is 100kHz. Maximum is 20MHz. + +An RSB device node may contain up to 15 child nodes each encoding an RSB +slave device. + +Slave device properties: + Required properties: + - reg : The runtime address used to access the device. + - allwinner,rsb-hw-addr : The RSB hardware address for the device. This + is only used when configuring the runtime + address of the device. + + Valid runtime addresses - There are only 15 valid runtime addresses: + + 0x17, 0x2d, 0x3a, 0x4e, 0x59, 0x63, 0x74, 0x8b, + 0x9c, 0xa6, 0xb1, 0xc5, 0xd2, 0xe8, 0xff + + +Example: + + rsb@01f03400 { + compatible = "allwinner,sun8i-a23-rsb"; + reg = <0x01f03400 0x400>; + interrupts = <0 39 4>; + clocks = <&apb0_gates 3>; + clock-frequency = <3000000>; + resets = <&apb0_rst 3>; + + axp223: pmic@2d { + compatible = "x-powers,axp223", "x-powers,axp221"; + reg = <0x2d>; + allwinner,rsb-hw-addr = <0x3e3>; + + /* ... */ + }; + };
Reduced Serial Bus (RSB) is an SMBus like bus used to communicate with some PMICs (like the AXP223) or other peripherals. The RSB DT bindings are pretty much the same as the one defined for the marvell's mv64xxx controller, with the additional RSB specific "allwinner,rsb-hw-addr" property for slave device nodes. Signed-off-by: Chen-Yu Tsai <wens@csie.org> --- .../devicetree/bindings/i2c/i2c-sunxi-rsb.txt | 50 ++++++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 Documentation/devicetree/bindings/i2c/i2c-sunxi-rsb.txt