Re: [PATCH v2 26/27] drm/tegra: Add DSI support

[Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2689 bytes --]

On Fri, Oct 11, 2013 at 04:43:35PM -0600, Stephen Warren wrote:
> On 10/07/2013 02:34 AM, Thierry Reding wrote:
> > This commit adds support for both DSI outputs found on Tegra. Only very
> > minimal functionality is implemented, so advanced features like ganged
> > mode won't work.
> > 
> > Due to the lack of other test hardware, some sections of the driver are
> > hardcoded to work with Dalmore.
> 
> > diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
> 
> > +static int tegra_dsi_show_regs(struct seq_file *s, void *data)
> > +{
> > +	struct drm_info_node *node = s->private;
> > +	struct tegra_dsi *dsi = node->info_ent->data;
> > +
> > +#define DUMP_REG(name)						\
> > +	seq_printf(s, "%-32s %#05x %08lx\n", #name, name,	\
> > +		   tegra_dsi_readl(dsi, name))
> > +
> > +	DUMP_REG(DSI_INCR_SYNCPT);
> 
> Does it make sense to use an MMIO regmap instead? That way, you get all
> the debugfs files for free...

As far as I know, regmap doesn't give you the symbolic names for the
registers. I find that a rather useful feature because it allows to
easily compare the registers to the ones in our downstream kernels.

> > +static int tegra_dsi_probe(struct platform_device *pdev)
> 
> > +	dsi->clk_parent = devm_clk_get(&pdev->dev, "parent");
> > +	if (IS_ERR(dsi->clk_parent))
> > +		return PTR_ERR(dsi->clk_parent);
> ...
> > +static const struct of_device_id tegra_dsi_of_match[] = {
> > +	{ .compatible = "nvidia,tegra114-dsi", },
> 
> Is this DT binding documented? The clk_get() call above in particular
> imposes the requirement that DT contain a clock with that name, which
> should be part of the binding documentation.

I've documented the requirement for both the regular "dsi" as well as
the "parent" clock in the binding documentation, which I forgot to
update in the previous series.

Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt is where
this is documented. The DSI node has a compatible property of
nvidia,tegra<chip>-dsi, which I think is a common way to write the
binding at least for Tegra.

> Hopefully the values that this driver hard-codes won't be an issue for
> the DT binding; we can simply make those values the default if
> properties are missing. I assume it's likely that such a strategy will
> work here?

They shouldn't. In fact I think it should be possible to probe them
either using mechanisms built into DSI, or by querying the attached
panel (as matched by the corresponding device tree node).

I haven't done the latter yet because I plan to investigate whether
builtin DSI functionality can be used to probe for the information.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]