Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
References: <20221022162742.21671-1-aidanmacdonald.0x0@gmail.com>
 <20221022162742.21671-2-aidanmacdonald.0x0@gmail.com>
 <ef6a326b-5c61-988b-2ec2-cd8e233e5d28@linaro.org>
From:   Aidan MacDonald <aidanmacdonald.0x0@gmail.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc:     broonie@kernel.org, lgirdwood@gmail.com, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org,
        kuninori.morimoto.gx@renesas.com, perex@perex.cz, tiwai@suse.com,
        alsa-devel@alsa-project.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 2/2] dt-bindings: ASoC: simple-card: Add
 system-clock-id property
Date:   Sun, 23 Oct 2022 14:47:56 +0100
In-reply-to: <ef6a326b-5c61-988b-2ec2-cd8e233e5d28@linaro.org>
Message-ID: <GMvEU8xVTkjIoQ518XWAaLkhldSZHlk7@localhost>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk


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.

Regards,
Aidan