On Fri, Sep 09, 2016 at 06:57:54PM +0200, Geert Uytterhoeven wrote:
> Hi Simon,
>
> On Fri, Sep 9, 2016 at 5:26 PM, Simon Horman <horms@xxxxxxxxxxxx> wrote:
> >> > + /* GP6_16-23 -> bits 31-24 of pocctrl
> >> > + * GP6_06 -> bit 23 of pocctrl
> >> > + * GP6_00-05 -> bits 22-17 of pocctrl
> >> > + * GP6_07 -> bit 16 of pocctrl
> >> > + * GP6_14 -> bit 15 of pocctrl
> >> > + * GP6_08-13 -> bits 14-09 of pocctrl
> >> > + * GP6_15 -> bit 08 of pocctrl
> >> > + */
> >> > + if (pin == RCAR_GP_PIN(6, 6) || pin == RCAR_GP_PIN(6, 14))
> >> > + return 31 - 2 - (pin & 0x1f);
> >> > + else if (pin == RCAR_GP_PIN(6, 7) || pin == RCAR_GP_PIN(6, 15))
> >> > + return 31 - 8 - (pin & 0x1f);
> >> > + else if (pin < RCAR_GP_PIN(6, 14))
> >> > + return 31 - 9 - (pin & 0x1f);
> >> > + else
> >> > + return 31 + 16 - (pin & 0x1f);
> >>
> >> While your code is correct, I think it's easier for the casual reader to use
> >> a plain switch () statement, and let the optimizer handle the rest.
> >
> > Like this? If so I don't particularly mind but it doesn't seem clearer to
> > me.
> >
> > +static int r8a7794_pin_to_pocctrl(struct sh_pfc *pfc, unsigned int pin, u32 *pocctrl)
> > +{
> > + *pocctrl = 0xe606006c;
> > +
> > + switch (pin) {
> > + case RCAR_GP_PIN(6, 0):
> > + return 22;
> > + case RCAR_GP_PIN(6, 1):
> > + return 21;
>
> Right, a full list of cases indeed doesn't look that much better.
>
> Note that you can use "switch (pin & 0x1f)", and have ranges in case
> statements:
>
> switch (pin & 0x1f) {
> case 6: return 23:
> case 7: return 16;
> case 14: return 15;
> case 15: return 8;
> case 0...5:
> case 7...13:
> return 22 - (pin & 0x1f);
> case 16..23:
> return 47 - (pin & 0x1f);
> }
>
> Does that look better?
Yes, I think that could be a winning combination.
