From: Sam Ravnborg <sam@ravnborg.org>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: David Airlie <airlied@linux.ie>,
Huacai Chen <chenhuacai@kernel.org>,
dri-devel@lists.freedesktop.org,
lichenyang <lichenyang@loongson.cn>,
Thomas Zimmermann <tzimmermann@suse.de>,
devel@linuxdriverproject.org,
Dan Carpenter <dan.carpenter@oracle.com>,
Maxime Ripard <maxime@cerno.tech>
Subject: Re: [PATCH v3 1/3] drm/loongson: Add DRM Driver for Loongson 7A1000 bridge chip
Date: Tue, 27 Jul 2021 19:41:30 +0200 [thread overview]
Message-ID: <YQBFSsecMNXKcXCy@ravnborg.org> (raw)
In-Reply-To: <YP/TZ2VRqYsw+jQN@phenom.ffwll.local>
Hi Chenyang,
I browsed the code on lore and noticed a few things and thought it
better to bring it to your attention now.
The general structure of the drivers seems good and coding style is
fine. The feedback is mostly stuff we have decided to do different over
time, so likely because you based the driver on some older driver.
And it can be hard to follow all the refactoring going on - something
that you get for free (almost) when is is mainlined.
1) Use drm_ for logging whereever possible (need drm_device)
2) Do not use irq mid-layer support in drm_driver, it is about to be
legacy only. In other words avoid using drm_irq* stuff.
3) Look at drm_drv to see code snippet how to use the drmm*
infrastructure. It will save you some cleanup and in general make the
driver more stable
4) Sort includes alphabetically, and split them on in blocks.
<linux/*> is one block
<newline>
<drm/drm_*> is next block
5) Put entry in makefile in alphabetical order
6) You most like can use the simple_encoder stuff we have today
7) The *_load and *_unlod names where used in the past. Maybe be
inspired by some newer driver here. _load functiosn is something used
by legacy drivers so it confuses me a little.
I look forward to see next revision of the patch-set.
And sorry for not providing these high-level feedback issues before - I
have not had time to look at your driver.
Sam
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
next prev parent reply other threads:[~2021-07-27 17:41 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-23 3:12 [PATCH v3 1/3] drm/loongson: Add DRM Driver for Loongson 7A1000 bridge chip lichenyang
2021-07-23 3:12 ` [PATCH v3 2/3] drm/loongson: Add GPIO and I2C driver for loongson drm lichenyang
2021-07-23 3:12 ` [PATCH v3 3/3] drm/loongson: Add interrupt driver for LS7A lichenyang
2021-07-23 7:22 ` [PATCH v3 1/3] drm/loongson: Add DRM Driver for Loongson 7A1000 bridge chip Dan Carpenter
2021-07-23 8:57 ` Daniel Vetter
2021-07-23 17:22 ` Sam Ravnborg
2021-07-27 9:35 ` Daniel Vetter
2021-07-27 17:41 ` Sam Ravnborg [this message]
2021-07-28 2:55 ` Reply: " 李晨阳
2021-07-28 6:54 ` 李晨阳
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=YQBFSsecMNXKcXCy@ravnborg.org \
--to=sam@ravnborg.org \
--cc=airlied@linux.ie \
--cc=chenhuacai@kernel.org \
--cc=dan.carpenter@oracle.com \
--cc=daniel@ffwll.ch \
--cc=devel@linuxdriverproject.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=lichenyang@loongson.cn \
--cc=maxime@cerno.tech \
--cc=tzimmermann@suse.de \
/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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).