[PATCH] ARM: mxs: Add initial support for Bluegiga APX4 Development Kit

[Lauri Hintsala]

On 10/18/2011 10:39 AM, Uwe Kleine-König wrote:
> On Tue, Oct 18, 2011 at 10:27:04AM +0300, Lauri Hintsala wrote:
>> Hi Uwe,
>>
>> On 10/17/2011 12:44 PM, Uwe Kleine-König wrote:
>>> Hello,
>>>
>>> On Mon, Oct 17, 2011 at 11:08:36AM +0300, Lauri Hintsala wrote:
>>>> Added initial support for Bluegiga APX4 module and Development Kit.
>>>> Patches are based on Linux v3.1-rc9.
>>> Specifying the base isn't helpful in the commit log. You can better
>>> write things like these after the tripple dash below.
>>
>> I'll drop the version information from commit log.
>>
>>
>>>> +config MODULE_APX4
>>>> +	bool
>>>> +	select SOC_IMX28
>>>> +	select LEDS_GPIO_REGISTER
>>>> +	select MXS_HAVE_AMBA_DUART
>>>> +	select MXS_HAVE_PLATFORM_AUART
>>>> +	select MXS_HAVE_PLATFORM_FEC
>>>> +    select MXS_HAVE_PLATFORM_MXS_I2C
>>> broken indention
>>
>> Thanks for catching it.
>>
>>
>>>> +	select MXS_HAVE_PLATFORM_MXS_MMC
>>>> +	select MXS_OCOTP
>>>> +
>>> MODULE_APX4 is unused, do you plan to use it later?
>>
>> It is already used. MODULE_APX4 is selected by MACH_APX4DEVKIT.
> Yeah, so you have two symbols that are =y for your board. One of them
> doesn't do anything apart from being there and having a value. Just
> removing config MODULE_APX4, putting all the selects into
> MACH_APX4DEVKIT and remove the select MODULE_APX4 from it doesn't break
> anything.

We are planning to add several boards based on the same APX4 module in 
the future. Then it would be useful to collect generic code into one 
module source file.


> MODULE_TX28 at least has a separate file that is compiled for it and
> that might be useful for maschines that don't necessarily use
> mach-tx28.c .
>
>>>>   config MACH_TX28
>>>>   	bool "Ka-Ro TX28 module"
>>>>   	select MODULE_TX28
>>>>
>>>> +config MACH_APX4DEVKIT
>>>> +	bool "Support Bluegiga APX4 Development Kit"
>>>> +	select MODULE_APX4
>>>> +
>>>>   endif
>>>> +static int __init apx4devkit_fec_get_mac(char *macstr)
>>>> +{
>>>> +	int i, h, l;
>>>> +
>>>> +	macstr++;
>>>> +
>>>> +	for (i = 0; i<   6; i++) {
>>>> +		if (i != 5&&   *(macstr + 2) != ':')
>>>> +			goto error;
>>>> +
>>>> +		h = hex_to_bin(*macstr++);
>>>> +		if (h == -1)
>>>> +			goto error;
>>>> +
>>>> +		l = hex_to_bin(*macstr++);
>>>> +		if (l == -1)
>>>> +			goto error;
>>>> +
>>>> +		macstr++;
>>>> +		mx28_fec_pdata.mac[i] = (h<<   4) + l;
>>>> +	}
>>>> +	return 0;
>>> I wonder if there isn't a more generic way to parse a mac address.
>>
>> Maybe it is better to use sscanf for parsing mac address, is it. I
>> didn't found any generic function for this purpose. Is there any?
> sscanf("%pM", ...) would be straight forward, but I'd be surprised if
> that worked. I'd ask on netdev at vger.kernel.org.
>
>> Other boards seems to use similar methods for parsing mac address:
>> arch/arm/mach-orion5x/dns323-setup.c: dns323_read_mac_addr
>> arch/arm/mach-orion5x/tsx09-common.c: qnap_tsx09_check_mac_addr
>> arch/mips/rb532/devices.c: parse_mac_addr
>>
>> I dont't mean it is right/best way to do parsing but it seems to be
>> generic problem.
> Yeah, so a generic solution would be great!
>
>>> Other machines put the mac into the otp.
>>
>> We do not want to use otps for mac.
>>
>>
>>>> +error:
>>>> +	pr_err("%s: invalid mac address\n", __func__);
>>>> +	return -EINVAL;
>>>> +}
>>>> +
>>>> +__setup("ethaddr", apx4devkit_fec_get_mac);
>>> the name is IMHO too generic for a board specific parameter. Think about
>>> a kernel that runs on all mxs based machines. ethaddr=... only affects
>>> your machine type. And it's not possible to add the same parameter to a
>>> different board.
>>
>> I could rename the parameter name.
> fec.macaddr should work already today.

fec.macaddr is only getting mac address as array not in colon separated 
string format as uboot is handling it.


>>>> +static void __init apx4devkit_timer_init(void)
>>>> +{
>>>> +	mx28_clocks_init();
>>>> +}
>>>> +
>>>> +static struct sys_timer apx4devkit_timer = {
>>>> +	.init	= apx4devkit_timer_init,
>>>> +};
>>>> +
>>>> +MACHINE_START(APX4DEVKIT, "Bluegiga APX4 Development Kit")
>>>> +	.map_io		= mx28_map_io,
>>>> +	.init_irq	= mx28_init_irq,
>>>> +	.init_machine	= apx4devkit_init,
>>>> +	.timer		=&apx4devkit_timer,
>>>> +MACHINE_END
>>> please read http://article.gmane.org/gmane.linux.ports.arm.omap/50721
>>
>> Do you mean init_machine and timer are in wrong order?
> yes.

Ok.

I noticed mx28evk and mx28evk have also those functions in wrong order.


Best Regards,
Lauri Hintsala