Hello, Konrad Rzeszutek Wilk. > -----Original Message----- > From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@xxxxxxxxxx] > Sent: Wednesday, August 31, 2011 11:07 AM > To: Rob Clark > Cc: Inki Dae; sw0312.kim@xxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; dri- > devel@xxxxxxxxxxxxxxxxxxxxx; kyungmin.park@xxxxxxxxxxx; linux-arm- > kernel@xxxxxxxxxxxxxxxxxxx > Subject: Re: [RFC][PATCH v3] DRM: add DRM Driver for Samsung SoC > EXYNOS4210. > > > > + entry->vaddr = dma_alloc_writecombine(dev->dev, entry->size, > > > + (dma_addr_t *)&entry->paddr, GFP_KERNEL); > > > + if (!entry->paddr) { > > > + DRM_ERROR("failed to allocate buffer.\n"); > > > + return -ENOMEM; > > > + } > > > + > > > + DRM_DEBUG_KMS("allocated : vaddr(0x%x), paddr(0x%x), > size(0x%x)\n", > > > + (unsigned int)entry->vaddr, entry->paddr, entry- > >size); > > > + > > > + return 0; > > > +} > > > + > > [snip] > > > > > diff --git a/drivers/gpu/drm/samsung/samsung_drm_buf.h > b/drivers/gpu/drm/samsung/samsung_drm_buf.h > > > new file mode 100644 > > > index 0000000..d6a7e95 > > > --- /dev/null > > > +++ b/drivers/gpu/drm/samsung/samsung_drm_buf.h > > [snip] > > > +/** > > > + * samsung drm buffer entry structure. > > > + * > > > + * @paddr: physical address of allocated memory. > > > + * @vaddr: kernel virtual address of allocated memory. > > > + * @size: size of allocated memory. > > > + */ > > > +struct samsung_drm_buf_entry { > > > + unsigned int paddr; > > This could be made 'dma_addr_t' and then you can drop all of the > casts to (dma_addr_t *). > Ok, I will correct it right now. thank you. > .. snip.. > > > +static int samsung_drm_connector_get_modes(struct drm_connector > *connector) > > Why not make the return be 'unsigned int'? > Yes, I think so, but please, see drm_connector_helper_funcs structure of drm_crtc_helper.h. get_modes callback has int type as return. > .. snip.. > > > +/* get detection status of display device. */ > > > +static enum drm_connector_status > > > +samsung_drm_connector_detect(struct drm_connector *connector, bool > force) > > > +{ > > > + struct samsung_drm_connector *samsung_connector = > > > + to_samsung_connector(connector); > > > + struct samsung_drm_display *display = > > > + samsung_drm_get_manager(samsung_connector->encoder)- > >display; > > > + unsigned int ret = connector_status_unknown; > > Not 'enum drm_connector_status ret = connector_status_unknown' ? > Oh, you are right, I will fix up it. thank you. > > > + > > > + DRM_DEBUG_KMS("%s\n", __FILE__); > > > + > > > + if (display && display->is_connected) { > > > + if (display->is_connected()) > > > + ret = connector_status_connected; > > > + else > > > + ret = connector_status_disconnected; > > > + } > > > + > > > + return ret; > > > +} > > .. snip.. > > > +static void samsung_drm_fb_destroy(struct drm_framebuffer *fb) > > > +{ > > > + struct samsung_drm_fb *samsung_fb = to_samsung_fb(fb); > > > + int ret; > > Get rid of 'ret' It seems that it doesn't need 'ret' but it needs to check 'ret' because of drm_gem_handle_delete(). > > > + > > > + DRM_DEBUG_KMS("%s\n", __FILE__); > > > + > > > + drm_framebuffer_cleanup(fb); > > > + > > > + if (samsung_fb->is_default) { > > > + ret = drm_gem_handle_delete(samsung_fb->file_priv, > > > + samsung_fb->gem_handle); > > > > why not keep the gem buffer ptr, and do something like: > > > > drm_gem_object_unreference_unlocked(samsung_fb->bo).. > > > > this way, you get the right behavior if someone somewhere else took a > > ref to the gem buffer object? And it avoids needing to keep the > > file_priv ptr in the fb (which seems a bit strange) > > > > > > > + if (ret < 0) > > > + DRM_ERROR("failed to delete drm_gem_handle.\n"); > > And just do the check on the function return value here. You are not using > the 'ret' for anything. Yes, right. it just prints out error message because drm_gem_handle_delete function which is mainline function doesn't leave any error message. anyway using 'ret' is not clear. so I will remove 'ret' and check drm_gem_handle_delete function directly instead to print out error message. > > > + } > > > + > > > + kfree(samsung_fb); > > > +} > > > + > > [snip] > > Hm, so I stopped here - just realized that I am missing some of the code > and I should look at the original patch.. Thank you for your comments. it's been very useful. please give me your comments and advices anytime then I will be pleased. _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
