From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tomi Valkeinen Subject: Re: [PATCH v2 1/2] drm/bridge: Add Cadence DSI driver Date: Tue, 6 Jun 2017 16:30:15 +0300 Message-ID: References: <1496405096-18275-1-git-send-email-boris.brezillon@free-electrons.com> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="cgrjWv4Jt9RbNcTNIu2X7xOX6e4S12GM7" Return-path: In-Reply-To: <1496405096-18275-1-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Boris Brezillon , David Airlie , Daniel Vetter , dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org, Archit Taneja Cc: Mark Rutland , devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Cyprian Wronka , Thomas Petazzoni , Pawel Moll , Ian Campbell , Simon Hatliff , Alan Douglas , Rob Herring , Kumar Gala , Maxime Ripard , Richard Sproul , Neil Webb List-Id: devicetree@vger.kernel.org --cgrjWv4Jt9RbNcTNIu2X7xOX6e4S12GM7 Content-Type: multipart/mixed; boundary="P190XFrL3DC9TurVVlT8AlUT3APuAMuTU"; protected-headers="v1" From: Tomi Valkeinen To: Boris Brezillon , David Airlie , Daniel Vetter , dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org, Archit Taneja Cc: Mark Rutland , devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Cyprian Wronka , Thomas Petazzoni , Pawel Moll , Ian Campbell , Simon Hatliff , Alan Douglas , Rob Herring , Kumar Gala , Maxime Ripard , Richard Sproul , Neil Webb Message-ID: Subject: Re: [PATCH v2 1/2] drm/bridge: Add Cadence DSI driver References: <1496405096-18275-1-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> In-Reply-To: <1496405096-18275-1-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> --P190XFrL3DC9TurVVlT8AlUT3APuAMuTU Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 02/06/17 15:04, Boris Brezillon wrote: > +enum cdns_dsi_output_type { > + CDNS_DSI_PANEL =3D 0, > + CDNS_DSI_BRIDGE =3D 1, > +}; Just my opinion, but I think you should only define values for enums when those values actually mean something and are needed. In this case, it doesn't matter which values those enums have. > +static int cdns_dsi_init_link(struct cdns_dsi *dsi) > +{ > + u32 val; > + int i; > + > + writel(CLK_UNIT_INTERVAL(16), dsi->regs + MCTL_DPHY_STATIC); > + writel(CLK_DIV(0xb) | HSTX_TIMEOUT(0xed8afff), > + dsi->regs + MCTL_DPHY_TIMEOUT1); > + writel(LPRX_TIMEOUT(0xf30fffff), dsi->regs + MCTL_DPHY_TIMEOUT2); > + > + val =3D WAIT_BURST_TIME(0xf); > + for (i =3D 1; i < dsi->output->dev->lanes; i++) > + val |=3D DATA_LANE_EN(i); > + > + if (!(dsi->output->dev->mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS)) > + val |=3D CLK_CONTINUOUS; > + > + writel(val, dsi->regs + MCTL_MAIN_PHY_CTL); > + > + writel(CLK_LANE_ULPOUT_TIME(0x105) | DATA_LANE_ULPOUT_TIME(0x1d5), > + dsi->regs + MCTL_ULPOUT_TIME); > + > + writel(LINK_EN, dsi->regs + MCTL_MAIN_DATA_CTL); > + > + val =3D CLK_LANE_EN | PLL_START; > + for (i =3D 0; i < dsi->output->dev->lanes; i++) > + val |=3D DATA_LANE_START(i); > + > + writel(val, dsi->regs + MCTL_MAIN_EN); > + > + ndelay(100); > + > + return 0; > +} There are quite a bit of magic values here (elsewhere too). Looking at the names of the macros, many of them probably represent spans of time, in clock ticks? Would it make more sense to have the times defined in, say, nanoseconds, and a function to convert the ns to clock ticks? And a hardcoded CLK_DIV, does that work? Is the clock rate fixed? (I don't have the datasheet yet =3D) Tomi --P190XFrL3DC9TurVVlT8AlUT3APuAMuTU-- --cgrjWv4Jt9RbNcTNIu2X7xOX6e4S12GM7 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 iQIcBAEBCAAGBQJZNq5nAAoJEPo9qoy8lh717zMP/3tLA14AoRVGZuAv0mhHBF1w 1sb7+x3A2MSFZthRDfy4XnDdvxE2Snn8tPOUk98MoszM7qkkrX6ge5rFRAJjVq4m GDQqfol9Z2dSCKYloPDmExOzcii1qJCbexcHuLHMsdeI9Aoh8kZsKNW02MccX2PY OTiwR/ECaUteM/hKwAOC/Mz6tKdD3Bb9zM2COn0NUxgCE1XgV5aYmVpon6kXGnmf LxBvSuFq0gp4s6cu8uki9KCSxqiKIjNAdfEe07O8U08oVRU7JEkxr1FM90LHjvwQ vWVG3y101fOkjHzd0bIYF9ceEzbCRFEzVSh9u81Wkmpb24dGdq1DrAyJ2SVLoXYs 4pLXB4VNhA2r6WtCZ6mjkA0zpBfTU6jlTIiizVMkgjtYr0tXbcJ3BM8/+p/6Va8F ClFF4JfmV3xSqzAMh7M3L36fphGy89hZmHBFhMi3jBOhS9H1APzPoIURpiObNYLS I9gGjztTe6CPaC1Dpmtb2oHqtGRnA+zProYbbjuNuvd+TsXveuh8s39Dqs/P1qLW K7qpZ1ZjWD3iqgyYXCXjXBUN+qWe1BEOUyaOQE+o6YDhky7MzrG404AmgcSlPEsh 4Z8fU/4ma4uoKM74ES6c4hQuAb41B9n9bxysQXhHwYEPd3RChieHHvK8HO4JKQt2 g7TvtYGCUCj9R9OnMauN =dGyD -----END PGP SIGNATURE----- --cgrjWv4Jt9RbNcTNIu2X7xOX6e4S12GM7-- -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html