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