[RESEND PATCH v2 1/3] drm: dw_hdmi: use of_get_i2c_adapter_by_node interface

2016-08-23 Thread Vladimir Zapolskiy 
Hello Russell,

On 08/18/2016 05:32 PM, Russell King - ARM Linux wrote:
> On Tue, Aug 16, 2016 at 11:26:43PM +0300, Vladimir Zapolskiy wrote:
>> This change is needed to properly lock I2C bus driver, which serves
>> DDC.
>>
>> The change fixes an overflow over zero of I2C bus driver user counter:
>>
>>   root at imx6q:~# lsmod
>>   Not tainted
>>   dw_hdmi_ahb_audio 4082 0 - Live 0xbf02c000
>>   dw_hdmi_imx 3498 0 - Live 0xbf00d000
>>   dw_hdmi 16398 2 dw_hdmi_ahb_audio,dw_hdmi_imx, Live 0xbf004000
>>   i2c_imx 16687 0 - Live 0xbf017000
>>
>>   root at imx6q:~# rmmod dw_hdmi_imx
>>   root at imx6q:~# lsmod
>>   Not tainted
>>   dw_hdmi_ahb_audio 4082 0 - Live 0xbf02c000
>>   dw_hdmi 16398 1 dw_hdmi_ahb_audio, Live 0xbf004000
>>   i2c_imx 16687 -1 - Live 0xbf017000
>> ^^
>>
>>   root at imx6q:~# rmmod i2c_imx
>>   rmmod: ERROR: Module i2c_imx is in use
>>
>> Note that prior to this change put_device() coupled with
>> of_find_i2c_adapter_by_node() was missing on error path of
>> dw_hdmi_bind(), added i2c_put_adapter() there along with the change.
>
> I *guess* this is the right thing, but it would be nice to see the
> results with the patch applied in the commit description.  Nevertheless:
>
> Acked-by: Russell King 
>

thank you for review.

For information this is a result after applying the fix (+1 to i2c_imx users):

root at imx6q# lsmod
 Not tainted
dw_hdmi_ahb_audio 4082 0 - Live 0xbf02c000
i2c_imx 16687 1 - Live 0xbf011000
dw_hdmi_imx 3498 0 - Live 0xbf00d000
dw_hdmi 18925 2 dw_hdmi_ahb_audio,dw_hdmi_imx, Live 0xbf004000

root at imx6q:~# rmmod dw_hdmi_imx

root at imx6q:~# lsmod
 Not tainted
dw_hdmi_ahb_audio 4082 0 - Live 0xbf02c000
i2c_imx 16687 0 - Live 0xbf011000
dw_hdmi 18925 1 dw_hdmi_ahb_audio, Live 0xbf004000

No weird negative use count.

I have another pending change against DW HDMI, to avoid git-am conflicts
I'll rebase it on top of this one and resend today later on.

>
> I'd also like to see the DDC bits extracted from the various imx
> drivers, because this is surely common code - I've had this floating
> around for a few years but never got around to finishing it off and
> submitting it.  If folk think this is a good idea, and want to take
> the idea on, that's fine by me.
>
>  drivers/gpu/drm/Makefile|  2 +
>  drivers/gpu/drm/bridge/dw-hdmi.c| 98 
> +
>  drivers/gpu/drm/drm_ddc_connector.c | 91 ++
>  drivers/gpu/drm/imx/imx-tve.c   | 59 ++
>  include/drm/drm_ddc_connector.h | 33 +
>  5 files changed, 163 insertions(+), 120 deletions(-)

I've briefly reviewed the changes and in my opinion this is a good
to have generalization of DDC connector, hopefully DRM people agree.

Moreover I assume that in case of getting modes over I2C/DDC most of
the .get_modes callbacks share almost the same code sequence:

   drm_get_edid()
   drm_detect_hdmi_monitor()
   drm_mode_connector_update_edid_property()
   drm_add_edid_modes()
   drm_edid_to_eld()

I'm not absolutely sure, but probably this can be generalized as well.

--
With best wishes,
Vladimir

[RESEND PATCH v2 1/3] drm: dw_hdmi: use of_get_i2c_adapter_by_node interface

2016-08-18 Thread Russell King - ARM Linux 
On Tue, Aug 16, 2016 at 11:26:43PM +0300, Vladimir Zapolskiy wrote:
> This change is needed to properly lock I2C bus driver, which serves
> DDC.
> 
> The change fixes an overflow over zero of I2C bus driver user counter:
> 
>   root at imx6q:~# lsmod
>   Not tainted
>   dw_hdmi_ahb_audio 4082 0 - Live 0xbf02c000
>   dw_hdmi_imx 3498 0 - Live 0xbf00d000
>   dw_hdmi 16398 2 dw_hdmi_ahb_audio,dw_hdmi_imx, Live 0xbf004000
>   i2c_imx 16687 0 - Live 0xbf017000
> 
>   root at imx6q:~# rmmod dw_hdmi_imx
>   root at imx6q:~# lsmod
>   Not tainted
>   dw_hdmi_ahb_audio 4082 0 - Live 0xbf02c000
>   dw_hdmi 16398 1 dw_hdmi_ahb_audio, Live 0xbf004000
>   i2c_imx 16687 -1 - Live 0xbf017000
> ^^
> 
>   root at imx6q:~# rmmod i2c_imx
>   rmmod: ERROR: Module i2c_imx is in use
> 
> Note that prior to this change put_device() coupled with
> of_find_i2c_adapter_by_node() was missing on error path of
> dw_hdmi_bind(), added i2c_put_adapter() there along with the change.

I *guess* this is the right thing, but it would be nice to see the
results with the patch applied in the commit description.  Nevertheless:

Acked-by: Russell King 


I'd also like to see the DDC bits extracted from the various imx
drivers, because this is surely common code - I've had this floating
around for a few years but never got around to finishing it off and
submitting it.  If folk think this is a good idea, and want to take
the idea on, that's fine by me.

 drivers/gpu/drm/Makefile|  2 +
 drivers/gpu/drm/bridge/dw-hdmi.c| 98 +
 drivers/gpu/drm/drm_ddc_connector.c | 91 ++
 drivers/gpu/drm/imx/imx-tve.c   | 59 ++
 include/drm/drm_ddc_connector.h | 33 +
 5 files changed, 163 insertions(+), 120 deletions(-)

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 0238bf8bc8c3..e31c72f86f8c 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -21,6 +21,8 @@ drm-$(CONFIG_DRM_PANEL) += drm_panel.o
 drm-$(CONFIG_OF) += drm_of.o
 drm-$(CONFIG_AGP) += drm_agpsupport.o

+drm-y += drm_ddc_connector.o
+
 drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
drm_kms_helper_common.o drm_dp_dual_mode_helper.o \
diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index 65dd102689ef..786c0c076585 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -27,6 +27,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 

 #include "dw-hdmi.h"
@@ -103,7 +104,7 @@ struct hdmi_data_info {
 };

 struct dw_hdmi {
-   struct drm_connector connector;
+   struct drm_ddc_connector *ddc_conn;
struct drm_encoder *encoder;
struct drm_bridge *bridge;

@@ -124,10 +125,7 @@ struct dw_hdmi {
bool phy_enabled;
struct drm_display_mode previous_mode;

-   struct i2c_adapter *ddc;
void __iomem *regs;
-   bool sink_is_hdmi;
-   bool sink_has_audio;

struct mutex mutex; /* for state below and previous_mode */
enum drm_connector_force force; /* mutex-protected force state */
@@ -862,7 +860,7 @@ static int dw_hdmi_phy_init(struct dw_hdmi *hdmi)
bool cscon;

/*check csc whether needed activated in HDMI mode */
-   cscon = hdmi->sink_is_hdmi && is_color_space_conversion(hdmi);
+   cscon = hdmi->ddc_conn->sink_is_hdmi && is_color_space_conversion(hdmi);

/* HDMI Phy spec says to do the phy initialization sequence twice */
for (i = 0; i < 2; i++) {
@@ -1039,7 +1037,7 @@ static void hdmi_av_composer(struct dw_hdmi *hdmi,
HDMI_FC_INVIDCONF_IN_I_P_INTERLACED :
HDMI_FC_INVIDCONF_IN_I_P_PROGRESSIVE;

-   inv_val |= hdmi->sink_is_hdmi ?
+   inv_val |= hdmi->ddc_conn->sink_is_hdmi ?
HDMI_FC_INVIDCONF_DVI_MODEZ_HDMI_MODE :
HDMI_FC_INVIDCONF_DVI_MODEZ_DVI_MODE;

@@ -1238,7 +1236,7 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct 
drm_display_mode *mode)
/* HDMI Initialization Step B.3 */
dw_hdmi_enable_video_path(hdmi);

-   if (hdmi->sink_has_audio) {
+   if (hdmi->ddc_conn->sink_has_audio) {
dev_dbg(hdmi->dev, "sink has audio support\n");

/* HDMI Initialization Step E - Configure audio */
@@ -1262,7 +1260,7 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct 
drm_display_mode *mode)
hdmi_tx_hdcp_config(hdmi);

dw_hdmi_clear_overflow(hdmi);
-   if (hdmi->cable_plugin && hdmi->sink_is_hdmi)
+   if (hdmi->cable_plugin && hdmi->ddc_conn->sink_is_hdmi)
hdmi_enable_overflow_interrupts(hdmi);

return 0;
@@ -1438,8 +1436,7 @@ static void dw_hdmi_bridge_enable(struct drm_bridge 
*bridge)
 static enum drm_connector_status

[RESEND PATCH v2 1/3] drm: dw_hdmi: use of_get_i2c_adapter_by_node interface

2016-08-17 Thread Vladimir Zapolskiy 
This change is needed to properly lock I2C bus driver, which serves
DDC.

The change fixes an overflow over zero of I2C bus driver user counter:

  root at imx6q:~# lsmod
  Not tainted
  dw_hdmi_ahb_audio 4082 0 - Live 0xbf02c000
  dw_hdmi_imx 3498 0 - Live 0xbf00d000
  dw_hdmi 16398 2 dw_hdmi_ahb_audio,dw_hdmi_imx, Live 0xbf004000
  i2c_imx 16687 0 - Live 0xbf017000

  root at imx6q:~# rmmod dw_hdmi_imx
  root at imx6q:~# lsmod
  Not tainted
  dw_hdmi_ahb_audio 4082 0 - Live 0xbf02c000
  dw_hdmi 16398 1 dw_hdmi_ahb_audio, Live 0xbf004000
  i2c_imx 16687 -1 - Live 0xbf017000
^^

  root at imx6q:~# rmmod i2c_imx
  rmmod: ERROR: Module i2c_imx is in use

Note that prior to this change put_device() coupled with
of_find_i2c_adapter_by_node() was missing on error path of
dw_hdmi_bind(), added i2c_put_adapter() there along with the change.

Signed-off-by: Vladimir Zapolskiy 
Cc: Russell King 
Cc: Philipp Zabel 
Cc: Fabio Estevam 
---
 drivers/gpu/drm/bridge/dw-hdmi.c | 14 +-
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index 77ab47341658..ce3527cd0d25 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -1686,7 +1686,7 @@ int dw_hdmi_bind(struct device *dev, struct device 
*master,

ddc_node = of_parse_phandle(np, "ddc-i2c-bus", 0);
if (ddc_node) {
-   hdmi->ddc = of_find_i2c_adapter_by_node(ddc_node);
+   hdmi->ddc = of_get_i2c_adapter_by_node(ddc_node);
of_node_put(ddc_node);
if (!hdmi->ddc) {
dev_dbg(hdmi->dev, "failed to read ddc node\n");
@@ -1698,20 +1698,22 @@ int dw_hdmi_bind(struct device *dev, struct device 
*master,
}

hdmi->regs = devm_ioremap_resource(dev, iores);
-   if (IS_ERR(hdmi->regs))
-   return PTR_ERR(hdmi->regs);
+   if (IS_ERR(hdmi->regs)) {
+   ret = PTR_ERR(hdmi->regs);
+   goto err_res;
+   }

hdmi->isfr_clk = devm_clk_get(hdmi->dev, "isfr");
if (IS_ERR(hdmi->isfr_clk)) {
ret = PTR_ERR(hdmi->isfr_clk);
dev_err(hdmi->dev, "Unable to get HDMI isfr clk: %d\n", ret);
-   return ret;
+   goto err_res;
}

ret = clk_prepare_enable(hdmi->isfr_clk);
if (ret) {
dev_err(hdmi->dev, "Cannot enable HDMI isfr clock: %d\n", ret);
-   return ret;
+   goto err_res;
}

hdmi->iahb_clk = devm_clk_get(hdmi->dev, "iahb");
@@ -1797,6 +1799,8 @@ err_iahb:
clk_disable_unprepare(hdmi->iahb_clk);
 err_isfr:
clk_disable_unprepare(hdmi->isfr_clk);
+err_res:
+   i2c_put_adapter(hdmi->ddc);

return ret;
 }
-- 
2.8.1

[PATCH v2 1/3] drm: dw_hdmi: use of_get_i2c_adapter_by_node interface

2015-10-12 Thread Vladimir Zapolskiy 
David, Russel,

ping.

On 23.09.2015 00:48, Vladimir Zapolskiy wrote:
> This change is needed to properly lock I2C bus driver, which serves DDC.
> 
> The change fixes an overflow over zero of I2C bus driver user counter:
> 
> root at mx6q:~# lsmod | grep i2c
> i2c_imx15348  0
> root at mx6q:~# lsmod | grep dw_hdmi_imx
> dw_hdmi_imx 3567  0
> dw_hdmi15850  1 dw_hdmi_imx
> imxdrm  8610  3 dw_hdmi_imx,imx_ipuv3_crtc,imx_ldb
> root at mx6q:~# rmmod dw_hdmi_imx
> root at mx6q:~# lsmod | grep i2c
> i2c_imx15348  -1
> 
>  ^
> 
> root at mx6q:~# rmmod i2c_imx
> rmmod: ERROR: Module i2c_imx is in use
> 
> Note that prior to this change put_device() coupled with
> of_find_i2c_adapter_by_node() was missing on error path of
> dw_hdmi_bind(), added i2c_put_adapter() there along with the change.
> 
> Signed-off-by: Vladimir Zapolskiy 
> Cc: Russell King 
> Cc: Philipp Zabel 
> Cc: Andy Yan 

--
With best wishes,
Vladimir

[PATCH v2 1/3] drm: dw_hdmi: use of_get_i2c_adapter_by_node interface

2015-09-23 Thread Vladimir Zapolskiy 
This change is needed to properly lock I2C bus driver, which serves DDC.

The change fixes an overflow over zero of I2C bus driver user counter:

root at mx6q:~# lsmod | grep i2c
i2c_imx15348  0
root at mx6q:~# lsmod | grep dw_hdmi_imx
dw_hdmi_imx 3567  0
dw_hdmi15850  1 dw_hdmi_imx
imxdrm  8610  3 dw_hdmi_imx,imx_ipuv3_crtc,imx_ldb
root at mx6q:~# rmmod dw_hdmi_imx
root at mx6q:~# lsmod | grep i2c
i2c_imx15348  -1

 ^

root at mx6q:~# rmmod i2c_imx
rmmod: ERROR: Module i2c_imx is in use

Note that prior to this change put_device() coupled with
of_find_i2c_adapter_by_node() was missing on error path of
dw_hdmi_bind(), added i2c_put_adapter() there along with the change.

Signed-off-by: Vladimir Zapolskiy 
Cc: Russell King 
Cc: Philipp Zabel 
Cc: Andy Yan 
---
Changes from v1 to v2:
- none

 drivers/gpu/drm/bridge/dw_hdmi.c | 14 +-
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c
index 0083d4e..c2d804f 100644
--- a/drivers/gpu/drm/bridge/dw_hdmi.c
+++ b/drivers/gpu/drm/bridge/dw_hdmi.c
@@ -1638,7 +1638,7 @@ int dw_hdmi_bind(struct device *dev, struct device 
*master,

ddc_node = of_parse_phandle(np, "ddc-i2c-bus", 0);
if (ddc_node) {
-   hdmi->ddc = of_find_i2c_adapter_by_node(ddc_node);
+   hdmi->ddc = of_get_i2c_adapter_by_node(ddc_node);
of_node_put(ddc_node);
if (!hdmi->ddc) {
dev_dbg(hdmi->dev, "failed to read ddc node\n");
@@ -1650,20 +1650,22 @@ int dw_hdmi_bind(struct device *dev, struct device 
*master,
}

hdmi->regs = devm_ioremap_resource(dev, iores);
-   if (IS_ERR(hdmi->regs))
-   return PTR_ERR(hdmi->regs);
+   if (IS_ERR(hdmi->regs)) {
+   ret = PTR_ERR(hdmi->regs);
+   goto err_res;
+   }

hdmi->isfr_clk = devm_clk_get(hdmi->dev, "isfr");
if (IS_ERR(hdmi->isfr_clk)) {
ret = PTR_ERR(hdmi->isfr_clk);
dev_err(hdmi->dev, "Unable to get HDMI isfr clk: %d\n", ret);
-   return ret;
+   goto err_res;
}

ret = clk_prepare_enable(hdmi->isfr_clk);
if (ret) {
dev_err(hdmi->dev, "Cannot enable HDMI isfr clock: %d\n", ret);
-   return ret;
+   goto err_res;
}

hdmi->iahb_clk = devm_clk_get(hdmi->dev, "iahb");
@@ -1729,6 +1731,8 @@ err_iahb:
clk_disable_unprepare(hdmi->iahb_clk);
 err_isfr:
clk_disable_unprepare(hdmi->isfr_clk);
+err_res:
+   i2c_put_adapter(hdmi->ddc);

return ret;
 }
-- 
2.5.0