On Tue, 2012-08-14 at 18:00 +0530, Mahapatra, Chandrabhanu wrote:
> On Tue, Aug 14, 2012 at 3:18 PM, Tomi Valkeinen <tomi.valkeinen@xxxxxx> wrote:
> > On Mon, 2012-08-13 at 17:29 +0530, Chandrabhanu Mahapatra wrote:
> >> 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.
> >> Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@xxxxxx>
> >> ---
> >> 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.
>
> >> #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.
>
> > 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).
> >> + 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.
Attachment:
signature.asc
Description: This is a digitally signed message part
