From patchwork Thu Mar 1 16:02:17 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Fabio Estevam X-Patchwork-Id: 47520 Received: from vger.kernel.org ([209.132.180.67]) by www.linuxtv.org with esmtp (Exim 4.84_2) (envelope-from ) id 1erQfD-0006LY-NB; Thu, 01 Mar 2018 16:02:36 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1031808AbeCAQCT (ORCPT + 1 other); Thu, 1 Mar 2018 11:02:19 -0500 Received: from mail-oi0-f66.google.com ([209.85.218.66]:37587 "EHLO mail-oi0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1031763AbeCAQCS (ORCPT ); Thu, 1 Mar 2018 11:02:18 -0500 Received: by mail-oi0-f66.google.com with SMTP id f186so4839169oig.4; Thu, 01 Mar 2018 08:02:18 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=wMzEW0hw1kJTlLlwsM4+3KqtnhApgaxMTGL62zhI3zE=; b=hsui5btBmLPc/m4k9g3xojIs5XGxfPHeE7XxyX3TzrARMUAB6I4izEP8+shkWq+uZE HPHCRBBJbMK+AnFCzhM8UZffArFup+ZROIVSmudAdQRIuf2pSH3zdAElzjLsanZJw32m mMuWtm6IldQBFNi0dfO7ApnHXo1IqwQFu9T4/echCd9s7jd4gLLafZTJwRI92lRnNcJz iDzI4Jy3iTccZpzR57Q0/ISArEVBZsOBt/kY6MkKu9HqN+1PFGvw5wPzXSm2e99yA42n NpgTrTA3T/tIDvVXL0nVqpK9eE0p525KUCC6NqVd2VxHkDCX48stdPmO8AFScqA+SJ9f lAUw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=wMzEW0hw1kJTlLlwsM4+3KqtnhApgaxMTGL62zhI3zE=; b=QiULr/37rQS2uDVFYXwd5u+MlL4uqwnTcbgOG0dBlVvG6KKx5UN7C6Xm0H8dqOxdaa CbvDtFC9Af2C2OPVEdyjnJocQZBI5LGv+JyGZydDGjopz8YMqTvu+RSHOQPTZdPodjSA XJU76u8rdEOVGZcac6zNK4PC+tLD54wnhK1eF5hMxtfitzzdvRHTo4fa2329EJ7Vmg3P 34s6m/PDi57C6ux3+iY/+BmCyW+l28akL2GFyYJMc0Ti4xMWctpqO/NBi+Zh69ewmiLJ vSa95rQf/qvm2CnRA2PHu9Do6z998Lrl45MkTto0G8Z43gRqbi3SZHp3Gca1faEreheR nLYA== X-Gm-Message-State: APf1xPDz92CnsuS7ApzxHGlizMRTWNdAaP3KOWCTZW8FSJw0ux6Ca3pp 2KDEsxPCJfDFj00PBVTZM+qy/V4ipUYlCV3td6E= X-Google-Smtp-Source: AG47ELtLThmUiNd/bpbhKpqMjTa0kr6rHWmIO1KLg09KuuRsoz+/fxmNt/8++QDIhRSh81+fo37MrGCGKS4QhxmStZg= X-Received: by 10.202.16.10 with SMTP id 10mr1627521oiq.69.1519920137965; Thu, 01 Mar 2018 08:02:17 -0800 (PST) MIME-Version: 1.0 Received: by 10.157.5.137 with HTTP; Thu, 1 Mar 2018 08:02:17 -0800 (PST) In-Reply-To: <20180301040939.GA13274@embeddedgus> References: <20180301040939.GA13274@embeddedgus> From: Fabio Estevam Date: Thu, 1 Mar 2018 13:02:17 -0300 Message-ID: Subject: Re: [PATCH] staging/imx: Fix inconsistent IS_ERR and PTR_ERR To: "Gustavo A. R. Silva" Cc: Steve Longerbeam , Philipp Zabel , Mauro Carvalho Chehab , Greg Kroah-Hartman , linux-media , devel@driverdev.osuosl.org, linux-kernel , "Gustavo A. R. Silva" Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org On Thu, Mar 1, 2018 at 1:09 AM, Gustavo A. R. Silva wrote: > Fix inconsistent IS_ERR and PTR_ERR in imx_csi_probe. > The proper pointer to be passed as argument is pinctrl > instead of priv->vdev. > > This issue was detected with the help of Coccinelle. > > Fixes: 52e17089d185 ("media: imx: Don't initialize vars that won't be used") > Signed-off-by: Gustavo A. R. Silva > --- > drivers/staging/media/imx/imx-media-csi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c > index 5a195f8..4f290a0 100644 > --- a/drivers/staging/media/imx/imx-media-csi.c > +++ b/drivers/staging/media/imx/imx-media-csi.c > @@ -1798,7 +1798,7 @@ static int imx_csi_probe(struct platform_device *pdev) > priv->dev->of_node = pdata->of_node; > pinctrl = devm_pinctrl_get_select_default(priv->dev); > if (IS_ERR(pinctrl)) { > - ret = PTR_ERR(priv->vdev); > + ret = PTR_ERR(pinctrl); > goto free; This patch is correct, but now I am seeing that devm_pinctrl_get_select_default() always fails. I added this debug line: @@ -1789,19 +1787,7 @@ static int imx_csi_probe(struct platform_device *pdev) v4l2_ctrl_handler_init(&priv->ctrl_hdlr, 0); priv->sd.ctrl_handler = &priv->ctrl_hdlr; - /* - * The IPUv3 driver did not assign an of_node to this - * device. As a result, pinctrl does not automatically - * configure our pin groups, so we need to do that manually - * here, after setting this device's of_node. - */ priv->dev->of_node = pdata->of_node; - pinctrl = devm_pinctrl_get_select_default(priv->dev); - if (IS_ERR(pinctrl)) { - ret = PTR_ERR(pinctrl); - goto free; - } - ret = v4l2_async_register_subdev(&priv->sd); if (ret) goto free; --- a/drivers/staging/media/imx/imx-media-csi.c +++ b/drivers/staging/media/imx/imx-media-csi.c @@ -1799,6 +1799,7 @@ static int imx_csi_probe(struct platform_device *pdev) pinctrl = devm_pinctrl_get_select_default(priv->dev); if (IS_ERR(pinctrl)) { ret = PTR_ERR(pinctrl); + pr_err("************ pinctrl failed\n"); goto free; } and this is what I get in imx6q-sabresd: [ 3.453905] imx-media: subdev ipu1_vdic bound [ 3.458601] imx-media: subdev ipu2_vdic bound [ 3.463341] imx-media: subdev ipu1_ic_prp bound [ 3.468924] ipu1_ic_prpenc: Registered ipu1_ic_prpenc capture as /dev/video0 [ 3.476237] imx-media: subdev ipu1_ic_prpenc bound [ 3.481621] ipu1_ic_prpvf: Registered ipu1_ic_prpvf capture as /dev/video1 [ 3.488805] imx-media: subdev ipu1_ic_prpvf bound [ 3.493659] imx-media: subdev ipu2_ic_prp bound [ 3.498839] ipu2_ic_prpenc: Registered ipu2_ic_prpenc capture as /dev/video2 [ 3.505958] imx-media: subdev ipu2_ic_prpenc bound [ 3.511318] ipu2_ic_prpvf: Registered ipu2_ic_prpvf capture as /dev/video3 [ 3.518335] imx-media: subdev ipu2_ic_prpvf bound [ 3.524622] ipu1_csi0: Registered ipu1_csi0 capture as /dev/video4 [ 3.530902] imx-media: subdev ipu1_csi0 bound [ 3.535453] ************ pinctrl failed [ 3.539684] ************ pinctrl failed [ 3.543677] ************ pinctrl failed [ 3.548278] imx-media: subdev imx6-mipi-csi2 bound So imx_csi_probe() does not succeed anymore since devm_pinctrl_get_select_default() always fails. Not sure I understand the comments that explain the need for pinctrl handling inside the driver. Can't we just get rid of it like this? --- a/drivers/staging/media/imx/imx-media-csi.c +++ b/drivers/staging/media/imx/imx-media-csi.c @@ -14,7 +14,6 @@ #include #include #include -#include #include #include #include @@ -1739,7 +1738,6 @@ static const struct v4l2_subdev_internal_ops csi_internal_ops = { static int imx_csi_probe(struct platform_device *pdev) { struct ipu_client_platformdata *pdata; - struct pinctrl *pinctrl; struct csi_priv *priv; int ret;