[PATCH] ASoC: dt-bindings: dai-common: Narrow possible sound-dai-cells

[PATCH] ASoC: dt-bindings: dai-common: Narrow possible sound-dai-cells

[Krzysztof Kozlowski]

Instead of accepting any value for sound-dai-cells, the common DAI
properties schema should narrow them to sane choice.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

---

Mostly sound-dai-cells are 0 or 1, but
Documentation/devicetree/bindings/sound/amlogic,aiu.yaml has value of 2.
---
 Documentation/devicetree/bindings/sound/dai-common.yaml | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/sound/dai-common.yaml b/Documentation/devicetree/bindings/sound/dai-common.yaml
index 1aed2f0f1775..6db35887cbe6 100644
--- a/Documentation/devicetree/bindings/sound/dai-common.yaml
+++ b/Documentation/devicetree/bindings/sound/dai-common.yaml
@@ -13,6 +13,7 @@ allOf:
   - $ref: component-common.yaml#
 
 properties:
-  '#sound-dai-cells': true
+  '#sound-dai-cells':
+    enum: [0, 1, 2]
 
 additionalProperties: true
-- 
2.34.1

Re: [PATCH] ASoC: dt-bindings: dai-common: Narrow possible sound-dai-cells

[Jerome Brunet]

On Tue 09 Jan 2024 at 22:38, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> Instead of accepting any value for sound-dai-cells, the common DAI
> properties schema should narrow them to sane choice.
>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>

Adding a constraint solely based on current usage feels wrong.

A DAI provider in its generic form must have the sound-dai-cells to
provide one. It says nothing about how many parameters an actual device
might need. That is the idea behind this binding.

It is up to the device specific bindings to define that value.

If restricting things here is really important, defaulting to 0 (with a
comment explaining it) and letting actual devices then override the
value would feel less 'made up'

> ---
>
> Mostly sound-dai-cells are 0 or 1, but
> Documentation/devicetree/bindings/sound/amlogic,aiu.yaml has value of 2.
> ---
>  Documentation/devicetree/bindings/sound/dai-common.yaml | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/sound/dai-common.yaml b/Documentation/devicetree/bindings/sound/dai-common.yaml
> index 1aed2f0f1775..6db35887cbe6 100644
> --- a/Documentation/devicetree/bindings/sound/dai-common.yaml
> +++ b/Documentation/devicetree/bindings/sound/dai-common.yaml
> @@ -13,6 +13,7 @@ allOf:
>    - $ref: component-common.yaml#
>  
>  properties:
> -  '#sound-dai-cells': true
> +  '#sound-dai-cells':
> +    enum: [0, 1, 2]
>  
>  additionalProperties: true


-- 
Jerome

Re: [PATCH] ASoC: dt-bindings: dai-common: Narrow possible sound-dai-cells

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 830 bytes --]

On Wed, Jan 10, 2024 at 12:07:30PM +0100, Jerome Brunet wrote:
> On Tue 09 Jan 2024 at 22:38, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
> > Instead of accepting any value for sound-dai-cells, the common DAI
> > properties schema should narrow them to sane choice.

> Adding a constraint solely based on current usage feels wrong.

> A DAI provider in its generic form must have the sound-dai-cells to
> provide one. It says nothing about how many parameters an actual device
> might need. That is the idea behind this binding.

> It is up to the device specific bindings to define that value.

> If restricting things here is really important, defaulting to 0 (with a
> comment explaining it) and letting actual devices then override the
> value would feel less 'made up'

I tend to agree.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: dt-bindings: dai-common: Narrow possible sound-dai-cells

[Krzysztof Kozlowski]

On 10/01/2024 12:37, Mark Brown wrote:
> On Wed, Jan 10, 2024 at 12:07:30PM +0100, Jerome Brunet wrote:
>> On Tue 09 Jan 2024 at 22:38, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>>
>>> Instead of accepting any value for sound-dai-cells, the common DAI
>>> properties schema should narrow them to sane choice.
> 
>> Adding a constraint solely based on current usage feels wrong.

Current usage comes from current and past experience. There is no upper
limit in theory, but there is a limit which we found reasonable.

> 
>> A DAI provider in its generic form must have the sound-dai-cells to
>> provide one. It says nothing about how many parameters an actual device
>> might need. That is the idea behind this binding.

Just like with every #cells. Why sound should be different?

> 
>> It is up to the device specific bindings to define that value.

And device specific bindings define specific value applicable to the
device. From allowed choice of some reasonable values.

> 
>> If restricting things here is really important, defaulting to 0 (with a
>> comment explaining it) and letting actual devices then override the
>> value would feel less 'made up'

Wait, what do you mean by "letting actual devices then override"? It's
already like this. Nothing changed. What do you refer to?


Best regards,
Krzysztof

Re: [PATCH] ASoC: dt-bindings: dai-common: Narrow possible sound-dai-cells

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 645 bytes --]

On Wed, Jan 10, 2024 at 01:51:03PM +0100, Krzysztof Kozlowski wrote:
> On 10/01/2024 12:37, Mark Brown wrote:
> > On Wed, Jan 10, 2024 at 12:07:30PM +0100, Jerome Brunet wrote:

> >> If restricting things here is really important, defaulting to 0 (with a
> >> comment explaining it) and letting actual devices then override the
> >> value would feel less 'made up'

> Wait, what do you mean by "letting actual devices then override"? It's
> already like this. Nothing changed. What do you refer to?

The suggestion is that instead of limiting to 1 and having one device
override limit to 0 and have all the devices that need 1 override as
well.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: dt-bindings: dai-common: Narrow possible sound-dai-cells

[Krzysztof Kozlowski]

On 10/01/2024 13:57, Mark Brown wrote:
> On Wed, Jan 10, 2024 at 01:51:03PM +0100, Krzysztof Kozlowski wrote:
>> On 10/01/2024 12:37, Mark Brown wrote:
>>> On Wed, Jan 10, 2024 at 12:07:30PM +0100, Jerome Brunet wrote:
> 
>>>> If restricting things here is really important, defaulting to 0 (with a
>>>> comment explaining it) and letting actual devices then override the
>>>> value would feel less 'made up'
> 
>> Wait, what do you mean by "letting actual devices then override"? It's
>> already like this. Nothing changed. What do you refer to?
> 
> The suggestion is that instead of limiting to 1 and having one device

Nothing limits here to 0. I limit from all technically possible values
to reasonable subset.

> override limit to 0 and have all the devices that need 1 override as
> well.

I don't think that actual default value for this should be provided.
This should be conscious choice when writing bindings and driver.
Similarly we do already for some other #cells:
#io-channel-cells, address/size-cells (dtschema), #mux-control-cells and
others.

I agree we do not restrict all of them, though. However I do not see
single reason to allow developers use 3 as #sound-dai-cells.

Best regards,
Krzysztof

Re: [PATCH] ASoC: dt-bindings: dai-common: Narrow possible sound-dai-cells

[Jerome Brunet]

On Wed 10 Jan 2024 at 14:24, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 10/01/2024 13:57, Mark Brown wrote:
>> On Wed, Jan 10, 2024 at 01:51:03PM +0100, Krzysztof Kozlowski wrote:
>>> On 10/01/2024 12:37, Mark Brown wrote:
>>>> On Wed, Jan 10, 2024 at 12:07:30PM +0100, Jerome Brunet wrote:
>> 
>>>>> If restricting things here is really important, defaulting to 0 (with a
>>>>> comment explaining it) and letting actual devices then override the
>>>>> value would feel less 'made up'
>> 
>>> Wait, what do you mean by "letting actual devices then override"? It's
>>> already like this. Nothing changed. What do you refer to?
>> 
>> The suggestion is that instead of limiting to 1 and having one device
>
> Nothing limits here to 0. I limit from all technically possible values
> to reasonable subset.
>
>> override limit to 0 and have all the devices that need 1 override as
>> well.
>
> I don't think that actual default value for this should be provided.
> This should be conscious choice when writing bindings and driver.
> Similarly we do already for some other #cells:
> #io-channel-cells, address/size-cells (dtschema), #mux-control-cells and
> others.
>
> I agree we do not restrict all of them, though. However I do not see
> single reason to allow developers use 3 as #sound-dai-cells.
>

Similarly, I do not see a reason to forbid it.
Submitter should not have to update the generic bindings every time we
come up with something new.

I agree allowing 0, 1, 2 is a reasonable choice, for now. However it is
not correct and has not technical justification.
Trying to list every possibly value for something that is not limited
is bound to be wrong.

The commit description says you don't want to accept any value.
A default value is an alternate way to do that.
It does not require a justification because it is just convenience.

If someone fail to it document properly, it will be picked up by the
checking scripts.

> Best regards,
> Krzysztof


-- 
Jerome

Re: [PATCH] ASoC: dt-bindings: dai-common: Narrow possible sound-dai-cells

[Rob Herring]

On Wed, Jan 10, 2024 at 02:36:01PM +0100, Jerome Brunet wrote:
> 
> On Wed 10 Jan 2024 at 14:24, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
> > On 10/01/2024 13:57, Mark Brown wrote:
> >> On Wed, Jan 10, 2024 at 01:51:03PM +0100, Krzysztof Kozlowski wrote:
> >>> On 10/01/2024 12:37, Mark Brown wrote:
> >>>> On Wed, Jan 10, 2024 at 12:07:30PM +0100, Jerome Brunet wrote:
> >> 
> >>>>> If restricting things here is really important, defaulting to 0 (with a
> >>>>> comment explaining it) and letting actual devices then override the
> >>>>> value would feel less 'made up'
> >> 
> >>> Wait, what do you mean by "letting actual devices then override"? It's
> >>> already like this. Nothing changed. What do you refer to?
> >> 
> >> The suggestion is that instead of limiting to 1 and having one device
> >
> > Nothing limits here to 0. I limit from all technically possible values
> > to reasonable subset.
> >
> >> override limit to 0 and have all the devices that need 1 override as
> >> well.
> >
> > I don't think that actual default value for this should be provided.
> > This should be conscious choice when writing bindings and driver.
> > Similarly we do already for some other #cells:
> > #io-channel-cells, address/size-cells (dtschema), #mux-control-cells and
> > others.
> >
> > I agree we do not restrict all of them, though. However I do not see
> > single reason to allow developers use 3 as #sound-dai-cells.
> >
> 
> Similarly, I do not see a reason to forbid it.
> Submitter should not have to update the generic bindings every time we
> come up with something new.

Why not? If someone comes up with a use for N cells, I'd like to know 
about it which would be more easily seen here than hidden in some device 
specific binding.

That being said, there's a global max of 8 in dtschema already, so 
limiting here doesn't add that much.

Rob