Hi Karol,
Thank you for the review.
Please find my comments inline.
On Wed, Sep 19, 2012 at 3:08 PM, Karol Lewandowski <k.lewandowsk@xxxxxxxxxxx> wrote:
> 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.
>
Yes codec makes it more generic.
I too dont see the addresses used everywhere in node names.
If somebody can please suggest which one is better
mfc OR codec OR codec@0x11000000 as node name.
>
>>
>> +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.
>
Yes I can move it to common.c file and the structure definition to common.h
file for function reusability. Will make this change.
>> +
>> +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@xxxxxxxxxxx> */
>> .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
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.htmlÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ;¥Šwÿº{.nÇ+‰·¥Š{±þƦ²éàþÊ þ)í…æèw* jg¬±¨ ¶‰šŽŠÝ¢jÿ¾«þG«?éÿ¢¸ ¢·¦j:+v‰¨ŠwèjØm¶Ÿÿþø ¯ù ®w¥þŠàþf£¢·hš?â?úÿ†Ù¥
