Den 23.03.2016 18:37, skrev David Herrmann:
> Hey
>
> On Wed, Mar 16, 2016 at 2:34 PM, Noralf Trønnes <noralf at tronnes.org>
> wrote:
>> tinydrm provides a very simplified view of DRM for displays that has
>> onboard video memory and is connected through a slow bus like SPI/I2C.
>>
>> Signed-off-by: Noralf Trønnes <noralf at tronnes.org>
>> ---
[...]
>> +static struct drm_driver tinydrm_driver = {
>> + .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME
>> + | DRIVER_ATOMIC,
>> + .load = tinydrm_load,
>> + .lastclose = tinydrm_lastclose,
>> +// .unload = tinydrm_unload,
>> + .get_vblank_counter = drm_vblank_count,
>> +// .enable_vblank = tinydrm_enable_vblank,
>> +// .disable_vblank = tinydrm_disable_vblank,
>> + .gem_free_object = drm_gem_cma_free_object,
>> + .gem_vm_ops = &drm_gem_cma_vm_ops,
>> + .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>> + .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
>> + .gem_prime_import = drm_gem_prime_import,
>> + .gem_prime_export = drm_gem_prime_export,
>> + .gem_prime_get_sg_table = drm_gem_cma_prime_get_sg_table,
>> + .gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table,
>> + .gem_prime_vmap = drm_gem_cma_prime_vmap,
>> + .gem_prime_vunmap = drm_gem_cma_prime_vunmap,
>> + .gem_prime_mmap = drm_gem_cma_prime_mmap,
>> + .dumb_create = drm_gem_cma_dumb_create,
>> + .dumb_map_offset = drm_gem_cma_dumb_map_offset,
>> + .dumb_destroy = drm_gem_dumb_destroy,
>> + .fops = &tinydrm_fops,
>> + .name = "tinydrm",
>> + .desc = "tinydrm",
>> + .date = "20150916",
> Can we just drop "date" and "desc" from new drivers? It doesn't add any value.
>
>> + .major = 1,
>> + .minor = 0,
>> +};
>> +
>> +void tinydrm_release(struct tinydrm_device *tdev)
> We usually prefer "unregister()" to stay consistent with "register()".
Sure.
>> +{
>> + DRM_DEBUG_KMS("\n");
>> +
>> + if (tdev->deferred)
>> + cancel_delayed_work_sync(&tdev->deferred->dwork);
>> +
>> + tinydrm_fbdev_fini(tdev);
>> +
>> + drm_panel_detach(&tdev->panel);
>> + drm_panel_remove(&tdev->panel);
>> +
>> + drm_mode_config_cleanup(tdev->base);
>> + drm_dev_unregister(tdev->base);
>> + drm_dev_unref(tdev->base);
>> +}
>> +EXPORT_SYMBOL(tinydrm_release);
>> +
>> +int tinydrm_register(struct device *dev, struct tinydrm_device *tdev)
>> +{
>> + struct drm_driver *driver = &tinydrm_driver;
>> + struct drm_device *ddev;
>> + int ret;
>> +
>> + dev_info(dev, "%s\n", __func__);
>> +
>> +dev->coherent_dma_mask = DMA_BIT_MASK(32);
>> +
>> + if (WARN_ON(!tdev->dirtyfb))
>> + return -EINVAL;
>> +
>> + ddev = drm_dev_alloc(driver, dev);
>> + if (!ddev)
>> + return -ENOMEM;
>> +
>> + tdev->base = ddev;
>> + ddev->dev_private = tdev;
>> +
>> + ret = drm_dev_set_unique(ddev, dev_name(ddev->dev));
>> + if (ret)
>> + goto err_free;
>> +
>> + ret = drm_dev_register(ddev, 0);
>> + if (ret)
>> + goto err_free;
> Whatever your .load() callback does, do that here and drop it. It is
> really not needed. Optionally do it before calling drm_dev_register(),
> depending on which semantics you want.
Ok.
> In general, this looks very similar to what I did with SimpleDRM.
SimpleDRM was the first drm code I studied and tinydrm started with chunks
of code from it :-)
> However, I wonder whether we can make it more modular. Right now it
> always adds code for fbdev, CMA, backlight, etc., but as Daniel
> mentioned those better live in DRM-core helpers.
Yes I will make the next version more modular.
Instead of trying to guess which parts would fit as core helpers, I just
put everything into one module and posted an RFC hoping to get some
feedback.
I will try an implement the suggestions Daniel has made.
> I'll try forward porting the SimpleDRM drivers to it, and let you know
> whether it works out.
>
> Thanks
> David
