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

[Konrad Dybcio]

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

[...]

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

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

[...]

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

[...]

> +	/*
> +	 * Map register value to battery configuration.
> +	 * As per hardware documentation:
> +	 * 0 -> 2S, 1 -> 3S, 2 -> 4S (unsupported)

Would it be hard to support?

[...]

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

Konrad