Re: [PATCH v3 13/32] drm/exynos: hdmi: remove the i2c drivers and use devtree

From: Tomasz Figa <t.figa@samsung.com>
To: dri-devel@lists.freedesktop.org
Cc: marcheu@chromium.org
Subject: Re: [PATCH v3 13/32] drm/exynos: hdmi: remove the i2c drivers and	use devtree
Date: Thu, 28 Nov 2013 14:30:24 +0100	[thread overview]
Message-ID: <20408902.mr0KEZob0h@amdc1227> (raw)
In-Reply-To: <20131111084426.GA3884@ulmo.nvidia.com>

On Monday 11 of November 2013 09:44:27 Thierry Reding wrote:
> On Sun, Nov 10, 2013 at 09:46:02PM +0100, Tomasz Figa wrote:
> [...]
> > On Tuesday 29 of October 2013 12:12:59 Sean Paul wrote:
> [...]
> > [snip]
> > > @@ -1957,21 +1943,30 @@ static int hdmi_probe(struct platform_device *pdev)
> > >  	}
> > >  
> > >  	/* DDC i2c driver */
> > > -	if (i2c_add_driver(&ddc_driver)) {
> > > -		DRM_ERROR("failed to register ddc i2c driver\n");
> > > -		return -ENOENT;
> > > +	ddc_node = of_find_node_by_name(NULL, "hdmiddc");
> > 
> > This is wrong. You shall not reference a device tree node by its name,
> > except some very specific well-defined cases, such as cpus or memory
> > nodes.
> > 
> > A solution closest to yours, but correct, would be to use the same match
> > table as in the I2C driver you are removing and call
> > of_find_matching_node().
> 
> Isn't the correct solution to use a phandle? That might need the binding
> to change in a backwards incompatible way.

Yes, phandle is an even better option as it can point you precisely to the
node you are interested in, but this will be incompatible, meaning that
you would have to support both variants anyway.

> Then again, if something as
> simple as specifying a DDC I2C bus causes the binding to change in a
> backwards incompatible way then it can't have been a very good binding
> in the first place, right? +1 for unstable DT bindings...

Well, some of already existing bindings should have been definitely marked
unstable, as they haven't been thought and reviewed well enough, if at all
(especially reviewed, as we only started seriously reviewing DT bindings
not so long ago).

Honestly, I'm not quite sure about this binding in particular, especially
how much it would be a problem if we broke compatibility. I mean, how much
tied to old DTBs are existing boards using this binding. The affected
boards are:
 - exynos5250-snow,
 - exynos5250-arndale,
 - exynos5250-smdk5250,
 - exynos5420-smdk5420.
The last three are most likely to be used only with DTB appended, so
I don't think that anyone would complain. However I'm not sure about the
first one, which is supposed to be a Chromebook if I'm not mistaken.

Best regards,
Tomasz