| Message ID | 1426377952-12383-1-git-send-email-stefan@agner.ch |
|---|---|
| State | New |
| Headers | show |
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? -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }; /**
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... drivers/pinctrl/pinctrl-tegra.h | 48 ++++++++++++++++++++--------------------- 1 file changed, 24 insertions(+), 24 deletions(-)