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

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

[Naresh Solanki]

Add regulator flag map for PMBUS status byte & status input.

Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
---
 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,
+		.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 },
 			{ },
 		},
 	}, {
@@ -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 },
+			{ },
+		},
 	},
 };
 
@@ -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
-- 
2.37.3

[PATCH v2 2/3] hwmon: (pmbus/core): Add events to regulator flag map

[Naresh Solanki]

From: Patrick Rudolph <patrick.rudolph@9elements.com>

Add regulator events corresponding to regulator error in regulator flag
map.

Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
---
 drivers/hwmon/pmbus/pmbus_core.c | 62 +++++++++++++++++++++-----------
 1 file changed, 41 insertions(+), 21 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index f5caceaaef2a..060e9d0a55bd 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -2738,9 +2738,9 @@ static int pmbus_regulator_disable(struct regulator_dev *rdev)
 	return _pmbus_regulator_on_off(rdev, 0);
 }
 
-/* A PMBus status flag and the corresponding REGULATOR_ERROR_* flag */
+/* A PMBus status flag and the corresponding REGULATOR_ERROR_* and REGULATOR_EVENTS_* flag */
 struct pmbus_regulator_status_assoc {
-	int pflag, rflag;
+	int pflag, rflag, eflag;
 };
 
 /* PMBus->regulator bit mappings for a PMBus status register */
@@ -2755,51 +2755,71 @@ static const struct pmbus_regulator_status_category pmbus_regulator_flag_map[] =
 		.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 },
+			{ PB_STATUS_IOUT_OC,   REGULATOR_ERROR_OVER_CURRENT,
+			REGULATOR_EVENT_OVER_CURRENT },
+			{ PB_STATUS_VOUT_OV,   REGULATOR_ERROR_REGULATION_OUT,
+			REGULATOR_EVENT_OVER_VOLTAGE_WARN },
+			{ PB_STATUS_VIN_UV,    REGULATOR_ERROR_UNDER_VOLTAGE,
+			REGULATOR_EVENT_UNDER_VOLTAGE },
 			{ },
 		},
 	}, {
 		.func = PMBUS_HAVE_STATUS_VOUT,
 		.reg = PMBUS_STATUS_VOUT,
 		.bits = (const struct pmbus_regulator_status_assoc[]) {
-			{ PB_VOLTAGE_UV_WARNING, REGULATOR_ERROR_UNDER_VOLTAGE_WARN },
-			{ PB_VOLTAGE_UV_FAULT,   REGULATOR_ERROR_UNDER_VOLTAGE },
-			{ PB_VOLTAGE_OV_WARNING, REGULATOR_ERROR_OVER_VOLTAGE_WARN },
-			{ PB_VOLTAGE_OV_FAULT,   REGULATOR_ERROR_REGULATION_OUT },
+			{ PB_VOLTAGE_UV_WARNING, REGULATOR_ERROR_UNDER_VOLTAGE_WARN,
+			REGULATOR_EVENT_UNDER_VOLTAGE_WARN },
+			{ PB_VOLTAGE_UV_FAULT,   REGULATOR_ERROR_UNDER_VOLTAGE,
+			REGULATOR_EVENT_UNDER_VOLTAGE },
+			{ PB_VOLTAGE_OV_WARNING, REGULATOR_ERROR_OVER_VOLTAGE_WARN,
+			REGULATOR_EVENT_OVER_VOLTAGE_WARN },
+			{ PB_VOLTAGE_OV_FAULT,   REGULATOR_ERROR_REGULATION_OUT,
+			REGULATOR_EVENT_OVER_VOLTAGE_WARN },
 			{ },
 		},
 	}, {
 		.func = PMBUS_HAVE_STATUS_IOUT,
 		.reg = PMBUS_STATUS_IOUT,
 		.bits = (const struct pmbus_regulator_status_assoc[]) {
-			{ 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 },
+			{ PB_IOUT_OC_WARNING,   REGULATOR_ERROR_OVER_CURRENT_WARN,
+			REGULATOR_EVENT_OVER_CURRENT_WARN },
+			{ PB_IOUT_OC_FAULT,     REGULATOR_ERROR_OVER_CURRENT,
+			REGULATOR_EVENT_OVER_CURRENT },
+			{ PB_IOUT_OC_LV_FAULT,  REGULATOR_ERROR_OVER_CURRENT,
+			REGULATOR_EVENT_OVER_CURRENT },
+			{ PB_POUT_OP_FAULT,     REGULATOR_ERROR_OVER_CURRENT,
+			REGULATOR_EVENT_OVER_CURRENT },
 			{ },
 		},
 	}, {
 		.func = PMBUS_HAVE_STATUS_TEMP,
 		.reg = PMBUS_STATUS_TEMPERATURE,
 		.bits = (const struct pmbus_regulator_status_assoc[]) {
-			{ PB_TEMP_OT_WARNING,    REGULATOR_ERROR_OVER_TEMP_WARN },
-			{ PB_TEMP_OT_FAULT,      REGULATOR_ERROR_OVER_TEMP },
+			{ PB_TEMP_OT_WARNING,    REGULATOR_ERROR_OVER_TEMP_WARN,
+			REGULATOR_EVENT_OVER_TEMP_WARN },
+			{ PB_TEMP_OT_FAULT,      REGULATOR_ERROR_OVER_TEMP,
+			REGULATOR_EVENT_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 },
+			{ PB_IIN_OC_FAULT,       REGULATOR_ERROR_OVER_CURRENT,
+			REGULATOR_EVENT_OVER_CURRENT },
+			{ PB_IIN_OC_WARNING,     REGULATOR_ERROR_OVER_CURRENT_WARN,
+			REGULATOR_EVENT_OVER_CURRENT_WARN },
+			{ PB_VOLTAGE_UV_FAULT,       REGULATOR_ERROR_UNDER_VOLTAGE,
+			REGULATOR_EVENT_UNDER_VOLTAGE },
+			{ PB_VOLTAGE_UV_WARNING,     REGULATOR_ERROR_UNDER_VOLTAGE_WARN,
+			REGULATOR_EVENT_UNDER_VOLTAGE_WARN },
+			{ PB_VOLTAGE_OV_WARNING,     REGULATOR_ERROR_OVER_VOLTAGE_WARN,
+			REGULATOR_ERROR_OVER_VOLTAGE_WARN },
+			{ PB_VOLTAGE_OV_FAULT,       REGULATOR_ERROR_OVER_VOLTAGE_WARN,
+			REGULATOR_ERROR_OVER_VOLTAGE_WARN },
 			{ },
 		},
+
 	},
 };
 
-- 
2.37.3

[PATCH v2 3/3] hwmon: (pmbus/core): Implement irq support

[Naresh Solanki]

From: Patrick Rudolph <patrick.rudolph@9elements.com>

Implement PMBUS irq handler to notify regulator events.

Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
---
 drivers/hwmon/pmbus/pmbus.h      |   2 +-
 drivers/hwmon/pmbus/pmbus_core.c | 151 ++++++++++++++++++++++++++++---
 2 files changed, 137 insertions(+), 16 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index 10fb17879f8e..6b2e6cf93b19 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -26,7 +26,7 @@ enum pmbus_regs {
 
 	PMBUS_CAPABILITY		= 0x19,
 	PMBUS_QUERY			= 0x1A,
-
+	PMBUS_SMBALERT_MASK		= 0x1B,
 	PMBUS_VOUT_MODE			= 0x20,
 	PMBUS_VOUT_COMMAND		= 0x21,
 	PMBUS_VOUT_TRIM			= 0x22,
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 060e9d0a55bd..e1f84fa127ba 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -81,6 +81,7 @@ struct pmbus_label {
 struct pmbus_data {
 	struct device *dev;
 	struct device *hwmon_dev;
+	struct regulator_dev **rdevs;
 
 	u32 flags;		/* from platform data */
 
@@ -2823,7 +2824,8 @@ static const struct pmbus_regulator_status_category pmbus_regulator_flag_map[] =
 	},
 };
 
-static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned int *flags)
+static int pmbus_regulator_get_flags(struct regulator_dev *rdev, unsigned int *error,
+				    unsigned int *event)
 {
 	int i, status;
 	const struct pmbus_regulator_status_category *cat;
@@ -2834,7 +2836,8 @@ static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned
 	u8 page = rdev_get_id(rdev);
 	int func = data->info->func[page];
 
-	*flags = 0;
+	*error = 0;
+	*event = 0;
 
 	mutex_lock(&data->update_lock);
 
@@ -2850,8 +2853,10 @@ static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned
 		}
 
 		for (bit = cat->bits; bit->pflag; bit++) {
-			if (status & bit->pflag)
-				*flags |= bit->rflag;
+			if (status & bit->pflag) {
+				*error |= bit->rflag;
+				*event |= bit->eflag;
+			}
 		}
 	}
 
@@ -2870,11 +2875,15 @@ static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned
 		return status;
 
 	if (pmbus_regulator_is_enabled(rdev)) {
-		if (status & PB_STATUS_OFF)
-			*flags |= REGULATOR_ERROR_FAIL;
+		if (status & PB_STATUS_OFF) {
+			*error |= REGULATOR_ERROR_FAIL;
+			*event |= REGULATOR_EVENT_FAIL;
+		}
 
-		if (status & PB_STATUS_POWER_GOOD_N)
-			*flags |= REGULATOR_ERROR_REGULATION_OUT;
+		if (status & PB_STATUS_POWER_GOOD_N) {
+			*error |= REGULATOR_ERROR_REGULATION_OUT;
+			*event |= REGULATOR_EVENT_REGULATION_OUT;
+		}
 	}
 
 	/*
@@ -2882,13 +2891,22 @@ static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned
 	 * PMBUS_STATUS_TEMPERATURE, map PB_STATUS_TEMPERATURE to a warning as
 	 * a (conservative) best-effort interpretation.
 	 */
-	if (!(*flags & (REGULATOR_ERROR_OVER_TEMP | REGULATOR_ERROR_OVER_TEMP_WARN)) &&
-	    (status & PB_STATUS_TEMPERATURE))
-		*flags |= REGULATOR_ERROR_OVER_TEMP_WARN;
+	if (!(*error & (REGULATOR_ERROR_OVER_TEMP | REGULATOR_ERROR_OVER_TEMP_WARN)) &&
+	    (status & PB_STATUS_TEMPERATURE)) {
+		*error |= REGULATOR_ERROR_OVER_TEMP_WARN;
+		*event |= REGULATOR_EVENT_OVER_TEMP_WARN;
+	}
 
 	return 0;
 }
 
+static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned int *flags)
+{
+	unsigned int event;
+
+	return pmbus_regulator_get_flags(rdev, flags, &event);
+}
+
 static int pmbus_regulator_get_status(struct regulator_dev *rdev)
 {
 	struct device *dev = rdev_get_dev(rdev);
@@ -3079,14 +3097,61 @@ const struct regulator_ops pmbus_regulator_ops = {
 };
 EXPORT_SYMBOL_NS_GPL(pmbus_regulator_ops, PMBUS);
 
+static int pmbus_write_smbalert_mask(struct i2c_client *client, u8 page, u8 reg, u8 val)
+{
+	return pmbus_write_word_data(client, page, PMBUS_SMBALERT_MASK, reg | (val << 8));
+}
+
+static irqreturn_t pmbus_fault_handler(int irq, void *pdata)
+{
+	struct pmbus_data *data = pdata;
+	struct i2c_client *client = to_i2c_client(data->dev);
+	int i, ret = IRQ_NONE, status, event;
+	u8 page;
+
+	for (i = 0; i < data->info->num_regulators; i++) {
+
+		if (!data->rdevs[i])
+			continue;
+
+		ret = pmbus_regulator_get_flags(data->rdevs[i], &status, &event);
+		if (ret)
+			return ret;
+
+		if (event) {
+			regulator_notifier_call_chain(data->rdevs[i], event, NULL);
+			ret = IRQ_HANDLED;
+		}
+
+		page = rdev_get_id(data->rdevs[i]);
+		mutex_lock(&data->update_lock);
+		status = pmbus_read_status_word(client, page);
+		if (status < 0) {
+			mutex_unlock(&data->update_lock);
+			return status;
+		}
+
+		if (status & ~(PB_STATUS_OFF | PB_STATUS_BUSY | PB_STATUS_POWER_GOOD_N))
+			pmbus_clear_fault_page(client, page);
+
+		mutex_unlock(&data->update_lock);
+	}
+
+	return ret;
+}
+
 static int pmbus_regulator_register(struct pmbus_data *data)
 {
 	struct device *dev = data->dev;
 	const struct pmbus_driver_info *info = data->info;
 	const struct pmbus_platform_data *pdata = dev_get_platdata(dev);
-	struct regulator_dev *rdev;
 	int i;
 
+	data->rdevs = devm_kzalloc(dev, sizeof(struct regulator_dev *) * info->num_regulators,
+				  GFP_KERNEL);
+	if (!data->rdevs)
+		return -ENOMEM;
+
 	for (i = 0; i < info->num_regulators; i++) {
 		struct regulator_config config = { };
 
@@ -3096,21 +3161,71 @@ static int pmbus_regulator_register(struct pmbus_data *data)
 		if (pdata && pdata->reg_init_data)
 			config.init_data = &pdata->reg_init_data[i];
 
-		rdev = devm_regulator_register(dev, &info->reg_desc[i],
+		data->rdevs[i] = devm_regulator_register(dev, &info->reg_desc[i],
 					       &config);
-		if (IS_ERR(rdev))
-			return dev_err_probe(dev, PTR_ERR(rdev),
+		if (IS_ERR(data->rdevs[i]))
+			return dev_err_probe(dev, PTR_ERR(data->rdevs[i]),
 					     "Failed to register %s regulator\n",
 					     info->reg_desc[i].name);
 	}
 
 	return 0;
 }
+
+static int pmbus_irq_setup(struct i2c_client *client, struct pmbus_data *data)
+{
+	struct device *dev = &client->dev;
+	const struct pmbus_regulator_status_category *cat;
+	const struct pmbus_regulator_status_assoc *bit;
+	int i, j, err, ret;
+	u8 mask;
+	int func;
+
+	for (i = 0; i < data->info->pages; i++) {
+		func = data->info->func[i];
+
+		for (j = 0; j < ARRAY_SIZE(pmbus_regulator_flag_map); j++) {
+			cat = &pmbus_regulator_flag_map[i];
+			if (!(func & cat->func))
+				continue;
+			mask = 0;
+			for (bit = cat->bits; bit->pflag; bit++)
+				mask |= bit->pflag;
+
+			err = pmbus_write_smbalert_mask(client, i, cat->reg, ~mask);
+			if (err)
+				dev_err(dev, "Failed to set smbalert for reg 0x%02x\n",	cat->reg);
+		}
+
+		pmbus_write_smbalert_mask(client, i, PMBUS_STATUS_CML, 0xff);
+		pmbus_write_smbalert_mask(client, i, PMBUS_STATUS_OTHER, 0xff);
+		pmbus_write_smbalert_mask(client, i, PMBUS_STATUS_MFR_SPECIFIC, 0xff);
+		if (data->info->func[i] & PMBUS_HAVE_FAN12)
+			pmbus_write_smbalert_mask(client, i, PMBUS_STATUS_FAN_12, 0xff);
+		if (data->info->func[i] & PMBUS_HAVE_FAN34)
+			pmbus_write_smbalert_mask(client, i, PMBUS_STATUS_FAN_34, 0xff);
+
+	}
+
+	/* Register notifiers - can fail if IRQ is not given */
+	ret = devm_request_threaded_irq(dev, client->irq, NULL, pmbus_fault_handler,
+			      0, "pmbus-irq", data);
+	if (ret) {
+		dev_warn(dev, "IRQ disabled %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
 #else
 static int pmbus_regulator_register(struct pmbus_data *data)
 {
 	return 0;
 }
+static int pmbus_irq_setup(struct i2c_client *client, struct pmbus_data *data)
+{
+	return 0;
+}
 #endif
 
 static struct dentry *pmbus_debugfs_dir;	/* pmbus debugfs directory */
@@ -3475,6 +3590,12 @@ int pmbus_do_probe(struct i2c_client *client, struct pmbus_driver_info *info)
 	if (ret)
 		return ret;
 
+	if (client->irq > 0) {
+		ret = pmbus_irq_setup(client, data);
+		if (ret)
+			return ret;
+	}
+
 	ret = pmbus_init_debugfs(client, data);
 	if (ret)
 		dev_warn(dev, "Failed to register debugfs\n");
-- 
2.37.3

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

[Guenter Roeck]

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.

> ---
>   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.

> +		.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.

>   			{ },
>   		},
>   	}, {
> @@ -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) ?

> +			{ },
> +		},
>   	},
>   };
>   
> @@ -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

Re: [PATCH v2 3/3] hwmon: (pmbus/core): Implement irq support

[Guenter Roeck]

On 11/28/22 09:47, Naresh Solanki wrote:
> From: Patrick Rudolph <patrick.rudolph@9elements.com>
> 
> Implement PMBUS irq handler to notify regulator events.
> 
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>

As I am sure I have mentioned before, this needs to primarily handle
sysfs notifications to hwmon status attributes and to generate kobject
events. Regulator events are secondary / optional.

Thanks,
Guenter

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

[Naresh Solanki]

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

Re: [PATCH v2 3/3] hwmon: (pmbus/core): Implement irq support

[Naresh Solanki]

Hi Guenter,

On 29-11-2022 04:39 am, Guenter Roeck wrote:
> On 11/28/22 09:47, Naresh Solanki wrote:
>> From: Patrick Rudolph <patrick.rudolph@9elements.com>
>>
>> Implement PMBUS irq handler to notify regulator events.
>>
>> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
>> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> 
> As I am sure I have mentioned before, this needs to primarily handle
> sysfs notifications to hwmon status attributes and to generate kobject
> events. Regulator events are secondary / optional.

Based on previous feedback, PMBus interrupt handler is made generic
Based on the use case I have in my machine, my application need to 
monitor regulator event as soon as they occur and hence the patch.

> 
> Thanks,
> Guenter
> 
Regards,
Naresh

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

[Guenter Roeck]

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.

>>
>>> ---
>>>   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.

>>
>>> +        .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.

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.

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
>>

Re: [PATCH v2 3/3] hwmon: (pmbus/core): Implement irq support

[Guenter Roeck]

On 11/29/22 00:16, Naresh Solanki wrote:
> Hi Guenter,
> 
> On 29-11-2022 04:39 am, Guenter Roeck wrote:
>> On 11/28/22 09:47, Naresh Solanki wrote:
>>> From: Patrick Rudolph <patrick.rudolph@9elements.com>
>>>
>>> Implement PMBUS irq handler to notify regulator events.
>>>
>>> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
>>> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
>>
>> As I am sure I have mentioned before, this needs to primarily handle
>> sysfs notifications to hwmon status attributes and to generate kobject
>> events. Regulator events are secondary / optional.
> 
> Based on previous feedback, PMBus interrupt handler is made generic
> Based on the use case I have in my machine, my application need to monitor regulator event as soon as they occur and hence the patch.
> 

I understand, but this isn't just about your specific use case. Your use case is
what triggers the change, and ensures that the code change is tested, but the
impact by far reaches beyond your specific use case and needs to address other
(much more common) use cases as well. Interrupt support is needed in the pmbus
code, but it needs to address the common use case first, and that is reporting
the status via sysfs notifications and kobject events.

Thanks,
Guenter

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

[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

Re: [PATCH v2 3/3] hwmon: (pmbus/core): Implement irq support

[Naresh Solanki]

Hi Guenter,

On 29-11-2022 08:59 pm, Guenter Roeck wrote:
> On 11/29/22 00:16, Naresh Solanki wrote:
>> Hi Guenter,
>>
>> On 29-11-2022 04:39 am, Guenter Roeck wrote:
>>> On 11/28/22 09:47, Naresh Solanki wrote:
>>>> From: Patrick Rudolph <patrick.rudolph@9elements.com>
>>>>
>>>> Implement PMBUS irq handler to notify regulator events.
>>>>
>>>> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
>>>> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
>>>
>>> As I am sure I have mentioned before, this needs to primarily handle
>>> sysfs notifications to hwmon status attributes and to generate kobject
>>> events. Regulator events are secondary / optional.
>>
>> Based on previous feedback, PMBus interrupt handler is made generic
>> Based on the use case I have in my machine, my application need to 
>> monitor regulator event as soon as they occur and hence the patch.
>>
> 
> I understand, but this isn't just about your specific use case. Your use 
> case is
> what triggers the change, and ensures that the code change is tested, 
> but the
> impact by far reaches beyond your specific use case and needs to address 
> other
> (much more common) use cases as well. Interrupt support is needed in the 
> pmbus
> code, but it needs to address the common use case first, and that is 
> reporting
> the status via sysfs notifications and kobject events.
Agree. I've done the implementation. Will submit the change as separate 
patch along with the series.
> 
> Thanks,
> Guenter
> 
Regards,
Naresh