Re: [PATCH v4 02/12] hwmon: Add Qualcomm PMIC BCL driver

[Manaf Meethalavalappu Pallikunhi]

Hi Konrad,


On 10/12/2026 4:59 PM, Konrad Dybcio wrote:
> On 10/5/26 2:24 PM, Manaf Meethalavalappu Pallikunhi wrote:
> > Add driver for Qualcomm SPMI PMIC Battery Current Limiting (BCL)
> > hardware monitor. The driver exposes battery voltage and current
> > monitoring through hwmon interface.
>
> [...]
>
> > +static int bcl_in_thresh_write(struct bcl_device *bcl, long value, enum bcl_limit_alarm lvl)
> > +{
>
> [...]
>
> > +static int bcl_curr_thresh_write(struct bcl_device *bcl, long value, enum bcl_limit_alarm lvl)
>
> these two and bcl_in/curr_thresh_read can be unified, then some
> helpers can be collapsed inline as they become single-use

ACK

> [...]
>
> > +static unsigned int bcl_get_version_major(const struct bcl_device *bcl)
> > +{
> > +	u32 raw_val = 0;
> > +
> > +	regmap_field_read(bcl->fields[F_V_MAJOR], &raw_val);
> > +
> > +	return raw_val;
> > +}
>
> This is single-use and trivial, inline it into the call site

ACK

> > +
> > +static unsigned int bcl_get_version_minor(const struct bcl_device *bcl)
> > +{
> > +	u32 raw_val = 0;
> > +
> > +	regmap_field_read(bcl->fields[F_V_MINOR], &raw_val);
> > +
> > +	return raw_val;
> > +}
>
> Likewise

ACK

> [...]
>
> > +static int bcl_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
> > +			   u32 attr, int channel, long val)
>
> Do we expect userland to program these, or are the default limits
> sensible?

There are some usecases where userspace needs to modify these settings
like in different ambient condition.


> [...]
>
> > +	/*
> > +	 * Map register value to battery configuration.
> > +	 * As per hardware documentation:
> > +	 * 0 -> 2S, 1 -> 3S, 2 -> 4S (unsupported)
>
> Would it be hard to support?

As per hardware description, upto 3S is enabled, will remove 4S part 
from documentation.

> [...]
>
> > +	/* Check if battery detection is supported */
> > +	if (desc->battery_config_field.reg == 0) {
>
> This is slightly sketchy, but SPMI peripherals always have some ID
> registers at address zero, so okay..
>
> [...]
>
> > +static const struct of_device_id bcl_match[] = {
> > +	{
> > +		.compatible = "qcom,pm7250b-bcl",
> > +		.data = &pm7250b_data,
> > +	}, {
> > +		.compatible = "qcom,pm7550-bcl",
> > +		.data = &pm8550_data,
>
> Are they exactly identical? If so, please use a fallback compatible

ACK

Thanks,
Manaf

> Konrad