Re: [PATCH v5 1/2] drm/bridge: Add Cadence DSI driver

From: Boris Brezillon <boris.brezillon@free-electrons.com>
To: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>,
	"Menon, Nishanth" <nm@ti.com>, Pawel Moll <pawel.moll@arm.com>,
	Ian Campbell <ijc+devicetree@hellion.org.uk>,
	David Airlie <airlied@linux.ie>,
	Simon Hatliff <hatliff@cadence.com>,
	Mark Rutland <mark.rutland@arm.com>,
	dri-devel@lists.freedesktop.org,
	Alan Douglas <adouglas@cadence.com>,
	Rob Herring <robh+dt@kernel.org>, Jyri Sarha <jsarha@ti.com>,
	Suresh Punnoose <sureshp@cadence.com>,
	Kumar Gala <galak@codeaurora.org>,
	Maxime Ripard <maxime.ripard@free-electrons.com>,
	Richard Sproul <sproul@cadence.com>,
	devicetree@vger.kernel.org
Subject: Re: [PATCH v5 1/2] drm/bridge: Add Cadence DSI driver
Date: Mon, 29 Jan 2018 14:46:23 +0100	[thread overview]
Message-ID: <20180129144623.495f66bf@bbrezillon> (raw)
In-Reply-To: <20180129141409.0ec356d1@bbrezillon>

On Mon, 29 Jan 2018 14:14:09 +0100
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> On Mon, 29 Jan 2018 13:56:21 +0200
> Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> 
> > > +
> > > +static void cdns_dsi_init_link(struct cdns_dsi *dsi)
> > > +{
> > > +	struct cdns_dsi_output *output = &dsi->output;
> > > +	unsigned long sysclk_period, ulpout;
> > > +	u32 val;
> > > +	int i;
> > > +
> > > +	if (dsi->link_initialized)
> > > +		return;
> > > +
> > > +	val = 0;
> > > +	for (i = 1; i < output->dev->lanes; i++)
> > > +		val |= DATA_LANE_EN(i);
> > > +
> > > +	if (!(output->dev->mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS))
> > > +		val |= CLK_CONTINUOUS;
> > > +
> > > +	writel(val, dsi->regs + MCTL_MAIN_PHY_CTL);
> > > +
> > > +	/* ULPOUT should be set to 1ms and is expressed in sysclk cycles. */
> > > +	sysclk_period = NSEC_PER_SEC / clk_get_rate(dsi->dsi_sys_clk);
> > > +	ulpout = DIV_ROUND_UP(NSEC_PER_MSEC, sysclk_period);
> > > +	writel(CLK_LANE_ULPOUT_TIME(ulpout) | DATA_LANE_ULPOUT_TIME(ulpout),
> > > +	       dsi->regs + MCTL_ULPOUT_TIME);
> > > +
> > > +	writel(LINK_EN, dsi->regs + MCTL_MAIN_DATA_CTL);
> > > +
> > > +	val = CLK_LANE_EN | PLL_START;
> > > +	for (i = 0; i < output->dev->lanes; i++)
> > > +		val |= DATA_LANE_START(i);
> > > +
> > > +	writel(val, dsi->regs + MCTL_MAIN_EN);
> > > +
> > > +	ndelay(100);    
> > 
> > It's good to have a comment about each sleep/delay on what is for and
> > where does the value come from.  
> 
> Yep, I'll figure this out.

Looks like this delay is not needed in the end. I'll remove it.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel