* RE: [PATCH v4] da8xx-fb: add 24bpp LCD configuration support
From: Manjunathappa, Prakash @ 2012-08-13 3:52 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1344440145-27117-1-git-send-email-prakash.pm@ti.com>
Hi Sergei,
Thanks for reviewing the patch.
On Wed, Aug 08, 2012 at 21:44:55, Sergei Shtylyov wrote:
> Hello.
>
> On 08-08-2012 19:35, Manjunathappa, Prakash wrote:
>
> > LCD controller on am335x supports 24bpp raster configuration in addition
> > to ones on da850. LCDC also supports 24bpp in unpacked format having
> > ARGB:8888 32bpp format data in DDR, but it doesn't interpret alpha
> > component of the data.
>
> > Signed-off-by: Manjunathappa, Prakash <prakash.pm@ti.com>
> > Cc: Anatolij Gustschin <agust@denx.de>
> [...]
>
> > diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
> > index 7ae9d53..1abcfa9 100644
> > --- a/drivers/video/da8xx-fb.c
> > +++ b/drivers/video/da8xx-fb.c
> [...]
> > @@ -499,6 +501,9 @@ static int lcd_cfg_frame_buffer(struct da8xx_fb_par *par, u32 width, u32 height,
> > {
> > u32 reg;
> >
> > + if ((bpp > 16) && (lcd_revision = LCD_VERSION_1))
>
> Parens around operands of && not necessary.
>
I agree it is not necessary, isn't it more readable?
> > @@ -542,6 +547,12 @@ static int lcd_cfg_frame_buffer(struct da8xx_fb_par *par, u32 width, u32 height,
> > reg = lcdc_read(LCD_RASTER_CTRL_REG) & ~(1 << 8);
> > if (raster_order)
> > reg |= LCD_RASTER_ORDER;
> > +
> > + if (bpp = 24)
> > + reg |= LCD_V2_TFT_24BPP_MODE;
> > + else if (bpp = 32)
> > + reg |= (LCD_V2_TFT_24BPP_MODE | LCD_V2_TFT_24BPP_UNPACK);
> > +
>
> This asks to be a *switch* statement.
>
I will move it to *switch* statement below.
> > lcdc_write(reg, LCD_RASTER_CTRL_REG);
> >
> > switch (bpp) {
> > @@ -549,6 +560,8 @@ static int lcd_cfg_frame_buffer(struct da8xx_fb_par *par, u32 width, u32 height,
> > case 2:
> > case 4:
> > case 16:
> > + case 24:
> > + case 32:
> > par->palette_sz = 16 * 2;
> > break;
> >
> > @@ -578,13 +591,36 @@ static int fb_setcolreg(unsigned regno, unsigned red, unsigned green,
> > if (info->fix.visual = FB_VISUAL_DIRECTCOLOR)
> > return 1;
> >
> > - if (info->var.bits_per_pixel = 4) {
> > - if (regno > 15)
> > - return 1;
> > + if ((info->var.bits_per_pixel > 16) && (lcd_revision = LCD_VERSION_1))
>
> Parens around operands of && not necessary.
>
Same as above, can I retain it for purpose of readability?
> > + switch (info->fix.visual) {
> > + case FB_VISUAL_TRUECOLOR:
> > + red = CNVT_TOHW(red, info->var.red.length);
> > + green = CNVT_TOHW(green, info->var.green.length);
> > + blue = CNVT_TOHW(blue, info->var.blue.length);
> > + break;
> > + case FB_VISUAL_PSEUDOCOLOR:
> > + if (info->var.bits_per_pixel = 4) {
> > + if (regno > 15)
> > + return -EINVAL;
> > +
> > + if (info->var.grayscale) {
> > + pal = regno;
> > + } else {
> > + red >>= 4;
> > + green >>= 8;
> > + blue >>= 12;
> > +
> > + pal = (red & 0x0f00);
> > + pal |= (green & 0x00f0);
> > + pal |= (blue & 0x000f);
>
> Parens not needed.
>
Will remove parens around.
> > + }
> > + if (regno = 0)
> > + pal |= 0x2000;
> > + palette[regno] = pal;
> > +
> > + } else if (info->var.bits_per_pixel = 8) {
>
> This asks to be a *switch* statement.
>
Agreed, I will replace with *switch* statement.
Thanks,
Prakash
> > @@ -842,6 +877,9 @@ static int fb_check_var(struct fb_var_screeninfo *var,
> > {
> > int err = 0;
[...]
^ permalink raw reply
* RE: [PATCH] video: move CNVT_TOHW to a common place
From: Manjunathappa, Prakash @ 2012-08-13 5:27 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1344426866-8789-1-git-send-email-prakash.pm@ti.com>
SGkgR2VlcnQgVXl0dGVyaG9ldmVuLA0KDQpUaGFua3MgZm9yIHJldmlld2luZyB0aGUgcGF0Y2gu
DQoNCk9uIFdlZCwgQXVnIDA4LCAyMDEyIGF0IDIyOjMyOjUxLCBHZWVydCBVeXR0ZXJob2V2ZW4g
d3JvdGU6DQo+IE9uIFdlZCwgQXVnIDgsIDIwMTIgYXQgMTo1NCBQTSwgTWFuanVuYXRoYXBwYSwg
UHJha2FzaA0KPiA8cHJha2FzaC5wbUB0aS5jb20+IHdyb3RlOg0KPiA+IC0tLSBhL2luY2x1ZGUv
bGludXgvZmIuaA0KPiA+ICsrKyBiL2luY2x1ZGUvbGludXgvZmIuaA0KPiA+IEBAIC0zOTcsNiAr
Mzk3LDExIEBAIHN0cnVjdCBmYl9jdXJzb3Igew0KPiA+ICAgICAgICAgc3RydWN0IGZiX2ltYWdl
IGltYWdlOyAgLyogQ3Vyc29yIGltYWdlICovDQo+ID4gIH07DQo+ID4NCj4gPiArc3RhdGljIGlu
bGluZSBfX3UzMiBDTlZUX1RPSFcoX191MzIgdmFsLCBfX3UzMiB3aWR0aCkNCj4gDQo+ICJfX3Uz
MiIgaXMgYSBiaXQgbWlzbGVhZGluZywgYXMgInZhbCIgaXMgYWN0dWFsbHkgYW4gdW5zaWduZWQg
MTYtYml0IHZhbHVlLg0KPiANCg0KSSBhZ3JlZSwgd2lsbCBjb3JyZWN0IHRoaXMuDQoNCj4gPiAr
ew0KPiA+ICsgICAgICAgcmV0dXJuICgodmFsIDw8IHdpZHRoKSArIDB4N0ZGRiAtIHZhbCkgPj4g
MTY7DQo+ID4gK30NCj4gDQo+IEFsc28sIGlmIHlvdSBtb3ZlIGl0IHRvIGEgcHVibGljIGhlYWRl
ciwgcGVyaGFwcyB0aGUgZnVuY3Rpb24gbmVlZHMgYSBtb3JlDQo+IGRlc2NyaXB0aXZlIG5hbWU/
IFdoYXQgaXQgZG9lcyBpcyByZW1hcCBhIDE2LWJpdCB2YWx1ZSAidmFsIiBvbnRvIGEgdmFsdWUg
d2l0aA0KPiAid2lkdGgiIGJpdHMgb2YgcmVzb2x1dGlvbiwgd2hlcmUgIndpZHRoIiBtdXN0IGJl
IHNtYWxsZXIgdGhhbiAxNi4NCj4gInJlZHVjZV9yZXNvbHV0aW9uKCkiPw0KPiANCg0KSSBhZ3Jl
ZSB0aGF0IGl0IHJlZHVjZXMgdGhlIHJlc29sdXRpb24sIGJ1dCBpdCByZWR1Y2VzIHRvIG1hdGNo
IHRoZSBoYXJkd2FyZS4NClNvICJDTlZUX1RPSFciOiBjb252ZXJ0IHRvIGhhcmR3YXJlIHN1cHBv
cnRlZCByZXNvbHV0aW9uLCBpc24ndCBpdCBtb3JlIGFwcHJvcHJpYXRlPw0KDQpJIHdpbGwgYWRk
IGRlc2NyaXB0aW9uIGFzIGl0IGlzIGdvaW5nIHRvIHB1YmxpYyBoZWFkZXIgZmlsZS4NCg0KPiBB
bHNvLCBpdCBjYW4gYmVjb21lIG1vcmUgZ2VuZXJpYywgbm90IGxpbWl0aW5nIHRvIDE2LWJpdCBp
bnB1dCB2YWx1ZXMuDQo+IFRoZSAiMHg3ZmZmIiBwYXJ0IGlzICIoMSA8PCAod2lkdGggLTEpKSAt
IDEiLiBCdXQgSSdtIG5vdCBzdXJlIGl0J3MNCj4gd29ydGggdGhlIGVmZm9ydA0KPiAoYXJlIHRo
ZXJlIHBsYWNlcyB3aGVyZSB0aGlzIGlzIG5lZWRlZD8pLg0KPiANCg0KRmxvcmlhbiBUb2JpYXMg
U2NoYW5kaW5hdDogQ291bGQgeW91IHBsZWFzZSBjb21tZW50IGhlcmU/DQoNClRoYW5rcywNClBy
YWthc2gNCg0KPiBHcntvZXRqZSxlZXRpbmd9cywNCj4gDQo+ICAgICAgICAgICAgICAgICAgICAg
ICAgIEdlZXJ0DQo+IA0KPiAtLQ0KPiBHZWVydCBVeXR0ZXJob2V2ZW4gLS0gVGhlcmUncyBsb3Rz
IG9mIExpbnV4IGJleW9uZCBpYTMyIC0tIGdlZXJ0QGxpbnV4LW02OGsub3JnDQo+IA0KPiBJbiBw
ZXJzb25hbCBjb252ZXJzYXRpb25zIHdpdGggdGVjaG5pY2FsIHBlb3BsZSwgSSBjYWxsIG15c2Vs
ZiBhIGhhY2tlci4gQnV0DQo+IHdoZW4gSSdtIHRhbGtpbmcgdG8gam91cm5hbGlzdHMgSSBqdXN0
IHNheSAicHJvZ3JhbW1lciIgb3Igc29tZXRoaW5nIGxpa2UgdGhhdC4NCj4gICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAtLSBMaW51cyBUb3J2YWxkcw0KPiANCg0K
^ permalink raw reply
* [PATCH] video/mx3fb: remove stray l from debug output
From: Uwe Kleine-König @ 2012-08-13 7:44 UTC (permalink / raw)
To: linux-fbdev
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
drivers/video/mx3fb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/mx3fb.c b/drivers/video/mx3fb.c
index c89f8a8..1dfdeb1 100644
--- a/drivers/video/mx3fb.c
+++ b/drivers/video/mx3fb.c
@@ -771,7 +771,7 @@ static int __set_par(struct fb_info *fbi, bool lock)
if (fbi->var.sync & FB_SYNC_SHARP_MODE)
mode = IPU_PANEL_SHARP_TFT;
- dev_dbg(fbi->device, "pixclock = %ul Hz\n",
+ dev_dbg(fbi->device, "pixclock = %u Hz\n",
(u32) (PICOS2KHZ(fbi->var.pixclock) * 1000UL));
if (sdc_init_panel(mx3fb, mode,
--
1.7.10.4
^ permalink raw reply related
* [PATCH] video:uvesafb: check the return value of kzalloc
From: Wang YanQing @ 2012-08-13 10:02 UTC (permalink / raw)
To: FlorianSchandinat; +Cc: linux-fbdev, linux-kernel, spock
Michal maybe forgot it merely, we should add code
to check the return value of kzalloc to make the
code more robust.
Signed-off-by: Wang YanQing <udknight@gmail.com>
---
drivers/video/uvesafb.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/video/uvesafb.c b/drivers/video/uvesafb.c
index b0e2a42..2f8f82d 100644
--- a/drivers/video/uvesafb.c
+++ b/drivers/video/uvesafb.c
@@ -659,6 +659,8 @@ static int __devinit uvesafb_vbe_getedid(struct uvesafb_ktask *task,
task->t.flags = TF_BUF_RET | TF_BUF_ESDI;
task->t.buf_len = EDID_LENGTH;
task->buf = kzalloc(EDID_LENGTH, GFP_KERNEL);
+ if (!task->buf)
+ return -ENOMEM;
err = uvesafb_exec(task);
--
1.7.11.1.116.g8228a23
^ permalink raw reply related
* [PATCH 1/6] OMAPDSS: DISPC: cleanup cpu_is_xxxx checks
From: Chandrabhanu Mahapatra @ 2012-08-13 12:10 UTC (permalink / raw)
To: tomi.valkeinen; +Cc: linux-omap, linux-fbdev, Chandrabhanu Mahapatra
In-Reply-To: <1344425874-28222-1-git-send-email-cmahapatra@ti.com>
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.
Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
---
drivers/video/omap2/dss/dispc.c | 428 +++++++++++++++++++++++++--------------
1 file changed, 273 insertions(+), 155 deletions(-)
diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
index 5b289c5..11ca3f9 100644
--- a/drivers/video/omap2/dss/dispc.c
+++ b/drivers/video/omap2/dss/dispc.c
@@ -81,6 +81,23 @@ struct dispc_irq_stats {
unsigned irqs[32];
};
+struct dispc_features {
+ int hp_max;
+ int vp_max;
+ int sw_max;
+ int sw_start;
+ int fp_start;
+ int bp_start;
+ int (*calc_scaling) (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);
+ unsigned long (*calc_core_clk) (enum omap_channel channel,
+ u16 width, u16 height, u16 out_width, u16 out_height);
+};
+
static struct {
struct platform_device *pdev;
void __iomem *base;
@@ -101,6 +118,8 @@ static struct {
bool ctx_valid;
u32 ctx[DISPC_SZ_REGS / sizeof(u32)];
+ const struct dispc_features *feat;
+
#ifdef CONFIG_OMAP2_DSS_COLLECT_IRQ_STATS
spinlock_t irq_stats_lock;
struct dispc_irq_stats irq_stats;
@@ -1939,7 +1958,18 @@ static unsigned long calc_core_clk_five_taps(enum omap_channel channel,
return core_clk;
}
-static unsigned long calc_core_clk(enum omap_channel channel, u16 width,
+static unsigned long calc_core_clk_24xx(enum omap_channel channel, u16 width,
+ u16 height, u16 out_width, u16 out_height)
+{
+ unsigned long pclk = dispc_mgr_pclk_rate(channel);
+
+ if (height > out_height && width > out_width)
+ return pclk * 4;
+ else
+ return pclk * 2;
+}
+
+static unsigned long calc_core_clk_34xx(enum omap_channel channel, u16 width,
u16 height, u16 out_width, u16 out_height)
{
unsigned int hf, vf;
@@ -1958,25 +1988,163 @@ static unsigned long calc_core_clk(enum omap_channel channel, u16 width,
hf = 2;
else
hf = 1;
-
if (height > out_height)
vf = 2;
else
vf = 1;
- if (cpu_is_omap24xx()) {
- if (vf > 1 && hf > 1)
- return pclk * 4;
- else
- return pclk * 2;
- } else if (cpu_is_omap34xx()) {
- return pclk * vf * hf;
- } else {
- if (hf > 1)
- return DIV_ROUND_UP(pclk, out_width) * width;
- else
- return pclk;
+ return pclk * vf * hf;
+}
+
+static unsigned long calc_core_clk_44xx(enum omap_channel channel, u16 width,
+ u16 height, u16 out_width, u16 out_height)
+{
+ unsigned long pclk = dispc_mgr_pclk_rate(channel);
+
+ if (width > out_width)
+ return DIV_ROUND_UP(pclk, out_width) * width;
+ else
+ return pclk;
+}
+
+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)
+{
+ int error;
+ u16 in_width, in_height;
+ int min_factor = min(*decim_x, *decim_y);
+ const int maxsinglelinewidth + dss_feat_get_param_max(FEAT_PARAM_LINEWIDTH);
+ *five_taps = false;
+
+ do {
+ in_height = DIV_ROUND_UP(height, *decim_y);
+ in_width = DIV_ROUND_UP(width, *decim_x);
+ *core_clk = dispc.feat->calc_core_clk(channel, in_width,
+ in_height, out_width, out_height);
+ error = (in_width > maxsinglelinewidth || !*core_clk ||
+ *core_clk > dispc_core_clk_rate());
+ if (error) {
+ if (*decim_x = *decim_y) {
+ *decim_x = min_factor;
+ ++*decim_y;
+ } else {
+ swap(*decim_x, *decim_y);
+ if (*decim_x < *decim_y)
+ ++*decim_x;
+ }
+ }
+ } while (*decim_x <= *x_predecim && *decim_y <= *y_predecim && error);
+
+ if (in_width > maxsinglelinewidth) {
+ DSSERR("Cannot scale max input width exceeded");
+ return -EINVAL;
+ }
+ return 0;
+}
+
+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)
+{
+ int error;
+ u16 in_width, in_height;
+ int min_factor = min(*decim_x, *decim_y);
+ const int maxsinglelinewidth + dss_feat_get_param_max(FEAT_PARAM_LINEWIDTH);
+
+ do {
+ in_height = DIV_ROUND_UP(height, *decim_y);
+ in_width = DIV_ROUND_UP(width, *decim_x);
+ *core_clk = calc_core_clk_five_taps(channel, mgr_timings,
+ in_width, in_height, out_width, out_height, color_mode);
+
+ error = check_horiz_timing_omap3(channel, mgr_timings, pos_x,
+ in_width, in_height, out_width, out_height);
+
+ if (in_width > maxsinglelinewidth)
+ if (in_height > out_height &&
+ in_height < out_height * 2)
+ *five_taps = false;
+ if (!*five_taps)
+ *core_clk = dispc.feat->calc_core_clk(channel, in_width,
+ in_height, out_width, out_height);
+
+ error = (error || in_width > maxsinglelinewidth * 2 ||
+ (in_width > maxsinglelinewidth && *five_taps) ||
+ !*core_clk || *core_clk > dispc_core_clk_rate());
+ if (error) {
+ if (*decim_x = *decim_y) {
+ *decim_x = min_factor;
+ ++*decim_y;
+ } else {
+ swap(*decim_x, *decim_y);
+ if (*decim_x < *decim_y)
+ ++*decim_x;
+ }
+ }
+ } while (*decim_x <= *x_predecim && *decim_y <= *y_predecim && error);
+
+ if (check_horiz_timing_omap3(channel, mgr_timings, pos_x, width, height,
+ out_width, out_height)){
+ DSSERR("horizontal timing too tight\n");
+ return -EINVAL;
}
+
+ if (in_width > (maxsinglelinewidth * 2)) {
+ DSSERR("Cannot setup scaling");
+ DSSERR("width exceeds maximum width possible");
+ return -EINVAL;
+ }
+
+ if (in_width > maxsinglelinewidth && *five_taps) {
+ DSSERR("cannot setup scaling with five taps");
+ return -EINVAL;
+ }
+ return 0;
+}
+
+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)
+{
+ u16 in_width, in_width_max;
+ int decim_x_min = *decim_x;
+ u16 in_height = DIV_ROUND_UP(height, *decim_y);
+ const int maxsinglelinewidth + dss_feat_get_param_max(FEAT_PARAM_LINEWIDTH);
+
+ in_width_max = dispc_core_clk_rate() /
+ DIV_ROUND_UP(dispc_mgr_pclk_rate(channel), out_width);
+ *decim_x = DIV_ROUND_UP(width, in_width_max);
+
+ *decim_x = *decim_x > decim_x_min ? *decim_x : decim_x_min;
+ if (*decim_x > *x_predecim)
+ return -EINVAL;
+
+ do {
+ in_width = DIV_ROUND_UP(width, *decim_x);
+ } while (*decim_x <= *x_predecim &&
+ in_width > maxsinglelinewidth && ++*decim_x);
+
+ if (in_width > maxsinglelinewidth) {
+ DSSERR("Cannot scale width exceeds max line width");
+ return -EINVAL;
+ }
+
+ *core_clk = dispc.feat->calc_core_clk(channel, in_width, in_height,
+ out_width, out_height);
+ return 0;
}
static int dispc_ovl_calc_scaling(enum omap_plane plane,
@@ -1988,12 +2156,9 @@ static int dispc_ovl_calc_scaling(enum omap_plane plane,
{
struct omap_overlay *ovl = omap_dss_get_overlay(plane);
const int maxdownscale = dss_feat_get_param_max(FEAT_PARAM_DOWNSCALE);
- const int maxsinglelinewidth - dss_feat_get_param_max(FEAT_PARAM_LINEWIDTH);
const int max_decim_limit = 16;
unsigned long core_clk = 0;
- int decim_x, decim_y, error, min_factor;
- u16 in_width, in_height, in_width_max = 0;
+ int decim_x, decim_y, ret;
if (width = out_width && height = out_height)
return 0;
@@ -2017,118 +2182,17 @@ static int dispc_ovl_calc_scaling(enum omap_plane plane,
decim_x = DIV_ROUND_UP(DIV_ROUND_UP(width, out_width), maxdownscale);
decim_y = DIV_ROUND_UP(DIV_ROUND_UP(height, out_height), maxdownscale);
- min_factor = min(decim_x, decim_y);
-
if (decim_x > *x_predecim || out_width > width * 8)
return -EINVAL;
if (decim_y > *y_predecim || out_height > height * 8)
return -EINVAL;
- if (cpu_is_omap24xx()) {
- *five_taps = false;
-
- do {
- in_height = DIV_ROUND_UP(height, decim_y);
- in_width = DIV_ROUND_UP(width, decim_x);
- core_clk = calc_core_clk(channel, in_width, in_height,
- out_width, out_height);
- error = (in_width > maxsinglelinewidth || !core_clk ||
- core_clk > dispc_core_clk_rate());
- if (error) {
- if (decim_x = decim_y) {
- decim_x = min_factor;
- decim_y++;
- } else {
- swap(decim_x, decim_y);
- if (decim_x < decim_y)
- decim_x++;
- }
- }
- } while (decim_x <= *x_predecim && decim_y <= *y_predecim &&
- error);
-
- if (in_width > maxsinglelinewidth) {
- DSSERR("Cannot scale max input width exceeded");
- return -EINVAL;
- }
- } else if (cpu_is_omap34xx()) {
-
- do {
- in_height = DIV_ROUND_UP(height, decim_y);
- in_width = DIV_ROUND_UP(width, decim_x);
- core_clk = calc_core_clk_five_taps(channel, mgr_timings,
- in_width, in_height, out_width, out_height,
- color_mode);
-
- error = check_horiz_timing_omap3(channel, mgr_timings,
- pos_x, in_width, in_height, out_width,
- out_height);
-
- if (in_width > maxsinglelinewidth)
- if (in_height > out_height &&
- in_height < out_height * 2)
- *five_taps = false;
- if (!*five_taps)
- core_clk = calc_core_clk(channel, in_width,
- in_height, out_width, out_height);
- error = (error || in_width > maxsinglelinewidth * 2 ||
- (in_width > maxsinglelinewidth && *five_taps) ||
- !core_clk || core_clk > dispc_core_clk_rate());
- if (error) {
- if (decim_x = decim_y) {
- decim_x = min_factor;
- decim_y++;
- } else {
- swap(decim_x, decim_y);
- if (decim_x < decim_y)
- decim_x++;
- }
- }
- } while (decim_x <= *x_predecim && decim_y <= *y_predecim
- && error);
-
- if (check_horiz_timing_omap3(channel, mgr_timings, pos_x, width,
- height, out_width, out_height)){
- DSSERR("horizontal timing too tight\n");
- return -EINVAL;
- }
-
- if (in_width > (maxsinglelinewidth * 2)) {
- DSSERR("Cannot setup scaling");
- DSSERR("width exceeds maximum width possible");
- return -EINVAL;
- }
-
- if (in_width > maxsinglelinewidth && *five_taps) {
- DSSERR("cannot setup scaling with five taps");
- return -EINVAL;
- }
- } else {
- int decim_x_min = decim_x;
- in_height = DIV_ROUND_UP(height, decim_y);
- in_width_max = dispc_core_clk_rate() /
- DIV_ROUND_UP(dispc_mgr_pclk_rate(channel),
- out_width);
- decim_x = DIV_ROUND_UP(width, in_width_max);
-
- decim_x = decim_x > decim_x_min ? decim_x : decim_x_min;
- if (decim_x > *x_predecim)
- return -EINVAL;
-
- do {
- in_width = DIV_ROUND_UP(width, decim_x);
- } while (decim_x <= *x_predecim &&
- in_width > maxsinglelinewidth && decim_x++);
-
- if (in_width > maxsinglelinewidth) {
- DSSERR("Cannot scale width exceeds max line width");
- return -EINVAL;
- }
-
- core_clk = calc_core_clk(channel, in_width, in_height,
- out_width, out_height);
- }
+ ret = dispc.feat->calc_scaling(channel, mgr_timings, width, height,
+ out_width, out_height, color_mode, five_taps, x_predecim,
+ y_predecim, &decim_x, &decim_y, pos_x, &core_clk);
+ if (ret)
+ return ret;
DSSDBG("required core clk rate = %lu Hz\n", core_clk);
DSSDBG("current core clk rate = %lu Hz\n", dispc_core_clk_rate());
@@ -2604,24 +2668,13 @@ static bool _dispc_mgr_size_ok(u16 width, u16 height)
static bool _dispc_lcd_timings_ok(int hsw, int hfp, int hbp,
int vsw, int vfp, int vbp)
{
- if (cpu_is_omap24xx() || omap_rev() < OMAP3430_REV_ES3_0) {
- if (hsw < 1 || hsw > 64 ||
- hfp < 1 || hfp > 256 ||
- hbp < 1 || hbp > 256 ||
- vsw < 1 || vsw > 64 ||
- vfp < 0 || vfp > 255 ||
- vbp < 0 || vbp > 255)
- return false;
- } else {
- if (hsw < 1 || hsw > 256 ||
- hfp < 1 || hfp > 4096 ||
- hbp < 1 || hbp > 4096 ||
- vsw < 1 || vsw > 256 ||
- vfp < 0 || vfp > 4095 ||
- vbp < 0 || vbp > 4095)
- return false;
- }
-
+ if (hsw < 1 || hsw > dispc.feat->sw_max ||
+ hfp < 1 || hfp > dispc.feat->hp_max ||
+ hbp < 1 || hbp > dispc.feat->hp_max ||
+ vsw < 1 || vsw > dispc.feat->sw_max ||
+ vfp < 0 || vfp > dispc.feat->vp_max ||
+ vbp < 0 || vbp > dispc.feat->vp_max)
+ return false;
return true;
}
@@ -2653,19 +2706,12 @@ static void _dispc_mgr_set_lcd_timings(enum omap_channel channel, int hsw,
u32 timing_h, timing_v, l;
bool onoff, rf, ipc;
- if (cpu_is_omap24xx() || omap_rev() < OMAP3430_REV_ES3_0) {
- timing_h = FLD_VAL(hsw-1, 5, 0) | FLD_VAL(hfp-1, 15, 8) |
- FLD_VAL(hbp-1, 27, 20);
-
- timing_v = FLD_VAL(vsw-1, 5, 0) | FLD_VAL(vfp, 15, 8) |
- FLD_VAL(vbp, 27, 20);
- } else {
- timing_h = FLD_VAL(hsw-1, 7, 0) | FLD_VAL(hfp-1, 19, 8) |
- FLD_VAL(hbp-1, 31, 20);
-
- timing_v = FLD_VAL(vsw-1, 7, 0) | FLD_VAL(vfp, 19, 8) |
- FLD_VAL(vbp, 31, 20);
- }
+ timing_h = FLD_VAL(hsw-1, dispc.feat->sw_start, 0) |
+ FLD_VAL(hfp-1, dispc.feat->fp_start, 8) |
+ FLD_VAL(hbp-1, dispc.feat->bp_start, 20);
+ timing_v = FLD_VAL(vsw-1, dispc.feat->sw_start, 0) |
+ FLD_VAL(vfp, dispc.feat->fp_start, 8) |
+ FLD_VAL(vbp, dispc.feat->bp_start, 20);
dispc_write_reg(DISPC_TIMING_H(channel), timing_h);
dispc_write_reg(DISPC_TIMING_V(channel), timing_v);
@@ -3671,6 +3717,72 @@ static void _omap_dispc_initial_config(void)
dispc_ovl_enable_zorder_planes();
}
+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,
+};
+
+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;
+}
+
/* DISPC HW IP initialisation */
static int __init omap_dispchw_probe(struct platform_device *pdev)
{
@@ -3725,6 +3837,10 @@ static int __init omap_dispchw_probe(struct platform_device *pdev)
dispc.dss_clk = clk;
+ r = dispc_init_features(&dispc.pdev->dev);
+ if (r)
+ return r;
+
pm_runtime_enable(&pdev->dev);
r = dispc_runtime_get();
@@ -3760,6 +3876,8 @@ static int __exit omap_dispchw_remove(struct platform_device *pdev)
clk_put(dispc.dss_clk);
+ devm_kfree(&dispc.pdev->dev, (void *)dispc.feat);
+
return 0;
}
--
1.7.10
^ permalink raw reply related
* [PATCH 3/6] OMAPDSS: DSS: Cleanup cpu_is_xxxx checks
From: Chandrabhanu Mahapatra @ 2012-08-13 12:11 UTC (permalink / raw)
To: tomi.valkeinen; +Cc: linux-omap, linux-fbdev, Chandrabhanu Mahapatra
In-Reply-To: <1344425899-28267-1-git-send-email-cmahapatra@ti.com>
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.
Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
---
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>
#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);
+};
+
static struct {
struct platform_device *pdev;
void __iomem *base;
@@ -83,6 +91,8 @@ static struct {
bool ctx_valid;
u32 ctx[DSS_SZ_REGS / sizeof(u32)];
+
+ const struct dss_features *feat;
} dss;
static const char * const dss_generic_clk_source_names[] = {
@@ -91,6 +101,30 @@ static const char * const dss_generic_clk_source_names[] = {
[OMAP_DSS_CLK_SRC_FCK] = "DSS_FCK",
};
+static const struct __initdata dss_features omap2_dss_features = {
+ .fck_div_max = 16,
+ .factor = 2,
+ .clk_name = NULL,
+};
+
+static const struct __initdata dss_features omap34_dss_features = {
+ .fck_div_max = 16,
+ .factor = 2,
+ .clk_name = "dpll4_m4_ck",
+};
+
+static const struct __initdata dss_features omap36_dss_features = {
+ .fck_div_max = 32,
+ .factor = 1,
+ .clk_name = "dpll4_m4_ck",
+};
+
+static const struct __initdata dss_features omap4_dss_features = {
+ .fck_div_max = 32,
+ .factor = 1,
+ .clk_name = "dpll_per_m5x2_ck",
+};
+
static inline void dss_write_reg(const struct dss_reg idx, u32 val)
{
__raw_writel(val, dss.base + idx.idx);
@@ -236,7 +270,6 @@ const char *dss_get_generic_clk_source_name(enum omap_dss_clk_source clk_src)
return dss_generic_clk_source_names[clk_src];
}
-
void dss_dump_clocks(struct seq_file *s)
{
unsigned long dpll4_ck_rate;
@@ -259,18 +292,10 @@ void dss_dump_clocks(struct seq_file *s)
seq_printf(s, "dpll4_ck %lu\n", dpll4_ck_rate);
- if (cpu_is_omap3630() || cpu_is_omap44xx())
- seq_printf(s, "%s (%s) = %lu / %lu = %lu\n",
- fclk_name, fclk_real_name,
- dpll4_ck_rate,
- dpll4_ck_rate / dpll4_m4_ck_rate,
- fclk_rate);
- else
- seq_printf(s, "%s (%s) = %lu / %lu * 2 = %lu\n",
- fclk_name, fclk_real_name,
- dpll4_ck_rate,
- dpll4_ck_rate / dpll4_m4_ck_rate,
- fclk_rate);
+ seq_printf(s, "%s (%s) = %lu / %lu * %d = %lu\n",
+ fclk_name, fclk_real_name, dpll4_ck_rate,
+ dpll4_ck_rate / dpll4_m4_ck_rate,
+ dss.feat->factor, fclk_rate);
} else {
seq_printf(s, "%s (%s) = %lu\n",
fclk_name, fclk_real_name,
@@ -470,7 +495,7 @@ int dss_calc_clock_div(unsigned long req_pck, struct dss_clock_info *dss_cinfo,
unsigned long fck, max_dss_fck;
- u16 fck_div, fck_div_max = 16;
+ u16 fck_div;
int match = 0;
int min_fck_per_pck;
@@ -480,9 +505,8 @@ int dss_calc_clock_div(unsigned long req_pck, struct dss_clock_info *dss_cinfo,
max_dss_fck = dss_feat_get_param_max(FEAT_PARAM_DSS_FCK);
fck = clk_get_rate(dss.dss_clk);
- if (req_pck = dss.cache_req_pck &&
- ((cpu_is_omap34xx() && prate = dss.cache_prate) ||
- dss.cache_dss_cinfo.fck = fck)) {
+ if (req_pck = dss.cache_req_pck && prate = dss.cache_prate &&
+ dss.cache_dss_cinfo.fck = fck) {
DSSDBG("dispc clock info found from cache.\n");
*dss_cinfo = dss.cache_dss_cinfo;
*dispc_cinfo = dss.cache_dispc_cinfo;
@@ -519,16 +543,10 @@ retry:
goto found;
} else {
- if (cpu_is_omap3630() || cpu_is_omap44xx())
- fck_div_max = 32;
-
- for (fck_div = fck_div_max; fck_div > 0; --fck_div) {
+ for (fck_div = dss.feat->fck_div_max; fck_div > 0; --fck_div) {
struct dispc_clock_info cur_dispc;
- if (fck_div_max = 32)
- fck = prate / fck_div;
- else
- fck = prate / fck_div * 2;
+ fck = prate / fck_div * dss.feat->factor;
if (fck > max_dss_fck)
continue;
@@ -633,22 +651,11 @@ static int dss_get_clocks(void)
dss.dss_clk = clk;
- if (cpu_is_omap34xx()) {
- clk = clk_get(NULL, "dpll4_m4_ck");
- if (IS_ERR(clk)) {
- DSSERR("Failed to get dpll4_m4_ck\n");
- r = PTR_ERR(clk);
- goto err;
- }
- } else if (cpu_is_omap44xx()) {
- clk = clk_get(NULL, "dpll_per_m5x2_ck");
- if (IS_ERR(clk)) {
- DSSERR("Failed to get dpll_per_m5x2_ck\n");
- r = PTR_ERR(clk);
- goto err;
- }
- } else { /* omap24xx */
- clk = NULL;
+ clk = clk_get(NULL, dss.feat->clk_name);
+ if (IS_ERR(clk)) {
+ DSSERR("Failed to get %s\n", dss.feat->clk_name);
+ r = PTR_ERR(clk);
+ goto err;
}
dss.dpll4_m4_ck = clk;
@@ -704,6 +711,26 @@ void dss_debug_dump_clocks(struct seq_file *s)
}
#endif
+static int __init dss_init_features(struct device *dev)
+{
+ dss.feat = devm_kzalloc(dev, sizeof(*dss.feat), GFP_KERNEL);
+ if (!dss.feat) {
+ dev_err(dev, "Failed to allocate local DSS Features\n");
+ return -ENOMEM;
+ }
+
+ 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;
+
+ return 0;
+}
+
/* DSS HW IP initialisation */
static int __init omap_dsshw_probe(struct platform_device *pdev)
{
@@ -750,6 +777,10 @@ static int __init omap_dsshw_probe(struct platform_device *pdev)
dss.lcd_clk_source[0] = OMAP_DSS_CLK_SRC_FCK;
dss.lcd_clk_source[1] = OMAP_DSS_CLK_SRC_FCK;
+ r = dss_init_features(&dss.pdev->dev);
+ if (r)
+ return r;
+
rev = dss_read_reg(DSS_REVISION);
printk(KERN_INFO "OMAP DSS rev %d.%d\n",
FLD_GET(rev, 7, 4), FLD_GET(rev, 3, 0));
@@ -772,6 +803,8 @@ static int __exit omap_dsshw_remove(struct platform_device *pdev)
dss_put_clocks();
+ devm_kfree(&dss.pdev->dev, (void *)dss.feat);
+
return 0;
}
--
1.7.10
^ permalink raw reply related
* re: s3fb: fix up DDC build with MTRR disabled.
From: Dan Carpenter @ 2012-08-13 16:39 UTC (permalink / raw)
To: linux-fbdev
[Sorry this is reall old. I don't think I've emailed about it before]
Hello Paul Mundt,
This is a semi-automatic email about new static checker warnings.
The patch 4f2970b9f622: "s3fb: fix up DDC build with MTRR disabled."
from Apr 20, 2011, leads to the following Smatch complaint:
drivers/video/s3fb.c:1411 s3_pci_remove()
warn: variable dereferenced before check 'info' (see line 1409)
drivers/video/s3fb.c
1408 struct fb_info *info = pci_get_drvdata(dev);
1409 struct s3fb_info __maybe_unused *par = info->par;
^^^^^^^^^
New dereference.
1410
1411 if (info) {
^^^^
Old check.
1412
1413 #ifdef CONFIG_MTRR
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH] video/mx3fb: remove stray l from debug output
From: Geert Uytterhoeven @ 2012-08-13 17:09 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1344843875-8174-1-git-send-email-u.kleine-koenig@pengutronix.de>
On Mon, Aug 13, 2012 at 9:44 AM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> drivers/video/mx3fb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/video/mx3fb.c b/drivers/video/mx3fb.c
> index c89f8a8..1dfdeb1 100644
> --- a/drivers/video/mx3fb.c
> +++ b/drivers/video/mx3fb.c
> @@ -771,7 +771,7 @@ static int __set_par(struct fb_info *fbi, bool lock)
> if (fbi->var.sync & FB_SYNC_SHARP_MODE)
> mode = IPU_PANEL_SHARP_TFT;
>
> - dev_dbg(fbi->device, "pixclock = %ul Hz\n",
> + dev_dbg(fbi->device, "pixclock = %u Hz\n",
> (u32) (PICOS2KHZ(fbi->var.pixclock) * 1000UL));
I think a better fix is to change the dyslectic "%ul" to "%lu", and
drop the cast to
u32, which was probably only added to kill the compiler warning caused by the
dyslectia issue.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: 转发: Siliconmotion new kernel driver initial patch
From: Konrad Rzeszutek Wilk @ 2012-08-13 18:03 UTC (permalink / raw)
To: Aaron.Chen 陈俊杰
Cc: linux-fbdev, mill.chen 陈军,
Mandy.Wang 王少媚, dri-devel,
Paul.Chen 陈波, caesar.qiu 裘赛海,
Sunny.Yang 杨光
In-Reply-To: <4CE6A5494DEECD498EBCD4C5488B1AFCD04CD1@CNDC08.cn.smi.ad>
On Mon, Aug 13, 2012 at 04:56:33PM +0800, Aaron.Chen 陈俊杰 wrote:
>
> Since there is no response for the last mail. Maybe it didn't sent successfully. So I send it again. Here is the initial patch for siliconmotion kernel driver.
What's with the #ifdef 0 or #ifdef 1?
Why is there a bunch of ddkxxx something? Can those header files
be squashed together?
>
> Aaron
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH 0/7] HID: picoLCD updates
From: Tejun Heo @ 2012-08-13 23:27 UTC (permalink / raw)
To: Bruno Prémont; +Cc: linux-input, linux-kernel, Jiri Kosina, linux-fbdev
In-Reply-To: <20120809200947.2194e89f@neptune.home>
Hello,
On Thu, Aug 09, 2012 at 08:09:47PM +0200, Bruno Prémont wrote:
> As you are working on workqueues and related code, could you have a look
> at my usage of them in combination with db_defio?
>
> The delayed memory corruptions or system reboots after unbinding/unplugging
> the PicoLCD seem very much related to workqueue used to handle the deferred
> IO to framebuffer.
>
> I think things don't get cleaned-up as they should though I'm not sure
> where the trouble lies.
>
> For ease of reading, I'm inlineing below the framebuffer related code of
> PicoLCD (for complete driver after this patch series apply the whole series
> on top of 3.5 https://lkml.org/lkml/2012/7/30/375 )
...
> void picolcd_exit_framebuffer(struct picolcd_data *data)
> {
> struct fb_info *info = data->fb_info;
> u8 *fb_vbitmap = data->fb_vbitmap;
>
> if (!info)
> return;
>
> device_remove_file(&data->hdev->dev, &dev_attr_fb_update_rate);
> info->par = NULL;
> unregister_framebuffer(info);
> data->fb_vbitmap = NULL;
> data->fb_bitmap = NULL;
> data->fb_bpp = 0;
> data->fb_info = NULL;
> kfree(fb_vbitmap);
> }
I'm kinda shooting in the dark but who flushes / cancels
fb_info->deferred_work?
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH] video: exynos-mipi-dsi: Add missing static storage class specifiers
From: Donghwa Lee @ 2012-08-14 5:25 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1343888051-406-1-git-send-email-sachin.kamat@linaro.org>
This patch looks good to me.
Acked-by: Donghwa Lee <dh09.lee@samsung.com>
On Thursday, August 02, 2012³â 15:14, Sachin Kamat wrote:
> Fixes the following sparse warnings:
> drivers/video/exynos/exynos_mipi_dsi.c:208:22: warning:
> symbol 'exynos_mipi_dsi_find_lcd_device' was not declared. Should it be static?
> drivers/video/exynos/exynos_mipi_dsi.c:268:22: warning:
> symbol 'exynos_mipi_dsi_bind_lcd_ddi' was not declared. Should it be static?
>
> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> ---
> drivers/video/exynos/exynos_mipi_dsi.c | 6 ++++--
> 1 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/exynos/exynos_mipi_dsi.c b/drivers/video/exynos/exynos_mipi_dsi.c
> index 4bc2b8a..ef68228 100644
> --- a/drivers/video/exynos/exynos_mipi_dsi.c
> +++ b/drivers/video/exynos/exynos_mipi_dsi.c
> @@ -205,7 +205,8 @@ int exynos_mipi_dsi_register_lcd_device(struct mipi_dsim_lcd_device *lcd_dev)
> return 0;
> }
>
> -struct mipi_dsim_ddi *exynos_mipi_dsi_find_lcd_device(struct mipi_dsim_lcd_driver *lcd_drv)
> +static struct mipi_dsim_ddi *exynos_mipi_dsi_find_lcd_device(
> + struct mipi_dsim_lcd_driver *lcd_drv)
> {
> struct mipi_dsim_ddi *dsim_ddi, *next;
> struct mipi_dsim_lcd_device *lcd_dev;
> @@ -265,7 +266,8 @@ int exynos_mipi_dsi_register_lcd_driver(struct mipi_dsim_lcd_driver *lcd_drv)
>
> }
>
> -struct mipi_dsim_ddi *exynos_mipi_dsi_bind_lcd_ddi(struct mipi_dsim_device *dsim,
> +static struct mipi_dsim_ddi *exynos_mipi_dsi_bind_lcd_ddi(
> + struct mipi_dsim_device *dsim,
> const char *name)
> {
> struct mipi_dsim_ddi *dsim_ddi, *next;
^ permalink raw reply
* Re: [PATCH 0/7] HID: picoLCD updates
From: Bruno Prémont @ 2012-08-14 6:30 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-input, linux-kernel, Jiri Kosina, linux-fbdev
In-Reply-To: <20120813232708.GF25632@google.com>
Hello Tejun,
[Tejun: sorry for duplicate, did hit "reply" instead of "reply to all"]
On Mon, 13 Aug 2012 16:27:08 Tejun Heo <tj@kernel.org> wrote:
> On Thu, Aug 09, 2012 at 08:09:47PM +0200, Bruno Prémont wrote:
> > As you are working on workqueues and related code, could you have a look
> > at my usage of them in combination with db_defio?
> >
> > The delayed memory corruptions or system reboots after unbinding/unplugging
> > the PicoLCD seem very much related to workqueue used to handle the deferred
> > IO to framebuffer.
> >
> > I think things don't get cleaned-up as they should though I'm not sure
> > where the trouble lies.
> >
> > For ease of reading, I'm inlineing below the framebuffer related code of
> > PicoLCD (for complete driver after this patch series apply the whole series
> > on top of 3.5 https://lkml.org/lkml/2012/7/30/375 )
> ...
> > void picolcd_exit_framebuffer(struct picolcd_data *data)
> > {
> > struct fb_info *info = data->fb_info;
> > u8 *fb_vbitmap = data->fb_vbitmap;
> >
> > if (!info)
> > return;
> >
> > device_remove_file(&data->hdev->dev, &dev_attr_fb_update_rate);
> > info->par = NULL;
> > unregister_framebuffer(info);
> > data->fb_vbitmap = NULL;
> > data->fb_bitmap = NULL;
> > data->fb_bpp = 0;
> > data->fb_info = NULL;
> > kfree(fb_vbitmap);
> > }
>
> I'm kinda shooting in the dark but who flushes / cancels
> fb_info->deferred_work?
fb_deferred_io_cleanup() does so and is called by destroy fbops
(when last reference to fb_info is returned):
static void picolcd_fb_destroy(struct fb_info *info)
{
/* make sure no work is deferred */
fb_deferred_io_cleanup(info);
vfree((u8 *)info->fix.smem_start);
framebuffer_release(info);
printk(KERN_DEBUG "picolcd_fb_destroy(%p)\n", info);
}
=== drivers/video/fb_defio.c ==void fb_deferred_io_cleanup(struct fb_info *info)
{
struct fb_deferred_io *fbdefio = info->fbdefio;
struct page *page;
int i;
BUG_ON(!fbdefio);
cancel_delayed_work_sync(&info->deferred_work);
/* clear out the mapping that we setup */
for (i = 0 ; i < info->fix.smem_len; i += PAGE_SIZE) {
page = fb_deferred_io_page(info, i);
page->mapping = NULL;
}
info->fbops->fb_mmap = NULL;
mutex_destroy(&fbdefio->lock);
}
Thanks,
Bruno
^ permalink raw reply
* RE: [PATCH v3] video: da8xx-fb: add 24bpp LCD configuration support
From: Manjunathappa, Prakash @ 2012-08-14 6:31 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1342712266-4381-1-git-send-email-prakash.pm@ti.com>
Hi,
On Mon, Jul 23, 2012 at 13:56:42, Manjunathappa, Prakash wrote:
> Hi,
>
> On Thu, Jul 19, 2012 at 21:07:46, Manjunathappa, Prakash wrote:
> > LCD controller on am335x supports 24bpp raster configuration in addition
> > to ones on da850. LCDC also supports 24bpp in unpacked format having
> > ARGB:8888 32bpp format data in DDR, but it doesn't interpret alpha
> > component of the data.
> >
> > Signed-off-by: Manjunathappa, Prakash <prakash.pm@ti.com>
> > Cc: Anatolij Gustschin <agust@denx.de>
> > ---
[...]
> > @@ -546,6 +559,8 @@ static int lcd_cfg_frame_buffer(struct da8xx_fb_par *par, u32 width, u32 height,
> > return 0;
> > }
> >
> > +
> > +#define CNVT_TOHW(val, width) ((((val)<<(width))+0x7FFF-(val))>>16)
>
> since multiple FB drivers have re-defined this macro, I will move this to common place(linux/fb.h) and
> convert it as inline function.
>
This macro has lot of variations in various hardware and as I do not have bigger picture to address them
all, so for now I will be dropping the attempt of moving it to common place. This will be taken up later
point of time.
Thanks,
Prakash
>
> > static int fb_setcolreg(unsigned regno, unsigned red, unsigned green,
> [...]
>
^ permalink raw reply
* [PATCH RESEND] video/udlfb: fix line counting in fb_write
From: Alexander Holler @ 2012-08-14 7:11 UTC (permalink / raw)
To: linux-kernel; +Cc: Bernie Thompson, Alexander Holler, linux-fbdev
Line 0 and 1 were both written to line 0 (on the display) and all subsequent
lines had an offset of -1. The result was that the last line on the display
was never overwritten by writes to /dev/fbN.
Cc: stable@vger.kernel.org
Signed-off-by: Alexander Holler <holler@ahsoftware.de>
---
drivers/video/udlfb.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/video/udlfb.c b/drivers/video/udlfb.c
index a159b63..85d8110 100644
--- a/drivers/video/udlfb.c
+++ b/drivers/video/udlfb.c
@@ -647,7 +647,7 @@ static ssize_t dlfb_ops_write(struct fb_info *info,
const char __user *buf,
result = fb_sys_write(info, buf, count, ppos);
if (result > 0) {
- int start = max((int)(offset / info->fix.line_length) - 1, 0);
+ int start = max((int)(offset / info->fix.line_length), 0);
int lines = min((u32)((result / info->fix.line_length) + 1),
(u32)info->var.yres);
-- 1.7.6.5
^ permalink raw reply related
* Re: [PATCH 03/11] fblog: new framebuffer kernel log dummy driver
From: David Herrmann @ 2012-08-14 9:42 UTC (permalink / raw)
To: Ryan Mallon
Cc: linux-fbdev, Florian Tobias Schandinat, Greg Kroah-Hartman,
linux-serial, Alan Cox, linux-kernel, Geert Uytterhoeven
In-Reply-To: <50283D7A.4090309@gmail.com>
Hi Ryan
On Mon, Aug 13, 2012 at 1:34 AM, Ryan Mallon <rmallon@gmail.com> wrote:
> > config VGA_CONSOLE
> > bool "VGA text console" if EXPERT || !X86
> > - depends on !4xx && !8xx && !SPARC && !M68K && !PARISC && !FRV &&
> > !SUPERH && !BLACKFIN && !AVR32 && !MN10300 && (!ARM || ARCH_FOOTBRIDGE ||
> > ARCH_INTEGRATOR || ARCH_NETWINDER)
> > + depends on VT && !4xx && !8xx && !SPARC && !M68K && !PARISC &&
> > !FRV && !SUPERH && !BLACKFIN && !AVR32 && !MN10300 && (!ARM ||
> > ARCH_FOOTBRIDGE || ARCH_INTEGRATOR || ARCH_NETWINDER)
> > default y
> > help
> > Saying Y here will allow you to use Linux in text mode through a
> > @@ -45,7 +45,7 @@ config VGACON_SOFT_SCROLLBACK_SIZE
> > screenfuls of scrollback buffer
>
> You could just place all of the CONFIG_VT options inside an if VT and
> then the new fblog options in the else case rather than modifying all of
> the (already overly long) depends statements.
Indeed, that sounds better.
> > +config FBLOG
> > + tristate "Framebuffer Kernel Log Driver"
> > + depends on !VT && FB
> > + default n
>
> Default n is not required, all options are default n unless otherwise
> specified. I don't think you need the dependency on FB either, since
> this file should only get included if CONFIG_FB is set?
I will remove the default-line, but the FB-dependency is required. The
"console/" directory includes also drivers like vgacon which do not
depend on FB so it gets always included.
> > + help
> > + This driver displays all kernel log messages on all connected
> > + framebuffers. It is mutually exclusive with
> > CONFIG_FRAMEBUFFER_CONSOLE
> > + and CONFIG_VT. It was mainly created for debugging purposes when
> > + CONFIG_VT is not selected but you still want kernel boot
> > messages on
> > + the screen.
>
> Do command line options exist to specify screens to write the debug log
> to? It might be a useful feature to have.
See patch-7. However, I am not entirely happy with it. I will probably
have to rethink the parameters.
Thanks for reviewing. I will fix all these in the next series.
Regards
David
^ permalink raw reply
* Re: [PATCH 3/6] OMAPDSS: DSS: Cleanup cpu_is_xxxx checks
From: Tomi Valkeinen @ 2012-08-14 9:48 UTC (permalink / raw)
To: Chandrabhanu Mahapatra; +Cc: linux-omap, linux-fbdev
In-Reply-To: <1344859176-4512-1-git-send-email-cmahapatra@ti.com>
[-- Attachment #1: Type: text/plain, Size: 8283 bytes --]
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
> Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
> ---
> 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?
> #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?
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 =).
> +
> static struct {
> struct platform_device *pdev;
> void __iomem *base;
> @@ -83,6 +91,8 @@ static struct {
>
> bool ctx_valid;
> u32 ctx[DSS_SZ_REGS / sizeof(u32)];
> +
> + const struct dss_features *feat;
> } dss;
>
> static const char * const dss_generic_clk_source_names[] = {
> @@ -91,6 +101,30 @@ static const char * const dss_generic_clk_source_names[] = {
> [OMAP_DSS_CLK_SRC_FCK] = "DSS_FCK",
> };
>
> +static const struct __initdata dss_features omap2_dss_features = {
> + .fck_div_max = 16,
> + .factor = 2,
> + .clk_name = NULL,
> +};
include/linux/init.h says says __initdata should be used like:
static int init_variable __initdata = 0;
And there actually seems to be __initconst also, which I think is the
correct one to use here. Didn't know about that =):
static const char linux_logo[] __initconst = { 0x32, 0x36, ... };
> +static const struct __initdata dss_features omap34_dss_features = {
Perhaps the names could be more consistent. "omap34" doesn't sound like
any omap we have ;). So maybe just omap2xxx, omap34xx, etc, the same way
we have in the cpu_is calls.
> + .fck_div_max = 16,
> + .factor = 2,
> + .clk_name = "dpll4_m4_ck",
> +};
> +
> +static const struct __initdata dss_features omap36_dss_features = {
> + .fck_div_max = 32,
> + .factor = 1,
> + .clk_name = "dpll4_m4_ck",
> +};
> +
> +static const struct __initdata dss_features omap4_dss_features = {
> + .fck_div_max = 32,
> + .factor = 1,
> + .clk_name = "dpll_per_m5x2_ck",
> +};
> +
> static inline void dss_write_reg(const struct dss_reg idx, u32 val)
> {
> __raw_writel(val, dss.base + idx.idx);
> @@ -236,7 +270,6 @@ const char *dss_get_generic_clk_source_name(enum omap_dss_clk_source clk_src)
> return dss_generic_clk_source_names[clk_src];
> }
>
> -
> void dss_dump_clocks(struct seq_file *s)
> {
> unsigned long dpll4_ck_rate;
> @@ -259,18 +292,10 @@ void dss_dump_clocks(struct seq_file *s)
>
> seq_printf(s, "dpll4_ck %lu\n", dpll4_ck_rate);
>
> - if (cpu_is_omap3630() || cpu_is_omap44xx())
> - seq_printf(s, "%s (%s) = %lu / %lu = %lu\n",
> - fclk_name, fclk_real_name,
> - dpll4_ck_rate,
> - dpll4_ck_rate / dpll4_m4_ck_rate,
> - fclk_rate);
> - else
> - seq_printf(s, "%s (%s) = %lu / %lu * 2 = %lu\n",
> - fclk_name, fclk_real_name,
> - dpll4_ck_rate,
> - dpll4_ck_rate / dpll4_m4_ck_rate,
> - fclk_rate);
> + seq_printf(s, "%s (%s) = %lu / %lu * %d = %lu\n",
> + fclk_name, fclk_real_name, dpll4_ck_rate,
> + dpll4_ck_rate / dpll4_m4_ck_rate,
> + dss.feat->factor, fclk_rate);
> } else {
> seq_printf(s, "%s (%s) = %lu\n",
> fclk_name, fclk_real_name,
> @@ -470,7 +495,7 @@ int dss_calc_clock_div(unsigned long req_pck, struct dss_clock_info *dss_cinfo,
>
> unsigned long fck, max_dss_fck;
>
> - u16 fck_div, fck_div_max = 16;
> + u16 fck_div;
>
> int match = 0;
> int min_fck_per_pck;
> @@ -480,9 +505,8 @@ int dss_calc_clock_div(unsigned long req_pck, struct dss_clock_info *dss_cinfo,
> max_dss_fck = dss_feat_get_param_max(FEAT_PARAM_DSS_FCK);
>
> fck = clk_get_rate(dss.dss_clk);
> - if (req_pck == dss.cache_req_pck &&
> - ((cpu_is_omap34xx() && prate == dss.cache_prate) ||
> - dss.cache_dss_cinfo.fck == fck)) {
> + if (req_pck == dss.cache_req_pck && prate == dss.cache_prate &&
> + dss.cache_dss_cinfo.fck == fck) {
> DSSDBG("dispc clock info found from cache.\n");
> *dss_cinfo = dss.cache_dss_cinfo;
> *dispc_cinfo = dss.cache_dispc_cinfo;
> @@ -519,16 +543,10 @@ retry:
>
> goto found;
> } else {
> - if (cpu_is_omap3630() || cpu_is_omap44xx())
> - fck_div_max = 32;
> -
> - for (fck_div = fck_div_max; fck_div > 0; --fck_div) {
> + for (fck_div = dss.feat->fck_div_max; fck_div > 0; --fck_div) {
> struct dispc_clock_info cur_dispc;
>
> - if (fck_div_max == 32)
> - fck = prate / fck_div;
> - else
> - fck = prate / fck_div * 2;
> + fck = prate / fck_div * dss.feat->factor;
>
> if (fck > max_dss_fck)
> continue;
> @@ -633,22 +651,11 @@ static int dss_get_clocks(void)
>
> dss.dss_clk = clk;
>
> - if (cpu_is_omap34xx()) {
> - clk = clk_get(NULL, "dpll4_m4_ck");
> - if (IS_ERR(clk)) {
> - DSSERR("Failed to get dpll4_m4_ck\n");
> - r = PTR_ERR(clk);
> - goto err;
> - }
> - } else if (cpu_is_omap44xx()) {
> - clk = clk_get(NULL, "dpll_per_m5x2_ck");
> - if (IS_ERR(clk)) {
> - DSSERR("Failed to get dpll_per_m5x2_ck\n");
> - r = PTR_ERR(clk);
> - goto err;
> - }
> - } else { /* omap24xx */
> - clk = NULL;
> + clk = clk_get(NULL, dss.feat->clk_name);
> + if (IS_ERR(clk)) {
> + DSSERR("Failed to get %s\n", dss.feat->clk_name);
> + r = PTR_ERR(clk);
> + goto err;
> }
>
> dss.dpll4_m4_ck = clk;
> @@ -704,6 +711,26 @@ void dss_debug_dump_clocks(struct seq_file *s)
> }
> #endif
>
> +static int __init dss_init_features(struct device *dev)
> +{
> + dss.feat = devm_kzalloc(dev, sizeof(*dss.feat), GFP_KERNEL);
> + if (!dss.feat) {
> + dev_err(dev, "Failed to allocate local DSS Features\n");
> + return -ENOMEM;
> + }
> +
> + 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.
> +
> + return 0;
> +}
> +
> /* DSS HW IP initialisation */
> static int __init omap_dsshw_probe(struct platform_device *pdev)
> {
> @@ -750,6 +777,10 @@ static int __init omap_dsshw_probe(struct platform_device *pdev)
> dss.lcd_clk_source[0] = OMAP_DSS_CLK_SRC_FCK;
> dss.lcd_clk_source[1] = OMAP_DSS_CLK_SRC_FCK;
>
> + r = dss_init_features(&dss.pdev->dev);
> + if (r)
> + return r;
> +
> rev = dss_read_reg(DSS_REVISION);
> printk(KERN_INFO "OMAP DSS rev %d.%d\n",
> FLD_GET(rev, 7, 4), FLD_GET(rev, 3, 0));
> @@ -772,6 +803,8 @@ static int __exit omap_dsshw_remove(struct platform_device *pdev)
>
> dss_put_clocks();
>
> + devm_kfree(&dss.pdev->dev, (void *)dss.feat);
You don't need to free the memory allocated with devm_kzalloc. The
framework will free it automatically on unload (that's the idea of
devm_* functions).
Otherwise looks good, cleans up nicely the cpu_is_* mess.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 1/6] OMAPDSS: DISPC: cleanup cpu_is_xxxx checks
From: Tomi Valkeinen @ 2012-08-14 9:58 UTC (permalink / raw)
To: Chandrabhanu Mahapatra; +Cc: linux-omap, linux-fbdev
In-Reply-To: <1344859135-4470-1-git-send-email-cmahapatra@ti.com>
[-- Attachment #1: Type: text/plain, Size: 498 bytes --]
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
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 05/11] fblog: register one fblog object per framebuffer
From: David Herrmann @ 2012-08-14 11:01 UTC (permalink / raw)
To: Ryan Mallon
Cc: linux-fbdev, Florian Tobias Schandinat, Greg Kroah-Hartman,
linux-serial, Alan Cox, linux-kernel, Geert Uytterhoeven
In-Reply-To: <50284234.40306@gmail.com>
Hi Ryan
On Mon, Aug 13, 2012 at 1:54 AM, Ryan Mallon <rmallon@gmail.com> wrote:
> On 13/08/12 00:53, David Herrmann wrote:
>> drivers/video/console/fblog.c | 195 ++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 195 insertions(+)
>>
>> diff --git a/drivers/video/console/fblog.c b/drivers/video/console/fblog.c
>> index fb39737..279f4d8 100644
>> --- a/drivers/video/console/fblog.c
>> +++ b/drivers/video/console/fblog.c
>> @@ -23,15 +23,210 @@
>> * all fblog instances before running other graphics applications.
>> */
>>
>> +#define pr_fmt(_fmt) KBUILD_MODNAME ": " _fmt
>> +
>> +#include <linux/device.h>
>> +#include <linux/fb.h>
>> #include <linux/module.h>
>> +#include <linux/mutex.h>
>> +
>> +enum fblog_flags {
>> + FBLOG_KILLED,
>> +};
>> +
>> +struct fblog_fb {
>> + unsigned long flags;
>
> Are more flags added in later patches? If not, why not just have:
>
> bool is_killed;
>
> ?
Yes, more are added in patch-6 and patch-8 which includes FBLOG_OPEN,
FBLOG_SUSPENDED, FBLOG_BLANKED.
>> +static void fblog_release(struct device *dev)
>> +{
>> + struct fblog_fb *fb = to_fblog_dev(dev);
>> +
>> + kfree(fb);
>> + module_put(THIS_MODULE);
>> +}
>> +
>> +static void fblog_do_unregister(struct fb_info *info)
>> +{
>> + struct fblog_fb *fb;
>> +
>> + fb = fblog_fbs[info->node];
>> + if (!fb || fb->info != info)
>> + return;
>> +
>> + fblog_fbs[info->node] = NULL;
>> +
>> + device_del(&fb->dev);
>> + put_device(&fb->dev);
>
> device_unregister?
Right, I will replace it.
>> +}
>> +
>> +static void fblog_do_register(struct fb_info *info, bool force)
>> +{
>> + struct fblog_fb *fb;
>> + int ret;
>> +
>> + fb = fblog_fbs[info->node];
>> + if (fb && fb->info != info) {
>> + if (!force)
>> + return;
>> +
>> + fblog_do_unregister(fb->info);
>> + }
>> +
>> + fb = kzalloc(sizeof(*fb), GFP_KERNEL);
>> + if (!fb)
>> + return;
>> +
>> + fb->info = info;
>> + __module_get(THIS_MODULE);
>> + device_initialize(&fb->dev);
>> + fb->dev.class = fb_class;
>> + fb->dev.release = fblog_release;
>> + dev_set_name(&fb->dev, "fblog%d", info->node);
>> + fblog_fbs[info->node] = fb;
>> +
>> + ret = device_add(&fb->dev);
>> + if (ret) {
>> + fblog_fbs[info->node] = NULL;
>> + set_bit(FBLOG_KILLED, &fb->flags);
>> + put_device(&fb->dev);
>
> kfree(fb); ?
No. See device_initialize() in ./drivers/base/core.c. After a call to
device_initialize() the object is ref-counted so put_device() will
invoke the fblog_release() callback which will call kfree(fb) itself.
>> + return;
>> + }
>> +}
>> +
>> +static void fblog_register(struct fb_info *info, bool force)
>> +{
>> + mutex_lock(&fblog_registration_lock);
>> + fblog_do_register(info, force);
>> + mutex_unlock(&fblog_registration_lock);
>> +}
>> +
>> +static void fblog_unregister(struct fb_info *info)
>> +{
>> + mutex_lock(&fblog_registration_lock);
>> + fblog_do_unregister(info);
>> + mutex_unlock(&fblog_registration_lock);
>> +}
>
> This locking is needlessly heavy, and could easily pushed down into the
> fb_do_(un)register functions. It would also help make it clear exactly
> what the lock is protecting.
I need to call fblog_do_unregister() from within fblog_do_register().
I cannot release the locks while calling fblog_do_unregister() so I
need the unlocked fblog_do_unregister() function. So the locking must
be in a wrapper function.
See below for an explanation of the locks.
>> +static int fblog_event(struct notifier_block *self, unsigned long action,
>> + void *data)
>> +{
>> + struct fb_event *event = data;
>> + struct fb_info *info = event->info;
>> +
>> + switch(action) {
>> + case FB_EVENT_FB_REGISTERED:
>> + /* This is called when a low-level system driver registers a new
>> + * framebuffer. The registration lock is held but the console
>> + * lock might not be held when this is called. */
>
> Nitpick:
>
> /*
> * The Linux kernel multi-line
> * comment style looks like
> * this.
> */
I was confused by a recent discussion on the LKML:
http://comments.gmane.org/gmane.linux.kernel/1282421
However, turns out they didn't add this to CodingStyle so I will adopt
the old style again. Will be fixed in the next revision, thanks.
>> + fblog_register(info, true);
>> + break;
>> + case FB_EVENT_FB_UNREGISTERED:
>> + /* This is called when a low-level system driver unregisters a
>> + * framebuffer. The registration lock is held but the console
>> + * lock might not be held. */
>> + fblog_unregister(info);
>> + break;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void fblog_scan(void)
>> +{
>> + unsigned int i;
>> + struct fb_info *info, *tmp;
>> +
>> + for (i = 0; i < FB_MAX; ++i) {
>> + info = get_fb_info(i);
>> + if (!info || IS_ERR(info))
>
> Nitpick:
>
> if (IS_ERR_OR_NULL(info))
Didn't know of this macro, thanks.
>> + continue;
>> +
>> + fblog_register(info, false);
>
> This function should really return a value to indicate if it failed.
> There is no point continuing if it didn't register anything.
Indeed.
>> + /* There is a very subtle race-condition. Even though we might
>> + * own a reference to the fb, it may still get unregistered
>> + * between our call from get_fb_info() and fblog_register().
>> + * Therefore, we simply check whether the same fb still is
>> + * registered by calling get_fb_info() again. Only if they
>> + * differ we know that it got unregistered, therefore, we
>> + * call fblog_unregister() with the old pointer. */
>> +
>> + tmp = get_fb_info(i);
>> + if (tmp && !IS_ERR(tmp))
>> + put_fb_info(tmp);
>> + if (tmp != info)
>> + fblog_unregister(info);
>
> It would be better to fix this issue properly. Calling fblog_unregister
> here also looks unsafe if the call to fblog_register above failed.
fblog_unregister() does nothing if the passed "info" does not match
the found "info" so this is safe. And as we are holding a reference to
"info" here, the pointer is always valid and cannot be used by other
FBs.
Fixing this properly means changing the locking of fbdev. This can
either be done by exporting the fbdev-registration-lock (which I want
to avoid as locking should never be exported in an API) or by changing
fbdev to provide an scan/enumeration function itself. However, in my
opinion both would be uglier than using this race-condition-check
here.
The best way would be redesigning the fbdev in-kernel API. But that
means checking that fbcon will not break and this is something I
really don't want to touch.
>> + /* Here we either called fblog_unregister() and therefore do not
>> + * need any reference to the fb, or we can be sure that the FB
>> + * is registered and FB_EVENT_FB_UNREGISTERED will be called
>> + * before the last reference is dropped. Hence, we can drop our
>> + * reference here. */
>
> This seems a slightly odd reasoning. Why would you not hold a reference
> to something you are using?
It's because get_fb_info() takes the fbdev-registration-lock so we
cannot call it from fblog_event(). And fblog_event() calls
fblog_register() for hotplugged framebuffers so we cannot get a refcnt
for hotplugged framebuffers.
Now I can either increase the fbdev-refcount manually without calling
get_fb_info() in fblog_register() or I can simply drop the framebuffer
when the fbdev-core notifies me that it is gone. I chose the latter
one.
>> + put_fb_info(info);
>> + }
>> +}
>> +
>> +static struct notifier_block fblog_notifier = {
>> + .notifier_call = fblog_event,
>> +};
>>
>> static int __init fblog_init(void)
>> {
>> + int ret;
>> +
>> + ret = fb_register_client(&fblog_notifier);
>> + if (ret) {
>> + pr_err("cannot register framebuffer notifier\n");
>> + return ret;
>> + }
>> +
>> + fblog_scan();
>> +
>> return 0;
>> }
>>
>> static void __exit fblog_exit(void)
>> {
>> + unsigned int i;
>> + struct fb_info *info;
>> +
>> + fb_unregister_client(&fblog_notifier);
>> +
>> + /* We scan through the whole registered_fb array here instead of
>> + * fblog_fbs because we need to get the device lock _before_ the
>> + * fblog-registration-lock. */
>> +
>> + for (i = 0; i < FB_MAX; ++i) {
>> + info = get_fb_info(i);
>> + if (!info || IS_ERR(info))
>> + continue;
>> +
>> + fblog_unregister(info);
>
> Given the description of the get_fb_info/fblog_register race above, can
> this unregister the wrong framebuffer?
No. fblog_unregister() will do nothing if the "info" pointers do not
match. Moreover, this unregisters _all_ framebuffers, so I don't
understand how this can unregister the "wrong" framebuffer?
But I just noticed a race here. If the fbdev core unregisters a
framebuffer during my loop, I will not call fblog_unregister() on it,
as it does not exist anymore. Therefore, I will not free it in my
fblog_fbs array as the fblog_notifier has already been unregistered.
So I need to set a global "exiting" flag and fblog_event() shall only
handle the UNBIND/UNREGISTER events during exit. Then I simply move
the fb_unregister_client() call below this loop.
>> + put_fb_info(info);
>> + }
>> }
>>
>> module_init(fblog_init);
>>
>
A short explanation for all locks:
First of all, I spent hours getting this right. The easy way would be
removing all locks and relying on the fbdev locking (fbcon does this).
But obviously, that doesn't work as fbdev has no safe way of
scanning/enumerating all existing devices. And this is needed as fblog
can be compiled as a module.
fbdev has a core registration-lock and each framebuffer has its own
fb-lock. fblog_event() is sometimes called with these locks held,
sometimes without any locks held (see the comments in this function)
and I need to work around this.
So fblog_event() as entry point for hotplugging is called with the
fbdev-registration-lock held. And it calls fblog_(un)register() which
uses its own locks. So when calling this from fblog_scan(), I need to
make sure to guarantee that the locks are acquired in the same order
as in fblog_event(), otherwise I might have deadlocks. But I have no
outside access to the fbdev-registration-lock, therefore, the
fblog-registration-lock is needed and fblog_scan() needs to check for
those ugly races.
As you mentioned that this locking is needlessly complex, I have to
disagree. I use one lock to protect the registration
(fblog_registration_lock) and one lock to protect each registered
framebuffer from concurrent access (struct fblog_fb->lock). This is
the most common way to protect hotplugged devices and I don't see how
this can be done with less locks? (these are the only locks that are
added by fblog).
Normally, this would be all locks I have to access. However, the
fbdev-core wasn't designed as in-kernel API and thus has very
inconsistent locking. And all entry points to fblog that are not
through fbdev-notifier-callbacks (eg, fblog_scan) need to make sure to
acquire the same locks as the fbdev-core to avoid races with the
fbdev-core. As this is not possible, because the fbdev-locks are not
exported, I need to carefully use fbdev-functions that guarantee that
I have no races. And I think I found the only way to guarantee this.
If anyone has other ideas, I would be glad to hear them.
Thanks for reviewing the code!
Regards
David
^ permalink raw reply
* Re: [PATCH 06/11] fblog: open fb on registration
From: David Herrmann @ 2012-08-14 11:05 UTC (permalink / raw)
To: Ryan Mallon
Cc: linux-fbdev, Florian Tobias Schandinat, Greg Kroah-Hartman,
linux-serial, Alan Cox, linux-kernel, Geert Uytterhoeven
In-Reply-To: <50284383.7020100@gmail.com>
Hi Ryan
On Mon, Aug 13, 2012 at 2:00 AM, Ryan Mallon <rmallon@gmail.com> wrote:
> On 13/08/12 00:53, David Herrmann wrote:
>> /*
>> + * fblog_open/close()
>> + * These functions manage access to the underlying framebuffer. While opened, we
>> + * have a valid reference to the fb and can use it for drawing operations. When
>> + * the fb is unregistered, we drop our reference and close the fb so it can get
>> + * deleted properly. We also mark it as dead so no further fblog_open() call
>> + * will succeed.
>> + * Both functions must be called with the fb->info->lock mutex held! But make
>> + * sure to lock it _before_ locking the fblog-registration-lock. Otherwise, we
>> + * will dead-lock with fb-registration.
>> + */
>> +
>> +static int fblog_open(struct fblog_fb *fb)
>> +{
>> + int ret;
>> +
>> + mutex_lock(&fb->lock);
>> +
>> + if (test_bit(FBLOG_KILLED, &fb->flags)) {
>> + ret = -ENODEV;
>> + goto unlock;
>> + }
>> +
>> + if (test_bit(FBLOG_OPEN, &fb->flags)) {
>> + ret = 0;
>> + goto unlock;
>> + }
>> +
>> + if (!try_module_get(fb->info->fbops->owner)) {
>> + ret = -ENODEV;
>> + goto out_killed;
>> + }
>> +
>> + if (fb->info->fbops->fb_open && fb->info->fbops->fb_open(fb->info, 0)) {
>
> Should propagate the error here:
>
> if (fb->info->fbops->fb_open) {
> ret = fb->info->fbops->fb_open(fb->info, 0);
> if (ret)
> gotou out_unref;
> }
Nice catch, thanks.
>> +/*
>> * fblog framebuffer list
>> * The fblog_fbs[] array contains all currently registered framebuffers. If a
>> * framebuffer is in that list, we always must make sure that we own a reference
>> @@ -77,6 +147,7 @@ static void fblog_do_unregister(struct fb_info *info)
>>
>> fblog_fbs[info->node] = NULL;
>>
>> + fblog_close(fb, true);
>> device_del(&fb->dev);
>> put_device(&fb->dev);
>
> device_unregister?
Heh, you already mentioned this in the previous patch. I definitely
need to fix that.
>> }
>> @@ -99,6 +170,7 @@ static void fblog_do_register(struct fb_info *info, bool force)
>> return;
>>
>> fb->info = info;
>> + mutex_init(&fb->lock);
>> __module_get(THIS_MODULE);
>> device_initialize(&fb->dev);
>> fb->dev.class = fb_class;
>> @@ -113,6 +185,8 @@ static void fblog_do_register(struct fb_info *info, bool force)
>> put_device(&fb->dev);
>> return;
>> }
>> +
>> + fblog_open(fb);
>> }
>
> This function should really return an errno value.
I never used the return value in my code but as mentioned in the
previous patches, fblog_scan() should check them. So yes, I will add
this.
Thanks!
David
^ permalink raw reply
* Re: [PATCH 07/11] fblog: allow selecting fbs via sysfs and module-parameters
From: David Herrmann @ 2012-08-14 11:07 UTC (permalink / raw)
To: Ryan Mallon
Cc: linux-fbdev, Florian Tobias Schandinat, Greg Kroah-Hartman,
linux-serial, Alan Cox, linux-kernel, Geert Uytterhoeven
In-Reply-To: <5028447E.7000208@gmail.com>
Hi Ryan
On Mon, Aug 13, 2012 at 2:04 AM, Ryan Mallon <rmallon@gmail.com> wrote:
> On 13/08/12 00:53, David Herrmann wrote:
>> +static ssize_t fblog_dev_active_show(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct fblog_fb *fb = to_fblog_dev(dev);
>> +
>> + return snprintf(buf, PAGE_SIZE, "%d\n",
>> + !!test_bit(FBLOG_OPEN, &fb->flags));
>
> Nitpick. sprintf is okay here, %d is rarely longer than PAGE_SIZE :-).
>
>> +}
>> +
>> +static ssize_t fblog_dev_active_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf,
>> + size_t count)
>> +{
>> + struct fblog_fb *fb = to_fblog_dev(dev);
>> + unsigned long num;
>> + int ret = 0;
>> +
>> + num = simple_strtoul(buf, NULL, 10);
>
> kstrtoul is preferred these days I think, it also catches errors.
>
>> +
>> + mutex_lock(&fb->info->lock);
>> + if (num)
>> + ret = fblog_open(fb);
>> + else
>> + fblog_close(fb, false);
>> + mutex_unlock(&fb->info->lock);
>> +
>> + return ret ? ret : count;
>
> Nitpick, you can use gcc's shortcut form of the ? operator here:
>
> return ret ?: count;
>
>> @@ -186,7 +228,20 @@ static void fblog_do_register(struct fb_info *info, bool force)
>> return;
>> }
>>
>> - fblog_open(fb);
>> + ret = device_create_file(&fb->dev, &dev_attr_active);
>> + if (ret) {
>> + pr_err("fblog: cannot create sysfs entry");
>
> Shouldn't need the "fblog: " prefix, since you have pr_fmt defined.
>
>> + /* do not open fb if we cannot create control file */
>> + do_open = false;
>> + }
>> +
>> + if (!activate_on_hotplug)
>> + do_open = false;
>> + if (main_only && info->node != 0)
>> + do_open = false;
>> +
>> + if (do_open)
>> + fblog_open(fb);
>> }
>>
>> static void fblog_register(struct fb_info *info, bool force)
All four fixes make sense, thanks!
David
^ permalink raw reply
* Re: [PATCH 1/6] OMAPDSS: DISPC: cleanup cpu_is_xxxx checks
From: Mahapatra, Chandrabhanu @ 2012-08-14 12:15 UTC (permalink / raw)
To: tomi.valkeinen; +Cc: linux-omap, linux-fbdev
In-Reply-To: <1344859135-4470-1-git-send-email-cmahapatra@ti.com>
> +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.
^ permalink raw reply
* Re: [PATCH 1/6] OMAPDSS: DISPC: cleanup cpu_is_xxxx checks
From: Tomi Valkeinen @ 2012-08-14 12:16 UTC (permalink / raw)
To: Mahapatra, Chandrabhanu; +Cc: linux-omap, linux-fbdev
In-Reply-To: <CAF0AtAt9SakEnavHMFXYZ2+nPXXreGcQNyXiJ7TuarRa1tATXA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2590 bytes --]
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
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 3/6] OMAPDSS: DSS: Cleanup cpu_is_xxxx checks
From: Mahapatra, Chandrabhanu @ 2012-08-14 12:42 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-omap, linux-fbdev
In-Reply-To: <1344937705.2345.17.camel@deskari>
On Tue, Aug 14, 2012 at 3:18 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> 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.
>> Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
>> ---
>> 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
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?
>> +
>> static struct {
>> struct platform_device *pdev;
>> void __iomem *base;
>> @@ -83,6 +91,8 @@ static struct {
>>
>> bool ctx_valid;
>> u32 ctx[DSS_SZ_REGS / sizeof(u32)];
>> +
>> + const struct dss_features *feat;
>> } dss;
>>
>> static const char * const dss_generic_clk_source_names[] = {
>> @@ -91,6 +101,30 @@ static const char * const dss_generic_clk_source_names[] = {
>> [OMAP_DSS_CLK_SRC_FCK] = "DSS_FCK",
>> };
>>
>> +static const struct __initdata dss_features omap2_dss_features = {
>> + .fck_div_max = 16,
>> + .factor = 2,
>> + .clk_name = NULL,
>> +};
>
> include/linux/init.h says says __initdata should be used like:
>
> static int init_variable __initdata = 0;
>
> And there actually seems to be __initconst also, which I think is the
> correct one to use here. Didn't know about that =):
>
> static const char linux_logo[] __initconst = { 0x32, 0x36, ... };
Thanks for mentioning.
>
>> +static const struct __initdata dss_features omap34_dss_features = {
>
> Perhaps the names could be more consistent. "omap34" doesn't sound like
> any omap we have ;). So maybe just omap2xxx, omap34xx, etc, the same way
> we have in the cpu_is calls.
>
ok.
>> + .fck_div_max = 16,
>> + .factor = 2,
>> + .clk_name = "dpll4_m4_ck",
>> +};
>> +
>> +static const struct __initdata dss_features omap36_dss_features = {
>> + .fck_div_max = 32,
>> + .factor = 1,
>> + .clk_name = "dpll4_m4_ck",
>> +};
>> +
>> +static const struct __initdata dss_features omap4_dss_features = {
>> + .fck_div_max = 32,
>> + .factor = 1,
>> + .clk_name = "dpll_per_m5x2_ck",
>> +};
>> +
>> static inline void dss_write_reg(const struct dss_reg idx, u32 val)
>> {
>> __raw_writel(val, dss.base + idx.idx);
>> @@ -236,7 +270,6 @@ const char *dss_get_generic_clk_source_name(enum omap_dss_clk_source clk_src)
>> return dss_generic_clk_source_names[clk_src];
>> }
>>
>> -
>> void dss_dump_clocks(struct seq_file *s)
>> {
>> unsigned long dpll4_ck_rate;
>> @@ -259,18 +292,10 @@ void dss_dump_clocks(struct seq_file *s)
>>
>> seq_printf(s, "dpll4_ck %lu\n", dpll4_ck_rate);
>>
>> - if (cpu_is_omap3630() || cpu_is_omap44xx())
>> - seq_printf(s, "%s (%s) = %lu / %lu = %lu\n",
>> - fclk_name, fclk_real_name,
>> - dpll4_ck_rate,
>> - dpll4_ck_rate / dpll4_m4_ck_rate,
>> - fclk_rate);
>> - else
>> - seq_printf(s, "%s (%s) = %lu / %lu * 2 = %lu\n",
>> - fclk_name, fclk_real_name,
>> - dpll4_ck_rate,
>> - dpll4_ck_rate / dpll4_m4_ck_rate,
>> - fclk_rate);
>> + seq_printf(s, "%s (%s) = %lu / %lu * %d = %lu\n",
>> + fclk_name, fclk_real_name, dpll4_ck_rate,
>> + dpll4_ck_rate / dpll4_m4_ck_rate,
>> + dss.feat->factor, fclk_rate);
>> } else {
>> seq_printf(s, "%s (%s) = %lu\n",
>> fclk_name, fclk_real_name,
>> @@ -470,7 +495,7 @@ int dss_calc_clock_div(unsigned long req_pck, struct dss_clock_info *dss_cinfo,
>>
>> unsigned long fck, max_dss_fck;
>>
>> - u16 fck_div, fck_div_max = 16;
>> + u16 fck_div;
>>
>> int match = 0;
>> int min_fck_per_pck;
>> @@ -480,9 +505,8 @@ int dss_calc_clock_div(unsigned long req_pck, struct dss_clock_info *dss_cinfo,
>> max_dss_fck = dss_feat_get_param_max(FEAT_PARAM_DSS_FCK);
>>
>> fck = clk_get_rate(dss.dss_clk);
>> - if (req_pck = dss.cache_req_pck &&
>> - ((cpu_is_omap34xx() && prate = dss.cache_prate) ||
>> - dss.cache_dss_cinfo.fck = fck)) {
>> + if (req_pck = dss.cache_req_pck && prate = dss.cache_prate &&
>> + dss.cache_dss_cinfo.fck = fck) {
>> DSSDBG("dispc clock info found from cache.\n");
>> *dss_cinfo = dss.cache_dss_cinfo;
>> *dispc_cinfo = dss.cache_dispc_cinfo;
>> @@ -519,16 +543,10 @@ retry:
>>
>> goto found;
>> } else {
>> - if (cpu_is_omap3630() || cpu_is_omap44xx())
>> - fck_div_max = 32;
>> -
>> - for (fck_div = fck_div_max; fck_div > 0; --fck_div) {
>> + for (fck_div = dss.feat->fck_div_max; fck_div > 0; --fck_div) {
>> struct dispc_clock_info cur_dispc;
>>
>> - if (fck_div_max = 32)
>> - fck = prate / fck_div;
>> - else
>> - fck = prate / fck_div * 2;
>> + fck = prate / fck_div * dss.feat->factor;
>>
>> if (fck > max_dss_fck)
>> continue;
>> @@ -633,22 +651,11 @@ static int dss_get_clocks(void)
>>
>> dss.dss_clk = clk;
>>
>> - if (cpu_is_omap34xx()) {
>> - clk = clk_get(NULL, "dpll4_m4_ck");
>> - if (IS_ERR(clk)) {
>> - DSSERR("Failed to get dpll4_m4_ck\n");
>> - r = PTR_ERR(clk);
>> - goto err;
>> - }
>> - } else if (cpu_is_omap44xx()) {
>> - clk = clk_get(NULL, "dpll_per_m5x2_ck");
>> - if (IS_ERR(clk)) {
>> - DSSERR("Failed to get dpll_per_m5x2_ck\n");
>> - r = PTR_ERR(clk);
>> - goto err;
>> - }
>> - } else { /* omap24xx */
>> - clk = NULL;
>> + clk = clk_get(NULL, dss.feat->clk_name);
>> + if (IS_ERR(clk)) {
>> + DSSERR("Failed to get %s\n", dss.feat->clk_name);
>> + r = PTR_ERR(clk);
>> + goto err;
>> }
>>
>> dss.dpll4_m4_ck = clk;
>> @@ -704,6 +711,26 @@ void dss_debug_dump_clocks(struct seq_file *s)
>> }
>> #endif
>>
>> +static int __init dss_init_features(struct device *dev)
>> +{
>> + dss.feat = devm_kzalloc(dev, sizeof(*dss.feat), GFP_KERNEL);
>> + if (!dss.feat) {
>> + dev_err(dev, "Failed to allocate local DSS Features\n");
>> + return -ENOMEM;
>> + }
>> +
>> + 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.
>> +
>> + return 0;
>> +}
>> +
>> /* DSS HW IP initialisation */
>> static int __init omap_dsshw_probe(struct platform_device *pdev)
>> {
>> @@ -750,6 +777,10 @@ static int __init omap_dsshw_probe(struct platform_device *pdev)
>> dss.lcd_clk_source[0] = OMAP_DSS_CLK_SRC_FCK;
>> dss.lcd_clk_source[1] = OMAP_DSS_CLK_SRC_FCK;
>>
>> + r = dss_init_features(&dss.pdev->dev);
>> + if (r)
>> + return r;
>> +
>> rev = dss_read_reg(DSS_REVISION);
>> printk(KERN_INFO "OMAP DSS rev %d.%d\n",
>> FLD_GET(rev, 7, 4), FLD_GET(rev, 3, 0));
>> @@ -772,6 +803,8 @@ static int __exit omap_dsshw_remove(struct platform_device *pdev)
>>
>> dss_put_clocks();
>>
>> + devm_kfree(&dss.pdev->dev, (void *)dss.feat);
>
> You don't need to free the memory allocated with devm_kzalloc. The
> framework will free it automatically on unload (that's the idea of
> devm_* functions).
>
Ok.
--
Chandrabhanu Mahapatra
Texas Instruments India Pvt. Ltd.
^ permalink raw reply
* Re: [PATCH v2 07/13] OMAPDSS: HDMI: Add a get_timing function for HDMI interface
From: Tomi Valkeinen @ 2012-08-14 13:02 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-fbdev, linux-omap
In-Reply-To: <1344512989-4071-8-git-send-email-archit@ti.com>
[-- Attachment #1: Type: text/plain, Size: 1055 bytes --]
Hi,
On Thu, 2012-08-09 at 17:19 +0530, Archit Taneja wrote:
> Add function omapdss_hdmi_display_get_timing() which returns the timings
> maintained by the HDMI interface driver in it's hdmi_config field. This
> prevents the need for the panel driver to configure default timings in it's
> probe.
>
> This function is just intended to be used once during the panel driver's probe.
> It makes sense for those interfaces which can be configured to a default timing.
I'm not sure about this patch. So I think the basic idea here is that
HDMI will use VGA video mode if, for whatever reason, no better mode is
found out.
After this change, the panel driver doesn't seem to do that. Instead it
uses whatever mode was used previously by the hdmi output driver.
Is the only reason for this patch to clean up the hdmi_panel driver? We
could have a dss helper function which returns the timings for VGA
(well, just a public const variable would be enough), and the panel
driver could use that to initialize the timings struct.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v2 07/13] OMAPDSS: HDMI: Add a get_timing function for HDMI interface
From: Archit Taneja @ 2012-08-14 13:27 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-fbdev, linux-omap
In-Reply-To: <1344949348.4845.46.camel@lappyti>
On Tuesday 14 August 2012 06:32 PM, Tomi Valkeinen wrote:
> Hi,
>
> On Thu, 2012-08-09 at 17:19 +0530, Archit Taneja wrote:
>> Add function omapdss_hdmi_display_get_timing() which returns the timings
>> maintained by the HDMI interface driver in it's hdmi_config field. This
>> prevents the need for the panel driver to configure default timings in it's
>> probe.
>>
>> This function is just intended to be used once during the panel driver's probe.
>> It makes sense for those interfaces which can be configured to a default timing.
>
> I'm not sure about this patch. So I think the basic idea here is that
> HDMI will use VGA video mode if, for whatever reason, no better mode is
> found out.
>
> After this change, the panel driver doesn't seem to do that. Instead it
> uses whatever mode was used previously by the hdmi output driver.
>
> Is the only reason for this patch to clean up the hdmi_panel driver? We
> could have a dss helper function which returns the timings for VGA
> (well, just a public const variable would be enough), and the panel
> driver could use that to initialize the timings struct.
Yes, that's the only reason, basically, to sync the panel with the
timings to which the hdmi output driver configured itself during it's
probe. We don't use hdmi_get_timing anywhere apart from here.
I thought it would be clean to retrieve the timings by taking whatever
is stored in the hdmi output driver, but we could leave it to a
hardcoded VGA as before.
I have done the same for venc, let me know if you think these should be
removed.
Archit
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox