Re: [PATCH v2 1/2] ARM: mvebu: Add support to get the ID and the revision of a SoC

[Gregory CLEMENT <gregory.clement@free-electrons.com>]

On 03/01/2014 19:48, Thomas Petazzoni wrote:
> Dear Gregory CLEMENT,
> 
> On Fri,  3 Jan 2014 10:59:44 +0100, Gregory CLEMENT wrote:
>> +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

> 
>> +		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.

> 
>> +
>> +		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
> 
>> +		/* 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

> 
>> +	return ret;
>> +}
>> +arch_initcall(mvebu_soc_id_init);
> 
>> +#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