On 03/01/2014 19:48, Thomas Petazzoni wrote:

Dear Gregory CLEMENT,

On Fri,  3 Jan 2014 10:59:44 +0100, Gregory CLEMENT wrote:

quoted

+static int __init mvebu_soc_id_init(void)
+{
+	struct device_node *np;
+	int ret = 0;
+
+	np = of_find_matching_node(NULL, mvebu_pcie_of_match_table);
+	if (np) {

Change this to:

	if (!np)
		return 0;

so that you avoid indenting the entire function code inside the if
{ ... } block.

ok

quoted

+		void __iomem *pci_base;
+		struct clk *clk;
+		/*
+		 * ID and revision are available from any port, so we
+		 * just pick the first one
+		 */
+		struct device_node *child = of_get_next_child(np, NULL);

Maybe check that child is not NULL here?

yes I was a little lazy with it: if child is NULL then of_clk_get_by_name
will return an error but then the error message will be a misleading.

quoted

+
+		clk = of_clk_get_by_name(child, NULL);
+		if (IS_ERR(clk)) {
+			pr_err("%s: cannot get clock\n", __func__);

I think you should rather do:

#define pr_fmt(fmt) "mvebu-soc-id: " fmt

at the beginning of your C file and get rid of the "%s" for the
__func__.

ok thanks for the tip

quoted

+		/* SoC ID */
+		soc_dev_id = __raw_readl(pci_base + PCIE_DEV_ID_OFF) >> 16;
+
+		/* SoC revision */
+		soc_rev = __raw_readl(pci_base + PCIE_DEV_REV_OFF)
+			& SOC_REV_MASK;

Use readl() for both of these reads. __raw_readl() will not work
properly when the system is booted big-endian.

ok

quoted

+	return ret;
+}
+arch_initcall(mvebu_soc_id_init);

quoted

+#ifdef CONFIG_ARCH_MVEBU
+int mvebu_get_soc_id(u32 *dev, u32 *rev);
+#else
+int mvebu_get_soc_id(u32 *dev, u32 *rev)

Missing "static inline". Without these, if this header file is included
more than once, you will have two times the same symbol defined.

ok

Best regards,

Thomas

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com