Re: [PATCH v4 02/12] hwmon: Add Qualcomm PMIC BCL driver
From: Manaf Meethalavalappu Pallikunhi
Date: Wed Nov 04 2026 - 08:01:25 EST
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@xxxxxxxxxxxxxxxx>
---
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