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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <eaca7150-a259-40a7-8faa-b4ad7897f375@roeck-us.net>
Date: Tue, 7 Jan 2025 09:24:50 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Tobias Jakobi <tjakobi@...h.uni-bielefeld.de>
Cc: Derek John Clark <derekjohn.clark@...il.com>,
	Joaquín Ignacio Aramendía <samsagax@...il.com>,
	Jean Delvare <jdelvare@...e.com>, linux-hwmon@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/4] hwmon: (oxp-sensors) Fix wording in code comment

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@...h.uni-bielefeld.de wrote:
> > > From: Tobias Jakobi <tjakobi@...h.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@...h.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.
> 

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

Guenter