Hi Linus,
Looks pretty good. just some minors.
2011/8/19 Linus Walleij <linus.wall...@stericsson.com>:
> From: Linus Walleij <linus.wall...@linaro.org>
>
> 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.wall...@linaro.org>
>
> +
> +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
_______________________________________________
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
