Re: [PATCH 4/4] fb: vt8500: Add VGA output support to wm8505fb driver.

[Tony Prisk]

On 19/05/13 01:28, Alexey Charkov wrote:
> 2013/5/18 Tony Prisk <linux@xxxxxxxxxxxxxxx>:
> > The APC8750 does not support an LCD panel, but provides a VGA connector.
> > This patch adds support for the VGA interface, and defines an optional
> > devicetree property to specify the output interface. The default if not
> > specified is LCD for backward compatibility.
> >
> > Signed-off-by: Tony Prisk <linux@xxxxxxxxxxxxxxx>
> > ---
> >   .../devicetree/bindings/video/wm,wm8505-fb.txt     |    5 ++++
> >   drivers/video/wm8505fb.c                           |   31 ++++++++++++++++++--
> >   2 files changed, 34 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/video/wm,wm8505-fb.txt b/Documentation/devicetree/bindings/video/wm,wm8505-fb.txt
> > index 601416c..9f1d648 100644
> > --- a/Documentation/devicetree/bindings/video/wm,wm8505-fb.txt
> > +++ b/Documentation/devicetree/bindings/video/wm,wm8505-fb.txt
> > @@ -7,6 +7,10 @@ Required properties:
> >   - bits-per-pixel : bit depth of framebuffer (16 or 32)
> >   - clocks : phandle to DVO clock
> >
> > +Optional properties:
> > +- output-interface : the interface the fb should output on. Valid values are
> > +       "lcd" or "vga". If not specified, the default is "lcd".
> > +
> >   Required subnodes:
> >   - display-timings: see display-timing.txt for information
> >
> > @@ -17,6 +21,7 @@ Example:
> >                  reg = <0xd8051700 0x200>;
> >                  bits-per-pixel = <16>;
> >                  clocks = <&clkdvo>;
> > +               output-interface = "vga";
> >
> >                  display-timings {
> >                          native-mode = <&timing0>;
> > diff --git a/drivers/video/wm8505fb.c b/drivers/video/wm8505fb.c
> > index f8bffc2..d1f7f33 100644
> > --- a/drivers/video/wm8505fb.c
> > +++ b/drivers/video/wm8505fb.c
> > @@ -130,12 +130,17 @@
> >
> >   #define to_wm8505fb_info(__info) container_of(__info, \
> >                                                  struct wm8505fb_info, fb)
> > +
> > +#define INTERFACE_LCD  1
> > +#define INTERFACE_VGA  2
> > +
> >   struct wm8505fb_info {
> >          struct fb_info fb;
> >          void __iomem *regbase;
> >          unsigned int contrast;
> >          struct device *dev;
> >          struct clk *clk_dvo;
> > +       int interface;
> >   };
> >
> >
> > @@ -158,7 +163,11 @@ static int wm8505fb_init_hw(struct fb_info *info)
> >           * 0x31C sets the correct color mode (RGB565) for WM8650
> >           * Bit 8+9 (0x300) are ignored on WM8505 as reserved
> >           */
> > -       writel(0x31c,                  fbi->regbase + REG_GOVRH_YUVRGB);
> > +       if (fbi->interface == INTERFACE_VGA)
> > +               writel(0x338, fbi->regbase + REG_GOVRH_YUVRGB);
> > +       else
> > +               writel(0x31c, fbi->regbase + REG_GOVRH_YUVRGB);
> > +
> >          writel(1,                      fbi->regbase + REG_GOVRH_DVO_PIX);
> Tony,
>
> Would it be possible to also define known bit offsets for those
> registers, while you are at this? It would probably reduce the black
> magic quite a bit :)
On my list of things to do :)
> >          /* Virtual buffer size */
> > @@ -167,7 +176,12 @@ static int wm8505fb_init_hw(struct fb_info *info)
> >
> >          /* black magic ;) */
> >          writel(0xf,                    fbi->regbase + REG_GOVRH_FHI);
> > -       writel(4,                      fbi->regbase + REG_GOVRH_DVO_SET);
> > +
> > +       if (fbi->interface == INTERFACE_VGA)
> > +               writel(0xe, fbi->regbase + REG_GOVRH_DVO_SET);
> > +       else
> > +               writel(4, fbi->regbase + REG_GOVRH_DVO_SET);
> I don't remember if HDMI is yet another option for this register or
> not... If it is, it would probably warrant defining fbi->interface as
> an enum and changing this if-else into a switch statement to let the
> compiler add its checks/warnings.
This register defines the h/v syncpolarity and enable/disable for DVO.
> >          writel(1,                      fbi->regbase + REG_GOVRH_MIF);
> >          writel(1,                      fbi->regbase + REG_GOVRH_REG_STS);
> >
> > @@ -194,11 +208,15 @@ static int wm8505fb_set_timing(struct fb_info *info)
> >          writel(h_end,   fbi->regbase + REG_GOVRH_ACTPX_END);
> >          writel(h_all,   fbi->regbase + REG_GOVRH_H_ALLPXL);
> >          writel(h_sync,  fbi->regbase + REG_GOVRH_HDMI_HSYNW);
> > +       if (fbi->interface == INTERFACE_VGA)
> > +               writel(h_sync,  fbi->regbase + REG_GOVRH_VGA_HSYNW);
> Will it misbehave on LCD if you write to the VGA register unconditionally?
Don't know - wouldn't imagine so. I will test it and see.
> >          writel(v_start, fbi->regbase + REG_GOVRH_ACTLN_BG);
> >          writel(v_end,   fbi->regbase + REG_GOVRH_ACTLN_END);
> >          writel(v_all,   fbi->regbase + REG_GOVRH_V_ALLLN);
> >          writel(v_sync,  fbi->regbase + REG_GOVRH_HDMI_VBISW);
> > +       if (fbi->interface == INTERFACE_VGA)
> > +               writel(info->var.pixclock,  fbi->regbase + REG_GOVRH_VGA_VSYNW);
> Same here. I would assume that setting the pixclock should not hurt
> LCD, which would then simplify the code a little.
>
> Thanks,
> Alexey
Regards
Tony Prisk
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/