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

[Manaf Meethalavalappu Pallikunhi]

Hi Konrad,


On 2/6/2026 2:57 PM, Konrad Dybcio wrote:
> On 2/5/26 10:14 PM, 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)
> >
> > 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@xxxxxxxxxxxxxxxx>
> > ---
> [...]
>
> > +/* Interrupt names for each alarm level */
> > +static const char * const bcl_int_names[ALARM_MAX] = {
> > +	[LVL0] = "bcl-max-min",
> > +	[LVL1] = "bcl-critical",
> > +};
> > +
> > +static const char * const bcl_channel_label[CHANNEL_MAX] = {
> > +	"BCL Voltage",
> > +	"BCL Current",
> > +};
> Let's strip the BCL prefix
Ack
> [...]
>
> > +/**
> > + * 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,
> raw_val is an int here, a u32 when you retrieve it and a s64 in the math..
Will check and update in next revision
> > +					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;
> Would this be equivalent?
>
> if (field_width > 8)
> 	def_scale <<= field_width;
Ack
> [...]
>
> > +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;
> > +}
> Do we really need so many read-1-value functions?
Ack, will remove those functions
> > +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);
> > +}
> > +
> > +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;
> Do we ever expect regmap_read to *actually* fail?
Since regmap_read API itself is saying, it can fail, added check. Will 
remove it.
> [...]
>
> > +static int bcl_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
> > +			   u32 attr, int channel, long val)
> > +{
> > +	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;
> Please don't "ret = ...;break;, just return directly, also in the function
> below
Ack
> [...]
>
> > +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;
> This too, fails if a regmap_write() fails and leaves other registers
> unconfigured if that happens for $reasons
Ack
> [...]
>
> > +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;
> If you don't expect this to grow, just inline it in .probe
For now, there is no any other requirement, will move it to probe
> [...]
>
> > +	if (!bcl_hw_is_enabled(bcl))
> > +		return -ENODEV;
> Please make this print a meaningful error - also, should we expect this to
Ack
> ever happen, or would it mean that the bootloader (or something) hasn't
> configured BCL prior to Linux booting?

Most of the cases, it will be enabled in firmware. But there is scenario 
for some variant, firmware will disable it at runtime

based on underlying battery sensing element due to hw issue. So We 
wanted to enable driver only if peripheral is enabled.

> [...]
>
>
> > + * 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 {
> > +	IN,
> > +	CURR,
> The enum defines could use a prefix, say CHANNEL_CURR
Ack
> > +
> > +	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,
> Same here, THRESH_TYPE_ADC
Ack
> [...]
>
> > +/**
> > + * 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);
> > +}
> This produces more characters than it would to inline the function
>
> Now, that doesn't mean it can't be like that, but it's certainly curious

Ack, will remove it in v2

Thanks,

Manaf

> Konrad