| Message ID | 20240424090453.2292185-1-peter@korsgaard.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/2] dt-bindings: hwmon: pwm-fan: Document target-pwm property | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On 4/24/24 02:04, Peter Korsgaard wrote: > Similar to target-rpm from fan-common.yaml but for the PWM setting > (0..255). > > Signed-off-by: Peter Korsgaard <peter@korsgaard.com> > --- > Documentation/devicetree/bindings/hwmon/pwm-fan.yaml | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml b/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml > index 4e5abf7580cc..58513ff732af 100644 > --- a/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml > +++ b/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml > @@ -46,6 +46,14 @@ properties: > > "#cooling-cells": true > > + target-pwm: > + description: > + The default desired fan PWM. Unlike target-rpm, there is no "desired" here. If this is the default pwm, to be set, name and describe it accordingly. Guenter > + $ref: /schemas/types.yaml#/definitions/uint32 > + minimum: 0 > + maximum: 255 > + default: 255 > + > required: > - compatible > - pwms
On 4/24/24 02:04, Peter Korsgaard wrote: > For some use cases defaulting the PWM to full fan speed is not ideal > (noise, power consumption, ..), so support an optional target-pwm > property (0..255) to override the default PWM value. > > Signed-off-by: Peter Korsgaard <peter@korsgaard.com> > --- > drivers/hwmon/pwm-fan.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c > index b67bc9e833c0..ebdefbd5789c 100644 > --- a/drivers/hwmon/pwm-fan.c > +++ b/drivers/hwmon/pwm-fan.c > @@ -482,6 +482,7 @@ static int pwm_fan_probe(struct platform_device *pdev) > const struct hwmon_channel_info **channels; > u32 *fan_channel_config; > int channel_count = 1; /* We always have a PWM channel. */ > + u32 target_pwm = MAX_PWM; > int i; > > ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL); > @@ -527,11 +528,17 @@ static int pwm_fan_probe(struct platform_device *pdev) > > ctx->enable_mode = pwm_disable_reg_enable; > > + of_property_read_u32(dev->of_node, "target-pwm", &target_pwm); > + if (target_pwm > (u32)MAX_PWM) { > + dev_err(dev, "Invalid target-pwm: %u > %d\n", target_pwm, MAX_PWM); > + return -EINVAL; > + } > + > /* > - * Set duty cycle to maximum allowed and enable PWM output as well as > + * Set duty cycle to target and enable PWM output as well as > * the regulator. In case of error nothing is changed > */ > - ret = set_pwm(ctx, MAX_PWM); > + ret = set_pwm(ctx, target_pwm); > if (ret) { > dev_err(dev, "Failed to configure PWM: %d\n", ret); > return ret; A much better name would be default-pwm for the property name. target-pwm is misleading and doesn't really make sense because there is no "target", just a value that is being set. Guenter
On 4/24/24 02:04, Peter Korsgaard wrote: > For some use cases defaulting the PWM to full fan speed is not ideal > (noise, power consumption, ..), so support an optional target-pwm > property (0..255) to override the default PWM value. > > Signed-off-by: Peter Korsgaard <peter@korsgaard.com> > --- > drivers/hwmon/pwm-fan.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c > index b67bc9e833c0..ebdefbd5789c 100644 > --- a/drivers/hwmon/pwm-fan.c > +++ b/drivers/hwmon/pwm-fan.c > @@ -482,6 +482,7 @@ static int pwm_fan_probe(struct platform_device *pdev) > const struct hwmon_channel_info **channels; > u32 *fan_channel_config; > int channel_count = 1; /* We always have a PWM channel. */ > + u32 target_pwm = MAX_PWM; > int i; > > ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL); > @@ -527,11 +528,17 @@ static int pwm_fan_probe(struct platform_device *pdev) > > ctx->enable_mode = pwm_disable_reg_enable; > > + of_property_read_u32(dev->of_node, "target-pwm", &target_pwm); > + if (target_pwm > (u32)MAX_PWM) { Unnecessary type cast. Guenter > + dev_err(dev, "Invalid target-pwm: %u > %d\n", target_pwm, MAX_PWM); > + return -EINVAL; > + } > + > /* > - * Set duty cycle to maximum allowed and enable PWM output as well as > + * Set duty cycle to target and enable PWM output as well as > * the regulator. In case of error nothing is changed > */ > - ret = set_pwm(ctx, MAX_PWM); > + ret = set_pwm(ctx, target_pwm); > if (ret) { > dev_err(dev, "Failed to configure PWM: %d\n", ret); > return ret;
On 24/04/2024 16.10, Guenter Roeck wrote: > On 4/24/24 02:04, Peter Korsgaard wrote: >> Similar to target-rpm from fan-common.yaml but for the PWM setting >> (0..255). >> >> Signed-off-by: Peter Korsgaard <peter@korsgaard.com> >> --- >> Documentation/devicetree/bindings/hwmon/pwm-fan.yaml | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml >> b/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml >> index 4e5abf7580cc..58513ff732af 100644 >> --- a/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml >> +++ b/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml >> @@ -46,6 +46,14 @@ properties: >> "#cooling-cells": true >> + target-pwm: >> + description: >> + The default desired fan PWM. > > Unlike target-rpm, there is no "desired" here. If this is the default pwm, > to be set, name and describe it accordingly. OK, I will rename to default-pwm and send a v2 - Thanks.
On Wed, Apr 24, 2024 at 11:04:52AM +0200, Peter Korsgaard wrote: > Similar to target-rpm from fan-common.yaml but for the PWM setting > (0..255). IIRC, we have a map of RPMs to PWM duty cycle, so why can't you use that plus target-rpm? Anything new for existing fan bindings should ideally use what fan-common.yaml defined or be added to it. > > Signed-off-by: Peter Korsgaard <peter@korsgaard.com> > --- > Documentation/devicetree/bindings/hwmon/pwm-fan.yaml | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml b/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml > index 4e5abf7580cc..58513ff732af 100644 > --- a/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml > +++ b/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml > @@ -46,6 +46,14 @@ properties: > > "#cooling-cells": true > > + target-pwm: > + description: > + The default desired fan PWM. > + $ref: /schemas/types.yaml#/definitions/uint32 > + minimum: 0 > + maximum: 255 > + default: 255 > + > required: > - compatible > - pwms > -- > 2.39.2 >
On 4/24/24 15:04, Rob Herring wrote: > On Wed, Apr 24, 2024 at 11:04:52AM +0200, Peter Korsgaard wrote: >> Similar to target-rpm from fan-common.yaml but for the PWM setting >> (0..255). > > IIRC, we have a map of RPMs to PWM duty cycle, so why can't you > use that plus target-rpm? > target-rpm is the target fan speed. The property defined here is the default pwm to set when the device is instantiated. The two values are also orthogonal. The fan rpm is fan dependent. Each fan will require a different pwm value to reach the target speed. Trying to use target-rpm to set a default pwm value would really not make much if any sense. Guenter > Anything new for existing fan bindings should ideally use what > fan-common.yaml defined or be added to it. > >> >> Signed-off-by: Peter Korsgaard <peter@korsgaard.com> >> --- >> Documentation/devicetree/bindings/hwmon/pwm-fan.yaml | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml b/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml >> index 4e5abf7580cc..58513ff732af 100644 >> --- a/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml >> +++ b/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml >> @@ -46,6 +46,14 @@ properties: >> >> "#cooling-cells": true >> >> + target-pwm: >> + description: >> + The default desired fan PWM. >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + minimum: 0 >> + maximum: 255 >> + default: 255 >> + >> required: >> - compatible >> - pwms >> -- >> 2.39.2 >>
diff --git a/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml b/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml index 4e5abf7580cc..58513ff732af 100644 --- a/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml +++ b/Documentation/devicetree/bindings/hwmon/pwm-fan.yaml @@ -46,6 +46,14 @@ properties: "#cooling-cells": true + target-pwm: + description: + The default desired fan PWM. + $ref: /schemas/types.yaml#/definitions/uint32 + minimum: 0 + maximum: 255 + default: 255 + required: - compatible - pwms
Similar to target-rpm from fan-common.yaml but for the PWM setting (0..255). Signed-off-by: Peter Korsgaard <peter@korsgaard.com> --- Documentation/devicetree/bindings/hwmon/pwm-fan.yaml | 8 ++++++++ 1 file changed, 8 insertions(+)