From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tomi Valkeinen Subject: Re: [PATCH v2 23/28] drm: omapdrm: Merge the dss_features and omap_dss_features structures Date: Wed, 10 May 2017 10:43:07 +0300 Message-ID: <1263aa75-63c4-2e34-0b8b-ea14e6c80dc5@ti.com> References: <20170508113303.27521-1-laurent.pinchart@ideasonboard.com> <20170508113303.27521-24-laurent.pinchart@ideasonboard.com> <12054023.GhQOfgyNIu@avalon> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0150047264==" Return-path: Received: from fllnx209.ext.ti.com (fllnx209.ext.ti.com [198.47.19.16]) by gabe.freedesktop.org (Postfix) with ESMTPS id 753BF6E346 for ; Wed, 10 May 2017 07:43:37 +0000 (UTC) In-Reply-To: <12054023.GhQOfgyNIu@avalon> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Laurent Pinchart Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============0150047264== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="iDRNBVjl4CWAchv8xNUp52ws8EPWafk4f" --iDRNBVjl4CWAchv8xNUp52ws8EPWafk4f Content-Type: multipart/mixed; boundary="ST7cPw8Ar5SSqP3lms7WkHuMX8dPNt34w"; protected-headers="v1" From: Tomi Valkeinen To: Laurent Pinchart Cc: dri-devel@lists.freedesktop.org Message-ID: <1263aa75-63c4-2e34-0b8b-ea14e6c80dc5@ti.com> Subject: Re: [PATCH v2 23/28] drm: omapdrm: Merge the dss_features and omap_dss_features structures References: <20170508113303.27521-1-laurent.pinchart@ideasonboard.com> <20170508113303.27521-24-laurent.pinchart@ideasonboard.com> <12054023.GhQOfgyNIu@avalon> In-Reply-To: <12054023.GhQOfgyNIu@avalon> --ST7cPw8Ar5SSqP3lms7WkHuMX8dPNt34w Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 10/05/17 01:09, Laurent Pinchart wrote: > Hi Tomi, >=20 > On Tuesday 09 May 2017 15:02:36 Tomi Valkeinen wrote: >> On 08/05/17 14:32, Laurent Pinchart wrote: >>> Both structures describe features of a particular OMAP DSS version, >>> there's no reason to keep them separate. Merge them together, allowin= g >>> initialization of the features based on the DSS compatible string >>> instead of the OMAP SoC version. >> >> I don't think this is the correct way. As I mentioned earlier, >> dss_features should go. We should work on moving the parts from >> dss_features to each individual driver. I think there are probably onl= y >> a very few dss-features that need to be accessed by multiple files, an= d >> those features could have a function of their own to query them. >=20 > I don't disagree with that, but can't we do so on top of this series ? = I don't=20 > think that these patches make the situation worse. >=20 >> But that may also be a bit bigger work, so... I thought about it alrea= dy >> earlier in this series: wouldn't it be easier to first reconstruct the= >> current OMAPDSS_VER_* with soc_device_match(), at module init time, >> which would allow us to keep most of the omapdss side unchanged. Then >> continue removing the arch/arm/ stuff. >=20 > I considered that to start with, but decided it was really a hack. I in= stead=20 Is it? You already use the dss compat string and soc_device_match to figure out some versions. Isn't that a proper way to find out about the SoC? But I agree that a more fine grained version management in each individual driver would be better. > went for cleaning things up where possible, and keeping hacks where a p= roper=20 > rework would require a set of new patch series. The result is a balance= =20 > between a few hacks and lots of cleanup patches, which I considered was= right=20 > when I realized that the series was already 28 patches long. Well, my concern is that these patches change a lot of things, moving stuff from here to there. And I think they would be moved or changed again later. So lots of room for conflicts and errors. For example, in this patch you move things from dss.c to dss_features.c, but I think the dss.c is the right place for them (at least most of them), so we'd just end up moving them back. And if instead we'd move things from dss_features.c to dss.c, well, many of the things don't belong to dss.c and would be moved again later. Then the HDMI PLL/PHY changes, well, as I mentioned, I don't fully agree with those. So, while I think recreating the omapdss version in the dss driver would clearly be a temporary measure, still afaics it would be not that many lines of code and in just one place, and would allow us to easily move on to remove the extra platform devices and dependencies to arch/arm/. After that we could start cleaning up the version handling inside the driver. It is just the omapdss version and the dsi pinmuxing we get from the platform data, so getting rid of just those two should allow removal of the platform device. So, correct me if I'm wrong, but all the hdmi, dss "model" and dss_feature changes could as well be done in a separate series later. Tomi --ST7cPw8Ar5SSqP3lms7WkHuMX8dPNt34w-- --iDRNBVjl4CWAchv8xNUp52ws8EPWafk4f Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJZEsSLAAoJEPo9qoy8lh71aLwP/2VGzPo1se5ydG74+xjhQn2K zx7SFu9I8jwWEO0dbr/Uq15alo4/kleYjXSXqIEk5t3sw9HVzCXNrV/31/x8JNWP ZjP+1ljedJFnFN2LipT1vhYXwJ/uVgVyZ8kUsMKpRfNW8eRHtJpeeemwNlm5p5cX IWCt6tAbATV5Lo+Ody2VbTR3mqs+49UgntGMIXt2oYGG6gc4E38Kaey6MqGLEyBl JIfEmt4YMzp2pbarKFRg5t1HlMBxSoEgLD8yP9//lKbVKTqYxKNF8awuKJNiiwZ+ sfF88HyBBLZtSmrVCXVSBgEq2U97FPDzjXRfDsBI7PG7Kt9/3OlGJyblpptTQmGs rpI8LTzAEqQi67U92gqnhCEfKqpBc8qQI1cz0iMqVD2xaiNdI0LJkBhhrCYslY1K mhtOpKo1hieKIXNJDz/WwO+vL0HJzSNo8dNn/nuFGbxKrFWjhp28r5z+2pkuaagz z0tsQU7Pep7AnVuwUA45CloNVU185iB97Yrql3tAaaJdGHeWgS7673OliQs/M9mT Fie3nbASNMK0ecfygWI/evzJtpD2C742+T5ki9nFPwzp0rQvXPMDsaqmtPc9poPu EtPR1g5/qgATp4vAlTSVY/lIFDtXa8lzGFFdnw+gudTPRnmJe3ebWlVu4oatBFtK ZHTWqP2LTYyp/nfr+nHJ =KfIq -----END PGP SIGNATURE----- --iDRNBVjl4CWAchv8xNUp52ws8EPWafk4f-- --===============0150047264== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============0150047264==--