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

[Noralf TrÃnnes]

Den 12.02.2017 12.05, skrev 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?

That means 81 columns and checkpatch complaint.
This in one of the annoying things about Linux
kernel coding. Always trying
to avoid
breaking up lines.
Because for me it hinders redability.

> > +const struct file_operations tinydrm_fops = {
> > +       .owner          = THIS_MODULE,
> Do we still need this?

Looks like it, see drm_stub_open()

> > +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?

yes, that last sentence can be dropped.

> > +        * 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?

A comment might have helped here:
/* fbdev is of minor importance, don't let it stop us */

> > +
> > +       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_* ?

sphinx wants it that way.

> > + * @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_... ?

Yep.


tinydrm will be merged the way it is now, unless someone points to
something that is broken. But I collect your comments for a later
cleanup patchset. It takes a lot of effort for me as an amateur to
keeps this code up-to-date out-of-tree for months. It's not even
sure that I've hit the mark with this, so there will most likely be
changes when I start converting fbtft drivers, and I'll implement the
remaining bits and pieces as I make changes. The core of tinydrm won't
change because of unforseen fbtft quirks, but other parts might.


Noralf.

> > +}
> > +EXPORT_SYMBOL(tinydrm_display_pipe_init);