| Message ID | 20240715170717.2490688-1-Frank.Li@nxp.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,1/1] watchdog: imx7ulp_wdt: move post_rcs_wait into struct imx_wdt_hw_feature | expand |
Hi Frank, On Mon, Jul 15, 2024 at 2:07 PM Frank Li <Frank.Li@nxp.com> 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. > > Reviewed-by: Guenter Roeck <linux@roeck-us.net> > Signed-off-by: Alice Guo <alice.guo@nxp.com> > Reviewed-by: Ye Li <ye.li@nxp.com> > Signed-off-by: Frank Li <Frank.Li@nxp.com> It looks better now, thanks: Reviewed-by: Fabio Estevam <festevam@gmail.com>
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. > Reviewed-by: Guenter Roeck <linux@roeck-us.net> Please drop. I did not approve this change. Guenter > Signed-off-by: Alice Guo <alice.guo@nxp.com> > Reviewed-by: Ye Li <ye.li@nxp.com> > Signed-off-by: Frank Li <Frank.Li@nxp.com> > --- > 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 */ }
On 7/15/24 10:16, Fabio Estevam wrote: > Hi Frank, > > On Mon, Jul 15, 2024 at 2:07 PM Frank Li <Frank.Li@nxp.com> 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. >> >> Reviewed-by: Guenter Roeck <linux@roeck-us.net> >> Signed-off-by: Alice Guo <alice.guo@nxp.com> >> Reviewed-by: Ye Li <ye.li@nxp.com> >> Signed-off-by: Frank Li <Frank.Li@nxp.com> > > It looks better now, thanks: > Not in my opinion. As requested in my other e-mail, please drop my Reviewed-by: tag. Guenter > Reviewed-by: Fabio Estevam <festevam@gmail.com>
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@roeck-us.net> > > Please drop. I did not approve this change. > > Guenter > > > Signed-off-by: Alice Guo <alice.guo@nxp.com> > > Reviewed-by: Ye Li <ye.li@nxp.com> > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > > --- > > 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 */ } >
On 7/15/24 11:34, Frank Li wrote: > 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. > That is not the point. The point here is that you made an - in my opinion unnecessary - change to this patch while at the same time adding my Reviewed-by: tag which applied to another version of the patch. This is inappropriate. Please refrain from doing that in the future. Guenter
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. > > Reviewed-by: Guenter Roeck <linux@roeck-us.net> > Signed-off-by: Alice Guo <alice.guo@nxp.com> > Reviewed-by: Ye Li <ye.li@nxp.com> > Signed-off-by: Frank Li <Frank.Li@nxp.com> Being more explicit here, to make sure this isn't picked up with my Reviewed-by: tag: NACK for the explicit and unnecessary additional initializations. Guenter
On Mon, Jul 15, 2024 at 02:42:23PM -0700, Guenter Roeck wrote: > On 7/15/24 11:34, Frank Li wrote: > > 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. > > > > That is not the point. The point here is that you made an - in my opinion > unnecessary - change to this patch while at the same time adding my > Reviewed-by: tag which applied to another version of the patch. > This is inappropriate. Please refrain from doing that in the future. According to my previous experience, drop review tag only when there are 'big' change in new version. Of cause, the 'big' is quite subjective. I think "set false explicitly" is not 'big' enough to drop review tags. Frank > > Guenter >
On 7/15/24 14:56, Frank Li wrote: > On Mon, Jul 15, 2024 at 02:42:23PM -0700, Guenter Roeck wrote: >> On 7/15/24 11:34, Frank Li wrote: >>> 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. >>> >> >> That is not the point. The point here is that you made an - in my opinion >> unnecessary - change to this patch while at the same time adding my >> Reviewed-by: tag which applied to another version of the patch. >> This is inappropriate. Please refrain from doing that in the future. > > According to my previous experience, drop review tag only when there are > 'big' change in new version. Of cause, the 'big' is quite subjective. I > think "set false explicitly" is not 'big' enough to drop review tags. > This is not your call to make. Guenter
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 */ }