Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 13 Sep 2022 13:12:55 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Philipp Hortmann <philipp.g.hortmann@gmail.com>
Cc:     Forest Bond <forest@alittletooquiet.net>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 04/12] staging: vt6655: Cleanup and rename function
 MACbSafeSoftwareReset
Message-ID: <YyBXp9x1LnBkku2g@kadam>
References: <cover.1662890990.git.philipp.g.hortmann@gmail.com>
 <540a684266610f7618b3ef6000d4699d065c8e6f.1662890990.git.philipp.g.hortmann@gmail.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <540a684266610f7618b3ef6000d4699d065c8e6f.1662890990.git.philipp.g.hortmann@gmail.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?gc73hUrGW9bXurmZ579p7iQEBzoAKI7ndEAx+zbh7cfezBG+ktDMl83LSQ4K?=
 =?us-ascii?Q?Iar8BGQOfG7I2Z1Q8YOcDXLHzX6+cwB88avjLW8XTRDDt0NyGZD5rMWqpkC7?=
 =?us-ascii?Q?Sht25VI7Fq3t2texOaEY38WXV1sr59et/sQmfmAq25z3FPNz15CjGardhrp/?=
 =?us-ascii?Q?CMwj7KmAxEBNALio722M77+uDz6afDsIYhDWV+Px3NAr324e6u0ePKDb7rUK?=
 =?us-ascii?Q?NuwaOw3qcFj8PxktMuZLd1zgqV18Pl8yrMyRM3oQxBKnqSm1PYYlisdDTZaM?=
 =?us-ascii?Q?Tf3JS0Hu8e3rX78mhskoLQMCor1ShvTY2kxgluvN8vJwPm/O7sl4w8zufqxu?=
 =?us-ascii?Q?WDrmLMhHxeX5vJ+vzhEB+531ohgMZ3mP+MH9hXD40Aq268sqbAFBnYOI8sQ8?=
 =?us-ascii?Q?W6kCp1hgHGUYK+YFbv8Qk3snhc3ylk5FRtSrzCAcuRQxMs6WNNosxB+e9p0U?=
 =?us-ascii?Q?CjMgQ1N7jMdjqFs1J/PET8BX6esVmKhg+cAky+m+2lHVgbnRfTilZgYKLzUi?=
 =?us-ascii?Q?60QZWxH5rCULupFkXA8qAADyiXClWBh9et7yz3udGU9xEh1suL499Y9BNG3g?=
 =?us-ascii?Q?KRSL70hdQy/IEIHFUALDO6tkXeIWsIdMfneTDjsc0x+zlvbd/UbQEpcu1YyY?=
 =?us-ascii?Q?yKi1GIEF4cuHkD6qAe70xeA6VmB/FU4HCFuYOM1NAoNaADGEb+RfLnNYvmXq?=
 =?us-ascii?Q?potGHjpFSpab/CmBTsxbFsBSFkCREeCbQGbW6zDgBfQ+ACCTf/q0jwNhjtS0?=
 =?us-ascii?Q?xg0J7IaFy27Y2WlKHcICQ8hLi6evMmAjTs8at2KrUdkcKSc+vZQnHXWdwecH?=
 =?us-ascii?Q?d72GmB0+BN8kZQBzAr6jj5Sf8ZsJbgDl51zGRR7KE3dmbyQXSzjOrzUgMLey?=
 =?us-ascii?Q?4CeCHwlgtWOBQjQdgVN6I7QK0gH/JHbIZoWhZjmrirlIg9NV1oQlqg5i6Zcm?=
 =?us-ascii?Q?vAjE5YHz4keq1p8HQ+WjuAXwqFHvMgUzCzwMkbkOF1Qz17yDjlfHeFeYeupt?=
 =?us-ascii?Q?CKFrI/4SbagsjwNbkZKX7GDN8HH445D735XX15RGB1ucw4E5W4l7kkMatwJQ?=
 =?us-ascii?Q?Wp3kMGWBgU1mfA0ik+AvMma5ACSlheoo0v2jYMbkA6x4cce0IvYTS1cMDtKU?=
 =?us-ascii?Q?CrRYbVPkMLR0q2aKJ/PzwHxz+QHLCCpzxrLIykS/A8GdvTQxVWcpJJg3gqXf?=
 =?us-ascii?Q?ScdZd4RsoY5R+lKWAXOaS0ewJpK9FYS2IfLG04iLy9xrjVjMChMKEVcvaOs1?=
 =?us-ascii?Q?BZ/h6pA0s97VnYnb2kvDIHwuekKSSXLmi2AFgSG+wGm2Wt6nuAPo4p4J45Ep?=
 =?us-ascii?Q?yCYWKwL0Ay7FNKHVSm70iNd7krzmNlRVYH8C8jucFzzOGGHAJfAAG3zWmeL+?=
 =?us-ascii?Q?zJix+gvsm6ezsyCFj5Hz4n7M0O5/it0OYMbgPSH+8seGfRfqcUz0D0TwKyam?=
 =?us-ascii?Q?E8Kl8ejSYrBxUZ56fNjdVA05YsKg7QcykgeYX/hihLJMcRBkckWUzNMnDzrV?=
 =?us-ascii?Q?e8MbDcuvIiP0dsBISxlI9SLxrMM9U50arMWnGMF0gXzeUM1Wi/HPUNndo7/j?=
 =?us-ascii?Q?8zKX2aMwAHAveX8lHeiuZhCbFGAYI9KHyCX8Q2od7fqSCtdIBT/XjH3IaUtk?=
 =?us-ascii?Q?zA=3D=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 404df514-0ae4-44a2-fbe9-08da95708ec7
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2365.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Sep 2022 10:13:09.3661
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 4gwCojduqZT+9oPB3GX/qw8cUZotQpsTFZursPDWshmDNQICSp+mX2+i4w53qHHNJPydiFTIchO/9TrBTH+JOZ7JDDhzcpdTxlu+90E+ZLA=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR10MB4550
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.528,FMLib:17.11.122.1
 definitions=2022-09-13_03,2022-09-13_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 bulkscore=0 spamscore=0
 mlxscore=0 phishscore=0 suspectscore=0 malwarescore=0 adultscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2207270000
 definitions=main-2209130045
X-Proofpoint-ORIG-GUID: 5wISrQe3F9VsFLSxOsl4FZdFTyaDoAvJ
X-Proofpoint-GUID: 5wISrQe3F9VsFLSxOsl4FZdFTyaDoAvJ
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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