On 31/05/2023 08:59, Fabrizio Lamarque wrote: > On Tue, May 30, 2023 at 7:22 PM Conor Dooley <conor@xxxxxxxxxx> wrote: >> >> On Tue, May 30, 2023 at 09:53:11AM +0200, fl.scratchpad@xxxxxxxxx wrote: >>> From: Fabrizio Lamarque <fl.scratchpad@xxxxxxxxx> >>> >>> 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@xxxxxxxxx> >>> --- >>> .../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
