| Message ID | 20171214103011.24713-2-miquel.raynal@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Miquel On Thu, Dec 14, 2017 at 11:30:01AM +0100, Miquel Raynal wrote: > +- marvell,thermal-zone-name: The name to identify the thermal zone > + within the sysfs, useful when multiple > + thermal zones are registered (AP, CPx...). I don't think that would be acceptable. DT is about describing the hardware. sysfs is a Linux implementation detail which is not tied to any specific hardware. If this is accepted, the property should be named 'linux,thermal-zone-name'. baruch
Hello Baruch, On Fri, 15 Dec 2017 10:27:59 +0200 Baruch Siach <baruch@tkos.co.il> wrote: > Hi Miquel > > On Thu, Dec 14, 2017 at 11:30:01AM +0100, Miquel Raynal wrote: > > +- marvell,thermal-zone-name: The name to identify the thermal zone > > + within the sysfs, useful when multiple > > + thermal zones are registered (AP, > > CPx...). > > I don't think that would be acceptable. DT is about describing the > hardware. sysfs is a Linux implementation detail which is not tied to > any specific hardware. If this is accepted, the property should be > named 'linux,thermal-zone-name'. You are right the sysfs mention should not appear in the description. Otherwise for the naming I'm not sure "linux," is a valid prefix in that case. Miquèl
HI Miquèl On Fri, Dec 15, 2017 at 09:32:03AM +0100, Miquel RAYNAL wrote: > On Fri, 15 Dec 2017 10:27:59 +0200 > Baruch Siach <baruch@tkos.co.il> wrote: > > > > On Thu, Dec 14, 2017 at 11:30:01AM +0100, Miquel Raynal wrote: > > > +- marvell,thermal-zone-name: The name to identify the thermal zone > > > + within the sysfs, useful when multiple > > > + thermal zones are registered (AP, > > > CPx...). > > > > I don't think that would be acceptable. DT is about describing the > > hardware. sysfs is a Linux implementation detail which is not tied to > > any specific hardware. If this is accepted, the property should be > > named 'linux,thermal-zone-name'. > > You are right the sysfs mention should not appear in the description. My comment was not about the description language. I don't think that this property is acceptable at all. But I'll let DT maintainers comment on that. > Otherwise for the naming I'm not sure "linux," is a valid prefix in > that case. We use the 'linux' prefix for input key names and led triggers, for example. baruch
Hi Miquel, On ven., déc. 15 2017, Miquel RAYNAL <miquel.raynal@free-electrons.com> wrote: > Hello Baruch, > > On Fri, 15 Dec 2017 10:27:59 +0200 > Baruch Siach <baruch@tkos.co.il> wrote: > >> Hi Miquel >> >> On Thu, Dec 14, 2017 at 11:30:01AM +0100, Miquel Raynal wrote: >> > +- marvell,thermal-zone-name: The name to identify the thermal zone >> > + within the sysfs, useful when multiple >> > + thermal zones are registered (AP, >> > CPx...). >> >> I don't think that would be acceptable. DT is about describing the >> hardware. sysfs is a Linux implementation detail which is not tied to >> any specific hardware. If this is accepted, the property should be >> named 'linux,thermal-zone-name'. > > You are right the sysfs mention should not appear in the description. > > Otherwise for the naming I'm not sure "linux," is a valid prefix in > that case. Actually the choice between linux or marvell make me realize that there is something wrong. Having a name associated to a device is something pretty usual with the device tree, however it is as the class device level, such as clock-names, line-name, or regulator-name. So in my opinion if we want to support naming from device tree it would be done for all the thermal device not just for the Marvell one. However I don't think we need it. For example for the clocks we created the name dynamically using of the base address of the register to keep them unique. Gregory > > Miquèl > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hello Baruch and Gregory, On Fri, 15 Dec 2017 09:44:19 +0100 Gregory CLEMENT <gregory.clement@free-electrons.com> wrote: > Hi Miquel, > > On ven., déc. 15 2017, Miquel RAYNAL > <miquel.raynal@free-electrons.com> wrote: > > > Hello Baruch, > > > > On Fri, 15 Dec 2017 10:27:59 +0200 > > Baruch Siach <baruch@tkos.co.il> wrote: > > > >> Hi Miquel > >> > >> On Thu, Dec 14, 2017 at 11:30:01AM +0100, Miquel Raynal wrote: > >> > +- marvell,thermal-zone-name: The name to identify the thermal > >> > zone > >> > + within the sysfs, useful when > >> > multiple > >> > + thermal zones are registered (AP, > >> > CPx...). > >> > >> I don't think that would be acceptable. DT is about describing the > >> hardware. sysfs is a Linux implementation detail which is not tied > >> to any specific hardware. If this is accepted, the property should > >> be named 'linux,thermal-zone-name'. > > > > You are right the sysfs mention should not appear in the > > description. Actually, you are right for all of it, this property should not exist, sorry for my too quick answer. > > > > Otherwise for the naming I'm not sure "linux," is a valid prefix in > > that case. Thank you both for your explanations, I was also wrong about the prefix. > > Actually the choice between linux or marvell make me realize that > there is something wrong. Having a name associated to a device is > something pretty usual with the device tree, however it is as the > class device level, such as clock-names, line-name, or > regulator-name. So in my opinion if we want to support naming from > device tree it would be done for all the thermal device not just for > the Marvell one. > > However I don't think we need it. For example for the clocks we > created the name dynamically using of the base address of the > register to keep them unique. I was convinced that dev_name's would be the same but after trying it on a 8040-DB, using dev_name(&pdev->dev) gives: f06f808c.thermal f2400078.thermal f4400078.thermal which I found meaningful enough. I will drop the property and use dev_name instead. I still need your help to solve one problem though: how to make the distinction between using "armada_thermal" (the previous name) and dev_name() ? If I don't it kind of breaks userspace, doesn't it ? Thank you, Miquèl
On Fri, Dec 15, 2017 at 11:52:30AM +0100, Miquel RAYNAL wrote: > Hello Baruch and Gregory, > > On Fri, 15 Dec 2017 09:44:19 +0100 > Gregory CLEMENT <gregory.clement@free-electrons.com> wrote: > > > Hi Miquel, > > > > On ven., déc. 15 2017, Miquel RAYNAL > > <miquel.raynal@free-electrons.com> wrote: > > > > > Hello Baruch, > > > > > > On Fri, 15 Dec 2017 10:27:59 +0200 > > > Baruch Siach <baruch@tkos.co.il> wrote: > > > > > >> Hi Miquel > > >> > > >> On Thu, Dec 14, 2017 at 11:30:01AM +0100, Miquel Raynal wrote: > > >> > +- marvell,thermal-zone-name: The name to identify the thermal > > >> > zone > > >> > + within the sysfs, useful when > > >> > multiple > > >> > + thermal zones are registered (AP, > > >> > CPx...). > > >> > > >> I don't think that would be acceptable. DT is about describing the > > >> hardware. sysfs is a Linux implementation detail which is not tied > > >> to any specific hardware. If this is accepted, the property should > > >> be named 'linux,thermal-zone-name'. > > > > > > You are right the sysfs mention should not appear in the > > > description. > > Actually, you are right for all of it, this property should not > exist, sorry for my too quick answer. > > > > > > > Otherwise for the naming I'm not sure "linux," is a valid prefix in > > > that case. > > Thank you both for your explanations, I was also wrong about the prefix. > > > > > Actually the choice between linux or marvell make me realize that > > there is something wrong. Having a name associated to a device is > > something pretty usual with the device tree, however it is as the > > class device level, such as clock-names, line-name, or > > regulator-name. So in my opinion if we want to support naming from > > device tree it would be done for all the thermal device not just for > > the Marvell one. > > > > However I don't think we need it. For example for the clocks we > > created the name dynamically using of the base address of the > > register to keep them unique. > > I was convinced that dev_name's would be the same but after trying it on > a 8040-DB, using dev_name(&pdev->dev) gives: > > f06f808c.thermal > f2400078.thermal > f4400078.thermal > > which I found meaningful enough. > > I will drop the property and use dev_name instead. I still need your > help to solve one problem though: how to make the distinction between > using "armada_thermal" (the previous name) and dev_name() ? If I don't > it kind of breaks userspace, doesn't it ? No. The /sys/devices/... or /sys/bus/platform/... paths and names are not guaranteed to be stable. These changed for every platform converted to DT for example. Userspace should be accessing things through /sys/class/... (or deal with changes). Rob
Hello Rob, On Fri, 15 Dec 2017 17:28:54 -0600 Rob Herring <robh@kernel.org> wrote: > On Fri, Dec 15, 2017 at 11:52:30AM +0100, Miquel RAYNAL wrote: > > Hello Baruch and Gregory, > > > > On Fri, 15 Dec 2017 09:44:19 +0100 > > Gregory CLEMENT <gregory.clement@free-electrons.com> wrote: > > > > > Hi Miquel, > > > > > > On ven., déc. 15 2017, Miquel RAYNAL > > > <miquel.raynal@free-electrons.com> wrote: > > > > > > > Hello Baruch, > > > > > > > > On Fri, 15 Dec 2017 10:27:59 +0200 > > > > Baruch Siach <baruch@tkos.co.il> wrote: > > > > > > > >> Hi Miquel > > > >> > > > >> On Thu, Dec 14, 2017 at 11:30:01AM +0100, Miquel Raynal > > > >> wrote: > > > >> > +- marvell,thermal-zone-name: The name to identify the > > > >> > thermal zone > > > >> > + within the sysfs, useful when > > > >> > multiple > > > >> > + thermal zones are registered > > > >> > (AP, CPx...). > > > >> > > > >> I don't think that would be acceptable. DT is about describing > > > >> the hardware. sysfs is a Linux implementation detail which is > > > >> not tied to any specific hardware. If this is accepted, the > > > >> property should be named 'linux,thermal-zone-name'. > > > > > > > > You are right the sysfs mention should not appear in the > > > > description. > > > > Actually, you are right for all of it, this property should not > > exist, sorry for my too quick answer. > > > > > > > > > > Otherwise for the naming I'm not sure "linux," is a valid > > > > prefix in that case. > > > > Thank you both for your explanations, I was also wrong about the > > prefix. > > > > > > Actually the choice between linux or marvell make me realize that > > > there is something wrong. Having a name associated to a device is > > > something pretty usual with the device tree, however it is as the > > > class device level, such as clock-names, line-name, or > > > regulator-name. So in my opinion if we want to support naming from > > > device tree it would be done for all the thermal device not just > > > for the Marvell one. > > > > > > However I don't think we need it. For example for the clocks we > > > created the name dynamically using of the base address of the > > > register to keep them unique. > > > > I was convinced that dev_name's would be the same but after trying > > it on a 8040-DB, using dev_name(&pdev->dev) gives: > > > > f06f808c.thermal > > f2400078.thermal > > f4400078.thermal > > > > which I found meaningful enough. > > > > I will drop the property and use dev_name instead. I still need your > > help to solve one problem though: how to make the distinction > > between using "armada_thermal" (the previous name) and dev_name() ? > > If I don't it kind of breaks userspace, doesn't it ? > > No. The /sys/devices/... or /sys/bus/platform/... paths and names are > not guaranteed to be stable. These changed for every platform > converted to DT for example. Userspace should be accessing things > through /sys/class/... (or deal with changes). Ok, thanks for the explanation. I will sent a v4 early next week about all the changes requested. Thank you all for reviewing. Miquèl
diff --git a/Documentation/devicetree/bindings/thermal/armada-thermal.txt b/Documentation/devicetree/bindings/thermal/armada-thermal.txt index 24aacf8948c5..1602dc2ee220 100644 --- a/Documentation/devicetree/bindings/thermal/armada-thermal.txt +++ b/Documentation/devicetree/bindings/thermal/armada-thermal.txt @@ -7,17 +7,36 @@ Required properties: marvell,armada375-thermal marvell,armada380-thermal marvell,armadaxp-thermal + marvell,armada-ap806-thermal + marvell,armada-cp110-thermal - reg: Device's register space. Two entries are expected, see the examples below. - The first one is required for the sensor register; - the second one is required for the control register - to be used for sensor initialization (a.k.a. calibration). + The first one points to the status register (4B). + The second one points to the control registers (8B). + Note: with legacy bindings, the second entry pointed + only on the so called "control MSB" ("control 1"), was + 4B wide and did not let the possibility to reach the + "control LSB" ("control 0") register. -Example: +Optional properties: +- marvell,thermal-zone-name: The name to identify the thermal zone + within the sysfs, useful when multiple + thermal zones are registered (AP, CPx...). + +Examples: + + /* Legacy bindings */ thermal@d0018300 { compatible = "marvell,armada370-thermal"; - reg = <0xd0018300 0x4 + reg = <0xd0018300 0x4 0xd0018304 0x4>; }; + + ap_thermal: thermal@6f8084 { + compatible = "marvell,armada-ap806-thermal"; + reg = <0x6f808C 0x4>, + <0x6f8084 0x8>; + marvell,thermal-zone-name = "ap_thermal_zone"; + };