[PATCH] pinctrl: tegra: use signed bitfields for optional fields

[PATCH] pinctrl: tegra: use signed bitfields for optional fields

[Stefan Agner]

Optional fields are set to -1 by various preprocessor macros. Make
sure the struct fields can actually store them.

Signed-off-by: Stefan Agner <stefan-XLVq0VzYD2Y@public.gmane.org>
---
This lead to a lot of warnings when compiling the Tegra pinctrl drivers
using LLVM/clang:

drivers/pinctrl/pinctrl-tegra124.c:2048:2: warning: implicit truncation from
'int' to bitfield changes value from -1 to 63 [-Wbitfield-constant-conversion]
        MIPI_PAD_CTRL_PINGROUP(dsi_b,   0x820, 1, CSI, DSI_B)
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/pinctrl/pinctrl-tegra124.c:1807:18: note: expanded from macro
'MIPI_PAD_CTRL_PINGROUP'
                .rcv_sel_bit = -1,                                      \
                               ^~

However, I did not check if this could actually lead to an unintended
pin configuration...

 drivers/pinctrl/pinctrl-tegra.h | 48 ++++++++++++++++++++---------------------
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-tegra.h b/drivers/pinctrl/pinctrl-tegra.h
index 8d94d13..0711ae62 100644
--- a/drivers/pinctrl/pinctrl-tegra.h
+++ b/drivers/pinctrl/pinctrl-tegra.h
@@ -135,30 +135,30 @@ struct tegra_pingroup {
 	s16 pupd_reg;
 	s16 tri_reg;
 	s16 drv_reg;
-	u32 mux_bank:2;
-	u32 pupd_bank:2;
-	u32 tri_bank:2;
-	u32 drv_bank:2;
-	u32 mux_bit:6;
-	u32 pupd_bit:6;
-	u32 tri_bit:6;
-	u32 einput_bit:6;
-	u32 odrain_bit:6;
-	u32 lock_bit:6;
-	u32 ioreset_bit:6;
-	u32 rcv_sel_bit:6;
-	u32 hsm_bit:6;
-	u32 schmitt_bit:6;
-	u32 lpmd_bit:6;
-	u32 drvdn_bit:6;
-	u32 drvup_bit:6;
-	u32 slwr_bit:6;
-	u32 slwf_bit:6;
-	u32 drvtype_bit:6;
-	u32 drvdn_width:6;
-	u32 drvup_width:6;
-	u32 slwr_width:6;
-	u32 slwf_width:6;
+	u8 mux_bank:2;
+	u8 pupd_bank:2;
+	u8 tri_bank:2;
+	u8 drv_bank:2;
+	s8 mux_bit:6;
+	s8 pupd_bit:6;
+	s8 tri_bit:6;
+	s8 einput_bit:6;
+	s8 odrain_bit:6;
+	s8 lock_bit:6;
+	s8 ioreset_bit:6;
+	s8 rcv_sel_bit:6;
+	s8 hsm_bit:6;
+	s8 schmitt_bit:6;
+	s8 lpmd_bit:6;
+	s8 drvdn_bit:6;
+	s8 drvup_bit:6;
+	s8 slwr_bit:6;
+	s8 slwf_bit:6;
+	s8 drvtype_bit:6;
+	s8 drvdn_width:6;
+	s8 drvup_width:6;
+	s8 slwr_width:6;
+	s8 slwf_width:6;
 };
 
 /**
-- 
2.3.3

Re: [PATCH] pinctrl: tegra: use signed bitfields for optional fields

[Stephen Warren]

On 03/14/2015 06:05 PM, Stefan Agner wrote:
> Optional fields are set to -1 by various preprocessor macros. Make
> sure the struct fields can actually store them.

> diff --git a/drivers/pinctrl/pinctrl-tegra.h b/drivers/pinctrl/pinctrl-tegra.h

> -	u32 mux_bit:6;
> -	u32 pupd_bit:6;
> -	u32 tri_bit:6;
...
> +	s8 mux_bit:6;
> +	s8 pupd_bit:6;
> +	s8 tri_bit:6;

Could we make these s32s instead? According to the C standard, the type 
should be a signed or unsigned int, and s32 matches that better than s8 
for existing Tegra 32-bit platforms. Equally, for bitfields that don't 
fit into the remaining space within a container (s8 above), 
implementations are allowed to either span bitfields across multiple 
containers, or pad the current container and start the bitfield in the 
next container. Using the larger s32 as the "container" yields less 
opportunity for potential padding and thus wasting space.

Do you observe any increase in the sizes reported by 
"${CROSS_COMPILE}size pinctrl-tegra*.o" with this patch?

Re: [PATCH] pinctrl: tegra: use signed bitfields for optional fields

[Stefan Agner]

On 2015-03-16 17:59, Stephen Warren wrote:
> On 03/14/2015 06:05 PM, Stefan Agner wrote:
>> Optional fields are set to -1 by various preprocessor macros. Make
>> sure the struct fields can actually store them.
> 
>> diff --git a/drivers/pinctrl/pinctrl-tegra.h b/drivers/pinctrl/pinctrl-tegra.h
> 
>> -	u32 mux_bit:6;
>> -	u32 pupd_bit:6;
>> -	u32 tri_bit:6;
> ...
>> +	s8 mux_bit:6;
>> +	s8 pupd_bit:6;
>> +	s8 tri_bit:6;
> 
> Could we make these s32s instead? According to the C standard, the
> type should be a signed or unsigned int, and s32 matches that better
> than s8 for existing Tegra 32-bit platforms. Equally, for bitfields
> that don't fit into the remaining space within a container (s8 above),
> implementations are allowed to either span bitfields across multiple
> containers, or pad the current container and start the bitfield in the
> next container. Using the larger s32 as the "container" yields less
> opportunity for potential padding and thus wasting space.

The reason I took the signed byte variant was that it gets filled into
that type later on. So I thought it would be kind of nice to see that
fact already in the definition of the struct...

But your argument has actual real significance. Will change that.

> Do you observe any increase in the sizes reported by
> "${CROSS_COMPILE}size pinctrl-tegra*.o" with this patch?

Baseline:
   5406	    180	      1	   5587	  
15d3	drivers/pinctrl/pinctrl-tegra-xusb.o
   5244	     64	      0	   5308	   14bc	drivers/pinctrl/pinctrl-tegra.o
  18072	   1032	      0	  19104	  
4aa0	drivers/pinctrl/pinctrl-tegra114.o
  19214	   1128	      0	  20342	  
4f76	drivers/pinctrl/pinctrl-tegra124.o
  18352	    876	      0	  19228	  
4b1c	drivers/pinctrl/pinctrl-tegra20.o
  24621	   1068	      0	  25689	  
6459	drivers/pinctrl/pinctrl-tegra30.o


With the patch above...
   text	   data	    bss	    dec	    hex	filename
   5406	    180	      1	   5587	  
15d3	drivers/pinctrl/pinctrl-tegra-xusb.o
   5252	     64	      0	   5316	   14c4	drivers/pinctrl/pinctrl-tegra.o
  18932	   1032	      0	  19964	  
4dfc	drivers/pinctrl/pinctrl-tegra114.o
  20142	   1128	      0	  21270	  
5316	drivers/pinctrl/pinctrl-tegra124.o
  18988	    876	      0	  19864	  
4d98	drivers/pinctrl/pinctrl-tegra20.o
  25781	   1068	      0	  26849	  
68e1	drivers/pinctrl/pinctrl-tegra30.o

--
Stefan

Re: [PATCH] pinctrl: tegra: use signed bitfields for optional fields

[Stefan Agner]

On 2015-03-15 01:05, Stefan Agner wrote:
> Optional fields are set to -1 by various preprocessor macros. Make
> sure the struct fields can actually store them.
> 
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
> This lead to a lot of warnings when compiling the Tegra pinctrl drivers
> using LLVM/clang:
> 
> drivers/pinctrl/pinctrl-tegra124.c:2048:2: warning: implicit truncation from
> 'int' to bitfield changes value from -1 to 63 [-Wbitfield-constant-conversion]
>         MIPI_PAD_CTRL_PINGROUP(dsi_b,   0x820, 1, CSI, DSI_B)
>         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/pinctrl/pinctrl-tegra124.c:1807:18: note: expanded from macro
> 'MIPI_PAD_CTRL_PINGROUP'
>                 .rcv_sel_bit = -1,                                      \
>                                ^~
> 
> However, I did not check if this could actually lead to an unintended
> pin configuration...

Stephen, what do you think about this? While I think the patch  is the
right thing to do, it could actually introduce nasty regressions (stuff
which was working due to  "accidentally" wrong pin configuration)...

I need to admit that I did not actually run a kernel with that patch.

--
Stefan