| Message ID | 1391116773-28471-15-git-send-email-seanpaul@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On 30.01.2014 22:19, Sean Paul wrote: > From: Daniel Kurtz <djkurtz@chromium.org> > > 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 <djkurtz@chromium.org> > [seanpaul changed to phandle lookup instead of using of node name] > Signed-off-by: Sean Paul <seanpaul@chromium.org> > --- > > Changes in v2: > - Change include to linux/i2c.h instead of linux/of_i2c.h > Changes in v3: None > Changes in v4: > - Changed to find phy via phandle instead of by name > > .../devicetree/bindings/video/exynos_hdmi.txt | 5 ++ > 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 ---------------------- > 6 files changed, 32 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/Documentation/devicetree/bindings/video/exynos_hdmi.txt b/Documentation/devicetree/bindings/video/exynos_hdmi.txt > index 50decf8..f9187a2 100644 > --- a/Documentation/devicetree/bindings/video/exynos_hdmi.txt > +++ b/Documentation/devicetree/bindings/video/exynos_hdmi.txt > @@ -25,6 +25,9 @@ Required properties: > sclk_pixel. > - clock-names: aliases as per driver requirements for above clock IDs: > "hdmi", "sclk_hdmi", "sclk_pixel", "sclk_hdmiphy" and "mout_hdmi". > +- ddc: phandle to the hdmi ddc node > +- phy: phandle to the hdmi phy node > + Adding new required properties to an already defined binding is breaking backwards compatibility, which is supposed to be preserved, since DT is an ABI. Now, I'm not really a big fan of DT stability, but if we decide to maintain it for other Exynos drivers as well (e.g. USB), then we probably should do the same here... Best regards, Tomasz
2014-02-08 11:52 GMT+09:00 Tomasz Figa <tomasz.figa@gmail.com>: > Hi, > > > On 30.01.2014 22:19, Sean Paul wrote: >> >> From: Daniel Kurtz <djkurtz@chromium.org> >> >> 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 <djkurtz@chromium.org> >> [seanpaul changed to phandle lookup instead of using of node name] >> Signed-off-by: Sean Paul <seanpaul@chromium.org> >> --- >> >> Changes in v2: >> - Change include to linux/i2c.h instead of linux/of_i2c.h >> Changes in v3: None >> Changes in v4: >> - Changed to find phy via phandle instead of by name >> >> .../devicetree/bindings/video/exynos_hdmi.txt | 5 ++ >> 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 >> ---------------------- >> 6 files changed, 32 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/Documentation/devicetree/bindings/video/exynos_hdmi.txt >> b/Documentation/devicetree/bindings/video/exynos_hdmi.txt >> index 50decf8..f9187a2 100644 >> --- a/Documentation/devicetree/bindings/video/exynos_hdmi.txt >> +++ b/Documentation/devicetree/bindings/video/exynos_hdmi.txt >> @@ -25,6 +25,9 @@ Required properties: >> sclk_pixel. >> - clock-names: aliases as per driver requirements for above clock IDs: >> "hdmi", "sclk_hdmi", "sclk_pixel", "sclk_hdmiphy" and "mout_hdmi". >> +- ddc: phandle to the hdmi ddc node >> +- phy: phandle to the hdmi phy node >> + > > > Adding new required properties to an already defined binding is breaking > backwards compatibility, which is supposed to be preserved, since DT is an > ABI. Right, that's a problem. If we use this patch, and we add additional codes for these property to hdmi driver, then the existing dtb will be broken by dt binding fail. However, as long as I know, there is another case that phy property is used like this way; MIPI-CSI device node. And also it seems reasonable to add ddc property in similar reason if we could handle that the existing dt node is deplicated. So what we could select I think are, 1. to deplicate the existing dt node, and using this patch. 2. or, to separate phy and ddc parts as each module so that each module can bind device tree. However, this way would make hdmi part of exynos drm to be complicated because we should resolve probe ordering issue. It seems that we need to discuss and make a consensus about this issue. Tomasz and Sean, please, share your opinion. I tend to proper 1. Thanks, Inki Dae > > Now, I'm not really a big fan of DT stability, but if we decide to maintain > it for other Exynos drivers as well (e.g. USB), then we probably should do > the same here... > > Best regards, > Tomasz > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On 10.02.2014 08:30, Inki Dae wrote: > 2014-02-08 11:52 GMT+09:00 Tomasz Figa <tomasz.figa@gmail.com>: >> Hi, >> >> >> On 30.01.2014 22:19, Sean Paul wrote: >>> >>> From: Daniel Kurtz <djkurtz@chromium.org> >>> >>> 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 <djkurtz@chromium.org> >>> [seanpaul changed to phandle lookup instead of using of node name] >>> Signed-off-by: Sean Paul <seanpaul@chromium.org> >>> --- >>> >>> Changes in v2: >>> - Change include to linux/i2c.h instead of linux/of_i2c.h >>> Changes in v3: None >>> Changes in v4: >>> - Changed to find phy via phandle instead of by name >>> >>> .../devicetree/bindings/video/exynos_hdmi.txt | 5 ++ >>> 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 >>> ---------------------- >>> 6 files changed, 32 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/Documentation/devicetree/bindings/video/exynos_hdmi.txt >>> b/Documentation/devicetree/bindings/video/exynos_hdmi.txt >>> index 50decf8..f9187a2 100644 >>> --- a/Documentation/devicetree/bindings/video/exynos_hdmi.txt >>> +++ b/Documentation/devicetree/bindings/video/exynos_hdmi.txt >>> @@ -25,6 +25,9 @@ Required properties: >>> sclk_pixel. >>> - clock-names: aliases as per driver requirements for above clock IDs: >>> "hdmi", "sclk_hdmi", "sclk_pixel", "sclk_hdmiphy" and "mout_hdmi". >>> +- ddc: phandle to the hdmi ddc node >>> +- phy: phandle to the hdmi phy node >>> + >> >> >> Adding new required properties to an already defined binding is breaking >> backwards compatibility, which is supposed to be preserved, since DT is an >> ABI. > > Right, that's a problem. If we use this patch, and we add additional > codes for these property to hdmi driver, then the existing dtb will be > broken by dt binding fail. However, as long as I know, there is > another case that phy property is used like this way; MIPI-CSI device > node. And also it seems reasonable to add ddc property in similar > reason if we could handle that the existing dt node is deplicated. > > So what we could select I think are, > 1. to deplicate the existing dt node, and using this patch. > 2. or, to separate phy and ddc parts as each module so that each > module can bind device tree. However, this way would make hdmi part of > exynos drm to be complicated because we should resolve probe ordering > issue. > > It seems that we need to discuss and make a consensus about this issue. > > Tomasz and Sean, please, share your opinion. I tend to proper 1. I tend to prefer option 1 too. I don't have anything against this patch itself. All I'm just worried about is applying different standards to patches changing DT bindings depending on subsystem (and authors?). Either we declare that we are okay with making Exynos-specific bindings unstable or we keep stability for all of them. I wouldn't be upset if we went with the former, as the whole stability making it impossible to get rid of some fugly broken bindings is getting more and more annoying. Best regards, Tomasz
On Fri, Feb 7, 2014 at 6:52 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote: > Adding new required properties to an already defined binding is breaking > backwards compatibility, which is supposed to be preserved, since DT is an > ABI. > > Now, I'm not really a big fan of DT stability, but if we decide to maintain > it for other Exynos drivers as well (e.g. USB), then we probably should do > the same here... A binding without users _can_ be changed. If you're sure that there are no users of the old binding, and that any users can and will (without complaint or surprise) update to a newer DT with the newer binding, then you can change it. I'm not sure if the cat is out of the bag w.r.t. users of upstream Exynos kernels that this is true or not. -Olof
On 12.02.2014 00:02, Olof Johansson wrote: > On Fri, Feb 7, 2014 at 6:52 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote: > >> Adding new required properties to an already defined binding is breaking >> backwards compatibility, which is supposed to be preserved, since DT is an >> ABI. >> >> Now, I'm not really a big fan of DT stability, but if we decide to maintain >> it for other Exynos drivers as well (e.g. USB), then we probably should do >> the same here... > > A binding without users _can_ be changed. Of course it can. However hdmi binding apparently already has users, since git grep hdmi over Exynos dts files shows 4 boards using HDMI, so I'm not sure it's the case here. > If you're sure that there > are no users of the old binding, and that any users can and will > (without complaint or surprise) update to a newer DT with the newer > binding, then you can change it. I'm not sure if the cat is out of the > bag w.r.t. users of upstream Exynos kernels that this is true or not. The question is whether there are really users of upstream kernels that would care, i.e. don't always simply use DTB and kernel built from the same tree. Best regards, Tomasz
Hi Daniel, I think that it would be better to change the semantic of phy and ddc bindings. Rather than pointing to I2C client it should point to I2C bus instead. The exynos DRM driver can create dummy I2C clients using i2c_new_dummy() function. There is quite strong rationale for this: - DDC is always accessible on fixed addresses described in HDMI specification. - HDMIPHY (including I2C address) is a part of HDMI IP and it is bound to specific version of Exynos SoC - no need to add virtual nodes in DTS - NVIDIA already use bindings to DDC bus instead of DDC client. Take a look to arch/arm/boot/dts/tegra*.dts Regards, Tomasz Stanislawski
Hi Tomasz, 2014-02-14 23:13 GMT+09:00 Tomasz Stanislawski <t.stanislaws@samsung.com>: > Hi Daniel, > I think that it would be better to change the semantic of phy and ddc > bindings. > > Rather than pointing to I2C client it should point to I2C bus instead. > The exynos DRM driver can create dummy I2C clients using i2c_new_dummy() > function. It seems better way. As of now, all we need for HDMI DDC is i2c_adapter (including i2c phy), not i2c_client. > > There is quite strong rationale for this: > - DDC is always accessible on fixed addresses described in HDMI specification. > - HDMIPHY (including I2C address) is a part of HDMI IP and it is bound to > specific version of Exynos SoC > - no need to add virtual nodes in DTS You mean hdmiddc and hdmiphy nodes? If so, I think they are real nodes, not virtual nodes. Otherwise, plz give me more comments. Thanks, Inki Dae > - NVIDIA already use bindings to DDC bus instead of DDC client. Take a look to > arch/arm/boot/dts/tegra*.dts > > Regards, > Tomasz Stanislawski > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
2014-01-31 6:19 GMT+09:00 Sean Paul <seanpaul@chromium.org>: > From: Daniel Kurtz <djkurtz@chromium.org> > > 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 <djkurtz@chromium.org> > [seanpaul changed to phandle lookup instead of using of node name] > Signed-off-by: Sean Paul <seanpaul@chromium.org> > --- > > Changes in v2: > - Change include to linux/i2c.h instead of linux/of_i2c.h > Changes in v3: None > Changes in v4: > - Changed to find phy via phandle instead of by name > > .../devicetree/bindings/video/exynos_hdmi.txt | 5 ++ > 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 ---------------------- > 6 files changed, 32 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/Documentation/devicetree/bindings/video/exynos_hdmi.txt b/Documentation/devicetree/bindings/video/exynos_hdmi.txt > index 50decf8..f9187a2 100644 > --- a/Documentation/devicetree/bindings/video/exynos_hdmi.txt > +++ b/Documentation/devicetree/bindings/video/exynos_hdmi.txt > @@ -25,6 +25,9 @@ Required properties: > sclk_pixel. > - clock-names: aliases as per driver requirements for above clock IDs: > "hdmi", "sclk_hdmi", "sclk_pixel", "sclk_hdmiphy" and "mout_hdmi". > +- ddc: phandle to the hdmi ddc node > +- phy: phandle to the hdmi phy node > + > Example: > > hdmi { > @@ -32,4 +35,6 @@ Example: > reg = <0x14530000 0x100000>; > interrupts = <0 95 0>; > hpd-gpio = <&gpx3 7 1>; > + ddc = <&hdmi_ddc_node>; > + phy = <&hdmi_phy_node>; > }; > 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..0000000 > --- 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 <sw0312.kim@samsung.com> > - * Inki Dae <inki.dae@samsung.com> > - * > - * 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 <drm/drmP.h> > - > -#include <linux/kernel.h> > -#include <linux/i2c.h> > -#include <linux/of.h> > - > -#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 7e70228..97a0e57 100644 > --- a/drivers/gpu/drm/exynos/exynos_hdmi.c > +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c > @@ -33,6 +33,7 @@ > #include <linux/regulator/consumer.h> > #include <linux/io.h> > #include <linux/of.h> > +#include <linux/i2c.h> > #include <linux/of_gpio.h> > > #include <drm/exynos_drm.h> > @@ -40,8 +41,6 @@ > #include "exynos_drm_drv.h" > #include "exynos_drm_hdmi.h" > > -#include "exynos_hdmi.h" > - > #include <linux/gpio.h> > #include <media/s5p_hdmi.h> > > @@ -1855,20 +1854,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) > { > @@ -1913,6 +1898,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) > @@ -1963,21 +1949,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_parse_phandle(dev->of_node, "ddc", 0); > + 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); As Tomasz commented, it would better to get i2c_adapter instead of i2c_client. > + if (!hdata->ddc_port) { > + DRM_ERROR("Failed to get ddc i2c client by node\n"); > + return -ENODEV; > } > - > - hdata->ddc_port = hdmi_ddc; > > /* hdmiphy i2c driver */ > - if (i2c_add_driver(&hdmiphy_driver)) { > - DRM_ERROR("failed to register hdmiphy i2c driver\n"); > - ret = -ENOENT; > + phy_node = of_parse_phandle(dev->of_node, "phy", 0); > + if (!phy_node) { > + DRM_ERROR("Failed to find hdmiphy node in device tree\n"); > + ret = -ENODEV; > + goto err_ddc; > + } > + hdata->hdmiphy_port = of_find_i2c_device_by_node(phy_node); You should also consider other SoCs having APB PHY, not I2C PHY. Actually, Exynos5420 SoC has APB PHY. Thanks, Inki Dae > + if (!hdata->hdmiphy_port) { > + DRM_ERROR("Failed to get hdmi phy i2c client from node\n"); > + ret = -ENODEV; > goto err_ddc; > } > - > - hdata->hdmiphy_port = hdmi_hdmiphy; > > hdata->irq = gpio_to_irq(hdata->hpd_gpio); > if (hdata->irq < 0) { > @@ -2008,22 +2003,22 @@ static int hdmi_probe(struct platform_device *pdev) > return 0; > > err_hdmiphy: > - i2c_del_driver(&hdmiphy_driver); > + put_device(&hdata->hdmiphy_port->dev); > err_ddc: > - i2c_del_driver(&ddc_driver); > + put_device(&hdata->ddc_port->dev); > return ret; > } > > static int hdmi_remove(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > + struct exynos_drm_hdmi_context *ctx = get_hdmi_context(dev); > + struct hdmi_context *hdata = ctx->ctx; > > pm_runtime_disable(dev); > > - /* hdmiphy i2c driver */ > - i2c_del_driver(&hdmiphy_driver); > - /* DDC i2c driver */ > - i2c_del_driver(&ddc_driver); > + put_device(&hdata->hdmiphy_port->dev); > + put_device(&hdata->ddc_port->dev); > > return 0; > } > diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.h b/drivers/gpu/drm/exynos/exynos_hdmi.h > deleted file mode 100644 > index 0ddf395..0000000 > --- a/drivers/gpu/drm/exynos/exynos_hdmi.h > +++ /dev/null > @@ -1,23 +0,0 @@ > -/* > - * > - * Copyright (c) 2011 Samsung Electronics Co., Ltd. > - * Authors: > - * Inki Dae <inki.dae@samsung.com> > - * Seung-Woo Kim <sw0312.kim@samsung.com> > - * > - * 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. > - */ > - > -#ifndef _EXYNOS_HDMI_H_ > -#define _EXYNOS_HDMI_H_ > - > -void hdmi_attach_ddc_client(struct i2c_client *ddc); > -void hdmi_attach_hdmiphy_client(struct i2c_client *hdmiphy); > - > -extern struct i2c_driver hdmiphy_driver; > -extern struct i2c_driver ddc_driver; > - > -#endif > diff --git a/drivers/gpu/drm/exynos/exynos_hdmiphy.c b/drivers/gpu/drm/exynos/exynos_hdmiphy.c > deleted file mode 100644 > index 59abb14..0000000 > --- a/drivers/gpu/drm/exynos/exynos_hdmiphy.c > +++ /dev/null > @@ -1,65 +0,0 @@ > -/* > - * Copyright (C) 2011 Samsung Electronics Co.Ltd > - * Authors: > - * Seung-Woo Kim <sw0312.kim@samsung.com> > - * Inki Dae <inki.dae@samsung.com> > - * > - * 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 <drm/drmP.h> > - > -#include <linux/kernel.h> > -#include <linux/i2c.h> > -#include <linux/of.h> > - > -#include "exynos_drm_drv.h" > -#include "exynos_hdmi.h" > - > - > -static int hdmiphy_probe(struct i2c_client *client, > - const struct i2c_device_id *id) > -{ > - hdmi_attach_hdmiphy_client(client); > - > - dev_info(&client->adapter->dev, "attached s5p_hdmiphy " > - "into i2c adapter successfully\n"); > - > - return 0; > -} > - > -static int hdmiphy_remove(struct i2c_client *client) > -{ > - dev_info(&client->adapter->dev, "detached s5p_hdmiphy " > - "from i2c adapter successfully\n"); > - > - return 0; > -} > - > -static struct of_device_id hdmiphy_match_types[] = { > - { > - .compatible = "samsung,exynos5-hdmiphy", > - }, { > - .compatible = "samsung,exynos4210-hdmiphy", > - }, { > - .compatible = "samsung,exynos4212-hdmiphy", > - }, { > - /* end node */ > - } > -}; > - > -struct i2c_driver hdmiphy_driver = { > - .driver = { > - .name = "exynos-hdmiphy", > - .owner = THIS_MODULE, > - .of_match_table = hdmiphy_match_types, > - }, > - .probe = hdmiphy_probe, > - .remove = hdmiphy_remove, > - .command = NULL, > -}; > -EXPORT_SYMBOL(hdmiphy_driver); > -- > 1.8.5.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Inki, Sorry for a very late reply. On 02/19/2014 12:14 PM, Inki Dae wrote: > Hi Tomasz, > > > 2014-02-14 23:13 GMT+09:00 Tomasz Stanislawski <t.stanislaws@samsung.com>: >> Hi Daniel, >> I think that it would be better to change the semantic of phy and ddc >> bindings. >> >> Rather than pointing to I2C client it should point to I2C bus instead. >> The exynos DRM driver can create dummy I2C clients using i2c_new_dummy() >> function. > > It seems better way. As of now, all we need for HDMI DDC is > i2c_adapter (including i2c phy), not i2c_client. > >> >> There is quite strong rationale for this: >> - DDC is always accessible on fixed addresses described in HDMI specification. >> - HDMIPHY (including I2C address) is a part of HDMI IP and it is bound to >> specific version of Exynos SoC >> - no need to add virtual nodes in DTS > > You mean hdmiddc and hdmiphy nodes? If so, I think they are real > nodes, not virtual nodes. Otherwise, plz give me more comments. > > Thanks, > Inki Dae > The problem is that those nodes have no dedicated driver. Moreover, the I2C address for HDMIPHY is always present on a fixed address dependant on SoC (read HDMI IP) version. Moreover, both devices share HDMI and HDMIPHY share registers from HDMI's pool. Additionally, the hdmiphy I2C client is currently configured using exynos-hdmi code its DRM driver. So technically the hdmiphy bus is only a resource used by exynos-drm-hdmi driver. Regards, Tomasz Stanislawski >> - NVIDIA already use bindings to DDC bus instead of DDC client. Take a look to >> arch/arm/boot/dts/tegra*.dts >> >> Regards, >> Tomasz Stanislawski >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/Documentation/devicetree/bindings/video/exynos_hdmi.txt b/Documentation/devicetree/bindings/video/exynos_hdmi.txt index 50decf8..f9187a2 100644 --- a/Documentation/devicetree/bindings/video/exynos_hdmi.txt +++ b/Documentation/devicetree/bindings/video/exynos_hdmi.txt @@ -25,6 +25,9 @@ Required properties: sclk_pixel. - clock-names: aliases as per driver requirements for above clock IDs: "hdmi", "sclk_hdmi", "sclk_pixel", "sclk_hdmiphy" and "mout_hdmi". +- ddc: phandle to the hdmi ddc node +- phy: phandle to the hdmi phy node + Example: hdmi { @@ -32,4 +35,6 @@ Example: reg = <0x14530000 0x100000>; interrupts = <0 95 0>; hpd-gpio = <&gpx3 7 1>; + ddc = <&hdmi_ddc_node>; + phy = <&hdmi_phy_node>; }; 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..0000000 --- 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 <sw0312.kim@samsung.com> - * Inki Dae <inki.dae@samsung.com> - * - * 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 <drm/drmP.h> - -#include <linux/kernel.h> -#include <linux/i2c.h> -#include <linux/of.h> - -#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 7e70228..97a0e57 100644 --- a/drivers/gpu/drm/exynos/exynos_hdmi.c +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c @@ -33,6 +33,7 @@ #include <linux/regulator/consumer.h> #include <linux/io.h> #include <linux/of.h> +#include <linux/i2c.h> #include <linux/of_gpio.h> #include <drm/exynos_drm.h> @@ -40,8 +41,6 @@ #include "exynos_drm_drv.h" #include "exynos_drm_hdmi.h" -#include "exynos_hdmi.h" - #include <linux/gpio.h> #include <media/s5p_hdmi.h> @@ -1855,20 +1854,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) { @@ -1913,6 +1898,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) @@ -1963,21 +1949,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_parse_phandle(dev->of_node, "ddc", 0); + 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; } - - hdata->ddc_port = hdmi_ddc; /* hdmiphy i2c driver */ - if (i2c_add_driver(&hdmiphy_driver)) { - DRM_ERROR("failed to register hdmiphy i2c driver\n"); - ret = -ENOENT; + phy_node = of_parse_phandle(dev->of_node, "phy", 0); + if (!phy_node) { + DRM_ERROR("Failed to find hdmiphy node in device tree\n"); + ret = -ENODEV; + goto err_ddc; + } + hdata->hdmiphy_port = of_find_i2c_device_by_node(phy_node); + if (!hdata->hdmiphy_port) { + DRM_ERROR("Failed to get hdmi phy i2c client from node\n"); + ret = -ENODEV; goto err_ddc; } - - hdata->hdmiphy_port = hdmi_hdmiphy; hdata->irq = gpio_to_irq(hdata->hpd_gpio); if (hdata->irq < 0) { @@ -2008,22 +2003,22 @@ static int hdmi_probe(struct platform_device *pdev) return 0; err_hdmiphy: - i2c_del_driver(&hdmiphy_driver); + put_device(&hdata->hdmiphy_port->dev); err_ddc: - i2c_del_driver(&ddc_driver); + put_device(&hdata->ddc_port->dev); return ret; } static int hdmi_remove(struct platform_device *pdev) { struct device *dev = &pdev->dev; + struct exynos_drm_hdmi_context *ctx = get_hdmi_context(dev); + struct hdmi_context *hdata = ctx->ctx; pm_runtime_disable(dev); - /* hdmiphy i2c driver */ - i2c_del_driver(&hdmiphy_driver); - /* DDC i2c driver */ - i2c_del_driver(&ddc_driver); + put_device(&hdata->hdmiphy_port->dev); + put_device(&hdata->ddc_port->dev); return 0; } diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.h b/drivers/gpu/drm/exynos/exynos_hdmi.h deleted file mode 100644 index 0ddf395..0000000 --- a/drivers/gpu/drm/exynos/exynos_hdmi.h +++ /dev/null @@ -1,23 +0,0 @@ -/* - * - * Copyright (c) 2011 Samsung Electronics Co., Ltd. - * Authors: - * Inki Dae <inki.dae@samsung.com> - * Seung-Woo Kim <sw0312.kim@samsung.com> - * - * 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. - */ - -#ifndef _EXYNOS_HDMI_H_ -#define _EXYNOS_HDMI_H_ - -void hdmi_attach_ddc_client(struct i2c_client *ddc); -void hdmi_attach_hdmiphy_client(struct i2c_client *hdmiphy); - -extern struct i2c_driver hdmiphy_driver; -extern struct i2c_driver ddc_driver; - -#endif diff --git a/drivers/gpu/drm/exynos/exynos_hdmiphy.c b/drivers/gpu/drm/exynos/exynos_hdmiphy.c deleted file mode 100644 index 59abb14..0000000 --- a/drivers/gpu/drm/exynos/exynos_hdmiphy.c +++ /dev/null @@ -1,65 +0,0 @@ -/* - * Copyright (C) 2011 Samsung Electronics Co.Ltd - * Authors: - * Seung-Woo Kim <sw0312.kim@samsung.com> - * Inki Dae <inki.dae@samsung.com> - * - * 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 <drm/drmP.h> - -#include <linux/kernel.h> -#include <linux/i2c.h> -#include <linux/of.h> - -#include "exynos_drm_drv.h" -#include "exynos_hdmi.h" - - -static int hdmiphy_probe(struct i2c_client *client, - const struct i2c_device_id *id) -{ - hdmi_attach_hdmiphy_client(client); - - dev_info(&client->adapter->dev, "attached s5p_hdmiphy " - "into i2c adapter successfully\n"); - - return 0; -} - -static int hdmiphy_remove(struct i2c_client *client) -{ - dev_info(&client->adapter->dev, "detached s5p_hdmiphy " - "from i2c adapter successfully\n"); - - return 0; -} - -static struct of_device_id hdmiphy_match_types[] = { - { - .compatible = "samsung,exynos5-hdmiphy", - }, { - .compatible = "samsung,exynos4210-hdmiphy", - }, { - .compatible = "samsung,exynos4212-hdmiphy", - }, { - /* end node */ - } -}; - -struct i2c_driver hdmiphy_driver = { - .driver = { - .name = "exynos-hdmiphy", - .owner = THIS_MODULE, - .of_match_table = hdmiphy_match_types, - }, - .probe = hdmiphy_probe, - .remove = hdmiphy_remove, - .command = NULL, -}; -EXPORT_SYMBOL(hdmiphy_driver);