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

Hi Karol,
Thank you for the review.
Please find my comments inline.

On Wed, Sep 19, 2012 at 3:08 PM, Karol Lewandowski [off-list ref] wrote:

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

quoted

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.

quoted

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.

quoted

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

  char *compatible;

quoted

+};
+
+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;

quoted

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

quoted

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

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

quoted

+
+     /* 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 [off-list ref] */
      .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