Re: [PATCH 2/4] pinctrl: sh-pfc: Store register/field widths in u8 instead of unsigned long

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Geert,

On Thursday 05 March 2015 19:02:10 Geert Uytterhoeven wrote:
> On Thu, Mar 5, 2015 at 10:19 AM, Geert Uytterhoeven wrote:
> > On Thu, Mar 5, 2015 at 10:03 AM, Laurent Pinchart
> > 
> > <laurent.pinchart@ideasonboard.com> wrote:
> >>> --- a/drivers/pinctrl/sh-pfc/sh_pfc.h
> >>> +++ b/drivers/pinctrl/sh-pfc/sh_pfc.h
> >>> @@ -69,9 +69,10 @@ struct pinmux_func {
> >>>  };
> >>>  
> >>>  struct pinmux_cfg_reg {
> >>> -     unsigned long reg, reg_width, field_width;
> >>> +     unsigned long reg;
> >> 
> >> How about making reg a u32 ? It won't make a difference in practice on
> >> 32-bit systems, but it would be more explicit.
> >> 
> >> We could also save space by making reg a u16 and storing the register
> >> offset only instead of the full address (assuming it can always fit in
> >> 16 bits, which should be checked). We'll also need to support 64-bit
> >> systems at some point,
>
> Unfortunately there's a (single) exception: sh7734 has two register blocks,
> at 0xFFFC0000 and 0xFFC40000, which is more than 64 KiB apart.
> And we also can't say that all config registers are in the first
> block, and all data registers are in the second block (for SoCs with
> multiple blocks).
> 
> We could use u32 instead of u16 if CONFIG_PINCTRL_PFC_SH7734 is set, though.

Or drop arch/sh support ;-)

> >> and making reg a u64 would increase space waste.
> > 
> > That would be more intrusive (and definitely needs to be in a separate
> > patch), as reg is used here to store a physical register address, for
> > conversion between physical and virtual addresses. I didn't want to go
> > that far yet.
> 
> I had a quick check, and using 16-bit register offsets instead of 32-bit
> registers addresses would save ca. 3 KiB in a shmobile_defconfig kernel.
> Which is a saving of less than 2% of the current pinctrl binary size.
> 
> Of course it's something to keep in mind for future 64-bit SoCs...

Let's do that for now then, just keep it in mind.

-- 
Regards,

Laurent Pinchart