Message ID | 20230530075311.400686-6-fl.scratchpad@gmail.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:994d:0:b0:3d9:f83d:47d9 with SMTP id k13csp2018638vqr; Tue, 30 May 2023 01:21:12 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ6jGneTcqK/kZxpPQR1+62GgmnI7sBWeGTao6IbHK5HyAiR+1EsaeveNsl1A29dKJ5AyH34 X-Received: by 2002:a05:6a21:9992:b0:f8:1101:c074 with SMTP id ve18-20020a056a21999200b000f81101c074mr1814109pzb.54.1685434872585; Tue, 30 May 2023 01:21:12 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1685434872; cv=none; d=google.com; s=arc-20160816; b=Aa3MpVjSZs367MFtyqJZfvURzDc8hHDf9mC2ut3CAKPnQ2tFBKrlD9TwSnHBL9VwVN eqluRoVDm/NejRYCED2Lxr8i9QTcmxxzqHrSTlae32GUULkeU9FwJsUtytGBHfRimGzY oD0pR80OhCnkZF2TKsSSqtronovOsvhPZdaubwMRYRkOzsjELBEWfEPgfRnBsQG1NALP AV6q65piv82TsGC/CSI1EeQZPfHIc43gQjC1neyTAk85MdY57lvoqzOAzU2Y8wxur2MF si1o1aNlENS7fT2uorCbrY6yuGmXjFrVUSOk1iGe+Oz99q414MwbCpUXS3BxMecwQkF0 0tUg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=mizo9gnkF3NdkFdByq3QjeHMqHH/cKSCHXOmzX7m6qI=; b=JU2mAPcTylDgxfa6m2zxNnJdV4ZTBqPwIkYVRD9+jDueFhOX1hYVETlMQwM84qoJmQ B6fTzdE5os7VVJWQLyNBwC69ox0wrP29maqVXKuy3JW76SZK9c8wNTof4yfPGrRESM1Z Cr11u7RVdXFelsYYRV0ACCYCzrOAwOQQMD/hk/hZFEU899VnIqRCY6/PpplR0PmMS35X jsAbMKedHaNSaOYe4Ot+M5nvXoLRQRuNECr/I0lXQftcjX9F76nJuUlU6yfvidnYoch1 ElDdpRa7fa04az3adNeIIC597jp+XyO0S2WYgcqXB56LlNa8fU7UV8uHoJ6n8PockUQB E53A== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20221208 header.b=CiTz7HwU; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id b7-20020a633407000000b00518cf8916e2si11049169pga.415.2023.05.30.01.20.58; Tue, 30 May 2023 01:21:12 -0700 (PDT) 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; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20221208 header.b=CiTz7HwU; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229651AbjE3Hx7 (ORCPT <rfc822;andrewvogler123@gmail.com> + 99 others); Tue, 30 May 2023 03:53:59 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:47896 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229698AbjE3Hxq (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 30 May 2023 03:53:46 -0400 Received: from mail-wm1-x334.google.com (mail-wm1-x334.google.com [IPv6:2a00:1450:4864:20::334]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0B002F0; Tue, 30 May 2023 00:53:39 -0700 (PDT) Received: by mail-wm1-x334.google.com with SMTP id 5b1f17b1804b1-3f60804faf4so26869735e9.3; Tue, 30 May 2023 00:53:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1685433217; x=1688025217; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=mizo9gnkF3NdkFdByq3QjeHMqHH/cKSCHXOmzX7m6qI=; b=CiTz7HwUaUiU19fDMjGcyNpQMym6AXUPkMPMO8RN85jgjhc8V2EJRvcFAuKM3jkWlR mOxCIveZEZgC8sB4XDVKw0wAkALSGqd8UT2yl+ppK9eEeS7MIEuKZMQ+M+302iwfoD4U O/cgGQdpm8a9V9Ss/woUm+e0X+pS8ahcLrwEut9BeePMmuql/voi0pHsmB1bSSmro5tr HQcbtuwU4ZZaGUAuyWlKwEiMF0xcbu6sDrZA9aLcdgN48iOIy5Y3q9eHQPlVz2dd5UE7 1z862mWoekHg4sXcd8PcAMdmHcLuN0s9OlMhE4cbcF4ySmRtjcdzSJT7iThX8Z1IvR0I NaRQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1685433217; x=1688025217; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=mizo9gnkF3NdkFdByq3QjeHMqHH/cKSCHXOmzX7m6qI=; b=Y2r1Vw0tps22k1GmyvPbyVb8cvbAMMm3blyAxfQyDYMcHWsZo7wbBUX7oolWfxtoum 8LqbvxTAZ9MFYzK54tCrgdYsGCkb/EI4jOD2e5RO8aRnxeje8ZLWdZv8tUPDNRSpPmQ9 sC88L0WUEh5EXV+eqfYb320MB74WofQY1GPPNm8uRPwpajXWEuX+zP54iVxm6gFZ451f B3rxwfW5cO6ssjB4LdaEsfEsSBLiky2CH658T+SYLj8QDSNxLbGEMEI1jjovvybDN4OX CdiZ0zaeNeIQprHPtQU+6yLjw8s1/4MeVsbkA+DiG+0Ld494c9qIp8HMb6f+J0Dd6fWq XfZA== X-Gm-Message-State: AC+VfDxg2DOfbaOf7iTiT29YsQ09V9lnEGCOtGY0UIEPh4CDmYKeqyI5 zchvnGdI8uuA7Ba4qSz36ws= X-Received: by 2002:a1c:7716:0:b0:3f4:298b:d925 with SMTP id t22-20020a1c7716000000b003f4298bd925mr805890wmi.41.1685433217367; Tue, 30 May 2023 00:53:37 -0700 (PDT) Received: from PC-UT2.ad.ennebielettronica.com ([78.152.97.130]) by smtp.gmail.com with ESMTPSA id x21-20020a05600c21d500b003f6041f5a6csm16561275wmj.12.2023.05.30.00.53.36 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 30 May 2023 00:53:37 -0700 (PDT) From: fl.scratchpad@gmail.com To: jic23@kernel.org, Lars-Peter Clausen <lars@metafoo.de>, Michael Hennerich <Michael.Hennerich@analog.com>, Alexandru Tachici <alexandru.tachici@analog.com>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org> Cc: linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org, Fabrizio Lamarque <fl.scratchpad@gmail.com>, Michael Hennerich <michael.hennerich@analog.com>, devicetree@vger.kernel.org Subject: [PATCH v3 5/5] dt-bindings: iio: ad7192: Allow selection of clock modes Date: Tue, 30 May 2023 09:53:11 +0200 Message-Id: <20230530075311.400686-6-fl.scratchpad@gmail.com> X-Mailer: git-send-email 2.34.1 In-Reply-To: <20230530075311.400686-1-fl.scratchpad@gmail.com> References: <20230530075311.400686-1-fl.scratchpad@gmail.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM, RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1767306557172684296?= X-GMAIL-MSGID: =?utf-8?q?1767306557172684296?= |
Series |
Fix ad7192 driver issues
|
|
Commit Message
Fabrizio Lamarque
May 30, 2023, 7:53 a.m. UTC
From: Fabrizio Lamarque <fl.scratchpad@gmail.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@gmail.com> --- .../bindings/iio/adc/adi,ad7192.yaml | 27 ++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-)
Comments
On Tue, May 30, 2023 at 09:53:11AM +0200, fl.scratchpad@gmail.com wrote: > From: Fabrizio Lamarque <fl.scratchpad@gmail.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@gmail.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? > + 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. Cheers, Conor. > + > adi,rejection-60-Hz-enable: > description: | > This bit enables a notch at 60 Hz when the first notch of the sinc > @@ -84,11 +96,12 @@ properties: > description: see Documentation/devicetree/bindings/iio/adc/adc.yaml > type: boolean > > +dependencies: > + adi,clock-xtal: ['clocks', 'clock-names'] > + > required: > - compatible > - reg > - - clocks > - - clock-names > - interrupts > - dvdd-supply > - avdd-supply > @@ -98,6 +111,13 @@ required: > > allOf: > - $ref: /schemas/spi/spi-peripheral-props.yaml# > + - if: > + required: > + - clocks > + - clock-names > + then: > + properties: > + adi,int-clock-output-enable: false > > unevaluatedProperties: false > > @@ -115,6 +135,7 @@ examples: > spi-cpha; > clocks = <&ad7192_mclk>; > clock-names = "mclk"; > + adi,clock-xtal; > interrupts = <25 0x2>; > interrupt-parent = <&gpio>; > dvdd-supply = <&dvdd>; > -- > 2.34.1 >
On Tue, May 30, 2023 at 7:22 PM Conor Dooley <conor@kernel.org> wrote: > > On Tue, May 30, 2023 at 09:53:11AM +0200, fl.scratchpad@gmail.com wrote: > > From: Fabrizio Lamarque <fl.scratchpad@gmail.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@gmail.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, > > > + 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 there is a very limited usage of a low precision clock output for synchronization purposes between multiple ADCs. In the remote case, I would rather use a precise, dedicated external digital clock. Would you agree if I remove the related lines from the change set? If not, I kindly ask for your suggestions. The existing implementation from AD already includes all these configurations (there are no driver patches, the proposed changes are just related to documentation). Thank you! Fabrizio > > Cheers, > Conor. > > > + > > adi,rejection-60-Hz-enable: > > description: | > > This bit enables a notch at 60 Hz when the first notch of the sinc > > @@ -84,11 +96,12 @@ properties: > > description: see Documentation/devicetree/bindings/iio/adc/adc.yaml > > type: boolean > > > > +dependencies: > > + adi,clock-xtal: ['clocks', 'clock-names'] > > + > > required: > > - compatible > > - reg > > - - clocks > > - - clock-names > > - interrupts > > - dvdd-supply > > - avdd-supply > > @@ -98,6 +111,13 @@ required: > > > > allOf: > > - $ref: /schemas/spi/spi-peripheral-props.yaml# > > + - if: > > + required: > > + - clocks > > + - clock-names > > + then: > > + properties: > > + adi,int-clock-output-enable: false > > > > unevaluatedProperties: false > > > > @@ -115,6 +135,7 @@ examples: > > spi-cpha; > > clocks = <&ad7192_mclk>; > > clock-names = "mclk"; > > + adi,clock-xtal; > > interrupts = <25 0x2>; > > interrupt-parent = <&gpio>; > > dvdd-supply = <&dvdd>; > > -- > > 2.34.1 > >
On 31/05/2023 08:59, Fabrizio Lamarque wrote: > On Tue, May 30, 2023 at 7:22 PM Conor Dooley <conor@kernel.org> wrote: >> >> On Tue, May 30, 2023 at 09:53:11AM +0200, fl.scratchpad@gmail.com wrote: >>> From: Fabrizio Lamarque <fl.scratchpad@gmail.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@gmail.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
On Wed, May 31, 2023 at 9:14 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 31/05/2023 08:59, Fabrizio Lamarque wrote: > > On Tue, May 30, 2023 at 7:22 PM Conor Dooley <conor@kernel.org> wrote: > >> > >> On Tue, May 30, 2023 at 09:53:11AM +0200, fl.scratchpad@gmail.com wrote: > >>> From: Fabrizio Lamarque <fl.scratchpad@gmail.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@gmail.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? The AD7192 needs to be configured according to the type of external clock that is applied on MCLK1/MCLK2 pins in order to activate the correct circuitry. Here are some citations from the datasheet: MCLK2 pin description: "The AD7192 has an internal 4.92 MHz clock. This internal clock can be made available on the MCLK2 pin. The clock for the AD7192 can be provided externally also in the form of a crystal or external clock. A crystal can be tied across the MCLK1 and MCLK2 pins. Alternatively, the MCLK2 pin can be driven with a CMOS-compatible clock and the MCLK1 pin left unconnected." Each of these clock modes have to be configured via AD7192 mode register. (Clock source configuration bits, mode register, CLK0 and CLK1). Here is their description from datasheet: "Either the on-chip 4.92 MHz clock or an external clock can be used. The ability to use an external clock allows several AD7192 devices to be synchronized. Also, 50 Hz/60 Hz rejection is improved when an accurate external clock drives the AD7192." The choice between internal clock, external crystal oscillator or external CMOS digital clock is a decision of the HW designer driven by noise rejection, synchronization, and cost requirements. If possible, I kindly ask you suggestions on how to adjust the description so that it would be cleaner. > > > > >> > >>> + 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. Ok, I understand. I will remove the bit from the patch in V4. Thank you. The bit was already existing upstream in the driver, but I would just drop the change in documentation without any additional patch that removes it from the driver. Best regards, Fabrizio Lamarque
On Wed, May 31, 2023 at 11:40:08AM +0200, Fabrizio Lamarque wrote: > On Wed, May 31, 2023 at 9:14 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: > > > > On 31/05/2023 08:59, Fabrizio Lamarque wrote: > > > On Tue, May 30, 2023 at 7:22 PM Conor Dooley <conor@kernel.org> wrote: > > >> > > >> On Tue, May 30, 2023 at 09:53:11AM +0200, fl.scratchpad@gmail.com wrote: > > >>> From: Fabrizio Lamarque <fl.scratchpad@gmail.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@gmail.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? > > The AD7192 needs to be configured according to the type of external > clock that is > applied on MCLK1/MCLK2 pins in order to activate the correct circuitry. > > Here are some citations from the datasheet: > > MCLK2 pin description: > "The AD7192 has an internal 4.92 MHz clock. This internal clock can be > made available > on the MCLK2 pin. The clock for the AD7192 can be provided externally > also in the form > of a crystal or external clock. A crystal can be tied across the MCLK1 > and MCLK2 pins. > Alternatively, the MCLK2 pin can be driven with a CMOS-compatible clock and the > MCLK1 pin left unconnected." > > Each of these clock modes have to be configured via AD7192 mode register. > (Clock source configuration bits, mode register, CLK0 and CLK1). > Here is their description from datasheet: > > "Either the on-chip 4.92 MHz clock or an external clock can be used. > The ability to > use an external clock allows several AD7192 devices to be synchronized. Also, > 50 Hz/60 Hz rejection is improved when an accurate external clock > drives the AD7192." > > The choice between internal clock, external crystal oscillator or > external CMOS digital > clock is a decision of the HW designer driven by noise rejection, > synchronization, and > cost requirements. > > If possible, I kindly ask you suggestions on how to adjust the description > so that it would be cleaner. For me at least, I partially wanted it explained so that intimate knowledge of the part was not required to review the binding! To me, the original description is perfectly clear about how the hardware is configured, but nothing says why software needs to actually know about it. I'd be happy if you worked > Each of these clock modes have to be configured via AD7192 mode register. into the description, but perhaps Krzysztof disagrees. Cheers, Conor. > > >>> + 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. > > Ok, I understand. I will remove the bit from the patch in V4. Thank you. > > The bit was already existing upstream in the driver, but I would just drop > the change in documentation without any additional patch that removes it > from the driver. > > Best regards, > Fabrizio Lamarque
On 31/05/2023 11:40, Fabrizio Lamarque wrote: > On Wed, May 31, 2023 at 9:14 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 31/05/2023 08:59, Fabrizio Lamarque wrote: >>> On Tue, May 30, 2023 at 7:22 PM Conor Dooley <conor@kernel.org> wrote: >>>> >>>> On Tue, May 30, 2023 at 09:53:11AM +0200, fl.scratchpad@gmail.com wrote: >>>>> From: Fabrizio Lamarque <fl.scratchpad@gmail.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@gmail.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? > > The AD7192 needs to be configured according to the type of external > clock that is > applied on MCLK1/MCLK2 pins in order to activate the correct circuitry. > > Here are some citations from the datasheet: > > MCLK2 pin description: > "The AD7192 has an internal 4.92 MHz clock. This internal clock can be > made available > on the MCLK2 pin. The clock for the AD7192 can be provided externally > also in the form > of a crystal or external clock. A crystal can be tied across the MCLK1 > and MCLK2 pins. > Alternatively, the MCLK2 pin can be driven with a CMOS-compatible clock and the > MCLK1 pin left unconnected." > > Each of these clock modes have to be configured via AD7192 mode register. > (Clock source configuration bits, mode register, CLK0 and CLK1). > Here is their description from datasheet: > > "Either the on-chip 4.92 MHz clock or an external clock can be used. > The ability to > use an external clock allows several AD7192 devices to be synchronized. Also, > 50 Hz/60 Hz rejection is improved when an accurate external clock > drives the AD7192." > > The choice between internal clock, external crystal oscillator or > external CMOS digital > clock is a decision of the HW designer driven by noise rejection, > synchronization, and > cost requirements. > > If possible, I kindly ask you suggestions on how to adjust the description > so that it would be cleaner. It's fine. I missed that part that first option requires feeding the clock through two pins ("and"). Best regards, Krzysztof
On Wed, May 31, 2023 at 9:24 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 31/05/2023 11:40, Fabrizio Lamarque wrote: > > On Wed, May 31, 2023 at 9:14 AM Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > >> > >> On 31/05/2023 08:59, Fabrizio Lamarque wrote: > >>> On Tue, May 30, 2023 at 7:22 PM Conor Dooley <conor@kernel.org> wrote: > >>>> > >>>> On Tue, May 30, 2023 at 09:53:11AM +0200, fl.scratchpad@gmail.com wrote: > >>>>> From: Fabrizio Lamarque <fl.scratchpad@gmail.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@gmail.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? > > > > The AD7192 needs to be configured according to the type of external > > clock that is > > applied on MCLK1/MCLK2 pins in order to activate the correct circuitry. > > > > Here are some citations from the datasheet: > > > > MCLK2 pin description: > > "The AD7192 has an internal 4.92 MHz clock. This internal clock can be > > made available > > on the MCLK2 pin. The clock for the AD7192 can be provided externally > > also in the form > > of a crystal or external clock. A crystal can be tied across the MCLK1 > > and MCLK2 pins. > > Alternatively, the MCLK2 pin can be driven with a CMOS-compatible clock and the > > MCLK1 pin left unconnected." > > > > Each of these clock modes have to be configured via AD7192 mode register. > > (Clock source configuration bits, mode register, CLK0 and CLK1). > > Here is their description from datasheet: > > > > "Either the on-chip 4.92 MHz clock or an external clock can be used. > > The ability to > > use an external clock allows several AD7192 devices to be synchronized. Also, > > 50 Hz/60 Hz rejection is improved when an accurate external clock > > drives the AD7192." > > > > The choice between internal clock, external crystal oscillator or > > external CMOS digital > > clock is a decision of the HW designer driven by noise rejection, > > synchronization, and > > cost requirements. > > > > If possible, I kindly ask you suggestions on how to adjust the description > > so that it would be cleaner. > > It's fine. I missed that part that first option requires feeding the > clock through two pins ("and"). Thank you once again. I've reworded the commit message, removed the adi,int-clock-output-enable option and clarified the adi,clock-xtal entry. This would be the new commit message: AD7192 can be clocked from either: - Internal clock - CMOS-compatible clock on MCLK2 - Crystal oscillator on MCLK1 and MCLK2 Each of these modes have to be configured via AD7192 mode register. Describe choice between these clock modes. And this is the new description for adi,clock-xtal: This bit sets external clock mode. When set, master clock is sourced from a crystal connected from MCLK1 to MCLK2. When cleared, a CMOS-compatible clock source is expected on MCLK2. Is this description cleaner? If possible, I kindly ask you for feedback before posting a v4 (with only patch 3/5 and 5/5 since the others have already been applied). Best regards, Fabrizio Lamarque
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 + + adi,int-clock-output-enable: + description: | + When internal clock is selected, this bit enables clock out pin. + type: boolean + adi,rejection-60-Hz-enable: description: | This bit enables a notch at 60 Hz when the first notch of the sinc @@ -84,11 +96,12 @@ properties: description: see Documentation/devicetree/bindings/iio/adc/adc.yaml type: boolean +dependencies: + adi,clock-xtal: ['clocks', 'clock-names'] + required: - compatible - reg - - clocks - - clock-names - interrupts - dvdd-supply - avdd-supply @@ -98,6 +111,13 @@ required: allOf: - $ref: /schemas/spi/spi-peripheral-props.yaml# + - if: + required: + - clocks + - clock-names + then: + properties: + adi,int-clock-output-enable: false unevaluatedProperties: false @@ -115,6 +135,7 @@ examples: spi-cpha; clocks = <&ad7192_mclk>; clock-names = "mclk"; + adi,clock-xtal; interrupts = <25 0x2>; interrupt-parent = <&gpio>; dvdd-supply = <&dvdd>;