Messages in this thread | | | Date | Thu, 18 Aug 2022 09:45:39 +0200 | From | Uwe Kleine-König <> | Subject | Re: [PATCH] pwm: meson: Simplify probe function with dev_err_probe() |
| |
On Thu, Aug 18, 2022 at 03:22:34PM +0800, zhaoxiao wrote: > Switch to dev_err_probe() to remove all dev_err() -> return repeated > patterns, simplifying and shortening the probe function. > > Signed-off-by: zhaoxiao <zhaoxiao@uniontech.com> > --- > drivers/pwm/pwm-meson.c | 53 ++++++++++++++--------------------------- > 1 file changed, 18 insertions(+), 35 deletions(-) > > diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c > index 57112f438c6d..43ce8d9a33d2 100644 > --- a/drivers/pwm/pwm-meson.c > +++ b/drivers/pwm/pwm-meson.c > @@ -126,20 +126,16 @@ static int meson_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) > > if (channel->clk_parent) { > err = clk_set_parent(channel->clk, channel->clk_parent); > - if (err < 0) { > - dev_err(dev, "failed to set parent %s for %s: %d\n", > + if (err < 0) > + return dev_err_probe(dev, err, "failed to set parent %s for %s\n", > __clk_get_name(channel->clk_parent), > - __clk_get_name(channel->clk), err); > - return err; > - } > + __clk_get_name(channel->clk)); > } > > err = clk_prepare_enable(channel->clk); > - if (err < 0) { > - dev_err(dev, "failed to enable clock %s: %d\n", > - __clk_get_name(channel->clk), err); > - return err; > - } > + if (err < 0) > + return dev_err_probe(dev, err, "failed to enable clock %s\n", > + __clk_get_name(channel->clk));
It's wrong to use dev_err_probe in .request(). The function is only supposed to be used in the .probe() call.
> return 0; > } > @@ -166,24 +162,18 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, > duty = period - duty; > > fin_freq = clk_get_rate(channel->clk); > - if (fin_freq == 0) { > - dev_err(meson->chip.dev, "invalid source clock frequency\n"); > - return -EINVAL; > - } > + if (fin_freq == 0) > + return dev_err_probe(meson->chip.dev, -EINVAL, "invalid source clock frequency\n"); > > dev_dbg(meson->chip.dev, "fin_freq: %lu Hz\n", fin_freq); > > pre_div = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * 0xffffLL); > - if (pre_div > MISC_CLK_DIV_MASK) { > - dev_err(meson->chip.dev, "unable to get period pre_div\n"); > - return -EINVAL; > - } > + if (pre_div > MISC_CLK_DIV_MASK) > + return dev_err_probe(meson->chip.dev, -EINVAL, "unable to get period pre_div\n"); > > cnt = div64_u64(fin_freq * (u64)period, NSEC_PER_SEC * (pre_div + 1)); > - if (cnt > 0xffff) { > - dev_err(meson->chip.dev, "unable to get period cnt\n"); > - return -EINVAL; > - } > + if (cnt > 0xffff) > + return dev_err_probe(meson->chip.dev, -EINVAL, "unable to get period cnt\n"); > > dev_dbg(meson->chip.dev, "period=%u pre_div=%u cnt=%u\n", period, > pre_div, cnt); > @@ -200,10 +190,8 @@ static int meson_pwm_calc(struct meson_pwm *meson, struct pwm_device *pwm, > /* Then check is we can have the duty with the same pre_div */ > duty_cnt = div64_u64(fin_freq * (u64)duty, > NSEC_PER_SEC * (pre_div + 1)); > - if (duty_cnt > 0xffff) { > - dev_err(meson->chip.dev, "unable to get duty cycle\n"); > - return -EINVAL; > - } > + if (duty_cnt > 0xffff) > + return dev_err_probe(meson->chip.dev, -EINVAL, "unable to get duty cycle\n"); > > dev_dbg(meson->chip.dev, "duty=%u pre_div=%u duty_cnt=%u\n", > duty, pre_div, duty_cnt); > @@ -509,11 +497,8 @@ static int meson_pwm_init_channels(struct meson_pwm *meson) > channel->mux.hw.init = &init; > > channel->clk = devm_clk_register(dev, &channel->mux.hw); > - if (IS_ERR(channel->clk)) { > - err = PTR_ERR(channel->clk); > - dev_err(dev, "failed to register %s: %d\n", name, err); > - return err; > - } > + if (IS_ERR(channel->clk)) > + return dev_err_probe(dev, PTR_ERR(channel->clk), "failed to register %s\n", name);
This is the first conversion that is OK, as meson_pwm_init_channels() is (only) called by meson_pwm_probe().
> > snprintf(name, sizeof(name), "clkin%u", i);
Here follows another return that could benefit for a conversion to dev_err_probe. Currently it only returns an error code and doesn't emit a message, but a message would be good.
> @@ -550,10 +535,8 @@ static int meson_pwm_probe(struct platform_device *pdev) > return err; > > err = devm_pwmchip_add(&pdev->dev, &meson->chip); > - if (err < 0) { > - dev_err(&pdev->dev, "failed to register PWM chip: %d\n", err); > - return err; > - } > + if (err < 0) > + return dev_err_probe(&pdev->dev, err, "failed to register PWM chip\n"); > > return 0; > }
This hunk is fine.
Best regards Uwe
-- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [unhandled content-type:application/pgp-signature] | |