| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <7911bbae-c507-4420-a05c-89242941f774@oss.qualcomm.com>
Date: Fri, 6 Feb 2026 10:27:38 +0100
From: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
To: Manaf Meethalavalappu Pallikunhi <manaf.pallikunhi@....qualcomm.com>,
Guenter Roeck <linux@...ck-us.net>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley
<conor+dt@...nel.org>,
Bjorn Andersson <andersson@...nel.org>,
Konrad Dybcio <konradybcio@...nel.org>, amit.kucheria@....qualcomm.com,
Daniel Lezcano <daniel.lezcano@...aro.org>,
Gaurav Kohli <gaurav.kohli@....qualcomm.com>
Cc: linux-hwmon@...r.kernel.org, linux-arm-msm@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/4] hwmon: Add Qualcomm PMIC BCL hardware monitor driver
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@....qualcomm.com>
> ---
[...]
> +/* 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
[...]
> +/**
> + * 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..
> + 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;
[...]
> +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?
> +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?
[...]
> +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
[...]
> +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
[...]
> +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", ®);
> + 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
[...]
> + if (!bcl_hw_is_enabled(bcl))
> + return -ENODEV;
Please make this print a meaningful error - also, should we expect this to
ever happen, or would it mean that the bootloader (or something) hasn't
configured BCL prior to Linux booting?
[...]
> + * 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
> +
> + 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
[...]
> +/**
> + * 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
Konrad
Powered by blists - more mailing lists