Re: [PATCH v3 1/4] hwmon: (pmbus/core): Add status byte to regulator flag map

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 30-11-2022 11:49 pm, Guenter Roeck wrote:
On Wed, Nov 30, 2022 at 11:42:32PM +0530, Naresh Solanki wrote:
Hi,

On 30-11-2022 11:36 pm, Guenter Roeck wrote:
On Wed, Nov 30, 2022 at 05:58:28PM +0100, Naresh Solanki wrote:
Add PMBus status byte to regulator flag map.

Signed-off-by: Naresh Solanki <Naresh.Solanki@xxxxxxxxxxxxx>

---
Changes:
- Remove status input

Not really.

This was about PMBUS_STATUS_INPUT & not STATUS_BYTE
- Add comment for PMBUS status byte.
---
   drivers/hwmon/pmbus/pmbus_core.c | 20 +++++++++++---------
   1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 95e95783972a..a7b4ae0f1f3b 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -2751,7 +2751,16 @@ struct pmbus_regulator_status_category {
   };
   static const struct pmbus_regulator_status_category pmbus_regulator_flag_map[] = {
-	{
+	{	/* STATUS byte is always present. */
+		.func = -1,
+		.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 },

Still there.

STATUS_INPUT remove & STATUS_BYTE retained.

You added a check for PB_STATUS_VIN_UV which reports an input failure
which was not reported before. You do so silently without mentioning the
change in the description. In the change log you claim "Remove status input".
Yet, the above adds reporting on an input voltage error. Maybe the
change log was supposed to refer to the input status register, but that
doesn't mean you can silently add a check for PB_STATUS_VIN_UV.
Right. I removed STATUS_INPUT but overlooked VIN in STATUS_BYTE.
Will fix this in next revision.

Guenter

+			{ },
+		},
+	}, {
   		.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_FAIL },

Please document your changes in the description. There are two functional
changes hidden under "Add PMBus status byte to regulator flag map".
That is inappropriate. Those, as I have pointed out, should be separate
patches.
My bad. I kept this change in this patch. Will remove this from this patch.

Guenter

   			{ },
   		},
   	}, {
@@ -2834,14 +2844,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
--
2.37.3


Regards,
Naresh
Regards,
Naresh



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux