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

[Konrad Dybcio]

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

[...]

> +/* 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"


> +/* 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]

[...]

> +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


> +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

[...]

> +	/* 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()

[...]

> +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?

[...]
> +static const struct of_device_id spel_of_match[] = {
> +	{ .compatible = "qcom,spel" },

The compatible must contain a SoC name

Konrad