Hi Liu Ying, Thanks for looking at the patches. On Sun, Dec 12, 2010 at 01:21:57PM +0800, Liu Ying wrote: > Hello, Sascha, > > IPU is not embedded in i,MX50 SoC, but in i.MX51/53 SoCs, please > modify the commit message. > > I have the following comments for the files editted respectively: > 1) ipu-common.c > - ipu_idmac_get()/ipu_idmac_put() need a mechanism to protect IPU > IDMAC resources, namely, get rid of potential race condition issue. As > only framebuffer support is added in your patches, there should be no > race condition issue now. After IC channels are supported(maybe as DMA > engine), perhaps, there will be such kind of issue. ok. > - ipu_idmac_select_buffer() need to add spinlock to protect > IPU_CHA_BUFx_RDY registers. ok. > - It looks that ipu_uninit_channel() only clears > IPU_CHA_DB_MODE_SEL register, so why not put it in > ipu_idmac_disable_channel()? ok. > - It looks that ipu_add_client_devices() and your framebuffer > patch assume /dev/fb0 uses DP_BG, /dev/fb1 uses DP_FG and /dev/fb2 > uses DC. > But fb1_platform_data->ipu_channel_bg is > MX51_IPU_CHANNEL_MEM_DC_SYNC, this may make confusion for driver > readers and it is not easy for code maintenance. Do you have a better suggestion? fb2 becomes fb1 when overlay support is not used. > - It also looks that ipu_add_client_devices() and your framebuffer > driver assume DP_BG/DP_FG are bound with DI0 and DC is bound with DI1. > It is ok for babbage board to support this kind of > configuration, but it may bring some limitation. For example, TVE(TV > encoder) module embedded in MX51 SoC can only connected with DI1 and > users may like to use TV as the primary device(support HW overlay), > and FSL Android BSP does support to use DI1 with LCD as the primary > device on babbage board. SO we need to put the display number into the platform data, right? > > 2) ipu-cpmem.c > - In order to improve performance, maybe writing > ipu_ch_param_addr(ch) directly will be fine, but not using memset() > and memcpy() in ipu_ch_param_init(). I don't see this function in any performance critical path. > > 3) ipu-dc.c > - Looks '#include <asm/atomic.h>' and '#include > <linux/spinlock.h>' are unnecessary. > - Need to call 'ipu_module_disable(IPU_CONF_DC_EN);' somewhere. ok. > > 4) ipu-di.c > - Looks '#include <asm/atomic.h>' and '#include <linux/delay.h>' > are unnecessary. ok. > > 5) ipu-dmfc.c > - Looks '#include <linux/delay.h>' are unnecessary. ok. > > 6) ipu-dp.c > - Looks '#include <asm/atomic.h>' and '#include <linux/delay.h>' > are unnecessary. ok. > > 7) ipu-prv.h > - Looks '#include <linux/interrupt.h>' is unnecessary. > - Please rename 'MX51_IPU_CHANNEL_xxxx' to be 'IPU_CHANNEL_xxxx', > IPUv3 channel names do not depend on SoC name. I was proven wrong each time I believed this. > > 8) include/linux/mfd/imx-ipu-v3.h > - Looks '#include <linux/slab.h>' and '#include > <linux/interrupt.h>' are unnecessary. ok. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html