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

Message ID 1425058685-12956-5-git-send-email-geert+renesas@glider.be
State New
Headers show

Commit Message

Geert Uytterhoeven Feb. 27, 2015, 5:38 p.m. UTC
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(-)

Comments

Laurent Pinchart March 5, 2015, 9:20 a.m. UTC | #1
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);
Geert Uytterhoeven March 5, 2015, 9:34 a.m. UTC | #2
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
--
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
Laurent Pinchart March 6, 2015, 10:57 a.m. UTC | #3
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;

Patch
diff mbox

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);