Re: [PATCH 1/9] SOC: brcmstb: add memory API

From: Christoph Hellwig
Date: Tue Oct 17 2017 - 04:24:21 EST


> +/* Macros to help extract property data */
> +#define U8TOU32(b, offs) \
> + ((((u32)b[0 + offs] << 0) & 0x000000ff) | \
> + (((u32)b[1 + offs] << 8) & 0x0000ff00) | \
> + (((u32)b[2 + offs] << 16) & 0x00ff0000) | \
> + (((u32)b[3 + offs] << 24) & 0xff000000))

Please us helpers like get_unaligned_le32 instead opencoding them.

> +#define DT_PROP_DATA_TO_U32(b, offs) (fdt32_to_cpu(U8TOU32(b, offs)))

And together with this it looks really whacky. So you're converting
from le to native first and then do another conversion from be to cpu?

Something doesn't work out here.

> +/* -------------------- Functions -------------------- */

Please remove pointless comments like this one.

> +
> +/*
> + * If the DT nodes are handy, determine which MEMC holds the specified
> + * physical address.
> + */
> +#ifdef CONFIG_ARCH_BRCMSTB
> +int __brcmstb_memory_phys_addr_to_memc(phys_addr_t pa, void __iomem *base)

Please move this into the arm arch code.

> +#elif defined(CONFIG_MIPS)

And this into the mips arch code.

> +EXPORT_SYMBOL(brcmstb_memory_phys_addr_to_memc);

> +EXPORT_SYMBOL(brcmstb_memory_memc_size);

Why is this exported?