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

[Manaf Meethalavalappu Pallikunhi <manaf.pallikunhi@oss.qualcomm.com>]

Hi Daniel,

On 10/27/2026 4:51 PM, Daniel Lezcano wrote:
> 
> Hi Manaf,
> 
> On 10/5/26 14:24, 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.
>>
>> The BCL driver provides
>> - Real-time voltage and current readings
>> - Configurable threshold-based alarms
>> - Interrupt-driven notifications when thresholds are exceeded
>> - Automatic threshold management with polling-based recovery
>> - Hardware-specific scaling factors and threshold representations
>>
>> Signed-off-by: Manaf Meethalavalappu Pallikunhi 
>> <manaf.pallikunhi@oss.qualcomm.com>
>> ---
> 
> There are a couple of issues in sashiko related to this patch (devres 
> LIFO issue and 6 bits encoding value too short for the represented value).

will check and update in next revision

> 
> [ ... ]
> 
>> +/**
>> + * struct bcl_channel_cfg - BCL channel related configuration
>> + * @default_scale_nu:  Default scaling factor in nano unit
>> + * @base_mu: Base threshold value in milli unit
>> + * @max_mu: Maximum threshold value in milli unit
>> + * @step_mu: step increment value between two indexed threshold value 
>> in milli unit
>> + * @thresh_type: Array specifying threshold representation type for 
>> each alarm level
>> + *
>> + * Contains hardware-specific configuration and scaling parameters 
>> for different
>> + * channels (voltage and current).
>> + */
>> +struct bcl_channel_cfg {
>> +    u32 default_scale_nu;
>> +    u32 base_mu;
>> +    u32 max_mu;
>> +    u32 step_mu;
>> +    u8 thresh_type[BCL_LIMIT_ALARM_MAX];
>> +};
>> +
>> +/**
>> + * struct bcl_desc - BCL device descriptor
>> + * @reg_fields: Array of register field definitions for this device 
>> variant
>> + * @channel_cfg: Array of channel configurations indexed by battery 
>> config
>> + *               PMICs without battery detection: only [BCL_BATT_1S] 
>> is defined
>> + *               PMICs with battery detection: [BCL_BATT_2S], 
>> [BCL_BATT_3S]
>> + * @battery_config_field: Register field for battery configuration 
>> detection
>> + *                        NOTE: This is an ABSOLUTE address, not 
>> relative to BCL base
>> + *                        Set to REG_FIELD(0, 0, 0) if not used
>> + * @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
>> + * @in_mon_enabled: Voltage monitoring enabled at boot
>> + * @in_input_enabled: Voltage input reading enabled at boot
>> + * @curr_mon_enabled: Current monitoring enabled at boot
>> + *
>> + * 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_cfg[BCL_BATT_3S + 1][CHANNEL_MAX];
>> +    const struct reg_field battery_config_field;
>> +    u8 num_reg_fields;
>> +    u8 data_field_bits_size;
>> +    u8 thresh_field_bits_size;
>> +    bool in_mon_enabled;
>> +    bool in_input_enabled;
>> +    bool curr_mon_enabled;
>> +};
> 
> It is your call but I suggest to use a single variable:
> 
> eg. u8 features
> 
> and use masks
> 
>      features = IN_MON | IN_INPUT;
> 
> that will reduce the declaration size below.

ACK

> 
>> +/**
>> + * struct bcl_alarm_data - BCL alarm interrupt data
>> + * @irq: IRQ number assigned to this alarm
>> + * @irq_enabled: Flag indicating if IRQ is enabled
>> + * @irq_wake_enabled: Flag indicating if IRQ wake is enabled
>> + * @shutting_down: Flag preventing work from re-enabling IRQ during 
>> teardown
>> + * @type: Alarm level type (LVL0, or LVL1)
>> + * @device: Pointer to parent BCL device structure
>> + * @a_lock: Mutex for protecting alarm state
>> + * @alarm_poll_work: delayed_work to poll alarm status
>> + *
>> + * 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;
>> +    bool            irq_wake_enabled;
>> +    bool            shutting_down;
> 
> Same suggestion

I wanted to keep these as separate bools. The shutting_down is accessed 
lock-free via READ_ONCE/WRITE_ONCE in the IRQ-handler fast path, while 
irq_enabled and irq_wake_enabled are accessed under a_lock using plain 
reads and writes. Packing them into one flags byte would make updates to 
the other flags read-modify-write operations on the same byte that the 
lock-free path reads with READ_ONCE.

> 
>> +    enum bcl_limit_alarm    type;
>> +    void            *device;
>> +    /* Protects alarm IRQ enable/disable state */
>> +    struct mutex        a_lock;
>> +    struct delayed_work    alarm_poll_work;
>> +};
>> +
>> +/**
>> + * struct bcl_device - Main BCL device structure
>> + * @dev: Pointer to device structure
>> + * @regmap: Regmap for accessing PMIC registers
>> + * @base: the BCL regbase offset from regmap
>> + * @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
>> + * @last_in_input: Last valid voltage input reading in millivolts
>> + * @last_curr_input: Last valid current input reading in milliamps
>> + * @last_in_updated: Timestamp of last voltage input update
>> + * @last_curr_updated: Timestamp of last current input update
>> + * @desc: Pointer to device descriptor with hardware-specific parameters
>> + * @hwmon_dev: Pointer to registered hwmon device
>> + * @hwmon_name: Sanitized name for hwmon device
>> + * @batt_config: Detected battery configuration (only for SMB2360/2370)
>> + * @batt_config_regfield: Regmap field for battery configuration 
>> register
>> + *
>> + * 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;
>> +    struct regmap        *regmap;
>> +    u16            base;
>> +    struct regmap_field    *fields[F_MAX_FIELDS];
>> +    struct bcl_alarm_data    bcl_alarms[BCL_LIMIT_ALARM_MAX];
>> +    /* Protects hardware register access and device state */
>> +    struct mutex        lock;
>> +    u32            last_in_input;
>> +    s32            last_curr_input;
>> +    unsigned long        last_in_updated;
>> +    unsigned long        last_curr_updated;
>> +    const struct bcl_desc    *desc;
>> +    struct device        *hwmon_dev;
>> +    char            *hwmon_name;
>> +    enum bcl_battery_config    batt_config;
>> +    struct regmap_field    *batt_config_regfield;
>> +};
>> +
>> +static const u8 in_attr_to_lvl_map[] = {
>> +    [hwmon_in_min] = BCL_LIMIT_ALARM_LVL0,
>> +    [hwmon_in_lcrit] = BCL_LIMIT_ALARM_LVL1,
>> +    [hwmon_in_lemergency] = BCL_LIMIT_ALARM_LVL2,
>> +    [hwmon_in_min_alarm] = BCL_LIMIT_ALARM_LVL0,
>> +    [hwmon_in_lcrit_alarm] = BCL_LIMIT_ALARM_LVL1,
>> +    [hwmon_in_lemergency_alarm] = BCL_LIMIT_ALARM_LVL2,
>> +};
>> +
>> +static const u8 in_lvl_to_attr_map[BCL_LIMIT_ALARM_MAX] = {
>> +    [BCL_LIMIT_ALARM_LVL0] = hwmon_in_min_alarm,
>> +    [BCL_LIMIT_ALARM_LVL1] = hwmon_in_lcrit_alarm,
>> +    [BCL_LIMIT_ALARM_LVL2] = hwmon_in_lemergency_alarm,
>> +};
>> +
>> +static const u8 curr_attr_to_lvl_map[] = {
>> +    [hwmon_curr_max] = BCL_LIMIT_ALARM_LVL0,
>> +    [hwmon_curr_crit] = BCL_LIMIT_ALARM_LVL1,
>> +    [hwmon_curr_max_alarm] = BCL_LIMIT_ALARM_LVL0,
>> +    [hwmon_curr_crit_alarm] = BCL_LIMIT_ALARM_LVL1,
>> +    [hwmon_curr_emergency_alarm] = BCL_LIMIT_ALARM_LVL2,
>> +};
>> +
>> +static const u8 curr_lvl_to_attr_map[BCL_LIMIT_ALARM_MAX] = {
>> +    [BCL_LIMIT_ALARM_LVL0] = hwmon_curr_max_alarm,
>> +    [BCL_LIMIT_ALARM_LVL1] = hwmon_curr_crit_alarm,
>> +    [BCL_LIMIT_ALARM_LVL2] = hwmon_curr_emergency_alarm,
>> +};
>> +
>> +/* Interrupt names for each alarm level */
>> +static const char * const bcl_int_names[BCL_LIMIT_ALARM_MAX] = {
>> +    [BCL_LIMIT_ALARM_LVL0] = "max-min",
>> +    [BCL_LIMIT_ALARM_LVL1] = "critical",
>> +    [BCL_LIMIT_ALARM_LVL2] = "emergency",
>> +};
> 
> why not fold the structures into a single one?
> 
> eg.
> 
> struct attr_lvl_map {
>      const int level;
>      const int attr;
>      const char *name;
> };
> 
> const curr_attr_lvl_map[] = {
>      {
>        .level = BCL_LIMIT_ALARM_LVL0,
>        .attr = hwmon_curr_max_alarm,
>        .name = "max-min"
>      }
>      ...
> };
> 
> And a function lvl_to_attr() / attr_to_lvl()
> 
> ?
> 
> 
> [ ... ]

ACK

Thanks,
Manaf

> 
> Thanks
> 
>    -- Daniel