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

[Andy Shevchenko]

On Sat, Feb 11, 2017 at 8:48 PM, Noralf TrÃnnes <noralf@xxxxxxxxxxx> 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