Re: [PATCH v3] ARM: EXYNOS: Add MFC device tree support

[Karol Lewandowski <k.lewandowsk@samsung.com>]

On 09/14/2012 05:38 PM, Arun Kumar K wrote:

> This patch adds device tree entry for MFC v6 in the Exynos5
> SoC. Makes the required changes in the clock files and adds
> MFC to the DT device list.


Hi!

Thanks for working on this patch. Please allow me to add few
comments.


> diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi
> index b55794b..5df2f99 100644
> --- a/arch/arm/boot/dts/exynos5250.dtsi
> +++ b/arch/arm/boot/dts/exynos5250.dtsi
> @@ -62,6 +62,12 @@
>  		interrupts = <0 42 0>;
>  	};
>  
> +	mfc {


Nitpick - shouldn't node names be generic?  MFC is strictly
samsung specific, something like "codec"/"video-codec" would make
more sense (IMVHO). I would prefer to see address too (e.g.
codec@0x11000000).

However, I do see that rtc below doesn't specify address in node too,
so maybe I'm missing something here.


>  
> +struct mfc_dt_meminfo {
> +	unsigned long loff;
> +	unsigned long lsize;
> +	unsigned long roff;
> +	unsigned long rsize;


  char *compatible;

> +};
> +
> +int fdt_find_mfc_mem(unsigned long node, const char *uname, int depth,
> +		void *data)
> +{
> +	__be32 *prop;
> +	unsigned long len;
> +	struct mfc_dt_meminfo *mfc_mem = data;
> +
> +	if (!of_flat_dt_is_compatible(node, "samsung,mfc-v6"))
> +		return 0;


  if (!of_flat_dt_is_compatible(node, mfc_mem->compatible))
	return 0;

> +
> +	prop = of_get_flat_dt_prop(node, "samsung,mfc-l", &len);
> +	if (!prop)
> +		return 0;
> +	mfc_mem->loff = of_read_ulong(prop, len/4);
> +
> +	prop = of_get_flat_dt_prop(node, "samsung,mfc-l-size", &len);
> +	if (!prop)
> +		return 0;
> +	mfc_mem->lsize = of_read_ulong(prop, len/4);
> +
> +	prop = of_get_flat_dt_prop(node, "samsung,mfc-r", &len);
> +	if (!prop)
> +		return 0;
> +	mfc_mem->roff = of_read_ulong(prop, len/4);
> +
> +	prop = of_get_flat_dt_prop(node, "samsung,mfc-r-size", &len);
> +	if (!prop)
> +		return 0;
> +	mfc_mem->rsize = of_read_ulong(prop, len/4);
> +
> +	return 1;
> +}


Above function could be reused for mfc-v5 (exynos4-dt.c) if compatible
string weren't hardcoded. Thus, please consider changing that and
moving this function to some common(.c?) file - you can see one possible
solution inline.

> +
> +static void __init exynos5_reserve(void)
> +{
> +	struct mfc_dt_meminfo mfc_mem;


        mfc_mem.compatible = "samsung,mfc-v6";

> +
> +	/* Reserve memory for MFC only if it's available */
> +	if (of_scan_flat_dt(fdt_find_mfc_mem, &mfc_mem))
> +		s5p_mfc_reserve_mem(mfc_mem.roff, mfc_mem.rsize, mfc_mem.loff,
> +				mfc_mem.lsize);
> +}
> +
>  DT_MACHINE_START(EXYNOS5_DT, "SAMSUNG EXYNOS5 (Flattened Device Tree)")
>  	/* Maintainer: Kukjin Kim <kgene.kim@samsung.com> */
>  	.init_irq	= exynos5_init_irq,
> @@ -94,4 +148,5 @@ DT_MACHINE_START(EXYNOS5_DT, "SAMSUNG EXYNOS5 (Flattened Device Tree)")
>  	.timer		= &exynos4_timer,
>  	.dt_compat	= exynos5250_dt_compat,
>  	.restart        = exynos5_restart,
> +	.reserve	= exynos5_reserve,
>  MACHINE_END


Regards,
-- 
Karol Lewandowski | Samsung Poland R&D Center | Linux/Platform