Re: [PATCH 2/3] hwmon: (pmbus/core) add POWER_GOOD signal limits support

[Guenter Roeck]

On 9/9/24 08:39, Jerome Brunet wrote:
> Add support for POWER_GOOD_ON and POWER_GOOD_OFF standard PMBus commands.
>
> For PMBus devices that offer a POWER_GOOD signal, these commands are used
> for setting the output voltage at which a power good signal should be
> asserted and negated.
>
> Power Good signals are device and manufacturer specific. Many factors other
> than output voltage may be used to determine whether or not the POWER_GOOD
> signal is to be asserted. PMBus device users are instructed to consult the
> device manufacturer’s product literature for the specifics of the device
> they are using.
>
> Note that depending on the choice of the device manufacturer that a device
> may drive a POWER_GOOD signal high or low to indicate that the signal is
> asserted.
>
> Signed-off-by: Jerome Brunet <jbrunet@xxxxxxxxxxxx>
> ---
>   drivers/hwmon/pmbus/pmbus.h      | 3 +++
>   drivers/hwmon/pmbus/pmbus_core.c | 6 ++++++
>   2 files changed, 9 insertions(+)
>
> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> index 5d5dc774187b..e322d2dd9fb7 100644
> --- a/drivers/hwmon/pmbus/pmbus.h
> +++ b/drivers/hwmon/pmbus/pmbus.h
> @@ -78,6 +78,9 @@ enum pmbus_regs {
>   	PMBUS_IIN_OC_FAULT_LIMIT	= 0x5B,
>   	PMBUS_IIN_OC_WARN_LIMIT		= 0x5D,
>   
> +	PMBUS_POWER_GOOD_ON		= 0x5E,
> +	PMBUS_POWER_GOOD_OFF		= 0x5F,
> +
>   	PMBUS_POUT_OP_FAULT_LIMIT	= 0x68,
>   	PMBUS_POUT_OP_WARN_LIMIT	= 0x6A,
>   	PMBUS_PIN_OP_WARN_LIMIT		= 0x6B,
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 0ea6fe7eb17c..94ddf0166770 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -1768,6 +1768,12 @@ static const struct pmbus_limit_attr vout_limit_attrs[] = {
>   		.attr = "crit",
>   		.alarm = "crit_alarm",
>   		.sbit = PB_VOLTAGE_OV_FAULT,
> +	}, {
> +		.reg = PMBUS_POWER_GOOD_ON,
> +		.attr = "good_on",
> +	}, {
> +		.reg = PMBUS_POWER_GOOD_OFF,
> +		.attr = "good_off",
>   	}, {
>   		.reg = PMBUS_VIRT_READ_VOUT_AVG,
>   		.update = true,

Those attributes are not hardware monitoring attributes and therefore not
acceptable. In general I am not sure if they should be configurable in the
first place, but definitely not from the hardware monitoring subsystem.
Maybe the regulator subsystem callbacks set_over_voltage_protection and
set_under_voltage_protection would be appropriate (with severity
REGULATOR_SEVERITY_PROT), but that should be discussed with regulator
subsystem maintainers.

Thanks,
Guenter