Re: [PATCH v4 1/7] drm: Add DRM support for tiny LCD displays

Andy Shevchenko
2017-02-12T11:05:06Z

On Sat, Feb 11, 2017 at 8:48 PM, Noralf Trønnes <noralf@tronnes.org> wrote:
> tinydrm provides helpers for very simple displays that can use
> CMA backed framebuffers and need flushing on changes.

> +/**

> + * tinydrm_gem_cma_free_object - Free resources associated with a CMA GEM
> + * object

Keep on one line?

> +const struct file_operations tinydrm_fops = {

> + .owner = THIS_MODULE,

Do we still need this?

> +static int tinydrm_init(struct device *parent, struct tinydrm_device *tdev,
> + const struct drm_framebuffer_funcs *fb_funcs,
> + struct drm_driver *driver)
> +{
> + struct drm_device *drm;
> +
> + mutex_init(&tdev->dirty_lock);
> + tdev->fb_funcs = fb_funcs;
> +
> + /*
> + * We don't embed drm_device, because that prevent us from using
> + * devm_kzalloc() to allocate tinydrm_device in the driver since

> + * drm_dev_unref() frees the structure. The devm_ functions provide

"devm_ functions" -> "devm_kzalloc()" ?

Otherwise what else do you refer to and why here?

> + * for easy error handling.
> + */

> +static int tinydrm_register(struct tinydrm_device *tdev)
> +{
> + struct drm_device *drm = tdev->drm;
> + int bpp = drm->mode_config.preferred_depth;
> + struct drm_fbdev_cma *fbdev;
> + int ret;
> +
> + ret = drm_dev_register(tdev->drm, 0);
> + if (ret)
> + return ret;
> +
> + fbdev = drm_fbdev_cma_init_with_funcs(drm, bpp ? bpp : 32,
> + drm->mode_config.num_connector,
> + tdev->fb_funcs);

> + if (IS_ERR(fbdev))
> + DRM_ERROR("Failed to initialize fbdev: %ld\n", PTR_ERR(fbdev));
> + else
> + tdev->fbdev_cma = fbdev;

Apparently I missed previous round of reviews, can you just put in
short words why error is not propagated here to the caller?

> +
> + return 0;
> +}

> +/**
> + * tinydrm_display_pipe_init - Initialize display pipe
> + * @tdev: tinydrm device
> + * @funcs: Display pipe functions
> + * @connector_type: Connector type

> + * @formats: Array of supported formats (DRM_FORMAT\_\*)

DRM_FORMAT_* ?

> + * @format_count: Number of elements in @formats
> + * @mode: Supported mode
> + * @rotation: Initial @mode rotation in degrees Counter Clock Wise
> + *
> + * This function sets up a &drm_simple_display_pipe with a &drm_connector that
> + * has one fixed &drm_display_mode which is rotated according to @rotation.
> + *
> + * Returns:
> + * Zero on success, negative error code on failure.
> + */

> +{
> + struct drm_device *drm = tdev->drm;
> + struct drm_display_mode *mode_copy;
> + struct drm_connector *connector;
> + int ret;

> + connector = tinydrm_connector_create(drm, mode_copy, connector_type);
> + if (IS_ERR(connector))
> + return PTR_ERR(connector);
> +

> + ret = drm_simple_display_pipe_init(drm, &tdev->pipe, funcs, formats,
> + format_count, connector);
> + if (ret)
> + return ret;
> +
> + return 0;

return drm_... ?

> +}
> +EXPORT_SYMBOL(tinydrm_display_pipe_init);

--
With Best Regards,
Andy Shevchenko