* Re: [PATCH 2/2] offb: Add palette hack for qemu "standard vga" framebuffer
From: Andreas Färber @ 2011-12-15 7:49 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linux-fbdev, linuxppc-dev, kvm-ppc
In-Reply-To: <1323907109.21839.28.camel@pasglop>
Am 15.12.2011 00:58, schrieb Benjamin Herrenschmidt:
> We rename the mach64 hack to "simple" since that's also applicable
> to anything using VGA-style DAC IO ports (set to 8-bit DAC) and we
> use it for qemu vga.
>
> Note that this is keyed on a device-tree "compatible" property that
> is currently only set by an upcoming version of SLOF when using the
> qemu "pseries" platform. This is on purpose as other qemu ppc platforms
> using OpenBIOS aren't properly setting the DAC to 8-bit at the time of
> the writing of this patch.
>
> We can fix OpenBIOS later to do that and add the required property, in
> which case it will be matched by this change.
Just let me know what's needed for OpenBIOS.
Is this just for -vga std as opposed to the default cirrus?
Cheers,
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply
* Re: [PATCH 2/2] offb: Add palette hack for qemu "standard vga"
From: Benjamin Herrenschmidt @ 2011-12-15 8:09 UTC (permalink / raw)
To: Andreas Färber; +Cc: linux-fbdev, linuxppc-dev, kvm-ppc
In-Reply-To: <4EE9A6A5.2050600@suse.de>
On Thu, 2011-12-15 at 08:49 +0100, Andreas Färber wrote:
> Am 15.12.2011 00:58, schrieb Benjamin Herrenschmidt:
> > We rename the mach64 hack to "simple" since that's also applicable
> > to anything using VGA-style DAC IO ports (set to 8-bit DAC) and we
> > use it for qemu vga.
> >
> > Note that this is keyed on a device-tree "compatible" property that
> > is currently only set by an upcoming version of SLOF when using the
> > qemu "pseries" platform. This is on purpose as other qemu ppc platforms
> > using OpenBIOS aren't properly setting the DAC to 8-bit at the time of
> > the writing of this patch.
> >
> > We can fix OpenBIOS later to do that and add the required property, in
> > which case it will be matched by this change.
>
> Just let me know what's needed for OpenBIOS.
> Is this just for -vga std as opposed to the default cirrus?
Yes. Cirrus isn't the default on mac99 and on pseries (tho I will
eventually add a SLOF driver for it as well).
For OpenBIOS I was thinking about just sending you a patch :-) But if
you have more time than I do, what is needed is:
- Set the 8-bit DAC bit in the VBE enable register when initializing
the card (0x20 off the top of my mind but dbl check). Remove your >> 2
in your palette setting.
- Implement color! so prom_init can set the initial palette (but that's
not strictly necessary).
- I assume that the VGA device already has a device_type of "display",
can be open()'ed from the client interface and will have the necessary
properties to be used by offb (width, height, linebytes, depth, and
address if fits in 32-bit (if not, ignore it, offb will pick the largest
BAR)).
- Stick "qemu,std-vga" into the compatible property of the vga PCI
device.
Cheers,
Ben.
^ permalink raw reply
* Re: [PATCH 1/2] OMAPDSS: DISPC: Update Fir Coefficients
From: Tomi Valkeinen @ 2011-12-15 8:55 UTC (permalink / raw)
To: Chandrabhanu Mahapatra; +Cc: linux-omap, linux-fbdev
In-Reply-To: <1323838310-4439-1-git-send-email-cmahapatra@ti.com>
[-- Attachment #1: Type: text/plain, Size: 1746 bytes --]
On Wed, 2011-12-14 at 10:21 +0530, Chandrabhanu Mahapatra wrote:
> +const struct dispc_coef *dispc_ovl_get_scale_coef(int inc, int five_taps)
> +{
> + int i;
> + static const struct {
> + int Mmin;
> + int Mmax;
> + const struct dispc_coef *coef_3;
> + const struct dispc_coef *coef_5;
> + } coefs[] = {
> + { 26, 32, coef3_M32, coef5_M32 },
> + { 22, 26, coef3_M26, coef5_M26 },
> + { 19, 22, coef3_M22, coef5_M22 },
> + { 16, 19, coef3_M19, coef5_M19 },
> + { 14, 16, coef3_M16, coef5_M16 },
> + { 13, 14, coef3_M14, coef5_M14 },
> + { 12, 13, coef3_M13, coef5_M13 },
> + { 11, 12, coef3_M12, coef5_M12 },
> + { 10, 11, coef3_M11, coef5_M11 },
> + { 9, 10, coef3_M10, coef5_M10 },
> + { 8, 9, coef3_M9, coef5_M9 },
> + { 3, 8, coef3_M8, coef5_M8 },
> + /*
> + * When upscaling more than two times, blockiness and outlines
> + * around the image are observed when M8 tables are used. M11,
> + * M16 and M19 tables are used to prevent this.
> + */
> + { 2, 3, coef3_M11, coef5_M11 },
> + { 1, 2, coef3_M16, coef5_M16 },
> + };
> +
> + inc /= 128;
> + for (i = 0; i < ARRAY_LEN(coefs); ++i)
> + if (inc > coefs[i].Mmin && inc <= coefs[i].Mmax)
> + return five_taps ? coefs[i].coef_5 : coefs[i].coef_3;
> + if (inc == 1)
> + return five_taps ? coef3_M19 : coef5_M19;
> + return NULL;
> +}
Why don't you handle the inc == 1 case the same as others? Just have an
entry in the table for Mmin=0, Mmax = 1.
Also, I think it's a bit confusing that Mmin is exclusive and Mmax is
inclusive in the comparison. It makes the table a bit hard to read, when
looking at which entry is used for which inc. I'd recommend using
inclusive comparison for both.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 47/57] fbdev: sh_mobile_meram: Use genalloc to manage MERAM allocation
From: Laurent Pinchart @ 2011-12-15 11:37 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1323784972-24205-48-git-send-email-laurent.pinchart@ideasonboard.com>
Hi Damian,
Thanks for the review.
On Thursday 15 December 2011 03:43:06 Damian Hobson-Garcia wrote:
> On 2011/12/13 23:02, Laurent Pinchart wrote:
> > @@ -195,43 +196,40 @@ static int meram_check_overlap(struct
> > sh_mobile_meram_priv *priv,
> >
> > test_bit(new->cache_icb, &priv->used_icb))
> >
> > return 1;
> >
> > - for (i = 0; i < MERAM_ICB_NUM; i++) {
> > - if (!test_bit(i, &priv->used_icb))
> > - continue;
> > -
> > - used_start = MERAM_CACHE_START(priv->icbs[i].region);
> > - used_end = MERAM_CACHE_END(priv->icbs[i].region);
> > - meram_start = new->meram_offset;
> > - meram_end = new->meram_offset + new->meram_size;
> > -
> > - if ((meram_start >= used_start && meram_start < used_end) ||
> > - (meram_end > used_start && meram_end < used_end))
> > - return 1;
> > - }
> > -
> >
> > return 0;
> >
> > }
> >
> > -/* Mark the specified ICB as used. */
> > -static void meram_mark(struct sh_mobile_meram_priv *priv,
> > +/* Allocate memory for the ICBs and mark them as used. */
> > +static int meram_alloc(struct sh_mobile_meram_priv *priv,
> >
> > const struct sh_mobile_meram_icb_cfg *new,
> > int pixelformat)
> >
> > {
> >
> > + struct sh_mobile_meram_icb *marker = &priv->icbs[new->marker_icb];
> > + unsigned long mem;
> > +
> > + mem = gen_pool_alloc(priv->pool, new->meram_size * 1024);
> > + if (mem = 0)
> > + return -ENOMEM;
> > +
> >
> > __set_bit(new->marker_icb, &priv->used_icb);
> > __set_bit(new->cache_icb, &priv->used_icb);
> >
> > - priv->icbs[new->marker_icb].region = MERAM_CACHE_SET(new->meram_offset,
> > - new->meram_size);
> > - priv->icbs[new->cache_icb].region = MERAM_CACHE_SET(new->meram_offset,
> > - new->meram_size);
> > - priv->icbs[new->marker_icb].current_reg = 1;
> > - priv->icbs[new->marker_icb].pixelformat = pixelformat;
> > + marker->offset = mem - priv->meram;
>
> I might have missed this somewhere, but it doesn't look like priv->meram
> is assigned anywhere.
Oops. That got lost in a rebase operation :-/ I'll fix it.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH 48/57] fbdev: sh_mobile_meram: Allocate ICBs automatically
From: Laurent Pinchart @ 2011-12-15 11:39 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1323784972-24205-49-git-send-email-laurent.pinchart@ideasonboard.com>
Hi Damian,
On Thursday 15 December 2011 03:58:12 Damian Hobson-Garcia wrote:
> Hi Laurent,
>
> On 2011/12/13 23:02, Laurent Pinchart wrote:
> > Instead of manually specifying the ICBs to use in platform data,
> > allocate them automatically at runtime.
> >
> > The MERAM registration function now returns a pointer to an opaque MERAM
> > object, which is passed to the update and unregistration functions.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
>
> So one concern that I have about this is in regards to sharing ICBs with
> user-space drivers. Since there are user space drivers (via UIO) for blocks
> like the VEU, that may want to have access to MERAM, we need a way to
> communicate which ICBs are free to user space. With the hard-coded platform
> data we could easily assume that kernel drivers would use, for example, the
> upper 16 ICBs and so user-space drivers were free to use the lower 16.
You're right, I forgot about that :-/
> One simple temporary workaround might be to provide a range of useable ICBs
> in the platform data. The MERAM memory allocation range can always easily be
> tweaked by using the meram_resources structure.
That sounds good to me. I'll implement that.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH 12/57] arm: mach-shmobile: Add LCDC tx_dev field to
From: Guennadi Liakhovetski @ 2011-12-15 14:01 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1323784972-24205-13-git-send-email-laurent.pinchart@ideasonboard.com>
Hi Laurent
On Tue, 13 Dec 2011, Laurent Pinchart wrote:
> Make sure the transmitter devices get registered before the associated
> LCDC devices.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> arch/arm/mach-shmobile/board-ap4evb.c | 272 ++++++++++++++++---------------
> arch/arm/mach-shmobile/board-mackerel.c | 66 ++++----
> 2 files changed, 176 insertions(+), 162 deletions(-)
>
> diff --git a/arch/arm/mach-shmobile/board-ap4evb.c b/arch/arm/mach-shmobile/board-ap4evb.c
> index dca4860..e0a6b3d 100644
> --- a/arch/arm/mach-shmobile/board-ap4evb.c
> +++ b/arch/arm/mach-shmobile/board-ap4evb.c
> @@ -445,82 +445,6 @@ static struct platform_device usb1_host_device = {
> .resource = usb1_host_resources,
> };
>
> -static const struct fb_videomode ap4evb_lcdc_modes[] = {
> - {
> -#ifdef CONFIG_AP4EVB_QHD
> - .name = "R63302(QHD)",
> - .xres = 544,
> - .yres = 961,
> - .left_margin = 72,
> - .right_margin = 600,
> - .hsync_len = 16,
> - .upper_margin = 8,
> - .lower_margin = 8,
> - .vsync_len = 2,
> - .sync = FB_SYNC_VERT_HIGH_ACT | FB_SYNC_HOR_HIGH_ACT,
> -#else
> - .name = "WVGA Panel",
> - .xres = 800,
> - .yres = 480,
> - .left_margin = 220,
> - .right_margin = 110,
> - .hsync_len = 70,
> - .upper_margin = 20,
> - .lower_margin = 5,
> - .vsync_len = 5,
> - .sync = 0,
> -#endif
> - },
> -};
> -static struct sh_mobile_meram_cfg lcd_meram_cfg = {
> - .icb[0] = {
> - .marker_icb = 28,
> - .cache_icb = 24,
> - .meram_offset = 0x0,
> - .meram_size = 0x40,
> - },
> - .icb[1] = {
> - .marker_icb = 29,
> - .cache_icb = 25,
> - .meram_offset = 0x40,
> - .meram_size = 0x40,
> - },
> -};
> -
> -static struct sh_mobile_lcdc_info lcdc_info = {
> - .meram_dev = &meram_info,
> - .ch[0] = {
> - .chan = LCDC_CHAN_MAINLCD,
> - .fourcc = V4L2_PIX_FMT_RGB565,
> - .lcd_cfg = ap4evb_lcdc_modes,
> - .num_cfg = ARRAY_SIZE(ap4evb_lcdc_modes),
> - .meram_cfg = &lcd_meram_cfg,
> - }
> -};
> -
> -static struct resource lcdc_resources[] = {
> - [0] = {
> - .name = "LCDC",
> - .start = 0xfe940000, /* P4-only space */
> - .end = 0xfe943fff,
> - .flags = IORESOURCE_MEM,
> - },
> - [1] = {
> - .start = intcs_evt2irq(0x580),
> - .flags = IORESOURCE_IRQ,
> - },
> -};
> -
> -static struct platform_device lcdc_device = {
> - .name = "sh_mobile_lcdc_fb",
> - .num_resources = ARRAY_SIZE(lcdc_resources),
> - .resource = lcdc_resources,
> - .dev = {
> - .platform_data = &lcdc_info,
> - .coherent_dma_mask = ~0,
> - },
> -};
> -
> /*
> * QHD display
> */
> @@ -601,6 +525,8 @@ static struct resource mipidsi0_resources[] = {
> },
> };
>
> +static struct sh_mobile_lcdc_info lcdc_info;
> +
> static struct sh_mipi_dsi_info mipidsi0_info = {
> .data_format = MIPI_RGB888,
> .lcd_chan = &lcdc_info.ch[0],
> @@ -627,6 +553,86 @@ static struct platform_device *qhd_devices[] __initdata = {
> };
> #endif /* CONFIG_AP4EVB_QHD */
>
> +/* LCDC0 */
> +static const struct fb_videomode ap4evb_lcdc_modes[] = {
Hm, I must be missing something. I thought you were moving all the structs
around to make them available for reference _without_ forward
declarations. But you _do_ end up using a forward declaration anyway. Here
and for HDMI below too. So, what's the point? Why not just forward declare
that one struct, that's needed and leave the rest where it currently is
in the file?
> + {
> +#ifdef CONFIG_AP4EVB_QHD
> + .name = "R63302(QHD)",
> + .xres = 544,
> + .yres = 961,
> + .left_margin = 72,
> + .right_margin = 600,
> + .hsync_len = 16,
> + .upper_margin = 8,
> + .lower_margin = 8,
> + .vsync_len = 2,
> + .sync = FB_SYNC_VERT_HIGH_ACT | FB_SYNC_HOR_HIGH_ACT,
> +#else
> + .name = "WVGA Panel",
> + .xres = 800,
> + .yres = 480,
> + .left_margin = 220,
> + .right_margin = 110,
> + .hsync_len = 70,
> + .upper_margin = 20,
> + .lower_margin = 5,
> + .vsync_len = 5,
> + .sync = 0,
> +#endif
> + },
> +};
> +static struct sh_mobile_meram_cfg lcd_meram_cfg = {
> + .icb[0] = {
> + .marker_icb = 28,
> + .cache_icb = 24,
> + .meram_offset = 0x0,
> + .meram_size = 0x40,
> + },
> + .icb[1] = {
> + .marker_icb = 29,
> + .cache_icb = 25,
> + .meram_offset = 0x40,
> + .meram_size = 0x40,
> + },
> +};
> +
> +static struct sh_mobile_lcdc_info lcdc_info = {
> + .meram_dev = &meram_info,
> + .ch[0] = {
> + .chan = LCDC_CHAN_MAINLCD,
> + .fourcc = V4L2_PIX_FMT_RGB565,
> + .lcd_cfg = ap4evb_lcdc_modes,
> + .num_cfg = ARRAY_SIZE(ap4evb_lcdc_modes),
> + .meram_cfg = &lcd_meram_cfg,
> +#ifdef CONFIG_AP4EVB_QHD
> + .tx_dev = &mipidsi0_device,
> +#endif
> + }
> +};
> +
> +static struct resource lcdc_resources[] = {
> + [0] = {
> + .name = "LCDC",
> + .start = 0xfe940000, /* P4-only space */
> + .end = 0xfe943fff,
> + .flags = IORESOURCE_MEM,
> + },
> + [1] = {
> + .start = intcs_evt2irq(0x580),
> + .flags = IORESOURCE_IRQ,
> + },
> +};
> +
> +static struct platform_device lcdc_device = {
> + .name = "sh_mobile_lcdc_fb",
> + .num_resources = ARRAY_SIZE(lcdc_resources),
> + .resource = lcdc_resources,
> + .dev = {
> + .platform_data = &lcdc_info,
> + .coherent_dma_mask = ~0,
> + },
> +};
> +
> /* FSI */
> #define IRQ_FSI evt2irq(0x1840)
> static int __fsi_set_rate(struct clk *clk, long rate, int enable)
> @@ -793,6 +799,62 @@ static struct platform_device fsi_device = {
> static struct platform_device fsi_ak4643_device = {
> .name = "sh_fsi2_a_ak4643",
> };
> +
> +/* LCDC1 */
> +static long ap4evb_clk_optimize(unsigned long target, unsigned long *best_freq,
> + unsigned long *parent_freq);
> +
> +static struct sh_mobile_lcdc_info sh_mobile_lcdc1_info;
> +
> +static struct sh_mobile_hdmi_info hdmi_info = {
> + .lcd_chan = &sh_mobile_lcdc1_info.ch[0],
> + .flags = HDMI_SND_SRC_SPDIF,
> + .clk_optimize_parent = ap4evb_clk_optimize,
> +};
> +
> +static struct resource hdmi_resources[] = {
> + [0] = {
> + .name = "HDMI",
> + .start = 0xe6be0000,
> + .end = 0xe6be00ff,
> + .flags = IORESOURCE_MEM,
> + },
> + [1] = {
> + /* There's also an HDMI interrupt on INTCS @ 0x18e0 */
> + .start = evt2irq(0x17e0),
> + .flags = IORESOURCE_IRQ,
> + },
> +};
> +
> +static struct platform_device hdmi_device = {
> + .name = "sh-mobile-hdmi",
> + .num_resources = ARRAY_SIZE(hdmi_resources),
> + .resource = hdmi_resources,
> + .id = -1,
> + .dev = {
> + .platform_data = &hdmi_info,
> + },
> +};
> +
> +static long ap4evb_clk_optimize(unsigned long target, unsigned long *best_freq,
> + unsigned long *parent_freq)
> +{
> + struct clk *hdmi_ick = clk_get(&hdmi_device.dev, "ick");
> + long error;
> +
> + if (IS_ERR(hdmi_ick)) {
> + int ret = PTR_ERR(hdmi_ick);
> + pr_err("Cannot get HDMI ICK: %d\n", ret);
> + return ret;
> + }
> +
> + error = clk_round_parent(hdmi_ick, target, best_freq, parent_freq, 1, 64);
> +
> + clk_put(hdmi_ick);
> +
> + return error;
> +}
> +
> static struct sh_mobile_meram_cfg hdmi_meram_cfg = {
> .icb[0] = {
> .marker_icb = 30,
> @@ -818,6 +880,7 @@ static struct sh_mobile_lcdc_info sh_mobile_lcdc1_info = {
> .clock_divider = 1,
> .flags = LCDC_FLAGS_DWPOL,
> .meram_cfg = &hdmi_meram_cfg,
> + .tx_dev = &hdmi_device,
> }
> };
>
> @@ -845,63 +908,10 @@ static struct platform_device lcdc1_device = {
> },
> };
>
> -static long ap4evb_clk_optimize(unsigned long target, unsigned long *best_freq,
> - unsigned long *parent_freq);
> -
> -
> -static struct sh_mobile_hdmi_info hdmi_info = {
> - .lcd_chan = &sh_mobile_lcdc1_info.ch[0],
> - .flags = HDMI_SND_SRC_SPDIF,
> - .clk_optimize_parent = ap4evb_clk_optimize,
> -};
> -
> -static struct resource hdmi_resources[] = {
> - [0] = {
> - .name = "HDMI",
> - .start = 0xe6be0000,
> - .end = 0xe6be00ff,
> - .flags = IORESOURCE_MEM,
> - },
> - [1] = {
> - /* There's also an HDMI interrupt on INTCS @ 0x18e0 */
> - .start = evt2irq(0x17e0),
> - .flags = IORESOURCE_IRQ,
> - },
> -};
> -
> -static struct platform_device hdmi_device = {
> - .name = "sh-mobile-hdmi",
> - .num_resources = ARRAY_SIZE(hdmi_resources),
> - .resource = hdmi_resources,
> - .id = -1,
> - .dev = {
> - .platform_data = &hdmi_info,
> - },
> -};
> -
> static struct platform_device fsi_hdmi_device = {
> .name = "sh_fsi2_b_hdmi",
> };
>
> -static long ap4evb_clk_optimize(unsigned long target, unsigned long *best_freq,
> - unsigned long *parent_freq)
> -{
> - struct clk *hdmi_ick = clk_get(&hdmi_device.dev, "ick");
> - long error;
> -
> - if (IS_ERR(hdmi_ick)) {
> - int ret = PTR_ERR(hdmi_ick);
> - pr_err("Cannot get HDMI ICK: %d\n", ret);
> - return ret;
> - }
> -
> - error = clk_round_parent(hdmi_ick, target, best_freq, parent_freq, 1, 64);
> -
> - clk_put(hdmi_ick);
> -
> - return error;
> -}
> -
> static struct gpio_led ap4evb_leds[] = {
> {
> .name = "led4",
> @@ -1036,9 +1046,9 @@ static struct platform_device *ap4evb_devices[] __initdata = {
> &fsi_ak4643_device,
> &fsi_hdmi_device,
> &sh_mmcif_device,
> - &lcdc1_device,
> - &lcdc_device,
> &hdmi_device,
> + &lcdc_device,
> + &lcdc1_device,
> &ceu_device,
> &ap4evb_camera,
> &meram_device,
> diff --git a/arch/arm/mach-shmobile/board-mackerel.c b/arch/arm/mach-shmobile/board-mackerel.c
> index 4ed0138..ccdd9fc 100644
> --- a/arch/arm/mach-shmobile/board-mackerel.c
> +++ b/arch/arm/mach-shmobile/board-mackerel.c
> @@ -431,6 +431,38 @@ static struct platform_device lcdc_device = {
> },
> };
>
> +/* HDMI */
> +static struct sh_mobile_lcdc_info hdmi_lcdc_info;
> +
> +static struct sh_mobile_hdmi_info hdmi_info = {
> + .lcd_chan = &hdmi_lcdc_info.ch[0],
> + .flags = HDMI_SND_SRC_SPDIF,
> +};
> +
> +static struct resource hdmi_resources[] = {
> + [0] = {
> + .name = "HDMI",
> + .start = 0xe6be0000,
> + .end = 0xe6be00ff,
> + .flags = IORESOURCE_MEM,
> + },
> + [1] = {
> + /* There's also an HDMI interrupt on INTCS @ 0x18e0 */
> + .start = evt2irq(0x17e0),
> + .flags = IORESOURCE_IRQ,
> + },
> +};
> +
> +static struct platform_device hdmi_device = {
> + .name = "sh-mobile-hdmi",
> + .num_resources = ARRAY_SIZE(hdmi_resources),
> + .resource = hdmi_resources,
> + .id = -1,
> + .dev = {
> + .platform_data = &hdmi_info,
> + },
> +};
> +
> static struct sh_mobile_meram_cfg hdmi_meram_cfg = {
> .icb[0] = {
> .marker_icb = 30,
> @@ -445,7 +477,7 @@ static struct sh_mobile_meram_cfg hdmi_meram_cfg = {
> .meram_size = 0x100,
> },
> };
> -/* HDMI */
> +
> static struct sh_mobile_lcdc_info hdmi_lcdc_info = {
> .meram_dev = &mackerel_meram_info,
> .clock_source = LCDC_CLK_EXTERNAL,
> @@ -456,6 +488,7 @@ static struct sh_mobile_lcdc_info hdmi_lcdc_info = {
> .clock_divider = 1,
> .flags = LCDC_FLAGS_DWPOL,
> .meram_cfg = &hdmi_meram_cfg,
> + .tx_dev = &hdmi_device,
> }
> };
>
> @@ -483,35 +516,6 @@ static struct platform_device hdmi_lcdc_device = {
> },
> };
>
> -static struct sh_mobile_hdmi_info hdmi_info = {
> - .lcd_chan = &hdmi_lcdc_info.ch[0],
> - .flags = HDMI_SND_SRC_SPDIF,
> -};
> -
> -static struct resource hdmi_resources[] = {
> - [0] = {
> - .name = "HDMI",
> - .start = 0xe6be0000,
> - .end = 0xe6be00ff,
> - .flags = IORESOURCE_MEM,
> - },
> - [1] = {
> - /* There's also an HDMI interrupt on INTCS @ 0x18e0 */
> - .start = evt2irq(0x17e0),
> - .flags = IORESOURCE_IRQ,
> - },
> -};
> -
> -static struct platform_device hdmi_device = {
> - .name = "sh-mobile-hdmi",
> - .num_resources = ARRAY_SIZE(hdmi_resources),
> - .resource = hdmi_resources,
> - .id = -1,
> - .dev = {
> - .platform_data = &hdmi_info,
> - },
> -};
> -
> static struct platform_device fsi_hdmi_device = {
> .name = "sh_fsi2_b_hdmi",
> };
> @@ -1313,8 +1317,8 @@ static struct platform_device *mackerel_devices[] __initdata = {
> &sh_mmcif_device,
> &ceu_device,
> &mackerel_camera,
> - &hdmi_lcdc_device,
> &hdmi_device,
> + &hdmi_lcdc_device,
> &meram_device,
> };
>
Sorry, could you elaborate, why this is needed? If this is really
important, then I'd prefer to test this before committing. If OTOH this is
unimportant, and my assumption, that normally it's the platform driver
registration order, that's important, not device registration order, is
correct, then you don't have to swap the order?
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply
* Re: [PATCH 21/57] sh_mobile_lcdc: Add an lcdc channel pointer to
From: Guennadi Liakhovetski @ 2011-12-15 16:16 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1323784972-24205-22-git-send-email-laurent.pinchart@ideasonboard.com>
Hi Laurent
On Tue, 13 Dec 2011, Laurent Pinchart wrote:
> The field will be used by the transmitter drivers to access
> sh_mobile_lcdc_chan fields such as fb_info.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/video/sh_mobile_lcdcfb.c | 5 ++++-
> drivers/video/sh_mobile_lcdcfb.h | 2 ++
> 2 files changed, 6 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/video/sh_mobile_lcdcfb.c b/drivers/video/sh_mobile_lcdcfb.c
> index cb5ea3c..2dccfde 100644
> --- a/drivers/video/sh_mobile_lcdcfb.c
> +++ b/drivers/video/sh_mobile_lcdcfb.c
> @@ -1497,8 +1497,10 @@ static int sh_mobile_lcdc_remove(struct platform_device *pdev)
> if (!info || !info->device)
> continue;
>
> - if (ch->tx_dev)
> + if (ch->tx_dev) {
> + ch->tx_dev->lcdc = NULL;
> module_put(ch->cfg.tx_dev->dev.driver->owner);
> + }
>
> if (ch->sglist)
> vfree(ch->sglist);
> @@ -1608,6 +1610,7 @@ sh_mobile_lcdc_channel_init(struct sh_mobile_lcdc_priv *priv,
> return -EINVAL;
> }
> ch->tx_dev = platform_get_drvdata(cfg->tx_dev);
> + ch->tx_dev->lcdc = ch;
I do not have a kernel, patched with your patches up to 20/57;-) so, I
cannot verify - can ch->tx_dev at this point not be NULL?
> }
>
> /* Iterate through the modes to validate them and find the highest
> diff --git a/drivers/video/sh_mobile_lcdcfb.h b/drivers/video/sh_mobile_lcdcfb.h
> index 9601b92..36cd564 100644
> --- a/drivers/video/sh_mobile_lcdcfb.h
> +++ b/drivers/video/sh_mobile_lcdcfb.h
> @@ -19,6 +19,7 @@ struct fb_info;
> struct module;
> struct sh_mobile_lcdc_entity;
> struct sh_mobile_lcdc_priv;
> +struct sh_mobile_lcdc_chan;
>
> struct sh_mobile_lcdc_entity_ops {
> /* Display */
> @@ -30,6 +31,7 @@ struct sh_mobile_lcdc_entity_ops {
> struct sh_mobile_lcdc_entity {
> struct module *owner;
> const struct sh_mobile_lcdc_entity_ops *ops;
> + struct sh_mobile_lcdc_chan *lcdc;
> };
>
> /*
> --
> 1.7.3.4
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply
* Re: [PATCH 24/57] fbdev: sh_mobile_lcdc: Return display connection
From: Guennadi Liakhovetski @ 2011-12-15 17:11 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1323784972-24205-25-git-send-email-laurent.pinchart@ideasonboard.com>
On Tue, 13 Dec 2011, Laurent Pinchart wrote:
> Return true if the display is connected and false otherwise. Set the fb
> info state to FBINFO_STATE_SUSPENDED in the sh_mobile_lcdc driver when
> the display is not connected.
Hmm... I'm not sure I like functions, that return either a negative error,
or _several_ non-negative success values. How about returning -ENODEV?
Thanks
Guennadi
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/video/sh_mipi_dsi.c | 2 +-
> drivers/video/sh_mobile_hdmi.c | 9 +++++----
> drivers/video/sh_mobile_lcdcfb.c | 3 +++
> drivers/video/sh_mobile_lcdcfb.h | 3 +++
> 4 files changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/video/sh_mipi_dsi.c b/drivers/video/sh_mipi_dsi.c
> index 1ede247..5ff3742 100644
> --- a/drivers/video/sh_mipi_dsi.c
> +++ b/drivers/video/sh_mipi_dsi.c
> @@ -412,7 +412,7 @@ static int mipi_display_on(struct sh_mobile_lcdc_entity *entity)
>
> sh_mipi_dsi_enable(mipi, true);
>
> - return 0;
> + return SH_MOBILE_LCDC_DISPLAY_CONNECTED;
>
> mipi_display_on_fail1:
> pm_runtime_put_sync(&mipi->pdev->dev);
> diff --git a/drivers/video/sh_mobile_hdmi.c b/drivers/video/sh_mobile_hdmi.c
> index c22e123..1464abf 100644
> --- a/drivers/video/sh_mobile_hdmi.c
> +++ b/drivers/video/sh_mobile_hdmi.c
> @@ -1004,9 +1004,9 @@ static int sh_hdmi_display_on(struct sh_mobile_lcdc_entity *entity)
> {
> struct sh_hdmi *hdmi = entity_to_sh_hdmi(entity);
> struct sh_mobile_lcdc_chan *ch = entity->lcdc;
> - struct fb_info *info = ch->info;
>
> - dev_dbg(hdmi->dev, "%s(%p): state %x\n", __func__, hdmi, info->state);
> + dev_dbg(hdmi->dev, "%s(%p): state %x\n", __func__, hdmi,
> + hdmi->hp_state);
>
> /*
> * hp_state can be set to
> @@ -1021,12 +1021,13 @@ static int sh_hdmi_display_on(struct sh_mobile_lcdc_entity *entity)
> dev_dbg(hdmi->dev, "HDMI running\n");
> break;
> case HDMI_HOTPLUG_DISCONNECTED:
> - info->state = FBINFO_STATE_SUSPENDED;
> default:
> hdmi->var = ch->display_var;
> }
>
> - return 0;
> + return hdmi->hp_state = HDMI_HOTPLUG_DISCONNECTED
> + ? SH_MOBILE_LCDC_DISPLAY_DISCONNECTED
> + : SH_MOBILE_LCDC_DISPLAY_CONNECTED;
> }
>
> static void sh_hdmi_display_off(struct sh_mobile_lcdc_entity *entity)
> diff --git a/drivers/video/sh_mobile_lcdcfb.c b/drivers/video/sh_mobile_lcdcfb.c
> index 47108aa..4b03aa5 100644
> --- a/drivers/video/sh_mobile_lcdcfb.c
> +++ b/drivers/video/sh_mobile_lcdcfb.c
> @@ -342,6 +342,9 @@ static void sh_mobile_lcdc_display_on(struct sh_mobile_lcdc_chan *ch)
> ret = ch->tx_dev->ops->display_on(ch->tx_dev);
> if (ret < 0)
> return;
> +
> + if (ret = SH_MOBILE_LCDC_DISPLAY_DISCONNECTED)
> + ch->info->state = FBINFO_STATE_SUSPENDED;
> }
>
> /* HDMI must be enabled before LCDC configuration */
> diff --git a/drivers/video/sh_mobile_lcdcfb.h b/drivers/video/sh_mobile_lcdcfb.h
> index b2cb8e6..6fb956c 100644
> --- a/drivers/video/sh_mobile_lcdcfb.h
> +++ b/drivers/video/sh_mobile_lcdcfb.h
> @@ -21,6 +21,9 @@ struct sh_mobile_lcdc_entity;
> struct sh_mobile_lcdc_priv;
> struct sh_mobile_lcdc_chan;
>
> +#define SH_MOBILE_LCDC_DISPLAY_DISCONNECTED 0
> +#define SH_MOBILE_LCDC_DISPLAY_CONNECTED 1
> +
> struct sh_mobile_lcdc_entity_ops {
> /* Display */
> int (*display_on)(struct sh_mobile_lcdc_entity *entity);
> --
> 1.7.3.4
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply
* Re: [PATCH v4 3/5] powerpc/mpc5121: shared DIU framebuffer support
From: Tabi Timur-B04825 @ 2011-12-15 17:27 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1279893639-24333-4-git-send-email-agust@denx.de>
On Fri, Jul 23, 2010 at 9:00 AM, Anatolij Gustschin <agust@denx.de> wrote:
> @@ -1471,7 +1476,9 @@ static int __devinit fsl_diu_probe(struct of_device *ofdev,
> goto error2;
> }
>
> - out_be32(&dr.diu_reg->diu_mode, 0); /* disable DIU anyway*/
> + diu_mode = in_be32(&dr.diu_reg->diu_mode);
> + if (diu_mode != MFB_MODE1)
> + out_be32(&dr.diu_reg->diu_mode, 0); /* disable DIU */
Anatolij,
I know this patch is old, but I just noticed something odd about it
that I need your help with.
In the above snippet, you test for != MFB_MODE1. My understanding is
that U-boot only supports modes 0 and 1, never modes 2 or 3. So
diu_mode can only ever be 0 or 1. That means that that the above code
is equivalent to:
diu_mode = in_be32(&dr.diu_reg->diu_mode);
if (diu_mode = 0)
out_be32(&dr.diu_reg->diu_mode, 0); /* disable DIU */
which is silly, because now we're writing 0 to diu_mode only if it's already 0.
Am I missing something?
--
Timur Tabi
Linux kernel developer at Freescale
^ permalink raw reply
* Re: [PATCH 27/57] fbdev: sh_mobile_lcdc: Pass a video mode to the
From: Guennadi Liakhovetski @ 2011-12-15 19:01 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1323784972-24205-28-git-send-email-laurent.pinchart@ideasonboard.com>
On Tue, 13 Dec 2011, Laurent Pinchart wrote:
> Pass pointers to struct fb_videomode and struct fb_monspecs instead of
> struct fb_var_screeninfo to the notify callback.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/video/sh_mobile_hdmi.c | 59 +++++++++++++++++--------------------
> drivers/video/sh_mobile_lcdcfb.c | 40 ++++++++++++++-----------
> drivers/video/sh_mobile_lcdcfb.h | 3 +-
> 3 files changed, 51 insertions(+), 51 deletions(-)
[snip]
> diff --git a/drivers/video/sh_mobile_lcdcfb.c b/drivers/video/sh_mobile_lcdcfb.c
> index 21e5f10..4ec216e 100644
> --- a/drivers/video/sh_mobile_lcdcfb.c
> +++ b/drivers/video/sh_mobile_lcdcfb.c
[ditto]
> @@ -433,12 +432,17 @@ static int sh_mobile_lcdc_display_notify(struct sh_mobile_lcdc_chan *ch,
> }
> break;
>
> - case SH_MOBILE_LCDC_EVENT_DISPLAY_MODE:
> + case SH_MOBILE_LCDC_EVENT_DISPLAY_MODE: {
> + struct fb_var_screeninfo var;
> +
> /* Validate a proposed new mode */
> - var->bits_per_pixel = info->var.bits_per_pixel;
> - ret = info->fbops->fb_check_var(var, info);
> + fb_videomode_to_var(&var, mode);
> + var.bits_per_pixel = info->var.bits_per_pixel;
> + var.grayscale = info->var.grayscale;
> + ret = info->fbops->fb_check_var(&var, info);
> break;
> }
> + }
nitpick - please, realign:-)
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply
* Should I use FBINFO_VIRTFB?
From: Timur Tabi @ 2011-12-15 20:50 UTC (permalink / raw)
To: linux-fbdev
I'm cleaning up my fbdev driver, and I just noticed FBINFO_VIRTFB:
#define FBINFO_VIRTFB 0x0004 /* FB is System RAM, not device. */
I am currently not setting this flag, but I am allocating my framebuffer in system ram via dma_alloc_coherent(). I don't see any good documentation for this flag, but I suspect I should be enabling it. What exactly does this flag do?
I'd also like some explanation for these two macros, which appear to be related:
#define FBINFO_PARTIAL_PAN_OK 0x0040 /* otw use pan only for double-buffering */
#define FBINFO_READS_FAST 0x0080 /* soft-copy faster than rendering */
--
Timur Tabi
Linux kernel developer at Freescale
^ permalink raw reply
* Re: Should I use FBINFO_VIRTFB?
From: Konrad Rzeszutek Wilk @ 2011-12-15 21:13 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <4EEA5DA7.3010406@freescale.com>
On Thu, Dec 15, 2011 at 02:50:47PM -0600, Timur Tabi wrote:
> I'm cleaning up my fbdev driver, and I just noticed FBINFO_VIRTFB:
>
> #define FBINFO_VIRTFB 0x0004 /* FB is System RAM, not device. */
>
> I am currently not setting this flag, but I am allocating my framebuffer in system ram via dma_alloc_coherent(). I don't see any good documentation for this flag, but I suspect I should be enabling it. What exactly does this flag do?
It basically inhibits from using VM_IO which should not be set on
System RAM. In your case you are using System RAM, so please do set it.
>
> I'd also like some explanation for these two macros, which appear to be related:
>
> #define FBINFO_PARTIAL_PAN_OK 0x0040 /* otw use pan only for double-buffering */
> #define FBINFO_READS_FAST 0x0080 /* soft-copy faster than rendering */
Not really. They have a different function, but you are better of looking in the code
to see how they are used.
>
> --
> Timur Tabi
> Linux kernel developer at Freescale
>
> --
> 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: [PATCH v4 3/5] powerpc/mpc5121: shared DIU framebuffer support
From: Anatolij Gustschin @ 2011-12-15 21:26 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1279893639-24333-4-git-send-email-agust@denx.de>
On Thu, 15 Dec 2011 17:27:53 +0000
Tabi Timur-B04825 <B04825@freescale.com> wrote:
> On Fri, Jul 23, 2010 at 9:00 AM, Anatolij Gustschin <agust@denx.de> wrote:
>
> > @@ -1471,7 +1476,9 @@ static int __devinit fsl_diu_probe(struct of_device *ofdev,
> > goto error2;
> > }
> >
> > - out_be32(&dr.diu_reg->diu_mode, 0); /* disable DIU anyway*/
> > + diu_mode = in_be32(&dr.diu_reg->diu_mode);
> > + if (diu_mode != MFB_MODE1)
> > + out_be32(&dr.diu_reg->diu_mode, 0); /* disable DIU */
>
> Anatolij,
>
> I know this patch is old, but I just noticed something odd about it
> that I need your help with.
>
> In the above snippet, you test for != MFB_MODE1. My understanding is
> that U-boot only supports modes 0 and 1, never modes 2 or 3. So
> diu_mode can only ever be 0 or 1. That means that that the above code
> is equivalent to:
>
> diu_mode = in_be32(&dr.diu_reg->diu_mode);
> if (diu_mode = 0)
> out_be32(&dr.diu_reg->diu_mode, 0); /* disable DIU */
>
> which is silly, because now we're writing 0 to diu_mode only if it's already 0.
>
> Am I missing something?
the intention of the above code snippet was:
not to disable DIU if it is in mode 1 (displaying splash screen)
and to disable DIU if it is in modes 2, 3 or 4 for some reason.
We cannot guarantee that the DIU is not in these modes. Even
if U-Boot didn't set these modes there is still a possibility
that such mode is configured. E.g. I've seen U-Boot binary
standalone applications for other display controllers initializing
the display controller.
But you are right. With this snippet, if the DIU is already
disabled, there will be not needed mode register access. So
the code should better look like:
diu_mode = in_be32(&dr.diu_reg->diu_mode);
if (diu_mode && diu_mode != MFB_MODE1)
out_be32(&dr.diu_reg->diu_mode, 0);
Anatolij
^ permalink raw reply
* Re: Should I use FBINFO_VIRTFB?
From: Timur Tabi @ 2011-12-15 22:10 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <4EEA5DA7.3010406@freescale.com>
Konrad Rzeszutek Wilk wrote:
>> > #define FBINFO_PARTIAL_PAN_OK 0x0040 /* otw use pan only for double-buffering */
>> > #define FBINFO_READS_FAST 0x0080 /* soft-copy faster than rendering */
> Not really. They have a different function, but you are better of looking in the code
> to see how they are used.
Well, I tried that, which is why I said, "I don't see any good documentation." FBINFO_PARTIAL_PAN_OK appears to be only used in one place:
case SCROLL_PAN_REDRAW:
if ((p->yscroll + count < 2 * (p->vrows - vc->vc_rows))
&& ((!scroll_partial && (b - t = vc->vc_rows))
|| (scroll_partial
&& (b - t - count >
3 * vc->vc_rows >> 2)))) {
if (t > 0)
fbcon_redraw_move(vc, p, 0, t, count);
ypan_up_redraw(vc, t, count);
if (vc->vc_rows - b > 0)
fbcon_redraw_move(vc, p, b,
vc->vc_rows - b, b);
} else
fbcon_redraw_move(vc, p, t + count, b - t - count, t);
fbcon_clear(vc, b - count, 0, count, vc->vc_cols);
break;
I can't parse this, and I can't figure out if my driver is better off with or without FBINFO_PARTIAL_PAN_OK.
I have the same problem with FBINFO_READS_FAST.
So I really would like someone explain these macros to me.
--
Timur Tabi
Linux kernel developer at Freescale
^ permalink raw reply
* Re: [PATCH 33/57] fbdev: sh_mobile_lcdc: Add sh_mobile_format_info()
From: Guennadi Liakhovetski @ 2011-12-15 22:17 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1323784972-24205-34-git-send-email-laurent.pinchart@ideasonboard.com>
On Tue, 13 Dec 2011, Laurent Pinchart wrote:
> The function returns a pointer to a structure describing a format based
> on its fourcc. Use the function where applicable instead of hardcoded
> switch-case statements.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/video/sh_mobile_lcdcfb.c | 174 ++++++++++++++++++++++----------------
> 1 files changed, 102 insertions(+), 72 deletions(-)
>
> diff --git a/drivers/video/sh_mobile_lcdcfb.c b/drivers/video/sh_mobile_lcdcfb.c
> index 3bc82ae..c6b6b9d 100644
> --- a/drivers/video/sh_mobile_lcdcfb.c
> +++ b/drivers/video/sh_mobile_lcdcfb.c
[snip]
> @@ -665,37 +726,20 @@ static void __sh_mobile_lcdc_start(struct sh_mobile_lcdc_priv *priv)
>
> /* Setup geometry, format, frame buffer memory and operation mode. */
> for (k = 0; k < ARRAY_SIZE(priv->ch); k++) {
> + const struct sh_mobile_lcdc_format_info *format;
> + u32 fourcc;
> +
> ch = &priv->ch[k];
> if (!ch->enabled)
> continue;
>
> sh_mobile_lcdc_geometry(ch);
>
> - switch (sh_mobile_format_fourcc(&ch->info->var)) {
> - case V4L2_PIX_FMT_RGB565:
> - tmp = LDDFR_PKF_RGB16;
> - break;
> - case V4L2_PIX_FMT_BGR24:
> - tmp = LDDFR_PKF_RGB24;
> - break;
> - case V4L2_PIX_FMT_BGR32:
> - tmp = LDDFR_PKF_ARGB32;
> - break;
> - case V4L2_PIX_FMT_NV12:
> - case V4L2_PIX_FMT_NV21:
> - tmp = LDDFR_CC | LDDFR_YF_420;
> - break;
> - case V4L2_PIX_FMT_NV16:
> - case V4L2_PIX_FMT_NV61:
> - tmp = LDDFR_CC | LDDFR_YF_422;
> - break;
> - case V4L2_PIX_FMT_NV24:
> - case V4L2_PIX_FMT_NV42:
> - tmp = LDDFR_CC | LDDFR_YF_444;
> - break;
> - }
> + fourcc = sh_mobile_format_fourcc(&ch->info->var);
> + format = sh_mobile_format_info(fourcc);
> + tmp = format->lddfr;
Why don't you just store a pointer to the selected format struct per
channel in sh_mobile_lcdc_channel_init() to avoid recalculation here? If
OTOH this can be a new info here from a hotplug event, the above risks an
Oops?
>
> - if (sh_mobile_format_is_yuv(&ch->info->var)) {
> + if (format->yuv) {
> switch (ch->info->var.colorspace) {
> case V4L2_COLORSPACE_REC709:
> tmp |= LDDFR_CF1;
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply
* Re: [PATCH 34/57] fbdev: sh_mobile_lcdc: Store the format in struct
From: Guennadi Liakhovetski @ 2011-12-15 22:29 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1323784972-24205-35-git-send-email-laurent.pinchart@ideasonboard.com>
On Tue, 13 Dec 2011, Laurent Pinchart wrote:
> Store the active format in the channel structure, and use it instead of
> parsing info->var all over the place when the format is needed.
Right, this is what I was wondering about, while looking at the previous
patch:-) But:
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/video/sh_mobile_lcdcfb.c | 21 ++++++++++-----------
> drivers/video/sh_mobile_lcdcfb.h | 4 +++-
> 2 files changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/video/sh_mobile_lcdcfb.c b/drivers/video/sh_mobile_lcdcfb.c
> index c6b6b9d..9829e01 100644
> --- a/drivers/video/sh_mobile_lcdcfb.c
> +++ b/drivers/video/sh_mobile_lcdcfb.c
[snip]
> @@ -1350,6 +1345,8 @@ static int sh_mobile_set_par(struct fb_info *info)
> info->fix.line_length = info->var.xres
> * info->var.bits_per_pixel / 8;
>
> + ch->format = sh_mobile_format_info(sh_mobile_format_fourcc(&info->var));
Cannot this be NULL? As far as I could trace it back, I'm not sure with
hotplug bits_per_pixel would be initialised correctly along the lines of
sh_mobile_fb_reconfig().
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply
* Re: [PATCH 1/2] OMAPDSS: DISPC: Update Fir Coefficients
From: Mahapatra, Chandrabhanu @ 2011-12-16 4:58 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-omap, linux-fbdev
In-Reply-To: <1323939331.2010.27.camel@deskari>
On Thu, Dec 15, 2011 at 2:25 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Wed, 2011-12-14 at 10:21 +0530, Chandrabhanu Mahapatra wrote:
>
>> +const struct dispc_coef *dispc_ovl_get_scale_coef(int inc, int five_taps)
>> +{
>> + int i;
>> + static const struct {
>> + int Mmin;
>> + int Mmax;
>> + const struct dispc_coef *coef_3;
>> + const struct dispc_coef *coef_5;
>> + } coefs[] = {
>> + { 26, 32, coef3_M32, coef5_M32 },
>> + { 22, 26, coef3_M26, coef5_M26 },
>> + { 19, 22, coef3_M22, coef5_M22 },
>> + { 16, 19, coef3_M19, coef5_M19 },
>> + { 14, 16, coef3_M16, coef5_M16 },
>> + { 13, 14, coef3_M14, coef5_M14 },
>> + { 12, 13, coef3_M13, coef5_M13 },
>> + { 11, 12, coef3_M12, coef5_M12 },
>> + { 10, 11, coef3_M11, coef5_M11 },
>> + { 9, 10, coef3_M10, coef5_M10 },
>> + { 8, 9, coef3_M9, coef5_M9 },
>> + { 3, 8, coef3_M8, coef5_M8 },
>> + /*
>> + * When upscaling more than two times, blockiness and outlines
>> + * around the image are observed when M8 tables are used. M11,
>> + * M16 and M19 tables are used to prevent this.
>> + */
>> + { 2, 3, coef3_M11, coef5_M11 },
>> + { 1, 2, coef3_M16, coef5_M16 },
>> + };
>> +
>> + inc /= 128;
>> + for (i = 0; i < ARRAY_LEN(coefs); ++i)
>> + if (inc > coefs[i].Mmin && inc <= coefs[i].Mmax)
>> + return five_taps ? coefs[i].coef_5 : coefs[i].coef_3;
>> + if (inc = 1)
>> + return five_taps ? coef3_M19 : coef5_M19;
>> + return NULL;
>> +}
>
> Why don't you handle the inc = 1 case the same as others? Just have an
> entry in the table for Mmin=0, Mmax = 1.
>
For inc=1 i.e. M=1 , scaling ratio is maximum as L/M=8. DISPC scaler
doesnot support upscaling more than 8 itmes. Having an (Mmin,Mmax] of
(0,1] will allow such cases.
> Also, I think it's a bit confusing that Mmin is exclusive and Mmax is
> inclusive in the comparison. It makes the table a bit hard to read, when
> looking at which entry is used for which inc. I'd recommend using
> inclusive comparison for both.
>
> Tomi
>
Having both inclusive will allow us to delete the extra comparison for
inc=1 but in my opinion having Mmin exclusive and Mmax inclusive
actually gives an clear idea of comparison. The tables mostly go by
the Mmax value.
For example, for inc& coef3/5_M26 table is selected, for inc"
coef3/5_M22 is selected etc.
If we have both Mmin and Mmax as inclusive above case becomes slightly
incoherent. Say for M& instead of coef3/5_M26 which seems more
obvious choice coef3/5_M32 is selected.
For both inclusive cases to work and avoid confusion and delete extra
comparison for inc=1 , I have to reverse the order of table entries
in "coef" table. But for that I will have to put the "When upscaling
more than two times, blockiness and outlines" comment at the beginning
of the table and then start with { 1, 2, coef3_M16, coef5_M16 }.
This will create even more confusion.
--
Chandrabhanu Mahapatra
Texas Instruments India Pvt. Ltd.
^ permalink raw reply
* Re: [PATCH 1/2] OMAPDSS: DISPC: Update Fir Coefficients
From: Tomi Valkeinen @ 2011-12-16 8:15 UTC (permalink / raw)
To: Mahapatra, Chandrabhanu; +Cc: linux-omap, linux-fbdev
In-Reply-To: <CAF0AtAtaCGpgeYxss0H_jbYWOwJJ_LW0W8-eqM20juKTAGassA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 4198 bytes --]
On Fri, 2011-12-16 at 10:26 +0530, Mahapatra, Chandrabhanu wrote:
> On Thu, Dec 15, 2011 at 2:25 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> > On Wed, 2011-12-14 at 10:21 +0530, Chandrabhanu Mahapatra wrote:
> >
> >> +const struct dispc_coef *dispc_ovl_get_scale_coef(int inc, int five_taps)
> >> +{
> >> + int i;
> >> + static const struct {
> >> + int Mmin;
> >> + int Mmax;
> >> + const struct dispc_coef *coef_3;
> >> + const struct dispc_coef *coef_5;
> >> + } coefs[] = {
> >> + { 26, 32, coef3_M32, coef5_M32 },
> >> + { 22, 26, coef3_M26, coef5_M26 },
> >> + { 19, 22, coef3_M22, coef5_M22 },
> >> + { 16, 19, coef3_M19, coef5_M19 },
> >> + { 14, 16, coef3_M16, coef5_M16 },
> >> + { 13, 14, coef3_M14, coef5_M14 },
> >> + { 12, 13, coef3_M13, coef5_M13 },
> >> + { 11, 12, coef3_M12, coef5_M12 },
> >> + { 10, 11, coef3_M11, coef5_M11 },
> >> + { 9, 10, coef3_M10, coef5_M10 },
> >> + { 8, 9, coef3_M9, coef5_M9 },
> >> + { 3, 8, coef3_M8, coef5_M8 },
> >> + /*
> >> + * When upscaling more than two times, blockiness and outlines
> >> + * around the image are observed when M8 tables are used. M11,
> >> + * M16 and M19 tables are used to prevent this.
> >> + */
> >> + { 2, 3, coef3_M11, coef5_M11 },
> >> + { 1, 2, coef3_M16, coef5_M16 },
> >> + };
> >> +
> >> + inc /= 128;
> >> + for (i = 0; i < ARRAY_LEN(coefs); ++i)
> >> + if (inc > coefs[i].Mmin && inc <= coefs[i].Mmax)
> >> + return five_taps ? coefs[i].coef_5 : coefs[i].coef_3;
> >> + if (inc == 1)
> >> + return five_taps ? coef3_M19 : coef5_M19;
> >> + return NULL;
> >> +}
> >
> > Why don't you handle the inc == 1 case the same as others? Just have an
> > entry in the table for Mmin=0, Mmax = 1.
> >
> For inc=1 i.e. M=1 , scaling ratio is maximum as L/M=8. DISPC scaler
> doesnot support upscaling more than 8 itmes. Having an (Mmin,Mmax] of
> (0,1] will allow such cases.
I don't think I understand. A table entry for 0,1 would match exactly
one inc value, which is 1. Which is the same as you do with the separate
if statement now.
> > Also, I think it's a bit confusing that Mmin is exclusive and Mmax is
> > inclusive in the comparison. It makes the table a bit hard to read, when
> > looking at which entry is used for which inc. I'd recommend using
> > inclusive comparison for both.
> >
> > Tomi
> >
> Having both inclusive will allow us to delete the extra comparison for
> inc==1 but in my opinion having Mmin exclusive and Mmax inclusive
> actually gives an clear idea of comparison. The tables mostly go by
> the Mmax value.
> For example, for inc=26 coef3/5_M26 table is selected, for inc=22
> coef3/5_M22 is selected etc.
> If we have both Mmin and Mmax as inclusive above case becomes slightly
> incoherent. Say for M=26 instead of coef3/5_M26 which seems more
> obvious choice coef3/5_M32 is selected.
I don't understand this either... If you now have:
{ 26, 32, coef3_M32, coef5_M32 },
{ 22, 26, coef3_M26, coef5_M26 },
It would be changed to
{ 27, 32, coef3_M32, coef5_M32 },
{ 23, 26, coef3_M26, coef5_M26 },
and it would match the same inc values as before after changing the Mmin
comparison to >=.
> For both inclusive cases to work and avoid confusion and delete extra
> comparison for inc==1 , I have to reverse the order of table entries
> in "coef" table. But for that I will have to put the "When upscaling
> more than two times, blockiness and outlines" comment at the beginning
> of the table and then start with { 1, 2, coef3_M16, coef5_M16 }.
> This will create even more confusion.
The ranges for the table elements are exclusive. The order doesn't
matter because one inc value can only match one table entry. So I have
to say I don't understand this comment either =). Am I missing
something?
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 1/2] OMAPDSS: DISPC: Update Fir Coefficients
From: Mahapatra, Chandrabhanu @ 2011-12-16 8:48 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-fbdev, linux-omap
In-Reply-To: <1324023305.1859.9.camel@deskari>
On Fri, Dec 16, 2011 at 1:45 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Fri, 2011-12-16 at 10:26 +0530, Mahapatra, Chandrabhanu wrote:
>> On Thu, Dec 15, 2011 at 2:25 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> > On Wed, 2011-12-14 at 10:21 +0530, Chandrabhanu Mahapatra wrote:
>> >
>> >> +const struct dispc_coef *dispc_ovl_get_scale_coef(int inc, int five_taps)
>> >> +{
>> >> + int i;
>> >> + static const struct {
>> >> + int Mmin;
>> >> + int Mmax;
>> >> + const struct dispc_coef *coef_3;
>> >> + const struct dispc_coef *coef_5;
>> >> + } coefs[] = {
>> >> + { 26, 32, coef3_M32, coef5_M32 },
>> >> + { 22, 26, coef3_M26, coef5_M26 },
>> >> + { 19, 22, coef3_M22, coef5_M22 },
>> >> + { 16, 19, coef3_M19, coef5_M19 },
>> >> + { 14, 16, coef3_M16, coef5_M16 },
>> >> + { 13, 14, coef3_M14, coef5_M14 },
>> >> + { 12, 13, coef3_M13, coef5_M13 },
>> >> + { 11, 12, coef3_M12, coef5_M12 },
>> >> + { 10, 11, coef3_M11, coef5_M11 },
>> >> + { 9, 10, coef3_M10, coef5_M10 },
>> >> + { 8, 9, coef3_M9, coef5_M9 },
>> >> + { 3, 8, coef3_M8, coef5_M8 },
>> >> + /*
>> >> + * When upscaling more than two times, blockiness and outlines
>> >> + * around the image are observed when M8 tables are used. M11,
>> >> + * M16 and M19 tables are used to prevent this.
>> >> + */
>> >> + { 2, 3, coef3_M11, coef5_M11 },
>> >> + { 1, 2, coef3_M16, coef5_M16 },
>> >> + };
>> >> +
>> >> + inc /= 128;
>> >> + for (i = 0; i < ARRAY_LEN(coefs); ++i)
>> >> + if (inc > coefs[i].Mmin && inc <= coefs[i].Mmax)
>> >> + return five_taps ? coefs[i].coef_5 : coefs[i].coef_3;
>> >> + if (inc = 1)
>> >> + return five_taps ? coef3_M19 : coef5_M19;
>> >> + return NULL;
>> >> +}
>> >
>> > Why don't you handle the inc = 1 case the same as others? Just have an
>> > entry in the table for Mmin=0, Mmax = 1.
>> >
>> For inc=1 i.e. M=1 , scaling ratio is maximum as L/M=8. DISPC scaler
>> doesnot support upscaling more than 8 itmes. Having an (Mmin,Mmax] of
>> (0,1] will allow such cases.
>
> I don't think I understand. A table entry for 0,1 would match exactly
> one inc value, which is 1. Which is the same as you do with the separate
> if statement now.
>
yes, you are right.
>> > Also, I think it's a bit confusing that Mmin is exclusive and Mmax is
>> > inclusive in the comparison. It makes the table a bit hard to read, when
>> > looking at which entry is used for which inc. I'd recommend using
>> > inclusive comparison for both.
>> >
>> > Tomi
>> >
>> Having both inclusive will allow us to delete the extra comparison for
>> inc=1 but in my opinion having Mmin exclusive and Mmax inclusive
>> actually gives an clear idea of comparison. The tables mostly go by
>> the Mmax value.
>> For example, for inc& coef3/5_M26 table is selected, for inc"
>> coef3/5_M22 is selected etc.
>> If we have both Mmin and Mmax as inclusive above case becomes slightly
>> incoherent. Say for M& instead of coef3/5_M26 which seems more
>> obvious choice coef3/5_M32 is selected.
>
> I don't understand this either... If you now have:
>
> { 26, 32, coef3_M32, coef5_M32 },
> { 22, 26, coef3_M26, coef5_M26 },
>
> It would be changed to
>
> { 27, 32, coef3_M32, coef5_M32 },
> { 23, 26, coef3_M26, coef5_M26 },
>
> and it would match the same inc values as before after changing the Mmin
> comparison to >=.
>
yes, I had mistakenly overlooked it.
>> For both inclusive cases to work and avoid confusion and delete extra
>> comparison for inc=1 , I have to reverse the order of table entries
>> in "coef" table. But for that I will have to put the "When upscaling
>> more than two times, blockiness and outlines" comment at the beginning
>> of the table and then start with { 1, 2, coef3_M16, coef5_M16 }.
>> This will create even more confusion.
>
> The ranges for the table elements are exclusive. The order doesn't
> matter because one inc value can only match one table entry. So I have
> to say I don't understand this comment either =). Am I missing
> something?
>
> Tomi
>
I mistakenly thought inc to be float which created all this confusion.
--
Chandrabhanu Mahapatra
Texas Instruments India Pvt. Ltd.
^ permalink raw reply
* Re: [PATCH 25/57] sh_mobile_lcdc: Add display notify callback to
From: Guennadi Liakhovetski @ 2011-12-16 9:40 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1323784972-24205-26-git-send-email-laurent.pinchart@ideasonboard.com>
On Tue, 13 Dec 2011, Laurent Pinchart wrote:
> The callback implements 3 notification events:
>
> - SH_MOBILE_LCDC_EVENT_DISPLAY_CONNECT notifies the LCDC that the
> display has been connected
> - SH_MOBILE_LCDC_EVENT_DISPLAY_DISCONNECT notifies the LCDC that the
> display has been disconnected
> - SH_MOBILE_LCDC_EVENT_DISPLAY_MODE notifies that LCDC that a display
> mode has been detected
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/video/sh_mobile_lcdcfb.c | 81 ++++++++++++++++++++++++++++++++++++++
> drivers/video/sh_mobile_lcdcfb.h | 10 +++++
> 2 files changed, 91 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/video/sh_mobile_lcdcfb.c b/drivers/video/sh_mobile_lcdcfb.c
> index 4b03aa5..21e5f10 100644
> --- a/drivers/video/sh_mobile_lcdcfb.c
> +++ b/drivers/video/sh_mobile_lcdcfb.c
> @@ -363,6 +363,86 @@ static void sh_mobile_lcdc_display_off(struct sh_mobile_lcdc_chan *ch)
> ch->tx_dev->ops->display_off(ch->tx_dev);
> }
>
> +static bool
> +sh_mobile_lcdc_must_reconfigure(struct sh_mobile_lcdc_chan *ch,
> + const struct fb_var_screeninfo *new_var)
> +{
> + struct fb_var_screeninfo *old_var = &ch->display_var;
> + struct fb_videomode old_mode;
> + struct fb_videomode new_mode;
> +
> + fb_var_to_videomode(&old_mode, old_var);
> + fb_var_to_videomode(&new_mode, new_var);
> +
> + dev_dbg(ch->info->dev, "Old %ux%u, new %ux%u\n",
> + old_mode.xres, old_mode.yres, new_mode.xres, new_mode.yres);
> +
> + if (fb_mode_is_equal(&old_mode, &new_mode)) {
> + /* It can be a different monitor with an equal video-mode */
> + old_var->width = new_var->width;
> + old_var->height = new_var->height;
> + return false;
> + }
> +
> + dev_dbg(ch->info->dev, "Switching %u -> %u lines\n",
> + old_mode.yres, new_mode.yres);
> + *old_var = *new_var;
> +
> + return true;
> +}
> +
> +static int sh_mobile_lcdc_display_notify(struct sh_mobile_lcdc_chan *ch,
> + enum sh_mobile_lcdc_entity_event event,
> + struct fb_var_screeninfo *var)
> +{
> + struct fb_info *info = ch->info;
> + int ret = 0;
> +
> + switch (event) {
> + case SH_MOBILE_LCDC_EVENT_DISPLAY_CONNECT:
> + /* HDMI plug in */
> + if (lock_fb_info(info)) {
> + console_lock();
> +
> + if (!sh_mobile_lcdc_must_reconfigure(ch, var) &&
> + info->state = FBINFO_STATE_RUNNING) {
> + /* First activation with the default monitor.
> + * Just turn on, if we run a resume here, the
> + * logo disappears.
> + */
> + info->var.width = var->width;
> + info->var.height = var->height;
> + sh_mobile_lcdc_display_on(ch);
> + } else {
> + /* New monitor or have to wake up */
> + fb_set_suspend(info, 0);
> + }
> +
> + console_unlock();
> + unlock_fb_info(info);
> + }
> + break;
> +
> + case SH_MOBILE_LCDC_EVENT_DISPLAY_DISCONNECT:
> + /* HDMI disconnect */
> + if (lock_fb_info(info)) {
> + console_lock();
> + fb_set_suspend(info, 1);
> + console_unlock();
> + unlock_fb_info(info);
> + }
> + break;
> +
> + case SH_MOBILE_LCDC_EVENT_DISPLAY_MODE:
> + /* Validate a proposed new mode */
> + var->bits_per_pixel = info->var.bits_per_pixel;
> + ret = info->fbops->fb_check_var(var, info);
You can call sh_mobile_lcdc_check_var() directly here too, right?
Thanks
Guennadi
> + break;
> + }
> +
> + return ret;
> +}
> +
> /* -----------------------------------------------------------------------------
> * Format helpers
> */
> @@ -1591,6 +1671,7 @@ sh_mobile_lcdc_channel_init(struct sh_mobile_lcdc_priv *priv,
> int i;
>
> mutex_init(&ch->open_lock);
> + ch->notify = sh_mobile_lcdc_display_notify;
>
> /* Allocate the frame buffer device. */
> ch->info = framebuffer_alloc(0, priv->dev);
> diff --git a/drivers/video/sh_mobile_lcdcfb.h b/drivers/video/sh_mobile_lcdcfb.h
> index 6fb956c..e2eb7af 100644
> --- a/drivers/video/sh_mobile_lcdcfb.h
> +++ b/drivers/video/sh_mobile_lcdcfb.h
> @@ -30,6 +30,12 @@ struct sh_mobile_lcdc_entity_ops {
> void (*display_off)(struct sh_mobile_lcdc_entity *entity);
> };
>
> +enum sh_mobile_lcdc_entity_event {
> + SH_MOBILE_LCDC_EVENT_DISPLAY_CONNECT,
> + SH_MOBILE_LCDC_EVENT_DISPLAY_DISCONNECT,
> + SH_MOBILE_LCDC_EVENT_DISPLAY_MODE,
> +};
> +
> struct sh_mobile_lcdc_entity {
> struct module *owner;
> const struct sh_mobile_lcdc_entity_ops *ops;
> @@ -70,6 +76,10 @@ struct sh_mobile_lcdc_chan {
> unsigned long base_addr_y;
> unsigned long base_addr_c;
> unsigned int pitch;
> +
> + int (*notify)(struct sh_mobile_lcdc_chan *ch,
> + enum sh_mobile_lcdc_entity_event event,
> + struct fb_var_screeninfo *var);
> };
>
> #endif
> --
> 1.7.3.4
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply
* Re: [PATCH 21/57] sh_mobile_lcdc: Add an lcdc channel pointer to sh_mobile_lcdc_entity
From: Laurent Pinchart @ 2011-12-16 9:52 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1323784972-24205-22-git-send-email-laurent.pinchart@ideasonboard.com>
Hi Guennadi,
On Thursday 15 December 2011 17:16:16 Guennadi Liakhovetski wrote:
> On Tue, 13 Dec 2011, Laurent Pinchart wrote:
> > The field will be used by the transmitter drivers to access
> > sh_mobile_lcdc_chan fields such as fb_info.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >
> > drivers/video/sh_mobile_lcdcfb.c | 5 ++++-
> > drivers/video/sh_mobile_lcdcfb.h | 2 ++
> > 2 files changed, 6 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/video/sh_mobile_lcdcfb.c
> > b/drivers/video/sh_mobile_lcdcfb.c index cb5ea3c..2dccfde 100644
> > --- a/drivers/video/sh_mobile_lcdcfb.c
> > +++ b/drivers/video/sh_mobile_lcdcfb.c
> > @@ -1497,8 +1497,10 @@ static int sh_mobile_lcdc_remove(struct
> > platform_device *pdev)
> >
> > if (!info || !info->device)
> >
> > continue;
> >
> > - if (ch->tx_dev)
> > + if (ch->tx_dev) {
> > + ch->tx_dev->lcdc = NULL;
> >
> > module_put(ch->cfg.tx_dev->dev.driver->owner);
> >
> > + }
> >
> > if (ch->sglist)
> >
> > vfree(ch->sglist);
> >
> > @@ -1608,6 +1610,7 @@ sh_mobile_lcdc_channel_init(struct
> > sh_mobile_lcdc_priv *priv,
> >
> > return -EINVAL;
> >
> > }
> > ch->tx_dev = platform_get_drvdata(cfg->tx_dev);
> >
> > + ch->tx_dev->lcdc = ch;
>
> I do not have a kernel, patched with your patches up to 20/57;-) so, I
> cannot verify - can ch->tx_dev at this point not be NULL?
I don't think so, as the
if (!cfg->tx_dev->dev.driver ||
!try_module_get(cfg->tx_dev->dev.driver->owner))
check above ensures that the transmitter driver is loaded and bound to the
device. As the transmitter driver calls platform_Set_drvdata() at probe time,
platform_get_drvdata() will not return NULL.
> > }
> >
> > /* Iterate through the modes to validate them and find the highest
> >
> > diff --git a/drivers/video/sh_mobile_lcdcfb.h
> > b/drivers/video/sh_mobile_lcdcfb.h index 9601b92..36cd564 100644
> > --- a/drivers/video/sh_mobile_lcdcfb.h
> > +++ b/drivers/video/sh_mobile_lcdcfb.h
> > @@ -19,6 +19,7 @@ struct fb_info;
> >
> > struct module;
> > struct sh_mobile_lcdc_entity;
> > struct sh_mobile_lcdc_priv;
> >
> > +struct sh_mobile_lcdc_chan;
> >
> > struct sh_mobile_lcdc_entity_ops {
> >
> > /* Display */
> >
> > @@ -30,6 +31,7 @@ struct sh_mobile_lcdc_entity_ops {
> >
> > struct sh_mobile_lcdc_entity {
> >
> > struct module *owner;
> > const struct sh_mobile_lcdc_entity_ops *ops;
> >
> > + struct sh_mobile_lcdc_chan *lcdc;
> >
> > };
> >
> > /*
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH 12/57] arm: mach-shmobile: Add LCDC tx_dev field to platform data
From: Laurent Pinchart @ 2011-12-16 10:04 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1323784972-24205-13-git-send-email-laurent.pinchart@ideasonboard.com>
Hi Guennadi,
On Thursday 15 December 2011 15:01:27 Guennadi Liakhovetski wrote:
> On Tue, 13 Dec 2011, Laurent Pinchart wrote:
> > Make sure the transmitter devices get registered before the associated
> > LCDC devices.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
[snip]
> > diff --git a/arch/arm/mach-shmobile/board-ap4evb.c
> > b/arch/arm/mach-shmobile/board-ap4evb.c index dca4860..e0a6b3d 100644
> > --- a/arch/arm/mach-shmobile/board-ap4evb.c
> > +++ b/arch/arm/mach-shmobile/board-ap4evb.c
[snip]
> > @@ -627,6 +553,86 @@ static struct platform_device *qhd_devices[]
> > __initdata = {
> >
> > };
> > #endif /* CONFIG_AP4EVB_QHD */
> >
> > +/* LCDC0 */
> > +static const struct fb_videomode ap4evb_lcdc_modes[] = {
>
> Hm, I must be missing something. I thought you were moving all the structs
> around to make them available for reference _without_ forward
> declarations. But you _do_ end up using a forward declaration anyway. Here
> and for HDMI below too. So, what's the point? Why not just forward declare
> that one struct, that's needed and leave the rest where it currently is
> in the file?
As we have circular dependencies forward declaration can't be avoided. I found
it cleaner to have devices data in registration order in the board file, but I
can remove this if you prefer.
> > + {
> > +#ifdef CONFIG_AP4EVB_QHD
> > + .name = "R63302(QHD)",
> > + .xres = 544,
> > + .yres = 961,
> > + .left_margin = 72,
> > + .right_margin = 600,
> > + .hsync_len = 16,
> > + .upper_margin = 8,
> > + .lower_margin = 8,
> > + .vsync_len = 2,
> > + .sync = FB_SYNC_VERT_HIGH_ACT | FB_SYNC_HOR_HIGH_ACT,
> > +#else
> > + .name = "WVGA Panel",
> > + .xres = 800,
> > + .yres = 480,
> > + .left_margin = 220,
> > + .right_margin = 110,
> > + .hsync_len = 70,
> > + .upper_margin = 20,
> > + .lower_margin = 5,
> > + .vsync_len = 5,
> > + .sync = 0,
> > +#endif
> > + },
> > +};
[snip]
> > diff --git a/arch/arm/mach-shmobile/board-mackerel.c
> > b/arch/arm/mach-shmobile/board-mackerel.c index 4ed0138..ccdd9fc 100644
> > --- a/arch/arm/mach-shmobile/board-mackerel.c
> > +++ b/arch/arm/mach-shmobile/board-mackerel.c
[snip]
> > @@ -1313,8 +1317,8 @@ static struct platform_device *mackerel_devices[]
> > __initdata = {
> > &sh_mmcif_device,
> > &ceu_device,
> > &mackerel_camera,
> > - &hdmi_lcdc_device,
> > &hdmi_device,
> > + &hdmi_lcdc_device,
> > &meram_device,
> > };
>
> Sorry, could you elaborate, why this is needed? If this is really
> important, then I'd prefer to test this before committing. If OTOH this is
> unimportant, and my assumption, that normally it's the platform driver
> registration order, that's important, not device registration order, is
> correct, then you don't have to swap the order?
If both the transmitter and LCDC drivers are built-in, the device probe order
is the device registration order. As we need the transmitter to be probed
first, it needs to be registered first.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH 27/57] fbdev: sh_mobile_lcdc: Pass a video mode to the notify callback
From: Laurent Pinchart @ 2011-12-16 10:10 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1323784972-24205-28-git-send-email-laurent.pinchart@ideasonboard.com>
Hi Guennadi,
On Thursday 15 December 2011 20:01:54 Guennadi Liakhovetski wrote:
> On Tue, 13 Dec 2011, Laurent Pinchart wrote:
> > Pass pointers to struct fb_videomode and struct fb_monspecs instead of
> > struct fb_var_screeninfo to the notify callback.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >
> > drivers/video/sh_mobile_hdmi.c | 59
> > +++++++++++++++++-------------------- drivers/video/sh_mobile_lcdcfb.c
> > | 40 ++++++++++++++----------- drivers/video/sh_mobile_lcdcfb.h |
> > 3 +-
> > 3 files changed, 51 insertions(+), 51 deletions(-)
>
> [snip]
>
> > diff --git a/drivers/video/sh_mobile_lcdcfb.c
> > b/drivers/video/sh_mobile_lcdcfb.c index 21e5f10..4ec216e 100644
> > --- a/drivers/video/sh_mobile_lcdcfb.c
> > +++ b/drivers/video/sh_mobile_lcdcfb.c
>
> [ditto]
>
> > @@ -433,12 +432,17 @@ static int sh_mobile_lcdc_display_notify(struct
> > sh_mobile_lcdc_chan *ch,
> >
> > }
> > break;
> >
> > - case SH_MOBILE_LCDC_EVENT_DISPLAY_MODE:
> > + case SH_MOBILE_LCDC_EVENT_DISPLAY_MODE: {
> > + struct fb_var_screeninfo var;
> > +
> >
> > /* Validate a proposed new mode */
> >
> > - var->bits_per_pixel = info->var.bits_per_pixel;
> > - ret = info->fbops->fb_check_var(var, info);
> > + fb_videomode_to_var(&var, mode);
> > + var.bits_per_pixel = info->var.bits_per_pixel;
> > + var.grayscale = info->var.grayscale;
> > + ret = info->fbops->fb_check_var(&var, info);
> >
> > break;
> >
> > }
> >
> > + }
>
> nitpick - please, realign:-)
It's common in driver code not to increase the identation level twice in a
case statement if braces are needed. However, those "braced" statements are
usually not the last ones in the switch. In this particular case this creates
an alignment issue at the end, as your correctly pointed out. However, I'd
like to avoid increasing the indentation of the whole block. Increasing the
indentation of the closing brace only also causes an alignment issue. I can
add a default case that returns an error to fix this, but that's adding code
to fix a cosmetic problem :-) What's your preference.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH 34/57] fbdev: sh_mobile_lcdc: Store the format in struct sh_mobile_lcdc_chan
From: Laurent Pinchart @ 2011-12-16 10:17 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1323784972-24205-35-git-send-email-laurent.pinchart@ideasonboard.com>
Hi Guennadi,
On Thursday 15 December 2011 23:29:29 Guennadi Liakhovetski wrote:
> On Tue, 13 Dec 2011, Laurent Pinchart wrote:
> > Store the active format in the channel structure, and use it instead of
> > parsing info->var all over the place when the format is needed.
>
> Right, this is what I was wondering about, while looking at the previous
> patch:-)
So I'll consider your comment to patch 33/57 to be addressed :-)
> But:
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >
> > drivers/video/sh_mobile_lcdcfb.c | 21 ++++++++++-----------
> > drivers/video/sh_mobile_lcdcfb.h | 4 +++-
> > 2 files changed, 13 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/video/sh_mobile_lcdcfb.c
> > b/drivers/video/sh_mobile_lcdcfb.c index c6b6b9d..9829e01 100644
> > --- a/drivers/video/sh_mobile_lcdcfb.c
> > +++ b/drivers/video/sh_mobile_lcdcfb.c
>
> [snip]
>
> > @@ -1350,6 +1345,8 @@ static int sh_mobile_set_par(struct fb_info *info)
> >
> > info->fix.line_length = info->var.xres
> >
> > * info->var.bits_per_pixel / 8;
> >
> > + ch->format > > sh_mobile_format_info(sh_mobile_format_fourcc(&info->var));
>
> Cannot this be NULL? As far as I could trace it back, I'm not sure with
> hotplug bits_per_pixel would be initialised correctly along the lines of
> sh_mobile_fb_reconfig().
You're right, sh_mobile_fb_reconfig() doesn't set the bits_per_pixel (and
other format-related) field. I'll fix this by initializing those fields from
the current var in sh_mobile_fb_reconfig() in patch 30/57, as that's the one
that introduces the problem.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH 25/57] sh_mobile_lcdc: Add display notify callback to sh_mobile_lcdc_chan
From: Laurent Pinchart @ 2011-12-16 10:22 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1323784972-24205-26-git-send-email-laurent.pinchart@ideasonboard.com>
Hi Guennadi,
On Friday 16 December 2011 10:40:48 Guennadi Liakhovetski wrote:
> On Tue, 13 Dec 2011, Laurent Pinchart wrote:
> > The callback implements 3 notification events:
> >
> > - SH_MOBILE_LCDC_EVENT_DISPLAY_CONNECT notifies the LCDC that the
> > display has been connected
> >
> > - SH_MOBILE_LCDC_EVENT_DISPLAY_DISCONNECT notifies the LCDC that the
> > display has been disconnected
> >
> > - SH_MOBILE_LCDC_EVENT_DISPLAY_MODE notifies that LCDC that a display
> > mode has been detected
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > diff --git a/drivers/video/sh_mobile_lcdcfb.c
> > b/drivers/video/sh_mobile_lcdcfb.c index 4b03aa5..21e5f10 100644
> > --- a/drivers/video/sh_mobile_lcdcfb.c
> > +++ b/drivers/video/sh_mobile_lcdcfb.c
> > @@ -363,6 +363,86 @@ static void sh_mobile_lcdc_display_off(struct
[snip]
> > +static int sh_mobile_lcdc_display_notify(struct sh_mobile_lcdc_chan *ch,
> > + enum sh_mobile_lcdc_entity_event event,
> > + struct fb_var_screeninfo *var)
> > +{
> > + struct fb_info *info = ch->info;
> > + int ret = 0;
> > +
> > + switch (event) {
> > + case SH_MOBILE_LCDC_EVENT_DISPLAY_CONNECT:
> > + /* HDMI plug in */
> > + if (lock_fb_info(info)) {
> > + console_lock();
> > +
> > + if (!sh_mobile_lcdc_must_reconfigure(ch, var) &&
> > + info->state = FBINFO_STATE_RUNNING) {
> > + /* First activation with the default monitor.
> > + * Just turn on, if we run a resume here, the
> > + * logo disappears.
> > + */
> > + info->var.width = var->width;
> > + info->var.height = var->height;
> > + sh_mobile_lcdc_display_on(ch);
> > + } else {
> > + /* New monitor or have to wake up */
> > + fb_set_suspend(info, 0);
> > + }
> > +
> > + console_unlock();
> > + unlock_fb_info(info);
> > + }
> > + break;
> > +
> > + case SH_MOBILE_LCDC_EVENT_DISPLAY_DISCONNECT:
> > + /* HDMI disconnect */
> > + if (lock_fb_info(info)) {
> > + console_lock();
> > + fb_set_suspend(info, 1);
> > + console_unlock();
> > + unlock_fb_info(info);
> > + }
> > + break;
> > +
> > + case SH_MOBILE_LCDC_EVENT_DISPLAY_MODE:
> > + /* Validate a proposed new mode */
> > + var->bits_per_pixel = info->var.bits_per_pixel;
> > + ret = info->fbops->fb_check_var(var, info);
>
> You can call sh_mobile_lcdc_check_var() directly here too, right?
This was a way to avoid a forward declaration. You would prefer a forward
declaration, right ? :-) I think you would be right.
> > + break;
> > + }
> > +
> > + return ret;
> > +}
> > +
--
Regards,
Laurent Pinchart
^ 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