[PATCH] pwm: meson: Simplify using dev_err_probe()

[Thierry Reding]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-amlogic/attachments/20231211/d49cc4f5/attachment.sig>