| Message ID | 20240109213812.558492-1-krzysztof.kozlowski@linaro.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | ASoC: dt-bindings: dai-common: Narrow possible sound-dai-cells | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
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
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.
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
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.
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
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
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
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
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(-)