* [PATCH 4/6] xen-fbfront: handle backend CLOSED without CLOSING
From: David Vrabel @ 2012-09-21 16:04 UTC (permalink / raw)
To: xen-devel; +Cc: David Vrabel, Konrad Rzeszutek Wilk, linux-kernel, linux-fbdev
In-Reply-To: <1348243464-15903-1-git-send-email-david.vrabel@citrix.com>
From: David Vrabel <david.vrabel@citrix.com>
Backend drivers shouldn't transistion to CLOSED unless the frontend is
CLOSED. If a backend does transition to CLOSED too soon then the
frontend may not see the CLOSING state and will not properly shutdown.
So, treat an unexpected backend CLOSED state the same as CLOSING.
Signed-off-by: David Vrabel <david.vrabel@citrix.com>
Cc: <linux-fbdev@vger.kernel.org>
---
drivers/video/xen-fbfront.c | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/drivers/video/xen-fbfront.c b/drivers/video/xen-fbfront.c
index b7f5173..917bb56 100644
--- a/drivers/video/xen-fbfront.c
+++ b/drivers/video/xen-fbfront.c
@@ -641,7 +641,6 @@ static void xenfb_backend_changed(struct xenbus_device *dev,
case XenbusStateReconfiguring:
case XenbusStateReconfigured:
case XenbusStateUnknown:
- case XenbusStateClosed:
break;
case XenbusStateInitWait:
@@ -670,6 +669,10 @@ InitWait:
info->feature_resize = val;
break;
+ case XenbusStateClosed:
+ if (dev->state = XenbusStateClosed)
+ break;
+ /* Missed the backend's CLOSING state -- fallthrough */
case XenbusStateClosing:
xenbus_frontend_closed(dev);
break;
--
1.7.2.5
^ permalink raw reply related
* [PATCH v2] da8xx-fb: add pm_runtime support
From: Manjunathappa, Prakash @ 2012-09-21 15:51 UTC (permalink / raw)
To: linux-fbdev
Add pm_runtime support to the da8xx-fb frame buffer driver.
Signed-off-by: Manjunathappa, Prakash <prakash.pm@ti.com>
---
Applies on top of patch "da8xx-fb: enable LCDC if FB is unblanked" under review.
Since v1:
Use pm_runtime_put_sync in PM suspend callback.
drivers/video/da8xx-fb.c | 27 +++++++++++++--------------
1 files changed, 13 insertions(+), 14 deletions(-)
diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index af2aa38..4280d23 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -26,6 +26,7 @@
#include <linux/device.h>
#include <linux/platform_device.h>
#include <linux/uaccess.h>
+#include <linux/pm_runtime.h>
#include <linux/interrupt.h>
#include <linux/wait.h>
#include <linux/clk.h>
@@ -1037,8 +1038,8 @@ static int __devexit fb_remove(struct platform_device *dev)
dma_free_coherent(NULL, par->vram_size, par->vram_virt,
par->vram_phys);
free_irq(par->irq, par);
- clk_disable(par->lcdc_clk);
- clk_put(par->lcdc_clk);
+ pm_runtime_put_sync(&dev->dev);
+ pm_runtime_disable(&dev->dev);
framebuffer_release(info);
iounmap((void __iomem *)da8xx_fb_reg_base);
release_mem_region(lcdc_regs->start, resource_size(lcdc_regs));
@@ -1272,9 +1273,9 @@ static int __devinit fb_probe(struct platform_device *device)
ret = -ENODEV;
goto err_ioremap;
}
- ret = clk_enable(fb_clk);
- if (ret)
- goto err_clk_put;
+
+ pm_runtime_enable(&device->dev);
+ pm_runtime_get_sync(&device->dev);
/* Determine LCD IP Version */
switch (lcdc_read(LCD_PID_REG)) {
@@ -1302,7 +1303,7 @@ static int __devinit fb_probe(struct platform_device *device)
if (i = ARRAY_SIZE(known_lcd_panels)) {
dev_err(&device->dev, "GLCD: No valid panel found\n");
ret = -ENODEV;
- goto err_clk_disable;
+ goto err_pm_runtime_disable;
} else
dev_info(&device->dev, "GLCD: Found %s panel\n",
fb_pdata->type);
@@ -1314,7 +1315,7 @@ static int __devinit fb_probe(struct platform_device *device)
if (!da8xx_fb_info) {
dev_dbg(&device->dev, "Memory allocation failed for fb_info\n");
ret = -ENOMEM;
- goto err_clk_disable;
+ goto err_pm_runtime_disable;
}
par = da8xx_fb_info->par;
@@ -1476,11 +1477,9 @@ err_release_fb_mem:
err_release_fb:
framebuffer_release(da8xx_fb_info);
-err_clk_disable:
- clk_disable(fb_clk);
-
-err_clk_put:
- clk_put(fb_clk);
+err_pm_runtime_disable:
+ pm_runtime_put_sync(&device->dev);
+ pm_runtime_disable(&device->dev);
err_ioremap:
iounmap((void __iomem *)da8xx_fb_reg_base);
@@ -1503,7 +1502,7 @@ static int fb_suspend(struct platform_device *dev, pm_message_t state)
fb_set_suspend(info, 1);
lcd_disable_raster(true);
- clk_disable(par->lcdc_clk);
+ pm_runtime_put_sync(&dev->dev);
console_unlock();
return 0;
@@ -1514,7 +1513,7 @@ static int fb_resume(struct platform_device *dev)
struct da8xx_fb_par *par = info->par;
console_lock();
- clk_enable(par->lcdc_clk);
+ pm_runtime_get_sync(&dev->dev);
if (par->blank = FB_BLANK_UNBLANK) {
lcd_enable_raster();
--
1.7.0.4
^ permalink raw reply related
* RE: [PATCH] da8xx-fb: add pm_runtime support
From: Manjunathappa, Prakash @ 2012-09-21 15:45 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <79CD15C6BA57404B839C016229A409A83EABE8E3@DBDE01.ent.ti.com>
Hi Vaibhav,
On Fri, Sep 21, 2012 at 21:03:15, Hiremath, Vaibhav wrote:
> On Fri, Sep 21, 2012 at 20:54:48, Manjunathappa, Prakash wrote:
> > Add pm_runtime support to the da8xx-fb frame buffer driver.
> >
> > Signed-off-by: Manjunathappa, Prakash <prakash.pm@ti.com>
> > ---
> > Applies on top of patch "da8xx-fb: enable LCDC if FB is unblanked" under review.
> >
> > drivers/video/da8xx-fb.c | 24 +++++++++++++-----------
> > 1 files changed, 13 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
> > index af2aa38..17c00b2 100644
> > --- a/drivers/video/da8xx-fb.c
> > +++ b/drivers/video/da8xx-fb.c
> > @@ -26,6 +26,7 @@
> > #include <linux/device.h>
> > #include <linux/platform_device.h>
> > #include <linux/uaccess.h>
> > +#include <linux/pm_runtime.h>
> > #include <linux/interrupt.h>
> > #include <linux/wait.h>
> > #include <linux/clk.h>
> > @@ -1037,8 +1038,8 @@ static int __devexit fb_remove(struct platform_device *dev)
> > dma_free_coherent(NULL, par->vram_size, par->vram_virt,
> > par->vram_phys);
> > free_irq(par->irq, par);
> > - clk_disable(par->lcdc_clk);
> > - clk_put(par->lcdc_clk);
> > + pm_runtime_put_sync(&dev->dev);
> > + pm_runtime_disable(&dev->dev);
> > framebuffer_release(info);
> > iounmap((void __iomem *)da8xx_fb_reg_base);
> > release_mem_region(lcdc_regs->start, resource_size(lcdc_regs));
> > @@ -1272,9 +1273,9 @@ static int __devinit fb_probe(struct platform_device *device)
> > ret = -ENODEV;
> > goto err_ioremap;
> > }
> > - ret = clk_enable(fb_clk);
> > - if (ret)
> > - goto err_clk_put;
> > +
> > + pm_runtime_enable(&device->dev);
> > + pm_runtime_get_sync(&device->dev);
> >
> > /* Determine LCD IP Version */
> > switch (lcdc_read(LCD_PID_REG)) {
> > @@ -1302,7 +1303,7 @@ static int __devinit fb_probe(struct platform_device *device)
> > if (i = ARRAY_SIZE(known_lcd_panels)) {
> > dev_err(&device->dev, "GLCD: No valid panel found\n");
> > ret = -ENODEV;
> > - goto err_clk_disable;
> > + goto err_pm_runtime_disable;
> > } else
> > dev_info(&device->dev, "GLCD: Found %s panel\n",
> > fb_pdata->type);
> > @@ -1314,7 +1315,7 @@ static int __devinit fb_probe(struct platform_device *device)
> > if (!da8xx_fb_info) {
> > dev_dbg(&device->dev, "Memory allocation failed for fb_info\n");
> > ret = -ENOMEM;
> > - goto err_clk_disable;
> > + goto err_pm_runtime_disable;
> > }
> >
> > par = da8xx_fb_info->par;
> > @@ -1476,8 +1477,9 @@ err_release_fb_mem:
> > err_release_fb:
> > framebuffer_release(da8xx_fb_info);
> >
> > -err_clk_disable:
> > - clk_disable(fb_clk);
> > +err_pm_runtime_disable:
> > + pm_runtime_put_sync(&device->dev);
> > + pm_runtime_disable(&device->dev);
> >
> > err_clk_put:
> > clk_put(fb_clk);
> > @@ -1503,7 +1505,7 @@ static int fb_suspend(struct platform_device *dev, pm_message_t state)
> >
> > fb_set_suspend(info, 1);
> > lcd_disable_raster(true);
> > - clk_disable(par->lcdc_clk);
> > + pm_runtime_put(&dev->dev);
>
> Isn't here you should use pm_runtime_put_sync()??
>
Yes, I will correct it send next version.
Thanks,
Prakash
> Thanks,
> Vaibhv
>
> > console_unlock();
> >
> > return 0;
> > @@ -1514,7 +1516,7 @@ static int fb_resume(struct platform_device *dev)
> > struct da8xx_fb_par *par = info->par;
> >
> > console_lock();
> > - clk_enable(par->lcdc_clk);
> > + pm_runtime_get_sync(&dev->dev);
> > if (par->blank = FB_BLANK_UNBLANK) {
> > lcd_enable_raster();
> >
> > --
> > 1.7.0.4
> >
> > _______________________________________________
> > Davinci-linux-open-source mailing list
> > Davinci-linux-open-source@linux.davincidsp.com
> > http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
> >
>
>
^ permalink raw reply
* [PATCH] da8xx-fb: add pm_runtime support
From: Manjunathappa, Prakash @ 2012-09-21 15:36 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <79CD15C6BA57404B839C016229A409A83EABE8E3@DBDE01.ent.ti.com>
Add pm_runtime support to the da8xx-fb frame buffer driver.
Signed-off-by: Manjunathappa, Prakash <prakash.pm@ti.com>
---
Applies on top of patch "da8xx-fb: enable LCDC if FB is unblanked" under review.
drivers/video/da8xx-fb.c | 24 +++++++++++++-----------
1 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index af2aa38..17c00b2 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -26,6 +26,7 @@
#include <linux/device.h>
#include <linux/platform_device.h>
#include <linux/uaccess.h>
+#include <linux/pm_runtime.h>
#include <linux/interrupt.h>
#include <linux/wait.h>
#include <linux/clk.h>
@@ -1037,8 +1038,8 @@ static int __devexit fb_remove(struct platform_device *dev)
dma_free_coherent(NULL, par->vram_size, par->vram_virt,
par->vram_phys);
free_irq(par->irq, par);
- clk_disable(par->lcdc_clk);
- clk_put(par->lcdc_clk);
+ pm_runtime_put_sync(&dev->dev);
+ pm_runtime_disable(&dev->dev);
framebuffer_release(info);
iounmap((void __iomem *)da8xx_fb_reg_base);
release_mem_region(lcdc_regs->start, resource_size(lcdc_regs));
@@ -1272,9 +1273,9 @@ static int __devinit fb_probe(struct platform_device *device)
ret = -ENODEV;
goto err_ioremap;
}
- ret = clk_enable(fb_clk);
- if (ret)
- goto err_clk_put;
+
+ pm_runtime_enable(&device->dev);
+ pm_runtime_get_sync(&device->dev);
/* Determine LCD IP Version */
switch (lcdc_read(LCD_PID_REG)) {
@@ -1302,7 +1303,7 @@ static int __devinit fb_probe(struct platform_device *device)
if (i = ARRAY_SIZE(known_lcd_panels)) {
dev_err(&device->dev, "GLCD: No valid panel found\n");
ret = -ENODEV;
- goto err_clk_disable;
+ goto err_pm_runtime_disable;
} else
dev_info(&device->dev, "GLCD: Found %s panel\n",
fb_pdata->type);
@@ -1314,7 +1315,7 @@ static int __devinit fb_probe(struct platform_device *device)
if (!da8xx_fb_info) {
dev_dbg(&device->dev, "Memory allocation failed for fb_info\n");
ret = -ENOMEM;
- goto err_clk_disable;
+ goto err_pm_runtime_disable;
}
par = da8xx_fb_info->par;
@@ -1476,8 +1477,9 @@ err_release_fb_mem:
err_release_fb:
framebuffer_release(da8xx_fb_info);
-err_clk_disable:
- clk_disable(fb_clk);
+err_pm_runtime_disable:
+ pm_runtime_put_sync(&device->dev);
+ pm_runtime_disable(&device->dev);
err_clk_put:
clk_put(fb_clk);
@@ -1503,7 +1505,7 @@ static int fb_suspend(struct platform_device *dev, pm_message_t state)
fb_set_suspend(info, 1);
lcd_disable_raster(true);
- clk_disable(par->lcdc_clk);
+ pm_runtime_put(&dev->dev);
console_unlock();
return 0;
@@ -1514,7 +1516,7 @@ static int fb_resume(struct platform_device *dev)
struct da8xx_fb_par *par = info->par;
console_lock();
- clk_enable(par->lcdc_clk);
+ pm_runtime_get_sync(&dev->dev);
if (par->blank = FB_BLANK_UNBLANK) {
lcd_enable_raster();
--
1.7.0.4
^ permalink raw reply related
* RE: [PATCH] da8xx-fb: add pm_runtime support
From: Hiremath, Vaibhav @ 2012-09-21 15:33 UTC (permalink / raw)
To: linux-fbdev
On Fri, Sep 21, 2012 at 20:54:48, Manjunathappa, Prakash wrote:
> Add pm_runtime support to the da8xx-fb frame buffer driver.
>
> Signed-off-by: Manjunathappa, Prakash <prakash.pm@ti.com>
> ---
> Applies on top of patch "da8xx-fb: enable LCDC if FB is unblanked" under review.
>
> drivers/video/da8xx-fb.c | 24 +++++++++++++-----------
> 1 files changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
> index af2aa38..17c00b2 100644
> --- a/drivers/video/da8xx-fb.c
> +++ b/drivers/video/da8xx-fb.c
> @@ -26,6 +26,7 @@
> #include <linux/device.h>
> #include <linux/platform_device.h>
> #include <linux/uaccess.h>
> +#include <linux/pm_runtime.h>
> #include <linux/interrupt.h>
> #include <linux/wait.h>
> #include <linux/clk.h>
> @@ -1037,8 +1038,8 @@ static int __devexit fb_remove(struct platform_device *dev)
> dma_free_coherent(NULL, par->vram_size, par->vram_virt,
> par->vram_phys);
> free_irq(par->irq, par);
> - clk_disable(par->lcdc_clk);
> - clk_put(par->lcdc_clk);
> + pm_runtime_put_sync(&dev->dev);
> + pm_runtime_disable(&dev->dev);
> framebuffer_release(info);
> iounmap((void __iomem *)da8xx_fb_reg_base);
> release_mem_region(lcdc_regs->start, resource_size(lcdc_regs));
> @@ -1272,9 +1273,9 @@ static int __devinit fb_probe(struct platform_device *device)
> ret = -ENODEV;
> goto err_ioremap;
> }
> - ret = clk_enable(fb_clk);
> - if (ret)
> - goto err_clk_put;
> +
> + pm_runtime_enable(&device->dev);
> + pm_runtime_get_sync(&device->dev);
>
> /* Determine LCD IP Version */
> switch (lcdc_read(LCD_PID_REG)) {
> @@ -1302,7 +1303,7 @@ static int __devinit fb_probe(struct platform_device *device)
> if (i = ARRAY_SIZE(known_lcd_panels)) {
> dev_err(&device->dev, "GLCD: No valid panel found\n");
> ret = -ENODEV;
> - goto err_clk_disable;
> + goto err_pm_runtime_disable;
> } else
> dev_info(&device->dev, "GLCD: Found %s panel\n",
> fb_pdata->type);
> @@ -1314,7 +1315,7 @@ static int __devinit fb_probe(struct platform_device *device)
> if (!da8xx_fb_info) {
> dev_dbg(&device->dev, "Memory allocation failed for fb_info\n");
> ret = -ENOMEM;
> - goto err_clk_disable;
> + goto err_pm_runtime_disable;
> }
>
> par = da8xx_fb_info->par;
> @@ -1476,8 +1477,9 @@ err_release_fb_mem:
> err_release_fb:
> framebuffer_release(da8xx_fb_info);
>
> -err_clk_disable:
> - clk_disable(fb_clk);
> +err_pm_runtime_disable:
> + pm_runtime_put_sync(&device->dev);
> + pm_runtime_disable(&device->dev);
>
> err_clk_put:
> clk_put(fb_clk);
> @@ -1503,7 +1505,7 @@ static int fb_suspend(struct platform_device *dev, pm_message_t state)
>
> fb_set_suspend(info, 1);
> lcd_disable_raster(true);
> - clk_disable(par->lcdc_clk);
> + pm_runtime_put(&dev->dev);
Isn't here you should use pm_runtime_put_sync()??
Thanks,
Vaibhv
> console_unlock();
>
> return 0;
> @@ -1514,7 +1516,7 @@ static int fb_resume(struct platform_device *dev)
> struct da8xx_fb_par *par = info->par;
>
> console_lock();
> - clk_enable(par->lcdc_clk);
> + pm_runtime_get_sync(&dev->dev);
> if (par->blank = FB_BLANK_UNBLANK) {
> lcd_enable_raster();
>
> --
> 1.7.0.4
>
> _______________________________________________
> Davinci-linux-open-source mailing list
> Davinci-linux-open-source@linux.davincidsp.com
> http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
>
^ permalink raw reply
* Re: [PATCH 1/2] video: Add support for the Solomon SSD1307 OLED Controller
From: Maxime Ripard @ 2012-09-21 14:04 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1348158800-23039-2-git-send-email-maxime.ripard@free-electrons.com>
Hi Florian,
Le 20/09/2012 18:33, Maxime Ripard a écrit :
> This patch adds support for the Solomon SSD1307 OLED
> controller found on the Crystalfontz CFA10036 board.
>
> This controller can drive a display with a resolution up
> to 128x39 and can operate over I2C or SPI.
>
> The current driver has only been tested on the CFA-10036,
> that is using this controller over I2C to driver a 96x16
> OLED screen.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> Cc: Brian Lilly <brian@crystalfontz.com>
Would you consider merging this patch or review it?
It has been around for quite some time now and has not received any
comments for at least the 2 last versions, so I guess there is not much
left holding it back (unless, of course, if you have some comments to
address), and I really would like to have it in 3.7.
Thanks,
Maxime
--
Maxime Ripard, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
^ permalink raw reply
* Re: [PATCH v4] of: Add videomode helper
From: Hans Verkuil @ 2012-09-21 13:07 UTC (permalink / raw)
To: dri-devel
Cc: devicetree-discuss, Steffen Trumtrar, linux-fbdev, kernel,
Laurent Pinchart
In-Reply-To: <1348042843-24673-1-git-send-email-s.trumtrar@pengutronix.de>
On Wed September 19 2012 10:20:43 Steffen Trumtrar wrote:
> This patch adds a helper function for parsing videomodes from the devicetree.
> The videomode can be either converted to a struct drm_display_mode or a
> struct fb_videomode.
>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> ---
>
> Hi!
>
> changes since v3:
> - print error messages
> - free alloced memory
> - general cleanup
>
> Regards
> Steffen
>
> .../devicetree/bindings/video/displaymode | 74 +++++
> drivers/of/Kconfig | 5 +
> drivers/of/Makefile | 1 +
> drivers/of/of_videomode.c | 283 ++++++++++++++++++++
> include/linux/of_videomode.h | 56 ++++
> 5 files changed, 419 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/video/displaymode
> create mode 100644 drivers/of/of_videomode.c
> create mode 100644 include/linux/of_videomode.h
>
> diff --git a/Documentation/devicetree/bindings/video/displaymode b/Documentation/devicetree/bindings/video/displaymode
> new file mode 100644
> index 0000000..990ca52
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/displaymode
> @@ -0,0 +1,74 @@
> +videomode bindings
> +=========
> +
> +Required properties:
> + - hactive, vactive: Display resolution
> + - hfront-porch, hback-porch, hsync-len: Horizontal Display timing parameters
> + in pixels
> + vfront-porch, vback-porch, vsync-len: Vertical display timing parameters in
> + lines
In the case of interlaced formats, can the vfront-porch, vback-porch and vsync-len
for the second field always be calculated from these values? Is that fully
standardized or can there be exceptions? struct v4l2_bt_timings has separate fields
for these, but that may have been overkill.
> + - clock: displayclock in Hz
CEA-861 defined the option to slightly lower the clock frequency by 1.000/1.001 to
achieve frequencies like 29.97 or 59.94. In v4l2_bt_timings I made a flag for that.
I'm not sure whether it is better to just set the clock to the correct frequency
(which is a weird value) or mark it with a bool.
> +
> +Optional properties:
> + - width-mm, height-mm: Display dimensions in mm
> + - hsync-active-high (bool): Hsync pulse is active high
> + - vsync-active-high (bool): Vsync pulse is active high
> + - interlaced (bool): This is an interlaced mode
> + - doublescan (bool): This is a doublescan mode
> +
> +There are different ways of describing a display mode. The devicetree representation
> +corresponds to the one commonly found in datasheets for displays.
> +The description of the display and its mode is split in two parts: first the display
> +properties like size in mm and (optionally) multiple subnodes with the supported modes.
> +
> +Example:
> +
> + display@0 {
> + width-mm = <800>;
> + height-mm = <480>;
> + modes {
> + mode0: mode@0 {
> + /* 1920x1080p24 */
> + clock = <52000000>;
> + hactive = <1920>;
> + vactive = <1080>;
> + hfront-porch = <25>;
> + hback-porch = <25>;
> + hsync-len = <25>;
> + vback-porch = <2>;
> + vfront-porch = <2>;
> + vsync-len = <2>;
> + hsync-active-high;
> + };
> + };
> + };
> +
> +Every property also supports the use of ranges, so the commonly used datasheet
> +description with <min typ max>-tuples can be used.
> +
> +Example:
> +
> + mode1: mode@1 {
> + /* 1920x1080p24 */
> + clock = <148500000>;
> + hactive = <1920>;
> + vactive = <1080>;
> + hsync-len = <0 44 60>;
> + hfront-porch = <80 88 95>;
> + hback-porch = <100 148 160>;
> + vfront-porch = <0 4 6>;
> + vback-porch = <0 36 50>;
> + vsync-len = <0 5 6>;
> + };
> +
> +The videomode can be linked to a connector via phandles. The connector has to
> +support the display- and default-mode-property to link to and select a mode.
> +
> +Example:
> +
> + hdmi@00120000 {
> + status = "okay";
> + display = <&benq>;
> + default-mode = <&mode1>;
> + };
> +
> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index dfba3e6..a3acaa3 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -83,4 +83,9 @@ config OF_MTD
> depends on MTD
> def_bool y
>
> +config OF_VIDEOMODE
> + def_bool y
> + help
> + helper to parse videomodes from the devicetree
> +
> endmenu # OF
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index e027f44..80e6db3 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -11,3 +11,4 @@ obj-$(CONFIG_OF_MDIO) += of_mdio.o
> obj-$(CONFIG_OF_PCI) += of_pci.o
> obj-$(CONFIG_OF_PCI_IRQ) += of_pci_irq.o
> obj-$(CONFIG_OF_MTD) += of_mtd.o
> +obj-$(CONFIG_OF_VIDEOMODE) += of_videomode.o
> diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
> new file mode 100644
> index 0000000..52bfc74
> --- /dev/null
> +++ b/drivers/of/of_videomode.c
> @@ -0,0 +1,283 @@
> +/*
> + * OF helpers for parsing display modes
> + *
> + * Copyright (c) 2012 Sascha Hauer <s.hauer@pengutronix.de>, Pengutronix
> + * Copyright (c) 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>, Pengutronix
> + *
> + * This file is released under the GPLv2
> + */
> +#include <linux/of.h>
> +#include <linux/fb.h>
> +#include <linux/export.h>
> +#include <linux/slab.h>
> +#include <drm/drmP.h>
> +#include <drm/drm_crtc.h>
> +#include <linux/of_videomode.h>
> +
> +static u32 of_video_get_value(struct mode_property *prop)
> +{
> + return (prop->min >= prop->typ) ? prop->min : prop->typ;
> +}
> +
> +/* read property into new mode_property */
> +static int of_video_parse_property(struct device_node *np, char *name,
> + struct mode_property *result)
> +{
> + struct property *prop;
> + int length;
> + int cells;
> + int ret;
> +
> + prop = of_find_property(np, name, &length);
> + if (!prop) {
> + pr_err("%s: could not find property %s\n", __func__,
> + name);
> + return -EINVAL;
> + }
> +
> + cells = length / sizeof(u32);
> +
> + memset(result, 0, sizeof(*result));
> +
> + ret = of_property_read_u32_array(np, name, &result->min, cells);
> +
> + return ret;
> +}
> +
> +static int of_video_free(struct display *disp)
> +{
> + int i;
> +
> + for (i=0; i<disp->num_modes; i++)
> + kfree(disp->modes[i]);
> + kfree(disp->modes);
> +
> + return 0;
> +}
> +
> +int videomode_to_display_mode(struct display *disp, struct drm_display_mode *dmode, int index)
> +{
> + struct videomode *vm;
> +
> + memset(dmode, 0, sizeof(*dmode));
> +
> + if (index > disp->num_modes) {
> + pr_err("%s: wrong index: %d from %d\n", __func__, index, disp->num_modes);
> + return -EINVAL;
> + }
> +
> + vm = disp->modes[index];
> +
> + dmode->hdisplay = of_video_get_value(&vm->hactive);
> + dmode->hsync_start = of_video_get_value(&vm->hactive) + of_video_get_value(&vm->hfront_porch);
> + dmode->hsync_end = of_video_get_value(&vm->hactive) + of_video_get_value(&vm->hfront_porch)
> + + of_video_get_value(&vm->hsync_len);
> + dmode->htotal = of_video_get_value(&vm->hactive) + of_video_get_value(&vm->hfront_porch)
> + + of_video_get_value(&vm->hsync_len) + of_video_get_value(&vm->hback_porch);
> +
> + dmode->vdisplay = of_video_get_value(&vm->vactive);
> + dmode->vsync_start = of_video_get_value(&vm->vactive) + of_video_get_value(&vm->vfront_porch);
> + dmode->vsync_end = of_video_get_value(&vm->vactive) + of_video_get_value(&vm->vfront_porch)
> + + of_video_get_value(&vm->vsync_len);
> + dmode->vtotal = of_video_get_value(&vm->vactive) + of_video_get_value(&vm->vfront_porch) +
> + of_video_get_value(&vm->vsync_len) + of_video_get_value(&vm->vback_porch);
> +
> + dmode->width_mm = disp->width_mm;
> + dmode->height_mm = disp->height_mm;
> +
> + dmode->clock = of_video_get_value(&vm->clock) / 1000;
> +
> + if (vm->hah)
> + dmode->flags |= DRM_MODE_FLAG_PHSYNC;
> + else
> + dmode->flags |= DRM_MODE_FLAG_NHSYNC;
> + if (vm->vah)
> + dmode->flags |= DRM_MODE_FLAG_PVSYNC;
> + else
> + dmode->flags |= DRM_MODE_FLAG_NVSYNC;
> + if (vm->interlaced)
> + dmode->flags |= DRM_MODE_FLAG_INTERLACE;
> + if (vm->doublescan)
> + dmode->flags |= DRM_MODE_FLAG_DBLSCAN;
> +
> + drm_mode_set_name(dmode);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(videomode_to_display_mode);
> +
> +int videomode_to_fb_mode(struct display *disp, struct fb_videomode *fbmode, int index)
> +{
> + struct videomode *vm;
> +
> + memset(fbmode, 0, sizeof(*fbmode));
> +
> + if (index > disp->num_modes) {
> + pr_err("%s: wrong index: %d from %d\n", __func__, index, disp->num_modes);
> + return -EINVAL;
> + }
> +
> + vm = disp->modes[index];
> +
> + fbmode->xres = of_video_get_value(&vm->hactive);
> + fbmode->left_margin = of_video_get_value(&vm->hback_porch);
> + fbmode->right_margin = of_video_get_value(&vm->hfront_porch);
> + fbmode->hsync_len = of_video_get_value(&vm->hsync_len);
> +
> + fbmode->yres = of_video_get_value(&vm->vactive);
> + fbmode->upper_margin = of_video_get_value(&vm->vback_porch);
> + fbmode->lower_margin = of_video_get_value(&vm->vfront_porch);
> + fbmode->vsync_len = of_video_get_value(&vm->vsync_len);
> +
> + fbmode->pixclock = KHZ2PICOS(of_video_get_value(&vm->clock) / 1000);
> +
> + if (vm->hah)
> + fbmode->sync |= FB_SYNC_HOR_HIGH_ACT;
> + if (vm->vah)
> + fbmode->sync |= FB_SYNC_VERT_HIGH_ACT;
> + if (vm->interlaced)
> + fbmode->vmode |= FB_VMODE_INTERLACED;
> + if (vm->doublescan)
> + fbmode->vmode |= FB_VMODE_DOUBLE;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(videomode_to_fb_mode);
> +
> +int of_get_video_modes(struct device_node *np, struct display *disp)
> +{
> + struct device_node *display_np;
> + struct device_node *mode_np;
> + struct device_node *modes_np;
> + char *default_mode;
> +
> + int ret = 0;
> +
> + memset(disp, 0, sizeof(*disp));
> + disp->modes = kmalloc(sizeof(struct videomode*), GFP_KERNEL);
> +
> + if (!np) {
> + pr_err("%s: no node provided\n", __func__);
> + return -EINVAL;
> + }
> +
> + display_np = of_parse_phandle(np, "display", 0);
> +
> + if (!display_np) {
> + pr_err("%s: could not find display node\n", __func__);
> + return -EINVAL;
> + }
> +
> + of_property_read_u32(display_np, "width-mm", &disp->width_mm);
> + of_property_read_u32(display_np, "height-mm", &disp->height_mm);
> +
> + mode_np = of_parse_phandle(np, "default-mode", 0);
> +
> + if (!mode_np) {
> + pr_info("%s: no default-mode specified.\n", __func__);
> + mode_np = of_find_node_by_name(np, "mode");
> + }
> +
> + if (!mode_np) {
> + pr_err("%s: could not find any mode specification\n", __func__);
> + return -EINVAL;
> + }
> +
> + default_mode = (char *)mode_np->full_name;
> +
> + modes_np = of_find_node_by_name(np, "modes");
> + for_each_child_of_node(modes_np, mode_np) {
> + struct videomode *vm;
> +
> + vm = kmalloc(sizeof(struct videomode*), GFP_KERNEL);
> + disp->modes[disp->num_modes] = kmalloc(sizeof(struct videomode*), GFP_KERNEL);
> +
> + ret |= of_video_parse_property(mode_np, "hback-porch", &vm->hback_porch);
> + ret |= of_video_parse_property(mode_np, "hfront-porch", &vm->hfront_porch);
> + ret |= of_video_parse_property(mode_np, "hactive", &vm->hactive);
> + ret |= of_video_parse_property(mode_np, "hsync-len", &vm->hsync_len);
> + ret |= of_video_parse_property(mode_np, "vback-porch", &vm->vback_porch);
> + ret |= of_video_parse_property(mode_np, "vfront-porch", &vm->vfront_porch);
> + ret |= of_video_parse_property(mode_np, "vactive", &vm->vactive);
> + ret |= of_video_parse_property(mode_np, "vsync-len", &vm->vsync_len);
> + ret |= of_video_parse_property(mode_np, "clock", &vm->clock);
> +
> + if (ret)
> + return -EINVAL;
> +
> + vm->hah = of_property_read_bool(mode_np, "hsync-active-high");
> + vm->vah = of_property_read_bool(mode_np, "vsync-active-high");
> + vm->interlaced = of_property_read_bool(mode_np, "interlaced");
> + vm->doublescan = of_property_read_bool(mode_np, "doublescan");
> +
> + if (strcmp(default_mode,mode_np->full_name) = 0)
> + disp->default_mode = disp->num_modes;
> +
> + disp->modes[disp->num_modes] = vm;
> + disp->num_modes++;
> + }
> + of_node_put(display_np);
> +
> + pr_info("%s: found %d modelines. Using #%d as default\n", __func__,
> + disp->num_modes, disp->default_mode + 1);
> +
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_get_video_modes);
> +
> +int of_video_mode_exists(struct device_node *np)
> +{
> + struct device_node *display_np;
> + struct device_node *mode_np;
> +
> + if (!np)
> + return -EINVAL;
> +
> + display_np = of_parse_phandle(np, "display", 0);
> +
> + if (!display_np)
> + return -EINVAL;
> +
> + mode_np = of_parse_phandle(np, "default-mode", 0);
> +
> + if (mode_np)
> + return 0;
> +
> + return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(of_video_mode_exists);
> +
> +int of_get_display_mode(struct device_node *np, struct drm_display_mode *dmode, int index)
> +{
> + struct display disp;
> +
> + of_get_video_modes(np, &disp);
> +
> + if (index = OF_MODE_SELECTION)
> + index = disp.default_mode;
> + if (dmode)
> + videomode_to_display_mode(&disp, dmode, index);
> +
> + of_video_free(&disp);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_get_display_mode);
> +
> +int of_get_fb_videomode(struct device_node *np, struct fb_videomode *fbmode, int index)
> +{
> + struct display disp;
> +
> + of_get_video_modes(np, &disp);
> +
> + if (index = OF_MODE_SELECTION)
> + index = disp.default_mode;
> + if (fbmode)
> + videomode_to_fb_mode(&disp, fbmode, index);
> +
> + of_video_free(&disp);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_get_fb_videomode);
> diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h
> new file mode 100644
> index 0000000..5571ce3
> --- /dev/null
> +++ b/include/linux/of_videomode.h
> @@ -0,0 +1,56 @@
> +/*
> + * Copyright 2012 Sascha Hauer <s.hauer@pengutronix.de>
> + * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>
> + *
> + * OF helpers for videomodes.
> + *
> + * This file is released under the GPLv2
> + */
> +
> +#ifndef __LINUX_OF_VIDEOMODE_H
> +#define __LINUX_OF_VIDEOMODE_H
> +
> +#define OF_MODE_SELECTION -1
> +
> +struct mode_property {
> + u32 min;
> + u32 typ;
> + u32 max;
> +};
For what purpose are the min and max values added? To specify hardware limitations?
If it is the latter, then I think it would be a better approach to split that off
in its own struct.
For V4L2 we made a separate capability struct:
http://hverkuil.home.xs4all.nl/spec/media.html#vidioc-dv-timings-cap
Regards,
Hans
> +
> +struct display {
> + u32 width_mm;
> + u32 height_mm;
> + struct videomode **modes;
> + int default_mode;
> + int num_modes;
> +};
> +
> +/* describe videomode in terms of hardware parameters */
> +struct videomode {
> + struct mode_property hback_porch;
> + struct mode_property hfront_porch;
> + struct mode_property hactive;
> + struct mode_property hsync_len;
> +
> + struct mode_property vback_porch;
> + struct mode_property vfront_porch;
> + struct mode_property vactive;
> + struct mode_property vsync_len;
> +
> + struct mode_property clock;
> +
> + bool hah;
> + bool vah;
> + bool interlaced;
> + bool doublescan;
> +};
> +
> +int of_video_mode_exists(struct device_node *np);
> +int videomode_to_display_mode(struct display *disp, struct drm_display_mode *dmode, int index);
> +int videomode_to_fb_mode(struct display *disp, struct fb_videomode *fbmode, int index);
> +int of_get_video_modes(struct device_node *np, struct display *disp);
> +int of_video_mode_exists(struct device_node *np);
> +int of_get_display_mode(struct device_node *np, struct drm_display_mode *dmode, int index);
> +int of_get_fb_videomode(struct device_node *np, struct fb_videomode *fbmode, int index);
> +#endif /* __LINUX_OF_VIDEOMODE_H */
>
^ permalink raw reply
* Re: [PATCH v4] of: Add videomode helper
From: Steffen Trumtrar @ 2012-09-21 12:47 UTC (permalink / raw)
To: Laurent Pinchart
Cc: devicetree-discuss, linux-fbdev, dri-devel, kernel, Sascha Hauer,
linux-media
In-Reply-To: <1650842.iWU5MDhFZR@avalon>
On Thu, Sep 20, 2012 at 11:29:42PM +0200, Laurent Pinchart wrote:
> Hi,
>
> (CC'ing the linux-media mailing list, as video modes are of interest there as
> well)
>
> On Wednesday 19 September 2012 12:19:22 Tomi Valkeinen wrote:
> > On Wed, 2012-09-19 at 10:20 +0200, Steffen Trumtrar wrote:
> > > This patch adds a helper function for parsing videomodes from the
> > > devicetree. The videomode can be either converted to a struct
> > > drm_display_mode or a struct fb_videomode.
> > >
> > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> > > ---
> > >
> > > Hi!
> > >
> > > changes since v3:
> > > - print error messages
> > > - free alloced memory
> > > - general cleanup
> > >
> > > Regards
> > > Steffen
> > >
> > > .../devicetree/bindings/video/displaymode | 74 +++++
> > > drivers/of/Kconfig | 5 +
> > > drivers/of/Makefile | 1 +
> > > drivers/of/of_videomode.c | 283 +++++++++++++++
> > > include/linux/of_videomode.h | 56 ++++
> > > 5 files changed, 419 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/video/displaymode
> > > create mode 100644 drivers/of/of_videomode.c
> > > create mode 100644 include/linux/of_videomode.h
> > >
> > > diff --git a/Documentation/devicetree/bindings/video/displaymode
> > > b/Documentation/devicetree/bindings/video/displaymode new file mode
> > > 100644
> > > index 0000000..990ca52
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/video/displaymode
> > > @@ -0,0 +1,74 @@
> > > +videomode bindings
> > > +=========
> > > +
> > > +Required properties:
> > > + - hactive, vactive: Display resolution
> > > + - hfront-porch, hback-porch, hsync-len: Horizontal Display timing
> > > parameters
> > > + in pixels
> > > + vfront-porch, vback-porch, vsync-len: Vertical display timing
> > > parameters in
> > > + lines
> > > + - clock: displayclock in Hz
> > > +
> > > +Optional properties:
> > > + - width-mm, height-mm: Display dimensions in mm
> > > + - hsync-active-high (bool): Hsync pulse is active high
> > > + - vsync-active-high (bool): Vsync pulse is active high
> > > + - interlaced (bool): This is an interlaced mode
> > > + - doublescan (bool): This is a doublescan mode
> > > +
> > > +There are different ways of describing a display mode. The devicetree
> > > representation
> > > +corresponds to the one commonly found in datasheets for displays.
> > > +The description of the display and its mode is split in two parts: first
> > > the display
> > > +properties like size in mm and (optionally) multiple subnodes with the
> > > supported modes.
> > > +
> > > +Example:
> > > +
> > > + display@0 {
> > > + width-mm = <800>;
> > > + height-mm = <480>;
> > > + modes {
> > > + mode0: mode@0 {
> > > + /* 1920x1080p24 */
> > > + clock = <52000000>;
> > > + hactive = <1920>;
> > > + vactive = <1080>;
> > > + hfront-porch = <25>;
> > > + hback-porch = <25>;
> > > + hsync-len = <25>;
> > > + vback-porch = <2>;
> > > + vfront-porch = <2>;
> > > + vsync-len = <2>;
> > > + hsync-active-high;
> > > + };
> > > + };
> > > + };
> > > +
> > > +Every property also supports the use of ranges, so the commonly used
> > > datasheet +description with <min typ max>-tuples can be used.
> > > +
> > > +Example:
> > > +
> > > + mode1: mode@1 {
> > > + /* 1920x1080p24 */
> > > + clock = <148500000>;
> > > + hactive = <1920>;
> > > + vactive = <1080>;
> > > + hsync-len = <0 44 60>;
> > > + hfront-porch = <80 88 95>;
> > > + hback-porch = <100 148 160>;
> > > + vfront-porch = <0 4 6>;
> > > + vback-porch = <0 36 50>;
> > > + vsync-len = <0 5 6>;
> > > + };
> > > +
> > > +The videomode can be linked to a connector via phandles. The connector
> > > has to
> > > +support the display- and default-mode-property to link to and select a
> > > mode.
> > > +
> > > +Example:
> > > +
> > > + hdmi@00120000 {
> > > + status = "okay";
> > > + display = <&benq>;
> > > + default-mode = <&mode1>;
> > > + };
> > > +
> > > diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> > > index dfba3e6..a3acaa3 100644
> > > --- a/drivers/of/Kconfig
> > > +++ b/drivers/of/Kconfig
> > > @@ -83,4 +83,9 @@ config OF_MTD
> > >
> > > depends on MTD
> > > def_bool y
> > >
> > > +config OF_VIDEOMODE
> > > + def_bool y
> > > + help
> > > + helper to parse videomodes from the devicetree
> > > +
> > >
> > > endmenu # OF
> > >
> > > diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> > > index e027f44..80e6db3 100644
> > > --- a/drivers/of/Makefile
> > > +++ b/drivers/of/Makefile
> > > @@ -11,3 +11,4 @@ obj-$(CONFIG_OF_MDIO) += of_mdio.o
> > >
> > > obj-$(CONFIG_OF_PCI) += of_pci.o
> > > obj-$(CONFIG_OF_PCI_IRQ) += of_pci_irq.o
> > > obj-$(CONFIG_OF_MTD) += of_mtd.o
> > >
> > > +obj-$(CONFIG_OF_VIDEOMODE) += of_videomode.o
> > > diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
> > > new file mode 100644
> > > index 0000000..52bfc74
> > > --- /dev/null
> > > +++ b/drivers/of/of_videomode.c
> > > @@ -0,0 +1,283 @@
> > > +/*
> > > + * OF helpers for parsing display modes
> > > + *
> > > + * Copyright (c) 2012 Sascha Hauer <s.hauer@pengutronix.de>, Pengutronix
> > > + * Copyright (c) 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>,
> > > Pengutronix
> > > + *
> > > + * This file is released under the GPLv2
> > > + */
> > > +#include <linux/of.h>
> > > +#include <linux/fb.h>
> > > +#include <linux/export.h>
> > > +#include <linux/slab.h>
> > > +#include <drm/drmP.h>
> > > +#include <drm/drm_crtc.h>
> > > +#include <linux/of_videomode.h>
> > > +
> > > +static u32 of_video_get_value(struct mode_property *prop)
> > > +{
> > > + return (prop->min >= prop->typ) ? prop->min : prop->typ;
> > > +}
> >
> > Why is this needed? If the prop->min is higher than prop->typ, isn't
> > that an error in the DT data, and should be rejected when parsing?
> >
> > > +
> > > +/* read property into new mode_property */
> > > +static int of_video_parse_property(struct device_node *np, char *name,
> > > + struct mode_property *result)
> > > +{
> > > + struct property *prop;
> > > + int length;
> > > + int cells;
> > > + int ret;
> > > +
> > > + prop = of_find_property(np, name, &length);
> > > + if (!prop) {
> > > + pr_err("%s: could not find property %s\n", __func__,
> > > + name);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + cells = length / sizeof(u32);
> > > +
> > > + memset(result, 0, sizeof(*result));
> > > +
> > > + ret = of_property_read_u32_array(np, name, &result->min, cells);
> > > +
> > > + return ret;
> >
> > Ah, I guess this is the reason for the of_video_get_value... Wouldn't it
> > be cleaner to be more explicit here? If there's one cell, it's the
> > typical value, otherwise if there are 3 cells, they are min/typ/max,
> > else it's an error.
>
> I agree. We should flag errors and fail instead of trying to fix them
> silently. That's the only way to make sure that device tree authors will get
> it right (or at least less wrong). We might need to accept wrong DT data later
> to support buggy devices found in the wild, but we should not start that way.
>
Well, I can only agree, too ;-) Fixed.
> > And I think the above also trashes memory if there happens to be 4+ cells.
> >
> > > +}
> > > +
> > > +static int of_video_free(struct display *disp)
> > > +{
> > > + int i;
> > > +
> > > + for (i=0; i<disp->num_modes; i++)
>
> Spaces around = and < please.
>
> > > + kfree(disp->modes[i]);
> > > + kfree(disp->modes);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +int videomode_to_display_mode(struct display *disp, struct
> > > drm_display_mode *dmode, int index)
>
> Shouldn't this use drm_mode_modeinfo instead of drm_display_mode ?
>
> > > +{
> > > + struct videomode *vm;
> > > +
> > > + memset(dmode, 0, sizeof(*dmode));
> > > +
> > > + if (index > disp->num_modes) {
> > > + pr_err("%s: wrong index: %d from %d\n", __func__, index,
> > > disp->num_modes); + return -EINVAL;
> > > + }
> > > +
> > > + vm = disp->modes[index];
> > > +
> > > + dmode->hdisplay = of_video_get_value(&vm->hactive);
> > > + dmode->hsync_start = of_video_get_value(&vm->hactive) +
> > > of_video_get_value(&vm->hfront_porch);
>
> You could replace of_video_get_value(&vm->hactive) with dmode->hdisplay here,
> and similarly below (hsync_end = hsync_start + of_video_get_value(&vm-
> >hsync_len), ...). Beside shortening lines, it would save calls to
> of_video_get_value().
>
> > > + dmode->hsync_end = of_video_get_value(&vm->hactive) +
> > > of_video_get_value(&vm->hfront_porch)
> > > + + of_video_get_value(&vm->hsync_len);
> > > + dmode->htotal = of_video_get_value(&vm->hactive) +
> > > of_video_get_value(&vm->hfront_porch)
> > > + + of_video_get_value(&vm->hsync_len) +
> > > of_video_get_value(&vm->hback_porch);
> > > + dmode->vdisplay = of_video_get_value(&vm->vactive);
> > > + dmode->vsync_start = of_video_get_value(&vm->vactive) +
> > > of_video_get_value(&vm->vfront_porch); + dmode->vsync_end > > > of_video_get_value(&vm->vactive) + of_video_get_value(&vm->vfront_porch)
> > > + + of_video_get_value(&vm->vsync_len);
> > > + dmode->vtotal = of_video_get_value(&vm->vactive) +
> > > of_video_get_value(&vm->vfront_porch) +
> > > + of_video_get_value(&vm->vsync_len) +
> > > of_video_get_value(&vm->vback_porch); +
> > > + dmode->width_mm = disp->width_mm;
> > > + dmode->height_mm = disp->height_mm;
> > > +
> > > + dmode->clock = of_video_get_value(&vm->clock) / 1000;
> > > +
> > > + if (vm->hah)
> > > + dmode->flags |= DRM_MODE_FLAG_PHSYNC;
> > > + else
> > > + dmode->flags |= DRM_MODE_FLAG_NHSYNC;
> > > + if (vm->vah)
> > > + dmode->flags |= DRM_MODE_FLAG_PVSYNC;
> > > + else
> > > + dmode->flags |= DRM_MODE_FLAG_NVSYNC;
> > > + if (vm->interlaced)
> > > + dmode->flags |= DRM_MODE_FLAG_INTERLACE;
> > > + if (vm->doublescan)
> > > + dmode->flags |= DRM_MODE_FLAG_DBLSCAN;
> > > +
> > > + drm_mode_set_name(dmode);
> > > +
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(videomode_to_display_mode);
> > > +
> > > +int videomode_to_fb_mode(struct display *disp, struct fb_videomode
> > > *fbmode, int index)
>
> disp is only used to retrieve the mode, you could pass a struct videomode
> instead of disp and index.
>
> Thinking about it, I would make videomode_to_display_mode take a struct
> videomode as well. The only reason it needs struct display is to get the
> display physical dimensions. Those are only used in very specific cases in the
> DRM subsystem, I don't think they should be copied to every struct
> drm_mode_modeinfo.
>
Sounds reasonable. Will fix this.
> > > +{
> > > + struct videomode *vm;
> > > +
> > > + memset(fbmode, 0, sizeof(*fbmode));
> > > +
> > > + if (index > disp->num_modes) {
> > > + pr_err("%s: wrong index: %d from %d\n", __func__, index,
> > > disp->num_modes);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + vm = disp->modes[index];
> > > +
> > > + fbmode->xres = of_video_get_value(&vm->hactive);
> > > + fbmode->left_margin = of_video_get_value(&vm->hback_porch);
> > > + fbmode->right_margin = of_video_get_value(&vm->hfront_porch);
> > > + fbmode->hsync_len = of_video_get_value(&vm->hsync_len);
> > > +
> > > + fbmode->yres = of_video_get_value(&vm->vactive);
> > > + fbmode->upper_margin = of_video_get_value(&vm->vback_porch);
> > > + fbmode->lower_margin = of_video_get_value(&vm->vfront_porch);
> > > + fbmode->vsync_len = of_video_get_value(&vm->vsync_len);
> > > +
> > > + fbmode->pixclock = KHZ2PICOS(of_video_get_value(&vm->clock) / 1000);
> > > +
> > > + if (vm->hah)
> > > + fbmode->sync |= FB_SYNC_HOR_HIGH_ACT;
> > > + if (vm->vah)
> > > + fbmode->sync |= FB_SYNC_VERT_HIGH_ACT;
> > > + if (vm->interlaced)
> > > + fbmode->vmode |= FB_VMODE_INTERLACED;
> > > + if (vm->doublescan)
> > > + fbmode->vmode |= FB_VMODE_DOUBLE;
> > > +
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(videomode_to_fb_mode);
> > > +
> > > +int of_get_video_modes(struct device_node *np, struct display *disp)
> > > +{
> > > + struct device_node *display_np;
> > > + struct device_node *mode_np;
> > > + struct device_node *modes_np;
> > > + char *default_mode;
> > > +
> > > + int ret = 0;
> > > +
> > > + memset(disp, 0, sizeof(*disp));
> > > + disp->modes = kmalloc(sizeof(struct videomode*), GFP_KERNEL);
> > > +
> > > + if (!np) {
> > > + pr_err("%s: no node provided\n", __func__);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + display_np = of_parse_phandle(np, "display", 0);
> > > +
> > > + if (!display_np) {
> > > + pr_err("%s: could not find display node\n", __func__);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + of_property_read_u32(display_np, "width-mm", &disp->width_mm);
> > > + of_property_read_u32(display_np, "height-mm", &disp->height_mm);
> > > +
> > > + mode_np = of_parse_phandle(np, "default-mode", 0);
> > > +
> > > + if (!mode_np) {
> > > + pr_info("%s: no default-mode specified.\n", __func__);
> > > + mode_np = of_find_node_by_name(np, "mode");
> > > + }
> > > +
> > > + if (!mode_np) {
> > > + pr_err("%s: could not find any mode specification\n", __func__);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + default_mode = (char *)mode_np->full_name;
> > > +
> > > + modes_np = of_find_node_by_name(np, "modes");
> > > + for_each_child_of_node(modes_np, mode_np) {
> > > + struct videomode *vm;
> > > +
> > > + vm = kmalloc(sizeof(struct videomode*), GFP_KERNEL);
> > > + disp->modes[disp->num_modes] = kmalloc(sizeof(struct videomode*),
> > > GFP_KERNEL);
> > > +
> > > + ret |= of_video_parse_property(mode_np, "hback-porch",
> > > &vm->hback_porch);
> > > + ret |= of_video_parse_property(mode_np, "hfront-porch",
> > > &vm->hfront_porch);
> > > + ret |= of_video_parse_property(mode_np, "hactive", &vm->hactive);
> > > + ret |= of_video_parse_property(mode_np, "hsync-len",
> > > &vm->hsync_len);
> > > + ret |= of_video_parse_property(mode_np, "vback-porch",
> > > &vm->vback_porch);
> > > + ret |= of_video_parse_property(mode_np, "vfront-porch",
> > > &vm->vfront_porch);
> > > + ret |= of_video_parse_property(mode_np, "vactive", &vm->vactive);
> > > + ret |= of_video_parse_property(mode_np, "vsync-len",
> > > &vm->vsync_len);
> > > + ret |= of_video_parse_property(mode_np, "clock", &vm->clock);
> > > +
> > > + if (ret)
> > > + return -EINVAL;
> > > +
> > > + vm->hah = of_property_read_bool(mode_np, "hsync-active-high");
> > > + vm->vah = of_property_read_bool(mode_np, "vsync-active-high");
> > > + vm->interlaced = of_property_read_bool(mode_np, "interlaced");
> > > + vm->doublescan = of_property_read_bool(mode_np, "doublescan");
> > > +
> > > + if (strcmp(default_mode,mode_np->full_name) = 0)
> > > + disp->default_mode = disp->num_modes;
> > > +
> > > + disp->modes[disp->num_modes] = vm;
> > > + disp->num_modes++;
> > > + }
> > > + of_node_put(display_np);
> > > +
> > > + pr_info("%s: found %d modelines. Using #%d as default\n", __func__,
> > > + disp->num_modes, disp->default_mode + 1);
> > > +
> > > +
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(of_get_video_modes);
> > > +
> > > +int of_video_mode_exists(struct device_node *np)
> > > +{
> > > + struct device_node *display_np;
> > > + struct device_node *mode_np;
> > > +
> > > + if (!np)
> > > + return -EINVAL;
> > > +
> > > + display_np = of_parse_phandle(np, "display", 0);
> > > +
> > > + if (!display_np)
> > > + return -EINVAL;
> > > +
> > > + mode_np = of_parse_phandle(np, "default-mode", 0);
> > > +
> > > + if (mode_np)
> > > + return 0;
> > > +
> > > + return -EINVAL;
> > > +}
> > > +EXPORT_SYMBOL_GPL(of_video_mode_exists);
> > > +
> > > +int of_get_display_mode(struct device_node *np, struct drm_display_mode
> > > *dmode, int index)
> > > +{
> > > + struct display disp;
> > > +
> > > + of_get_video_modes(np, &disp);
> > > +
> > > + if (index = OF_MODE_SELECTION)
> > > + index = disp.default_mode;
> > > + if (dmode)
> > > + videomode_to_display_mode(&disp, dmode, index);
> > > +
> > > + of_video_free(&disp);
> > > +
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(of_get_display_mode);
> > > +
> > > +int of_get_fb_videomode(struct device_node *np, struct fb_videomode
> > > *fbmode, int index)
> > > +{
> > > + struct display disp;
> > > +
> > > + of_get_video_modes(np, &disp);
> > > +
> > > + if (index = OF_MODE_SELECTION)
> > > + index = disp.default_mode;
> > > + if (fbmode)
> > > + videomode_to_fb_mode(&disp, fbmode, index);
> > > +
> > > + of_video_free(&disp);
> > > +
> > > + return 0;
> >
> > This and of_get_display_mode() do not handle errors from
> > of_get_video_modes() nor from videomode_to_xxx_mode(). And I don't see a
> > reason for the if (fbmode) check (and the same for dmode), as there's no
> > reason to call these functions except to get the video modes.
> >
> > > +}
> > > +EXPORT_SYMBOL_GPL(of_get_fb_videomode);
>
> What are those two functions used for ? Aren't drivers expected to parse the
> modes into a struct display and then operate on that structure, instead of
> getting individual modes from the DT node ?
>
> > > diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h
> > > new file mode 100644
> > > index 0000000..5571ce3
> > > --- /dev/null
> > > +++ b/include/linux/of_videomode.h
> > > @@ -0,0 +1,56 @@
> > > +/*
> > > + * Copyright 2012 Sascha Hauer <s.hauer@pengutronix.de>
> > > + * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>
> > > + *
> > > + * OF helpers for videomodes.
> > > + *
> > > + * This file is released under the GPLv2
> > > + */
> > > +
> > > +#ifndef __LINUX_OF_VIDEOMODE_H
> > > +#define __LINUX_OF_VIDEOMODE_H
> > > +
> > > +#define OF_MODE_SELECTION -1
> > > +
> > > +struct mode_property {
> > > + u32 min;
> > > + u32 typ;
> > > + u32 max;
> > > +};
> > > +
> > > +struct display {
> > > + u32 width_mm;
> > > + u32 height_mm;
> > > + struct videomode **modes;
> > > + int default_mode;
> > > + int num_modes;
>
> default_mode and num_modes are non-negative, what about using an unsigned int
> type for them ?
>
> > > +};
> > > +
> > > +/* describe videomode in terms of hardware parameters */
> > > +struct videomode {
> > > + struct mode_property hback_porch;
> > > + struct mode_property hfront_porch;
> > > + struct mode_property hactive;
> > > + struct mode_property hsync_len;
> > > +
> > > + struct mode_property vback_porch;
> > > + struct mode_property vfront_porch;
> > > + struct mode_property vactive;
> > > + struct mode_property vsync_len;
> > > +
> > > + struct mode_property clock;
> > > +
> > > + bool hah;
> > > + bool vah;
> > > + bool interlaced;
> > > + bool doublescan;
> > > +};
> >
> > I think the names display and videomode are a bit too generic here, and
> > could clash with names from kernel drivers. Also, the videomode is not
> > really a videomode, but a "template" (or something) for videomode. A
> > real videomode doesn't have min & max values, only the actual value.
> >
> > Perhaps these should be prefixed with "of_"? Then again, they are not
> > really of specific either...
>
> If feel this is an important topic.
>
> A generic video mode structure is definitely needed, as well as helper
> functions to convert between the new structure and existing video mode
> structures (struct fb_videomode, struct drm_mode_modeinfo and struct
> v4l2_bt_timings). The structure and the helper functions should be generic, as
> the goal is to gradually replace subsystem-specific video mode structures
> where possible (userspace APIs will still need to keep the old structures). We
> thus need to drop the dependency on OF for everything but the DT parsing code.
Okay. Here I'm in a dilemma. Sounds like I should split up the code as it is
right now: a generic displaymode/videomode-handler and the of-functions to handle it.
Where would I put the displaymode/videomode-stuff ? drivers/media/video ?
I will keep it all in the same location for the next patch version. But I guess,
I will not be finished with that and have to split up the code (apart from any
coding errors I might still have in there ;-))
> For that reason an of_ prefix wouldn't be a good idea. As the goal is to
> create a truly generic video mode structure, a generic name is a good idea (I
> might prefer struct video_mode instead of struct videomode, but that's just
> bikeshedding).
>
An internal meeting (well, me and Sascha) came to the conclusion, that I keep
struct videomode for the time beeing. But I may be persuaded to change this, if
other people chime in and want struct video_mode, too.
> We might need two video mode structures, one to represent a video mode, and
> one to represent a range of video modes. I don't have enough experience with
> video modes to have a really strong opinion on this, having a single structure
> to represent both could be useful as well. This particular topic needs to be
> discussed.
>
> --
> Regards,
>
> Laurent Pinchart
>
>
Regards,
Steffen
--
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
* Re: [RFC PATCH 1/3] amba-clcd: Add Device Tree support to amba-clcd driver
From: Russell King - ARM Linux @ 2012-09-21 12:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20120921114345.GE24458@pengutronix.de>
On Fri, Sep 21, 2012 at 01:43:45PM +0200, Sascha Hauer wrote:
> We are currently discussing a common panel description for the
> devicetree. You are invited to join in here:
>
> http://comments.gmane.org/gmane.linux.drivers.devicetree/21386
>
> We shouldn't add any more device specific panel descriptions.
Just make sure that you don't end up encoding in integers in the device
tree Linux kernel specific bitfields such as the 'sync' property above.
Also, look at how the AMBA CLCD driver handles the panels with its
capability bitmask for the colour formats - this allows the driver to
determine what colour formats are supported, and allows platform code
to work out how to program any MUXes for any particular panel and
requested colour depth setting. It also allows figuring out whether
BGR or RGB format should be used.
This is necessary because of the beweildering array of panels, muxes
and other funky hardware which ARM Ltd like to put into their boards,
and the fact that the LCDs tend to be entirely separate entities to
the boards themselves.
^ permalink raw reply
* Re: [RFC PATCH 1/3] amba-clcd: Add Device Tree support to amba-clcd driver
From: Sascha Hauer @ 2012-09-21 11:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1348070666-9153-2-git-send-email-ryan.harkin@linaro.org>
On Wed, Sep 19, 2012 at 05:04:24PM +0100, Ryan Harkin wrote:
> Add support to parse the display configuration from device tree.
>
> If the board does not provide platform specific functions in the struct
> clcd_board contained with the amba device info, then defaults are provided
> by the driver.
>
> The device tree configuration can either ask for a DMA setup or provide a
> framebuffer address to be remapped into the driver.
>
> Signed-off-by: Ryan Harkin <ryan.harkin@linaro.org>
> ---
[...]
> +
> +struct clcd_panel *getPanel(struct device_node *node)
> +{
> + static struct clcd_panel panel;
> +
> + panel.mode.refresh = get_val(node, "refresh");
> + panel.mode.xres = get_val(node, "xres");
> + panel.mode.yres = get_val(node, "yres");
> + panel.mode.pixclock = get_val(node, "pixclock");
> + panel.mode.left_margin = get_val(node, "left_margin");
> + panel.mode.right_margin = get_val(node, "right_margin");
> + panel.mode.upper_margin = get_val(node, "upper_margin");
> + panel.mode.lower_margin = get_val(node, "lower_margin");
> + panel.mode.hsync_len = get_val(node, "hsync_len");
> + panel.mode.vsync_len = get_val(node, "vsync_len");
> + panel.mode.sync = get_val(node, "sync");
We are currently discussing a common panel description for the
devicetree. You are invited to join in here:
http://comments.gmane.org/gmane.linux.drivers.devicetree/21386
We shouldn't add any more device specific panel descriptions.
Sascha
--
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
* Re: [RFC PATCH 1/3] amba-clcd: Add Device Tree support to amba-clcd driver
From: Russell King - ARM Linux @ 2012-09-21 11:02 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1348070666-9153-2-git-send-email-ryan.harkin@linaro.org>
This patch has a number of serious problems with it.
On Wed, Sep 19, 2012 at 05:04:24PM +0100, Ryan Harkin wrote:
> @@ -391,6 +394,19 @@ static int clcdfb_blank(int blank_mode, struct fb_info *info)
> }
> return 0;
> }
This needs a blank line.
> +int clcdfb_mmap_dma(struct clcd_fb *fb, struct vm_area_struct *vma)
> +{
> + return dma_mmap_writecombine(&fb->dev->dev, vma,
> + fb->fb.screen_base,
> + fb->fb.fix.smem_start,
> + fb->fb.fix.smem_len);
> +}
> +
> +void clcdfb_remove_dma(struct clcd_fb *fb)
> +{
> + dma_free_writecombine(&fb->dev->dev, fb->fb.fix.smem_len,
> + fb->fb.screen_base, fb->fb.fix.smem_start);
> +}
>
> static int clcdfb_mmap(struct fb_info *info,
> struct vm_area_struct *vma)
> @@ -542,12 +558,249 @@ static int clcdfb_register(struct clcd_fb *fb)
> return ret;
> }
>
> +struct string_lookup {
> + const char *string;
> + const u32 val;
> +};
> +
> +static struct string_lookup vmode_lookups[] = {
> + { "FB_VMODE_NONINTERLACED", FB_VMODE_NONINTERLACED},
> + { "FB_VMODE_INTERLACED", FB_VMODE_INTERLACED},
> + { "FB_VMODE_DOUBLE", FB_VMODE_DOUBLE},
> + { "FB_VMODE_ODD_FLD_FIRST", FB_VMODE_ODD_FLD_FIRST},
> + { NULL, 0 },
> +};
> +
> +static struct string_lookup tim2_lookups[] = {
> + { "TIM2_CLKSEL", TIM2_CLKSEL},
> + { "TIM2_IVS", TIM2_IVS},
> + { "TIM2_IHS", TIM2_IHS},
Inversion of the sync control signals are part of the framebuffer API, and
should not be specified as part of this register definition. Instead, they
should be specified using FB_SYNC_HOR_HIGH_ACT and FB_SYNC_VERT_HIGH_ACT.
Setting them in ->tim2 is bad news because it just confuses these settings.
> + { "TIM2_IOE", TIM2_IOE},
> + { "TIM2_BCD", TIM2_BCD},
> + { NULL, 0},
> +};
Another blank line required.
> +static struct string_lookup cntl_lookups[] = {
> + {"CNTL_LCDEN", CNTL_LCDEN},
Err, no.
> + {"CNTL_LCDBPP1", CNTL_LCDBPP1},
> + {"CNTL_LCDBPP2", CNTL_LCDBPP2},
> + {"CNTL_LCDBPP4", CNTL_LCDBPP4},
> + {"CNTL_LCDBPP8", CNTL_LCDBPP8},
> + {"CNTL_LCDBPP16", CNTL_LCDBPP16},
> + {"CNTL_LCDBPP16_565", CNTL_LCDBPP16_565},
> + {"CNTL_LCDBPP16_444", CNTL_LCDBPP16_444},
> + {"CNTL_LCDBPP24", CNTL_LCDBPP24},
The colour depth is derived from the video mode setting, which will be
overridden anyway - and the allowable depths are derived from the panel
capabilities and the board capabilities.
> + {"CNTL_LCDBW", CNTL_LCDBW},
> + {"CNTL_LCDTFT", CNTL_LCDTFT},
> + {"CNTL_LCDMONO8", CNTL_LCDMONO8},
> + {"CNTL_LCDDUAL", CNTL_LCDDUAL},
These four are properties of the panel, so they're fine.
> + {"CNTL_BGR", CNTL_BGR},
BGR is also overridden by the driver.
> + {"CNTL_BEBO", CNTL_BEBO},
> + {"CNTL_BEPO", CNTL_BEPO},
From what I remember, these are framebuffer layout control bits. They
aren't panel properties, and so they shouldn't be specified in DT.
> + {"CNTL_LCDPWR", CNTL_LCDPWR},
Err, no. Specifying power control here is bad news - have you read the
data sheet for the CLCD controller (which specifies a certain sequence
for LCDEN and LCDPWR)? Have you read the driver code and realized that
the driver controls these bits, and therefore specifying them in DT is
absurd?
> + {"CNTL_LCDVCOMP(1)", CNTL_LCDVCOMP(1)},
> + {"CNTL_LCDVCOMP(2)", CNTL_LCDVCOMP(2)},
> + {"CNTL_LCDVCOMP(3)", CNTL_LCDVCOMP(3)},
> + {"CNTL_LCDVCOMP(4)", CNTL_LCDVCOMP(4)},
> + {"CNTL_LCDVCOMP(5)", CNTL_LCDVCOMP(5)},
> + {"CNTL_LCDVCOMP(6)", CNTL_LCDVCOMP(6)},
> + {"CNTL_LCDVCOMP(7)", CNTL_LCDVCOMP(7)},
> + {"CNTL_LDMAFIFOTIME", CNTL_LDMAFIFOTIME},
> + {"CNTL_WATERMARK", CNTL_WATERMARK},
> + { NULL, 0},
> +};
Another blank line required.
> +static struct string_lookup caps_lookups[] = {
> + {"CLCD_CAP_RGB444", CLCD_CAP_RGB444},
> + {"CLCD_CAP_RGB5551", CLCD_CAP_RGB5551},
> + {"CLCD_CAP_RGB565", CLCD_CAP_RGB565},
> + {"CLCD_CAP_RGB888", CLCD_CAP_RGB888},
> + {"CLCD_CAP_BGR444", CLCD_CAP_BGR444},
> + {"CLCD_CAP_BGR5551", CLCD_CAP_BGR5551},
> + {"CLCD_CAP_BGR565", CLCD_CAP_BGR565},
> + {"CLCD_CAP_BGR888", CLCD_CAP_BGR888},
> + {"CLCD_CAP_444", CLCD_CAP_444},
> + {"CLCD_CAP_5551", CLCD_CAP_5551},
> + {"CLCD_CAP_565", CLCD_CAP_565},
> + {"CLCD_CAP_888", CLCD_CAP_888},
> + {"CLCD_CAP_RGB", CLCD_CAP_RGB},
> + {"CLCD_CAP_BGR", CLCD_CAP_BGR},
> + {"CLCD_CAP_ALL", CLCD_CAP_ALL},
> + { NULL, 0},
> +};
> +
> +u32 parse_setting(struct string_lookup *lookup, const char *name)
> +{
> + int i = 0;
> + while (lookup[i].string != NULL) {
> + if (strcmp(lookup[i].string, name) = 0)
> + return lookup[i].val;
> + ++i;
> + }
> + return -EINVAL;
> +}
Why is this non-static?
> +
> +u32 get_string_lookup(struct device_node *node, const char *name,
> + struct string_lookup *lookup)
> +{
> + const char *string;
> + int count, i, ret = 0;
> +
> + count = of_property_count_strings(node, name);
> + if (count >= 0)
> + for (i = 0; i < count; i++)
> + if (of_property_read_string_index(node, name, i,
> + &string) = 0)
> + ret |= parse_setting(lookup, string);
> + return ret;
> +}
And this?
> +
> +int get_val(struct device_node *node, const char *string)
> +{
> + u32 ret = 0;
> +
> + if (of_property_read_u32(node, string, &ret))
> + ret = -1;
> + return ret;
> +}
And this?
> +
> +struct clcd_panel *getPanel(struct device_node *node)
Have you read coding style? We don't use capitals in function names.
> +{
> + static struct clcd_panel panel;
> +
> + panel.mode.refresh = get_val(node, "refresh");
> + panel.mode.xres = get_val(node, "xres");
> + panel.mode.yres = get_val(node, "yres");
> + panel.mode.pixclock = get_val(node, "pixclock");
It's debatable whether we want to specify the pixel clock in DT or not -
it can be calculated from the other parameters here 1e12/(refresh *
htotal * vtotal) ps.
> + panel.mode.left_margin = get_val(node, "left_margin");
> + panel.mode.right_margin = get_val(node, "right_margin");
> + panel.mode.upper_margin = get_val(node, "upper_margin");
> + panel.mode.lower_margin = get_val(node, "lower_margin");
> + panel.mode.hsync_len = get_val(node, "hsync_len");
> + panel.mode.vsync_len = get_val(node, "vsync_len");
> + panel.mode.sync = get_val(node, "sync");
An integer sync property? You're exposing kernel internal definitions
into DT here which is unacceptable.
> + panel.bpp = get_val(node, "bpp");
> + panel.width = (signed short) get_val(node, "width");
> + panel.height = (signed short) get_val(node, "height");
> +
> + panel.mode.vmode = get_string_lookup(node, "vmode", vmode_lookups);
> + panel.tim2 = get_string_lookup(node, "tim2", tim2_lookups);
> + panel.cntl = get_string_lookup(node, "cntl", cntl_lookups);
> + panel.caps = get_string_lookup(node, "caps", caps_lookups);
> +
> + return &panel;
> +}
> +
> +struct clcd_panel *clcdfb_get_panel(const char *name)
Why is this exported?
> +{
> + struct device_node *node = NULL;
> + const char *mode;
> + struct clcd_panel *panel = NULL;
> +
> + do {
> + node = of_find_compatible_node(node, NULL, "panel");
> + if (node)
> + if (of_property_read_string(node, "mode", &mode) = 0)
> + if (strcmp(mode, name) = 0) {
> + panel = getPanel(node);
> + panel->mode.name = name;
> + }
> + } while (node != NULL);
> +
> + return panel;
> +}
> +
> +#ifdef CONFIG_OF
> +static int clcdfb_dt_init(struct clcd_fb *fb)
> +{
> + int err = 0;
> + struct device_node *node;
> + const char *mode;
> + dma_addr_t dma;
> + u32 use_dma;
> + const __be32 *prop;
> + int len, na, ns;
> + phys_addr_t fb_base, fb_size;
> +
> + node = fb->dev->dev.of_node;
> + if (!node)
> + return -ENODEV;
> +
> + na = of_n_addr_cells(node);
> + ns = of_n_size_cells(node);
> +
> + if (WARN_ON(of_property_read_string(node, "mode", &mode)))
> + return -ENODEV;
> +
> + fb->panel = clcdfb_get_panel(mode);
> + if (!fb->panel)
> + return -EINVAL;
> + fb->fb.fix.smem_len = fb->panel->mode.xres * fb->panel->mode.yres * 2;
> +
> + if (of_property_read_u32(node, "use_dma", &use_dma))
> + use_dma = 0;
> + if (use_dma) {
> + fb->fb.screen_base = dma_alloc_writecombine(&fb->dev->dev,
> + fb->fb.fix.smem_len, &dma, GFP_KERNEL);
> + if (!fb->fb.screen_base) {
> + pr_err("CLCD: unable to map framebuffer\n");
> + err = -ENOMEM;
> + } else
> + fb->fb.fix.smem_start = dma;
> + } else {
> + prop = of_get_property(node, "framebuffer", &len);
> + if (WARN_ON(!prop || len < (na + ns) * sizeof(*prop)))
> + return -EINVAL;
> + fb_base = of_read_number(prop, na);
> + fb_size = of_read_number(prop + na, ns);
> +
> + if (memblock_remove(fb_base, fb_size) != 0)
> + return -EINVAL;
No. You can not do this here, calling this function after the reserve
callback into the platform code has completed will corrupt the kernel.
> +
> + fb->fb.fix.smem_start = fb_base;
> + fb->fb.screen_base = ioremap_wc(fb->fb.fix.smem_start, fb_size);
> + }
> + return err;
> +}
> +#endif /* CONFIG_OF */
> +
> static int clcdfb_probe(struct amba_device *dev, const struct amba_id *id)
> {
> struct clcd_board *board = dev->dev.platform_data;
> struct clcd_fb *fb;
> int ret;
>
> +#ifdef CONFIG_OF
> + if (dev->dev.of_node) {
> + const __be32 *prop;
> + int len, na, ns;
> + phys_addr_t reg_base;
> +
> + na = of_n_addr_cells(dev->dev.of_node);
> + ns = of_n_size_cells(dev->dev.of_node);
> +
> + prop = of_get_property(dev->dev.of_node, "reg", &len);
> + if (WARN_ON(!prop || len < (na + ns) * sizeof(*prop)))
> + return -EINVAL;
> + reg_base = of_read_number(prop, na);
> +
> + if (dev->res.start != reg_base)
> + return -EINVAL;
> +
> + if (!board) {
> + board = kzalloc(sizeof(struct clcd_board), GFP_KERNEL);
> + if (!board)
> + return -EINVAL;
> + board->name = "Device Tree CLCD PL111";
> + board->caps = CLCD_CAP_5551 | CLCD_CAP_565;
This looks like it's been hard-coded for one particular board, and one
particular board only.
> + board->check = clcdfb_check;
> + board->decode = clcdfb_decode;
> + board->setup = clcdfb_dt_init;
> + board->mmap = clcdfb_mmap_dma;
> + board->remove = clcdfb_remove_dma;
> + }
> + }
> +#endif /* CONFIG_OF */
> +
> if (!board)
> return -EINVAL;
>
^ permalink raw reply
* Re: [RFC PATCH 1/3] amba-clcd: Add Device Tree support to amba-clcd driver
From: Pawel Moll @ 2012-09-21 10:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAD0U-h+Z1zQYTiEm+aKwNJa48SdHfs6-qUu4vNRwFr=mqxyXCg@mail.gmail.com>
On Fri, 2012-09-21 at 11:35 +0100, Ryan Harkin wrote:
> Good point. Sorry for my ignorance, but is there a place I should put
> such documentation?
Documentation/devicetree/bindings/fb/amba-clcd.txt :-)
> When the A9 CoreTile uses the on-tile CLCD controller, it can use DMA
> to handle the framebuffer. DMA is not available to the motherboard
> CLCD controller.
Uh, you probably mean that the motherboard CLCD must use the specialised
video memory, while the one in the test chip would normally use a buffer
allocated via the DMA API...
> I was trying to keep the properties simple, but they are more complex
> than just the two settings: use_dma and framebuffer. If use_dma is
> specified, the framebuffer property is not used; in this case, the
> framebuffer is allocated by the DMA framework and the framebuffer
> property is ignored. If use_dma is not present or is set to <0>, the
> code uses the framebuffer property to specify the address.
I'm not sure if the "framebuffer" property is really needed, at least in
the form you have it there now. I think I know you where you get it from
- HDLCD driver, right? It was sort-of-needed there, because we had to
reserve memory for the framebuffer, because you couldn't allocate big
enough (8MB if I remember correctly) area using the DMA alloc function.
But now, with the CMA in place it should be possible, so I believe it's
not even necessary in that case. Simply speaking - if the driver is not
told what address to use, it should get the memory on its own.
Now, as to the motherboard CLCD... That's where you actually could use
this property in the way you have there, to enforce the address of the
video RAM as the framebuffer. But maybe it would be better to have a
phandle to the video RAM node?
Cheers!
Paweł
^ permalink raw reply
* Re: [RFC PATCH 1/3] amba-clcd: Add Device Tree support to amba-clcd driver
From: Ryan Harkin @ 2012-09-21 10:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20120920102453.GG32603@e102652-lin.cambridge.arm.com>
Hi Liviu,
Thanks for your feedback. All good stuff...
On 20 September 2012 11:24, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> On Wed, Sep 19, 2012 at 05:04:24PM +0100, Ryan Harkin wrote:
>> Add support to parse the display configuration from device tree.
>>
>> If the board does not provide platform specific functions in the struct
>> clcd_board contained with the amba device info, then defaults are provided
>> by the driver.
>>
>> The device tree configuration can either ask for a DMA setup or provide a
>> framebuffer address to be remapped into the driver.
>>
>> Signed-off-by: Ryan Harkin <ryan.harkin@linaro.org>
>> ---
>> drivers/video/amba-clcd.c | 253 +++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 253 insertions(+)
>>
>> diff --git a/drivers/video/amba-clcd.c b/drivers/video/amba-clcd.c
>> index 0a2cce7..01dbad1 100644
>> --- a/drivers/video/amba-clcd.c
>> +++ b/drivers/video/amba-clcd.c
>> @@ -16,7 +16,10 @@
>> #include <linux/string.h>
>> #include <linux/slab.h>
>> #include <linux/delay.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/memblock.h>
>> #include <linux/mm.h>
>> +#include <linux/of.h>
>> #include <linux/fb.h>
>> #include <linux/init.h>
>> #include <linux/ioport.h>
>> @@ -391,6 +394,19 @@ static int clcdfb_blank(int blank_mode, struct fb_info *info)
>> }
>> return 0;
>> }
>> +int clcdfb_mmap_dma(struct clcd_fb *fb, struct vm_area_struct *vma)
>> +{
>> + return dma_mmap_writecombine(&fb->dev->dev, vma,
>> + fb->fb.screen_base,
>> + fb->fb.fix.smem_start,
>> + fb->fb.fix.smem_len);
>> +}
>> +
>> +void clcdfb_remove_dma(struct clcd_fb *fb)
>> +{
>> + dma_free_writecombine(&fb->dev->dev, fb->fb.fix.smem_len,
>> + fb->fb.screen_base, fb->fb.fix.smem_start);
>> +}
>>
>> static int clcdfb_mmap(struct fb_info *info,
>> struct vm_area_struct *vma)
>> @@ -542,12 +558,249 @@ static int clcdfb_register(struct clcd_fb *fb)
>> return ret;
>> }
>>
>> +struct string_lookup {
>> + const char *string;
>> + const u32 val;
>> +};
>> +
>> +static struct string_lookup vmode_lookups[] = {
>> + { "FB_VMODE_NONINTERLACED", FB_VMODE_NONINTERLACED},
>> + { "FB_VMODE_INTERLACED", FB_VMODE_INTERLACED},
>> + { "FB_VMODE_DOUBLE", FB_VMODE_DOUBLE},
>> + { "FB_VMODE_ODD_FLD_FIRST", FB_VMODE_ODD_FLD_FIRST},
>> + { NULL, 0 },
>> +};
>> +
>> +static struct string_lookup tim2_lookups[] = {
>> + { "TIM2_CLKSEL", TIM2_CLKSEL},
>> + { "TIM2_IVS", TIM2_IVS},
>> + { "TIM2_IHS", TIM2_IHS},
>> + { "TIM2_IPC", TIM2_IPC},
>> + { "TIM2_IOE", TIM2_IOE},
>> + { "TIM2_BCD", TIM2_BCD},
>> + { NULL, 0},
>> +};
>> +static struct string_lookup cntl_lookups[] = {
>> + {"CNTL_LCDEN", CNTL_LCDEN},
>> + {"CNTL_LCDBPP1", CNTL_LCDBPP1},
>> + {"CNTL_LCDBPP2", CNTL_LCDBPP2},
>> + {"CNTL_LCDBPP4", CNTL_LCDBPP4},
>> + {"CNTL_LCDBPP8", CNTL_LCDBPP8},
>> + {"CNTL_LCDBPP16", CNTL_LCDBPP16},
>> + {"CNTL_LCDBPP16_565", CNTL_LCDBPP16_565},
>> + {"CNTL_LCDBPP16_444", CNTL_LCDBPP16_444},
>> + {"CNTL_LCDBPP24", CNTL_LCDBPP24},
>> + {"CNTL_LCDBW", CNTL_LCDBW},
>> + {"CNTL_LCDTFT", CNTL_LCDTFT},
>> + {"CNTL_LCDMONO8", CNTL_LCDMONO8},
>> + {"CNTL_LCDDUAL", CNTL_LCDDUAL},
>> + {"CNTL_BGR", CNTL_BGR},
>> + {"CNTL_BEBO", CNTL_BEBO},
>> + {"CNTL_BEPO", CNTL_BEPO},
>> + {"CNTL_LCDPWR", CNTL_LCDPWR},
>> + {"CNTL_LCDVCOMP(1)", CNTL_LCDVCOMP(1)},
>> + {"CNTL_LCDVCOMP(2)", CNTL_LCDVCOMP(2)},
>> + {"CNTL_LCDVCOMP(3)", CNTL_LCDVCOMP(3)},
>> + {"CNTL_LCDVCOMP(4)", CNTL_LCDVCOMP(4)},
>> + {"CNTL_LCDVCOMP(5)", CNTL_LCDVCOMP(5)},
>> + {"CNTL_LCDVCOMP(6)", CNTL_LCDVCOMP(6)},
>> + {"CNTL_LCDVCOMP(7)", CNTL_LCDVCOMP(7)},
>> + {"CNTL_LDMAFIFOTIME", CNTL_LDMAFIFOTIME},
>> + {"CNTL_WATERMARK", CNTL_WATERMARK},
>> + { NULL, 0},
>> +};
>> +static struct string_lookup caps_lookups[] = {
>> + {"CLCD_CAP_RGB444", CLCD_CAP_RGB444},
>> + {"CLCD_CAP_RGB5551", CLCD_CAP_RGB5551},
>> + {"CLCD_CAP_RGB565", CLCD_CAP_RGB565},
>> + {"CLCD_CAP_RGB888", CLCD_CAP_RGB888},
>> + {"CLCD_CAP_BGR444", CLCD_CAP_BGR444},
>> + {"CLCD_CAP_BGR5551", CLCD_CAP_BGR5551},
>> + {"CLCD_CAP_BGR565", CLCD_CAP_BGR565},
>> + {"CLCD_CAP_BGR888", CLCD_CAP_BGR888},
>> + {"CLCD_CAP_444", CLCD_CAP_444},
>> + {"CLCD_CAP_5551", CLCD_CAP_5551},
>> + {"CLCD_CAP_565", CLCD_CAP_565},
>> + {"CLCD_CAP_888", CLCD_CAP_888},
>> + {"CLCD_CAP_RGB", CLCD_CAP_RGB},
>> + {"CLCD_CAP_BGR", CLCD_CAP_BGR},
>> + {"CLCD_CAP_ALL", CLCD_CAP_ALL},
>> + { NULL, 0},
>> +};
>> +
>> +u32 parse_setting(struct string_lookup *lookup, const char *name)
>> +{
>> + int i = 0;
>> + while (lookup[i].string != NULL) {
>> + if (strcmp(lookup[i].string, name) = 0)
>> + return lookup[i].val;
>> + ++i;
>> + }
>> + return -EINVAL;
>> +}
>> +
>> +u32 get_string_lookup(struct device_node *node, const char *name,
>> + struct string_lookup *lookup)
>> +{
>
> I have this feeling that swapping the names of the two functions above
> would reflect better their actual functionality.
I see what you mean. I'm happy to swap them if there are no objections.
>
>> + const char *string;
>> + int count, i, ret = 0;
>> +
>> + count = of_property_count_strings(node, name);
>> + if (count >= 0)
>> + for (i = 0; i < count; i++)
>> + if (of_property_read_string_index(node, name, i,
>> + &string) = 0)
>> + ret |= parse_setting(lookup, string);
>> + return ret;
>> +}
>> +
>> +int get_val(struct device_node *node, const char *string)
>> +{
>> + u32 ret = 0;
>> +
>> + if (of_property_read_u32(node, string, &ret))
>> + ret = -1;
>> + return ret;
>> +}
>> +
>> +struct clcd_panel *getPanel(struct device_node *node)
>> +{
>> + static struct clcd_panel panel;
>> +
>> + panel.mode.refresh = get_val(node, "refresh");
>> + panel.mode.xres = get_val(node, "xres");
>> + panel.mode.yres = get_val(node, "yres");
>> + panel.mode.pixclock = get_val(node, "pixclock");
>> + panel.mode.left_margin = get_val(node, "left_margin");
>> + panel.mode.right_margin = get_val(node, "right_margin");
>> + panel.mode.upper_margin = get_val(node, "upper_margin");
>> + panel.mode.lower_margin = get_val(node, "lower_margin");
>> + panel.mode.hsync_len = get_val(node, "hsync_len");
>> + panel.mode.vsync_len = get_val(node, "vsync_len");
>> + panel.mode.sync = get_val(node, "sync");
>> + panel.bpp = get_val(node, "bpp");
>> + panel.width = (signed short) get_val(node, "width");
>> + panel.height = (signed short) get_val(node, "height");
>> +
>> + panel.mode.vmode = get_string_lookup(node, "vmode", vmode_lookups);
>> + panel.tim2 = get_string_lookup(node, "tim2", tim2_lookups);
>> + panel.cntl = get_string_lookup(node, "cntl", cntl_lookups);
>> + panel.caps = get_string_lookup(node, "caps", caps_lookups);
>> +
>> + return &panel;
>> +}
>> +
>> +struct clcd_panel *clcdfb_get_panel(const char *name)
>> +{
>> + struct device_node *node = NULL;
>> + const char *mode;
>> + struct clcd_panel *panel = NULL;
>> +
>> + do {
>> + node = of_find_compatible_node(node, NULL, "panel");
>> + if (node)
>> + if (of_property_read_string(node, "mode", &mode) = 0)
>> + if (strcmp(mode, name) = 0) {
>> + panel = getPanel(node);
>> + panel->mode.name = name;
>> + }
>> + } while (node != NULL);
>> +
>> + return panel;
>> +}
>> +
>> +#ifdef CONFIG_OF
>
> shouldn't this #ifdef be earlier? you are calling of_property_read_string()
> and while there are empty definitions if CONFIG_OF is not defined, the code
> will do nothing in that case. Is that intended? The clcdfb_get_panel()
> function only gets called if CONFIG_OF *is* defined.
Agree.
>
>
>> +static int clcdfb_dt_init(struct clcd_fb *fb)
>> +{
>> + int err = 0;
>> + struct device_node *node;
>> + const char *mode;
>> + dma_addr_t dma;
>> + u32 use_dma;
>> + const __be32 *prop;
>> + int len, na, ns;
>> + phys_addr_t fb_base, fb_size;
>> +
>> + node = fb->dev->dev.of_node;
>> + if (!node)
>> + return -ENODEV;
>> +
>> + na = of_n_addr_cells(node);
>> + ns = of_n_size_cells(node);
>> +
>> + if (WARN_ON(of_property_read_string(node, "mode", &mode)))
>> + return -ENODEV;
>> +
>> + fb->panel = clcdfb_get_panel(mode);
>> + if (!fb->panel)
>> + return -EINVAL;
>> + fb->fb.fix.smem_len = fb->panel->mode.xres * fb->panel->mode.yres * 2;
>> +
>> + if (of_property_read_u32(node, "use_dma", &use_dma))
>
> I haven't seen any documentation for this property. What's the intended use?
Good point. Sorry for my ignorance, but is there a place I should put
such documentation?
When the A9 CoreTile uses the on-tile CLCD controller, it can use DMA
to handle the framebuffer. DMA is not available to the motherboard
CLCD controller.
I was trying to keep the properties simple, but they are more complex
than just the two settings: use_dma and framebuffer. If use_dma is
specified, the framebuffer property is not used; in this case, the
framebuffer is allocated by the DMA framework and the framebuffer
property is ignored. If use_dma is not present or is set to <0>, the
code uses the framebuffer property to specify the address.
>
>> + use_dma = 0;
>> + if (use_dma) {
>> + fb->fb.screen_base = dma_alloc_writecombine(&fb->dev->dev,
>> + fb->fb.fix.smem_len, &dma, GFP_KERNEL);
>> + if (!fb->fb.screen_base) {
>> + pr_err("CLCD: unable to map framebuffer\n");
>> + err = -ENOMEM;
>> + } else
>> + fb->fb.fix.smem_start = dma;
>> + } else {
>> + prop = of_get_property(node, "framebuffer", &len);
>> + if (WARN_ON(!prop || len < (na + ns) * sizeof(*prop)))
>> + return -EINVAL;
>> + fb_base = of_read_number(prop, na);
>> + fb_size = of_read_number(prop + na, ns);
>> +
>> + if (memblock_remove(fb_base, fb_size) != 0)
>> + return -EINVAL;
>> +
>> + fb->fb.fix.smem_start = fb_base;
>> + fb->fb.screen_base = ioremap_wc(fb->fb.fix.smem_start, fb_size);
>> + }
>> + return err;
>> +}
>> +#endif /* CONFIG_OF */
>> +
>> static int clcdfb_probe(struct amba_device *dev, const struct amba_id *id)
>> {
>> struct clcd_board *board = dev->dev.platform_data;
>> struct clcd_fb *fb;
>> int ret;
>>
>> +#ifdef CONFIG_OF
>> + if (dev->dev.of_node) {
>> + const __be32 *prop;
>> + int len, na, ns;
>> + phys_addr_t reg_base;
>> +
>> + na = of_n_addr_cells(dev->dev.of_node);
>> + ns = of_n_size_cells(dev->dev.of_node);
>> +
>> + prop = of_get_property(dev->dev.of_node, "reg", &len);
>> + if (WARN_ON(!prop || len < (na + ns) * sizeof(*prop)))
>> + return -EINVAL;
>> + reg_base = of_read_number(prop, na);
>> +
>> + if (dev->res.start != reg_base)
>> + return -EINVAL;
>> +
>> + if (!board) {
>> + board = kzalloc(sizeof(struct clcd_board), GFP_KERNEL);
>> + if (!board)
>> + return -EINVAL;
>> + board->name = "Device Tree CLCD PL111";
>> + board->caps = CLCD_CAP_5551 | CLCD_CAP_565;
>> + board->check = clcdfb_check;
>> + board->decode = clcdfb_decode;
>> + board->setup = clcdfb_dt_init;
>> + board->mmap = clcdfb_mmap_dma;
>> + board->remove = clcdfb_remove_dma;
>> + }
>> + }
>> +#endif /* CONFIG_OF */
>> +
>> if (!board)
>> return -EINVAL;
>>
>> --
>> 1.7.9.5
>>
>>
>
> --
> ==========
> | I would like to |
> | fix the world, |
> | but they're not |
> | giving me the |
> \ source code! /
> ---------------
> ¯\_(ツ)_/¯
>
> -- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
>
^ permalink raw reply
* Re: [alsa-devel] [PATCH v2 00/34] i.MX multi-platform support
From: Olof Johansson @ 2012-09-21 8:26 UTC (permalink / raw)
To: Shawn Guo
Cc: alsa-devel, linux-usb, Artem Bityutskiy, linux-fbdev,
Wim Van Sebroeck, linux-mtd, linux-i2c, rtc-linux,
Florian Tobias Schandinat, Paulius Zaleckas, Chris Ball,
linux-media, linux-watchdog, Arnd Bergmann, Sascha Hauer,
Rob Herring, linux-arm-kernel, Vinod Koul, Greg Kroah-Hartman,
Mark Brown, linux-mmc, Wolfram Sang, Javier Martin, Andrew Morton
In-Reply-To: <20120921080123.GM2450@S2101-09.ap.freescale.net>
On Fri, Sep 21, 2012 at 1:01 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
> On Thu, Sep 20, 2012 at 03:56:56PM +0000, Arnd Bergmann wrote:
>> Ok, fair enough. I think we can put it in arm-soc/for-next as a staging
>> branch anyway to give it some exposure to linux-next, and then we can
>> decide whether a rebase is necessary before sending it to Linus.
>>
> I just saw the announcement from Olof - no more major merge for 3.7
> will be accepted from now on. Can this be an exception or should I
> plan this for 3.8?
I'll take a look at merging it tomorrow after I've dealt with smp_ops;
if it looks reasonably conflict-free I'll pull it in. We need the
sound dependency sorted out (or agreed upon) first though.
-Olof
^ permalink raw reply
* Re: [alsa-devel] [PATCH v2 00/34] i.MX multi-platform support
From: Shawn Guo @ 2012-09-21 8:01 UTC (permalink / raw)
To: Arnd Bergmann, Olof Johansson
Cc: alsa-devel, Mark Brown, Artem Bityutskiy, linux-fbdev,
Wim Van Sebroeck, linux-mtd, linux-i2c, Florian Tobias Schandinat,
Paulius Zaleckas, Chris Ball, linux-media, linux-watchdog,
rtc-linux, Sascha Hauer, Rob Herring, linux-arm-kernel,
Vinod Koul, Greg Kroah-Hartman, linux-usb, linux-mmc,
Wolfram Sang, Javier Martin, Andrew Morton, Guennadi
In-Reply-To: <201209201556.57171.arnd@arndb.de>
On Thu, Sep 20, 2012 at 03:56:56PM +0000, Arnd Bergmann wrote:
> Ok, fair enough. I think we can put it in arm-soc/for-next as a staging
> branch anyway to give it some exposure to linux-next, and then we can
> decide whether a rebase is necessary before sending it to Linus.
>
I just saw the announcement from Olof - no more major merge for 3.7
will be accepted from now on. Can this be an exception or should I
plan this for 3.8?
Shawn
^ permalink raw reply
* Re: [PATCHv3 1/4] video: mmp display subsystem
From: Zhou Zhu @ 2012-09-21 7:50 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1347007704-27645-1-git-send-email-zzhu3@marvell.com>
Hi, Florian,
Would you please help to apply these patches?
Looks they have already been submitted for a long time, and no more
review comments recently.
Thank you for your help!
> On Fri, Sep 7, 2012 at 4:48 PM, Zhou Zhu <zzhu3@marvell.com> wrote:
>> Added mmp display subsystem to support Marvell MMP display controllers.
>>
>> This subsystem contains 4 parts:
>> --fb folder
>> --core.c
>> --hw folder
>> --panel folder
>>
>> 1. fb folder contains implementation of fb.
>> fb get path and ovly from common interface and operates on these structures.
>>
>> 2. core.c provides common interface for a hardware abstraction.
>> Major parts of this interface are:
>> a) Path: path is a output device connected to a panel or HDMI TV.
>> Main operations of the path is set/get timing/output color.
>> fb operates output device through path structure.
>> b) Ovly: Ovly is a buffer shown on the path.
>> Ovly describes frame buffer and its source/destination size, offset, input
>> color, buffer address, z-order, and so on.
>> Each fb device maps to one ovly.
>>
>> 3. hw folder contains implementation of hardware operations defined by core.c.
>> It registers paths for fb use.
>>
>> 4. panel folder contains implementation of panels.
>> It's connected to path. Panel drivers would also regiester panels and linked
>> to path when probe.
>>
>> Signed-off-by: Zhou Zhu <zzhu3@marvell.com>
>> Signed-off-by: Lisa Du <cldu@marvell.com>
>
> Thanks,
> -Zhou
--
Thanks,
-Zhou
^ permalink raw reply
* RE: [PATCH v2 2/2] video: exynos_mipi_dsi: Remove unnecessary NULL check
From: Inki Dae @ 2012-09-21 6:01 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1348204007-1347-2-git-send-email-sachin.kamat@linaro.org>
Acked-by: Inki Dae <inki.dae@samsung.com>
> -----Original Message-----
> From: Sachin Kamat [mailto:sachin.kamat@linaro.org]
> Sent: Friday, September 21, 2012 2:07 PM
> To: linux-fbdev@vger.kernel.org
> Cc: inki.dae@samsung.com; FlorianSchandinat@gmx.de;
> sachin.kamat@linaro.org; patches@linaro.org
> Subject: [PATCH v2 2/2] video: exynos_mipi_dsi: Remove unnecessary NULL
> check
>
> 'dsim' is allocated and checked for NULL in the probe function.
> Hence this check is redundant. This cleanup also fixes a potential NULL
> pointer dereference error when dsim which is NULL references its member
> in the error print message.
>
> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> ---
> drivers/video/exynos/exynos_mipi_dsi_common.c | 5 -----
> 1 files changed, 0 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/video/exynos/exynos_mipi_dsi_common.c
> b/drivers/video/exynos/exynos_mipi_dsi_common.c
> index 7cc4113..3cd29a4 100644
> --- a/drivers/video/exynos/exynos_mipi_dsi_common.c
> +++ b/drivers/video/exynos/exynos_mipi_dsi_common.c
> @@ -79,11 +79,6 @@ irqreturn_t exynos_mipi_dsi_interrupt_handler(int irq,
> void *dev_id)
> struct mipi_dsim_device *dsim = dev_id;
> unsigned int intsrc, intmsk;
>
> - if (dsim = NULL) {
> - dev_err(dsim->dev, "%s: wrong parameter\n", __func__);
> - return IRQ_NONE;
> - }
> -
> intsrc = exynos_mipi_dsi_read_interrupt(dsim);
> intmsk = exynos_mipi_dsi_read_interrupt_mask(dsim);
> intmsk = ~intmsk & intsrc;
> --
> 1.7.4.1
^ permalink raw reply
* RE: [PATCH v2 1/2] video: exynos_mipi_dsi: Remove unused code
From: Inki Dae @ 2012-09-21 6:01 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1348204007-1347-1-git-send-email-sachin.kamat@linaro.org>
Acked-by: Inki Dae <inki.dae@samsung.com>
> -----Original Message-----
> From: Sachin Kamat [mailto:sachin.kamat@linaro.org]
> Sent: Friday, September 21, 2012 2:07 PM
> To: linux-fbdev@vger.kernel.org
> Cc: inki.dae@samsung.com; FlorianSchandinat@gmx.de;
> sachin.kamat@linaro.org; patches@linaro.org
> Subject: [PATCH v2 1/2] video: exynos_mipi_dsi: Remove unused code
>
> This code is never executed and hence removed.
>
> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> Acked-by: Inki Dae <inki.dae@samsung.com>
>
> ---
> drivers/video/exynos/exynos_mipi_dsi_common.c | 3 ---
> 1 files changed, 0 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/video/exynos/exynos_mipi_dsi_common.c
> b/drivers/video/exynos/exynos_mipi_dsi_common.c
> index 47b533a..7cc4113 100644
> --- a/drivers/video/exynos/exynos_mipi_dsi_common.c
> +++ b/drivers/video/exynos/exynos_mipi_dsi_common.c
> @@ -288,9 +288,6 @@ int exynos_mipi_dsi_wr_data(struct mipi_dsim_device
> *dsim, unsigned int data_id,
> mutex_unlock(&dsim->lock);
> return -EINVAL;
> }
> -
> - mutex_unlock(&dsim->lock);
> - return 0;
> }
>
> static unsigned int exynos_mipi_dsi_long_data_rd(struct mipi_dsim_device
> *dsim,
> --
> 1.7.4.1
^ permalink raw reply
* [PATCH v2 2/2] video: exynos_mipi_dsi: Remove unnecessary NULL check
From: Sachin Kamat @ 2012-09-21 5:18 UTC (permalink / raw)
To: linux-fbdev
'dsim' is allocated and checked for NULL in the probe function.
Hence this check is redundant. This cleanup also fixes a potential NULL
pointer dereference error when dsim which is NULL references its member
in the error print message.
Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
---
drivers/video/exynos/exynos_mipi_dsi_common.c | 5 -----
1 files changed, 0 insertions(+), 5 deletions(-)
diff --git a/drivers/video/exynos/exynos_mipi_dsi_common.c b/drivers/video/exynos/exynos_mipi_dsi_common.c
index 7cc4113..3cd29a4 100644
--- a/drivers/video/exynos/exynos_mipi_dsi_common.c
+++ b/drivers/video/exynos/exynos_mipi_dsi_common.c
@@ -79,11 +79,6 @@ irqreturn_t exynos_mipi_dsi_interrupt_handler(int irq, void *dev_id)
struct mipi_dsim_device *dsim = dev_id;
unsigned int intsrc, intmsk;
- if (dsim = NULL) {
- dev_err(dsim->dev, "%s: wrong parameter\n", __func__);
- return IRQ_NONE;
- }
-
intsrc = exynos_mipi_dsi_read_interrupt(dsim);
intmsk = exynos_mipi_dsi_read_interrupt_mask(dsim);
intmsk = ~intmsk & intsrc;
--
1.7.4.1
^ permalink raw reply related
* [PATCH v2 1/2] video: exynos_mipi_dsi: Remove unused code
From: Sachin Kamat @ 2012-09-21 5:18 UTC (permalink / raw)
To: linux-fbdev
This code is never executed and hence removed.
Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
Acked-by: Inki Dae <inki.dae@samsung.com>
---
drivers/video/exynos/exynos_mipi_dsi_common.c | 3 ---
1 files changed, 0 insertions(+), 3 deletions(-)
diff --git a/drivers/video/exynos/exynos_mipi_dsi_common.c b/drivers/video/exynos/exynos_mipi_dsi_common.c
index 47b533a..7cc4113 100644
--- a/drivers/video/exynos/exynos_mipi_dsi_common.c
+++ b/drivers/video/exynos/exynos_mipi_dsi_common.c
@@ -288,9 +288,6 @@ int exynos_mipi_dsi_wr_data(struct mipi_dsim_device *dsim, unsigned int data_id,
mutex_unlock(&dsim->lock);
return -EINVAL;
}
-
- mutex_unlock(&dsim->lock);
- return 0;
}
static unsigned int exynos_mipi_dsi_long_data_rd(struct mipi_dsim_device *dsim,
--
1.7.4.1
^ permalink raw reply related
* Re: [PATCHv4 7/9] arm: vt8500: doc: Add device tree bindings for arch-vt8500 devices
From: Rob Herring @ 2012-09-20 23:13 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1345707346-9035-8-git-send-email-linux@prisktech.co.nz>
On 08/23/2012 02:35 AM, Tony Prisk wrote:
> Bindings for gpio, interrupt controller, power management controller,
> timer, realtime clock, serial uart, ehci and uhci controllers and
> framebuffer controllers used on the arch-vt8500 platform.
>
> Framebuffer binding also specifies a 'display' node which is required
> for determining the lcd panel data.
Overall, these bindings look okay, but there is some effort to
standardize a binding for displays:
http://comments.gmane.org/gmane.linux.drivers.devicetree/21386
I would like to see that used rather than something custom.
Rob
^ permalink raw reply
* Re: [PATCH 2/5] drivers/video/gbefb.c: use devm_ functions
From: Florian Tobias Schandinat @ 2012-09-20 21:58 UTC (permalink / raw)
To: Geert Uytterhoeven, Damien Cassou
Cc: kernel-janitors, linux-fbdev, linux-kernel
In-Reply-To: <CAMuHMdWsiXmt=2uo3C8ZC_apnHRV2aHi3yZrYV_jgwB22=3Mfw@mail.gmail.com>
Hi Geert,
On 09/13/2012 07:06 PM, Geert Uytterhoeven wrote:
> On Thu, Aug 23, 2012 at 10:37 PM, Florian Tobias Schandinat
> <FlorianSchandinat@gmx.de> wrote:
>> On 07/31/2012 04:39 PM, Damien Cassou wrote:
>>> From: Damien Cassou <damien.cassou@lifl.fr>
>>>
>>> The various devm_ functions allocate memory that is released when a driver
>>> detaches. This patch uses these functions for data that is allocated in
>>> the probe function of a platform device and is only freed in the remove
>>> function.
>>>
>>> Signed-off-by: Damien Cassou <damien.cassou@lifl.fr>
>>
>> Applied.
>
>>> diff --git a/drivers/video/gbefb.c b/drivers/video/gbefb.c
>>> index 7e7b7a9..9b79d38 100644
>>> --- a/drivers/video/gbefb.c
>>> +++ b/drivers/video/gbefb.c
>>> @@ -1156,7 +1156,8 @@ static int __devinit gbefb_probe(struct platform_device *p_dev)
>>> goto out_release_framebuffer;
>>> }
>>>
>>> - gbe = (struct sgi_gbe *) ioremap(GBE_BASE, sizeof(struct sgi_gbe));
>>> + gbe = (struct sgi_gbe *) devm_ioremap(&p_dev->dev, GBE_BASE,
>>> + sizeof(struct sgi_gbe));
>
> drivers/video/gbefb.c:1159:16: error: implicit declaration of function
> 'devm_ioremap' [-Werror=implicit-function-declaration]
> drivers/video/gbefb.c:1179:3: error: implicit declaration of function
> 'devm_ioremap_nocache' [-Werror=implicit-function-declaration]
Thanks for pointing this out. I guess the solution is to replace the
include asm/io.h by linux/io.h as Axel Lin did for some other driver in
this patch series.
@Damien:
Can you please do a patch as I suggested and preferable also recheck the
other drivers you touched?
And it probably would have been a good idea to mention that you didn't
compile-test it. I do some compile tests before pushing my branch but
that is far from complete due to arch and platform specific drivers.
Best regards,
Florian Tobias Schandinat
>
> http://kisskb.ellerman.id.au/kisskb/buildresult/7187731/
>
> 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: [PATCH v4] of: Add videomode helper
From: Laurent Pinchart @ 2012-09-20 21:29 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Steffen Trumtrar, devicetree-discuss, linux-fbdev, dri-devel,
kernel, Sascha Hauer, linux-media
In-Reply-To: <1348046362.2565.16.camel@deskari>
Hi,
(CC'ing the linux-media mailing list, as video modes are of interest there as
well)
On Wednesday 19 September 2012 12:19:22 Tomi Valkeinen wrote:
> On Wed, 2012-09-19 at 10:20 +0200, Steffen Trumtrar wrote:
> > This patch adds a helper function for parsing videomodes from the
> > devicetree. The videomode can be either converted to a struct
> > drm_display_mode or a struct fb_videomode.
> >
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> > ---
> >
> > Hi!
> >
> > changes since v3:
> > - print error messages
> > - free alloced memory
> > - general cleanup
> >
> > Regards
> > Steffen
> >
> > .../devicetree/bindings/video/displaymode | 74 +++++
> > drivers/of/Kconfig | 5 +
> > drivers/of/Makefile | 1 +
> > drivers/of/of_videomode.c | 283 +++++++++++++++
> > include/linux/of_videomode.h | 56 ++++
> > 5 files changed, 419 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/video/displaymode
> > create mode 100644 drivers/of/of_videomode.c
> > create mode 100644 include/linux/of_videomode.h
> >
> > diff --git a/Documentation/devicetree/bindings/video/displaymode
> > b/Documentation/devicetree/bindings/video/displaymode new file mode
> > 100644
> > index 0000000..990ca52
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/video/displaymode
> > @@ -0,0 +1,74 @@
> > +videomode bindings
> > +=========
> > +
> > +Required properties:
> > + - hactive, vactive: Display resolution
> > + - hfront-porch, hback-porch, hsync-len: Horizontal Display timing
> > parameters
> > + in pixels
> > + vfront-porch, vback-porch, vsync-len: Vertical display timing
> > parameters in
> > + lines
> > + - clock: displayclock in Hz
> > +
> > +Optional properties:
> > + - width-mm, height-mm: Display dimensions in mm
> > + - hsync-active-high (bool): Hsync pulse is active high
> > + - vsync-active-high (bool): Vsync pulse is active high
> > + - interlaced (bool): This is an interlaced mode
> > + - doublescan (bool): This is a doublescan mode
> > +
> > +There are different ways of describing a display mode. The devicetree
> > representation
> > +corresponds to the one commonly found in datasheets for displays.
> > +The description of the display and its mode is split in two parts: first
> > the display
> > +properties like size in mm and (optionally) multiple subnodes with the
> > supported modes.
> > +
> > +Example:
> > +
> > + display@0 {
> > + width-mm = <800>;
> > + height-mm = <480>;
> > + modes {
> > + mode0: mode@0 {
> > + /* 1920x1080p24 */
> > + clock = <52000000>;
> > + hactive = <1920>;
> > + vactive = <1080>;
> > + hfront-porch = <25>;
> > + hback-porch = <25>;
> > + hsync-len = <25>;
> > + vback-porch = <2>;
> > + vfront-porch = <2>;
> > + vsync-len = <2>;
> > + hsync-active-high;
> > + };
> > + };
> > + };
> > +
> > +Every property also supports the use of ranges, so the commonly used
> > datasheet +description with <min typ max>-tuples can be used.
> > +
> > +Example:
> > +
> > + mode1: mode@1 {
> > + /* 1920x1080p24 */
> > + clock = <148500000>;
> > + hactive = <1920>;
> > + vactive = <1080>;
> > + hsync-len = <0 44 60>;
> > + hfront-porch = <80 88 95>;
> > + hback-porch = <100 148 160>;
> > + vfront-porch = <0 4 6>;
> > + vback-porch = <0 36 50>;
> > + vsync-len = <0 5 6>;
> > + };
> > +
> > +The videomode can be linked to a connector via phandles. The connector
> > has to
> > +support the display- and default-mode-property to link to and select a
> > mode.
> > +
> > +Example:
> > +
> > + hdmi@00120000 {
> > + status = "okay";
> > + display = <&benq>;
> > + default-mode = <&mode1>;
> > + };
> > +
> > diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> > index dfba3e6..a3acaa3 100644
> > --- a/drivers/of/Kconfig
> > +++ b/drivers/of/Kconfig
> > @@ -83,4 +83,9 @@ config OF_MTD
> >
> > depends on MTD
> > def_bool y
> >
> > +config OF_VIDEOMODE
> > + def_bool y
> > + help
> > + helper to parse videomodes from the devicetree
> > +
> >
> > endmenu # OF
> >
> > diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> > index e027f44..80e6db3 100644
> > --- a/drivers/of/Makefile
> > +++ b/drivers/of/Makefile
> > @@ -11,3 +11,4 @@ obj-$(CONFIG_OF_MDIO) += of_mdio.o
> >
> > obj-$(CONFIG_OF_PCI) += of_pci.o
> > obj-$(CONFIG_OF_PCI_IRQ) += of_pci_irq.o
> > obj-$(CONFIG_OF_MTD) += of_mtd.o
> >
> > +obj-$(CONFIG_OF_VIDEOMODE) += of_videomode.o
> > diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
> > new file mode 100644
> > index 0000000..52bfc74
> > --- /dev/null
> > +++ b/drivers/of/of_videomode.c
> > @@ -0,0 +1,283 @@
> > +/*
> > + * OF helpers for parsing display modes
> > + *
> > + * Copyright (c) 2012 Sascha Hauer <s.hauer@pengutronix.de>, Pengutronix
> > + * Copyright (c) 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>,
> > Pengutronix
> > + *
> > + * This file is released under the GPLv2
> > + */
> > +#include <linux/of.h>
> > +#include <linux/fb.h>
> > +#include <linux/export.h>
> > +#include <linux/slab.h>
> > +#include <drm/drmP.h>
> > +#include <drm/drm_crtc.h>
> > +#include <linux/of_videomode.h>
> > +
> > +static u32 of_video_get_value(struct mode_property *prop)
> > +{
> > + return (prop->min >= prop->typ) ? prop->min : prop->typ;
> > +}
>
> Why is this needed? If the prop->min is higher than prop->typ, isn't
> that an error in the DT data, and should be rejected when parsing?
>
> > +
> > +/* read property into new mode_property */
> > +static int of_video_parse_property(struct device_node *np, char *name,
> > + struct mode_property *result)
> > +{
> > + struct property *prop;
> > + int length;
> > + int cells;
> > + int ret;
> > +
> > + prop = of_find_property(np, name, &length);
> > + if (!prop) {
> > + pr_err("%s: could not find property %s\n", __func__,
> > + name);
> > + return -EINVAL;
> > + }
> > +
> > + cells = length / sizeof(u32);
> > +
> > + memset(result, 0, sizeof(*result));
> > +
> > + ret = of_property_read_u32_array(np, name, &result->min, cells);
> > +
> > + return ret;
>
> Ah, I guess this is the reason for the of_video_get_value... Wouldn't it
> be cleaner to be more explicit here? If there's one cell, it's the
> typical value, otherwise if there are 3 cells, they are min/typ/max,
> else it's an error.
I agree. We should flag errors and fail instead of trying to fix them
silently. That's the only way to make sure that device tree authors will get
it right (or at least less wrong). We might need to accept wrong DT data later
to support buggy devices found in the wild, but we should not start that way.
> And I think the above also trashes memory if there happens to be 4+ cells.
>
> > +}
> > +
> > +static int of_video_free(struct display *disp)
> > +{
> > + int i;
> > +
> > + for (i=0; i<disp->num_modes; i++)
Spaces around = and < please.
> > + kfree(disp->modes[i]);
> > + kfree(disp->modes);
> > +
> > + return 0;
> > +}
> > +
> > +int videomode_to_display_mode(struct display *disp, struct
> > drm_display_mode *dmode, int index)
Shouldn't this use drm_mode_modeinfo instead of drm_display_mode ?
> > +{
> > + struct videomode *vm;
> > +
> > + memset(dmode, 0, sizeof(*dmode));
> > +
> > + if (index > disp->num_modes) {
> > + pr_err("%s: wrong index: %d from %d\n", __func__, index,
> > disp->num_modes); + return -EINVAL;
> > + }
> > +
> > + vm = disp->modes[index];
> > +
> > + dmode->hdisplay = of_video_get_value(&vm->hactive);
> > + dmode->hsync_start = of_video_get_value(&vm->hactive) +
> > of_video_get_value(&vm->hfront_porch);
You could replace of_video_get_value(&vm->hactive) with dmode->hdisplay here,
and similarly below (hsync_end = hsync_start + of_video_get_value(&vm-
>hsync_len), ...). Beside shortening lines, it would save calls to
of_video_get_value().
> > + dmode->hsync_end = of_video_get_value(&vm->hactive) +
> > of_video_get_value(&vm->hfront_porch)
> > + + of_video_get_value(&vm->hsync_len);
> > + dmode->htotal = of_video_get_value(&vm->hactive) +
> > of_video_get_value(&vm->hfront_porch)
> > + + of_video_get_value(&vm->hsync_len) +
> > of_video_get_value(&vm->hback_porch);
> > + dmode->vdisplay = of_video_get_value(&vm->vactive);
> > + dmode->vsync_start = of_video_get_value(&vm->vactive) +
> > of_video_get_value(&vm->vfront_porch); + dmode->vsync_end > > of_video_get_value(&vm->vactive) + of_video_get_value(&vm->vfront_porch)
> > + + of_video_get_value(&vm->vsync_len);
> > + dmode->vtotal = of_video_get_value(&vm->vactive) +
> > of_video_get_value(&vm->vfront_porch) +
> > + of_video_get_value(&vm->vsync_len) +
> > of_video_get_value(&vm->vback_porch); +
> > + dmode->width_mm = disp->width_mm;
> > + dmode->height_mm = disp->height_mm;
> > +
> > + dmode->clock = of_video_get_value(&vm->clock) / 1000;
> > +
> > + if (vm->hah)
> > + dmode->flags |= DRM_MODE_FLAG_PHSYNC;
> > + else
> > + dmode->flags |= DRM_MODE_FLAG_NHSYNC;
> > + if (vm->vah)
> > + dmode->flags |= DRM_MODE_FLAG_PVSYNC;
> > + else
> > + dmode->flags |= DRM_MODE_FLAG_NVSYNC;
> > + if (vm->interlaced)
> > + dmode->flags |= DRM_MODE_FLAG_INTERLACE;
> > + if (vm->doublescan)
> > + dmode->flags |= DRM_MODE_FLAG_DBLSCAN;
> > +
> > + drm_mode_set_name(dmode);
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(videomode_to_display_mode);
> > +
> > +int videomode_to_fb_mode(struct display *disp, struct fb_videomode
> > *fbmode, int index)
disp is only used to retrieve the mode, you could pass a struct videomode
instead of disp and index.
Thinking about it, I would make videomode_to_display_mode take a struct
videomode as well. The only reason it needs struct display is to get the
display physical dimensions. Those are only used in very specific cases in the
DRM subsystem, I don't think they should be copied to every struct
drm_mode_modeinfo.
> > +{
> > + struct videomode *vm;
> > +
> > + memset(fbmode, 0, sizeof(*fbmode));
> > +
> > + if (index > disp->num_modes) {
> > + pr_err("%s: wrong index: %d from %d\n", __func__, index,
> > disp->num_modes);
> > + return -EINVAL;
> > + }
> > +
> > + vm = disp->modes[index];
> > +
> > + fbmode->xres = of_video_get_value(&vm->hactive);
> > + fbmode->left_margin = of_video_get_value(&vm->hback_porch);
> > + fbmode->right_margin = of_video_get_value(&vm->hfront_porch);
> > + fbmode->hsync_len = of_video_get_value(&vm->hsync_len);
> > +
> > + fbmode->yres = of_video_get_value(&vm->vactive);
> > + fbmode->upper_margin = of_video_get_value(&vm->vback_porch);
> > + fbmode->lower_margin = of_video_get_value(&vm->vfront_porch);
> > + fbmode->vsync_len = of_video_get_value(&vm->vsync_len);
> > +
> > + fbmode->pixclock = KHZ2PICOS(of_video_get_value(&vm->clock) / 1000);
> > +
> > + if (vm->hah)
> > + fbmode->sync |= FB_SYNC_HOR_HIGH_ACT;
> > + if (vm->vah)
> > + fbmode->sync |= FB_SYNC_VERT_HIGH_ACT;
> > + if (vm->interlaced)
> > + fbmode->vmode |= FB_VMODE_INTERLACED;
> > + if (vm->doublescan)
> > + fbmode->vmode |= FB_VMODE_DOUBLE;
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(videomode_to_fb_mode);
> > +
> > +int of_get_video_modes(struct device_node *np, struct display *disp)
> > +{
> > + struct device_node *display_np;
> > + struct device_node *mode_np;
> > + struct device_node *modes_np;
> > + char *default_mode;
> > +
> > + int ret = 0;
> > +
> > + memset(disp, 0, sizeof(*disp));
> > + disp->modes = kmalloc(sizeof(struct videomode*), GFP_KERNEL);
> > +
> > + if (!np) {
> > + pr_err("%s: no node provided\n", __func__);
> > + return -EINVAL;
> > + }
> > +
> > + display_np = of_parse_phandle(np, "display", 0);
> > +
> > + if (!display_np) {
> > + pr_err("%s: could not find display node\n", __func__);
> > + return -EINVAL;
> > + }
> > +
> > + of_property_read_u32(display_np, "width-mm", &disp->width_mm);
> > + of_property_read_u32(display_np, "height-mm", &disp->height_mm);
> > +
> > + mode_np = of_parse_phandle(np, "default-mode", 0);
> > +
> > + if (!mode_np) {
> > + pr_info("%s: no default-mode specified.\n", __func__);
> > + mode_np = of_find_node_by_name(np, "mode");
> > + }
> > +
> > + if (!mode_np) {
> > + pr_err("%s: could not find any mode specification\n", __func__);
> > + return -EINVAL;
> > + }
> > +
> > + default_mode = (char *)mode_np->full_name;
> > +
> > + modes_np = of_find_node_by_name(np, "modes");
> > + for_each_child_of_node(modes_np, mode_np) {
> > + struct videomode *vm;
> > +
> > + vm = kmalloc(sizeof(struct videomode*), GFP_KERNEL);
> > + disp->modes[disp->num_modes] = kmalloc(sizeof(struct videomode*),
> > GFP_KERNEL);
> > +
> > + ret |= of_video_parse_property(mode_np, "hback-porch",
> > &vm->hback_porch);
> > + ret |= of_video_parse_property(mode_np, "hfront-porch",
> > &vm->hfront_porch);
> > + ret |= of_video_parse_property(mode_np, "hactive", &vm->hactive);
> > + ret |= of_video_parse_property(mode_np, "hsync-len",
> > &vm->hsync_len);
> > + ret |= of_video_parse_property(mode_np, "vback-porch",
> > &vm->vback_porch);
> > + ret |= of_video_parse_property(mode_np, "vfront-porch",
> > &vm->vfront_porch);
> > + ret |= of_video_parse_property(mode_np, "vactive", &vm->vactive);
> > + ret |= of_video_parse_property(mode_np, "vsync-len",
> > &vm->vsync_len);
> > + ret |= of_video_parse_property(mode_np, "clock", &vm->clock);
> > +
> > + if (ret)
> > + return -EINVAL;
> > +
> > + vm->hah = of_property_read_bool(mode_np, "hsync-active-high");
> > + vm->vah = of_property_read_bool(mode_np, "vsync-active-high");
> > + vm->interlaced = of_property_read_bool(mode_np, "interlaced");
> > + vm->doublescan = of_property_read_bool(mode_np, "doublescan");
> > +
> > + if (strcmp(default_mode,mode_np->full_name) = 0)
> > + disp->default_mode = disp->num_modes;
> > +
> > + disp->modes[disp->num_modes] = vm;
> > + disp->num_modes++;
> > + }
> > + of_node_put(display_np);
> > +
> > + pr_info("%s: found %d modelines. Using #%d as default\n", __func__,
> > + disp->num_modes, disp->default_mode + 1);
> > +
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(of_get_video_modes);
> > +
> > +int of_video_mode_exists(struct device_node *np)
> > +{
> > + struct device_node *display_np;
> > + struct device_node *mode_np;
> > +
> > + if (!np)
> > + return -EINVAL;
> > +
> > + display_np = of_parse_phandle(np, "display", 0);
> > +
> > + if (!display_np)
> > + return -EINVAL;
> > +
> > + mode_np = of_parse_phandle(np, "default-mode", 0);
> > +
> > + if (mode_np)
> > + return 0;
> > +
> > + return -EINVAL;
> > +}
> > +EXPORT_SYMBOL_GPL(of_video_mode_exists);
> > +
> > +int of_get_display_mode(struct device_node *np, struct drm_display_mode
> > *dmode, int index)
> > +{
> > + struct display disp;
> > +
> > + of_get_video_modes(np, &disp);
> > +
> > + if (index = OF_MODE_SELECTION)
> > + index = disp.default_mode;
> > + if (dmode)
> > + videomode_to_display_mode(&disp, dmode, index);
> > +
> > + of_video_free(&disp);
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(of_get_display_mode);
> > +
> > +int of_get_fb_videomode(struct device_node *np, struct fb_videomode
> > *fbmode, int index)
> > +{
> > + struct display disp;
> > +
> > + of_get_video_modes(np, &disp);
> > +
> > + if (index = OF_MODE_SELECTION)
> > + index = disp.default_mode;
> > + if (fbmode)
> > + videomode_to_fb_mode(&disp, fbmode, index);
> > +
> > + of_video_free(&disp);
> > +
> > + return 0;
>
> This and of_get_display_mode() do not handle errors from
> of_get_video_modes() nor from videomode_to_xxx_mode(). And I don't see a
> reason for the if (fbmode) check (and the same for dmode), as there's no
> reason to call these functions except to get the video modes.
>
> > +}
> > +EXPORT_SYMBOL_GPL(of_get_fb_videomode);
What are those two functions used for ? Aren't drivers expected to parse the
modes into a struct display and then operate on that structure, instead of
getting individual modes from the DT node ?
> > diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h
> > new file mode 100644
> > index 0000000..5571ce3
> > --- /dev/null
> > +++ b/include/linux/of_videomode.h
> > @@ -0,0 +1,56 @@
> > +/*
> > + * Copyright 2012 Sascha Hauer <s.hauer@pengutronix.de>
> > + * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>
> > + *
> > + * OF helpers for videomodes.
> > + *
> > + * This file is released under the GPLv2
> > + */
> > +
> > +#ifndef __LINUX_OF_VIDEOMODE_H
> > +#define __LINUX_OF_VIDEOMODE_H
> > +
> > +#define OF_MODE_SELECTION -1
> > +
> > +struct mode_property {
> > + u32 min;
> > + u32 typ;
> > + u32 max;
> > +};
> > +
> > +struct display {
> > + u32 width_mm;
> > + u32 height_mm;
> > + struct videomode **modes;
> > + int default_mode;
> > + int num_modes;
default_mode and num_modes are non-negative, what about using an unsigned int
type for them ?
> > +};
> > +
> > +/* describe videomode in terms of hardware parameters */
> > +struct videomode {
> > + struct mode_property hback_porch;
> > + struct mode_property hfront_porch;
> > + struct mode_property hactive;
> > + struct mode_property hsync_len;
> > +
> > + struct mode_property vback_porch;
> > + struct mode_property vfront_porch;
> > + struct mode_property vactive;
> > + struct mode_property vsync_len;
> > +
> > + struct mode_property clock;
> > +
> > + bool hah;
> > + bool vah;
> > + bool interlaced;
> > + bool doublescan;
> > +};
>
> I think the names display and videomode are a bit too generic here, and
> could clash with names from kernel drivers. Also, the videomode is not
> really a videomode, but a "template" (or something) for videomode. A
> real videomode doesn't have min & max values, only the actual value.
>
> Perhaps these should be prefixed with "of_"? Then again, they are not
> really of specific either...
If feel this is an important topic.
A generic video mode structure is definitely needed, as well as helper
functions to convert between the new structure and existing video mode
structures (struct fb_videomode, struct drm_mode_modeinfo and struct
v4l2_bt_timings). The structure and the helper functions should be generic, as
the goal is to gradually replace subsystem-specific video mode structures
where possible (userspace APIs will still need to keep the old structures). We
thus need to drop the dependency on OF for everything but the DT parsing code.
For that reason an of_ prefix wouldn't be a good idea. As the goal is to
create a truly generic video mode structure, a generic name is a good idea (I
might prefer struct video_mode instead of struct videomode, but that's just
bikeshedding).
We might need two video mode structures, one to represent a video mode, and
one to represent a range of video modes. I don't have enough experience with
video modes to have a really strong opinion on this, having a single structure
to represent both could be useful as well. This particular topic needs to be
discussed.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH] pwm: Call pwm_enable() before pwm_config()
From: Mark Brown @ 2012-09-20 19:05 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20120823190402.GA8127@avionic-0098.mockup.avionic-design.de>
On Thu, Aug 23, 2012 at 09:04:02PM +0200, Thierry Reding wrote:
> Using runtime PM for this sounds indeed like the most generic approach.
> I'm not very familiar with the API, but I thought it required explicit
> architecture or bus support (or the driver itself can provide hooks via
> pm_ops), so it might be difficult to implement for platforms that don't
> have working runtime PM support. I'll have to look into this some more.
It doesn't require explicit arch support, though many architectures
choose to integrate their PM frameworks with it either directly or via
power domains. If the architecture doesn't do anything with it then it
should just work at the driver level.
^ permalink raw reply
* Re: [PATCH] pwm: Call pwm_enable() before pwm_config()
From: Mark Brown @ 2012-09-20 19:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <50366464.4070801@metafoo.de>
On Thu, Aug 23, 2012 at 07:12:04PM +0200, Lars-Peter Clausen wrote:
> On 08/23/2012 06:57 PM, Beno??t Th??baudeau wrote:
> > exists with several other PWM drivers.
> Since this seems to be a common pattern in a number of PWM drivers it might
> make sense to simply add support for enabling/disabling a clk to the pwm core.
> Or maybe just use the runtime pm API for this. This probably makes even more
> sense and grab a reference to the pm context when the enable() is called,
> release it when disable() is called and also grab it before calling the
> device's config callback and release it afterward.
It's a problem with IPs in general, a good proportion of modern SoCs
need a clock supplying in order to interact with the registers on most
of their IPs. runtime PM is the normal way forwards, Tegra recently
started using regmap's cache code to handle this transparently in the
driver.
^ 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