Re: [PATCH v2 1/3] hwmon: (pmbus/core): Update regulator flag map

[Naresh Solanki <naresh.solanki@9elements.com>]

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