* [PATCH v2 0/2] video: imxfb DT support
@ 2013-03-05 17:30 Markus Pargmann
2013-03-05 17:30 ` [PATCH v2 1/2] imxfb: Set alpha value of the framebuffer Markus Pargmann
2013-03-05 17:30 ` [PATCH v2 2/2] video: imxfb: Add DT support Markus Pargmann
0 siblings, 2 replies; 5+ messages in thread
From: Markus Pargmann @ 2013-03-05 17:30 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
version 2 of this patch for imx framebuffer driver DT support.
There are some changes of DT bindings and documentation as
described in notes of patch 2.
Regards,
Markus
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 1/2] imxfb: Set alpha value of the framebuffer
2013-03-05 17:30 [PATCH v2 0/2] video: imxfb DT support Markus Pargmann
@ 2013-03-05 17:30 ` Markus Pargmann
2013-03-05 17:30 ` [PATCH v2 2/2] video: imxfb: Add DT support Markus Pargmann
1 sibling, 0 replies; 5+ messages in thread
From: Markus Pargmann @ 2013-03-05 17:30 UTC (permalink / raw)
To: linux-arm-kernel
From: Christian Hemp <c.hemp@phytec.de>
Based on Sascha Hauer's patch i.MX27 clock: Do not disable lcd clocks during
startup.
This patch gives a interface to chance the alphavalue of the framebuffer.
Signed-off-by: Christian Hemp <c.hemp@phytec.de>
rebased to 3.7
Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
drivers/video/imxfb.c | 35 +++++++++++++++++++++++++++++++
include/linux/platform_data/video-imxfb.h | 3 +++
2 files changed, 38 insertions(+)
diff --git a/drivers/video/imxfb.c b/drivers/video/imxfb.c
index 0abf2bf..ef2b587 100644
--- a/drivers/video/imxfb.c
+++ b/drivers/video/imxfb.c
@@ -31,6 +31,7 @@
#include <linux/dma-mapping.h>
#include <linux/io.h>
#include <linux/math64.h>
+#include <linux/uaccess.h>
#include <linux/platform_data/video-imxfb.h>
@@ -112,6 +113,10 @@
#define LCDISR_EOF (1<<1)
#define LCDISR_BOF (1<<0)
+#define LCDC_LGWCR 0x64
+#define LGWCR_GWAV(alpha) (((alpha) & 0xff) << 24)
+#define LGWCR_GWE (1 << 22)
+
/* Used fb-mode. Can be set on kernel command line, therefore file-static. */
static const char *fb_mode;
@@ -610,6 +615,35 @@ static int imxfb_blank(int blank, struct fb_info *info)
return 0;
}
+static int imxfb_ioctl(struct fb_info *info, unsigned int cmd,
+ unsigned long arg)
+{
+ struct imxfb_info *fbi = info->par;
+ int alpha, ret = 0;
+ unsigned int tmp;
+
+ switch (cmd) {
+ case IMXFB_ALPHA:
+ if (get_user(alpha, (int __user *)arg)) {
+ ret = -EFAULT;
+ } else {
+ tmp = readl(fbi->regs + LCDC_LGWCR);
+ tmp &= ~LGWCR_GWAV(0xff);
+ tmp |= LGWCR_GWAV(alpha);
+ if (!alpha)
+ tmp &= ~LGWCR_GWE;
+ else
+ tmp |= LGWCR_GWE;
+ writel(tmp , fbi->regs + LCDC_LGWCR);
+ }
+ break;
+ default:
+ ret = -EINVAL;
+ }
+
+ return ret;
+}
+
static struct fb_ops imxfb_ops = {
.owner = THIS_MODULE,
.fb_check_var = imxfb_check_var,
@@ -619,6 +653,7 @@ static struct fb_ops imxfb_ops = {
.fb_copyarea = cfb_copyarea,
.fb_imageblit = cfb_imageblit,
.fb_blank = imxfb_blank,
+ .fb_ioctl = imxfb_ioctl,
};
/*
diff --git a/include/linux/platform_data/video-imxfb.h b/include/linux/platform_data/video-imxfb.h
index 9de8f06..ce3875f 100644
--- a/include/linux/platform_data/video-imxfb.h
+++ b/include/linux/platform_data/video-imxfb.h
@@ -51,6 +51,9 @@
#define DMACR_HM(x) (((x) & 0xf) << 16)
#define DMACR_TM(x) ((x) & 0xf)
+#define IMXFB_IOW(num, dtype) _IOW('I', num, dtype)
+#define IMXFB_ALPHA IMXFB_IOW(31, int)
+
struct imx_fb_videomode {
struct fb_videomode mode;
u32 pcr;
--
1.8.1.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v2 2/2] video: imxfb: Add DT support
2013-03-05 17:30 [PATCH v2 0/2] video: imxfb DT support Markus Pargmann
2013-03-05 17:30 ` [PATCH v2 1/2] imxfb: Set alpha value of the framebuffer Markus Pargmann
@ 2013-03-05 17:30 ` Markus Pargmann
2013-03-11 10:25 ` Mark Rutland
1 sibling, 1 reply; 5+ messages in thread
From: Markus Pargmann @ 2013-03-05 17:30 UTC (permalink / raw)
To: linux-arm-kernel
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
+
+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,
};
--
1.8.1.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2 2/2] video: imxfb: Add DT support
2013-03-05 17:30 ` [PATCH v2 2/2] video: imxfb: Add DT support Markus Pargmann
@ 2013-03-11 10:25 ` Mark Rutland
2013-03-11 19:39 ` Markus Pargmann
0 siblings, 1 reply; 5+ messages in thread
From: Mark Rutland @ 2013-03-11 10:25 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 2/2] video: imxfb: Add DT support
2013-03-11 10:25 ` Mark Rutland
@ 2013-03-11 19:39 ` Markus Pargmann
0 siblings, 0 replies; 5+ messages in thread
From: Markus Pargmann @ 2013-03-11 19:39 UTC (permalink / raw)
To: linux-arm-kernel
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
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-03-11 19:39 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-03-05 17:30 [PATCH v2 0/2] video: imxfb DT support Markus Pargmann
2013-03-05 17:30 ` [PATCH v2 1/2] imxfb: Set alpha value of the framebuffer Markus Pargmann
2013-03-05 17:30 ` [PATCH v2 2/2] video: imxfb: Add DT support Markus Pargmann
2013-03-11 10:25 ` Mark Rutland
2013-03-11 19:39 ` Markus Pargmann
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).