Message ID | 20241120035826.3920-3-cedricjustine.encarnacion@analog.com (mailing list archive) |
---|---|
State | Changes Requested |
Headers | show |
Series | Add support for ADP1051/ADP1055 and LTP8800-1A/-2/-4A | expand |
On 11/19/24 7:58 PM, Cedric Encarnacion wrote: > 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. > > Co-developed-by: Alexis Czezar Torreno <alexisczezar.torreno@analog.com> > Signed-off-by: Alexis Czezar Torreno <alexisczezar.torreno@analog.com> > Signed-off-by: Cedric Encarnacion <cedricjustine.encarnacion@analog.com> > --- > Documentation/hwmon/adp1050.rst | 63 +++++++++++++++++++++++++++-- > drivers/hwmon/pmbus/Kconfig | 9 +++++ > drivers/hwmon/pmbus/adp1050.c | 72 +++++++++++++++++++++++++++++++-- > 3 files changed, 137 insertions(+), 7 deletions(-) > > diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig > index f6d352841953..5d03a307824e 100644 > --- a/drivers/hwmon/pmbus/Kconfig > +++ b/drivers/hwmon/pmbus/Kconfig > @@ -67,6 +67,15 @@ config SENSORS_ADP1050 > This driver can also be built as a module. If so, the module will > be called adp1050. > > +config SENSORS_ADP1050_REGULATOR > + bool "Regulator support for ADP1050 and compatibles" > + depends on SENSORS_ADP1050 && REGULATOR > + help > + If you say yes here you get regulator support for Analog Devices > + LTP8800-1A, LTP8800-4A, and LTP8800-2. LTP8800 is a family of DC/DC > + µModule regulators that can provide microprocessor power from 54V > + power distribution architecture. > + > config SENSORS_BEL_PFE > tristate "Bel PFE Compatible Power Supplies" > help FYI: The 'micro' symbol displays as a blank space in 'menuconfig' or 'nconfig'. (It shows up correctly in gconfig and xconfig.) This problem is not unique to this driver entry. See https://lore.kernel.org/all/20231006202942.GA865945@bhelgaas/ from 2023. AFAIK no one is working on this issue. Feel free to change the help text or leave it...
On Tue, Nov 19, 2024 at 09:00:28PM -0800, Randy Dunlap wrote: > On 11/19/24 7:58 PM, Cedric Encarnacion wrote: ... > > +config SENSORS_ADP1050_REGULATOR > > + bool "Regulator support for ADP1050 and compatibles" > > + depends on SENSORS_ADP1050 && REGULATOR > > + help > > + If you say yes here you get regulator support for Analog Devices > > + LTP8800-1A, LTP8800-4A, and LTP8800-2. LTP8800 is a family of DC/DC > > + µModule regulators that can provide microprocessor power from 54V > > + power distribution architecture. > FYI: > > The 'micro' symbol displays as a blank space in 'menuconfig' or 'nconfig'. > (It shows up correctly in gconfig and xconfig.) > > This problem is not unique to this driver entry. > See https://lore.kernel.org/all/20231006202942.GA865945@bhelgaas/ from 2023. > > AFAIK no one is working on this issue. > Feel free to change the help text or leave it... If it's part of the commercial / official name, I would leave it. The bug is not in the help text anyway.
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@analog.com> > > - > 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? > 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? > +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. ... > +#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? ... > 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; 2) add new devices. > + { .name = "adp1051", .driver_data = (kernel_ulong_t)&adp1051_info}, > + { .name = "adp1055", .driver_data = (kernel_ulong_t)&adp1055_info}, > + { .name = "ltp8800", .driver_data = (kernel_ulong_t)<p8800_info}, > {} > };
On 11/19/24 19:58, Cedric Encarnacion wrote: > 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. > > Co-developed-by: Alexis Czezar Torreno <alexisczezar.torreno@analog.com> > Signed-off-by: Alexis Czezar Torreno <alexisczezar.torreno@analog.com> > Signed-off-by: Cedric Encarnacion <cedricjustine.encarnacion@analog.com> > --- > Documentation/hwmon/adp1050.rst | 63 +++++++++++++++++++++++++++-- > drivers/hwmon/pmbus/Kconfig | 9 +++++ > drivers/hwmon/pmbus/adp1050.c | 72 +++++++++++++++++++++++++++++++-- > 3 files changed, 137 insertions(+), 7 deletions(-) > > diff --git a/Documentation/hwmon/adp1050.rst b/Documentation/hwmon/adp1050.rst > index 8fa937064886..1692373ee5af 100644 > --- a/Documentation/hwmon/adp1050.rst > +++ b/Documentation/hwmon/adp1050.rst > @@ -13,18 +13,43 @@ 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 > + > + * Analog Devices LTP8800-1A/-2/-4A > + > + Prefix: 'ltp8800' > + > + Addresses scanned: - > + > + Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/LTP8800-1A.pdf > + https://www.analog.com/media/en/technical-documentation/data-sheets/LTP8800-2.pdf > + https://www.analog.com/media/en/technical-documentation/data-sheets/LTP8800-4A.pdf > + > Authors: > > - Radu Sabau <radu.sabau@analog.com> > > - Unrelated > 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™ Please refrain from using device name wildcards, There is no guarantee that all chips in the name range of ADP105[0-9] will have the same functionality. Either name the chips, or say something like "The supported chips are ...". Guenter
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@analog.com> >> >> - >> 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
On Wed, Nov 20, 2024 at 06:53:58AM -0800, Guenter Roeck wrote: > On 11/20/24 05:52, Andy Shevchenko wrote: > > On Wed, Nov 20, 2024 at 11:58:26AM +0800, Cedric Encarnacion wrote: ... > > > +#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. Yeah, it's a relatively new one... ... > > > +#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. I see, thanks for elaboration on all of this. ... > > Please, split this patch to at least two: > > 1) Introduce chip_info; > > That would really be "Use driver data to point to chip info". I agree on the title, what I meant is the rough description of what should be done in the change. > > 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. Indeed, that's why I mentioned "at least" in the reply.
diff --git a/Documentation/hwmon/adp1050.rst b/Documentation/hwmon/adp1050.rst index 8fa937064886..1692373ee5af 100644 --- a/Documentation/hwmon/adp1050.rst +++ b/Documentation/hwmon/adp1050.rst @@ -13,18 +13,43 @@ 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 + + * Analog Devices LTP8800-1A/-2/-4A + + Prefix: 'ltp8800' + + Addresses scanned: - + + Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/LTP8800-1A.pdf + https://www.analog.com/media/en/technical-documentation/data-sheets/LTP8800-2.pdf + https://www.analog.com/media/en/technical-documentation/data-sheets/LTP8800-4A.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 +74,46 @@ 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_alarm Output current alarm +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/Kconfig b/drivers/hwmon/pmbus/Kconfig index f6d352841953..5d03a307824e 100644 --- a/drivers/hwmon/pmbus/Kconfig +++ b/drivers/hwmon/pmbus/Kconfig @@ -67,6 +67,15 @@ config SENSORS_ADP1050 This driver can also be built as a module. If so, the module will be called adp1050. +config SENSORS_ADP1050_REGULATOR + bool "Regulator support for ADP1050 and compatibles" + depends on SENSORS_ADP1050 && REGULATOR + help + If you say yes here you get regulator support for Analog Devices + LTP8800-1A, LTP8800-4A, and LTP8800-2. LTP8800 is a family of DC/DC + µModule regulators that can provide microprocessor power from 54V + power distribution architecture. + config SENSORS_BEL_PFE tristate "Bel PFE Compatible Power Supplies" help diff --git a/drivers/hwmon/pmbus/adp1050.c b/drivers/hwmon/pmbus/adp1050.c index 20f22730fc01..46f2dda8adbb 100644 --- a/drivers/hwmon/pmbus/adp1050.c +++ b/drivers/hwmon/pmbus/adp1050.c @@ -11,6 +11,12 @@ #include "pmbus.h" +#if IS_ENABLED(CONFIG_SENSORS_ADP1050_REGULATOR) +static const struct regulator_desc adp1050_reg_desc[] = { + PMBUS_REGULATOR_ONE("vout"), +}; +#endif /* CONFIG_SENSORS_ADP1050_REGULATOR */ + static struct pmbus_driver_info adp1050_info = { .pages = 1, .format[PSC_VOLTAGE_IN] = linear, @@ -23,19 +29,79 @@ 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 struct pmbus_driver_info ltp8800_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_POUT + | PMBUS_HAVE_STATUS_VOUT + | PMBUS_HAVE_STATUS_INPUT + | PMBUS_HAVE_STATUS_TEMP, +#if IS_ENABLED(CONFIG_SENSORS_ADP1050_REGULATOR) + .num_regulators = 1, + .reg_desc = adp1050_reg_desc, +#endif +}; + 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}, + { .name = "ltp8800", .driver_data = (kernel_ulong_t)<p8800_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}, + { .compatible = "adi,ltp8800", .data = <p8800_info}, {} }; MODULE_DEVICE_TABLE(of, adp1050_of_match);