Re: [PATCH 2/4] hwmon: Add Qualcomm PMIC BCL hardware monitor driver

From: Manaf Meethalavalappu Pallikunhi

Date: Fri Feb 13 2026 - 06:38:21 EST

Hi Bjorn,

On 2/6/2026 6:54 PM, Bjorn Andersson wrote:
On Fri, Feb 06, 2026 at 02:44:06AM +0530, Manaf Meethalavalappu Pallikunhi wrote:
diff --git a/drivers/hwmon/qcom-bcl-hwmon.c b/drivers/hwmon/qcom-bcl-hwmon.c
new file mode 100644
index 000000000000..a7e3b865de5c
--- /dev/null
+++ b/drivers/hwmon/qcom-bcl-hwmon.c
@@ -0,0 +1,982 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Qualcomm pmic BCL hardware driver for battery overcurrent and
+ * battery or system under voltage monitor
+ *
+ * Copyright (c) 2026, Qualcomm Innovation Center, Inc. All rights reserved.
That's the wrong statement.
My bad, I copied old copyrights, will update

+ */
+
[..]
+static void bcl_hw_channel_mon_init(struct bcl_device *bcl)
+{
+ bcl->in_mon_enabled = bcl_in_monitor_enabled(bcl);
+ bcl->in_input_enabled = bcl_in_input_enabled(bcl);
+ bcl->curr_mon_enabled = bcl_curr_monitor_enabled(bcl);
+}
+
+static int bcl_alarm_irq_init(struct platform_device *pdev,
+ struct bcl_device *bcl)
+{
+ int ret = 0, irq_num = 0, i = 0;
First use of these three variables are assignments, no need to
zero-initialize them here.

+ struct bcl_alarm_data *alarm;
+
+ for (i = LVL0; i < ALARM_MAX; i++) {
I would prefer ARRAY_SIZE(bcl->bcl_alarms) over ALARM_MAX.
Ack

+ alarm = &bcl->bcl_alarms[i];
+ alarm->type = i;
+ alarm->device = bcl;
+
+ ret = devm_delayed_work_autocancel(bcl->dev, &alarm->alarm_poll_work,
+ bcl_alarm_enable_poll);
+ if (ret)
+ return ret;
+
+ irq_num = platform_get_irq_byname(pdev, bcl_int_names[i]);
+ if (irq_num <= 0)
+ continue;
+
+ ret = devm_request_threaded_irq(&pdev->dev, irq_num, NULL,
+ bcl_handle_alarm, IRQF_ONESHOT,
+ bcl_int_names[i], alarm);
+ if (ret) {
+ dev_err(&pdev->dev, "Error requesting irq(%s).err:%d\n",
+ bcl_int_names[i], ret);
+ return ret;
+ }
+ enable_irq_wake(alarm->irq);
+ alarm->irq_enabled = true;
+ alarm->irq = irq_num;
+ }
+
+ return 0;
+}
+
+static int bcl_regmap_field_init(struct device *dev, struct bcl_device *bcl,
+ const struct bcl_desc *data)
+{
+ int i;
+ struct reg_field fields[F_MAX_FIELDS];
+
+ BUILD_BUG_ON(ARRAY_SIZE(common_reg_fields) != COMMON_FIELD_MAX);
+
+ for (i = 0; i < data->num_reg_fields; i++) {
+ if (i < COMMON_FIELD_MAX)
+ fields[i] = common_reg_fields[i];
+ else
+ fields[i] = data->reg_fields[i];
+
+ /* Need to adjust BCL base from regmap dynamically */
+ fields[i].reg += bcl->base;
+ }
+
+ return devm_regmap_field_bulk_alloc(dev, bcl->regmap, bcl->fields,
+ fields, data->num_reg_fields);
+}
+
+static int bcl_get_device_property_data(struct platform_device *pdev,
+ struct bcl_device *bcl)
+{
+ struct device *dev = &pdev->dev;
+ int ret;
+ u32 reg;
+
+ ret = device_property_read_u32(dev, "reg", &reg);
+ if (ret < 0)
+ return ret;
+
+ bcl->base = reg;
+
+ device_property_read_u32_array(dev, "overcurrent-thresholds-milliamp",
+ bcl->curr_thresholds, 2);
+ return 0;
+}
+
+static int bcl_probe(struct platform_device *pdev)
+{
+ struct bcl_device *bcl;
+ int ret;
+
+ bcl = devm_kzalloc(&pdev->dev, sizeof(*bcl), GFP_KERNEL);
+ if (!bcl)
+ return -ENOMEM;
+
+ bcl->dev = &pdev->dev;
+ bcl->desc = device_get_match_data(&pdev->dev);
+ if (!bcl->desc)
+ return -EINVAL;
+
+ ret = devm_mutex_init(bcl->dev, &bcl->lock);
+ if (ret)
+ return ret;
+
+ bcl->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+ if (!bcl->regmap) {
+ dev_err(&pdev->dev, "Couldn't get parent's regmap\n");
+ return -EINVAL;
+ }
+
+ ret = bcl_get_device_property_data(pdev, bcl);
+ if (ret < 0)
+ return ret;
+
+ ret = bcl_regmap_field_init(bcl->dev, bcl, bcl->desc);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "Unable to allocate regmap fields, err:%d\n", ret);
+ return ret;
+ }
+
+ if (!bcl_hw_is_enabled(bcl))
+ return -ENODEV;
+
+ ret = bcl_curr_thresh_update(bcl);
+ if (ret < 0)
+ return ret;
+
+ ret = bcl_alarm_irq_init(pdev, bcl);
+ if (ret < 0)
+ return ret;
+
+ bcl_hw_channel_mon_init(bcl);
+
+ dev_set_drvdata(&pdev->dev, bcl);
+
+ bcl->hwmon_name = devm_hwmon_sanitize_name(&pdev->dev,
+ dev_name(bcl->dev));
+ if (IS_ERR(bcl->hwmon_name)) {
+ dev_err(&pdev->dev, "Failed to sanitize hwmon name\n");
Afaict, devm_hwmon_sanitize_name() can only return -ENOMEM, which
already printed an error.
Ack

+ return PTR_ERR(bcl->hwmon_name);
+ }
+
+ bcl->hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev,
+ bcl->hwmon_name,
+ bcl,
+ &bcl_hwmon_chip_info,
+ NULL);
+ if (IS_ERR(bcl->hwmon_dev)) {
+ dev_err(&pdev->dev, "Failed to register hwmon device: %ld\n",
+ PTR_ERR(bcl->hwmon_dev));
+ return PTR_ERR(bcl->hwmon_dev);
+ }
+
+ dev_dbg(&pdev->dev, "BCL hwmon device with version: %u.%u registered\n",
+ bcl_get_version_major(bcl), bcl_get_version_minor(bcl));
+
+ return 0;
+}
+
+static const struct of_device_id bcl_match[] = {
+ {
+ .compatible = "qcom,bcl-v1",
+ .data = &pm7250b_data,
Why generic compatibles but pmic-specific data structures? If anything
I'd expect tthe other way around...
Ack, what I thought is, there can be multiple pmics which share same data structure in same generation. In that case no need to add extra binding
config,  Just use generic compatibles. By reading  all these comments,  it shouldn't be like this. I will remove generic compatibles.

+ }, {
+ .compatible = "qcom,bcl-v2",
+ .data = &pm8350_data,
+ }, {
+ .compatible = "qcom,bcl-v3-bmx",
+ .data = &pm8550b_data,
+ }, {
+ .compatible = "qcom,bcl-v3-wb",
+ .data = &pmw5100_data,
+ }, {
+ .compatible = "qcom,bcl-v3-core",
+ .data = &pm8550_data,
+ }, {
+ .compatible = "qcom,bcl-v4-bmx",
+ .data = &pmih010_data,
+ }, {
+ .compatible = "qcom,bcl-v4-wb",
+ .data = &pmw6100_data,
+ }, {
+ .compatible = "qcom,bcl-v4-core",
+ .data = &pmh010_data,
+ }, {
+ .compatible = "qcom,bcl-v4-pmv010",
+ .data = &pmv010_data,
+ },
+ { }
+};
+MODULE_DEVICE_TABLE(of, bcl_match);
+
+static struct platform_driver bcl_driver = {
+ .probe = bcl_probe,
+ .driver = {
+ .name = BCL_DRIVER_NAME,
+ .of_match_table = bcl_match,
+ },
+};
+
+MODULE_AUTHOR("Manaf Meethalavalappu Pallikunhi <manaf.pallikunhi@xxxxxxxxxxxxxxxx>");
+MODULE_DESCRIPTION("QCOM BCL HWMON driver");
+module_platform_driver(bcl_driver);
This relates to the bcl_driver declaration, not module properties. So
move it there.
Ack

+MODULE_LICENSE("GPL");
diff --git a/drivers/hwmon/qcom-bcl-hwmon.h b/drivers/hwmon/qcom-bcl-hwmon.h
Why is there a header file, is this going to be accessed by some other
driver? It mostly contain driver-internal thing, and some helpers that
won't be useful outside of the driver.
Ack, will move all changes in to driver file

new file mode 100644
index 000000000000..28a7154d9486
--- /dev/null
+++ b/drivers/hwmon/qcom-bcl-hwmon.h
@@ -0,0 +1,311 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2026, Qualcomm Innovation Center, Inc. All rights reserved.
Please fix this one as well. (Or...drop the file)

+ */
+
+#ifndef __QCOM_BCL_HWMON_H__
+#define __QCOM_BCL_HWMON_H__
+
+#define BCL_DRIVER_NAME "qcom-bcl-hwmon"
This belong in the driver...but frankly, you can just put the string
directly in bcl_driver.driver.name, no need to have a define for it...
Ack

[..]
+/**
+ * bcl_field_enabled - Generic helper to check if a regmap field is enabled
+ * @bcl: BCL device structure
+ * @field: Index in bcl->fields[]
+ *
+ * Return: true if field is non-zero, false otherwise
+ */
+static inline bool bcl_field_enabled(const struct bcl_device *bcl, enum bcl_fields id)
+{
+ int ret;
+ u32 val = 0;
+
+ ret = regmap_field_read(bcl->fields[id], &val);
+ if (ret)
+ return false;
+
+ return !!val;
+}
+
+#define bcl_in_input_enabled(bcl) bcl_field_enabled(bcl, F_IN_INPUT_EN)
+#define bcl_curr_monitor_enabled(bcl) bcl_field_enabled(bcl, F_CURR_MON_EN)
+#define bcl_in_monitor_enabled(bcl) bcl_field_enabled(bcl, F_IN_MON_EN)
+#define bcl_hw_is_enabled(bcl) bcl_field_enabled(bcl, F_CTL_EN)
This whole thing is only used in bcl_hw_channel_mon_init(), just put the
code in bcl_hw_channel_mon_init().


You have a few other regmap_field_*() calls in the driver, I would
suggest that you just call that directly for these cases as well.
Ack

+
+/**
+ * bcl_enable_irq - Generic helper to enable alarm irq
+ * @alarm: BCL level alarm data
+ */
+static inline void bcl_enable_irq(struct bcl_alarm_data *alarm)
+{
+ if (alarm->irq_enabled)
+ return;
This can't happen, but because you separated this to a helper function
it's not obvious
Agree, will update in v2

I'd suggest that you inline the remaining 3 lines in the one place where
this function is called.
Ack

+ alarm->irq_enabled = true;
+ enable_irq(alarm->irq);
+ enable_irq_wake(alarm->irq);
+}
+
+/**
+ * bcl_disable_irq - Generic helper to disable alarm irq
+ * @alarm: BCL level alarm data
+ */
+static inline void bcl_disable_irq(struct bcl_alarm_data *alarm)
+{
+ if (!alarm->irq_enabled)
+ return;
This is tricker, because there's a window between
devm_request_threaded_irq() and the assignment of irq_enabled, where the
interrupt function might execute and the attempt to bcl_disable_irq()
will face irq_enabled == 0.

But I don't think that's intentional...
got it, I will synchronize this with per alarm lock in next patch

I think this too would be better to just inline in the one place its'
being called.

Ack

Thanks,
Manaf

Regards,
Bjorn

+ alarm->irq_enabled = false;
+ disable_irq_nosync(alarm->irq);
+ disable_irq_wake(alarm->irq);
+}
+
+#endif /* __QCOM_BCL_HWMON_H__ */

--
2.43.0