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

[Uwe Kleine-König]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-amlogic/attachments/20231211/86075565/attachment.sig>