[PATCH v2 0/2] Update handling of min and max channel in DisCo

[PATCH v2 0/2] Update handling of min and max channel in DisCo

[Charles Keepax]

The DisCo properties "mipi-sdw-min-channel-number" and
"mipi-sdw-max-channel-number" are both channel indexes rather than
counts. Update the kernel code to more accurately reflect this.

There are a few Qualcomm drivers that likely specify these incorrect,
min_ch=1, max_ch=X, rather than min_ch=0, max_ch=X-1, however these
should work fine as the number of supported channels is equivalent.
If people feel strongly I can send another series to fix those up.

Changes since v1:
 - Update wording on min_ch/max_ch comments

Charles Keepax (2):
  soundwire: Improve channel min/max comments
  soundwire: Assume default maximum valid channel if not specified

 drivers/soundwire/mipi_disco.c | 14 ++++++++------
 include/linux/soundwire/sdw.h  |  4 ++--
 2 files changed, 10 insertions(+), 8 deletions(-)

-- 
2.47.3

[PATCH v2 1/2] soundwire: Improve channel min/max comments

[Charles Keepax]

Presently, the comments for the minimum and maximum channels are
misleading. In the SoundWire/Disco specification these are actually
channel indexes not a count of the number of channels. So for example
min_ch=4 and max_ch=5 means that 1 or 2 channels are supported.
Update the comments associated with the structure to make this
more clear.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---

Changes since v1:
 - Update wording on min_ch/max_ch comments

 include/linux/soundwire/sdw.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index f710e5932b4b2..b8f5ad39e9b37 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -289,8 +289,8 @@ struct sdw_dp0_prop {
  * @ch_prep_timeout: Port-specific timeout value, in milliseconds
  * @imp_def_interrupts: If set, each bit corresponds to support for
  * implementation-defined interrupts
- * @max_ch: Maximum channels supported
- * @min_ch: Minimum channels supported
+ * @max_ch: Maximum Channel Number of this Audio Port
+ * @min_ch: Minimum Channel Number of this Audio Port
  * @num_channels: Number of discrete channels supported
  * @num_ch_combinations: Number of channel combinations supported
  * @channels: Discrete channels supported
-- 
2.47.3

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

[Charles Keepax]

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;
+
 		nval = fwnode_property_count_u32(node, "mipi-sdw-channel-combination-list");
 		if (nval > 0) {
 			dpn[i].num_ch_combinations = nval;
-- 
2.47.3

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

[Pierre-Louis Bossart]

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;

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

[Charles Keepax]

On Tue, Nov 10, 2026 at 03:30:43PM +0100, Pierre-Louis Bossart wrote:
> On 11/10/26 12:38, Charles Keepax wrote:
> > +		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;
> }

I am happy to do so if you feel strongly. However, the spec
doesn't really appear to rule out both being present, it does
imply that the channel list takes precedence. But given it
doesn't rule out having both, I quite like parsing both and
allowing the driver to make a decision about what to do with
that information.

Thanks,
Charles

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

[Pierre-Louis Bossart]

On 11/10/26 16:08, Charles Keepax wrote:
> On Tue, Nov 10, 2026 at 03:30:43PM +0100, Pierre-Louis Bossart wrote:
>> On 11/10/26 12:38, Charles Keepax wrote:
>>> +		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;
>> }
> 
> I am happy to do so if you feel strongly. However, the spec
> doesn't really appear to rule out both being present, it does
> imply that the channel list takes precedence. But given it
> doesn't rule out having both, I quite like parsing both and
> allowing the driver to make a decision about what to do with
> that information.
humm, the wording "Conditions: If Property mipi-sdw-channel-number-list
is present, then this [mipi-sdw-max-channel-number ]Property shall not
be present" is pretty strong.

I don't mind keeping your existing code but then we'd need to add a
comment to say that we discarded the 'shall' to cover potentially broken
cases.