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. Remove unnecessary declaration of function and make function
static. Change declaration of tmp_reg_data to shorten code.

Signed-off-by: Philipp Hortmann <philipp.g.hortmann@gmail.com>
---
 drivers/staging/vt6655/mac.c | 17 +++++++----------
 drivers/staging/vt6655/mac.h |  1 -
 2 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/vt6655/mac.c b/drivers/staging/vt6655/mac.c
index b1aa5fbe4430..f292a34c23dd 100644
--- a/drivers/staging/vt6655/mac.c
+++ b/drivers/staging/vt6655/mac.c
@@ -293,23 +293,20 @@ bool MACbSoftwareReset(struct vnt_private *priv)
  * Return Value: true if success; otherwise false
  *
  */
-bool MACbSafeSoftwareReset(struct vnt_private *priv)
+static void vt6655_mac_save_soft_reset(struct vnt_private *priv)
 {
-	unsigned char abyTmpRegData[MAC_MAX_CONTEXT_SIZE_PAGE0 + MAC_MAX_CONTEXT_SIZE_PAGE1];
-	bool bRetVal;
+	u8 tmp_reg_data[MAC_MAX_CONTEXT_SIZE_PAGE0 + MAC_MAX_CONTEXT_SIZE_PAGE1];
 
 	/* PATCH....
 	 * save some important register's value, then do
 	 * reset, then restore register's value
 	 */
 	/* save MAC context */
-	vt6655_mac_save_context(priv, abyTmpRegData);
+	vt6655_mac_save_context(priv, tmp_reg_data);
 	/* do reset */
-	bRetVal = MACbSoftwareReset(priv);
+	MACbSoftwareReset(priv);
 	/* restore MAC context, except CR0 */
-	vt6655_mac_restore_context(priv, abyTmpRegData);
-
-	return bRetVal;
+	vt6655_mac_restore_context(priv, tmp_reg_data);
 }
 
 /*
@@ -443,12 +440,12 @@ bool MACbSafeStop(struct vnt_private *priv)
 
 	if (!MACbSafeRxOff(priv)) {
 		pr_debug(" MACbSafeRxOff == false)\n");
-		MACbSafeSoftwareReset(priv);
+		vt6655_mac_save_soft_reset(priv);
 		return false;
 	}
 	if (!MACbSafeTxOff(priv)) {
 		pr_debug(" MACbSafeTxOff == false)\n");
-		MACbSafeSoftwareReset(priv);
+		vt6655_mac_save_soft_reset(priv);
 		return false;
 	}
 
diff --git a/drivers/staging/vt6655/mac.h b/drivers/staging/vt6655/mac.h
index 25247b0bf039..5dd8644749ec 100644
--- a/drivers/staging/vt6655/mac.h
+++ b/drivers/staging/vt6655/mac.h
@@ -554,7 +554,6 @@ void vt6655_mac_set_short_retry_limit(struct vnt_private *priv, unsigned char re
 void MACvSetLongRetryLimit(struct vnt_private *priv, unsigned char byRetryLimit);
 
 bool MACbSoftwareReset(struct vnt_private *priv);
-bool MACbSafeSoftwareReset(struct vnt_private *priv);
 bool MACbSafeRxOff(struct vnt_private *priv);
 bool MACbSafeTxOff(struct vnt_private *priv);
 bool MACbSafeStop(struct vnt_private *priv);
-- 
2.37.3

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

On 9/13/22 12:12, Dan Carpenter wrote:
> 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.
> 

In the past Greg let me know that I used to many patches for a change in 
the same lines of code and that it would be easier for him to review 
when less patches do the same.

As you wrote I am changing to many things at once. Sorry for breaking 
your automatism.

I will consider your hints for the next patches.

Thanks

Bye Philipp

On Tue, Sep 13, 2022 at 08:26:04PM +0200, Philipp Hortmann wrote:
> On 9/13/22 12:12, Dan Carpenter wrote:
> > 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.
> > 
> 
> In the past Greg let me know that I used to many patches for a change in the
> same lines of code and that it would be easier for him to review when less
> patches do the same.
> 

It's some kind of an art to write patches that are easy to review...

regards,
dan carpenter