| Message ID | 1425058685-12956-5-git-send-email-geert+renesas@glider.be |
|---|---|
| State | New |
| Headers | show |
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, ®, &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, ®, &bit);
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
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;
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, ®, &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, ®, &bit);
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(-)