[PATCH 0/4] pinctrl: sh-pfc: Fix pin bias and cleanups

[PATCH 0/4] pinctrl: sh-pfc: Fix pin bias and cleanups

[Geert Uytterhoeven]

	Hi,

This patch series fixes bias (pull up/down) for Renesas R-Mobile and
SH-Mobile SoCs, and does some code (type) cleanups.

  - Patch 1 fixes the overwriting of the bias field in the port control
    register when configuring a pin for GPIO,
  - Patch 2 changes the types used for storing register and field widths
    in the pinctrl data tables, shrinking them by an average of ca. 4
    KiB per SoC,
  - Patches 3 and 4 do some more cleanups w.r.t. the choice of types.

Bias pull up/down was tested on r8a73a4/ape6evm, which requires a
pull-up bias for the GPIO switches.

Boot-tested on r8a7740/armadillo, r8a7791/koelsch, and sh73a0/kzm9g.

Thanks for your comments!

Geert Uytterhoeven (4):
  pinctrl: sh-pfc: Do not overwrite bias configuration
  pinctrl: sh-pfc: Store register/field widths in u8 instead of unsigned
    long
  pinctrl: sh-pfc: Use u32 to store register data
  pinctrl: sh-pfc: Use unsigned int for register/field widths and
    offsets

 drivers/pinctrl/sh-pfc/core.c   | 36 ++++++++++++++++++------------------
 drivers/pinctrl/sh-pfc/core.h   |  7 +++----
 drivers/pinctrl/sh-pfc/gpio.c   | 19 ++++++++-----------
 drivers/pinctrl/sh-pfc/sh_pfc.h | 35 +++++++++++++++++++----------------
 4 files changed, 48 insertions(+), 49 deletions(-)

-- 
1.9.1

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[PATCH 1/4] pinctrl: sh-pfc: Do not overwrite bias configuration

[Geert Uytterhoeven]

After the last user of the in_pd/in_pu bias parameters of the _PCRH()
macro was removed in commit 80da8e02d22caaef ("sh-pfc: r8a7740: Add bias
(pull-up/down) pinconf support"), bias parameters are supposed to be
configured using the generic pinctl mechanism, which calls the
.set_bias() method.

However, the PORTCR() macro still represents the control register as
consisting of two 4-bit fields. Hence the bias configuration in the
uppermost 2 bits is always overwritten with zeroes when a pin is
configured for GPIO, disabling any previously configured bias.

Use the variable config register macro instead, to represent the
register as having 4 fields, and to make sure only the input/output
control and function fields are touched.

This affects R-Mobile APE6 (r8a73a4), R-Mobile A1 (r8a7740), SH-Mobile
AP4 (sh7372), and SH-Mobile AG5 (sh73a0).

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Tested on r8a73a4/ape6evm, which requires a pull-up bias for the
GPIO switches.
---
 drivers/pinctrl/sh-pfc/sh_pfc.h | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/pinctrl/sh-pfc/sh_pfc.h b/drivers/pinctrl/sh-pfc/sh_pfc.h
index c83728626906c16c..ed5cf4192fa1a2d0 100644
--- a/drivers/pinctrl/sh-pfc/sh_pfc.h
+++ b/drivers/pinctrl/sh-pfc/sh_pfc.h
@@ -302,20 +302,21 @@ struct sh_pfc_soc_info {
 /*
  * PORTnCR macro
  */
-#define _PCRH(in, in_pd, in_pu, out)	\
-	0, (out), (in), 0,		\
-	0, 0, 0, 0,			\
-	0, 0, (in_pd), 0,		\
-	0, 0, (in_pu), 0
-
 #define PORTCR(nr, reg)							\
 	{								\
-		PINMUX_CFG_REG("PORT" nr "CR", reg, 8, 4) {		\
-			_PCRH(PORT##nr##_IN, 0, 0, PORT##nr##_OUT),	\
-				PORT##nr##_FN0, PORT##nr##_FN1,		\
-				PORT##nr##_FN2, PORT##nr##_FN3,		\
-				PORT##nr##_FN4, PORT##nr##_FN5,		\
-				PORT##nr##_FN6, PORT##nr##_FN7 }	\
+		PINMUX_CFG_REG_VAR("PORT" nr "CR", reg, 8, 2, 2, 1, 3) {\
+			/* PULMD[1:0], handled by .set_bias() */	\
+			0, 0, 0, 0,					\
+			/* IE and OE */					\
+			0, PORT##nr##_OUT, PORT##nr##_IN, 0,		\
+			/* SEC, not supported */			\
+			0, 0,						\
+			/* PTMD[2:0] */					\
+			PORT##nr##_FN0, PORT##nr##_FN1,			\
+			PORT##nr##_FN2, PORT##nr##_FN3,			\
+			PORT##nr##_FN4, PORT##nr##_FN5,			\
+			PORT##nr##_FN6, PORT##nr##_FN7			\
+		}							\
 	}
 
 #endif /* __SH_PFC_H */
-- 
1.9.1

Re: [PATCH 1/4] pinctrl: sh-pfc: Do not overwrite bias configuration

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Friday 27 February 2015 18:38:02 Geert Uytterhoeven wrote:
> After the last user of the in_pd/in_pu bias parameters of the _PCRH()
> macro was removed in commit 80da8e02d22caaef ("sh-pfc: r8a7740: Add bias
> (pull-up/down) pinconf support"), bias parameters are supposed to be
> configured using the generic pinctl mechanism, which calls the
> .set_bias() method.
> 
> However, the PORTCR() macro still represents the control register as
> consisting of two 4-bit fields. Hence the bias configuration in the
> uppermost 2 bits is always overwritten with zeroes when a pin is
> configured for GPIO, disabling any previously configured bias.
> 
> Use the variable config register macro instead, to represent the
> register as having 4 fields, and to make sure only the input/output
> control and function fields are touched.
> 
> This affects R-Mobile APE6 (r8a73a4), R-Mobile A1 (r8a7740), SH-Mobile
> AP4 (sh7372), and SH-Mobile AG5 (sh73a0).
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> Tested on r8a73a4/ape6evm, which requires a pull-up bias for the
> GPIO switches.
> ---
>  drivers/pinctrl/sh-pfc/sh_pfc.h | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pinctrl/sh-pfc/sh_pfc.h
> b/drivers/pinctrl/sh-pfc/sh_pfc.h index c83728626906c16c..ed5cf4192fa1a2d0
> 100644
> --- a/drivers/pinctrl/sh-pfc/sh_pfc.h
> +++ b/drivers/pinctrl/sh-pfc/sh_pfc.h
> @@ -302,20 +302,21 @@ struct sh_pfc_soc_info {
>  /*
>   * PORTnCR macro
>   */
> -#define _PCRH(in, in_pd, in_pu, out)	\
> -	0, (out), (in), 0,		\
> -	0, 0, 0, 0,			\
> -	0, 0, (in_pd), 0,		\
> -	0, 0, (in_pu), 0
> -
>  #define PORTCR(nr, reg)							\
>  	{								\
> -		PINMUX_CFG_REG("PORT" nr "CR", reg, 8, 4) {		\
> -			_PCRH(PORT##nr##_IN, 0, 0, PORT##nr##_OUT),	\
> -				PORT##nr##_FN0, PORT##nr##_FN1,		\
> -				PORT##nr##_FN2, PORT##nr##_FN3,		\
> -				PORT##nr##_FN4, PORT##nr##_FN5,		\
> -				PORT##nr##_FN6, PORT##nr##_FN7 }	\
> +		PINMUX_CFG_REG_VAR("PORT" nr "CR", reg, 8, 2, 2, 1, 3) {\
> +			/* PULMD[1:0], handled by .set_bias() */	\
> +			0, 0, 0, 0,					\
> +			/* IE and OE */					\
> +			0, PORT##nr##_OUT, PORT##nr##_IN, 0,		\
> +			/* SEC, not supported */			\
> +			0, 0,						\
> +			/* PTMD[2:0] */					\
> +			PORT##nr##_FN0, PORT##nr##_FN1,			\
> +			PORT##nr##_FN2, PORT##nr##_FN3,			\
> +			PORT##nr##_FN4, PORT##nr##_FN5,			\
> +			PORT##nr##_FN6, PORT##nr##_FN7			\
> +		}							\
>  	}
> 
>  #endif /* __SH_PFC_H */

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/4] pinctrl: sh-pfc: Do not overwrite bias configuration

[Linus Walleij]

On Fri, Feb 27, 2015 at 6:38 PM, Geert Uytterhoeven
<geert+renesas@glider.be> wrote:

> After the last user of the in_pd/in_pu bias parameters of the _PCRH()
> macro was removed in commit 80da8e02d22caaef ("sh-pfc: r8a7740: Add bias
> (pull-up/down) pinconf support"), bias parameters are supposed to be
> configured using the generic pinctl mechanism, which calls the
> .set_bias() method.
>
> However, the PORTCR() macro still represents the control register as
> consisting of two 4-bit fields. Hence the bias configuration in the
> uppermost 2 bits is always overwritten with zeroes when a pin is
> configured for GPIO, disabling any previously configured bias.
>
> Use the variable config register macro instead, to represent the
> register as having 4 fields, and to make sure only the input/output
> control and function fields are touched.
>
> This affects R-Mobile APE6 (r8a73a4), R-Mobile A1 (r8a7740), SH-Mobile
> AP4 (sh7372), and SH-Mobile AG5 (sh73a0).
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Patch applied with Laurent's ACK.

Yours,
Linus Walleij

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

[Geert Uytterhoeven]

Register and field widths are in the range 1..32. Storing them in the
pinctrl data in (arrays of) unsigned long wastes space.

This decreases the size of a (32-bit) shmobile_defconfig kernel
supporting 7 SoCs by 26460 bytes.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/pinctrl/sh-pfc/core.c   |  2 +-
 drivers/pinctrl/sh-pfc/sh_pfc.h | 10 ++++++----
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/pinctrl/sh-pfc/core.c b/drivers/pinctrl/sh-pfc/core.c
index a56280814a3f884b..466b899ec78b15d7 100644
--- a/drivers/pinctrl/sh-pfc/core.c
+++ b/drivers/pinctrl/sh-pfc/core.c
@@ -210,7 +210,7 @@ static void sh_pfc_write_config_reg(struct sh_pfc *pfc,
 	sh_pfc_config_reg_helper(pfc, crp, field, &mapped_reg, &mask, &pos);
 
 	dev_dbg(pfc->dev, "write_reg addr = %lx, value = %ld, field = %ld, "
-		"r_width = %ld, f_width = %ld\n",
+		"r_width = %u, f_width = %u\n",
 		crp->reg, value, field, crp->reg_width, crp->field_width);
 
 	mask = ~(mask << pos);
diff --git a/drivers/pinctrl/sh-pfc/sh_pfc.h b/drivers/pinctrl/sh-pfc/sh_pfc.h
index ed5cf4192fa1a2d0..6aeec8152ea674cf 100644
--- 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;
+	u8 reg_width, field_width;
 	const u16 *enum_ids;
-	const unsigned long *var_field_width;
+	const u8 *var_field_width;
 };
 
 #define PINMUX_CFG_REG(name, r, r_width, f_width) \
@@ -80,12 +81,13 @@ struct pinmux_cfg_reg {
 
 #define PINMUX_CFG_REG_VAR(name, r, r_width, var_fw0, var_fwn...) \
 	.reg = r, .reg_width = r_width,	\
-	.var_field_width = (const unsigned long [r_width]) \
+	.var_field_width = (const u8 [r_width]) \
 		{ var_fw0, var_fwn, 0 }, \
 	.enum_ids = (const u16 [])
 
 struct pinmux_data_reg {
-	unsigned long reg, reg_width;
+	unsigned long reg;
+	u8 reg_width;
 	const u16 *enum_ids;
 };
 
-- 
1.9.1

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

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Friday 27 February 2015 18:38:03 Geert Uytterhoeven wrote:
> Register and field widths are in the range 1..32. Storing them in the
> pinctrl data in (arrays of) unsigned long wastes space.
> 
> This decreases the size of a (32-bit) shmobile_defconfig kernel
> supporting 7 SoCs by 26460 bytes.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/pinctrl/sh-pfc/core.c   |  2 +-
>  drivers/pinctrl/sh-pfc/sh_pfc.h | 10 ++++++----
>  2 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pinctrl/sh-pfc/core.c b/drivers/pinctrl/sh-pfc/core.c
> index a56280814a3f884b..466b899ec78b15d7 100644
> --- a/drivers/pinctrl/sh-pfc/core.c
> +++ b/drivers/pinctrl/sh-pfc/core.c
> @@ -210,7 +210,7 @@ static void sh_pfc_write_config_reg(struct sh_pfc *pfc,
>  	sh_pfc_config_reg_helper(pfc, crp, field, &mapped_reg, &mask, &pos);
> 
>  	dev_dbg(pfc->dev, "write_reg addr = %lx, value = %ld, field = %ld, "
> -		"r_width = %ld, f_width = %ld\n",
> +		"r_width = %u, f_width = %u\n",
>  		crp->reg, value, field, crp->reg_width, crp->field_width);
> 
>  	mask = ~(mask << pos);
> diff --git a/drivers/pinctrl/sh-pfc/sh_pfc.h
> b/drivers/pinctrl/sh-pfc/sh_pfc.h index ed5cf4192fa1a2d0..6aeec8152ea674cf
> 100644
> --- 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, 
and making reg a u64 would increase space waste.

> +	u8 reg_width, field_width;
>  	const u16 *enum_ids;
> -	const unsigned long *var_field_width;
> +	const u8 *var_field_width;
>  };
> 
>  #define PINMUX_CFG_REG(name, r, r_width, f_width) \
> @@ -80,12 +81,13 @@ struct pinmux_cfg_reg {
> 
>  #define PINMUX_CFG_REG_VAR(name, r, r_width, var_fw0, var_fwn...) \
>  	.reg = r, .reg_width = r_width,	\
> -	.var_field_width = (const unsigned long [r_width]) \
> +	.var_field_width = (const u8 [r_width]) \
>  		{ var_fw0, var_fwn, 0 }, \
>  	.enum_ids = (const u16 [])
> 
>  struct pinmux_data_reg {
> -	unsigned long reg, reg_width;
> +	unsigned long reg;
> +	u8 reg_width;
>  	const u16 *enum_ids;
>  };

-- 
Regards,

Laurent Pinchart

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

[Geert Uytterhoeven]

Hi Laurent,

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,
> 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.

u16 would indeed be nice, as it means reg, reg_width, and field_width
would fit in one 32-bit word, which I hadn't realized. That means we can reduce
each entry by 2 words instead of 1.

For 64-bit that would still be suboptimal, as pointers are aligned to 8 bytes,
leading to gaps.
Perhaps we do want __packed here, too? I don't think the performance drop of
doing some unaligned accesses would be significant. This isn't a hot path.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Geert Uytterhoeven]

Hi Laurent,

On Thu, Mar 5, 2015 at 10:19 AM, Geert Uytterhoeven
<geert@linux-m68k.org> 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.

>> 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...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Laurent Pinchart]

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

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

[Laurent Pinchart]

Hi Geert,

On Thursday 05 March 2015 10:19:33 Geert Uytterhoeven wrote:
> On Thu, Mar 5, 2015 at 10:03 AM, Laurent Pinchart 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.

You might have missed this comment.

> > 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, 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.
> 
> u16 would indeed be nice, as it means reg, reg_width, and field_width
> would fit in one 32-bit word, which I hadn't realized. That means we can
> reduce each entry by 2 words instead of 1.
> 
> For 64-bit that would still be suboptimal, as pointers are aligned to 8
> bytes, leading to gaps.
> Perhaps we do want __packed here, too? I don't think the performance drop of
> doing some unaligned accesses would be significant. This isn't a hot path.

-- 
Regards,

Laurent Pinchart

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

[Geert Uytterhoeven]

Hi Laurent,

On Fri, Mar 6, 2015 at 12:05 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Thursday 05 March 2015 10:19:33 Geert Uytterhoeven wrote:
>> On Thu, Mar 5, 2015 at 10:03 AM, Laurent Pinchart 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.
>
> You might have missed this comment.

I intended to, with "reg is used here to store a physical register address",
but probably didn't make it sufficiently clear. Before the advent of PAE and
phys_addr_t, unsigned long was used to store physical addresses.

We could indeed use u32, as the PFC regs are (currently) inside the 32-bit
part of the address space.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Laurent Pinchart]

Hi Geert,

On Friday 06 March 2015 12:21:29 Geert Uytterhoeven wrote:
> On Fri, Mar 6, 2015 at 12:05 PM, Laurent Pinchart wrote:
> > On Thursday 05 March 2015 10:19:33 Geert Uytterhoeven wrote:
> >> On Thu, Mar 5, 2015 at 10:03 AM, Laurent Pinchart 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.
> > 
> > You might have missed this comment.
> 
> I intended to, with "reg is used here to store a physical register address",
> but probably didn't make it sufficiently clear. Before the advent of PAE
> and phys_addr_t, unsigned long was used to store physical addresses.
> 
> We could indeed use u32, as the PFC regs are (currently) inside the 32-bit
> part of the address space.

Sounds good with me. If you want to make that change as a separate patch,

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

for this patch. Otherwise I'll ack v2.

-- 
Regards,

Laurent Pinchart

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

[Linus Walleij]

On Fri, Feb 27, 2015 at 6:38 PM, Geert Uytterhoeven
<geert+renesas@glider.be> wrote:

> Register and field widths are in the range 1..32. Storing them in the
> pinctrl data in (arrays of) unsigned long wastes space.
>
> This decreases the size of a (32-bit) shmobile_defconfig kernel
> supporting 7 SoCs by 26460 bytes.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

This 2/4 is not ACKed so pausing application after 1/4.

Is 3,4/4 independent of this patch?

Yours,
Linus Walleij

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

[Geert Uytterhoeven]

Hi Linus, Laurent,

On Fri, Mar 6, 2015 at 11:48 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Feb 27, 2015 at 6:38 PM, Geert Uytterhoeven
> <geert+renesas@glider.be> wrote:
>
>> Register and field widths are in the range 1..32. Storing them in the
>> pinctrl data in (arrays of) unsigned long wastes space.
>>
>> This decreases the size of a (32-bit) shmobile_defconfig kernel
>> supporting 7 SoCs by 26460 bytes.
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> This 2/4 is not ACKed so pausing application after 1/4.
>
> Is 3,4/4 independent of this patch?

Unfortunately not, as there are contextual dependencies.

Laurent: Is 2/4 OK for you as-is? Your comments about the size of reg are
not related to "Store register/field widths in u8 instead of unsigned long",
and thus an area for future improvement.

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Laurent Pinchart]

Hi Geert,

On Friday 06 March 2015 12:26:57 Geert Uytterhoeven wrote:
> On Fri, Mar 6, 2015 at 11:48 AM, Linus Walleij wrote:
> > On Fri, Feb 27, 2015 at 6:38 PM, Geert Uytterhoeven wrote:
> >> Register and field widths are in the range 1..32. Storing them in the
> >> pinctrl data in (arrays of) unsigned long wastes space.
> >> 
> >> This decreases the size of a (32-bit) shmobile_defconfig kernel
> >> supporting 7 SoCs by 26460 bytes.
> >> 
> >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > 
> > This 2/4 is not ACKed so pausing application after 1/4.
> > 
> > Is 3,4/4 independent of this patch?
> 
> Unfortunately not, as there are contextual dependencies.
> 
> Laurent: Is 2/4 OK for you as-is? Your comments about the size of reg are
> not related to "Store register/field widths in u8 instead of unsigned long",
> and thus an area for future improvement.

As just stated in a different reply to this patch, if you change reg to a u32 
in a separate patch (maybe as a v2 of 3/4 ?),

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

for this patch.

-- 
Regards,

Laurent Pinchart

[PATCH 3/4] pinctrl: sh-pfc: Use u32 to store register data

[Geert Uytterhoeven]

As PFC registers are either 8, 16, or 32 bits wide, use u32 (mostly
replacing unsigned long) to store (parts of) register values and masks.

Switch the shadow register operations from {set,clear}_bit() to plain C
bit operations, as the former can operate on long data only.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/pinctrl/sh-pfc/core.c | 25 +++++++++++++------------
 drivers/pinctrl/sh-pfc/core.h |  5 ++---
 drivers/pinctrl/sh-pfc/gpio.c | 13 ++++++-------
 3 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/drivers/pinctrl/sh-pfc/core.c b/drivers/pinctrl/sh-pfc/core.c
index 466b899ec78b15d7..1758043cfcec253b 100644
--- a/drivers/pinctrl/sh-pfc/core.c
+++ b/drivers/pinctrl/sh-pfc/core.c
@@ -144,8 +144,7 @@ static int sh_pfc_enum_in_range(u16 enum_id, const struct pinmux_range *r)
 	return 1;
 }
 
-unsigned long sh_pfc_read_raw_reg(void __iomem *mapped_reg,
-				  unsigned long reg_width)
+u32 sh_pfc_read_raw_reg(void __iomem *mapped_reg, unsigned long reg_width)
 {
 	switch (reg_width) {
 	case 8:
@@ -161,7 +160,7 @@ unsigned long sh_pfc_read_raw_reg(void __iomem *mapped_reg,
 }
 
 void sh_pfc_write_raw_reg(void __iomem *mapped_reg, unsigned long reg_width,
-			  unsigned long data)
+			  u32 data)
 {
 	switch (reg_width) {
 	case 8:
@@ -181,8 +180,7 @@ void sh_pfc_write_raw_reg(void __iomem *mapped_reg, unsigned long reg_width,
 static void sh_pfc_config_reg_helper(struct sh_pfc *pfc,
 				     const struct pinmux_cfg_reg *crp,
 				     unsigned long in_pos,
-				     void __iomem **mapped_regp,
-				     unsigned long *maskp,
+				     void __iomem **mapped_regp, u32 *maskp,
 				     unsigned long *posp)
 {
 	unsigned int k;
@@ -202,14 +200,15 @@ static void sh_pfc_config_reg_helper(struct sh_pfc *pfc,
 
 static void sh_pfc_write_config_reg(struct sh_pfc *pfc,
 				    const struct pinmux_cfg_reg *crp,
-				    unsigned long field, unsigned long value)
+				    unsigned long field, u32 value)
 {
 	void __iomem *mapped_reg;
-	unsigned long mask, pos, data;
+	unsigned long pos;
+	u32 mask, data;
 
 	sh_pfc_config_reg_helper(pfc, crp, field, &mapped_reg, &mask, &pos);
 
-	dev_dbg(pfc->dev, "write_reg addr = %lx, value = %ld, field = %ld, "
+	dev_dbg(pfc->dev, "write_reg addr = %lx, value = 0x%x, field = %ld, "
 		"r_width = %u, f_width = %u\n",
 		crp->reg, value, field, crp->reg_width, crp->field_width);
 
@@ -230,11 +229,12 @@ static void sh_pfc_write_config_reg(struct sh_pfc *pfc,
 
 static int sh_pfc_get_config_reg(struct sh_pfc *pfc, u16 enum_id,
 				 const struct pinmux_cfg_reg **crp, int *fieldp,
-				 int *valuep)
+				 u32 *valuep)
 {
 	const struct pinmux_cfg_reg *config_reg;
-	unsigned long r_width, f_width, curr_width, ncomb;
-	unsigned int k, m, n, pos, bit_pos;
+	unsigned long r_width, f_width, curr_width;
+	unsigned int k, m, pos, bit_pos;
+	u32 ncomb, n;
 
 	k = 0;
 	while (1) {
@@ -300,7 +300,8 @@ int sh_pfc_config_mux(struct sh_pfc *pfc, unsigned mark, int pinmux_type)
 	const struct pinmux_cfg_reg *cr = NULL;
 	u16 enum_id;
 	const struct pinmux_range *range;
-	int in_range, pos, field, value;
+	int in_range, pos, field;
+	u32 value;
 	int ret;
 
 	switch (pinmux_type) {
diff --git a/drivers/pinctrl/sh-pfc/core.h b/drivers/pinctrl/sh-pfc/core.h
index 6b59d63b9c01e7a6..8a10dd50ccdd2e0c 100644
--- a/drivers/pinctrl/sh-pfc/core.h
+++ b/drivers/pinctrl/sh-pfc/core.h
@@ -57,10 +57,9 @@ int sh_pfc_unregister_gpiochip(struct sh_pfc *pfc);
 int sh_pfc_register_pinctrl(struct sh_pfc *pfc);
 int sh_pfc_unregister_pinctrl(struct sh_pfc *pfc);
 
-unsigned long sh_pfc_read_raw_reg(void __iomem *mapped_reg,
-				  unsigned long reg_width);
+u32 sh_pfc_read_raw_reg(void __iomem *mapped_reg, unsigned long reg_width);
 void sh_pfc_write_raw_reg(void __iomem *mapped_reg, unsigned long reg_width,
-			  unsigned long data);
+			  u32 data);
 
 int sh_pfc_get_pin_index(struct sh_pfc *pfc, unsigned int pin);
 int sh_pfc_config_mux(struct sh_pfc *pfc, unsigned mark, int pinmux_type);
diff --git a/drivers/pinctrl/sh-pfc/gpio.c b/drivers/pinctrl/sh-pfc/gpio.c
index 80f641ee4dea3146..f2bb7d7398cdfc24 100644
--- a/drivers/pinctrl/sh-pfc/gpio.c
+++ b/drivers/pinctrl/sh-pfc/gpio.c
@@ -21,7 +21,7 @@
 
 struct sh_pfc_gpio_data_reg {
 	const struct pinmux_data_reg *info;
-	unsigned long shadow;
+	u32 shadow;
 };
 
 struct sh_pfc_gpio_pin {
@@ -59,8 +59,8 @@ static void gpio_get_data_reg(struct sh_pfc_chip *chip, unsigned int offset,
 	*bit = gpio_pin->dbit;
 }
 
-static unsigned long gpio_read_data_reg(struct sh_pfc_chip *chip,
-					const struct pinmux_data_reg *dreg)
+static u32 gpio_read_data_reg(struct sh_pfc_chip *chip,
+			      const struct pinmux_data_reg *dreg)
 {
 	void __iomem *mem = dreg->reg - chip->mem->phys + chip->mem->virt;
 
@@ -68,8 +68,7 @@ static unsigned long gpio_read_data_reg(struct sh_pfc_chip *chip,
 }
 
 static void gpio_write_data_reg(struct sh_pfc_chip *chip,
-				const struct pinmux_data_reg *dreg,
-				unsigned long value)
+				const struct pinmux_data_reg *dreg, u32 value)
 {
 	void __iomem *mem = dreg->reg - chip->mem->phys + chip->mem->virt;
 
@@ -162,9 +161,9 @@ static void gpio_pin_set_value(struct sh_pfc_chip *chip, unsigned offset,
 	pos = reg->info->reg_width - (bit + 1);
 
 	if (value)
-		set_bit(pos, &reg->shadow);
+		reg->shadow |= BIT(pos);
 	else
-		clear_bit(pos, &reg->shadow);
+		reg->shadow &= ~BIT(pos);
 
 	gpio_write_data_reg(chip, reg->info, reg->shadow);
 }
-- 
1.9.1

Re: [PATCH 3/4] pinctrl: sh-pfc: Use u32 to store register data

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Friday 27 February 2015 18:38:04 Geert Uytterhoeven wrote:
> As PFC registers are either 8, 16, or 32 bits wide, use u32 (mostly
> replacing unsigned long) to store (parts of) register values and masks.

I hope we won't get 64-bit registers in the future... Fingers crossed.

> Switch the shadow register operations from {set,clear}_bit() to plain C
> bit operations, as the former can operate on long data only.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/pinctrl/sh-pfc/core.c | 25 +++++++++++++------------
>  drivers/pinctrl/sh-pfc/core.h |  5 ++---
>  drivers/pinctrl/sh-pfc/gpio.c | 13 ++++++-------
>  3 files changed, 21 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/pinctrl/sh-pfc/core.c b/drivers/pinctrl/sh-pfc/core.c
> index 466b899ec78b15d7..1758043cfcec253b 100644
> --- a/drivers/pinctrl/sh-pfc/core.c
> +++ b/drivers/pinctrl/sh-pfc/core.c
> @@ -144,8 +144,7 @@ static int sh_pfc_enum_in_range(u16 enum_id, const
> struct pinmux_range *r) return 1;
>  }
> 
> -unsigned long sh_pfc_read_raw_reg(void __iomem *mapped_reg,
> -				  unsigned long reg_width)
> +u32 sh_pfc_read_raw_reg(void __iomem *mapped_reg, unsigned long reg_width)
>  {
>  	switch (reg_width) {
>  	case 8:
> @@ -161,7 +160,7 @@ unsigned long sh_pfc_read_raw_reg(void __iomem
> *mapped_reg, }
> 
>  void sh_pfc_write_raw_reg(void __iomem *mapped_reg, unsigned long
> reg_width,
> -			  unsigned long data)
> +			  u32 data)
>  {
>  	switch (reg_width) {
>  	case 8:
> @@ -181,8 +180,7 @@ void sh_pfc_write_raw_reg(void __iomem *mapped_reg,
> unsigned long reg_width, static void sh_pfc_config_reg_helper(struct sh_pfc
> *pfc,
>  				     const struct pinmux_cfg_reg *crp,
>  				     unsigned long in_pos,
> -				     void __iomem **mapped_regp,
> -				     unsigned long *maskp,
> +				     void __iomem **mapped_regp, u32 *maskp,
>  				     unsigned long *posp)
>  {
>  	unsigned int k;
> @@ -202,14 +200,15 @@ static void sh_pfc_config_reg_helper(struct sh_pfc
> *pfc,
> 
>  static void sh_pfc_write_config_reg(struct sh_pfc *pfc,
>  				    const struct pinmux_cfg_reg *crp,
> -				    unsigned long field, unsigned long value)
> +				    unsigned long field, u32 value)
>  {
>  	void __iomem *mapped_reg;
> -	unsigned long mask, pos, data;
> +	unsigned long pos;
> +	u32 mask, data;
> 
>  	sh_pfc_config_reg_helper(pfc, crp, field, &mapped_reg, &mask, &pos);
> 
> -	dev_dbg(pfc->dev, "write_reg addr = %lx, value = %ld, field = %ld, "
> +	dev_dbg(pfc->dev, "write_reg addr = %lx, value = 0x%x, field = %ld, "
>  		"r_width = %u, f_width = %u\n",
>  		crp->reg, value, field, crp->reg_width, crp->field_width);
> 
> @@ -230,11 +229,12 @@ static void sh_pfc_write_config_reg(struct sh_pfc
> *pfc,
> 
>  static int sh_pfc_get_config_reg(struct sh_pfc *pfc, u16 enum_id,
>  				 const struct pinmux_cfg_reg **crp, int *fieldp,
> -				 int *valuep)
> +				 u32 *valuep)
>  {
>  	const struct pinmux_cfg_reg *config_reg;
> -	unsigned long r_width, f_width, curr_width, ncomb;
> -	unsigned int k, m, n, pos, bit_pos;
> +	unsigned long r_width, f_width, curr_width;
> +	unsigned int k, m, pos, bit_pos;
> +	u32 ncomb, n;

Nitpicking, strictly speaking ncomb and n are not register data, but using u32 
for them seems good to me.

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  	k = 0;
>  	while (1) {
> @@ -300,7 +300,8 @@ int sh_pfc_config_mux(struct sh_pfc *pfc, unsigned mark,
> int pinmux_type) const struct pinmux_cfg_reg *cr = NULL;
>  	u16 enum_id;
>  	const struct pinmux_range *range;
> -	int in_range, pos, field, value;
> +	int in_range, pos, field;
> +	u32 value;
>  	int ret;
> 
>  	switch (pinmux_type) {
> diff --git a/drivers/pinctrl/sh-pfc/core.h b/drivers/pinctrl/sh-pfc/core.h
> index 6b59d63b9c01e7a6..8a10dd50ccdd2e0c 100644
> --- a/drivers/pinctrl/sh-pfc/core.h
> +++ b/drivers/pinctrl/sh-pfc/core.h
> @@ -57,10 +57,9 @@ int sh_pfc_unregister_gpiochip(struct sh_pfc *pfc);
>  int sh_pfc_register_pinctrl(struct sh_pfc *pfc);
>  int sh_pfc_unregister_pinctrl(struct sh_pfc *pfc);
> 
> -unsigned long sh_pfc_read_raw_reg(void __iomem *mapped_reg,
> -				  unsigned long reg_width);
> +u32 sh_pfc_read_raw_reg(void __iomem *mapped_reg, unsigned long reg_width);
> void sh_pfc_write_raw_reg(void __iomem *mapped_reg, unsigned long
> reg_width, -			  unsigned long data);
> +			  u32 data);
> 
>  int sh_pfc_get_pin_index(struct sh_pfc *pfc, unsigned int pin);
>  int sh_pfc_config_mux(struct sh_pfc *pfc, unsigned mark, int pinmux_type);
> diff --git a/drivers/pinctrl/sh-pfc/gpio.c b/drivers/pinctrl/sh-pfc/gpio.c
> index 80f641ee4dea3146..f2bb7d7398cdfc24 100644
> --- a/drivers/pinctrl/sh-pfc/gpio.c
> +++ b/drivers/pinctrl/sh-pfc/gpio.c
> @@ -21,7 +21,7 @@
> 
>  struct sh_pfc_gpio_data_reg {
>  	const struct pinmux_data_reg *info;
> -	unsigned long shadow;
> +	u32 shadow;
>  };
> 
>  struct sh_pfc_gpio_pin {
> @@ -59,8 +59,8 @@ static void gpio_get_data_reg(struct sh_pfc_chip *chip,
> unsigned int offset, *bit = gpio_pin->dbit;
>  }
> 
> -static unsigned long gpio_read_data_reg(struct sh_pfc_chip *chip,
> -					const struct pinmux_data_reg *dreg)
> +static u32 gpio_read_data_reg(struct sh_pfc_chip *chip,
> +			      const struct pinmux_data_reg *dreg)
>  {
>  	void __iomem *mem = dreg->reg - chip->mem->phys + chip->mem->virt;
> 
> @@ -68,8 +68,7 @@ static unsigned long gpio_read_data_reg(struct sh_pfc_chip
> *chip, }
> 
>  static void gpio_write_data_reg(struct sh_pfc_chip *chip,
> -				const struct pinmux_data_reg *dreg,
> -				unsigned long value)
> +				const struct pinmux_data_reg *dreg, u32 value)
>  {
>  	void __iomem *mem = dreg->reg - chip->mem->phys + chip->mem->virt;
> 
> @@ -162,9 +161,9 @@ static void gpio_pin_set_value(struct sh_pfc_chip *chip,
> unsigned offset, pos = reg->info->reg_width - (bit + 1);
> 
>  	if (value)
> -		set_bit(pos, &reg->shadow);
> +		reg->shadow |= BIT(pos);
>  	else
> -		clear_bit(pos, &reg->shadow);
> +		reg->shadow &= ~BIT(pos);
> 
>  	gpio_write_data_reg(chip, reg->info, reg->shadow);
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/4] pinctrl: sh-pfc: Use u32 to store register data

[Geert Uytterhoeven]

Hi Laurent,

On Thu, Mar 5, 2015 at 10:14 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>> @@ -230,11 +229,12 @@ static void sh_pfc_write_config_reg(struct sh_pfc
>> *pfc,
>>
>>  static int sh_pfc_get_config_reg(struct sh_pfc *pfc, u16 enum_id,
>>                                const struct pinmux_cfg_reg **crp, int *fieldp,
>> -                              int *valuep)
>> +                              u32 *valuep)
>>  {
>>       const struct pinmux_cfg_reg *config_reg;
>> -     unsigned long r_width, f_width, curr_width, ncomb;
>> -     unsigned int k, m, n, pos, bit_pos;
>> +     unsigned long r_width, f_width, curr_width;
>> +     unsigned int k, m, pos, bit_pos;
>> +     u32 ncomb, n;
>
> Nitpicking, strictly speaking ncomb and n are not register data, but using u32
> for them seems good to me.

n is assigned to the (partial) register value output parameter, so it is.
ncomb is an upper boundary for n.

And I didn't want to split this off to yet another patch ;-)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 3/4] pinctrl: sh-pfc: Use u32 to store register data

[Linus Walleij]

On Fri, Feb 27, 2015 at 6:38 PM, Geert Uytterhoeven
<geert+renesas@glider.be> wrote:

> As PFC registers are either 8, 16, or 32 bits wide, use u32 (mostly
> replacing unsigned long) to store (parts of) register values and masks.
>
> Switch the shadow register operations from {set,clear}_bit() to plain C
> bit operations, as the former can operate on long data only.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Patch applied with Laurent's ACK.

Yours,
Linus Walleij

Re: [PATCH 3/4] pinctrl: sh-pfc: Use u32 to store register data

[Geert Uytterhoeven]

Hi Linus,

On Mon, Mar 9, 2015 at 5:37 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Feb 27, 2015 at 6:38 PM, Geert Uytterhoeven
> <geert+renesas@glider.be> wrote:
>
>> As PFC registers are either 8, 16, or 32 bits wide, use u32 (mostly
>> replacing unsigned long) to store (parts of) register values and masks.
>>
>> Switch the shadow register operations from {set,clear}_bit() to plain C
>> bit operations, as the former can operate on long data only.
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> Patch applied with Laurent's ACK.

Thanks!

I will post the reworked/new patches when I see your published branch.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH 4/4] pinctrl: sh-pfc: Use unsigned int for register/field widths and offsets

[Geert Uytterhoeven]

As register and field widths and offsets are in the range 0..32, use
unsigned int (mostly replacing unsigned long) to store them in local
variables and for passing them around.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/pinctrl/sh-pfc/core.c | 25 ++++++++++++-------------
 drivers/pinctrl/sh-pfc/core.h |  4 ++--
 drivers/pinctrl/sh-pfc/gpio.c |  6 ++----
 3 files changed, 16 insertions(+), 19 deletions(-)

diff --git a/drivers/pinctrl/sh-pfc/core.c b/drivers/pinctrl/sh-pfc/core.c
index 1758043cfcec253b..0350c64229ea0734 100644
--- a/drivers/pinctrl/sh-pfc/core.c
+++ b/drivers/pinctrl/sh-pfc/core.c
@@ -144,7 +144,7 @@ static int sh_pfc_enum_in_range(u16 enum_id, const struct pinmux_range *r)
 	return 1;
 }
 
-u32 sh_pfc_read_raw_reg(void __iomem *mapped_reg, unsigned long reg_width)
+u32 sh_pfc_read_raw_reg(void __iomem *mapped_reg, unsigned int reg_width)
 {
 	switch (reg_width) {
 	case 8:
@@ -159,7 +159,7 @@ u32 sh_pfc_read_raw_reg(void __iomem *mapped_reg, unsigned long reg_width)
 	return 0;
 }
 
-void sh_pfc_write_raw_reg(void __iomem *mapped_reg, unsigned long reg_width,
+void sh_pfc_write_raw_reg(void __iomem *mapped_reg, unsigned int reg_width,
 			  u32 data)
 {
 	switch (reg_width) {
@@ -179,9 +179,9 @@ void sh_pfc_write_raw_reg(void __iomem *mapped_reg, unsigned long reg_width,
 
 static void sh_pfc_config_reg_helper(struct sh_pfc *pfc,
 				     const struct pinmux_cfg_reg *crp,
-				     unsigned long in_pos,
+				     unsigned int in_pos,
 				     void __iomem **mapped_regp, u32 *maskp,
-				     unsigned long *posp)
+				     unsigned int *posp)
 {
 	unsigned int k;
 
@@ -200,15 +200,15 @@ static void sh_pfc_config_reg_helper(struct sh_pfc *pfc,
 
 static void sh_pfc_write_config_reg(struct sh_pfc *pfc,
 				    const struct pinmux_cfg_reg *crp,
-				    unsigned long field, u32 value)
+				    unsigned int field, u32 value)
 {
 	void __iomem *mapped_reg;
-	unsigned long pos;
+	unsigned int pos;
 	u32 mask, data;
 
 	sh_pfc_config_reg_helper(pfc, crp, field, &mapped_reg, &mask, &pos);
 
-	dev_dbg(pfc->dev, "write_reg addr = %lx, value = 0x%x, field = %ld, "
+	dev_dbg(pfc->dev, "write_reg addr = %lx, value = 0x%x, field = %u, "
 		"r_width = %u, f_width = %u\n",
 		crp->reg, value, field, crp->reg_width, crp->field_width);
 
@@ -228,12 +228,11 @@ static void sh_pfc_write_config_reg(struct sh_pfc *pfc,
 }
 
 static int sh_pfc_get_config_reg(struct sh_pfc *pfc, u16 enum_id,
-				 const struct pinmux_cfg_reg **crp, int *fieldp,
-				 u32 *valuep)
+				 const struct pinmux_cfg_reg **crp,
+				 unsigned int *fieldp, u32 *valuep)
 {
 	const struct pinmux_cfg_reg *config_reg;
-	unsigned long r_width, f_width, curr_width;
-	unsigned int k, m, pos, bit_pos;
+	unsigned int r_width, f_width, curr_width, k, m, pos, bit_pos;
 	u32 ncomb, n;
 
 	k = 0;
@@ -300,9 +299,9 @@ int sh_pfc_config_mux(struct sh_pfc *pfc, unsigned mark, int pinmux_type)
 	const struct pinmux_cfg_reg *cr = NULL;
 	u16 enum_id;
 	const struct pinmux_range *range;
-	int in_range, pos, field;
+	int in_range, pos, ret;
+	unsigned int field;
 	u32 value;
-	int ret;
 
 	switch (pinmux_type) {
 	case PINMUX_TYPE_GPIO:
diff --git a/drivers/pinctrl/sh-pfc/core.h b/drivers/pinctrl/sh-pfc/core.h
index 8a10dd50ccdd2e0c..6dc8a6fc27468b39 100644
--- a/drivers/pinctrl/sh-pfc/core.h
+++ b/drivers/pinctrl/sh-pfc/core.h
@@ -57,8 +57,8 @@ int sh_pfc_unregister_gpiochip(struct sh_pfc *pfc);
 int sh_pfc_register_pinctrl(struct sh_pfc *pfc);
 int sh_pfc_unregister_pinctrl(struct sh_pfc *pfc);
 
-u32 sh_pfc_read_raw_reg(void __iomem *mapped_reg, unsigned long reg_width);
-void sh_pfc_write_raw_reg(void __iomem *mapped_reg, unsigned long reg_width,
+u32 sh_pfc_read_raw_reg(void __iomem *mapped_reg, unsigned int reg_width);
+void sh_pfc_write_raw_reg(void __iomem *mapped_reg, unsigned int reg_width,
 			  u32 data);
 
 int sh_pfc_get_pin_index(struct sh_pfc *pfc, unsigned int pin);
diff --git a/drivers/pinctrl/sh-pfc/gpio.c b/drivers/pinctrl/sh-pfc/gpio.c
index f2bb7d7398cdfc24..b68d24099593d3b8 100644
--- a/drivers/pinctrl/sh-pfc/gpio.c
+++ b/drivers/pinctrl/sh-pfc/gpio.c
@@ -153,8 +153,7 @@ static void gpio_pin_set_value(struct sh_pfc_chip *chip, unsigned offset,
 			       int value)
 {
 	struct sh_pfc_gpio_data_reg *reg;
-	unsigned long pos;
-	unsigned int bit;
+	unsigned int pos, bit;
 
 	gpio_get_data_reg(chip, offset, &reg, &bit);
 
@@ -185,8 +184,7 @@ static int gpio_pin_get(struct gpio_chip *gc, unsigned offset)
 {
 	struct sh_pfc_chip *chip = gpio_to_pfc_chip(gc);
 	struct sh_pfc_gpio_data_reg *reg;
-	unsigned long pos;
-	unsigned int bit;
+	unsigned int pos, bit;
 
 	gpio_get_data_reg(chip, offset, &reg, &bit);
 
-- 
1.9.1

Re: [PATCH 4/4] pinctrl: sh-pfc: Use unsigned int for register/field widths and offsets

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Friday 27 February 2015 18:38:05 Geert Uytterhoeven wrote:
> As register and field widths and offsets are in the range 0..32, use
> unsigned int (mostly replacing unsigned long) to store them in local
> variables and for passing them around.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/pinctrl/sh-pfc/core.c | 25 ++++++++++++-------------
>  drivers/pinctrl/sh-pfc/core.h |  4 ++--
>  drivers/pinctrl/sh-pfc/gpio.c |  6 ++----
>  3 files changed, 16 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/pinctrl/sh-pfc/core.c b/drivers/pinctrl/sh-pfc/core.c
> index 1758043cfcec253b..0350c64229ea0734 100644
> --- a/drivers/pinctrl/sh-pfc/core.c
> +++ b/drivers/pinctrl/sh-pfc/core.c
> @@ -144,7 +144,7 @@ static int sh_pfc_enum_in_range(u16 enum_id, const
> struct pinmux_range *r) return 1;
>  }
> 
> -u32 sh_pfc_read_raw_reg(void __iomem *mapped_reg, unsigned long reg_width)
> +u32 sh_pfc_read_raw_reg(void __iomem *mapped_reg, unsigned int reg_width)
>  {
>  	switch (reg_width) {
>  	case 8:
> @@ -159,7 +159,7 @@ u32 sh_pfc_read_raw_reg(void __iomem *mapped_reg,
> unsigned long reg_width) return 0;
>  }
> 
> -void sh_pfc_write_raw_reg(void __iomem *mapped_reg, unsigned long
> reg_width,
> +void sh_pfc_write_raw_reg(void __iomem *mapped_reg, unsigned
> int reg_width, u32 data)
>  {
>  	switch (reg_width) {
> @@ -179,9 +179,9 @@ void sh_pfc_write_raw_reg(void __iomem *mapped_reg,
> unsigned long reg_width,
> 
>  static void sh_pfc_config_reg_helper(struct sh_pfc *pfc,
>  				     const struct pinmux_cfg_reg *crp,
> -				     unsigned long in_pos,
> +				     unsigned int in_pos,
>  				     void __iomem **mapped_regp, u32 *maskp,
> -				     unsigned long *posp)
> +				     unsigned int *posp)
>  {
>  	unsigned int k;
> 
> @@ -200,15 +200,15 @@ static void sh_pfc_config_reg_helper(struct sh_pfc
> *pfc,
> 
>  static void sh_pfc_write_config_reg(struct sh_pfc *pfc,
>  				    const struct pinmux_cfg_reg *crp,
> -				    unsigned long field, u32 value)
> +				    unsigned int field, u32 value)
>  {
>  	void __iomem *mapped_reg;
> -	unsigned long pos;
> +	unsigned int pos;
>  	u32 mask, data;
> 
>  	sh_pfc_config_reg_helper(pfc, crp, field, &mapped_reg, &mask, &pos);
> 
> -	dev_dbg(pfc->dev, "write_reg addr = %lx, value = 0x%x, field = %ld, "
> +	dev_dbg(pfc->dev, "write_reg addr = %lx, value = 0x%x, field = %u, "
>  		"r_width = %u, f_width = %u\n",
>  		crp->reg, value, field, crp->reg_width, crp->field_width);
> 
> @@ -228,12 +228,11 @@ static void sh_pfc_write_config_reg(struct sh_pfc
> *pfc, }
> 
>  static int sh_pfc_get_config_reg(struct sh_pfc *pfc, u16 enum_id,
> -				 const struct pinmux_cfg_reg **crp, int *fieldp,
> -				 u32 *valuep)
> +				 const struct pinmux_cfg_reg **crp,
> +				 unsigned int *fieldp, u32 *valuep)
>  {
>  	const struct pinmux_cfg_reg *config_reg;
> -	unsigned long r_width, f_width, curr_width;
> -	unsigned int k, m, pos, bit_pos;
> +	unsigned int r_width, f_width, curr_width, k, m, pos, bit_pos;

I find declaring multiple variables per line quite difficult to read. I know 
it's the current coding style in this driver, but I'd like to fix it at some 
point. I would move to one variable per line as part of this patch for the 
code that it touches. Alternatively could you at least not make it worse (here 
and below) ? :-)

>  	u32 ncomb, n;
> 
>  	k = 0;
> @@ -300,9 +299,9 @@ int sh_pfc_config_mux(struct sh_pfc *pfc, unsigned mark,
> int pinmux_type) const struct pinmux_cfg_reg *cr = NULL;
>  	u16 enum_id;
>  	const struct pinmux_range *range;
> -	int in_range, pos, field;
> +	int in_range, pos, ret;
> +	unsigned int field;
>  	u32 value;
> -	int ret;
> 
>  	switch (pinmux_type) {
>  	case PINMUX_TYPE_GPIO:
> diff --git a/drivers/pinctrl/sh-pfc/core.h b/drivers/pinctrl/sh-pfc/core.h
> index 8a10dd50ccdd2e0c..6dc8a6fc27468b39 100644
> --- a/drivers/pinctrl/sh-pfc/core.h
> +++ b/drivers/pinctrl/sh-pfc/core.h
> @@ -57,8 +57,8 @@ int sh_pfc_unregister_gpiochip(struct sh_pfc *pfc);
>  int sh_pfc_register_pinctrl(struct sh_pfc *pfc);
>  int sh_pfc_unregister_pinctrl(struct sh_pfc *pfc);
> 
> -u32 sh_pfc_read_raw_reg(void __iomem *mapped_reg, unsigned long reg_width);
> -void sh_pfc_write_raw_reg(void __iomem *mapped_reg, unsigned long
> reg_width, +u32 sh_pfc_read_raw_reg(void __iomem *mapped_reg, unsigned int
> reg_width); +void sh_pfc_write_raw_reg(void __iomem *mapped_reg, unsigned
> int reg_width, u32 data);
> 
>  int sh_pfc_get_pin_index(struct sh_pfc *pfc, unsigned int pin);
> diff --git a/drivers/pinctrl/sh-pfc/gpio.c b/drivers/pinctrl/sh-pfc/gpio.c
> index f2bb7d7398cdfc24..b68d24099593d3b8 100644
> --- a/drivers/pinctrl/sh-pfc/gpio.c
> +++ b/drivers/pinctrl/sh-pfc/gpio.c
> @@ -153,8 +153,7 @@ static void gpio_pin_set_value(struct sh_pfc_chip *chip,
> unsigned offset, int value)
>  {
>  	struct sh_pfc_gpio_data_reg *reg;
> -	unsigned long pos;
> -	unsigned int bit;
> +	unsigned int pos, bit;
> 
>  	gpio_get_data_reg(chip, offset, &reg, &bit);
> 
> @@ -185,8 +184,7 @@ static int gpio_pin_get(struct gpio_chip *gc, unsigned
> offset) {
>  	struct sh_pfc_chip *chip = gpio_to_pfc_chip(gc);
>  	struct sh_pfc_gpio_data_reg *reg;
> -	unsigned long pos;
> -	unsigned int bit;
> +	unsigned int pos, bit;
> 
>  	gpio_get_data_reg(chip, offset, &reg, &bit);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/4] pinctrl: sh-pfc: Use unsigned int for register/field widths and offsets

[Geert Uytterhoeven]

Hi Laurent,

On Thu, Mar 5, 2015 at 10:20 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>> @@ -228,12 +228,11 @@ static void sh_pfc_write_config_reg(struct sh_pfc
>> *pfc, }
>>
>>  static int sh_pfc_get_config_reg(struct sh_pfc *pfc, u16 enum_id,
>> -                              const struct pinmux_cfg_reg **crp, int *fieldp,
>> -                              u32 *valuep)
>> +                              const struct pinmux_cfg_reg **crp,
>> +                              unsigned int *fieldp, u32 *valuep)
>>  {
>>       const struct pinmux_cfg_reg *config_reg;
>> -     unsigned long r_width, f_width, curr_width;
>> -     unsigned int k, m, pos, bit_pos;
>> +     unsigned int r_width, f_width, curr_width, k, m, pos, bit_pos;
>
> I find declaring multiple variables per line quite difficult to read. I know
> it's the current coding style in this driver, but I'd like to fix it at some
> point. I would move to one variable per line as part of this patch for the
> code that it touches. Alternatively could you at least not make it worse (here
> and below) ? :-)

There are just too many variables ;-)

I usually declare variables in the order of appearance, but try to
keep variables
of the same type together.

Some declarations could move inside the while, e.g.

        while (1) {
                const struct pinmux_cfg_reg *config_reg =
pfc->info->cfg_regs + k;
                unsigned int r_width = config_reg->reg_width;
                unsigned int f_width = config_reg->field_width;
                unsigned int pos = 0, m= 0;

                if (!r_width)
                        break;

Would you like that?

>>       u32 ncomb, n;
>>
>>       k = 0;
>> @@ -300,9 +299,9 @@ int sh_pfc_config_mux(struct sh_pfc *pfc, unsigned mark,
>> int pinmux_type) const struct pinmux_cfg_reg *cr = NULL;
>>       u16 enum_id;
>>       const struct pinmux_range *range;
>> -     int in_range, pos, field;
>> +     int in_range, pos, ret;
>> +     unsigned int field;
>>       u32 value;
>> -     int ret;

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 4/4] pinctrl: sh-pfc: Use unsigned int for register/field widths and offsets

[Laurent Pinchart]

Hi Geert,

On Thursday 05 March 2015 10:34:18 Geert Uytterhoeven wrote:
> On Thu, Mar 5, 2015 at 10:20 AM, Laurent Pinchart wrote:
> >> @@ -228,12 +228,11 @@ static void sh_pfc_write_config_reg(struct sh_pfc
> >> *pfc,
> >>  }
> >> 
> >>  static int sh_pfc_get_config_reg(struct sh_pfc *pfc, u16 enum_id,
> >> -                              const struct pinmux_cfg_reg **crp, int
> >> *fieldp,
> >> -                              u32 *valuep)
> >> +                              const struct pinmux_cfg_reg **crp,
> >> +                              unsigned int *fieldp, u32 *valuep)
> >>  {
> >>       const struct pinmux_cfg_reg *config_reg;
> >> -     unsigned long r_width, f_width, curr_width;
> >> -     unsigned int k, m, pos, bit_pos;
> >> +     unsigned int r_width, f_width, curr_width, k, m, pos, bit_pos;
> > 
> > I find declaring multiple variables per line quite difficult to read. I
> > know it's the current coding style in this driver, but I'd like to fix it
> > at some point. I would move to one variable per line as part of this
> > patch for the code that it touches. Alternatively could you at least not
> > make it worse (here and below) ? :-)
> 
> There are just too many variables ;-)

Well, if we need them, we need them :-)

> I usually declare variables in the order of appearance, but try to
> keep variables of the same type together.
> 
> Some declarations could move inside the while, e.g.
> 
>         while (1) {
>                 const struct pinmux_cfg_reg *config_reg =
> pfc->info->cfg_regs + k;
>                 unsigned int r_width = config_reg->reg_width;
>                 unsigned int f_width = config_reg->field_width;
>                 unsigned int pos = 0, m= 0;
> 
>                 if (!r_width)
>                         break;
> 
> Would you like that?

Yes, but please split pos and m on two different lines :-)

> >>       u32 ncomb, n;
> >>       
> >>       k = 0;
> >> @@ -300,9 +299,9 @@ int sh_pfc_config_mux(struct sh_pfc *pfc, unsigned
> >> mark, int pinmux_type) const struct pinmux_cfg_reg *cr = NULL;
> >>       u16 enum_id;
> >>       const struct pinmux_range *range;
> >> -     int in_range, pos, field;
> >> +     int in_range, pos, ret;
> >> +     unsigned int field;
> >>       u32 value;
> >> -     int ret;

-- 
Regards,

Laurent Pinchart