From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753266AbcG2OpM (ORCPT ); Fri, 29 Jul 2016 10:45:12 -0400 Received: from mail-pa0-f68.google.com ([209.85.220.68]:33604 "EHLO mail-pa0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753175AbcG2OpI (ORCPT ); Fri, 29 Jul 2016 10:45:08 -0400 Date: Fri, 29 Jul 2016 16:45:02 +0200 From: Thierry Reding To: Bibby Hsieh Cc: David Airlie , Matthias Brugger , Daniel Vetter , dri-devel@lists.freedesktop.org, linux-mediatek@lists.infradead.org, Yingjoe Chen , Cawa Cheng , Daniel Kurtz , Philipp Zabel , YT Shen , CK Hu , Mao Huang , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Sascha Hauer , Junzhi Zhao Subject: Re: [PATCH v2 3/3] drm/mediatek: fix the wrong pixel clock when resolution is 4K Message-ID: <20160729144502.GC3864@ulmo.ba.sec> References: <1469608292-6106-1-git-send-email-bibby.hsieh@mediatek.com> <1469608292-6106-4-git-send-email-bibby.hsieh@mediatek.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="IpbVkmxF4tDyP/Kb" Content-Disposition: inline In-Reply-To: <1469608292-6106-4-git-send-email-bibby.hsieh@mediatek.com> User-Agent: Mutt/1.6.2 (2016-07-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --IpbVkmxF4tDyP/Kb Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Jul 27, 2016 at 04:31:32PM +0800, Bibby Hsieh wrote: > From: Junzhi Zhao >=20 > Pixel clock should be 297MHz when resolution is 4K. >=20 > Signed-off-by: Junzhi Zhao > Signed-off-by: Bibby Hsieh > --- > drivers/gpu/drm/mediatek/mtk_dpi.c | 149 +++++++++++++++++++++++-------= ------ > 1 file changed, 96 insertions(+), 53 deletions(-) >=20 > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediate= k/mtk_dpi.c > index d05ca79..fa390e0 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > @@ -60,14 +60,25 @@ enum mtk_dpi_out_color_format { > MTK_DPI_COLOR_FORMAT_YCBCR_422_FULL > }; > =20 > +enum mtk_dpi_clk_id { > + MTK_DPI_CLK_DPI_ENGINE, > + MTK_DPI_CLK_DPI_PIXEL, > + MTK_DPI_CLK_TVD_PLL, > + MTK_DPI_CLK_COUNT, > +}; > + > +static const char * const mtk_dpi_clk_names[MTK_DPI_CLK_COUNT] =3D { > + [MTK_DPI_CLK_DPI_ENGINE] =3D "engine", > + [MTK_DPI_CLK_DPI_PIXEL] =3D "pixel", > + [MTK_DPI_CLK_TVD_PLL] =3D "pll", > +}; > + > struct mtk_dpi { > struct mtk_ddp_comp ddp_comp; > struct drm_encoder encoder; > void __iomem *regs; > struct device *dev; > - struct clk *engine_clk; > - struct clk *pixel_clk; > - struct clk *tvd_clk; > + struct clk *clk[MTK_DPI_CLK_COUNT]; This looks to me like a step backwards. All accesses to these clocks are now very clumsy and I don't see any advantage in using this array rather than individually named clocks. Also, that change seems completely unrelated to the description in the commit message. > int irq; > struct drm_display_mode mode; > enum mtk_dpi_out_color_format color_format; > @@ -76,6 +87,7 @@ struct mtk_dpi { > enum mtk_dpi_out_channel_swap channel_swap; > bool power_sta; > u8 power_ctl; > + void *data; This should probably be const. It's a bad idea to cast away the const =66rom the of_device_id.data that this is assigned from. > +static const struct of_device_id mtk_dpi_of_ids[] =3D { > + { .compatible =3D "mediatek,mt8173-dpi", > + .data =3D &mt8173_conf, > + }, Please align this in some standard way. It all fits on one line, so the most obvious choice would've been: { .compatible =3D "mediatek,mt8173-dpi", .data =3D &mt8173_conf }, > + {} > +}; > + > static int mtk_dpi_probe(struct platform_device *pdev) > { > struct device *dev =3D &pdev->dev; > struct mtk_dpi *dpi; > struct resource *mem; > + struct device_node *np =3D dev->of_node; > struct device_node *ep, *bridge_node =3D NULL; > int comp_id; > + const struct of_device_id *match; > + struct mtk_dpi_conf *conf; > int ret; > =20 > + match =3D of_match_node(mtk_dpi_of_ids, dev->of_node); > + if (!match) > + return -ENODEV; You introduce a variable np =3D dev->of_node above, but then you add code that uses dev->of_node directly? That said, you should use of_device_get_match_data() anyway, so that you can omit... > + > dpi =3D devm_kzalloc(dev, sizeof(*dpi), GFP_KERNEL); > if (!dpi) > return -ENOMEM; > =20 > dpi->dev =3D dev; > + dpi->data =3D (void *)match->data; > + conf =3D (struct mtk_dpi_conf *)match->data; =2E.. the dereferences here. Also, like I said before you should make dpi->data and conf both const to avoid casting away the constness of the data. If you do that you can even drop the casts because you'd be casting from a const void * to a const foo *, which gets casted automatically. > @@ -754,11 +802,6 @@ static int mtk_dpi_remove(struct platform_device *pd= ev) > return 0; > } > =20 > -static const struct of_device_id mtk_dpi_of_ids[] =3D { > - { .compatible =3D "mediatek,mt8173-dpi", }, > - {} > -}; Another advantage of using of_device_get_match_data() is that it doesn't need direct access to the of_device_id table (it'll obtain it via an indirect dev->driver->of_match_table), so you don't have to move this around and make this patch overall less churn. Thierry --IpbVkmxF4tDyP/Kb Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAABCAAGBQJXm2vsAAoJEN0jrNd/PrOhgtkP/i+eM1cko2tlYKtp0jusm3wB +cYDrL4prw0cQwJuEsGVMHN/aR/wLHUZajxpb+nSKYgfajNNYTASnFhiY7wMPRIi VS/naMJQdmwFSErAysOTdoG7/xOA77hIaj25XRrmv3Mb218ikdg9Sd625JZsfxKM u4rtl/vqpFJDmQI61mch1br/qGrRZWeg28m3NxpphWBbpzPHAXj6ORS/u89dlmoI nmQ30GSZmrHpL2QeFyOfQ/YGZ7ysJxGUbUgaW3lkq9IMNSERxzVEuDygzP9xhmA8 AyRnHTL1BKyYQtgWSM+pxLTPjC7gBopnk+dnWancGvS2qP6tsSEkM32w9i7AExuA fhqCpel5TojuFb/FKGcs+KR1264YSucMWEZv58/NrdC4zaHaiUY6UVqzcIaDB9Ll W67rq/IU6gexwOEy1Q2cuOFkrEAFO+nVS1A+KfTeQJE4rDpcVfcY+2evYyxUSFqm v6ibwpo5yqVNJyX+P7nY4aM60rLzkZiIf9FANsIFd4gNYD8IaKn6ufgzCiIrgTd3 3cj1IJm/4gztN5Bm3uWykbsadjZXpMWDZgDflQxKE9lLdx9qVyxc5In3xrk48Q/n EQPW6RWKvaOSBB+5ek7TctXH5lGCGFb+MIlHtOloROjSsQPXh+VWdRyhaTv1YzbP kUziBFFJBxaWEktSRYDz =I1Yi -----END PGP SIGNATURE----- --IpbVkmxF4tDyP/Kb-- From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH v2 3/3] drm/mediatek: fix the wrong pixel clock when resolution is 4K Date: Fri, 29 Jul 2016 16:45:02 +0200 Message-ID: <20160729144502.GC3864@ulmo.ba.sec> References: <1469608292-6106-1-git-send-email-bibby.hsieh@mediatek.com> <1469608292-6106-4-git-send-email-bibby.hsieh@mediatek.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1277905305==" Return-path: In-Reply-To: <1469608292-6106-4-git-send-email-bibby.hsieh@mediatek.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Bibby Hsieh Cc: Junzhi Zhao , linux-kernel@vger.kernel.org, Sascha Hauer , Daniel Vetter , Cawa Cheng , dri-devel@lists.freedesktop.org, Mao Huang , linux-mediatek@lists.infradead.org, Matthias Brugger , Yingjoe Chen , linux-arm-kernel@lists.infradead.org List-Id: linux-mediatek@lists.infradead.org --===============1277905305== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="IpbVkmxF4tDyP/Kb" Content-Disposition: inline --IpbVkmxF4tDyP/Kb Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Jul 27, 2016 at 04:31:32PM +0800, Bibby Hsieh wrote: > From: Junzhi Zhao >=20 > Pixel clock should be 297MHz when resolution is 4K. >=20 > Signed-off-by: Junzhi Zhao > Signed-off-by: Bibby Hsieh > --- > drivers/gpu/drm/mediatek/mtk_dpi.c | 149 +++++++++++++++++++++++-------= ------ > 1 file changed, 96 insertions(+), 53 deletions(-) >=20 > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediate= k/mtk_dpi.c > index d05ca79..fa390e0 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > @@ -60,14 +60,25 @@ enum mtk_dpi_out_color_format { > MTK_DPI_COLOR_FORMAT_YCBCR_422_FULL > }; > =20 > +enum mtk_dpi_clk_id { > + MTK_DPI_CLK_DPI_ENGINE, > + MTK_DPI_CLK_DPI_PIXEL, > + MTK_DPI_CLK_TVD_PLL, > + MTK_DPI_CLK_COUNT, > +}; > + > +static const char * const mtk_dpi_clk_names[MTK_DPI_CLK_COUNT] =3D { > + [MTK_DPI_CLK_DPI_ENGINE] =3D "engine", > + [MTK_DPI_CLK_DPI_PIXEL] =3D "pixel", > + [MTK_DPI_CLK_TVD_PLL] =3D "pll", > +}; > + > struct mtk_dpi { > struct mtk_ddp_comp ddp_comp; > struct drm_encoder encoder; > void __iomem *regs; > struct device *dev; > - struct clk *engine_clk; > - struct clk *pixel_clk; > - struct clk *tvd_clk; > + struct clk *clk[MTK_DPI_CLK_COUNT]; This looks to me like a step backwards. All accesses to these clocks are now very clumsy and I don't see any advantage in using this array rather than individually named clocks. Also, that change seems completely unrelated to the description in the commit message. > int irq; > struct drm_display_mode mode; > enum mtk_dpi_out_color_format color_format; > @@ -76,6 +87,7 @@ struct mtk_dpi { > enum mtk_dpi_out_channel_swap channel_swap; > bool power_sta; > u8 power_ctl; > + void *data; This should probably be const. It's a bad idea to cast away the const =66rom the of_device_id.data that this is assigned from. > +static const struct of_device_id mtk_dpi_of_ids[] =3D { > + { .compatible =3D "mediatek,mt8173-dpi", > + .data =3D &mt8173_conf, > + }, Please align this in some standard way. It all fits on one line, so the most obvious choice would've been: { .compatible =3D "mediatek,mt8173-dpi", .data =3D &mt8173_conf }, > + {} > +}; > + > static int mtk_dpi_probe(struct platform_device *pdev) > { > struct device *dev =3D &pdev->dev; > struct mtk_dpi *dpi; > struct resource *mem; > + struct device_node *np =3D dev->of_node; > struct device_node *ep, *bridge_node =3D NULL; > int comp_id; > + const struct of_device_id *match; > + struct mtk_dpi_conf *conf; > int ret; > =20 > + match =3D of_match_node(mtk_dpi_of_ids, dev->of_node); > + if (!match) > + return -ENODEV; You introduce a variable np =3D dev->of_node above, but then you add code that uses dev->of_node directly? That said, you should use of_device_get_match_data() anyway, so that you can omit... > + > dpi =3D devm_kzalloc(dev, sizeof(*dpi), GFP_KERNEL); > if (!dpi) > return -ENOMEM; > =20 > dpi->dev =3D dev; > + dpi->data =3D (void *)match->data; > + conf =3D (struct mtk_dpi_conf *)match->data; =2E.. the dereferences here. Also, like I said before you should make dpi->data and conf both const to avoid casting away the constness of the data. If you do that you can even drop the casts because you'd be casting from a const void * to a const foo *, which gets casted automatically. > @@ -754,11 +802,6 @@ static int mtk_dpi_remove(struct platform_device *pd= ev) > return 0; > } > =20 > -static const struct of_device_id mtk_dpi_of_ids[] =3D { > - { .compatible =3D "mediatek,mt8173-dpi", }, > - {} > -}; Another advantage of using of_device_get_match_data() is that it doesn't need direct access to the of_device_id table (it'll obtain it via an indirect dev->driver->of_match_table), so you don't have to move this around and make this patch overall less churn. Thierry --IpbVkmxF4tDyP/Kb Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAABCAAGBQJXm2vsAAoJEN0jrNd/PrOhgtkP/i+eM1cko2tlYKtp0jusm3wB +cYDrL4prw0cQwJuEsGVMHN/aR/wLHUZajxpb+nSKYgfajNNYTASnFhiY7wMPRIi VS/naMJQdmwFSErAysOTdoG7/xOA77hIaj25XRrmv3Mb218ikdg9Sd625JZsfxKM u4rtl/vqpFJDmQI61mch1br/qGrRZWeg28m3NxpphWBbpzPHAXj6ORS/u89dlmoI nmQ30GSZmrHpL2QeFyOfQ/YGZ7ysJxGUbUgaW3lkq9IMNSERxzVEuDygzP9xhmA8 AyRnHTL1BKyYQtgWSM+pxLTPjC7gBopnk+dnWancGvS2qP6tsSEkM32w9i7AExuA fhqCpel5TojuFb/FKGcs+KR1264YSucMWEZv58/NrdC4zaHaiUY6UVqzcIaDB9Ll W67rq/IU6gexwOEy1Q2cuOFkrEAFO+nVS1A+KfTeQJE4rDpcVfcY+2evYyxUSFqm v6ibwpo5yqVNJyX+P7nY4aM60rLzkZiIf9FANsIFd4gNYD8IaKn6ufgzCiIrgTd3 3cj1IJm/4gztN5Bm3uWykbsadjZXpMWDZgDflQxKE9lLdx9qVyxc5In3xrk48Q/n EQPW6RWKvaOSBB+5ek7TctXH5lGCGFb+MIlHtOloROjSsQPXh+VWdRyhaTv1YzbP kUziBFFJBxaWEktSRYDz =I1Yi -----END PGP SIGNATURE----- --IpbVkmxF4tDyP/Kb-- --===============1277905305== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============1277905305==-- From mboxrd@z Thu Jan 1 00:00:00 1970 From: thierry.reding@gmail.com (Thierry Reding) Date: Fri, 29 Jul 2016 16:45:02 +0200 Subject: [PATCH v2 3/3] drm/mediatek: fix the wrong pixel clock when resolution is 4K In-Reply-To: <1469608292-6106-4-git-send-email-bibby.hsieh@mediatek.com> References: <1469608292-6106-1-git-send-email-bibby.hsieh@mediatek.com> <1469608292-6106-4-git-send-email-bibby.hsieh@mediatek.com> Message-ID: <20160729144502.GC3864@ulmo.ba.sec> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Wed, Jul 27, 2016 at 04:31:32PM +0800, Bibby Hsieh wrote: > From: Junzhi Zhao > > Pixel clock should be 297MHz when resolution is 4K. > > Signed-off-by: Junzhi Zhao > Signed-off-by: Bibby Hsieh > --- > drivers/gpu/drm/mediatek/mtk_dpi.c | 149 +++++++++++++++++++++++------------- > 1 file changed, 96 insertions(+), 53 deletions(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c > index d05ca79..fa390e0 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > @@ -60,14 +60,25 @@ enum mtk_dpi_out_color_format { > MTK_DPI_COLOR_FORMAT_YCBCR_422_FULL > }; > > +enum mtk_dpi_clk_id { > + MTK_DPI_CLK_DPI_ENGINE, > + MTK_DPI_CLK_DPI_PIXEL, > + MTK_DPI_CLK_TVD_PLL, > + MTK_DPI_CLK_COUNT, > +}; > + > +static const char * const mtk_dpi_clk_names[MTK_DPI_CLK_COUNT] = { > + [MTK_DPI_CLK_DPI_ENGINE] = "engine", > + [MTK_DPI_CLK_DPI_PIXEL] = "pixel", > + [MTK_DPI_CLK_TVD_PLL] = "pll", > +}; > + > struct mtk_dpi { > struct mtk_ddp_comp ddp_comp; > struct drm_encoder encoder; > void __iomem *regs; > struct device *dev; > - struct clk *engine_clk; > - struct clk *pixel_clk; > - struct clk *tvd_clk; > + struct clk *clk[MTK_DPI_CLK_COUNT]; This looks to me like a step backwards. All accesses to these clocks are now very clumsy and I don't see any advantage in using this array rather than individually named clocks. Also, that change seems completely unrelated to the description in the commit message. > int irq; > struct drm_display_mode mode; > enum mtk_dpi_out_color_format color_format; > @@ -76,6 +87,7 @@ struct mtk_dpi { > enum mtk_dpi_out_channel_swap channel_swap; > bool power_sta; > u8 power_ctl; > + void *data; This should probably be const. It's a bad idea to cast away the const from the of_device_id.data that this is assigned from. > +static const struct of_device_id mtk_dpi_of_ids[] = { > + { .compatible = "mediatek,mt8173-dpi", > + .data = &mt8173_conf, > + }, Please align this in some standard way. It all fits on one line, so the most obvious choice would've been: { .compatible = "mediatek,mt8173-dpi", .data = &mt8173_conf }, > + {} > +}; > + > static int mtk_dpi_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > struct mtk_dpi *dpi; > struct resource *mem; > + struct device_node *np = dev->of_node; > struct device_node *ep, *bridge_node = NULL; > int comp_id; > + const struct of_device_id *match; > + struct mtk_dpi_conf *conf; > int ret; > > + match = of_match_node(mtk_dpi_of_ids, dev->of_node); > + if (!match) > + return -ENODEV; You introduce a variable np = dev->of_node above, but then you add code that uses dev->of_node directly? That said, you should use of_device_get_match_data() anyway, so that you can omit... > + > dpi = devm_kzalloc(dev, sizeof(*dpi), GFP_KERNEL); > if (!dpi) > return -ENOMEM; > > dpi->dev = dev; > + dpi->data = (void *)match->data; > + conf = (struct mtk_dpi_conf *)match->data; ... the dereferences here. Also, like I said before you should make dpi->data and conf both const to avoid casting away the constness of the data. If you do that you can even drop the casts because you'd be casting from a const void * to a const foo *, which gets casted automatically. > @@ -754,11 +802,6 @@ static int mtk_dpi_remove(struct platform_device *pdev) > return 0; > } > > -static const struct of_device_id mtk_dpi_of_ids[] = { > - { .compatible = "mediatek,mt8173-dpi", }, > - {} > -}; Another advantage of using of_device_get_match_data() is that it doesn't need direct access to the of_device_id table (it'll obtain it via an indirect dev->driver->of_match_table), so you don't have to move this around and make this patch overall less churn. Thierry -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: