Re: [PATCH 1/6] OMAPDSS: DISPC: cleanup cpu_is_xxxx checks
On Mon, 2012-08-13 at 17:28 +0530, Chandrabhanu Mahapatra wrote: All the cpu_is checks have been moved to dispc_init_features function providing a much more generic and cleaner interface. The OMAP version and revision specific functions and data are initialized by dispc_features structure which is local to dispc.c. The comments I gave for the dss.c patch apply to this one also, about devm_free, __initdata, and the names of the feat arrays. Otherwise, looks good. Tomi signature.asc Description: This is a digitally signed message part
Re: [PATCH 1/6] OMAPDSS: DISPC: cleanup cpu_is_xxxx checks
+static const struct __initdata dispc_features omap2_dispc_features = {
+ .hp_max = 256,
+ .vp_max = 255,
+ .sw_max = 64,
+ .sw_start = 5,
+ .fp_start = 15,
+ .bp_start = 27,
+ .calc_scaling = dispc_ovl_calc_scaling_24xx,
+ .calc_core_clk = calc_core_clk_24xx,
+};
+
+static const struct __initdata dispc_features omap3_2_1_dispc_features = {
+ .hp_max = 256,
+ .vp_max = 255,
+ .sw_max = 64,
+ .sw_start = 5,
+ .fp_start = 15,
+ .bp_start = 27,
+ .calc_scaling = dispc_ovl_calc_scaling_34xx,
+ .calc_core_clk = calc_core_clk_34xx,
+};
+
+static const struct __initdata dispc_features omap3_3_0_dispc_features = {
+ .hp_max = 4096,
+ .vp_max = 4095,
+ .sw_max = 256,
+ .sw_start = 7,
+ .fp_start = 19,
+ .bp_start = 31,
+ .calc_scaling = dispc_ovl_calc_scaling_34xx,
+ .calc_core_clk = calc_core_clk_34xx,
+};
+
+static const struct __initdata dispc_features omap4_dispc_features = {
+ .hp_max = 4096,
+ .vp_max = 4095,
+ .sw_max = 256,
+ .sw_start = 7,
+ .fp_start = 19,
+ .bp_start = 31,
+ .calc_scaling = dispc_ovl_calc_scaling_44xx,
+ .calc_core_clk = calc_core_clk_44xx,
+};
+
Here the dispc_features not only mention the omap name but also the
revision like omap3_3_0_dispc_features which initializes data for
OMAP3430_REV_ES3_0 and higher. May be omap34xx_rev3_0_dispc_features
is a better name for this. For others omap44xx_dispc_features kind of
name should be ok without revision number being mentioned. What d you
say?
+static int __init dispc_init_features(struct device *dev)
+{
+ dispc.feat = devm_kzalloc(dev, sizeof(*dispc.feat), GFP_KERNEL);
+ if (!dispc.feat) {
+ dev_err(dev, Failed to allocate DISPC Features\n);
+ return -ENOMEM;
+ }
+
+ if (cpu_is_omap24xx()) {
+ dispc.feat = omap2_dispc_features;
+ } else if (cpu_is_omap34xx()) {
+ if (omap_rev() OMAP3430_REV_ES3_0)
+ dispc.feat = omap3_2_1_dispc_features;
+ else
+ dispc.feat = omap3_3_0_dispc_features;
+ } else {
+ dispc.feat = omap4_dispc_features;
+ }
+
+ return 0;
+}
+
--
Chandrabhanu Mahapatra
Texas Instruments India Pvt. Ltd.
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/6] OMAPDSS: DISPC: cleanup cpu_is_xxxx checks
On Tue, 2012-08-14 at 17:33 +0530, Mahapatra, Chandrabhanu wrote:
+static const struct __initdata dispc_features omap2_dispc_features = {
+ .hp_max = 256,
+ .vp_max = 255,
+ .sw_max = 64,
+ .sw_start = 5,
+ .fp_start = 15,
+ .bp_start = 27,
+ .calc_scaling = dispc_ovl_calc_scaling_24xx,
+ .calc_core_clk = calc_core_clk_24xx,
+};
+
+static const struct __initdata dispc_features omap3_2_1_dispc_features = {
+ .hp_max = 256,
+ .vp_max = 255,
+ .sw_max = 64,
+ .sw_start = 5,
+ .fp_start = 15,
+ .bp_start = 27,
+ .calc_scaling = dispc_ovl_calc_scaling_34xx,
+ .calc_core_clk = calc_core_clk_34xx,
+};
+
+static const struct __initdata dispc_features omap3_3_0_dispc_features = {
+ .hp_max = 4096,
+ .vp_max = 4095,
+ .sw_max = 256,
+ .sw_start = 7,
+ .fp_start = 19,
+ .bp_start = 31,
+ .calc_scaling = dispc_ovl_calc_scaling_34xx,
+ .calc_core_clk = calc_core_clk_34xx,
+};
+
+static const struct __initdata dispc_features omap4_dispc_features = {
+ .hp_max = 4096,
+ .vp_max = 4095,
+ .sw_max = 256,
+ .sw_start = 7,
+ .fp_start = 19,
+ .bp_start = 31,
+ .calc_scaling = dispc_ovl_calc_scaling_44xx,
+ .calc_core_clk = calc_core_clk_44xx,
+};
+
Here the dispc_features not only mention the omap name but also the
revision like omap3_3_0_dispc_features which initializes data for
OMAP3430_REV_ES3_0 and higher. May be omap34xx_rev3_0_dispc_features
is a better name for this. For others omap44xx_dispc_features kind of
name should be ok without revision number being mentioned. What d you
say?
Sounds ok to me.
Tomi
signature.asc
Description: This is a digitally signed message part
Re: [PATCH 1/6] OMAPDSS: DISPC: cleanup cpu_is_xxxx checks
On Wed, 2012-08-08 at 17:07 +0530, Chandrabhanu Mahapatra wrote:
All the cpu_is checks have been moved to dispc_init_features function
providing
a much more generic and cleaner interface. The OMAP version and revision
specific functions are initialized by dispc_features structure local to
dispc.c.
Signed-off-by: Chandrabhanu Mahapatra cmahapa...@ti.com
---
drivers/video/omap2/dss/dispc.c | 476
++-
1 file changed, 315 insertions(+), 161 deletions(-)
diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
index 5b289c5..7e0b080 100644
--- a/drivers/video/omap2/dss/dispc.c
+++ b/drivers/video/omap2/dss/dispc.c
@@ -75,12 +75,60 @@ enum omap_burst_size {
#define REG_FLD_MOD(idx, val, start, end)\
dispc_write_reg(idx, FLD_MOD(dispc_read_reg(idx), val, start, end))
+static int dispc_ovl_calc_scaling_24xx(enum omap_channel channel,
+ const struct omap_video_timings *mgr_timings, u16 width, u16 height,
+ u16 out_width, u16 out_height, enum omap_color_mode color_mode,
+ bool *five_taps, int *x_predecim, int *y_predecim, int *decim_x,
+ int *decim_y, u16 pos_x, unsigned long *core_clk);
+static int dispc_ovl_calc_scaling_34xx(enum omap_channel channel,
+ const struct omap_video_timings *mgr_timings, u16 width, u16 height,
+ u16 out_width, u16 out_height, enum omap_color_mode color_mode,
+ bool *five_taps, int *x_predecim, int *y_predecim, int *decim_x,
+ int *decim_y, u16 pos_x, unsigned long *core_clk);
+static int dispc_ovl_calc_scaling_44xx(enum omap_channel channel,
+ const struct omap_video_timings *mgr_timings, u16 width, u16 height,
+ u16 out_width, u16 out_height, enum omap_color_mode color_mode,
+ bool *five_taps, int *x_predecim, int *y_predecim, int *decim_x,
+ int *decim_y, u16 pos_x, unsigned long *core_clk);
+
+static unsigned long calc_core_clk_24xx(enum omap_channel channel, u16 width,
+ u16 height, u16 out_width, u16 out_height);
+static unsigned long calc_core_clk_34xx(enum omap_channel channel, u16 width,
+ u16 height, u16 out_width, u16 out_height);
+static unsigned long calc_core_clk_44xx(enum omap_channel channel, u16 width,
+ u16 height, u16 out_width, u16 out_height);
+
+static bool _dispc_lcd_timings_ok_24xx(int hsw, int hfp, int hbp,
+ int vsw, int vfp, int vbp);
+static bool _dispc_lcd_timings_ok_44xx(int hsw, int hfp, int hbp,
+ int vsw, int vfp, int vbp);
+
+static void _dispc_mgr_set_lcd_timings_hv_24xx(enum omap_channel channel,
+ int hsw, int hfp, int hbp, int vsw, int vfp, int vbp);
+static void _dispc_mgr_set_lcd_timings_hv_44xx(enum omap_channel channel,
+ int hsw, int hfp, int hbp, int vsw, int vfp, int vbp);
While it's nice to have the initialization of struct dispc_features in
the beginning of dispc.c, it requires the above prototypes. And in the
future we may require more. For that reason I think it's better to
initialize the dispc_features at the end of dispc.c, just above
dispc_init_features(). This would be kinda similar to how drivers often
initialize their ops.
+static const struct dispc_features omap2_dispc_features = {
+ .calc_scaling = dispc_ovl_calc_scaling_24xx,
+ .calc_core_clk = calc_core_clk_24xx,
+ .lcd_timings_ok = _dispc_lcd_timings_ok_24xx,
+ .set_lcd_timings_hv = _dispc_mgr_set_lcd_timings_hv_24xx,
+};
+
+static const struct dispc_features omap3_2_1_dispc_features = {
+ .calc_scaling = dispc_ovl_calc_scaling_34xx,
+ .calc_core_clk = calc_core_clk_34xx,
+ .lcd_timings_ok = _dispc_lcd_timings_ok_24xx,
+ .set_lcd_timings_hv = _dispc_mgr_set_lcd_timings_hv_24xx,
+};
+
+static const struct dispc_features omap3_3_0_dispc_features = {
+ .calc_scaling = dispc_ovl_calc_scaling_34xx,
+ .calc_core_clk = calc_core_clk_34xx,
+ .lcd_timings_ok = _dispc_lcd_timings_ok_44xx,
+ .set_lcd_timings_hv = _dispc_mgr_set_lcd_timings_hv_44xx,
+};
+
+static const struct dispc_features omap4_dispc_features = {
+ .calc_scaling = dispc_ovl_calc_scaling_44xx,
+ .calc_core_clk = calc_core_clk_44xx,
+ .lcd_timings_ok = _dispc_lcd_timings_ok_44xx,
+ .set_lcd_timings_hv = _dispc_mgr_set_lcd_timings_hv_44xx,
+};
During runtime we only require one of these, others can be discarded.
This can be accomplished with the combination of __initdata for these,
and __init for dispc_init_features().
However, because even the one we need will be discarded, we need to copy
the values. This could be done either by having the dispc_features
struct inside dispc struct (instead of a pointer), or allocating memory
for
Re: [PATCH 1/6] OMAPDSS: DISPC: cleanup cpu_is_xxxx checks
On Wed, Aug 8, 2012 at 6:06 PM, Tomi Valkeinen tomi.valkei...@ti.com wrote:
On Wed, 2012-08-08 at 17:07 +0530, Chandrabhanu Mahapatra wrote:
All the cpu_is checks have been moved to dispc_init_features function
providing
a much more generic and cleaner interface. The OMAP version and revision
specific functions are initialized by dispc_features structure local to
dispc.c.
Signed-off-by: Chandrabhanu Mahapatra cmahapa...@ti.com
---
drivers/video/omap2/dss/dispc.c | 476
++-
1 file changed, 315 insertions(+), 161 deletions(-)
diff --git a/drivers/video/omap2/dss/dispc.c
b/drivers/video/omap2/dss/dispc.c
index 5b289c5..7e0b080 100644
--- a/drivers/video/omap2/dss/dispc.c
+++ b/drivers/video/omap2/dss/dispc.c
@@ -75,12 +75,60 @@ enum omap_burst_size {
#define REG_FLD_MOD(idx, val, start, end)\
dispc_write_reg(idx, FLD_MOD(dispc_read_reg(idx), val, start, end))
+static int dispc_ovl_calc_scaling_24xx(enum omap_channel channel,
+ const struct omap_video_timings *mgr_timings, u16 width, u16 height,
+ u16 out_width, u16 out_height, enum omap_color_mode color_mode,
+ bool *five_taps, int *x_predecim, int *y_predecim, int *decim_x,
+ int *decim_y, u16 pos_x, unsigned long *core_clk);
+static int dispc_ovl_calc_scaling_34xx(enum omap_channel channel,
+ const struct omap_video_timings *mgr_timings, u16 width, u16 height,
+ u16 out_width, u16 out_height, enum omap_color_mode color_mode,
+ bool *five_taps, int *x_predecim, int *y_predecim, int *decim_x,
+ int *decim_y, u16 pos_x, unsigned long *core_clk);
+static int dispc_ovl_calc_scaling_44xx(enum omap_channel channel,
+ const struct omap_video_timings *mgr_timings, u16 width, u16 height,
+ u16 out_width, u16 out_height, enum omap_color_mode color_mode,
+ bool *five_taps, int *x_predecim, int *y_predecim, int *decim_x,
+ int *decim_y, u16 pos_x, unsigned long *core_clk);
+
+static unsigned long calc_core_clk_24xx(enum omap_channel channel, u16
width,
+ u16 height, u16 out_width, u16 out_height);
+static unsigned long calc_core_clk_34xx(enum omap_channel channel, u16
width,
+ u16 height, u16 out_width, u16 out_height);
+static unsigned long calc_core_clk_44xx(enum omap_channel channel, u16
width,
+ u16 height, u16 out_width, u16 out_height);
+
+static bool _dispc_lcd_timings_ok_24xx(int hsw, int hfp, int hbp,
+ int vsw, int vfp, int vbp);
+static bool _dispc_lcd_timings_ok_44xx(int hsw, int hfp, int hbp,
+ int vsw, int vfp, int vbp);
+
+static void _dispc_mgr_set_lcd_timings_hv_24xx(enum omap_channel channel,
+ int hsw, int hfp, int hbp, int vsw, int vfp, int vbp);
+static void _dispc_mgr_set_lcd_timings_hv_44xx(enum omap_channel channel,
+ int hsw, int hfp, int hbp, int vsw, int vfp, int vbp);
While it's nice to have the initialization of struct dispc_features in
the beginning of dispc.c, it requires the above prototypes. And in the
future we may require more. For that reason I think it's better to
initialize the dispc_features at the end of dispc.c, just above
dispc_init_features(). This would be kinda similar to how drivers often
initialize their ops.
Yes, this sounds good, but I was just following general order of
structure and function declarations, structures initializations
followed by functions.
+static const struct dispc_features omap2_dispc_features = {
+ .calc_scaling = dispc_ovl_calc_scaling_24xx,
+ .calc_core_clk = calc_core_clk_24xx,
+ .lcd_timings_ok = _dispc_lcd_timings_ok_24xx,
+ .set_lcd_timings_hv = _dispc_mgr_set_lcd_timings_hv_24xx,
+};
+
+static const struct dispc_features omap3_2_1_dispc_features = {
+ .calc_scaling = dispc_ovl_calc_scaling_34xx,
+ .calc_core_clk = calc_core_clk_34xx,
+ .lcd_timings_ok = _dispc_lcd_timings_ok_24xx,
+ .set_lcd_timings_hv = _dispc_mgr_set_lcd_timings_hv_24xx,
+};
+
+static const struct dispc_features omap3_3_0_dispc_features = {
+ .calc_scaling = dispc_ovl_calc_scaling_34xx,
+ .calc_core_clk = calc_core_clk_34xx,
+ .lcd_timings_ok = _dispc_lcd_timings_ok_44xx,
+ .set_lcd_timings_hv = _dispc_mgr_set_lcd_timings_hv_44xx,
+};
+
+static const struct dispc_features omap4_dispc_features = {
+ .calc_scaling = dispc_ovl_calc_scaling_44xx,
+ .calc_core_clk = calc_core_clk_44xx,
+ .lcd_timings_ok = _dispc_lcd_timings_ok_44xx,
+ .set_lcd_timings_hv = _dispc_mgr_set_lcd_timings_hv_44xx,
+};
During runtime we only require one of these, others can be discarded.
This can be accomplished with the combination of __initdata for these,
and __init
Re: [PATCH 1/6] OMAPDSS: DISPC: cleanup cpu_is_xxxx checks
On Wed, 2012-08-08 at 18:31 +0530, Mahapatra, Chandrabhanu wrote: Yes, this sounds good, but I was just following general order of structure and function declarations, structures initializations followed by functions. Yep, it's a good rule. But there are exceptions when we'll get bloated code by following the rule =). During runtime we only require one of these, others can be discarded. This can be accomplished with the combination of __initdata for these, and __init for dispc_init_features(). The same also applies for all structures in dss_features.c. Just a thought that __init and __initdata should have also been used there. I agree. We should also see what things we can move from dss_features.c into dss.c, dispc.c, etc. Some things in dss_features are quite global, but some are really used only in one place in one file. Although this also makes me wonder, is it better to have all the hardware version information for all DSS modules in one place, as we tried with dss_features, or is it better to have the HW information in the respective DSS submodule, as you're doing with this patch. Both have benefits. Even though with the latter method there's no one place to look for DSS HW feature differences, I think it'll still give us cleaner code. So let's continue on this track. Tomi signature.asc Description: This is a digitally signed message part
