| Message ID | 20220830230012.9429-1-pali@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/3] dt-bindings: reset: syscon-reboot: Add priority property | expand |
On Wed, Aug 31, 2022, at 1:00 AM, Pali Rohár wrote: > a/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml > b/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml > index da2509724812..4c8b0d0a0111 100644 > --- a/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml > +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml > @@ -42,6 +42,11 @@ properties: > $ref: /schemas/types.yaml#/definitions/uint32 > description: The reset value written to the reboot register (32 > bit access). > > + priority: > + $ref: /schemas/types.yaml#/definitions/int32 > + description: Priority level of this syscon reset device. > + default: 192 > + My first thought was that this is looks very Linux specific and probably should be documented as such. However I see there is already precedent in Documentation/devicetree/bindings/power/reset/gpio-restart.yaml, which defines the same thing with a more detailed description. Since this is an optional property for both, and it has the same meaning here, is it possible to move the description to a common place where it either gets included from both, or from all reboot bindings? Arnd
On 31/08/2022 02:00, Pali Rohár wrote: > This new optional priority property allows to specify custom priority level > of reset device. Default level was always 192. You still did not explain why do we need this. You only explained what you did here, which is obvious and visible from the diff. What you should explain is why you are doing it, what problem you are solving. Best regards, Krzysztof
On Wednesday 31 August 2022 10:31:22 Krzysztof Kozlowski wrote: > On 31/08/2022 02:00, Pali Rohár wrote: > > This new optional priority property allows to specify custom priority level > > of reset device. Default level was always 192. > > You still did not explain why do we need this. You only explained what > you did here, which is obvious and visible from the diff. What you > should explain is why you are doing it, what problem you are solving. > > Best regards, > Krzysztof Look at patch 3/3, thanks.
On 31/08/2022 10:36, Pali Rohár wrote: > On Wednesday 31 August 2022 10:31:22 Krzysztof Kozlowski wrote: >> On 31/08/2022 02:00, Pali Rohár wrote: >>> This new optional priority property allows to specify custom priority level >>> of reset device. Default level was always 192. >> >> You still did not explain why do we need this. You only explained what >> you did here, which is obvious and visible from the diff. What you >> should explain is why you are doing it, what problem you are solving. >> >> Best regards, >> Krzysztof > > Look at patch 3/3, thanks. This commit should explain it why you add new property. Some other commits going via different trees/branches and ending up in entirely different time/place in history do not really count. Best regards, Krzysztof
On Wed, Aug 31, 2022 at 09:26:17AM +0200, Arnd Bergmann wrote: > On Wed, Aug 31, 2022, at 1:00 AM, Pali Rohár wrote: > > a/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml > > b/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml > > index da2509724812..4c8b0d0a0111 100644 > > --- a/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml > > +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml > > @@ -42,6 +42,11 @@ properties: > > $ref: /schemas/types.yaml#/definitions/uint32 > > description: The reset value written to the reboot register (32 > > bit access). > > > > + priority: > > + $ref: /schemas/types.yaml#/definitions/int32 > > + description: Priority level of this syscon reset device. > > + default: 192 > > + > > My first thought was that this is looks very Linux specific and > probably should be documented as such. However I see there is > already precedent in > Documentation/devicetree/bindings/power/reset/gpio-restart.yaml, > which defines the same thing with a more detailed description. > > Since this is an optional property for both, and it has the > same meaning here, is it possible to move the description > to a common place where it either gets included from both, > or from all reboot bindings? Yes, that is what I said in my other replies. Rob
diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml b/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml index da2509724812..4c8b0d0a0111 100644 --- a/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot.yaml @@ -42,6 +42,11 @@ properties: $ref: /schemas/types.yaml#/definitions/uint32 description: The reset value written to the reboot register (32 bit access). + priority: + $ref: /schemas/types.yaml#/definitions/int32 + description: Priority level of this syscon reset device. + default: 192 + required: - compatible - offset
This new optional priority property allows to specify custom priority level of reset device. Default level was always 192. Signed-off-by: Pali Rohár <pali@kernel.org> --- Changes in v2: * Change sint32 to int32 * Add default --- .../devicetree/bindings/power/reset/syscon-reboot.yaml | 5 +++++ 1 file changed, 5 insertions(+)