Re: [PATCH 2/3] powercap: qcom: Add SPEL powercap driver

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

Hi Konrad,


On 5/21/2026 4:46 PM, Konrad Dybcio wrote:
> On 5/19/26 12:49 PM, Manaf Meethalavalappu Pallikunhi wrote:
>> The Qualcomm SoC Power and Electrical Limits (SPEL) provides hardware
>> based power monitoring and limiting capabilities for various power
>> domains including System, SoC, CPU clusters, GPU, and various other
>> subsystems.
>>
>> The driver integrates with the Linux powercap framework, exposing SPEL
>> capabilities through powercap sysfs interfaces.
>>
>> Signed-off-by: Manaf Meethalavalappu Pallikunhi <manaf.pallikunhi@oss.qualcomm.com>
>> ---
> 
> [...]
> 
>> +/* SPEL register bitmasks */
>> +#define ENERGY_STATUS_MASK		0xFFFFFFFF
> 
> GENMASK(m, n), across the other defines too, please
> 
> Then, you can drop the _OFFSET defines as FIELD_PREP/GET/MODIFY
> accessors will derive them from the mask
> 
> Please also use lowercase hex, file-wide

ACK

> 
> [...]
> 
>> +/* Constraint configuration */
>> +static struct spel_constraint_info constraints[] = {
>> +	/* SYS domain constraints */
>> +	{ 0x10, 0x70, BIT(0), SPEL_DOMAIN_SYS, POWER_LIMIT1 },
>> +	{ 0x14, 0x74, BIT(1), SPEL_DOMAIN_SYS, POWER_LIMIT2 },
>> +	{ 0x18, 0x78, BIT(2), SPEL_DOMAIN_SYS, POWER_LIMIT3 },
>> +	{ 0x1C, 0x7C, BIT(3), SPEL_DOMAIN_SYS, POWER_LIMIT4 },
>> +	/* SOC domain constraints */
> 
> "SoC"

ACK

> 
> 
>> +/* Helper functions */
>> +static bool is_pl_valid(struct spel_domain *sd, int pl)
>> +{
>> +	if (pl < POWER_LIMIT1 || pl >= NR_POWER_LIMITS)
>> +		return false;
>> +	return sd->pl_name[pl] ? true : false;
> 
> return !!sd->pl_name[pl]

ACK

> 
> [...]
> 
>> +static u64 spel_unit_xlate(struct spel_domain *sd, enum unit_type type,
>> +			   u64 value, int to_raw)
>> +{
>> +	struct spel_system *sp = sd->sp;
>> +	u64 units = 1;
>> +	u64 scale = 1;
>> +
>> +	switch (type) {
>> +	case POWER_UNIT:
>> +		units = sp->power_unit;
>> +		break;
>> +	case ENERGY_UNIT:
>> +		scale = ENERGY_UNIT_SCALE;
>> +		units = sp->energy_unit;
>> +		break;
>> +	case TIME_UNIT:
>> +		units = sp->time_unit;
>> +		break;
>> +	default:
>> +		return value;
> 
> nit: maybe setting units and scale explicitly in each entry could
> be better for maintainability, but potayto/potahto

ACK

> 
>> +static int spel_register_powercap(struct spel_system *sp)
>> +{
>> +	struct spel_domain *sd;
>> +	struct powercap_zone *power_zone = NULL;
>> +	int nr_pl, ret, i;
>> +
>> +	/* Register SYS domain as parent zone */
>> +	for (sd = sp->domains; sd < sp->domains + SPEL_DOMAIN_MAX; sd++) {
>> +		if (sd->id == SPEL_DOMAIN_SYS) {
>> +			nr_pl = spel_find_nr_power_limit(sd);
>> +
>> +			power_zone = powercap_register_zone(&sd->power_zone,
>> +							    sp->control_type, sd->name,
>> +					NULL, &zone_ops, nr_pl,
>> +					&constraint_ops);
>> +			if (IS_ERR(power_zone)) {
>> +				dev_err(sp->dev, "Failed to register power zone %s\n",
>> +					sd->name);
>> +				return PTR_ERR(power_zone);
>> +			}
>> +			sp->power_zone = power_zone;
>> +			break;
>> +		}
>> +	}
>> +
>> +	if (!power_zone) {
> 
> I believe this is only possible if ARRAY_SIZE(sp->domains) == 0,
> but it's not obivous that it's to protect it from that specifically

It will also catch a case where domains defined without root domain 
SPEL_DOMAIN_SYS
> 
> [...]
> 
>> +	/* Map spel domain registers (energy counters) */
>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "nodes");
>> +	if (!res) {
>> +		dev_err(dev, "Failed to get nodes resource\n");
>> +		return -EINVAL;
>> +	}
>> +	sp->node_base = devm_ioremap_resource(dev, res);
>> +	if (IS_ERR(sp->node_base))
>> +		return PTR_ERR(sp->node_base);
> 
> devm_platform_get_and_ioremap_resource()

ACK

> 
> [...]
> 
>> +static void spel_remove(struct platform_device *pdev)
>> +{
>> +	struct spel_system *sp = platform_get_drvdata(pdev);
>> +	int i;
>> +
>> +	if (!sp)
>> +		return;
>> +
>> +	/* Unregister in reverse order: children first, then SOC, then SYS */
>> +	for (i = SPEL_DOMAIN_MAX - 1; i >= 0; i--)
>> +		powercap_unregister_zone(sp->control_type, &sp->domains[i].power_zone);
> 
> Could you try adding a devm_ variant of these register functions?

Powercap framework doesn't support any devm_* API, you meant add this 
support in framework in this series ?

> 
> [...]
>> +static const struct of_device_id spel_of_match[] = {
>> +	{ .compatible = "qcom,spel" },
> 
> The compatible must contain a SoC name

ACK

Thanks,
Manaf
> 
> Konrad