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: sba...@denx.de
=====================================================================
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
