Patchwork [01/18] mtd: abstract last MTD partition parser argument

login
register
mail settings
Submitter Artem Bityutskiy
Date June 22, 2011, 4:21 a.m.
Message ID <1308716496.18119.8.camel@sauron>
Download mbox | patch
Permalink /patch/101390/
State New
Headers show

Comments

Artem Bityutskiy - June 22, 2011, 4:21 a.m.
I see a lot of checkpatch.pl warnings, could you please take a look?

Also, my gcc produces warnings with this patch because you have not
fixed up MPT parsers, e.g., like this:



On Sun, 2011-06-12 at 03:11 +0400, Dmitry Eremin-Solenikov wrote:
> - * @origin: start address of MTD device, %0 unless you are sure you need this.
> + * @parser_data: data passed to mtd parsers

Nitpick, but could you call this MTD partition parser-specific data
instead.

>   * @pparts: array of partitions found is returned here
> - * @origin: MTD device start address (use %0 if unsure)
> + * @data: data passed to MTD partition parsers

And this.

> +/**
> + * struct mtd_part_parser_data - used to pass data to MTD partition parsers.
> + * @origin: for RedBoot, start address of MTD device, %0 unless you are sure you need this.
> + */
> +struct mtd_part_parser_data {
> +	unsigned long origin;
> +};

Could you please embrace the origin field into an anonymous union - once
we add the of_node field they do not have to be at separate addresses. I
mean:

struct mtd_part_parser_data {
	union {
		unsigned long origin;
		struct device_node *of_node;
	};
};
Artem Bityutskiy - June 22, 2011, 4:59 a.m.
On Wed, 2011-06-22 at 07:21 +0300, Artem Bityutskiy wrote:
> I see a lot of checkpatch.pl warnings, could you please take a look?

Ah, and would it please be possible to make patches cleanly apply to the
l2-mtd-2.6 git tree?
Dmitry Eremin-Solenikov - June 22, 2011, 8:21 a.m.
On 6/22/11, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> I see a lot of checkpatch.pl warnings, could you please take a look?
>
> Also, my gcc produces warnings with this patch because you have not
> fixed up MPT parsers, e.g., like this:

Sorry, I was also busy that time and forgot about this :)

BTW: the patches should be applicable clearly to the l2-mtd at the time
I've sent them.

I'll fix all of your comments except this one:

> Could you please embrace the origin field into an anonymous union - once
> we add the of_node field they do not have to be at separate addresses. I
> mean:
>
> struct mtd_part_parser_data {
> 	union {
> 		unsigned long origin;
> 		struct device_node *of_node;
> 	};
> };

No, no and no. This data is passed to all parsers, so it should be valid for all
of them. Either we have to add a way to specify, what exactly we have provided,
or we have to leave data as separate struct fields.
Artem Bityutskiy - June 22, 2011, 8:55 a.m.
On Wed, 2011-06-22 at 12:21 +0400, Dmitry Eremin-Solenikov wrote:
> On 6/22/11, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> > I see a lot of checkpatch.pl warnings, could you please take a look?
> >
> > Also, my gcc produces warnings with this patch because you have not
> > fixed up MPT parsers, e.g., like this:
> 
> Sorry, I was also busy that time and forgot about this :)
> 
> BTW: the patches should be applicable clearly to the l2-mtd at the time
> I've sent them.
> 
> I'll fix all of your comments except this one:
> 
> > Could you please embrace the origin field into an anonymous union - once
> > we add the of_node field they do not have to be at separate addresses. I
> > mean:
> >
> > struct mtd_part_parser_data {
> > 	union {
> > 		unsigned long origin;
> > 		struct device_node *of_node;
> > 	};
> > };
> 
> No, no and no. This data is passed to all parsers, so it should be valid for all
> of them. Either we have to add a way to specify, what exactly we have provided,
> or we have to leave data as separate struct fields.

I do not see why we should waste memory - union will work well. This is
parser-specific object and the parser should know which fields belong to
him. And this object is not shared between parsers so they cannot screw
each other. Yes, this is not the most beautiful way to go, but it is
simple enough and suits this situation, I think.
Dmitry Eremin-Solenikov - June 22, 2011, 9:05 a.m.
On 6/22/11, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Wed, 2011-06-22 at 12:21 +0400, Dmitry Eremin-Solenikov wrote:
>> On 6/22/11, Artem Bityutskiy <dedekind1@gmail.com> wrote:
>> > I see a lot of checkpatch.pl warnings, could you please take a look?
>> >
>> > Also, my gcc produces warnings with this patch because you have not
>> > fixed up MPT parsers, e.g., like this:
>>
>> Sorry, I was also busy that time and forgot about this :)
>>
>> BTW: the patches should be applicable clearly to the l2-mtd at the time
>> I've sent them.
>>
>> I'll fix all of your comments except this one:
>>
>> > Could you please embrace the origin field into an anonymous union - once
>> > we add the of_node field they do not have to be at separate addresses. I
>> > mean:
>> >
>> > struct mtd_part_parser_data {
>> > 	union {
>> > 		unsigned long origin;
>> > 		struct device_node *of_node;
>> > 	};
>> > };
>>
>> No, no and no. This data is passed to all parsers, so it should be valid
>> for all
>> of them. Either we have to add a way to specify, what exactly we have
>> provided,
>> or we have to leave data as separate struct fields.
>
> I do not see why we should waste memory - union will work well. This is
> parser-specific object and the parser should know which fields belong to
> him. And this object is not shared between parsers so they cannot screw
> each other. Yes, this is not the most beautiful way to go, but it is
> simple enough and suits this situation, I think.

It _is_ shared between parsers. See: driver creates one mtd_part_parser_data
instance, populates it and passes to parse_mtd_partitions (directly or
indirectly).
Then each parser uses the same object to get data. Consider what will happen
when ixp4xx driver (which currently uses origin for RedBoot) will also gain
OF support (as it's expected for all ARM-related things). It will set both
origin (for RedBoot) and of_node (for ofpart).
Artem Bityutskiy - June 22, 2011, 9:16 a.m.
On Wed, 2011-06-22 at 13:05 +0400, Dmitry Eremin-Solenikov wrote:
> >> No, no and no. This data is passed to all parsers, so it should be valid
> >> for all
> >> of them. Either we have to add a way to specify, what exactly we have
> >> provided,
> >> or we have to leave data as separate struct fields.
> >
> > I do not see why we should waste memory - union will work well. This is
> > parser-specific object and the parser should know which fields belong to
> > him. And this object is not shared between parsers so they cannot screw
> > each other. Yes, this is not the most beautiful way to go, but it is
> > simple enough and suits this situation, I think.
> 
> It _is_ shared between parsers. See: driver creates one mtd_part_parser_data
> instance, populates it and passes to parse_mtd_partitions (directly or
> indirectly).
> Then each parser uses the same object to get data. Consider what will happen
> when ixp4xx driver (which currently uses origin for RedBoot) will also gain
> OF support (as it's expected for all ARM-related things). It will set both
> origin (for RedBoot) and of_node (for ofpart).

Agree, please, ignore that part of my feedback.

Patch

diff --git a/drivers/mtd/ar7part.c b/drivers/mtd/ar7part.c
index 6697a1e..71bfa2e 100644
--- a/drivers/mtd/ar7part.c
+++ b/drivers/mtd/ar7part.c
@@ -46,7 +46,7 @@  struct ar7_bin_rec {
 
 static int create_mtd_partitions(struct mtd_info *master,
                                 struct mtd_partition **pparts,
-                                unsigned long origin)
+                                struct mtd_part_parser_data *data)
 {
        struct ar7_bin_rec header;
        unsigned int offset;