From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ray Jui Subject: Re: [PATCH v8 2/4] pinctrl: cygnus: add gpio/pinconf driver Date: Tue, 10 Feb 2015 13:47:37 -0800 Message-ID: <54DA7C79.2010606@broadcom.com> References: <1423070463-24483-1-git-send-email-rjui@broadcom.com> <1423070463-24483-3-git-send-email-rjui@broadcom.com> <20150209191951.GA15709@dtor-ws> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20150209191951.GA15709@dtor-ws> Sender: linux-kernel-owner@vger.kernel.org To: Dmitry Torokhov Cc: Linus Walleij , Alexandre Courbot , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Grant Likely , Christian Daudt , Matt Porter , Florian Fainelli , Russell King , Joe Perches , Arnd Bergmann , Scott Branden , Anatol Pomazau , linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-gpio@vger.kernel.org, bcm-kernel-feedback-list@broadcom.com, devicetree@vger.kernel.org List-Id: linux-gpio@vger.kernel.org On 2/9/2015 11:20 AM, Dmitry Torokhov wrote: > Hi Ray, > > On Wed, Feb 04, 2015 at 09:21:01AM -0800, Ray Jui wrote: >> +static int cygnus_gpio_pinmux_add_range(struct cygnus_gpio *chip) >> +{ >> + struct device_node *node = chip->dev->of_node; >> + struct device_node *pinmux_node; >> + struct platform_device *pinmux_pdev; >> + struct gpio_chip *gc = &chip->gc; >> + int i, ret; >> + >> + /* parse DT to find the phandle to the pinmux controller */ >> + pinmux_node = of_parse_phandle(node, "pinmux", 0); >> + if (!pinmux_node) >> + return -ENODEV; >> + >> + pinmux_pdev = of_find_device_by_node(pinmux_node); >> + if (!pinmux_pdev) { >> + dev_err(chip->dev, "failed to get pinmux device\n"); >> + return -EINVAL; >> + } >> + >> + /* now need to create the mapping between local GPIO and PINMUX pins */ >> + for (i = 0; i < ARRAY_SIZE(cygnus_gpio_pintable); i++) { >> + ret = gpiochip_add_pin_range(gc, dev_name(&pinmux_pdev->dev), >> + cygnus_gpio_pintable[i].offset, >> + cygnus_gpio_pintable[i].pin_base, >> + cygnus_gpio_pintable[i].num_pins); >> + if (ret) { >> + dev_err(chip->dev, "unable to add GPIO pin range\n"); >> + goto err_put_device; >> + } >> + } >> + >> + chip->pinmux_is_supported = true; >> + >> + /* no need for pinmux_pdev device reference anymore */ >> + put_device(&pinmux_pdev->dev); > > Sorry I did not notice it before, but of_parse_phandle() takes reference > to the returned device node, so you need to "put" it here and in error > path as well. Actually you can do: > > int ret = 0; > > pinmux_node = of_parse_phandle(node, "pinmux", 0); > if (!pinmux_node) > return -ENODEV; > > pinmux_pdev = of_find_device_by_node(pinmux_node); > /* We do not longer need pinmux node */ > of_node_put(pinmux_node); > > if (!pinmux_dev) > .... > > for (..) { > ... > if (ret) { > dev_err(...); > break; > } > } > > chip->pinmux_is_supported = (ret == 0); > put_device(..); > return ret; > } > > This way you free resources in the same path. > Thanks. I'll make the change. > ... > >> + >> +static struct platform_driver cygnus_gpio_driver = { >> + .driver = { >> + .name = "cygnus-gpio", >> + .of_match_table = cygnus_gpio_of_match, >> + .suppress_bind_attrs = true, >> + }, >> + .probe = cygnus_gpio_probe, >> +}; >> + >> +static int __init cygnus_gpio_init(void) >> +{ >> + return platform_driver_probe(&cygnus_gpio_driver, cygnus_gpio_probe); > > When I asked you to add ".suppress_bind_attrs = true" I missed the fact > that you were using platform_driver_probe() which already does this > internally. However platform_driver_probe() can't handle deferred > probing, which may or may not be OK. Is there a chance that any of the > resources needed by the driver return -EPROBE_DEFER? If not then it is > safe to continue using platform_driver_probe() and you can drop > suppress_bind_attrs assignment, otherwise it may be better to switch to > platform_driver_register(). > > Thanks. > No I do not expect any resource that this driver depends on to return -EPROBE_DEFER. The IOMUX driver that this driver depends on should be initialized before this driver. I'll drop .suppress_bind_attrs then. Thanks. Ray From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754432AbbBJVry (ORCPT ); Tue, 10 Feb 2015 16:47:54 -0500 Received: from mail-gw3-out.broadcom.com ([216.31.210.64]:7533 "EHLO mail-gw3-out.broadcom.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752370AbbBJVrw (ORCPT ); Tue, 10 Feb 2015 16:47:52 -0500 X-IronPort-AV: E=Sophos;i="5.09,553,1418112000"; d="scan'208";a="56676359" Message-ID: <54DA7C79.2010606@broadcom.com> Date: Tue, 10 Feb 2015 13:47:37 -0800 From: Ray Jui User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: Dmitry Torokhov CC: Linus Walleij , Alexandre Courbot , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , "Grant Likely" , Christian Daudt , Matt Porter , Florian Fainelli , Russell King , Joe Perches , "Arnd Bergmann" , Scott Branden , "Anatol Pomazau" , , , , , Subject: Re: [PATCH v8 2/4] pinctrl: cygnus: add gpio/pinconf driver References: <1423070463-24483-1-git-send-email-rjui@broadcom.com> <1423070463-24483-3-git-send-email-rjui@broadcom.com> <20150209191951.GA15709@dtor-ws> In-Reply-To: <20150209191951.GA15709@dtor-ws> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2/9/2015 11:20 AM, Dmitry Torokhov wrote: > Hi Ray, > > On Wed, Feb 04, 2015 at 09:21:01AM -0800, Ray Jui wrote: >> +static int cygnus_gpio_pinmux_add_range(struct cygnus_gpio *chip) >> +{ >> + struct device_node *node = chip->dev->of_node; >> + struct device_node *pinmux_node; >> + struct platform_device *pinmux_pdev; >> + struct gpio_chip *gc = &chip->gc; >> + int i, ret; >> + >> + /* parse DT to find the phandle to the pinmux controller */ >> + pinmux_node = of_parse_phandle(node, "pinmux", 0); >> + if (!pinmux_node) >> + return -ENODEV; >> + >> + pinmux_pdev = of_find_device_by_node(pinmux_node); >> + if (!pinmux_pdev) { >> + dev_err(chip->dev, "failed to get pinmux device\n"); >> + return -EINVAL; >> + } >> + >> + /* now need to create the mapping between local GPIO and PINMUX pins */ >> + for (i = 0; i < ARRAY_SIZE(cygnus_gpio_pintable); i++) { >> + ret = gpiochip_add_pin_range(gc, dev_name(&pinmux_pdev->dev), >> + cygnus_gpio_pintable[i].offset, >> + cygnus_gpio_pintable[i].pin_base, >> + cygnus_gpio_pintable[i].num_pins); >> + if (ret) { >> + dev_err(chip->dev, "unable to add GPIO pin range\n"); >> + goto err_put_device; >> + } >> + } >> + >> + chip->pinmux_is_supported = true; >> + >> + /* no need for pinmux_pdev device reference anymore */ >> + put_device(&pinmux_pdev->dev); > > Sorry I did not notice it before, but of_parse_phandle() takes reference > to the returned device node, so you need to "put" it here and in error > path as well. Actually you can do: > > int ret = 0; > > pinmux_node = of_parse_phandle(node, "pinmux", 0); > if (!pinmux_node) > return -ENODEV; > > pinmux_pdev = of_find_device_by_node(pinmux_node); > /* We do not longer need pinmux node */ > of_node_put(pinmux_node); > > if (!pinmux_dev) > .... > > for (..) { > ... > if (ret) { > dev_err(...); > break; > } > } > > chip->pinmux_is_supported = (ret == 0); > put_device(..); > return ret; > } > > This way you free resources in the same path. > Thanks. I'll make the change. > ... > >> + >> +static struct platform_driver cygnus_gpio_driver = { >> + .driver = { >> + .name = "cygnus-gpio", >> + .of_match_table = cygnus_gpio_of_match, >> + .suppress_bind_attrs = true, >> + }, >> + .probe = cygnus_gpio_probe, >> +}; >> + >> +static int __init cygnus_gpio_init(void) >> +{ >> + return platform_driver_probe(&cygnus_gpio_driver, cygnus_gpio_probe); > > When I asked you to add ".suppress_bind_attrs = true" I missed the fact > that you were using platform_driver_probe() which already does this > internally. However platform_driver_probe() can't handle deferred > probing, which may or may not be OK. Is there a chance that any of the > resources needed by the driver return -EPROBE_DEFER? If not then it is > safe to continue using platform_driver_probe() and you can drop > suppress_bind_attrs assignment, otherwise it may be better to switch to > platform_driver_register(). > > Thanks. > No I do not expect any resource that this driver depends on to return -EPROBE_DEFER. The IOMUX driver that this driver depends on should be initialized before this driver. I'll drop .suppress_bind_attrs then. Thanks. Ray From mboxrd@z Thu Jan 1 00:00:00 1970 From: rjui@broadcom.com (Ray Jui) Date: Tue, 10 Feb 2015 13:47:37 -0800 Subject: [PATCH v8 2/4] pinctrl: cygnus: add gpio/pinconf driver In-Reply-To: <20150209191951.GA15709@dtor-ws> References: <1423070463-24483-1-git-send-email-rjui@broadcom.com> <1423070463-24483-3-git-send-email-rjui@broadcom.com> <20150209191951.GA15709@dtor-ws> Message-ID: <54DA7C79.2010606@broadcom.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 2/9/2015 11:20 AM, Dmitry Torokhov wrote: > Hi Ray, > > On Wed, Feb 04, 2015 at 09:21:01AM -0800, Ray Jui wrote: >> +static int cygnus_gpio_pinmux_add_range(struct cygnus_gpio *chip) >> +{ >> + struct device_node *node = chip->dev->of_node; >> + struct device_node *pinmux_node; >> + struct platform_device *pinmux_pdev; >> + struct gpio_chip *gc = &chip->gc; >> + int i, ret; >> + >> + /* parse DT to find the phandle to the pinmux controller */ >> + pinmux_node = of_parse_phandle(node, "pinmux", 0); >> + if (!pinmux_node) >> + return -ENODEV; >> + >> + pinmux_pdev = of_find_device_by_node(pinmux_node); >> + if (!pinmux_pdev) { >> + dev_err(chip->dev, "failed to get pinmux device\n"); >> + return -EINVAL; >> + } >> + >> + /* now need to create the mapping between local GPIO and PINMUX pins */ >> + for (i = 0; i < ARRAY_SIZE(cygnus_gpio_pintable); i++) { >> + ret = gpiochip_add_pin_range(gc, dev_name(&pinmux_pdev->dev), >> + cygnus_gpio_pintable[i].offset, >> + cygnus_gpio_pintable[i].pin_base, >> + cygnus_gpio_pintable[i].num_pins); >> + if (ret) { >> + dev_err(chip->dev, "unable to add GPIO pin range\n"); >> + goto err_put_device; >> + } >> + } >> + >> + chip->pinmux_is_supported = true; >> + >> + /* no need for pinmux_pdev device reference anymore */ >> + put_device(&pinmux_pdev->dev); > > Sorry I did not notice it before, but of_parse_phandle() takes reference > to the returned device node, so you need to "put" it here and in error > path as well. Actually you can do: > > int ret = 0; > > pinmux_node = of_parse_phandle(node, "pinmux", 0); > if (!pinmux_node) > return -ENODEV; > > pinmux_pdev = of_find_device_by_node(pinmux_node); > /* We do not longer need pinmux node */ > of_node_put(pinmux_node); > > if (!pinmux_dev) > .... > > for (..) { > ... > if (ret) { > dev_err(...); > break; > } > } > > chip->pinmux_is_supported = (ret == 0); > put_device(..); > return ret; > } > > This way you free resources in the same path. > Thanks. I'll make the change. > ... > >> + >> +static struct platform_driver cygnus_gpio_driver = { >> + .driver = { >> + .name = "cygnus-gpio", >> + .of_match_table = cygnus_gpio_of_match, >> + .suppress_bind_attrs = true, >> + }, >> + .probe = cygnus_gpio_probe, >> +}; >> + >> +static int __init cygnus_gpio_init(void) >> +{ >> + return platform_driver_probe(&cygnus_gpio_driver, cygnus_gpio_probe); > > When I asked you to add ".suppress_bind_attrs = true" I missed the fact > that you were using platform_driver_probe() which already does this > internally. However platform_driver_probe() can't handle deferred > probing, which may or may not be OK. Is there a chance that any of the > resources needed by the driver return -EPROBE_DEFER? If not then it is > safe to continue using platform_driver_probe() and you can drop > suppress_bind_attrs assignment, otherwise it may be better to switch to > platform_driver_register(). > > Thanks. > No I do not expect any resource that this driver depends on to return -EPROBE_DEFER. The IOMUX driver that this driver depends on should be initialized before this driver. I'll drop .suppress_bind_attrs then. Thanks. Ray