On Sun, Sep 11, 2022 at 12:46:04PM +0200, Philipp Hortmann wrote: > Rename function MACbSafeSoftwareReset to vt6655_mac_save_soft_reset and > abyTmpRegData to tmp_reg_data to avoid CamelCase which is not accepted by > checkpatch.pl. Remove return value bRetVal as it is unused by the calling > functions. Please don't mix this kind of stuff into a patch like this. > Remove unnecessary declaration of function and make function > static. Change declaration of tmp_reg_data to shorten code. When I'm reviewing rename patches I have a script where I call `rename_rev.pl -a` and it just parses the patch and outputs: RENAMES: abyTmpRegData => tmp_reg_data MACbSafeSoftwareReset => vt6655_mac_save_soft_reset I don't invest much time in thinking about the new names. The review takes me about 5 seconds. Then once you start marking the functions as static then that's slightly a headache because now I have to look at that part by hand. But whatever, you can kind of sell that as one thing. But then when you mix ignoring the error codes in as well it's a big headache. At first I thought it was because MACbSoftwareReset() always returns true, so I pulled up that code and that's not true. So then I am annoyed. And I pull up the commit message to see what's going on and sure enough that change is burried in the middle of the paragraph. Don't do that. Just do the renames. Then change all the functions in one file to static at once. Then make the function void in a separate patch. The other thing is that making functions static is not at all controversial. So long as it builds we will always accept those patches. Renaming variables is not super controversial. Sometimes people quarrel about the new name. For example, I don't like a tmp variable which has a long name like "tmp_reg_data". Just call it either "tmp" or "reg_data". Not both. Don't write an essay. But making functions void is sort of controversial... It's probably the right thing in this context. But what I'm saying is don't mix things which different controversial levels, because it means that someone is going to complain. I would have ignored the "tmp_reg_data" thing except now I'm already complaining about stuff I may as well mention it. regards, dan carpenter
