From patchwork Fri Nov 23 07:28:08 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dmitry Torokhov X-Patchwork-Id: 13125 Return-Path: X-Original-To: patchwork@peony.canonical.com Delivered-To: patchwork@peony.canonical.com Received: from fiordland.canonical.com (fiordland.canonical.com [91.189.94.145]) by peony.canonical.com (Postfix) with ESMTP id B12D323FC6 for ; Fri, 23 Nov 2012 07:28:14 +0000 (UTC) Received: from mail-ie0-f180.google.com (mail-ie0-f180.google.com [209.85.223.180]) by fiordland.canonical.com (Postfix) with ESMTP id 3A0A5A1858A for ; Fri, 23 Nov 2012 07:28:14 +0000 (UTC) Received: by mail-ie0-f180.google.com with SMTP id c10so1468851ieb.11 for ; Thu, 22 Nov 2012 23:28:13 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=x-forwarded-to:x-forwarded-for:delivered-to:received-spf :dkim-signature:date:from:to:cc:subject:message-id:references :mime-version:content-type:content-disposition:in-reply-to :user-agent:x-gm-message-state; bh=IVGb9Zeko6T7CE/tAJIlk4zo9cTtVlvPOPowU3AL4zA=; b=iATy58kjSwFoAkxW3jjFvKeX7AYSNqWkoioCdDO7hlpKOcZuoGW8fQ0lr9nHrAf4jO ewf2/5B9PvpsEnJwTbXgFYYmMWlm4s3XiicReURWrBRFVRVVDQ4lJce11hfklrC1vQgE te6nO/YaDuJ9eDlzc4xJP4OUFWar9vACvzk8IjUshs5Hc+yGLfXOCP8sl3R1mJuCxxTe ffKV3tHNWM6waMqX1FH8Y5lm3xU6is8PToBgJbICw+6881/gs3D8cEK8WAX6Yl+DIzAK Qut86oH2YAMi63QQkKe/NT1Aw85fPNrHDGZkqOt0JVh5IZUJCU2Q3M9aTnpcI7nxmNZN gd+Q== Received: by 10.50.42.168 with SMTP id p8mr2718150igl.57.1353655693524; Thu, 22 Nov 2012 23:28:13 -0800 (PST) X-Forwarded-To: linaro-patchwork@canonical.com X-Forwarded-For: patch@linaro.org linaro-patchwork@canonical.com Delivered-To: patches@linaro.org Received: by 10.50.67.148 with SMTP id n20csp314176igt; Thu, 22 Nov 2012 23:28:13 -0800 (PST) Received: by 10.68.134.233 with SMTP id pn9mr11929088pbb.125.1353655692528; Thu, 22 Nov 2012 23:28:12 -0800 (PST) Received: from mail-pb0-f41.google.com (mail-pb0-f41.google.com [209.85.160.41]) by mx.google.com with ESMTPS id sh10si7752910pbb.233.2012.11.22.23.28.11 (version=TLSv1/SSLv3 cipher=OTHER); Thu, 22 Nov 2012 23:28:12 -0800 (PST) Received-SPF: pass (google.com: domain of dmitry.torokhov@gmail.com designates 209.85.160.41 as permitted sender) client-ip=209.85.160.41; Authentication-Results: mx.google.com; spf=pass (google.com: domain of dmitry.torokhov@gmail.com designates 209.85.160.41 as permitted sender) smtp.mail=dmitry.torokhov@gmail.com; dkim=pass header.i=@gmail.com Received: by mail-pb0-f41.google.com with SMTP id xa7so6790145pbc.14 for ; Thu, 22 Nov 2012 23:28:11 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=IVGb9Zeko6T7CE/tAJIlk4zo9cTtVlvPOPowU3AL4zA=; b=rteRmDQ/yF+co4tytdrVGnsYD5lPEwKaJpvYsgvBZ1ZCICQ4Y5tU0gYf9yYou5PLzu J9r47U3wb07BWMpVu+dMJvJTVGs95JgICfPaN/6EiwWMp2+JulsEd+g6Kh9GTiUrTqjy nBElJec7fAEoE1u2Ol0XTmQaexkRhIoPSWaxvezVsnHzwGzQxTStgt/j+25DNdzlDeHA iWYQCAbfOPvp/8HtYxKxIk+9Pgi100KR0pMgj4HUZXfp4G/4uBM4wh1/JwwFCSaffVli +DlHz7h4xm1Jf33/l+neVOJY/KRl85bLaIG1HR3eq3IyBtRcB7DCT/71w7f9enJTmduI Ukxg== Received: by 10.68.193.167 with SMTP id hp7mr12015253pbc.124.1353655691723; Thu, 22 Nov 2012 23:28:11 -0800 (PST) Received: from mailhub.coreip.homeip.net (c-67-188-112-76.hsd1.ca.comcast.net. [67.188.112.76]) by mx.google.com with ESMTPS id pv8sm3384075pbc.26.2012.11.22.23.28.09 (version=TLSv1/SSLv3 cipher=OTHER); Thu, 22 Nov 2012 23:28:10 -0800 (PST) Date: Thu, 22 Nov 2012 23:28:08 -0800 From: Dmitry Torokhov To: Sachin Kamat Cc: linux-input@vger.kernel.org, patches@linaro.org Subject: Re: [PATCH 1/1] input: samsung-keypad: Use devm_* functions Message-ID: <20121123072808.GC12631@core.coreip.homeip.net> References: <1352980598-6305-1-git-send-email-sachin.kamat@linaro.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1352980598-6305-1-git-send-email-sachin.kamat@linaro.org> User-Agent: Mutt/1.5.21 (2010-09-15) X-Gm-Message-State: ALoCoQnVd908WRr/smQJTCIOk8qtyvShdyLkBVieJsPPyH8XKlld8LJ0wORwR8r0DU1iFUOP58zu Hi Sachin, On Thu, Nov 15, 2012 at 05:26:38PM +0530, Sachin Kamat wrote: > > - keypad->base = ioremap(res->start, resource_size(res)); > - if (!keypad->base) { > - error = -EBUSY; > - goto err_free_mem; > - } > + keypad->base = devm_ioremap(&pdev->dev, res->start, resource_size(res)); Question: why don't you request this region? Is it done in platform code? > + if (!keypad->base) > + return -EBUSY; > > - keypad->clk = clk_get(&pdev->dev, "keypad"); > + keypad->clk = devm_clk_get(&pdev->dev, "keypad"); > if (IS_ERR(keypad->clk)) { > dev_err(&pdev->dev, "failed to get keypad clk\n"); > - error = PTR_ERR(keypad->clk); > - goto err_unmap_base; > + return PTR_ERR(keypad->clk); > } > > error = clk_prepare(keypad->clk); > if (error) { > dev_err(&pdev->dev, "keypad clock prepare failed\n"); > - goto err_put_clk; > + goto err_dt_gpio_free; Why are you trying to free GPIOs here, you have not parsed them yet. This should be just "return error;". > } > > keypad->input_dev = input_dev; > @@ -479,14 +472,15 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev) > keypad->irq = platform_get_irq(pdev, 0); > if (keypad->irq < 0) { > error = keypad->irq; > - goto err_put_clk; > + goto err_dt_gpio_free; No, this should do clk_unprepare(). > } > > - error = request_threaded_irq(keypad->irq, NULL, samsung_keypad_irq, > - IRQF_ONESHOT, dev_name(&pdev->dev), keypad); > + error = devm_request_threaded_irq(&pdev->dev, keypad->irq, NULL, > + samsung_keypad_irq, IRQF_ONESHOT, > + dev_name(&pdev->dev), keypad); > if (error) { > dev_err(&pdev->dev, "failed to register keypad interrupt\n"); > - goto err_put_clk; > + goto err_dt_gpio_free; Same here. Also, if you are converting to devm_* you do not need samsung_keypad_dt_gpio_free() as you can allocate gpios with devm_reguats_gpio(). Does the version of the patch below still work for you? Thanks. diff --git a/drivers/input/keyboard/samsung-keypad.c b/drivers/input/keyboard/samsung-keypad.c index 8b0b361..2b7cbdb 100644 --- a/drivers/input/keyboard/samsung-keypad.c +++ b/drivers/input/keyboard/samsung-keypad.c @@ -313,21 +313,22 @@ static void samsung_keypad_parse_dt_gpio(struct device *dev, struct samsung_keypad *keypad) { struct device_node *np = dev->of_node; - int gpio, ret, row, col; + int gpio, error, row, col; for (row = 0; row < keypad->rows; row++) { gpio = of_get_named_gpio(np, "row-gpios", row); keypad->row_gpios[row] = gpio; if (!gpio_is_valid(gpio)) { dev_err(dev, "keypad row[%d]: invalid gpio %d\n", - row, gpio); + row, gpio); continue; } - ret = gpio_request(gpio, "keypad-row"); - if (ret) - dev_err(dev, "keypad row[%d] gpio request failed\n", - row); + error = devm_gpio_request(dev, gpio, "keypad-row"); + if (error) + dev_err(dev, + "keypad row[%d] gpio request failed: %d\n", + rowi, error); } for (col = 0; col < keypad->cols; col++) { @@ -335,39 +336,23 @@ static void samsung_keypad_parse_dt_gpio(struct device *dev, keypad->col_gpios[col] = gpio; if (!gpio_is_valid(gpio)) { dev_err(dev, "keypad column[%d]: invalid gpio %d\n", - col, gpio); + col, gpio); continue; } - ret = gpio_request(gpio, "keypad-col"); - if (ret) - dev_err(dev, "keypad column[%d] gpio request failed\n", - col); + error = devm_gpio_request(dev, gpio, "keypad-col"); + if (error) + dev_err(dev, + "keypad column[%d] gpio request failed: %d\n", + col, error); } } - -static void samsung_keypad_dt_gpio_free(struct samsung_keypad *keypad) -{ - int cnt; - - for (cnt = 0; cnt < keypad->rows; cnt++) - if (gpio_is_valid(keypad->row_gpios[cnt])) - gpio_free(keypad->row_gpios[cnt]); - - for (cnt = 0; cnt < keypad->cols; cnt++) - if (gpio_is_valid(keypad->col_gpios[cnt])) - gpio_free(keypad->col_gpios[cnt]); -} #else static struct samsung_keypad_platdata *samsung_keypad_parse_dt(struct device *dev) { return NULL; } - -static void samsung_keypad_dt_gpio_free(struct samsung_keypad *keypad) -{ -} #endif static int __devinit samsung_keypad_probe(struct platform_device *pdev) @@ -408,36 +393,29 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev) row_shift = get_count_order(pdata->cols); keymap_size = (pdata->rows << row_shift) * sizeof(keypad->keycodes[0]); - keypad = kzalloc(sizeof(*keypad) + keymap_size, GFP_KERNEL); - input_dev = input_allocate_device(); - if (!keypad || !input_dev) { - error = -ENOMEM; - goto err_free_mem; - } + keypad = devm_kzalloc(&pdev->dev, sizeof(*keypad) + keymap_size, GFP_KERNEL); + input_dev = devm_input_allocate_device(&pdev->dev); + if (!keypad || !input_dev) + return -ENOMEM; res = platform_get_resource(pdev, IORESOURCE_MEM, 0); - if (!res) { - error = -ENODEV; - goto err_free_mem; - } + if (!res) + return -ENODEV; - keypad->base = ioremap(res->start, resource_size(res)); - if (!keypad->base) { - error = -EBUSY; - goto err_free_mem; - } + keypad->base = devm_ioremap(&pdev->dev, res->start, resource_size(res)); + if (!keypad->base) + return -EBUSY; - keypad->clk = clk_get(&pdev->dev, "keypad"); + keypad->clk = devm_clk_get(&pdev->dev, "keypad"); if (IS_ERR(keypad->clk)) { dev_err(&pdev->dev, "failed to get keypad clk\n"); - error = PTR_ERR(keypad->clk); - goto err_unmap_base; + return PTR_ERR(keypad->clk); } error = clk_prepare(keypad->clk); if (error) { dev_err(&pdev->dev, "keypad clock prepare failed\n"); - goto err_put_clk; + return error; } keypad->input_dev = input_dev; @@ -482,14 +460,15 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev) keypad->irq = platform_get_irq(pdev, 0); if (keypad->irq < 0) { error = keypad->irq; - goto err_put_clk; + goto err_unprepare_clk; } - error = request_threaded_irq(keypad->irq, NULL, samsung_keypad_irq, - IRQF_ONESHOT, dev_name(&pdev->dev), keypad); + error = devm_request_threaded_irq(&pdev->dev, keypad->irq, NULL, + samsung_keypad_irq, IRQF_ONESHOT, + dev_name(&pdev->dev), keypad); if (error) { dev_err(&pdev->dev, "failed to register keypad interrupt\n"); - goto err_put_clk; + goto err_unprepare_clk; } device_init_wakeup(&pdev->dev, pdata->wakeup); @@ -498,7 +477,7 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev) error = input_register_device(keypad->input_dev); if (error) - goto err_free_irq; + goto err_disable_runtime_pm; if (pdev->dev.of_node) { devm_kfree(&pdev->dev, (void *)pdata->keymap_data->keymap); @@ -507,22 +486,12 @@ static int __devinit samsung_keypad_probe(struct platform_device *pdev) } return 0; -err_free_irq: - free_irq(keypad->irq, keypad); +err_disable_runtime_pm: pm_runtime_disable(&pdev->dev); device_init_wakeup(&pdev->dev, 0); platform_set_drvdata(pdev, NULL); err_unprepare_clk: clk_unprepare(keypad->clk); -err_put_clk: - clk_put(keypad->clk); - samsung_keypad_dt_gpio_free(keypad); -err_unmap_base: - iounmap(keypad->base); -err_free_mem: - input_free_device(input_dev); - kfree(keypad); - return error; } @@ -536,18 +505,7 @@ static int __devexit samsung_keypad_remove(struct platform_device *pdev) input_unregister_device(keypad->input_dev); - /* - * It is safe to free IRQ after unregistering device because - * samsung_keypad_close will shut off interrupts. - */ - free_irq(keypad->irq, keypad); - clk_unprepare(keypad->clk); - clk_put(keypad->clk); - samsung_keypad_dt_gpio_free(keypad); - - iounmap(keypad->base); - kfree(keypad); return 0; }