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]
Date:	Fri, 17 Aug 2012 11:34:31 +0000
From:	Arnd Bergmann <arnd@...db.de>
To:	Chao Xie <xiechao.mail@...il.com>
Cc:	haojian.zhuang@...il.com, mturquette@...aro.org,
	viresh.linux@...il.com, s.hauer@...gutronix.de,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	chao.xie@...vell.com
Subject: Re: [PATCH V4 2/5] clk: mmp: add clock definition for pxa168

On Friday 17 August 2012, Chao Xie wrote:
> +void __init pxa168_clk_init(void)
> +{
> +       struct clk *clk;
> +       struct clk *uart_pll;
> +       void __iomem *mpmu_base;
> +       void __iomem *apmu_base;
> +       void __iomem *apbc_base;
> +
> +       mpmu_base = ioremap(APB_PHYS_BASE + 0x50000, SZ_4K);
> +       if (mpmu_base == NULL) {
> +               pr_err("error to ioremap MPMU base\n");
> +               return;
> +       }
> +
> +       apmu_base = ioremap(AXI_PHYS_BASE + 0x82800, SZ_4K);
> +       if (apmu_base == NULL) {
> +               pr_err("error to ioremap APMU base\n");
> +               return;
> +       }
> +
> +       apbc_base = ioremap(APB_PHYS_BASE + 0x15000, SZ_4K);
> +       if (apbc_base == NULL) {
> +               pr_err("error to ioremap APBC base\n");
> +               return;
> +       }

I hadn't noticed this before, but you are hardcoded physical address locations
in the driver, which is not so good since we're trying to move all those
locations to device tree. Maybe someone else has an idea to do this better.

> +       clk = clk_register_fixed_rate(NULL, "clk32", NULL, CLK_IS_ROOT, 3200);
> +       clk_register_clkdev(clk, "clk32", NULL);
> +
> +       clk =
> +           clk_register_fixed_rate(NULL, "vctcxo", NULL, CLK_IS_ROOT,
> +                                   26000000);
> +       clk_register_clkdev(clk, "vctcxo", NULL);
> +
> +       clk =
> +           clk_register_fixed_rate(NULL, "pll1", NULL, CLK_IS_ROOT, 624000000);
> +       clk_register_clkdev(clk, "pll1", NULL);

Ok, so you've managed to remove the array, good!

I'm still not overly happy with the style of newline after "=", that is
very unusual, and only saves you two characters per line. I'd say just move
them each into one line as you did in the first one here. It's less important
to align the arguments to the opening braces if you are trying to stay in the
80 character limit. Another option would be to rename the variable to just
'c' instead of 'clk' ;-)

	Arnd


	Arnd
--
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/

Powered by blists - more mailing lists