| Message ID | 20220422072713.3172345-1-primoz.fiser@norik.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/3] dt-bindings: mfd: da9063: watchdog: add suspend disable option | expand |
On Fri, Apr 22, 2022 at 09:27:11AM +0200, Primoz Fiser wrote: > Document the watchdog disable option which can be used if the hardware > automatic suspend option is broken. > > Based on commit c514430c51ee8 ("dt-bindings: watchdog: da9062: add > suspend disable option"). > > Signed-off-by: Primoz Fiser <primoz.fiser@norik.com> > --- > Documentation/devicetree/bindings/mfd/da9063.txt | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/mfd/da9063.txt b/Documentation/devicetree/bindings/mfd/da9063.txt > index 91b79a21d403..aa8b800cc4ad 100644 > --- a/Documentation/devicetree/bindings/mfd/da9063.txt > +++ b/Documentation/devicetree/bindings/mfd/da9063.txt > @@ -64,10 +64,13 @@ Sub-nodes: > and KEY_SLEEP. > > - watchdog : This node defines settings for the Watchdog timer associated > - with the DA9063 and DA9063L. There are currently no entries in this > - binding, however compatible = "dlg,da9063-watchdog" should be added > - if a node is created. > + with the DA9063 and DA9063L. The node should contain the compatible property > + with the value "dlg,da9063-watchdog". > > + Optional watchdog properties: > + - dlg,use-sw-pm: Add this property to disable the watchdog during suspend. Name the property based on the h/w quirk rather than what to do in response. Something like 'dlg,hw-suspend-broken' > + Only use this option if you can't use the watchdog automatic suspend > + function during a suspend (see register CONTROL_B). > > Example: > > -- > 2.25.1 > >
Hi Rob, >> Name the property based on the h/w quirk rather than what to do in >> response. Something like 'dlg,hw-suspend-broken' Shouldn't we match da9062's property? As this commit is based on c514430c51ee8 ("dt-bindings: watchdog: da9062: add suspend disable option") which uses "dlg,use-sw-pm" as property to implement the same functionality. Sure I can spin up v2 with your proposal but I think that would create unnecessary ambiguity and confusion? For example, phyCORE board uses da9062 PMIC while phyFLEX uses da9063 as PMIC. Boards are from the same SoM vendor. So one board would have to use "dlg,use-sw-pm" and the other one "dlg,hw-suspend-broken" property to achieve the same thing? On 2. 05. 22 22:56, Rob Herring wrote: > On Fri, Apr 22, 2022 at 09:27:11AM +0200, Primoz Fiser wrote: >> Document the watchdog disable option which can be used if the hardware >> automatic suspend option is broken. >> >> Based on commit c514430c51ee8 ("dt-bindings: watchdog: da9062: add >> suspend disable option"). >> >> Signed-off-by: Primoz Fiser <primoz.fiser@norik.com> >> --- >> Documentation/devicetree/bindings/mfd/da9063.txt | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/mfd/da9063.txt b/Documentation/devicetree/bindings/mfd/da9063.txt >> index 91b79a21d403..aa8b800cc4ad 100644 >> --- a/Documentation/devicetree/bindings/mfd/da9063.txt >> +++ b/Documentation/devicetree/bindings/mfd/da9063.txt >> @@ -64,10 +64,13 @@ Sub-nodes: >> and KEY_SLEEP. >> >> - watchdog : This node defines settings for the Watchdog timer associated >> - with the DA9063 and DA9063L. There are currently no entries in this >> - binding, however compatible = "dlg,da9063-watchdog" should be added >> - if a node is created. >> + with the DA9063 and DA9063L. The node should contain the compatible property >> + with the value "dlg,da9063-watchdog". >> >> + Optional watchdog properties: >> + - dlg,use-sw-pm: Add this property to disable the watchdog during suspend. > > Name the property based on the h/w quirk rather than what to do in > response. Something like 'dlg,hw-suspend-broken' > >> + Only use this option if you can't use the watchdog automatic suspend >> + function during a suspend (see register CONTROL_B). >> >> Example: >> >> -- >> 2.25.1 >> >>
On Tue, May 03, 2022 at 08:53:42AM +0200, Primoz Fiser wrote: > Hi Rob, > Please don't top post. Trim any irrelevant parts and reply below the original quoted text. > > > Name the property based on the h/w quirk rather than what to do in > > > response. Something like 'dlg,hw-suspend-broken' > > Shouldn't we match da9062's property? Ah, yes. I failed to grasp that from the 'based on commit...' in the commit msg. > > As this commit is based on c514430c51ee8 ("dt-bindings: watchdog: da9062: > add suspend disable option") which uses "dlg,use-sw-pm" as property to > implement the same functionality. The 'which uses "dlg,use-sw-pm" as property to implement the same functionality' part would be useful in the commit msg. With that, Reviewed-by: Rob Herring <robh@kernel.org> > > Sure I can spin up v2 with your proposal but I think that would create > unnecessary ambiguity and confusion? > > For example, phyCORE board uses da9062 PMIC while phyFLEX uses da9063 as > PMIC. Boards are from the same SoM vendor. So one board would have to use > "dlg,use-sw-pm" and the other one "dlg,hw-suspend-broken" property to > achieve the same thing? > > > On 2. 05. 22 22:56, Rob Herring wrote: > > On Fri, Apr 22, 2022 at 09:27:11AM +0200, Primoz Fiser wrote: > > > Document the watchdog disable option which can be used if the hardware > > > automatic suspend option is broken. > > > > > > Based on commit c514430c51ee8 ("dt-bindings: watchdog: da9062: add > > > suspend disable option"). > > > > > > Signed-off-by: Primoz Fiser <primoz.fiser@norik.com> > > > --- > > > Documentation/devicetree/bindings/mfd/da9063.txt | 9 ++++++--- > > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > > > diff --git a/Documentation/devicetree/bindings/mfd/da9063.txt b/Documentation/devicetree/bindings/mfd/da9063.txt > > > index 91b79a21d403..aa8b800cc4ad 100644 > > > --- a/Documentation/devicetree/bindings/mfd/da9063.txt > > > +++ b/Documentation/devicetree/bindings/mfd/da9063.txt > > > @@ -64,10 +64,13 @@ Sub-nodes: > > > and KEY_SLEEP. > > > - watchdog : This node defines settings for the Watchdog timer associated > > > - with the DA9063 and DA9063L. There are currently no entries in this > > > - binding, however compatible = "dlg,da9063-watchdog" should be added > > > - if a node is created. > > > + with the DA9063 and DA9063L. The node should contain the compatible property > > > + with the value "dlg,da9063-watchdog". > > > + Optional watchdog properties: > > > + - dlg,use-sw-pm: Add this property to disable the watchdog during suspend. > > > > Name the property based on the h/w quirk rather than what to do in > > response. Something like 'dlg,hw-suspend-broken' > > > > > + Only use this option if you can't use the watchdog automatic suspend > > > + function during a suspend (see register CONTROL_B). > > > Example: > > > -- > > > 2.25.1 > > > > > >
On 22 April 2022 08:27, Primoz Fiser wrote: > Document the watchdog disable option which can be used if the hardware > automatic suspend option is broken. > > Based on commit c514430c51ee8 ("dt-bindings: watchdog: da9062: add > suspend disable option"). > > Signed-off-by: Primoz Fiser <primoz.fiser@norik.com> > --- > Documentation/devicetree/bindings/mfd/da9063.txt | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/mfd/da9063.txt > b/Documentation/devicetree/bindings/mfd/da9063.txt > index 91b79a21d403..aa8b800cc4ad 100644 > --- a/Documentation/devicetree/bindings/mfd/da9063.txt > +++ b/Documentation/devicetree/bindings/mfd/da9063.txt > @@ -64,10 +64,13 @@ Sub-nodes: > and KEY_SLEEP. > > - watchdog : This node defines settings for the Watchdog timer associated I don't know if this is just me, but it looks like you're deleting this line above, but not replacing it..... > - with the DA9063 and DA9063L. There are currently no entries in this > - binding, however compatible = "dlg,da9063-watchdog" should be added > - if a node is created. ....here, if I'm reading this patch correctly. This means we're losing that property description, and starting a text block with the below text. > + with the DA9063 and DA9063L. The node should contain the compatible > property > + with the value "dlg,da9063-watchdog". > > + Optional watchdog properties: > + - dlg,use-sw-pm: Add this property to disable the watchdog during suspend. > + Only use this option if you can't use the watchdog automatic suspend > + function during a suspend (see register CONTROL_B). > > Example: > > -- > 2.25.1
On 9. 05. 22 10:50, DLG Adam Thomson wrote: > On 22 April 2022 08:27, Primoz Fiser wrote: > >> Document the watchdog disable option which can be used if the hardware >> automatic suspend option is broken. >> >> Based on commit c514430c51ee8 ("dt-bindings: watchdog: da9062: add >> suspend disable option"). >> >> Signed-off-by: Primoz Fiser <primoz.fiser@norik.com> >> --- >> Documentation/devicetree/bindings/mfd/da9063.txt | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/mfd/da9063.txt >> b/Documentation/devicetree/bindings/mfd/da9063.txt >> index 91b79a21d403..aa8b800cc4ad 100644 >> --- a/Documentation/devicetree/bindings/mfd/da9063.txt >> +++ b/Documentation/devicetree/bindings/mfd/da9063.txt >> @@ -64,10 +64,13 @@ Sub-nodes: >> and KEY_SLEEP. >> >> - watchdog : This node defines settings for the Watchdog timer associated > > I don't know if this is just me, but it looks like you're deleting this line > above, but not replacing it..... I am not deleting this line, please note the leading white-space. But yeah, if you don't pay close attention it looks a bit confusing indeed :) > >> - with the DA9063 and DA9063L. There are currently no entries in this >> - binding, however compatible = "dlg,da9063-watchdog" should be added >> - if a node is created. > > ....here, if I'm reading this patch correctly. This means we're losing that > property description, and starting a text block with the below text. > >> + with the DA9063 and DA9063L. The node should contain the compatible >> property >> + with the value "dlg,da9063-watchdog". >> >> + Optional watchdog properties: >> + - dlg,use-sw-pm: Add this property to disable the watchdog during suspend. >> + Only use this option if you can't use the watchdog automatic suspend >> + function during a suspend (see register CONTROL_B). >> >> Example: >> >> -- >> 2.25.1 > Would you like: The node should contain the compatible property with the value compatible = "dlg,da9063-watchdog". i.e. explicitly adding "compatible =" in front, for v2?
On 11 May 2022 09:10, Primoz Fiser wrote: > >> Document the watchdog disable option which can be used if the hardware > >> automatic suspend option is broken. > >> > >> Based on commit c514430c51ee8 ("dt-bindings: watchdog: da9062: add > >> suspend disable option"). > >> > >> Signed-off-by: Primoz Fiser <primoz.fiser@norik.com> > >> --- > >> Documentation/devicetree/bindings/mfd/da9063.txt | 9 ++++++--- > >> 1 file changed, 6 insertions(+), 3 deletions(-) > >> > >> diff --git a/Documentation/devicetree/bindings/mfd/da9063.txt > >> b/Documentation/devicetree/bindings/mfd/da9063.txt > >> index 91b79a21d403..aa8b800cc4ad 100644 > >> --- a/Documentation/devicetree/bindings/mfd/da9063.txt > >> +++ b/Documentation/devicetree/bindings/mfd/da9063.txt > >> @@ -64,10 +64,13 @@ Sub-nodes: > >> and KEY_SLEEP. > >> > >> - watchdog : This node defines settings for the Watchdog timer associated > > > > I don't know if this is just me, but it looks like you're deleting this line > > above, but not replacing it..... > > I am not deleting this line, please note the leading white-space. > > But yeah, if you don't pay close attention it looks a bit confusing > indeed :) There you go. Thought it would be a strange deletion. Ignore me :) > > > > >> - with the DA9063 and DA9063L. There are currently no entries in this > >> - binding, however compatible = "dlg,da9063-watchdog" should be added > >> - if a node is created. > > > > ....here, if I'm reading this patch correctly. This means we're losing that > > property description, and starting a text block with the below text. > > > >> + with the DA9063 and DA9063L. The node should contain the compatible > >> property > >> + with the value "dlg,da9063-watchdog". > >> > >> + Optional watchdog properties: > >> + - dlg,use-sw-pm: Add this property to disable the watchdog during suspend. > >> + Only use this option if you can't use the watchdog automatic suspend > >> + function during a suspend (see register CONTROL_B). > >> > >> Example: > >> > >> -- > >> 2.25.1 > > > > Would you like: > > The node should contain the compatible property with the value > compatible = "dlg,da9063-watchdog". > > i.e. explicitly adding "compatible =" in front, for v2? No change necessary. It just looked odd when I thought that first line was being deleted. Given that's not the case: Reviewed-by: Adam Thomson <DLG-Adam.Thomson.Opensource@dm.renesas.com>
On Fri, Apr 22, 2022 at 09:27:11AM +0200, Primoz Fiser wrote: > Document the watchdog disable option which can be used if the hardware > automatic suspend option is broken. > > Based on commit c514430c51ee8 ("dt-bindings: watchdog: da9062: add > suspend disable option"). > > Signed-off-by: Primoz Fiser <primoz.fiser@norik.com> > Reviewed-by: Rob Herring <robh@kernel.org> > Reviewed-by: Adam Thomson <DLG-Adam.Thomson.Opensource@dm.renesas.com> Reviewed-by: Guenter Roeck <linux@roeck-us.net> > --- > Documentation/devicetree/bindings/mfd/da9063.txt | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/mfd/da9063.txt b/Documentation/devicetree/bindings/mfd/da9063.txt > index 91b79a21d403..aa8b800cc4ad 100644 > --- a/Documentation/devicetree/bindings/mfd/da9063.txt > +++ b/Documentation/devicetree/bindings/mfd/da9063.txt > @@ -64,10 +64,13 @@ Sub-nodes: > and KEY_SLEEP. > > - watchdog : This node defines settings for the Watchdog timer associated > - with the DA9063 and DA9063L. There are currently no entries in this > - binding, however compatible = "dlg,da9063-watchdog" should be added > - if a node is created. > + with the DA9063 and DA9063L. The node should contain the compatible property > + with the value "dlg,da9063-watchdog". > > + Optional watchdog properties: > + - dlg,use-sw-pm: Add this property to disable the watchdog during suspend. > + Only use this option if you can't use the watchdog automatic suspend > + function during a suspend (see register CONTROL_B). > > Example: >
diff --git a/Documentation/devicetree/bindings/mfd/da9063.txt b/Documentation/devicetree/bindings/mfd/da9063.txt index 91b79a21d403..aa8b800cc4ad 100644 --- a/Documentation/devicetree/bindings/mfd/da9063.txt +++ b/Documentation/devicetree/bindings/mfd/da9063.txt @@ -64,10 +64,13 @@ Sub-nodes: and KEY_SLEEP. - watchdog : This node defines settings for the Watchdog timer associated - with the DA9063 and DA9063L. There are currently no entries in this - binding, however compatible = "dlg,da9063-watchdog" should be added - if a node is created. + with the DA9063 and DA9063L. The node should contain the compatible property + with the value "dlg,da9063-watchdog". + Optional watchdog properties: + - dlg,use-sw-pm: Add this property to disable the watchdog during suspend. + Only use this option if you can't use the watchdog automatic suspend + function during a suspend (see register CONTROL_B). Example:
Document the watchdog disable option which can be used if the hardware automatic suspend option is broken. Based on commit c514430c51ee8 ("dt-bindings: watchdog: da9062: add suspend disable option"). Signed-off-by: Primoz Fiser <primoz.fiser@norik.com> --- Documentation/devicetree/bindings/mfd/da9063.txt | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)