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] [day] [month] [year] [list]
Message-ID:
 <PH0PR03MB6351CDD33902BBBEE58A126DF15C2@PH0PR03MB6351.namprd03.prod.outlook.com>
Date: Thu, 7 Nov 2024 02:06:46 +0000
From: "Torreno, Alexis Czezar" <AlexisCzezar.Torreno@...log.com>
To: Guenter Roeck <linux@...ck-us.net>,
        Andy Shevchenko
	<andriy.shevchenko@...ux.intel.com>
CC: "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-hwmon@...r.kernel.org" <linux-hwmon@...r.kernel.org>,
        "Sabau, Radu
 bogdan" <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



> -----Original Message-----
> From: Guenter Roeck <groeck7@...il.com> On Behalf Of Guenter Roeck
> Sent: Wednesday, November 6, 2024 11:56 PM
> To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>; Torreno, Alexis
> Czezar <AlexisCzezar.Torreno@...log.com>
> Cc: linux-doc@...r.kernel.org; linux-kernel@...r.kernel.org;
> devicetree@...r.kernel.org; linux-hwmon@...r.kernel.org; Sabau, Radu
> bogdan <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
> 
> [External]
> 
> 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.

Will revert, I surprisingly don't remember touching this.
Thanks!

> 
> > ...
> >
> >> +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.

Will remove/revert. 

Regards,
Alexis

> >
> >>   MODULE_DEVICE_TABLE(i2c, adp1050_id);
> >