diff mbox series

[v3,3/7] iio: chemical: bme680: indent #defines consistently

Message ID 20180817190319.13119-4-dpfrey@gmail.com (mailing list archive)
State New, archived
Headers show
Series bme680 cleanup | expand

Commit Message

David Frey Aug. 17, 2018, 7:03 p.m. UTC 
Signed-off-by: David Frey <dpfrey@gmail.com>
---
 drivers/iio/chemical/bme680.h | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

Comments

Himanshu Jha Aug. 18, 2018, 11:07 a.m. UTC | #1 
On Fri, Aug 17, 2018 at 12:03:15PM -0700, David Frey wrote:
> Signed-off-by: David Frey <dpfrey@gmail.com>

Reviewed-by: Himanshu Jha <himanshujha199640@gmail.com>
Tested-by: Himanshu Jha <himanshujha199640@gmail.com>

Also, 0-day tested with build success!

Thanks
Jonathan Cameron Aug. 19, 2018, 4:02 p.m. UTC | #2 
On Sat, 18 Aug 2018 16:37:25 +0530
Himanshu Jha <himanshujha199640@gmail.com> wrote:

> On Fri, Aug 17, 2018 at 12:03:15PM -0700, David Frey wrote:
> > Signed-off-by: David Frey <dpfrey@gmail.com>  
> 
> Reviewed-by: Himanshu Jha <himanshujha199640@gmail.com>
> Tested-by: Himanshu Jha <himanshujha199640@gmail.com>
> 
> Also, 0-day tested with build success!
> 
> Thanks
> 
> 

Applied,

There is one more suspicious bit of indenting in here
#define BME680_REG_CTRL_GAS_1			0x71
#define   BME680_RUN_GAS_MASK			BIT(4)
#define   BME680_NB_CONV_MASK			GENMASK(3, 0)
#define     BME680_RUN_GAS_EN_BIT		BIT(4)
#define     BME680_NB_CONV_0_VAL		0

I think RUN_GAS_EN_BIT should be one level lower?

Thanks,

Jonathan
Himanshu Jha Aug. 19, 2018, 5:28 p.m. UTC | #3 
On Sun, Aug 19, 2018 at 05:02:15PM +0100, Jonathan Cameron wrote:
> On Sat, 18 Aug 2018 16:37:25 +0530
> Himanshu Jha <himanshujha199640@gmail.com> wrote:
> 
> > On Fri, Aug 17, 2018 at 12:03:15PM -0700, David Frey wrote:
> > > Signed-off-by: David Frey <dpfrey@gmail.com>  
> > 
> > Reviewed-by: Himanshu Jha <himanshujha199640@gmail.com>
> > Tested-by: Himanshu Jha <himanshujha199640@gmail.com>
> > 
> > Also, 0-day tested with build success!
> > 
> > Thanks
> > 
> > 
> 
> Applied,
> 
> There is one more suspicious bit of indenting in here
> #define BME680_REG_CTRL_GAS_1			0x71
> #define   BME680_RUN_GAS_MASK			BIT(4)
> #define   BME680_NB_CONV_MASK			GENMASK(3, 0)
> #define     BME680_RUN_GAS_EN_BIT		BIT(4)
> #define     BME680_NB_CONV_0_VAL		0
> 
> I think RUN_GAS_EN_BIT should be one level lower?

No, its fine. It is a value and third level indent is expected.
It should be set to 1 to initiate measurement. And
the total mask value is
	0b00001000 == BME680_RUN_GAS_EN_BIT | BME680_NB_CONV_0_VAL
Jonathan Cameron Aug. 19, 2018, 7:14 p.m. UTC | #4 
On Sun, 19 Aug 2018 22:58:44 +0530
Himanshu Jha <himanshujha199640@gmail.com> wrote:

> On Sun, Aug 19, 2018 at 05:02:15PM +0100, Jonathan Cameron wrote:
> > On Sat, 18 Aug 2018 16:37:25 +0530
> > Himanshu Jha <himanshujha199640@gmail.com> wrote:
> >   
> > > On Fri, Aug 17, 2018 at 12:03:15PM -0700, David Frey wrote:  
> > > > Signed-off-by: David Frey <dpfrey@gmail.com>    
> > > 
> > > Reviewed-by: Himanshu Jha <himanshujha199640@gmail.com>
> > > Tested-by: Himanshu Jha <himanshujha199640@gmail.com>
> > > 
> > > Also, 0-day tested with build success!
> > > 
> > > Thanks
> > > 
> > >   
> > 
> > Applied,
> > 
> > There is one more suspicious bit of indenting in here
> > #define BME680_REG_CTRL_GAS_1			0x71
> > #define   BME680_RUN_GAS_MASK			BIT(4)
> > #define   BME680_NB_CONV_MASK			GENMASK(3, 0)
> > #define     BME680_RUN_GAS_EN_BIT		BIT(4)
> > #define     BME680_NB_CONV_0_VAL		0
> > 
> > I think RUN_GAS_EN_BIT should be one level lower?  
> 
> No, its fine. It is a value and third level indent is expected.
> It should be set to 1 to initiate measurement. And
> the total mask value is
> 	0b00001000 == BME680_RUN_GAS_EN_BIT | BME680_NB_CONV_0_VAL
> 
Ah, then I'd argue it's name is wrong.  BIT elsewhere has been used
to indicate a field in the register, not a value.

Should probably just be BME680_RUN_GAS_EN?

Jonathan

>
Himanshu Jha Aug. 20, 2018, 3:37 p.m. UTC | #5 
On Sun, Aug 19, 2018 at 08:14:39PM +0100, Jonathan Cameron wrote:
> On Sun, 19 Aug 2018 22:58:44 +0530
> Himanshu Jha <himanshujha199640@gmail.com> wrote:
> 
> > On Sun, Aug 19, 2018 at 05:02:15PM +0100, Jonathan Cameron wrote:
> > > On Sat, 18 Aug 2018 16:37:25 +0530
> > > Himanshu Jha <himanshujha199640@gmail.com> wrote:
> > >   
> > > > On Fri, Aug 17, 2018 at 12:03:15PM -0700, David Frey wrote:  
> > > > > Signed-off-by: David Frey <dpfrey@gmail.com>    
> > > > 
> > > > Reviewed-by: Himanshu Jha <himanshujha199640@gmail.com>
> > > > Tested-by: Himanshu Jha <himanshujha199640@gmail.com>
> > > > 
> > > > Also, 0-day tested with build success!
> > > > 
> > > > Thanks
> > > > 
> > > >   
> > > 
> > > Applied,
> > > 
> > > There is one more suspicious bit of indenting in here
> > > #define BME680_REG_CTRL_GAS_1			0x71
> > > #define   BME680_RUN_GAS_MASK			BIT(4)
> > > #define   BME680_NB_CONV_MASK			GENMASK(3, 0)
> > > #define     BME680_RUN_GAS_EN_BIT		BIT(4)
> > > #define     BME680_NB_CONV_0_VAL		0
> > > 
> > > I think RUN_GAS_EN_BIT should be one level lower?  
> > 
> > No, its fine. It is a value and third level indent is expected.
> > It should be set to 1 to initiate measurement. And
> > the total mask value is
> > 	0b00001000 == BME680_RUN_GAS_EN_BIT | BME680_NB_CONV_0_VAL
> > 
> Ah, then I'd argue it's name is wrong.  BIT elsewhere has been used
> to indicate a field in the register, not a value.
> 
> Should probably just be BME680_RUN_GAS_EN?

Sure.
I will send a patch soon.
diff mbox series

Patch

diff --git a/drivers/iio/chemical/bme680.h b/drivers/iio/chemical/bme680.h
index e049323f209a..dd4247d364a0 100644
--- a/drivers/iio/chemical/bme680.h
+++ b/drivers/iio/chemical/bme680.h
@@ -4,10 +4,10 @@ 
 
 #define BME680_REG_CHIP_I2C_ID			0xD0
 #define BME680_REG_CHIP_SPI_ID			0x50
-#define BME680_CHIP_ID_VAL			0x61
+#define   BME680_CHIP_ID_VAL			0x61
 #define BME680_REG_SOFT_RESET_I2C		0xE0
 #define BME680_REG_SOFT_RESET_SPI		0x60
-#define BME680_CMD_SOFTRESET			0xB6
+#define   BME680_CMD_SOFTRESET			0xB6
 #define BME680_REG_STATUS			0x73
 #define   BME680_SPI_MEM_PAGE_BIT		BIT(4)
 #define     BME680_SPI_MEM_PAGE_1_VAL		1
@@ -18,6 +18,7 @@ 
 #define BME680_REG_GAS_MSB			0x2A
 #define BME680_REG_GAS_R_LSB			0x2B
 #define   BME680_GAS_STAB_BIT			BIT(4)
+#define   BME680_GAS_RANGE_MASK			0x0F
 
 #define BME680_REG_CTRL_HUMIDITY		0x72
 #define   BME680_OSRS_HUMIDITY_MASK		GENMASK(2, 0)
@@ -26,9 +27,8 @@ 
 #define   BME680_OSRS_TEMP_MASK			GENMASK(7, 5)
 #define   BME680_OSRS_PRESS_MASK		GENMASK(4, 2)
 #define   BME680_MODE_MASK			GENMASK(1, 0)
-
-#define BME680_MODE_FORCED			1
-#define BME680_MODE_SLEEP			0
+#define     BME680_MODE_FORCED			1
+#define     BME680_MODE_SLEEP			0
 
 #define BME680_REG_CONFIG			0x75
 #define   BME680_FILTER_MASK			GENMASK(4, 2)
@@ -42,13 +42,12 @@ 
 #define BME680_BIT_H1_DATA_MSK			0x0F
 
 #define BME680_REG_RES_HEAT_RANGE		0x02
-#define BME680_RHRANGE_MSK			0x30
+#define   BME680_RHRANGE_MSK			0x30
 #define BME680_REG_RES_HEAT_VAL			0x00
 #define BME680_REG_RANGE_SW_ERR			0x04
-#define BME680_RSERROR_MSK			0xF0
+#define   BME680_RSERROR_MSK			0xF0
 #define BME680_REG_RES_HEAT_0			0x5A
 #define BME680_REG_GAS_WAIT_0			0x64
-#define BME680_GAS_RANGE_MASK			0x0F
 #define BME680_ADC_GAS_RES_SHIFT		6
 #define BME680_AMB_TEMP				25