Re: 答复: [PATCH]Siliconmotion initial patch

Re: 答复: [PATCH]Siliconmotion initial patch

[Daniel Vetter]

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.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: 答复: [PATCH]Siliconmotion initial patch

[Greg KH]

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