| Message ID | 1461241558-26983-4-git-send-email-ldewangan@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@nvidia.com> > CC: Chanwoo Choi <cw00.choi@samsung.com> > CC: Krzysztof Kozlowski <k.kozlowski@samsung.com> > --- > 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: Tested-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> Best regards, Krzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@nvidia.com> > > CC: Chanwoo Choi <cw00.choi@samsung.com> > > CC: Krzysztof Kozlowski <k.kozlowski@samsung.com> > > --- > > 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. ;)
On Thu, 21 Apr 2016, 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@nvidia.com> > CC: Chanwoo Choi <cw00.choi@samsung.com> > CC: Krzysztof Kozlowski <k.kozlowski@samsung.com> > --- > drivers/mfd/max77686.c | 31 ++++++++----------------------- > 1 file changed, 8 insertions(+), 23 deletions(-) Applied, thanks. > diff --git a/drivers/mfd/max77686.c b/drivers/mfd/max77686.c > index 7a0457e..7b68ed7 100644 > --- a/drivers/mfd/max77686.c > +++ b/drivers/mfd/max77686.c > @@ -230,38 +230,24 @@ static int max77686_i2c_probe(struct i2c_client *i2c, > return -ENODEV; > } > > - ret = regmap_add_irq_chip(max77686->regmap, max77686->irq, > - IRQF_TRIGGER_FALLING | IRQF_ONESHOT | > - IRQF_SHARED, 0, irq_chip, > - &max77686->irq_data); > + ret = devm_regmap_add_irq_chip(&i2c->dev, max77686->regmap, > + max77686->irq, > + IRQF_TRIGGER_FALLING | IRQF_ONESHOT | > + IRQF_SHARED, 0, irq_chip, > + &max77686->irq_data); > if (ret < 0) { > dev_err(&i2c->dev, "failed to add PMIC irq chip: %d\n", ret); > return ret; > } > > - ret = mfd_add_devices(max77686->dev, -1, cells, n_devs, NULL, 0, NULL); > + ret = devm_mfd_add_devices(max77686->dev, -1, cells, n_devs, NULL, > + 0, NULL); > if (ret < 0) { > dev_err(&i2c->dev, "failed to add MFD devices: %d\n", ret); > - goto err_del_irqc; > + return ret; > } > > return 0; > - > -err_del_irqc: > - regmap_del_irq_chip(max77686->irq, max77686->irq_data); > - > - return ret; > -} > - > -static int max77686_i2c_remove(struct i2c_client *i2c) > -{ > - struct max77686_dev *max77686 = i2c_get_clientdata(i2c); > - > - mfd_remove_devices(max77686->dev); > - > - regmap_del_irq_chip(max77686->irq, max77686->irq_data); > - > - return 0; > } > > static const struct i2c_device_id max77686_i2c_id[] = { > @@ -317,7 +303,6 @@ static struct i2c_driver max77686_i2c_driver = { > .of_match_table = of_match_ptr(max77686_pmic_dt_match), > }, > .probe = max77686_i2c_probe, > - .remove = max77686_i2c_remove, > .id_table = max77686_i2c_id, > }; >
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@nvidia.com> >>> CC: Chanwoo Choi <cw00.choi@samsung.com> >>> CC: Krzysztof Kozlowski <k.kozlowski@samsung.com> >>> --- >>> 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. :-) 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. * 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. - 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. - 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_. - Reducing code size always better. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@nvidia.com> >>>> CC: Chanwoo Choi <cw00.choi@samsung.com> >>>> CC: Krzysztof Kozlowski <k.kozlowski@samsung.com> >>>> --- >>>> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mfd/max77686.c b/drivers/mfd/max77686.c index 7a0457e..7b68ed7 100644 --- a/drivers/mfd/max77686.c +++ b/drivers/mfd/max77686.c @@ -230,38 +230,24 @@ static int max77686_i2c_probe(struct i2c_client *i2c, return -ENODEV; } - ret = regmap_add_irq_chip(max77686->regmap, max77686->irq, - IRQF_TRIGGER_FALLING | IRQF_ONESHOT | - IRQF_SHARED, 0, irq_chip, - &max77686->irq_data); + ret = devm_regmap_add_irq_chip(&i2c->dev, max77686->regmap, + max77686->irq, + IRQF_TRIGGER_FALLING | IRQF_ONESHOT | + IRQF_SHARED, 0, irq_chip, + &max77686->irq_data); if (ret < 0) { dev_err(&i2c->dev, "failed to add PMIC irq chip: %d\n", ret); return ret; } - ret = mfd_add_devices(max77686->dev, -1, cells, n_devs, NULL, 0, NULL); + ret = devm_mfd_add_devices(max77686->dev, -1, cells, n_devs, NULL, + 0, NULL); if (ret < 0) { dev_err(&i2c->dev, "failed to add MFD devices: %d\n", ret); - goto err_del_irqc; + return ret; } return 0; - -err_del_irqc: - regmap_del_irq_chip(max77686->irq, max77686->irq_data); - - return ret; -} - -static int max77686_i2c_remove(struct i2c_client *i2c) -{ - struct max77686_dev *max77686 = i2c_get_clientdata(i2c); - - mfd_remove_devices(max77686->dev); - - regmap_del_irq_chip(max77686->irq, max77686->irq_data); - - return 0; } static const struct i2c_device_id max77686_i2c_id[] = { @@ -317,7 +303,6 @@ static struct i2c_driver max77686_i2c_driver = { .of_match_table = of_match_ptr(max77686_pmic_dt_match), }, .probe = max77686_i2c_probe, - .remove = max77686_i2c_remove, .id_table = max77686_i2c_id, };
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@nvidia.com> CC: Chanwoo Choi <cw00.choi@samsung.com> CC: Krzysztof Kozlowski <k.kozlowski@samsung.com> --- drivers/mfd/max77686.c | 31 ++++++++----------------------- 1 file changed, 8 insertions(+), 23 deletions(-)