[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <75f01776-a95f-d0a7-4803-0b3d17f19800@9elements.com>
Date: Wed, 30 Nov 2022 22:22:42 +0530
From: Naresh Solanki <naresh.solanki@...ements.com>
To: Guenter Roeck <linux@...ck-us.net>, linux-hwmon@...r.kernel.org,
Jean Delvare <jdelvare@...e.com>
Cc: Patrick Rudolph <patrick.rudolph@...ements.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/3] hwmon: (pmbus/core): Update regulator flag map
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@...ements.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
Powered by blists - more mailing lists