| Message ID | 20240613114001.270233-3-alisa.roman@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: adc: ad7192: Improvements | expand |
On Thu, Jun 13, 2024 at 02:39:58PM +0300, Alisa-Dariana Roman wrote: > There are actually 4 configuration modes of clock source for AD719X > devices. Either a crystal can be attached externally between MCLK1 and > MCLK2 pins, or an external CMOS-compatible clock can drive the MCLK2 > pin. The other 2 modes make use of the 4.92MHz internal clock. > > Add clock name xtal alongside mclk. When an external crystal is > attached, xtal should be chosen. When an external clock is used, mclk > should be chosen. This is still missing an explanation of why a new name is needed. Hint: do you need to change register settings to use one versus the other? > > The presence of an external clock source is optional, not required. When > absent, internal clock is used. Modify required property accordingly and > modify second example to showcase this. > > Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com> > --- > .../devicetree/bindings/iio/adc/adi,ad7192.yaml | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml > index a03da9489ed9..3ae2f860d24c 100644 > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml > @@ -39,11 +39,15 @@ properties: > > clocks: > maxItems: 1 > - description: phandle to the master clock (mclk) > + description: | > + Optionally, either a crystal can be attached externally between MCLK1 and > + MCLK2 pins, or an external CMOS-compatible clock can drive the MCLK2 > + pin. If absent, internal 4.92MHz clock is used. > > clock-names: > - items: > - - const: mclk > + enum: > + - xtal > + - mclk > > interrupts: > maxItems: 1 > @@ -135,8 +139,6 @@ patternProperties: > required: > - compatible > - reg > - - clocks > - - clock-names > - interrupts > - dvdd-supply > - avdd-supply > @@ -202,8 +204,6 @@ examples: > spi-max-frequency = <1000000>; > spi-cpol; > spi-cpha; > - clocks = <&ad7192_mclk>; > - clock-names = "mclk"; > interrupts = <25 0x2>; > interrupt-parent = <&gpio>; > aincom-supply = <&aincom>; > -- > 2.34.1 >
On Thu, 13 Jun 2024 17:41:52 +0100 Conor Dooley <conor@kernel.org> wrote: > On Thu, Jun 13, 2024 at 02:39:58PM +0300, Alisa-Dariana Roman wrote: > > There are actually 4 configuration modes of clock source for AD719X > > devices. Either a crystal can be attached externally between MCLK1 and > > MCLK2 pins, or an external CMOS-compatible clock can drive the MCLK2 > > pin. The other 2 modes make use of the 4.92MHz internal clock. > > > > Add clock name xtal alongside mclk. When an external crystal is > > attached, xtal should be chosen. When an external clock is used, mclk > > should be chosen. > > This is still missing an explanation of why a new name is needed. > Hint: do you need to change register settings to use one versus the > other? Absolutely - dt reviewer shouldn't need to look at the code to find this out as it wastes their over subscribed time! > > > > > The presence of an external clock source is optional, not required. When > > absent, internal clock is used. Modify required property accordingly and > > modify second example to showcase this. > > > > Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com> > > --- > > .../devicetree/bindings/iio/adc/adi,ad7192.yaml | 14 +++++++------- > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml > > index a03da9489ed9..3ae2f860d24c 100644 > > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml > > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml > > @@ -39,11 +39,15 @@ properties: > > > > clocks: > > maxItems: 1 > > - description: phandle to the master clock (mclk) > > + description: | > > + Optionally, either a crystal can be attached externally between MCLK1 and > > + MCLK2 pins, or an external CMOS-compatible clock can drive the MCLK2 > > + pin. If absent, internal 4.92MHz clock is used. Trivial but doesn't need to be formatted, so don't think the | matters. > > > > clock-names: > > - items: > > - - const: mclk > > + enum: > > + - xtal > > + - mclk > > > > interrupts: > > maxItems: 1 > > @@ -135,8 +139,6 @@ patternProperties: > > required: > > - compatible > > - reg > > - - clocks > > - - clock-names > > - interrupts > > - dvdd-supply > > - avdd-supply > > @@ -202,8 +204,6 @@ examples: > > spi-max-frequency = <1000000>; > > spi-cpol; > > spi-cpha; > > - clocks = <&ad7192_mclk>; > > - clock-names = "mclk"; Why drop it from the example? It's a valid setting to have one after all. > > interrupts = <25 0x2>; > > interrupt-parent = <&gpio>; > > aincom-supply = <&aincom>; > > -- > > 2.34.1 > >
diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml index a03da9489ed9..3ae2f860d24c 100644 --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7192.yaml @@ -39,11 +39,15 @@ properties: clocks: maxItems: 1 - description: phandle to the master clock (mclk) + description: | + Optionally, either a crystal can be attached externally between MCLK1 and + MCLK2 pins, or an external CMOS-compatible clock can drive the MCLK2 + pin. If absent, internal 4.92MHz clock is used. clock-names: - items: - - const: mclk + enum: + - xtal + - mclk interrupts: maxItems: 1 @@ -135,8 +139,6 @@ patternProperties: required: - compatible - reg - - clocks - - clock-names - interrupts - dvdd-supply - avdd-supply @@ -202,8 +204,6 @@ examples: spi-max-frequency = <1000000>; spi-cpol; spi-cpha; - clocks = <&ad7192_mclk>; - clock-names = "mclk"; interrupts = <25 0x2>; interrupt-parent = <&gpio>; aincom-supply = <&aincom>;
There are actually 4 configuration modes of clock source for AD719X devices. Either a crystal can be attached externally between MCLK1 and MCLK2 pins, or an external CMOS-compatible clock can drive the MCLK2 pin. The other 2 modes make use of the 4.92MHz internal clock. Add clock name xtal alongside mclk. When an external crystal is attached, xtal should be chosen. When an external clock is used, mclk should be chosen. The presence of an external clock source is optional, not required. When absent, internal clock is used. Modify required property accordingly and modify second example to showcase this. Signed-off-by: Alisa-Dariana Roman <alisa.roman@analog.com> --- .../devicetree/bindings/iio/adc/adi,ad7192.yaml | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)