Hi Rob,
As I wanted to re-use your nxp-tda998x driver for the Marvell Dove SoC,
I had a look at your IT LCD driver. Comments below.
On Tue, 22 Jan 2013 16:36:22 -0600
Rob Clark <robdclark@xxxxxxxxx> wrote:
> A simple DRM/KMS driver for the TI LCD Controller found in various
> smaller TI parts (AM33xx, OMAPL138, etc). This driver uses the
> CMA helpers. Currently only the TFP410 DVI encoder is supported
> (tested with beaglebone + DVI cape). There are also various LCD
> displays, for which support can be added (as I get hw to test on),
> and an external i2c HDMI encoder found on some boards.
>
> The display controller supports a single CRTC. And the encoder+
> connector are split out into sub-devices. Depending on which LCD
> or external encoder is actually present, the appropriate output
> module(s) will be loaded.
>
> v1: original
> v2: fix fb refcnting and few other cleanups
> v3: get +/- vsync/hsync from timings rather than panel-info, add
> option DT max-bandwidth field so driver doesn't attempt to
> pick a display mode with too high memory bandwidth, and other
> small cleanups
>
> Signed-off-by: Rob Clark <robdclark@xxxxxxxxx>
> ---
> drivers/gpu/drm/Kconfig | 2 +
> drivers/gpu/drm/Makefile | 1 +
> drivers/gpu/drm/tilcdc/Kconfig | 10 +
> drivers/gpu/drm/tilcdc/Makefile | 8 +
> drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 597 ++++++++++++++++++++++++++++++++
> drivers/gpu/drm/tilcdc/tilcdc_drv.c | 605 +++++++++++++++++++++++++++++++++
> drivers/gpu/drm/tilcdc/tilcdc_drv.h | 159 +++++++++
> drivers/gpu/drm/tilcdc/tilcdc_regs.h | 154 +++++++++
> drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 423 +++++++++++++++++++++++
> drivers/gpu/drm/tilcdc/tilcdc_tfp410.h | 26 ++
> 10 files changed, 1985 insertions(+)
> create mode 100644 drivers/gpu/drm/tilcdc/Kconfig
> create mode 100644 drivers/gpu/drm/tilcdc/Makefile
> create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_drv.c
> create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_drv.h
> create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_regs.h
> create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
> create mode 100644 drivers/gpu/drm/tilcdc/tilcdc_tfp410.h
[snip]
> diff --git a/drivers/gpu/drm/tilcdc/Kconfig b/drivers/gpu/drm/tilcdc/Kconfig
> new file mode 100644
> index 0000000..ee9b592
> --- /dev/null
> +++ b/drivers/gpu/drm/tilcdc/Kconfig
> @@ -0,0 +1,10 @@
> +config DRM_TILCDC
> + tristate "DRM Support for TI LCDC Display Controller"
> + depends on DRM && OF
> + select DRM_KMS_HELPER
> + select DRM_KMS_CMA_HELPER
> + select DRM_GEM_CMA_HELPER
> + help
> + Choose this option if you have an TI SoC with LCDC display
> + controller, for example AM33xx in beagle-bone, DA8xx, or
> + OMAP-L1xx. This driver replaces the FB_DA8XX fbdev driver.
> diff --git a/drivers/gpu/drm/tilcdc/Makefile b/drivers/gpu/drm/tilcdc/Makefile
> new file mode 100644
> index 0000000..1359cc2
> --- /dev/null
> +++ b/drivers/gpu/drm/tilcdc/Makefile
> @@ -0,0 +1,8 @@
> +ccflags-y := -Iinclude/drm -Werror
> +
> +tilcdc-y := \
> + tilcdc_crtc.o \
> + tilcdc_tfp410.o \
> + tilcdc_drv.o
As I understand, this means that the 3 objects will go into the
resident kernel.
I though that the device tree was created for Linux distributors who
might generate generic ARM kernels, the choice of the drivers being
done according the local device tree.
>From this point of vue, it would be nicer to have 2 separate modules:
tilcdc and tfp410, tilcdc_crtc being included in one of these ones
(I did not look deep enough at the code to know which).
[snip]
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> new file mode 100644
> index 0000000..cf1fddc
> --- /dev/null
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> @@ -0,0 +1,605 @@
[snip]
> +static struct drm_driver tilcdc_driver = {
> + .driver_features = DRIVER_HAVE_IRQ | DRIVER_GEM | DRIVER_MODESET,
> + .load = tilcdc_load,
> + .unload = tilcdc_unload,
> + .preclose = tilcdc_preclose,
> + .lastclose = tilcdc_lastclose,
> + .irq_handler = tilcdc_irq,
> + .irq_preinstall = tilcdc_irq_preinstall,
> + .irq_postinstall = tilcdc_irq_postinstall,
> + .irq_uninstall = tilcdc_irq_uninstall,
> + .get_vblank_counter = drm_vblank_count,
> + .enable_vblank = tilcdc_enable_vblank,
> + .disable_vblank = tilcdc_disable_vblank,
> + .gem_free_object = drm_gem_cma_free_object,
> + .gem_vm_ops = &drm_gem_cma_vm_ops,
> + .dumb_create = drm_gem_cma_dumb_create,
> + .dumb_map_offset = drm_gem_cma_dumb_map_offset,
> + .dumb_destroy = drm_gem_cma_dumb_destroy,
> +#ifdef CONFIG_DEBUG_FS
> + .debugfs_init = tilcdc_debugfs_init,
> + .debugfs_cleanup = tilcdc_debugfs_cleanup,
> +#endif
> + .fops = &fops,
> + .name = "tilcdc",
> + .desc = "TI LCD Controller DRM",
> + .date = "20121205",
> + .major = 1,
> + .minor = 0,
> +};
> +
> +/*
> + * Power management:
> + */
> +
> +#if CONFIG_PM_SLEEP
Should be:
#ifdef CONFIG_PM_SLEEP
[snip]
> +static struct of_device_id tilcdc_of_match[] = {
> + { .compatible = "ti,am33xx-tilcdc", },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, tilcdc_of_match);
> +
> +static struct platform_driver tilcdc_platform_driver = {
> + .probe = tilcdc_pdev_probe,
> + .remove = tilcdc_pdev_remove,
> + .driver = {
> + .owner = THIS_MODULE,
> + .name = "tilcdc",
> + .pm = &tilcdc_pm_ops,
> + .of_match_table = tilcdc_of_match,
> + },
> +};
> +
> +static int __init tilcdc_drm_init(void)
> +{
> + DBG("init");
> + tilcdc_tfp410_init();
This function may be called twice if "tilcdc,tfp410" is in the DT.
> + return platform_driver_register(&tilcdc_platform_driver);
> +}
> +
> +static void __exit tilcdc_drm_fini(void)
> +{
> + DBG("fini");
> + tilcdc_tfp410_fini();
> + platform_driver_unregister(&tilcdc_platform_driver);
> +}
> +
> +module_init(tilcdc_drm_init);
> +module_exit(tilcdc_drm_fini);
> +
> +MODULE_AUTHOR("Rob Clark <robdclark@xxxxxxxxx");
> +MODULE_DESCRIPTION("TI LCD Controller DRM Driver");
> +MODULE_LICENSE("GPL");
--
Ken ar c'hentañ | ** Breizh ha Linux atav! **
Jef | http://moinejf.free.fr/
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
