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

[Manaf Meethalavalappu Pallikunhi]

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@xxxxxxxxxxxxxxxx>
> > ---
>
> [...]
>
> > +/* 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