On 11/20/24 05:52, Andy Shevchenko wrote:
On Wed, Nov 20, 2024 at 11:58:26AM +0800, Cedric Encarnacion wrote:
I would start the commit message with the plain English sentence that describes
the list given below. E.g., "Introduce support for the following components:".
ADP1051: 6 PWM for I/O Voltage, I/O Current, Temperature
ADP1055: 6 PWM for I/O Voltage, I/O Current, Power, Temperature
LTP8800-1A/-2/-4A: 150A/135A/200A DC/DC µModule Regulator
The LTP8800 is a family of step-down μModule regulators that provides
microprocessor core voltage from 54V power distribution architecture.
LTP8800 features telemetry monitoring of input/output voltage, input
current, output power, and temperature over PMBus.
...
- Radu Sabau <radu.sabau@xxxxxxxxxx>
-
Description
-----------
Stray change.
...
-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™
Can we use small x to make it more visible that it's _not_ the part of the
name, but a glob-like placeholder?
As mentioned in my other reply, I'd rather not have a placeholder in the first
place.
interface targeting high density, high efficiency dc-to-dc power
conversion used to monitor system temperatures, voltages and currents.
...
+#if IS_ENABLED(CONFIG_SENSORS_ADP1050_REGULATOR)
Why? Is the data type undefined without this?
Look into other drivers. That is how it is implemented there,
and not really the point. One has to know about an alternative to use it.
+static const struct regulator_desc adp1050_reg_desc[] = {
+ PMBUS_REGULATOR_ONE("vout"),
+};
+#endif /* CONFIG_SENSORS_ADP1050_REGULATOR */
Note, this can be dropped anyway in order to use PTR_IF() below, if required.
FWIW, PTR_IF() isn't widely used, and I for my part was not aware that
it exists.
...
+#if IS_ENABLED(CONFIG_SENSORS_ADP1050_REGULATOR)
+ .num_regulators = 1,
+ .reg_desc = adp1050_reg_desc,
+#endif
Ditto, are the fields not defined without the symbol?
They are, but they must be 0/NULL. PTR_IF() would be an alternative.
It is a bit odd to use it for a non-pointer, but it is type-agnostic,
so using it should be ok to avoid the #ifdefs. We should maybe adopt
that mechanism for other PMBus drivers.
...
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);
Why not i2c_get_match_data()?
+ 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},
Please, split this patch to at least two:
1) Introduce chip_info;
That would really be "Use driver data to point to chip info".
2) add new devices.
I don't really care much about separating those two (after all, they are
related), but adding regulator support to the driver is a major change
and should be a separate patch. On top of that, it isn't even mentioned
in the patch description.
Thanks,
Guenter
