CC Eric (freescale), Rob (TI) who working the DRM with other ARM SoCs.
As Dave mentioned, Please review Samsung DRM codes.
Thank you, Kyungmin Park
-----Original Message----- From: Dave Airlie [mailto:airlied@gmail.com] Sent: Monday, August 29, 2011 11:27 PM To: Inki Dae Cc: airlied@linux.ie; kyungmin.park@samsung.com; dri-devel@lists.freedesktop.org Subject: Re: Review request to DRM Driver for Samsung SoC Exynos4210.
I had sent DRM Driver for Samsung SoC Exynos4210 over the three times.
But I didn't received any comments from you.
I was sort of hoping someone else would take a look at these ARM drivers before me, it might be worth getting some inter-company review between the vendors submitting drm components as I'm guessing its going to be a lot of the same thing.
But I've done a quick review and some things that stick out.
1. include/drm/samsung_drm.h should only contain public information for use on the userspace ioctl interface, it shouldn't contain any kernel private data structures, they should be in drivers/gpu/drm/samsung/samsung_drv.h or something very similiar.
2. I'm not sure why samsung_drm_fn_encoder exists, it looks like from the crtc mode set functions you call the encoder mode set functions, is it not possible to use the helper drm_crtc_helper_set_mode so that the crtc mode set is called then the encoder ones without having the explicit calls? If not please either document it or point at the explaination I missed.
3. Not terribly urgent but is samsung the best name for this? what happens if you bring out another chip, I also think there is a lot of samsung_drm_ in function names that seems not that useful. dropping _drm_ might help.
4 ioctls: drm_samsung_gem_map_off needs explicit padding before the 64-bit, drm_samsung_gem_mmap needs explicit padding before the 64-bit,, though I'm not sure you need these ioctls all now that the dumb interface is upstream,
what is usr_addr in the gem create ioctl for? this seems wrong, it also looks too short for 64-bit addresses, but passing in userspace addr to kernel is generally not a great plan.
5. you probably don't need master_create/set ops.
Dave.