| Message ID | 1383063198-10526-14-git-send-email-seanpaul@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Sean, Daniel, Please see my comments inline. On Tuesday 29 of October 2013 12:12:59 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> > 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 > > 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
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
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
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
On Fri, Nov 29, 2013 at 2:24 AM, Thierry Reding <thierry.reding@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
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 d35ab2a..e106309 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> @@ -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"); - return -ENOENT; + ddc_node = of_find_node_by_name(NULL, "hdmiddc"); + 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_find_node_by_name(NULL, "hdmiphy"); + 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) { @@ -2002,22 +1997,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);