Re: [PATCH 2/4 v4] pinmux: add a driver for the U300 pinmux

[Barry Song]

Hi Linus,
Looks pretty good. just some minors.

2011/8/19 Linus Walleij <linus.walleij@xxxxxxxxxxxxxx>:
> From: Linus Walleij <linus.walleij@xxxxxxxxxx>
>
> This adds a driver for the U300 pinmux portions of the system
> controller "SYSCON". It also serves as an example of how to use
> the pinmux subsystem. This driver also houses the platform data
> for the only supported platform.

yes. it is a good example for us to follow.

>
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
>
> +
> +static int __exit u300_pmx_remove(struct platform_device *pdev)
> +{
> + Â Â Â struct u300_pmx *upmx = platform_get_drvdata(pdev);
> +
> + Â Â Â if (upmx) {

why do you need to check whether upmx is not NULL. if it is NULL, we
have failed in probe().
and it is also impossible to call this remove() twice.

> + Â Â Â Â Â Â Â pinmux_unregister(upmx->pmx);
> + Â Â Â Â Â Â Â iounmap(upmx->virtbase);
> + Â Â Â Â Â Â Â release_mem_region(upmx->phybase, upmx->physize);
> + Â Â Â Â Â Â Â platform_set_drvdata(pdev, NULL);
> + Â Â Â Â Â Â Â kfree(upmx);
> + Â Â Â }
> +
> + Â Â Â return 0;
> +}
> +
> +static struct platform_driver u300_pmx_driver = {
> + Â Â Â .driver = {
> + Â Â Â Â Â Â Â .name = DRIVER_NAME,
> + Â Â Â Â Â Â Â .owner = THIS_MODULE,
> + Â Â Â },
> + Â Â Â .remove = __exit_p(u300_pmx_remove),

if you want to add deepsleep support(i mean pinmux crl will lose power
while suspending), what do you plan to add?
save U300_SYSCON_PMC1LR, U300_SYSCON_PMC1HR, U300_SYSCON_PMC2R,
U300_SYSCON_PMC3R, U300_SYSCON_PMC4R in suspend and restore them in
resume?

it is also a generic issue to pinmux.

> +/*
> + * Register definitions for the U300 Padmux control registers in the
> + * system controller
> + */
> +
> +/* PAD MUX Control register 1 (LOW) 16bit (R/W) */
> +#define U300_SYSCON_PMC1LR Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â (0x007C)
...
> +#define U300_SYSCON_PMC4R_APP_MISC_16_EMIF_1_STATIC_CS5_N Â Â Â(0x0200)

do you want these "(" ")" for macros?

Thanks
barry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/