From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.7 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id C95A3C433DF for ; Thu, 20 Aug 2020 14:48:28 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id AE913207DE for ; Thu, 20 Aug 2020 14:48:28 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728412AbgHTOsY (ORCPT ); Thu, 20 Aug 2020 10:48:24 -0400 Received: from asavdk3.altibox.net ([109.247.116.14]:58406 "EHLO asavdk3.altibox.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726792AbgHTOsR (ORCPT ); Thu, 20 Aug 2020 10:48:17 -0400 Received: from ravnborg.org (unknown [188.228.123.71]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by asavdk3.altibox.net (Postfix) with ESMTPS id B6D2820026; Thu, 20 Aug 2020 16:48:09 +0200 (CEST) Date: Thu, 20 Aug 2020 16:48:08 +0200 From: Sam Ravnborg To: Mauro Carvalho Chehab Cc: Greg Kroah-Hartman , Neil Armstrong , Xinliang Liu , Wanchun Zheng , linuxarm@huawei.com, dri-devel , Andrzej Hajda , Laurent Pinchart , devel@driverdev.osuosl.org, Daniel Borkmann , John Fastabend , Xiubin Zhang , Wei Xu , David Airlie , Xinwei Kong , Tomi Valkeinen , Bogdan Togorean , Laurentiu Palcu , linux-media@vger.kernel.org, devicetree@vger.kernel.org, Liwei Cai , Jesper Dangaard Brouer , Manivannan Sadhasivam , Chen Feng , Alexei Starovoitov , linaro-mm-sig@lists.linaro.org, Rob Herring , Jakub Kicinski , mauro.chehab@huawei.com, Rob Clark , linux-arm-kernel@lists.infradead.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Liuyao An , Rongrong Zou , bpf@vger.kernel.org, "David S. Miller" Subject: Re: [PATCH 00/49] DRM driver for Hikey 970 Message-ID: <20200820144808.GA186324@ravnborg.org> References: <20200819152120.GA106437@ravnborg.org> <20200819174027.70b39ee9@coco.lan> <20200819173558.GA3733@ravnborg.org> <20200820160649.54741194@coco.lan> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200820160649.54741194@coco.lan> X-CMAE-Score: 0 X-CMAE-Analysis: v=2.3 cv=f+hm+t6M c=1 sm=1 tr=0 a=S6zTFyMACwkrwXSdXUNehg==:117 a=S6zTFyMACwkrwXSdXUNehg==:17 a=kj9zAlcOel0A:10 a=7gkXJVJtAAAA:8 a=FQPn5FuWFEp6BKnZ598A:9 a=CjuIK1q_8ugA:10 a=E9Po1WZjFZOl8hwRPBS3:22 Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Mauro. On Thu, Aug 20, 2020 at 04:06:49PM +0200, Mauro Carvalho Chehab wrote: > Em Wed, 19 Aug 2020 19:35:58 +0200 > Sam Ravnborg escreveu: > > I'm already handling the other comments from your review (I'll send a > more complete comment about them after finishing), If you get back only on things you do not understand or do not agree on that would be fine. The rest should be visible in the changelog on the updated patch - no need to do extra work here. > but I have a doubt what you meant about this: > > > +static int kirin_drm_bind(struct device *dev) > > > +{ > > > + struct drm_driver *driver = &kirin_drm_driver; > > > + struct drm_device *drm_dev; > > > + struct kirin_drm_private *priv; > > > + int ret; > > > + > > > + drm_dev = drm_dev_alloc(driver, dev); > > > + if (!drm_dev) > > > + return -ENOMEM; > > > + > > > + ret = kirin_drm_kms_init(drm_dev); > > > + if (ret) > > > + goto err_drm_dev_unref; > > > + > > > + ret = drm_dev_register(drm_dev, 0); > > There is better ways to do this. See drm_drv.c for the code example. > > Not sure if I understood your comment here. The drm_drv.c example also calls > drm_dev_register(). This is indeed not obvious from my comments but what I wnated to say is that the driver should embed drm_device in some struct, maybe in "struct kirin_drm_private". This should also be part of the referenced example. I hope this clarifies it. Sam > > The only difference is that it calls it inside driver_probe(), while > on this driver, it uses: > > static const struct component_master_ops kirin_drm_ops = { > .bind = kirin_drm_bind, > .unbind = kirin_drm_unbind, > }; > > static int kirin_drm_platform_probe(struct platform_device *pdev) > { > ... > drm_of_component_match_add(dev, &match, compare_of, remote); > ... > return component_master_add_with_match(dev, &kirin_drm_ops, match); > } > > Are you meaning that I should get rid of this component API and call > kirin_drm_bind() from kirin_drm_platform_probe()? > > Thanks, > Mauro