Hi,
On 23-07-19 09:28, Sam Ravnborg wrote:
Hi Hans.
Driver looks good. Nce to see so much functionality in a driver less
than 1000 loc.
Following some bike-shedding only - that should have been sent for v1
already. But I thought better late then never.
Thank you for the feedback I've prepared a small followup cleanup
patch addressing some of your remarks.
<snip>
+#define DRIVER_NAME "gm12u320"
+#define DRIVER_DESC "Grain Media GM12U320 USB projector display"
+#define DRIVER_DATE "2019"
+#define DRIVER_MAJOR 1
+#define DRIVER_MINOR 0
+#define DRIVER_PATCHLEVEL 1
DRIVER_PATCHLEVEL is not used
Good one, fixed.
<snip>
+ vaddr = drm_gem_shmem_vmap(fb->obj[0]);
+ if (IS_ERR(vaddr)) {
+ DRM_ERROR("failed to vmap fb: %ld\n", PTR_ERR(vaddr));
+ goto put_fb;
+ }
+
+ if (fb->obj[0]->import_attach) {
+ ret = dma_buf_begin_cpu_access(
+ fb->obj[0]->import_attach->dmabuf, DMA_FROM_DEVICE);
+ if (ret) {
+ DRM_ERROR("dma_buf_begin_cpu_access err: %d\n", ret);
+ goto vunmap;
+ }
+ }
Here the code uses DRM_ERROR("...");
+ /* Do not log errors caused by module unload or device unplug */
+ if (ret != -ECONNRESET && ret != -ESHUTDOWN)
+ dev_err(&gm12u320->udev->dev, "Frame update error: %d\n", ret);
+}
And here dev_err(dev, "...");
It had been more consistent to use DRM_DEV_ERROR(dev, "..."); here.
Good one, I've added a second follow-up patch using DRM_DEV_ERROR
consistently everywhere.
+/* ------------------------------------------------------------------ */
+/* gm12u320 connector */
+
+
+#ifdef CONFIG_PM
+static int gm12u320_suspend(struct usb_interface *interface,
+ pm_message_t message)
+{
You can use __maybe_unused to get rid of the #ifdef,
Right, Noralf said that too, but I forgot. I've fixed this in the cleanup
patch.
Regards,
Hans
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
