| Message ID | 858c2a5b712eebdf2fc7c9c6e3a2d2f832a68dfc.1735232354.git.tjakobi@math.uni-bielefeld.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | hwmon: (oxp-sensors) Cleanup device type handling | expand |
On Thu, Dec 26, 2024 at 06:00:18PM +0100, tjakobi@math.uni-bielefeld.de wrote: > From: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> > > Despite what the current comment says, the register is used > both for reading and writing the PWM value. > > Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> > --- > drivers/hwmon/oxp-sensors.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c > index fbd1463d1494..8089349fa508 100644 > --- a/drivers/hwmon/oxp-sensors.c > +++ b/drivers/hwmon/oxp-sensors.c > @@ -46,14 +46,14 @@ static bool unlock_global_acpi_lock(void) > #define OXP_SENSOR_FAN_REG 0x76 /* Fan reading is 2 registers long */ > #define OXP_2_SENSOR_FAN_REG 0x58 /* Fan reading is 2 registers long */ > #define OXP_SENSOR_PWM_ENABLE_REG 0x4A /* PWM enable is 1 register long */ > -#define OXP_SENSOR_PWM_REG 0x4B /* PWM reading is 1 register long */ > +#define OXP_SENSOR_PWM_REG 0x4B /* PWM control is 1 register long */ I think that, if anything, this is more confusing than before. "control" is, for example, enabling or disabling pwm management, setting automatic or manual mode, or setting the pwm polarity. Together ith the next two defines, "control" would suggest that PWM_MODE_AUTO and PWM_MODE_MANUAL are set through that register - which is not the case. "value" maybe, but "control" is just wrong. Guenter > #define PWM_MODE_AUTO 0x00 > #define PWM_MODE_MANUAL 0x01 > > /* OrangePi fan reading and PWM */ > #define ORANGEPI_SENSOR_FAN_REG 0x78 /* Fan reading is 2 registers long */ > #define ORANGEPI_SENSOR_PWM_ENABLE_REG 0x40 /* PWM enable is 1 register long */ > -#define ORANGEPI_SENSOR_PWM_REG 0x38 /* PWM reading is 1 register long */ > +#define ORANGEPI_SENSOR_PWM_REG 0x38 /* PWM control is 1 register long */ > > /* Turbo button takeover function > * Different boards have different values and EC registers > -- > 2.45.2 > >
On 12/26/24 21:52, Guenter Roeck wrote: > On Thu, Dec 26, 2024 at 06:00:18PM +0100, tjakobi@math.uni-bielefeld.de wrote: >> From: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> >> >> Despite what the current comment says, the register is used >> both for reading and writing the PWM value. >> >> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de> >> --- >> drivers/hwmon/oxp-sensors.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c >> index fbd1463d1494..8089349fa508 100644 >> --- a/drivers/hwmon/oxp-sensors.c >> +++ b/drivers/hwmon/oxp-sensors.c >> @@ -46,14 +46,14 @@ static bool unlock_global_acpi_lock(void) >> #define OXP_SENSOR_FAN_REG 0x76 /* Fan reading is 2 registers long */ >> #define OXP_2_SENSOR_FAN_REG 0x58 /* Fan reading is 2 registers long */ >> #define OXP_SENSOR_PWM_ENABLE_REG 0x4A /* PWM enable is 1 register long */ >> -#define OXP_SENSOR_PWM_REG 0x4B /* PWM reading is 1 register long */ >> +#define OXP_SENSOR_PWM_REG 0x4B /* PWM control is 1 register long */ > > I think that, if anything, this is more confusing than before. > "control" is, for example, enabling or disabling pwm management, > setting automatic or manual mode, or setting the pwm polarity. > Together ith the next two defines, "control" would suggest that > PWM_MODE_AUTO and PWM_MODE_MANUAL are set through that register - > which is not the case. "value" maybe, but "control" is just wrong. Noted. What do you think about "target" then? My main point here was that reading implies that this register is read-only. Which it clearly isn't. And the documentation (which could be certainly be improved in general) should reflect that. With best wishes, Tobias > > Guenter > >> #define PWM_MODE_AUTO 0x00 >> #define PWM_MODE_MANUAL 0x01 >> >> /* OrangePi fan reading and PWM */ >> #define ORANGEPI_SENSOR_FAN_REG 0x78 /* Fan reading is 2 registers long */ >> #define ORANGEPI_SENSOR_PWM_ENABLE_REG 0x40 /* PWM enable is 1 register long */ >> -#define ORANGEPI_SENSOR_PWM_REG 0x38 /* PWM reading is 1 register long */ >> +#define ORANGEPI_SENSOR_PWM_REG 0x38 /* PWM control is 1 register long */ >> >> /* Turbo button takeover function >> * Different boards have different values and EC registers >> -- >> 2.45.2 >> >>
diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c index fbd1463d1494..8089349fa508 100644 --- a/drivers/hwmon/oxp-sensors.c +++ b/drivers/hwmon/oxp-sensors.c @@ -46,14 +46,14 @@ static bool unlock_global_acpi_lock(void) #define OXP_SENSOR_FAN_REG 0x76 /* Fan reading is 2 registers long */ #define OXP_2_SENSOR_FAN_REG 0x58 /* Fan reading is 2 registers long */ #define OXP_SENSOR_PWM_ENABLE_REG 0x4A /* PWM enable is 1 register long */ -#define OXP_SENSOR_PWM_REG 0x4B /* PWM reading is 1 register long */ +#define OXP_SENSOR_PWM_REG 0x4B /* PWM control is 1 register long */ #define PWM_MODE_AUTO 0x00 #define PWM_MODE_MANUAL 0x01 /* OrangePi fan reading and PWM */ #define ORANGEPI_SENSOR_FAN_REG 0x78 /* Fan reading is 2 registers long */ #define ORANGEPI_SENSOR_PWM_ENABLE_REG 0x40 /* PWM enable is 1 register long */ -#define ORANGEPI_SENSOR_PWM_REG 0x38 /* PWM reading is 1 register long */ +#define ORANGEPI_SENSOR_PWM_REG 0x38 /* PWM control is 1 register long */ /* Turbo button takeover function * Different boards have different values and EC registers