| Message ID | 1307453803-31950-6-git-send-email-dbaryshkov@gmail.com |
|---|---|
| State | Accepted |
| Commit | cd3aafd0bd3ad383fd43196bc8dcac2edfec95c2 |
| Headers | show |
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? :-)
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.
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 :)
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.
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.
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.
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.
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;
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(-)