Re: [PATCH v5 12/12] powercap: arm_scmi: Synthetic zone enable/disable

[Cristian Marussi <cristian.marussi@arm.com>]

On Tue, Apr 28, 2026 at 10:09:21AM +0100, Philip Radford wrote:
> Add functionality to disable and enable the synthetic zone which
> also affects the immediate children of the synthetic zone by applying
> the same command to them.
> 
> Signed-off-by: Philip Radford <philip.radford@arm.com>
> ---
>  drivers/powercap/arm_scmi_powercap.c | 81 ++++++++++++++++++++++++++++
>  1 file changed, 81 insertions(+)
> 
> diff --git a/drivers/powercap/arm_scmi_powercap.c b/drivers/powercap/arm_scmi_powercap.c
> index 81b5214acda4..1ed2949b06cb 100644
> --- a/drivers/powercap/arm_scmi_powercap.c
> +++ b/drivers/powercap/arm_scmi_powercap.c
> @@ -270,6 +270,85 @@ static int instance_root_release(struct powercap_zone *pz)
>  	return 0;
>  }
>  
> +static int instance_root_set_enable_state(struct powercap_zone *pz, bool enable)
> +{
> +	struct scmi_powercap_zone *root;
> +	struct scmi_powercap_root *pr;
> +	struct scmi_powercap_zone *child;

...child and root on the same line of declarations...

> +	int ret, first_err = 0;
> +
> +	if (!pz)
> +		return -EINVAL;
> +
> +	root = to_scmi_powercap_zone(pz);
> +	pr = container_of(root, struct scmi_powercap_root, instance_root);

...another user of you new macro !

> +
> +	list_for_each_entry(child, &pr->registered_zones[0], node) {
> +		if (child == &pr->instance_root)
> +			continue;
> +
> +		if (child->info->parent_id != SCMI_POWERCAP_ROOT_ZONE_ID)
> +			continue;
> +
> +		if (!child->info->cpli[0].cap_config)
> +			continue;
> +
> +		ret = powercap_ops->cap_enable_set(child->ph, child->info->id, enable);
> +
> +		if (ret && !first_err) {

...mmm what is the logic here ? why not bailing out on any error ?

> +			first_err = ret;
> +			dev_err(child->dev, "failed to %s zone %s: %d\n",
> +				enable ? "enable" : "disable",
> +				child->info->name, ret);
> +		}
> +	}
> +
> +	return first_err;

...especially if you anyway fails globally on any error....
..I am not completely sure bit given that youare operating on a
synthetic zone that you enable as a whole, while acting on its children
in the backstage...I would say that if any enable/disable fails on a
chidlren you should revert the enable status of the children thate were
succesffull and report the error...I mean the state of top synthatic
zones AND the states of the children MUST remain consistent...
...it CANNOT be that some chidlren fails, some succeeds and you report
an error..it must be all or nothing...
...example..top syntethic zone is OFF if all children were successfully
disabled...on a failure with one of the children you shoudl revert the
already successfully set children and report the global error...

> +}
> +
> +static int instance_root_set_enable(struct powercap_zone *pz, bool mode)
> +{
> +	return instance_root_set_enable_state(pz, mode);
> +}
> +
> +static int instance_root_get_enable(struct powercap_zone *pz, bool *mode)
> +{
> +	struct scmi_powercap_zone *root;
> +	struct scmi_powercap_root *pr;
> +	struct scmi_powercap_zone *child;
> +	bool enabled;
> +	int ret;
> +
> +	if (!pz || !mode)
> +		return -EINVAL;
> +
> +	root = to_scmi_powercap_zone(pz);
> +	pr = container_of(root, struct scmi_powercap_root, instance_root);
> +
> +	*mode = true;
> +
> +	list_for_each_entry(child, &pr->registered_zones[0], node) {

mmm...what is the point here of scanning the children to GET the
state...you should report the top syntethic zone state right ?
You could have disable children directly...that wont be reflected in the
Linux powercap hiearcrhy right ?
I mean should you NOT simply return the stae of the top syntethic zone
which is should have saved in the previous state_set operation above ?

I think that anyway if you disable a zone...any zone...ONLY that zone is
marked as disable in Lnux powercap ... am I right ? 

Then probably our SCMI fw will do much more on all the children..

Thanks,
Cristian