diff mbox series

[v1,02/13] iio: chemical: bme680: avoid using camel case

Message ID 20241010210030.33309-3-vassilisamir@gmail.com (mailing list archive)
State Changes Requested
Headers show
Series : chemical: bme680: 2nd set of clean and add | expand

Commit Message

Vasileios Amoiridis Oct. 10, 2024, 9 p.m. UTC 
From: Vasileios Amoiridis <vassilisamir@gmail.com>

Rename camel case variable, as checkpatch.pl complains.

Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/chemical/bme680_core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Andy Shevchenko Oct. 11, 2024, 10 a.m. UTC | #1 
On Thu, Oct 10, 2024 at 11:00:19PM +0200, vamoirid wrote:
> From: Vasileios Amoiridis <vassilisamir@gmail.com>
> 
> Rename camel case variable, as checkpatch.pl complains.

With given reply to the first patch...

...

>  	/* Look up table for the possible gas range values */
> -	static const u32 lookupTable[16] = {2147483647u, 2147483647u,
> +	static const u32 lookup_table[16] = {2147483647u, 2147483647u,
>  				2147483647u, 2147483647u, 2147483647u,
>  				2126008810u, 2147483647u, 2130303777u,
>  				2147483647u, 2147483647u, 2143188679u,

...here is the opportunity to fix indentation while at fixing the code.
I.o.w. I would reformat the entire table to be

	static const u32 lookup_table[16] = {
		2147483647u, 2147483647u, 2147483647u, 2147483647u,
		2147483647u, 2126008810u, 2147483647u, 2130303777u,
		2147483647u, 2147483647u, 2143188679u, ...

(also note power-of-2 number of items per line which much easier to read and
 find one you need).

...

>  	var1 = ((1340 + (5 * (s64)calib->range_sw_err)) *
> -			((s64)lookupTable[gas_range])) >> 16;
> +			((s64)lookup_table[gas_range])) >> 16;

Also an opportunity to make this neater like

	var1 = (1340 + (5 * (s64)calib->range_sw_err)) * (s64)lookup_table[gas_range]);
	var1 >>= 16;

So, at bare minumym there are redundant parentheses. And looking at the table
and the first argument of multiplication I'm puzzled why casting is needed for
the second? Shouldn't s64 already be implied by the first one?
Vasileios Amoiridis Oct. 11, 2024, 6:50 p.m. UTC | #2 
On Fri, Oct 11, 2024 at 01:00:33PM +0300, Andy Shevchenko wrote:
> On Thu, Oct 10, 2024 at 11:00:19PM +0200, vamoirid wrote:
> > From: Vasileios Amoiridis <vassilisamir@gmail.com>
> > 
> > Rename camel case variable, as checkpatch.pl complains.
> 
> With given reply to the first patch...
>

Hi Andy,

> ...
> 
> >  	/* Look up table for the possible gas range values */
> > -	static const u32 lookupTable[16] = {2147483647u, 2147483647u,
> > +	static const u32 lookup_table[16] = {2147483647u, 2147483647u,
> >  				2147483647u, 2147483647u, 2147483647u,
> >  				2126008810u, 2147483647u, 2130303777u,
> >  				2147483647u, 2147483647u, 2143188679u,
> 
> ...here is the opportunity to fix indentation while at fixing the code.
> I.o.w. I would reformat the entire table to be
> 
> 	static const u32 lookup_table[16] = {
> 		2147483647u, 2147483647u, 2147483647u, 2147483647u,
> 		2147483647u, 2126008810u, 2147483647u, 2130303777u,
> 		2147483647u, 2147483647u, 2143188679u, ...
> 
> (also note power-of-2 number of items per line which much easier to read and
>  find one you need).
> 

ACK.

> ...
> 
> >  	var1 = ((1340 + (5 * (s64)calib->range_sw_err)) *
> > -			((s64)lookupTable[gas_range])) >> 16;
> > +			((s64)lookup_table[gas_range])) >> 16;
> 
> Also an opportunity to make this neater like
> 
> 	var1 = (1340 + (5 * (s64)calib->range_sw_err)) * (s64)lookup_table[gas_range]);
> 	var1 >>= 16;
> 
> So, at bare minumym there are redundant parentheses. And looking at the table
> and the first argument of multiplication I'm puzzled why casting is needed for
> the second? Shouldn't s64 already be implied by the first one?
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>

I think the 2nd cast indeed is not needed since the 1st one forces the
s64 so I can remove it.

Cheers,
Vasilis
Andy Shevchenko Oct. 12, 2024, 8:34 p.m. UTC | #3 
Fri, Oct 11, 2024 at 08:50:27PM +0200, Vasileios Aoiridis kirjoitti:
> On Fri, Oct 11, 2024 at 01:00:33PM +0300, Andy Shevchenko wrote:
> > On Thu, Oct 10, 2024 at 11:00:19PM +0200, vamoirid wrote:

...

> > >  	var1 = ((1340 + (5 * (s64)calib->range_sw_err)) *
> > > -			((s64)lookupTable[gas_range])) >> 16;
> > > +			((s64)lookup_table[gas_range])) >> 16;
> > 
> > Also an opportunity to make this neater like
> > 
> > 	var1 = (1340 + (5 * (s64)calib->range_sw_err)) * (s64)lookup_table[gas_range]);
> > 	var1 >>= 16;
> > 
> > So, at bare minumym there are redundant parentheses. And looking at the table
> > and the first argument of multiplication I'm puzzled why casting is needed for
> > the second? Shouldn't s64 already be implied by the first one?
> 
> I think the 2nd cast indeed is not needed since the 1st one forces the
> s64 so I can remove it.

Thinking about this more, you don't need the first either,
if using 5LL instead of 5.
diff mbox series

Patch

diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
index 95a667d56527..3b4431998ca4 100644
--- a/drivers/iio/chemical/bme680_core.c
+++ b/drivers/iio/chemical/bme680_core.c
@@ -437,7 +437,7 @@  static u32 bme680_compensate_gas(struct bme680_data *data, u16 gas_res_adc,
 	u32 calc_gas_res;
 
 	/* Look up table for the possible gas range values */
-	static const u32 lookupTable[16] = {2147483647u, 2147483647u,
+	static const u32 lookup_table[16] = {2147483647u, 2147483647u,
 				2147483647u, 2147483647u, 2147483647u,
 				2126008810u, 2147483647u, 2130303777u,
 				2147483647u, 2147483647u, 2143188679u,
@@ -445,7 +445,7 @@  static u32 bme680_compensate_gas(struct bme680_data *data, u16 gas_res_adc,
 				2147483647u, 2147483647u};
 
 	var1 = ((1340 + (5 * (s64)calib->range_sw_err)) *
-			((s64)lookupTable[gas_range])) >> 16;
+			((s64)lookup_table[gas_range])) >> 16;
 	var2 = ((gas_res_adc << 15) - 16777216) + var1;
 	var3 = ((125000 << (15 - gas_range)) * var1) >> 9;
 	var3 += (var2 >> 1);