* [PATCH] pinctrl: tegra: use signed bitfields for optional fields
@ 2015-03-15 0:05 Stefan Agner
2015-03-16 16:59 ` Stephen Warren
2015-03-16 20:30 ` Stefan Agner
0 siblings, 2 replies; 4+ messages in thread
From: Stefan Agner @ 2015-03-15 0:05 UTC (permalink / raw)
To: linus.walleij
Cc: swarren, thierry.reding, gnurou, linux-gpio, linux-tegra,
linux-kernel, stefan
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(-)
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] pinctrl: tegra: use signed bitfields for optional fields
2015-03-15 0:05 [PATCH] pinctrl: tegra: use signed bitfields for optional fields Stefan Agner
@ 2015-03-16 16:59 ` Stephen Warren
2015-03-16 20:17 ` Stefan Agner
2015-03-16 20:30 ` Stefan Agner
1 sibling, 1 reply; 4+ messages in thread
From: Stephen Warren @ 2015-03-16 16:59 UTC (permalink / raw)
To: Stefan Agner
Cc: linus.walleij, thierry.reding, gnurou, linux-gpio, linux-tegra,
linux-kernel
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?
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] pinctrl: tegra: use signed bitfields for optional fields
2015-03-16 16:59 ` Stephen Warren
@ 2015-03-16 20:17 ` Stefan Agner
0 siblings, 0 replies; 4+ messages in thread
From: Stefan Agner @ 2015-03-16 20:17 UTC (permalink / raw)
To: Stephen Warren
Cc: linus.walleij, thierry.reding, gnurou, linux-gpio, linux-tegra,
linux-kernel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] pinctrl: tegra: use signed bitfields for optional fields
2015-03-15 0:05 [PATCH] pinctrl: tegra: use signed bitfields for optional fields Stefan Agner
2015-03-16 16:59 ` Stephen Warren
@ 2015-03-16 20:30 ` Stefan Agner
1 sibling, 0 replies; 4+ messages in thread
From: Stefan Agner @ 2015-03-16 20:30 UTC (permalink / raw)
To: swarren
Cc: linus.walleij, thierry.reding, gnurou, linux-gpio, linux-tegra,
linux-kernel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-03-16 20:30 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-15 0:05 [PATCH] pinctrl: tegra: use signed bitfields for optional fields Stefan Agner
2015-03-16 16:59 ` Stephen Warren
2015-03-16 20:17 ` Stefan Agner
2015-03-16 20:30 ` Stefan Agner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).