linux-kernel - Re: [PATCH v3 5/5] dt-bindings: iio: ad7192: Allow selection of clock modes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5d65b644-9b79-d232-d0d0-d2772325eef5@linaro.org>
Date:   Wed, 31 May 2023 09:14:18 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Fabrizio Lamarque <fl.scratchpad@...il.com>,
        Conor Dooley <conor@...nel.org>
Cc:     jic23@...nel.org, Lars-Peter Clausen <lars@...afoo.de>,
        Michael Hennerich <Michael.Hennerich@...log.com>,
        Alexandru Tachici <alexandru.tachici@...log.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>, linux-iio@...r.kernel.org,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v3 5/5] dt-bindings: iio: ad7192: Allow selection of clock
 modes

On 31/05/2023 08:59, Fabrizio Lamarque wrote:
> On Tue, May 30, 2023 at 7:22 PM Conor Dooley <conor@...nel.org> wrote:
>>
>> On Tue, May 30, 2023 at 09:53:11AM +0200, fl.scratchpad@...il.com wrote:
>>> From: Fabrizio Lamarque <fl.scratchpad@...il.com>
>>>
>>> AD7192 supports external clock sources, generated by a digital clock
>>> source or a crystal oscillator, or internally generated clock option
>>> without external components.
>>>
>>> Describe choice between internal and external clock, crystal or external
>>> oscillator, and internal clock output enable.
>>>
>>> Signed-off-by: Fabrizio Lamarque <fl.scratchpad@...il.com>
>>> ---
>>>  .../bindings/iio/adc/adi,ad7192.yaml          | 27 ++++++++++++++++---
>>>  1 file changed, 24 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
>>> index 16def2985ab4..f7ecfd65ad80 100644
>>> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
>>> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml
>>> @@ -32,7 +32,8 @@ properties:
>>>
>>>    clocks:
>>>      maxItems: 1
>>> -    description: phandle to the master clock (mclk)
>>> +    description: |
>>> +      Master clock (mclk). If not set, internal clock is used.
>>>
>>>    clock-names:
>>>      items:
>>> @@ -50,6 +51,17 @@ properties:
>>>    vref-supply:
>>>      description: VRef voltage supply
>>>
>>> +  adi,clock-xtal:
>>> +    description: |
>>> +      Select whether an external crystal oscillator or an external
>>> +      clock is applied as master (mclk) clock.
>>> +    type: boolean
>>
>> Am I being daft, or are these the same thing? If they are not, and use
>> different input pins, I think it should be explained as it not clear.
>> Could you explain why we actually care that the source is a xtal versus
>> it being mclk, and why just having master clock is not sufficient?
> 
> I may revise the description as follows. Feel free to add your suggestions
> in case it is still not clear enough.
> 
> "Select whether an external crystal oscillator between MCLK1 and MCLK2 or
> an external CMOS-compatible clock on MCLK2 is used as master clock".
> 
> This is used to properly set CLK0 and CLK1 bits in the MODE register.
> I guess most applications would use an external crystal or internal clock.
> The external digital clock would allow synchronization of multiple ADCs,

Description confuses me. Why would it matter what type of clock you have
as input - external crystal oscillator or external CMOS-compatible
clock? Later you refer to "internal", so maybe you meant here also
internal for one of the options?

> 
>>
>>> +  adi,int-clock-output-enable:
>>> +    description: |
>>> +      When internal clock is selected, this bit enables clock out pin.
>>> +    type: boolean
>>
>> And this one makes you a clock provider, so the devices advocate
>> position would be that you know that this bit should be set if
>> "clocks" is not present and a consumer requests a clock.
>> I don't seem to have got the driver patches (at least not in this
>> mailbox), so I have got no information on how you've actually implemented
>> this.
> 
> I see... When this bit is set, the AD7192 node should also be a clock provider.
> The clock is output on MCLK2 pin, hence it can be used with internally
> generated clock only.
> I tend to dislike the idea of a "conditional clock provider". Also, I'd guess

Either this is a clock provider via common clock framework or is not.
Don't re-implement clock provider via other properties but just skip
such feature.

Best regards,
Krzysztof