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

[Daniel Lezcano <daniel.lezcano@oss.qualcomm.com>]

Hi Manaf,

On Fri, Feb 06, 2026 at 02:44:06AM +0530, Manaf Meethalavalappu Pallikunhi wrote:
> Add support for Qualcomm PMIC Battery Current Limiting (BCL) hardware
> monitor driver. The BCL peripheral is present in Qualcomm PMICs and
> provides real-time monitoring and protection against battery
> overcurrent and under voltage conditions.
> 
> The driver monitors:
> - Battery voltage with configurable low voltage thresholds
> - Battery current with configurable high current thresholds
> - Two limit alarm interrupts (max/min, critical)

Can you describe the behavior of the alarm ?

I assume the alarm is raised when a threshold is crossed from normal
to anormal condition leading to a hwmon event.

 * Does the BCL trigger an interrupt when going back to the normal condition ?

 * When is the alarm flag reset ?

 * Can we have a flood of events if the current / voltage is wavering
   around the thresholds ?

Overall, the driver is too big, so hard to review. It is better to
provide a simplified version with one version supported.

> The driver integrates with the Linux hwmon subsystem and provides
> standard hwmon attributes for monitoring battery conditions.
> 
> Signed-off-by: Manaf Meethalavalappu Pallikunhi <manaf.pallikunhi@oss.qualcomm.com>
> ---
>  MAINTAINERS                    |   9 +
>  drivers/hwmon/Kconfig          |   9 +
>  drivers/hwmon/Makefile         |   1 +
>  drivers/hwmon/qcom-bcl-hwmon.c | 982 +++++++++++++++++++++++++++++++++++++++++
>  drivers/hwmon/qcom-bcl-hwmon.h | 311 +++++++++++++
>  5 files changed, 1312 insertions(+)

[ ... ]

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

Old copyright format

> +#include <linux/devm-helpers.h>
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/workqueue.h>
> +
> +#include "qcom-bcl-hwmon.h"
> +
> +ADD_BCL_HWMON_ALARM_MAPS(in, min, lcrit);
> +ADD_BCL_HWMON_ALARM_MAPS(curr, max, crit);
> +
> +/* Interrupt names for each alarm level */
> +static const char * const bcl_int_names[ALARM_MAX] = {
> +	[LVL0] = "bcl-max-min",
> +	[LVL1] = "bcl-critical",
> +};

IIUC there are three levels of alarms but the hwmon only has max/min
and critical. Would it make sense to do adaptative min / max ? So when
LVL0 is reached, update min / max with LVL1 value, LVL2 is critical
and does not change ?

> +static const char * const bcl_channel_label[CHANNEL_MAX] = {
> +	"BCL Voltage",
> +	"BCL Current",
> +};

s/[CHANNEL_MAX]/[]/

> +/* Common Reg Fields */
> +static const struct reg_field common_reg_fields[COMMON_FIELD_MAX] = {
> +	[F_V_MAJOR]	= REG_FIELD(REVISION2, 0, 7),
> +	[F_V_MINOR]	= REG_FIELD(REVISION1, 0, 7),
> +	[F_CTL_EN]	= REG_FIELD(EN_CTL1, 7, 7),
> +	[F_LVL0_ALARM]	= REG_FIELD(STATUS, 0, 0),
> +	[F_LVL1_ALARM]	= REG_FIELD(STATUS, 1, 1),
> +};
> +
> +/* BCL Version/Modes specific fields */
> +static const struct reg_field bcl_v1_reg_fields[] = {
> +	[F_IN_MON_EN]	= REG_FIELD(MODE_CTL1, 0, 2),
> +	[F_IN_L0_THR]	= REG_FIELD(VADC_L0_THR, 0, 7),
> +	[F_IN_L1_THR]	= REG_FIELD(VCMP_L1_THR, 0, 5),
> +	[F_IN_INPUT_EN]	= REG_FIELD(VADC_CONV_REQ, 0, 0),
> +	[F_IN_INPUT]	= REG_FIELD(VADC_DATA1, 0, 7),
> +	[F_CURR_MON_EN]	= REG_FIELD(IADC_CONV_REQ, 0, 0),
> +	[F_CURR_H0_THR]	= REG_FIELD(IADC_H0_THR, 0, 7),
> +	[F_CURR_H1_THR]	= REG_FIELD(IADC_H1_THR, 0, 7),
> +	[F_CURR_INPUT]	= REG_FIELD(IADC_DATA1, 0, 7),
> +};
> +
> +static const struct reg_field bcl_v2_reg_fields[] = {
> +	[F_IN_MON_EN]	= REG_FIELD(VCMP_CTL, 0, 1),
> +	[F_IN_L0_THR]	= REG_FIELD(VADC_L0_THR, 0, 7),
> +	[F_IN_L1_THR]	= REG_FIELD(VCMP_L1_THR, 0, 5),
> +	[F_IN_INPUT_EN]	= REG_FIELD(VADC_CONV_REQ, 0, 0),
> +	[F_IN_INPUT]	= REG_FIELD(VADC_DATA1, 0, 7),
> +	[F_CURR_MON_EN]	= REG_FIELD(IADC_CONV_REQ, 0, 0),
> +	[F_CURR_H0_THR]	= REG_FIELD(IADC_H0_THR, 0, 7),
> +	[F_CURR_H1_THR]	= REG_FIELD(IADC_H1_THR, 0, 7),
> +	[F_CURR_INPUT]	= REG_FIELD(IADC_DATA1, 0, 7),
> +};

Please begin with a simplified driver supporting one version and then
add more versions. That will help for the review process.

[ ... ]

> +/* V1 BMX and core */
> +static const struct bcl_desc pm7250b_data = {
> +	.reg_fields = bcl_v1_reg_fields,
> +	.num_reg_fields = F_MAX_FIELDS,
> +	.data_field_bits_size = 8,
> +	.thresh_field_bits_size = 7,
> +	.channel[IN] = {
> +		.base = 2250,
> +		.max = 3600,
> +		.step = 25,
> +		.default_scale_nu = 194637,
> +		.thresh_type = {ADC, INDEX},
> +	},
> +	.channel[CURR] = {
> +		.max = 10000,
> +		.default_scale_nu = 305180,
> +		.thresh_type = {ADC, ADC},
> +	},
> +};
>
> +/* V2 BMX and core */

Ditto

[ ... ]

> +/**
> + * bcl_convert_raw_to_milliunit - Convert raw value to milli unit
> + * @desc: BCL device descriptor
> + * @raw_val: Raw ADC value from hardware
> + * @type: type of the channel, in or curr
> + * @field_width: bits size for data or threshold field
> + *
> + * Return: value in milli unit
> + */
> +static unsigned int bcl_convert_raw_to_milliunit(const struct bcl_desc *desc, int raw_val,
> +					enum bcl_channel_type type, u8 field_width)
> +{
> +	u32 def_scale = desc->channel[type].default_scale_nu;
> +	u32 lsb_weight = field_width > 8 ? 1 : 1 << field_width;
> +	u32 scaling_factor = def_scale * lsb_weight;

This is confusing, can you explain ?

if 'field_width' == 7, then we do def_scale * 1^7 ?

Why ?

> +	return div_s64((s64)raw_val * scaling_factor, 1000000);

If it is milliunit why dividing by 10^6 ?

> +/**
> + * bcl_convert_milliunit_to_index - Convert milliunit to in or curr index
> + * @desc: BCL device descriptor
> + * @val: in or curr value in milli unit
> + * @type: type of the channel, in or curr
> + *
> + * Converts a value in milli unit to an index for BCL that use indexed thresholds.
> + *
> + * Return: Voltage index value
> + */
> +static unsigned int bcl_convert_milliunit_to_index(const struct bcl_desc *desc, int val,
> +					  enum bcl_channel_type type)
> +{
> +	return div_s64((s64)val - desc->channel[type].base, desc->channel[type].step);
> +}
> +
> +/**
> + * bcl_convert_index_to_milliunit - Convert in or curr index to milli unit
> + * @desc: BCL device descriptor
> + * @val: index value
> + * @type: type of the channel, in or curr
> + *
> + * Converts an index value to milli unit for BCL that use indexed thresholds.
> + *
> + * Return: Voltage value in millivolts
> + */
> +static unsigned int bcl_convert_index_to_milliunit(const struct bcl_desc *desc, int val,
> +					 enum bcl_channel_type type)
> +{
> +	return desc->channel[type].base + val * desc->channel[type].step;

To be sure, is it (A + val) * B, or, A + (val * B) ?

> +}
> +
> +static int bcl_in_thresh_write(struct bcl_device *bcl, long value, enum bcl_limit_alarm lvl)
> +{
> +	const struct bcl_desc *desc = bcl->desc;
> +	u32 raw_val;
> +
> +	int thresh = clamp_val(value, desc->channel[IN].base, desc->channel[IN].max);
> +
> +	if (desc->channel[IN].thresh_type[lvl] == ADC)
> +		raw_val = bcl_convert_milliunit_to_raw(desc, thresh, IN,
> +						       desc->thresh_field_bits_size);
> +	else
> +		raw_val = bcl_convert_milliunit_to_index(desc, thresh, IN);
> +
> +	return regmap_field_write(bcl->fields[F_IN_L0_THR + lvl], raw_val);
> +}
> +
> +static int bcl_curr_thresh_write(struct bcl_device *bcl, long value, enum bcl_limit_alarm lvl)
> +{
> +	const struct bcl_desc *desc = bcl->desc;
> +	u32 raw_val;
> +
> +	/* Clamp only to curr max */
> +	int thresh = clamp_val(value, value, desc->channel[CURR].max);
> +
> +	if (desc->channel[CURR].thresh_type[lvl] == ADC)
> +		raw_val = bcl_convert_milliunit_to_raw(desc, thresh, CURR,
> +						       desc->thresh_field_bits_size);
> +	else
> +		raw_val = bcl_convert_milliunit_to_index(desc, thresh, CURR);
> +
> +	return regmap_field_write(bcl->fields[F_CURR_H0_THR + lvl], raw_val);
> +}
> +
> +static int bcl_in_thresh_read(struct bcl_device *bcl, enum bcl_limit_alarm lvl, long *out)
> +{
> +	int ret, thresh;
> +	u32 raw_val = 0;
> +	const struct bcl_desc *desc = bcl->desc;
> +
> +	ret = bcl_read_field_value(bcl, F_IN_L0_THR + lvl, &raw_val);
> +	if (ret)
> +		return ret;
> +
> +	if (desc->channel[IN].thresh_type[lvl] == ADC)
> +		thresh = bcl_convert_raw_to_milliunit(desc, raw_val, IN,
> +						      desc->thresh_field_bits_size);
> +	else
> +		thresh = bcl_convert_index_to_milliunit(desc, raw_val, IN);
> +
> +	*out = thresh;
> +
> +	return 0;
> +}
> +
> +static int bcl_curr_thresh_read(struct bcl_device *bcl, enum bcl_limit_alarm lvl, long *out)
> +{
> +	int ret, thresh;
> +	u32 raw_val = 0;
> +	const struct bcl_desc *desc = bcl->desc;
> +
> +	ret = bcl_read_field_value(bcl, F_CURR_H0_THR + lvl, &raw_val);
> +	if (ret)
> +		return ret;
> +
> +	if (desc->channel[CURR].thresh_type[lvl] == ADC)
> +		thresh = bcl_convert_raw_to_milliunit(desc, raw_val, CURR,
> +						      desc->thresh_field_bits_size);
> +	else
> +		thresh = bcl_convert_index_to_milliunit(desc, raw_val, CURR);
> +
> +	*out = thresh;
> +
> +	return 0;
> +}
> +
> +static int bcl_curr_input_read(struct bcl_device *bcl, long *out)
> +{
> +	int ret;
> +	u32 raw_val = 0;
> +	const struct bcl_desc *desc = bcl->desc;
> +
> +	ret = bcl_read_field_value(bcl, F_CURR_INPUT, &raw_val);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * The sensor sometime can read a value 0 if there are
> +	 * consecutive reads
> +	 */

In order to prevent the userspace to read in a too high rate the
values of a hwmon, where I guess it is the reason why the value can be
0, the cached value is returned if two reads are under a minimal
jiffies based interval.

Something like:

	  if (time_before(jiffies, last_updated + HZ))
	     return bcl->last_curr_input;

If it is correct, then that could be put at the beginning of the
function instead of checking the zero value.

> +	if (raw_val != 0)
> +		bcl->last_curr_input =
> +			bcl_convert_raw_to_milliunit(desc, raw_val, CURR,
> +						     desc->data_field_bits_size);
> +
> +	*out = bcl->last_curr_input;
> +
> +	return 0;
> +}
> +
> +static int bcl_in_input_read(struct bcl_device *bcl, long *out)
> +{
> +	int ret;
> +	u32 raw_val = 0;
> +	const struct bcl_desc *desc = bcl->desc;
> +
> +	ret = bcl_read_field_value(bcl, F_IN_INPUT, &raw_val);
> +	if (ret)
> +		return ret;
> +
> +	if (raw_val < GENMASK(desc->data_field_bits_size - 1, 0))
> +		bcl->last_in_input =
> +			bcl_convert_raw_to_milliunit(desc, raw_val, IN,
> +						     desc->data_field_bits_size);
> +
> +	*out = bcl->last_in_input;
> +
> +	return 0;
> +}
> +
> +static int bcl_read_alarm_status(struct bcl_device *bcl,
> +				 enum bcl_limit_alarm lvl, long *status)
> +{
> +	int ret;
> +	u32 raw_val = 0;
> +
> +	ret = bcl_read_field_value(bcl, F_LVL0_ALARM + lvl, &raw_val);
> +	if (ret)
> +		return ret;
> +
> +	*status = raw_val;
> +
> +	return 0;
> +}
> +
> +static unsigned int bcl_get_version_major(const struct bcl_device *bcl)
> +{
> +	u32 raw_val = 0;
> +
> +	bcl_read_field_value(bcl, F_V_MAJOR, &raw_val);
> +
> +	return raw_val;
> +}
> +
> +static unsigned int bcl_get_version_minor(const struct bcl_device *bcl)
> +{
> +	u32 raw_val = 0;
> +
> +	bcl_read_field_value(bcl, F_V_MINOR, &raw_val);
> +
> +	return raw_val;
> +}
> +
> +static void bcl_hwmon_notify_event(struct bcl_device *bcl, enum bcl_limit_alarm alarm)
> +{
> +	if (bcl->in_mon_enabled)
> +		hwmon_notify_event(bcl->hwmon_dev, hwmon_in,
> +				in_lvl_to_attr_map[alarm], 0);
> +	if (bcl->curr_mon_enabled)
> +		hwmon_notify_event(bcl->hwmon_dev, hwmon_curr,
> +				curr_lvl_to_attr_map[alarm], 0);
> +}

What is the rate of the events we can face if they are mitigations
happening under the hood from the HW ?

> +static void bcl_alarm_enable_poll(struct work_struct *work)
> +{
> +	struct bcl_alarm_data *alarm = container_of(work, struct bcl_alarm_data,
> +							 alarm_poll_work.work);
> +	struct bcl_device *bcl = alarm->device;
> +	long status;
> +
> +	guard(mutex)(&bcl->lock);
> +
> +	if (bcl_read_alarm_status(bcl, alarm->type, &status))
> +		goto re_schedule;
> +
> +	if (!status & !alarm->irq_enabled) {
> +		bcl_enable_irq(alarm);
> +		bcl_hwmon_notify_event(bcl, alarm->type);
> +		return;
> +	}
> +
> +re_schedule:
> +	schedule_delayed_work(&alarm->alarm_poll_work,
> +				msecs_to_jiffies(BCL_ALARM_POLLING_MS));
> +}
> +
> +static irqreturn_t bcl_handle_alarm(int irq, void *data)
> +{
> +	struct bcl_alarm_data *alarm = data;
> +	struct bcl_device *bcl = alarm->device;
> +	long status;
> +
> +	guard(mutex)(&bcl->lock);
> +
> +	if (bcl_read_alarm_status(bcl, alarm->type, &status) || !status)
> +		return IRQ_HANDLED;

So it is possible to have an interrupt but not an alarm (!status) ?

> +	if (!bcl->hwmon_dev)
> +		return IRQ_HANDLED;

This should not be needed, revisit the init routine

> +
> +	bcl_hwmon_notify_event(bcl, alarm->type);
> +
> +	bcl_disable_irq(alarm);
> +	schedule_delayed_work(&alarm->alarm_poll_work,
> +			msecs_to_jiffies(BCL_ALARM_POLLING_MS));

Why ?

> +	dev_dbg(bcl->dev, "Irq:%d triggered for bcl type:%d\n",
> +			 irq, alarm->type);

Please remove, that is debug code

> +	return IRQ_HANDLED;
> +}
> +
> +static umode_t bcl_hwmon_is_visible(const void *data,
> +				    enum hwmon_sensor_types type,
> +				    u32 attr, int channel)
> +{
> +	const struct bcl_device *bcl = data;
> +
> +	switch (type) {
> +	case hwmon_in:
> +		if (!bcl->in_mon_enabled)
> +			return 0;
> +		switch (attr) {
> +		case hwmon_in_input:
> +			return bcl->in_input_enabled ? 0444 : 0;
> +		case hwmon_in_label:
> +		case hwmon_in_min_alarm:
> +		case hwmon_in_lcrit_alarm:
> +			return 0444;
> +		case hwmon_in_min:
> +		case hwmon_in_lcrit:
> +			return 0644;
> +		default:
> +			return 0;
> +		}
> +	case hwmon_curr:
> +		if (!bcl->curr_mon_enabled)
> +			return 0;
> +		switch (attr) {
> +		case hwmon_curr_input:
> +		case hwmon_curr_label:
> +		case hwmon_curr_max_alarm:
> +		case hwmon_curr_crit_alarm:
> +			return 0444;
> +		case hwmon_curr_max:
> +		case hwmon_curr_crit:
> +			return 0644;
> +		default:
> +			return 0;
> +		}
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static int bcl_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
> +			   u32 attr, int channel, long val)

What is the benefit of the userspace to set the thresholds ? Should
they be a platform property ?

Should the thresholds be check to be in ascending order and separated
with 100mV (for IN) ?

> +{
> +	struct bcl_device *bcl = dev_get_drvdata(dev);
> +	int ret = -EOPNOTSUPP;
> +
> +	guard(mutex)(&bcl->lock);
> +
> +	switch (type) {
> +	case hwmon_in:
> +		switch (attr) {
> +		case hwmon_in_min:
> +		case hwmon_in_lcrit:
> +			ret = bcl_in_thresh_write(bcl, val, in_attr_to_lvl_map[attr]);
> +			break;
> +		default:
> +			ret = -EOPNOTSUPP;
> +		}
> +		break;
> +	case hwmon_curr:
> +		switch (attr) {
> +		case hwmon_curr_max:
> +		case hwmon_curr_crit:
> +			ret = bcl_curr_thresh_write(bcl, val, curr_attr_to_lvl_map[attr]);
> +			break;
> +		default:
> +			ret = -EOPNOTSUPP;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +static int bcl_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> +			  u32 attr, int channel, long *value)
> +{
> +	struct bcl_device *bcl = dev_get_drvdata(dev);
> +	int ret;
> +
> +	guard(mutex)(&bcl->lock);
> +
> +	switch (type) {
> +	case hwmon_in:
> +		switch (attr) {
> +		case hwmon_in_input:
> +			ret = bcl_in_input_read(bcl, value);
> +			break;
> +		case hwmon_in_min:
> +		case hwmon_in_lcrit:
> +			ret = bcl_in_thresh_read(bcl, in_attr_to_lvl_map[attr], value);
> +			break;
> +		case hwmon_in_min_alarm:
> +		case hwmon_in_lcrit_alarm:
> +			ret = bcl_read_alarm_status(bcl, in_attr_to_lvl_map[attr], value);
> +			break;
> +		default:
> +			ret = -EOPNOTSUPP;
> +		}
> +		break;

Please split this function into:

       case hwmon_in:
       	    return bcl_in_read();
       case hwmon_curr:
       	    return bcl_curr_read();
       default:
             return -EOPNOTSUPP;

> +	case hwmon_curr:
> +		switch (attr) {
> +		case hwmon_curr_input:
> +			ret = bcl_curr_input_read(bcl, value);
> +			break;
> +		case hwmon_curr_max:
> +		case hwmon_curr_crit:
> +			ret = bcl_curr_thresh_read(bcl, curr_attr_to_lvl_map[attr], value);
> +			break;
> +		case hwmon_curr_max_alarm:
> +		case hwmon_curr_crit_alarm:
> +			ret = bcl_read_alarm_status(bcl, curr_attr_to_lvl_map[attr], value);
> +			break;
> +		default:
> +			ret = -EOPNOTSUPP;
> +		}
> +		break;
> +	default:
> +		ret = -EOPNOTSUPP;
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +static int bcl_hwmon_read_string(struct device *dev,
> +				 enum hwmon_sensor_types type,
> +				 u32 attr, int channel, const char **str)
> +{
> +	switch (type) {
> +	case hwmon_in:
> +		if (attr != hwmon_in_label)
> +			break;
> +		*str = bcl_channel_label[IN];

Why not use 'channel' passed as parameter ?

> +		return 0;
> +	case hwmon_curr:
> +		if (attr != hwmon_curr_label)
> +			break;
> +		*str = bcl_channel_label[CURR];
> +		return 0;
> +	default:
> +		break;
> +	}
> +
> +	return -EOPNOTSUPP;

Given there are few channels I suggest to simplify to:

      if (type == hwmon_in && attr == hwmon_in_label) {
      	 *str = "BCL Voltage";
      } else if (type == hwmon_curr && attr == hwmon_curr_label) {
         *str = "BCL Current";
      } else {
      	 *str = NULL;
      }

      return *str ? 0 : -EOPNOTSUPP;

Up to you.

> +}
> +
> +static const struct hwmon_ops bcl_hwmon_ops = {
> +	.is_visible	= bcl_hwmon_is_visible,
> +	.read		= bcl_hwmon_read,
> +	.read_string	= bcl_hwmon_read_string,
> +	.write		= bcl_hwmon_write,
> +};
> +
> +static const struct hwmon_channel_info *bcl_hwmon_info[] = {
> +	HWMON_CHANNEL_INFO(in,
> +			   HWMON_I_INPUT | HWMON_I_LABEL | HWMON_I_MIN |
> +			   HWMON_I_LCRIT | HWMON_I_MIN_ALARM |
> +			   HWMON_I_LCRIT_ALARM),
> +	HWMON_CHANNEL_INFO(curr,
> +			   HWMON_C_INPUT | HWMON_C_LABEL | HWMON_C_MAX |
> +			   HWMON_C_CRIT | HWMON_C_MAX_ALARM |
> +			   HWMON_C_CRIT_ALARM),
> +	NULL,
> +};
> +
> +static const struct hwmon_chip_info bcl_hwmon_chip_info = {
> +	.ops	= &bcl_hwmon_ops,
> +	.info	= bcl_hwmon_info,
> +};
> +
> +static int bcl_curr_thresh_update(struct bcl_device *bcl)
> +{
> +	int ret, i;
> +
> +	if (!bcl->curr_thresholds[0])
> +		return 0;
> +
> +	for (i = 0; i < ALARM_MAX; i++) {
> +		ret = bcl_curr_thresh_write(bcl, bcl->curr_thresholds[i], i);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +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);

Can you describe why we can have this optionnal ?

Why not set bcl_hwmon_info[] regarding what is enabled instead of
having boolean checks all over the place ?

> +}
> +
> +static int bcl_alarm_irq_init(struct platform_device *pdev,
> +			      struct bcl_device *bcl)
> +{
> +	int ret = 0, irq_num = 0, i = 0;
> +	struct bcl_alarm_data *alarm;
> +
> +	for (i = LVL0; i < ALARM_MAX; i++) {
> +		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;

Why ?

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

No, this error implies the way the fields are declared is fragile, may
be revisit the declaration.

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

*dev could be passed as parameter instead of *pdev

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

Why don't you want this as a required property ?

> +	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");
> +		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,
> +	}, {
> +		.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@oss.qualcomm.com>");
> +MODULE_DESCRIPTION("QCOM BCL HWMON driver");
> +module_platform_driver(bcl_driver);
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/hwmon/qcom-bcl-hwmon.h b/drivers/hwmon/qcom-bcl-hwmon.h
> new file mode 100644
> index 000000000000..28a7154d9486
> --- /dev/null
> +++ b/drivers/hwmon/qcom-bcl-hwmon.h

As there is nothing shared with other files, no header is needed.

> @@ -0,0 +1,311 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2026, Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#ifndef __QCOM_BCL_HWMON_H__
> +#define __QCOM_BCL_HWMON_H__
> +
> +#define BCL_DRIVER_NAME			"qcom-bcl-hwmon"
> +
> +/* BCL common regmap offset */
> +#define REVISION1			0x0
> +#define REVISION2			0x1
> +#define STATUS				0x8
> +#define INT_RT_STS			0x10
> +#define EN_CTL1				0x46
> +
> +/* BCL GEN1 regmap offsets */
> +#define MODE_CTL1			0x41
> +#define VADC_L0_THR			0x48
> +#define VCMP_L1_THR			0x49
> +#define IADC_H0_THR			0x4b
> +#define IADC_H1_THR			0x4c
> +#define VADC_CONV_REQ			0x72
> +#define IADC_CONV_REQ			0x82
> +#define VADC_DATA1			0x76
> +#define IADC_DATA1			0x86
> +
> +/* BCL GEN3 regmap offsets */
> +#define VCMP_CTL			0x44
> +#define VCMP_L0_THR			0x47
> +#define PARAM_1				0x0e
> +#define IADC_H1_THR_GEN3		0x4d
> +
> +#define BCL_IN_INC_MV			25
> +#define BCL_ALARM_POLLING_MS		50
> +
> +/**
> + * enum bcl_limit_alarm - BCL alarm threshold levels
> + * @LVL0: Level 0 alarm threshold (mapped to in_min_alarm or curr_max_alarm)
> + * @LVL1: Level 1 alarm threshold (mapped to in_lcrit_alarm or curr_crit_alarm)
> + * @ALARM_MAX: sentinel value
> + *
> + * Defines the three threshold levels for BCL monitoring. Each level corresponds
> + * to different severity of in or curr conditions.
> + */
> +enum bcl_limit_alarm {
> +	LVL0,
> +	LVL1,
> +	ALARM_MAX,
> +};

These are too generic names and they can collide with other
subsystems. BCL_LIMIT_ALRM_LVL0/1, BLC_LIMIT_ALRM_MAX should be fine.

> +/**
> + * enum bcl_channel_type - BCL supported sensor channel type
> + * @IN: in (voltage) channel
> + * @CURR: curr (current) channel
> + * @CHANNEL_MAX: sentinel value
> + *
> + * Defines the supported channel types for bcl.
> + */
> +enum bcl_channel_type {

s/bcl_channel_type/bcl_channel/ ?

> +	IN,
> +	CURR,
> +
> +	CHANNEL_MAX,
> +};

Same comment: BCL_CHANNEL_IN, BCL_CHANNEL_CURR, BCL_CHANNEL_MAX

> +/**
> + * enum bcl_thresh_type - voltage or current threshold representation type
> + * @ADC: Raw ADC value representation
> + * @INDEX: Index-based voltage or current representation
> + *
> + * Specifies how voltage or current thresholds are stored and interpreted in
> + * registers. Some PMICs use raw ADC values while others use indexed values.
> + */
> +enum bcl_thresh_type {
> +	ADC,
> +	INDEX,
> +};

Ditto

> +/**
> + * enum bcl_fields - BCL register field identifiers
> + * @F_V_MAJOR: Major revision info field
> + * @F_V_MINOR: Minor revision info field
> + * @F_CTL_EN: Monitor enable control field
> + * @F_LVL0_ALARM: Level 0 alarm status field
> + * @F_LVL1_ALARM: Level 1 alarm status field
> + * @COMMON_FIELD_MAX: sentinel value for common fields
> + * @F_IN_MON_EN: voltage monitor enable control field
> + * @F_IN_L0_THR: voltage level 0 threshold field
> + * @F_IN_L1_THR: voltage level 1 threshold field
> + * @F_IN_INPUT_EN: voltage input enable control field
> + * @F_IN_INPUT: voltage input data field
> + * @F_CURR_MON_EN: current monitor enable control field
> + * @F_CURR_H0_THR: current level 0 threshold field
> + * @F_CURR_H1_THR: current level 1 threshold field
> + * @F_CURR_INPUT: current input data field
> + * @F_MAX_FIELDS: sentinel value
> + *
> + * Enumeration of all register fields used by the BCL driver for accessing
> + * registers through regmap fields.
> + */
> +enum bcl_fields {
> +	F_V_MAJOR,
> +	F_V_MINOR,
> +
> +	F_CTL_EN,
> +
> +	/* common alarm for in and curr channel */
> +	F_LVL0_ALARM,
> +	F_LVL1_ALARM,
> +
> +	COMMON_FIELD_MAX,
> +
> +	F_IN_MON_EN = COMMON_FIELD_MAX,
> +	F_IN_L0_THR,
> +	F_IN_L1_THR,
> +
> +	F_IN_INPUT_EN,
> +	F_IN_INPUT,
> +
> +	F_CURR_MON_EN,
> +	F_CURR_H0_THR,
> +	F_CURR_H1_THR,
> +
> +	F_CURR_INPUT,
> +
> +	F_MAX_FIELDS
> +};

Usually this is described with macro, not enum.

> +#define ADD_BCL_HWMON_ALARM_MAPS(_type, lvl0_attr, lvl1_attr)	\
> +								\
> +static const u8 _type##_attr_to_lvl_map[] = {			\
> +	[hwmon_##_type##_##lvl0_attr] = LVL0,			\
> +	[hwmon_##_type##_##lvl1_attr] = LVL1,			\
> +	[hwmon_##_type##_##lvl0_attr##_alarm] = LVL0,		\
> +	[hwmon_##_type##_##lvl1_attr##_alarm] = LVL1,		\
> +};								\
> +								\
> +static const u8 _type##_lvl_to_attr_map[ALARM_MAX] = {		\
> +	[LVL0] = hwmon_##_type##_##lvl0_attr##_alarm,		\
> +	[LVL1] = hwmon_##_type##_##lvl1_attr##_alarm,		\
> +}

Please avoid these macros, they don't help to the readability.

> +/**
> + * struct bcl_channel_cfg - BCL channel related configuration
> + * @default_scale_nu:  Default scaling factor in nano unit
> + * @base: Base threshold value in milli unit
> + * @max: Maximum threshold value in milli unit
> + * @step: step increment value between two indexed threshold value
> + * @thresh_type: Array specifying threshold representation type for each alarm level
> + *
> + * Contains hardware-specific configuration and scaling parameters for different
> + * channel(voltage and current)..
> + */
> +
> +struct bcl_channel_cfg {
> +	u32 default_scale_nu;
> +	u32 base;
> +	u32 max;
> +	u32 step;
> +	u8 thresh_type[ALARM_MAX];
> +};
> +
> +/**
> + * struct bcl_desc - BCL device descriptor
> + * @reg_fields: Array of register field definitions for this device variant
> + * @channel: Each channel specific(voltage or current) configuration
> + * @num_reg_fields: Number of register field definitions for this device variant
> + * @data_field_bits_size: data read register bit size
> + * @thresh_field_bits_size: lsb bit size those are not included in threshold register
> + *
> + * Contains hardware-specific configuration and scaling parameters for different
> + * BCL variants. Each PMIC model may have different register layouts and
> + * conversion factors.
> + */
> +
> +struct bcl_desc {
> +	const struct reg_field *reg_fields;
> +	struct bcl_channel_cfg channel[CHANNEL_MAX];
> +	u8 num_reg_fields;
> +	u8 data_field_bits_size;
> +	u8 thresh_field_bits_size;
> +};
> +
> +struct bcl_device;

No forward declaration, reorg the structure definitions below

> +/**
> + * struct bcl_alarm_data - BCL alarm interrupt data
> + * @irq: IRQ number assigned to this alarm
> + * @irq_enabled: Flag indicating if IRQ is enabled
> + * @type: Alarm level type (LVL0, or LVL1)
> + * @device: Pointer to parent BCL device structure
> + * @alarm_poll_work: delayed_work to poll alarm status

Why do you want to poll the alarm status if there is an interrupt ?

> + *
> + * Stores interrupt-related information for each alarm threshold level.
> + * Used by the IRQ handler to identify which alarm triggered.
> + */
> +struct bcl_alarm_data {
> +	int			irq;
> +	bool			irq_enabled;
> +	enum bcl_limit_alarm	type;
> +	struct bcl_device	*device;
> +	struct delayed_work	alarm_poll_work;
> +};
> +
> +/**
> + * struct bcl_device - Main BCL device structure
> + * @dev: Pointer to device structure
> + * @regmap: Regmap for accessing PMIC registers
> + * @fields: Array of regmap fields for register access
> + * @bcl_alarms: Array of alarm data structures for each threshold level
> + * @lock: Mutex for protecting concurrent hardware access
> + * @in_mon_enabled: Flag indicating if voltage monitoring is enabled
> + * @curr_mon_enabled: Flag indicating if current monitoring is enabled
> + * @curr_thresholds: Current threshold values in milliamps from dt-binding(LVL0 and LVL1)
> + * @base: the BCL regbase offset from regmap
> + * @in_input_enabled: Flag indicating if voltage input reading is enabled
> + * @last_in_input: Last valid voltage input reading in millivolts
> + * @last_curr_input: Last valid current input reading in milliamps
> + * @desc: Pointer to device descriptor with hardware-specific parameters
> + * @hwmon_dev: Pointer to registered hwmon device
> + * @hwmon_name: Sanitized name for hwmon device
> + *
> + * Main driver structure containing all state and configuration for a BCL
> + * monitoring instance. Manages voltage and current monitoring, thresholds,
> + * and alarm handling.
> + */
> +struct bcl_device {
> +	struct device		*dev;

This field is unnecessary if the debug message is removed from the
interrupt handler.

> +	struct regmap		*regmap;
> +	u16			base;
> +	struct regmap_field	*fields[F_MAX_FIELDS];
> +	struct bcl_alarm_data	bcl_alarms[ALARM_MAX];
> +	struct mutex		lock;

What is this lock for ?

> +	u32			curr_thresholds[ALARM_MAX];
> +	u32			last_in_input;
> +	u32			last_curr_input;
> +	bool			in_mon_enabled;
> +	bool			curr_mon_enabled;
> +	bool			in_input_enabled;
> +	const struct bcl_desc	*desc;
> +	struct device		*hwmon_dev;
> +	char			*hwmon_name;
> +};
> +
> +/**
> + * bcl_read_field_value - Read alarm status for a given level
> + * @bcl: BCL device structure
> + * @id: Index in bcl->fields[]
> + * @val: Pointer to store val
> + *
> + * Return: 0 on success or regmap error code
> + */
> +static inline int bcl_read_field_value(const struct bcl_device *bcl, enum bcl_fields id, u32 *val)
> +{
> +	return regmap_field_read(bcl->fields[id], val);
> +}
> +
> +/**
> + * 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)
> +
> +/**
> + * 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;
> +	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;
> +	alarm->irq_enabled = false;
> +	disable_irq_nosync(alarm->irq);
> +	disable_irq_wake(alarm->irq);
> +}
> +#endif /* __QCOM_BCL_HWMON_H__ */
> 
> -- 
> 2.43.0
> 

--