| Message ID | 20231206214817.1783227-2-u.kleine-koenig@pengutronix.de |
|---|---|
| State | Accepted |
| Delegated to: | Uwe Kleine-König |
| Headers | show |
| Series | pwm: meson: Simplify using dev_err_probe() | expand |
On Wed, Dec 06, 2023 at 10:48:18PM +0100, Uwe Kleine-König wrote: > Using dev_err_probe() emitting an error message mentioning a return > value and returning that value can be done in a single statement. Make > use of that to simplify the probe part of the driver. This has the > additional advantage to emit the symbolic name for the error instead of > the integer error value. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > Hello, > > while trying to understand Jerome's series[1] I noticed this patch > opportunity. > > Best regards > Uwe > > [1] https://lore.kernel.org/linux-pwm/20231129134004.3642121-1-jbrunet@baylibre.com > > drivers/pwm/pwm-meson.c | 35 ++++++++++++++--------------------- > 1 file changed, 14 insertions(+), 21 deletions(-) > > diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c > index 5bea53243ed2..2971bbf3b5e7 100644 > --- a/drivers/pwm/pwm-meson.c > +++ b/drivers/pwm/pwm-meson.c > @@ -468,10 +468,9 @@ static int meson_pwm_init_channels(struct meson_pwm *meson) > channel->mux.hw.init = &init; > > err = devm_clk_hw_register(dev, &channel->mux.hw); > - if (err) { > - dev_err(dev, "failed to register %s: %d\n", name, err); > - return err; > - } > + if (err) > + return dev_err_probe(dev, err, > + "failed to register %s\n", name); > > snprintf(name, sizeof(name), "%s#div%u", dev_name(dev), i); > > @@ -491,10 +490,9 @@ static int meson_pwm_init_channels(struct meson_pwm *meson) > channel->div.lock = &meson->lock; > > err = devm_clk_hw_register(dev, &channel->div.hw); > - if (err) { > - dev_err(dev, "failed to register %s: %d\n", name, err); > - return err; > - } > + if (err) > + return dev_err_probe(dev, err, > + "failed to register %s\n", name); > > snprintf(name, sizeof(name), "%s#gate%u", dev_name(dev), i); > > @@ -513,17 +511,13 @@ static int meson_pwm_init_channels(struct meson_pwm *meson) > channel->gate.lock = &meson->lock; > > err = devm_clk_hw_register(dev, &channel->gate.hw); > - if (err) { > - dev_err(dev, "failed to register %s: %d\n", name, err); > - return err; > - } > + if (err) > + return dev_err_probe(dev, err, "failed to register %s\n", name); > > channel->clk = devm_clk_hw_get_clk(dev, &channel->gate.hw, NULL); > - 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); > } > > return 0; > @@ -554,10 +548,9 @@ 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 is a lot of churn for very little gain. None of these functions are ever going to return -EPROBE_DEFER. And yes, I know that function's doc says that it is "deemed acceptable to use" elsewhere. However, the existing error messages are just fine, no need to churn just for the sake of it. Thierry
Hello Thierry, On Fri, Dec 08, 2023 at 04:52:57PM +0100, Thierry Reding wrote: > This is a lot of churn for very little gain. We seem to have different conceptions of churn. Each hunk here is an improvement for both SLOC count and usefulness of the generated error message. failed to register somename: -5 is worse than error EIO: failed to register somename , isn't it? > None of these functions are ever going to return -EPROBE_DEFER. And > yes, I know that function's doc says that it is "deemed acceptable to > use" elsewhere. However, the existing error messages are just fine, no > need to churn just for the sake of it. We had this disagreement already before. Yes dev_err_probe() is useful for three reasons and this driver only benefits from two of these. That's IMHO still one reason more than needed to justify such a change. And if you think that a function should only be used if all advantages are useful for the caller, let us reconsider if we really need capture support in the pwm framework as only two of the 68 drivers make use of it. Best regards Uwe
On Fri, Dec 08, 2023 at 08:06:20PM +0100, Uwe Kleine-König wrote: > Hello Thierry, > > On Fri, Dec 08, 2023 at 04:52:57PM +0100, Thierry Reding wrote: > > This is a lot of churn for very little gain. > > We seem to have different conceptions of churn. Each hunk here is an > improvement for both SLOC count and usefulness of the generated error > message. > > failed to register somename: -5 > > is worse than > > error EIO: failed to register somename > > , isn't it? That's entirely subjective. I think the first version is just fine. I, and I suspect most developers will, know what to do with either of those error messages. > > None of these functions are ever going to return -EPROBE_DEFER. And > > yes, I know that function's doc says that it is "deemed acceptable to > > use" elsewhere. However, the existing error messages are just fine, no > > need to churn just for the sake of it. > > We had this disagreement already before. Yes dev_err_probe() is useful > for three reasons and this driver only benefits from two of these. > That's IMHO still one reason more than needed to justify such a change. I disagree. There are certainly cases where dev_err_probe() can be a significant improvement, but there are others where the improvement is very minor (if there's any at all) and in my opinion the churn isn't justified. Otherwise we'll just forever keep rewriting the same code over and over again because somebody comes up with yet another variant of mostly the same code. > And if you think that a function should only be used if all advantages > are useful for the caller, let us reconsider if we really need capture > support in the pwm framework as only two of the 68 drivers make use of > it. That's a ridiculous argument and you know it. You are comparing apples to oranges. Thierry
Hello, On Mon, Dec 11, 2023 at 12:01:33PM +0100, Thierry Reding wrote: > On Fri, Dec 08, 2023 at 08:06:20PM +0100, Uwe Kleine-König wrote: > > On Fri, Dec 08, 2023 at 04:52:57PM +0100, Thierry Reding wrote: > > > This is a lot of churn for very little gain. > > > > We seem to have different conceptions of churn. Each hunk here is an > > improvement for both SLOC count and usefulness of the generated error > > message. > > > > failed to register somename: -5 > > > > is worse than > > > > error EIO: failed to register somename > > > > , isn't it? > > That's entirely subjective. It's not. You and me both know that -5 is EIO. But there are quite a few people who don't. And for other error codes I'm not that fluent. (Do you know -2 and -19?) Also some constants are architecture specific, so e.g. -11 is -35 on alpha. > I think the first version is just fine. I, > and I suspect most developers will, know what to do with either of those > error messages. Error messages aren't only for (kernel) developers. If you don't know that the kernel uses negative error numbers as return codes, the translation of -5 to EIO is even further away than opening /usr/include/errno.h. > > > None of these functions are ever going to return -EPROBE_DEFER. And > > > yes, I know that function's doc says that it is "deemed acceptable to > > > use" elsewhere. However, the existing error messages are just fine, no > > > need to churn just for the sake of it. > > > > We had this disagreement already before. Yes dev_err_probe() is useful > > for three reasons and this driver only benefits from two of these. > > That's IMHO still one reason more than needed to justify such a change. > > I disagree. There are certainly cases where dev_err_probe() can be a > significant improvement, but there are others where the improvement is > very minor (if there's any at all) and in my opinion the churn isn't > justified. What is churn for you? Many changes? Big changes? For me churn is only a big amount of changes where a considerable part cancels out if it was squashed together. For you this concept seems to be broader. > Otherwise we'll just forever keep rewriting the same code > over and over again because somebody comes up with yet another variant > of mostly the same code. If there is an improvement in each adaption that's fine for me. > > And if you think that a function should only be used if all advantages > > are useful for the caller, let us reconsider if we really need capture > > support in the pwm framework as only two of the 68 drivers make use of > > it. > > That's a ridiculous argument and you know it. You are comparing apples > to oranges. I would have been surprised if it had convinced you, but I honestly think there is a (admittedly small) point. Best regards Uwe
On Mon, Dec 11, 2023 at 03:19:00PM +0100, Uwe Kleine-König wrote: > Hello, > > On Mon, Dec 11, 2023 at 12:01:33PM +0100, Thierry Reding wrote: > > On Fri, Dec 08, 2023 at 08:06:20PM +0100, Uwe Kleine-König wrote: > > > On Fri, Dec 08, 2023 at 04:52:57PM +0100, Thierry Reding wrote: > > > > This is a lot of churn for very little gain. > > > > > > We seem to have different conceptions of churn. Each hunk here is an > > > improvement for both SLOC count and usefulness of the generated error > > > message. > > > > > > failed to register somename: -5 > > > > > > is worse than > > > > > > error EIO: failed to register somename > > > > > > , isn't it? > > > > That's entirely subjective. > > It's not. You and me both know that -5 is EIO. But there are quite a few > people who don't. So it is, in fact, subjective. > And for other error codes I'm not that fluent. (Do you > know -2 and -19?) Also some constants are architecture specific, so e.g. > -11 is -35 on alpha. I didn't know about -11 and -35 on Alpha. Looks like %pe would handle those properly, so yeah, I suppose one could count that as a benefit. Not sure if we have PWM drivers that run on Alpha, and Meson in particular probably doesn't. > > I think the first version is just fine. I, > > and I suspect most developers will, know what to do with either of those > > error messages. > > Error messages aren't only for (kernel) developers. If you don't know > that the kernel uses negative error numbers as return codes, the > translation of -5 to EIO is even further away than opening > /usr/include/errno.h. Actually, kernel developers are exactly who these error messages are for. Regular users that don't know how to decipher the error codes are typically not going to know what to do about the error anyway, so they are likely just going to copy/paste into some forum or bug tracker. > > > > None of these functions are ever going to return -EPROBE_DEFER. And > > > > yes, I know that function's doc says that it is "deemed acceptable to > > > > use" elsewhere. However, the existing error messages are just fine, no > > > > need to churn just for the sake of it. > > > > > > We had this disagreement already before. Yes dev_err_probe() is useful > > > for three reasons and this driver only benefits from two of these. > > > That's IMHO still one reason more than needed to justify such a change. > > > > I disagree. There are certainly cases where dev_err_probe() can be a > > significant improvement, but there are others where the improvement is > > very minor (if there's any at all) and in my opinion the churn isn't > > justified. > > What is churn for you? Many changes? Big changes? For me churn is only a > big amount of changes where a considerable part cancels out if it was > squashed together. For you this concept seems to be broader. Churn for me is really any kind of change and it's not bad per se. What I don't like are changes that are basically done just for the sake of changing something. I don't have any strict rules for this, so I apply common sense. If you want to rewrite an error message because it contains typos or bad grammar, or is generally hard to understand, that's what I'd consider fine and an improvement. But this patch exchanges one format of the error message by another. It doesn't change the error message or the information content in any way, but instead just rearranges where the error is printed. On top of it not adding any benefit, this might cause somebody to get confused because some error message that they were looking out for is now different. They may have to adjust a script or something. You also mentioned that you saw the opportunity to do this while reviewing Jerome's patches and looking at those patches, Jerome will now have to solve a couple of conflicts when rebasing. They should admittedly be fairly minor, but if Jerome wasn't experienced this might be really annoying. Again, if this was a significant improvement I'm sure this would be easily acceptable, but if it's just for format's sake I'm not so sure it is. > > Otherwise we'll just forever keep rewriting the same code > > over and over again because somebody comes up with yet another variant > > of mostly the same code. > > If there is an improvement in each adaption that's fine for me. I can't speak for other maintainers, but I get very annoyed every time somebody introduces some new helper and then I get to deal with 60 or so patches just because there's a new thing that ultimately doesn't really change or improve anything. It's easier to apply 60 patches today that it was perhaps a few years ago, but it also entails a bunch of things like reviewing the code because sometimes even trivial conversions are faulty, the nrunning test builds and pushing changes. It all adds up in the end and keeps me from doing other things. Again, if this all has some sort of benefit it makes sense to put in the time, but if it's just for the sake of shuffling around parts of error messages it just makes me grumpy. Unfortunately, with all of that said I probably have no other choice but to apply this patch because if I don't, then I know somebody else will send the very same patch at some point... Thierry
Hello Thierry, On Mon, Dec 11, 2023 at 04:24:35PM +0100, Thierry Reding wrote: > On Mon, Dec 11, 2023 at 03:19:00PM +0100, Uwe Kleine-König wrote: > > On Mon, Dec 11, 2023 at 12:01:33PM +0100, Thierry Reding wrote: > > > On Fri, Dec 08, 2023 at 08:06:20PM +0100, Uwe Kleine-König wrote: > > > > On Fri, Dec 08, 2023 at 04:52:57PM +0100, Thierry Reding wrote: > > > > > This is a lot of churn for very little gain. > > > > > > > > We seem to have different conceptions of churn. Each hunk here is an > > > > improvement for both SLOC count and usefulness of the generated error > > > > message. > > > > > > > > failed to register somename: -5 > > > > > > > > is worse than > > > > > > > > error EIO: failed to register somename > > > > > > > > , isn't it? > > > > > > That's entirely subjective. > > > > It's not. You and me both know that -5 is EIO. But there are quite a few > > people who don't. > > So it is, in fact, subjective. For some individuals it's an improvement and for others it doesn't make things worse. So in sum it's an objective improvement. > > And for other error codes I'm not that fluent. (Do you > > know -2 and -19?) Also some constants are architecture specific, so e.g. > > -11 is -35 on alpha. > > I didn't know about -11 and -35 on Alpha. Looks like %pe would handle > those properly, so yeah, I suppose one could count that as a benefit. > Not sure if we have PWM drivers that run on Alpha, and Meson in > particular probably doesn't. So using a symbolic error number (e.g. by using dev_err_probe) makes it unnecessary to think about which arch the given driver runs on and if the returned values might differ by architecture. One less problem to think about, that's an advantage. Also the next person to write a driver on alpha (or another platform where the error codes differ from those on x86) will probably pick an existing driver as a template. So the meson driver being robust to run on alpha is a good thing. > > > I think the first version is just fine. I, > > > and I suspect most developers will, know what to do with either of those > > > error messages. > > > > Error messages aren't only for (kernel) developers. If you don't know > > that the kernel uses negative error numbers as return codes, the > > translation of -5 to EIO is even further away than opening > > /usr/include/errno.h. > > Actually, kernel developers are exactly who these error messages are > for. Regular users that don't know how to decipher the error codes are > typically not going to know what to do about the error anyway, so they > are likely just going to copy/paste into some forum or bug tracker. That seems to be a very cocky POV to me. But that explains a lot. The kernel community has a very l33t reputation, lowering the bar for newbies is a nice move IMHO. Also EIO in a forum or bug tracker is more relevantly better, because there are more readers that then all individually have to interpret the -5 and know/research it's -EIO. > > > > > None of these functions are ever going to return -EPROBE_DEFER. And > > > > > yes, I know that function's doc says that it is "deemed acceptable to > > > > > use" elsewhere. However, the existing error messages are just fine, no > > > > > need to churn just for the sake of it. > > > > > > > > We had this disagreement already before. Yes dev_err_probe() is useful > > > > for three reasons and this driver only benefits from two of these. > > > > That's IMHO still one reason more than needed to justify such a change. > > > > > > I disagree. There are certainly cases where dev_err_probe() can be a > > > significant improvement, but there are others where the improvement is > > > very minor (if there's any at all) and in my opinion the churn isn't > > > justified. > > > > What is churn for you? Many changes? Big changes? For me churn is only a > > big amount of changes where a considerable part cancels out if it was > > squashed together. For you this concept seems to be broader. > > Churn for me is really any kind of change and it's not bad per se. What > I don't like are changes that are basically done just for the sake of > changing something. That's not my motivation. > I don't have any strict rules for this, so I apply > common sense. If you want to rewrite an error message because it > contains typos or bad grammar, or is generally hard to understand, > that's what I'd consider fine and an improvement. I'd count "-5" as "generally hard to understand". > But this patch exchanges one format of the error message by another. It > doesn't change the error message or the information content in any way, > but instead just rearranges where the error is printed. > > On top of it not adding any benefit, this might cause somebody to get > confused because some error message that they were looking out for is > now different. They may have to adjust a script or something. Yeah sure, xkcd#1172. Best regards Uwe
Hello Thierry,
On Mon, Dec 11, 2023 at 04:24:35PM +0100, Thierry Reding wrote:
> It all adds up in the end and keeps me from doing other things.
If that means you'd be glad to give up the PWM maintainer job, I'd
happily take over this post.
Best regards
Uwe
On Tue, Dec 12, 2023 at 09:33:52PM +0100, Uwe Kleine-König wrote: > Hello Thierry, > > On Mon, Dec 11, 2023 at 04:24:35PM +0100, Thierry Reding wrote: > > It all adds up in the end and keeps me from doing other things. > > If that means you'd be glad to give up the PWM maintainer job, I'd > happily take over this post. "Glad" is not the word that I would choose. After all I've looked after this subsystem for almost 12 years, and letting it go isn't something that is particularly easy. However, I do realize that my heart isn't in it anymore and I don't want to be in the way of anyone. So I'll take you up on that offer. Do you want to send a patch? Thierry
diff --git a/drivers/pwm/pwm-meson.c b/drivers/pwm/pwm-meson.c index 5bea53243ed2..2971bbf3b5e7 100644 --- a/drivers/pwm/pwm-meson.c +++ b/drivers/pwm/pwm-meson.c @@ -468,10 +468,9 @@ static int meson_pwm_init_channels(struct meson_pwm *meson) channel->mux.hw.init = &init; err = devm_clk_hw_register(dev, &channel->mux.hw); - if (err) { - dev_err(dev, "failed to register %s: %d\n", name, err); - return err; - } + if (err) + return dev_err_probe(dev, err, + "failed to register %s\n", name); snprintf(name, sizeof(name), "%s#div%u", dev_name(dev), i); @@ -491,10 +490,9 @@ static int meson_pwm_init_channels(struct meson_pwm *meson) channel->div.lock = &meson->lock; err = devm_clk_hw_register(dev, &channel->div.hw); - if (err) { - dev_err(dev, "failed to register %s: %d\n", name, err); - return err; - } + if (err) + return dev_err_probe(dev, err, + "failed to register %s\n", name); snprintf(name, sizeof(name), "%s#gate%u", dev_name(dev), i); @@ -513,17 +511,13 @@ static int meson_pwm_init_channels(struct meson_pwm *meson) channel->gate.lock = &meson->lock; err = devm_clk_hw_register(dev, &channel->gate.hw); - if (err) { - dev_err(dev, "failed to register %s: %d\n", name, err); - return err; - } + if (err) + return dev_err_probe(dev, err, "failed to register %s\n", name); channel->clk = devm_clk_hw_get_clk(dev, &channel->gate.hw, NULL); - 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); } return 0; @@ -554,10 +548,9 @@ 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; }
Using dev_err_probe() emitting an error message mentioning a return value and returning that value can be done in a single statement. Make use of that to simplify the probe part of the driver. This has the additional advantage to emit the symbolic name for the error instead of the integer error value. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- Hello, while trying to understand Jerome's series[1] I noticed this patch opportunity. Best regards Uwe [1] https://lore.kernel.org/linux-pwm/20231129134004.3642121-1-jbrunet@baylibre.com drivers/pwm/pwm-meson.c | 35 ++++++++++++++--------------------- 1 file changed, 14 insertions(+), 21 deletions(-)