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

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3092 bytes --]

On Thu, Nov 28, 2013 at 02:30:24PM +0100, Tomasz Figa wrote:
> 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.

Oh come on. If a phandle is the right way to do it, then we should just
do it. Will it really be so difficult to carry code for both variants?
If nothing else it will at least set a good example and reduce the risk
of people doing the same mistakes over and over again.

Adding the right binding also gives you a way to start deprecating the
wrong one and eventually remove it. The longer you wait, the more people
will start to use the existing, broken binding and removing it will only
become more difficult over time.

> > 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.

Well, if it's a Chromebook it likely doesn't ship with a completely
mainline kernel. That frees it from the stability requirements, doesn't
it?

Thierry

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

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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