Message-ID: <ndkdgw6tiau4y7psfl53tmzylrfi27h6j5likx5mahufv34625@na3yyn56fgw6>
Date: Fri, 6 Feb 2026 07:24:00 -0600
From: Bjorn Andersson <andersson@...nel.org>
To: Manaf Meethalavalappu Pallikunhi <manaf.pallikunhi@....qualcomm.com>
Cc: Guenter Roeck <linux@...ck-us.net>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Konrad Dybcio <konradybcio@...nel.org>, amit.kucheria@....qualcomm.com, 
	Daniel Lezcano <daniel.lezcano@...aro.org>, Gaurav Kohli <gaurav.kohli@....qualcomm.com>, 
	linux-hwmon@...r.kernel.org, linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/4] hwmon: Add Qualcomm PMIC BCL hardware monitor driver

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.

> + */
> +
[..]
> +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.

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

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

> +	}, {
> +		.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@....qualcomm.com>");
> +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.

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

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

[..]
> +/**
> + * 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.

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

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

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

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

Regards,
Bjorn

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

Powered by blists - more mailing lists