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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
