From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1757141AbcBXSVW (ORCPT <rfc822;w@1wt.eu>);
	Wed, 24 Feb 2016 13:21:22 -0500
Received: from mail-pa0-f48.google.com ([209.85.220.48]:35146 "EHLO
	mail-pa0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1753101AbcBXSVU (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Wed, 24 Feb 2016 13:21:20 -0500
Date: Wed, 24 Feb 2016 10:21:16 -0800
From: Bjorn Andersson <bjorn.andersson@linaro.org>
To: Laxman Dewangan <ldewangan@nvidia.com>
Cc: linus.walleij@linaro.org, corbet@lwn.net, lee@kernel.org,
        linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-gpio@vger.kernel.org, linux-rpi-kernel@lists.infradead.org,
        linux-arm-kernel@lists.infradead.org,
        bcm-kernel-feedback-list@broadcom.com,
        linux-mediatek@lists.infradead.org
Subject: Re: [PATCH 01/50] pinctrl: Add devm_ apis for pinctrl_{register,
 unregister}
Message-ID: <20160224182116.GP21240@tuxbot>
References: <1456319775-3216-1-git-send-email-ldewangan@nvidia.com>
 <1456319775-3216-2-git-send-email-ldewangan@nvidia.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1456319775-3216-2-git-send-email-ldewangan@nvidia.com>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wed 24 Feb 05:15 PST 2016, Laxman Dewangan wrote:

> Add device managed APIs devm_pinctrl_register() and
> devm_pinctrl_unregister() for the APIs pinctrl_register()
> and pinctrl_unregister().
> 
> This helps in reducing code in error path and sometimes
> removal of .remove callback for driver unbind.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> ---
>  drivers/pinctrl/core.c          | 69 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/pinctrl/pinctrl.h |  6 ++++
>  2 files changed, 75 insertions(+)
> 
> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
[..]
> +static int devm_pinctrl_dev_match(struct device *dev, void *res, void *data)
> +{
> +	struct pctldev **r = res;
> +
> +	if (!r || !*r) {
> +		WARN_ON(!r || !*r);
> +		return 0;
> +	}

The idiomatic way is to wrap your condition in the WARN_ON; i.e. 

if (WARN_ON(!r || !*r))
	return 0;

> +
> +	return *r == data;
> +}
> +
[..]
> +
> +/**
> + * devm_pinctrl_unregister() - Resource manager of pinctrl_unregister()
> + * @dev: device for which which resource was allocated
> + * @pctldev: the pinctrl device to unregister.
> + */
> +void devm_pinctrl_unregister(struct device *dev, struct pinctrl_dev *pctldev)
> +{
> +	int ret;
> +
> +	ret = devres_release(dev, devm_pinctrl_dev_release,
> +			     devm_pinctrl_dev_match, pctldev);
> +	if (!ret)
> +		WARN_ON(ret);

ret is unlikely to be non-zero in a code path only taken when ret is
zero.

Do either:

WARN_ON(devres_release(...));

rc = devres_release(..);
WARN_ON(rc);

> +}
> +EXPORT_SYMBOL_GPL(devm_pinctrl_unregister);
> +
>  static int __init pinctrl_init(void)
>  {
>  	pr_info("initialized pinctrl subsystem\n");
> diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
> index 9ba59fc..a42e57d 100644
> --- a/include/linux/pinctrl/pinctrl.h
> +++ b/include/linux/pinctrl/pinctrl.h
> @@ -144,6 +144,12 @@ struct pinctrl_desc {
>  extern struct pinctrl_dev *pinctrl_register(struct pinctrl_desc *pctldesc,
>  				struct device *dev, void *driver_data);
>  extern void pinctrl_unregister(struct pinctrl_dev *pctldev);
> +extern struct pinctrl_dev *devm_pinctrl_register(struct device *dev,
> +				struct pinctrl_desc *pctldesc,
> +				void *driver_data);
> +extern void devm_pinctrl_unregister(struct device *dev,
> +				struct pinctrl_dev *pctldev);
> +
>  extern bool pin_is_valid(struct pinctrl_dev *pctldev, int pin);
>  extern void pinctrl_add_gpio_range(struct pinctrl_dev *pctldev,
>  				struct pinctrl_gpio_range *range);

Apart from these style issues you have my ack.

Regards,
Bjorn