| Message ID | 1418897591-18332-3-git-send-email-l.majewski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2014-12-18 at 11:13 +0100, Lukasz Majewski wrote: > Several new properties to allow PWM fan working as a cooling device have been > combined into this single commit. > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > --- > .../devicetree/bindings/hwmon/pwm-fan.txt | 28 ++++++++++++++++++++++ > 1 file changed, 28 insertions(+) > > diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt > index 610757c..3877810 100644 > --- a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt > +++ b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt > @@ -3,10 +3,38 @@ Bindings for a fan connected to the PWM lines > Required properties: > - compatible : "pwm-fan" > - pwms : the PWM that is used to control the PWM fan > +- cooling-pwm-values : PWM duty cycle values relative to > + cooling-max-pwm-value correspondig to > + proper cooling states > +- default-pulse-width : Property specifying default pulse width for FAN > + at system boot (zero to disable FAN on boot). > + Allowed range is 0 to 255 The 0..255 range seems somewhat random. Would be nicer to either use the approach of pwm-backlight (iotw, have the range go from the first to the last entry of cooling-pwm-values) or simply have be the duty lenght in NS as entries instead of the current indirection. I assumed your cooling-pwm-values are a [0..255] range as well instead of nanoseconds (would be good to make that more clear)? Also having more consistent names would be nice.. To take pwm-backlight as inspiration, cooling-levels and default-cooling-level would make it more clear the second property picks a default setting from the first one. One thing i do wonder, is having an explicit default setting useful? Should it not default to maximum cooling unless otherwise configured by either the thermal framework or sysfs ? > +Thorough description of the following bindings: > + cooling-min-state = <0>; > + cooling-max-state = <3>; > + #cooling-cells = <2>; > + thermal-zone { > + cpu_thermal: cpu-thermal { > + cooling-maps { > + map0 { > + trip = <&cpu_alert1>; > + cooling-device = <&fan0 0 1>; > + }; > + }; > + }; > + > +for PWM FAN used as cooling device can be found at: > +./Documentation/devicetree/bindings/thermal/thermal.txt > > Example: > pwm-fan { > compatible = "pwm-fan"; > status = "okay"; > pwms = <&pwm 0 10000 0>; > + cooling-min-state = <0>; > + cooling-max-state = <3>; > + #cooling-cells = <2>; > + cooling-pwm-values = <0 102 170 255>; > + default-pulse-width = <0>; > };
On 12/18/2014 02:13 AM, Lukasz Majewski wrote: > Several new properties to allow PWM fan working as a cooling device have been > combined into this single commit. > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > --- > .../devicetree/bindings/hwmon/pwm-fan.txt | 28 ++++++++++++++++++++++ > 1 file changed, 28 insertions(+) > > diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt > index 610757c..3877810 100644 > --- a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt > +++ b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt > @@ -3,10 +3,38 @@ Bindings for a fan connected to the PWM lines > Required properties: > - compatible : "pwm-fan" > - pwms : the PWM that is used to control the PWM fan > +- cooling-pwm-values : PWM duty cycle values relative to > + cooling-max-pwm-value correspondig to > + proper cooling states I don't understand what you mean with "relative to". Please elaborate. Do you mean "associated with" ? Where is "cooling-max-pwm-value" defined, and how is this all expected to relate to the maximum duty cycle value provided by the pwm device ? > +- default-pulse-width : Property specifying default pulse width for FAN > + at system boot (zero to disable FAN on boot). > + Allowed range is 0 to 255 You'll need dt maintainer approval for the new properties. One thing I wonder about though is why you use "default-pulse-width" and not "default-pwm". Seems to be arbitrary. I don't see "pulse-width" used anywhere in the upstream kernel. I am somewhat concerned that you define the new properties as mandatory. That means existing configurations will fail, which does not seem to be a good idea. It would be more appropriate to not configure the thermal device if the new properties are not provided. The newly introduced semantics also conflicts with the current semantics, which sets the initial duty cycle initially to the maximum allowed duty cycle as reported by the pwm device itself. Guenter > + > +Thorough description of the following bindings: > + cooling-min-state = <0>; > + cooling-max-state = <3>; > + #cooling-cells = <2>; > + thermal-zone { > + cpu_thermal: cpu-thermal { > + cooling-maps { > + map0 { > + trip = <&cpu_alert1>; > + cooling-device = <&fan0 0 1>; > + }; > + }; > + }; > + > +for PWM FAN used as cooling device can be found at: > +./Documentation/devicetree/bindings/thermal/thermal.txt > > Example: > pwm-fan { > compatible = "pwm-fan"; > status = "okay"; > pwms = <&pwm 0 10000 0>; > + cooling-min-state = <0>; > + cooling-max-state = <3>; > + #cooling-cells = <2>; > + cooling-pwm-values = <0 102 170 255>; > + default-pulse-width = <0>; > }; > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sjoerd, Thanks for your feedback and sorry for a late reply. > On Thu, 2014-12-18 at 11:13 +0100, Lukasz Majewski wrote: > > Several new properties to allow PWM fan working as a cooling device > > have been combined into this single commit. > > > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > > --- > > .../devicetree/bindings/hwmon/pwm-fan.txt | 28 > > ++++++++++++++++++++++ 1 file changed, 28 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt > > b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt index > > 610757c..3877810 100644 --- > > a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt +++ > > b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt @@ -3,10 > > +3,38 @@ Bindings for a fan connected to the PWM lines Required > > properties: > > - compatible : "pwm-fan" > > - pwms : the PWM that is used to control the PWM fan > > +- cooling-pwm-values : PWM duty cycle values relative to > > + cooling-max-pwm-value correspondig to > > + proper cooling states > > +- default-pulse-width : Property specifying default pulse > > width for FAN > > + at system boot (zero to disable FAN on > > boot). > > + Allowed range is 0 to 255 > > The 0..255 range seems somewhat random. Would be nicer to either use > the approach of pwm-backlight (iotw, have the range go from the first > to the last entry of cooling-pwm-values) I'm OK to change the default-pulse-width to be similar to "default-brightness-level" (as it is in Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt) > or simply have be the duty > lenght in NS as entries instead of the current indirection. I'd prefer to keep the indirection - as it is utilized in the current pwm-fan.c driver. > > I assumed your cooling-pwm-values are a [0..255] range as well instead > of nanoseconds (would be good to make that more clear)? Your assumption is correct. cooling-pwm-values are indeed in the [0..255] range. I will add this information in v2. > > Also having more consistent names would be nice.. To take > pwm-backlight as inspiration, cooling-levels and > default-cooling-level would make it more clear the second property > picks a default setting from the first one. I agree. > > One thing i do wonder, is having an explicit default setting useful? > Should it not default to maximum cooling unless otherwise configured > by either the thermal framework or sysfs ? Enabling pan to full RPM was the default behaviour in the current pwm-fan.c file. To be honest, there is no need to enable fan to full RPM speed in this board for following reasons: 1. In Odroid the FAN is optional (stacked on top of a heat sink) - very often it is just enough to only have the heat sink. 2. Odroid has thermal enabled by default and IMHO it would be more feasible to allow thermal to control fan from the very beginning. However, I can also understand if the policy for hwmon implies a rule to enable by default all fans to full RPM speed. > > > > +Thorough description of the following bindings: > > + cooling-min-state = <0>; > > + cooling-max-state = <3>; > > + #cooling-cells = <2>; > > + thermal-zone { > > + cpu_thermal: cpu-thermal { > > + cooling-maps { > > + map0 { > > + trip = <&cpu_alert1>; > > + cooling-device = <&fan0 0 1>; > > + }; > > + }; > > + }; > > + > > +for PWM FAN used as cooling device can be found at: > > +./Documentation/devicetree/bindings/thermal/thermal.txt > > > > Example: > > pwm-fan { > > compatible = "pwm-fan"; > > status = "okay"; > > pwms = <&pwm 0 10000 0>; > > + cooling-min-state = <0>; > > + cooling-max-state = <3>; > > + #cooling-cells = <2>; > > + cooling-pwm-values = <0 102 170 255>; > > + default-pulse-width = <0>; > > }; > >
On Fri, Dec 19, 2014 at 04:32:24PM +0100, Lukasz Majewski wrote: > Hi Sjoerd, > > Thanks for your feedback and sorry for a late reply. > > > On Thu, 2014-12-18 at 11:13 +0100, Lukasz Majewski wrote: > > > Several new properties to allow PWM fan working as a cooling device > > > have been combined into this single commit. > > > > > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > > > --- > > > .../devicetree/bindings/hwmon/pwm-fan.txt | 28 > > > ++++++++++++++++++++++ 1 file changed, 28 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt > > > b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt index > > > 610757c..3877810 100644 --- > > > a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt +++ > > > b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt @@ -3,10 > > > +3,38 @@ Bindings for a fan connected to the PWM lines Required > > > properties: > > > - compatible : "pwm-fan" > > > - pwms : the PWM that is used to control the PWM fan > > > +- cooling-pwm-values : PWM duty cycle values relative to > > > + cooling-max-pwm-value correspondig to > > > + proper cooling states > > > +- default-pulse-width : Property specifying default pulse > > > width for FAN > > > + at system boot (zero to disable FAN on > > > boot). > > > + Allowed range is 0 to 255 > > > > The 0..255 range seems somewhat random. Would be nicer to either use > > the approach of pwm-backlight (iotw, have the range go from the first > > to the last entry of cooling-pwm-values) > > I'm OK to change the default-pulse-width to be similar to > "default-brightness-level" (as it is in > Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt) > > > or simply have be the duty > > lenght in NS as entries instead of the current indirection. > > I'd prefer to keep the indirection - as it is utilized in the current > pwm-fan.c driver. > FWIW, devicetree information is supposed to be implementation independent. So this is a poor argument. > > Enabling pan to full RPM was the default behaviour in the current > pwm-fan.c file. > > To be honest, there is no need to enable fan to full RPM speed in this > board for following reasons: > 1. In Odroid the FAN is optional (stacked on top of a heat sink) - very > often it is just enough to only have the heat sink. > > 2. Odroid has thermal enabled by default and IMHO it would be more > feasible to allow thermal to control fan from the very beginning. > > However, I can also understand if the policy for hwmon implies a rule > to enable by default all fans to full RPM speed. > Why and how does that all suggest that the current default behavior should be changed ? Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Guenter, > On Fri, Dec 19, 2014 at 04:32:24PM +0100, Lukasz Majewski wrote: > > Hi Sjoerd, > > > > Thanks for your feedback and sorry for a late reply. > > > > > On Thu, 2014-12-18 at 11:13 +0100, Lukasz Majewski wrote: > > > > Several new properties to allow PWM fan working as a cooling > > > > device have been combined into this single commit. > > > > > > > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > > > > --- > > > > .../devicetree/bindings/hwmon/pwm-fan.txt | 28 > > > > ++++++++++++++++++++++ 1 file changed, 28 insertions(+) > > > > > > > > diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt > > > > b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt index > > > > 610757c..3877810 100644 --- > > > > a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt +++ > > > > b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt @@ -3,10 > > > > +3,38 @@ Bindings for a fan connected to the PWM lines Required > > > > properties: > > > > - compatible : "pwm-fan" > > > > - pwms : the PWM that is used to control the > > > > PWM fan +- cooling-pwm-values : PWM duty cycle values > > > > relative to > > > > + cooling-max-pwm-value correspondig > > > > to > > > > + proper cooling states > > > > +- default-pulse-width : Property specifying default pulse > > > > width for FAN > > > > + at system boot (zero to disable > > > > FAN on boot). > > > > + Allowed range is 0 to 255 > > > > > > The 0..255 range seems somewhat random. Would be nicer to either > > > use the approach of pwm-backlight (iotw, have the range go from > > > the first to the last entry of cooling-pwm-values) > > > > I'm OK to change the default-pulse-width to be similar to > > "default-brightness-level" (as it is in > > Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt) > > > > > or simply have be the duty > > > lenght in NS as entries instead of the current indirection. > > > > I'd prefer to keep the indirection - as it is utilized in the > > current pwm-fan.c driver. > > > FWIW, devicetree information is supposed to be implementation > independent. So this is a poor argument. Many other pwm drivers use the indirection - e.g. mentioned pwm-backlight. > > > > > Enabling pan to full RPM was the default behaviour in the current > > pwm-fan.c file. > > > > To be honest, there is no need to enable fan to full RPM speed in > > this board for following reasons: > > 1. In Odroid the FAN is optional (stacked on top of a heat sink) - > > very often it is just enough to only have the heat sink. > > > > 2. Odroid has thermal enabled by default and IMHO it would be more > > feasible to allow thermal to control fan from the very beginning. > > > > However, I can also understand if the policy for hwmon implies a > > rule to enable by default all fans to full RPM speed. > > > Why and how does that all suggest that the current default behavior > should be changed ? I wanted to avoid the unpleasant sound for full speed fan when thermal is not enabled by default. But as I said, I fully understand the policy and I would be happy to comply with it as thermal should reduce the fan speed anyway at boot time. > > Guenter
Hi Guenter, > On 12/18/2014 02:13 AM, Lukasz Majewski wrote: > > Several new properties to allow PWM fan working as a cooling device > > have been combined into this single commit. > > > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > > --- > > .../devicetree/bindings/hwmon/pwm-fan.txt | 28 > > ++++++++++++++++++++++ 1 file changed, 28 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt > > b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt index > > 610757c..3877810 100644 --- > > a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt +++ > > b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt @@ -3,10 > > +3,38 @@ Bindings for a fan connected to the PWM lines Required > > properties: > > - compatible : "pwm-fan" > > - pwms : the PWM that is used to control the PWM > > fan +- cooling-pwm-values : PWM duty cycle values relative to > > + cooling-max-pwm-value correspondig to > > + proper cooling states > > I don't understand what you mean with "relative to". Please elaborate. > Do you mean "associated with" ? I meant that cooling-pwm-values is no greater than cooling-max-pwm-value (which was present in some earlier version of this patch and had value of 255). This description is wrong and will be reworded. > > Where is "cooling-max-pwm-value" defined, It was present in some early version of this patch. > and how is this all expected > to relate to the maximum duty cycle value provided by the pwm device ? > > > +- default-pulse-width : Property specifying default pulse > > width for FAN > > + at system boot (zero to disable FAN on > > boot). > > + Allowed range is 0 to 255 > > You'll need dt maintainer approval for the new properties. I'm wondering how this patch series will get merged. It touches three different subsystems (thermal, hwmon and device tree for Exynos). For me it would be best to use thermal tree (hence Odroid U3 fan is supposed to work as a cooling device) with ACKs from other subsystems maintainers. > > One thing I wonder about though is why you use "default-pulse-width" > and not "default-pwm". Seems to be arbitrary. I don't see > "pulse-width" used anywhere in the upstream kernel. Believe or not I've also considered the default-pwm name. > > I am somewhat concerned that you define the new properties as > mandatory. That means existing configurations will fail, which does > not seem to be a good idea. It would be more appropriate to not > configure the thermal device if the new properties are not provided. Very good point. I will do that for v2. > > The newly introduced semantics also conflicts with the current > semantics, which sets the initial duty cycle initially to the maximum > allowed duty cycle as reported by the pwm device itself. Ok. I will stick to above policy and then the "default-pulse-width" property can be removed. > > Guenter > > > + > > +Thorough description of the following bindings: > > + cooling-min-state = <0>; > > + cooling-max-state = <3>; > > + #cooling-cells = <2>; > > + thermal-zone { > > + cpu_thermal: cpu-thermal { > > + cooling-maps { > > + map0 { > > + trip = <&cpu_alert1>; > > + cooling-device = <&fan0 0 1>; > > + }; > > + }; > > + }; > > + > > +for PWM FAN used as cooling device can be found at: > > +./Documentation/devicetree/bindings/thermal/thermal.txt > > > > Example: > > pwm-fan { > > compatible = "pwm-fan"; > > status = "okay"; > > pwms = <&pwm 0 10000 0>; > > + cooling-min-state = <0>; > > + cooling-max-state = <3>; > > + #cooling-cells = <2>; > > + cooling-pwm-values = <0 102 170 255>; > > + default-pulse-width = <0>; > > }; > > >
Hey Lukasz, Blame the holiday season for my late reply ;) On Fri, 2014-12-19 at 17:13 +0100, Lukasz Majewski wrote: > Hi Guenter, > > > On Fri, Dec 19, 2014 at 04:32:24PM +0100, Lukasz Majewski wrote: > > > Hi Sjoerd, > > > > > > Thanks for your feedback and sorry for a late reply. > > > > > > > On Thu, 2014-12-18 at 11:13 +0100, Lukasz Majewski wrote: > > > > > Several new properties to allow PWM fan working as a cooling > > > > > device have been combined into this single commit. > > > > > > > > > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > > > > > --- > > > > > .../devicetree/bindings/hwmon/pwm-fan.txt | 28 > > > > > ++++++++++++++++++++++ 1 file changed, 28 insertions(+) > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt > > > > > b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt index > > > > > 610757c..3877810 100644 --- > > > > > a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt +++ > > > > > b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt @@ -3,10 > > > > > +3,38 @@ Bindings for a fan connected to the PWM lines Required > > > > > properties: > > > > > - compatible : "pwm-fan" > > > > > - pwms : the PWM that is used to control the > > > > > PWM fan +- cooling-pwm-values : PWM duty cycle values > > > > > relative to > > > > > + cooling-max-pwm-value correspondig > > > > > to > > > > > + proper cooling states > > > > > +- default-pulse-width : Property specifying default pulse > > > > > width for FAN > > > > > + at system boot (zero to disable > > > > > FAN on boot). > > > > > + Allowed range is 0 to 255 > > > > > > > > The 0..255 range seems somewhat random. Would be nicer to either > > > > use the approach of pwm-backlight (iotw, have the range go from > > > > the first to the last entry of cooling-pwm-values) > > > > > > I'm OK to change the default-pulse-width to be similar to > > > "default-brightness-level" (as it is in > > > Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt) > > > > > > > or simply have be the duty > > > > lenght in NS as entries instead of the current indirection. > > > > > > I'd prefer to keep the indirection - as it is utilized in the > > > current pwm-fan.c driver. > > > > > FWIW, devicetree information is supposed to be implementation > > independent. So this is a poor argument. > > Many other pwm drivers use the indirection - e.g. mentioned > pwm-backlight. I don't specifically mind the indirection, i was just thinking out loud whether it added value (but if it's quite common, might indeed be good to keep the pattern). What i do dislike is the number of levels is being set to an arbitrary levels, as that will very rarely match the actual number of distinct pwm levels you One thing though, when following the pattern of the pwm-backlight driver; In pwm-backlight the highest index of brightness-levels is always 100% duty cycle.. On e.g. XU3 the vendor kernel never drives the fan at 100% duty (maximum of 91%). So it would be nice if the dt bindigns could model that e.g. by having: pwm-levels = <20>; // 21 distinct pwm levels valid-pwm-level = <5 15 18>; /* 5 15 and 18 are usable levels - pwm will default to highest level */ > > > Enabling pan to full RPM was the default behaviour in the current > > > pwm-fan.c file. > > > > > > To be honest, there is no need to enable fan to full RPM speed in > > > this board for following reasons: > > > 1. In Odroid the FAN is optional (stacked on top of a heat sink) - > > > very often it is just enough to only have the heat sink. > > > > > > 2. Odroid has thermal enabled by default and IMHO it would be more > > > feasible to allow thermal to control fan from the very beginning. > > > > > > However, I can also understand if the policy for hwmon implies a > > > rule to enable by default all fans to full RPM speed. > > > > > Why and how does that all suggest that the current default behavior > > should be changed ? > > I wanted to avoid the unpleasant sound for full speed fan when thermal > is not enabled by default. > > But as I said, I fully understand the policy and I would be happy to > comply with it as thermal should reduce the fan speed anyway at boot > time. Yeah, what happens on my XU3 is that u-boot sets the pwm to 100% duty and the thermal infrastructure turns it off as soon as it gets into control, which works quite nicely (and keeps my sanity as that fan at 100% is *loud*)... So if you want to avoid unpleasant sounds, just build with thermal :p
Hi Sjoerd, > Hey Lukasz, > > Blame the holiday season for my late reply ;) > > On Fri, 2014-12-19 at 17:13 +0100, Lukasz Majewski wrote: > > Hi Guenter, > > > > > On Fri, Dec 19, 2014 at 04:32:24PM +0100, Lukasz Majewski wrote: > > > > Hi Sjoerd, > > > > > > > > Thanks for your feedback and sorry for a late reply. > > > > > > > > > On Thu, 2014-12-18 at 11:13 +0100, Lukasz Majewski wrote: > > > > > > Several new properties to allow PWM fan working as a cooling > > > > > > device have been combined into this single commit. > > > > > > > > > > > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > > > > > > --- > > > > > > .../devicetree/bindings/hwmon/pwm-fan.txt | 28 > > > > > > ++++++++++++++++++++++ 1 file changed, 28 insertions(+) > > > > > > > > > > > > diff --git > > > > > > a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt > > > > > > b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt index > > > > > > 610757c..3877810 100644 --- > > > > > > a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt +++ > > > > > > b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt @@ > > > > > > -3,10 +3,38 @@ Bindings for a fan connected to the PWM > > > > > > lines Required properties: > > > > > > - compatible : "pwm-fan" > > > > > > - pwms : the PWM that is used to control the > > > > > > PWM fan +- cooling-pwm-values : PWM duty cycle values > > > > > > relative to > > > > > > + cooling-max-pwm-value > > > > > > correspondig to > > > > > > + proper cooling states > > > > > > +- default-pulse-width : Property specifying default > > > > > > pulse width for FAN > > > > > > + at system boot (zero to disable > > > > > > FAN on boot). > > > > > > + Allowed range is 0 to 255 > > > > > > > > > > The 0..255 range seems somewhat random. Would be nicer to > > > > > either use the approach of pwm-backlight (iotw, have the > > > > > range go from the first to the last entry of > > > > > cooling-pwm-values) > > > > > > > > I'm OK to change the default-pulse-width to be similar to > > > > "default-brightness-level" (as it is in > > > > Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt) > > > > > > > > > or simply have be the duty > > > > > lenght in NS as entries instead of the current indirection. > > > > > > > > I'd prefer to keep the indirection - as it is utilized in the > > > > current pwm-fan.c driver. > > > > > > > FWIW, devicetree information is supposed to be implementation > > > independent. So this is a poor argument. > > > > Many other pwm drivers use the indirection - e.g. mentioned > > pwm-backlight. > > I don't specifically mind the indirection, i was just thinking out > loud whether it added value (but if it's quite common, might indeed > be good to keep the pattern). What i do dislike is the number of > levels is being set to an arbitrary levels, as that will very rarely > match the actual number of distinct pwm levels you > > One thing though, when following the pattern of the pwm-backlight > driver; In pwm-backlight the highest index of brightness-levels is > always 100% duty cycle.. On e.g. XU3 the vendor kernel never drives > the fan at 100% duty (maximum of 91%). So it would be nice if the dt > bindigns could model that e.g. by having: > > pwm-levels = <20>; // 21 distinct pwm levels > valid-pwm-level = <5 15 18>; /* 5 15 and 18 are usable levels - pwm > will default to highest level */ > Could you review v2 of this patch series? http://www.spinics.net/lists/devicetree/msg63159.html > > > > > Enabling pan to full RPM was the default behaviour in the > > > > current pwm-fan.c file. > > > > > > > > To be honest, there is no need to enable fan to full RPM speed > > > > in this board for following reasons: > > > > 1. In Odroid the FAN is optional (stacked on top of a heat > > > > sink) - very often it is just enough to only have the heat sink. > > > > > > > > 2. Odroid has thermal enabled by default and IMHO it would be > > > > more feasible to allow thermal to control fan from the very > > > > beginning. > > > > > > > > However, I can also understand if the policy for hwmon implies a > > > > rule to enable by default all fans to full RPM speed. > > > > > > > Why and how does that all suggest that the current default > > > behavior should be changed ? > > > > I wanted to avoid the unpleasant sound for full speed fan when > > thermal is not enabled by default. > > > > But as I said, I fully understand the policy and I would be happy to > > comply with it as thermal should reduce the fan speed anyway at boot > > time. > > Yeah, what happens on my XU3 is that u-boot sets the pwm to 100% duty > and the thermal infrastructure turns it off as soon as it gets into > control, which works quite nicely (and keeps my sanity as that fan at > 100% is *loud*)... So if you want to avoid unpleasant sounds, just > build with thermal :p > >
On Wed, 2015-01-14 at 14:56 +0100, Lukasz Majewski wrote: > Hi Sjoerd, > Could you review v2 of this patch series? > > http://www.spinics.net/lists/devicetree/msg63159.html I skimmed it yesterday, I'll try to find some time to send you my review comments later in the week/start of next. Do you happen to have a git branch with these patches in? That would make it convenient for me to give your patches a spin on my XU3
diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt index 610757c..3877810 100644 --- a/Documentation/devicetree/bindings/hwmon/pwm-fan.txt +++ b/Documentation/devicetree/bindings/hwmon/pwm-fan.txt @@ -3,10 +3,38 @@ Bindings for a fan connected to the PWM lines Required properties: - compatible : "pwm-fan" - pwms : the PWM that is used to control the PWM fan +- cooling-pwm-values : PWM duty cycle values relative to + cooling-max-pwm-value correspondig to + proper cooling states +- default-pulse-width : Property specifying default pulse width for FAN + at system boot (zero to disable FAN on boot). + Allowed range is 0 to 255 + +Thorough description of the following bindings: + cooling-min-state = <0>; + cooling-max-state = <3>; + #cooling-cells = <2>; + thermal-zone { + cpu_thermal: cpu-thermal { + cooling-maps { + map0 { + trip = <&cpu_alert1>; + cooling-device = <&fan0 0 1>; + }; + }; + }; + +for PWM FAN used as cooling device can be found at: +./Documentation/devicetree/bindings/thermal/thermal.txt Example: pwm-fan { compatible = "pwm-fan"; status = "okay"; pwms = <&pwm 0 10000 0>; + cooling-min-state = <0>; + cooling-max-state = <3>; + #cooling-cells = <2>; + cooling-pwm-values = <0 102 170 255>; + default-pulse-width = <0>; };
Several new properties to allow PWM fan working as a cooling device have been combined into this single commit. Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> --- .../devicetree/bindings/hwmon/pwm-fan.txt | 28 ++++++++++++++++++++++ 1 file changed, 28 insertions(+)