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

[Arun Kumar K <arun.kk@samsung.com>]

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@samsung.com> 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@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
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html