From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH v4 2/6] drm/dsi: Refactor device creation Date: Thu, 21 Jan 2016 16:46:07 +0100 Message-ID: <20160121154605.GB647@ulmo.nvidia.com> References: <1448884892-7731-1-git-send-email-architt@codeaurora.org> <1449751300-2841-1-git-send-email-architt@codeaurora.org> <1449751300-2841-3-git-send-email-architt@codeaurora.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0869176465==" Return-path: In-Reply-To: <1449751300-2841-3-git-send-email-architt@codeaurora.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Archit Taneja Cc: linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, a.hajda@samsung.com, dri-devel@lists.freedesktop.org List-Id: linux-arm-msm@vger.kernel.org --===============0869176465== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="E39vaYmALEf/7YXx" Content-Disposition: inline --E39vaYmALEf/7YXx Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Dec 10, 2015 at 06:11:36PM +0530, Archit Taneja wrote: > Simplify the mipi dsi device creation process. device_initialize and "MIPI" and "DSI", please. > device_add don't need to be called separately when creating > mipi_dsi_device's. Use device_register instead to simplify things. >=20 > Create a helper function mipi_dsi_device_new which takes in struct > mipi_dsi_device_info and mipi_dsi_host. It clubs the functions > mipi_dsi_device_alloc and mipi_dsi_device_add into one. >=20 > mipi_dsi_device_info acts as a template to populate the dsi device > information. This is populated by of_mipi_dsi_device_add and passed to > mipi_dsi_device_new. >=20 > Later on, we'll provide mipi_dsi_device_new as a standalone way to create > a dsi device not available via DT. >=20 > The new device creation process tries to closely follow what's been done > in i2c_new_device in i2c-core. >=20 > Reviewed-by: Andrzej Hajda > Signed-off-by: Archit Taneja > --- > drivers/gpu/drm/drm_mipi_dsi.c | 61 +++++++++++++++++-------------------= ------ > include/drm/drm_mipi_dsi.h | 15 +++++++++++ > 2 files changed, 40 insertions(+), 36 deletions(-) To be honest, I'm not sure I like this. If you want to have a simpler helper, why not implement it using the lower-level helpers. Really the only thing you're doing here is add a high-level helper that takes an info struct, whereas previously the same would be done by storing the info directly in the structure between allocation and addition of the device. Initially the implementation was following that of platform devices, I see no reason to deviate from that. What you want here can easily be done by something like: struct mipi_dsi_device * mipi_dsi_device_register_full(struct mipi_dsi_host *host, const struct mipi_dsi_device_info *info) { struct mipi_dsi_device *dsi; dsi =3D mipi_dsi_device_alloc(host); if (IS_ERR(dsi)) return dsi; dsi->dev.of_node =3D info->node; dsi->channel =3D info->channel; err =3D mipi_dsi_device_add(dsi); if (err < 0) { ... } return dsi; } Thierry --E39vaYmALEf/7YXx Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAABCAAGBQJWoP07AAoJEN0jrNd/PrOhafsP/08aEtcMM/V/Vd4bZ46bhGbD CxCFFd8Qu9A6ERiwfr2eVb9updlrsCF7QW6cz0YKm043KQBk+Hb+NOg+fBNcU1Cu ItQ84wfB5yvcV7VImI/ATuV58/CHNMHuXZDCiXSY6hR4PrdSgAXcSe29YtkCFx9Y PQ+iIFykWs2N0i/K0IVboeWI9xU9ZIzDK2YvUZEHE+SMSInmMMpIJaz6EzthbZRj Lo5P8ikfqhtA/YsU5sKwW07Atl+2DADbNzUHf+Oa/TiFPXJycGXhgw8Lt+dLqRD7 uj+LFxlsbxUTZLpM3INQaVEptbaAgeNV1q0yoDQmm1MOKW8nIDinHZBA15WTrK4H AbpnWbEQ9uAeR3O+T11aREyz0quUHHPoFyrgROpQNCfg4K2zzsR31QczjT4rDFcS Pjfn2aP7UAdl1EQLbaeAnrufw6e91G43ssbT4iW/ZQZazhtr7W2NUqFOIvDfcKGW tPHAUcgSRHa2kI0mp8yAMigffIHm8X6JD/xRVqhDSIfry2ADGv6Adapz68NR66u5 v6lNhMsLpvdDC0r/6/VjXt0WViaqEDQZzeJGxwF1dBjXyArBcZpXjrQFRrW4p/Ui 3IJGRjG4J65VXbR0W/qYPiTM3sqTBIrylfr5ZR89gmEZzu3NGQrgSM8gs9X/LWpA WGTPkv3zt8W05RAeKR6v =3H4U -----END PGP SIGNATURE----- --E39vaYmALEf/7YXx-- --===============0869176465== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHA6Ly9saXN0 cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK --===============0869176465==-- From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759877AbcAUPqU (ORCPT ); Thu, 21 Jan 2016 10:46:20 -0500 Received: from hqemgate16.nvidia.com ([216.228.121.65]:5946 "EHLO hqemgate16.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759761AbcAUPqQ (ORCPT ); Thu, 21 Jan 2016 10:46:16 -0500 X-PGP-Universal: processed; by hqnvupgp08.nvidia.com on Thu, 21 Jan 2016 07:47:24 -0800 Date: Thu, 21 Jan 2016 16:46:07 +0100 From: Thierry Reding To: Archit Taneja CC: , , , , , , , , Subject: Re: [PATCH v4 2/6] drm/dsi: Refactor device creation Message-ID: <20160121154605.GB647@ulmo.nvidia.com> References: <1448884892-7731-1-git-send-email-architt@codeaurora.org> <1449751300-2841-1-git-send-email-architt@codeaurora.org> <1449751300-2841-3-git-send-email-architt@codeaurora.org> MIME-Version: 1.0 In-Reply-To: <1449751300-2841-3-git-send-email-architt@codeaurora.org> X-NVConfidentiality: public User-Agent: Mutt/1.5.24 (2015-08-30) X-Originating-IP: [10.2.68.189] X-ClientProxiedBy: UKMAIL102.nvidia.com (10.26.138.15) To UKMAIL102.nvidia.com (10.26.138.15) Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="E39vaYmALEf/7YXx" Content-Disposition: inline Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --E39vaYmALEf/7YXx Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Dec 10, 2015 at 06:11:36PM +0530, Archit Taneja wrote: > Simplify the mipi dsi device creation process. device_initialize and "MIPI" and "DSI", please. > device_add don't need to be called separately when creating > mipi_dsi_device's. Use device_register instead to simplify things. >=20 > Create a helper function mipi_dsi_device_new which takes in struct > mipi_dsi_device_info and mipi_dsi_host. It clubs the functions > mipi_dsi_device_alloc and mipi_dsi_device_add into one. >=20 > mipi_dsi_device_info acts as a template to populate the dsi device > information. This is populated by of_mipi_dsi_device_add and passed to > mipi_dsi_device_new. >=20 > Later on, we'll provide mipi_dsi_device_new as a standalone way to create > a dsi device not available via DT. >=20 > The new device creation process tries to closely follow what's been done > in i2c_new_device in i2c-core. >=20 > Reviewed-by: Andrzej Hajda > Signed-off-by: Archit Taneja > --- > drivers/gpu/drm/drm_mipi_dsi.c | 61 +++++++++++++++++-------------------= ------ > include/drm/drm_mipi_dsi.h | 15 +++++++++++ > 2 files changed, 40 insertions(+), 36 deletions(-) To be honest, I'm not sure I like this. If you want to have a simpler helper, why not implement it using the lower-level helpers. Really the only thing you're doing here is add a high-level helper that takes an info struct, whereas previously the same would be done by storing the info directly in the structure between allocation and addition of the device. Initially the implementation was following that of platform devices, I see no reason to deviate from that. What you want here can easily be done by something like: struct mipi_dsi_device * mipi_dsi_device_register_full(struct mipi_dsi_host *host, const struct mipi_dsi_device_info *info) { struct mipi_dsi_device *dsi; dsi =3D mipi_dsi_device_alloc(host); if (IS_ERR(dsi)) return dsi; dsi->dev.of_node =3D info->node; dsi->channel =3D info->channel; err =3D mipi_dsi_device_add(dsi); if (err < 0) { ... } return dsi; } Thierry --E39vaYmALEf/7YXx Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAABCAAGBQJWoP07AAoJEN0jrNd/PrOhafsP/08aEtcMM/V/Vd4bZ46bhGbD CxCFFd8Qu9A6ERiwfr2eVb9updlrsCF7QW6cz0YKm043KQBk+Hb+NOg+fBNcU1Cu ItQ84wfB5yvcV7VImI/ATuV58/CHNMHuXZDCiXSY6hR4PrdSgAXcSe29YtkCFx9Y PQ+iIFykWs2N0i/K0IVboeWI9xU9ZIzDK2YvUZEHE+SMSInmMMpIJaz6EzthbZRj Lo5P8ikfqhtA/YsU5sKwW07Atl+2DADbNzUHf+Oa/TiFPXJycGXhgw8Lt+dLqRD7 uj+LFxlsbxUTZLpM3INQaVEptbaAgeNV1q0yoDQmm1MOKW8nIDinHZBA15WTrK4H AbpnWbEQ9uAeR3O+T11aREyz0quUHHPoFyrgROpQNCfg4K2zzsR31QczjT4rDFcS Pjfn2aP7UAdl1EQLbaeAnrufw6e91G43ssbT4iW/ZQZazhtr7W2NUqFOIvDfcKGW tPHAUcgSRHa2kI0mp8yAMigffIHm8X6JD/xRVqhDSIfry2ADGv6Adapz68NR66u5 v6lNhMsLpvdDC0r/6/VjXt0WViaqEDQZzeJGxwF1dBjXyArBcZpXjrQFRrW4p/Ui 3IJGRjG4J65VXbR0W/qYPiTM3sqTBIrylfr5ZR89gmEZzu3NGQrgSM8gs9X/LWpA WGTPkv3zt8W05RAeKR6v =3H4U -----END PGP SIGNATURE----- --E39vaYmALEf/7YXx--