diff mbox

[v2,2/2] video: imxfb: Add DT support

Message ID 1362504608-15839-3-git-send-email-mpa@pengutronix.de (mailing list archive)
State New, archived
Headers show

Commit Message

Markus Pargmann March 5, 2013, 5:30 p.m. UTC
Add devicetree support for imx framebuffer driver. It uses the generic
display bindings and helper functions.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
Cc: Fabio Estevam <festevam@gmail.com>
---

Notes:
    Changes in v2:
    - Removed pwmr register property
    - Cleanup of devicetree binding documentation
    - Use default values for pwmr and lscr1

 .../devicetree/bindings/video/fsl,imx-fb.txt       |  49 ++++++
 drivers/video/imxfb.c                              | 182 +++++++++++++++++----
 2 files changed, 197 insertions(+), 34 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/video/fsl,imx-fb.txt

Comments

Mark Rutland March 11, 2013, 10:25 a.m. UTC | #1
Hi,

Any reason for not CCing devicetree-discuss?

I have a couple of comments on the binding and the way it's parsed.

On Tue, Mar 05, 2013 at 05:30:08PM +0000, Markus Pargmann wrote:
> Add devicetree support for imx framebuffer driver. It uses the generic
> display bindings and helper functions.
>
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> Cc: Fabio Estevam <festevam@gmail.com>
> ---
>
> Notes:
>     Changes in v2:
>     - Removed pwmr register property
>     - Cleanup of devicetree binding documentation
>     - Use default values for pwmr and lscr1
>
>  .../devicetree/bindings/video/fsl,imx-fb.txt       |  49 ++++++
>  drivers/video/imxfb.c                              | 182 +++++++++++++++++----
>  2 files changed, 197 insertions(+), 34 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/video/fsl,imx-fb.txt
>
> diff --git a/Documentation/devicetree/bindings/video/fsl,imx-fb.txt b/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
> new file mode 100644
> index 0000000..e1a53a3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
> @@ -0,0 +1,49 @@
> +Freescale imx21 Framebuffer
> +
> +This framebuffer driver supports devices imx1, imx21, imx25, and imx27.
> +
> +Required properties:
> +- compatible : "fsl,<chip>-fb", chip should be imx1 or imx21
> +- reg : Should contain 1 register ranges(address and length)
> +- interrupts : One interrupt of the fb dev
> +
> +Required nodes:
> +- display: Phandle to a display node as described in
> +       Documentation/devicetree/bindings/video/display-timing.txt
> +       Additional, the display node has to define properties:
> +       - bpp: Bits per pixel
> +       - pcr: LCDC PCR value

As these are non-standard, it would be good to prefix them (e.g. "fsl,pcr").

If you need them, why are they not a good fit for the generic binding?

I'm not familiar with the hardware, what is the PCR exactly?

[...]

> -static int __init imxfb_probe(struct platform_device *pdev)
> +static int imxfb_of_read_mode(struct device_node *np,
> +               struct imx_fb_videomode *imxfb_mode)
> +{
> +       int ret;
> +       struct fb_videomode *of_mode = &imxfb_mode->mode;
> +       u32 bpp;
> +       u32 pcr;
> +
> +       ret = of_property_read_string(np, "model", &of_mode->name);
> +       if (ret)
> +               of_mode->name = NULL;
> +
> +       ret = of_get_fb_videomode(np, of_mode, OF_USE_NATIVE_MODE);
> +       if (ret)
> +               return ret;
> +
> +       ret = of_property_read_u32(np, "bpp", &bpp);
> +       ret |= of_property_read_u32(np, "pcr", &pcr);
> +
> +       if (ret)
> +               return ret;

Is this return value used anywhere in anything more than an "if (!err)"
capacity?  If so it may be worth having individual return value checks:

If one call returns -EINVAL (-22) and the other -ENODATA (-61), out the other
end we'd get -EISDIR (-21). If we don't care particularly about which error
code we actually pass on, we could always return a sensible code when ret is
nonzero:

if (ret)
	return -EINVAL;

> +
> +       if (bpp > 255)
> +               return -EINVAL;

Might it also be worth checking for 0 here?

[...]

> @@ -837,15 +914,51 @@ static int __init imxfb_probe(struct platform_device *pdev)
>
>         fbi = info->par;
>
> -       if (!fb_mode)
> -               fb_mode = pdata->mode[0].mode.name;
> -
>         platform_set_drvdata(pdev, info);
>
>         ret = imxfb_init_fbinfo(pdev);
>         if (ret < 0)
>                 goto failed_init;
>
> +       if (pdata) {
> +               if (!fb_mode)
> +                       fb_mode = pdata->mode[0].mode.name;
> +
> +               fbi->mode = pdata->mode;
> +               fbi->num_modes = pdata->num_modes;
> +       } else {
> +               struct device_node *display_np;
> +               fb_mode = NULL;
> +
> +               display_np = of_parse_phandle(pdev->dev.of_node, "display", 0);
> +               if (!display_np) {
> +                       dev_err(&pdev->dev, "No display defined in devicetree\n");
> +                       ret = -EINVAL;
> +                       goto failed_of_parse;
> +               }
> +
> +               /*
> +                * imxfb does not support more modes, we choose only the native
> +                * mode.
> +                */
> +               fbi->num_modes = 1;
> +
> +               fbi->mode = devm_kzalloc(&pdev->dev,
> +                               sizeof(struct imx_fb_videomode), GFP_KERNEL);
> +               if (!fbi->mode) {
> +                       ret = -ENOMEM;
> +                       goto failed_of_parse;
> +               }
> +
> +               ret = imxfb_of_read_mode(display_np, fbi->mode);
> +               if (ret)
> +                       goto failed_of_parse;
> +       }
> +
> +       for (i = 0, m = &fbi->mode[0]; i < fbi->num_modes; i++, m++)
> +               info->fix.smem_len = max_t(size_t, info->fix.smem_len,
> +                               m->mode.xres * m->mode.yres * m->bpp / 8);

Surely this is broken if bpp is not as multiple of 8?

If we can only handle multiples of 8, could we not sanity check this earlier?

If there's no strong preference for describing it in bits, could we not
describe it in bytes and side-step the issue?

[...]

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Markus Pargmann March 11, 2013, 7:39 p.m. UTC | #2
Hi,

On Mon, Mar 11, 2013 at 10:25:40AM +0000, Mark Rutland wrote:
> Hi,
> 
> Any reason for not CCing devicetree-discuss?

There is no reason, sorry, I forgot CC, I will add it to CC for the next
version.

> 
> I have a couple of comments on the binding and the way it's parsed.
> 
> On Tue, Mar 05, 2013 at 05:30:08PM +0000, Markus Pargmann wrote:
> > Add devicetree support for imx framebuffer driver. It uses the generic
> > display bindings and helper functions.
> >
> > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> > Cc: Fabio Estevam <festevam@gmail.com>
> > ---
> >
> > Notes:
> >     Changes in v2:
> >     - Removed pwmr register property
> >     - Cleanup of devicetree binding documentation
> >     - Use default values for pwmr and lscr1
> >
> >  .../devicetree/bindings/video/fsl,imx-fb.txt       |  49 ++++++
> >  drivers/video/imxfb.c                              | 182 +++++++++++++++++----
> >  2 files changed, 197 insertions(+), 34 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/video/fsl,imx-fb.txt
> >
> > diff --git a/Documentation/devicetree/bindings/video/fsl,imx-fb.txt b/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
> > new file mode 100644
> > index 0000000..e1a53a3
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
> > @@ -0,0 +1,49 @@
> > +Freescale imx21 Framebuffer
> > +
> > +This framebuffer driver supports devices imx1, imx21, imx25, and imx27.
> > +
> > +Required properties:
> > +- compatible : "fsl,<chip>-fb", chip should be imx1 or imx21
> > +- reg : Should contain 1 register ranges(address and length)
> > +- interrupts : One interrupt of the fb dev
> > +
> > +Required nodes:
> > +- display: Phandle to a display node as described in
> > +       Documentation/devicetree/bindings/video/display-timing.txt
> > +       Additional, the display node has to define properties:
> > +       - bpp: Bits per pixel
> > +       - pcr: LCDC PCR value
> 
> As these are non-standard, it would be good to prefix them (e.g. "fsl,pcr").

Okay.

> If you need them, why are they not a good fit for the generic binding?

I think bpp could be used by some other drivers but not of the majority.
There are actually already some of them having bindings for bpp, e.g.
Documentation/devicetree/bindings/video/via,vt8500-fb.txt .

> 
> I'm not familiar with the hardware, what is the PCR exactly?

PCR is an integer that encodes a lot of bools to specify the behavior of
the imxfb-lcd interaction. The alternative would be a lot of optional
bool properties which are parsed in the driver to construct it that way.

> 
> [...]
> 
> > -static int __init imxfb_probe(struct platform_device *pdev)
> > +static int imxfb_of_read_mode(struct device_node *np,
> > +               struct imx_fb_videomode *imxfb_mode)
> > +{
> > +       int ret;
> > +       struct fb_videomode *of_mode = &imxfb_mode->mode;
> > +       u32 bpp;
> > +       u32 pcr;
> > +
> > +       ret = of_property_read_string(np, "model", &of_mode->name);
> > +       if (ret)
> > +               of_mode->name = NULL;
> > +
> > +       ret = of_get_fb_videomode(np, of_mode, OF_USE_NATIVE_MODE);
> > +       if (ret)
> > +               return ret;
> > +
> > +       ret = of_property_read_u32(np, "bpp", &bpp);
> > +       ret |= of_property_read_u32(np, "pcr", &pcr);
> > +
> > +       if (ret)
> > +               return ret;
> 
> Is this return value used anywhere in anything more than an "if (!err)"
> capacity?  If so it may be worth having individual return value checks:
> 
> If one call returns -EINVAL (-22) and the other -ENODATA (-61), out the other
> end we'd get -EISDIR (-21). If we don't care particularly about which error
> code we actually pass on, we could always return a sensible code when ret is
> nonzero:
> 
> if (ret)
> 	return -EINVAL;

Yes, the error codes are directly passed through the probe function,
so I will change it to return -EINVAL and print an device error message.

> 
> > +
> > +       if (bpp > 255)
> > +               return -EINVAL;
> 
> Might it also be worth checking for 0 here?

Yes, changed.

> 
> [...]
> 
> > @@ -837,15 +914,51 @@ static int __init imxfb_probe(struct platform_device *pdev)
> >
> >         fbi = info->par;
> >
> > -       if (!fb_mode)
> > -               fb_mode = pdata->mode[0].mode.name;
> > -
> >         platform_set_drvdata(pdev, info);
> >
> >         ret = imxfb_init_fbinfo(pdev);
> >         if (ret < 0)
> >                 goto failed_init;
> >
> > +       if (pdata) {
> > +               if (!fb_mode)
> > +                       fb_mode = pdata->mode[0].mode.name;
> > +
> > +               fbi->mode = pdata->mode;
> > +               fbi->num_modes = pdata->num_modes;
> > +       } else {
> > +               struct device_node *display_np;
> > +               fb_mode = NULL;
> > +
> > +               display_np = of_parse_phandle(pdev->dev.of_node, "display", 0);
> > +               if (!display_np) {
> > +                       dev_err(&pdev->dev, "No display defined in devicetree\n");
> > +                       ret = -EINVAL;
> > +                       goto failed_of_parse;
> > +               }
> > +
> > +               /*
> > +                * imxfb does not support more modes, we choose only the native
> > +                * mode.
> > +                */
> > +               fbi->num_modes = 1;
> > +
> > +               fbi->mode = devm_kzalloc(&pdev->dev,
> > +                               sizeof(struct imx_fb_videomode), GFP_KERNEL);
> > +               if (!fbi->mode) {
> > +                       ret = -ENOMEM;
> > +                       goto failed_of_parse;
> > +               }
> > +
> > +               ret = imxfb_of_read_mode(display_np, fbi->mode);
> > +               if (ret)
> > +                       goto failed_of_parse;
> > +       }
> > +
> > +       for (i = 0, m = &fbi->mode[0]; i < fbi->num_modes; i++, m++)
> > +               info->fix.smem_len = max_t(size_t, info->fix.smem_len,
> > +                               m->mode.xres * m->mode.yres * m->bpp / 8);
> 
> Surely this is broken if bpp is not as multiple of 8?
> 
> If we can only handle multiples of 8, could we not sanity check this earlier?
> 
> If there's no strong preference for describing it in bits, could we not
> describe it in bytes and side-step the issue?

I think it is more common using bits per pixel. Indeed the for loop
seems to be broken. I fixed it by calculating the maximum bytes used per
pixel before. A grep through the kernel shows that there seem to be some
displays using bpp that are not a multiple of 8.

Thanks for your comments,

Markus
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/video/fsl,imx-fb.txt b/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
new file mode 100644
index 0000000..e1a53a3
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
@@ -0,0 +1,49 @@ 
+Freescale imx21 Framebuffer
+
+This framebuffer driver supports devices imx1, imx21, imx25, and imx27.
+
+Required properties:
+- compatible : "fsl,<chip>-fb", chip should be imx1 or imx21
+- reg : Should contain 1 register ranges(address and length)
+- interrupts : One interrupt of the fb dev
+
+Required nodes:
+- display: Phandle to a display node as described in
+	Documentation/devicetree/bindings/video/display-timing.txt
+	Additional, the display node has to define properties:
+	- bpp: Bits per pixel
+	- pcr: LCDC PCR value
+
+Optional properties:
+- dmacr-eukrea: Should be set for eukrea boards.
+
+Example:
+
+	imxfb: fb@10021000 {
+		compatible = "fsl,imx27-fb", "fsl,imx21-fb";
+		interrupts = <61>;
+		reg = <0x10021000 0x1000>;
+		display = <&display0>;
+	};
+
+	...
+
+	display0: display0 {
+		model = "Primeview-PD050VL1";
+		native-mode = <&timing_disp0>;
+		bpp = <16>;		/* non-standard but required */
+		pcr = <0xf0c88080>;	/* non-standard but required */
+		display-timings {
+			timing_disp0: 640x480 {
+				hactive = <640>;
+				vactive = <480>;
+				hback-porch = <112>;
+				hfront-porch = <36>;
+				hsync-len = <32>;
+				vback-porch = <33>;
+				vfront-porch = <33>;
+				vsync-len = <2>;
+				clock-frequency = <25000000>;
+			};
+		};
+	};
diff --git a/drivers/video/imxfb.c b/drivers/video/imxfb.c
index ef2b587..be784ed 100644
--- a/drivers/video/imxfb.c
+++ b/drivers/video/imxfb.c
@@ -32,6 +32,12 @@ 
 #include <linux/io.h>
 #include <linux/math64.h>
 #include <linux/uaccess.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+
+#include <video/of_display_timing.h>
+#include <video/of_videomode.h>
+#include <video/videomode.h>
 
 #include <linux/platform_data/video-imxfb.h>
 
@@ -117,10 +123,13 @@ 
 #define LGWCR_GWAV(alpha)	(((alpha) & 0xff) << 24)
 #define LGWCR_GWE	(1 << 22)
 
+#define IMXFB_LSCR1_DEFAULT 0x00120300
+#define IMXFB_DMACR_DEFAULT 0x00020010
+#define IMXFB_DMACR_EUKREA_DEFAULT 0x00040060
+
 /* Used fb-mode. Can be set on kernel command line, therefore file-static. */
 static const char *fb_mode;
 
-
 /*
  * These are the bitfields for each
  * display depth that we support.
@@ -192,6 +201,19 @@  static struct platform_device_id imxfb_devtype[] = {
 };
 MODULE_DEVICE_TABLE(platform, imxfb_devtype);
 
+static struct of_device_id imxfb_of_dev_id[] = {
+	{
+		.compatible = "fsl,imx1-fb",
+		.data = &imxfb_devtype[IMX1_FB],
+	}, {
+		.compatible = "fsl,imx21-fb",
+		.data = &imxfb_devtype[IMX21_FB],
+	}, {
+		/* sentinel */
+	}
+};
+MODULE_DEVICE_TABLE(of, imxfb_of_dev_id);
+
 static inline int is_imx1_fb(struct imxfb_info *fbi)
 {
 	return fbi->devtype == IMX1_FB;
@@ -324,6 +346,9 @@  static const struct imx_fb_videomode *imxfb_find_mode(struct imxfb_info *fbi)
 	struct imx_fb_videomode *m;
 	int i;
 
+	if (!fb_mode)
+		return &fbi->mode[0];
+
 	for (i = 0, m = &fbi->mode[0]; i < fbi->num_modes; i++, m++) {
 		if (!strcmp(m->mode.name, fb_mode))
 			return m;
@@ -479,6 +504,9 @@  static int imxfb_bl_update_status(struct backlight_device *bl)
 	struct imxfb_info *fbi = bl_get_data(bl);
 	int brightness = bl->props.brightness;
 
+	if (!fbi->pwmr)
+		return 0;
+
 	if (bl->props.power != FB_BLANK_UNBLANK)
 		brightness = 0;
 	if (bl->props.fb_blank != FB_BLANK_UNBLANK)
@@ -719,7 +747,8 @@  static int imxfb_activate_var(struct fb_var_screeninfo *var, struct fb_info *inf
 
 	writel(fbi->pcr, fbi->regs + LCDC_PCR);
 #ifndef PWMR_BACKLIGHT_AVAILABLE
-	writel(fbi->pwmr, fbi->regs + LCDC_PWMR);
+	if (fbi->pwmr)
+		writel(fbi->pwmr, fbi->regs + LCDC_PWMR);
 #endif
 	writel(fbi->lscr1, fbi->regs + LCDC_LSCR1);
 	writel(fbi->dmacr, fbi->regs + LCDC_DMACR);
@@ -758,13 +787,13 @@  static int imxfb_resume(struct platform_device *dev)
 #define imxfb_resume	NULL
 #endif
 
-static int __init imxfb_init_fbinfo(struct platform_device *pdev)
+static int imxfb_init_fbinfo(struct platform_device *pdev)
 {
 	struct imx_fb_platform_data *pdata = pdev->dev.platform_data;
 	struct fb_info *info = dev_get_drvdata(&pdev->dev);
 	struct imxfb_info *fbi = info->par;
-	struct imx_fb_videomode *m;
-	int i;
+	struct device_node *np;
+	int ret;
 
 	pr_debug("%s\n",__func__);
 
@@ -795,41 +824,89 @@  static int __init imxfb_init_fbinfo(struct platform_device *pdev)
 	info->fbops			= &imxfb_ops;
 	info->flags			= FBINFO_FLAG_DEFAULT |
 					  FBINFO_READS_FAST;
-	info->var.grayscale		= pdata->cmap_greyscale;
-	fbi->cmap_inverse		= pdata->cmap_inverse;
-	fbi->cmap_static		= pdata->cmap_static;
-	fbi->lscr1			= pdata->lscr1;
-	fbi->dmacr			= pdata->dmacr;
-	fbi->pwmr			= pdata->pwmr;
-	fbi->lcd_power			= pdata->lcd_power;
-	fbi->backlight_power		= pdata->backlight_power;
-
-	for (i = 0, m = &pdata->mode[0]; i < pdata->num_modes; i++, m++)
-		info->fix.smem_len = max_t(size_t, info->fix.smem_len,
-				m->mode.xres * m->mode.yres * m->bpp / 8);
+	if (pdata) {
+		info->var.grayscale		= pdata->cmap_greyscale;
+		fbi->cmap_inverse		= pdata->cmap_inverse;
+		fbi->cmap_static		= pdata->cmap_static;
+		fbi->lscr1			= pdata->lscr1;
+		fbi->dmacr			= pdata->dmacr;
+		fbi->pwmr			= pdata->pwmr;
+		fbi->lcd_power			= pdata->lcd_power;
+		fbi->backlight_power		= pdata->backlight_power;
+	} else {
+		np = pdev->dev.of_node;
+		info->var.grayscale = of_property_read_bool(np,
+						"cmap-greyscale");
+		fbi->cmap_inverse = of_property_read_bool(np, "cmap-inverse");
+		fbi->cmap_static = of_property_read_bool(np, "cmap-static");
+
+		fbi->lscr1 = IMXFB_LSCR1_DEFAULT;
+		if (of_property_read_bool(np, "dmacr-eukrea"))
+			fbi->dmacr = IMXFB_DMACR_EUKREA_DEFAULT;
+		else
+			fbi->dmacr = IMXFB_DMACR_DEFAULT;
+
+		/* These two function pointers could be used by some specific
+		 * platforms. */
+		fbi->lcd_power = NULL;
+		fbi->backlight_power = NULL;
+	}
 
 	return 0;
 }
 
-static int __init imxfb_probe(struct platform_device *pdev)
+static int imxfb_of_read_mode(struct device_node *np,
+		struct imx_fb_videomode *imxfb_mode)
+{
+	int ret;
+	struct fb_videomode *of_mode = &imxfb_mode->mode;
+	u32 bpp;
+	u32 pcr;
+
+	ret = of_property_read_string(np, "model", &of_mode->name);
+	if (ret)
+		of_mode->name = NULL;
+
+	ret = of_get_fb_videomode(np, of_mode, OF_USE_NATIVE_MODE);
+	if (ret)
+		return ret;
+
+	ret = of_property_read_u32(np, "bpp", &bpp);
+	ret |= of_property_read_u32(np, "pcr", &pcr);
+
+	if (ret)
+		return ret;
+
+	if (bpp > 255)
+		return -EINVAL;
+
+	imxfb_mode->bpp = bpp;
+	imxfb_mode->pcr = pcr;
+
+	return ret;
+}
+
+static int imxfb_probe(struct platform_device *pdev)
 {
 	struct imxfb_info *fbi;
 	struct fb_info *info;
 	struct imx_fb_platform_data *pdata;
 	struct resource *res;
+	struct imx_fb_videomode *m;
+	const struct of_device_id *of_id;
 	int ret, i;
 
 	dev_info(&pdev->dev, "i.MX Framebuffer driver\n");
 
+	of_id = of_match_device(imxfb_of_dev_id, &pdev->dev);
+	if (of_id)
+		pdev->id_entry = of_id->data;
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res)
 		return -ENODEV;
 
 	pdata = pdev->dev.platform_data;
-	if (!pdata) {
-		dev_err(&pdev->dev,"No platform_data available\n");
-		return -ENOMEM;
-	}
 
 	info = framebuffer_alloc(sizeof(struct imxfb_info), &pdev->dev);
 	if (!info)
@@ -837,15 +914,51 @@  static int __init imxfb_probe(struct platform_device *pdev)
 
 	fbi = info->par;
 
-	if (!fb_mode)
-		fb_mode = pdata->mode[0].mode.name;
-
 	platform_set_drvdata(pdev, info);
 
 	ret = imxfb_init_fbinfo(pdev);
 	if (ret < 0)
 		goto failed_init;
 
+	if (pdata) {
+		if (!fb_mode)
+			fb_mode = pdata->mode[0].mode.name;
+
+		fbi->mode = pdata->mode;
+		fbi->num_modes = pdata->num_modes;
+	} else {
+		struct device_node *display_np;
+		fb_mode = NULL;
+
+		display_np = of_parse_phandle(pdev->dev.of_node, "display", 0);
+		if (!display_np) {
+			dev_err(&pdev->dev, "No display defined in devicetree\n");
+			ret = -EINVAL;
+			goto failed_of_parse;
+		}
+
+		/*
+		 * imxfb does not support more modes, we choose only the native
+		 * mode.
+		 */
+		fbi->num_modes = 1;
+
+		fbi->mode = devm_kzalloc(&pdev->dev,
+				sizeof(struct imx_fb_videomode), GFP_KERNEL);
+		if (!fbi->mode) {
+			ret = -ENOMEM;
+			goto failed_of_parse;
+		}
+
+		ret = imxfb_of_read_mode(display_np, fbi->mode);
+		if (ret)
+			goto failed_of_parse;
+	}
+
+	for (i = 0, m = &fbi->mode[0]; i < fbi->num_modes; i++, m++)
+		info->fix.smem_len = max_t(size_t, info->fix.smem_len,
+				m->mode.xres * m->mode.yres * m->bpp / 8);
+
 	res = request_mem_region(res->start, resource_size(res),
 				DRIVER_NAME);
 	if (!res) {
@@ -878,7 +991,8 @@  static int __init imxfb_probe(struct platform_device *pdev)
 		goto failed_ioremap;
 	}
 
-	if (!pdata->fixed_screen_cpu) {
+	/* Seems not being used by anyone, so no support for oftree */
+	if (!pdata || !pdata->fixed_screen_cpu) {
 		fbi->map_size = PAGE_ALIGN(info->fix.smem_len);
 		fbi->map_cpu = dma_alloc_writecombine(&pdev->dev,
 				fbi->map_size, &fbi->map_dma, GFP_KERNEL);
@@ -903,18 +1017,16 @@  static int __init imxfb_probe(struct platform_device *pdev)
 		info->fix.smem_start = fbi->screen_dma;
 	}
 
-	if (pdata->init) {
+	if (pdata && pdata->init) {
 		ret = pdata->init(fbi->pdev);
 		if (ret)
 			goto failed_platform_init;
 	}
 
-	fbi->mode = pdata->mode;
-	fbi->num_modes = pdata->num_modes;
 
 	INIT_LIST_HEAD(&info->modelist);
-	for (i = 0; i < pdata->num_modes; i++)
-		fb_add_videomode(&pdata->mode[i].mode, &info->modelist);
+	for (i = 0; i < fbi->num_modes; i++)
+		fb_add_videomode(&fbi->mode[i].mode, &info->modelist);
 
 	/*
 	 * This makes sure that our colour bitfield
@@ -944,10 +1056,10 @@  static int __init imxfb_probe(struct platform_device *pdev)
 failed_register:
 	fb_dealloc_cmap(&info->cmap);
 failed_cmap:
-	if (pdata->exit)
+	if (pdata && pdata->exit)
 		pdata->exit(fbi->pdev);
 failed_platform_init:
-	if (!pdata->fixed_screen_cpu)
+	if (pdata && !pdata->fixed_screen_cpu)
 		dma_free_writecombine(&pdev->dev,fbi->map_size,fbi->map_cpu,
 			fbi->map_dma);
 failed_map:
@@ -956,6 +1068,7 @@  failed_ioremap:
 failed_getclock:
 	release_mem_region(res->start, resource_size(res));
 failed_req:
+failed_of_parse:
 	kfree(info->pseudo_palette);
 failed_init:
 	platform_set_drvdata(pdev, NULL);
@@ -980,7 +1093,7 @@  static int imxfb_remove(struct platform_device *pdev)
 	unregister_framebuffer(info);
 
 	pdata = pdev->dev.platform_data;
-	if (pdata->exit)
+	if (pdata && pdata->exit)
 		pdata->exit(fbi->pdev);
 
 	fb_dealloc_cmap(&info->cmap);
@@ -1009,6 +1122,7 @@  static struct platform_driver imxfb_driver = {
 	.shutdown	= imxfb_shutdown,
 	.driver		= {
 		.name	= DRIVER_NAME,
+		.of_match_table = imxfb_of_dev_id,
 	},
 	.id_table	= imxfb_devtype,
 };