Re: [PATCH] staging: sm750fb: fix instances of camel case

[Greg Korah-Hartman <gregkh@linuxfoundation.org>]

On Thu, Apr 17, 2025 at 06:28:07PM +0100, Ruben Wauters wrote:
> On Thu, 2025-04-17 at 18:58 +0200, Greg Korah-Hartman wrote:
> > On Thu, Apr 17, 2025 at 04:27:47PM +0100, Ruben Wauters wrote:
> > > As per the kernel style guidelines, and as reported by
> > > checkpatch.pl,
> > > replaced instances of camel case with snake_case where appropriate
> > > and
> > > aligned names in the header with those in the c file.
> > > 
> > > Signed-off-by: Ruben Wauters <rubenru09@aol.com>
> > > ---
> > >  drivers/staging/sm750fb/ddk750_sii164.c | 113 ++++++++++++--------
> > > ----
> > >  drivers/staging/sm750fb/ddk750_sii164.h |  26 +++---
> > >  2 files changed, 69 insertions(+), 70 deletions(-)
> > > 
> > > diff --git a/drivers/staging/sm750fb/ddk750_sii164.c
> > > b/drivers/staging/sm750fb/ddk750_sii164.c
> > > index 89700fc5dd2e..20c2f386220c 100644
> > > --- a/drivers/staging/sm750fb/ddk750_sii164.c
> > > +++ b/drivers/staging/sm750fb/ddk750_sii164.c
> > > @@ -12,11 +12,11 @@
> > >  #define USE_HW_I2C
> > >  
> > >  #ifdef USE_HW_I2C
> > > -    #define i2cWriteReg sm750_hw_i2c_write_reg
> > > -    #define i2cReadReg  sm750_hw_i2c_read_reg
> > > +    #define I2C_WRITE_REG sm750_hw_i2c_write_reg
> > > +    #define I2C_READ_REG  sm750_hw_i2c_read_reg
> > 
> > Close, but these are really a function name, not a macro, right?
> > 
> > And what sets this #define?  If it's always enabled, then unwrap this
> > indirection instead of keeping it around
> 
> Will take a look into it, if it turns out that this is in fact actually
> used/different, what would be the best way to name this? checkpatch.pl
> doesn't like the camelCase that's currently there.
> 
> > >  #else
> > > -    #define i2cWriteReg sm750_sw_i2c_write_reg
> > > -    #define i2cReadReg  sm750_sw_i2c_read_reg
> > > +    #define I2C_WRITE_REG sm750_sw_i2c_write_reg
> > > +    #define I2C_READ_REG  sm750_sw_i2c_read_reg
> > >  #endif
> > >  
> > >  /* SII164 Vendor and Device ID */
> > > @@ -25,7 +25,7 @@
> > >  
> > >  #ifdef SII164_FULL_FUNCTIONS
> > >  /* Name of the DVI Controller chip */
> > > -static char *gDviCtrlChipName = "Silicon Image SiI 164";
> > > +static char *dvi_controller_chip_name = "Silicon Image SiI 164";
> > 
> > This is a totally different thing.
> 
> It is, however I believe it is somewhat more descriptive, I suppose it
> doesn't really matter though and if it should be the same, just made
> snake_case, I can do that.
> > 
> > >  #endif
> > >  
> > >  /*
> > > @@ -37,14 +37,14 @@ static char *gDviCtrlChipName = "Silicon Image
> > > SiI 164";
> > >   */
> > >  unsigned short sii164_get_vendor_id(void)
> > >  {
> > > -	unsigned short vendorID;
> > > +	unsigned short vendor;
> > 
> > Why change this?
> 
> Again removing camelCase, kernel style guide says that shorter names
> are preferred (unless I misinterpreted that), I could make it vendor_id
> if that is preferred, I believe the same would be with device_id below
> it.
> 
> > This is a mix of lots of different changes, please break things up
> > into
> > "one logical change per patch"
> 
> Would it be best to split each rename (function or variable) into a
> separate patch? I do agree it is quite a lot and I was a little unsure
> when sending this one, but I also don't want to make a lot of different
> patches and spam your email with 100 patches at once.

Do "one logical thing at a time".  Think about what you would want to
see if you were the reviewer.  Sometimes yes, 100 patches is fine, but
really, make it in smaller chunks as odds are something is going to go
wrong with that many at once.

See all of the other patches on the list for examples, you have
thousands to learn from :)

good luck!

greg k-h