> -----Original Message-----
> From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck
> Sent: Wednesday, November 6, 2024 11:56 PM
> To: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>; Torreno, Alexis
> Czezar <AlexisCzezar.Torreno@xxxxxxxxxx>
> Cc: linux-doc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-hwmon@xxxxxxxxxxxxxxx; Sabau, Radu
> bogdan <Radu.Sabau@xxxxxxxxxx>; Jean Delvare <jdelvare@xxxxxxxx>; Rob
> Herring <robh@xxxxxxxxxx>; Krzysztof Kozlowski <krzk+dt@xxxxxxxxxx>; Conor
> Dooley <conor+dt@xxxxxxxxxx>; Jonathan Corbet <corbet@xxxxxxx>; Uwe
> Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> 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@xxxxxxxxxx>
> >
> > ...
> >
> >> --- 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);
> >
