| Message ID | Y39PjU1BqBB8tZ98@makrotopia.org |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [RESEND,v2] dt-bindings: pwm: mediatek: Add compatible for MT7986 | expand |
| Context | Check | Description |
|---|---|---|
| robh/patch-applied | success | |
| robh/checkpatch | success |
On 24/11/2022 12:03, Daniel Golle wrote: > Add new compatible string for MT7986 PWM and list compatible units for > existing entries. Also make sure the number of pwm1-X clocks is listed > for all supported units. > > Signed-off-by: Daniel Golle <daniel@makrotopia.org> > --- > Changes since v1: list compatibles, fix pwm1-n clocks for all SoCs > > Rebased on linux-next and re-run scripts/get_maintainers.pl on patch to > makes sure dt maintainers are included. This has been requested by > Krzysztof Kozlowski. > > .../devicetree/bindings/pwm/pwm-mediatek.txt | 20 +++++++++++-------- > 1 file changed, 12 insertions(+), 8 deletions(-) > > diff --git a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt > index 554c96b6d0c3..952a338e06e7 100644 > --- a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt > +++ b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt > @@ -2,14 +2,15 @@ MediaTek PWM controller > > Required properties: > - compatible: should be "mediatek,<name>-pwm": > - - "mediatek,mt2712-pwm": found on mt2712 SoC. > + - "mediatek,mt2712-pwm", "mediatek,mt6795-pwm": found on mt2712 SoC. > - "mediatek,mt6795-pwm": found on mt6795 SoC. > - - "mediatek,mt7622-pwm": found on mt7622 SoC. > - - "mediatek,mt7623-pwm": found on mt7623 SoC. > + - "mediatek,mt7622-pwm", "mediatek,mt8195-pwm", "mediatek,mt8183-pwm", "mediatek,mt7986-pwm": found on mt7622 SoC. This does not look right. What you are saying is mt7622 is compatible with mt8195, which is compatible with mt8183, which is compatible with mt7986. It could be true, but I feel you wanted to say something else - mt7622 is compatible with one SoC which is generic and common to all other implementations. > + - "mediatek,mt7623-pwm", "mediatek,mt7628-pwm": found on mt7623 SoC. > - "mediatek,mt7628-pwm": found on mt7628 SoC. > - "mediatek,mt7629-pwm": found on mt7629 SoC. > - - "mediatek,mt8183-pwm": found on mt8183 SoC. > - - "mediatek,mt8195-pwm", "mediatek,mt8183-pwm": found on mt8195 SoC. > + - "mediatek,mt7986-pwm": found on mt7986 SoC. > + - "mediatek,mt8183-pwm", "mediatek,mt7986-pwm": found on mt8183 SoC. > + - "mediatek,mt8195-pwm", "mediatek,mt8183-pwm", "mediatek,mt7986-pwm": found on mt8195 SoC. This as well looks excessive. Best regards, Krzysztof
On Thu, Nov 24, 2022 at 12:30:44PM +0100, Krzysztof Kozlowski wrote: > On 24/11/2022 12:03, Daniel Golle wrote: > > Add new compatible string for MT7986 PWM and list compatible units for > > existing entries. Also make sure the number of pwm1-X clocks is listed > > for all supported units. > > > > Signed-off-by: Daniel Golle <daniel@makrotopia.org> > > --- > > Changes since v1: list compatibles, fix pwm1-n clocks for all SoCs > > > > Rebased on linux-next and re-run scripts/get_maintainers.pl on patch to > > makes sure dt maintainers are included. This has been requested by > > Krzysztof Kozlowski. > > > > .../devicetree/bindings/pwm/pwm-mediatek.txt | 20 +++++++++++-------- > > 1 file changed, 12 insertions(+), 8 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt > > index 554c96b6d0c3..952a338e06e7 100644 > > --- a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt > > +++ b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt > > @@ -2,14 +2,15 @@ MediaTek PWM controller > > > > Required properties: > > - compatible: should be "mediatek,<name>-pwm": > > - - "mediatek,mt2712-pwm": found on mt2712 SoC. > > + - "mediatek,mt2712-pwm", "mediatek,mt6795-pwm": found on mt2712 SoC. > > - "mediatek,mt6795-pwm": found on mt6795 SoC. > > - - "mediatek,mt7622-pwm": found on mt7622 SoC. > > - - "mediatek,mt7623-pwm": found on mt7623 SoC. > > + - "mediatek,mt7622-pwm", "mediatek,mt8195-pwm", "mediatek,mt8183-pwm", "mediatek,mt7986-pwm": found on mt7622 SoC. > > This does not look right. What you are saying is mt7622 is compatible > with mt8195, which is compatible with mt8183, which is compatible with > mt7986. It could be true, but I feel you wanted to say something else - > mt7622 is compatible with one SoC which is generic and common to all > other implementations. MT7622 has 6 PWM channels, it does have CK_26M_SEL register and does not need pwm45_fixup. Hence, when using a driver made for MT8195, only 4 out of 6 channels woukd work. MT8183 PWM is identical to MT8195, hence also compatible. When using driver for MT7986, only 2 channels would work, but otherwise it is also compatible. So unfortunately, that one generic implementation ("common ancestor") does not exist and development of the PWM unit found in MediaTek SoCs did not necessarily increase features in more recent iterations, but rather just reduce or increase the number of PWM channels available. Ironically, the unit with least features (only 2 channels) is found in the most recent SoC (MT7986). > > > + - "mediatek,mt7623-pwm", "mediatek,mt7628-pwm": found on mt7623 SoC. > > - "mediatek,mt7628-pwm": found on mt7628 SoC. > > - "mediatek,mt7629-pwm": found on mt7629 SoC. > > - - "mediatek,mt8183-pwm": found on mt8183 SoC. > > - - "mediatek,mt8195-pwm", "mediatek,mt8183-pwm": found on mt8195 SoC. > > + - "mediatek,mt7986-pwm": found on mt7986 SoC. > > + - "mediatek,mt8183-pwm", "mediatek,mt7986-pwm": found on mt8183 SoC. > > + - "mediatek,mt8195-pwm", "mediatek,mt8183-pwm", "mediatek,mt7986-pwm": found on mt8195 SoC. > > This as well looks excessive. I agree. But it's difficult to say which one should be ommitted.
On 24/11/2022 13:11, Daniel Golle wrote: > On Thu, Nov 24, 2022 at 12:30:44PM +0100, Krzysztof Kozlowski wrote: >> On 24/11/2022 12:03, Daniel Golle wrote: >>> Add new compatible string for MT7986 PWM and list compatible units for >>> existing entries. Also make sure the number of pwm1-X clocks is listed >>> for all supported units. >>> >>> Signed-off-by: Daniel Golle <daniel@makrotopia.org> >>> --- >>> Changes since v1: list compatibles, fix pwm1-n clocks for all SoCs >>> >>> Rebased on linux-next and re-run scripts/get_maintainers.pl on patch to >>> makes sure dt maintainers are included. This has been requested by >>> Krzysztof Kozlowski. >>> >>> .../devicetree/bindings/pwm/pwm-mediatek.txt | 20 +++++++++++-------- >>> 1 file changed, 12 insertions(+), 8 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt >>> index 554c96b6d0c3..952a338e06e7 100644 >>> --- a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt >>> +++ b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt >>> @@ -2,14 +2,15 @@ MediaTek PWM controller >>> >>> Required properties: >>> - compatible: should be "mediatek,<name>-pwm": >>> - - "mediatek,mt2712-pwm": found on mt2712 SoC. >>> + - "mediatek,mt2712-pwm", "mediatek,mt6795-pwm": found on mt2712 SoC. >>> - "mediatek,mt6795-pwm": found on mt6795 SoC. >>> - - "mediatek,mt7622-pwm": found on mt7622 SoC. >>> - - "mediatek,mt7623-pwm": found on mt7623 SoC. >>> + - "mediatek,mt7622-pwm", "mediatek,mt8195-pwm", "mediatek,mt8183-pwm", "mediatek,mt7986-pwm": found on mt7622 SoC. >> >> This does not look right. What you are saying is mt7622 is compatible >> with mt8195, which is compatible with mt8183, which is compatible with >> mt7986. It could be true, but I feel you wanted to say something else - >> mt7622 is compatible with one SoC which is generic and common to all >> other implementations. > > MT7622 has 6 PWM channels, it does have CK_26M_SEL register and does > not need pwm45_fixup. Hence, when using a driver made for MT8195, only > 4 out of 6 channels woukd work. MT8183 PWM is identical to MT8195, > hence also compatible. When using driver for MT7986, only 2 channels > would work, but otherwise it is also compatible. > > So unfortunately, that one generic implementation ("common ancestor") > does not exist and development of the PWM unit found in MediaTek SoCs > did not necessarily increase features in more recent iterations, but > rather just reduce or increase the number of PWM channels available. > Ironically, the unit with least features (only 2 channels) is found in > the most recent SoC (MT7986). None of these explain listing four compatibles. > >> >>> + - "mediatek,mt7623-pwm", "mediatek,mt7628-pwm": found on mt7623 SoC. >>> - "mediatek,mt7628-pwm": found on mt7628 SoC. >>> - "mediatek,mt7629-pwm": found on mt7629 SoC. >>> - - "mediatek,mt8183-pwm": found on mt8183 SoC. >>> - - "mediatek,mt8195-pwm", "mediatek,mt8183-pwm": found on mt8195 SoC. >>> + - "mediatek,mt7986-pwm": found on mt7986 SoC. >>> + - "mediatek,mt8183-pwm", "mediatek,mt7986-pwm": found on mt8183 SoC. >>> + - "mediatek,mt8195-pwm", "mediatek,mt8183-pwm", "mediatek,mt7986-pwm": found on mt8195 SoC. >> >> This as well looks excessive. > > I agree. But it's difficult to say which one should be ommitted. Best regards, Krzysztof
Hi Krzysztof, On Thu, Nov 24, 2022 at 02:33:35PM +0100, Krzysztof Kozlowski wrote: > On 24/11/2022 13:11, Daniel Golle wrote: > > On Thu, Nov 24, 2022 at 12:30:44PM +0100, Krzysztof Kozlowski wrote: > >> On 24/11/2022 12:03, Daniel Golle wrote: > >>> Add new compatible string for MT7986 PWM and list compatible units for > >>> existing entries. Also make sure the number of pwm1-X clocks is listed > >>> for all supported units. > >>> > >>> Signed-off-by: Daniel Golle <daniel@makrotopia.org> > >>> --- > >>> Changes since v1: list compatibles, fix pwm1-n clocks for all SoCs > >>> > >>> Rebased on linux-next and re-run scripts/get_maintainers.pl on patch to > >>> makes sure dt maintainers are included. This has been requested by > >>> Krzysztof Kozlowski. > >>> > >>> .../devicetree/bindings/pwm/pwm-mediatek.txt | 20 +++++++++++-------- > >>> 1 file changed, 12 insertions(+), 8 deletions(-) > >>> > >>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt > >>> index 554c96b6d0c3..952a338e06e7 100644 > >>> --- a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt > >>> +++ b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt > >>> @@ -2,14 +2,15 @@ MediaTek PWM controller > >>> > >>> Required properties: > >>> - compatible: should be "mediatek,<name>-pwm": > >>> - - "mediatek,mt2712-pwm": found on mt2712 SoC. > >>> + - "mediatek,mt2712-pwm", "mediatek,mt6795-pwm": found on mt2712 SoC. > >>> - "mediatek,mt6795-pwm": found on mt6795 SoC. > >>> - - "mediatek,mt7622-pwm": found on mt7622 SoC. > >>> - - "mediatek,mt7623-pwm": found on mt7623 SoC. > >>> + - "mediatek,mt7622-pwm", "mediatek,mt8195-pwm", "mediatek,mt8183-pwm", "mediatek,mt7986-pwm": found on mt7622 SoC. > >> > >> This does not look right. What you are saying is mt7622 is compatible > >> with mt8195, which is compatible with mt8183, which is compatible with > >> mt7986. It could be true, but I feel you wanted to say something else - > >> mt7622 is compatible with one SoC which is generic and common to all > >> other implementations. > > > > MT7622 has 6 PWM channels, it does have CK_26M_SEL register and does > > not need pwm45_fixup. Hence, when using a driver made for MT8195, only > > 4 out of 6 channels woukd work. MT8183 PWM is identical to MT8195, > > hence also compatible. When using driver for MT7986, only 2 channels > > would work, but otherwise it is also compatible. > > > > So unfortunately, that one generic implementation ("common ancestor") > > does not exist and development of the PWM unit found in MediaTek SoCs > > did not necessarily increase features in more recent iterations, but > > rather just reduce or increase the number of PWM channels available. > > Ironically, the unit with least features (only 2 channels) is found in > > the most recent SoC (MT7986). > > None of these explain listing four compatibles. So do I understand correctly that in this case only the newly introduced "mediatek,mt7986-pwm" should be listed as more generic compatible after the more specific "mediatek,mt7622-pwm", everything in between should be dropped? Or only drop "mediatek,mt8195-pwm" here? I'm asking because in your message from 23/10/2022 you were quoting Devicetree specification: > "The property value consists of a concatenated list of null terminated > strings, from most specific to most general. They allow a device to > express its compatibility with a family of similar devices, potentially > allowing a single device driver to match against several devices." And we have discussed in great length (as I had misunderstood it) that this should mean that units with the lowest number of channels are to be considered the "most general" if otherwise identical. > > > > >> > >>> + - "mediatek,mt7623-pwm", "mediatek,mt7628-pwm": found on mt7623 SoC. > >>> - "mediatek,mt7628-pwm": found on mt7628 SoC. > >>> - "mediatek,mt7629-pwm": found on mt7629 SoC. > >>> - - "mediatek,mt8183-pwm": found on mt8183 SoC. > >>> - - "mediatek,mt8195-pwm", "mediatek,mt8183-pwm": found on mt8195 SoC. > >>> + - "mediatek,mt7986-pwm": found on mt7986 SoC. > >>> + - "mediatek,mt8183-pwm", "mediatek,mt7986-pwm": found on mt8183 SoC. > >>> + - "mediatek,mt8195-pwm", "mediatek,mt8183-pwm", "mediatek,mt7986-pwm": found on mt8195 SoC. > >> > >> This as well looks excessive. > > > > I agree. But it's difficult to say which one should be ommitted. So are you suggesting to drop the "mediatek,mt8183-pwm" string here? Thank you for your patience! Daniel
On 24/11/2022 21:00, Daniel Golle wrote: > Hi Krzysztof, > > On Thu, Nov 24, 2022 at 02:33:35PM +0100, Krzysztof Kozlowski wrote: >> On 24/11/2022 13:11, Daniel Golle wrote: >>> On Thu, Nov 24, 2022 at 12:30:44PM +0100, Krzysztof Kozlowski wrote: >>>> On 24/11/2022 12:03, Daniel Golle wrote: >>>>> Add new compatible string for MT7986 PWM and list compatible units for >>>>> existing entries. Also make sure the number of pwm1-X clocks is listed >>>>> for all supported units. >>>>> >>>>> Signed-off-by: Daniel Golle <daniel@makrotopia.org> >>>>> --- >>>>> Changes since v1: list compatibles, fix pwm1-n clocks for all SoCs >>>>> >>>>> Rebased on linux-next and re-run scripts/get_maintainers.pl on patch to >>>>> makes sure dt maintainers are included. This has been requested by >>>>> Krzysztof Kozlowski. >>>>> >>>>> .../devicetree/bindings/pwm/pwm-mediatek.txt | 20 +++++++++++-------- >>>>> 1 file changed, 12 insertions(+), 8 deletions(-) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt >>>>> index 554c96b6d0c3..952a338e06e7 100644 >>>>> --- a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt >>>>> +++ b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt >>>>> @@ -2,14 +2,15 @@ MediaTek PWM controller >>>>> >>>>> Required properties: >>>>> - compatible: should be "mediatek,<name>-pwm": >>>>> - - "mediatek,mt2712-pwm": found on mt2712 SoC. >>>>> + - "mediatek,mt2712-pwm", "mediatek,mt6795-pwm": found on mt2712 SoC. >>>>> - "mediatek,mt6795-pwm": found on mt6795 SoC. >>>>> - - "mediatek,mt7622-pwm": found on mt7622 SoC. >>>>> - - "mediatek,mt7623-pwm": found on mt7623 SoC. >>>>> + - "mediatek,mt7622-pwm", "mediatek,mt8195-pwm", "mediatek,mt8183-pwm", "mediatek,mt7986-pwm": found on mt7622 SoC. >>>> >>>> This does not look right. What you are saying is mt7622 is compatible >>>> with mt8195, which is compatible with mt8183, which is compatible with >>>> mt7986. It could be true, but I feel you wanted to say something else - >>>> mt7622 is compatible with one SoC which is generic and common to all >>>> other implementations. >>> >>> MT7622 has 6 PWM channels, it does have CK_26M_SEL register and does >>> not need pwm45_fixup. Hence, when using a driver made for MT8195, only >>> 4 out of 6 channels woukd work. MT8183 PWM is identical to MT8195, >>> hence also compatible. When using driver for MT7986, only 2 channels >>> would work, but otherwise it is also compatible. >>> >>> So unfortunately, that one generic implementation ("common ancestor") >>> does not exist and development of the PWM unit found in MediaTek SoCs >>> did not necessarily increase features in more recent iterations, but >>> rather just reduce or increase the number of PWM channels available. >>> Ironically, the unit with least features (only 2 channels) is found in >>> the most recent SoC (MT7986). >> >> None of these explain listing four compatibles. > > So do I understand correctly that in this case only the newly > introduced "mediatek,mt7986-pwm" should be listed as more generic > compatible after the more specific "mediatek,mt7622-pwm", everything in > between should be dropped? Or only drop "mediatek,mt8195-pwm" here? I know nothing about Mediatek PWM and which one is more generic than others. The patch submitter should rather know... > > I'm asking because in your message from 23/10/2022 you were quoting > Devicetree specification: >> "The property value consists of a concatenated list of null terminated >> strings, from most specific to most general. They allow a device to >> express its compatibility with a family of similar devices, potentially >> allowing a single device driver to match against several devices." > > And we have discussed in great length (as I had misunderstood it) that > this should mean that units with the lowest number of channels are to > be considered the "most general" if otherwise identical. Yes, but we do not discuss this part. In most cases you have one generic device and several devices compatible with it. You on the other hand created here one device compatible with three other devices! And that raised all my questions. > > >> >>> >>>> >>>>> + - "mediatek,mt7623-pwm", "mediatek,mt7628-pwm": found on mt7623 SoC. >>>>> - "mediatek,mt7628-pwm": found on mt7628 SoC. >>>>> - "mediatek,mt7629-pwm": found on mt7629 SoC. >>>>> - - "mediatek,mt8183-pwm": found on mt8183 SoC. >>>>> - - "mediatek,mt8195-pwm", "mediatek,mt8183-pwm": found on mt8195 SoC. >>>>> + - "mediatek,mt7986-pwm": found on mt7986 SoC. >>>>> + - "mediatek,mt8183-pwm", "mediatek,mt7986-pwm": found on mt8183 SoC. >>>>> + - "mediatek,mt8195-pwm", "mediatek,mt8183-pwm", "mediatek,mt7986-pwm": found on mt8195 SoC. >>>> >>>> This as well looks excessive. >>> >>> I agree. But it's difficult to say which one should be ommitted. > > So are you suggesting to drop the "mediatek,mt8183-pwm" string here? Yes. Why mt183 is even there? commit msg was not explaining it. Best regards, Krzysztof
Il 24/11/22 12:03, Daniel Golle ha scritto: > Add new compatible string for MT7986 PWM and list compatible units for > existing entries. Also make sure the number of pwm1-X clocks is listed > for all supported units. > > Signed-off-by: Daniel Golle <daniel@makrotopia.org> > --- > Changes since v1: list compatibles, fix pwm1-n clocks for all SoCs > > Rebased on linux-next and re-run scripts/get_maintainers.pl on patch to > makes sure dt maintainers are included. This has been requested by > Krzysztof Kozlowski. > > .../devicetree/bindings/pwm/pwm-mediatek.txt | 20 +++++++++++-------- > 1 file changed, 12 insertions(+), 8 deletions(-) > > diff --git a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt > index 554c96b6d0c3..952a338e06e7 100644 > --- a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt > +++ b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt > @@ -2,14 +2,15 @@ MediaTek PWM controller > > Required properties: > - compatible: should be "mediatek,<name>-pwm": > - - "mediatek,mt2712-pwm": found on mt2712 SoC. > + - "mediatek,mt2712-pwm", "mediatek,mt6795-pwm": found on mt2712 SoC. > - "mediatek,mt6795-pwm": found on mt6795 SoC. > - - "mediatek,mt7622-pwm": found on mt7622 SoC. > - - "mediatek,mt7623-pwm": found on mt7623 SoC. > + - "mediatek,mt7622-pwm", "mediatek,mt8195-pwm", "mediatek,mt8183-pwm", "mediatek,mt7986-pwm": found on mt7622 SoC. > + - "mediatek,mt7623-pwm", "mediatek,mt7628-pwm": found on mt7623 SoC. > - "mediatek,mt7628-pwm": found on mt7628 SoC. > - "mediatek,mt7629-pwm": found on mt7629 SoC. > - - "mediatek,mt8183-pwm": found on mt8183 SoC. > - - "mediatek,mt8195-pwm", "mediatek,mt8183-pwm": found on mt8195 SoC. > + - "mediatek,mt7986-pwm": found on mt7986 SoC. > + - "mediatek,mt8183-pwm", "mediatek,mt7986-pwm": found on mt8183 SoC. > + - "mediatek,mt8195-pwm", "mediatek,mt8183-pwm", "mediatek,mt7986-pwm": found on mt8195 SoC. I'm sorry, but all these compatibles make little sense at best. Each of these PWM controllers have different properties as they may *by hardware* be featuring more or less channels, they may be a different IP revision and/or sub-revision requiring even ever-so-slightly different handling (check pwm45_fixup and has_ck_26m_sel). If you want to add MT7986, the best thing that you can do is to simply add + - "mediatek,mt7986-pwm": found on mt7986 SoC. this line ^ ...and then please don't touch the others. > - "mediatek,mt8365-pwm": found on mt8365 SoC. > - "mediatek,mt8516-pwm": found on mt8516 SoC. > - reg: physical base address and length of the controller's registers. > @@ -20,11 +21,14 @@ Required properties: > has no clocks > - "top": the top clock generator > - "main": clock used by the PWM core > + - "pwm1" : the PWM1 clock for mt7629 > + - "pwm1-2": the two per PWM clocks for mt7986 That's not your fault, but the binding is already wrong (yes it must be fixed!) and unless my brain is failing somewhere, there's only one clock per pwm (as if there's any children, it must be parented to .. well, its parent, in the clock driver), and note that the driver is actually parsing "pwmX" clocks, never "pwmX-Y" clocks. Relevant snippet: char name[8]; snprintf(name, sizeof(name), "pwm%d", i + 1); pc->clk_pwms[i] = devm_clk_get(&pdev->dev, name); Just... please don't keep doing the same mistake that is already inside of here... So, coming to an end: I think that this commit should be a one-liner that documents your "mediatek,mt7986-pwm" compatible and that's it. A schema conversion would be welcome: in that regard, I can make a conversion and send it next week, along with that clock-names fix. Regards, Angelo
On Fri, Nov 25, 2022 at 12:12:17PM +0100, AngeloGioacchino Del Regno wrote: > Il 24/11/22 12:03, Daniel Golle ha scritto: > > Add new compatible string for MT7986 PWM and list compatible units for > > existing entries. Also make sure the number of pwm1-X clocks is listed > > for all supported units. > > > > Signed-off-by: Daniel Golle <daniel@makrotopia.org> > > --- > > Changes since v1: list compatibles, fix pwm1-n clocks for all SoCs > > > > Rebased on linux-next and re-run scripts/get_maintainers.pl on patch to > > makes sure dt maintainers are included. This has been requested by > > Krzysztof Kozlowski. > > > > .../devicetree/bindings/pwm/pwm-mediatek.txt | 20 +++++++++++-------- > > 1 file changed, 12 insertions(+), 8 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt > > index 554c96b6d0c3..952a338e06e7 100644 > > --- a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt > > +++ b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt > > @@ -2,14 +2,15 @@ MediaTek PWM controller > > Required properties: > > - compatible: should be "mediatek,<name>-pwm": > > - - "mediatek,mt2712-pwm": found on mt2712 SoC. > > + - "mediatek,mt2712-pwm", "mediatek,mt6795-pwm": found on mt2712 SoC. > > - "mediatek,mt6795-pwm": found on mt6795 SoC. > > - - "mediatek,mt7622-pwm": found on mt7622 SoC. > > - - "mediatek,mt7623-pwm": found on mt7623 SoC. > > + - "mediatek,mt7622-pwm", "mediatek,mt8195-pwm", "mediatek,mt8183-pwm", "mediatek,mt7986-pwm": found on mt7622 SoC. > > + - "mediatek,mt7623-pwm", "mediatek,mt7628-pwm": found on mt7623 SoC. > > - "mediatek,mt7628-pwm": found on mt7628 SoC. > > - "mediatek,mt7629-pwm": found on mt7629 SoC. > > - - "mediatek,mt8183-pwm": found on mt8183 SoC. > > - - "mediatek,mt8195-pwm", "mediatek,mt8183-pwm": found on mt8195 SoC. > > + - "mediatek,mt7986-pwm": found on mt7986 SoC. > > + - "mediatek,mt8183-pwm", "mediatek,mt7986-pwm": found on mt8183 SoC. > > + - "mediatek,mt8195-pwm", "mediatek,mt8183-pwm", "mediatek,mt7986-pwm": found on mt8195 SoC. > > I'm sorry, but all these compatibles make little sense at best. > Each of these PWM controllers have different properties as they may *by hardware* > be featuring more or less channels, they may be a different IP revision and/or > sub-revision requiring even ever-so-slightly different handling (check pwm45_fixup > and has_ck_26m_sel). I did check the two pwm45_fixup and has_ck_26m_sel for each supported compatible and listed them accordingly. Ie. the ones listed as compatible in this submission really only differ by the number of channels offered from the driver's point of view and share the same boolean values for pwm45_fixup and has_ck_26m_sel. > > If you want to add MT7986, the best thing that you can do is to simply add > > + - "mediatek,mt7986-pwm": found on mt7986 SoC. > > this line ^ > > ...and then please don't touch the others. > > > - "mediatek,mt8365-pwm": found on mt8365 SoC. > > - "mediatek,mt8516-pwm": found on mt8516 SoC. > > - reg: physical base address and length of the controller's registers. > > @@ -20,11 +21,14 @@ Required properties: > > has no clocks > > - "top": the top clock generator > > - "main": clock used by the PWM core > > + - "pwm1" : the PWM1 clock for mt7629 > > + - "pwm1-2": the two per PWM clocks for mt7986 > > That's not your fault, but the binding is already wrong (yes it must be fixed!) and > unless my brain is failing somewhere, there's only one clock per pwm (as if there's > any children, it must be parented to .. well, its parent, in the clock driver), and > note that the driver is actually parsing "pwmX" clocks, never "pwmX-Y" clocks. > > Relevant snippet: > > char name[8]; > > snprintf(name, sizeof(name), "pwm%d", i + 1); > > pc->clk_pwms[i] = devm_clk_get(&pdev->dev, name); > > Just... please don't keep doing the same mistake that is already inside of here... Maybe better to write "pwm1"..."pwm2" or something like that... But I just won't bother with it if you are planning to convert this to YAML schema anyway. > > So, coming to an end: I think that this commit should be a one-liner that documents > your "mediatek,mt7986-pwm" compatible and that's it. > This is exactly what I did in the first submission of this patch: https://www.spinics.net/lists/devicetree/msg542568.html Should I just re-submit this then? > A schema conversion would be welcome: in that regard, I can make a conversion > and send it next week, along with that clock-names fix. Should I wait for you doing that and then re-submit or just re-submit immediately? Thank you! Daniel
Il 25/11/22 12:34, Daniel Golle ha scritto: > On Fri, Nov 25, 2022 at 12:12:17PM +0100, AngeloGioacchino Del Regno wrote: >> Il 24/11/22 12:03, Daniel Golle ha scritto: >>> Add new compatible string for MT7986 PWM and list compatible units for >>> existing entries. Also make sure the number of pwm1-X clocks is listed >>> for all supported units. >>> >>> Signed-off-by: Daniel Golle <daniel@makrotopia.org> >>> --- >>> Changes since v1: list compatibles, fix pwm1-n clocks for all SoCs >>> >>> Rebased on linux-next and re-run scripts/get_maintainers.pl on patch to >>> makes sure dt maintainers are included. This has been requested by >>> Krzysztof Kozlowski. >>> >>> .../devicetree/bindings/pwm/pwm-mediatek.txt | 20 +++++++++++-------- >>> 1 file changed, 12 insertions(+), 8 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt >>> index 554c96b6d0c3..952a338e06e7 100644 >>> --- a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt >>> +++ b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt >>> @@ -2,14 +2,15 @@ MediaTek PWM controller >>> Required properties: >>> - compatible: should be "mediatek,<name>-pwm": >>> - - "mediatek,mt2712-pwm": found on mt2712 SoC. >>> + - "mediatek,mt2712-pwm", "mediatek,mt6795-pwm": found on mt2712 SoC. >>> - "mediatek,mt6795-pwm": found on mt6795 SoC. >>> - - "mediatek,mt7622-pwm": found on mt7622 SoC. >>> - - "mediatek,mt7623-pwm": found on mt7623 SoC. >>> + - "mediatek,mt7622-pwm", "mediatek,mt8195-pwm", "mediatek,mt8183-pwm", "mediatek,mt7986-pwm": found on mt7622 SoC. >>> + - "mediatek,mt7623-pwm", "mediatek,mt7628-pwm": found on mt7623 SoC. >>> - "mediatek,mt7628-pwm": found on mt7628 SoC. >>> - "mediatek,mt7629-pwm": found on mt7629 SoC. >>> - - "mediatek,mt8183-pwm": found on mt8183 SoC. >>> - - "mediatek,mt8195-pwm", "mediatek,mt8183-pwm": found on mt8195 SoC. >>> + - "mediatek,mt7986-pwm": found on mt7986 SoC. >>> + - "mediatek,mt8183-pwm", "mediatek,mt7986-pwm": found on mt8183 SoC. >>> + - "mediatek,mt8195-pwm", "mediatek,mt8183-pwm", "mediatek,mt7986-pwm": found on mt8195 SoC. >> >> I'm sorry, but all these compatibles make little sense at best. >> Each of these PWM controllers have different properties as they may *by hardware* >> be featuring more or less channels, they may be a different IP revision and/or >> sub-revision requiring even ever-so-slightly different handling (check pwm45_fixup >> and has_ck_26m_sel). > > I did check the two pwm45_fixup and has_ck_26m_sel for each supported > compatible and listed them accordingly. > Ie. the ones listed as compatible in this submission really only differ > by the number of channels offered from the driver's point of view and > share the same boolean values for pwm45_fixup and has_ck_26m_sel. > >> >> If you want to add MT7986, the best thing that you can do is to simply add >> >> + - "mediatek,mt7986-pwm": found on mt7986 SoC. >> >> this line ^ >> >> ...and then please don't touch the others. >> >>> - "mediatek,mt8365-pwm": found on mt8365 SoC. >>> - "mediatek,mt8516-pwm": found on mt8516 SoC. >>> - reg: physical base address and length of the controller's registers. >>> @@ -20,11 +21,14 @@ Required properties: >>> has no clocks >>> - "top": the top clock generator >>> - "main": clock used by the PWM core >>> + - "pwm1" : the PWM1 clock for mt7629 >>> + - "pwm1-2": the two per PWM clocks for mt7986 >> >> That's not your fault, but the binding is already wrong (yes it must be fixed!) and >> unless my brain is failing somewhere, there's only one clock per pwm (as if there's >> any children, it must be parented to .. well, its parent, in the clock driver), and >> note that the driver is actually parsing "pwmX" clocks, never "pwmX-Y" clocks. >> >> Relevant snippet: >> >> char name[8]; >> >> snprintf(name, sizeof(name), "pwm%d", i + 1); >> >> pc->clk_pwms[i] = devm_clk_get(&pdev->dev, name); >> >> Just... please don't keep doing the same mistake that is already inside of here... > > Maybe better to write "pwm1"..."pwm2" or something like that... > But I just won't bother with it if you are planning to convert this to > YAML schema anyway. > >> >> So, coming to an end: I think that this commit should be a one-liner that documents >> your "mediatek,mt7986-pwm" compatible and that's it. >> > > This is exactly what I did in the first submission of this patch: > https://www.spinics.net/lists/devicetree/msg542568.html > > Should I just re-submit this then? > > >> A schema conversion would be welcome: in that regard, I can make a conversion >> and send it next week, along with that clock-names fix. > > Should I wait for you doing that and then re-submit or just re-submit > immediately? > I'm sorry but I forgot to Cc you on the schema conversion that I've just sent... Find it here: https://lore.kernel.org/linux-mediatek/20221128112028.58021-1-angelogioacchino.delregno@collabora.com/ ...and yes at this point you should document mt7986 on the new yaml, but please wait for some feedback on that series before doing anything so that you won't find yourself rebasing over and over. Cheers, Angelo
diff --git a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt index 554c96b6d0c3..952a338e06e7 100644 --- a/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt +++ b/Documentation/devicetree/bindings/pwm/pwm-mediatek.txt @@ -2,14 +2,15 @@ MediaTek PWM controller Required properties: - compatible: should be "mediatek,<name>-pwm": - - "mediatek,mt2712-pwm": found on mt2712 SoC. + - "mediatek,mt2712-pwm", "mediatek,mt6795-pwm": found on mt2712 SoC. - "mediatek,mt6795-pwm": found on mt6795 SoC. - - "mediatek,mt7622-pwm": found on mt7622 SoC. - - "mediatek,mt7623-pwm": found on mt7623 SoC. + - "mediatek,mt7622-pwm", "mediatek,mt8195-pwm", "mediatek,mt8183-pwm", "mediatek,mt7986-pwm": found on mt7622 SoC. + - "mediatek,mt7623-pwm", "mediatek,mt7628-pwm": found on mt7623 SoC. - "mediatek,mt7628-pwm": found on mt7628 SoC. - "mediatek,mt7629-pwm": found on mt7629 SoC. - - "mediatek,mt8183-pwm": found on mt8183 SoC. - - "mediatek,mt8195-pwm", "mediatek,mt8183-pwm": found on mt8195 SoC. + - "mediatek,mt7986-pwm": found on mt7986 SoC. + - "mediatek,mt8183-pwm", "mediatek,mt7986-pwm": found on mt8183 SoC. + - "mediatek,mt8195-pwm", "mediatek,mt8183-pwm", "mediatek,mt7986-pwm": found on mt8195 SoC. - "mediatek,mt8365-pwm": found on mt8365 SoC. - "mediatek,mt8516-pwm": found on mt8516 SoC. - reg: physical base address and length of the controller's registers. @@ -20,11 +21,14 @@ Required properties: has no clocks - "top": the top clock generator - "main": clock used by the PWM core + - "pwm1" : the PWM1 clock for mt7629 + - "pwm1-2": the two per PWM clocks for mt7986 - "pwm1-3": the three per PWM clocks for mt8365 - - "pwm1-8": the eight per PWM clocks for mt2712 + - "pwm1-4": the four per PWM clocks for mt7628 or mt8183 + - "pwm1-5": the five per PWM clocks for mt7623 or mt8516 - "pwm1-6": the six per PWM clocks for mt7622 - - "pwm1-5": the five per PWM clocks for mt7623 - - "pwm1" : the PWM1 clock for mt7629 + - "pwm1-7": the seven per PWM clocks for mt6795 + - "pwm1-8": the eight per PWM clocks for mt2712 - pinctrl-names: Must contain a "default" entry. - pinctrl-0: One property must exist for each entry in pinctrl-names. See pinctrl/pinctrl-bindings.txt for details of the property values.
Add new compatible string for MT7986 PWM and list compatible units for existing entries. Also make sure the number of pwm1-X clocks is listed for all supported units. Signed-off-by: Daniel Golle <daniel@makrotopia.org> --- Changes since v1: list compatibles, fix pwm1-n clocks for all SoCs Rebased on linux-next and re-run scripts/get_maintainers.pl on patch to makes sure dt maintainers are included. This has been requested by Krzysztof Kozlowski. .../devicetree/bindings/pwm/pwm-mediatek.txt | 20 +++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)