linux-kernel - Re: [PATCH 2/2] hwmon: (pmbus/adp1050): Support adp1051 and adp1055

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: <55825e91-b111-4689-bb3e-ede2c241728d@roeck-us.net>
Date: Wed, 6 Nov 2024 07:55:30 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
 Alexis Cezar Torreno <alexisczezar.torreno@...log.com>
Cc: linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org, linux-hwmon@...r.kernel.org,
 Radu Sabau <radu.sabau@...log.com>, Jean Delvare <jdelvare@...e.com>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Jonathan Corbet <corbet@....net>,
 Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Subject: Re: [PATCH 2/2] hwmon: (pmbus/adp1050): Support adp1051 and adp1055

On 11/6/24 03:24, Andy Shevchenko wrote:
> On Wed, Nov 06, 2024 at 05:03:11PM +0800, Alexis Cezar Torreno wrote:
>> ADP1051: 6 PWM for I/O Voltage, I/O Current, Temperature
>> ADP1055: 6 PWM for I/O Voltage, I/O Current, Power, Temperature
> 
> Missing blank line and perhaps you can add Datasheet: tag(s) for these HW?
> (see `git log --no-merges --grep Datasheet:` for the example)
> 

Is that an official tag ? Frankly, if so, I think it is quite useless
in the patch description because datasheet locations keep changing.
I think it is much better to provide a link in the driver documentation.

>> Signed-off-by: Alexis Cezar Torreno <alexisczezar.torreno@...log.com>
> 
> ...
> 
>> --- a/drivers/hwmon/pmbus/adp1050.c
>> +++ b/drivers/hwmon/pmbus/adp1050.c
>> @@ -6,8 +6,8 @@
>>    */
>>   #include <linux/bits.h>
>>   #include <linux/i2c.h>
>> -#include <linux/mod_devicetable.h>
>>   #include <linux/module.h>
>> +#include <linux/mod_devicetable.h>
>>   
>>   #include "pmbus.h"
> 
> Stray change. This pure depends on the your `locale` settings.
> The original one seems using en_US.UTF-8 and it's perfectly fine.
> 

Agreed.

> ...
> 
>> +static struct pmbus_driver_info adp1051_info = {
>> +	.pages = 1,
>> +	.format[PSC_VOLTAGE_IN] = linear,
>> +	.format[PSC_VOLTAGE_OUT] = linear,
>> +	.format[PSC_CURRENT_IN] = linear,
>> +	.format[PSC_TEMPERATURE] = linear,
>> +	.func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_IIN | PMBUS_HAVE_VOUT
>> +		   | PMBUS_HAVE_IOUT | PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_VOUT
>> +		   | PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_STATUS_INPUT
>> +		   | PMBUS_HAVE_STATUS_TEMP,
> 
> I dunno if the other entries in the file are written in the same style, but
> usual one is
> 
> 	.func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_IIN | PMBUS_HAVE_VOUT |
> 		   PMBUS_HAVE_IOUT | PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_VOUT |
> 		   PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_STATUS_INPUT |
> 		   PMBUS_HAVE_STATUS_TEMP,
> 
> Or even more logically
> 
> 	.func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_IIN |
> 		   PMBUS_HAVE_VOUT | PMBUS_HAVE_IOUT |
> 		   PMBUS_HAVE_TEMP |
> 		   PMBUS_HAVE_STATUS_INPUT |
> 		   PMBUS_HAVE_STATUS_VOUT | PMBUS_HAVE_STATUS_IOUT |
> 		   PMBUS_HAVE_STATUS_TEMP,
> 
>> +};
>> +
>> +static struct pmbus_driver_info adp1055_info = {
>> +	.pages = 1,
>> +	.format[PSC_VOLTAGE_IN] = linear,
>> +	.format[PSC_VOLTAGE_OUT] = linear,
>> +	.format[PSC_CURRENT_IN] = linear,
>> +	.format[PSC_TEMPERATURE] = linear,
>> +	.func[0] = PMBUS_HAVE_VIN | PMBUS_HAVE_IIN | PMBUS_HAVE_VOUT
>> +		   | PMBUS_HAVE_IOUT | PMBUS_HAVE_TEMP2 | PMBUS_HAVE_TEMP3
>> +		   | PMBUS_HAVE_POUT | PMBUS_HAVE_STATUS_VOUT
>> +		   | PMBUS_HAVE_STATUS_IOUT | PMBUS_HAVE_STATUS_INPUT
>> +		   | PMBUS_HAVE_STATUS_TEMP,
> 
> Ditto.
> 

That one slipped through with the original driver submission.
I thought that checkpatch complains about that, but it turns out that
it doesn't. I agree, though, that the usual style should be used.

Guenter

>> +};
> 
> ...
> 
>>   static const struct i2c_device_id adp1050_id[] = {
>> -	{"adp1050"},
>> +	{ .name = "adp1050", .driver_data = (kernel_ulong_t)&adp1050_info},
>> +	{ .name = "adp1051", .driver_data = (kernel_ulong_t)&adp1051_info},
>> +	{ .name = "adp1055", .driver_data = (kernel_ulong_t)&adp1055_info},
>>   	{}
>>   };
> 
>> +
> 
> Stray blank line.
> 
>>   MODULE_DEVICE_TABLE(i2c, adp1050_id);
>