[U-Boot] [PATCH V2 2/2] imx: mx6 add i2c4 clock support for i.MX6SX

[Stefano Babic]

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 at denx.de
=====================================================================