From: "Noralf Trønnes" <noralf@tronnes.org> To: Andy Shevchenko <andy.shevchenko@gmail.com> Cc: dri-devel@lists.freedesktop.org, devicetree <devicetree@vger.kernel.org>, robh@kernel.org, Thomas Petazzoni <thomas.petazzoni@free-electrons.com>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org> Subject: Re: [PATCH v4 1/7] drm: Add DRM support for tiny LCD displays Date: Sun, 12 Feb 2017 14:57:49 +0100 [thread overview] Message-ID: <70190e71-4d5a-7102-418f-09896b57c084@tronnes.org> (raw) In-Reply-To: <CAHp75VeZke=JCosvc3xQ_AiOn64hth=PMJp4X0vLHMS14bkp4w@mail.gmail.com> Den 12.02.2017 12.05, skrev Andy Shevchenko: > 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? 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);
WARNING: multiple messages have this Message-ID (diff)
From: "Noralf Trønnes" <noralf@tronnes.org> To: Andy Shevchenko <andy.shevchenko@gmail.com> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>, devicetree <devicetree@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, dri-devel@lists.freedesktop.org Subject: Re: [PATCH v4 1/7] drm: Add DRM support for tiny LCD displays Date: Sun, 12 Feb 2017 14:57:49 +0100 [thread overview] Message-ID: <70190e71-4d5a-7102-418f-09896b57c084@tronnes.org> (raw) In-Reply-To: <CAHp75VeZke=JCosvc3xQ_AiOn64hth=PMJp4X0vLHMS14bkp4w@mail.gmail.com> Den 12.02.2017 12.05, skrev Andy Shevchenko: > 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? 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); _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2017-02-12 13:57 UTC|newest] Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-02-11 18:48 [PATCH v4 0/7] drm: Add support for tiny LCD displays Noralf Trønnes 2017-02-11 18:48 ` Noralf Trønnes 2017-02-11 18:48 ` [PATCH v4 1/7] drm: Add DRM " Noralf Trønnes 2017-02-11 18:48 ` Noralf Trønnes 2017-02-12 11:05 ` Andy Shevchenko 2017-02-12 11:05 ` Andy Shevchenko 2017-02-12 13:57 ` Noralf Trønnes [this message] 2017-02-12 13:57 ` Noralf Trønnes 2017-02-14 19:54 ` Daniel Vetter 2017-02-14 19:54 ` Daniel Vetter 2017-03-07 22:21 ` Daniel Vetter 2017-03-07 22:21 ` Daniel Vetter 2017-03-08 12:23 ` Noralf Trønnes 2017-03-08 12:23 ` Noralf Trønnes 2017-02-11 18:48 ` [PATCH v4 2/7] drm/tinydrm: Add helper functions Noralf Trønnes 2017-02-11 18:48 ` Noralf Trønnes 2017-02-12 11:50 ` Andy Shevchenko 2017-02-12 11:50 ` Andy Shevchenko 2017-02-12 15:33 ` Noralf Trønnes 2017-02-12 15:33 ` Noralf Trønnes 2017-02-11 18:48 ` [PATCH v4 3/7] drm/tinydrm: Add MIPI DBI support Noralf Trønnes 2017-02-11 18:48 ` Noralf Trønnes 2017-03-12 18:42 ` Daniel Vetter 2017-03-12 18:42 ` Daniel Vetter 2017-03-12 19:02 ` Daniel Vetter 2017-03-12 19:02 ` Daniel Vetter 2017-02-11 18:48 ` [PATCH v4 4/7] of: Add vendor prefix for Multi-Inno Noralf Trønnes 2017-02-11 18:48 ` Noralf Trønnes 2017-02-11 18:48 ` [PATCH v4 5/7] dt-bindings: display/panel: Add common rotation property Noralf Trønnes 2017-02-11 18:48 ` Noralf Trønnes 2017-02-14 16:19 ` Rob Herring 2017-02-14 16:19 ` Rob Herring 2017-04-11 5:29 ` Laurent Pinchart 2017-04-11 5:29 ` Laurent Pinchart 2017-04-12 11:01 ` Noralf Trønnes 2017-04-12 11:01 ` Noralf Trønnes 2017-02-11 18:48 ` [PATCH v4 6/7] dt-bindings: Add Multi-Inno MI0283QT binding Noralf Trønnes 2017-02-11 18:48 ` Noralf Trønnes 2017-02-11 18:48 ` [PATCH v4 7/7] drm/tinydrm: Add support for Multi-Inno MI0283QT display Noralf Trønnes 2017-02-11 18:48 ` Noralf Trønnes
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=70190e71-4d5a-7102-418f-09896b57c084@tronnes.org \ --to=noralf@tronnes.org \ --cc=andy.shevchenko@gmail.com \ --cc=devicetree@vger.kernel.org \ --cc=dri-devel@lists.freedesktop.org \ --cc=linux-kernel@vger.kernel.org \ --cc=robh@kernel.org \ --cc=thomas.petazzoni@free-electrons.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.