Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 8 Jul 2020 17:00:55 +0800
From:   Xin Ji <xji@analogixsemi.com>
To:     Nicolas Boichat <drinkcat@chromium.org>
Cc:     devel@driverdev.osuosl.org,
        Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
        Hsin-Yi Wang <hsinyi@chromium.org>,
        Dan Carpenter <dan.carpenter@oracle.com>,
        Andrzej Hajda <a.hajda@samsung.com>,
        Neil Armstrong <narmstrong@baylibre.com>,
        Jonas Karlman <jonas@kwiboo.se>,
        Jernej Skrabec <jernej.skrabec@siol.net>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>, linux-kernel@vger.kernel.org,
        dri-devel@lists.freedesktop.org,
        Pi-Hsun Shih <pihsun@chromium.org>,
        Sheng Pan <span@analogixsemi.com>
Subject: Re: [PATCH v13 2/2] drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to
 DP
Message-ID: <20200708090055.GA21256@xin-VirtualBox>
References: <cover.1591345160.git.xji@analogixsemi.com>
 <a18316c118c7b8ac201911b0b96d41c84653a00f.1591345160.git.xji@analogixsemi.com>
 <CANMq1KDnoLSUxxYr82o=1eGBR7E3PxoYUr8h1sEVTyqYyHCC-Q@mail.gmail.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CANMq1KDnoLSUxxYr82o=1eGBR7E3PxoYUr8h1sEVTyqYyHCC-Q@mail.gmail.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
MIME-Version: 1.0
X-MS-Exchange-MessageSentRepresentingType: 1
Received: from xin-VirtualBox (114.247.245.254) by HK2PR04CA0050.apcprd04.prod.outlook.com (2603:1096:202:14::18) with Microsoft SMTP Server (version=TLS1_0, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA) id 15.20.3174.21 via Frontend Transport; Wed, 8 Jul 2020 09:01:03 +0000
X-Originating-IP: [114.247.245.254]
X-MS-PublicTrafficType: Email
X-MS-Office365-Filtering-Correlation-Id: 2b4f2af2-59cf-4d7a-014a-08d8231d71c7
X-MS-TrafficTypeDiagnostic: BYAPR04MB6150:
X-MS-Exchange-Transport-Forked: True
X-Microsoft-Antispam-PRVS: <BYAPR04MB6150A52AE3DFF8CBE7C2954EC7670@BYAPR04MB6150.namprd04.prod.outlook.com>
X-MS-Oob-TLC-OOBClassifiers: OLM:4941;
X-Forefront-PRVS: 04583CED1A
X-MS-Exchange-SenderADCheck: 1
X-Microsoft-Antispam: BCL:0;
X-Microsoft-Antispam-Message-Info: IGe+cPMhHPGUK9qLwGgH9XH6BVrjIlL+TAQjk232qTRlbvgarpQzHFVuBDGbMd8bz229RmkSIAu7xIjDizIgKP5IxzjULzqCfaiXvneIRHqwOIWiPcec99Ll4QXCWiIlAfB3Hubgvm4Ouul7aSxUFKIG3IebNK+w9zEUCgk1Wf/1Swxl5oroFEDDytXaUrQY1et5hGXGcFblCC1zOvKlqOpu0kvZHK8SEwbonTq4zk7tG8Vul/dVXtFlYNsl9st6WL94ZAVLZrLr9Uz9pIWhoBGswCvChm3nr1gLDYaf9FSkybYBDhZIfXDKFZXQvbxVEkLHXOXlX9cCgl3gFyMPHg==
X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:BY5PR04MB6739.namprd04.prod.outlook.com;PTR:;CAT:NONE;SFTY:;SFS:(396003)(366004)(39840400004)(376002)(136003)(346002)(107886003)(478600001)(26005)(54906003)(186003)(16526019)(6496006)(52116002)(8676002)(4326008)(66946007)(66476007)(66556008)(7416002)(83380400001)(9686003)(33656002)(86362001)(33716001)(53546011)(316002)(1076003)(956004)(6916009)(5660300002)(2906002)(55016002)(6666004)(8936002);DIR:OUT;SFP:1102;
X-MS-Exchange-AntiSpam-MessageData: 2kijO3D/ojoPCbtqnEjVgoBnXfciXJsKZS90n1HrWYhLQb8ERkhM3qushxjj9440lyOj7/Ow6XUsnp0vJFR7d8Z8czWXs29kEaZTD7XTTw0lXQlc29qcftCSTRbrPNrJhLt9zd4JxYCX+/jUC017Av5sYCjbhI45uHM4J9iduth8qh57e2IpOAcyT6rlImZWOdOkxo5A0IsVwoD2DujvB2pbKh72jyB3AjMBPJtV6Gu0AChWTTAugOBdftfAd5Uc0hMJ6EOUnJSk5bAg18lOvQWqnL0MpwcXFIuvSZAbxnAfrTGhA3VGn4ZqoSi4SMNFBsN6EN35YDjas54RUIIsW+jMpBMyHSUqYUq99QVznjO7f37V9sjBhugJbUmNL1Ebv+u5TZVyBS7wIzfl6nwKqoPDWTZTCqWkZ0BW6fNvH9ni7ReP5sD6VQOBMWqNu72sI3cT69H3djo7rJhsIdcOqng75sBJshdKqm71VBXZttiZPyUsJjsSkQVEmYRlF4zJ
X-OriginatorOrg: analogixsemi.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2b4f2af2-59cf-4d7a-014a-08d8231d71c7
X-MS-Exchange-CrossTenant-AuthSource: BY5PR04MB6739.namprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Jul 2020 09:01:04.6520
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: b099b0b4-f26c-4cf5-9a0f-d5be9acab205
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Am/QYnQSOUVEyos+VV7DPcX4FUv1yLzypaxF8A3fOfWbkeCdOEieBXmstyvbW6HNWZSaeav6gw+ypsk2/TEcRQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR04MB6150
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, Jul 07, 2020 at 03:01:25PM +0800, Nicolas Boichat wrote:

Hi Nicolas, thanks for the replay.

> On Tue, Jun 9, 2020 at 3:20 PM Xin Ji <xji@analogixsemi.com> wrote:
> >
> > The ANX7625 is an ultra-low power 4K Mobile HD Transmitter designed
> > for portable device. It converts MIPI DSI/DPI to DisplayPort 1.3 4K.
> >
> > Signed-off-by: Xin Ji <xji@analogixsemi.com>
> > ---
> >  drivers/gpu/drm/bridge/analogix/Kconfig   |    9 +
> >  drivers/gpu/drm/bridge/analogix/Makefile  |    1 +
> >  drivers/gpu/drm/bridge/analogix/anx7625.c | 1999 +++++++++++++++++++++++++++++
> >  drivers/gpu/drm/bridge/analogix/anx7625.h |  397 ++++++
> >  4 files changed, 2406 insertions(+)
> >  create mode 100644 drivers/gpu/drm/bridge/analogix/anx7625.c
> >  create mode 100644 drivers/gpu/drm/bridge/analogix/anx7625.h
> >
> > [snip]
> > +static int anx7625_parse_dt(struct device *dev,
> > +                           struct anx7625_platform_data *pdata)
> > +{
> > +       struct device_node *np = dev->of_node;
> > +       struct device_node *panel_node, *out_ep;
> > +
> > +       pdata->node.mipi_dsi_host_node = of_graph_get_remote_node(np, 0, 0);
> > +       if (!pdata->node.mipi_dsi_host_node) {
> > +               DRM_DEV_ERROR(dev, "fail to get internal panel.\n");
> > +               return -EPROBE_DEFER;
> 
> This does not look correct. I don't think of_graph_get_remote_node
> will ever return NULL if the device tree is configured properly, and
> it's useless to retry later (EPROBE_DEFER). You should just fail (e.g.
> return EINVAL).
OK
> 
> > +       }
> > +
> > +       of_node_put(pdata->node.mipi_dsi_host_node);
> 
> You are using pdata->node.mipi_dsi_host_node in other places in the
> code, so I don't think it's ok to call of_node_put?
I'll move the related code to here.
> 
> > +       DRM_DEV_DEBUG_DRIVER(dev, "found dsi host node.\n");
> > +
> > +       pdata->node.panel_node = of_graph_get_port_by_id(np, 1);
> > +       if (!pdata->node.panel_node) {
> > +               DRM_DEV_ERROR(dev, "fail to get panel node.\n");
> > +               return -EPROBE_DEFER;
> 
> -EINVAL.
OK
> 
> > +       }
> > +
> > +       of_node_put(pdata->node.panel_node);
> > +       out_ep = of_get_child_by_name(pdata->node.panel_node,
> > +                                     "endpoint");
> > +       if (!out_ep) {
> > +               DRM_DEV_DEBUG_DRIVER(dev, "cannot get endpoint.\n");
> 
> DRM_DEV_ERROR seems more appropriate
OK, also I'll remove drm_panel based on Sam comment.
> 
> > +               return -EPROBE_DEFER;
> 
> -EINVAL
OK
> 
> > +       }
> > +
> > +       panel_node = of_graph_get_remote_port_parent(out_ep);
> > +       of_node_put(out_ep);
> > +       pdata->panel = of_drm_find_panel(panel_node);
> > +       DRM_DEV_DEBUG_DRIVER(dev, "get panel node.\n");
> > +
> > +       of_node_put(panel_node);
> > +       if (IS_ERR_OR_NULL(pdata->panel))
> > +               return -EPROBE_DEFER;
> 
> of_drm_find_panel cannot return NULL, so, do this instead:
> 
> if (IS_ERR(pdata->panel))
>    return PTR_ERR(pdata->panel);
> 
> (which actually _may_ return EPROBE_DEFER)
I'll remove drm_panel, use panel_bridge.
> 
> > +
> > +       return 0;
> > +}
> > [snip]
> > +static int anx7625_i2c_probe(struct i2c_client *client,
> > +                            const struct i2c_device_id *id)
> > +{
> > +       struct anx7625_data *platform;
> > +       struct anx7625_platform_data *pdata;
> > +       int ret = 0;
> > +       struct device *dev = &client->dev;
> > +
> > +       if (!i2c_check_functionality(client->adapter,
> > +                                    I2C_FUNC_SMBUS_I2C_BLOCK)) {
> > +               DRM_DEV_ERROR(dev, "anx7625's i2c bus doesn't support\n");
> > +               return -ENODEV;
> > +       }
> > +
> > +       platform = kzalloc(sizeof(*platform), GFP_KERNEL);
> > +       if (!platform) {
> > +               DRM_DEV_ERROR(dev, "fail to allocate driver data\n");
> > +               return -ENOMEM;
> > +       }
> > +
> > +       pdata = &platform->pdata;
> > +
> > +       ret = anx7625_parse_dt(dev, pdata);
> > +       if (ret) {
> > +               DRM_DEV_ERROR(dev, "fail to parse devicetree.\n");
> 
> Please do not print this error (or at least not if err == -EPROBE_DEFER).
OK
> 
> > +               goto free_platform;
> > +       }