Hi Pekon,
On 10/12/2013 03:26 PM, Gupta, Pekon wrote:
I will try to summarize the standing of your patch series.
Patches 1 and 2 look good and have addressed all of the DT maintainers'
comments, AFAICT. They are ready to go in, except that the following
patches are not ready; they should probably go in together.
You ignored most of my comments to patch 3, and it is insufficiently
documented. Please take a look at my comments, both here (in v8) and in
v6. It still should be split into more patches.
I tried splitting the earlier [PATCH 3/6], therefore a new patch for merging
various Hamming ECC schemes was spawned. But, I could not clean more
because I would have broken the NAND functionality in between the
patches.
From what I understand, your comment "cleans redundant code" in patch 3
is entirely its own logical change; you're replacing the SW ECC in
omap2.c with the library wrapper in lib/bch.c, right? So this should be
described as such (as "replacing XXX with YYY") and can easily be its
own patch?
Also, you add an 'mtd' and a 'nand_chip' (can you just call this 'chip'
or maybe 'nand', like in other drivers?) variable to the probe function,
and then partially convert the function over to inconsistently using mtd
vs. info->mtd and nand_chip vs. info->nand. If you want to add these
variables, just make it a separate patch.
I also mentioned your changes to the buswidth code, which seem to have
snuck in again. This should be a separate patch.
My apologies, I missed some of you other comments, so I'm preparing
next version by splitting [PATCH 3/6] into more sub-patches for ease of
review.
Great, thanks.
Most #ifdef were put to suppress warning of un-used functions during
compile time. So those cannot be removed, otherwise this patch would
give compile warnings under randconfig.
Could be marked __maybe_unused instead? #ifdef's are also OK, if
necessary. There were just a few places I pointed out that they was
certainly not needed.
Patch 4 does too much without describing it. Why are you dropping the
old omap3_correct_data_bch() code? Is this just refactoring? If so, you
should say so. And this also suggests that you have two logical changes
going on that should be separated into different patches; one to
refactor the open-coded NAND/BCH library to replace it with the
nand_bch.{c,h} support library and one for the new ECC modes.
Agreed, I update commit log to be more explicit that here nand_bch.c
actually encapsulates lib/bch.c.
Here also I cannot remove #ifdef across nand_bch_free() because
it frees some bch control data, which would not be defined for all
ecc-schemes (it is specific to xx_SW ecc schemes). Hence removing
#ifdef here would give null-pointer exception.
Note that nand_bch_free() (and many others in nand_bch.h) are given
empty inline implementation for CONFIG_MTD_NAND_ECC_BCH=n. But if you
are unsure, then you can leave the #ifdef's to be conservative.
Patch 5 is good but should wait until the other DT parts are acceptable
and merged into MTD.
Patch 6 needs rewriting to use devm_* functions properly, but it was
never integral to this patch series. You can improve it and resend with
this series or just send it separately afterward.
Yes I understood this, but I think it would be still good to explicitly
free the resources in case of "module_remove()", because that would
free all resources instantaneously without waiting for devm_ to
iterate thru the list when it wakes-up (I guess).
devm_* is part of the driver core, so it does the cleanup immediately on
a non-zero return from the probe() callback, or after exit from the
remove() callback. It does not have to "wake up". Its only overhead is
the small amount of memory used for the linked-list management.
Again, this is an optional piece of work, so don't worry about it too
much. But you get 0 benefit (just a slight increase in memory usage) if
you just convert all the kfree() calls to devm_kfree().
I'm not knowledgeable of exact implementation of devm_ and how
often does it iterates the list and frees the resources for corresponding
un-registered devices. So trusting you I'll include your feedbacks here.
I haven't closely reviewed 100% of its implementation, but it is fairly
straightforward and commonly used in many drivers. You should be fine
using it as documented and recommended by me.
Thanks,
Brian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
