On 31/05/2024 19:57, Hironori KIKUCHI wrote: > Hello, > >>> This patch adds new options to select a clock source and DIV_M register >>> value for each coupled PWM channels. >> >> Please do not use "This commit/patch/change", but imperative mood. See >> longer explanation here: >> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95 >> >> Bindings are before their users. This should not be last patch, because >> this implies there is no user. > > I'm sorry, I'll fix them. > >> This applies to all variants? Or the one you add? Confused... > > Apologies for confusing you. This applies to all variants. > >> >>> >>> Signed-off-by: Hironori KIKUCHI <kikuchan98@xxxxxxxxx> >>> --- >>> .../bindings/pwm/allwinner,sun20i-pwm.yaml | 19 +++++++++++++++++++ >>> 1 file changed, 19 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml >>> index b9b6d7e7c87..436a1d344ab 100644 >>> --- a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml >>> +++ b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml >>> @@ -45,6 +45,25 @@ properties: >>> description: The number of PWM channels configured for this instance >>> enum: [6, 9] >>> >>> + allwinner,pwm-pair-clock-sources: >>> + description: The clock source names for each PWM pair >>> + items: >>> + enum: [hosc, apb] >>> + minItems: 1 >>> + maxItems: 8 >> >> Missing type... and add 8 of such items to your example to make it complete. > > Thank you. I'll fix it. > >> >>> + >>> + allwinner,pwm-pair-clock-prescales: >>> + description: The prescale (DIV_M register) values for each PWM pair >>> + $ref: /schemas/types.yaml#/definitions/uint32-matrix >>> + items: >>> + items: >>> + minimum: 0 >>> + maximum: 8 >>> + minItems: 1 >>> + maxItems: 1 >>> + minItems: 1 >>> + maxItems: 8 >> >> This does not look like matrix but array. > > I wanted to specify values like this: > > allwinner,pwm-pair-clock-prescales = <0>, <1>, <3>; > allwinner,pwm-pair-clock-sources = "hosc", "apb", "hosc": > > These should correspond to each PWM pair. > This way, I thought we might be able to visually understand the relationship > between prescalers and sources, like clock-names and clocks. > > Is this notation uncommon, perhaps? It's still an array. > >> >> Why clock DIV cannot be deduced from typical PWM attributes + clock >> frequency? > > This SoC's PWM system has one shared prescaler and clock source for each pair > of PWM channels. I should have noted this earlier, sorry. > > Actually, the original v9 patch automatically deduced the DIV value > from the frequency. > However, because the two channels share a single prescaler, once one channel is > enabled, it affects and restricts the DIV value for the other channel > in the pair. > This introduces a problem of determining which channel should set the shared DIV > value. The original behavior was that the first channel enabled would win. There's nothing bad in this. > > Instead, this patch try to resolve the issue by specifying these > values for each PWM > pairs deterministically. > That's why it requires the new options. This does not solve that wrong divider can be programmed for second channel in each pair. Best regards, Krzysztof
