| Message ID | 20241106090311.17536-3-alexisczezar.torreno@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add support for ADP1051 and ADP1055 | expand |
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) > Signed-off-by: Alexis Cezar Torreno <alexisczezar.torreno@analog.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. ... > +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. > +}; ... > 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);
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@analog.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); >
On Wed, Nov 06, 2024 at 07:55:30AM -0800, Guenter Roeck wrote: > 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. I believe it's semi-official, meaning that people use it from time to time. I'm fine with the Link in the documentation. Actually with any solution that saves the respective link in the kernel source tree (either in form of commit message or documentation / comments in the code). ... > > > +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. Oh, okay, that's up to you then.
Hi Alexis,
kernel test robot noticed the following build errors:
[auto build test ERROR on aa8cbc0898902070f1ad093a6e036cf57f0d47bc]
url: https://github.com/intel-lab-lkp/linux/commits/Alexis-Cezar-Torreno/dt-bindings-hwmon-pmbus-adp1050-Support-adp1051-and-adp1055-add-bindings/20241106-170853
base: aa8cbc0898902070f1ad093a6e036cf57f0d47bc
patch link: https://lore.kernel.org/r/20241106090311.17536-3-alexisczezar.torreno%40analog.com
patch subject: [PATCH 2/2] hwmon: (pmbus/adp1050): Support adp1051 and adp1055
config: i386-randconfig-141-20241106 (https://download.01.org/0day-ci/archive/20241107/202411070008.3X7zgKXO-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241107/202411070008.3X7zgKXO-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411070008.3X7zgKXO-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from drivers/hwmon/pmbus/adp1050.c:8:
In file included from include/linux/i2c.h:19:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:21:
In file included from include/linux/mm.h:2213:
include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
518 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
>> drivers/hwmon/pmbus/adp1050.c:59:32: error: passing 'const struct pmbus_driver_info *' to parameter of type 'struct pmbus_driver_info *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
59 | return pmbus_do_probe(client, info);
| ^~~~
drivers/hwmon/pmbus/pmbus.h:541:73: note: passing argument to parameter 'info' here
541 | int pmbus_do_probe(struct i2c_client *client, struct pmbus_driver_info *info);
| ^
1 warning and 1 error generated.
vim +59 drivers/hwmon/pmbus/adp1050.c
50
51 static int adp1050_probe(struct i2c_client *client)
52 {
53 const struct pmbus_driver_info *info;
54
55 info = device_get_match_data(&client->dev);
56 if (!info)
57 return -ENODEV;
58
> 59 return pmbus_do_probe(client, info);
60 }
61
Hi Alexis, kernel test robot noticed the following build warnings: [auto build test WARNING on aa8cbc0898902070f1ad093a6e036cf57f0d47bc] url: https://github.com/intel-lab-lkp/linux/commits/Alexis-Cezar-Torreno/dt-bindings-hwmon-pmbus-adp1050-Support-adp1051-and-adp1055-add-bindings/20241106-170853 base: aa8cbc0898902070f1ad093a6e036cf57f0d47bc patch link: https://lore.kernel.org/r/20241106090311.17536-3-alexisczezar.torreno%40analog.com patch subject: [PATCH 2/2] hwmon: (pmbus/adp1050): Support adp1051 and adp1055 config: s390-allyesconfig (https://download.01.org/0day-ci/archive/20241107/202411070017.nDsv8lMO-lkp@intel.com/config) compiler: s390-linux-gcc (GCC) 14.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241107/202411070017.nDsv8lMO-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202411070017.nDsv8lMO-lkp@intel.com/ All warnings (new ones prefixed by >>): drivers/hwmon/pmbus/adp1050.c: In function 'adp1050_probe': >> drivers/hwmon/pmbus/adp1050.c:59:39: warning: passing argument 2 of 'pmbus_do_probe' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers] 59 | return pmbus_do_probe(client, info); | ^~~~ In file included from drivers/hwmon/pmbus/adp1050.c:12: drivers/hwmon/pmbus/pmbus.h:541:73: note: expected 'struct pmbus_driver_info *' but argument is of type 'const struct pmbus_driver_info *' 541 | int pmbus_do_probe(struct i2c_client *client, struct pmbus_driver_info *info); | ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~ vim +59 drivers/hwmon/pmbus/adp1050.c 50 51 static int adp1050_probe(struct i2c_client *client) 52 { 53 const struct pmbus_driver_info *info; 54 55 info = device_get_match_data(&client->dev); 56 if (!info) 57 return -ENODEV; 58 > 59 return pmbus_do_probe(client, info); 60 } 61
Hi Alexis, kernel test robot noticed the following build warnings: [auto build test WARNING on aa8cbc0898902070f1ad093a6e036cf57f0d47bc] url: https://github.com/intel-lab-lkp/linux/commits/Alexis-Cezar-Torreno/dt-bindings-hwmon-pmbus-adp1050-Support-adp1051-and-adp1055-add-bindings/20241106-170853 base: aa8cbc0898902070f1ad093a6e036cf57f0d47bc patch link: https://lore.kernel.org/r/20241106090311.17536-3-alexisczezar.torreno%40analog.com patch subject: [PATCH 2/2] hwmon: (pmbus/adp1050): Support adp1051 and adp1055 config: x86_64-randconfig-r123-20241106 (https://download.01.org/0day-ci/archive/20241107/202411070427.lkz6nVFX-lkp@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241107/202411070427.lkz6nVFX-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202411070427.lkz6nVFX-lkp@intel.com/ sparse warnings: (new ones prefixed by >>) >> drivers/hwmon/pmbus/adp1050.c:59:39: sparse: sparse: incorrect type in argument 2 (different modifiers) @@ expected struct pmbus_driver_info *info @@ got struct pmbus_driver_info const *[assigned] info @@ drivers/hwmon/pmbus/adp1050.c:59:39: sparse: expected struct pmbus_driver_info *info drivers/hwmon/pmbus/adp1050.c:59:39: sparse: got struct pmbus_driver_info const *[assigned] info vim +59 drivers/hwmon/pmbus/adp1050.c 50 51 static int adp1050_probe(struct i2c_client *client) 52 { 53 const struct pmbus_driver_info *info; 54 55 info = device_get_match_data(&client->dev); 56 if (!info) 57 return -ENODEV; 58 > 59 return pmbus_do_probe(client, info); 60 } 61
> -----Original Message----- > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Sent: Thursday, November 7, 2024 12:01 AM > To: Guenter Roeck <linux@roeck-us.net> > Cc: Torreno, Alexis Czezar <AlexisCzezar.Torreno@analog.com>; linux- > doc@vger.kernel.org; linux-kernel@vger.kernel.org; > devicetree@vger.kernel.org; linux-hwmon@vger.kernel.org; Sabau, Radu > bogdan <Radu.Sabau@analog.com>; Jean Delvare <jdelvare@suse.com>; Rob > Herring <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor > Dooley <conor+dt@kernel.org>; Jonathan Corbet <corbet@lwn.net>; Uwe > Kleine-König <u.kleine-koenig@pengutronix.de> > Subject: Re: [PATCH 2/2] hwmon: (pmbus/adp1050): Support adp1051 and > adp1055 > > [External] > > On Wed, Nov 06, 2024 at 07:55:30AM -0800, Guenter Roeck wrote: > > 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. > > I believe it's semi-official, meaning that people use it from time to time. > I'm fine with the Link in the documentation. Actually with any solution that > saves the respective link in the kernel source tree (either in form of commit > message or documentation / comments in the code). > Will add the blank line after description. Am I right to understand that we leave this as is? No need to add driver link in patch description since it is in driver documentation? > ... > > > > > +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. > > Oh, okay, that's up to you then. > > -- > With Best Regards, > Andy Shevchenko > I based my code style on the original, but I agree that the usual style should be followed. I will change it to follow the usual style. Should I leave the original untouched or should I format it too? Regards, Alexis
> -----Original Message----- > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck > Sent: Wednesday, November 6, 2024 11:56 PM > To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; Torreno, Alexis > Czezar <AlexisCzezar.Torreno@analog.com> > Cc: linux-doc@vger.kernel.org; linux-kernel@vger.kernel.org; > devicetree@vger.kernel.org; linux-hwmon@vger.kernel.org; Sabau, Radu > bogdan <Radu.Sabau@analog.com>; Jean Delvare <jdelvare@suse.com>; Rob > Herring <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor > Dooley <conor+dt@kernel.org>; Jonathan Corbet <corbet@lwn.net>; Uwe > Kleine-König <u.kleine-koenig@pengutronix.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@analog.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); > >
On Thu, Nov 07, 2024 at 01:17:04AM +0000, Torreno, Alexis Czezar wrote: > > -----Original Message----- > > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Sent: Thursday, November 7, 2024 12:01 AM > > On Wed, Nov 06, 2024 at 07:55:30AM -0800, Guenter Roeck wrote: > > > On 11/6/24 03:24, Andy Shevchenko wrote: > > > > On Wed, Nov 06, 2024 at 05:03:11PM +0800, Alexis Cezar Torreno wrote: ... > > > 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. > > > > I believe it's semi-official, meaning that people use it from time to time. > > I'm fine with the Link in the documentation. Actually with any solution that > > saves the respective link in the kernel source tree (either in form of commit > > message or documentation / comments in the code). > > Will add the blank line after description. > Am I right to understand that we leave this as is? No need to add driver link > in patch description since it is in driver documentation? Add it to the documentation. ... > > > > > +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. > > > > Oh, okay, that's up to you then. > I based my code style on the original, but I agree that the usual style > should be followed. > I will change it to follow the usual style. No, please be consistent with the existing style. If you want to change it, add an additional patch to do that for the _existing_ code first and base your patch on top of that. > Should I leave the original untouched or should I format it too?
> -----Original Message----- > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Sent: Thursday, November 7, 2024 4:05 PM > To: Torreno, Alexis Czezar <AlexisCzezar.Torreno@analog.com> > Cc: Guenter Roeck <linux@roeck-us.net>; linux-doc@vger.kernel.org; linux- > kernel@vger.kernel.org; devicetree@vger.kernel.org; linux- > hwmon@vger.kernel.org; Sabau, Radu bogdan <Radu.Sabau@analog.com>; > Jean Delvare <jdelvare@suse.com>; Rob Herring <robh@kernel.org>; Krzysztof > Kozlowski <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; > Jonathan Corbet <corbet@lwn.net>; Uwe Kleine-König <u.kleine- > koenig@pengutronix.de> > Subject: Re: [PATCH 2/2] hwmon: (pmbus/adp1050): Support adp1051 and > adp1055 > > [External] > > On Thu, Nov 07, 2024 at 01:17:04AM +0000, Torreno, Alexis Czezar wrote: > > > -----Original Message----- > > > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > Sent: Thursday, November 7, 2024 12:01 AM On Wed, Nov 06, 2024 at > > > 07:55:30AM -0800, Guenter Roeck wrote: > > > > On 11/6/24 03:24, Andy Shevchenko wrote: > > > > > On Wed, Nov 06, 2024 at 05:03:11PM +0800, Alexis Cezar Torreno > wrote: > > ... > > > > > 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. > > > > > > I believe it's semi-official, meaning that people use it from time to time. > > > I'm fine with the Link in the documentation. Actually with any > > > solution that saves the respective link in the kernel source tree > > > (either in form of commit message or documentation / comments in the > code). > > > > Will add the blank line after description. > > Am I right to understand that we leave this as is? No need to add > > driver link in patch description since it is in driver documentation? > > Add it to the documentation. > Already added the links for the datasheets in the documentation. > ... > > > > > > > +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. > > > > > > Oh, okay, that's up to you then. > > > I based my code style on the original, but I agree that the usual > > style should be followed. > > > I will change it to follow the usual style. > > No, please be consistent with the existing style. If you want to change it, add an > additional patch to do that for the _existing_ code first and base your patch on > top of that. > I see, I'll keep it consistent with the existing style. Thank you, will send an updated patch soon. Regards, Alexis > > Should I leave the original untouched or should I format it too? > > -- > With Best Regards, > Andy Shevchenko >
diff --git a/Documentation/hwmon/adp1050.rst b/Documentation/hwmon/adp1050.rst index 8fa937064886..5e2edcbe0c59 100644 --- a/Documentation/hwmon/adp1050.rst +++ b/Documentation/hwmon/adp1050.rst @@ -13,18 +13,33 @@ Supported chips: Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ADP1050.pdf + * Analog Devices ADP1051 + + Prefix: 'adp1051' + + Addresses scanned: I2C 0x70 - 0x77 + + Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ADP1051.pdf + + * Analog Devices ADP1055 + + Prefix: 'adp1055' + + Addresses scanned: I2C 0x4B - 0x77 + + Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ADP1055.pdf + Authors: - Radu Sabau <radu.sabau@analog.com> - Description ----------- -This driver supprts hardware monitoring for Analog Devices ADP1050 Digital -Controller for Isolated Power Supply with PMBus interface. +This driver supports hardware monitoring for Analog Devices ADP1050, ADP1051, and +ADP1055 Digital Controller for Isolated Power Supply with PMBus interface. -The ADP1050 is an advanced digital controller with a PMBus™ +The ADP105X is an advanced digital controller with a PMBus™ interface targeting high density, high efficiency dc-to-dc power conversion used to monitor system temperatures, voltages and currents. Through the PMBus interface, the device can monitor input/output voltages, @@ -49,16 +64,45 @@ Sysfs Attributes in1_label "vin" in1_input Measured input voltage in1_alarm Input voltage alarm +in1_crit Critical maximum input voltage +in1_crit_alarm Input voltage high alarm +in1_lcrit Critical minimum input voltage +in1_lcrit_alarm Input voltage critical low alarm in2_label "vout1" in2_input Measured output voltage in2_crit Critical maximum output voltage in2_crit_alarm Output voltage high alarm in2_lcrit Critical minimum output voltage in2_lcrit_alarm Output voltage critical low alarm +in2_max Critical maximum output voltage +in2_max_alarm Output voltage critical max alarm +in2_min Critical minimum output voltage +in2_min_alarm Output voltage critical min alarm curr1_label "iin" curr1_input Measured input current. curr1_alarm Input current alarm +curr1_crit Critical maximum input current +curr1_crit_alarm Input current high alarm +curr2_label "iout1" +curr2_input Measured output current +curr2_crit Critical maximum output current +curr2_crit_alarm Output current high alarm +curr2_lcrit Critical minimum output current +curr2_lcrit_alarm Output current critical low alarm +curr2_max Critical maximum output current +curr2_max_alarm Output current critical max alarm +power1_label "pout1" +power1_input Measured output power +power1_crit Critical maximum output power +power1_crit_alarm Output power high alarm temp1_input Measured temperature temp1_crit Critical high temperature temp1_crit_alarm Chip temperature critical high alarm +temp1_max Critical maximum temperature +temp1_max_alarm Temperature critical max alarm +temp2_input Measured temperature +temp2_crit Critical high temperature +temp2_crit_alarm Chip temperature critical high alarm +temp2_max Critical maximum temperature +temp2_max_alarm Temperature critical max alarm ================= ======================================== diff --git a/drivers/hwmon/pmbus/adp1050.c b/drivers/hwmon/pmbus/adp1050.c index 20f22730fc01..db2054181d14 100644 --- 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" @@ -23,19 +23,55 @@ static struct pmbus_driver_info adp1050_info = { | PMBUS_HAVE_STATUS_TEMP, }; +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, +}; + +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, +}; + static int adp1050_probe(struct i2c_client *client) { - return pmbus_do_probe(client, &adp1050_info); + const struct pmbus_driver_info *info; + + info = device_get_match_data(&client->dev); + if (!info) + return -ENODEV; + + return pmbus_do_probe(client, info); } 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}, {} }; + MODULE_DEVICE_TABLE(i2c, adp1050_id); static const struct of_device_id adp1050_of_match[] = { - { .compatible = "adi,adp1050"}, + { .compatible = "adi,adp1050", .data = &adp1050_info}, + { .compatible = "adi,adp1051", .data = &adp1051_info}, + { .compatible = "adi,adp1055", .data = &adp1055_info}, {} }; MODULE_DEVICE_TABLE(of, adp1050_of_match);
ADP1051: 6 PWM for I/O Voltage, I/O Current, Temperature ADP1055: 6 PWM for I/O Voltage, I/O Current, Power, Temperature Signed-off-by: Alexis Cezar Torreno <alexisczezar.torreno@analog.com> --- Documentation/hwmon/adp1050.rst | 52 ++++++++++++++++++++++++++++++--- drivers/hwmon/pmbus/adp1050.c | 44 +++++++++++++++++++++++++--- 2 files changed, 88 insertions(+), 8 deletions(-)