Re: [PATCH v1 2/2] dt-bindings: ASoC: simple-card: Add system-clock-id property

[Aidan MacDonald]

Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> writes:

> On 23/10/2022 09:47, Aidan MacDonald wrote:
>>
>> Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> 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@xxxxxxxxx>
>>>> ---
>>>>  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