| Message ID | 20231216014850.1011344-2-jisheng.teoh@starfivetech.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add StarFive JH8100 watchdog | expand |
On 16/12/2023 02:48, Ji Sheng Teoh wrote: > Add "starfive,jh8100-wdt" compatible string for StarFive's JH8100 > watchdog. > Since JH8100 watchdog only has 1 reset signal, update binding > document to support one reset for "starfive,jh8100-wdt" compatible. > > Signed-off-by: Ley Foon Tan <leyfoon.tan@starfivetech.com> > Signed-off-by: Ji Sheng Teoh <jisheng.teoh@starfivetech.com> > --- > .../watchdog/starfive,jh7100-wdt.yaml | 29 +++++++++++++++---- > 1 file changed, 23 insertions(+), 6 deletions(-) > > diff --git a/Documentation/devicetree/bindings/watchdog/starfive,jh7100-wdt.yaml b/Documentation/devicetree/bindings/watchdog/starfive,jh7100-wdt.yaml > index 68f3f6fd08a6..ab077f64a83e 100644 > --- a/Documentation/devicetree/bindings/watchdog/starfive,jh7100-wdt.yaml > +++ b/Documentation/devicetree/bindings/watchdog/starfive,jh7100-wdt.yaml > @@ -19,14 +19,12 @@ description: > isn't cleared, the watchdog will reset the system unless the watchdog > reset is disabled. > > -allOf: > - - $ref: watchdog.yaml# > - > properties: > compatible: > enum: > - starfive,jh7100-wdt > - starfive,jh7110-wdt > + - starfive,jh8100-wdt What is happening with this patchset? I asked about one specific items. you know - comment is written under specific inline quopte. You wrote in changelog "Drop items in compatible field.", but I see oneOf gone! I have real doubts that you ever tested your entire solution with this binding. Where is the DTS? Best regards, Krzysztof
On Mon, 18 Dec 2023 09:41:07 +0100 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 16/12/2023 02:48, Ji Sheng Teoh wrote: > > Add "starfive,jh8100-wdt" compatible string for StarFive's JH8100 > > watchdog. > > Since JH8100 watchdog only has 1 reset signal, update binding > > document to support one reset for "starfive,jh8100-wdt" compatible. > > > > Signed-off-by: Ley Foon Tan <leyfoon.tan@starfivetech.com> > > Signed-off-by: Ji Sheng Teoh <jisheng.teoh@starfivetech.com> > > --- > > .../watchdog/starfive,jh7100-wdt.yaml | 29 > > +++++++++++++++---- 1 file changed, 23 insertions(+), 6 deletions(-) > > > > diff --git > > a/Documentation/devicetree/bindings/watchdog/starfive,jh7100-wdt.yaml > > b/Documentation/devicetree/bindings/watchdog/starfive,jh7100-wdt.yaml > > index 68f3f6fd08a6..ab077f64a83e 100644 --- > > a/Documentation/devicetree/bindings/watchdog/starfive,jh7100-wdt.yaml > > +++ > > b/Documentation/devicetree/bindings/watchdog/starfive,jh7100-wdt.yaml > > @@ -19,14 +19,12 @@ description: isn't cleared, the watchdog will > > reset the system unless the watchdog reset is disabled. -allOf: > > - - $ref: watchdog.yaml# > > - > > properties: > > compatible: > > enum: > > - starfive,jh7100-wdt > > - starfive,jh7110-wdt > > + - starfive,jh8100-wdt > > What is happening with this patchset? I asked about one specific > items. you know - comment is written under specific inline quopte. > You wrote in changelog "Drop items in compatible field.", but I see > oneOf gone! My bad, I've interpreted it wrongly. Will rework the compatible field to this instead: compatible: oneOf: - enum: - starfive,jh7100-wdt - starfive,jh7110-wdt - items: - enum: - starfive,jh8100-wdt - const: starfive,jh7110-wdt While reworking this, I've noticed reset field with maxItems alone will cause dt_binding_check to fail with following error: 'watchdog@12270000: resets: [[4294967295, 15]] is too short' This is fixed by defining minItems and maxItems as follow: resets: minItems: 1 maxItems: 2 > > I have real doubts that you ever tested your entire solution with this > binding. Where is the DTS? > Currently, the DTS is still in internal and yet to upstream as it depends on [1]. [1] https://lore.kernel.org/all/20231201121410.95298-1-jeeheng.sia@starfivetech.com/ > Best regards, > Krzysztof >
On 18/12/2023 15:27, Ji Sheng Teoh wrote: >> >> I have real doubts that you ever tested your entire solution with this >> binding. Where is the DTS? >> > > Currently, the DTS is still in internal and yet to upstream as it depends > on [1]. Yeah, so you send untested code which cannot work or pass tests. If you do not test your code, we need to be able to at least verify it, so send your DTS. Otherwise I cannot trust that this works at all. Best regards, Krzysztof
On Mon, 18 Dec 2023 15:41:37 +0100 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 18/12/2023 15:27, Ji Sheng Teoh wrote: > >> > >> I have real doubts that you ever tested your entire solution with > >> this binding. Where is the DTS? > >> > > > > Currently, the DTS is still in internal and yet to upstream as it > > depends on [1]. > > Yeah, so you send untested code which cannot work or pass tests. If > you do not test your code, we need to be able to at least verify it, > so send your DTS. Otherwise I cannot trust that this works at all. > Will submit it with DTS once things have cleared up. Thanks for the comment. > Best regards, > Krzysztof >
On Mon, Dec 18, 2023 at 11:37:38PM +0800, Ji Sheng Teoh wrote: > On Mon, 18 Dec 2023 15:41:37 +0100 > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > > On 18/12/2023 15:27, Ji Sheng Teoh wrote: > > >> > > >> I have real doubts that you ever tested your entire solution with > > >> this binding. Where is the DTS? > > >> > > > > > > Currently, the DTS is still in internal and yet to upstream as it > > > depends on [1]. > > > > Yeah, so you send untested code which cannot work or pass tests. If > > you do not test your code, we need to be able to at least verify it, > > so send your DTS. Otherwise I cannot trust that this works at all. > > > Will submit it with DTS once things have cleared up. > Thanks for the comment. [1] is not going to applied for a while since the SoC doesn't actually exist yet and is pre-tapeout on an FPGA. I would just send the dts patch adding the watchdog alongside the series, or else you'll be waiting for quite a while. Or even link to the node on github or whatever.
On Tue, 19 Dec 2023 15:43:07 +0000 Conor Dooley <conor@kernel.org> wrote: > On Mon, Dec 18, 2023 at 11:37:38PM +0800, Ji Sheng Teoh wrote: > > On Mon, 18 Dec 2023 15:41:37 +0100 > > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > > > > On 18/12/2023 15:27, Ji Sheng Teoh wrote: > > > >> > > > >> I have real doubts that you ever tested your entire solution > > > >> with this binding. Where is the DTS? > > > >> > > > > > > > > Currently, the DTS is still in internal and yet to upstream as > > > > it depends on [1]. > > > > > > Yeah, so you send untested code which cannot work or pass tests. > > > If you do not test your code, we need to be able to at least > > > verify it, so send your DTS. Otherwise I cannot trust that this > > > works at all. > > Will submit it with DTS once things have cleared up. > > Thanks for the comment. > > [1] is not going to applied for a while since the SoC doesn't actually > exist yet and is pre-tapeout on an FPGA. I would just send the dts > patch adding the watchdog alongside the series, or else you'll be > waiting for quite a while. Or even link to the node on github or > whatever. > Ok, will add the watchdog dts alongside this series and mention its dependency on [1]. Thanks.
diff --git a/Documentation/devicetree/bindings/watchdog/starfive,jh7100-wdt.yaml b/Documentation/devicetree/bindings/watchdog/starfive,jh7100-wdt.yaml index 68f3f6fd08a6..ab077f64a83e 100644 --- a/Documentation/devicetree/bindings/watchdog/starfive,jh7100-wdt.yaml +++ b/Documentation/devicetree/bindings/watchdog/starfive,jh7100-wdt.yaml @@ -19,14 +19,12 @@ description: isn't cleared, the watchdog will reset the system unless the watchdog reset is disabled. -allOf: - - $ref: watchdog.yaml# - properties: compatible: enum: - starfive,jh7100-wdt - starfive,jh7110-wdt + - starfive,jh8100-wdt reg: maxItems: 1 @@ -45,9 +43,7 @@ properties: - const: core resets: - items: - - description: APB reset - - description: Core reset + maxItems: 2 required: - compatible @@ -56,6 +52,27 @@ required: - clock-names - resets +allOf: + - $ref: watchdog.yaml# + + - if: + properties: + compatible: + contains: + enum: + - starfive,jh8100-wdt + then: + properties: + resets: + items: + - description: Core reset + else: + properties: + resets: + items: + - description: APB reset + - description: Core reset + unevaluatedProperties: false examples: