[PATCH 1/4] hwmon: (pmbus/core): Generalize pmbus status flag map

[PATCH 1/4] hwmon: (pmbus/core): Generalize pmbus status flag map

[Naresh Solanki]

The PMBus status flag map(pmbus_regulator_status_flag_map) is moved
outside of the regulator #if block and the associated variable/struct
name updated to reflect as generic PMBus status.

This will make the PMBus status flag map more versatile and easier to
incorporate into different contexts and functions.

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

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 95e95783972a..1b70cf3be313 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -2692,6 +2692,49 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
 	return 0;
 }
 
+/* A PMBus status flag and the corresponding REGULATOR_ERROR_* flag */
+struct pmbus_status_assoc {
+	int pflag, rflag;
+};
+
+/* PMBus->regulator bit mappings for a PMBus status register */
+struct pmbus_status_category {
+	int func;
+	int reg;
+	const struct pmbus_status_assoc *bits; /* zero-terminated */
+};
+
+static const struct pmbus_status_category __maybe_unused pmbus_status_flag_map[] = {
+	{
+		.func = PMBUS_HAVE_STATUS_VOUT,
+		.reg = PMBUS_STATUS_VOUT,
+		.bits = (const struct pmbus_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 },
+			{ },
+		},
+	}, {
+		.func = PMBUS_HAVE_STATUS_IOUT,
+		.reg = PMBUS_STATUS_IOUT,
+		.bits = (const struct pmbus_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 },
+			{ },
+		},
+	}, {
+		.func = PMBUS_HAVE_STATUS_TEMP,
+		.reg = PMBUS_STATUS_TEMPERATURE,
+		.bits = (const struct pmbus_status_assoc[]) {
+			{ PB_TEMP_OT_WARNING,    REGULATOR_ERROR_OVER_TEMP_WARN },
+			{ PB_TEMP_OT_FAULT,      REGULATOR_ERROR_OVER_TEMP },
+			{ },
+		},
+	},
+};
+
 #if IS_ENABLED(CONFIG_REGULATOR)
 static int pmbus_regulator_is_enabled(struct regulator_dev *rdev)
 {
@@ -2738,54 +2781,11 @@ 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 */
-struct pmbus_regulator_status_assoc {
-	int pflag, rflag;
-};
-
-/* PMBus->regulator bit mappings for a PMBus status register */
-struct pmbus_regulator_status_category {
-	int func;
-	int reg;
-	const struct pmbus_regulator_status_assoc *bits; /* zero-terminated */
-};
-
-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 },
-			{ },
-		},
-	}, {
-		.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 },
-			{ },
-		},
-	}, {
-		.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 },
-			{ },
-		},
-	},
-};
-
 static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned int *flags)
 {
 	int i, status;
-	const struct pmbus_regulator_status_category *cat;
-	const struct pmbus_regulator_status_assoc *bit;
+	const struct pmbus_status_category *cat;
+	const struct pmbus_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);
@@ -2796,8 +2796,8 @@ static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned
 
 	mutex_lock(&data->update_lock);
 
-	for (i = 0; i < ARRAY_SIZE(pmbus_regulator_flag_map); i++) {
-		cat = &pmbus_regulator_flag_map[i];
+	for (i = 0; i < ARRAY_SIZE(pmbus_status_flag_map); i++) {
+		cat = &pmbus_status_flag_map[i];
 		if (!(func & cat->func))
 			continue;
 

base-commit: 4d891f76a30d3be4194a805c5e4277786140ef05
-- 
2.38.1

[PATCH 2/4] hwmon: (pmbus/core) Generalise pmbus get status

[Naresh Solanki]

Add function pmbus get status that can be used to get both pmbus
specific status & regulator status

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

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 1b70cf3be313..12b662b91306 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -2735,64 +2735,16 @@ static const struct pmbus_status_category __maybe_unused pmbus_status_flag_map[]
 	},
 };
 
-#if IS_ENABLED(CONFIG_REGULATOR)
-static int pmbus_regulator_is_enabled(struct regulator_dev *rdev)
-{
-	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);
-	int ret;
 
-	mutex_lock(&data->update_lock);
-	ret = _pmbus_read_byte_data(client, page, PMBUS_OPERATION);
-	mutex_unlock(&data->update_lock);
-
-	if (ret < 0)
-		return ret;
-
-	return !!(ret & PB_OPERATION_CONTROL_ON);
-}
-
-static int _pmbus_regulator_on_off(struct regulator_dev *rdev, bool enable)
+static int pmbus_get_flags(struct pmbus_data *data, u8 page, unsigned int *error)
 {
-	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);
-	int ret;
-
-	mutex_lock(&data->update_lock);
-	ret = pmbus_update_byte_data(client, page, PMBUS_OPERATION,
-				     PB_OPERATION_CONTROL_ON,
-				     enable ? PB_OPERATION_CONTROL_ON : 0);
-	mutex_unlock(&data->update_lock);
-
-	return ret;
-}
-
-static int pmbus_regulator_enable(struct regulator_dev *rdev)
-{
-	return _pmbus_regulator_on_off(rdev, 1);
-}
-
-static int pmbus_regulator_disable(struct regulator_dev *rdev)
-{
-	return _pmbus_regulator_on_off(rdev, 0);
-}
-
-static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned int *flags)
-{
-	int i, status;
 	const struct pmbus_status_category *cat;
 	const struct pmbus_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 +2755,15 @@ 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;
 		}
+
 	}
 
 	/*
@@ -2823,36 +2776,99 @@ 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;
+}
+
+#if IS_ENABLED(CONFIG_REGULATOR)
+static int pmbus_regulator_is_enabled(struct regulator_dev *rdev)
+{
+	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);
+	int ret;
+
+	mutex_lock(&data->update_lock);
+	ret = _pmbus_read_byte_data(client, page, PMBUS_OPERATION);
+	mutex_unlock(&data->update_lock);
+
+	if (ret < 0)
+		return ret;
+
+	return !!(ret & PB_OPERATION_CONTROL_ON);
+}
+
+static int _pmbus_regulator_on_off(struct regulator_dev *rdev, bool enable)
+{
+	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);
+	int ret;
+
+	mutex_lock(&data->update_lock);
+	ret = pmbus_update_byte_data(client, page, PMBUS_OPERATION,
+				     PB_OPERATION_CONTROL_ON,
+				     enable ? PB_OPERATION_CONTROL_ON : 0);
+	mutex_unlock(&data->update_lock);
+
+	return ret;
+}
+
+static int pmbus_regulator_enable(struct regulator_dev *rdev)
+{
+	return _pmbus_regulator_on_off(rdev, 1);
+}
+
+static int pmbus_regulator_disable(struct regulator_dev *rdev)
+{
+	return _pmbus_regulator_on_off(rdev, 0);
+}
+
+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);
 }
 
 static int pmbus_regulator_get_status(struct regulator_dev *rdev)
-- 
2.38.1

[PATCH 3/4] 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 | 85 ++++++++++++++++++++++++++++++++
 2 files changed, 86 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index 713ea7915425..11e84e141126 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 12b662b91306..3c1c68e93943 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -3095,6 +3095,85 @@ static int pmbus_regulator_register(struct pmbus_data *data)
 }
 #endif
 
+static int pmbus_write_smbalert_mask(struct i2c_client *client, u8 page, u8 reg, u8 val)
+{
+	int err;
+
+	err = pmbus_check_word_register(client, page, reg | (val << 8));
+	if (err)
+		return err;
+
+	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;
+
+	mutex_lock(&data->update_lock);
+	for (i = 0; i < data->info->pages; i++) {
+		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_status_category *cat;
+	const struct pmbus_status_assoc *bit;
+	int i, j, err, ret, func;
+	u8 mask;
+	u8 misc_status[] = {PMBUS_STATUS_CML, PMBUS_STATUS_OTHER, PMBUS_STATUS_MFR_SPECIFIC,
+			    PMBUS_STATUS_FAN_12, PMBUS_STATUS_FAN_34};
+
+	for (i = 0; i < data->info->pages; i++) {
+		func = data->info->func[i];
+
+		for (j = 0; j < ARRAY_SIZE(pmbus_status_flag_map); j++) {
+			cat = &pmbus_status_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_once(dev, "Failed to set smbalert for reg 0x%02x\n",
+					     cat->reg);
+		}
+
+		for (j = 0; j < ARRAY_SIZE(misc_status); j++) {
+			err = pmbus_write_smbalert_mask(client, i, misc_status[j], 0xff);
+			if (err)
+				dev_err_once(dev, "Failed to set smbalert for reg 0x%02x\n",
+					     misc_status[j]);
+		}
+	}
+
+	/* 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;
+}
+
 static struct dentry *pmbus_debugfs_dir;	/* pmbus debugfs directory */
 
 #if IS_ENABLED(CONFIG_DEBUG_FS)
@@ -3457,6 +3536,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.38.1

[PATCH 4/4] 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 | 45 ++++++++++++++++++++++++++++----
 1 file changed, 40 insertions(+), 5 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 3c1c68e93943..cbea92d3a1e6 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -2735,8 +2735,35 @@ static const struct pmbus_status_category __maybe_unused pmbus_status_flag_map[]
 	},
 };
 
+#define to_dev_attr(_dev_attr) \
+	container_of(_dev_attr, struct device_attribute, attr)
 
-static int pmbus_get_flags(struct pmbus_data *data, u8 page, unsigned int *error)
+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)
 {
 	const struct pmbus_status_category *cat;
 	const struct pmbus_status_assoc *bit;
@@ -2764,6 +2791,8 @@ static int pmbus_get_flags(struct pmbus_data *data, u8 page, unsigned int *error
 				*error |= bit->rflag;
 		}
 
+		if (notify && status)
+			pmbus_notify(data, page, cat->reg, status);
 	}
 
 	/*
@@ -2868,7 +2897,7 @@ static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned
 	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);
+	return pmbus_get_flags(data, rdev_get_id(rdev), flags, false);
 }
 
 static int pmbus_regulator_get_status(struct regulator_dev *rdev)
@@ -3110,10 +3139,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;
 
-	mutex_lock(&data->update_lock);
 	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) {
 			mutex_unlock(&data->update_lock);
@@ -3122,8 +3155,10 @@ static irqreturn_t pmbus_fault_handler(int irq, void *pdata)
 
 		if (status & ~(PB_STATUS_OFF | PB_STATUS_BUSY | PB_STATUS_POWER_GOOD_N))
 			pmbus_clear_fault_page(client, i);
+
+		mutex_unlock(&data->update_lock);
 	}
-	mutex_unlock(&data->update_lock);
+
 
 	return IRQ_HANDLED;
 }
-- 
2.38.1

Re: [PATCH 2/4] hwmon: (pmbus/core) Generalise pmbus get status

[Guenter Roeck]

On Mon, Jan 23, 2023 at 07:40:19AM +0100, Naresh Solanki wrote:
> Add function pmbus get status that can be used to get both pmbus
> specific status & regulator status
> 
> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> ---
>  drivers/hwmon/pmbus/pmbus_core.c | 148 +++++++++++++++++--------------
>  1 file changed, 82 insertions(+), 66 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 1b70cf3be313..12b662b91306 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -2735,64 +2735,16 @@ static const struct pmbus_status_category __maybe_unused pmbus_status_flag_map[]
>  	},
>  };
>  
> -#if IS_ENABLED(CONFIG_REGULATOR)
> -static int pmbus_regulator_is_enabled(struct regulator_dev *rdev)
> -{
> -	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);
> -	int ret;
>  
> -	mutex_lock(&data->update_lock);
> -	ret = _pmbus_read_byte_data(client, page, PMBUS_OPERATION);
> -	mutex_unlock(&data->update_lock);
> -
> -	if (ret < 0)
> -		return ret;
> -
> -	return !!(ret & PB_OPERATION_CONTROL_ON);
> -}
> -
> -static int _pmbus_regulator_on_off(struct regulator_dev *rdev, bool enable)
> +static int pmbus_get_flags(struct pmbus_data *data, u8 page, unsigned int *error)

Moving this code outside the conditional without usage there will result
in compile errors due to an unused function if compiled with REGULATOR
disabled. While this will (hopefully) change in one of the later patches,
it is still unacceptable because it may result in bisect failures.

>  {
> -	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);
> -	int ret;
> -
> -	mutex_lock(&data->update_lock);
> -	ret = pmbus_update_byte_data(client, page, PMBUS_OPERATION,
> -				     PB_OPERATION_CONTROL_ON,
> -				     enable ? PB_OPERATION_CONTROL_ON : 0);
> -	mutex_unlock(&data->update_lock);
> -
> -	return ret;
> -}
> -
> -static int pmbus_regulator_enable(struct regulator_dev *rdev)
> -{
> -	return _pmbus_regulator_on_off(rdev, 1);
> -}
> -
> -static int pmbus_regulator_disable(struct regulator_dev *rdev)
> -{
> -	return _pmbus_regulator_on_off(rdev, 0);
> -}
> -
> -static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned int *flags)
> -{
> -	int i, status;
>  	const struct pmbus_status_category *cat;
>  	const struct pmbus_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;

You are making personal preference changes here. Maybe that is the reason
why the patch looks that large. Please try to leave existing code alone.
If there is a reason to change a variable name (or other cosmetic changes,
like moving variable declarations around), do it in a separate patch which
only does that, and explain why it is needed and/or makes sense.

Thanks,
Guenter

>  
>  	mutex_lock(&data->update_lock);
>  
> @@ -2803,14 +2755,15 @@ 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;
>  		}
> +
>  	}
>  
>  	/*
> @@ -2823,36 +2776,99 @@ 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;
> +}
> +
> +#if IS_ENABLED(CONFIG_REGULATOR)
> +static int pmbus_regulator_is_enabled(struct regulator_dev *rdev)
> +{
> +	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);
> +	int ret;
> +
> +	mutex_lock(&data->update_lock);
> +	ret = _pmbus_read_byte_data(client, page, PMBUS_OPERATION);
> +	mutex_unlock(&data->update_lock);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	return !!(ret & PB_OPERATION_CONTROL_ON);
> +}
> +
> +static int _pmbus_regulator_on_off(struct regulator_dev *rdev, bool enable)
> +{
> +	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);
> +	int ret;
> +
> +	mutex_lock(&data->update_lock);
> +	ret = pmbus_update_byte_data(client, page, PMBUS_OPERATION,
> +				     PB_OPERATION_CONTROL_ON,
> +				     enable ? PB_OPERATION_CONTROL_ON : 0);
> +	mutex_unlock(&data->update_lock);
> +
> +	return ret;
> +}
> +
> +static int pmbus_regulator_enable(struct regulator_dev *rdev)
> +{
> +	return _pmbus_regulator_on_off(rdev, 1);
> +}
> +
> +static int pmbus_regulator_disable(struct regulator_dev *rdev)
> +{
> +	return _pmbus_regulator_on_off(rdev, 0);
> +}
> +
> +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);
>  }
>  
>  static int pmbus_regulator_get_status(struct regulator_dev *rdev)
> -- 
> 2.38.1
> 

Re: [PATCH 2/4] hwmon: (pmbus/core) Generalise pmbus get status

[Naresh Solanki]

Hi

On 24-01-2023 11:18 pm, Guenter Roeck wrote:
> On Mon, Jan 23, 2023 at 07:40:19AM +0100, Naresh Solanki wrote:
>> Add function pmbus get status that can be used to get both pmbus
>> specific status & regulator status
>>
>> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
>> ---
>>   drivers/hwmon/pmbus/pmbus_core.c | 148 +++++++++++++++++--------------
>>   1 file changed, 82 insertions(+), 66 deletions(-)
>>
>> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
>> index 1b70cf3be313..12b662b91306 100644
>> --- a/drivers/hwmon/pmbus/pmbus_core.c
>> +++ b/drivers/hwmon/pmbus/pmbus_core.c
>> @@ -2735,64 +2735,16 @@ static const struct pmbus_status_category __maybe_unused pmbus_status_flag_map[]
>>   	},
>>   };
>>   
>> -#if IS_ENABLED(CONFIG_REGULATOR)
>> -static int pmbus_regulator_is_enabled(struct regulator_dev *rdev)
>> -{
>> -	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);
>> -	int ret;
>>   
>> -	mutex_lock(&data->update_lock);
>> -	ret = _pmbus_read_byte_data(client, page, PMBUS_OPERATION);
>> -	mutex_unlock(&data->update_lock);
>> -
>> -	if (ret < 0)
>> -		return ret;
>> -
>> -	return !!(ret & PB_OPERATION_CONTROL_ON);
>> -}
>> -
>> -static int _pmbus_regulator_on_off(struct regulator_dev *rdev, bool enable)
>> +static int pmbus_get_flags(struct pmbus_data *data, u8 page, unsigned int *error)
> 
> Moving this code outside the conditional without usage there will result
> in compile errors due to an unused function if compiled with REGULATOR
> disabled. While this will (hopefully) change in one of the later patches,
> it is still unacceptable because it may result in bisect failures.
> 
Yes. With REGULATOR disabled. There is warning:
warning: ‘pmbus_get_flags’ defined but not used [-Wunused-function]
  2739 | static int pmbus_get_flags(struct pmbus_data *data, u8 page, 
unsigned int *error)

Will add maybe unused as below:
static int __maybe_unused pmbus_get_flags(struct pmbus_data *data, u8 
page, unsigned int *error)

Let me know your view.

>>   {
>> -	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);
>> -	int ret;
>> -
>> -	mutex_lock(&data->update_lock);
>> -	ret = pmbus_update_byte_data(client, page, PMBUS_OPERATION,
>> -				     PB_OPERATION_CONTROL_ON,
>> -				     enable ? PB_OPERATION_CONTROL_ON : 0);
>> -	mutex_unlock(&data->update_lock);
>> -
>> -	return ret;
>> -}
>> -
>> -static int pmbus_regulator_enable(struct regulator_dev *rdev)
>> -{
>> -	return _pmbus_regulator_on_off(rdev, 1);
>> -}
>> -
>> -static int pmbus_regulator_disable(struct regulator_dev *rdev)
>> -{
>> -	return _pmbus_regulator_on_off(rdev, 0);
>> -}
>> -
>> -static int pmbus_regulator_get_error_flags(struct regulator_dev *rdev, unsigned int *flags)
>> -{
>> -	int i, status;
>>   	const struct pmbus_status_category *cat;
>>   	const struct pmbus_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;
> 
> You are making personal preference changes here. Maybe that is the reason
> why the patch looks that large. Please try to leave existing code alone.
> If there is a reason to change a variable name (or other cosmetic changes,
> like moving variable declarations around), do it in a separate patch which
> only does that, and explain why it is needed and/or makes sense.
Sure. Will do.

Best Regards,
Naresh
> 
> Thanks,
> Guenter
> 
>>   
>>   	mutex_lock(&data->update_lock);
>>   
>> @@ -2803,14 +2755,15 @@ 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;
>>   		}
>> +
>>   	}
>>   
>>   	/*
>> @@ -2823,36 +2776,99 @@ 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;
>> +}
>> +
>> +#if IS_ENABLED(CONFIG_REGULATOR)
>> +static int pmbus_regulator_is_enabled(struct regulator_dev *rdev)
>> +{
>> +	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);
>> +	int ret;
>> +
>> +	mutex_lock(&data->update_lock);
>> +	ret = _pmbus_read_byte_data(client, page, PMBUS_OPERATION);
>> +	mutex_unlock(&data->update_lock);
>> +
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return !!(ret & PB_OPERATION_CONTROL_ON);
>> +}
>> +
>> +static int _pmbus_regulator_on_off(struct regulator_dev *rdev, bool enable)
>> +{
>> +	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);
>> +	int ret;
>> +
>> +	mutex_lock(&data->update_lock);
>> +	ret = pmbus_update_byte_data(client, page, PMBUS_OPERATION,
>> +				     PB_OPERATION_CONTROL_ON,
>> +				     enable ? PB_OPERATION_CONTROL_ON : 0);
>> +	mutex_unlock(&data->update_lock);
>> +
>> +	return ret;
>> +}
>> +
>> +static int pmbus_regulator_enable(struct regulator_dev *rdev)
>> +{
>> +	return _pmbus_regulator_on_off(rdev, 1);
>> +}
>> +
>> +static int pmbus_regulator_disable(struct regulator_dev *rdev)
>> +{
>> +	return _pmbus_regulator_on_off(rdev, 0);
>> +}
>> +
>> +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);
>>   }
>>   
>>   static int pmbus_regulator_get_status(struct regulator_dev *rdev)
>> -- 
>> 2.38.1
>>