| Message ID | 20231004063712.3348978-1-alvin@pqrs.dk |
|---|---|
| Headers | show |
| Series | clk: si5351: add option to adjust PLL without glitches | expand |
On Wed, Oct 4, 2023 at 4:40 PM Alvin Šipraga <ALSI@bang-olufsen.dk> wrote: > > On Wed, Oct 04, 2023 at 09:39:37AM -0500, Rob Herring wrote: > > > + silabs,multisynth-source: > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > + enum: [ 0, 1 ] > > > + description: | > > > > Don't need '|' if no formatting to preserve. > > I thought the line would be too long otherwise. > Column width is 80 in dt-schema as well, right? Yes, and up to 100 is fine as an exception. > > > > > + Source PLL A (0) or B (1) for the corresponding multisynth divider. But this doesn't look like it is over 80. Maybe if you put after 'description:' on the same line, but that's not what I said. It can still be on the next line. No '|' just means the line endings aren't fixed. Not important now, but if we were to generate pretty documentation from the schemas, then it would matter. > > [...] > > > > + - if: > > > + properties: > > > + compatible: > > > + contains: > > > + enum: > > > + - silabs,si5351a > > > + - silabs,si5351a-msop > > > + - silabs,si5351b > > > > Isn't this just the 'else' for the next one? Or more parts are coming? > > Not sure if more parts are coming - these are the only ones I am aware of. But I > have not checked thoroughly. I thought it better to be explicit, but I will > change the next one to an else: in v3 unless you change your mind. > > > > > > + then: > > > + properties: > > > + clocks: > > > + minItems: 1 > > > + maxItems: 1 > > > + clock-names: > > > + items: > > > + - const: xtal > > > + > > > + - if: > > > + properties: > > > + compatible: > > > + contains: > > > + const: silabs,si5351c > > > + then: > > > + properties: > > > + clocks: > > > + minItems: 1 > > > + maxItems: 2 > > > + clock-names: > > > + minItems: 1 > > > + items: > > > + - const: xtal > > > + - const: clkin > > > > Define clocks and clock-names at the top level and just use > > minItems/maxItems in the if/then schemas. > > I was trying to imply here that it is invalid to specify clkin for the former > three part types - only for the si5351c. If I specify both in the top-level > clock-names:items then it would allow something like this: > > clk { > compatible = "silabs,si5351a-msop"; > clocks = <&ref25>; > clock-names = "clkin"; /* not OK - Si5351A-MSOP only supports XTAL */ > }; What I'm saying will work. There are lots of examples in the tree. The top-level defines the full array and then the if/then schema just sets the max size to 1 so that the clkin entry is not allowed. properties: clock-names: minItems: 1 items: - const: xtal - const: clkin if: properties: compatible: contains: const: silabs,si5351a-msop then: properties: clock-names: maxItems: 1 (and then "minItems: 2" for the cases with 2 clocks) Rob
On Tue, Oct 10, 2023 at 09:50:03AM -0500, Rob Herring wrote: > On Wed, Oct 4, 2023 at 4:40 PM Alvin Šipraga <ALSI@bang-olufsen.dk> wrote: > > > > On Wed, Oct 04, 2023 at 09:39:37AM -0500, Rob Herring wrote: > > > > + silabs,multisynth-source: > > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > > + enum: [ 0, 1 ] > > > > + description: | > > > > > > Don't need '|' if no formatting to preserve. > > > > I thought the line would be too long otherwise. > > Column width is 80 in dt-schema as well, right? > > Yes, and up to 100 is fine as an exception. > > > > > > > > + Source PLL A (0) or B (1) for the corresponding multisynth divider. > > But this doesn't look like it is over 80. Maybe if you put after > 'description:' on the same line, but that's not what I said. It can > still be on the next line. No '|' just means the line endings aren't > fixed. Not important now, but if we were to generate pretty > documentation from the schemas, then it would matter. Ah I see. OK, I removed the | now and it works fine. :) > > > > > [...] > > > > > > + - if: > > > > + properties: > > > > + compatible: > > > > + contains: > > > > + enum: > > > > + - silabs,si5351a > > > > + - silabs,si5351a-msop > > > > + - silabs,si5351b > > > > > > Isn't this just the 'else' for the next one? Or more parts are coming? > > > > Not sure if more parts are coming - these are the only ones I am aware of. But I > > have not checked thoroughly. I thought it better to be explicit, but I will > > change the next one to an else: in v3 unless you change your mind. > > > > > > > > > + then: > > > > + properties: > > > > + clocks: > > > > + minItems: 1 > > > > + maxItems: 1 > > > > + clock-names: > > > > + items: > > > > + - const: xtal > > > > + > > > > + - if: > > > > + properties: > > > > + compatible: > > > > + contains: > > > > + const: silabs,si5351c > > > > + then: > > > > + properties: > > > > + clocks: > > > > + minItems: 1 > > > > + maxItems: 2 > > > > + clock-names: > > > > + minItems: 1 > > > > + items: > > > > + - const: xtal > > > > + - const: clkin > > > > > > Define clocks and clock-names at the top level and just use > > > minItems/maxItems in the if/then schemas. > > > > I was trying to imply here that it is invalid to specify clkin for the former > > three part types - only for the si5351c. If I specify both in the top-level > > clock-names:items then it would allow something like this: > > > > clk { > > compatible = "silabs,si5351a-msop"; > > clocks = <&ref25>; > > clock-names = "clkin"; /* not OK - Si5351A-MSOP only supports XTAL */ > > }; > > What I'm saying will work. There are lots of examples in the tree. The > top-level defines the full array and then the if/then schema just sets > the max size to 1 so that the clkin entry is not allowed. > > properties: > clock-names: > minItems: 1 > items: > - const: xtal > - const: clkin > > if: > properties: > compatible: > contains: > const: silabs,si5351a-msop > then: > properties: > clock-names: > maxItems: 1 > > (and then "minItems: 2" for the cases with 2 clocks) Thanks Rob, your suggestion worked! I will send a new version now. Kind regards, Alvin
From: Alvin Šipraga <alsi@bang-olufsen.dk> This series intends to address a problem I had when using the Si5351A as a runtime adjustable audio bit clock. The basic issue is that the driver in its current form unconditionally resets the PLL whenever adjusting its rate. But this reset causes an unwanted ~1.4 ms LOW signal glitch in the clock output. As a remedy, a new property is added to control the reset behaviour of the PLLs more precisely. In the process I also converted the bindings to YAML. Changes: v1 -> v2: - address Rob's comments on the two dt-bindings patches - new patch to correct the clock node names in the only upstream device tree using si5351 Alvin Šipraga (4): dt-bindings: clock: si5351: convert to yaml ARM: dts: dove-cubox: fix si5351 node names dt-bindings: clock: si5351: add PLL reset mode property clk: si5351: allow PLLs to be adjusted without reset .../bindings/clock/silabs,si5351.txt | 126 -------- .../bindings/clock/silabs,si5351.yaml | 277 ++++++++++++++++++ arch/arm/boot/dts/marvell/dove-cubox.dts | 4 +- drivers/clk/clk-si5351.c | 47 ++- include/linux/platform_data/si5351.h | 2 + 5 files changed, 325 insertions(+), 131 deletions(-) delete mode 100644 Documentation/devicetree/bindings/clock/silabs,si5351.txt create mode 100644 Documentation/devicetree/bindings/clock/silabs,si5351.yaml