On Wed, Jan 23, 2013 at 3:42 AM, Jean-Francois Moine <moinejf@xxxxxxx> wrote:
> 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.
Just fyi, you can re-use the nxp-tda998x part independently of tilcdc
(just in case that wasn't clear).
It may be that we need to add some configuration info struct, if for
example there are some differences in video signal mux on the dove
board, but that shouldn't be a big deal. I figure we can see what is
needed as others start to use it and add as needed.
> 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).
drv and crtc are the "core" driver... arguably the tfp410 part could
be optional. I'd prefer *not* as a separate module, as this implies
some independence of module lifetime, which is not true. I generally
prefer to start simple and add complexity later if someone really
thinks they need it, which is why I avoided adding a bunch of kconfig
options to start with.
> [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.
hmm, I don't think it was but I'll have to double check that. At
least if it was it wasn't seeming to cause anything bad, because I did
have "tilcdc,tfp410" in DT..
BR,
-R
>> + 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/
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
