Re: [PATCH v2 2/2] soundwire: honor clock_reg_supported in the clock scaling check

On 7/28/26 19:35, Jorijn van der Graaf wrote:
> sdw_slave_set_frequency() treats class_id and prop.clock_reg_supported
> as equivalent evidence that a slave implements the bus-clock base and
> scale registers, but the bank-switch reprogramming path checks class_id
> alone, so a class-0 slave that declared the registers never gets the
> next-bank scale written there. The registers are SoundWire 1.2, not
> SDCA, so a device may well implement them without setting the class
> field.
> 
> Extend the helper to honor clock_reg_supported, as discussed with
> Pierre-Louis in the WCD9378 review. This also makes a link whose
> peripherals all declare clock_reg_supported eligible for dynamic clock
> scaling in the generic bandwidth allocation, which is what declaring
> the registers means.
> 
> With the helper extended, sdw_slave_set_frequency()'s open-coded test
> computes the same predicate; call the helper there instead, so future
> quirks or updates land in one place.
> 
> Link: https://lore.kernel.org/all/[email protected]/
> Link: https://lore.kernel.org/all/[email protected]/
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Jorijn van der Graaf <[email protected]>

Reviewed-by: Pierre-Louis Bossart <[email protected]>

> ---
> v2: also call the helper from sdw_slave_set_frequency() instead of
> keeping the same test open-coded there, as Pierre-Louis suggested.
> 
> Both patches re-verified together at v2 on the Fairphone 6 (SM7635,
> WCD9378): "Configured bus base 1, scale 2, mclk 19200000, curr_freq
> 9600000" for both slaves at enumeration, no codec errors, capture
> works. On the qcom bus the helper extension only adds next-bank scale
> writes of the same value on bank switches (the clock is fixed).
> 
> One behavior change I cannot test: the helper also feeds
> is_clock_scaling_supported() in the generic bandwidth allocation, so an
> Intel link whose peripherals all pass the check - max98363 is the
> in-tree clock_reg_supported case - becomes eligible for dynamic clock
> scaling where it previously ran at a fixed clock. Only configurations
> that fail the bandwidth check today can select a different frequency; I
> could not test that combination, flagging it for the Intel side.

I don't see this as a major risk, the bandwidth needs are limited and
the BPT/BRA protocol isn't used. Worst-case there will be a bug report
on Chromebooks and we'll quirk this...

>  drivers/soundwire/bus.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index 0490777fa406..0c1cdd603926 100644
> --- a/drivers/soundwire/bus.c
> +++ b/drivers/soundwire/bus.c
> @@ -817,8 +817,11 @@ bool is_clock_scaling_supported_by_slave(struct sdw_slave *slave)
>  	/*
>  	 * Dynamic scaling is a defined by SDCA. However, some devices expose the class ID but
>  	 * can't support dynamic scaling. We might need a quirk to handle such devices.
> +	 * The clock base and scale registers themselves are SoundWire 1.2, so a device
> +	 * may implement them without setting the class field; the driver says so with
> +	 * clock_reg_supported.
>  	 */
> -	return slave->id.class_id;
> +	return slave->id.class_id || slave->prop.clock_reg_supported;
>  }
>  EXPORT_SYMBOL(is_clock_scaling_supported_by_slave);
>  
> @@ -1385,7 +1388,7 @@ static int sdw_slave_set_frequency(struct sdw_slave *slave)
>  	 * DisCo property to discover support for the scaling registers
>  	 * from platform firmware.
>  	 */
> -	if (!slave->id.class_id && !slave->prop.clock_reg_supported)
> +	if (!is_clock_scaling_supported_by_slave(slave))
>  		return 0;
>  
>  	scale_index = sdw_slave_get_scale_index(slave, &base);