[PATCH v3 13/32] drm/exynos: hdmi: remove the i2c drivers and use devtree
On Fri, Nov 29, 2013 at 2:24 AM, Thierry Reding
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
[PATCH v3 13/32] drm/exynos: hdmi: remove the i2c drivers and use devtree
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
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL:
[PATCH v3 13/32] drm/exynos: hdmi: remove the i2c drivers and use devtree
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
[PATCH v3 13/32] drm/exynos: hdmi: remove the i2c drivers and use devtree
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. 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...
Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL:
[PATCH v3 13/32] drm/exynos: hdmi: remove the i2c drivers and use devtree
Hi Sean, Daniel,
Please see my comments inline.
On Tuesday 29 of October 2013 12:12:59 Sean Paul wrote:
> From: Daniel Kurtz
>
> The i2c client was previously being passed into the hdmi driver via a
> dedicated i2c driver, and then a global variable. This patch removes all
> of that and just uses the device tree to get the i2c_client. This patch
> also properly references the client so we don't lose it before we're
> done with it.
>
> Signed-off-by: Daniel Kurtz
> Signed-off-by: Sean Paul
> ---
>
> Changes in v2:
> - Change include to linux/i2c.h instead of linux/of_i2c.h
> Changes in v3: None
>
> drivers/gpu/drm/exynos/Makefile | 1 -
> drivers/gpu/drm/exynos/exynos_ddc.c | 63
> drivers/gpu/drm/exynos/exynos_hdmi.c| 59 ++
> drivers/gpu/drm/exynos/exynos_hdmi.h| 23
> drivers/gpu/drm/exynos/exynos_hdmiphy.c | 65
> -
> 5 files changed, 27 insertions(+), 184 deletions(-)
> delete mode 100644 drivers/gpu/drm/exynos/exynos_ddc.c
> delete mode 100644 drivers/gpu/drm/exynos/exynos_hdmi.h
> delete mode 100644 drivers/gpu/drm/exynos/exynos_hdmiphy.c
I hope this patch does not alter the existing Exynos HDMI bindings.
This would have to be reflected in respective documentation otherwise
and of course done in a backwards compatible way.
Well, let's see...
> diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c
> b/drivers/gpu/drm/exynos/exynos_hdmi.c
> index d35ab2a..e106309 100644
> --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
[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().
> + if (!ddc_node) {
> + DRM_ERROR("Failed to find ddc node in device tree\n");
> + return -ENODEV;
> + }
> + hdata->ddc_port = of_find_i2c_device_by_node(ddc_node);
> + if (!hdata->ddc_port) {
> + DRM_ERROR("Failed to get ddc i2c client by node\n");
> + return -ENODEV;
> }
In general, this looks like a better approach, without using a fake
driver. However you should check how this will play together with runtime
PM of the I2C adapter, as I'm not sure about it.
Both this comment and the comment above apply as well to the hdmiphy
handling.
Best regards,
Tomasz
[PATCH v3 13/32] drm/exynos: hdmi: remove the i2c drivers and use devtree
From: Daniel Kurtz
The i2c client was previously being passed into the hdmi driver via a
dedicated i2c driver, and then a global variable. This patch removes all
of that and just uses the device tree to get the i2c_client. This patch
also properly references the client so we don't lose it before we're
done with it.
Signed-off-by: Daniel Kurtz
Signed-off-by: Sean Paul
---
Changes in v2:
- Change include to linux/i2c.h instead of linux/of_i2c.h
Changes in v3: None
drivers/gpu/drm/exynos/Makefile | 1 -
drivers/gpu/drm/exynos/exynos_ddc.c | 63
drivers/gpu/drm/exynos/exynos_hdmi.c| 59 ++
drivers/gpu/drm/exynos/exynos_hdmi.h| 23
drivers/gpu/drm/exynos/exynos_hdmiphy.c | 65 -
5 files changed, 27 insertions(+), 184 deletions(-)
delete mode 100644 drivers/gpu/drm/exynos/exynos_ddc.c
delete mode 100644 drivers/gpu/drm/exynos/exynos_hdmi.h
delete mode 100644 drivers/gpu/drm/exynos/exynos_hdmiphy.c
diff --git a/drivers/gpu/drm/exynos/Makefile b/drivers/gpu/drm/exynos/Makefile
index 639b49e..819961a 100644
--- a/drivers/gpu/drm/exynos/Makefile
+++ b/drivers/gpu/drm/exynos/Makefile
@@ -12,7 +12,6 @@ exynosdrm-$(CONFIG_DRM_EXYNOS_IOMMU) += exynos_drm_iommu.o
exynosdrm-$(CONFIG_DRM_EXYNOS_DMABUF) += exynos_drm_dmabuf.o
exynosdrm-$(CONFIG_DRM_EXYNOS_FIMD)+= exynos_drm_fimd.o
exynosdrm-$(CONFIG_DRM_EXYNOS_HDMI)+= exynos_hdmi.o exynos_mixer.o \
- exynos_ddc.o exynos_hdmiphy.o \
exynos_drm_hdmi.o
exynosdrm-$(CONFIG_DRM_EXYNOS_VIDI)+= exynos_drm_vidi.o
exynosdrm-$(CONFIG_DRM_EXYNOS_G2D) += exynos_drm_g2d.o
diff --git a/drivers/gpu/drm/exynos/exynos_ddc.c
b/drivers/gpu/drm/exynos/exynos_ddc.c
deleted file mode 100644
index 6a8c84e..000
--- a/drivers/gpu/drm/exynos/exynos_ddc.c
+++ /dev/null
@@ -1,63 +0,0 @@
-/*
- * Copyright (C) 2011 Samsung Electronics Co.Ltd
- * Authors:
- * Seung-Woo Kim
- * Inki Dae
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms of the GNU General Public License as published by the
- * Free Software Foundation; either version 2 of the License, or (at your
- * option) any later version.
- *
- */
-
-#include
-
-#include
-#include
-#include
-
-#include "exynos_drm_drv.h"
-#include "exynos_hdmi.h"
-
-static int s5p_ddc_probe(struct i2c_client *client,
- const struct i2c_device_id *dev_id)
-{
- hdmi_attach_ddc_client(client);
-
- dev_info(&client->adapter->dev,
- "attached %s into i2c adapter successfully\n",
- client->name);
-
- return 0;
-}
-
-static int s5p_ddc_remove(struct i2c_client *client)
-{
- dev_info(&client->adapter->dev,
- "detached %s from i2c adapter successfully\n",
- client->name);
-
- return 0;
-}
-
-static struct of_device_id hdmiddc_match_types[] = {
- {
- .compatible = "samsung,exynos5-hdmiddc",
- }, {
- .compatible = "samsung,exynos4210-hdmiddc",
- }, {
- /* end node */
- }
-};
-
-struct i2c_driver ddc_driver = {
- .driver = {
- .name = "exynos-hdmiddc",
- .owner = THIS_MODULE,
- .of_match_table = hdmiddc_match_types,
- },
- .probe = s5p_ddc_probe,
- .remove = s5p_ddc_remove,
- .command= NULL,
-};
diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c
b/drivers/gpu/drm/exynos/exynos_hdmi.c
index d35ab2a..e106309 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -33,6 +33,7 @@
#include
#include
#include
+#include
#include
#include
@@ -40,8 +41,6 @@
#include "exynos_drm_drv.h"
#include "exynos_drm_hdmi.h"
-#include "exynos_hdmi.h"
-
#include
#include
@@ -1849,20 +1848,6 @@ fail:
return -ENODEV;
}
-static struct i2c_client *hdmi_ddc, *hdmi_hdmiphy;
-
-void hdmi_attach_ddc_client(struct i2c_client *ddc)
-{
- if (ddc)
- hdmi_ddc = ddc;
-}
-
-void hdmi_attach_hdmiphy_client(struct i2c_client *hdmiphy)
-{
- if (hdmiphy)
- hdmi_hdmiphy = hdmiphy;
-}
-
static struct s5p_hdmi_platform_data *drm_hdmi_dt_parse_pdata
(struct device *dev)
{
@@ -1907,6 +1892,7 @@ static int hdmi_probe(struct platform_device *pdev)
struct s5p_hdmi_platform_data *pdata;
struct resource *res;
const struct of_device_id *match;
+ struct device_node *ddc_node, *phy_node;
int ret;
if (!dev->of_node)
@@ -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");
-
