[RESEND][PATCH v2] OMAP: DSS: Adding two APIs for panel-taal: check_timings and set_timings

[RESEND][PATCH v2] OMAP: DSS: Adding two APIs for panel-taal: check_timings and set_timings

[Mayuresh Janorkar]

check_timings and set_timings APIS are not present for panel-taal.

OMAPFB provides a bootarg omapfb.mode for setting mode parameters which include display,
resolution, bits-per-pixel.

OMAPFB expects panel driver to have check_timings and set_timings APIs.
These are checked by omapfb in case we wish to set default mode through bootargs.
e.g.: omapfb.mode="lcd:864x480-16" (display device:width X height - bits per pixel)

omapfb_set_def_mode function in omapfb-main.c essentially needs these functions
otherwise it would return -EINVAL and default mode sent through bootargs
would be ignored.

Signed-off-by: Mayuresh Janorkar <mayur@ti.com>
---
 drivers/video/omap2/displays/panel-taal.c |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/drivers/video/omap2/displays/panel-taal.c b/drivers/video/omap2/displays/panel-taal.c
index 61026f9..aded08c 100644
--- a/drivers/video/omap2/displays/panel-taal.c
+++ b/drivers/video/omap2/displays/panel-taal.c
@@ -476,6 +476,31 @@ static void taal_get_timings(struct omap_dss_device *dssdev,
 	*timings = dssdev->panel.timings;
 }
 
+static void taal_set_timings(struct omap_dss_device *dssdev,
+			struct omap_video_timings *timings)
+{
+	/*
+	 * TAAL panel's timing struct has only x_res and y_res
+	 * other timing parameters are not set
+	 */
+	dssdev->panel.timings.x_res = timings->x_res;
+	dssdev->panel.timings.y_res = timings->y_res;
+}
+
+static int taal_check_timings(struct omap_dss_device *dssdev,
+			struct omap_video_timings *timings)
+{
+	/*
+	 * TAAL panel's timing struct has only x_res and y_res
+	 * other timing parameters are not set
+	 */
+	if (!timings || timings->x_res != dssdev->panel.timings.x_res ||
+		timings->y_res != dssdev->panel.timings.y_res)
+		return -EINVAL;
+
+	return 0;
+}
+
 static void taal_get_resolution(struct omap_dss_device *dssdev,
 		u16 *xres, u16 *yres)
 {
@@ -1563,6 +1588,8 @@ static struct omap_dss_driver taal_driver = {
 	.memory_read	= taal_memory_read,
 
 	.get_timings	= taal_get_timings,
+	.set_timings	= taal_set_timings,
+	.check_timings	= taal_check_timings,
 
 	.driver         = {
 		.name   = "taal",
-- 
1.7.1

Re: [RESEND][PATCH v2] OMAP: DSS: Adding two APIs for panel-taal: check_timings and set_timings

[Tomi Valkeinen]

Hi,

On Wed, 2011-02-16 at 07:54 -0600, Janorkar, Mayuresh wrote:
> check_timings and set_timings APIS are not present for panel-taal.
> 
> OMAPFB provides a bootarg omapfb.mode for setting mode parameters which include display,
> resolution, bits-per-pixel.
> 
> OMAPFB expects panel driver to have check_timings and set_timings APIs.
> These are checked by omapfb in case we wish to set default mode through bootargs.
> e.g.: omapfb.mode="lcd:864x480-16" (display device:width X height - bits per pixel)
> 
> omapfb_set_def_mode function in omapfb-main.c essentially needs these functions
> otherwise it would return -EINVAL and default mode sent through bootargs
> would be ignored.

I don't like this patch. You cannot change the timings for Taal, so
those added functions look quite hacky.

The reason for this patch isn't clear from the description (it should).
If I guess correctly, the point of the patch is to be able to change the
default color format via boot arguments when using taal panel?

If so, I think the change should be in the omapfb driver. Perhaps the
omapfb driver should:

1. check if check_timings & set_timings exist
2. if they do exist, do the same thing as the code does now
3. if they don't, use get_timings to verify that the given resolution is
correct

That wouldn't be perfect either, but I guess it should do the job. But
this is again something where FB framework and OMAP HW do not quite
match, and we end up with hacky solution, no matter what we do. But we
can try to keep the hacks in the omapfb driver =).

 Tomi

RE: [RESEND][PATCH v2] OMAP: DSS: Adding two APIs for panel-taal: check_timings and set_timings

[Janorkar, Mayuresh]

Tomi,

> -----Original Message-----
> From: Valkeinen, Tomi
> Sent: Wednesday, February 16, 2011 7:28 PM
> To: Janorkar, Mayuresh
> Cc: linux-omap@vger.kernel.org
> Subject: Re: [RESEND][PATCH v2] OMAP: DSS: Adding two APIs for panel-taal:
> check_timings and set_timings
> 
> Hi,
> 
> On Wed, 2011-02-16 at 07:54 -0600, Janorkar, Mayuresh wrote:
> > check_timings and set_timings APIS are not present for panel-taal.
> >
> > OMAPFB provides a bootarg omapfb.mode for setting mode parameters which
> include display,
> > resolution, bits-per-pixel.
> >
> > OMAPFB expects panel driver to have check_timings and set_timings APIs.
> > These are checked by omapfb in case we wish to set default mode through
> bootargs.
> > e.g.: omapfb.mode="lcd:864x480-16" (display device:width X height - bits
> per pixel)
> >
> > omapfb_set_def_mode function in omapfb-main.c essentially needs these
> functions
> > otherwise it would return -EINVAL and default mode sent through bootargs
> > would be ignored.
> 
> I don't like this patch. You cannot change the timings for Taal, so
> those added functions look quite hacky.
> 
> The reason for this patch isn't clear from the description (it should).
> If I guess correctly, the point of the patch is to be able to change the
> default color format via boot arguments when using taal panel?

The intent of this patch was ability to change bits-per-pixel of framebuffer through bootargs.

> 
> If so, I think the change should be in the omapfb driver. Perhaps the
> omapfb driver should:
> 
> 1. check if check_timings & set_timings exist
> 2. if they do exist, do the same thing as the code does now
> 3. if they don't, use get_timings to verify that the given resolution is
> correct

But that would lead to a situation in which you would set the bits-per-pixel without checking if panel supports it.

> That wouldn't be perfect either, but I guess it should do the job. But
> this is again something where FB framework and OMAP HW do not quite
> match, and we end up with hacky solution, no matter what we do. But we
> can try to keep the hacks in the omapfb driver =).


OMAPFB depends a on the attached panel for configuring FB parameters.
panel-generic-dpi.c it has an API dpi_check_timings which checks whether timings are supported or not.
How about adding a similar API for dsi_check_timings? We would call it from panel-taal.
(I do understand that timings required by taal are only xres and yres much less compared to dpi)

> 
>  Tomi
> 

RE: [RESEND][PATCH v2] OMAP: DSS: Adding two APIs for panel-taal: check_timings and set_timings

[Tomi Valkeinen]

On Thu, 2011-02-17 at 02:50 -0600, Janorkar, Mayuresh wrote:
> Tomi,
> 

<snip>

> > If so, I think the change should be in the omapfb driver. Perhaps the
> > omapfb driver should:
> > 
> > 1. check if check_timings & set_timings exist
> > 2. if they do exist, do the same thing as the code does now
> > 3. if they don't, use get_timings to verify that the given resolution is
> > correct
> 
> But that would lead to a situation in which you would set the bits-per-pixel without checking if panel supports it.

You are mixing up the panel's color depth and the framebuffer's color
depth. The panel has a certain color depth, and that is not changed.

The fb parameter sets up the framebuffer color depth. DISPC will convert
that input color depth to 24bpp internally, and then again convert it to
16/18/24 bpp when outputting it to the panel. So fb and panel color
depths are independent.

> > That wouldn't be perfect either, but I guess it should do the job. But
> > this is again something where FB framework and OMAP HW do not quite
> > match, and we end up with hacky solution, no matter what we do. But we
> > can try to keep the hacks in the omapfb driver =).
> 
> 
> OMAPFB depends a on the attached panel for configuring FB parameters.
> panel-generic-dpi.c it has an API dpi_check_timings which checks whether timings are supported or not.
> How about adding a similar API for dsi_check_timings? We would call it from panel-taal.
> (I do understand that timings required by taal are only xres and yres much less compared to dpi)

For command mode panels the xres and yres are hardcoded, you cannot
change them. I don't see a reason to have a set or check function for
that. Omapfb should not try to change the values, if they cannot be
changed.

I think in this case omapfb should just check if the given xres/yres is
correct, and use the given bpp for the framebuffer.

 Tomi