From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stefano Babic <sbabic@denx.de>
Date: Mon, 29 Jun 2015 09:13:01 +0200
Subject: [U-Boot] [PATCH V2 2/2] imx: mx6 add i2c4 clock support for
	i.MX6SX
In-Reply-To: <20150629013332.GA31251@shlinux2>
References: <1435195985-11753-1-git-send-email-Peng.Fan@freescale.com>
 <1435195985-11753-2-git-send-email-Peng.Fan@freescale.com>
 <558FB5E2.2030404@denx.de> <20150629013332.GA31251@shlinux2>
Message-ID: <5590EFFD.70003@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hi Peng,

On 29/06/2015 03:33, Peng Fan wrote:
> Hi Stefano,
> 
> On Sun, Jun 28, 2015 at 10:52:50AM +0200, Stefano Babic wrote:
>> Hi Peng,
>>
>> On 25/06/2015 03:33, Peng Fan wrote:
>>> Add I2C4 clock support for i.MX6SX. Since we use runtime check,
>>> but not macro, we need to remove `#ifdef ..` in crm_regs.h, or
>>> we will get compliation failure for other platforms.
>>
>>               ^----- compliation ?
> I mean gcc will complains about failing to compile the code.

Right. Fix the commit message.


>> Agree to group together macros that belong only to a SOC variant, else
>> it is difficult to find them. But what has
>> MXC_CCM_CCGR6_VDOAXICLK_OFFSET to do with addings I2C4 clock to i.MX6SX
>> ? It should be posted in a separate patch.
> 
> I just move this piece of code from the back to the front, see the end
> of the patch.
> I removed the #ifdef in header file, because it is really not a good idea
> to see so many #ifdefs in header file. The bad point here is some CCM macros
> are exposed to other platforms, so we should take care to use the CCM macros.

That is correct. We have a lot of macro across all i.MXes, not only
i.MX6es. This allow to have consistent code, and we must take care of
not breaking other SOCs.

> 
> Take the following as example:
> In crm_regs.h
> #ifdef CONFIG_MX6SX
> #define MXC_CCM_CCGR6_PWM7_MASK 	(3 << 30)
> #else
> #define MXC_CCM_CCGR6_VDOAXICLK_MASK	(3 << 12)
> #endif
> 
> u32 test()
> {
> 	if (is_cpu_type(MXC_CPU_MX6SX))
> 		return MXC_CCM_CCGR6_PWM7_MASK;		
> 	else
> 		return MXC_CCM_CCGR6_VDOAXICLK_OFFSET;
> }

Here there is a bad use of these kind of macro. To use a define, we
should have the same name and not different names as in this case.

> 
> Here gcc can not successfully compile the code if we want to build for i.mx6sx,
> becuase MXC_CCM_CCGR6_VDOAXICLK_OFFSET is not defined for i.mx6sx.

Right,  but this is bad code and must be fixed.

> 
> So:
> #define MXC_CCM_CCGR6_PWM7_MASK     (3 << 30)
> /* This CCM Definition does not exist on i.MX6SX */
> #define MXC_CCM_CCGR6_VDOAXICLK_MASK    (3 << 12)

Better should be to add also a common name for both, that is then used
in the function.

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de
=====================================================================