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

[Pierre-Louis Bossart]

On 7/27/26 18:27, 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.
> 
> Link: https://lore.kernel.org/all/5717102b-f7ab-42b2-8065-064d94dd2bee@xxxxxxxxx/
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Jorijn van der Graaf <jorijnvdgraaf@xxxxxxxxxxxxx>
> ---
> Pierre-Louis agreed the helper should be extended in the WCD9378 v1
> thread [1]. On the qcom bus this only adds next-bank scale writes of
> the same value on bank switches (the clock is fixed); verified on the
> Fairphone 6 that capture still works with them.
> 
> 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.
> 
> [1] https://lore.kernel.org/all/5717102b-f7ab-42b2-8065-064d94dd2bee@xxxxxxxxx/
> 
>  drivers/soundwire/bus.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index 0490777fa406..d94e44b59050 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);

This looks fine, but can I suggest an improvement? It'd be good if that
helper was used below instead of having the same test twice. If we ever
need quirks or updates it'll be done in a single location.

static int sdw_slave_set_frequency(struct sdw_slave *slave)
{
	int scale_index;
	u8 base;
	int ret;

	/*
	 * frequency base and scale registers are required for SDCA
	 * devices. They may also be used for 1.2+/non-SDCA devices.
	 * Driver can set the property directly, for now there's no
	 * DisCo property to discover support for the scaling registers
	 * from platform firmware.
	 */
	if (!slave->id.class_id && !slave->prop.clock_reg_supported)
		return 0;



>