Re: [PATCH v3 07/10] Documentation: dt-bindings: Add documents for PECI hwmon client drivers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 4/18/2018 7:32 AM, Rob Herring wrote:
On Tue, Apr 17, 2018 at 3:40 PM, Jae Hyun Yoo
<jae.hyun.yoo@xxxxxxxxxxxxxxx> wrote:
On 4/16/2018 4:51 PM, Jae Hyun Yoo wrote:

On 4/16/2018 4:22 PM, Jae Hyun Yoo wrote:

On 4/16/2018 11:14 AM, Rob Herring wrote:

On Tue, Apr 10, 2018 at 11:32:09AM -0700, Jae Hyun Yoo wrote:

This commit adds dt-bindings documents for PECI cputemp and dimmtemp
client
drivers.



[...]

+Example:
+    peci-bus@0 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        < more properties >
+
+        peci-dimmtemp@cpu0 {


unit-address is wrong.


Will fix it using the reg value.

It is a different bus from cputemp? Otherwise, you have conflicting
addresses. If that's the case, probably should make it clear by showing
different host adapters for each example.


It could be the same bus with cputemp. Also, client address sharing is
possible by PECI core if the functionality is different. I mean, cputemp and
dimmtemp targeting the same client is possible case like this.
peci-cputemp@30
peci-dimmtemp@30


Oh, I got your point. Probably, I should change these separate settings
into one like

peci-client@30 {
      compatible = "intel,peci-client";
      reg = <0x30>;
};

Then cputemp and dimmtemp drivers could refer the same compatible string.
Will rewrite it.


I've checked it again and realized that it should use function based node
name like:

peci-cputemp@30
peci-dimmtemp@30

If it use the same string like 'peci-client@30', the drivers cannot be
selectively enabled. The client address sharing way is well handled in PECI
core and this way would be better for the future implementations of other
PECI functional drivers such as crash dump driver and so on. So I'm going
change the unit-address only.

2 nodes at the same address is wrong (and soon dtc will warn you on
this). You have 2 potential options. The first is you need additional
address information in the DT if these are in fact 2 independent
devices. This could be something like a function number to use
something from PCI addressing. From what I found on PECI, it doesn't
seem to have anything like that. The 2nd option is you have a single
DT node which registers multiple hwmon devices. DT nodes and drivers
don't have to be 1-1. Don't design your DT nodes from how you want to
partition drivers in some OS.

Rob


Please correct me if I'm wrong but I'm still thinking that it is
possible. Also, I did compile it but dtc doesn't make a warning. Let me
show an another use case which is similar to this case:

In arch/arm/boot/dts/aspeed-g5.dtsi
[...]
lpc_host: lpc-host@80 {
        compatible = "aspeed,ast2500-lpc-host", "simple-mfd", "syscon";
        reg = <0x80 0x1e0>;
        reg-io-width = <4>;

        #address-cells = <1>;
        #size-cells = <1>;
        ranges = <0x0 0x80 0x1e0>;

        lpc_ctrl: lpc-ctrl@0 {
                compatible = "aspeed,ast2500-lpc-ctrl";
                reg = <0x0 0x80>;
                clocks = <&syscon ASPEED_CLK_GATE_LCLK>;
                status = "disabled";
        };

        lpc_snoop: lpc-snoop@0 {
                compatible = "aspeed,ast2500-lpc-snoop";
                reg = <0x0 0x80>;
                interrupts = <8>;
                status = "disabled";
        };
}
[...]

This is device tree setting for LPC interface and its child nodes.
LPC interface can be used as a multi-functional interface such as
snoop 80, KCS, SIO and so on. In this use case, lpc-ctrl@0 and
lpc-snoop@0 are sharing their address range from their individual
driver modules and they can be registered quite well through both
static dt or dynamic dtoverlay. PECI is also a multi-functional
interface which is similar to the above case, I think.

Thanks,

Jae
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux