| Message ID | 20221022162742.21671-2-aidanmacdonald.0x0@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes: > On 23/10/2022 09:47, Aidan MacDonald wrote: >> >> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes: >> >>> On 22/10/2022 12:27, Aidan MacDonald wrote: >>>> This is a new per-DAI property used to specify the clock ID argument >>>> to snd_soc_dai_set_sysclk(). >>> >>> You did no show the use of this property and here you refer to some >>> specific Linux driver implementation, so in total this does no look like >>> a hardware property. >>> >>> You also did not explain why do you need it (the most important piece of >>> commit msg). >>> >>>> >>>> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com> >>>> --- >>>> Documentation/devicetree/bindings/sound/simple-card.yaml | 8 ++++++++ >>>> 1 file changed, 8 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/sound/simple-card.yaml b/Documentation/devicetree/bindings/sound/simple-card.yaml >>>> index ed19899bc94b..cb7774e235d0 100644 >>>> --- a/Documentation/devicetree/bindings/sound/simple-card.yaml >>>> +++ b/Documentation/devicetree/bindings/sound/simple-card.yaml >>>> @@ -57,6 +57,12 @@ definitions: >>>> single fixed sampling rate. >>>> $ref: /schemas/types.yaml#/definitions/flag >>>> >>>> + system-clock-id: >>>> + description: | >>>> + Specify the clock ID used for setting the DAI system clock. >>> >>> >>> With lack of explanation above, I would say - use common clock framework >>> to choose a clock... >>> >>> >>> Best regards, >>> Krzysztof >> >> Sorry, I didn't explain things very well. The system clock ID is indeed >> a property of the DAI hardware. The ID is not specific to Linux in any >> way, and really it's an enumeration that requires a dt-binding. >> >> A DAI may support multiple system clock inputs or outputs identified by >> the clock ID. In the case of outputs, these could be distinct clocks >> that have their own I/O pins, or the clock ID could select the internal >> source clock used for a clock generator. For inputs, the system clock ID >> may inform the DAI how or where the system clock is being provided so >> hardware registers can be configured appropriately. >> >> Really the details do not matter, except that in a particular DAI link >> configuration a specific clock ID must be used. This is determined by >> the actual hardware connection between the DAIs; if the wrong clock is >> used, the DAI may not function correctly. >> >> Currently the device tree is ambiguous as to which system clock should >> be used when the DAI supports more than one, because there is no way to >> specify which clock was intended. Linux just treats the ID as zero, but >> that's currently a Linux-specific numbering so there's guarantee that >> another OS would choose the same clock as Linux. >> >> The system-clock-id property is therefore necessary to fully describe >> the hardware connection between DAIs in a DAI link when a DAI offers >> more than one choice of system clock. >> >> I will resend the patch with the above in the commit message. > > For example if you want to define which input pin to use (so you have > internal mux), it's quite unspecific to give them some indexes. What is > 0? What is 1? Number of pin? Number of pin counting from where? > > Since this is unanswered, the IDs are also driver and implementation > dependent, thus you still have the same problem - another OS can choose > different clock. That's not then a hardware description, but software > configuration. > > Best regards, > Krzysztof I answered this already. The enumeration is arbitrary. Create some dt-bindings and voila, it becomes standardized and OS-independent. Regards, Aidan
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes: > And the remaining piece I don't get is that these are not bindings for > codec, but for sound audio card. You want to set "system-clock-id" > property for audio card, while putting clock from codec, which will be > used to pass back to the codec... so it is a property of the codec, not > of the audio card. IOW, NAU8821_CLK_* does not configure here the clock > of the system, but only, only clock of the codec. The system clock is controlled at the DAI level, it's specific to one DAI on one component. The simple-card device node has sub-nodes for the DAI links, and each DAI link node has sub-nodes for the DAIs within the link. "system-clock-id" is a property on the DAI nodes, so it's not a card-level property, just one part of the overall card definition. Since the clock ID is something defined by the codec it would naturally be a value defined by the codec, but the *configuration* of the codec is part of the sound card because it depends on how everything is connected together. If you used the same codec in a different machine it would have a different configuration. Regards, Aidan
On 22/10/2022 12:27, Aidan MacDonald wrote: > This is a new per-DAI property used to specify the clock ID argument > to snd_soc_dai_set_sysclk(). > > Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com> > --- > Documentation/devicetree/bindings/sound/simple-card.yaml | 8 ++++++++ > 1 file changed, 8 insertions(+) My concerns were addressed, so: Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes: > On 26/10/2022 10:48, Aidan MacDonald wrote: >> >> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes: >> >>> And the remaining piece I don't get is that these are not bindings for >>> codec, but for sound audio card. You want to set "system-clock-id" >>> property for audio card, while putting clock from codec, which will be >>> used to pass back to the codec... so it is a property of the codec, not >>> of the audio card. IOW, NAU8821_CLK_* does not configure here the clock >>> of the system, but only, only clock of the codec. >> >> The system clock is controlled at the DAI level, it's specific to one >> DAI on one component. The simple-card device node has sub-nodes for the >> DAI links, and each DAI link node has sub-nodes for the DAIs within the >> link. "system-clock-id" is a property on the DAI nodes, so it's not a >> card-level property, just one part of the overall card definition. >> >> Since the clock ID is something defined by the codec it would naturally >> be a value defined by the codec, but the *configuration* of the codec is >> part of the sound card because it depends on how everything is connected >> together. If you used the same codec in a different machine it would >> have a different configuration. > > OK, that sounds reasonable. Thank you for explaining this. You still > need to convince Mark :) No problem, thanks for bearing with all my explanations! Mark raised some good points, and I have to agree with him. This could create too many future issues, and the problem might be better solved with the clock API -- but unfortunately that's not yet feasible. Regards, Aidan
diff --git a/Documentation/devicetree/bindings/sound/simple-card.yaml b/Documentation/devicetree/bindings/sound/simple-card.yaml index ed19899bc94b..cb7774e235d0 100644 --- a/Documentation/devicetree/bindings/sound/simple-card.yaml +++ b/Documentation/devicetree/bindings/sound/simple-card.yaml @@ -57,6 +57,12 @@ definitions: single fixed sampling rate. $ref: /schemas/types.yaml#/definitions/flag + system-clock-id: + description: | + Specify the clock ID used for setting the DAI system clock. + Defaults to 0 if unspecified. + $ref: /schemas/types.yaml#/definitions/uint32 + mclk-fs: description: | Multiplication factor between stream rate and codec mclk. @@ -145,6 +151,8 @@ definitions: $ref: "#/definitions/system-clock-direction-out" system-clock-fixed: $ref: "#/definitions/system-clock-fixed" + system-clock-id: + $ref: "#/definitions/system-clock-id" required: - sound-dai
This is a new per-DAI property used to specify the clock ID argument to snd_soc_dai_set_sysclk(). Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com> --- Documentation/devicetree/bindings/sound/simple-card.yaml | 8 ++++++++ 1 file changed, 8 insertions(+)