| Message ID | 1423088830-31671-1-git-send-email-alexander.deucher@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am 04.02.2015 um 23:27 schrieb Alex Deucher: > 0-255 seems to be the preferred range for the pwm interface. > > Signed-off-by: Alex Deucher <alexander.deucher@amd.com> Yeah, using 100 on a 8bit pwm timer sounds rather obviously wrong. Patch is Reviewed-by: Christian König <christian.koenig@amd.com> > --- > drivers/gpu/drm/radeon/radeon_pm.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/radeon/radeon_pm.c b/drivers/gpu/drm/radeon/radeon_pm.c > index 91e1bd2..9f758d3 100644 > --- a/drivers/gpu/drm/radeon/radeon_pm.c > +++ b/drivers/gpu/drm/radeon/radeon_pm.c > @@ -585,7 +585,7 @@ static ssize_t radeon_hwmon_set_pwm1_enable(struct device *dev, > if (err) > return err; > > - switch(value) { > + switch (value) { > case 1: /* manual, percent-based */ > rdev->asic->dpm.fan_ctrl_set_mode(rdev, FDO_PWM_MODE_STATIC); > break; > @@ -608,7 +608,7 @@ static ssize_t radeon_hwmon_get_pwm1_max(struct device *dev, > struct device_attribute *attr, > char *buf) > { > - return sprintf(buf, "%i\n", 100); /* pwm uses percent-based fan-control */ > + return sprintf(buf, "%i\n", 255); > } > > static ssize_t radeon_hwmon_set_pwm1(struct device *dev, > @@ -623,6 +623,8 @@ static ssize_t radeon_hwmon_set_pwm1(struct device *dev, > if (err) > return err; > > + value = (value * 100) / 255; > + > err = rdev->asic->dpm.set_fan_speed_percent(rdev, value); > if (err) > return err; > @@ -642,6 +644,8 @@ static ssize_t radeon_hwmon_get_pwm1(struct device *dev, > if (err) > return err; > > + speed = (speed * 255) / 100; > + > return sprintf(buf, "%i\n", speed); > } >
On 05/02/2015 11:49, Christian König wrote: > Am 04.02.2015 um 23:27 schrieb Alex Deucher: >> 0-255 seems to be the preferred range for the pwm interface. >> >> Signed-off-by: Alex Deucher <alexander.deucher@amd.com> > Yeah, using 100 on a 8bit pwm timer sounds rather obviously wrong. > > Patch is Reviewed-by: Christian König <christian.koenig@amd.com> > >> --- >> drivers/gpu/drm/radeon/radeon_pm.c | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/radeon/radeon_pm.c b/drivers/gpu/drm/radeon/radeon_pm.c >> index 91e1bd2..9f758d3 100644 >> --- a/drivers/gpu/drm/radeon/radeon_pm.c >> +++ b/drivers/gpu/drm/radeon/radeon_pm.c >> @@ -585,7 +585,7 @@ static ssize_t radeon_hwmon_set_pwm1_enable(struct device *dev, >> if (err) >> return err; >> >> - switch(value) { >> + switch (value) { >> case 1: /* manual, percent-based */ >> rdev->asic->dpm.fan_ctrl_set_mode(rdev, FDO_PWM_MODE_STATIC); >> break; >> @@ -608,7 +608,7 @@ static ssize_t radeon_hwmon_get_pwm1_max(struct device *dev, >> struct device_attribute *attr, >> char *buf) >> { >> - return sprintf(buf, "%i\n", 100); /* pwm uses percent-based fan-control */ >> + return sprintf(buf, "%i\n", 255); >> } This is not a standard name as it is not documented in https://www.kernel.org/doc/Documentation/hwmon/sysfs-interface Apparently, the pwm value should always have been exposed as 0-255 and I screwed it up in nouveau by having it be 0-100... Maybe we should ask pwm*_max to be defined so as new applications could do the right thing while not having nouveau change its ABI. I guess it is ok to change it for radeon as there are so few users currently but the interface has been in nouveau for multiple years already!
On Sat, Apr 11, 2015 at 5:52 AM, Martin Peres <martin.peres@free.fr> wrote: > On 05/02/2015 11:49, Christian König wrote: >> >> Am 04.02.2015 um 23:27 schrieb Alex Deucher: >>> >>> 0-255 seems to be the preferred range for the pwm interface. >>> >>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com> >> >> Yeah, using 100 on a 8bit pwm timer sounds rather obviously wrong. >> >> Patch is Reviewed-by: Christian König <christian.koenig@amd.com> >> >>> --- >>> drivers/gpu/drm/radeon/radeon_pm.c | 8 ++++++-- >>> 1 file changed, 6 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/radeon/radeon_pm.c >>> b/drivers/gpu/drm/radeon/radeon_pm.c >>> index 91e1bd2..9f758d3 100644 >>> --- a/drivers/gpu/drm/radeon/radeon_pm.c >>> +++ b/drivers/gpu/drm/radeon/radeon_pm.c >>> @@ -585,7 +585,7 @@ static ssize_t radeon_hwmon_set_pwm1_enable(struct >>> device *dev, >>> if (err) >>> return err; >>> - switch(value) { >>> + switch (value) { >>> case 1: /* manual, percent-based */ >>> rdev->asic->dpm.fan_ctrl_set_mode(rdev, >>> FDO_PWM_MODE_STATIC); >>> break; >>> @@ -608,7 +608,7 @@ static ssize_t radeon_hwmon_get_pwm1_max(struct >>> device *dev, >>> struct device_attribute *attr, >>> char *buf) >>> { >>> - return sprintf(buf, "%i\n", 100); /* pwm uses percent-based >>> fan-control */ >>> + return sprintf(buf, "%i\n", 255); >>> } > > > This is not a standard name as it is not documented in > https://www.kernel.org/doc/Documentation/hwmon/sysfs-interface > > Apparently, the pwm value should always have been exposed as 0-255 and I > screwed it up in nouveau by having it be 0-100... > > Maybe we should ask pwm*_max to be defined so as new applications could do > the right thing while not having nouveau change its ABI. I guess it is ok to > change it for radeon as there are so few users currently but the interface > has been in nouveau for multiple years already! > IIRC, I changed it in the same kernel as the original patch went in. Alex
On 13/04/2015 02:42, Alex Deucher wrote: > On Sat, Apr 11, 2015 at 5:52 AM, Martin Peres <martin.peres@free.fr> wrote: >> On 05/02/2015 11:49, Christian König wrote: >>> Am 04.02.2015 um 23:27 schrieb Alex Deucher: >>>> 0-255 seems to be the preferred range for the pwm interface. >>>> >>>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com> >>> Yeah, using 100 on a 8bit pwm timer sounds rather obviously wrong. >>> >>> Patch is Reviewed-by: Christian König <christian.koenig@amd.com> >>> >>>> --- >>>> drivers/gpu/drm/radeon/radeon_pm.c | 8 ++++++-- >>>> 1 file changed, 6 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/radeon/radeon_pm.c >>>> b/drivers/gpu/drm/radeon/radeon_pm.c >>>> index 91e1bd2..9f758d3 100644 >>>> --- a/drivers/gpu/drm/radeon/radeon_pm.c >>>> +++ b/drivers/gpu/drm/radeon/radeon_pm.c >>>> @@ -585,7 +585,7 @@ static ssize_t radeon_hwmon_set_pwm1_enable(struct >>>> device *dev, >>>> if (err) >>>> return err; >>>> - switch(value) { >>>> + switch (value) { >>>> case 1: /* manual, percent-based */ >>>> rdev->asic->dpm.fan_ctrl_set_mode(rdev, >>>> FDO_PWM_MODE_STATIC); >>>> break; >>>> @@ -608,7 +608,7 @@ static ssize_t radeon_hwmon_get_pwm1_max(struct >>>> device *dev, >>>> struct device_attribute *attr, >>>> char *buf) >>>> { >>>> - return sprintf(buf, "%i\n", 100); /* pwm uses percent-based >>>> fan-control */ >>>> + return sprintf(buf, "%i\n", 255); >>>> } >> >> This is not a standard name as it is not documented in >> https://www.kernel.org/doc/Documentation/hwmon/sysfs-interface >> >> Apparently, the pwm value should always have been exposed as 0-255 and I >> screwed it up in nouveau by having it be 0-100... >> >> Maybe we should ask pwm*_max to be defined so as new applications could do >> the right thing while not having nouveau change its ABI. I guess it is ok to >> change it for radeon as there are so few users currently but the interface >> has been in nouveau for multiple years already! >> > IIRC, I changed it in the same kernel as the original patch went in. > > Alex Wonderful!
diff --git a/drivers/gpu/drm/radeon/radeon_pm.c b/drivers/gpu/drm/radeon/radeon_pm.c index 91e1bd2..9f758d3 100644 --- a/drivers/gpu/drm/radeon/radeon_pm.c +++ b/drivers/gpu/drm/radeon/radeon_pm.c @@ -585,7 +585,7 @@ static ssize_t radeon_hwmon_set_pwm1_enable(struct device *dev, if (err) return err; - switch(value) { + switch (value) { case 1: /* manual, percent-based */ rdev->asic->dpm.fan_ctrl_set_mode(rdev, FDO_PWM_MODE_STATIC); break; @@ -608,7 +608,7 @@ static ssize_t radeon_hwmon_get_pwm1_max(struct device *dev, struct device_attribute *attr, char *buf) { - return sprintf(buf, "%i\n", 100); /* pwm uses percent-based fan-control */ + return sprintf(buf, "%i\n", 255); } static ssize_t radeon_hwmon_set_pwm1(struct device *dev, @@ -623,6 +623,8 @@ static ssize_t radeon_hwmon_set_pwm1(struct device *dev, if (err) return err; + value = (value * 100) / 255; + err = rdev->asic->dpm.set_fan_speed_percent(rdev, value); if (err) return err; @@ -642,6 +644,8 @@ static ssize_t radeon_hwmon_get_pwm1(struct device *dev, if (err) return err; + speed = (speed * 255) / 100; + return sprintf(buf, "%i\n", speed); }
0-255 seems to be the preferred range for the pwm interface. Signed-off-by: Alex Deucher <alexander.deucher@amd.com> --- drivers/gpu/drm/radeon/radeon_pm.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)