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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGsJ_4wK6V0aQJd7f1geM8XnzQO+3TNCE6Q8_j=VYf_z_gcmpA@mail.gmail.com>
Date:	Sun, 21 Aug 2011 21:55:53 +0800
From:	Barry Song <21cnbao@...il.com>
To:	Linus Walleij <linus.walleij@...ricsson.com>
Cc:	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	Grant Likely <grant.likely@...retlab.ca>,
	Russell King <linux@....linux.org.uk>,
	Joe Perches <joe@...ches.com>,
	Linaro Dev <linaro-dev@...ts.linaro.org>
Subject: Re: [PATCH 2/4 v4] pinmux: add a driver for the U300 pinmux

Hi Linus,
Looks pretty good. just some minors.

2011/8/19 Linus Walleij <linus.walleij@...ricsson.com>:
> From: Linus Walleij <linus.walleij@...aro.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.walleij@...aro.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/