Patchwork [05/44] mtd: bfin-async-flash.c: use mtd_device_parse_register

login
register
mail settings
Submitter Dmitry Eremin-Solenikov
Date June 7, 2011, 1:36 p.m.
Message ID <1307453803-31950-6-git-send-email-dbaryshkov@gmail.com>
Download mbox | patch
Permalink /patch/99168/
State New
Headers show

Comments

Dmitry Eremin-Solenikov - June 7, 2011, 1:36 p.m.
Replace custom invocations of parse_mtd_partitions and mtd_device_register
with common mtd_device_parse_register call. This would bring: standard
handling of all errors, fallback to default partitions, etc.

Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
---
 drivers/mtd/maps/bfin-async-flash.c |   16 ++--------------
 1 files changed, 2 insertions(+), 14 deletions(-)
Artem Bityutskiy - June 8, 2011, 9:01 a.m.
On Tue, 2011-06-07 at 17:36 +0400, Dmitry Eremin-Solenikov wrote:
> +       mtd_device_parse_register(state->mtd, part_probe_types, 0,
> +                       pdata->parts, pdata->nr_parts);

How about checking the return code? :-)
Artem Bityutskiy - June 8, 2011, 9:02 a.m.
On Wed, 2011-06-08 at 12:01 +0300, Artem Bityutskiy wrote:
> On Tue, 2011-06-07 at 17:36 +0400, Dmitry Eremin-Solenikov wrote:
> > +       mtd_device_parse_register(state->mtd, part_probe_types, 0,
> > +                       pdata->parts, pdata->nr_parts);
> 
> How about checking the return code? :-)

Ok, you did not do it because the original function did not do it. Fair
enough, this is a separate thing.
Dmitry Eremin-Solenikov - June 8, 2011, 2:06 p.m.
On 6/8/11, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Wed, 2011-06-08 at 12:01 +0300, Artem Bityutskiy wrote:
>> On Tue, 2011-06-07 at 17:36 +0400, Dmitry Eremin-Solenikov wrote:
>> > +       mtd_device_parse_register(state->mtd, part_probe_types, 0,
>> > +                       pdata->parts, pdata->nr_parts);
>>
>> How about checking the return code? :-)
>
> Ok, you did not do it because the original function did not do it. Fair
> enough, this is a separate thing.

Nice idea. Making mtd_device_parse_register a __must_check function :)
Artem Bityutskiy - June 8, 2011, 2:12 p.m.
On Wed, 2011-06-08 at 18:06 +0400, Dmitry Eremin-Solenikov wrote:
> On 6/8/11, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> > On Wed, 2011-06-08 at 12:01 +0300, Artem Bityutskiy wrote:
> >> On Tue, 2011-06-07 at 17:36 +0400, Dmitry Eremin-Solenikov wrote:
> >> > +       mtd_device_parse_register(state->mtd, part_probe_types, 0,
> >> > +                       pdata->parts, pdata->nr_parts);
> >>
> >> How about checking the return code? :-)
> >
> > Ok, you did not do it because the original function did not do it. Fair
> > enough, this is a separate thing.
> 
> Nice idea. Making mtd_device_parse_register a __must_check function :)

But please, do it as a separate patch.
Dmitry Eremin-Solenikov - June 8, 2011, 2:22 p.m.
On 6/8/11, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Wed, 2011-06-08 at 18:06 +0400, Dmitry Eremin-Solenikov wrote:
>> On 6/8/11, Artem Bityutskiy <dedekind1@gmail.com> wrote:
>> > On Wed, 2011-06-08 at 12:01 +0300, Artem Bityutskiy wrote:
>> >> On Tue, 2011-06-07 at 17:36 +0400, Dmitry Eremin-Solenikov wrote:
>> >> > +       mtd_device_parse_register(state->mtd, part_probe_types, 0,
>> >> > +                       pdata->parts, pdata->nr_parts);
>> >>
>> >> How about checking the return code? :-)
>> >
>> > Ok, you did not do it because the original function did not do it. Fair
>> > enough, this is a separate thing.
>>
>> Nice idea. Making mtd_device_parse_register a __must_check function :)
>
> But please, do it as a separate patch.

Why not make it a __must_check from the beginning? And fixup problematic
drivers later.
Artem Bityutskiy - June 8, 2011, 2:24 p.m.
On Wed, 2011-06-08 at 18:22 +0400, Dmitry Eremin-Solenikov wrote:
> On 6/8/11, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> > On Wed, 2011-06-08 at 18:06 +0400, Dmitry Eremin-Solenikov wrote:
> >> On 6/8/11, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> >> > On Wed, 2011-06-08 at 12:01 +0300, Artem Bityutskiy wrote:
> >> >> On Tue, 2011-06-07 at 17:36 +0400, Dmitry Eremin-Solenikov wrote:
> >> >> > +       mtd_device_parse_register(state->mtd, part_probe_types, 0,
> >> >> > +                       pdata->parts, pdata->nr_parts);
> >> >>
> >> >> How about checking the return code? :-)
> >> >
> >> > Ok, you did not do it because the original function did not do it. Fair
> >> > enough, this is a separate thing.
> >>
> >> Nice idea. Making mtd_device_parse_register a __must_check function :)
> >
> > But please, do it as a separate patch.
> 
> Why not make it a __must_check from the beginning? And fixup problematic
> drivers later.

Because it is a separate problem and separate set of patches is needed.
First, solve one problem, then start solving the other.

Secondly, if someone will later prove that __must_check was a mistake,
we can just revert that separate patch.
Dmitry Eremin-Solenikov - June 8, 2011, 2:31 p.m.
On 6/8/11, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Wed, 2011-06-08 at 18:22 +0400, Dmitry Eremin-Solenikov wrote:
>> On 6/8/11, Artem Bityutskiy <dedekind1@gmail.com> wrote:
>> > On Wed, 2011-06-08 at 18:06 +0400, Dmitry Eremin-Solenikov wrote:
>> >> On 6/8/11, Artem Bityutskiy <dedekind1@gmail.com> wrote:
>> >> > On Wed, 2011-06-08 at 12:01 +0300, Artem Bityutskiy wrote:
>> >> >> On Tue, 2011-06-07 at 17:36 +0400, Dmitry Eremin-Solenikov wrote:
>> >> >> > +       mtd_device_parse_register(state->mtd, part_probe_types, 0,
>> >> >> > +                       pdata->parts, pdata->nr_parts);
>> >> >>
>> >> >> How about checking the return code? :-)
>> >> >
>> >> > Ok, you did not do it because the original function did not do it.
>> >> > Fair
>> >> > enough, this is a separate thing.
>> >>
>> >> Nice idea. Making mtd_device_parse_register a __must_check function :)
>> >
>> > But please, do it as a separate patch.
>>
>> Why not make it a __must_check from the beginning? And fixup problematic
>> drivers later.
>
> Because it is a separate problem and separate set of patches is needed.
> First, solve one problem, then start solving the other.
>
> Secondly, if someone will later prove that __must_check was a mistake,
> we can just revert that separate patch.

Roger.

Patch

diff --git a/drivers/mtd/maps/bfin-async-flash.c b/drivers/mtd/maps/bfin-async-flash.c
index d4297a9..246890e 100644
--- a/drivers/mtd/maps/bfin-async-flash.c
+++ b/drivers/mtd/maps/bfin-async-flash.c
@@ -41,7 +41,6 @@  struct async_state {
 	uint32_t flash_ambctl0, flash_ambctl1;
 	uint32_t save_ambctl0, save_ambctl1;
 	unsigned long irq_flags;
-	struct mtd_partition *parts;
 };
 
 static void switch_to_flash(struct async_state *state)
@@ -165,18 +164,8 @@  static int __devinit bfin_flash_probe(struct platform_device *pdev)
 		return -ENXIO;
 	}
 
-	ret = parse_mtd_partitions(state->mtd, part_probe_types, &pdata->parts, 0);
-	if (ret > 0) {
-		pr_devinit(KERN_NOTICE DRIVER_NAME ": Using commandline partition definition\n");
-		mtd_device_register(state->mtd, pdata->parts, ret);
-		state->parts = pdata->parts;
-	} else if (pdata->nr_parts) {
-		pr_devinit(KERN_NOTICE DRIVER_NAME ": Using board partition definition\n");
-		mtd_device_register(state->mtd, pdata->parts, pdata->nr_parts);
-	} else {
-		pr_devinit(KERN_NOTICE DRIVER_NAME ": no partition info available, registering whole flash at once\n");
-		mtd_device_register(state->mtd, NULL, 0);
-	}
+	mtd_device_parse_register(state->mtd, part_probe_types, 0,
+			pdata->parts, pdata->nr_parts);
 
 	platform_set_drvdata(pdev, state);
 
@@ -188,7 +177,6 @@  static int __devexit bfin_flash_remove(struct platform_device *pdev)
 	struct async_state *state = platform_get_drvdata(pdev);
 	gpio_free(state->enet_flash_pin);
 	mtd_device_unregister(state->mtd);
-	kfree(state->parts);
 	map_destroy(state->mtd);
 	kfree(state);
 	return 0;