linux-kernel - Re: [PATCH 3/9] Add a mfd IPUv3 driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20101213112345.GD6017@pengutronix.de>
Date:	Mon, 13 Dec 2010 12:23:45 +0100
From:	Sascha Hauer <s.hauer@...gutronix.de>
To:	Liu Ying <liu.y.victor@...il.com>
Cc:	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	linux-fbdev@...r.kernel.org,
	Zhang Lily-R58066 <r58066@...escale.com>,
	Arnaud Patard <arnaud.patard@...-net.org>
Subject: Re: [PATCH 3/9] Add a mfd IPUv3 driver

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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/