From: "Christian König" <christian.koenig@amd.com> To: Thomas Zimmermann <tzimmermann@suse.de>, Cai Huoqing <cai.huoqing@linux.dev> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, Maxime Ripard <mripard@kernel.org>, David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>, Sumit Semwal <sumit.semwal@linaro.org>, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-media@vger.kernel.org, linaro-mm-sig@lists.linaro.org Subject: Re: [PATCH 2/2] drm/nvdla: Add driver support for NVDLA Date: Thu, 21 Apr 2022 10:34:20 +0200 [thread overview] Message-ID: <b34fa2a5-58b4-6270-2d19-2ad591e3e250@amd.com> (raw) In-Reply-To: <2aeee5a2-b5a5-348e-ccf7-04f49e1119da@suse.de> Am 21.04.22 um 10:30 schrieb Thomas Zimmermann: > (Resending, as some MLs didn't like the size of the origninal mail.) > > Hi, > > thanks for your submission. Some general comments: > > * some functions are prefixed with dla_, others use nvdla_. It seems > arbitrary to me. Please use nvdla_ consistently throughout the source > code. > > * For reporting errors, please use drm_err(), drm_warn(), etc. I > suggest to rearrange the error messages to not be located in the > innermost functions. If you plan to have multiple instances of the driver loaded at the same time, using drm_dev_err(), drm_dev_warn() etc.. would be even better. BTW: I'm still absolutely not keen to enforcing drm_* log functions. So if you prefer to stick with pr_err() and dev_err() we could discuss that once more. Regards, Christian. > > * Could you please split this patch into smaller pieces? It > currently hits size limits of some mailing lists. Maybe add the > register constants separately. > > Please find more review comments below. It's not a full review, but at > least something to start with. > > Best regards > Thomas
WARNING: multiple messages have this Message-ID (diff)
From: "Christian König" <christian.koenig@amd.com> To: Thomas Zimmermann <tzimmermann@suse.de>, Cai Huoqing <cai.huoqing@linux.dev> Cc: David Airlie <airlied@linux.ie>, linux-kernel@vger.kernel.org, linaro-mm-sig@lists.linaro.org, dri-devel@lists.freedesktop.org, Sumit Semwal <sumit.semwal@linaro.org>, linux-media@vger.kernel.org Subject: Re: [PATCH 2/2] drm/nvdla: Add driver support for NVDLA Date: Thu, 21 Apr 2022 10:34:20 +0200 [thread overview] Message-ID: <b34fa2a5-58b4-6270-2d19-2ad591e3e250@amd.com> (raw) In-Reply-To: <2aeee5a2-b5a5-348e-ccf7-04f49e1119da@suse.de> Am 21.04.22 um 10:30 schrieb Thomas Zimmermann: > (Resending, as some MLs didn't like the size of the origninal mail.) > > Hi, > > thanks for your submission. Some general comments: > > * some functions are prefixed with dla_, others use nvdla_. It seems > arbitrary to me. Please use nvdla_ consistently throughout the source > code. > > * For reporting errors, please use drm_err(), drm_warn(), etc. I > suggest to rearrange the error messages to not be located in the > innermost functions. If you plan to have multiple instances of the driver loaded at the same time, using drm_dev_err(), drm_dev_warn() etc.. would be even better. BTW: I'm still absolutely not keen to enforcing drm_* log functions. So if you prefer to stick with pr_err() and dev_err() we could discuss that once more. Regards, Christian. > > * Could you please split this patch into smaller pieces? It > currently hits size limits of some mailing lists. Maybe add the > register constants separately. > > Please find more review comments below. It's not a full review, but at > least something to start with. > > Best regards > Thomas
next prev parent reply other threads:[~2022-04-21 8:34 UTC|newest] Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-04-19 13:58 [PATCH 0/2] drm/nvdla: Add driver support for NVDLA Cai Huoqing 2022-04-19 13:58 ` Cai Huoqing 2022-04-19 13:58 ` [PATCH 1/2] MAINTAINERS: Add the driver info of the NVDLA Cai Huoqing 2022-04-19 13:58 ` Cai Huoqing 2022-04-19 13:59 ` [PATCH 2/2] drm/nvdla: Add driver support for NVDLA Cai Huoqing 2022-04-19 13:59 ` Cai Huoqing 2022-04-19 14:07 ` Christian König 2022-04-19 14:07 ` Christian König 2022-04-19 14:35 ` Cai Huoqing 2022-04-19 14:35 ` Cai Huoqing 2022-04-20 7:53 ` kernel test robot 2022-04-20 7:53 ` kernel test robot 2022-04-20 9:57 ` kernel test robot 2022-04-20 9:57 ` kernel test robot 2022-04-20 10:26 ` Thomas Zimmermann 2022-04-20 10:26 ` Thomas Zimmermann 2022-04-21 8:09 ` Cai Huoqing 2022-04-21 8:30 ` Thomas Zimmermann 2022-04-21 8:30 ` Thomas Zimmermann 2022-04-21 8:34 ` Christian König [this message] 2022-04-21 8:34 ` Christian König 2022-04-21 8:57 ` Thomas Zimmermann 2022-04-21 8:57 ` Thomas Zimmermann 2022-04-21 9:07 ` Thomas Zimmermann 2022-04-21 9:07 ` Thomas Zimmermann 2022-04-21 9:13 ` Thomas Zimmermann 2022-04-21 9:13 ` Thomas Zimmermann 2022-04-21 9:23 ` Christian König 2022-04-21 9:23 ` Christian König 2022-04-21 22:01 ` Kari Argillander 2022-04-21 22:01 ` Kari Argillander 2022-04-25 14:28 ` Cai Huoqing 2022-04-25 14:28 ` Cai Huoqing 2022-04-27 15:23 ` Daniel Vetter 2022-04-27 15:23 ` Daniel Vetter 2022-04-26 5:20 ` [PATCH 0/2] " Peter Robinson 2022-04-26 5:20 ` Peter Robinson
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=b34fa2a5-58b4-6270-2d19-2ad591e3e250@amd.com \ --to=christian.koenig@amd.com \ --cc=airlied@linux.ie \ --cc=cai.huoqing@linux.dev \ --cc=daniel@ffwll.ch \ --cc=dri-devel@lists.freedesktop.org \ --cc=linaro-mm-sig@lists.linaro.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-media@vger.kernel.org \ --cc=maarten.lankhorst@linux.intel.com \ --cc=mripard@kernel.org \ --cc=sumit.semwal@linaro.org \ --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: 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.