Re: 答复: [PATCH]Siliconmotion initial patch

[Greg KH <gregkh@linuxfoundation.org>]

On Tue, Feb 05, 2013 at 10:04:27PM +0100, Daniel Vetter wrote:
> On Tue, Feb 05, 2013 at 03:51:55PM +0800, Aaron.Chen  陈俊杰 wrote:
> > This is an initial patch for Siliconmotion Graphics chips. It add SM750
> > chip support with 2d accelerate and hw curser support. It is a complete
> > new driver. So the patch is a little bit big. Is it OK for review?
> 
> Adding a new fbdev driver while established consensus pretty much boils
> down to fbdev being a dead subsystem which should only be used for legacy
> applications and drivers: Not good. You should implement this as a drm
> modesetting driver, and if required base your 2d acceleration on top of
> the drm fb helpers. Or just implement a drm/gem driver to expose this.
> 
> Additionally you should include the appropriate mailing lists, which is
> linux-fbdev for fbdev drivers.
> 
> On a very quick read the patch has serious issues:
> 
> - checkpatch.pl:
> 
> total: 2 errors, 779 warnings, 7961 lines checked
> 
> NOTE: whitespace errors detected, you may wish to use scripts/cleanpatch or
>       scripts/cleanfile
> 
> - Remants of hungarian notation (dunno whether checkpatch checks for
>   those, too much other stuff flying by, but it really stuck out).
> 
> - Not using the linux i2c layer in the ddk750_hwi2c.c file.
> 
> - Reimplementing i2c bit-banging code in the ddk750_swi2c.c
> 
> - Adding a new implementation of a sil 164 dvi driver, we already have at
>   least two in drivers/gpu/drm/i2c/sil164_drv.c and
>   drivers/gpu/drm/i915/dvo_sil164.c.  This should be unified and probably
>   use the drm encoder slave framework.
> 
> At that point I've given up on further review, since there's clearly a lot
> of work left to do. Please review SubmittingPatches from the kernel
> documentation carefully. Also cc'ing our dear staging maintainer, he
> should be able to point you at further useful resources for getting this
> going.

If you wish to put this in the drivers/staging/ directory and do the
cleanup there before it moves to the "real" part of the kernel, I have
no objection to that.

thanks,

greg k-h