Re: [PATCH 3/4] hwmon: (oxp-sensors) Fix wording in code comment

[Guenter Roeck]

On Thu, Dec 26, 2024 at 11:56:12PM +0100, Tobias Jakobi wrote:
> 
> On 12/26/24 21:52, Guenter Roeck wrote:
> > On Thu, Dec 26, 2024 at 06:00:18PM +0100, tjakobi@xxxxxxxxxxxxxxxxxxxxx wrote:
> > > From: Tobias Jakobi <tjakobi@xxxxxxxxxxxxxxxxxxxxx>
> > > 
> > > Despite what the current comment says, the register is used
> > > both for reading and writing the PWM value.
> > > 
> > > Signed-off-by: Tobias Jakobi <tjakobi@xxxxxxxxxxxxxxxxxxxxx>
> > > ---
> > >   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.
> 

"reading and writing the PWM value". "target" implies writing. "register"
implies neither, so you could use that.

Guenter