Re: [PATCH v2 1/3] hwmon: (pmbus/core): Update regulator flag map
From: Naresh Solanki
Date: Wed Nov 30 2022 - 11:52:59 EST
Hi Guenter,
On 29-11-2022 08:54 pm, Guenter Roeck wrote:
On 11/28/22 23:55, Naresh Solanki wrote:
Hi Guenter,
On 29-11-2022 04:11 am, Guenter Roeck wrote:
On 11/28/22 09:47, Naresh Solanki wrote:
Add regulator flag map for PMBUS status byte & status input.
Signed-off-by: Naresh Solanki <Naresh.Solanki@xxxxxxxxxxxxx>
You are adding a lot of input errors here. The regulator documentation
only covers output errors. I am not sure if this set of changes is
really appropriate. You'll have to make a much better case for those
changes;
from what I can see they are all controversial and were originally
left out
on purpose.
I felt it may be worth to monitor status input, but you feel otherwise
then shall I remove this in next revision ?
It is a set of changes which needs input from regulator subsystem
maintainers.
Maybe it even needs changes on the regulator side, for example to report
input and/or power failures properly.
It isn't something I would have expected as part of a patch or patch series
series which is supposed to add interrupt support to pmbus drivers.
Since it is the first patch in your series, in may hold up the series
for some period of time until the questions around it are resolved.
Your call, really, how to handle it. Just don't be surprised if it takes
a while to resolve the issues.
I'll check with regulator subsystem maintainer on input error & based on
feedback will post separate patch.
---
drivers/hwmon/pmbus/pmbus_core.c | 30 ++++++++++++++++++++++--------
1 file changed, 22 insertions(+), 8 deletions(-)
diff --git a/drivers/hwmon/pmbus/pmbus_core.c
b/drivers/hwmon/pmbus/pmbus_core.c
index 95e95783972a..f5caceaaef2a 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -2752,6 +2752,15 @@ struct pmbus_regulator_status_category {
static const struct pmbus_regulator_status_category
pmbus_regulator_flag_map[] = {
{
+ .func = -1,
This would need a comment. I don't really see the benefit over the
original
code.
I pulled that in so as to handle it in same way as other status register.
That would have to be a separate patch. It took me a while to understand
how .func = -1 is handled, so without comment it just adds confusion.
Yes. Will make separate patch & add comment here.
+ .reg = PMBUS_STATUS_BYTE,
+ .bits = (const struct pmbus_regulator_status_assoc[]) {
+ { PB_STATUS_IOUT_OC, REGULATOR_ERROR_OVER_CURRENT },
+ { PB_STATUS_VOUT_OV, REGULATOR_ERROR_REGULATION_OUT },
+ { PB_STATUS_VIN_UV, REGULATOR_ERROR_UNDER_VOLTAGE },
+ { },
+ },
+ }, {
.func = PMBUS_HAVE_STATUS_VOUT,
.reg = PMBUS_STATUS_VOUT,
.bits = (const struct pmbus_regulator_status_assoc[]) {
@@ -2768,6 +2777,7 @@ static const struct
pmbus_regulator_status_category pmbus_regulator_flag_map[] =
{ PB_IOUT_OC_WARNING,
REGULATOR_ERROR_OVER_CURRENT_WARN },
{ PB_IOUT_OC_FAULT, REGULATOR_ERROR_OVER_CURRENT },
{ PB_IOUT_OC_LV_FAULT, REGULATOR_ERROR_OVER_CURRENT },
+ { PB_POUT_OP_FAULT, REGULATOR_ERROR_OVER_CURRENT },
OP_FAULT (power fault) and over current are really not the same thing.
I agree. But thats best I could think of. Not sure if there is better
REGULATOR_ERROR_* code for this scenario. Suggestions?
Options are REGULATOR_ERROR_OVER_CURRENT or REGULATOR_ERROR_FAIL or
a new failure code or doing nothing. Personally I think
REGULATOR_ERROR_FAIL
would be better if adding a new failure code is not an option.
Will update to REGULATOR_ERROR_FAIL.
Anyway, clarify on the regulator subsystem mailing list how to handle input
errors, and how to handle power failures. If they say it is acceptable to
report input errors as output errors, and to report power failures as
current failures, resubmit. Say in comments that this is what you are
doing,
and in the commit description that this is how input errors and power
failures are handled in the regulator subsystem. Copy regulator subsystem
maintainers on your patch.
Sure. Will hold back input errors for now & after checking with
regulator maintainer, will make separate patch accordingly.
Thanks,
Guenter
{ },
},
}, {
@@ -2778,6 +2788,18 @@ static const struct
pmbus_regulator_status_category pmbus_regulator_flag_map[] =
{ PB_TEMP_OT_FAULT, REGULATOR_ERROR_OVER_TEMP },
{ },
},
+ }, {
+ .func = PMBUS_HAVE_STATUS_INPUT,
+ .reg = PMBUS_STATUS_INPUT,
+ .bits = (const struct pmbus_regulator_status_assoc[]) {
+ { PB_IIN_OC_FAULT, REGULATOR_ERROR_OVER_CURRENT },
+ { PB_IIN_OC_WARNING, REGULATOR_ERROR_OVER_CURRENT_WARN },
+ { PB_VOLTAGE_UV_FAULT, REGULATOR_ERROR_UNDER_VOLTAGE },
+ { PB_VOLTAGE_UV_WARNING,
REGULATOR_ERROR_UNDER_VOLTAGE_WARN },
+ { PB_VOLTAGE_OV_WARNING,
REGULATOR_ERROR_OVER_VOLTAGE_WARN },
+ { PB_VOLTAGE_OV_FAULT,
REGULATOR_ERROR_OVER_VOLTAGE_WARN },
fault -> warning ? Shouldn't this be REGULATOR_ERROR_FAIL (Regulator
output has failed) ?
Yes. REGULATOR_ERROR_FAIL is best fit here. Will update in next revision.
+ { },
+ },
},
};
@@ -2834,14 +2856,6 @@ static int
pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned
if (status & PB_STATUS_POWER_GOOD_N)
*flags |= REGULATOR_ERROR_REGULATION_OUT;
}
- /*
- * Unlike most other status bits, PB_STATUS_{IOUT_OC,VOUT_OV} are
- * defined strictly as fault indicators (not warnings).
- */
- if (status & PB_STATUS_IOUT_OC)
- *flags |= REGULATOR_ERROR_OVER_CURRENT;
- if (status & PB_STATUS_VOUT_OV)
- *flags |= REGULATOR_ERROR_REGULATION_OUT;
/*
* If we haven't discovered any thermal faults or warnings via
base-commit: 9494c53e1389b120ba461899207ac8a3aab2632c
Regards,
Naresh