On Tue, Apr 14, 2009 at 2:08 PM, John Linn <[email protected]> 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. <[email protected]>");
> -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.
_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://ozlabs.org/mailman/listinfo/linuxppc-dev
