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

[Boris Brezillon <boris.brezillon@free-electrons.com>]

On Thu, 7 Sep 2017 15:06:13 +0530
Archit Taneja <architt@codeaurora.org> wrote:

> Hi,
> 
> On 08/31/2017 09:25 PM, Boris Brezillon wrote:
> > Add a driver for Cadence DPI -> DSI bridge.
> > 
> > This driver only support a subset of Cadence DSI bridge capabilities.
> > 
> > Here is a non-exhaustive list of missing features:
> >   * burst mode
> >   * dynamic configuration of the DPHY based on the
> >   * support for additional input interfaces (SDI input)
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> > Changes in v3:
> > - replace magic values by real timing calculation. The DPHY PLL clock
> >    is still hardcoded since we don't have a working DPHY block yet, and
> >    this is the piece of HW we need to dynamically configure the PLL
> >    rate based on the display refresh rate and the resolution.
> > - parse DSI devices represented with the OF-graph. This is needed to
> >    support DSI devices controlled through an external bus like I2C or
> >    SPI.
> > - use the DRM panel-bridge infrastructure to simplify the DRM panel
> >    logic
> > 
> > Changes in v2:
> > - rebase on v4.12-rc1 and adapt to driver to the drm_bridge API changes
> > - return the correct error when devm_clk_get(sysclk) fails
> > - add missing depends on OF and select DRM_PANEL in the Kconfig entry
> > ---
> >   drivers/gpu/drm/bridge/Kconfig    |    9 +
> >   drivers/gpu/drm/bridge/Makefile   |    1 +
> >   drivers/gpu/drm/bridge/cdns-dsi.c | 1090 +++++++++++++++++++++++++++++++++++++
> >   3 files changed, 1100 insertions(+)
> >   create mode 100644 drivers/gpu/drm/bridge/cdns-dsi.c
> > 
> > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> > index adf9ae0e0b7c..88c324b12e16 100644
> > --- a/drivers/gpu/drm/bridge/Kconfig
> > +++ b/drivers/gpu/drm/bridge/Kconfig
> > @@ -25,6 +25,15 @@ config DRM_ANALOGIX_ANX78XX
> >   	  the HDMI output of an application processor to MyDP
> >   	  or DisplayPort.
> >   
> > +config DRM_CDNS_DSI
> > +	tristate "Cadence DPI/DSI bridge"
> > +	select DRM_KMS_HELPER
> > +	select DRM_MIPI_DSI
> > +	select DRM_PANEL
> > +	depends on OF
> > +	help
> > +	  Support Cadence DPI to DSI bridge.  
> 
> Maybe we can briefly mention here that it's a internal bridge/IP, and not an external chip?

Sure.

> 
> > +
> >   config DRM_DUMB_VGA_DAC
> >   	tristate "Dumb VGA DAC Bridge support"
> >   	depends on OF
> > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> > index defcf1e7ca1c..77b65e8ecf59 100644
> > --- a/drivers/gpu/drm/bridge/Makefile
> > +++ b/drivers/gpu/drm/bridge/Makefile
> > @@ -1,4 +1,5 @@
> >   obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
> > +obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o
> >   obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o
> >   obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o
> >   obj-$(CONFIG_DRM_MEGACHIPS_STDPXXXX_GE_B850V3_FW) += megachips-stdpxxxx-ge-b850v3-fw.o
> > diff --git a/drivers/gpu/drm/bridge/cdns-dsi.c b/drivers/gpu/drm/bridge/cdns-dsi.c  
> 
> <snip>
> 
> > +
> > +static void cdns_dsi_bridge_disable(struct drm_bridge *bridge)
> > +{
> > +	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
> > +	struct cdns_dsi *dsi = input_to_dsi(input);
> > +	u32 val;
> > +
> > +	val = readl(dsi->regs + MCTL_MAIN_DATA_CTL);
> > +	val &= ~(IF_VID_SELECT_MASK | IF_VID_MODE | VID_EN | HOST_EOT_GEN |
> > +		 DISP_EOT_GEN);  
> 
> I see some truncation related sparse warnings here and a couple of other places
> when building against arm32. Those would be nice to fix.

I'll have a look.


> > +static int cdns_dsi_attach(struct mipi_dsi_host *host,
> > +			   struct mipi_dsi_device *dev)
> > +{
> > +	struct cdns_dsi *dsi = to_cdns_dsi(host);
> > +	struct cdns_dsi_output *output = &dsi->output;
> > +	struct cdns_dsi_input *input = &dsi->input;
> > +	struct drm_bridge *bridge;
> > +	struct drm_panel *panel;
> > +	struct device_node *np;
> > +	int ret;
> > +
> > +	/*
> > +	 * We currently do not support connecting several DSI devices to the
> > +	 * same host. In order to support that we'd need the DRM bridge
> > +	 * framework to allow dynamic reconfiguration of the bridge chain.
> > +	 */
> > +	if (output->dev)
> > +		return -EBUSY;
> > +
> > +	/* We do not support burst mode yet. */
> > +	if (dev->mode_flags & MIPI_DSI_MODE_VIDEO_BURST)
> > +		return -ENOTSUPP;
> > +
> > +	/*
> > +	 * The host <-> device link might be described using an OF-graph
> > +	 * representation, in this case we extract the device of_node from
> > +	 * this representation, otherwise we use dsidev->dev.of_node which
> > +	 * should have been filled by the core.
> > +	 */
> > +	np = of_graph_get_remote_node(dsi->base.dev->of_node, DSI_OUTPUT_PORT,
> > +				      dev->channel);
> > +	if (!np)
> > +		np = of_node_get(dev->dev.of_node); > +
> > +	panel = of_drm_find_panel(np);
> > +	if (panel) {
> > +		bridge = drm_panel_bridge_add(panel, DRM_MODE_CONNECTOR_DSI);
> > +	} else {
> > +		bridge = of_drm_find_bridge(dev->dev.of_node);
> > +		if (!bridge)
> > +			bridge = ERR_PTR(-EINVAL);
> > +	}
> > +
> > +	of_node_put(np);  
> 
> It would have been nice to use drm_of_find_panel_or_bridge() here, but I guess
> you couldn't use it because you have to handle both OF-graph based links, and
> children directly under the host DSI bus.

Yep.

Thanks for you review, and sorry for taking so long to reply.

Boris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel