Re: [PATCH v9 1/4] ASoC: SDCA: Add PDE verification reusable helper

[Pierre-Louis Bossart]

On 4/17/26 15:13, Niranjan H Y wrote:
>   Implement sdca_asoc_pde_ensure_ps() helper function to poll for PDE
> power state transitions. Per SDCA specification, after writing
> REQUESTED_PS, drivers must poll ACTUAL_PS until the target power state
> is reached.

Good initiative to introduce a new common helper...

> +/**
> + * sdca_asoc_pde_ensure_ps - Verify PDE power state reached target state
> + * @dev: Pointer to the device for error logging.
> + * @regmap: Register map for reading ACTUAL_PS register.
> + * @function_id: SDCA function identifier.
> + * @entity_id: SDCA entity identifier for the power domain.
> + * @from_ps: Source power state (SDCA_PDE_PSn value).
> + * @to_ps: Target power state (SDCA_PDE_PSn value).
> + * @pde_delays: Pointer to array of PDE delay specifications for this device,
> + *              or NULL to use default polling interval.
> + * @num_delays: Number of entries in pde_delays array.
> + *
> + * This function polls the ACTUAL_PS register to verify that a PDE power state
> + * transition has completed. Per SDCA specification, after writing REQUESTED_PS,
> + * the caller must poll ACTUAL_PS until it reflects the requested state.
> + *
> + * This function implements the polling logic but does NOT modify the power state.
> + * The caller is responsible for writing REQUESTED_PS before invoking this function.

Erm, why not dealing with the write to REQUESTED_PS in this helper? You have all the 'to' and 'from' information in the parameters.

> + *
> + * If a delay table is provided, appropriate polling intervals are extracted based
> + * on the from_ps and to_ps transition. If no table is provided or no matching entry
> + * is found, a default polling interval is used.
> + *
> + * Return: Returns zero when ACTUAL_PS reaches the target state, -ETIMEDOUT if the
> + * polling times out before reaching the target state, or a negative error code if
> + * a register read fails.
> + */
> +int sdca_asoc_pde_ensure_ps(struct device *dev, struct regmap *regmap,
> +			    int function_id, int entity_id,
> +			    int from_ps, int to_ps,
> +			    const struct sdca_pde_delay *pde_delays,
> +			    int num_delays)
> +{
> +	static const int polls = 100;
> +	static const int default_poll_us = 1000;
> +	unsigned int reg, val;
> +	int i, poll_us = default_poll_us;
> +	int ret;
> +
> +	if (pde_delays && num_delays > 0) {
> +		for (i = 0; i < num_delays; i++) {
> +			if (pde_delays[i].from_ps == from_ps && pde_delays[i].to_ps == to_ps) {
> +				poll_us = pde_delays[i].us / polls;
> +				break;
> +			}
> +		}
> +	}
> +
> +	reg = SDW_SDCA_CTL(function_id, entity_id, SDCA_CTL_PDE_ACTUAL_PS, 0);
> +
> +	for (i = 0; i < polls; i++) {
> +		if (i)
> +			fsleep(poll_us);

This solution will loop for up to 100 times, and the sleep duration could be questionable.

Say for example you have a 10ms transition, do you really want to read ACTUAL_PS every 100us?

If the pde_delay is 1ms then a read every 10us makes no sense, the SoundWire command protocol would not be able to handle such reads.

A minimum threshold on poll_us would make sense IMHO.

> +
> +		ret = regmap_read(regmap, reg, &val);
> +		if (ret)
> +			return ret;
> +		else if (val == to_ps)
> +			return 0;
> +	}
> +
> +	dev_err(dev, "PDE power transition failed: expected 0x%x, got 0x%x\n", to_ps, val);
> +	return -ETIMEDOUT;
> +}
> +EXPORT_SYMBOL(sdca_asoc_pde_ensure_ps);