[PATCH RESEND v6 1/5] hwmon: (pmbus/core): Add interrupt support

[PATCH RESEND v6 1/5] hwmon: (pmbus/core): Add interrupt support

[Naresh Solanki]

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

Implement PMBUS irq handler.

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 | 84 ++++++++++++++++++++++++++++++++
 2 files changed, 85 insertions(+), 1 deletion(-)

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 95e95783972a..244fd2597252 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -3072,11 +3072,89 @@ static int pmbus_regulator_register(struct pmbus_data *data)
 
 	return 0;
 }
+
+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, status;
+
+	for (i = 0; i < data->info->pages; i++) {
+
+		mutex_lock(&data->update_lock);
+		status = pmbus_read_status_word(client, i);
+		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, i);
+
+		mutex_unlock(&data->update_lock);
+	}
+
+	return IRQ_HANDLED;
+}
+
+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, func;
+	u8 mask;
+
+	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[j];
+			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 */
@@ -3441,6 +3519,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");

base-commit: 364ffd2537c44cb6914ff5669153f4a86fffad29
-- 
2.37.3

[PATCH RESEND v6 2/5] hwmon: (pmbus/core): Notify hwmon events

[Naresh Solanki]

Notify hwmon events using the pmbus irq handler.

Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
---
 drivers/hwmon/pmbus/pmbus_core.c | 95 ++++++++++++++++++++++++--------
 1 file changed, 72 insertions(+), 23 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 244fd2597252..b005a1c8ad7e 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -2781,18 +2781,43 @@ 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)
+#define to_dev_attr(_dev_attr) \
+	container_of(_dev_attr, struct device_attribute, attr)
+
+static void pmbus_notify(struct pmbus_data *data, int page, int reg, int flags)
+{
+	int i;
+
+	for (i = 0; i < data->num_attributes; i++) {
+		struct device_attribute *da = to_dev_attr(data->group.attrs[i]);
+		struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
+		int index = attr->index;
+		u16 smask = pb_index_to_mask(index);
+		u8 spage = pb_index_to_page(index);
+		u16 sreg = pb_index_to_reg(index);
+
+		if (reg == sreg && page == spage && (smask & flags)) {
+			dev_dbg(data->dev, "sysfs notify: %s", da->attr.name);
+			sysfs_notify(&data->dev->kobj, NULL, da->attr.name);
+			kobject_uevent(&data->dev->kobj, KOBJ_CHANGE);
+			flags &= ~smask;
+		}
+
+		if (!flags)
+			break;
+	}
+}
+
+static int pmbus_get_flags(struct pmbus_data *data, u8 page, unsigned int *error,
+				    bool notify)
 {
-	int i, status;
 	const struct pmbus_regulator_status_category *cat;
 	const struct pmbus_regulator_status_assoc *bit;
-	struct device *dev = rdev_get_dev(rdev);
-	struct i2c_client *client = to_i2c_client(dev->parent);
-	struct pmbus_data *data = i2c_get_clientdata(client);
-	u8 page = rdev_get_id(rdev);
+	struct i2c_client *client = to_i2c_client(data->dev);
 	int func = data->info->func[page];
+	int i, status, ret;
 
-	*flags = 0;
+	*error = 0;
 
 	mutex_lock(&data->update_lock);
 
@@ -2803,14 +2828,17 @@ static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned
 
 		status = _pmbus_read_byte_data(client, page, cat->reg);
 		if (status < 0) {
-			mutex_unlock(&data->update_lock);
-			return status;
+			ret = status;
+			goto unlock;
 		}
 
 		for (bit = cat->bits; bit->pflag; bit++) {
 			if (status & bit->pflag)
-				*flags |= bit->rflag;
+				*error |= bit->rflag;
 		}
+
+		if (notify && status)
+			pmbus_notify(data, page, cat->reg, status);
 	}
 
 	/*
@@ -2823,36 +2851,53 @@ static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned
 	 * REGULATOR_ERROR_<foo>_WARN.
 	 */
 	status = pmbus_get_status(client, page, PMBUS_STATUS_WORD);
-	mutex_unlock(&data->update_lock);
-	if (status < 0)
-		return status;
 
-	if (pmbus_regulator_is_enabled(rdev)) {
+	if (status < 0) {
+		ret = status;
+		goto unlock;
+	}
+
+	ret = _pmbus_read_byte_data(client, page, PMBUS_OPERATION);
+	if (ret < 0)
+		goto unlock;
+
+	if (ret & PB_OPERATION_CONTROL_ON) {
 		if (status & PB_STATUS_OFF)
-			*flags |= REGULATOR_ERROR_FAIL;
+			*error |= REGULATOR_ERROR_FAIL;
 
 		if (status & PB_STATUS_POWER_GOOD_N)
-			*flags |= REGULATOR_ERROR_REGULATION_OUT;
+			*error |= 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;
+		*error |= REGULATOR_ERROR_OVER_CURRENT;
 	if (status & PB_STATUS_VOUT_OV)
-		*flags |= REGULATOR_ERROR_REGULATION_OUT;
+		*error |= REGULATOR_ERROR_REGULATION_OUT;
 
 	/*
 	 * If we haven't discovered any thermal faults or warnings via
 	 * 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)) &&
+	if (!(*error & (REGULATOR_ERROR_OVER_TEMP | REGULATOR_ERROR_OVER_TEMP_WARN)) &&
 	    (status & PB_STATUS_TEMPERATURE))
-		*flags |= REGULATOR_ERROR_OVER_TEMP_WARN;
+		*error |= REGULATOR_ERROR_OVER_TEMP_WARN;
 
-	return 0;
+unlock:
+	mutex_unlock(&data->update_lock);
+	return ret;
+}
+
+static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned int *flags)
+{
+	struct device *dev = rdev_get_dev(rdev);
+	struct i2c_client *client = to_i2c_client(dev->parent);
+	struct pmbus_data *data = i2c_get_clientdata(client);
+
+	return pmbus_get_flags(data, rdev_get_id(rdev), flags, false);
 }
 
 static int pmbus_regulator_get_status(struct regulator_dev *rdev)
@@ -3082,10 +3127,14 @@ 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, status;
+	int i, status, ret;
 
 	for (i = 0; i < data->info->pages; i++) {
 
+		ret = pmbus_get_flags(data, i, &status, true);
+		if (ret)
+			return ret;
+
 		mutex_lock(&data->update_lock);
 		status = pmbus_read_status_word(client, i);
 		if (status < 0) {
@@ -3099,7 +3148,7 @@ static irqreturn_t pmbus_fault_handler(int irq, void *pdata)
 		mutex_unlock(&data->update_lock);
 	}
 
-	return IRQ_HANDLED;
+	return ret;
 }
 
 static int pmbus_irq_setup(struct i2c_client *client, struct pmbus_data *data)
-- 
2.37.3

[PATCH RESEND v6 3/5] hwmon: (pmbus/core): Add rdev in pmbus_data struct

[Naresh Solanki]

Add regulator device in pmbus_data & initialize the same during PMBus
regulator register.

Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>

---
Change in V5:
- Fix error check for rdev
---
 drivers/hwmon/pmbus/pmbus_core.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index b005a1c8ad7e..afd98e639b4f 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 */
 
@@ -3095,9 +3096,13 @@ 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 = { };
 
@@ -3107,10 +3112,10 @@ 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);
 	}
-- 
2.37.3

[PATCH RESEND v6 4/5] hwmon: (pmbus/core): Add regulator event support

[Naresh Solanki]

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

Add regulator events corresponding to regulator error in regulator flag
map.
Also capture the same in pmbus_regulator_get_flags.

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

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index afd98e639b4f..22176f266891 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -2739,9 +2739,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 */
@@ -2756,27 +2756,36 @@ static const struct pmbus_regulator_status_category pmbus_regulator_flag_map[] =
 		.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_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 },
 			{ },
 		},
 	}, {
 		.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 },
 			{ },
 		},
 	},
@@ -2810,7 +2819,7 @@ static void pmbus_notify(struct pmbus_data *data, int page, int reg, int flags)
 }
 
 static int pmbus_get_flags(struct pmbus_data *data, u8 page, unsigned int *error,
-				    bool notify)
+				    unsigned int *event, bool notify)
 {
 	const struct pmbus_regulator_status_category *cat;
 	const struct pmbus_regulator_status_assoc *bit;
@@ -2819,6 +2828,7 @@ static int pmbus_get_flags(struct pmbus_data *data, u8 page, unsigned int *error
 	int i, status, ret;
 
 	*error = 0;
+	*event = 0;
 
 	mutex_lock(&data->update_lock);
 
@@ -2833,10 +2843,11 @@ static int pmbus_get_flags(struct pmbus_data *data, u8 page, unsigned int *error
 			goto unlock;
 		}
 
-		for (bit = cat->bits; bit->pflag; bit++) {
-			if (status & bit->pflag)
+		for (bit = cat->bits; bit->pflag; bit++)
+			if (status & bit->pflag) {
 				*error |= bit->rflag;
-		}
+				*event |= bit->eflag;
+			}
 
 		if (notify && status)
 			pmbus_notify(data, page, cat->reg, status);
@@ -2863,20 +2874,28 @@ static int pmbus_get_flags(struct pmbus_data *data, u8 page, unsigned int *error
 		goto unlock;
 
 	if (ret & PB_OPERATION_CONTROL_ON) {
-		if (status & PB_STATUS_OFF)
+		if (status & PB_STATUS_OFF) {
 			*error |= REGULATOR_ERROR_FAIL;
+			*event |= REGULATOR_EVENT_FAIL;
+		}
 
-		if (status & PB_STATUS_POWER_GOOD_N)
+		if (status & PB_STATUS_POWER_GOOD_N) {
 			*error |= REGULATOR_ERROR_REGULATION_OUT;
+			*event |= REGULATOR_EVENT_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)
+	if (status & PB_STATUS_IOUT_OC) {
 		*error |= REGULATOR_ERROR_OVER_CURRENT;
-	if (status & PB_STATUS_VOUT_OV)
+		*event |= REGULATOR_EVENT_OVER_CURRENT;
+	}
+	if (status & PB_STATUS_VOUT_OV) {
 		*error |= REGULATOR_ERROR_REGULATION_OUT;
+		*event |= REGULATOR_EVENT_FAIL;
+	}
 
 	/*
 	 * If we haven't discovered any thermal faults or warnings via
@@ -2884,8 +2903,10 @@ static int pmbus_get_flags(struct pmbus_data *data, u8 page, unsigned int *error
 	 * a (conservative) best-effort interpretation.
 	 */
 	if (!(*error & (REGULATOR_ERROR_OVER_TEMP | REGULATOR_ERROR_OVER_TEMP_WARN)) &&
-	    (status & PB_STATUS_TEMPERATURE))
+	    (status & PB_STATUS_TEMPERATURE)) {
 		*error |= REGULATOR_ERROR_OVER_TEMP_WARN;
+		*event |= REGULATOR_EVENT_OVER_TEMP_WARN;
+	}
 
 unlock:
 	mutex_unlock(&data->update_lock);
@@ -2897,8 +2918,9 @@ static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned
 	struct device *dev = rdev_get_dev(rdev);
 	struct i2c_client *client = to_i2c_client(dev->parent);
 	struct pmbus_data *data = i2c_get_clientdata(client);
+	int event;
 
-	return pmbus_get_flags(data, rdev_get_id(rdev), flags, false);
+	return pmbus_get_flags(data, rdev_get_id(rdev), flags, &event, false);
 }
 
 static int pmbus_regulator_get_status(struct regulator_dev *rdev)
@@ -3132,11 +3154,12 @@ 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, status, ret;
+	int i, status, ret, event;
 
 	for (i = 0; i < data->info->pages; i++) {
 
-		ret = pmbus_get_flags(data, i, &status, true);
+		ret = pmbus_get_flags(data, i, &status, &event, true);
+
 		if (ret)
 			return ret;
 
-- 
2.37.3

[PATCH RESEND v6 5/5] hwmon: (pmbus/core): Notify regulator events

[Naresh Solanki]

Notify regulator events in PMBus irq handler.

Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
---
 drivers/hwmon/pmbus/pmbus_core.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 22176f266891..c8fae2a9502d 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -3154,7 +3154,7 @@ 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, status, ret, event;
+	int i, j, status, ret, event;
 
 	for (i = 0; i < data->info->pages; i++) {
 
@@ -3163,6 +3163,15 @@ static irqreturn_t pmbus_fault_handler(int irq, void *pdata)
 		if (ret)
 			return ret;
 
+		if (event) {
+			for (j = 0; j < data->info->num_regulators; j++) {
+				if (i == rdev_get_id(data->rdevs[i])) {
+					regulator_notifier_call_chain(data->rdevs[i], event, NULL);
+					ret = IRQ_HANDLED;
+				}
+			}
+		}
+
 		mutex_lock(&data->update_lock);
 		status = pmbus_read_status_word(client, i);
 		if (status < 0) {
-- 
2.37.3

Re: [PATCH RESEND v6 1/5] hwmon: (pmbus/core): Add interrupt support

[Guenter Roeck]

On Wed, Dec 14, 2022 at 09:07:11AM +0100, Naresh Solanki wrote:
> From: Patrick Rudolph <patrick.rudolph@9elements.com>
> 
> Implement PMBUS irq handler.
> 
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>

$ scripts/checkpatch.pl --strict index.html
CHECK: Blank lines aren't necessary after an open brace '{'
#131: FILE: drivers/hwmon/pmbus/pmbus_core.c:3088:
+	for (i = 0; i < data->info->pages; i++) {
+

CHECK: Alignment should match open parenthesis
#183: FILE: drivers/hwmon/pmbus/pmbus_core.c:3140:
+	ret = devm_request_threaded_irq(dev, client->irq, NULL, pmbus_fault_handler,
+			      0, "pmbus-irq", data);

CHECK: Please use a blank line after function/struct/union/enum declarations
#197: FILE: drivers/hwmon/pmbus/pmbus_core.c:3154:
 }
+static int pmbus_irq_setup(struct i2c_client *client, struct pmbus_data *data)

total: 0 errors, 0 warnings, 3 checks, 109 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

index.html has style problems, please review.

Please run checkpatch --strict on your patches.
Also see Documentation/hwmon/submitting-patches.rst.

> ---
>  drivers/hwmon/pmbus/pmbus.h      |  2 +-
>  drivers/hwmon/pmbus/pmbus_core.c | 84 ++++++++++++++++++++++++++++++++
>  2 files changed, 85 insertions(+), 1 deletion(-)
> 
> 
> base-commit: 364ffd2537c44cb6914ff5669153f4a86fffad29
> 
> 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 95e95783972a..244fd2597252 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -3072,11 +3072,89 @@ static int pmbus_regulator_register(struct pmbus_data *data)
>  
>  	return 0;
>  }
> +
> +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, status;
> +
> +	for (i = 0; i < data->info->pages; i++) {
> +
> +		mutex_lock(&data->update_lock);
> +		status = pmbus_read_status_word(client, i);
> +		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, i);
> +
> +		mutex_unlock(&data->update_lock);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +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, func;
> +	u8 mask;
> +
> +	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[j];
> +			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);

This concerns me. It might mean that the chip does not support
PMBUS_SMBALERT_MASK. If so, there would be lots of error messages.

> +		}
> +
> +		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);

Why check the return value from pmbus_write_smbalert_mask above but not here ?

> +		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 */

The comment does not make sense. pmbus_irq_setup() is not called
if the interrupt "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);

This is not a warning, it is an error.

> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  #else

This is still in regulator code. I said several times that this is not
acceptable.

>  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 */
> @@ -3441,6 +3519,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");

Re: [PATCH RESEND v6 2/5] hwmon: (pmbus/core): Notify hwmon events

[Guenter Roeck]

On Wed, Dec 14, 2022 at 09:07:12AM +0100, Naresh Solanki wrote:
> Notify hwmon events using the pmbus irq handler.
> 
> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> ---
>  drivers/hwmon/pmbus/pmbus_core.c | 95 ++++++++++++++++++++++++--------
>  1 file changed, 72 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 244fd2597252..b005a1c8ad7e 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -2781,18 +2781,43 @@ 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)
> +#define to_dev_attr(_dev_attr) \
> +	container_of(_dev_attr, struct device_attribute, attr)
> +
> +static void pmbus_notify(struct pmbus_data *data, int page, int reg, int flags)
> +{
> +	int i;
> +
> +	for (i = 0; i < data->num_attributes; i++) {
> +		struct device_attribute *da = to_dev_attr(data->group.attrs[i]);
> +		struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> +		int index = attr->index;
> +		u16 smask = pb_index_to_mask(index);
> +		u8 spage = pb_index_to_page(index);
> +		u16 sreg = pb_index_to_reg(index);
> +
> +		if (reg == sreg && page == spage && (smask & flags)) {
> +			dev_dbg(data->dev, "sysfs notify: %s", da->attr.name);
> +			sysfs_notify(&data->dev->kobj, NULL, da->attr.name);
> +			kobject_uevent(&data->dev->kobj, KOBJ_CHANGE);
> +			flags &= ~smask;
> +		}
> +
> +		if (!flags)
> +			break;
> +	}
> +}

Interrupt aupport as well as sysfs and kobject notifications are not
regulator specific and do not depend on regulator support. It has to be
independent of regulator support, meaning it must also work if regulator
support is disabled.

I seem to have trouble expressing myself, but I don't know how else to say
it, sorry.

It doesn't make sense to review the series further until this is addressed.

Guenter

> +
> +static int pmbus_get_flags(struct pmbus_data *data, u8 page, unsigned int *error,
> +				    bool notify)
>  {
> -	int i, status;
>  	const struct pmbus_regulator_status_category *cat;
>  	const struct pmbus_regulator_status_assoc *bit;
> -	struct device *dev = rdev_get_dev(rdev);
> -	struct i2c_client *client = to_i2c_client(dev->parent);
> -	struct pmbus_data *data = i2c_get_clientdata(client);
> -	u8 page = rdev_get_id(rdev);
> +	struct i2c_client *client = to_i2c_client(data->dev);
>  	int func = data->info->func[page];
> +	int i, status, ret;
>  
> -	*flags = 0;
> +	*error = 0;
>  
>  	mutex_lock(&data->update_lock);
>  
> @@ -2803,14 +2828,17 @@ static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned
>  
>  		status = _pmbus_read_byte_data(client, page, cat->reg);
>  		if (status < 0) {
> -			mutex_unlock(&data->update_lock);
> -			return status;
> +			ret = status;
> +			goto unlock;
>  		}
>  
>  		for (bit = cat->bits; bit->pflag; bit++) {
>  			if (status & bit->pflag)
> -				*flags |= bit->rflag;
> +				*error |= bit->rflag;
>  		}
> +
> +		if (notify && status)
> +			pmbus_notify(data, page, cat->reg, status);
>  	}
>  
>  	/*
> @@ -2823,36 +2851,53 @@ static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned
>  	 * REGULATOR_ERROR_<foo>_WARN.
>  	 */
>  	status = pmbus_get_status(client, page, PMBUS_STATUS_WORD);
> -	mutex_unlock(&data->update_lock);
> -	if (status < 0)
> -		return status;
>  
> -	if (pmbus_regulator_is_enabled(rdev)) {
> +	if (status < 0) {
> +		ret = status;
> +		goto unlock;
> +	}
> +
> +	ret = _pmbus_read_byte_data(client, page, PMBUS_OPERATION);
> +	if (ret < 0)
> +		goto unlock;
> +
> +	if (ret & PB_OPERATION_CONTROL_ON) {
>  		if (status & PB_STATUS_OFF)
> -			*flags |= REGULATOR_ERROR_FAIL;
> +			*error |= REGULATOR_ERROR_FAIL;
>  
>  		if (status & PB_STATUS_POWER_GOOD_N)
> -			*flags |= REGULATOR_ERROR_REGULATION_OUT;
> +			*error |= 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;
> +		*error |= REGULATOR_ERROR_OVER_CURRENT;
>  	if (status & PB_STATUS_VOUT_OV)
> -		*flags |= REGULATOR_ERROR_REGULATION_OUT;
> +		*error |= REGULATOR_ERROR_REGULATION_OUT;
>  
>  	/*
>  	 * If we haven't discovered any thermal faults or warnings via
>  	 * 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)) &&
> +	if (!(*error & (REGULATOR_ERROR_OVER_TEMP | REGULATOR_ERROR_OVER_TEMP_WARN)) &&
>  	    (status & PB_STATUS_TEMPERATURE))
> -		*flags |= REGULATOR_ERROR_OVER_TEMP_WARN;
> +		*error |= REGULATOR_ERROR_OVER_TEMP_WARN;
>  
> -	return 0;
> +unlock:
> +	mutex_unlock(&data->update_lock);
> +	return ret;
> +}
> +
> +static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned int *flags)
> +{
> +	struct device *dev = rdev_get_dev(rdev);
> +	struct i2c_client *client = to_i2c_client(dev->parent);
> +	struct pmbus_data *data = i2c_get_clientdata(client);
> +
> +	return pmbus_get_flags(data, rdev_get_id(rdev), flags, false);
>  }
>  
>  static int pmbus_regulator_get_status(struct regulator_dev *rdev)
> @@ -3082,10 +3127,14 @@ 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, status;
> +	int i, status, ret;
>  
>  	for (i = 0; i < data->info->pages; i++) {
>  
> +		ret = pmbus_get_flags(data, i, &status, true);
> +		if (ret)
> +			return ret;
> +
>  		mutex_lock(&data->update_lock);
>  		status = pmbus_read_status_word(client, i);
>  		if (status < 0) {
> @@ -3099,7 +3148,7 @@ static irqreturn_t pmbus_fault_handler(int irq, void *pdata)
>  		mutex_unlock(&data->update_lock);
>  	}
>  
> -	return IRQ_HANDLED;
> +	return ret;
>  }
>  
>  static int pmbus_irq_setup(struct i2c_client *client, struct pmbus_data *data)

Re: [PATCH RESEND v6 1/5] hwmon: (pmbus/core): Add interrupt support

[Naresh Solanki]

Hi Guenter,

On 29-12-2022 08:10 pm, Guenter Roeck wrote:
> On Wed, Dec 14, 2022 at 09:07:11AM +0100, Naresh Solanki wrote:
>> From: Patrick Rudolph <patrick.rudolph@9elements.com>
>>
>> Implement PMBUS irq handler.
>>
>> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
>> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> 
> $ scripts/checkpatch.pl --strict index.html
> CHECK: Blank lines aren't necessary after an open brace '{'
> #131: FILE: drivers/hwmon/pmbus/pmbus_core.c:3088:
> +	for (i = 0; i < data->info->pages; i++) {
> +
> 
> CHECK: Alignment should match open parenthesis
> #183: FILE: drivers/hwmon/pmbus/pmbus_core.c:3140:
> +	ret = devm_request_threaded_irq(dev, client->irq, NULL, pmbus_fault_handler,
> +			      0, "pmbus-irq", data);
> 
> CHECK: Please use a blank line after function/struct/union/enum declarations
> #197: FILE: drivers/hwmon/pmbus/pmbus_core.c:3154:
>   }
> +static int pmbus_irq_setup(struct i2c_client *client, struct pmbus_data *data)
> 
> total: 0 errors, 0 warnings, 3 checks, 109 lines checked
> 
> NOTE: For some of the reported defects, checkpatch may be able to
>        mechanically convert to the typical style using --fix or --fix-inplace.
> 
> index.html has style problems, please review.
> 
> Please run checkpatch --strict on your patches.
> Also see Documentation/hwmon/submitting-patches.rst.
I will take care of these errors in the updated version.
> 
>> ---
>>   drivers/hwmon/pmbus/pmbus.h      |  2 +-
>>   drivers/hwmon/pmbus/pmbus_core.c | 84 ++++++++++++++++++++++++++++++++
>>   2 files changed, 85 insertions(+), 1 deletion(-)
>>
>>
>> base-commit: 364ffd2537c44cb6914ff5669153f4a86fffad29
>>
>> 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 95e95783972a..244fd2597252 100644
>> --- a/drivers/hwmon/pmbus/pmbus_core.c
>> +++ b/drivers/hwmon/pmbus/pmbus_core.c
>> @@ -3072,11 +3072,89 @@ static int pmbus_regulator_register(struct pmbus_data *data)
>>   
>>   	return 0;
>>   }
>> +
>> +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, status;
>> +
>> +	for (i = 0; i < data->info->pages; i++) {
>> +
>> +		mutex_lock(&data->update_lock);
>> +		status = pmbus_read_status_word(client, i);
>> +		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, i);
>> +
>> +		mutex_unlock(&data->update_lock);
>> +	}
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +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, func;
>> +	u8 mask;
>> +
>> +	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[j];
>> +			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);
> 
> This concerns me. It might mean that the chip does not support
> PMBUS_SMBALERT_MASK. If so, there would be lots of error messages.
After going through the PMBus specification, it appears that this should 
not be an issue unless there is a violation of the specification.
> 
>> +		}
>> +
>> +		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);
> 
> Why check the return value from pmbus_write_smbalert_mask above but not here ?
Thank you for pointing out the oversight. I will make sure to include an 
error check at this point.
> 
>> +		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 */
> 
> The comment does not make sense. pmbus_irq_setup() is not called
> if the interrupt "is not given".
Yes. The comment here is not relevant and will be removed.
> 
>> +	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);
> 
> This is not a warning, it is an error.
Thank you for bringing this to my attention. I will make sure to update 
the code to reflect that this is an error.
> 
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   #else
> 
> This is still in regulator code. I said several times that this is not
> acceptable.
Thank you for pointing out the mistake. I will make sure to correct this 
in the next revision.
> 
>>   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 */
>> @@ -3441,6 +3519,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");

Thanks,
Naresh

Re: [PATCH RESEND v6 2/5] hwmon: (pmbus/core): Notify hwmon events

[Naresh Solanki]

Hi Guenter,

On 29-12-2022 08:16 pm, Guenter Roeck wrote:
> On Wed, Dec 14, 2022 at 09:07:12AM +0100, Naresh Solanki wrote:
>> Notify hwmon events using the pmbus irq handler.
>>
>> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
>> ---
>>   drivers/hwmon/pmbus/pmbus_core.c | 95 ++++++++++++++++++++++++--------
>>   1 file changed, 72 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
>> index 244fd2597252..b005a1c8ad7e 100644
>> --- a/drivers/hwmon/pmbus/pmbus_core.c
>> +++ b/drivers/hwmon/pmbus/pmbus_core.c
>> @@ -2781,18 +2781,43 @@ 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)
>> +#define to_dev_attr(_dev_attr) \
>> +	container_of(_dev_attr, struct device_attribute, attr)
>> +
>> +static void pmbus_notify(struct pmbus_data *data, int page, int reg, int flags)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < data->num_attributes; i++) {
>> +		struct device_attribute *da = to_dev_attr(data->group.attrs[i]);
>> +		struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
>> +		int index = attr->index;
>> +		u16 smask = pb_index_to_mask(index);
>> +		u8 spage = pb_index_to_page(index);
>> +		u16 sreg = pb_index_to_reg(index);
>> +
>> +		if (reg == sreg && page == spage && (smask & flags)) {
>> +			dev_dbg(data->dev, "sysfs notify: %s", da->attr.name);
>> +			sysfs_notify(&data->dev->kobj, NULL, da->attr.name);
>> +			kobject_uevent(&data->dev->kobj, KOBJ_CHANGE);
>> +			flags &= ~smask;
>> +		}
>> +
>> +		if (!flags)
>> +			break;
>> +	}
>> +}
> 
> Interrupt aupport as well as sysfs and kobject notifications are not
> regulator specific and do not depend on regulator support. It has to be
> independent of regulator support, meaning it must also work if regulator
> support is disabled.
> 
> I seem to have trouble expressing myself, but I don't know how else to say
> it, sorry.
> 
> It doesn't make sense to review the series further until this is addressed.
I understand your concern about the independence of the interrupt 
support, sysfs, and kobject notifications from the regulator support. I 
will make sure to address this issue and ensure that these features work 
regardless of whether regulator support is enabled or disabled in the 
next revision. Thank you for your patience and for your feedback.
I will make sure to address your concerns.
> 
> Guenter
> 
>> +
>> +static int pmbus_get_flags(struct pmbus_data *data, u8 page, unsigned int *error,
>> +				    bool notify)
>>   {
>> -	int i, status;
>>   	const struct pmbus_regulator_status_category *cat;
>>   	const struct pmbus_regulator_status_assoc *bit;
>> -	struct device *dev = rdev_get_dev(rdev);
>> -	struct i2c_client *client = to_i2c_client(dev->parent);
>> -	struct pmbus_data *data = i2c_get_clientdata(client);
>> -	u8 page = rdev_get_id(rdev);
>> +	struct i2c_client *client = to_i2c_client(data->dev);
>>   	int func = data->info->func[page];
>> +	int i, status, ret;
>>   
>> -	*flags = 0;
>> +	*error = 0;
>>   
>>   	mutex_lock(&data->update_lock);
>>   
>> @@ -2803,14 +2828,17 @@ static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned
>>   
>>   		status = _pmbus_read_byte_data(client, page, cat->reg);
>>   		if (status < 0) {
>> -			mutex_unlock(&data->update_lock);
>> -			return status;
>> +			ret = status;
>> +			goto unlock;
>>   		}
>>   
>>   		for (bit = cat->bits; bit->pflag; bit++) {
>>   			if (status & bit->pflag)
>> -				*flags |= bit->rflag;
>> +				*error |= bit->rflag;
>>   		}
>> +
>> +		if (notify && status)
>> +			pmbus_notify(data, page, cat->reg, status);
>>   	}
>>   
>>   	/*
>> @@ -2823,36 +2851,53 @@ static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned
>>   	 * REGULATOR_ERROR_<foo>_WARN.
>>   	 */
>>   	status = pmbus_get_status(client, page, PMBUS_STATUS_WORD);
>> -	mutex_unlock(&data->update_lock);
>> -	if (status < 0)
>> -		return status;
>>   
>> -	if (pmbus_regulator_is_enabled(rdev)) {
>> +	if (status < 0) {
>> +		ret = status;
>> +		goto unlock;
>> +	}
>> +
>> +	ret = _pmbus_read_byte_data(client, page, PMBUS_OPERATION);
>> +	if (ret < 0)
>> +		goto unlock;
>> +
>> +	if (ret & PB_OPERATION_CONTROL_ON) {
>>   		if (status & PB_STATUS_OFF)
>> -			*flags |= REGULATOR_ERROR_FAIL;
>> +			*error |= REGULATOR_ERROR_FAIL;
>>   
>>   		if (status & PB_STATUS_POWER_GOOD_N)
>> -			*flags |= REGULATOR_ERROR_REGULATION_OUT;
>> +			*error |= 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;
>> +		*error |= REGULATOR_ERROR_OVER_CURRENT;
>>   	if (status & PB_STATUS_VOUT_OV)
>> -		*flags |= REGULATOR_ERROR_REGULATION_OUT;
>> +		*error |= REGULATOR_ERROR_REGULATION_OUT;
>>   
>>   	/*
>>   	 * If we haven't discovered any thermal faults or warnings via
>>   	 * 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)) &&
>> +	if (!(*error & (REGULATOR_ERROR_OVER_TEMP | REGULATOR_ERROR_OVER_TEMP_WARN)) &&
>>   	    (status & PB_STATUS_TEMPERATURE))
>> -		*flags |= REGULATOR_ERROR_OVER_TEMP_WARN;
>> +		*error |= REGULATOR_ERROR_OVER_TEMP_WARN;
>>   
>> -	return 0;
>> +unlock:
>> +	mutex_unlock(&data->update_lock);
>> +	return ret;
>> +}
>> +
>> +static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned int *flags)
>> +{
>> +	struct device *dev = rdev_get_dev(rdev);
>> +	struct i2c_client *client = to_i2c_client(dev->parent);
>> +	struct pmbus_data *data = i2c_get_clientdata(client);
>> +
>> +	return pmbus_get_flags(data, rdev_get_id(rdev), flags, false);
>>   }
>>   
>>   static int pmbus_regulator_get_status(struct regulator_dev *rdev)
>> @@ -3082,10 +3127,14 @@ 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, status;
>> +	int i, status, ret;
>>   
>>   	for (i = 0; i < data->info->pages; i++) {
>>   
>> +		ret = pmbus_get_flags(data, i, &status, true);
>> +		if (ret)
>> +			return ret;
>> +
>>   		mutex_lock(&data->update_lock);
>>   		status = pmbus_read_status_word(client, i);
>>   		if (status < 0) {
>> @@ -3099,7 +3148,7 @@ static irqreturn_t pmbus_fault_handler(int irq, void *pdata)
>>   		mutex_unlock(&data->update_lock);
>>   	}
>>   
>> -	return IRQ_HANDLED;
>> +	return ret;
>>   }
>>   
>>   static int pmbus_irq_setup(struct i2c_client *client, struct pmbus_data *data)
Regards,
Naresh

Re: [PATCH RESEND v6 1/5] hwmon: (pmbus/core): Add interrupt support

[Guenter Roeck]

On Tue, Jan 03, 2023 at 12:18:49PM +0530, Naresh Solanki wrote:
> Hi Guenter,
> 
> On 29-12-2022 08:10 pm, Guenter Roeck wrote:
> > On Wed, Dec 14, 2022 at 09:07:11AM +0100, Naresh Solanki wrote:
> > > From: Patrick Rudolph <patrick.rudolph@9elements.com>
> > > 
> > > Implement PMBUS irq handler.
> > > 
> > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> > > Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> > 
> > $ scripts/checkpatch.pl --strict index.html
> > CHECK: Blank lines aren't necessary after an open brace '{'
> > #131: FILE: drivers/hwmon/pmbus/pmbus_core.c:3088:
> > +	for (i = 0; i < data->info->pages; i++) {
> > +
> > 
> > CHECK: Alignment should match open parenthesis
> > #183: FILE: drivers/hwmon/pmbus/pmbus_core.c:3140:
> > +	ret = devm_request_threaded_irq(dev, client->irq, NULL, pmbus_fault_handler,
> > +			      0, "pmbus-irq", data);
> > 
> > CHECK: Please use a blank line after function/struct/union/enum declarations
> > #197: FILE: drivers/hwmon/pmbus/pmbus_core.c:3154:
> >   }
> > +static int pmbus_irq_setup(struct i2c_client *client, struct pmbus_data *data)
> > 
> > total: 0 errors, 0 warnings, 3 checks, 109 lines checked
> > 
> > NOTE: For some of the reported defects, checkpatch may be able to
> >        mechanically convert to the typical style using --fix or --fix-inplace.
> > 
> > index.html has style problems, please review.
> > 
> > Please run checkpatch --strict on your patches.
> > Also see Documentation/hwmon/submitting-patches.rst.
> I will take care of these errors in the updated version.
> > 
> > > ---
> > >   drivers/hwmon/pmbus/pmbus.h      |  2 +-
> > >   drivers/hwmon/pmbus/pmbus_core.c | 84 ++++++++++++++++++++++++++++++++
> > >   2 files changed, 85 insertions(+), 1 deletion(-)
> > > 
> > > 
> > > base-commit: 364ffd2537c44cb6914ff5669153f4a86fffad29
> > > 
> > > 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 95e95783972a..244fd2597252 100644
> > > --- a/drivers/hwmon/pmbus/pmbus_core.c
> > > +++ b/drivers/hwmon/pmbus/pmbus_core.c
> > > @@ -3072,11 +3072,89 @@ static int pmbus_regulator_register(struct pmbus_data *data)
> > >   	return 0;
> > >   }
> > > +
> > > +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, status;
> > > +
> > > +	for (i = 0; i < data->info->pages; i++) {
> > > +
> > > +		mutex_lock(&data->update_lock);
> > > +		status = pmbus_read_status_word(client, i);
> > > +		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, i);
> > > +
> > > +		mutex_unlock(&data->update_lock);
> > > +	}
> > > +
> > > +	return IRQ_HANDLED;
> > > +}
> > > +
> > > +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, func;
> > > +	u8 mask;
> > > +
> > > +	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[j];
> > > +			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);
> > 
> > This concerns me. It might mean that the chip does not support
> > PMBUS_SMBALERT_MASK. If so, there would be lots of error messages.
> After going through the PMBus specification, it appears that this should not
> be an issue unless there is a violation of the specification.

PMBus chips have lots of issues which violate the specification.
Have a look at the various drivers and the workarounds implemented there.
You'll need to check if the command/register is supported before using it.
Also, if you want to keep the error message, make it dev_err_once().

Either case, an error is an error, not to be ignored. An error here
should result in an error abort.

> > 
> > > +		}
> > > +
> > > +		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);
> > 
> > Why check the return value from pmbus_write_smbalert_mask above but not here ?
> Thank you for pointing out the oversight. I will make sure to include an
> error check at this point.
> > 
> > > +		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 */
> > 
> > The comment does not make sense. pmbus_irq_setup() is not called
> > if the interrupt "is not given".
> Yes. The comment here is not relevant and will be removed.
> > 
> > > +	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);
> > 
> > This is not a warning, it is an error.
> Thank you for bringing this to my attention. I will make sure to update the
> code to reflect that this is an error.
> > 
> > > +		return ret;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >   #else
> > 
> > This is still in regulator code. I said several times that this is not
> > acceptable.
> Thank you for pointing out the mistake. I will make sure to correct this in
> the next revision.
> > 
> > >   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 */
> > > @@ -3441,6 +3519,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");
> 
> Thanks,
> Naresh

Re: [PATCH RESEND v6 1/5] hwmon: (pmbus/core): Add interrupt support

[Naresh Solanki]

Hi Guenter

On 03-01-2023 05:56 pm, Guenter Roeck wrote:
> On Tue, Jan 03, 2023 at 12:18:49PM +0530, Naresh Solanki wrote:
>> Hi Guenter,
>>
>> On 29-12-2022 08:10 pm, Guenter Roeck wrote:
>>> On Wed, Dec 14, 2022 at 09:07:11AM +0100, Naresh Solanki wrote:
>>>> From: Patrick Rudolph <patrick.rudolph@9elements.com>
>>>>
>>>> Implement PMBUS irq handler.
>>>>
>>>> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
>>>> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
>>>
>>> $ scripts/checkpatch.pl --strict index.html
>>> CHECK: Blank lines aren't necessary after an open brace '{'
>>> #131: FILE: drivers/hwmon/pmbus/pmbus_core.c:3088:
>>> +	for (i = 0; i < data->info->pages; i++) {
>>> +
>>>
>>> CHECK: Alignment should match open parenthesis
>>> #183: FILE: drivers/hwmon/pmbus/pmbus_core.c:3140:
>>> +	ret = devm_request_threaded_irq(dev, client->irq, NULL, pmbus_fault_handler,
>>> +			      0, "pmbus-irq", data);
>>>
>>> CHECK: Please use a blank line after function/struct/union/enum declarations
>>> #197: FILE: drivers/hwmon/pmbus/pmbus_core.c:3154:
>>>    }
>>> +static int pmbus_irq_setup(struct i2c_client *client, struct pmbus_data *data)
>>>
>>> total: 0 errors, 0 warnings, 3 checks, 109 lines checked
>>>
>>> NOTE: For some of the reported defects, checkpatch may be able to
>>>         mechanically convert to the typical style using --fix or --fix-inplace.
>>>
>>> index.html has style problems, please review.
>>>
>>> Please run checkpatch --strict on your patches.
>>> Also see Documentation/hwmon/submitting-patches.rst.
>> I will take care of these errors in the updated version.
>>>
>>>> ---
>>>>    drivers/hwmon/pmbus/pmbus.h      |  2 +-
>>>>    drivers/hwmon/pmbus/pmbus_core.c | 84 ++++++++++++++++++++++++++++++++
>>>>    2 files changed, 85 insertions(+), 1 deletion(-)
>>>>
>>>>
>>>> base-commit: 364ffd2537c44cb6914ff5669153f4a86fffad29
>>>>
>>>> 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 95e95783972a..244fd2597252 100644
>>>> --- a/drivers/hwmon/pmbus/pmbus_core.c
>>>> +++ b/drivers/hwmon/pmbus/pmbus_core.c
>>>> @@ -3072,11 +3072,89 @@ static int pmbus_regulator_register(struct pmbus_data *data)
>>>>    	return 0;
>>>>    }
>>>> +
>>>> +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, status;
>>>> +
>>>> +	for (i = 0; i < data->info->pages; i++) {
>>>> +
>>>> +		mutex_lock(&data->update_lock);
>>>> +		status = pmbus_read_status_word(client, i);
>>>> +		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, i);
>>>> +
>>>> +		mutex_unlock(&data->update_lock);
>>>> +	}
>>>> +
>>>> +	return IRQ_HANDLED;
>>>> +}
>>>> +
>>>> +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, func;
>>>> +	u8 mask;
>>>> +
>>>> +	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[j];
>>>> +			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);
>>>
>>> This concerns me. It might mean that the chip does not support
>>> PMBUS_SMBALERT_MASK. If so, there would be lots of error messages.
>> After going through the PMBus specification, it appears that this should not
>> be an issue unless there is a violation of the specification.
> 
> PMBus chips have lots of issues which violate the specification.
> Have a look at the various drivers and the workarounds implemented there.
> You'll need to check if the command/register is supported before using it.
> Also, if you want to keep the error message, make it dev_err_once().
> 
> Either case, an error is an error, not to be ignored. An error here
> should result in an error abort.
Yes, I agree that PMBus chips can have issues that violate the 
specification, and that it is important to check whether a command or 
register is supported before using it.
I have noticed that many drivers use the PMBUS_HAVE_* flags to expose 
the presence of specific registers, and I think it would be a good idea 
to add a PMBUS_HAVE_SMBALERT flag as well, so that drivers for supported 
chips can use it to determine whether they should set up an IRQ handler 
or not. If PMBUS_HAVE_SMBALERT is set, then the IRQ handler should be 
set up, otherwise it should be ignored.
Will this approach be right?
> 
>>>
>>>> +		}
>>>> +
>>>> +		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);
>>>
>>> Why check the return value from pmbus_write_smbalert_mask above but not here ?
>> Thank you for pointing out the oversight. I will make sure to include an
>> error check at this point.
>>>
>>>> +		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 */
>>>
>>> The comment does not make sense. pmbus_irq_setup() is not called
>>> if the interrupt "is not given".
>> Yes. The comment here is not relevant and will be removed.
>>>
>>>> +	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);
>>>
>>> This is not a warning, it is an error.
>> Thank you for bringing this to my attention. I will make sure to update the
>> code to reflect that this is an error.
>>>
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>>    #else
>>>
>>> This is still in regulator code. I said several times that this is not
>>> acceptable.
>> Thank you for pointing out the mistake. I will make sure to correct this in
>> the next revision.
>>>
>>>>    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 */
>>>> @@ -3441,6 +3519,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");
>>
>> Thanks,
>> Naresh

Re: [PATCH RESEND v6 1/5] hwmon: (pmbus/core): Add interrupt support

[Guenter Roeck]

On Tue, Jan 03, 2023 at 08:56:59PM +0530, Naresh Solanki wrote:
> Hi Guenter
> 
> On 03-01-2023 05:56 pm, Guenter Roeck wrote:
> > On Tue, Jan 03, 2023 at 12:18:49PM +0530, Naresh Solanki wrote:
> > > Hi Guenter,
> > > 
> > > On 29-12-2022 08:10 pm, Guenter Roeck wrote:
> > > > On Wed, Dec 14, 2022 at 09:07:11AM +0100, Naresh Solanki wrote:
> > > > > From: Patrick Rudolph <patrick.rudolph@9elements.com>
> > > > > 
> > > > > Implement PMBUS irq handler.
> > > > > 
> > > > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> > > > > Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> > > > 
> > > > $ scripts/checkpatch.pl --strict index.html
> > > > CHECK: Blank lines aren't necessary after an open brace '{'
> > > > #131: FILE: drivers/hwmon/pmbus/pmbus_core.c:3088:
> > > > +	for (i = 0; i < data->info->pages; i++) {
> > > > +
> > > > 
> > > > CHECK: Alignment should match open parenthesis
> > > > #183: FILE: drivers/hwmon/pmbus/pmbus_core.c:3140:
> > > > +	ret = devm_request_threaded_irq(dev, client->irq, NULL, pmbus_fault_handler,
> > > > +			      0, "pmbus-irq", data);
> > > > 
> > > > CHECK: Please use a blank line after function/struct/union/enum declarations
> > > > #197: FILE: drivers/hwmon/pmbus/pmbus_core.c:3154:
> > > >    }
> > > > +static int pmbus_irq_setup(struct i2c_client *client, struct pmbus_data *data)
> > > > 
> > > > total: 0 errors, 0 warnings, 3 checks, 109 lines checked
> > > > 
> > > > NOTE: For some of the reported defects, checkpatch may be able to
> > > >         mechanically convert to the typical style using --fix or --fix-inplace.
> > > > 
> > > > index.html has style problems, please review.
> > > > 
> > > > Please run checkpatch --strict on your patches.
> > > > Also see Documentation/hwmon/submitting-patches.rst.
> > > I will take care of these errors in the updated version.
> > > > 
> > > > > ---
> > > > >    drivers/hwmon/pmbus/pmbus.h      |  2 +-
> > > > >    drivers/hwmon/pmbus/pmbus_core.c | 84 ++++++++++++++++++++++++++++++++
> > > > >    2 files changed, 85 insertions(+), 1 deletion(-)
> > > > > 
> > > > > 
> > > > > base-commit: 364ffd2537c44cb6914ff5669153f4a86fffad29
> > > > > 
> > > > > 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 95e95783972a..244fd2597252 100644
> > > > > --- a/drivers/hwmon/pmbus/pmbus_core.c
> > > > > +++ b/drivers/hwmon/pmbus/pmbus_core.c
> > > > > @@ -3072,11 +3072,89 @@ static int pmbus_regulator_register(struct pmbus_data *data)
> > > > >    	return 0;
> > > > >    }
> > > > > +
> > > > > +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, status;
> > > > > +
> > > > > +	for (i = 0; i < data->info->pages; i++) {
> > > > > +
> > > > > +		mutex_lock(&data->update_lock);
> > > > > +		status = pmbus_read_status_word(client, i);
> > > > > +		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, i);
> > > > > +
> > > > > +		mutex_unlock(&data->update_lock);
> > > > > +	}
> > > > > +
> > > > > +	return IRQ_HANDLED;
> > > > > +}
> > > > > +
> > > > > +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, func;
> > > > > +	u8 mask;
> > > > > +
> > > > > +	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[j];
> > > > > +			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);
> > > > 
> > > > This concerns me. It might mean that the chip does not support
> > > > PMBUS_SMBALERT_MASK. If so, there would be lots of error messages.
> > > After going through the PMBus specification, it appears that this should not
> > > be an issue unless there is a violation of the specification.
> > 
> > PMBus chips have lots of issues which violate the specification.
> > Have a look at the various drivers and the workarounds implemented there.
> > You'll need to check if the command/register is supported before using it.
> > Also, if you want to keep the error message, make it dev_err_once().
> > 
> > Either case, an error is an error, not to be ignored. An error here
> > should result in an error abort.
> Yes, I agree that PMBus chips can have issues that violate the
> specification, and that it is important to check whether a command or
> register is supported before using it.
> I have noticed that many drivers use the PMBUS_HAVE_* flags to expose the
> presence of specific registers, and I think it would be a good idea to add a
> PMBUS_HAVE_SMBALERT flag as well, so that drivers for supported chips can
> use it to determine whether they should set up an IRQ handler or not. If
> PMBUS_HAVE_SMBALERT is set, then the IRQ handler should be set up, otherwise
> it should be ignored.
> Will this approach be right?

Not really. PMBUS_HAVE_ flags are intended to indicate sensor register
support, not to indicate compliance problems. What you'd be looking for
would be the flags in struct pmbus_platform_data. However, those are only
intended to be used if registers/commands can not be auto-detected or if
doing so causes problems. See include/linux/pmbus.h for details.
Unless there is reason to believe that chips are misbehaving when trying
to read from or to set PMBUS_SMBALERT_MASK, we should stick with auto-
detection. After all, that is what pmbus_check_{status,byte,word}_register()
functions are for.

Thanks,
Guenter