* Re: [PATCH v12 3/6] fbmon: add videomode helpers
From: Steffen Trumtrar @ 2012-11-21 11:58 UTC (permalink / raw)
To: Leela Krishna Amudala
Cc: Manjunathappa, Prakash, devicetree-discuss@lists.ozlabs.org,
linux-fbdev@vger.kernel.org, David Airlie,
Florian Tobias Schandinat, dri-devel@lists.freedesktop.org,
Valkeinen, Tomi, Laurent Pinchart, kernel@pengutronix.de,
Guennady Liakhovetski, linux-media@vger.kernel.org
In-Reply-To: <CAL1wa8dQ4QL0SzbXdo8nogBfBjQ8GpaJ134v6zu_iMkWQeXefA@mail.gmail.com>
Hi!
On Wed, Nov 21, 2012 at 04:39:01PM +0530, Leela Krishna Amudala wrote:
> Yes,
> Even I got the same build error.
> later I fixed it by including "#include <linux/mxsfb.h>"
>
> Best Wishes,
> Leela Krishna.
>
> On Wed, Nov 21, 2012 at 3:39 PM, Manjunathappa, Prakash
> <prakash.pm@ti.com> wrote:
> > Hi Steffen,
> >
> > I am trying to add DT support for da8xx-fb driver on top of your patches.
> > Encountered below build error. Sorry for reporting it late.
> >
> > On Tue, Nov 20, 2012 at 21:24:53, Steffen Trumtrar wrote:
> >> Add a function to convert from the generic videomode to a fb_videomode.
> >>
> >> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> >> Reviewed-by: Thierry Reding <thierry.reding@avionic-design.de>
> >> Acked-by: Thierry Reding <thierry.reding@avionic-design.de>
> >> Tested-by: Thierry Reding <thierry.reding@avionic-design.de>
> >> Tested-by: Philipp Zabel <p.zabel@pengutronix.de>
> >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> ---
> >> drivers/video/fbmon.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
> >> include/linux/fb.h | 6 ++++++
> >> 2 files changed, 52 insertions(+)
> >>
> >> diff --git a/drivers/video/fbmon.c b/drivers/video/fbmon.c
> >> index cef6557..c1939a6 100644
> >> --- a/drivers/video/fbmon.c
> >> +++ b/drivers/video/fbmon.c
> >> @@ -31,6 +31,7 @@
> >> #include <linux/pci.h>
> >> #include <linux/slab.h>
> >> #include <video/edid.h>
> >> +#include <linux/videomode.h>
> >> #ifdef CONFIG_PPC_OF
> >> #include <asm/prom.h>
> >> #include <asm/pci-bridge.h>
> >> @@ -1373,6 +1374,51 @@ int fb_get_mode(int flags, u32 val, struct fb_var_screeninfo *var, struct fb_inf
> >> kfree(timings);
> >> return err;
> >> }
> >> +
> >> +#if IS_ENABLED(CONFIG_VIDEOMODE)
> >> +int fb_videomode_from_videomode(const struct videomode *vm,
> >> + struct fb_videomode *fbmode)
> >> +{
> >> + unsigned int htotal, vtotal;
> >> +
> >> + fbmode->xres = vm->hactive;
> >> + fbmode->left_margin = vm->hback_porch;
> >> + fbmode->right_margin = vm->hfront_porch;
> >> + fbmode->hsync_len = vm->hsync_len;
> >> +
> >> + fbmode->yres = vm->vactive;
> >> + fbmode->upper_margin = vm->vback_porch;
> >> + fbmode->lower_margin = vm->vfront_porch;
> >> + fbmode->vsync_len = vm->vsync_len;
> >> +
> >> + fbmode->pixclock = KHZ2PICOS(vm->pixelclock / 1000);
> >> +
> >> + fbmode->sync = 0;
> >> + fbmode->vmode = 0;
> >> + 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;
> >> + if (vm->de)
> >> + fbmode->sync |= FB_SYNC_DATA_ENABLE_HIGH_ACT;
> >
> > "FB_SYNC_DATA_ENABLE_HIGH_ACT" seems to be mxsfb specific flag, I am getting
> > build error on this. Please let me know if I am missing something.
> >
> > Thanks,
> > Prakash
> >
I compile-tested the series and didn't have that error. But obviously I should
have. As this is a mxsfs flag, I will throw it out.
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: [PATCH 298/493] video: remove use of __devinitdata
From: Russell King - ARM Linux @ 2012-11-21 12:00 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1353349642-3677-298-git-send-email-wfp5p@virginia.edu>
On Mon, Nov 19, 2012 at 01:24:07PM -0500, Bill Pemberton wrote:
> drivers/video/acornfb.c | 8 +++---
> drivers/video/cyber2000fb.c | 2 +-
> drivers/video/sa1100fb.c | 2 +-
For these three,
Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
^ permalink raw reply
* Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences
From: Tomi Valkeinen @ 2012-11-21 12:04 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20121121114018.GA31576@avionic-0098.adnet.avionic-design.de>
[-- Attachment #1: Type: text/plain, Size: 5646 bytes --]
On 2012-11-21 13:40, Thierry Reding wrote:
> On Wed, Nov 21, 2012 at 01:06:03PM +0200, Tomi Valkeinen wrote:
(sorry for bouncing back and forth with my private and my @ti addresses.
I can't find an option in thunderbird to only use one sender address,
and I always forget to change it when responding...)
>> My suggestion would be to go forward with an in-driver solution, and
>> look at the DT based solution later if we are seeing an increasing bloat
>> in the drivers.
>
> Assuming we go with your approach, what's the plan? We're actually
> facing this problem right now for Tegra. Basically we have a DRM driver
> that can drive the panel, but we're still missing a way to hook up the
> backlight and panel enabling code. So we effectively can't support any
> of the LVDS devices out there without this series.
Could you describe the hardware setup you have related to the LCD and
backlight? Is it a public board with public schematics?
I've understood that you don't have anything special in your board, just
an LCD and a backlight, and the power sequences are related to powering
up the LCD and the backlight, without anything board specific. If so,
there's no need for board specific code, but just improving the panel
and backlight drivers to support the models you use.
> As I understand it, what you propose is similar to what ASoC does. For a
> specific board, you'd have to write a driver, presumably for the new
> panel/display framework, that provides code to power the panel on and
> off. That means we'll have to have a driver for each panel out there
> basically, or we'd need to write generic drivers that can be configured
> to some degree (via platform data or DT). This is similar to how ASoC
> works, where we have a driver that provides support for a specific codec
> connected to the Tegra SoC. For the display framework things could be
> done in a similar way I suppose, so that Tegra could have one display
> driver to handle all aspects of powering on and off the various panels
> for the various boards out there.
I think we should only need the board drivers for very special cases. If
there's just a panel and a backlight, without any special dynamic muxing
or other trickery needed, I don't see a need for a board driver. I
presume this is the case for most of the boards.
> Obviously, a lot of the code will be similar for other SoCs, but maybe
> that's just the way things are if we choose that approach. There's also
> the potential for factoring out large chunks of common code later on
> once we start to see common patterns.
>
> One thing that's not very clear is how the backlight subsystem should be
> wired up with the display framework. I have a patch on top of the Tegra
> DRM driver which adds some ad-hoc display support using this power
> sequences series and the pwm-backlight.
I think that's a separate issue: how to associate the lcd device and
backlight device together. I don't have a clear answer to this.
There are many ways the backlight may be handled. In some cases the
panel and the backlight are truly independent, and you can use the other
without using the other (not very practical, though =).
But then with some LCDs the backlight may be controlled by sending
commands to the panel, and in this case the two may be quite linked.
Changing the backlight requires the panel driver to be up and running,
and sometimes the sending the backlight commands may need to be (say,
DSI display, with backlight commands going over the DSI bus).
So my feeling is that the panel driver should know about the related
backlight device. In the first case the panel driver would just call
enable/disable in the backlight device when the panel is turned on.
In the second case of the DSI panel... I'm not sure. I've implemented it
so that the panel driver creates the backlight device, and implements
the backlight callbacks. It then sends the DSI commands from those
callbacks.
> From reading the proposal for the panel/display framework, it sounds
> like a lot more is planned than just enabling or disabling panels, but
> it also seems like a lot of code needs to be written to support things
> like DSI, DBI or other control busses.
>
> At least for Tegra, and I think the same holds for a wide variety of
> other SoCs, dumb panels would be enough for a start. In the interest of
> getting a working solution for those setups, maybe we can start small
> and add just enough framework to register dumb panel drivers to along
> with code to wire up a backlight to light up the display. Then we could
> possibly still make it to have a proper solution to support the various
> LVDS panels for Tegra with 3.9.
Yes, we (Laurent and me) both agree that we should start simple.
However, the common panel framework is not strictly needed for this. I'm
not sure of the current architecture for Tegra, but for OMAP we already
have panel drivers (omap specific ones, though). The panel drivers may
support multiple models, (for example,
drivers/video/omap2/displays/panel-generic-dpi.c).
I don't see any problem with adding small Tegra specific panel drivers
for the time being, with the intention of converting to common panel
framework when that's available.
Of course, the DT side is an issue. If you now create DT bindings for a
temporary model, and need to change it again later, you'll have some
headaches trying managing that without breaking the old bindings... This
is why I haven't pushed DT bindings for OMAP, as I know I have to change
them in the near future.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]
^ permalink raw reply
* Re: [PATCH v12 2/6] video: add of helper for videomode
From: Tomi Valkeinen @ 2012-11-21 12:22 UTC (permalink / raw)
To: Steffen Trumtrar
Cc: linux-fbdev-u79uwXL29TY76Z2rM5mHXA, kernel-bIcnvbaLZ9MEGnE8C9+IrQ,
David Airlie, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
Florian Tobias Schandinat,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Laurent Pinchart,
Philipp Zabel, Guennady Liakhovetski,
linux-media-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1353426896-6045-3-git-send-email-s.trumtrar-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 3641 bytes --]
On 2012-11-20 17:54, Steffen Trumtrar wrote:
> +timings subnode
> +---------------
> +
> +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-frequency: display clock in Hz
> +
> +optional properties:
> + - hsync-active: Hsync pulse is active low/high/ignored
> + - vsync-active: Vsync pulse is active low/high/ignored
> + - de-active: Data-Enable pulse is active low/high/ignored
> + - pixelclk-inverted: pixelclock is inverted/non-inverted/ignored
Inverted related to what? And isn't this a bool? Pixel clock is either
normal (whatever that is), or inverted. It can't be "not used".
I guess normal case is "pixel data is driven on the rising edge of pixel
clock"? If that's common knowledge, I guess it doesn't need to be
mentioned. But I always have to verify from the documentation what
"normal" means on this particular panel/soc =).
> + - interlaced (bool)
> + - doublescan (bool)
> +
> +All the optional properties that are not bool follow the following logic:
> + <1>: high active
> + <0>: low active
> + omitted: not used on hardware
Perhaps it's obvious, but no harm being explicit: mention that the bool
properties are off is omitted.
And I didn't read the rest of the patches yet, so perhaps this is
already correct, but as I think this framework is usable without DT
also, the meanings of the fields in the structs should be explained in
the header files also in case they are not obvious.
> +Example:
> +
> + display-timings {
> + native-mode = <&timing0>;
> + timing0: 1920p24 {
This should still be 1080p24, not 1920p24 =).
> + /* 1920x1080p24 */
> + clock-frequency = <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 = <1>;
> + };
> + };
> +
> diff --git a/include/linux/of_display_timings.h b/include/linux/of_display_timings.h
> new file mode 100644
> index 0000000..2b4fa0a
> --- /dev/null
> +++ b/include/linux/of_display_timings.h
> @@ -0,0 +1,20 @@
> +/*
> + * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>
> + *
> + * display timings of helpers
> + *
> + * This file is released under the GPLv2
> + */
> +
> +#ifndef __LINUX_OF_DISPLAY_TIMINGS_H
> +#define __LINUX_OF_DISPLAY_TIMINGS_H
> +
> +#include <linux/display_timing.h>
> +#include <linux/of.h>
No need to include these, just add "struct ...;".
> +#define OF_USE_NATIVE_MODE -1
> +
> +struct display_timings *of_get_display_timings(const struct device_node *np);
> +int of_display_timings_exists(const struct device_node *np);
> +
> +#endif
> diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h
> new file mode 100644
> index 0000000..4de5fcc
> --- /dev/null
> +++ b/include/linux/of_videomode.h
> @@ -0,0 +1,18 @@
> +/*
> + * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>
> + *
> + * videomode of-helpers
> + *
> + * This file is released under the GPLv2
> + */
> +
> +#ifndef __LINUX_OF_VIDEOMODE_H
> +#define __LINUX_OF_VIDEOMODE_H
> +
> +#include <linux/videomode.h>
> +#include <linux/of.h>
Same here.
> +int of_get_videomode(const struct device_node *np, struct videomode *vm,
> + int index);
> +
> +#endif /* __LINUX_OF_VIDEOMODE_H */
>
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]
^ permalink raw reply
* Re: [PATCH v12 3/6] fbmon: add videomode helpers
From: Tomi Valkeinen @ 2012-11-21 12:27 UTC (permalink / raw)
To: Steffen Trumtrar
Cc: devicetree-discuss, Rob Herring, linux-fbdev, dri-devel,
Laurent Pinchart, Thierry Reding, Guennady Liakhovetski,
linux-media, Stephen Warren, kernel, Florian Tobias Schandinat,
David Airlie
In-Reply-To: <1353426896-6045-4-git-send-email-s.trumtrar@pengutronix.de>
[-- Attachment #1: Type: text/plain, Size: 969 bytes --]
On 2012-11-20 17:54, Steffen Trumtrar wrote:
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index c7a9571..920cbe3 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -14,6 +14,7 @@
> #include <linux/backlight.h>
> #include <linux/slab.h>
> #include <asm/io.h>
> +#include <linux/videomode.h>
No need for this, just add "struct xxx;".
> struct vm_area_struct;
> struct fb_info;
> @@ -714,6 +715,11 @@ extern void fb_destroy_modedb(struct fb_videomode *modedb);
> extern int fb_find_mode_cvt(struct fb_videomode *mode, int margins, int rb);
> extern unsigned char *fb_ddc_read(struct i2c_adapter *adapter);
>
> +#if IS_ENABLED(CONFIG_VIDEOMODE)
> +extern int fb_videomode_from_videomode(const struct videomode *vm,
> + struct fb_videomode *fbmode);
> +#endif
> +
> /* drivers/video/modedb.c */
> #define VESA_MODEDB_SIZE 34
> extern void fb_var_to_videomode(struct fb_videomode *mode,
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]
^ permalink raw reply
* Re: [PATCH v12 4/6] fbmon: add of_videomode helpers
From: Tomi Valkeinen @ 2012-11-21 12:49 UTC (permalink / raw)
To: Steffen Trumtrar
Cc: linux-fbdev-u79uwXL29TY76Z2rM5mHXA, David Airlie,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
Florian Tobias Schandinat,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Laurent Pinchart,
kernel-bIcnvbaLZ9MEGnE8C9+IrQ, Guennady Liakhovetski,
linux-media-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1353426896-6045-5-git-send-email-s.trumtrar-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1994 bytes --]
On 2012-11-20 17:54, Steffen Trumtrar wrote:
> Add helper to get fb_videomode from devicetree.
>
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> Reviewed-by: Thierry Reding <thierry.reding@avionic-design.de>
> Acked-by: Thierry Reding <thierry.reding@avionic-design.de>
> Tested-by: Thierry Reding <thierry.reding@avionic-design.de>
> Tested-by: Philipp Zabel <p.zabel@pengutronix.de>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/video/fbmon.c | 42 +++++++++++++++++++++++++++++++++++++++++-
> include/linux/fb.h | 7 +++++++
> 2 files changed, 48 insertions(+), 1 deletion(-)
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index 920cbe3..41b5e49 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -15,6 +15,8 @@
> #include <linux/slab.h>
> #include <asm/io.h>
> #include <linux/videomode.h>
> +#include <linux/of.h>
> +#include <linux/of_videomode.h>
Guess what? =)
To be honest, I don't know what the general opinion is about including
header files from header files. But I always leave them out if they are
not strictly needed.
> struct vm_area_struct;
> struct fb_info;
> @@ -715,6 +717,11 @@ extern void fb_destroy_modedb(struct fb_videomode *modedb);
> extern int fb_find_mode_cvt(struct fb_videomode *mode, int margins, int rb);
> extern unsigned char *fb_ddc_read(struct i2c_adapter *adapter);
>
> +#if IS_ENABLED(CONFIG_OF_VIDEOMODE)
> +extern int of_get_fb_videomode(const struct device_node *np,
> + struct fb_videomode *fb,
> + unsigned int index);
> +#endif
> #if IS_ENABLED(CONFIG_VIDEOMODE)
> extern int fb_videomode_from_videomode(const struct videomode *vm,
> struct fb_videomode *fbmode);
Do you really need these #ifs in the header files? They do make it look
a bit messy. If somebody uses the functions and CONFIG_VIDEOMODE is not
enabled, he'll get a linker error anyway.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]
^ permalink raw reply
* Re: [PATCH v12 5/6] drm_modes: add videomode helpers
From: Tomi Valkeinen @ 2012-11-21 12:50 UTC (permalink / raw)
To: Steffen Trumtrar
Cc: devicetree-discuss, Rob Herring, linux-fbdev, dri-devel,
Laurent Pinchart, Thierry Reding, Guennady Liakhovetski,
linux-media, Stephen Warren, kernel, Florian Tobias Schandinat,
David Airlie
In-Reply-To: <1353426896-6045-6-git-send-email-s.trumtrar@pengutronix.de>
[-- Attachment #1: Type: text/plain, Size: 1642 bytes --]
On 2012-11-20 17:54, Steffen Trumtrar wrote:
> Add conversion from videomode to drm_display_mode
>
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> Reviewed-by: Thierry Reding <thierry.reding@avionic-design.de>
> Acked-by: Thierry Reding <thierry.reding@avionic-design.de>
> Tested-by: Thierry Reding <thierry.reding@avionic-design.de>
> Tested-by: Philipp Zabel <p.zabel@pengutronix.de>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/gpu/drm/drm_modes.c | 37 +++++++++++++++++++++++++++++++++++++
> include/drm/drmP.h | 6 ++++++
> 2 files changed, 43 insertions(+)
>
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 3fd8280..de2f6cf 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -56,6 +56,7 @@
> #include <linux/cdev.h>
> #include <linux/mutex.h>
> #include <linux/slab.h>
> +#include <linux/videomode.h>
> #if defined(__alpha__) || defined(__powerpc__)
> #include <asm/pgtable.h> /* For pte_wrprotect */
> #endif
> @@ -1454,6 +1455,11 @@ extern struct drm_display_mode *
> drm_mode_create_from_cmdline_mode(struct drm_device *dev,
> struct drm_cmdline_mode *cmd);
>
> +#if IS_ENABLED(CONFIG_VIDEOMODE)
> +extern int drm_display_mode_from_videomode(const struct videomode *vm,
> + struct drm_display_mode *dmode);
> +#endif
> +
> /* Modesetting support */
> extern void drm_vblank_pre_modeset(struct drm_device *dev, int crtc);
> extern void drm_vblank_post_modeset(struct drm_device *dev, int crtc);
>
And the same comments apply to this header file.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]
^ permalink raw reply
* Re: [PATCH v12 6/6] drm_modes: add of_videomode helpers
From: Tomi Valkeinen @ 2012-11-21 12:52 UTC (permalink / raw)
To: Steffen Trumtrar
Cc: linux-fbdev-u79uwXL29TY76Z2rM5mHXA, David Airlie,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
Florian Tobias Schandinat,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Laurent Pinchart,
kernel-bIcnvbaLZ9MEGnE8C9+IrQ, Guennady Liakhovetski,
linux-media-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1353426896-6045-7-git-send-email-s.trumtrar-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1654 bytes --]
On 2012-11-20 17:54, Steffen Trumtrar wrote:
> Add helper to get drm_display_mode from devicetree.
>
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> Reviewed-by: Thierry Reding <thierry.reding@avionic-design.de>
> Acked-by: Thierry Reding <thierry.reding@avionic-design.de>
> Tested-by: Thierry Reding <thierry.reding@avionic-design.de>
> Tested-by: Philipp Zabel <p.zabel@pengutronix.de>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/gpu/drm/drm_modes.c | 35 ++++++++++++++++++++++++++++++++++-
> include/drm/drmP.h | 6 ++++++
> 2 files changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index de2f6cf..377280f 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -56,6 +56,7 @@
> #include <linux/cdev.h>
> #include <linux/mutex.h>
> #include <linux/slab.h>
> +#include <linux/of.h>
> #include <linux/videomode.h>
> #if defined(__alpha__) || defined(__powerpc__)
> #include <asm/pgtable.h> /* For pte_wrprotect */
> @@ -1459,6 +1460,11 @@ drm_mode_create_from_cmdline_mode(struct drm_device *dev,
> extern int drm_display_mode_from_videomode(const struct videomode *vm,
> struct drm_display_mode *dmode);
> #endif
> +#if IS_ENABLED(CONFIG_OF_VIDEOMODE)
> +extern int of_get_drm_display_mode(const struct device_node *np,
> + struct drm_display_mode *dmode,
> + unsigned int index);
> +#endif
>
> /* Modesetting support */
> extern void drm_vblank_pre_modeset(struct drm_device *dev, int crtc);
And the same comments here also.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]
^ permalink raw reply
* Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences
From: Thierry Reding @ 2012-11-21 13:00 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <50ACC341.3090204@ti.com>
[-- Attachment #1: Type: text/plain, Size: 7937 bytes --]
On Wed, Nov 21, 2012 at 02:04:17PM +0200, Tomi Valkeinen wrote:
> On 2012-11-21 13:40, Thierry Reding wrote:
> > On Wed, Nov 21, 2012 at 01:06:03PM +0200, Tomi Valkeinen wrote:
>
> (sorry for bouncing back and forth with my private and my @ti addresses.
> I can't find an option in thunderbird to only use one sender address,
> and I always forget to change it when responding...)
>
> >> My suggestion would be to go forward with an in-driver solution, and
> >> look at the DT based solution later if we are seeing an increasing bloat
> >> in the drivers.
> >
> > Assuming we go with your approach, what's the plan? We're actually
> > facing this problem right now for Tegra. Basically we have a DRM driver
> > that can drive the panel, but we're still missing a way to hook up the
> > backlight and panel enabling code. So we effectively can't support any
> > of the LVDS devices out there without this series.
>
> Could you describe the hardware setup you have related to the LCD and
> backlight? Is it a public board with public schematics?
I don't think any of the schematics are public. The Tamonten Evaluation
Carrier is available publicly from our website and the schematics are
available on demand as well. If required I can probably arrange to send
you a copy.
> I've understood that you don't have anything special in your board, just
> an LCD and a backlight, and the power sequences are related to powering
> up the LCD and the backlight, without anything board specific. If so,
> there's no need for board specific code, but just improving the panel
> and backlight drivers to support the models you use.
Correct. Basically we have two GPIOs that each enable the panel or the
backlight respectively and one PWM to control the brightness. Are the
panel drivers that you refer to those in drivers/video? I'm not sure if
adding more ad-hoc drivers there just to move them to a generic
framework in the next cycle is a good idea. I'd rather spend some time
on helping to get the framework right and have drivers for that instead.
From what I understand by looking at the OMAP display drivers, they also
provide the timings for the displays. Steffen's videomode helpers can be
used to represent these easily in DT, but I suppose if all of those per-
panel specifics are represented in the drivers then that won't be needed
anymore either.
> > As I understand it, what you propose is similar to what ASoC does. For a
> > specific board, you'd have to write a driver, presumably for the new
> > panel/display framework, that provides code to power the panel on and
> > off. That means we'll have to have a driver for each panel out there
> > basically, or we'd need to write generic drivers that can be configured
> > to some degree (via platform data or DT). This is similar to how ASoC
> > works, where we have a driver that provides support for a specific codec
> > connected to the Tegra SoC. For the display framework things could be
> > done in a similar way I suppose, so that Tegra could have one display
> > driver to handle all aspects of powering on and off the various panels
> > for the various boards out there.
>
> I think we should only need the board drivers for very special cases. If
> there's just a panel and a backlight, without any special dynamic muxing
> or other trickery needed, I don't see a need for a board driver. I
> presume this is the case for most of the boards.
For Tegra ASoC, the way to provide for this is to allow a specific board
to introduce a separate compatible value to enable per-board quirks or
special handling if it cannot be supported by the generic driver and
configuration mechanisms.
> > Obviously, a lot of the code will be similar for other SoCs, but maybe
> > that's just the way things are if we choose that approach. There's also
> > the potential for factoring out large chunks of common code later on
> > once we start to see common patterns.
> >
> > One thing that's not very clear is how the backlight subsystem should be
> > wired up with the display framework. I have a patch on top of the Tegra
> > DRM driver which adds some ad-hoc display support using this power
> > sequences series and the pwm-backlight.
>
> I think that's a separate issue: how to associate the lcd device and
> backlight device together. I don't have a clear answer to this.
>
> There are many ways the backlight may be handled. In some cases the
> panel and the backlight are truly independent, and you can use the other
> without using the other (not very practical, though =).
At least for DT I think we can easily wire that up. I've actually posted
a patch recently that does so. I think in most cases it makes sense to
control them together, such as on DPMS changes, where you really want to
turn both the backlight and the LCD off, independent of how they are
tied together.
> But then with some LCDs the backlight may be controlled by sending
> commands to the panel, and in this case the two may be quite linked.
> Changing the backlight requires the panel driver to be up and running,
> and sometimes the sending the backlight commands may need to be (say,
> DSI display, with backlight commands going over the DSI bus).
>
> So my feeling is that the panel driver should know about the related
> backlight device. In the first case the panel driver would just call
> enable/disable in the backlight device when the panel is turned on.
Exactly.
> In the second case of the DSI panel... I'm not sure. I've implemented it
> so that the panel driver creates the backlight device, and implements
> the backlight callbacks. It then sends the DSI commands from those
> callbacks.
That certainly sounds like the right approach to me.
> > From reading the proposal for the panel/display framework, it sounds
> > like a lot more is planned than just enabling or disabling panels, but
> > it also seems like a lot of code needs to be written to support things
> > like DSI, DBI or other control busses.
> >
> > At least for Tegra, and I think the same holds for a wide variety of
> > other SoCs, dumb panels would be enough for a start. In the interest of
> > getting a working solution for those setups, maybe we can start small
> > and add just enough framework to register dumb panel drivers to along
> > with code to wire up a backlight to light up the display. Then we could
> > possibly still make it to have a proper solution to support the various
> > LVDS panels for Tegra with 3.9.
>
> Yes, we (Laurent and me) both agree that we should start simple.
>
> However, the common panel framework is not strictly needed for this. I'm
> not sure of the current architecture for Tegra, but for OMAP we already
> have panel drivers (omap specific ones, though). The panel drivers may
> support multiple models, (for example,
> drivers/video/omap2/displays/panel-generic-dpi.c).
>
> I don't see any problem with adding small Tegra specific panel drivers
> for the time being, with the intention of converting to common panel
> framework when that's available.
I can take a look at how such a driver could be implemented, but again,
I'm a bit reluctant to add something ad-hoc now when maybe we can have
it supported in a proper framework not too far away in the future.
> Of course, the DT side is an issue. If you now create DT bindings for a
> temporary model, and need to change it again later, you'll have some
> headaches trying managing that without breaking the old bindings... This
> is why I haven't pushed DT bindings for OMAP, as I know I have to change
> them in the near future.
We're already keeping back on this and none of the patches that define
the bindings have been merged yet. Although bindings have been known to
change every once in a while even for code that is already in mainline.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v12 4/6] fbmon: add of_videomode helpers
From: Laurent Pinchart @ 2012-11-21 13:08 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Steffen Trumtrar, devicetree-discuss, Rob Herring, linux-fbdev,
dri-devel, Thierry Reding, Guennady Liakhovetski, linux-media,
Stephen Warren, kernel, Florian Tobias Schandinat, David Airlie
In-Reply-To: <50ACCDDA.2070606@ti.com>
[-- Attachment #1: Type: text/plain, Size: 2230 bytes --]
Hi Tomi,
On Wednesday 21 November 2012 14:49:30 Tomi Valkeinen wrote:
> On 2012-11-20 17:54, Steffen Trumtrar wrote:
> > Add helper to get fb_videomode from devicetree.
> >
> > Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> > Reviewed-by: Thierry Reding <thierry.reding@avionic-design.de>
> > Acked-by: Thierry Reding <thierry.reding@avionic-design.de>
> > Tested-by: Thierry Reding <thierry.reding@avionic-design.de>
> > Tested-by: Philipp Zabel <p.zabel@pengutronix.de>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >
> > drivers/video/fbmon.c | 42 +++++++++++++++++++++++++++++++++++++++++-
> > include/linux/fb.h | 7 +++++++
> > 2 files changed, 48 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/fb.h b/include/linux/fb.h
> > index 920cbe3..41b5e49 100644
> > --- a/include/linux/fb.h
> > +++ b/include/linux/fb.h
> > @@ -15,6 +15,8 @@
> >
> > #include <linux/slab.h>
> > #include <asm/io.h>
> > #include <linux/videomode.h>
> >
> > +#include <linux/of.h>
> > +#include <linux/of_videomode.h>
>
> Guess what? =)
>
> To be honest, I don't know what the general opinion is about including
> header files from header files. But I always leave them out if they are
> not strictly needed.
I agree, I favor structure declaration as well when possible.
> > struct vm_area_struct;
> > struct fb_info;
> >
> > @@ -715,6 +717,11 @@ extern void fb_destroy_modedb(struct fb_videomode
> > *modedb);>
> > extern int fb_find_mode_cvt(struct fb_videomode *mode, int margins, int
> > rb); extern unsigned char *fb_ddc_read(struct i2c_adapter *adapter);
> >
> > +#if IS_ENABLED(CONFIG_OF_VIDEOMODE)
> > +extern int of_get_fb_videomode(const struct device_node *np,
> > + struct fb_videomode *fb,
> > + unsigned int index);
> > +#endif
> >
> > #if IS_ENABLED(CONFIG_VIDEOMODE)
> > extern int fb_videomode_from_videomode(const struct videomode *vm,
> >
> > struct fb_videomode *fbmode);
>
> Do you really need these #ifs in the header files? They do make it look
> a bit messy. If somebody uses the functions and CONFIG_VIDEOMODE is not
> enabled, he'll get a linker error anyway.
--
Regards,
Laurent Pinchart
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: [PATCH v12 0/6] of: add display helper
From: Laurent Pinchart @ 2012-11-21 13:18 UTC (permalink / raw)
To: Steffen Trumtrar
Cc: Thierry Reding, Robert Schwebel, devicetree-discuss, Rob Herring,
linux-fbdev, dri-devel, Guennady Liakhovetski, linux-media,
Tomi Valkeinen, Stephen Warren, kernel, Florian Tobias Schandinat,
David Airlie
In-Reply-To: <20121121082822.GB14013@pengutronix.de>
On Wednesday 21 November 2012 09:28:22 Steffen Trumtrar wrote:
> On Tue, Nov 20, 2012 at 08:35:31PM +0100, Thierry Reding wrote:
> > On Tue, Nov 20, 2012 at 07:11:29PM +0100, Robert Schwebel wrote:
> > > On Tue, Nov 20, 2012 at 05:13:19PM +0100, Laurent Pinchart wrote:
> > > > On Tuesday 20 November 2012 16:54:50 Steffen Trumtrar wrote:
> > > > > Hi!
> > > > >
> > > > > Changes since v11:
> > > > > - make pointers const where applicable
> > > > > - add reviewed-by Laurent Pinchart
> > > >
> > > > Looks good to me.
> > > >
> > > > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > >
> > > > Through which tree do you plan to push this ?
> > >
> > > We have no idea yet, and none of the people on Cc: have volunteered
> > > so far... what do you think?
> >
> > The customary approach would be to take the patches through separate
> > trees, but I think this particular series is sufficiently interwoven to
> > warrant taking them all through one tree. However the least that we
> > should do is collect Acked-by's from the other tree maintainers.
> >
> > Given that most of the patches modify files in drivers/video, Florian's
> > fbdev tree would be the most obvious candidate, right? If Florian agrees
> > to take the patches, all we would need is David's Acked-by.
> >
> > How does that sound?
> >
> > Thierry
>
> Hi!
>
> That is exactly the way I thought this could happen.
Sounds good to me as well.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences
From: Tomi Valkeinen @ 2012-11-21 13:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20121121130039.GA12191@avionic-0098.adnet.avionic-design.de>
[-- Attachment #1: Type: text/plain, Size: 5456 bytes --]
On 2012-11-21 15:00, Thierry Reding wrote:
> On Wed, Nov 21, 2012 at 02:04:17PM +0200, Tomi Valkeinen wrote:
>> On 2012-11-21 13:40, Thierry Reding wrote:
>>> On Wed, Nov 21, 2012 at 01:06:03PM +0200, Tomi Valkeinen wrote:
>>
>> (sorry for bouncing back and forth with my private and my @ti addresses.
>> I can't find an option in thunderbird to only use one sender address,
>> and I always forget to change it when responding...)
>>
>>>> My suggestion would be to go forward with an in-driver solution, and
>>>> look at the DT based solution later if we are seeing an increasing bloat
>>>> in the drivers.
>>>
>>> Assuming we go with your approach, what's the plan? We're actually
>>> facing this problem right now for Tegra. Basically we have a DRM driver
>>> that can drive the panel, but we're still missing a way to hook up the
>>> backlight and panel enabling code. So we effectively can't support any
>>> of the LVDS devices out there without this series.
>>
>> Could you describe the hardware setup you have related to the LCD and
>> backlight? Is it a public board with public schematics?
>
> I don't think any of the schematics are public. The Tamonten Evaluation
> Carrier is available publicly from our website and the schematics are
> available on demand as well. If required I can probably arrange to send
> you a copy.
No need, I think your answer below is enough.
>> I've understood that you don't have anything special in your board, just
>> an LCD and a backlight, and the power sequences are related to powering
>> up the LCD and the backlight, without anything board specific. If so,
>> there's no need for board specific code, but just improving the panel
>> and backlight drivers to support the models you use.
>
> Correct. Basically we have two GPIOs that each enable the panel or the
> backlight respectively and one PWM to control the brightness. Are the
The panel GPIO goes to the panel hardware device, and enables the panel?
And similarly for the PWM GPIO, it goes to the PWM hardware device? Just
making sure there are no other components involved.
> panel drivers that you refer to those in drivers/video? I'm not sure if
> adding more ad-hoc drivers there just to move them to a generic
> framework in the next cycle is a good idea. I'd rather spend some time
> on helping to get the framework right and have drivers for that instead.
We have panel drivers for omap in drivers/video/omap2/displays/. I'm not
sure if other platforms have their own versions of panel drivers, but
probably adding a simple panel driver system for a platform would not be
too difficult. It could even be quite hardcoded, i.e. embedded directly
into the display subsystem driver, just to get something working until
the common panel framework is available.
Yes, I agree it's not good idea to add more platform specific panel
drivers. But it's unclear when CPF will be merged, so if you need to get
the panel working now, I don't see a simple ad-hoc driver as too
horrible. But, of course, I'm not the one making the decision whether to
merge or not =).
> From what I understand by looking at the OMAP display drivers, they also
> provide the timings for the displays. Steffen's videomode helpers can be
> used to represent these easily in DT, but I suppose if all of those per-
Right. Note that I didn't present omap panel drivers as perfect
examples, just examples =).
> panel specifics are represented in the drivers then that won't be needed
> anymore either.
Yes, for most panels with just one native mode and nothing else, the
panel driver can contain the timings.
However, this subject has been discussed earlier a few times. If the
panel in question doesn't need any special power-on/off sequences, just,
perhaps, one gpio or such, we could still use DT video modes. This would
simplify the cases where you have lots of different very simple panels.
Obviously the same questions apply to DT video modes than to the power
sequences, and while I do think it's better to handle the timings inside
the driver, I'm not too much against video timings in DT. The reason
being that the video modes are quite clear, simple and stable data,
versus the much more complex and open-ended power sequences.
>> I don't see any problem with adding small Tegra specific panel drivers
>> for the time being, with the intention of converting to common panel
>> framework when that's available.
>
> I can take a look at how such a driver could be implemented, but again,
Don't look at the mainline omap panel drivers, at least not too closely
=). They contain hackery that will be cleaned up with CPF.
I think there are two methods to implements simple panel drivers:
The hardcoded one, where the display subsystem driver manages a few
different panel models. This is obviously not very expandable or
"correct", but should probably work just fine for a few models, until
CPF is usable.
Something like CPF will have: have the panel device/driver as a platform
device/driver, which will register itself to the display subsystem. And
with "itself" I mean some kind of struct panel_entity, with a few ops
implemented by the panel driver.
Well, this goes a bit out of subject. If you want to discuss panel
drivers more, please start a new thread. Laurent's upcoming CPF v2
should give you good ideas what the model will be.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]
^ permalink raw reply
* Re: [PATCH 0/5] OMAPFB: use dma_alloc instead of omap's vram
From: Jello huang @ 2012-11-21 14:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20121120221456.GO18567@atomide.com>
HI Tomi,
we need one rank of cma to allocate the memory for driver in kernel
space .And the default CMA is for allocating memory frome usespace.So
if we allocate the memory from the
default CMA zone ,there maybe introduce fragmention to the default CMA
zone.The kernel space memory donot touch the memory from userspace
Jello Huang
On 21/11/2012, Tony Lindgren <tony@atomide.com> wrote:
> * Tomi Valkeinen <tomi.valkeinen@ti.com> [121115 23:17]:
>>
>> I added your acks, and pushed:
>>
>> git://gitorious.org/linux-omap-dss2/linux.git 3.8/vram-conversion
>>
>> It's based on -rc4 as my other branches are based on that.
>
> OK thanks!
>
> Tony
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Thanks
Jello Huang
^ permalink raw reply
* Re: efifb swapping Red/Blue color
From: Peter Jones @ 2012-11-21 14:38 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <20121121001457.2672d788@neptune.home>
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="windows-1254", Size: 1391 bytes --]
On Wed, Nov 21, 2012 at 12:14:57AM +0100, Bruno Prémont wrote:
> On a MacBookAir2,1 with 9400M C79 nVidia GPU fbcon on efifb has
> red and blue colors swapped.
>
> Extract from kernel log:
> efifb: probing for efifb
> efifb: framebuffer at 0x80010000, mapped to 0xffffc90004580000, using 6400k, total 6400k
> efifb: mode is 1280x800x32, linelength92, pages=1
> efifb: scrolling: redraw
> efifb: Truecolor: size=8:8:8:8, shift$:0:8:16
>
> On another system with radeon GPU and externally connected DVI monitor
> colors are set properly.
>
> Probably efifb should be looking at something to determine if display
> is RGB or BGR...
Right - normally we get it from the firmware graphics driver, but on a
few generations of Intel Macs they gracefully omitted an implementation
of that part of the spec, which is why we have a DMI table that just has
best-effort entries.
So that on that hardware, it doesn't really give us anything to look
at. Though if the same machine comes with two different video cards
that are, by default, configured opposite ways, there may be something
we can do. Could show me the "lspci -v" for each of those devices (the
nvidia and the radeon)?
--
Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: efifb swapping Red/Blue color
From: Bruno Prémont @ 2012-11-21 14:54 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <20121121001457.2672d788@neptune.home>
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="windows-1254", Size: 5021 bytes --]
Hi Peter,
On Wed, 21 November 2012 Peter Jones <pjones@redhat.com> wrote:
> On Wed, Nov 21, 2012 at 12:14:57AM +0100, Bruno Prémont wrote:
> > On a MacBookAir2,1 with 9400M C79 nVidia GPU fbcon on efifb has
> > red and blue colors swapped.
> >
> > Extract from kernel log:
> > efifb: probing for efifb
> > efifb: framebuffer at 0x80010000, mapped to 0xffffc90004580000, using 6400k, total 6400k
> > efifb: mode is 1280x800x32, linelength92, pages=1
> > efifb: scrolling: redraw
> > efifb: Truecolor: size=8:8:8:8, shift$:0:8:16
> >
> > On another system with radeon GPU and externally connected DVI monitor
> > colors are set properly.
> >
> > Probably efifb should be looking at something to determine if display
> > is RGB or BGR...
>
> Right - normally we get it from the firmware graphics driver, but on a
> few generations of Intel Macs they gracefully omitted an implementation
> of that part of the spec, which is why we have a DMI table that just has
> best-effort entries.
>
> So that on that hardware, it doesn't really give us anything to look
> at. Though if the same machine comes with two different video cards
> that are, by default, configured opposite ways, there may be something
> we can do. Could show me the "lspci -v" for each of those devices (the
> nvidia and the radeon)?
The system with Radeon is not the Mac (it's some Gigabyte mainboard that
has EFI support).
Here is lspci for the MacBook:
00:00.0 Host bridge [0600]: NVIDIA Corporation MCP79 Host Bridge [10de:0a82] (rev b1)
00:00.1 RAM memory [0500]: NVIDIA Corporation MCP79 Memory Controller [10de:0a88] (rev b1)
00:03.0 ISA bridge [0601]: NVIDIA Corporation MCP79 LPC Bridge [10de:0aaf] (rev b2)
00:03.1 RAM memory [0500]: NVIDIA Corporation MCP79 Memory Controller [10de:0aa4] (rev b1)
00:03.2 SMBus [0c05]: NVIDIA Corporation MCP79 SMBus [10de:0aa2] (rev b1)
00:03.3 RAM memory [0500]: NVIDIA Corporation MCP79 Memory Controller [10de:0a89] (rev b1)
00:03.4 RAM memory [0500]: NVIDIA Corporation Device [10de:0a98] (rev b1)
00:03.5 Co-processor [0b40]: NVIDIA Corporation MCP79 Co-processor [10de:0aa3] (rev b1)
00:04.0 USB controller [0c03]: NVIDIA Corporation MCP79 OHCI USB 1.1 Controller [10de:0aa5] (rev b1)
00:04.1 USB controller [0c03]: NVIDIA Corporation MCP79 EHCI USB 2.0 Controller [10de:0aa6] (rev b1)
00:06.0 USB controller [0c03]: NVIDIA Corporation MCP79 OHCI USB 1.1 Controller [10de:0aa7] (rev b1)
00:06.1 USB controller [0c03]: NVIDIA Corporation MCP79 EHCI USB 2.0 Controller [10de:0aa9] (rev b1)
00:08.0 Audio device [0403]: NVIDIA Corporation MCP79 High Definition Audio [10de:0ac0] (rev b1)
00:09.0 PCI bridge [0604]: NVIDIA Corporation MCP79 PCI Bridge [10de:0aab] (rev b1)
00:0b.0 SATA controller [0106]: NVIDIA Corporation MCP79 AHCI Controller [10de:0ab9] (rev b1)
00:10.0 PCI bridge [0604]: NVIDIA Corporation MCP79 PCI Express Bridge [10de:0aa0] (rev b1)
00:15.0 PCI bridge [0604]: NVIDIA Corporation MCP79 PCI Express Bridge [10de:0ac6] (rev b1)
02:00.0 VGA compatible controller [0300]: NVIDIA Corporation C79 [GeForce 9400M] [10de:0870] (rev b1)
03:00.0 Network controller [0280]: Broadcom Corporation BCM4321 802.11a/b/g/n [14e4:4328] (rev 05)
and DMI info:
# dmidecode 2.10
# SMBIOS entry point at 0x7fec7000
SMBIOS 2.4 present.
37 structures occupying 1888 bytes.
Table at 0x7FEC6000.
...
Handle 0x000E, DMI type 0, 24 bytes
BIOS Information
Vendor: Apple Inc.
Version: MBA21.88Z.0075.B03.0811141325
Release Date: 11/14/08
ROM Size: 4096 kB
Characteristics:
PCI is supported
BIOS is upgradeable
BIOS shadowing is allowed
Boot from CD is supported
Selectable boot is supported
ACPI is supported
IEEE 1394 boot is supported
Smart battery is supported
Function key-initiated network boot is supported
BIOS Revision: 0.1
Handle 0x000F, DMI type 1, 27 bytes
System Information
Manufacturer: Apple Inc.
Product Name: MacBookAir2,1
Version: 1.0
Serial Number: xxxxxxxxxxx
UUID: xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx
Wake-up Type: Power Switch
SKU Number: System SKU#
Family: MacBook
Handle 0x0010, DMI type 2, 16 bytes
Base Board Information
Manufacturer: Apple Inc.
Product Name: Mac-F42D88C8
Version: Not Specified
Serial Number: Base Board Serial#
Asset Tag: Base Board Asset Tag#
Features:
Board is a hosting board
Board is replaceable
Location In Chassis: Part Component
Chassis Handle: 0x0011
Type: Motherboard
Contained Object Handles: 0
Handle 0x0011, DMI type 3, 21 bytes
Chassis Information
Manufacturer: Apple Inc.
Type: Notebook
Lock: Not Present
Version: Mac-F42D88C8
Serial Number: xxxxxxxxxxx
Asset Tag: Asset Tag#
Boot-up State: Safe
Power Supply State: Safe
Thermal State: Other
Security Status: Other
OEM Information: 0x00000000
Height: Unspecified
Number Of Power Cords: Unspecified
Contained Elements: 0
...
Bruno
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences
From: Alexandre Courbot @ 2012-11-21 15:02 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <50ACD7DC.5060405@ti.com>
Mmmm so maybe I am misinterpreting things, but it looks like we
have just buried the power sequences here, haven't we?
Alex.
On Wed, Nov 21, 2012 at 10:32 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On 2012-11-21 15:00, Thierry Reding wrote:
>> On Wed, Nov 21, 2012 at 02:04:17PM +0200, Tomi Valkeinen wrote:
>>> On 2012-11-21 13:40, Thierry Reding wrote:
>>>> On Wed, Nov 21, 2012 at 01:06:03PM +0200, Tomi Valkeinen wrote:
>>>
>>> (sorry for bouncing back and forth with my private and my @ti addresses.
>>> I can't find an option in thunderbird to only use one sender address,
>>> and I always forget to change it when responding...)
>>>
>>>>> My suggestion would be to go forward with an in-driver solution, and
>>>>> look at the DT based solution later if we are seeing an increasing bloat
>>>>> in the drivers.
>>>>
>>>> Assuming we go with your approach, what's the plan? We're actually
>>>> facing this problem right now for Tegra. Basically we have a DRM driver
>>>> that can drive the panel, but we're still missing a way to hook up the
>>>> backlight and panel enabling code. So we effectively can't support any
>>>> of the LVDS devices out there without this series.
>>>
>>> Could you describe the hardware setup you have related to the LCD and
>>> backlight? Is it a public board with public schematics?
>>
>> I don't think any of the schematics are public. The Tamonten Evaluation
>> Carrier is available publicly from our website and the schematics are
>> available on demand as well. If required I can probably arrange to send
>> you a copy.
>
> No need, I think your answer below is enough.
>
>>> I've understood that you don't have anything special in your board, just
>>> an LCD and a backlight, and the power sequences are related to powering
>>> up the LCD and the backlight, without anything board specific. If so,
>>> there's no need for board specific code, but just improving the panel
>>> and backlight drivers to support the models you use.
>>
>> Correct. Basically we have two GPIOs that each enable the panel or the
>> backlight respectively and one PWM to control the brightness. Are the
>
> The panel GPIO goes to the panel hardware device, and enables the panel?
> And similarly for the PWM GPIO, it goes to the PWM hardware device? Just
> making sure there are no other components involved.
>
>> panel drivers that you refer to those in drivers/video? I'm not sure if
>> adding more ad-hoc drivers there just to move them to a generic
>> framework in the next cycle is a good idea. I'd rather spend some time
>> on helping to get the framework right and have drivers for that instead.
>
> We have panel drivers for omap in drivers/video/omap2/displays/. I'm not
> sure if other platforms have their own versions of panel drivers, but
> probably adding a simple panel driver system for a platform would not be
> too difficult. It could even be quite hardcoded, i.e. embedded directly
> into the display subsystem driver, just to get something working until
> the common panel framework is available.
>
> Yes, I agree it's not good idea to add more platform specific panel
> drivers. But it's unclear when CPF will be merged, so if you need to get
> the panel working now, I don't see a simple ad-hoc driver as too
> horrible. But, of course, I'm not the one making the decision whether to
> merge or not =).
>
>> From what I understand by looking at the OMAP display drivers, they also
>> provide the timings for the displays. Steffen's videomode helpers can be
>> used to represent these easily in DT, but I suppose if all of those per-
>
> Right. Note that I didn't present omap panel drivers as perfect
> examples, just examples =).
>
>> panel specifics are represented in the drivers then that won't be needed
>> anymore either.
>
> Yes, for most panels with just one native mode and nothing else, the
> panel driver can contain the timings.
>
> However, this subject has been discussed earlier a few times. If the
> panel in question doesn't need any special power-on/off sequences, just,
> perhaps, one gpio or such, we could still use DT video modes. This would
> simplify the cases where you have lots of different very simple panels.
>
> Obviously the same questions apply to DT video modes than to the power
> sequences, and while I do think it's better to handle the timings inside
> the driver, I'm not too much against video timings in DT. The reason
> being that the video modes are quite clear, simple and stable data,
> versus the much more complex and open-ended power sequences.
>
>>> I don't see any problem with adding small Tegra specific panel drivers
>>> for the time being, with the intention of converting to common panel
>>> framework when that's available.
>>
>> I can take a look at how such a driver could be implemented, but again,
>
> Don't look at the mainline omap panel drivers, at least not too closely
> =). They contain hackery that will be cleaned up with CPF.
>
> I think there are two methods to implements simple panel drivers:
>
> The hardcoded one, where the display subsystem driver manages a few
> different panel models. This is obviously not very expandable or
> "correct", but should probably work just fine for a few models, until
> CPF is usable.
>
> Something like CPF will have: have the panel device/driver as a platform
> device/driver, which will register itself to the display subsystem. And
> with "itself" I mean some kind of struct panel_entity, with a few ops
> implemented by the panel driver.
>
> Well, this goes a bit out of subject. If you want to discuss panel
> drivers more, please start a new thread. Laurent's upcoming CPF v2
> should give you good ideas what the model will be.
>
> Tomi
>
>
^ permalink raw reply
* Re: [PATCH v12 2/6] video: add of helper for videomode
From: Rob Herring @ 2012-11-21 15:03 UTC (permalink / raw)
To: Thierry Reding
Cc: Steffen Trumtrar, Manjunathappa, Prakash,
devicetree-discuss@lists.ozlabs.org, Philipp Zabel,
linux-fbdev@vger.kernel.org, dri-devel@lists.freedesktop.org,
Laurent Pinchart, Guennady Liakhovetski,
linux-media@vger.kernel.org, Valkeinen, Tomi, Stephen Warren,
kernel@pengutronix.de, Florian Tobias Schandinat, David Airlie,
Grant Likely
In-Reply-To: <20121121115236.GA8886@avionic-0098.adnet.avionic-design.de>
On 11/21/2012 05:52 AM, Thierry Reding wrote:
> On Wed, Nov 21, 2012 at 12:48:43PM +0100, Steffen Trumtrar wrote:
>> Hi!
>>
>> On Wed, Nov 21, 2012 at 10:12:43AM +0000, Manjunathappa, Prakash wrote:
>>> Hi Steffen,
>>>
>>> On Tue, Nov 20, 2012 at 21:24:52, Steffen Trumtrar wrote:
>>>> +/**
>>>> + * of_get_display_timings - parse all display_timing entries from a device_node
>>>> + * @np: device_node with the subnodes
>>>> + **/
>>>> +struct display_timings *of_get_display_timings(const struct device_node *np)
>>>> +{
>>>> + struct device_node *timings_np;
>>>> + struct device_node *entry;
>>>> + struct device_node *native_mode;
>>>> + struct display_timings *disp;
>>>> +
>>>> + if (!np) {
>>>> + pr_err("%s: no devicenode given\n", __func__);
>>>> + return NULL;
>>>> + }
>>>> +
>>>> + timings_np = of_find_node_by_name(np, "display-timings");
>>>
>>> I get below build warnings on this line
>>> drivers/video/of_display_timing.c: In function 'of_get_display_timings':
>>> drivers/video/of_display_timing.c:109:2: warning: passing argument 1 of 'of_find_node_by_name' discards qualifiers from pointer target type
>>> include/linux/of.h:167:28: note: expected 'struct device_node *' but argument is of type 'const struct device_node *'
>>>
>>>> + * of_display_timings_exists - check if a display-timings node is provided
>>>> + * @np: device_node with the timing
>>>> + **/
>>>> +int of_display_timings_exists(const struct device_node *np)
>>>> +{
>>>> + struct device_node *timings_np;
>>>> +
>>>> + if (!np)
>>>> + return -EINVAL;
>>>> +
>>>> + timings_np = of_parse_phandle(np, "display-timings", 0);
>>>
>>> Also here:
>>> drivers/video/of_display_timing.c: In function 'of_display_timings_exists':
>>> drivers/video/of_display_timing.c:209:2: warning: passing argument 1 of 'of_parse_phandle' discards qualifiers from pointer target type
>>> include/linux/of.h:258:28: note: expected 'struct device_node *' but argument is of type 'const struct device_node *'
>>>
>>
>> The warnings are because the of-functions do not use const pointers where they
>> should. I had two options: don't use const pointers even if they should be and
>> have no warnings or use const pointers and have a correct API. (Third option:
>> send patches for of-functions). I chose the second option.
>
> Maybe a better approach would be a combination of 1 and 3: don't use
> const pointers for struct device_node for now and bring the issue up
> with the OF maintainers, possibly with patches attached that fix the
> problematic functions.
Why does this need to be const? Since some DT functions increment
refcount the node, I'm not sure that making struct device_node const in
general is right thing to do. I do think it should be okay for
of_parse_phandle.
Rob
^ permalink raw reply
* Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences
From: Thierry Reding @ 2012-11-21 15:12 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAAVeFuJofYwj926=3v0ApBCq_vNhCdC1PnzDc-B3d=LL_L40Uw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 660 bytes --]
On Thu, Nov 22, 2012 at 12:02:47AM +0900, Alexandre Courbot wrote:
> Mmmm so maybe I am misinterpreting things, but it looks like we
> have just buried the power sequences here, haven't we?
I don't think so. In fact I was just starting to think that maybe for
Tegra we could have a generic panel driver which used power sequences
to abstract away the pin differences for powering up the panel. Since
most likely that will be where the differences are there is a lot of
potential to factor things out into sequences.
Perhaps what we may want to postpone for now is the DT representation
since that's what Tomi and Grant seem to be mostly opposed to.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v12 2/6] video: add of helper for videomode
From: Thierry Reding @ 2012-11-21 15:13 UTC (permalink / raw)
To: Rob Herring
Cc: linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org, David Airlie,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
Florian Tobias Schandinat,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org,
Manjunathappa, Prakash, Valkeinen, Tomi, Laurent Pinchart,
Philipp Zabel, Steffen Trumtrar, Guennady Liakhovetski,
linux-media-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <50ACED4A.5040806-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 3155 bytes --]
On Wed, Nov 21, 2012 at 09:03:38AM -0600, Rob Herring wrote:
> On 11/21/2012 05:52 AM, Thierry Reding wrote:
> > On Wed, Nov 21, 2012 at 12:48:43PM +0100, Steffen Trumtrar wrote:
> >> Hi!
> >>
> >> On Wed, Nov 21, 2012 at 10:12:43AM +0000, Manjunathappa, Prakash wrote:
> >>> Hi Steffen,
> >>>
> >>> On Tue, Nov 20, 2012 at 21:24:52, Steffen Trumtrar wrote:
> >>>> +/**
> >>>> + * of_get_display_timings - parse all display_timing entries from a device_node
> >>>> + * @np: device_node with the subnodes
> >>>> + **/
> >>>> +struct display_timings *of_get_display_timings(const struct device_node *np)
> >>>> +{
> >>>> + struct device_node *timings_np;
> >>>> + struct device_node *entry;
> >>>> + struct device_node *native_mode;
> >>>> + struct display_timings *disp;
> >>>> +
> >>>> + if (!np) {
> >>>> + pr_err("%s: no devicenode given\n", __func__);
> >>>> + return NULL;
> >>>> + }
> >>>> +
> >>>> + timings_np = of_find_node_by_name(np, "display-timings");
> >>>
> >>> I get below build warnings on this line
> >>> drivers/video/of_display_timing.c: In function 'of_get_display_timings':
> >>> drivers/video/of_display_timing.c:109:2: warning: passing argument 1 of 'of_find_node_by_name' discards qualifiers from pointer target type
> >>> include/linux/of.h:167:28: note: expected 'struct device_node *' but argument is of type 'const struct device_node *'
> >>>
> >>>> + * of_display_timings_exists - check if a display-timings node is provided
> >>>> + * @np: device_node with the timing
> >>>> + **/
> >>>> +int of_display_timings_exists(const struct device_node *np)
> >>>> +{
> >>>> + struct device_node *timings_np;
> >>>> +
> >>>> + if (!np)
> >>>> + return -EINVAL;
> >>>> +
> >>>> + timings_np = of_parse_phandle(np, "display-timings", 0);
> >>>
> >>> Also here:
> >>> drivers/video/of_display_timing.c: In function 'of_display_timings_exists':
> >>> drivers/video/of_display_timing.c:209:2: warning: passing argument 1 of 'of_parse_phandle' discards qualifiers from pointer target type
> >>> include/linux/of.h:258:28: note: expected 'struct device_node *' but argument is of type 'const struct device_node *'
> >>>
> >>
> >> The warnings are because the of-functions do not use const pointers where they
> >> should. I had two options: don't use const pointers even if they should be and
> >> have no warnings or use const pointers and have a correct API. (Third option:
> >> send patches for of-functions). I chose the second option.
> >
> > Maybe a better approach would be a combination of 1 and 3: don't use
> > const pointers for struct device_node for now and bring the issue up
> > with the OF maintainers, possibly with patches attached that fix the
> > problematic functions.
>
> Why does this need to be const? Since some DT functions increment
> refcount the node, I'm not sure that making struct device_node const in
> general is right thing to do. I do think it should be okay for
> of_parse_phandle.
I wasn't proposing to do it everywhere but only where possible. If the
node is modified in some way then obviously it shouldn't be const.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v12 1/6] video: add display_timing and videomode
From: Steffen Trumtrar @ 2012-11-21 16:11 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: devicetree-discuss, Rob Herring, linux-fbdev, dri-devel,
Laurent Pinchart, Thierry Reding, Guennady Liakhovetski,
linux-media, Stephen Warren, kernel, Florian Tobias Schandinat,
David Airlie
In-Reply-To: <50ACBCE4.60701@ti.com>
Hi,
On Wed, Nov 21, 2012 at 01:37:08PM +0200, Tomi Valkeinen wrote:
> Hi,
>
> On 2012-11-20 17:54, Steffen Trumtrar wrote:
> > Add display_timing structure and the according helper functions. This allows
> > the description of a display via its supported timing parameters.
> >
> > Every timing parameter can be specified as a single value or a range
> > <min typ max>.
> >
> > Also, add helper functions to convert from display timings to a generic videomode
> > structure. This videomode can then be converted to the corresponding subsystem
> > mode representation (e.g. fb_videomode).
>
> Sorry for reviewing this so late.
>
> One thing I'd like to see is some explanation of the structs involved.
> For example, in this patch you present structs videomode, display_timing
> and display_timings without giving any hint what they represent.
>
> I'm not asking for you to write a long documentation, but perhaps the
> header files could include a few lines of comments above the structs,
> explaining the idea.
>
Okay. Will do.
> > +void display_timings_release(struct display_timings *disp)
> > +{
> > + if (disp->timings) {
> > + unsigned int i;
> > +
> > + for (i = 0; i < disp->num_timings; i++)
> > + kfree(disp->timings[i]);
> > + kfree(disp->timings);
> > + }
> > + kfree(disp);
> > +}
> > +EXPORT_SYMBOL_GPL(display_timings_release);
>
> Perhaps this will become clearer after reading the following patches,
> but it feels a bit odd to add a release function, without anything in
> this patch that would actually allocate the timings.
>
2/6 uses this function. And as this does not belong to the DT part, it
is added in this patch.
> > diff --git a/drivers/video/videomode.c b/drivers/video/videomode.c
> > new file mode 100644
> > index 0000000..e24f879
> > --- /dev/null
> > +++ b/drivers/video/videomode.c
> > @@ -0,0 +1,46 @@
> > +/*
> > + * generic display timing functions
> > + *
> > + * Copyright (c) 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>, Pengutronix
> > + *
> > + * This file is released under the GPLv2
> > + */
> > +
> > +#include <linux/export.h>
> > +#include <linux/errno.h>
> > +#include <linux/display_timing.h>
> > +#include <linux/kernel.h>
> > +#include <linux/videomode.h>
> > +
> > +int videomode_from_timing(const struct display_timings *disp,
> > + struct videomode *vm, unsigned int index)
> > +{
> > + struct display_timing *dt;
> > +
> > + dt = display_timings_get(disp, index);
> > + if (!dt)
> > + return -EINVAL;
> > +
> > + vm->pixelclock = display_timing_get_value(&dt->pixelclock, 0);
> > + vm->hactive = display_timing_get_value(&dt->hactive, 0);
> > + vm->hfront_porch = display_timing_get_value(&dt->hfront_porch, 0);
> > + vm->hback_porch = display_timing_get_value(&dt->hback_porch, 0);
> > + vm->hsync_len = display_timing_get_value(&dt->hsync_len, 0);
> > +
> > + vm->vactive = display_timing_get_value(&dt->vactive, 0);
> > + vm->vfront_porch = display_timing_get_value(&dt->vfront_porch, 0);
> > + vm->vback_porch = display_timing_get_value(&dt->vback_porch, 0);
> > + vm->vsync_len = display_timing_get_value(&dt->vsync_len, 0);
>
> Shouldn't all these calls get the typical value, with index 1?
>
Yes. I omitted the indexing until now. So it didn't matter what value was used.
But I will integrate it in the next version.
> > +
> > + vm->vah = dt->vsync_pol_active;
> > + vm->hah = dt->hsync_pol_active;
> > + vm->de = dt->de_pol_active;
> > + vm->pixelclk_pol = dt->pixelclk_pol;
> > +
> > + vm->interlaced = dt->interlaced;
> > + vm->doublescan = dt->doublescan;
> > +
> > + return 0;
> > +}
> > +
> > +EXPORT_SYMBOL_GPL(videomode_from_timing);
> > diff --git a/include/linux/display_timing.h b/include/linux/display_timing.h
> > new file mode 100644
> > index 0000000..d5bf03f
> > --- /dev/null
> > +++ b/include/linux/display_timing.h
> > @@ -0,0 +1,70 @@
> > +/*
> > + * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>
> > + *
> > + * description of display timings
> > + *
> > + * This file is released under the GPLv2
> > + */
> > +
> > +#ifndef __LINUX_DISPLAY_TIMINGS_H
> > +#define __LINUX_DISPLAY_TIMINGS_H
> > +
> > +#include <linux/types.h>
>
> What is this needed for? u32? I don't see it defined in types.h
>
Yes, u32. What would be the right header for that if not types.h?
> > +
> > +struct timing_entry {
> > + u32 min;
> > + u32 typ;
> > + u32 max;
> > +};
> > +
> > +struct display_timing {
> > + struct timing_entry pixelclock;
> > +
> > + struct timing_entry hactive;
> > + struct timing_entry hfront_porch;
> > + struct timing_entry hback_porch;
> > + struct timing_entry hsync_len;
> > +
> > + struct timing_entry vactive;
> > + struct timing_entry vfront_porch;
> > + struct timing_entry vback_porch;
> > + struct timing_entry vsync_len;
> > +
> > + unsigned int vsync_pol_active;
> > + unsigned int hsync_pol_active;
> > + unsigned int de_pol_active;
> > + unsigned int pixelclk_pol;
> > + bool interlaced;
> > + bool doublescan;
> > +};
> > +
> > +struct display_timings {
> > + unsigned int num_timings;
> > + unsigned int native_mode;
> > +
> > + struct display_timing **timings;
> > +};
> > +
> > +/*
> > + * placeholder function until ranges are really needed
> > + * the index parameter should then be used to select one of [min typ max]
> > + */
> > +static inline u32 display_timing_get_value(const struct timing_entry *te,
> > + unsigned int index)
> > +{
> > + return te->typ;
> > +}
>
> Why did you opt for a placeholder here? It feels trivial to me to have
> support to get the min/typ/max value properly.
>
I will add that in the next version.
> > +static inline struct display_timing *display_timings_get(const struct
> > + display_timings *disp,
> > + unsigned int index)
> > +{
> > + if (disp->num_timings > index)
> > + return disp->timings[index];
> > + else
> > + return NULL;
> > +}
> > +
> > +void display_timings_release(struct display_timings *disp);
> > +
> > +#endif
> > diff --git a/include/linux/videomode.h b/include/linux/videomode.h
> > new file mode 100644
> > index 0000000..5d3e796
> > --- /dev/null
> > +++ b/include/linux/videomode.h
> > @@ -0,0 +1,40 @@
> > +/*
> > + * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>
> > + *
> > + * generic videomode description
> > + *
> > + * This file is released under the GPLv2
> > + */
> > +
> > +#ifndef __LINUX_VIDEOMODE_H
> > +#define __LINUX_VIDEOMODE_H
> > +
> > +#include <linux/display_timing.h>
>
> This is not needed, just add:
>
> struct display_timings;
>
Okay.
> > +struct videomode {
> > + u32 pixelclock;
> > + u32 refreshrate;
> > +
> > + u32 hactive;
> > + u32 hfront_porch;
> > + u32 hback_porch;
> > + u32 hsync_len;
> > +
> > + u32 vactive;
> > + u32 vfront_porch;
> > + u32 vback_porch;
> > + u32 vsync_len;
> > +
> > + u32 hah;
> > + u32 vah;
> > + u32 de;
> > + u32 pixelclk_pol;
> > +
> > + bool interlaced;
> > + bool doublescan;
> > +};
> > +
> > +int videomode_from_timing(const struct display_timings *disp,
> > + struct videomode *vm, unsigned int index);
> > +
>
> Are this and the few other functions above meant to be used from
> drivers? If so, some explanation of the parameters here would be nice.
> If they are just framework internal, they don't probably need that.
>
Okay. I will add some more documentation.
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: [PATCH v12 4/6] fbmon: add of_videomode helpers
From: Steffen Trumtrar @ 2012-11-21 16:24 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: devicetree-discuss, Rob Herring, linux-fbdev, dri-devel,
Laurent Pinchart, Thierry Reding, Guennady Liakhovetski,
linux-media, Stephen Warren, kernel, Florian Tobias Schandinat,
David Airlie
In-Reply-To: <50ACCDDA.2070606@ti.com>
On Wed, Nov 21, 2012 at 02:49:30PM +0200, Tomi Valkeinen wrote:
> On 2012-11-20 17:54, Steffen Trumtrar wrote:
> > Add helper to get fb_videomode from devicetree.
> >
> > Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> > Reviewed-by: Thierry Reding <thierry.reding@avionic-design.de>
> > Acked-by: Thierry Reding <thierry.reding@avionic-design.de>
> > Tested-by: Thierry Reding <thierry.reding@avionic-design.de>
> > Tested-by: Philipp Zabel <p.zabel@pengutronix.de>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > drivers/video/fbmon.c | 42 +++++++++++++++++++++++++++++++++++++++++-
> > include/linux/fb.h | 7 +++++++
> > 2 files changed, 48 insertions(+), 1 deletion(-)
>
>
> > diff --git a/include/linux/fb.h b/include/linux/fb.h
> > index 920cbe3..41b5e49 100644
> > --- a/include/linux/fb.h
> > +++ b/include/linux/fb.h
> > @@ -15,6 +15,8 @@
> > #include <linux/slab.h>
> > #include <asm/io.h>
> > #include <linux/videomode.h>
> > +#include <linux/of.h>
> > +#include <linux/of_videomode.h>
>
> Guess what? =)
>
> To be honest, I don't know what the general opinion is about including
> header files from header files. But I always leave them out if they are
> not strictly needed.
>
Okay. Seems to fit with the rest of the file;
> > struct vm_area_struct;
> > struct fb_info;
> > @@ -715,6 +717,11 @@ extern void fb_destroy_modedb(struct fb_videomode *modedb);
> > extern int fb_find_mode_cvt(struct fb_videomode *mode, int margins, int rb);
> > extern unsigned char *fb_ddc_read(struct i2c_adapter *adapter);
> >
> > +#if IS_ENABLED(CONFIG_OF_VIDEOMODE)
> > +extern int of_get_fb_videomode(const struct device_node *np,
> > + struct fb_videomode *fb,
> > + unsigned int index);
> > +#endif
> > #if IS_ENABLED(CONFIG_VIDEOMODE)
> > extern int fb_videomode_from_videomode(const struct videomode *vm,
> > struct fb_videomode *fbmode);
>
> Do you really need these #ifs in the header files? They do make it look
> a bit messy. If somebody uses the functions and CONFIG_VIDEOMODE is not
> enabled, he'll get a linker error anyway.
>
Well, I don't remember at the moment who requested this, but it was not my
idea to put them there. So, this is a matter of style I guess.
But maybe I understood that wrong.
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: [PATCH v12 1/6] video: add display_timing and videomode
From: Tomi Valkeinen @ 2012-11-21 16:42 UTC (permalink / raw)
To: Steffen Trumtrar
Cc: linux-fbdev-u79uwXL29TY76Z2rM5mHXA, David Airlie,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
Florian Tobias Schandinat,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Laurent Pinchart,
kernel-bIcnvbaLZ9MEGnE8C9+IrQ, Guennady Liakhovetski,
linux-media-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20121121161103.GA12657-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 460 bytes --]
On 2012-11-21 18:11, Steffen Trumtrar wrote:
> Hi,
>
> On Wed, Nov 21, 2012 at 01:37:08PM +0200, Tomi Valkeinen wrote:
>>> +#include <linux/types.h>
>>
>> What is this needed for? u32? I don't see it defined in types.h
>>
>
> Yes, u32. What would be the right header for that if not types.h?
Yes, it looks like u32 comes via linux/types.h. It's defined elsewhere,
but linux/types.h sounds like the most reasonable header for it.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]
^ permalink raw reply
* Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences
From: Grant Likely @ 2012-11-21 16:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20121121013133.GE4673@opensource.wolfsonmicro.com>
On Wed, 21 Nov 2012 10:31:34 +0900, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:
> On Tue, Nov 20, 2012 at 09:54:29PM +0000, Grant Likely wrote:
> > On Sat, 17 Nov 2012 19:55:45 +0900, Alexandre Courbot <acourbot@nvidia.com> wrote:
>
> > > With the advent of the device tree and of ARM kernels that are not
> > > board-tied, we cannot rely on these board-specific hooks anymore but
>
> > This isn't strictly true. It is still perfectly fine to have board
> > specific code when necessary. However, there is strong encouragement to
> > enable that code in device drivers as much as possible and new board
> > files need to have very strong justification.
>
> This isn't the message that's gone over, and even for device drivers
> everyone seems to be taking the whole device tree thing as a move to
> pull all data out of the kernel. In some cases there are some real
> practical advantages to doing this but a lot of the people making these
> changes seem to view having things in DT as a goal in itself.
I know, and I do apologize for my part in not communicating and
shepherding enough. For anyone reading; please be pragmatic on putting
things into the DT. If a binding starts to look really complex, ask
whether or not it helps things.
> > I'm thinking about the scripts as trivial-to-parse ascii strings that
> > have a very simple set of commands. The commands use resources already
> > defined in the node. ie. 'g0' refers to the first entry in the gpios
> > property. 'r0' for the regulator, 'p0' for the pwms, 'd' means delay. By
> > no means take this as the format to use, it is just an example off the
> > top of my head, but it is already way easier to work with than putting
> > each command into a node.
>
> It does appear to have some legibility challenges, especially with using
> the indexes into the arrays of resources. On the other hand the arrays
> should be fairly small.
This is merely an example off the top of my head. I'm in no way wed to
the syntax.
> > > +Platform Data Format
> > > +--------------------
> > > +All relevant data structures for declaring power sequences are located in
> > > +include/linux/power_seq.h.
>
> > Hmm... If sequences are switched to a string instead, then platform_data
> > should also use it. The power sequence data structures can be created at
> > runtime by parsing the string.
>
> Seems like a step backwards to me - why not let people store the more
> parsed version of the data? It's going to be less convenient for users
> on non-DT systems.
>
> As I said in my earlier reviews I think this is a useful thing to have
> as a utility library for drivers independantly of the DT bindings, it
> would allow drivers to become more data driven. Perhaps we can rework
> the series so that the DT bindings are added as a final patch? All the
> rest of the code seems OK.
I'm fine with that if the bindings don't get merged yet.
g.
^ permalink raw reply
* Re: [PATCH v12 4/6] fbmon: add of_videomode helpers
From: Tomi Valkeinen @ 2012-11-21 16:47 UTC (permalink / raw)
To: Steffen Trumtrar
Cc: devicetree-discuss, Rob Herring, linux-fbdev, dri-devel,
Laurent Pinchart, Thierry Reding, Guennady Liakhovetski,
linux-media, Stephen Warren, kernel, Florian Tobias Schandinat,
David Airlie
In-Reply-To: <20121121162436.GB12657@pengutronix.de>
[-- Attachment #1: Type: text/plain, Size: 1435 bytes --]
On 2012-11-21 18:24, Steffen Trumtrar wrote:
> On Wed, Nov 21, 2012 at 02:49:30PM +0200, Tomi Valkeinen wrote:
>>> @@ -715,6 +717,11 @@ extern void fb_destroy_modedb(struct fb_videomode *modedb);
>>> extern int fb_find_mode_cvt(struct fb_videomode *mode, int margins, int rb);
>>> extern unsigned char *fb_ddc_read(struct i2c_adapter *adapter);
>>>
>>> +#if IS_ENABLED(CONFIG_OF_VIDEOMODE)
>>> +extern int of_get_fb_videomode(const struct device_node *np,
>>> + struct fb_videomode *fb,
>>> + unsigned int index);
>>> +#endif
>>> #if IS_ENABLED(CONFIG_VIDEOMODE)
>>> extern int fb_videomode_from_videomode(const struct videomode *vm,
>>> struct fb_videomode *fbmode);
>>
>> Do you really need these #ifs in the header files? They do make it look
>> a bit messy. If somebody uses the functions and CONFIG_VIDEOMODE is not
>> enabled, he'll get a linker error anyway.
>>
>
> Well, I don't remember at the moment who requested this, but it was not my
> idea to put them there. So, this is a matter of style I guess.
> But maybe I understood that wrong.
Right, one reviewer says this way, and another says that way =).
With the header files I've made I only use #ifs with #else, when I want
to give a static inline empty/no-op implementation for the function in
case the feature is not compiled into the kernel.
As you said, matter of taste. Up to you.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]
^ permalink raw reply
* [PATCH 1/2] OMAP: DSS: do not fail if dpll4_m4_ck is missing
From: Aaro Koskinen @ 2012-11-21 19:48 UTC (permalink / raw)
To: Tomi Valkeinen, linux-omap, linux-fbdev; +Cc: Aaro Koskinen
Do not fail if dpll4_m4_ck is missing. The clock is not there on omap24xx,
so this should not be a hard error.
The patch retains the functionality before the commit 185bae10 (OMAPDSS:
DSS: Cleanup cpu_is_xxxx checks).
Signed-off-by: Aaro Koskinen <aaro.koskinen@iki.fi>
---
drivers/video/omap2/dss/dss.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c
index 2ab1c3e..55f2ea9 100644
--- a/drivers/video/omap2/dss/dss.c
+++ b/drivers/video/omap2/dss/dss.c
@@ -697,11 +697,15 @@ static int dss_get_clocks(void)
dss.dss_clk = clk;
- clk = clk_get(NULL, dss.feat->clk_name);
- if (IS_ERR(clk)) {
- DSSERR("Failed to get %s\n", dss.feat->clk_name);
- r = PTR_ERR(clk);
- goto err;
+ if (dss.feat->clk_name) {
+ clk = clk_get(NULL, dss.feat->clk_name);
+ if (IS_ERR(clk)) {
+ DSSERR("Failed to get %s\n", dss.feat->clk_name);
+ r = PTR_ERR(clk);
+ goto err;
+ }
+ } else {
+ clk = NULL;
}
dss.dpll4_m4_ck = clk;
--
1.7.10.4
^ permalink raw reply related
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