[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;