[PATCH] [V3] Xilinx : Framebuffer Driver: Add PLB support and cleanup DCR
Grant Likely
grant.likely at secretlab.ca
Thu Apr 16 01:14:41 EST 2009
On Tue, Apr 14, 2009 at 2:08 PM, John Linn <john.linn at xilinx.com> wrote:
> Added support for the new xps tft controller. The new core
> has PLB interface support in addition to existing DCR interface.
Good looking patch. A few more comments below.
g.
> /*
> * Xilinx calls it "PLB TFT LCD Controller" though it can also be used for
> - * the VGA port on the Xilinx ML40x board. This is a hardware display controller
> - * for a 640x480 resolution TFT or VGA screen.
> + * the VGA port on the Xilinx ML40x board. This is a hardware display
> + * controller for a 640x480 resolution TFT or VGA screen.
This isn't necessarily true. This driver will handle controllers
using different resolutions (I know, I know, this is just cleaning up
an existing comment, but no need to leave inaccuracies in there) :-)
> -#define xilinx_fb_out_be32(driverdata, offset, val) \
> - out_be32(driverdata->regs + offset, val)
> +static void xilinx_fb_out_be32(struct xilinxfb_drvdata *drvdata, u32 offset,
> + u32 val)
> +{
> + if (drvdata->flags & PLB_ACCESS_FLAG)
> + out_be32(drvdata->regs + (offset << 2), val);
> + else
> + dcr_write(drvdata->dcr_host, offset, val);
> +
This could more simply be:
if (drvdata->regs)
out_be32(drvdata->regs + (offset << 2), val);
else
dcr_write(drvdata->dcr_host, offset, val);
regs will be NULL if DCR is active.
> -static int xilinxfb_assign(struct device *dev, unsigned long physaddr,
> +static int xilinxfb_assign(struct device *dev,
> + struct xilinxfb_drvdata *drvdata,
> + unsigned long physaddr,
> struct xilinxfb_platform_data *pdata)
> {
> - struct xilinxfb_drvdata *drvdata;
> int rc;
> int fbsize = pdata->xvirt * pdata->yvirt * BYTES_PER_PIXEL;
>
> - /* Allocate the driver data region */
> - drvdata = kzalloc(sizeof(*drvdata), GFP_KERNEL);
> - if (!drvdata) {
> - dev_err(dev, "Couldn't allocate device private record\n");
> - return -ENOMEM;
> - }
> - dev_set_drvdata(dev, drvdata);
> -
> - /* Map the control registers in */
> - if (!request_mem_region(physaddr, 8, DRIVER_NAME)) {
> - dev_err(dev, "Couldn't lock memory region at 0x%08lX\n",
> - physaddr);
> - rc = -ENODEV;
> - goto err_region;
> - }
> - drvdata->regs_phys = physaddr;
> - drvdata->regs = ioremap(physaddr, 8);
> - if (!drvdata->regs) {
> - dev_err(dev, "Couldn't lock memory region at 0x%08lX\n",
> - physaddr);
> - rc = -ENODEV;
> - goto err_map;
> + if (drvdata->flags & PLB_ACCESS_FLAG) {
Again, I think using "if (physaddr) {" would be simpler and make more
sense and would eliminate even needing a flags data member.
> + /*
> + * Map the control registers in if the controller
> + * is on direct PLB interface.
> + */
> + if (!request_mem_region(physaddr, 8, DRIVER_NAME)) {
> + dev_err(dev, "Couldn't lock memory region at 0x%08lX\n",
> + physaddr);
> + rc = -ENODEV;
> + goto err_region;
> + }
> +
> + drvdata->regs_phys = physaddr;
> + drvdata->regs = ioremap(physaddr, 8);
> + if (!drvdata->regs) {
> + dev_err(dev, "Couldn't lock memory region at 0x%08lX\n",
> + physaddr);
> + rc = -ENODEV;
> + goto err_map;
> + }
> }
>
> /* Allocate the framebuffer memory */
> @@ -247,7 +267,10 @@ static int xilinxfb_assign(struct device *dev, unsigned long physaddr,
> if (!drvdata->fb_virt) {
> dev_err(dev, "Could not allocate frame buffer memory\n");
> rc = -ENOMEM;
> - goto err_fbmem;
> + if (drvdata->flags & PLB_ACCESS_FLAG)
> + goto err_fbmem;
> + else
> + goto err_region;
This change doesn't make much sense because the same condition is
tested for again at the err_fbmem: label. To avoid confusion later
on, keep the unwind path simple (don't jump to different labels).
> }
>
> /* Clear (turn to black) the framebuffer */
> @@ -260,7 +283,8 @@ static int xilinxfb_assign(struct device *dev, unsigned long physaddr,
> drvdata->reg_ctrl_default = REG_CTRL_ENABLE;
> if (pdata->rotate_screen)
> drvdata->reg_ctrl_default |= REG_CTRL_ROTATE;
> - xilinx_fb_out_be32(drvdata, REG_CTRL, drvdata->reg_ctrl_default);
> + xilinx_fb_out_be32(drvdata, REG_CTRL,
> + drvdata->reg_ctrl_default);
>
> /* Fill struct fb_info */
> drvdata->info.device = dev;
> @@ -296,11 +320,14 @@ static int xilinxfb_assign(struct device *dev, unsigned long physaddr,
> goto err_regfb;
> }
>
> + if (drvdata->flags & PLB_ACCESS_FLAG) {
if (drvdata->regs) {
> + /* Put a banner in the log (for DEBUG) */
> + dev_dbg(dev, "regs: phys=%lx, virt=%p\n", physaddr,
> + drvdata->regs);
> + }
> /* Put a banner in the log (for DEBUG) */
> - dev_dbg(dev, "regs: phys=%lx, virt=%p\n", physaddr, drvdata->regs);
> - dev_dbg(dev, "fb: phys=%llx, virt=%p, size=%x\n",
> - (unsigned long long) drvdata->fb_phys, drvdata->fb_virt,
> - fbsize);
> + dev_dbg(dev, "fb: phys=%p, virt=%p, size=%x\n",
> + (void *)drvdata->fb_phys, drvdata->fb_virt, fbsize);
>
> return 0; /* success */
>
> @@ -311,14 +338,19 @@ err_cmap:
> if (drvdata->fb_alloced)
> dma_free_coherent(dev, PAGE_ALIGN(fbsize), drvdata->fb_virt,
> drvdata->fb_phys);
> + else
> + iounmap(drvdata->fb_virt);
> +
> /* Turn off the display */
> xilinx_fb_out_be32(drvdata, REG_CTRL, 0);
>
> err_fbmem:
> - iounmap(drvdata->regs);
> + if (drvdata->flags & PLB_ACCESS_FLAG)
> + iounmap(drvdata->regs);
if (drvdata->regs)
>
> err_map:
> - release_mem_region(physaddr, 8);
> + if (drvdata->flags & PLB_ACCESS_FLAG)
> + release_mem_region(physaddr, 8);
if (physaddr)
>
> err_region:
> kfree(drvdata);
> @@ -342,12 +374,18 @@ static int xilinxfb_release(struct device *dev)
> if (drvdata->fb_alloced)
> dma_free_coherent(dev, PAGE_ALIGN(drvdata->info.fix.smem_len),
> drvdata->fb_virt, drvdata->fb_phys);
> + else
> + iounmap(drvdata->fb_virt);
>
> /* Turn off the display */
> xilinx_fb_out_be32(drvdata, REG_CTRL, 0);
> - iounmap(drvdata->regs);
>
> - release_mem_region(drvdata->regs_phys, 8);
> + /* Release the resources, as allocated based on interface */
> + if (drvdata->flags & PLB_ACCESS_FLAG) {
> + iounmap(drvdata->regs);
> + release_mem_region(drvdata->regs_phys, 8);
> + } else
> + dcr_unmap(drvdata->dcr_host, drvdata->dcr_len);
et cetera
> -#if defined(CONFIG_OF)
> static int __devinit
> xilinxfb_of_probe(struct of_device *op, const struct of_device_id *match)
> {
> - struct resource res;
> const u32 *prop;
> + u32 *p;
> + u32 tft_access;
> struct xilinxfb_platform_data pdata;
> + struct resource res;
> int size, rc;
> + int start = 0, len = 0;
> + dcr_host_t dcr_host;
> + struct xilinxfb_drvdata *drvdata;
>
> /* Copy with the default pdata (not a ptr reference!) */
> pdata = xilinx_fb_default_pdata;
>
> dev_dbg(&op->dev, "xilinxfb_of_probe(%p, %p)\n", op, match);
>
> - rc = of_address_to_resource(op->node, 0, &res);
> - if (rc) {
> - dev_err(&op->dev, "invalid address\n");
> - return rc;
> + /*
> + * To check whether the core is connected directly to DCR or PLB
> + * interface and initialize the tft_access accordingly.
> + */
> + p = (u32 *)of_get_property(op->node, "xlnx,dcr-splb-slave-if", NULL);
Hmmm. This binding is undocumented. It would be better to make the
decision on the presence/absence of the dcr-reg and/or reg properties.
> +
> + if (p)
> + tft_access = *p;
> + else
> + tft_access = 0; /* For backward compatibility */
Common pattern: tft_access = p ? *p : 0;
> +
> + /*
> + * Fill the resource structure if its direct PLB interface
> + * otherwise fill the dcr_host structure.
> + */
> + if (tft_access) {
> + rc = of_address_to_resource(op->node, 0, &res);
> + if (rc) {
> + dev_err(&op->dev, "invalid address\n");
> + return -ENODEV;
> + }
> +
> + } else {
> + start = dcr_resource_start(op->node, 0);
> + len = dcr_resource_len(op->node, 0);
> + dcr_host = dcr_map(op->node, start, len);
> + if (!DCR_MAP_OK(dcr_host)) {
> + dev_err(&op->dev, "invalid address\n");
> + return -ENODEV;
> + }
> }
>
> prop = of_get_property(op->node, "phys-size", &size);
> @@ -450,7 +468,26 @@ xilinxfb_of_probe(struct of_device *op, const struct of_device_id *match)
> if (of_find_property(op->node, "rotate-display", NULL))
> pdata.rotate_screen = 1;
>
> - return xilinxfb_assign(&op->dev, res.start, &pdata);
> + /* Allocate the driver data region */
> + drvdata = kzalloc(sizeof(*drvdata), GFP_KERNEL);
> + if (!drvdata) {
> + dev_err(&op->dev, "Couldn't allocate device private record\n");
> + return -ENOMEM;
> + }
> + dev_set_drvdata(&op->dev, drvdata);
> +
> + if (tft_access)
> + drvdata->flags |= PLB_ACCESS_FLAG;
> +
> + /* Arguments are passed based on the interface */
> + if (drvdata->flags & PLB_ACCESS_FLAG) {
> + return xilinxfb_assign(&op->dev, drvdata, res.start, &pdata);
> + } else {
> + drvdata->dcr_start = start;
> + drvdata->dcr_len = len;
> + drvdata->dcr_host = dcr_host;
> + return xilinxfb_assign(&op->dev, drvdata, 0, &pdata);
> + }
> }
>
> static int __devexit xilinxfb_of_remove(struct of_device *op)
> @@ -460,7 +497,9 @@ static int __devexit xilinxfb_of_remove(struct of_device *op)
>
> /* Match table for of_platform binding */
> static struct of_device_id xilinxfb_of_match[] __devinitdata = {
> + { .compatible = "xlnx,xps-tft-1.00.a", },
> { .compatible = "xlnx,plb-tft-cntlr-ref-1.00.a", },
> + { .compatible = "xlnx,plb-dvi-cntlr-ref-1.00.c", },
> {},
> };
> MODULE_DEVICE_TABLE(of, xilinxfb_of_match);
> @@ -476,22 +515,6 @@ static struct of_platform_driver xilinxfb_of_driver = {
> },
> };
>
> -/* Registration helpers to keep the number of #ifdefs to a minimum */
> -static inline int __init xilinxfb_of_register(void)
> -{
> - pr_debug("xilinxfb: calling of_register_platform_driver()\n");
> - return of_register_platform_driver(&xilinxfb_of_driver);
> -}
> -
> -static inline void __exit xilinxfb_of_unregister(void)
> -{
> - of_unregister_platform_driver(&xilinxfb_of_driver);
> -}
> -#else /* CONFIG_OF */
> -/* CONFIG_OF not enabled; do nothing helpers */
> -static inline int __init xilinxfb_of_register(void) { return 0; }
> -static inline void __exit xilinxfb_of_unregister(void) { }
> -#endif /* CONFIG_OF */
>
> /* ---------------------------------------------------------------------
> * Module setup and teardown
> @@ -500,28 +523,18 @@ static inline void __exit xilinxfb_of_unregister(void) { }
> static int __init
> xilinxfb_init(void)
> {
> - int rc;
> - rc = xilinxfb_of_register();
> - if (rc)
> - return rc;
> -
> - rc = platform_driver_register(&xilinxfb_platform_driver);
> - if (rc)
> - xilinxfb_of_unregister();
> -
> - return rc;
> + return of_register_platform_driver(&xilinxfb_of_driver);
> }
>
> static void __exit
> xilinxfb_cleanup(void)
> {
> - platform_driver_unregister(&xilinxfb_platform_driver);
> - xilinxfb_of_unregister();
> + of_unregister_platform_driver(&xilinxfb_of_driver);
> }
>
> module_init(xilinxfb_init);
> module_exit(xilinxfb_cleanup);
>
> MODULE_AUTHOR("MontaVista Software, Inc. <source at mvista.com>");
> -MODULE_DESCRIPTION(DRIVER_DESCRIPTION);
> +MODULE_DESCRIPTION("Xilinx TFT frame buffer driver");
> MODULE_LICENSE("GPL");
> --
> 1.6.2.1
>
>
>
> This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.
>
>
>
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
More information about the Linuxppc-dev
mailing list