Messages in this thread | | | Date | Wed, 30 Nov 2022 22:22:42 +0530 | Subject | Re: [PATCH v2 1/3] hwmon: (pmbus/core): Update regulator flag map | From | Naresh Solanki <> |
| |
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@9elements.com> >>> >>> 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
| |