On 04/28/2016 12:02 PM, Laxman Dewangan wrote: > > On Thursday 28 April 2016 02:31 PM, Lee Jones wrote: >> On Mon, 25 Apr 2016, Krzysztof Kozlowski wrote: >> >>> On 04/21/2016 02:25 PM, Laxman Dewangan wrote: >>>> Use devm_mfd_add_devices() for adding MFD child devices and >>>> devm_regmap_add_irq_chip() for IRQ chip registration. >>>> >>>> This reduces the error code path and .remove callback for removing >>>> MFD child devices and deleting IRQ chip data. >>>> >>>> Signed-off-by: Laxman Dewangan <ldewangan@xxxxxxxxxx> >>>> CC: Chanwoo Choi <cw00.choi@xxxxxxxxxxx> >>>> CC: Krzysztof Kozlowski <k.kozlowski@xxxxxxxxxxx> >>>> --- >>>> drivers/mfd/max77686.c | 31 ++++++++----------------------- >>>> 1 file changed, 8 insertions(+), 23 deletions(-) >>> Switching existing code to devm-like interface doesn't bring huge >>> benefits but looks okay and I'm fine with it: >> This is pretty much my view, but it get's Laxman's patch count up. ;) > > Yaah. :-) The patch was applied so discussion seems useful only for future work. I don't agree with some of your motivation. > There is some other motivation of doing this: > * I got the review comment about the resource leak and sequencing in my > max77620. It was silly mistake done by me and it causes recycle of > patch. To avoid this in future, devm_ was better option. This is new code. You made error in new code... this is not an argument to change something in existing well tested code. > * Spent lots of time on unbinding test during my RTC patch. Although fix > was not related to the devm_ but gave the impression that something we > are doing on probe. devm_ looks straight forward. Nope, the opposite. Usage of devm-like interface hides the order of cleaning up. With explicit clean it is easy to spot the reverse work of probe and to find the differences (like when the code is not in the exact reverse order). > - Some of code quality tools suggest to avoid goto statement. Only > possible if we dont have any code in error path i.e. return from any place. The 'goto' with one exit point is a part of kernel coding style. Of course lack of error paths is easier to read... usually. > - If we have devm_ apis for few resource and some does not support then > difficult to use them as this affect the sequence of deallocation. > Existing devm_ can be used effectively only if we have all resource > allocation using devm_. Ekhm? I don't understand. > - Reducing code size always better. Usually... but (again) introducing such questionable improvements for existing code which was well tested and is working fine: 1. May introduce bugs. Code is already working, error path maybe was or was not tested. At least it was reviewed. Your change may break this. 2. Is unnecessary code churn. If there are no clear benefits (and for me in case of max77686 there are no benefits) you just used mine and maintainer's time. This is even different kind of change than improvements for defensive coding (like adding 'consts'). Here, the code does not necessarily improve. Error paths and order of cleanup are hidden. Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
