Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933149Ab3E1Gnc (ORCPT ); Tue, 28 May 2013 02:43:32 -0400 Received: from szxga01-in.huawei.com ([119.145.14.64]:4712 "EHLO szxga01-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932977Ab3E1Gna (ORCPT ); Tue, 28 May 2013 02:43:30 -0400 Message-ID: <51A451CE.8070801@huawei.com> Date: Tue, 28 May 2013 14:42:22 +0800 From: Libo Chen User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:17.0) Gecko/20130509 Thunderbird/17.0.6 MIME-Version: 1.0 To: Sachin Kamat CC: Libo Chen , , , , , , , Subject: Re: [PATCH RFC v3 1/3] i2c: i2c-bfin-twi: convert to devm_* API References: <1369310405-21112-1-git-send-email-libo.chen@huawei.com> In-Reply-To: Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.135.72.158] X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4066 Lines: 118 thanks for your review. I will update later. Libo On 2013/5/24 11:19, Sachin Kamat wrote: > On 23 May 2013 17:30, Libo Chen wrote: >> peripheral_request_list has got free if any one faild, so no need to free again in err case. >> aovid this, convert them to devm_* API >> >> Signed-off-by: Libo Chen > > It is a good practice to include changelog while submitting revised > versions of the patch. > >> --- >> drivers/i2c/busses/i2c-bfin-twi.c | 38 +++++++++--------------------------- >> 1 files changed, 10 insertions(+), 28 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-bfin-twi.c b/drivers/i2c/busses/i2c-bfin-twi.c >> index 05080c4..2b99c48 100644 >> --- a/drivers/i2c/busses/i2c-bfin-twi.c >> +++ b/drivers/i2c/busses/i2c-bfin-twi.c >> @@ -621,35 +621,27 @@ static int i2c_bfin_twi_probe(struct platform_device *pdev) >> int rc; >> unsigned int clkhilow; >> >> - iface = kzalloc(sizeof(struct bfin_twi_iface), GFP_KERNEL); >> + iface = devm_kzalloc(&pdev->dev, sizeof(struct bfin_twi_iface), >> + GFP_KERNEL); >> if (!iface) { >> dev_err(&pdev->dev, "Cannot allocate memory\n"); > > This error message is not really required. > >> - rc = -ENOMEM; >> - goto out_error_nomem; >> + return -ENOMEM; >> } >> >> spin_lock_init(&(iface->lock)); >> >> /* Find and map our resources */ >> res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> - if (res == NULL) { >> - dev_err(&pdev->dev, "Cannot get IORESOURCE_MEM\n"); >> - rc = -ENOENT; >> - goto out_error_get_res; >> - } >> - >> - iface->regs_base = ioremap(res->start, resource_size(res)); >> - if (iface->regs_base == NULL) { >> + iface->regs_base = devm_ioremap_resource(&pdev->dev, res); >> + if (IS_ERR(iface->regs_base)) { >> dev_err(&pdev->dev, "Cannot map IO\n"); > > devm_ioremap_resource prints out the error messages. So the above line > is redundant. > >> - rc = -ENXIO; >> - goto out_error_ioremap; >> + return -ENXIO; > > Why not return PTR_ERR(iface->regs_base)? > >> } >> >> iface->irq = platform_get_irq(pdev, 0); >> if (iface->irq < 0) { >> dev_err(&pdev->dev, "No IRQ specified\n"); >> - rc = -ENOENT; >> - goto out_error_no_irq; >> + return -ENOENT; >> } >> >> p_adap = &iface->adap; >> @@ -666,10 +658,10 @@ static int i2c_bfin_twi_probe(struct platform_device *pdev) >> "i2c-bfin-twi"); >> if (rc) { >> dev_err(&pdev->dev, "Can't setup pin mux!\n"); >> - goto out_error_pin_mux; >> + return rc; >> } >> >> - rc = request_irq(iface->irq, bfin_twi_interrupt_entry, >> + rc = devm_request_irq(&pdev->dev, iface->irq, bfin_twi_interrupt_entry, >> 0, pdev->name, iface); >> if (rc) { >> dev_err(&pdev->dev, "Can't get IRQ %d !\n", iface->irq); >> @@ -707,16 +699,9 @@ static int i2c_bfin_twi_probe(struct platform_device *pdev) >> return 0; >> >> out_error_add_adapter: >> - free_irq(iface->irq, iface); >> out_error_req_irq: >> -out_error_no_irq: >> peripheral_free_list((unsigned short *)pdev->dev.platform_data); >> -out_error_pin_mux: >> - iounmap(iface->regs_base); >> -out_error_ioremap: >> -out_error_get_res: >> - kfree(iface); >> -out_error_nomem: >> + > > Empty line not required. > > You could probably combine all the empty labels into one meaningful label above. > >> return rc; > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/