| Message ID | 20220826170716.6886-2-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [v2,1/4] pwm: sysfs: Switch to DEFINE_SIMPLE_DEV_PM_OPS() and pm_sleep_ptr() | expand |
On Fri, Aug 26, 2022 at 08:07:14PM +0300, Andy Shevchenko wrote: > There is no need to assign ret to 0 and then break the loop just > for returning the error to the caller. Instead, return directly > from the for-loop, and 0 otherwise. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > v2: added tag (Uwe) > drivers/pwm/sysfs.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) I fail to see how this is an improvement. The outcome is exactly the same and this doesn't even make the code shorter. Why bother? Thierry
On Wed, Sep 28, 2022 at 02:31:23PM +0200, Thierry Reding wrote: > On Fri, Aug 26, 2022 at 08:07:14PM +0300, Andy Shevchenko wrote: > > There is no need to assign ret to 0 and then break the loop just > > for returning the error to the caller. Instead, return directly > > from the for-loop, and 0 otherwise. > I fail to see how this is an improvement. The outcome is exactly the > same and this doesn't even make the code shorter. Why bother? The improvement is in maintenance. It's proven that assignments in the definition block might lead to the subtle mistakes when it's not close enough to the actual use. That said, this is an improvement from maintaining and developing perspectives.
diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c index c21b6046067b..1543fe07765b 100644 --- a/drivers/pwm/sysfs.c +++ b/drivers/pwm/sysfs.c @@ -413,7 +413,7 @@ static int pwm_class_resume_npwm(struct device *parent, unsigned int npwm) { struct pwm_chip *chip = dev_get_drvdata(parent); unsigned int i; - int ret = 0; + int ret; for (i = 0; i < npwm; i++) { struct pwm_device *pwm = &chip->pwms[i]; @@ -427,17 +427,17 @@ static int pwm_class_resume_npwm(struct device *parent, unsigned int npwm) state.enabled = export->suspend.enabled; ret = pwm_class_apply_state(export, pwm, &state); if (ret < 0) - break; + return ret; } - return ret; + return 0; } static int pwm_class_suspend(struct device *parent) { struct pwm_chip *chip = dev_get_drvdata(parent); unsigned int i; - int ret = 0; + int ret; for (i = 0; i < chip->npwm; i++) { struct pwm_device *pwm = &chip->pwms[i]; @@ -457,11 +457,11 @@ static int pwm_class_suspend(struct device *parent) * this suspend function. */ pwm_class_resume_npwm(parent, i); - break; + return ret; } } - return ret; + return 0; } static int pwm_class_resume(struct device *parent)