Re: [PATCH v4 15/18] staging: r8188eu: hal: Clean up usbctrl_vendorreq()

From: Fabio M. De Francesco
Date: Tue Sep 14 2021 - 07:18:13 EST


On Tuesday, September 14, 2021 11:24:05 AM CEST Dan Carpenter wrote:
> On Mon, Sep 13, 2021 at 08:09:59PM +0200, Fabio M. De Francesco wrote:
> > Clean up usbctrl_vendorreq () in usb_ops_linux.c because some
> > of its code will be reused in this series. This cleanup is in
> > preparation for shortening the call chains of rtw_read{8,16,32}()
> > and rtw_write{8,16,32,N}(). More insights about the reasons why at
> > https://lore.kernel.org/lkml/5319192.FrU0QrjFp7@localhost.localdomain/
> >
>
> This commit message is quite bad.
>
> This patch has nothing to do with reusing the code or shortening call
> chains.

It has to do, in a certain sense. Let me explain please...

Some days ago, David Laight made the review of "Shorten calls chain of
rtw_write8/16/32/n()" version 3.

In that patch he noticed some lines of usb_read() that I had created with the
help of reusing some lines of the code of usbctrl_vendorreq() that is deleted
in the same patch.

He thought that they were clean-ups and renames and so he suggested to make
those "clean-ups" in a separate patch.

However they were _not_ renames or other clean-ups, because usb_read() was
not touched in that patch and, above all, it was a new function.

I am sure that when I write new functions I can use whatever name of
variables I like, even if people may think I'm renaming the variables that
were in a old function that now is deleted. Am I not permitted?

However, because I also think that readability of the diffs matters, I
decided to do some clean-up of the code I'm about to reuse in the new
functions. It improves readability of the above-mentioned patch that is also
the 18/18 of this series.

That is the reason why I'm cleaning up a function that is going to be deleted
in the last patch of the series.

> Don't use a link like that in the commit message especially when it's a
> link to an email you wrote. If it's someone else's email you can say,
> something like "As <name> points out in <his/her> email <url>. Blah
> blah blah." That way you give credit to the other person but all the
> information is in the commit message.

I agree with you. I'll redo the commit message for in order to summarize in
few lines why I'm doing clean-ups of functions that must be deleted in 18/18.
The same for 16/18. I think that a short explanation like the one that I gave
you above should suffice (much shorter, obviously).

I hope that I've been clear now. Please let me know if you have more
suggestions about this patch and the next (16/18).

Regards,

Fabio