| Message ID | 35f43a8cfc32b5a065e4a04eb6cc6abf311f2700.1681370153.git.zhoubinbin@loongson.cn |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | rtc: ls2x: Add support for the Loongson-2K/LS7A RTC | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On 13/04/2023 09:57, Binbin Zhou wrote: > The LS2X RTC alarm depends on the associated registers in a separate > power management domain. > > In order to define the PM domain addresses of the different chips, a > more detailed description of compatible is required. This does not match your diff at all. > > Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn> > --- > Documentation/devicetree/bindings/rtc/trivial-rtc.yaml | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml b/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml > index a3603e638c37..2928811b83a0 100644 > --- a/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml > +++ b/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml > @@ -47,8 +47,11 @@ properties: > - isil,isl1218 > # Intersil ISL12022 Real-time Clock > - isil,isl12022 > - # Loongson-2K Socs/LS7A bridge Real-time Clock > - - loongson,ls2x-rtc Why removing it? > + # Loongson LS7A bridge Real-time Clock > + - loongson,ls7a-rtc > + # Loongson-2K Socs Real-time Clock > + - loongson,ls2k0500-rtc > + - loongson,ls2k1000-rtc That's even more surprising... I don't understand what you are doing here at all. Best regards, Krzysztof
On Mon, Apr 17, 2023 at 1:31 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 13/04/2023 09:57, Binbin Zhou wrote: > > The LS2X RTC alarm depends on the associated registers in a separate > > power management domain. > > > > In order to define the PM domain addresses of the different chips, a > > more detailed description of compatible is required. > > This does not match your diff at all. > > > > > Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn> > > --- > > Documentation/devicetree/bindings/rtc/trivial-rtc.yaml | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml b/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml > > index a3603e638c37..2928811b83a0 100644 > > --- a/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml > > +++ b/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml > > @@ -47,8 +47,11 @@ properties: > > - isil,isl1218 > > # Intersil ISL12022 Real-time Clock > > - isil,isl12022 > > - # Loongson-2K Socs/LS7A bridge Real-time Clock > > - - loongson,ls2x-rtc > > Why removing it? > > > + # Loongson LS7A bridge Real-time Clock > > + - loongson,ls7a-rtc > > + # Loongson-2K Socs Real-time Clock > > + - loongson,ls2k0500-rtc > > + - loongson,ls2k1000-rtc > > That's even more surprising... > > I don't understand what you are doing here at all. Hi Krzysztof: Sorry, maybe my description was not accurate. Looking back at my V2 patchset, the first patch was to add ls2x-rtc compatible. (https://lore.kernel.org/linux-rtc/0288efeb4209e4a49af07de6399045aaa00a970c.1673227292.git.zhoubinbin@loongson.cn/) In the process of modifying the V2 patchset, it was discovered that the ACPI domain offset addresses on some of the socs (LS2K1000) were different and I wanted to differentiate them by soc compatible. So I was going to drop the ls2x-rtc compatible directly from the V3 patch set. However, when I rebased the V3 patchset, I found that the previous ls2x-rtc compatible patch had been merged (commit 473a8ce756fd). So I had to remove it and add soc compatible. How about the following description: Since commit 473a8ce756fd (dt-bindings: rtc: Add Loongson LS2X RTC support), the ls2x-rtc compatible has been added. But the specific soc compatible is needed to be used to define different ACPI domain offset addresses. Thanks. Binbin > > Best regards, > Krzysztof > >
On 19/04/2023 09:12, Binbin Zhou wrote: > On Mon, Apr 17, 2023 at 1:31 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 13/04/2023 09:57, Binbin Zhou wrote: >>> The LS2X RTC alarm depends on the associated registers in a separate >>> power management domain. >>> >>> In order to define the PM domain addresses of the different chips, a >>> more detailed description of compatible is required. >> >> This does not match your diff at all. >> >>> >>> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn> >>> --- >>> Documentation/devicetree/bindings/rtc/trivial-rtc.yaml | 7 +++++-- >>> 1 file changed, 5 insertions(+), 2 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml b/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml >>> index a3603e638c37..2928811b83a0 100644 >>> --- a/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml >>> +++ b/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml >>> @@ -47,8 +47,11 @@ properties: >>> - isil,isl1218 >>> # Intersil ISL12022 Real-time Clock >>> - isil,isl12022 >>> - # Loongson-2K Socs/LS7A bridge Real-time Clock >>> - - loongson,ls2x-rtc >> >> Why removing it? >> >>> + # Loongson LS7A bridge Real-time Clock >>> + - loongson,ls7a-rtc >>> + # Loongson-2K Socs Real-time Clock >>> + - loongson,ls2k0500-rtc >>> + - loongson,ls2k1000-rtc >> >> That's even more surprising... >> >> I don't understand what you are doing here at all. > Hi Krzysztof: > > Sorry, maybe my description was not accurate. > > Looking back at my V2 patchset, the first patch was to add ls2x-rtc compatible. > (https://lore.kernel.org/linux-rtc/0288efeb4209e4a49af07de6399045aaa00a970c.1673227292.git.zhoubinbin@loongson.cn/) > > In the process of modifying the V2 patchset, it was discovered that > the ACPI domain offset addresses on some of the socs (LS2K1000) were > different and I wanted to differentiate them by soc compatible. So I > was going to drop the ls2x-rtc compatible directly from the V3 patch > set. > However, when I rebased the V3 patchset, I found that the previous > ls2x-rtc compatible patch had been merged (commit 473a8ce756fd). So I > had to remove it and add soc compatible. Can all folks in Loongson stop adding wildcards as compatibles? Several compatibles were acked, because we do not know what 'x' stands for. Now, it turns out it's a wildcard which is not allowed. https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42 > > How about the following description: > Since commit 473a8ce756fd (dt-bindings: rtc: Add Loongson LS2X RTC > support), the ls2x-rtc compatible has been added. But the specific soc > compatible is needed to be used to define different ACPI domain offset > addresses. > It's better. Anyway, SoC parts are rarely trivial devices, so this should be probably moved to its own schema. Best regards, Krzysztof
On Wed, Apr 19, 2023 at 4:52 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 19/04/2023 09:12, Binbin Zhou wrote: > > On Mon, Apr 17, 2023 at 1:31 AM Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > >> > >> On 13/04/2023 09:57, Binbin Zhou wrote: > >>> The LS2X RTC alarm depends on the associated registers in a separate > >>> power management domain. > >>> > >>> In order to define the PM domain addresses of the different chips, a > >>> more detailed description of compatible is required. > >> > >> This does not match your diff at all. > >> > >>> > >>> Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn> > >>> --- > >>> Documentation/devicetree/bindings/rtc/trivial-rtc.yaml | 7 +++++-- > >>> 1 file changed, 5 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml b/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml > >>> index a3603e638c37..2928811b83a0 100644 > >>> --- a/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml > >>> +++ b/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml > >>> @@ -47,8 +47,11 @@ properties: > >>> - isil,isl1218 > >>> # Intersil ISL12022 Real-time Clock > >>> - isil,isl12022 > >>> - # Loongson-2K Socs/LS7A bridge Real-time Clock > >>> - - loongson,ls2x-rtc > >> > >> Why removing it? > >> > >>> + # Loongson LS7A bridge Real-time Clock > >>> + - loongson,ls7a-rtc > >>> + # Loongson-2K Socs Real-time Clock > >>> + - loongson,ls2k0500-rtc > >>> + - loongson,ls2k1000-rtc > >> > >> That's even more surprising... > >> > >> I don't understand what you are doing here at all. > > Hi Krzysztof: > > > > Sorry, maybe my description was not accurate. > > > > Looking back at my V2 patchset, the first patch was to add ls2x-rtc compatible. > > (https://lore.kernel.org/linux-rtc/0288efeb4209e4a49af07de6399045aaa00a970c.1673227292.git.zhoubinbin@loongson.cn/) > > > > In the process of modifying the V2 patchset, it was discovered that > > the ACPI domain offset addresses on some of the socs (LS2K1000) were > > different and I wanted to differentiate them by soc compatible. So I > > was going to drop the ls2x-rtc compatible directly from the V3 patch > > set. > > However, when I rebased the V3 patchset, I found that the previous > > ls2x-rtc compatible patch had been merged (commit 473a8ce756fd). So I > > had to remove it and add soc compatible. > > Can all folks in Loongson stop adding wildcards as compatibles? Several > compatibles were acked, because we do not know what 'x' stands for. Now, > it turns out it's a wildcard which is not allowed. > > https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42 > > > > > How about the following description: > > Since commit 473a8ce756fd (dt-bindings: rtc: Add Loongson LS2X RTC > > support), the ls2x-rtc compatible has been added. But the specific soc > > compatible is needed to be used to define different ACPI domain offset > > addresses. > > > It's better. Anyway, SoC parts are rarely trivial devices, so this > should be probably moved to its own schema. OK, I'll move these from rivial-rtc.yaml to a separate schema file. Thanks. Binbin > > Best regards, > Krzysztof >
diff --git a/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml b/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml index a3603e638c37..2928811b83a0 100644 --- a/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml +++ b/Documentation/devicetree/bindings/rtc/trivial-rtc.yaml @@ -47,8 +47,11 @@ properties: - isil,isl1218 # Intersil ISL12022 Real-time Clock - isil,isl12022 - # Loongson-2K Socs/LS7A bridge Real-time Clock - - loongson,ls2x-rtc + # Loongson LS7A bridge Real-time Clock + - loongson,ls7a-rtc + # Loongson-2K Socs Real-time Clock + - loongson,ls2k0500-rtc + - loongson,ls2k1000-rtc # Real Time Clock Module with I2C-Bus - microcrystal,rv3029 # Real Time Clock
The LS2X RTC alarm depends on the associated registers in a separate power management domain. In order to define the PM domain addresses of the different chips, a more detailed description of compatible is required. Signed-off-by: Binbin Zhou <zhoubinbin@loongson.cn> --- Documentation/devicetree/bindings/rtc/trivial-rtc.yaml | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)