Re: [PATCH v3 1/1] watchdog: imx7ulp_wdt: move post_rcs_wait into struct imx_wdt_hw_feature

[Frank Li]

On Mon, Jul 15, 2024 at 11:01:04AM -0700, Guenter Roeck wrote:
> On 7/15/24 10:07, Frank Li wrote:
> > Move post_rcs_wait into struct imx_wdt_hw_feature to simplify code logic
> > for different compatible strings
> > 
> > i.MX93 and i.MX8ULP watchdog do not need to wait 2.5 clocks after RCS is
> > done. Set post_rcs_wait to false explicitly to maintain code consistency.
> > 
> 
> Why ? That is not necessary and typically frowned upon for static variables.

Some maintainer in other subsystem like explicity set to false to read code
easily even though not necessary for static variable espcially there are
already one which set to false.

I am fine for each ways. You are free to pick up v2 instead of v3 if you
don't like v3's change.

Frank

> 
> > Reviewed-by: Guenter Roeck <linux@xxxxxxxxxxxx>
> 
> Please drop. I did not approve this change.
> 
> Guenter
> 
> > Signed-off-by: Alice Guo <alice.guo@xxxxxxx>
> > Reviewed-by: Ye Li <ye.li@xxxxxxx>
> > Signed-off-by: Frank Li <Frank.Li@xxxxxxx>
> > ---
> > Change from v2 to v3
> > - Set post_rcs_wait to false explicitly to maintain code consistency
> > - Add Guenter review tag.
> > Change from v1 to v2
> > - Combine to one patch
> > ---
> >   drivers/watchdog/imx7ulp_wdt.c | 23 +++++++++++------------
> >   1 file changed, 11 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c
> > index 94914a22daff7..a4aa02f388b15 100644
> > --- a/drivers/watchdog/imx7ulp_wdt.c
> > +++ b/drivers/watchdog/imx7ulp_wdt.c
> > @@ -55,6 +55,7 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> >   struct imx_wdt_hw_feature {
> >   	bool prescaler_enable;
> > +	bool post_rcs_wait;
> >   	u32 wdog_clock_rate;
> >   };
> > @@ -62,7 +63,6 @@ struct imx7ulp_wdt_device {
> >   	struct watchdog_device wdd;
> >   	void __iomem *base;
> >   	struct clk *clk;
> > -	bool post_rcs_wait;
> >   	bool ext_reset;
> >   	const struct imx_wdt_hw_feature *hw;
> >   };
> > @@ -95,7 +95,7 @@ static int imx7ulp_wdt_wait_rcs(struct imx7ulp_wdt_device *wdt)
> >   		ret = -ETIMEDOUT;
> >   	/* Wait 2.5 clocks after RCS done */
> > -	if (wdt->post_rcs_wait)
> > +	if (wdt->hw->post_rcs_wait)
> >   		usleep_range(wait_min, wait_min + 2000);
> >   	return ret;
> > @@ -334,15 +334,6 @@ static int imx7ulp_wdt_probe(struct platform_device *pdev)
> >   	/* The WDOG may need to do external reset through dedicated pin */
> >   	imx7ulp_wdt->ext_reset = of_property_read_bool(dev->of_node, "fsl,ext-reset-output");
> > -	imx7ulp_wdt->post_rcs_wait = true;
> > -	if (of_device_is_compatible(dev->of_node,
> > -				    "fsl,imx8ulp-wdt")) {
> > -		dev_info(dev, "imx8ulp wdt probe\n");
> > -		imx7ulp_wdt->post_rcs_wait = false;
> > -	} else {
> > -		dev_info(dev, "imx7ulp wdt probe\n");
> > -	}
> > -
> >   	wdog = &imx7ulp_wdt->wdd;
> >   	wdog->info = &imx7ulp_wdt_info;
> >   	wdog->ops = &imx7ulp_wdt_ops;
> > @@ -402,16 +393,24 @@ static const struct dev_pm_ops imx7ulp_wdt_pm_ops = {
> >   static const struct imx_wdt_hw_feature imx7ulp_wdt_hw = {
> >   	.prescaler_enable = false,
> > +	.post_rcs_wait = true,
> > +	.wdog_clock_rate = 1000,
> > +};
> > +
> > +static const struct imx_wdt_hw_feature imx8ulp_wdt_hw = {
> > +	.prescaler_enable = false,
> > +	.post_rcs_wait = false,
> >   	.wdog_clock_rate = 1000,
> >   };
> >   static const struct imx_wdt_hw_feature imx93_wdt_hw = {
> >   	.prescaler_enable = true,
> > +	.post_rcs_wait = false,
> >   	.wdog_clock_rate = 125,
> >   };
> >   static const struct of_device_id imx7ulp_wdt_dt_ids[] = {
> > -	{ .compatible = "fsl,imx8ulp-wdt", .data = &imx7ulp_wdt_hw, },
> > +	{ .compatible = "fsl,imx8ulp-wdt", .data = &imx8ulp_wdt_hw, },
> >   	{ .compatible = "fsl,imx7ulp-wdt", .data = &imx7ulp_wdt_hw, },
> >   	{ .compatible = "fsl,imx93-wdt", .data = &imx93_wdt_hw, },
> >   	{ /* sentinel */ }
>