Re: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add watchdog binding

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2/23/19 7:04 PM, Anson Huang wrote:
Hi, Guenter/Rob

Best Regards!
Anson Huang

-----Original Message-----
From: Guenter Roeck [mailto:groeck7@xxxxxxxxx] On Behalf Of Guenter
Roeck
Sent: 2019年2月24日 1:08
To: Rob Herring <robh@xxxxxxxxxx>; Anson Huang <anson.huang@xxxxxxx>
Cc: mark.rutland@xxxxxxx; shawnguo@xxxxxxxxxx;
s.hauer@xxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx;
catalin.marinas@xxxxxxx; will.deacon@xxxxxxx; wim@xxxxxxxxxxxxxxxxxx;
Aisheng Dong <aisheng.dong@xxxxxxx>; ulf.hansson@xxxxxxxxxx; Daniel
Baluta <daniel.baluta@xxxxxxx>; Andy Gross <andy.gross@xxxxxxxxxx>;
horms+renesas@xxxxxxxxxxxx; heiko@xxxxxxxxx; arnd@xxxxxxxx;
bjorn.andersson@xxxxxxxxxx; jagan@xxxxxxxxxxxxxxxxxxxx;
enric.balletbo@xxxxxxxxxxxxx; marc.w.gonzalez@xxxxxxx; olof@xxxxxxxxx;
devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-arm-
kernel@xxxxxxxxxxxxxxxxxxx; linux-watchdog@xxxxxxxxxxxxxxx; dl-linux-imx
<linux-imx@xxxxxxx>
Subject: Re: [PATCH RESEND V2 1/4] dt-bindings: fsl: scu: add watchdog
binding

On 2/22/19 11:52 AM, Rob Herring wrote:
On Mon, Feb 18, 2019 at 06:53:48AM +0000, Anson Huang wrote:
Add i.MX8QXP system controller watchdog binding.

Signed-off-by: Anson Huang <Anson.Huang@xxxxxxx>
---
Changes since V1:
	- update dts node name to "watchdog";
---
   Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt | 10
++++++++++
   1 file changed, 10 insertions(+)

diff --git
a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
index 4b79751..f388ec6 100644
--- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
+++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt
@@ -136,6 +136,12 @@ Required properties:
   			   resource id for thermal driver to get temperature
via
   			   SCU IPC.

+Watchdog bindings based on SCU Message Protocol
+------------------------------------------------------------
+
+Required properties:
+- compatible: should be "fsl,imx8qxp-sc-wdt";
+
   Example (imx8qxp):
   -------------
   lsio_mu1: mailbox@5d1c0000 {
@@ -188,6 +194,10 @@ firmware {
   			tsens-num = <1>;
   			#thermal-sensor-cells = <1>;
   		};
+
+		watchdog: watchdog {
+			compatible = "fsl,imx8qxp-sc-wdt";

As-is, there's no reason for this to be in DT. The parent node's
driver can instantiate the wdog.


As the driver is currently written, you are correct, since it doesn't accept
watchdog timeout configuration through devicetree.

Question is if that is intended. Is it ?

I am a little confused, do you mean we no need to have "watchdog" node in side "scu"
node? Or we need to modify the watchdog node's compatible string to " fsl,imx-sc-wdt" to make
it more generic for other platforms? If yes, I can resend the patch series to modify it.

I think Rob suggested that the SCU parent driver should instantiate the watchdog
without explicit watchdog node. That would be possible, but it currently uses
devm_of_platform_populate() to do the instantiation, and changing that would
be a mess. Besides, it does sem to me that your suggested node would describe
the hardware, so I am not sure I understand the reasoning.

For my part I referred to
	watchdog_init_timeout(imx_sc_wdd, DEFAULT_TIMEOUT, &pdev->dev);
in the driver, which guarantees that the timeout property will not be used
to set the timeout. A more common implementation would have been

	imx_sc_wdd->timeout = DEFAULT_TIMEOUT;
	ret = watchdog_init_timeout(imx_sc_wdd, timeout, &pdev->dev);

where 'timeout' is the module parameter. Which is actually not used in your driver.
Hmm ... I wasn't careful enough with my review. The timeout initialization as-is
doesn't make sense. I'll comment on that in the patch.

Guenter

Anson.


Thanks,
Guenter





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux