On Tue, 2012-08-14 at 18:00 +0530, Mahapatra, Chandrabhanu wrote:

On Tue, Aug 14, 2012 at 3:18 PM, Tomi Valkeinen [off-list ref] wrote:

quoted

On Mon, 2012-08-13 at 17:29 +0530, Chandrabhanu Mahapatra wrote:

quoted

All the cpu_is checks have been moved to dss_init_features function providing a
much more generic and cleaner interface. The OMAP version and revision specific
initializations in various functions are cleaned and the necessary data are
moved to dss_features structure which is local to dss.c.

It'd be good to have some version information here. Even just [PATCH v3
3/6] in the subject is good, but of course the best is if there's a
short change log

Is it ok to have short change log in the individual patch description
itself. I normally prefer the cover letter for this.

No, the patch description is added to git repository, and shouldn't
contain that kind of "extra" information. But you can add the version
information to the posted patch.

The diff stats below, after the "---" line, are discarded by git when
applying the patch. So I think you can just insert any text below ---
(but before the actual diffs) and they do not appear in the final git
commit.

I've not actually used that myself, and I can't find notes about it in
the documentation, so I'm not sure if there are any restrictions about
it. I've seen it used, though.

quoted

quoted

Signed-off-by: Chandrabhanu Mahapatra <redacted>
---
 drivers/video/omap2/dss/dss.c |  115 ++++++++++++++++++++++++++---------------
 1 file changed, 74 insertions(+), 41 deletions(-)

diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c
index 7b1c6ac..6ab236e 100644
--- a/drivers/video/omap2/dss/dss.c
+++ b/drivers/video/omap2/dss/dss.c

@@ -31,6 +31,7 @@
 #include <linux/clk.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/dma-mapping.h>

Hmm, what is this for?

I should have included "include/linux/gfp.h" instead. It defines GFP_KERNEL.

quoted

quoted

 #include <video/omapdss.h>

@@ -65,6 +66,13 @@ struct dss_reg {
 static int dss_runtime_get(void);
 static void dss_runtime_put(void);

+struct dss_features {
+     u16 fck_div_max;
+     int factor;
+     char *clk_name;
+     bool (*check_cinfo_fck) (void);
+};

Is the check_cinfo_fck a leftover from previous versions?

Sorry, to have skipped that.

quoted

I think "factor" name is too vague. If I remember right, it's some kind
of implicit multiplier for the dss fck. So "dss_fck_multiplier"? You
could check the clock trees to verify what the multiplier exactly was,
and see if you can come up with a better name =).

dss_fck_multiplier sounds good to me. DSS clocks trees do not seem to

Yes, it's not really DSS thing, but PRCM. If things were perfect, DSS
wouldn't even need to know about the clock.

mention anything about this multiplier. I found clock "dpll4_mx4_clk"
in omap3 trm but nothing called "dpll_per_m5x2_clk" in omap4 trm. Any
references please you know about?

Hmm, I think that's the linux's name for it. TRM seems to call it
CLKOUTX2_M5 (in the power, reset and clock management section).

quoted

quoted

+     if (cpu_is_omap24xx())
+             dss.feat = &omap2_dss_features;
+     else if (cpu_is_omap34xx())
+             dss.feat = &omap34_dss_features;
+     else if (cpu_is_omap3630())
+             dss.feat = &omap36_dss_features;
+     else
+             dss.feat = &omap4_dss_features;

Check for omap4 also, and if the cpu is none of the above, return error.

I was wondering what error value to return. I was considering EINVAL
(error in value) and ENODEV (no such device). But nothing seems to fit
this case.

ENODEV sounds fine to me. I think EINVAL should be used when some given
parameter was wrong.