fHi
now that the basic support for the sl28 board is upstream I'd want to
continue with the onboard management CPLD. Connected via I2C, the CPLD
has different building blocks at different internal addresses. Eg. there
is a watchog, GPIO, PWM, interrupt controller and a fan monitoring.
Somehow like a PMIC but without the regulator support. The basic
building blocks might be there multiple times, eg. there are two PWM
controllers (at different internal addresses) and multiple GPIO
controllers. But due to size constraints some of them are output-only
and some of them are input only. This board management CPLD might be
reused by other boards but with different components inside at
different addresses. Thus, naturally this is a MFD.
How would you implement this regarding the device tree binding? At the
moment there are individual drivers (like in gpio/, pwm/ watchdog/,..)
and one MFD driver which basically just exports one regmap, which is
fetched by the individual drivers by dev_get_regmap(pdev->dev.parent).
The current device tree binding is as follows:
&i2c {
sl28cpld@4a {
#address-cells = <1>;
#size-cells = <0>;
compatible = "kontron,sl28cpld";
reg = <0x4a>;
watchdog@4 {
compatible = "kontron,sl28cpld-wdt";
reg = <0x4>;
};
hwmon@b {
compatible = "kontron,sl28cpld-hwmon";
reg = <0xb>;
};
pwm0: pwm@c {
#pwm-cells = <2>;
compatible = "kontron,sl28cpld-pwm";
reg = <0xc>;
};
pwm1: pwm@e {
#pwm-cells = <2>;
compatible = "kontron,sl28cpld-pwm";
reg = <0xe>;
};
gpio0: gpio-controller@10 {
compatible = "kontron,sl28cpld-gpio";
reg = <0x10>;
interrupt-parent = <&gpio2>;
interrupts = <6 IRQ_TYPE_EDGE_FALLING>;
gpio-controller;
#gpio-cells = <2>;
gpio-line-names = "a", "b", "c";
interrupt-controller;
#interrupt-cells = <2>;
};
gpio1: gpio-controller@1a {
compatible = "kontron,sl28cpld-gpo";
reg = <0x1a>;
gpio-controller;
#gpio-cells = <2>;
};
intc: interrupt-controller@1c {
compatible = "kontron,sl28cpld-intc";
reg = <0x1c>;
interrupt-parent = <&gpio2>;
interrupts = <6 IRQ_TYPE_EDGE_FALLING>;
interrupt-controller;
#interrupt-cells = <2>;
};
[..snip..]
Note that the reg property is the internal offset of the building block.
Because there might be multiple variants of this CPLD on different
boards, the register map is not fixed. Thus individual drivers need to
know the base offset of their registers. At the moment, they read the
reg property using of_get_address(np, 0, NULL, NULL) to get the base
offset.
This is working but has some drawbacks. First of all, that might fall
into the category "this is no information about the hardware and thus
should not go into the device tree". Second, if there is an update in
the future I would like to be able to support also these CPLDs. Eg. you
can read a global version register and for example know that since this
version something has changed like the register map. Thus it might make
sense to have the base offsets inside the MFD base driver, where they
can be adjusted in _probe(). I guess that also falls into the first
argument to not have to much information in the device tree.
I've looked into how you could do the second implementation. The MFD can
pass the register offset via resources and IORESOURCE_REG like the
wm831x-core.c does it. Also the interrupt which in the device tree above
is a property of the children (which I think is kinda hacky), could be
a property of the mfd and passed to the children with IORESOURCE_IRQ.
What is still missing is the device tree binding. Eg. if I need to have
a phandle to the first pwm controller. Here, I could think of two
different implementations:
&i2c {
bmc: sl28cpld@4a {
#address-cells = <1>;
#size-cells = <0>;
compatible = "kontron,sl28cpld";
reg = <0x4a>;
interrupt-parent = <&gpio2>;
interrupts = <6 IRQ_TYPE_EDGE_FALLING>;
gpio-controller;
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
#pwm-cells = <2>;
};
&i2c {
sl28cpld@4a {
#address-cells = <1>;
#size-cells = <0>;
compatible = "kontron,sl28cpld";
reg = <0x4a>;
watchdog {
compatible = "kontron,sl28cpld-wdt";
};
hwmon {
compatible = "kontron,sl28cpld-hwmon";
};
pwm0: pwm@0 {
#pwm-cells = <2>;
compatible = "kontron,sl28cpld-pwm";
reg = <0>;
};
pwm1: pwm@1 {
#pwm-cells = <2>;
compatible = "kontron,sl28cpld-pwm";
reg = <1>;
};
gpio0: gpio-controller@0 {
compatible = "kontron,sl28cpld-gpio";
reg = <0>;
gpio-controller;
#gpio-cells = <2>;
gpio-line-names = "a", "b", "c";
interrupt-controller;
#interrupt-cells = <2>;
};
gpio1: gpio-controller@1a {
compatible = "kontron,sl28cpld-gpo";
reg = <0x1a>;
gpio-controller;
#gpio-cells = <2>;
};
intc: interrupt-controller {
compatible = "kontron,sl28cpld-intc";
interrupt-controller;
#interrupt-cells = <2>;
};
[..snip..]
};
The first implementation would just be some kind of super node which
exposes all features, eg. you'd do "pwms = <&bmc 0>;" or "gpios = <&bmc
0 GPIO_ACTIVE_LOW>;". I don't know it this is a good idea or if this is
even possible.
The second one is almost like the current implemention only that there
are no register offsets or irqs in the child nodes. But because there
might be two children of the same building block (eg. two pwm nodes)
the reg property is now an ID.
Phew, that was a long mail. I wanted to know your thoughts/ideas if
(1) the current solution would be accepted, (if one would life with the
drawback of not being able to detect multiple verions of the CPLD
on runtime)
(2) whould you do the super-node contains everything or the more
elaborate device tree.
I tend to have the last one (the more elaborate device tree). The mfd
core can already match mfd_cells to device tree nodes, but only if the
compatible string is unique (see mfd_add_device()). Eg. the current code
would match the mfd cells with of_compatible = "kontron,sl28cpld-pwm"
only to the first node. I thought of also comparing the reg property
against the id property of struct mfd_cell.
-michael
