Re: [PATCH 06/10] ARM: a9m2410: convert to devfs_create_partitions

From: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: barebox@lists.infradead.org
Subject: Re: [PATCH 06/10] ARM: a9m2410: convert to devfs_create_partitions
Date: Wed, 26 Feb 2014 17:30:00 +0100	[thread overview]
Message-ID: <530E1688.7030600@gmail.com> (raw)
In-Reply-To: <20140226155546.GJ6865@pengutronix.de>

On 02/26/14 16:55, Uwe Kleine-König wrote:
> On Wed, Feb 26, 2014 at 12:25:20AM +0100, Sebastian Hesselbarth wrote:
>> On 02/25/2014 11:51 PM, Uwe Kleine-König wrote:
>>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>>> ---
>>>   arch/arm/boards/a9m2410/a9m2410.c | 23 +++++++++++++++++------
>>>   1 file changed, 17 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/arch/arm/boards/a9m2410/a9m2410.c b/arch/arm/boards/a9m2410/a9m2410.c
>>> index b2b6c87117a3..8d528cf60378 100644
>>> --- a/arch/arm/boards/a9m2410/a9m2410.c
>>> +++ b/arch/arm/boards/a9m2410/a9m2410.c
>>> @@ -117,13 +117,24 @@ static int a9m2410_devices_init(void)
>>>   			16, IORESOURCE_MEM, NULL);
>>>
>>>   #ifdef CONFIG_NAND
>>> -	/* ----------- add some vital partitions -------- */
>>> -	devfs_add_partition("nand0", 0x00000, 0x40000, DEVFS_PARTITION_FIXED, "self_raw");
>>> -	dev_add_bb_dev("self_raw", "self0");
>>> -
>>> -	devfs_add_partition("nand0", 0x40000, 0x20000, DEVFS_PARTITION_FIXED, "env_raw");
>>> -	dev_add_bb_dev("env_raw", "env0");
>>> +	devfs_create_partitions("nand0", (struct devfs_partition[]){
>>
>> nit: It would be even more readable, if you move the struct
>> devfs_partition[] out of a9m2410_device_init() and reference
>> it here instead, i.e.
>>
>> static struct devfs_partition a9m2410_nand_partitions[] = {
>> 	...
>> 	{ }
>> };
> nit: I'd add "const" here. And you'd need something to not let the
> compiler generate a "a9m2410_nand_partitions not used" warning if

static const __maybe_unused devfs_partition a9m2410_nand_partitions[] ..

> CONFIG_NAND is disabled. Another (related) upside of using compound
> literals as I did is that "nand0" and the respecitve partition array is
> to be found at the same place.

You talk about "readability" in the cover letter. From that point of
view, moving it outside the code section, makes it _more_ readable, not
less.

> But having said that I don't really care how the boards are converted.
> If you want your approach, fine for me. (Ideally send a patch yourself,
> you can get my Ack then :-)

I don't care about the board conversion. It's a review, take it or leave
it. You seem to care how the boards are converted, as you've sent a
patch.

Sebastian

>> ...
>>
>> static int a9m2410_devices_init(void)
>> {
>> ...
>> 	#ifdef CONFIG_NAND
>> 	devfs_create_partitions("nand0", a9m2410_nand_partitions);
>> 	#endif
>> ...
>>
>> in here and the following patches.
>>
>> Sebastian
>>
>>> +			{
>>> +				.offset = 0,
>>> +				.size = 0x40000,
>>> +				.flags = DEVFS_PARTITION_FIXED,
>>> +				.name = "self_raw",
>>> +				.bbname = "self0",
>>> +			}, {
>>> +				.offset = DEVFS_PARTITION_APPEND,
>>> +				.size = 0x20000,
>>> +				.flags = DEVFS_PARTITION_FIXED,
>>> +				.name = "env_raw",
>>> +				.bbname = "env0",
>>> +			}, {
>>> +				/* sentinel (detected by .name = NULL) */
>>> +			}});
>>>   #endif
>>> +
>>>   	armlinux_set_architecture(MACH_TYPE_A9M2410);
>>>
>>>   	return 0;
>>>
>>
>>
>> _______________________________________________
>> barebox mailing list
>> barebox@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/barebox
>

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox