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

[Olof Johansson]

On Fri, Nov 29, 2013 at 2:24 AM, Thierry Reding
<thierry.reding at gmail.com> wrote:
> 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?

Correct, there are absolutely no stability requirements between
mainline and the chromium.org kernel.


-Olof