Re: [PATCH v2 2/2] soundwire: Assume default maximum valid channel if not specified

[Pierre-Louis Bossart <pierre-louis.bossart@linux.dev>]

On 11/10/26 12:38, Charles Keepax wrote:
> The SoundWire/DisCo specification defines all the supported channel
> properties as optional and states that in the case none are specified
> defaults of min_ch=0 and max_ch=7 should be used. If both properties
> are not present in the DisCo the current code results in min_ch=0
> and max_ch=0. As min_ch and max_ch are both channel indexes, users
> can't discern if they should interpret this as the default case
> (1-8 channels supported) or the DisCo specified a single valid
> channel of 0 (1 channel supported).
> 
> In the case there are no valid supported channel properties populate
> max_ch with 7 as the specification suggests, to avoid the ambiguity.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> ---
> 
> No changes since v1.
> 
>  drivers/soundwire/mipi_disco.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/soundwire/mipi_disco.c b/drivers/soundwire/mipi_disco.c
> index 259ad98b5b7a9..eadbda55f377f 100644
> --- a/drivers/soundwire/mipi_disco.c
> +++ b/drivers/soundwire/mipi_disco.c
> @@ -284,12 +284,6 @@ static int sdw_slave_read_dpn(struct sdw_slave *slave,
>  				"mipi-sdw-imp-def-dpn-interrupts-supported",
>  				&dpn[i].imp_def_interrupts);
>  
> -		fwnode_property_read_u32(node, "mipi-sdw-min-channel-number",
> -					 &dpn[i].min_ch);
> -
> -		fwnode_property_read_u32(node, "mipi-sdw-max-channel-number",
> -					 &dpn[i].max_ch);
> -
>  		nval = fwnode_property_count_u32(node, "mipi-sdw-channel-number-list");
>  		if (nval > 0) {
>  			dpn[i].num_channels = nval;
> @@ -309,6 +303,14 @@ static int sdw_slave_read_dpn(struct sdw_slave *slave,
>  				return ret;
>  		}
>  
> +		fwnode_property_read_u32(node, "mipi-sdw-min-channel-number",
> +					 &dpn[i].min_ch);
> +
> +		ret = fwnode_property_read_u32(node, "mipi-sdw-max-channel-number",
> +					       &dpn[i].max_ch);
> +		if (ret && !dpn[i].num_channels)
> +			dpn[i].max_ch = 7;
> +

this works, maybe with one possible improvement: in theory if
"mipi-sdw-channel-number-list" is defined one can ignore the min/max,
the two approaches with the list or min/max are mutually exclusive.

Something like this:

if (!dpn[i].num_channels) {
	fwnode_property_read_u32(node, "mipi-sdw-min-channel-number",
				 &dpn[i].min_ch);
	ret = fwnode_property_read_u32(node, "mipi-sdw-max-channel-number",
					       &dpn[i].max_ch);
	if (ret)
		dpn[i].max_ch = 7;
}

>  		nval = fwnode_property_count_u32(node, "mipi-sdw-channel-combination-list");
>  		if (nval > 0) {
>  			dpn[i].num_ch_combinations = nval;