On Fri, Jan 20, 2012 at 7:22 PM, Stephen Warren <swarren@xxxxxxxxxx> wrote:
> This adds a driver for the Tegra pinmux, and required parameterization
> data for Tegra20 and Tegra30.
Very appetizing - the only scary thing is the size. Some
comments, if noone else yells after these are addressed I might
just merge it (or I find more things to complain about...)
> +static int tegra_pinconf_reg(struct tegra_pmx *pmx,
> + const struct tegra_pingroup *g,
> + enum tegra_pinconf_param param,
> + s8 *bank, s16 *reg, s8 *bit, s8 *width)
Why are the four last parameters signed?
Or, rather as it seems to be a consequence of this:
> +struct tegra_pingroup {
> + const char *name;
> + const unsigned *pins;
> + unsigned npins;
> + int funcs[4];
> + int func_safe;
> + s16 mux_reg; /* Mux register offset */
> + s8 mux_bank; /* Mux register bank */
> + s8 mux_bit; /* Mux register bit */
> + s16 pupd_reg; /* Pull-up/down register offset */
> + s8 pupd_bank; /* Pull-up/down register bank */
> + s8 pupd_bit; /* Pull-up/down register bit */
> + s16 tri_reg; /* Tri-state register offset */
> + s8 tri_bank; /* Tri-state register bank */
> + s8 tri_bit; /* Tri-state register bit */
> + s16 einput_reg; /* Enable-input register offset */
> + s8 einput_bank; /* Enable-input register bank */
> + s8 einput_bit; /* Enable-input register bit */
> + s16 odrain_reg; /* Open-drain register offset */
> + s8 odrain_bank; /* Open-drain register bank */
> + s8 odrain_bit; /* Open-drain register bit */
> + s16 lock_reg; /* Lock register offset */
> + s8 lock_bank; /* Lock register bank */
> + s8 lock_bit; /* Lock register bit */
> + s16 ioreset_reg; /* IO reset register offset */
> + s8 ioreset_bank; /* IO reset register bank */
> + s8 ioreset_bit; /* IO reset register bit */
> + s16 drv_reg; /* Drive fields register offset */
> + s8 drv_bank; /* Drive fields register bank */
> + s8 hsm_bit; /* High Speed Mode register bit */
> + s8 schmitt_bit; /* Scmitt register bit */
> + s8 lpmd_bit; /* Low Power Mode register bit */
> + s8 drvdn_bit; /* Drive Down register bit */
> + s8 drvdn_width; /* Drive Down field width */
> + s8 drvup_bit; /* Drive Up register bit */
> + s8 drvup_width; /* Drive Up field width */
> + s8 slwr_bit; /* Slew Rising register bit */
> + s8 slwr_width; /* Slew Rising field width */
> + s8 slwf_bit; /* Slew Falling register bit */
> + s8 slwf_width; /* Slew Falling field width */
> +};
Why are all of these things (reg bank bit ets) signed?
Especially since a lot of them appear to translate into
readl() writel() calls in the end, and these are unsigned.
Also, things named *_bit are a bit (no pun intended) binary,
are they containing a single bit? In that case say
u8 foo:1
To mark that it's only one bit wide, or u8 foo:4 for four bits
etc.
Looking from the use of them in this manner:
val &= ~(0x3 << g->mux_bit);
It looks like mux_bit should be
u8 mux_bit:2;
And so on.
func_safe doesn't look like an int either when I look at
the code. It's something else, u8?
Second, please move out the inline comment to kerneldoc
above the struct.
The rest is pretty straight forward I think.
Yours,
Linus Wallleij
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
