Re: Revert "arm64: dts: qcom: sm8250: remove bus clock from the mdss node for sm8250 target"

[Vladimir Zapolskiy]

Hi Dmitry,

On 10/14/21 4:54 PM, Dmitry Baryshkov wrote:

From: Amit Pundir 

This reverts commit 001ce9785c0674d913531345e86222c965fc8bf4.

This upstream commit broke AOSP (post Android 12 merge) build
on RB5. The device either silently crashes into USB crash mode
after android boot animation or we see a blank blue screen
with following dpu errors in dmesg:

[  T444] hw recovery is not complete for ctl:3
[  T444] [drm:dpu_encoder_phys_vid_prepare_for_kickoff:539] [dpu error]enc31 
intf1 ctl 3 reset failure: -22
[  T444] [drm:dpu_encoder_phys_vid_wait_for_commit_done:513] [dpu error]vblank 
timeout
[  T444] [drm:dpu_kms_wait_for_commit_done:454] [dpu error]wait for commit done 
returned -110
[C7] [drm:dpu_encoder_frame_done_timeout:2127] [dpu error]enc31 frame done 
timeout
[  T444] [drm:dpu_encoder_phys_vid_wait_for_commit_done:513] [dpu error]vblank 
timeout
[  T444] [drm:dpu_kms_wait_for_commit_done:454] [dpu error]wait for commit done 
returned -110

Signed-off-by: Amit Pundir 


your sob tag is missing.


---
  arch/arm64/boot/dts/qcom/sm8250.dtsi | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi 
b/arch/arm64/boot/dts/qcom/sm8250.dtsi
index 8c15d9fed08f..d12e4cbfc852 100644
--- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
@@ -2590,9 +2590,10 @@
power-domains = < MDSS_GDSC>;
  
  			clocks = < DISP_CC_MDSS_AHB_CLK>,

+< GCC_DISP_HF_AXI_CLK>,
 < GCC_DISP_SF_AXI_CLK>,
 < DISP_CC_MDSS_MDP_CLK>;
-   clock-names = "iface", "nrt_bus", "core";
+   clock-names = "iface", "bus", "nrt_bus", "core";
  
  			assigned-clocks = < DISP_CC_MDSS_MDP_CLK>;

assigned-clock-rates = <46000>;



--
Best wishes,
Vladimir

Re: [PATCH v3 4/5] amba: Make the remove callback return void

[Vladimir Zapolskiy]

On 1/26/21 6:58 PM, Uwe Kleine-König wrote:

All amba drivers return 0 in their remove callback. Together with the
driver core ignoring the return value anyhow, it doesn't make sense to
return a value here.

Change the remove prototype to return void, which makes it explicit that
returning an error value doesn't work as expected. This simplifies changing
the core remove callback to return void, too.

Reviewed-by: Ulf Hansson 
Reviewed-by: Arnd Bergmann 
Acked-by: Alexandre Belloni 
Acked-by: Dmitry Torokhov 
Acked-by: Krzysztof Kozlowski  # for drivers/memory
Acked-by: Mark Brown 
Acked-by: Dmitry Torokhov 
Acked-by: Linus Walleij 
Signed-off-by: Uwe Kleine-König 


For drivers/memory/pl172.c:

Acked-by: Vladimir Zapolskiy 

--
Best wishes,
Vladimir
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH net-next] net: lpc-enet: fix error return code in lpc_mii_init()

[Vladimir Zapolskiy]

On 4/27/20 3:15 PM, Wei Yongjun wrote:
> Fix to return a negative error code from the error handling
> case instead of 0, as done elsewhere in this function.
> 
> Fixes: b7370112f519 ("lpc32xx: Added ethernet driver")
> Signed-off-by: Wei Yongjun 

Acked-by: Vladimir Zapolskiy 

--
Best wishes,
Vladimir
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 27/33] drm/panel-simple: Fix dotclock for Sharp LQ035Q7DB03

[Vladimir Zapolskiy]

Hi Ville,

On 3/2/20 10:34 PM, Ville Syrjala wrote:
> From: Ville Syrjälä 
> 
> The currently listed dotclock disagrees with the currently
> listed vrefresh rate. Change the dotclock to match the vrefresh.
> 
> Someone tell me which (if either) of the dotclock or vreresh is
> correct?

yes, I will tell you, see my answer below.

Adding Linus as a person who may be interested in PL111 specifics.

> Cc: Vladimir Zapolskiy 
> Cc: Rob Herring 
> Cc: Thierry Reding 
> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/panel/panel-simple.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-simple.c 
> b/drivers/gpu/drm/panel/panel-simple.c
> index 3012b47c1e4e..7526af2d6d95 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -2949,7 +2949,7 @@ static const struct panel_desc sharp_lq070y3dg3b = {
>  };
>  
>  static const struct drm_display_mode sharp_lq035q7db03_mode = {
> - .clock = 5500,
> + .clock = 5419,
>   .hdisplay = 240,
>   .hsync_start = 240 + 16,
>   .hsync_end = 240 + 16 + 7,
> 

Here .clock is correct, you may find the usage of the panel in
lpc3250-phy3250.dts example, and the PL111 controller on the SoC
won't be able to provide the exactly computed `.clock = 5419'.

So, I have to NAK this change, in this example the difference
between the declared and the computed .vreresh is one Hz, which
I hope can be accepted as negligible and ignorable, otherwise,
if you insist, please correct the .vrefresh from 60 to 61.

--
Best wishes,
Vladimir
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2] drm/panel: simple: Add Sharp LQ035Q7DB03 panel support

[Vladimir Zapolskiy]

The change adds support for Sharp LQ035Q7DB03 3.5" QVGA TFT panel.

Note that this aged panel is already found in the kernel sources,
for instance in board mach files mach-mx21ads.c, mach-mx27ads.c,
mach-pcm043.c, lpd270.c and imx27-phytec-phycore-rdk.dts.

Signed-off-by: Vladimir Zapolskiy 
---
Changes from v1 to v2:
* added description of power-supply, enable-gpios and backlight properties

 .../bindings/display/panel/sharp,lq035q7db03.txt   | 12 ++
 drivers/gpu/drm/panel/panel-simple.c   | 27 ++
 2 files changed, 39 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/display/panel/sharp,lq035q7db03.txt

diff --git 
a/Documentation/devicetree/bindings/display/panel/sharp,lq035q7db03.txt 
b/Documentation/devicetree/bindings/display/panel/sharp,lq035q7db03.txt
new file mode 100644
index 000..0753f69
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/sharp,lq035q7db03.txt
@@ -0,0 +1,12 @@
+Sharp LQ035Q7DB03 3.5" QVGA TFT LCD panel
+
+Required properties:
+- compatible: should be "sharp,lq035q7db03"
+- power-supply: phandle of the regulator that provides the supply voltage
+
+Optional properties:
+- enable-gpios: GPIO pin to enable or disable the panel
+- backlight: phandle of the backlight device attached to the panel
+
+This binding is compatible with the simple-panel binding, which is specified
+in simple-panel.txt in this directory.
diff --git a/drivers/gpu/drm/panel/panel-simple.c 
b/drivers/gpu/drm/panel/panel-simple.c
index cbf1ab4..8970261 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -1764,6 +1764,30 @@ static const struct panel_desc samsung_ltn140at29_301 = {
},
 };
 
+static const struct drm_display_mode sharp_lq035q7db03_mode = {
+   .clock = 5500,
+   .hdisplay = 240,
+   .hsync_start = 240 + 16,
+   .hsync_end = 240 + 16 + 7,
+   .htotal = 240 + 16 + 7 + 5,
+   .vdisplay = 320,
+   .vsync_start = 320 + 9,
+   .vsync_end = 320 + 9 + 1,
+   .vtotal = 320 + 9 + 1 + 7,
+   .vrefresh = 60,
+};
+
+static const struct panel_desc sharp_lq035q7db03 = {
+   .modes = _lq035q7db03_mode,
+   .num_modes = 1,
+   .bpc = 6,
+   .size = {
+   .width = 54,
+   .height = 72,
+   },
+   .bus_format = MEDIA_BUS_FMT_RGB666_1X18,
+};
+
 static const struct display_timing sharp_lq101k1ly04_timing = {
.pixelclock = { 6000, 6500, 8000 },
.hactive = { 1280, 1280, 1280 },
@@ -2236,6 +2260,9 @@ static const struct of_device_id platform_of_match[] = {
.compatible = "samsung,ltn140at29-301",
.data = _ltn140at29_301,
}, {
+   .compatible = "sharp,lq035q7db03",
+   .data = _lq035q7db03,
+   }, {
.compatible = "sharp,lq101k1ly04",
.data = _lq101k1ly04,
}, {
-- 
2.10.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/panel: simple: Add Sharp LQ035Q7DB03 panel support

[Vladimir Zapolskiy]

The change adds support for Sharp LQ035Q7DB03 3.5" QVGA panel.

Note that this aged panel is already found in the kernel sources,
for instance in board mach files mach-mx21ads.c, mach-mx27ads.c,
mach-pcm043.c, lpd270.c and imx27-phytec-phycore-rdk.dts.

Signed-off-by: Vladimir Zapolskiy <v...@mleia.com>
---
 .../bindings/display/panel/sharp,lq035q7db03.txt   |  7 ++
 drivers/gpu/drm/panel/panel-simple.c   | 27 ++
 2 files changed, 34 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/display/panel/sharp,lq035q7db03.txt

diff --git 
a/Documentation/devicetree/bindings/display/panel/sharp,lq035q7db03.txt 
b/Documentation/devicetree/bindings/display/panel/sharp,lq035q7db03.txt
new file mode 100644
index 000..42027e5
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/sharp,lq035q7db03.txt
@@ -0,0 +1,7 @@
+Sharp LQ035Q7DB03 3.5" QVGA TFT LCD panel
+
+Required properties:
+- compatible: should be "sharp,lq035q7db03"
+
+This binding is compatible with the simple-panel binding, which is specified
+in simple-panel.txt in this directory.
diff --git a/drivers/gpu/drm/panel/panel-simple.c 
b/drivers/gpu/drm/panel/panel-simple.c
index cbf1ab4..8970261 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -1764,6 +1764,30 @@ static const struct panel_desc samsung_ltn140at29_301 = {
},
 };
 
+static const struct drm_display_mode sharp_lq035q7db03_mode = {
+   .clock = 5500,
+   .hdisplay = 240,
+   .hsync_start = 240 + 16,
+   .hsync_end = 240 + 16 + 7,
+   .htotal = 240 + 16 + 7 + 5,
+   .vdisplay = 320,
+   .vsync_start = 320 + 9,
+   .vsync_end = 320 + 9 + 1,
+   .vtotal = 320 + 9 + 1 + 7,
+   .vrefresh = 60,
+};
+
+static const struct panel_desc sharp_lq035q7db03 = {
+   .modes = _lq035q7db03_mode,
+   .num_modes = 1,
+   .bpc = 6,
+   .size = {
+   .width = 54,
+   .height = 72,
+   },
+   .bus_format = MEDIA_BUS_FMT_RGB666_1X18,
+};
+
 static const struct display_timing sharp_lq101k1ly04_timing = {
.pixelclock = { 6000, 6500, 8000 },
.hactive = { 1280, 1280, 1280 },
@@ -2236,6 +2260,9 @@ static const struct of_device_id platform_of_match[] = {
.compatible = "samsung,ltn140at29-301",
.data = _ltn140at29_301,
}, {
+   .compatible = "sharp,lq035q7db03",
+   .data = _lq035q7db03,
+   }, {
.compatible = "sharp,lq101k1ly04",
.data = _lq101k1ly04,
}, {
-- 
2.10.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v9 1/2] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder

[Vladimir Zapolskiy]

Hi Jacopo,

On 04/18/2018 05:40 PM, Jacopo Mondi wrote:
> Document Thine THC63LVD1024 LVDS decoder device tree bindings.
> 
> Signed-off-by: Jacopo Mondi <jacopo+rene...@jmondi.org>
> Reviewed-by: Andrzej Hajda <a.ha...@samsung.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund+rene...@ragnatech.se>
> Reviewed-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> Reviewed-by: Rob Herring <r...@kernel.org>
> ---
>  .../bindings/display/bridge/thine,thc63lvd1024.txt | 60 
> ++
>  1 file changed, 60 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> 

Reviewed-by: Vladimir Zapolskiy <vladimir_zapols...@mentor.com>

--
With best wishes,
Vladimir
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v9 2/2] drm: bridge: Add thc63lvd1024 LVDS decoder driver

[Vladimir Zapolskiy]

Hi Jacopo,

On 04/18/2018 05:40 PM, Jacopo Mondi wrote:
> Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
> output converter.
> 
> Signed-off-by: Jacopo Mondi <jacopo+rene...@jmondi.org>
> Reviewed-by: Andrzej Hajda <a.ha...@samsung.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund+rene...@ragnatech.se>
> Reviewed-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> ---
>  drivers/gpu/drm/bridge/Kconfig|   6 +
>  drivers/gpu/drm/bridge/Makefile   |   1 +
>  drivers/gpu/drm/bridge/thc63lvd1024.c | 206 
> ++
>  3 files changed, 213 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c

Reviewed-by: Vladimir Zapolskiy <vladimir_zapols...@mentor.com>

--
With best wishes,
Vladimir
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v8 2/2] drm: bridge: Add thc63lvd1024 LVDS decoder driver

[Vladimir Zapolskiy]

Hi Jacopo,

On 04/10/2018 01:53 PM, Jacopo Mondi wrote:
> Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
> output converter.
> 
> Signed-off-by: Jacopo Mondi <jacopo+rene...@jmondi.org>
> Reviewed-by: Andrzej Hajda <a.ha...@samsung.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund+rene...@ragnatech.se>

Reviewed-by: Vladimir Zapolskiy <vladimir_zapols...@mentor.com>

Generally I have only one pretty ignorable comment.

> +
> +enum thc63_ports {
> + THC63_LVDS_IN0,
> + THC63_LVDS_IN1,
> + THC63_RGB_OUT0,
> + THC63_RGB_OUT1,
> +};
> +

The driver uses only THC63_RGB_OUT0 value, or port@2, and MODE{0,1,2} IC
configuration is ignored.

I don't know if right from the beginning it would be better to support
dual-out modes, preferably both single-in and dual-in ones. Will it
impact port enumeration?

I do understand that the extension is possible, and likely only hardware
accessibility postpones it.

--
With best wishes,
Vladimir
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v8 1/2] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder

[Vladimir Zapolskiy]

Hi Jacopo,

On 04/19/2018 12:48 PM, Vladimir Zapolskiy wrote:
> Hi Jacopo,
> 
> On 04/19/2018 12:44 PM, Vladimir Zapolskiy wrote:
>> Hi Jacopo, Laurent,
>>
>> On 04/10/2018 01:53 PM, Jacopo Mondi wrote:
>>> Document Thine THC63LVD1024 LVDS decoder device tree bindings.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo+rene...@jmondi.org>
>>> Reviewed-by: Andrzej Hajda <a.ha...@samsung.com>
>>> Reviewed-by: Niklas Söderlund <niklas.soderlund+rene...@ragnatech.se>
>>> Reviewed-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
>>
>> Reviewed-by: Vladimir Zapolskiy <vladimir_zapols...@mentor.com>
>>
>>> ---

[snip]

>>> +Optional properties:
>>> +- powerdown-gpios: Power down GPIO signal, pin name "/PDWN". Active low
>>
> 
> sorry for a follow-up, I've just noticed it, could you please double check
> spelling PDWN vs PWDN? Thank you in advance.
> 

please ignore it, I did it myself and the datasheet describes pin as /PDWN,
I won't exclude a typo in the datasheet though...

--
With best wishes,
Vladimir
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v8 1/2] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder

[Vladimir Zapolskiy]

Hi Jacopo,

On 04/19/2018 12:44 PM, Vladimir Zapolskiy wrote:
> Hi Jacopo, Laurent,
> 
> On 04/10/2018 01:53 PM, Jacopo Mondi wrote:
>> Document Thine THC63LVD1024 LVDS decoder device tree bindings.
>>
>> Signed-off-by: Jacopo Mondi <jacopo+rene...@jmondi.org>
>> Reviewed-by: Andrzej Hajda <a.ha...@samsung.com>
>> Reviewed-by: Niklas Söderlund <niklas.soderlund+rene...@ragnatech.se>
>> Reviewed-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> 
> Reviewed-by: Vladimir Zapolskiy <vladimir_zapols...@mentor.com>
> 
>> ---
>>  .../bindings/display/bridge/thine,thc63lvd1024.txt | 60 
>> ++
>>  1 file changed, 60 insertions(+)
>>  create mode 100644 
>> Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>
>> diff --git 
>> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt 
>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>> new file mode 100644
>> index 000..0b23e70
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>> @@ -0,0 +1,60 @@
>> +Thine Electronics THC63LVD1024 LVDS decoder
>> +---
>> +
>> +The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS 
>> streams
>> +to parallel data outputs. The chip supports single/dual input/output modes,
>> +handling up to two LVDS input streams and up to two digital CMOS/TTL 
>> outputs.
>> +
>> +Single or dual operation mode, output data mapping and DDR output modes are
>> +configured through input signals and the chip does not expose any control 
>> bus.
>> +
>> +Required properties:
>> +- compatible: Shall be "thine,thc63lvd1024"
>> +- vcc-supply: Power supply for TTL output, TTL CLOCKOUT signal, LVDS input,
>> +  PPL and digital circuitry
>> +
>> +Optional properties:
>> +- powerdown-gpios: Power down GPIO signal, pin name "/PDWN". Active low
> 

sorry for a follow-up, I've just noticed it, could you please double check
spelling PDWN vs PWDN? Thank you in advance.

--
With best wishes,
Vladimir
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v8 1/2] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder

[Vladimir Zapolskiy]

Hi Jacopo, Laurent,

On 04/10/2018 01:53 PM, Jacopo Mondi wrote:
> Document Thine THC63LVD1024 LVDS decoder device tree bindings.
> 
> Signed-off-by: Jacopo Mondi <jacopo+rene...@jmondi.org>
> Reviewed-by: Andrzej Hajda <a.ha...@samsung.com>
> Reviewed-by: Niklas Söderlund <niklas.soderlund+rene...@ragnatech.se>
> Reviewed-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>

Reviewed-by: Vladimir Zapolskiy <vladimir_zapols...@mentor.com>

> ---
>  .../bindings/display/bridge/thine,thc63lvd1024.txt | 60 
> ++
>  1 file changed, 60 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> 
> diff --git 
> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt 
> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> new file mode 100644
> index 000..0b23e70
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
> @@ -0,0 +1,60 @@
> +Thine Electronics THC63LVD1024 LVDS decoder
> +---
> +
> +The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS 
> streams
> +to parallel data outputs. The chip supports single/dual input/output modes,
> +handling up to two LVDS input streams and up to two digital CMOS/TTL outputs.
> +
> +Single or dual operation mode, output data mapping and DDR output modes are
> +configured through input signals and the chip does not expose any control 
> bus.
> +
> +Required properties:
> +- compatible: Shall be "thine,thc63lvd1024"
> +- vcc-supply: Power supply for TTL output, TTL CLOCKOUT signal, LVDS input,
> +  PPL and digital circuitry
> +
> +Optional properties:
> +- powerdown-gpios: Power down GPIO signal, pin name "/PDWN". Active low

Thank you for the change.

I would suggest to rename 'pwdn-gpios' property of THC63LVDM83D as well,
as far as I understand it is only described in DT bindings documentation,
and the property is unused in the driver or board DTS files at the moment.

> +- oe-gpios: Output enable GPIO signal, pin name "OE". Active high

Okay :)

--
With best wishes,
Vladimir
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v6 1/3] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder

[Vladimir Zapolskiy]

Hi Jacopo,

On 03/27/2018 01:10 PM, jacopo mondi wrote:
> Hi Vladimir,
> 
> On Tue, Mar 27, 2018 at 12:37:31PM +0300, Vladimir Zapolskiy wrote:
>> Hi Jacopo,
>>
>> On 03/27/2018 11:57 AM, jacopo mondi wrote:
>>> Hi Vladimir,
>>>
>>> On Tue, Mar 27, 2018 at 11:30:29AM +0300, Vladimir Zapolskiy wrote:
>>>> Hi Sergei,
>>>>
>>>> On 03/27/2018 11:27 AM, Sergei Shtylyov wrote:
>>>>> Hello!
>>>>>
>>>>> On 3/27/2018 10:33 AM, jacopo mondi wrote:
>>>>> [...]
>>>>>>>>>>> Document Thine THC63LVD1024 LVDS decoder device tree bindings.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Jacopo Mondi <jacopo+rene...@jmondi.org>
>>>>>>>>>>> Reviewed-by: Andrzej Hajda <a.ha...@samsung.com>
>>>>>>>>>>> Reviewed-by: Niklas Söderlund 
>>>>>>>>>>> <niklas.soderlund+rene...@ragnatech.se>
>>>>>>>>>>> ---
>>>>>>>>>>>   .../bindings/display/bridge/thine,thc63lvd1024.txt | 66 
>>>>>>>>>>> +++
>>>>>>>>>>>   1 file changed, 66 insertions(+)
>>>>>>>>>>>   create mode 100644
>>>>>>>>>>> Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>>>>>>>>>>
>>>>>>>>>>> diff --git
>>>>>>>>>>> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>>>>>>>>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>>>>>>>>>> new file mode 100644
>>>>>>>>>>> index 000..8225c6a
>>>>>>>>>>> --- /dev/null
>>>>>>>>>>> +++
>>>>>>>>>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>>>>>>>>>> @@ -0,0 +1,66 @@
>>>>>>>>>>> +Thine Electronics THC63LVD1024 LVDS decoder
>>>>>>>>>>> +---
>>>>>>>>>>> +
>>>>>>>>>>> +The THC63LVD1024 is a dual link LVDS receiver designed to convert 
>>>>>>>>>>> LVDS
>>>>>>>>>>> streams
>>>>>>>>>>> +to parallel data outputs. The chip supports single/dual 
>>>>>>>>>>> input/output modes,
>>>>>>>>>>> +handling up to two two input LVDS stream and up to two digital 
>>>>>>>>>>> CMOS/TTL
>>>>>>>>>>> outputs.
>>>>>>>>>>> +
>>>>>>>>>>> +Single or dual operation modes, output data mapping and DDR output 
>>>>>>>>>>> modes
>>>>>>>>>>> are
>>>>>>>>>>> +configured through input signals and the chip does not expose any 
>>>>>>>>>>> control
>>>>>>>>>>> bus.
>>>>>>>>>>> +
>>>>>>>>>>> +Required properties:
>>>>>>>>>>> +- compatible: Shall be "thine,thc63lvd1024"
>>>>>>>>>>> +
>>>>>>>>>>> +Optional properties:
>>>>>>>>>>> +- vcc-supply: Power supply for TTL output and digital circuitry
>>>>>>>>>>> +- cvcc-supply: Power supply for TTL CLOCKOUT signal
>>>>>>>>>>> +- lvcc-supply: Power supply for LVDS inputs
>>>>>>>>>>> +- pvcc-supply: Power supply for PLL circuitry
>>>>>>>>>> As explained in a comment to one of the previous versions of this 
>>>>>>>>>> series, I'm
>>>>>>>>>> tempted to make vcc-supply mandatory and drop the three other power 
>>>>>>>>>> supplies
>>>>>>>>>> for now, as I believe there's very little chance they will be 
>>>>>>>>>> connected to
>>>>>>>>>> separately controllable regulators (all supplies use the same 
>>>>

Re: [PATCH v6 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver

[Vladimir Zapolskiy]

Hi Andrzej,

On 03/27/2018 10:28 AM, Andrzej Hajda wrote:
> On 27.03.2018 08:24, Vladimir Zapolskiy wrote:
>> Hi Jacopo,
>>
>> On 03/16/2018 05:16 PM, Jacopo Mondi wrote:
>>> Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
>>> output converter.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo+rene...@jmondi.org>
>>> Reviewed-by: Andrzej Hajda <a.ha...@samsung.com>
>>> Reviewed-by: Niklas Söderlund <niklas.soderlund+rene...@ragnatech.se>
>>> ---
>>>  drivers/gpu/drm/bridge/Kconfig|   6 +
>>>  drivers/gpu/drm/bridge/Makefile   |   1 +
>>>  drivers/gpu/drm/bridge/thc63lvd1024.c | 255 
>>> ++
>>>  3 files changed, 262 insertions(+)
>>>  create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c
>>>
>>> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
>>> index 3b99d5a..5815a20 100644
>>> --- a/drivers/gpu/drm/bridge/Kconfig
>>> +++ b/drivers/gpu/drm/bridge/Kconfig
>>> @@ -92,6 +92,12 @@ config DRM_SII9234
>>>   It is an I2C driver, that detects connection of MHL bridge
>>>   and starts encapsulation of HDMI signal.
>>>  
>>> +config DRM_THINE_THC63LVD1024
>>> +   tristate "Thine THC63LVD1024 LVDS decoder bridge"
>>> +   depends on OF
>>> +   ---help---
>>> + Thine THC63LVD1024 LVDS/parallel converter driver.
>>> +
>>>  config DRM_TOSHIBA_TC358767
>>> tristate "Toshiba TC358767 eDP bridge"
>>> depends on OF
>>> diff --git a/drivers/gpu/drm/bridge/Makefile 
>>> b/drivers/gpu/drm/bridge/Makefile
>>> index 373eb28..fd90b16 100644
>>> --- a/drivers/gpu/drm/bridge/Makefile
>>> +++ b/drivers/gpu/drm/bridge/Makefile
>>> @@ -8,6 +8,7 @@ obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
>>>  obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o
>>>  obj-$(CONFIG_DRM_SII902X) += sii902x.o
>>>  obj-$(CONFIG_DRM_SII9234) += sii9234.o
>>> +obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o
>>>  obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
>>>  obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
>>>  obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
>>> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c 
>>> b/drivers/gpu/drm/bridge/thc63lvd1024.c
>>> new file mode 100644
>>> index 000..02a54c2
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
>>> @@ -0,0 +1,255 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * THC63LVD1024 LVDS to parallel data DRM bridge driver.
>>> + *
>>> + * Copyright (C) 2018 Jacopo Mondi <jacopo+rene...@jmondi.org>
>>> + */
>>> +
>>> +#include 
>>> +#include 
>>> +#include 
>>> +
>>> +#include 
>>> +#include 
>>> +#include 
>>> +
>>> +enum {
>>> +   THC63_LVDS_IN0,
>>> +   THC63_LVDS_IN1,
>>> +   THC63_DIGITAL_OUT0,
>>> +   THC63_DIGITAL_OUT1,
>>> +};
>>> +
>>> +static const char * const thc63_reg_names[] = {
>>> +   "vcc", "lvcc", "pvcc", "cvcc",
>>> +};
>>> +
>>> +struct thc63_dev {
>>> +   struct device *dev;
>>> +
>>> +   struct regulator *vccs[ARRAY_SIZE(thc63_reg_names)];
>>> +
>>> +   struct gpio_desc *pdwn;
>>> +   struct gpio_desc *oe;
>>> +
>>> +   struct drm_bridge bridge;
>>> +   struct drm_bridge *next;
>>> +};
>>> +
>>> +static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge)
>>> +{
>>> +   return container_of(bridge, struct thc63_dev, bridge);
>>> +}
>>> +
>>> +static int thc63_attach(struct drm_bridge *bridge)
>>> +{
>>> +   struct thc63_dev *thc63 = to_thc63(bridge);
>>> +
>>> +   return drm_bridge_attach(bridge->encoder, thc63->next, bridge);
>>> +}
>>> +
>>> +static void thc63_enable(struct drm_bridge *bridge)
>>> +{
>>> +   struct thc63_dev *thc63 = to_thc63(bridge);
>>> +   struct regulator *vcc;
>>> +   int i;
>> unsigned int i;
> 
> Why? You are introducing silly bug this way, see few lines below.

You see that the comment was too terse to describe the context in full.
Thank you for exposing a flaw.

>>
>>> +
>>> +   for (i = 0; i < ARRAY_SIZE(thc63->vccs);

Re: [PATCH v6 1/3] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder

[Vladimir Zapolskiy]

Hi Jacopo,

On 03/27/2018 11:57 AM, jacopo mondi wrote:
> Hi Vladimir,
> 
> On Tue, Mar 27, 2018 at 11:30:29AM +0300, Vladimir Zapolskiy wrote:
>> Hi Sergei,
>>
>> On 03/27/2018 11:27 AM, Sergei Shtylyov wrote:
>>> Hello!
>>>
>>> On 3/27/2018 10:33 AM, jacopo mondi wrote:
>>> [...]
>>>>>>>>> Document Thine THC63LVD1024 LVDS decoder device tree bindings.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Jacopo Mondi <jacopo+rene...@jmondi.org>
>>>>>>>>> Reviewed-by: Andrzej Hajda <a.ha...@samsung.com>
>>>>>>>>> Reviewed-by: Niklas Söderlund <niklas.soderlund+rene...@ragnatech.se>
>>>>>>>>> ---
>>>>>>>>>   .../bindings/display/bridge/thine,thc63lvd1024.txt | 66 
>>>>>>>>> +++
>>>>>>>>>   1 file changed, 66 insertions(+)
>>>>>>>>>   create mode 100644
>>>>>>>>> Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>>>>>>>>
>>>>>>>>> diff --git
>>>>>>>>> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>>>>>>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>>>>>>>> new file mode 100644
>>>>>>>>> index 000..8225c6a
>>>>>>>>> --- /dev/null
>>>>>>>>> +++
>>>>>>>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>>>>>>>> @@ -0,0 +1,66 @@
>>>>>>>>> +Thine Electronics THC63LVD1024 LVDS decoder
>>>>>>>>> +---
>>>>>>>>> +
>>>>>>>>> +The THC63LVD1024 is a dual link LVDS receiver designed to convert 
>>>>>>>>> LVDS
>>>>>>>>> streams
>>>>>>>>> +to parallel data outputs. The chip supports single/dual input/output 
>>>>>>>>> modes,
>>>>>>>>> +handling up to two two input LVDS stream and up to two digital 
>>>>>>>>> CMOS/TTL
>>>>>>>>> outputs.
>>>>>>>>> +
>>>>>>>>> +Single or dual operation modes, output data mapping and DDR output 
>>>>>>>>> modes
>>>>>>>>> are
>>>>>>>>> +configured through input signals and the chip does not expose any 
>>>>>>>>> control
>>>>>>>>> bus.
>>>>>>>>> +
>>>>>>>>> +Required properties:
>>>>>>>>> +- compatible: Shall be "thine,thc63lvd1024"
>>>>>>>>> +
>>>>>>>>> +Optional properties:
>>>>>>>>> +- vcc-supply: Power supply for TTL output and digital circuitry
>>>>>>>>> +- cvcc-supply: Power supply for TTL CLOCKOUT signal
>>>>>>>>> +- lvcc-supply: Power supply for LVDS inputs
>>>>>>>>> +- pvcc-supply: Power supply for PLL circuitry
>>>>>>>> As explained in a comment to one of the previous versions of this 
>>>>>>>> series, I'm
>>>>>>>> tempted to make vcc-supply mandatory and drop the three other power 
>>>>>>>> supplies
>>>>>>>> for now, as I believe there's very little chance they will be 
>>>>>>>> connected to
>>>>>>>> separately controllable regulators (all supplies use the same 
>>>>>>>> voltage). In the
>>>>>>>> very unlikely event that this occurs in design we need to support in 
>>>>>>>> the
>>>>>>>> future, the cvcc, lvcc and pvcc supplies can be added later as optional
>>>>>>>> without breaking backward compatibility.
>>>>>>> I'm okay with that.
>>>>>>>
>>>>>>>> Apart from that,
>>>>>>>>
>>>>>>>> Reviewed-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
>>>>>>>>
>>>>>>>>> +- pdwn-gpios: Power down GPIO signal. Active low
>

Re: [PATCH v6 1/3] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder

[Vladimir Zapolskiy]

Hi Sergei,

On 03/27/2018 11:27 AM, Sergei Shtylyov wrote:
> Hello!
> 
> On 3/27/2018 10:33 AM, jacopo mondi wrote:
> [...]
>>> Document Thine THC63LVD1024 LVDS decoder device tree bindings.
>>>
>>> Signed-off-by: Jacopo Mondi 
>>> Reviewed-by: Andrzej Hajda 
>>> Reviewed-by: Niklas Söderlund 
>>> ---
>>>   .../bindings/display/bridge/thine,thc63lvd1024.txt | 66 
>>> +++
>>>   1 file changed, 66 insertions(+)
>>>   create mode 100644
>>> Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>> new file mode 100644
>>> index 000..8225c6a
>>> --- /dev/null
>>> +++
>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>> @@ -0,0 +1,66 @@
>>> +Thine Electronics THC63LVD1024 LVDS decoder
>>> +---
>>> +
>>> +The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS
>>> streams
>>> +to parallel data outputs. The chip supports single/dual input/output 
>>> modes,
>>> +handling up to two two input LVDS stream and up to two digital CMOS/TTL
>>> outputs.
>>> +
>>> +Single or dual operation modes, output data mapping and DDR output 
>>> modes
>>> are
>>> +configured through input signals and the chip does not expose any 
>>> control
>>> bus.
>>> +
>>> +Required properties:
>>> +- compatible: Shall be "thine,thc63lvd1024"
>>> +
>>> +Optional properties:
>>> +- vcc-supply: Power supply for TTL output and digital circuitry
>>> +- cvcc-supply: Power supply for TTL CLOCKOUT signal
>>> +- lvcc-supply: Power supply for LVDS inputs
>>> +- pvcc-supply: Power supply for PLL circuitry
>> As explained in a comment to one of the previous versions of this 
>> series, I'm
>> tempted to make vcc-supply mandatory and drop the three other power 
>> supplies
>> for now, as I believe there's very little chance they will be connected 
>> to
>> separately controllable regulators (all supplies use the same voltage). 
>> In the
>> very unlikely event that this occurs in design we need to support in the
>> future, the cvcc, lvcc and pvcc supplies can be added later as optional
>> without breaking backward compatibility.
> I'm okay with that.
>
>> Apart from that,
>>
>> Reviewed-by: Laurent Pinchart 
>>
>>> +- pdwn-gpios: Power down GPIO signal. Active low
> powerdown-gpios is the semi-standard name.
>
 right, I've also noticed it. If possible please avoid shortenings in
 property names.
>>>
>>> It is not shortening, it just follow pin name from decoder's datasheet.
>>>

>>> +- oe-gpios: Output enable GPIO signal. Active high
>>> +
 And this one is also a not ever met property name, please consider to
 rename it to 'enable-gpios', for instance display panels define it.
>>>
>>>
>>> Again, it follows datasheet naming scheme. Has something changed in DT
>>> conventions?
>>
>> Seconded. My understanding is that the property name should reflect
>> what reported in the the chip manual. For THC63LVD1024 the enable and
>> power down pins are named 'OE' and 'PDWN' respectively.
> 
> But don't we need the vendor prefix in the prop names then, like 
> "renesas,oe-gpios" then?
> 

Seconded, with a correction to "thine,oe-gpios".

If vendor agnostic properties are supposed to be used, then please follow
the referenced by Rob semi-standard notations.

--
With best wishes,
Vladimir
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v6 3/3] arm64: dts: renesas: Add LVDS decoder to R-Car V3M Eagle

[Vladimir Zapolskiy]

Hi Jacopo,

On 03/20/2018 03:01 PM, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> Thank you for the patch.
> 
> On Friday, 16 March 2018 17:16:39 EET Jacopo Mondi wrote:
>> The R-Car V3M Eagle board includes a transparent THC63LVD1024 LVDS
>> decoder, connected to the on-chip LVDS encoder output on one side
>> and to HDMI encoder ADV7511w on the other one.
>>
>> As the decoder does not need any configuration it has been so-far
>> omitted from DTS. Now that a driver is available, describe it in DT
>> as well.
>>
>> Signed-off-by: Jacopo Mondi 
>> Reviewed-by: Andrzej Hajda 
> 
> The patch looks OK to me, but I think it should be squashed with Niklas' 
> patch 
> that added display HDMI output support to the V3M Eagle DT.
> 
>> ---
>>
>> List of patch dependencies, as of renesas-drivers-2018-03-13-v4.16-rc5:
>>
>> - [PATCH v2 0/5] arm64: dts: renesas: r8a77970: enable HDMI output
>>which includes DU, LVDS and FCPD enablement from:
>>   [PATCH v2 0/5] Add R8A77970/V3MSK LVDS/HDMI support
>> - [PATCH v4] v4l: vsp1: Fix video output on R8A77970
>>
>> Patches to be applied on top of
>> "arm64: dts: renesas: eagle: add HDMI output using the ADV7511W"
>>
>> Thanks
>>j
>> ---
>>  arch/arm64/boot/dts/renesas/r8a77970-eagle.dts | 33 ---
>>  1 file changed, 30 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
>> b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts index c0fd144..69f43b8
>> 100644
>> --- a/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
>> +++ b/arch/arm64/boot/dts/renesas/r8a77970-eagle.dts
>> @@ -42,6 +42,33 @@
>>  };
>>  };
>>  };
>> +
>> +thc63lvd1024: lvds-decoder {
>> +compatible = "thine,thc63lvd1024";
>> +
>> +ports {
>> +#address-cells = <1>;
>> +#size-cells = <0>;
>> +
>> +port@0 {
>> +reg = <0>;
>> +
>> +thc63lvd1024_in_0: endpoint {
>> +remote-endpoint = <_out>;
>> +};
>> +};
>> +
>> +port@2{
>> +reg = <2>;
>> +
>> +thc63lvd1024_out_2: endpoint {
>> +remote-endpoint = <_in>;
>> +};
>> +

Remove the empty line above.

>> +};
>> +

Remove the empty line above.

>> +};
>> +};
>>  };
>>

--
With best wishes,
Vladimir
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v6 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver

[Vladimir Zapolskiy]

Hi Jacopo,

On 03/16/2018 05:16 PM, Jacopo Mondi wrote:
> Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
> output converter.
> 
> Signed-off-by: Jacopo Mondi 
> Reviewed-by: Andrzej Hajda 
> Reviewed-by: Niklas Söderlund 
> ---
>  drivers/gpu/drm/bridge/Kconfig|   6 +
>  drivers/gpu/drm/bridge/Makefile   |   1 +
>  drivers/gpu/drm/bridge/thc63lvd1024.c | 255 
> ++
>  3 files changed, 262 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c
> 
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 3b99d5a..5815a20 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -92,6 +92,12 @@ config DRM_SII9234
> It is an I2C driver, that detects connection of MHL bridge
> and starts encapsulation of HDMI signal.
>  
> +config DRM_THINE_THC63LVD1024
> + tristate "Thine THC63LVD1024 LVDS decoder bridge"
> + depends on OF
> + ---help---
> +   Thine THC63LVD1024 LVDS/parallel converter driver.
> +
>  config DRM_TOSHIBA_TC358767
>   tristate "Toshiba TC358767 eDP bridge"
>   depends on OF
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index 373eb28..fd90b16 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
>  obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o
>  obj-$(CONFIG_DRM_SII902X) += sii902x.o
>  obj-$(CONFIG_DRM_SII9234) += sii9234.o
> +obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o
>  obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
>  obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
>  obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c 
> b/drivers/gpu/drm/bridge/thc63lvd1024.c
> new file mode 100644
> index 000..02a54c2
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
> @@ -0,0 +1,255 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * THC63LVD1024 LVDS to parallel data DRM bridge driver.
> + *
> + * Copyright (C) 2018 Jacopo Mondi 
> + */
> +
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +
> +enum {
> + THC63_LVDS_IN0,
> + THC63_LVDS_IN1,
> + THC63_DIGITAL_OUT0,
> + THC63_DIGITAL_OUT1,
> +};
> +
> +static const char * const thc63_reg_names[] = {
> + "vcc", "lvcc", "pvcc", "cvcc",
> +};
> +
> +struct thc63_dev {
> + struct device *dev;
> +
> + struct regulator *vccs[ARRAY_SIZE(thc63_reg_names)];
> +
> + struct gpio_desc *pdwn;
> + struct gpio_desc *oe;
> +
> + struct drm_bridge bridge;
> + struct drm_bridge *next;
> +};
> +
> +static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge)
> +{
> + return container_of(bridge, struct thc63_dev, bridge);
> +}
> +
> +static int thc63_attach(struct drm_bridge *bridge)
> +{
> + struct thc63_dev *thc63 = to_thc63(bridge);
> +
> + return drm_bridge_attach(bridge->encoder, thc63->next, bridge);
> +}
> +
> +static void thc63_enable(struct drm_bridge *bridge)
> +{
> + struct thc63_dev *thc63 = to_thc63(bridge);
> + struct regulator *vcc;
> + int i;

unsigned int i;

> +
> + for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
> + vcc = thc63->vccs[i];
> + if (!vcc)
> + continue;
> +
> + if (regulator_enable(vcc))
> + goto error_vcc_enable;
> + }
> +
> + if (thc63->pdwn)
> + gpiod_set_value(thc63->pdwn, 0);
> +
> + if (thc63->oe)
> + gpiod_set_value(thc63->oe, 1);
> +
> + return;
> +
> +error_vcc_enable:
> + dev_err(thc63->dev, "Failed to enable regulator %s\n",
> + thc63_reg_names[i]);
> +
> + for (i = i - 1; i >= 0; i--) {
> + vcc = thc63->vccs[i];
> + if (!vcc)
> + continue;
> +
> + regulator_disable(vcc);
> + }
> +}
> +
> +static void thc63_disable(struct drm_bridge *bridge)
> +{
> + struct thc63_dev *thc63 = to_thc63(bridge);
> + struct regulator *vcc;
> + int i;
> +
> + if (thc63->oe)
> + gpiod_set_value(thc63->oe, 0);
> +
> + if (thc63->pdwn)
> + gpiod_set_value(thc63->pdwn, 1);
> +
> + for (i = ARRAY_SIZE(thc63->vccs) - 1; i >= 0; i--) {
> + vcc = thc63->vccs[i];
> + if (!vcc)
> + continue;
> +
> + if (regulator_disable(vcc))
> + dev_dbg(thc63->dev,
> + "Failed to disable regulator %s\n",
> + thc63_reg_names[i]);
> + }
> +}
> +
> +struct drm_bridge_funcs thc63_bridge_func = {

Apparently you missed all my comments from v2 review.

If you like to avoid non-NULL function pointers 

Re: [PATCH v6 1/3] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder

[Vladimir Zapolskiy]

Hi Jacopo,

On 03/27/2018 01:22 AM, Rob Herring wrote:
> On Tue, Mar 20, 2018 at 02:43:33PM +0200, Laurent Pinchart wrote:
>> Hi Jacopo,
>>
>> (CC'ing Rob)
>>
>> Thank you for the patch.
>>
>> On Friday, 16 March 2018 17:16:37 EET Jacopo Mondi wrote:
>>> Document Thine THC63LVD1024 LVDS decoder device tree bindings.
>>>
>>> Signed-off-by: Jacopo Mondi 
>>> Reviewed-by: Andrzej Hajda 
>>> Reviewed-by: Niklas Söderlund 
>>> ---
>>>  .../bindings/display/bridge/thine,thc63lvd1024.txt | 66 +++
>>>  1 file changed, 66 insertions(+)
>>>  create mode 100644
>>> Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>> new file mode 100644
>>> index 000..8225c6a
>>> --- /dev/null
>>> +++
>>> b/Documentation/devicetree/bindings/display/bridge/thine,thc63lvd1024.txt
>>> @@ -0,0 +1,66 @@
>>> +Thine Electronics THC63LVD1024 LVDS decoder
>>> +---
>>> +
>>> +The THC63LVD1024 is a dual link LVDS receiver designed to convert LVDS
>>> streams
>>> +to parallel data outputs. The chip supports single/dual input/output modes,
>>> +handling up to two two input LVDS stream and up to two digital CMOS/TTL
>>> outputs.
>>> +
>>> +Single or dual operation modes, output data mapping and DDR output modes
>>> are
>>> +configured through input signals and the chip does not expose any control
>>> bus.
>>> +
>>> +Required properties:
>>> +- compatible: Shall be "thine,thc63lvd1024"
>>> +
>>> +Optional properties:
>>> +- vcc-supply: Power supply for TTL output and digital circuitry
>>> +- cvcc-supply: Power supply for TTL CLOCKOUT signal
>>> +- lvcc-supply: Power supply for LVDS inputs
>>> +- pvcc-supply: Power supply for PLL circuitry
>>
>> As explained in a comment to one of the previous versions of this series, 
>> I'm 
>> tempted to make vcc-supply mandatory and drop the three other power supplies 
>> for now, as I believe there's very little chance they will be connected to 
>> separately controllable regulators (all supplies use the same voltage). In 
>> the 
>> very unlikely event that this occurs in design we need to support in the 
>> future, the cvcc, lvcc and pvcc supplies can be added later as optional 
>> without breaking backward compatibility.
> 
> I'm okay with that.
> 
>> Apart from that,
>>
>> Reviewed-by: Laurent Pinchart 
>>
>>> +- pdwn-gpios: Power down GPIO signal. Active low
> 
> powerdown-gpios is the semi-standard name.
> 

right, I've also noticed it. If possible please avoid shortenings in
property names.

>>> +- oe-gpios: Output enable GPIO signal. Active high
>>> +

And this one is also a not ever met property name, please consider to
rename it to 'enable-gpios', for instance display panels define it.

>>> +The THC63LVD1024 video port connections are modeled according
>>> +to OF graph bindings specified by
>>> Documentation/devicetree/bindings/graph.txt

[snip]

>>> +
>>> +   port@2{
>>> +   reg = <2>;
>>> +
>>> +   lvds_dec_out_2: endpoint {
>>> +   remote-endpoint = <_in>;
>>> +   };
>>> +

Drop a surplus empty line above.

>>> +   };
>>> +

Drop a surplus empty line above.

>>> +   };
>>> +   };

--
With best wishes,
Vladimir
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/3] drm: bridge: Add LVDS decoder driver

[Vladimir Zapolskiy]

Hi Jacopo,

On 03/09/2018 03:51 PM, Jacopo Mondi wrote:
> Add transparent LVDS decoder driver.
> 
> A transparent LVDS decoder is a DRM bridge device that does not require
> any configuration and converts LVDS input to digital CMOS/TTL parallel
> data output.
> 
> Signed-off-by: Jacopo Mondi 
> ---
>  drivers/gpu/drm/bridge/Kconfig|   8 +++
>  drivers/gpu/drm/bridge/Makefile   |   1 +
>  drivers/gpu/drm/bridge/lvds-decoder.c | 129 
> ++
>  3 files changed, 138 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/lvds-decoder.c
> 
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 3b99d5a..e52a5af 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -32,6 +32,14 @@ config DRM_DUMB_VGA_DAC
>   help
> Support for RGB to VGA DAC based bridges
> 
> +config DRM_LVDS_DECODER
> + tristate "Transparent LVDS to parallel decoder support"
> + depends on OF
> + select DRM_PANEL_BRIDGE
> + help
> +   Support for transparent LVDS to parallel decoders that don't require
> +   any configuration.
> +
>  config DRM_LVDS_ENCODER
>   tristate "Transparent parallel to LVDS encoder support"
>   depends on OF
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index 373eb28..edc2332 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
>  obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o
> +obj-$(CONFIG_DRM_LVDS_DECODER) += lvds-decoder.o
>  obj-$(CONFIG_DRM_LVDS_ENCODER) += lvds-encoder.o
>  obj-$(CONFIG_DRM_MEGACHIPS_STDP_GE_B850V3_FW) += 
> megachips-stdp-ge-b850v3-fw.o
>  obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
> diff --git a/drivers/gpu/drm/bridge/lvds-decoder.c 
> b/drivers/gpu/drm/bridge/lvds-decoder.c
> new file mode 100644
> index 000..319f4d5
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/lvds-decoder.c
> @@ -0,0 +1,129 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * LVDS to parallel data DRM bridge driver.
> + *
> + * Copyright (C) 2018 Jacopo Mondi 
> + */
> +
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +struct lvds_decoder {
> + struct device *dev;
> +
> + struct drm_bridge bridge;
> + struct drm_bridge *next;
> + struct drm_encoder *bridge_encoder;
> +};
> +
> +static inline struct lvds_decoder *to_lvds_decoder(struct drm_bridge *bridge)
> +{
> + return container_of(bridge, struct lvds_decoder, bridge);
> +}
> +
> +static int lvds_decoder_attach(struct drm_bridge *bridge)
> +{
> + struct lvds_decoder *lvds = to_lvds_decoder(bridge);
> +
> + return drm_bridge_attach(bridge->encoder, lvds->next, bridge);
> +}
> +
> +struct drm_bridge_funcs lvds_dec_bridge_func = {

static const struct drm_bridge_funcs lvds_dec_bridge_func

> + .attach = lvds_decoder_attach,
> +};
> +
> +static int lvds_decoder_parse_dt(struct lvds_decoder *lvds)
> +{
> + struct device_node *lvds_output;
> + struct device_node *remote;
> + int ret = 0;
> +
> + lvds_output = of_graph_get_endpoint_by_regs(lvds->dev->of_node, 1, -1);
> + if (!lvds_output) {
> + dev_err(lvds->dev, "Missing endpoint in Port@1\n");
> + return -ENODEV;
> + }
> +
> + remote = of_graph_get_remote_port_parent(lvds_output);

Add of_node_put(lvds_output) right here to get rid of error_put_lvds_node
goto label.

> + if (!remote) {
> + dev_err(lvds->dev, "Endpoint in Port@1 unconnected\n");
> + ret = -ENODEV;
> + goto error_put_lvds_node;
> + }
> +
> + if (!of_device_is_available(remote)) {
> + dev_err(lvds->dev, "Port@1 remote endpoint is disabled\n");
> + ret = -ENODEV;
> + goto error_put_remote_node;
> + }
> +
> + lvds->next = of_drm_find_bridge(remote);
> + if (!lvds->next)
> + ret = -EPROBE_DEFER;
> +
> +error_put_remote_node:
> + of_node_put(remote);
> +error_put_lvds_node:
> + of_node_put(lvds_output);
> +
> + return ret;
> +}
> +
> +static int lvds_decoder_probe(struct platform_device *pdev)
> +{
> + struct lvds_decoder *lvds;
> + int ret;
> +
> + lvds = devm_kzalloc(>dev, sizeof(*lvds), GFP_KERNEL);
> + if (!lvds)
> + return -ENOMEM;
> +
> + lvds->dev = >dev;
> + platform_set_drvdata(pdev, lvds);
> +
> + ret = lvds_decoder_parse_dt(lvds);
> + if (ret)
> + return ret;
> +
> + lvds->bridge.driver_private = lvds;
> + lvds->bridge.of_node = pdev->dev.of_node;
> + lvds->bridge.funcs = _dec_bridge_func;
> +
> + drm_bridge_add(>bridge);
> +
> + return 0;
> +}
> +
> +static int lvds_decoder_remove(struct platform_device *pdev)
> +{
> + struct lvds_decoder *lvds = 

[PATCH] video: of: display_timing: Remove of_display_timings_exist() function

[Vladimir Zapolskiy]

Since introduction of of_display_timings_exist() function in commit
cc3f414cf2e40 ("video: add of helper for display timings/videomode") it
didn't attract any users, and the function has no potential, because
of_get_display_timings() covers its functionality and does more.

Drop the unused exported function from the kernel.

Signed-off-by: Vladimir Zapolskiy <v...@mleia.com>
---
 drivers/video/of_display_timing.c | 20 
 include/video/of_display_timing.h |  5 -
 2 files changed, 25 deletions(-)

diff --git a/drivers/video/of_display_timing.c 
b/drivers/video/of_display_timing.c
index 8ce0a99..83b8963 100644
--- a/drivers/video/of_display_timing.c
+++ b/drivers/video/of_display_timing.c
@@ -244,23 +244,3 @@ struct display_timings *of_get_display_timings(const 
struct device_node *np)
return NULL;
 }
 EXPORT_SYMBOL_GPL(of_get_display_timings);
-
-/**
- * of_display_timings_exist - check if a display-timings node is provided
- * @np: device_node with the timing
- **/
-int of_display_timings_exist(const struct device_node *np)
-{
-   struct device_node *timings_np;
-
-   if (!np)
-   return -EINVAL;
-
-   timings_np = of_parse_phandle(np, "display-timings", 0);
-   if (!timings_np)
-   return -EINVAL;
-
-   of_node_put(timings_np);
-   return 1;
-}
-EXPORT_SYMBOL_GPL(of_display_timings_exist);
diff --git a/include/video/of_display_timing.h 
b/include/video/of_display_timing.h
index 956455f..bb29e59 100644
--- a/include/video/of_display_timing.h
+++ b/include/video/of_display_timing.h
@@ -19,7 +19,6 @@ struct display_timings;
 int of_get_display_timing(const struct device_node *np, const char *name,
struct display_timing *dt);
 struct display_timings *of_get_display_timings(const struct device_node *np);
-int of_display_timings_exist(const struct device_node *np);
 #else
 static inline int of_get_display_timing(const struct device_node *np,
const char *name, struct display_timing *dt)
@@ -31,10 +30,6 @@ of_get_display_timings(const struct device_node *np)
 {
return NULL;
 }
-static inline int of_display_timings_exist(const struct device_node *np)
-{
-   return -ENOSYS;
-}
 #endif
 
 #endif
-- 
2.10.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 02/10] dt-bindings: display: renesas: Deprecate LVDS support in the DU bindings

[Vladimir Zapolskiy]

Hi Laurent,

On 01/12/2018 02:58 AM, Laurent Pinchart wrote:
> The internal LVDS encoders now have their own DT bindings, representing
> them as part of the DU is deprecated.
> 
> Signed-off-by: Laurent Pinchart 
> ---
>  .../devicetree/bindings/display/renesas,du.txt | 26 
> +-
>  1 file changed, 11 insertions(+), 15 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/renesas,du.txt 
> b/Documentation/devicetree/bindings/display/renesas,du.txt
> index cd48aba3bc8c..8f6e0e118e3e 100644
> --- a/Documentation/devicetree/bindings/display/renesas,du.txt
> +++ b/Documentation/devicetree/bindings/display/renesas,du.txt
> @@ -17,9 +17,7 @@ Required Properties:
>- reg: A list of base address and length of each memory resource, one for
>  each entry in the reg-names property.
>- reg-names: Name of the memory resources. The DU requires one memory
> -resource for the DU core (named "du") and one memory resource for each
> -LVDS encoder (named "lvds.x" with "x" being the LVDS controller numerical
> -index).
> +resource for the DU core (named "du").
>  
>- interrupt-parent: phandle of the parent interrupt controller.
>- interrupts: Interrupt specifiers for the DU interrupts.
> @@ -29,14 +27,13 @@ Required Properties:
>- clock-names: Name of the clocks. This property is model-dependent.
>  - R8A7779 uses a single functional clock. The clock doesn't need to be
>named.
> -- All other DU instances use one functional clock per channel and one
> -  clock per LVDS encoder (if available). The functional clocks must be
> -  named "du.x" with "x" being the channel numerical index. The LVDS 
> clocks
> -  must be named "lvds.x" with "x" being the LVDS encoder numerical index.
> -- In addition to the functional and encoder clocks, all DU versions also
> -  support externally supplied pixel clocks. Those clocks are optional.
> -  When supplied they must be named "dclkin.x" with "x" being the input
> -  clock numerical index.
> +- All other DU instances use one functional clock per channel The
> +  functional clocks must be named "du.x" with "x" being the channel
> +  numerical index.
> +- In addition to the functional clocks, all DU versions also support
> +  externally supplied pixel clocks. Those clocks are optional. When
> +  supplied they must be named "dclkin.x" with "x" being the input clock
> +  numerical index.
>  
>- vsps: A list of phandle and channel index tuples to the VSPs that handle
>  the memory interfaces for the DU channels. The phandle identifies the VSP
> @@ -71,7 +68,7 @@ Example: R8A7795 (R-Car H3) ES2.0 DU
>   compatible = "renesas,du-r8a7795";
>   reg = <0 0xfeb0 0 0x8>,
> <0 0xfeb9 0 0x14>;
> - reg-names = "du", "lvds.0";
> + reg-names = "du";

Since "reg-names" list is changed, I believe you'd like to update
the "reg" property value as well.

>   interrupts = ,
>,
>,
> @@ -79,9 +76,8 @@ Example: R8A7795 (R-Car H3) ES2.0 DU
>   clocks = < CPG_MOD 724>,
>< CPG_MOD 723>,
>< CPG_MOD 722>,
> -  < CPG_MOD 721>,
> -  < CPG_MOD 727>;
> - clock-names = "du.0", "du.1", "du.2", "du.3", "lvds.0";
> +  < CPG_MOD 721>;
> + clock-names = "du.0", "du.1", "du.2", "du.3";
>   vsps = < 0>, < 0>, < 0>, < 1>;
>  
>   ports {
> 

--
With best wishes,
Vladimir
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/bridge: dw_hdmi: support i2c extended read mode

[Vladimir Zapolskiy]

Hi Nickey,

On 03/20/2017 04:57 AM, Nickey Yang wrote:
> "I2C Master Interface Extended Read Mode" implements a segment
> pointer-based read operation using the Special Register configuration.
> 
> This patch fix https://patchwork.kernel.org/patch/7098101/ mentioned
> "The current implementation does not support "I2C Master Interface
> Extended Read Mode" to read data addressed by non-zero segment
> pointer, this means that if EDID has more than 1 extension blocks"

... EDID reading operation won't succeed.

Indeed.

> 
> With this patch,dw-hdmi can read EDID data with 1/2/4 blocks.
> 
> Signed-off-by: Nickey Yang <nickey.y...@rock-chips.com>
> Reviewed-by: Douglas Anderson <diand...@chromium.org>
> ---

Unfortunately I don't have a chance to test the change thoroughly, but
from what I see the implementation is correct. Thank you for the change.

Please feel free to update the commit message and add my

Acked-by: Vladimir Zapolskiy <vladimir_zapols...@mentor.com>

--
With best wishes,
Vladimir
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/5] of: introduce of_graph_get_remote_node

[Vladimir Zapolskiy]

Hi Rob,

On 02/04/2017 05:36 AM, Rob Herring wrote:
> The OF graph API leaves too much of the graph walking to clients when
> in many cases the driver doesn't care about accessing the port or
> endpoint nodes. The drivers typically just want the device connected via
> a particular graph connection. of_graph_get_remote_node provides this
> functionality.
> 
> Signed-off-by: Rob Herring 
> ---
>  drivers/of/base.c| 28 
>  include/linux/of_graph.h |  8 
>  2 files changed, 36 insertions(+)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index d4bea3c797d6..ea18ab16b92c 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -2469,3 +2469,31 @@ struct device_node *of_graph_get_remote_port(const 
> struct device_node *node)
>   return of_get_next_parent(np);
>  }
>  EXPORT_SYMBOL(of_graph_get_remote_port);
> +
> +struct device_node *of_graph_get_remote_node(const struct device_node *node,
> +  int port, int endpoint)

it would be nice to add a short comment that a returned device
node is expected to be dereferenced with of_node_put().

> +{
> + struct device_node *endpoint_node, *remote;
> +
> + endpoint_node = of_graph_get_endpoint_by_regs(node, port, endpoint);
> + if (!endpoint_node) {
> + pr_debug("no valid endpoint (%d, %d) for node %s\n",
> +  port, endpoint, node->full_name);
> + return NULL;
> + }
> +
> + remote = of_graph_get_remote_port_parent(endpoint_node);
> + of_node_put(endpoint);

Typo, here it should be of_node_put(endpoint_node);

> + if (!remote) {
> + pr_debug("no valid remote node\n");
> + return NULL;
> + }
> +
> + if (!of_device_is_available(remote)) {
> + pr_debug("not available for remote node\n");
> + return NULL;
> + }
> +
> + return remote;
> +}
> +EXPORT_SYMBOL(of_graph_get_remote_node);

--
With best wishes,
Vladimir
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Question regarding clocks in the DW-HDMI DT bindings

[Vladimir Zapolskiy]

Hi Laurent,

On 12/03/2016 07:16 PM, Laurent Pinchart wrote:
> Hi Fabio,
>
> On Friday 25 Nov 2016 13:29:37 Fabio Estevam wrote:
>> On Fri, Nov 25, 2016 at 1:22 PM, Laurent Pinchart wrote:
 I got the clock name from I.MX6Q TRM, I also checked the name again
 with Rockchip IC design team now, hope to get some new information soon.
>>>
>>> Thank you. While at it, could you ask them which version of the DW HDMI IP
>>> used in the SoC ?
>>
>> DW HDMI IP used in Rockchip is:
>> dwhdmi-rockchip ff98.hdmi: Detected HDMI controller 0x20:0xa:0xa0:0xc1
>>
>> as shown at
>> https://storage.kernelci.org/mainline/v4.9-rc6-157-g16ae16c6e561/arm-multi_
>> v7_defconfig/lab-collabora/boot-rk3288-rock2-square_rootfs:nfs.html
>>
>> DW HDMI IP used  on mx6q is:
>> dwhdmi-imx 12.hdmi: Detected HDMI controller 0x13:0xa:0xa0:0xc1
>
> Would you be able to print the value of the CONFIG[0-3]_ID registers as well ?
> I'm also interested in the same information for RK3288, as well as for IMX6DL.
>

  i.MX6Qi.MX6DL
DESIGN_ID 0x13  0x13
REVISION_ID   0x0a  0x1a<--- the only difference
PRODUCT_ID0   0xa0  0xa0
PRODUCT_ID1   0xc1  0xc1
CONFIG0_ID0x8f  0x8f
CONFIG1_ID0x01  0x01
CONFIG2_ID0xf2  0xf2<--- HDMI 3D TX PHY
CONFIG3_ID0x02  0x02

I'm not sure, if i.MX6D and MX6S have the same DW HDMI IP as on i.MX6Q
and i.MXDL respectively, and I don't have i.MX6DP or i.MX6QP powered board
on hand to dump the registers.

--
With best wishes,
Vladimir

Question regarding clocks in the DW-HDMI DT bindings

[Vladimir Zapolskiy]

On 11/25/2016 03:06 PM, Fabio Estevam wrote:
> Hi Vladimir,
>
> On Fri, Nov 25, 2016 at 11:00 AM, Vladimir Zapolskiy
>  wrote:
>
>> according to the DTSI files in the vanilla kernel DW HDMI IP is found
>> only on iMX6Q/D and iMX6DL/iMX6S SoCs (but please double check it),
>> so this approach should work ideally.
>
> After thinking more about this I think we can not get rid of "gpr". If
> we have a new i.MX SoC that is not compatible with
> "fsl,imx6q-iomuxc-gpr" then the lookup will fail.
>

Practically and when it becomes needed it should be possible to add SoC
specific hooks related to IOMUX_GPRx controls on basis of device
compatibles bound to the SoC variant.

Hypothetically if in future there is one more iMX SoC with a similar PCIe
or SATA controller but different GPR controls, to preserve backward
compatibility with old iMX6* DTB firmware the same handling of device
compatibles must be done in the drivers.

>> I see that the same has already been done in PCIe and SATA drivers,
>> but please consider to send a similar change against iMX LDB driver
>
> The i.MX LDB driver is also used on imx53, so we cannot search for
> "fsl,imx6q-iomuxc-gpr" compatible, as it will fail on imx53.
>
> So it seems we need to keep the "gpr" property in this case.
>

Right, I missed it. By chance GPR controls of LDB/LVDS are the same
on iMX53 and iMX6*, otherwise the driver shall care about GPR controls
differently, for example get a SoC specific GPR device compatible.

Nevertheless it is just a suggestion and it may remain just a mental
exercise of how to beautify/standardize iMX device binding descriptions.

--
With best wishes,
Vladimir

Question regarding clocks in the DW-HDMI DT bindings

[Vladimir Zapolskiy]

Hi Fabio,

On 11/25/2016 02:29 PM, Fabio Estevam wrote:
> Hi Vladimir,
>
> On Thu, Nov 24, 2016 at 8:16 PM, Vladimir Zapolskiy
>  wrote:
>
>> By the way while we're discussing DW HDMI bindings specific to iMX,
>> I would recommend to remove utterly hackish and iMX only "gpr"
>> property from the example in bindings/display/bridge/dw_hdmi.txt
>
> What if we get rid of the "gpr" property completely?
>
> --- a/drivers/gpu/drm/imx/dw_hdmi-imx.c
> +++ b/drivers/gpu/drm/imx/dw_hdmi-imx.c
> @@ -99,9 +99,8 @@ static const struct dw_hdmi_phy_config imx_phy_config[] = {
>
>  static int dw_hdmi_imx_parse_dt(struct imx_hdmi *hdmi)
>  {
> -   struct device_node *np = hdmi->dev->of_node;
> +   hdmi->regmap =
> syscon_regmap_lookup_by_compatible("fsl,imx6q-iomuxc-gpr");
>
> -   hdmi->regmap = syscon_regmap_lookup_by_phandle(np, "gpr");
> if (IS_ERR(hdmi->regmap)) {
> dev_err(hdmi->dev, "Unable to get gpr\n");
> return PTR_ERR(hdmi->regmap);
>
> Then we can remove the gpr from the device tree files.
>

according to the DTSI files in the vanilla kernel DW HDMI IP is found
only on iMX6Q/D and iMX6DL/iMX6S SoCs (but please double check it),
so this approach should work ideally.

I see that the same has already been done in PCIe and SATA drivers,
but please consider to send a similar change against iMX LDB driver
including removal of the property from imx6qdl.dtsi and
Documentation/devicetree/bindings/display/imx/ldb.txt.

And back to DW HDMI IP it seems that after removing "gpr" property
Documentation/devicetree/bindings/display/imx/hdmi.txt can be removed,
because bindings/display/bridge/dw_hdmi.txt replaces it.

--
With best wishes,
Vladimir

Question regarding clocks in the DW-HDMI DT bindings

[Vladimir Zapolskiy]

Hi,

On 11/25/2016 12:07 AM, Fabio Estevam wrote:
> Hi Laurent,
>
> On Thu, Nov 24, 2016 at 7:16 PM, Laurent Pinchart
>  wrote:
>> Hi Andy,
>>
>> As the author of the DW-HDMI DT bindings this question is addressed to you,
>> but information from anyone is more than welcome.
>>
>> The DT bindings specify two clocks named "iahb" and "isfr" but don't describe
>> them. While I assume that the "isfr" clock corresponds to the "isfrclk" input
>> signal of the DW HDMI, there is no "iahb" clock described in the IP core
>> datasheet.
>
> i.MX6Q has a DW-HDMI IP block.
>
> The names in the devicetree binding matches the ones listed at the
> i.MX6Q Reference Manual - Table 33-1. HDMI Clocks

correct, for your convenience the table is copied below:

Clock name | Clock Root | Description
---++---
   iahbclk  | ahb_clk_root   | Bus clock
   icecclk  | ckil_sync_clk_root | CEC low-frequency clock (32kHZ)
   ihclk| ahb_clk_root   | Module clock
   isfrclk  | video_27m_clk_root | Internal SFR clock (video clock 27MHz)

Here AHB stands for ARM Advanced High-performance Bus.

By the way while we're discussing DW HDMI bindings specific to iMX,
I would recommend to remove utterly hackish and iMX only "gpr"
property from the example in bindings/display/bridge/dw_hdmi.txt

--
With best wishes,
Vladimir

[PATCH 01/10] drm: bridge/dw_hdmi: add dw hdmi i2c bus adapter support

[Vladimir Zapolskiy]

Hi Ulrich,

On 11/11/2016 07:07 PM, Ulrich Hecht wrote:
> From: Vladimir Zapolskiy 
>
> The change adds support of internal HDMI I2C master controller, this
> subdevice is used by default, if "ddc-i2c-bus" DT property is omitted.
>
> The main purpose of this functionality is to support reading EDID from
> an HDMI monitor on boards, which don't have an I2C bus connected to
> DDC pins.
>
> The current implementation does not support "I2C Master Interface
> Extended Read Mode" to read data addressed by non-zero segment
> pointer, this means that if EDID has more than 1 extension blocks,
> EDID reading operation won't succeed, in my practice all tested HDMI
> monitors have at maximum one extension block.
>
> Signed-off-by: Vladimir Zapolskiy 
> Acked-by: Rob Herring 

many thanks to Philipp for pushing the change, as for today, it is in drm-next:

   https://cgit.freedesktop.org/~airlied/linux/commit/?h=drm-next=90233ee5d1

--
With best wishes,
Vladimir

[PATCH v6] drm: bridge/dw_hdmi: add dw hdmi i2c bus adapter support

[Vladimir Zapolskiy]

Hi Philipp,

On 09/16/2016 06:21 PM, Philipp Zabel wrote:
> Hi Vladimir,
>
> Am Mittwoch, den 14.09.2016, 21:46 +0300 schrieb Vladimir Zapolskiy:
>> Hi Philipp,
>>
>> On 08/29/2016 06:50 PM, Rob Herring wrote:
>>> On Wed, Aug 24, 2016 at 08:46:37AM +0300, Vladimir Zapolskiy wrote:
>>>> The change adds support of internal HDMI I2C master controller, this
>>>> subdevice is used by default, if "ddc-i2c-bus" DT property is omitted.
>>>>
>>>> The main purpose of this functionality is to support reading EDID from
>>>> an HDMI monitor on boards, which don't have an I2C bus connected to
>>>> DDC pins.
>>>>
>>>> The current implementation does not support "I2C Master Interface
>>>> Extended Read Mode" to read data addressed by non-zero segment
>>>> pointer, this means that if EDID has more than 1 extension blocks,
>>>> EDID reading operation won't succeed, in my practice all tested HDMI
>>>> monitors have at maximum one extension block.
>>>>
>>>> Signed-off-by: Vladimir Zapolskiy 
>>>> ---
>>>> The change is based on top of v4.8.0-rc1 and a fix in DW HDMI driver
>>>> https://patchwork.kernel.org/patch/9284717/
>>>>
>>>> Changes from v5 to v6:
>>>> * rebased on top of v4.8.0-rc1
>>>> * fixed one improper resource deallocation on error path of dw_hdmi_bind()
>>>> * added a comment describing a mutex asked by checkpatch.pl --strict
>>>>
>>>> Link to version v5: https://patchwork.kernel.org/patch/7279831/
>>>>
>>>> Changes from v4 to v5:
>>>> * do I2C bus controller initialization only once in bind() as it was done
>>>>   in v1-v3 of the change.
>>>>
>>>> Changes from v3 to v4, thanks to Doug and Philipp for review:
>>>> * set speed mode after software reset in dw_hdmi_i2c_init()
>>>> * by default set standard speed mode instead of fast speed mode, on iMX6Q
>>>>   this configures SCL to 100 KHz, which is compliant with HDMI 1.3a spec
>>>> * do I2C bus controller reinitialization on every data transfer,
>>>>   this change hopefully solves some observed problems on RK3288 platform
>>>> * added short functional change description to dw_hdmi.txt
>>>>
>>>> v3 of the change was
>>>>
>>>>   Tested-by: Philipp Zabel 
>>>>
>>>> Changes from v2 to v3, thanks to Russell:
>>>> * moved register field value definitions to dw_hdmi.h
>>>> * made completions uninterruptible to avoid transfer retries if interrupted
>>>> * use one completion for both read and write transfers as in v1,
>>>>   operation_reg is removed
>>>> * redundant i2c->stat = 0 is removed from dw_hdmi_i2c_read/write()
>>>> * struct i2c_algorithm dw_hdmi_algorithm is qualified as const
>>>> * dw_hdmi_i2c_adapter() does not modify struct dw_hdmi on error path
>>>> * dw_hdmi_i2c_irq() does not modify hdmi->i2c->stat, if interrupt is
>>>>   not for I2CM
>>>> * spin lock is removed from dw_hdmi_i2c_irq()
>>>> * removed spin lock from dw_hdmi_i2c_xfer() around write to
>>>>   HDMI_IH_MUTE_I2CM_STAT0 register
>>>> * split loop over message array in dw_hdmi_i2c_xfer() to validation
>>>>   and transfer parts
>>>> * added a mutex to serialize I2C transfer requests, i2c->lock is
>>>>   completely removed
>>>> * removed if(len) check from dw_hdmi_i2c_write(), hardware supports
>>>>   only len>0 transfers
>>>> * described extension blocks <= 1 limitation in the commit message
>>>> * a number of minor clean ups
>>>>
>>>> Changes from v1 to v2:
>>>> * fixed a devm_kfree() signature
>>>> * split completions for read and write operations
>>>>
>>>>  .../devicetree/bindings/display/bridge/dw_hdmi.txt |   4 +-
>>>
>>> Acked-by: Rob Herring 
>>>
>>>>  drivers/gpu/drm/bridge/dw-hdmi.c   | 265 
>>>> -
>>>>  drivers/gpu/drm/bridge/dw-hdmi.h   |  19 ++
>>>>  3 files changed, 281 insertions(+), 7 deletions(-)
>>
>> as far as I know David accepts pull requests from you, can you please
>> create and send a pull request for v4.9 containing these changes?
>>
>> https://patchwork.kernel.org/patch/9284717/ -- with Russell's ack
>
> Is that a forward looking statement? I don't see Russell's ack.

Here it is: http://www.spinics.net/lists/dri-devel/msg115880.html

Please don't bother about two other commits from that series, at least
there are official maintainers for the changes. And by the way please
consider to add youself as a maintainer of DW HDMI.

>> https://patchwork.kernel.org/patch/9296883/ -- with Rob's ack and yours 
>> tested-by
>>
>> Some users anticipate this change, for example see 
>> https://lkml.org/lkml/2016/9/14/55
>
> Those I see. I can re-test and prepare a pull request.
>

Thank you in advance!

--
With best wishes,
Vladimir

[PATCH v6] drm: bridge/dw_hdmi: add dw hdmi i2c bus adapter support

[Vladimir Zapolskiy]

Hi Philipp,

On 08/29/2016 06:50 PM, Rob Herring wrote:
> On Wed, Aug 24, 2016 at 08:46:37AM +0300, Vladimir Zapolskiy wrote:
>> The change adds support of internal HDMI I2C master controller, this
>> subdevice is used by default, if "ddc-i2c-bus" DT property is omitted.
>>
>> The main purpose of this functionality is to support reading EDID from
>> an HDMI monitor on boards, which don't have an I2C bus connected to
>> DDC pins.
>>
>> The current implementation does not support "I2C Master Interface
>> Extended Read Mode" to read data addressed by non-zero segment
>> pointer, this means that if EDID has more than 1 extension blocks,
>> EDID reading operation won't succeed, in my practice all tested HDMI
>> monitors have at maximum one extension block.
>>
>> Signed-off-by: Vladimir Zapolskiy 
>> ---
>> The change is based on top of v4.8.0-rc1 and a fix in DW HDMI driver
>> https://patchwork.kernel.org/patch/9284717/
>>
>> Changes from v5 to v6:
>> * rebased on top of v4.8.0-rc1
>> * fixed one improper resource deallocation on error path of dw_hdmi_bind()
>> * added a comment describing a mutex asked by checkpatch.pl --strict
>>
>> Link to version v5: https://patchwork.kernel.org/patch/7279831/
>>
>> Changes from v4 to v5:
>> * do I2C bus controller initialization only once in bind() as it was done
>>   in v1-v3 of the change.
>>
>> Changes from v3 to v4, thanks to Doug and Philipp for review:
>> * set speed mode after software reset in dw_hdmi_i2c_init()
>> * by default set standard speed mode instead of fast speed mode, on iMX6Q
>>   this configures SCL to 100 KHz, which is compliant with HDMI 1.3a spec
>> * do I2C bus controller reinitialization on every data transfer,
>>   this change hopefully solves some observed problems on RK3288 platform
>> * added short functional change description to dw_hdmi.txt
>>
>> v3 of the change was
>>
>>   Tested-by: Philipp Zabel 
>>
>> Changes from v2 to v3, thanks to Russell:
>> * moved register field value definitions to dw_hdmi.h
>> * made completions uninterruptible to avoid transfer retries if interrupted
>> * use one completion for both read and write transfers as in v1,
>>   operation_reg is removed
>> * redundant i2c->stat = 0 is removed from dw_hdmi_i2c_read/write()
>> * struct i2c_algorithm dw_hdmi_algorithm is qualified as const
>> * dw_hdmi_i2c_adapter() does not modify struct dw_hdmi on error path
>> * dw_hdmi_i2c_irq() does not modify hdmi->i2c->stat, if interrupt is
>>   not for I2CM
>> * spin lock is removed from dw_hdmi_i2c_irq()
>> * removed spin lock from dw_hdmi_i2c_xfer() around write to
>>   HDMI_IH_MUTE_I2CM_STAT0 register
>> * split loop over message array in dw_hdmi_i2c_xfer() to validation
>>   and transfer parts
>> * added a mutex to serialize I2C transfer requests, i2c->lock is
>>   completely removed
>> * removed if(len) check from dw_hdmi_i2c_write(), hardware supports
>>   only len>0 transfers
>> * described extension blocks <= 1 limitation in the commit message
>> * a number of minor clean ups
>>
>> Changes from v1 to v2:
>> * fixed a devm_kfree() signature
>> * split completions for read and write operations
>>
>>  .../devicetree/bindings/display/bridge/dw_hdmi.txt |   4 +-
>
> Acked-by: Rob Herring 
>
>>  drivers/gpu/drm/bridge/dw-hdmi.c   | 265 
>> -
>>  drivers/gpu/drm/bridge/dw-hdmi.h   |  19 ++
>>  3 files changed, 281 insertions(+), 7 deletions(-)

as far as I know David accepts pull requests from you, can you please
create and send a pull request for v4.9 containing these changes?

https://patchwork.kernel.org/patch/9284717/ -- with Russell's ack
https://patchwork.kernel.org/patch/9296883/ -- with Rob's ack and yours 
tested-by

Some users anticipate this change, for example see 
https://lkml.org/lkml/2016/9/14/55

--
With best wishes,
Vladimir

[PATCH v6] drm: bridge/dw_hdmi: add dw hdmi i2c bus adapter support

[Vladimir Zapolskiy]

Hello Russell,

On 08/24/2016 08:46 AM, Vladimir Zapolskiy wrote:
> The change adds support of internal HDMI I2C master controller, this
> subdevice is used by default, if "ddc-i2c-bus" DT property is omitted.
>
> The main purpose of this functionality is to support reading EDID from
> an HDMI monitor on boards, which don't have an I2C bus connected to
> DDC pins.
>
> The current implementation does not support "I2C Master Interface
> Extended Read Mode" to read data addressed by non-zero segment
> pointer, this means that if EDID has more than 1 extension blocks,
> EDID reading operation won't succeed, in my practice all tested HDMI
> monitors have at maximum one extension block.
>
> Signed-off-by: Vladimir Zapolskiy 
> ---

do you object to this change?

--
With best wishes,
Vladimir

[PATCH v6] drm: bridge/dw_hdmi: add dw hdmi i2c bus adapter support

[Vladimir Zapolskiy]

Hi Emil,

On 08/29/2016 07:41 PM, Emil Velikov wrote:
> Hi all,
>
> On 24 August 2016 at 06:46, Vladimir Zapolskiy
>  wrote:
>
>>  MODULE_AUTHOR("Sascha Hauer ");
>>  MODULE_AUTHOR("Andy Yan ");
>>  MODULE_AUTHOR("Yakir Yang ");
>> +MODULE_AUTHOR("Vladimir Zapolskiy ");
> Don't meant to start a flame-war or alike but to educate myself:
> Where does one draw the line about adding new author(s) of said
> module/subsystem ?

I support you, let's don't sink. Since it evokes questions I'm
ready to provide you with more background information.

> Afaict this is the most common (?) driver in DRM where the list has
> grown over time. Should we do the same with others ?

If you look into the essense of this change it adds support to
an I2C master controller, which is a part of DW HDMI controller.

Originally this particular change was done as a separate driver
in I2C subsystem about 3 years ago and delivered to the customers,
about 2 years ago I published its RFC version:

  RFC:   https://patchwork.ozlabs.org/patch/405100/
  v1 discussion: http://www.mailbrowse.com/linux-i2c/20949.html

As you may see this was a stand-alone driver, which served as
the fourth I2C master controller on iMX6Q, however I believe
it is better to merge two drivers into one. Does it sound like
a good enough reason to merge lists of authors as well?

Hope it clarifies the situation a bit.

--
With best wishes,
Vladimir

[PATCH v6] drm: bridge/dw_hdmi: add dw hdmi i2c bus adapter support

[Vladimir Zapolskiy]

The change adds support of internal HDMI I2C master controller, this
subdevice is used by default, if "ddc-i2c-bus" DT property is omitted.

The main purpose of this functionality is to support reading EDID from
an HDMI monitor on boards, which don't have an I2C bus connected to
DDC pins.

The current implementation does not support "I2C Master Interface
Extended Read Mode" to read data addressed by non-zero segment
pointer, this means that if EDID has more than 1 extension blocks,
EDID reading operation won't succeed, in my practice all tested HDMI
monitors have at maximum one extension block.

Signed-off-by: Vladimir Zapolskiy 
---
The change is based on top of v4.8.0-rc1 and a fix in DW HDMI driver
https://patchwork.kernel.org/patch/9284717/

Changes from v5 to v6:
* rebased on top of v4.8.0-rc1
* fixed one improper resource deallocation on error path of dw_hdmi_bind()
* added a comment describing a mutex asked by checkpatch.pl --strict

Link to version v5: https://patchwork.kernel.org/patch/7279831/

Changes from v4 to v5:
* do I2C bus controller initialization only once in bind() as it was done
  in v1-v3 of the change.

Changes from v3 to v4, thanks to Doug and Philipp for review:
* set speed mode after software reset in dw_hdmi_i2c_init()
* by default set standard speed mode instead of fast speed mode, on iMX6Q
  this configures SCL to 100 KHz, which is compliant with HDMI 1.3a spec
* do I2C bus controller reinitialization on every data transfer,
  this change hopefully solves some observed problems on RK3288 platform
* added short functional change description to dw_hdmi.txt

v3 of the change was

  Tested-by: Philipp Zabel 

Changes from v2 to v3, thanks to Russell:
* moved register field value definitions to dw_hdmi.h
* made completions uninterruptible to avoid transfer retries if interrupted
* use one completion for both read and write transfers as in v1,
  operation_reg is removed
* redundant i2c->stat = 0 is removed from dw_hdmi_i2c_read/write()
* struct i2c_algorithm dw_hdmi_algorithm is qualified as const
* dw_hdmi_i2c_adapter() does not modify struct dw_hdmi on error path
* dw_hdmi_i2c_irq() does not modify hdmi->i2c->stat, if interrupt is
  not for I2CM
* spin lock is removed from dw_hdmi_i2c_irq()
* removed spin lock from dw_hdmi_i2c_xfer() around write to
  HDMI_IH_MUTE_I2CM_STAT0 register
* split loop over message array in dw_hdmi_i2c_xfer() to validation
  and transfer parts
* added a mutex to serialize I2C transfer requests, i2c->lock is
  completely removed
* removed if(len) check from dw_hdmi_i2c_write(), hardware supports
  only len>0 transfers
* described extension blocks <= 1 limitation in the commit message
* a number of minor clean ups

Changes from v1 to v2:
* fixed a devm_kfree() signature
* split completions for read and write operations

 .../devicetree/bindings/display/bridge/dw_hdmi.txt |   4 +-
 drivers/gpu/drm/bridge/dw-hdmi.c   | 265 -
 drivers/gpu/drm/bridge/dw-hdmi.h   |  19 ++
 3 files changed, 281 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/bridge/dw_hdmi.txt 
b/Documentation/devicetree/bindings/display/bridge/dw_hdmi.txt
index dc1452f0d5d8..5e9a84d6e5f1 100644
--- a/Documentation/devicetree/bindings/display/bridge/dw_hdmi.txt
+++ b/Documentation/devicetree/bindings/display/bridge/dw_hdmi.txt
@@ -19,7 +19,9 @@ Required properties:

 Optional properties
 - reg-io-width: the width of the reg:1,4, default set to 1 if not present
-- ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
+- ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing,
+  if the property is omitted, a functionally reduced I2C bus
+  controller on DW HDMI is probed
 - clocks, clock-names: phandle to the HDMI CEC clock, name should be "cec"

 Example:
diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index ce3527cd0d25..cdf0a3a2e6f8 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -1,14 +1,15 @@
 /*
+ * DesignWare High-Definition Multimedia Interface (HDMI) driver
+ *
+ * Copyright (C) 2013-2015 Mentor Graphics Inc.
  * Copyright (C) 2011-2013 Freescale Semiconductor, Inc.
+ * Copyright (C) 2010, Guennadi Liakhovetski 
  *
  * 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.
  *
- * Designware High-Definition Multimedia Interface (HDMI) driver
- *
- * Copyright (C) 2010, Guennadi Liakhovetski 
  */
 #include 
 #include 
@@ -101,6 +102,17 @@ struct hdmi_data_info {
struct hdmi_vmode video_mode;
 };

+struct dw_hdmi_i2c {
+   struct i2c_adapter  adap;
+
+   struct mutexlock;   /* used to serialize data transfers */

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

[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 <rmk+kernel at armlinux.org.uk>
>

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 3/3] drm: tegra: use of_get_i2c_adapter_by_node interface

[Vladimir Zapolskiy]

This change is needed to properly lock I2C bus driver, which serves
DDC, otherwise there is an error in I2C bus driver user counting.

Note, that prior to the change put_device() coupled with
of_find_i2c_adapter_by_node() was missing on error path of
tegra_output_probe().

Signed-off-by: Vladimir Zapolskiy 
Cc: Thierry Reding 
---
 drivers/gpu/drm/tegra/output.c | 25 ++---
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c
index 595d1ec3e02e..1edfde77bb6d 100644
--- a/drivers/gpu/drm/tegra/output.c
+++ b/drivers/gpu/drm/tegra/output.c
@@ -113,14 +113,12 @@ int tegra_output_probe(struct tegra_output *output)

ddc = of_parse_phandle(output->of_node, "nvidia,ddc-i2c-bus", 0);
if (ddc) {
-   output->ddc = of_find_i2c_adapter_by_node(ddc);
+   output->ddc = of_get_i2c_adapter_by_node(ddc);
+   of_node_put(ddc);
if (!output->ddc) {
err = -EPROBE_DEFER;
-   of_node_put(ddc);
return err;
}
-
-   of_node_put(ddc);
}

output->hpd_gpio = of_get_named_gpio_flags(output->of_node,
@@ -133,14 +131,13 @@ int tegra_output_probe(struct tegra_output *output)
   "HDMI hotplug detect");
if (err < 0) {
dev_err(output->dev, "gpio_request_one(): %d\n", err);
-   return err;
+   goto i2c_release;
}

err = gpio_to_irq(output->hpd_gpio);
if (err < 0) {
dev_err(output->dev, "gpio_to_irq(): %d\n", err);
-   gpio_free(output->hpd_gpio);
-   return err;
+   goto gpio_release;
}

output->hpd_irq = err;
@@ -153,8 +150,7 @@ int tegra_output_probe(struct tegra_output *output)
if (err < 0) {
dev_err(output->dev, "failed to request IRQ#%u: %d\n",
output->hpd_irq, err);
-   gpio_free(output->hpd_gpio);
-   return err;
+   goto gpio_release;
}

output->connector.polled = DRM_CONNECTOR_POLL_HPD;
@@ -168,6 +164,14 @@ int tegra_output_probe(struct tegra_output *output)
}

return 0;
+
+gpio_release:
+   gpio_free(output->hpd_gpio);
+
+i2c_release:
+   i2c_put_adapter(output->ddc);
+
+   return err;
 }

 void tegra_output_remove(struct tegra_output *output)
@@ -177,8 +181,7 @@ void tegra_output_remove(struct tegra_output *output)
gpio_free(output->hpd_gpio);
}

-   if (output->ddc)
-   put_device(>ddc->dev);
+   i2c_put_adapter(output->ddc);
 }

 int tegra_output_init(struct drm_device *drm, struct tegra_output *output)
-- 
2.8.1

[RESEND PATCH v2 2/3] drm: tilcdc: use of_get_i2c_adapter_by_node interface

[Vladimir Zapolskiy]

This change is needed to properly lock I2C bus driver, which serves
DDC, otherwise there is an error in I2C bus driver user counting.

Prior to this change i2c_put_adapter() is misused, which may lead
to an overflow over zero of I2C bus driver user counter.

Signed-off-by: Vladimir Zapolskiy 
Cc: Jyri Sarha 
Cc: Tomi Valkeinen 
---
 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c 
b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
index 6b8c5b3bf588..73f29dc75d33 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
@@ -331,15 +331,13 @@ static int tfp410_probe(struct platform_device *pdev)
goto fail;
}

-   tfp410_mod->i2c = of_find_i2c_adapter_by_node(i2c_node);
+   tfp410_mod->i2c = of_get_i2c_adapter_by_node(i2c_node);
+   of_node_put(i2c_node);
if (!tfp410_mod->i2c) {
dev_err(>dev, "could not get i2c\n");
-   of_node_put(i2c_node);
goto fail;
}

-   of_node_put(i2c_node);
-
tfp410_mod->gpio = of_get_named_gpio_flags(node, "powerdn-gpio",
0, NULL);
if (tfp410_mod->gpio < 0) {
-- 
2.8.1

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

[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

[RESEND PATCH v2 0/3] drm: fix invalid user counters of i2c bus device

[Vladimir Zapolskiy]

This is a resend of the changes, the bugs were identified and fixed
one year ago, however the fixes are still not found in the mainline:

  
http://dri-devel.freedesktop.narkive.com/cWNFTOZC/patch-v2-0-3-drm-fix-i2c-adapter-device-driver-user-counter

of_find_i2c_adapter_by_node() call requires quite often missing
put_device(), and i2c_put_adapter() releases a device locked by
i2c_get_adapter() only.

Below is a common error reproduction scenario as a result of the
misusage described above, this is run on iMX6 platform 4.8.0-rc1
with HDMI and I2C bus drivers compiled as kernel modules for
clarity after changing arch/arm/configs/imx_v6_v7_defconfig:

  -CONFIG_I2C_IMX=y
  +CONFIG_I2C_IMX=m
  -CONFIG_DRM_IMX_HDMI=y
  +CONFIG_DRM_IMX_HDMI=m

  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

To fix existing users of these interfaces use of_get_i2c_adapter_by_node()
interface, which is similar to i2c_get_adapter() in sense that an I2C bus
device driver found and locked by a user can be correctly unlocked by
i2c_put_adapter() call.

Changes from v2 to v2 resend:
- none

Changes from v1 to v2:
- none, this series is a straightforward bugfix, v1 was a blend of
  I2C core changes, bugfixes and improvements

Vladimir Zapolskiy (3):
  drm: dw_hdmi: use of_get_i2c_adapter_by_node interface
  drm: tilcdc: use of_get_i2c_adapter_by_node interface
  drm: tegra: use of_get_i2c_adapter_by_node interface

 drivers/gpu/drm/bridge/dw-hdmi.c   | 14 +-
 drivers/gpu/drm/tegra/output.c | 25 ++---
 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c |  6 ++
 3 files changed, 25 insertions(+), 20 deletions(-)

-- 
2.8.1

[RFC 04/21] drm: bridge/dw_hdmi: add dw hdmi i2c bus adapter support

[Vladimir Zapolskiy]

Hi Ulrich,

On 30.05.2016 19:00, Ulrich Hecht wrote:
> From: Vladimir Zapolskiy 
> 
> The change adds support of internal HDMI I2C master controller, this
> subdevice is used by default, if "ddc-i2c-bus" DT property is omitted.
> 
> The main purpose of this functionality is to support reading EDID from
> an HDMI monitor on boards, which don't have an I2C bus connected to
> DDC pins.
> 
> Signed-off-by: Vladimir Zapolskiy 
> Signed-off-by: Koji Matsuoka 
> Signed-off-by: Geert Uytterhoeven <geert+renesas at glider.be>
> ---

thank you for pushing it, this is v3 of the change, right?

I think it might be better to consider to take the latest v5, it has
updated documentation notes, various improvements based on review from
Russell King, Doug Anderson and Philipp Zabel:

  http://www.spinics.net/lists/arm-kernel/msg447954.html

Still you may find it a bit outdated and it may require rebase, the change
was based on v4.3.0-rc3. Please let me know, if I can provide you support
of any kind regarding the change.

--
With best wishes,
Vladimir

[PATCH v2 0/3] drm: fix i2c adapter device driver user counter

[Vladimir Zapolskiy]

David, Russell,

ping. No response for more than 2 months.

On 02.11.2015 17:10, Vladimir Zapolskiy wrote:
> David, Russell,
> 
> ping.
> 
> On 12.10.2015 16:15, Vladimir Zapolskiy wrote:
>> David, Russell,
>>
>> ping.
>>
>> On 23.09.2015 00:46, Vladimir Zapolskiy wrote:
>>> of_find_i2c_adapter_by_node() call requires quite often missing
>>> put_device(), and i2c_put_adapter() releases a device locked by
>>> i2c_get_adapter() only.
>>>
>>> Below is a common error reproduction scenario as a result of the
>>> misusage described above (this is run on iMX6 platform with
>>> HDMI and I2C bus drivers compiled as kernel modules for clearness):
>>>
>>> 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
>>>
>>> To fix existing users of these interfaces use of_get_i2c_adapter_by_node()
>>> interface, which is similar to i2c_get_adapter() in sense that an I2C bus
>>> device driver found and locked by a user can be correctly unlocked by
>>> i2c_put_adapter() call.
>>>
>>> Changes from v1 to v2:
>>> - none, this series is a straightforward bugfix, v1 was a blend of
>>>   I2C core changes, bugfixes and improvements
>>>
>>> The change is based on dri/drm-next.
>>>
>>> Vladimir Zapolskiy (3):
>>>   drm: dw_hdmi: use of_get_i2c_adapter_by_node interface
>>>   drm: tilcdc: use of_get_i2c_adapter_by_node interface
>>>   drm: tegra: use of_get_i2c_adapter_by_node interface
>>>
>>>  drivers/gpu/drm/bridge/dw_hdmi.c   | 14 +-
>>>  drivers/gpu/drm/tegra/output.c | 23 ---
>>>  drivers/gpu/drm/tilcdc/tilcdc_tfp410.c |  6 ++
>>>  3 files changed, 23 insertions(+), 20 deletions(-)
>>>
>>
> 

--
With best wishes,
Vladimir

[PATCH v2 0/3] drm: fix i2c adapter device driver user counter

[Vladimir Zapolskiy]

David, Russell,

ping.

On 12.10.2015 16:15, Vladimir Zapolskiy wrote:
> David, Russell,
> 
> ping.
> 
> On 23.09.2015 00:46, Vladimir Zapolskiy wrote:
>> of_find_i2c_adapter_by_node() call requires quite often missing
>> put_device(), and i2c_put_adapter() releases a device locked by
>> i2c_get_adapter() only.
>>
>> Below is a common error reproduction scenario as a result of the
>> misusage described above (this is run on iMX6 platform with
>> HDMI and I2C bus drivers compiled as kernel modules for clearness):
>>
>> 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
>>
>> To fix existing users of these interfaces use of_get_i2c_adapter_by_node()
>> interface, which is similar to i2c_get_adapter() in sense that an I2C bus
>> device driver found and locked by a user can be correctly unlocked by
>> i2c_put_adapter() call.
>>
>> Changes from v1 to v2:
>> - none, this series is a straightforward bugfix, v1 was a blend of
>>   I2C core changes, bugfixes and improvements
>>
>> The change is based on dri/drm-next.
>>
>> Vladimir Zapolskiy (3):
>>   drm: dw_hdmi: use of_get_i2c_adapter_by_node interface
>>   drm: tilcdc: use of_get_i2c_adapter_by_node interface
>>   drm: tegra: use of_get_i2c_adapter_by_node interface
>>
>>  drivers/gpu/drm/bridge/dw_hdmi.c   | 14 +-
>>  drivers/gpu/drm/tegra/output.c | 23 ---
>>  drivers/gpu/drm/tilcdc/tilcdc_tfp410.c |  6 ++
>>  3 files changed, 23 insertions(+), 20 deletions(-)
>>
> 

--
With best wishes,
Vladimir

[PATCH v2 0/3] drm: fix i2c adapter device driver user counter

[Vladimir Zapolskiy]

David, Russell,

ping.

On 23.09.2015 00:46, Vladimir Zapolskiy wrote:
> of_find_i2c_adapter_by_node() call requires quite often missing
> put_device(), and i2c_put_adapter() releases a device locked by
> i2c_get_adapter() only.
> 
> Below is a common error reproduction scenario as a result of the
> misusage described above (this is run on iMX6 platform with
> HDMI and I2C bus drivers compiled as kernel modules for clearness):
> 
> 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
> 
> To fix existing users of these interfaces use of_get_i2c_adapter_by_node()
> interface, which is similar to i2c_get_adapter() in sense that an I2C bus
> device driver found and locked by a user can be correctly unlocked by
> i2c_put_adapter() call.
> 
> Changes from v1 to v2:
> - none, this series is a straightforward bugfix, v1 was a blend of
>   I2C core changes, bugfixes and improvements
> 
> The change is based on dri/drm-next.
> 
> Vladimir Zapolskiy (3):
>   drm: dw_hdmi: use of_get_i2c_adapter_by_node interface
>   drm: tilcdc: use of_get_i2c_adapter_by_node interface
>   drm: tegra: use of_get_i2c_adapter_by_node interface
> 
>  drivers/gpu/drm/bridge/dw_hdmi.c   | 14 +-
>  drivers/gpu/drm/tegra/output.c | 23 ---
>  drivers/gpu/drm/tilcdc/tilcdc_tfp410.c |  6 ++
>  3 files changed, 23 insertions(+), 20 deletions(-)
> 

--
With best wishes,
Vladimir

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

[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 <rmk+kernel at arm.linux.org.uk>
> Cc: Philipp Zabel 
> Cc: Andy Yan 

--
With best wishes,
Vladimir

[PATCH] drm: bridge/dw_hdmi: add dw hdmi i2c bus adapter support

[Vladimir Zapolskiy]

On 28.09.2015 23:06, Vladimir Zapolskiy wrote:
> The change adds support of internal HDMI I2C master controller, this
> subdevice is used by default, if "ddc-i2c-bus" DT property is omitted.
> 
> The main purpose of this functionality is to support reading EDID from
> an HDMI monitor on boards, which don't have an I2C bus connected to
> DDC pins.
> 
> The current implementation does not support "I2C Master Interface
> Extended Read Mode" to read data addressed by non-zero segment
> pointer, this means that if EDID has more than 1 extension blocks,
> EDID reading operation won't succeed, in my practice all tested HDMI
> monitors have at maximum one extension block.
> 
> Signed-off-by: Vladimir Zapolskiy 
> ---
>  .../devicetree/bindings/drm/bridge/dw_hdmi.txt |   4 +-
>  drivers/gpu/drm/bridge/dw_hdmi.c   | 263 
> -
>  drivers/gpu/drm/bridge/dw_hdmi.h   |  19 ++
>  3 files changed, 279 insertions(+), 7 deletions(-)
> 

This change is the same as one in reply, but no changelog/version info
(sent by mistake two copies, sorry for confusion).

--
With best wishes,
Vladimir

[PATCH v5] drm: bridge/dw_hdmi: add dw hdmi i2c bus adapter support

[Vladimir Zapolskiy]

The change adds support of internal HDMI I2C master controller, this
subdevice is used by default, if "ddc-i2c-bus" DT property is omitted.

The main purpose of this functionality is to support reading EDID from
an HDMI monitor on boards, which don't have an I2C bus connected to
DDC pins.

The current implementation does not support "I2C Master Interface
Extended Read Mode" to read data addressed by non-zero segment
pointer, this means that if EDID has more than 1 extension blocks,
EDID reading operation won't succeed, in my practice all tested HDMI
monitors have at maximum one extension block.

Signed-off-by: Vladimir Zapolskiy 
---
The change is based on v4.3.0-rc3 and it is applicable to rmk/drm-dwhdmi-devel,
please let me know, if it should be rebased.

Changes from v4 to v5:
- do I2C bus controller initialization only once in bind() as it was done
  in v1-v3 of the change.

Changes from v3 to v4, thanks to Doug and Philipp for review:
- set speed mode after software reset in dw_hdmi_i2c_init()
- by default set standard speed mode instead of fast speed mode, on iMX6Q
  this configures SCL to 100 KHz, which is compliant with HDMI 1.3a spec
- do I2C bus controller reinitialization on every data transfer,
  this change hopefully solves some observed problems on RK3288 platform
- added short functional change description to dw_hdmi.txt

v3 of the change was

  Tested-by: Philipp Zabel 

Changes from v2 to v3, thanks to Russell:
- moved register field value definitions to dw_hdmi.h
- made completions uninterruptible to avoid transfer retries if interrupted
- use one completion for both read and write transfers as in v1, operation_reg 
is removed
- redundant i2c->stat = 0 is removed from dw_hdmi_i2c_read/write()
- struct i2c_algorithm dw_hdmi_algorithm is qualified as const
- dw_hdmi_i2c_adapter() does not modify struct dw_hdmi on error path
- dw_hdmi_i2c_irq() does not modify hdmi->i2c->stat, if interrupt is not for 
I2CM
- spin lock is removed from dw_hdmi_i2c_irq()
- removed spin lock from dw_hdmi_i2c_xfer() around write to 
HDMI_IH_MUTE_I2CM_STAT0 register
- split loop over message array in dw_hdmi_i2c_xfer() to validation and 
transfer parts
- added a mutex to serialize I2C transfer requests, i2c->lock is completely 
removed
- removed if(len) check from dw_hdmi_i2c_write(), hardware supports only len>0 
transfers
- described extension blocks <= 1 limitation in the commit message
- a number of minor clean ups

Changes from v1 to v2:
- fixed a devm_kfree() signature
- split completions for read and write operations

 .../devicetree/bindings/drm/bridge/dw_hdmi.txt |   4 +-
 drivers/gpu/drm/bridge/dw_hdmi.c   | 263 -
 drivers/gpu/drm/bridge/dw_hdmi.h   |  19 ++
 3 files changed, 279 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt 
b/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt
index a905c14..55f5a7c 100644
--- a/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt
+++ b/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt
@@ -19,7 +19,9 @@ Required properties:

 Optional properties
 - reg-io-width: the width of the reg:1,4, default set to 1 if not present
-- ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
+- ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing,
+  if the property is omitted, a functionally reduced I2C bus
+  controller on DW HDMI is probed
 - clocks, clock-names: phandle to the HDMI CEC clock, name should be "cec"

 Example:
diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c
index 0083d4e..2a0a2d6 100644
--- a/drivers/gpu/drm/bridge/dw_hdmi.c
+++ b/drivers/gpu/drm/bridge/dw_hdmi.c
@@ -1,14 +1,15 @@
 /*
+ * DesignWare High-Definition Multimedia Interface (HDMI) driver
+ *
+ * Copyright (C) 2013-2015 Mentor Graphics Inc.
  * Copyright (C) 2011-2013 Freescale Semiconductor, Inc.
+ * Copyright (C) 2010, Guennadi Liakhovetski 
  *
  * 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.
  *
- * Designware High-Definition Multimedia Interface (HDMI) driver
- *
- * Copyright (C) 2010, Guennadi Liakhovetski 
  */
 #include 
 #include 
@@ -99,6 +100,17 @@ struct hdmi_data_info {
struct hdmi_vmode video_mode;
 };

+struct dw_hdmi_i2c {
+   struct i2c_adapter  adap;
+
+   struct mutexlock;
+   struct completion   cmp;
+   u8  stat;
+
+   u8  slave_reg;
+   boolis_regaddr;
+};
+
 struct dw_hdmi {
struct drm_connector connector;
struct drm_encoder *encoder;
@@ -108,6 +120,7 @@ struct dw_hdmi {
struct device *dev;
st

[PATCH] drm: bridge/dw_hdmi: add dw hdmi i2c bus adapter support

[Vladimir Zapolskiy]

The change adds support of internal HDMI I2C master controller, this
subdevice is used by default, if "ddc-i2c-bus" DT property is omitted.

The main purpose of this functionality is to support reading EDID from
an HDMI monitor on boards, which don't have an I2C bus connected to
DDC pins.

The current implementation does not support "I2C Master Interface
Extended Read Mode" to read data addressed by non-zero segment
pointer, this means that if EDID has more than 1 extension blocks,
EDID reading operation won't succeed, in my practice all tested HDMI
monitors have at maximum one extension block.

Signed-off-by: Vladimir Zapolskiy 
---
 .../devicetree/bindings/drm/bridge/dw_hdmi.txt |   4 +-
 drivers/gpu/drm/bridge/dw_hdmi.c   | 263 -
 drivers/gpu/drm/bridge/dw_hdmi.h   |  19 ++
 3 files changed, 279 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt 
b/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt
index a905c14..55f5a7c 100644
--- a/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt
+++ b/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt
@@ -19,7 +19,9 @@ Required properties:

 Optional properties
 - reg-io-width: the width of the reg:1,4, default set to 1 if not present
-- ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
+- ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing,
+  if the property is omitted, a functionally reduced I2C bus
+  controller on DW HDMI is probed
 - clocks, clock-names: phandle to the HDMI CEC clock, name should be "cec"

 Example:
diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c
index 0083d4e..2a0a2d6 100644
--- a/drivers/gpu/drm/bridge/dw_hdmi.c
+++ b/drivers/gpu/drm/bridge/dw_hdmi.c
@@ -1,14 +1,15 @@
 /*
+ * DesignWare High-Definition Multimedia Interface (HDMI) driver
+ *
+ * Copyright (C) 2013-2015 Mentor Graphics Inc.
  * Copyright (C) 2011-2013 Freescale Semiconductor, Inc.
+ * Copyright (C) 2010, Guennadi Liakhovetski 
  *
  * 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.
  *
- * Designware High-Definition Multimedia Interface (HDMI) driver
- *
- * Copyright (C) 2010, Guennadi Liakhovetski 
  */
 #include 
 #include 
@@ -99,6 +100,17 @@ struct hdmi_data_info {
struct hdmi_vmode video_mode;
 };

+struct dw_hdmi_i2c {
+   struct i2c_adapter  adap;
+
+   struct mutexlock;
+   struct completion   cmp;
+   u8  stat;
+
+   u8  slave_reg;
+   boolis_regaddr;
+};
+
 struct dw_hdmi {
struct drm_connector connector;
struct drm_encoder *encoder;
@@ -108,6 +120,7 @@ struct dw_hdmi {
struct device *dev;
struct clk *isfr_clk;
struct clk *iahb_clk;
+   struct dw_hdmi_i2c *i2c;

struct hdmi_data_info hdmi_data;
const struct dw_hdmi_plat_data *plat_data;
@@ -184,6 +197,201 @@ static void hdmi_mask_writeb(struct dw_hdmi *hdmi, u8 
data, unsigned int reg,
hdmi_modb(hdmi, data << shift, mask, reg);
 }

+static void dw_hdmi_i2c_init(struct dw_hdmi *hdmi)
+{
+   /* Software reset */
+   hdmi_writeb(hdmi, 0x00, HDMI_I2CM_SOFTRSTZ);
+
+   /* Set Standard Mode speed (determined to be 100KHz on iMX6) */
+   hdmi_writeb(hdmi, 0x00, HDMI_I2CM_DIV);
+
+   /* Set done, not acknowledged and arbitration interrupt polarities */
+   hdmi_writeb(hdmi, HDMI_I2CM_INT_DONE_POL, HDMI_I2CM_INT);
+   hdmi_writeb(hdmi, HDMI_I2CM_CTLINT_NAC_POL | HDMI_I2CM_CTLINT_ARB_POL,
+   HDMI_I2CM_CTLINT);
+
+   /* Clear DONE and ERROR interrupts */
+   hdmi_writeb(hdmi, HDMI_IH_I2CM_STAT0_ERROR | HDMI_IH_I2CM_STAT0_DONE,
+   HDMI_IH_I2CM_STAT0);
+
+   /* Mute DONE and ERROR interrupts */
+   hdmi_writeb(hdmi, HDMI_IH_I2CM_STAT0_ERROR | HDMI_IH_I2CM_STAT0_DONE,
+   HDMI_IH_MUTE_I2CM_STAT0);
+}
+
+static int dw_hdmi_i2c_read(struct dw_hdmi *hdmi,
+   unsigned char *buf, unsigned int length)
+{
+   struct dw_hdmi_i2c *i2c = hdmi->i2c;
+   int stat;
+
+   if (!i2c->is_regaddr) {
+   dev_dbg(hdmi->dev, "set read register address to 0\n");
+   i2c->slave_reg = 0x00;
+   i2c->is_regaddr = true;
+   }
+
+   while (length--) {
+   reinit_completion(>cmp);
+
+   hdmi_writeb(hdmi, i2c->slave_reg++, HDMI_I2CM_ADDRESS);
+   hdmi_writeb(hdmi, HDMI_I2CM_OPERATION_READ,
+   HDMI_I2CM_OPERATION);
+
+   stat = wait_for_completion_timeout(>cmp, HZ / 

[PATCH v4] drm: bridge/dw_hdmi: add dw hdmi i2c bus adapter support

[Vladimir Zapolskiy]

Hi Doug,

On 28.09.2015 19:29, Doug Anderson wrote:
> Vladimir,
> 
> On Mon, Sep 21, 2015 at 8:07 AM, Vladimir Zapolskiy
>  wrote:
>> - do I2C bus controller reinitialization on every data transfer,
>>   this change hopefully solves some observed problems on RK3288 platform
> 
> My apologies, but this appears to be a bad idea...
> 
> We just discovered that this was the root cause of a bug.  If an HDCP
> transfer is happening then the soft reset messes things up.  In our
> tree we've moving things back to the way they were.  :(

therefore I do the same.

Since unfortunately I'm limited in testing your tree and RK3288
platform, probably it might be more productive, if you send any updates
on top of my changes -- I'll be capable to review/test them for
regressions on iMX6 in vanilla.

--
With best wishes,
Vladimir

[PATCH v2 3/3] drm: tegra: use of_get_i2c_adapter_by_node interface

[Vladimir Zapolskiy]

This change is needed to properly lock I2C bus driver, which serves DDC.

On release of_get_i2c_adapter_by_node() requires i2c_put_adapter() call.

Note, that prior to the change put_device() coupled with
of_find_i2c_adapter_by_node() was missing on error path of
tegra_output_probe().

Signed-off-by: Vladimir Zapolskiy 
Cc: Thierry Reding 
Cc: Terje Bergström 
---
Changes from v1 to v2:
- converted two of_node_put(ddc) calls into one

 drivers/gpu/drm/tegra/output.c | 23 ---
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c
index 46664b6..9f3cec5 100644
--- a/drivers/gpu/drm/tegra/output.c
+++ b/drivers/gpu/drm/tegra/output.c
@@ -120,14 +120,12 @@ int tegra_output_probe(struct tegra_output *output)

ddc = of_parse_phandle(output->of_node, "nvidia,ddc-i2c-bus", 0);
if (ddc) {
-   output->ddc = of_find_i2c_adapter_by_node(ddc);
+   output->ddc = of_get_i2c_adapter_by_node(ddc);
+   of_node_put(ddc);
if (!output->ddc) {
err = -EPROBE_DEFER;
-   of_node_put(ddc);
return err;
}
-
-   of_node_put(ddc);
}

output->hpd_gpio = of_get_named_gpio_flags(output->of_node,
@@ -140,14 +138,13 @@ int tegra_output_probe(struct tegra_output *output)
   "HDMI hotplug detect");
if (err < 0) {
dev_err(output->dev, "gpio_request_one(): %d\n", err);
-   return err;
+   goto i2c_release;
}

err = gpio_to_irq(output->hpd_gpio);
if (err < 0) {
dev_err(output->dev, "gpio_to_irq(): %d\n", err);
-   gpio_free(output->hpd_gpio);
-   return err;
+   goto gpio_release;
}

output->hpd_irq = err;
@@ -160,8 +157,7 @@ int tegra_output_probe(struct tegra_output *output)
if (err < 0) {
dev_err(output->dev, "failed to request IRQ#%u: %d\n",
output->hpd_irq, err);
-   gpio_free(output->hpd_gpio);
-   return err;
+   goto gpio_release;
}

output->connector.polled = DRM_CONNECTOR_POLL_HPD;
@@ -175,6 +171,12 @@ int tegra_output_probe(struct tegra_output *output)
}

return 0;
+
+ gpio_release:
+   gpio_free(output->hpd_gpio);
+ i2c_release:
+   i2c_put_adapter(output->ddc);
+   return err;
 }

 void tegra_output_remove(struct tegra_output *output)
@@ -184,8 +186,7 @@ void tegra_output_remove(struct tegra_output *output)
gpio_free(output->hpd_gpio);
}

-   if (output->ddc)
-   put_device(>ddc->dev);
+   i2c_put_adapter(output->ddc);
 }

 int tegra_output_init(struct drm_device *drm, struct tegra_output *output)
-- 
2.5.0

[PATCH v2 2/3] drm: tilcdc: use of_get_i2c_adapter_by_node interface

[Vladimir Zapolskiy]

This change is needed to properly lock I2C bus driver, which serves DDC.

Prior to this change i2c_put_adapter() is misused, which may lead
to an overflow over zero of I2C bus driver user counter.

Signed-off-by: Vladimir Zapolskiy 
---
Changes from v1 to v2:
- none

 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c 
b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
index 354c47c..4dc78c7 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
@@ -348,15 +348,13 @@ static int tfp410_probe(struct platform_device *pdev)
goto fail;
}

-   tfp410_mod->i2c = of_find_i2c_adapter_by_node(i2c_node);
+   tfp410_mod->i2c = of_get_i2c_adapter_by_node(i2c_node);
+   of_node_put(i2c_node);
if (!tfp410_mod->i2c) {
dev_err(>dev, "could not get i2c\n");
-   of_node_put(i2c_node);
goto fail;
}

-   of_node_put(i2c_node);
-
tfp410_mod->gpio = of_get_named_gpio_flags(node, "powerdn-gpio",
0, NULL);
if (IS_ERR_VALUE(tfp410_mod->gpio)) {
-- 
2.5.0

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

[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 <rmk+kernel at arm.linux.org.uk>
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

[PATCH v2 0/3] drm: fix i2c adapter device driver user counter

[Vladimir Zapolskiy]

of_find_i2c_adapter_by_node() call requires quite often missing
put_device(), and i2c_put_adapter() releases a device locked by
i2c_get_adapter() only.

Below is a common error reproduction scenario as a result of the
misusage described above (this is run on iMX6 platform with
HDMI and I2C bus drivers compiled as kernel modules for clearness):

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

To fix existing users of these interfaces use of_get_i2c_adapter_by_node()
interface, which is similar to i2c_get_adapter() in sense that an I2C bus
device driver found and locked by a user can be correctly unlocked by
i2c_put_adapter() call.

Changes from v1 to v2:
- none, this series is a straightforward bugfix, v1 was a blend of
  I2C core changes, bugfixes and improvements

The change is based on dri/drm-next.

Vladimir Zapolskiy (3):
  drm: dw_hdmi: use of_get_i2c_adapter_by_node interface
  drm: tilcdc: use of_get_i2c_adapter_by_node interface
  drm: tegra: use of_get_i2c_adapter_by_node interface

 drivers/gpu/drm/bridge/dw_hdmi.c   | 14 +-
 drivers/gpu/drm/tegra/output.c | 23 ---
 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c |  6 ++
 3 files changed, 23 insertions(+), 20 deletions(-)

-- 
2.5.0

[PATCH 2/2] drm: sti_hdmi: use of_get_i2c_adapter_by_node interface

[Vladimir Zapolskiy]

This change is needed to properly lock I2C bus device and driver,
which serve DDC lines. Without this change I2C bus driver module
may gone in runtime and this won't be noticed by the driver.

Signed-off-by: Vladimir Zapolskiy 
---
 drivers/gpu/drm/sti/sti_hdmi.c | 17 ++---
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
index c23b580..b892f29a 100644
--- a/drivers/gpu/drm/sti/sti_hdmi.c
+++ b/drivers/gpu/drm/sti/sti_hdmi.c
@@ -792,13 +792,10 @@ static int sti_hdmi_probe(struct platform_device *pdev)

ddc = of_parse_phandle(pdev->dev.of_node, "ddc", 0);
if (ddc) {
-   hdmi->ddc_adapt = of_find_i2c_adapter_by_node(ddc);
-   if (!hdmi->ddc_adapt) {
-   of_node_put(ddc);
-   return -EPROBE_DEFER;
-   }
-
+   hdmi->ddc_adapt = of_get_i2c_adapter_by_node(ddc);
of_node_put(ddc);
+   if (!hdmi->ddc_adapt)
+   return -EPROBE_DEFER;
}

hdmi->dev = pdev->dev;
@@ -887,8 +884,7 @@ static int sti_hdmi_probe(struct platform_device *pdev)
return component_add(>dev, _hdmi_ops);

  release_adapter:
-   if (hdmi->ddc_adapt)
-   put_device(>ddc_adapt->dev);
+   i2c_put_adapter(hdmi->ddc_adapt);

return ret;
 }
@@ -897,10 +893,9 @@ static int sti_hdmi_remove(struct platform_device *pdev)
 {
struct sti_hdmi *hdmi = dev_get_drvdata(>dev);

-   if (hdmi->ddc_adapt)
-   put_device(>ddc_adapt->dev);
-
+   i2c_put_adapter(hdmi->ddc_adapt);
component_del(>dev, _hdmi_ops);
+
return 0;
 }

-- 
2.5.0

[PATCH 1/2] drm: sti_hdmi: fix i2c adapter device refcounting

[Vladimir Zapolskiy]

The commit 53bdcf5f026c ("drm: sti: fix sub-components bind") moves
i2c adapter search and locking from .bind() to .probe(), however
proper error path in the modified .probe() is not implemented and
leftover of the related error path in .bind() remains. This change
fixes these issues.

Fixes: 53bdcf5f026c ("drm: sti: fix sub-components bind")
Signed-off-by: Vladimir Zapolskiy 
---
 drivers/gpu/drm/sti/sti_hdmi.c | 49 ++
 1 file changed, 31 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
index 09e29e4..c23b580 100644
--- a/drivers/gpu/drm/sti/sti_hdmi.c
+++ b/drivers/gpu/drm/sti/sti_hdmi.c
@@ -700,18 +700,17 @@ static int sti_hdmi_bind(struct device *dev, struct 
device *master, void *data)

encoder = sti_hdmi_find_encoder(drm_dev);
if (!encoder)
-   goto err_adapt;
+   return -EINVAL;

connector = devm_kzalloc(dev, sizeof(*connector), GFP_KERNEL);
if (!connector)
-   goto err_adapt;
-
+   return -EINVAL;

connector->hdmi = hdmi;

bridge = devm_kzalloc(dev, sizeof(*bridge), GFP_KERNEL);
if (!bridge)
-   goto err_adapt;
+   return -EINVAL;

bridge->driver_private = hdmi;
bridge->funcs = _hdmi_bridge_funcs;
@@ -748,8 +747,7 @@ err_sysfs:
drm_connector_unregister(drm_connector);
 err_connector:
drm_connector_cleanup(drm_connector);
-err_adapt:
-   put_device(>ddc_adapt->dev);
+
return -EINVAL;
 }

@@ -809,24 +807,29 @@ static int sti_hdmi_probe(struct platform_device *pdev)
res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "hdmi-reg");
if (!res) {
DRM_ERROR("Invalid hdmi resource\n");
-   return -ENOMEM;
+   ret = -ENOMEM;
+   goto release_adapter;
}
hdmi->regs = devm_ioremap_nocache(dev, res->start, resource_size(res));
-   if (!hdmi->regs)
-   return -ENOMEM;
+   if (!hdmi->regs) {
+   ret = -ENOMEM;
+   goto release_adapter;
+   }

if (of_device_is_compatible(np, "st,stih416-hdmi")) {
res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
   "syscfg");
if (!res) {
DRM_ERROR("Invalid syscfg resource\n");
-   return -ENOMEM;
+   ret = -ENOMEM;
+   goto release_adapter;
}
hdmi->syscfg = devm_ioremap_nocache(dev, res->start,
resource_size(res));
-   if (!hdmi->syscfg)
-   return -ENOMEM;
-
+   if (!hdmi->syscfg) {
+   ret = -ENOMEM;
+   goto release_adapter;
+   }
}

hdmi->phy_ops = (struct hdmi_phy_ops *)
@@ -836,25 +839,29 @@ static int sti_hdmi_probe(struct platform_device *pdev)
hdmi->clk_pix = devm_clk_get(dev, "pix");
if (IS_ERR(hdmi->clk_pix)) {
DRM_ERROR("Cannot get hdmi_pix clock\n");
-   return PTR_ERR(hdmi->clk_pix);
+   ret = PTR_ERR(hdmi->clk_pix);
+   goto release_adapter;
}

hdmi->clk_tmds = devm_clk_get(dev, "tmds");
if (IS_ERR(hdmi->clk_tmds)) {
DRM_ERROR("Cannot get hdmi_tmds clock\n");
-   return PTR_ERR(hdmi->clk_tmds);
+   ret = PTR_ERR(hdmi->clk_tmds);
+   goto release_adapter;
}

hdmi->clk_phy = devm_clk_get(dev, "phy");
if (IS_ERR(hdmi->clk_phy)) {
DRM_ERROR("Cannot get hdmi_phy clock\n");
-   return PTR_ERR(hdmi->clk_phy);
+   ret = PTR_ERR(hdmi->clk_phy);
+   goto release_adapter;
}

hdmi->clk_audio = devm_clk_get(dev, "audio");
if (IS_ERR(hdmi->clk_audio)) {
DRM_ERROR("Cannot get hdmi_audio clock\n");
-   return PTR_ERR(hdmi->clk_audio);
+   ret = PTR_ERR(hdmi->clk_audio);
+   goto release_adapter;
}

hdmi->hpd = readl(hdmi->regs + HDMI_STA) & HDMI_STA_HOT_PLUG;
@@ -867,7 +874,7 @@ static int sti_hdmi_probe(struct platform_device *pdev)
hdmi_irq_thread, IRQF_ONESHOT, dev_name(dev), hdmi);
if (ret) {
DRM_ERROR("Failed to register HDMI interrupt\n");
-   return ret;
+   goto release_adapter;
}

hdmi->reset = devm_reset_control_get(dev, "hdmi");
@@ -878,6

[PATCH v4] drm: bridge/dw_hdmi: add dw hdmi i2c bus adapter support

[Vladimir Zapolskiy]

The change adds support of internal HDMI I2C master controller, this
subdevice is used by default, if "ddc-i2c-bus" DT property is omitted.

The main purpose of this functionality is to support reading EDID from
an HDMI monitor on boards, which don't have an I2C bus connected to
DDC pins.

The current implementation does not support "I2C Master Interface
Extended Read Mode" to read data addressed by non-zero segment
pointer, this means that if EDID has more than 1 extension blocks,
EDID reading operation won't succeed, in my practice all tested HDMI
monitors have at maximum one extension block.

Signed-off-by: Vladimir Zapolskiy 
---
The change is based on v4.3.0-rc2 and it is applicable to rmk/drm-dwhdmi-devel,
please let me know, if it should be rebased.

v3 of the change was

  Tested-by: Philipp Zabel 

Changes frov v3 to v4, thanks to Doug and Philipp for review:
- set speed mode after software reset in dw_hdmi_i2c_init()
- by default set standard speed mode instead of fast speed mode, on iMX6Q
  this configures SCL to 100 KHz, which is compliant with HDMI 1.3a spec
- do I2C bus controller reinitialization on every data transfer,
  this change hopefully solves some observed problems on RK3288 platform
- added short functional change description to dw_hdmi.txt

Changes from v2 to v3, thanks to Russell:
- moved register field value definitions to dw_hdmi.h
- made completions uninterruptible to avoid transfer retries if interrupted
- use one completion for both read and write transfers as in v1, operation_reg 
is removed
- redundant i2c->stat = 0 is removed from dw_hdmi_i2c_read/write()
- struct i2c_algorithm dw_hdmi_algorithm is qualified as const
- dw_hdmi_i2c_adapter() does not modify struct dw_hdmi on error path
- dw_hdmi_i2c_irq() does not modify hdmi->i2c->stat, if interrupt is not for 
I2CM
- spin lock is removed from dw_hdmi_i2c_irq()
- removed spin lock from dw_hdmi_i2c_xfer() around write to 
HDMI_IH_MUTE_I2CM_STAT0 register
- split loop over message array in dw_hdmi_i2c_xfer() to validation and 
transfer parts
- added a mutex to serialize I2C transfer requests, i2c->lock is completely 
removed
- removed if(len) check from dw_hdmi_i2c_write(), hardware supports only len>0 
transfers
- described extension blocks <= 1 limitation in the commit message
- a number of minor clean ups

Changes from v1 to v2:
- fixed a devm_kfree() signature
- split completions for read and write operations

 .../devicetree/bindings/drm/bridge/dw_hdmi.txt |   4 +-
 drivers/gpu/drm/bridge/dw_hdmi.c   | 258 -
 drivers/gpu/drm/bridge/dw_hdmi.h   |  19 ++
 3 files changed, 274 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt 
b/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt
index a905c14..55f5a7c 100644
--- a/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt
+++ b/Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt
@@ -19,7 +19,9 @@ Required properties:

 Optional properties
 - reg-io-width: the width of the reg:1,4, default set to 1 if not present
-- ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
+- ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing,
+  if the property is omitted, a functionally reduced I2C bus
+  controller on DW HDMI is probed
 - clocks, clock-names: phandle to the HDMI CEC clock, name should be "cec"

 Example:
diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c
index 0083d4e..58dcacf 100644
--- a/drivers/gpu/drm/bridge/dw_hdmi.c
+++ b/drivers/gpu/drm/bridge/dw_hdmi.c
@@ -1,14 +1,15 @@
 /*
+ * DesignWare High-Definition Multimedia Interface (HDMI) driver
+ *
+ * Copyright (C) 2013-2015 Mentor Graphics Inc.
  * Copyright (C) 2011-2013 Freescale Semiconductor, Inc.
+ * Copyright (C) 2010, Guennadi Liakhovetski 
  *
  * 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.
  *
- * Designware High-Definition Multimedia Interface (HDMI) driver
- *
- * Copyright (C) 2010, Guennadi Liakhovetski 
  */
 #include 
 #include 
@@ -99,6 +100,17 @@ struct hdmi_data_info {
struct hdmi_vmode video_mode;
 };

+struct dw_hdmi_i2c {
+   struct i2c_adapter  adap;
+
+   struct mutexlock;
+   struct completion   cmp;
+   u8  stat;
+
+   u8  slave_reg;
+   boolis_regaddr;
+};
+
 struct dw_hdmi {
struct drm_connector connector;
struct drm_encoder *encoder;
@@ -108,6 +120,7 @@ struct dw_hdmi {
struct device *dev;
struct clk *isfr_clk;
struct clk *iahb_clk;
+   struct dw_hdmi_i2c *i2c;

struct hdmi_data_info hdmi_data;
cons

[PATCH v3 2/2] drm: bridge/dw_hdmi: add dw hdmi i2c bus adapter support

[Vladimir Zapolskiy]

On 17.09.2015 01:18, Russell King - ARM Linux wrote:
> On Wed, Sep 16, 2015 at 02:56:57PM -0700, Doug Anderson wrote:
>> Yes, I'd expect 100kHz and 400kHz.
>>
>> I agree that 50ms is non-trivial, but it's also not something you're
>> doing lots of.  I'd expect that the EDID is read over this channel at
>> cable plugin time and then not used much after that.  Adding an extra
>> 40ms (10ms vs 50ms) before we can access the TV doesn't seem terrible
>> for compatibility.
>>
>> Doing a quick scan for what others in mainline do:
>>
>> A few can be found with:
>>
>> $ git grep -A3 hdmiddc | grep clock-freq
>> arch/arm/boot/dts/stihxxx-b2120.dtsi-
>> clock-frequency = <10>;
>> arch/arm/boot/dts/tegra30-apalis.dtsi-  clock-frequency = <10>;
>> arch/arm/boot/dts/tegra30-beaver.dts-   clock-frequency = <10>;
>> arch/arm/boot/dts/tegra30-colibri.dtsi- clock-frequency = <10>;
> 
> This is a sure way to propagate a bug.
> 
> I said in a previous email that you need to check the HDMI and CEA
> specs.  I've done this, and HDMI 1.3a specifies a maximum SCL clock
> rate of 100kHz.
> 
> So that's settled then.  100kHz is must be.  Using 400kHz is out of
> specification.
> 

FYI I've managed to measure SCL on iMX6, fast mode stands for 333KHz and
standard mode stands for 100KHz, therefore I'll set the latter mode.

--
With best wishes,
Vladimir

[PATCH v3 2/2] drm: bridge/dw_hdmi: add dw hdmi i2c bus adapter support

[Vladimir Zapolskiy]

Hi Doug,

On 03.09.2015 02:19, Doug Anderson wrote:
> Russell,
> 
> On Wed, Sep 2, 2015 at 4:04 PM, Russell King - ARM Linux
>  wrote:
 Also, is it appropriate to hook non-DDC devices to a DDC bus?  I suspect
 that's asking for trouble.
>>>
>>> I doubt it's appropriate.  Why do you ask?
>>
>> To find out why you want to "specify the I2C bus".
>>
>> Surely if we're talking about the DDC bus, we either want to use a
>> separate I2C bus (in which case the HDMI DT description needs to
>> specify which I2C bus to use) or we want to use the HDMI-internal
>> I2C bus, which being part of the HDMI driver, the HDMI driver will
>> know how to find it itself - there should be no need to put an
>> explicit ddc-i2c-bus self-reference there in that case.
> 
> Overall it comes down to bus numbering.  Possibly that's a stupid
> reason, but it is my reason nevertheless.

this is a known issue regarding I2C bus numbering.

> Specifically it significantly helps my brain process kernel log
> messages if the i2c bus that's referred to "bus 5" in my SoC's user
> manual shows up consistently as "i2c5" in kernel log messages.  It's
> helpful it it shows up as "i2c5" even if "i2c0" - "i2c4" aren't
> enabled.
> 
> That's all totally possible by using this type of syntax, like in rk3288.dtsi:
> 
> aliases {
>   i2c0 = 
>   i2c1 = 
>   i2c2 = 
>   i2c3 = 
>   i2c4 = 
>   i2c5 = 
> 
> Similarly, I'd like "bus 0" to show up as i2c0, which will happen as
> you can see in the above.
> 
> The problem is that if another bus registers itself before the SoC's
> i2c0 registers itself and that bus doesn't give a number to itself
> then the i2c subsystem will chose "I2C 0".  ...and then when the main
> SoC i2c bus registers itself it will fail because i2c0 is already
> taken.
> 
> By having a of_node for the hdmi i2c bus, we can assign a number to it like:
>   i2c15 = 
> 
> This is all described in the two links I referenced in my original reply.
> 
> 
> A possible other option is to have the i2c subsystem try to start
> numbering at a larger base for all automatically numbered busses
> (those that didn't specify a number).  Then it's more likely (though
> still not guaranteed) to conflict with another bus...

Could you please check if commit 03bde7c31a3 serves you?

--
With best wishes,
Vladimir

[PATCH v3 2/2] drm: bridge/dw_hdmi: add dw hdmi i2c bus adapter support

[Vladimir Zapolskiy]

Hi Doug,

On 16.09.2015 23:04, Doug Anderson wrote:
> Hi,
> 
> On Sun, Aug 30, 2015 at 2:34 PM, Vladimir Zapolskiy
>  wrote:
>> The change adds support of internal HDMI I2C master controller, this
>> subdevice is used by default, if "ddc-i2c-bus" DT property is omitted.
>>
>> The main purpose of this functionality is to support reading EDID from
>> an HDMI monitor on boards, which don't have an I2C bus connected to
>> DDC pins.
>>
>> The current implementation does not support "I2C Master Interface
>> Extended Read Mode" to read data addressed by non-zero segment
>> pointer, this means that if EDID has more than 1 extension blocks,
>> EDID reading operation won't succeed, in my practice all tested HDMI
>> monitors have at maximum one extension block.
>>
>> Signed-off-by: Vladimir Zapolskiy 
>> ---
>> The change is based on today's v4.2, please let me know, if it should be 
>> rebased.
>>
>> The change has compilation dependency on I2CM_ADDRESS register name fix,
>> see http://lists.freedesktop.org/archives/dri-devel/2015-May/082980.html
>>
>> From http://lists.freedesktop.org/archives/dri-devel/2015-May/082981.html
>> v1 of the change was
>>
>>   Tested-by: Philipp Zabel 
>>
>> but I hesitate to add the tag here due to multiple updates in v2.
>>
>> Changes from v2 to v3, thanks to Russell:
>> - moved register field value definitions to dw_hdmi.h
>> - made completions uninterruptible to avoid transfer retries if interrupted
>> - use one completion for both read and write transfers as in v1, 
>> operation_reg is removed
>> - redundant i2c->stat = 0 is removed from dw_hdmi_i2c_read/write()
>> - struct i2c_algorithm dw_hdmi_algorithm is qualified as const
>> - dw_hdmi_i2c_adapter() does not modify struct dw_hdmi on error path
>> - dw_hdmi_i2c_irq() does not modify hdmi->i2c->stat, if interrupt is not for 
>> I2CM
>> - spin lock is removed from dw_hdmi_i2c_irq()
>> - removed spin lock from dw_hdmi_i2c_xfer() around write to 
>> HDMI_IH_MUTE_I2CM_STAT0 register
>> - split loop over message array in dw_hdmi_i2c_xfer() to validation and 
>> transfer parts
>> - added a mutex to serialize I2C transfer requests, i2c->lock is completely 
>> removed
>> - removed if(len) check from dw_hdmi_i2c_write(), hardware supports only 
>> len>0 transfers
>> - described extension blocks <= 1 limitation in the commit message
>> - a number of minor clean ups
>>
>> Changes from v1 to v2:
>> - fixed a devm_kfree() signature
>> - split completions for read and write operations
>>
>>  drivers/gpu/drm/bridge/dw_hdmi.c | 262 
>> ++-
>>  drivers/gpu/drm/bridge/dw_hdmi.h |  19 +++
>>  2 files changed, 275 insertions(+), 6 deletions(-)
> 
> Not sure what the status of this patch is, but I'll make a few more
> suggestions in case they're helpful.

sure, all review comments are welcome.

I have in mind that Philipp asked to add an update to documentation, the
next version will contain the fixes.

> 
>> +static void dw_hdmi_i2c_init(struct dw_hdmi *hdmi)
>> +{
>> +   /* Set Fast Mode speed */
>> +   hdmi_writeb(hdmi, 0x0b, HDMI_I2CM_DIV);
> 
> If you're going to hardcode to something, it seems like hardcoding to
> standard mode might be better?  It's likely that users will plug into
> all kinds of broken / crappy HDMI devices out there.  I think you'll
> get better compatibility by using the slower mode, and EDID transfers
> really aren't too performance critical are they?
> 

Accepted. I don't know what are the exact divisors of master clock for
fast and standard modes, for reference I'll make a simple performance
test and publish its results. I expect that in standard mode SCL is
about 100KHz and in fast mode SCL is about 400KHz, and, if in standard
mode SCL is less than 100KHz, it will take more than 50ms to read out
512 bytes of data, which might be not acceptable from performance
perspective.

>> +
>> +   /* Software reset */
>> +   hdmi_writeb(hdmi, 0x00, HDMI_I2CM_SOFTRSTZ);
> 
> Unless there's a reason not to, it seems like the reset ought to be
> the first thing in this function (before setting the I2CM_DIV).  Even
> if it doesn't actually reset the speed on current hardware, it just
> seems cleaner.

It makes sense for me.

> 
>> +static int dw_hdmi_i2c_xfer(struct i2c_adapter *adap,
>> +   struct i2c_msg *msgs, int num)
>> +{
>> +   struct dw_hdmi *hdmi = i2c_get_adapdata(adap);
>> +   struct dw_hdmi_i2c *i2c = hdmi->i2c;
>

[PATCH v3 2/2] drm: bridge/dw_hdmi: add dw hdmi i2c bus adapter support

[Vladimir Zapolskiy]

The change adds support of internal HDMI I2C master controller, this
subdevice is used by default, if "ddc-i2c-bus" DT property is omitted.

The main purpose of this functionality is to support reading EDID from
an HDMI monitor on boards, which don't have an I2C bus connected to
DDC pins.

The current implementation does not support "I2C Master Interface
Extended Read Mode" to read data addressed by non-zero segment
pointer, this means that if EDID has more than 1 extension blocks,
EDID reading operation won't succeed, in my practice all tested HDMI
monitors have at maximum one extension block.

Signed-off-by: Vladimir Zapolskiy 
---
The change is based on today's v4.2, please let me know, if it should be 
rebased.

The change has compilation dependency on I2CM_ADDRESS register name fix,
see http://lists.freedesktop.org/archives/dri-devel/2015-May/082980.html

>From http://lists.freedesktop.org/archives/dri-devel/2015-May/082981.html
v1 of the change was

  Tested-by: Philipp Zabel 

but I hesitate to add the tag here due to multiple updates in v2.

Changes from v2 to v3, thanks to Russell:
- moved register field value definitions to dw_hdmi.h
- made completions uninterruptible to avoid transfer retries if interrupted
- use one completion for both read and write transfers as in v1, operation_reg 
is removed
- redundant i2c->stat = 0 is removed from dw_hdmi_i2c_read/write()
- struct i2c_algorithm dw_hdmi_algorithm is qualified as const
- dw_hdmi_i2c_adapter() does not modify struct dw_hdmi on error path
- dw_hdmi_i2c_irq() does not modify hdmi->i2c->stat, if interrupt is not for 
I2CM
- spin lock is removed from dw_hdmi_i2c_irq()
- removed spin lock from dw_hdmi_i2c_xfer() around write to 
HDMI_IH_MUTE_I2CM_STAT0 register
- split loop over message array in dw_hdmi_i2c_xfer() to validation and 
transfer parts
- added a mutex to serialize I2C transfer requests, i2c->lock is completely 
removed
- removed if(len) check from dw_hdmi_i2c_write(), hardware supports only len>0 
transfers
- described extension blocks <= 1 limitation in the commit message
- a number of minor clean ups

Changes from v1 to v2:
- fixed a devm_kfree() signature
- split completions for read and write operations

 drivers/gpu/drm/bridge/dw_hdmi.c | 262 ++-
 drivers/gpu/drm/bridge/dw_hdmi.h |  19 +++
 2 files changed, 275 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c
index 816d104..d329e04 100644
--- a/drivers/gpu/drm/bridge/dw_hdmi.c
+++ b/drivers/gpu/drm/bridge/dw_hdmi.c
@@ -1,14 +1,15 @@
 /*
+ * DesignWare High-Definition Multimedia Interface (HDMI) driver
+ *
+ * Copyright (C) 2013-2015 Mentor Graphics Inc.
  * Copyright (C) 2011-2013 Freescale Semiconductor, Inc.
+ * Copyright (C) 2010, Guennadi Liakhovetski 
  *
  * 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.
  *
- * Designware High-Definition Multimedia Interface (HDMI) driver
- *
- * Copyright (C) 2010, Guennadi Liakhovetski 
  */
 #include 
 #include 
@@ -102,6 +103,17 @@ struct hdmi_data_info {
struct hdmi_vmode video_mode;
 };

+struct dw_hdmi_i2c {
+   struct i2c_adapter  adap;
+
+   struct mutexlock;
+   struct completion   cmp;
+   u8  stat;
+
+   u8  slave_reg;
+   boolis_regaddr;
+};
+
 struct dw_hdmi {
struct drm_connector connector;
struct drm_encoder *encoder;
@@ -111,6 +123,7 @@ struct dw_hdmi {
struct device *dev;
struct clk *isfr_clk;
struct clk *iahb_clk;
+   struct dw_hdmi_i2c *i2c;

struct hdmi_data_info hdmi_data;
const struct dw_hdmi_plat_data *plat_data;
@@ -179,6 +192,200 @@ static void hdmi_mask_writeb(struct dw_hdmi *hdmi, u8 
data, unsigned int reg,
hdmi_modb(hdmi, data << shift, mask, reg);
 }

+static void dw_hdmi_i2c_init(struct dw_hdmi *hdmi)
+{
+   /* Set Fast Mode speed */
+   hdmi_writeb(hdmi, 0x0b, HDMI_I2CM_DIV);
+
+   /* Software reset */
+   hdmi_writeb(hdmi, 0x00, HDMI_I2CM_SOFTRSTZ);
+
+   /* Set done, not acknowledged and arbitration interrupt polarities */
+   hdmi_writeb(hdmi, HDMI_I2CM_INT_DONE_POL, HDMI_I2CM_INT);
+   hdmi_writeb(hdmi, HDMI_I2CM_CTLINT_NAC_POL | HDMI_I2CM_CTLINT_ARB_POL,
+   HDMI_I2CM_CTLINT);
+
+   /* Clear DONE and ERROR interrupts */
+   hdmi_writeb(hdmi, HDMI_IH_I2CM_STAT0_ERROR | HDMI_IH_I2CM_STAT0_DONE,
+   HDMI_IH_I2CM_STAT0);
+
+   /* Mute DONE and ERROR interrupts */
+   hdmi_writeb(hdmi, HDMI_IH_I2CM_STAT0_ERROR | HDMI_IH_I2CM_STAT0_DONE,
+   HDMI_IH_MUTE_I2CM_STAT0);
+}
+
+s

[PATCH v2 2/2] drm: bridge/dw_hdmi: add dw hdmi i2c bus adapter support

[Vladimir Zapolskiy]

Hi Russell,

On 14.08.2015 01:56, Russell King - ARM Linux wrote:
> On Mon, May 18, 2015 at 03:32:21PM +0300, Vladimir Zapolskiy wrote:
>> +/* HDMI_IH_I2CM_STAT0 and HDMI_IH_MUTE_I2CM_STAT0 register bits */
>> +#define HDMI_IH_I2CM_STAT0_ERRORBIT(0)
>> +#define HDMI_IH_I2CM_STAT0_DONE BIT(1)
>> +
>> +/* HDMI_I2CM_OPERATION register bits */
>> +#define HDMI_I2CM_OPERATION_READBIT(0)
>> +#define HDMI_I2CM_OPERATION_READ_EXTBIT(1)
>> +#define HDMI_I2CM_OPERATION_WRITE   BIT(4)
>> +
>> +/* HDMI_I2CM_INT register bits */
>> +#define HDMI_I2CM_INT_DONE_MASK BIT(2)
>> +#define HDMI_I2CM_INT_DONE_POL  BIT(3)
>> +
>> +/* HDMI_I2CM_CTLINT register bits */
>> +#define HDMI_I2CM_CTLINT_ARB_MASK   BIT(2)
>> +#define HDMI_I2CM_CTLINT_ARB_POLBIT(3)
>> +#define HDMI_I2CM_CTLINT_NAC_MASK   BIT(6)
>> +#define HDMI_I2CM_CTLINT_NAC_POLBIT(7)
> 
> Please put these definitions in dw_hdmi.h
> 

okay.

>> +
>> +
>>  #define HDMI_EDID_LEN   512
>>  
>>  #define RGB 0
>> @@ -102,6 +124,19 @@ struct hdmi_data_info {
>>  struct hdmi_vmode video_mode;
>>  };
>>  
>> +struct dw_hdmi_i2c {
>> +struct i2c_adapter  adap;
>> +
>> +spinlock_t  lock;
>> +struct completion   cmp_r;
>> +struct completion   cmp_w;
>> +u8  stat;
>> +
>> +u8  operation_reg;
>> +u8  slave_reg;
>> +boolis_regaddr;
>> +};
>> +
>>  struct dw_hdmi {
>>  struct drm_connector connector;
>>  struct drm_encoder *encoder;
>> @@ -111,6 +146,7 @@ struct dw_hdmi {
>>  struct device *dev;
>>  struct clk *isfr_clk;
>>  struct clk *iahb_clk;
>> +struct dw_hdmi_i2c *i2c;
>>  
>>  struct hdmi_data_info hdmi_data;
>>  const struct dw_hdmi_plat_data *plat_data;
>> @@ -179,6 +215,249 @@ static void hdmi_mask_writeb(struct dw_hdmi *hdmi, u8 
>> data, unsigned int reg,
>>  hdmi_modb(hdmi, data << shift, mask, reg);
>>  }
>>  
>> +static void dw_hdmi_i2c_init(struct dw_hdmi *hdmi)
>> +{
>> +unsigned long flags;
>> +
>> +spin_lock_irqsave(>i2c->lock, flags);
>> +
>> +/* Set Fast Mode speed */
>> +hdmi_writeb(hdmi, 0x0b, HDMI_I2CM_DIV);
>> +
>> +/* Software reset */
>> +hdmi_writeb(hdmi, 0x00, HDMI_I2CM_SOFTRSTZ);
>> +
>> +/* Set done, not acknowledged and arbitration interrupt polarities */
>> +hdmi_writeb(hdmi, HDMI_I2CM_INT_DONE_POL, HDMI_I2CM_INT);
>> +hdmi_writeb(hdmi, HDMI_I2CM_CTLINT_NAC_POL | HDMI_I2CM_CTLINT_ARB_POL,
>> +HDMI_I2CM_CTLINT);
>> +
>> +/* Clear DONE and ERROR interrupts */
>> +hdmi_writeb(hdmi, HDMI_IH_I2CM_STAT0_ERROR | HDMI_IH_I2CM_STAT0_DONE,
>> +HDMI_IH_I2CM_STAT0);
>> +
>> +/* Mute DONE and ERROR interrupts */
>> +hdmi_writeb(hdmi, HDMI_IH_I2CM_STAT0_ERROR | HDMI_IH_I2CM_STAT0_DONE,
>> +HDMI_IH_MUTE_I2CM_STAT0);
>> +
>> +spin_unlock_irqrestore(>i2c->lock, flags);
> 
> What exactly is this spinlock protecting against with the above code?
> 

On second inspection I don't see a need of locking here.

>> +}
>> +
>> +static int dw_hdmi_i2c_read(struct dw_hdmi *hdmi,
>> +unsigned char *buf, int length)
>> +{
>> +int stat;
>> +unsigned long flags;
>> +struct dw_hdmi_i2c *i2c = hdmi->i2c;
>> +
>> +spin_lock_irqsave(>lock, flags);
>> +
>> +i2c->operation_reg = HDMI_I2CM_OPERATION_READ;
>> +
>> +if (!i2c->is_regaddr) {
>> +dev_dbg(hdmi->dev, "set read register address to 0\n");
>> +i2c->slave_reg = 0x00;
>> +i2c->is_regaddr = true;
>> +}
>> +
>> +while (length--) {
>> +reinit_completion(>cmp_r);
> 
> If you're re-initialising the completion on every byte, why do you need
> separate completions for the read path and the write path?
> 
> If a single completion can be used, you then don't have to store the
> operation register value in struct dw_hdmi_i2c either.

Correct, I had only one completion and no i2c->operation_reg in v1, will
revert the added complexity to match the previous version
http://lists.freedesktop.org/

[PATCH 1/2] drm: bridge/dw_hdmi: fix register I2CM_ADDRESS register name

[Vladimir Zapolskiy]

Hello Russell, David,

On 24.07.2015 18:09, Vladimir Zapolskiy wrote:
> Hello Russell, David,
> 
> On 26.06.2015 18:02, Russell King - ARM Linux wrote:
>> On Fri, Jun 26, 2015 at 05:24:12PM +0300, Vladimir Zapolskiy wrote:
>>> Hello David,
>>>
>>> On 08.06.2015 17:17, Vladimir Zapolskiy wrote:
>>>> what would be the next action regarding these two patches? If review is
>>>> done, should they go to drm-dwhdmi-devel or drm-next ?
>>>
>>> ping.
>>
>> I don't think it impacts any builds at the moment, so we'll see about
>> merging it after the current merge window has finished.  Please remind
>> us after about a week and a half if we haven't already picked it up
>> by then.
>>
> 
> this is a reminder, please review the patches.

ping.

--
With best wishes,
Vladimir

[PATCH 1/2] drm: bridge/dw_hdmi: fix register I2CM_ADDRESS register name

[Vladimir Zapolskiy]

Hello Russell, David,

On 26.06.2015 18:02, Russell King - ARM Linux wrote:
> On Fri, Jun 26, 2015 at 05:24:12PM +0300, Vladimir Zapolskiy wrote:
>> Hello David,
>>
>> On 08.06.2015 17:17, Vladimir Zapolskiy wrote:
>>> what would be the next action regarding these two patches? If review is
>>> done, should they go to drm-dwhdmi-devel or drm-next ?
>>
>> ping.
> 
> I don't think it impacts any builds at the moment, so we'll see about
> merging it after the current merge window has finished.  Please remind
> us after about a week and a half if we haven't already picked it up
> by then.
> 

this is a reminder, please review the patches.

--
With best wishes,
Vladimir

[PATCH 01/10] i2c: add and export of_get_i2c_adapter_by_node() interface

[Vladimir Zapolskiy]

Hi Thierry,

On 08.07.2015 16:11, Thierry Reding wrote:
> On Wed, Jul 08, 2015 at 03:59:12PM +0300, Vladimir Zapolskiy wrote:
>> of_find_i2c_adapter_by_node() call requires quite often missing
>> put_device(), and i2c_put_adapter() releases a device locked by
>> i2c_get_adapter() only. In general module_put(adapter->owner) and
>> put_device(dev) are not interchangeable.
>>
>> This is a common error reproduction scenario as a result of the
>> misusage described above (for clearness this is run on iMX6 platform
>> with HDMI and I2C bus drivers compiled as kernel modules):
>>
>> root at mx6q:~# lsmod | grep i2c
>> i2c_imx10213  0
>> root at mx6q:~# lsmod | grep dw_hdmi_imx
>> dw_hdmi_imx 3631  0
>> dw_hdmi11846  1 dw_hdmi_imx
>> imxdrm  8674  3 dw_hdmi_imx,imx_ipuv3_crtc,imx_ldb
>> drm_kms_helper113765  5 dw_hdmi,imxdrm,imx_ipuv3_crtc,imx_ldb
>> root at mx6q:~# rmmod dw_hdmi_imx
>> root at mx6q:~# lsmod | grep i2c
>> i2c_imx10213  -1
>>
>>  ^
>>
>> root at mx6q:~# rmmod i2c_imx
>> rmmod: ERROR: Module i2c_imx is in use
>>
>> To fix existing users of these interfaces and to avoid any further
>> confusion and misusage in future, add one more interface
>> of_get_i2c_adapter_by_node(), it is similar to i2c_get_adapter() in
>> sense that an I2C bus device driver found and locked by user can be
>> correctly unlocked by i2c_put_adapter().
>>
>> Signed-off-by: Vladimir Zapolskiy 
>> ---
>> The change is based on RFC 
>> http://www.spinics.net/lists/linux-i2c/msg20257.html
>>
>> * added new exported function declaration in include/linux/i2c.h
>> * added put_device(dev) call right inside of_get_i2c_adapter_by_node()
>> * corrected authorship of the change
>>
>>  drivers/i2c/i2c-core.c | 20 
>>  include/linux/i2c.h|  6 ++
>>  2 files changed, 26 insertions(+)
>>
>> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
>> index 069a41f..0d902ab 100644
>> --- a/drivers/i2c/i2c-core.c
>> +++ b/drivers/i2c/i2c-core.c
>> @@ -1356,6 +1356,26 @@ struct i2c_adapter 
>> *of_find_i2c_adapter_by_node(struct device_node *node)
>>  return i2c_verify_adapter(dev);
>>  }
>>  EXPORT_SYMBOL(of_find_i2c_adapter_by_node);
>> +
>> +struct i2c_adapter *of_get_i2c_adapter_by_node(struct device_node *node)
>> +{
>> +struct device *dev;
>> +struct i2c_adapter *adapter;
>> +
>> +dev = bus_find_device(_bus_type, NULL, node,
>> +  of_dev_node_match);
>> +if (!dev)
>> +return NULL;
>> +
>> +adapter = i2c_verify_adapter(dev);
>> +if (adapter && !try_module_get(adapter->owner))
>> +adapter = NULL;
>> +
>> +put_device(dev);
> 
> I don't think this is correct. Users still need to keep a reference to
> the device, otherwise it can simply disappear even if the module stays
> around (think sysfs bind/unbind attributes).
> 
> Looking at i2c_put_adapter() it seems like it would need to do more than
> just drop the module reference. Then again, that probably means that we
> need to add a get_device() somewhere in i2c_get_adapter() to balance the
> put_device() in i2c_put_adapter().

it makes sense for me, thanks for momentary review.

I'm hesitating to add put_device(dev) to i2c_put_adapter() etc. in this
series though. After development and testing I would like to send
another preceding independent change updating i2c_get_adapter(),
i2c_put_adapter() and clients (or if you wish you can do it), then I'll
rebase 01/10 on top of it, the rest most probably is unchanged.

--
With best wishes,
Vladimir

[PATCH 09/10] fbdev: omap2: connector-dvi: use of_get_i2c_adapter_by_node interface

[Vladimir Zapolskiy]

This change is needed to properly lock I2C bus driver, which serves DDC.

Prior to this change i2c_put_adapter() is misused, which may lead to
an overflow over zero of I2C bus driver user counter.

Signed-off-by: Vladimir Zapolskiy 
---
 drivers/video/fbdev/omap2/displays-new/connector-dvi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/omap2/displays-new/connector-dvi.c 
b/drivers/video/fbdev/omap2/displays-new/connector-dvi.c
index a8ce920..d811e6d 100644
--- a/drivers/video/fbdev/omap2/displays-new/connector-dvi.c
+++ b/drivers/video/fbdev/omap2/displays-new/connector-dvi.c
@@ -294,7 +294,7 @@ static int dvic_probe_of(struct platform_device *pdev)

adapter_node = of_parse_phandle(node, "ddc-i2c-bus", 0);
if (adapter_node) {
-   adapter = of_find_i2c_adapter_by_node(adapter_node);
+   adapter = of_get_i2c_adapter_by_node(adapter_node);
if (adapter == NULL) {
dev_err(>dev, "failed to parse ddc-i2c-bus\n");
omap_dss_put_device(ddata->in);
-- 
2.1.4

[PATCH 08/10] drm: tilcdc: use of_get_i2c_adapter_by_node interface

[Vladimir Zapolskiy]

This change is needed to properly lock I2C bus driver, which serves DDC.

Prior to this change i2c_put_adapter() is misused, which may lead
to an overflow over zero of I2C bus driver user counter.

Signed-off-by: Vladimir Zapolskiy 
---
 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c 
b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
index 354c47c..4dc78c7 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_tfp410.c
@@ -348,15 +348,13 @@ static int tfp410_probe(struct platform_device *pdev)
goto fail;
}

-   tfp410_mod->i2c = of_find_i2c_adapter_by_node(i2c_node);
+   tfp410_mod->i2c = of_get_i2c_adapter_by_node(i2c_node);
+   of_node_put(i2c_node);
if (!tfp410_mod->i2c) {
dev_err(>dev, "could not get i2c\n");
-   of_node_put(i2c_node);
goto fail;
}

-   of_node_put(i2c_node);
-
tfp410_mod->gpio = of_get_named_gpio_flags(node, "powerdn-gpio",
0, NULL);
if (IS_ERR_VALUE(tfp410_mod->gpio)) {
-- 
2.1.4

[PATCH 07/10] drm: tegra: use of_get_i2c_adapter_by_node interface

[Vladimir Zapolskiy]

This change is needed to properly lock I2C bus driver, which serves DDC.

On release of_get_i2c_adapter_by_node() requires i2c_put_adapter() call.

Note, that prior to the change put_device() coupled with
of_find_i2c_adapter_by_node() was missing on error path of
tegra_output_probe().

Signed-off-by: Vladimir Zapolskiy 
---
 drivers/gpu/drm/tegra/output.c | 19 +++
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c
index 37db479..d7731cd 100644
--- a/drivers/gpu/drm/tegra/output.c
+++ b/drivers/gpu/drm/tegra/output.c
@@ -116,7 +116,7 @@ int tegra_output_probe(struct tegra_output *output)

ddc = of_parse_phandle(output->of_node, "nvidia,ddc-i2c-bus", 0);
if (ddc) {
-   output->ddc = of_find_i2c_adapter_by_node(ddc);
+   output->ddc = of_get_i2c_adapter_by_node(ddc);
if (!output->ddc) {
err = -EPROBE_DEFER;
of_node_put(ddc);
@@ -136,14 +136,13 @@ int tegra_output_probe(struct tegra_output *output)
   "HDMI hotplug detect");
if (err < 0) {
dev_err(output->dev, "gpio_request_one(): %d\n", err);
-   return err;
+   goto i2c_release;
}

err = gpio_to_irq(output->hpd_gpio);
if (err < 0) {
dev_err(output->dev, "gpio_to_irq(): %d\n", err);
-   gpio_free(output->hpd_gpio);
-   return err;
+   goto gpio_release;
}

output->hpd_irq = err;
@@ -156,8 +155,7 @@ int tegra_output_probe(struct tegra_output *output)
if (err < 0) {
dev_err(output->dev, "failed to request IRQ#%u: %d\n",
output->hpd_irq, err);
-   gpio_free(output->hpd_gpio);
-   return err;
+   goto gpio_release;
}

output->connector.polled = DRM_CONNECTOR_POLL_HPD;
@@ -171,6 +169,12 @@ int tegra_output_probe(struct tegra_output *output)
}

return 0;
+
+ gpio_release:
+   gpio_free(output->hpd_gpio);
+ i2c_release:
+   i2c_put_adapter(output->ddc);
+   return err;
 }

 void tegra_output_remove(struct tegra_output *output)
@@ -180,8 +184,7 @@ void tegra_output_remove(struct tegra_output *output)
gpio_free(output->hpd_gpio);
}

-   if (output->ddc)
-   put_device(>ddc->dev);
+   i2c_put_adapter(output->ddc);
 }

 int tegra_output_init(struct drm_device *drm, struct tegra_output *output)
-- 
2.1.4

[PATCH 06/10] drm: sti_hdmi: use of_get_i2c_adapter_by_node interface

[Vladimir Zapolskiy]

This change is needed to properly lock I2C bus driver, which serves DDC.

On release of_get_i2c_adapter_by_node() requires i2c_put_adapter() call.

Note, that prior to the change put_device() coupled with
of_find_i2c_adapter_by_node() was incorrectly placed to sti_hdmi_remove()
instead of sti_hdmi_unbind().

Signed-off-by: Vladimir Zapolskiy 
---
 drivers/gpu/drm/sti/sti_hdmi.c | 19 +++
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/sti/sti_hdmi.c b/drivers/gpu/drm/sti/sti_hdmi.c
index f28a4d5..580a413 100644
--- a/drivers/gpu/drm/sti/sti_hdmi.c
+++ b/drivers/gpu/drm/sti/sti_hdmi.c
@@ -698,14 +698,10 @@ static int sti_hdmi_bind(struct device *dev, struct 
device *master, void *data)

ddc = of_parse_phandle(dev->of_node, "ddc", 0);
if (ddc) {
-   hdmi->ddc_adapt = of_find_i2c_adapter_by_node(ddc);
-   if (!hdmi->ddc_adapt) {
-   err = -EPROBE_DEFER;
-   of_node_put(ddc);
-   return err;
-   }
-
+   hdmi->ddc_adapt = of_get_i2c_adapter_by_node(ddc);
of_node_put(ddc);
+   if (!hdmi->ddc_adapt)
+   return -EPROBE_DEFER;
}

/* Set the drm device handle */
@@ -762,14 +758,15 @@ err_sysfs:
 err_connector:
drm_connector_cleanup(drm_connector);
 err_adapt:
-   put_device(>ddc_adapt->dev);
+   i2c_put_adapter(hdmi->ddc_adapt);
+
return -EINVAL;
 }

 static void sti_hdmi_unbind(struct device *dev,
struct device *master, void *data)
 {
-   /* do nothing */
+   i2c_put_adapter(hdmi->ddc_adapt);
 }

 static const struct component_ops sti_hdmi_ops = {
@@ -885,10 +882,8 @@ static int sti_hdmi_remove(struct platform_device *pdev)
 {
struct sti_hdmi *hdmi = dev_get_drvdata(>dev);

-   if (hdmi->ddc_adapt)
-   put_device(>ddc_adapt->dev);
-
component_del(>dev, _hdmi_ops);
+
return 0;
 }

-- 
2.1.4

[PATCH 05/10] drm: panel-simple: use of_get_i2c_adapter_by_node interface

[Vladimir Zapolskiy]

This change is needed to properly lock I2C bus driver, which serves DDC.

On release of_get_i2c_adapter_by_node() requires i2c_put_adapter() call,
which replaces put_device().

Signed-off-by: Vladimir Zapolskiy 
---
 drivers/gpu/drm/panel/panel-simple.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-simple.c 
b/drivers/gpu/drm/panel/panel-simple.c
index f94201b..b8a8b23 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -313,7 +313,7 @@ static int panel_simple_probe(struct device *dev, const 
struct panel_desc *desc)

ddc = of_parse_phandle(dev->of_node, "ddc-i2c-bus", 0);
if (ddc) {
-   panel->ddc = of_find_i2c_adapter_by_node(ddc);
+   panel->ddc = of_get_i2c_adapter_by_node(ddc);
of_node_put(ddc);

if (!panel->ddc) {
@@ -335,8 +335,8 @@ static int panel_simple_probe(struct device *dev, const 
struct panel_desc *desc)
return 0;

 free_ddc:
-   if (panel->ddc)
-   put_device(>ddc->dev);
+   i2c_put_adapter(panel->ddc);
+
 free_backlight:
if (panel->backlight)
put_device(>backlight->dev);
@@ -353,8 +353,7 @@ static int panel_simple_remove(struct device *dev)

panel_simple_disable(>base);

-   if (panel->ddc)
-   put_device(>ddc->dev);
+   i2c_put_adapter(panel->ddc);

if (panel->backlight)
put_device(>backlight->dev);
-- 
2.1.4

[PATCH 04/10] drm: imx-tve: use of_get_i2c_adapter_by_node interface

[Vladimir Zapolskiy]

This change is needed to properly lock I2C bus driver, which serves DDC.

Note that prior to this change put_device() coupled with
of_find_i2c_adapter_by_node() is missing on imx_tve_bind() error path
and imx_tve_unbind(), also the change fixes possibly left enabled voltage
regulator on imx_tve_bind() error path.

Signed-off-by: Vladimir Zapolskiy 
---
 drivers/gpu/drm/imx/imx-tve.c | 56 +--
 1 file changed, 38 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
index 214ecee..f1ac927 100644
--- a/drivers/gpu/drm/imx/imx-tve.c
+++ b/drivers/gpu/drm/imx/imx-tve.c
@@ -581,14 +581,15 @@ static int imx_tve_bind(struct device *dev, struct device 
*master, void *data)

ddc_node = of_parse_phandle(np, "ddc-i2c-bus", 0);
if (ddc_node) {
-   tve->ddc = of_find_i2c_adapter_by_node(ddc_node);
+   tve->ddc = of_get_i2c_adapter_by_node(ddc_node);
of_node_put(ddc_node);
}

tve->mode = of_get_tve_mode(np);
if (tve->mode != TVE_MODE_VGA) {
dev_err(dev, "only VGA mode supported, currently\n");
-   return -EINVAL;
+   ret = -EINVAL;
+   goto i2c_release;
}

if (tve->mode == TVE_MODE_VGA) {
@@ -597,7 +598,7 @@ static int imx_tve_bind(struct device *dev, struct device 
*master, void *data)

if (ret < 0) {
dev_err(dev, "failed to get vsync pin\n");
-   return ret;
+   goto i2c_release;
}

ret |= of_property_read_u32(np, "fsl,vsync-pin",
@@ -605,14 +606,16 @@ static int imx_tve_bind(struct device *dev, struct device 
*master, void *data)

if (ret < 0) {
dev_err(dev, "failed to get vsync pin\n");
-   return ret;
+   goto i2c_release;
}
}

res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
base = devm_ioremap_resource(dev, res);
-   if (IS_ERR(base))
-   return PTR_ERR(base);
+   if (IS_ERR(base)) {
+   ret = PTR_ERR(base);
+   goto i2c_release;
+   }

tve_regmap_config.lock_arg = tve;
tve->regmap = devm_regmap_init_mmio_clk(dev, "tve", base,
@@ -620,13 +623,15 @@ static int imx_tve_bind(struct device *dev, struct device 
*master, void *data)
if (IS_ERR(tve->regmap)) {
dev_err(dev, "failed to init regmap: %ld\n",
PTR_ERR(tve->regmap));
-   return PTR_ERR(tve->regmap);
+   ret = PTR_ERR(tve->regmap);
+   goto i2c_release;
}

irq = platform_get_irq(pdev, 0);
if (irq < 0) {
dev_err(dev, "failed to get irq\n");
-   return irq;
+   ret = irq;
+   goto i2c_release;
}

ret = devm_request_threaded_irq(dev, irq, NULL,
@@ -634,7 +639,7 @@ static int imx_tve_bind(struct device *dev, struct device 
*master, void *data)
"imx-tve", tve);
if (ret < 0) {
dev_err(dev, "failed to request irq: %d\n", ret);
-   return ret;
+   goto i2c_release;
}

tve->dac_reg = devm_regulator_get(dev, "dac");
@@ -642,14 +647,15 @@ static int imx_tve_bind(struct device *dev, struct device 
*master, void *data)
regulator_set_voltage(tve->dac_reg, 275, 275);
ret = regulator_enable(tve->dac_reg);
if (ret)
-   return ret;
+   goto i2c_release;
}

tve->clk = devm_clk_get(dev, "tve");
if (IS_ERR(tve->clk)) {
dev_err(dev, "failed to get high speed tve clock: %ld\n",
PTR_ERR(tve->clk));
-   return PTR_ERR(tve->clk);
+   ret = PTR_ERR(tve->clk);
+   goto regulator_release;
}

/* this is the IPU DI clock input selector, can be parented to tve_di */
@@ -657,36 +663,48 @@ static int imx_tve_bind(struct device *dev, struct device 
*master, void *data)
if (IS_ERR(tve->di_sel_clk)) {
dev_err(dev, "failed to get ipu di mux clock: %ld\n",
PTR_ERR(tve->di_sel_clk));
-   return PTR_ERR(tve->di_sel_clk);
+   ret = PTR_ERR(tve->di_sel_clk);
+   goto regulator_release;
}

ret = tve_clk_init(tve, base);
if (ret < 0)
-   return ret;
+   goto regulator_release;

ret = regmap_read(tve->regmap, TVE_COM_CONF_REG, )

[PATCH 03/10] drm: exynos_hdmi: use of_get_i2c_adapter_by_node interface

[Vladimir Zapolskiy]

This change is needed to properly lock I2C bus driver, which serves DDC.

On release of_get_i2c_adapter_by_node() requires i2c_put_adapter() call,
which replaces put_device(). By the way added of_node_put(ddc_node) to
eliminate memory leak, if OF_DYNAMIC is enabled.

Signed-off-by: Vladimir Zapolskiy 
---
 drivers/gpu/drm/exynos/exynos_hdmi.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c 
b/drivers/gpu/drm/exynos/exynos_hdmi.c
index 99e2864..399eff9 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -2407,7 +2407,8 @@ static int hdmi_probe(struct platform_device *pdev)
}

 out_get_ddc_adpt:
-   hdata->ddc_adpt = of_find_i2c_adapter_by_node(ddc_node);
+   hdata->ddc_adpt = of_get_i2c_adapter_by_node(ddc_node);
+   of_node_put(ddc_node);
if (!hdata->ddc_adpt) {
DRM_ERROR("Failed to get ddc i2c adapter by node\n");
return -EPROBE_DEFER;
@@ -2485,7 +2486,7 @@ err_hdmiphy:
if (hdata->hdmiphy_port)
put_device(>hdmiphy_port->dev);
 err_ddc:
-   put_device(>ddc_adpt->dev);
+   i2c_put_adapter(hdata->ddc_adpt);

return ret;
 }
@@ -2501,7 +2502,7 @@ static int hdmi_remove(struct platform_device *pdev)

if (hdata->hdmiphy_port)
put_device(>hdmiphy_port->dev);
-   put_device(>ddc_adpt->dev);
+   i2c_put_adapter(hdata->ddc_adpt);

pm_runtime_disable(>dev);
component_del(>dev, _component_ops);
-- 
2.1.4

[PATCH 02/10] drm: dw_hdmi: use of_get_i2c_adapter_by_node interface

[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_imx10213  0
root at mx6q:~# lsmod | grep dw_hdmi_imx
dw_hdmi_imx 3631  0
dw_hdmi11846  1 dw_hdmi_imx
imxdrm  8674  3 dw_hdmi_imx,imx_ipuv3_crtc,imx_ldb
drm_kms_helper113765  5 dw_hdmi,imxdrm,imx_ipuv3_crtc,imx_ldb
root at mx6q:~# rmmod dw_hdmi_imx
root at mx6q:~# lsmod | grep i2c
i2c_imx10213  -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 
---
 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 816d104..e86776c 100644
--- a/drivers/gpu/drm/bridge/dw_hdmi.c
+++ b/drivers/gpu/drm/bridge/dw_hdmi.c
@@ -1591,7 +1591,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");
@@ -1603,20 +1603,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");
@@ -1682,6 +1684,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.1.4

[PATCH 01/10] i2c: add and export of_get_i2c_adapter_by_node() interface

[Vladimir Zapolskiy]

of_find_i2c_adapter_by_node() call requires quite often missing
put_device(), and i2c_put_adapter() releases a device locked by
i2c_get_adapter() only. In general module_put(adapter->owner) and
put_device(dev) are not interchangeable.

This is a common error reproduction scenario as a result of the
misusage described above (for clearness this is run on iMX6 platform
with HDMI and I2C bus drivers compiled as kernel modules):

root at mx6q:~# lsmod | grep i2c
i2c_imx10213  0
root at mx6q:~# lsmod | grep dw_hdmi_imx
dw_hdmi_imx 3631  0
dw_hdmi11846  1 dw_hdmi_imx
imxdrm  8674  3 dw_hdmi_imx,imx_ipuv3_crtc,imx_ldb
drm_kms_helper113765  5 dw_hdmi,imxdrm,imx_ipuv3_crtc,imx_ldb
root at mx6q:~# rmmod dw_hdmi_imx
root at mx6q:~# lsmod | grep i2c
i2c_imx10213  -1

 ^

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

To fix existing users of these interfaces and to avoid any further
confusion and misusage in future, add one more interface
of_get_i2c_adapter_by_node(), it is similar to i2c_get_adapter() in
sense that an I2C bus device driver found and locked by user can be
correctly unlocked by i2c_put_adapter().

Signed-off-by: Vladimir Zapolskiy 
---
The change is based on RFC http://www.spinics.net/lists/linux-i2c/msg20257.html

* added new exported function declaration in include/linux/i2c.h
* added put_device(dev) call right inside of_get_i2c_adapter_by_node()
* corrected authorship of the change

 drivers/i2c/i2c-core.c | 20 
 include/linux/i2c.h|  6 ++
 2 files changed, 26 insertions(+)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 069a41f..0d902ab 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1356,6 +1356,26 @@ struct i2c_adapter *of_find_i2c_adapter_by_node(struct 
device_node *node)
return i2c_verify_adapter(dev);
 }
 EXPORT_SYMBOL(of_find_i2c_adapter_by_node);
+
+struct i2c_adapter *of_get_i2c_adapter_by_node(struct device_node *node)
+{
+   struct device *dev;
+   struct i2c_adapter *adapter;
+
+   dev = bus_find_device(_bus_type, NULL, node,
+ of_dev_node_match);
+   if (!dev)
+   return NULL;
+
+   adapter = i2c_verify_adapter(dev);
+   if (adapter && !try_module_get(adapter->owner))
+   adapter = NULL;
+
+   put_device(dev);
+
+   return adapter;
+}
+EXPORT_SYMBOL(of_get_i2c_adapter_by_node);
 #else
 static void of_i2c_register_devices(struct i2c_adapter *adap) { }
 #endif /* CONFIG_OF */
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index e83a738..87bb217 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -638,6 +638,7 @@ extern struct i2c_client *of_find_i2c_device_by_node(struct 
device_node *node);
 /* must call put_device() when done with returned i2c_adapter device */
 extern struct i2c_adapter *of_find_i2c_adapter_by_node(struct device_node 
*node);

+struct i2c_adapter *of_get_i2c_adapter_by_node(struct device_node *node);
 #else

 static inline struct i2c_client *of_find_i2c_device_by_node(struct device_node 
*node)
@@ -649,6 +650,11 @@ static inline struct i2c_adapter 
*of_find_i2c_adapter_by_node(struct device_node
 {
return NULL;
 }
+
+static inline struct i2c_adapter *of_get_i2c_adapter_by_node(struct 
device_node *node)
+{
+   return NULL;
+}
 #endif /* CONFIG_OF */

 #endif /* _LINUX_I2C_H */
-- 
2.1.4

[PATCH 00/10] i2c/drm: fix i2c adapter device driver user counter

[Vladimir Zapolskiy]

The 01/10 change adds and exports new of_get_i2c_adapter_by_node()
interface of i2c core, the rest of patches fix current users of
of_find_i2c_adapter_by_node() interface.

of_find_i2c_adapter_by_node() call requires quite often missing
put_device(), and i2c_put_adapter() releases a device locked by
i2c_get_adapter() only. In general module_put(adapter->owner) and
put_device(dev) are not interchangeable.

This is a common error reproduction scenario as a result of the
misusage described above (this is run on iMX6 platform with
HDMI and I2C bus drivers compiled as kernel modules for clearness):

root at mx6q:~# lsmod | grep i2c
i2c_imx10213  0
root at mx6q:~# lsmod | grep dw_hdmi_imx
dw_hdmi_imx 3631  0
dw_hdmi11846  1 dw_hdmi_imx
imxdrm  8674  3 dw_hdmi_imx,imx_ipuv3_crtc,imx_ldb
drm_kms_helper113765  5 dw_hdmi,imxdrm,imx_ipuv3_crtc,imx_ldb
root at mx6q:~# rmmod dw_hdmi_imx
root at mx6q:~# lsmod | grep i2c
i2c_imx10213  -1

 ^

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

To fix existing users of these interfaces and to avoid any further
confusion and misusage in future, add one more interface
of_get_i2c_adapter_by_node(), it is similar to i2c_get_adapter() in
sense that an I2C bus device driver found and locked by user can be
correctly unlocked by i2c_put_adapter().

Mainly the change concerns DRM users of I2C bus device.

The change is based on torvalds/master branch, d6ac4ffc61a

RFC of the 01/10 change is http://www.spinics.net/lists/linux-i2c/msg20257.html

Vladimir Zapolskiy (10):
  i2c: add and export of_get_i2c_adapter_by_node() interface
  drm: dw_hdmi: use of_get_i2c_adapter_by_node interface
  drm: exynos_hdmi: use of_get_i2c_adapter_by_node interface
  drm: imx-tve: use of_get_i2c_adapter_by_node interface
  drm: panel-simple: use of_get_i2c_adapter_by_node interface
  drm: sti_hdmi: use of_get_i2c_adapter_by_node interface
  drm: tegra: use of_get_i2c_adapter_by_node interface
  drm: tilcdc: use of_get_i2c_adapter_by_node interface
  fbdev: omap2: connector-dvi: use of_get_i2c_adapter_by_node interface
  i2c: i2c-arb-gpio-challenge: use of_get_i2c_adapter_by_node interface

 drivers/gpu/drm/bridge/dw_hdmi.c   | 14 --
 drivers/gpu/drm/exynos/exynos_hdmi.c   |  7 +--
 drivers/gpu/drm/imx/imx-tve.c  | 56 +++---
 drivers/gpu/drm/panel/panel-simple.c   |  9 ++--
 drivers/gpu/drm/sti/sti_hdmi.c | 19 +++-
 drivers/gpu/drm/tegra/output.c | 19 
 drivers/gpu/drm/tilcdc/tilcdc_tfp410.c |  6 +--
 drivers/i2c/i2c-core.c | 20 
 drivers/i2c/muxes/i2c-arb-gpio-challenge.c |  3 +-
 .../video/fbdev/omap2/displays-new/connector-dvi.c |  2 +-
 include/linux/i2c.h|  6 +++
 11 files changed, 104 insertions(+), 57 deletions(-)

-- 
2.1.4

[PATCH 1/2] drm: bridge/dw_hdmi: fix register I2CM_ADDRESS register name

[Vladimir Zapolskiy]

Hello David,

On 08.06.2015 17:17, Vladimir Zapolskiy wrote:
> Hi David, Philipp, Andy, Russell,
> 
> On 19.05.2015 17:39, Andy Yan wrote:
>> Hi Vladimir,
>>Thanks for you patch.
>>
>> On 2015年05月18日 20:32, Vladimir Zapolskiy wrote:
>>> I2CM_ADDRESS became a MESS, fix it, also change guarding define
>>> to __DW_HDMI_H__ , since the driver is not IMX specific.
>>>
>>> Signed-off-by: Vladimir Zapolskiy 
>> Acked-by: Andy Yan 
>>> ---
>>>   drivers/gpu/drm/bridge/dw_hdmi.h | 8 
>>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/dw_hdmi.h 
>>> b/drivers/gpu/drm/bridge/dw_hdmi.h
>>> index 175dbc8..ee7f7ed 100644
>>> --- a/drivers/gpu/drm/bridge/dw_hdmi.h
>>> +++ b/drivers/gpu/drm/bridge/dw_hdmi.h
>>> @@ -7,8 +7,8 @@
>>>* (at your option) any later version.
>>>*/
>>>   
>>> -#ifndef __IMX_HDMI_H__
>>> -#define __IMX_HDMI_H__
>>> +#ifndef __DW_HDMI_H__
>>> +#define __DW_HDMI_H__
>>>   
>>>   /* Identification Registers */
>>>   #define HDMI_DESIGN_ID  0x
>>> @@ -525,7 +525,7 @@
>>>   
>>>   /* I2C Master Registers (E-DDC) */
>>>   #define HDMI_I2CM_SLAVE 0x7E00
>>> -#define HDMI_I2CMESS0x7E01
>>> +#define HDMI_I2CM_ADDRESS   0x7E01
>>>   #define HDMI_I2CM_DATAO 0x7E02
>>>   #define HDMI_I2CM_DATAI 0x7E03
>>>   #define HDMI_I2CM_OPERATION 0x7E04
>>> @@ -1031,4 +1031,4 @@ enum {
>>> HDMI_A_VIDPOLCFG_HSYNCPOL_ACTIVE_LOW = 0x0,
>>>   };
>>>   
>>> -#endif /* __IMX_HDMI_H__ */
>>> +#endif /* __DW_HDMI_H__ */
>>
>>
> 
> what would be the next action regarding these two patches? If review is
> done, should they go to drm-dwhdmi-devel or drm-next ?

ping.

--
With best wishes,
Vladimir

[PATCH 1/2] drm: bridge/dw_hdmi: fix register I2CM_ADDRESS register name

[Vladimir Zapolskiy]

Hi David, Philipp, Andy, Russell,

On 19.05.2015 17:39, Andy Yan wrote:
> Hi Vladimir,
>Thanks for you patch.
> 
> On 2015年05月18日 20:32, Vladimir Zapolskiy wrote:
>> I2CM_ADDRESS became a MESS, fix it, also change guarding define
>> to __DW_HDMI_H__ , since the driver is not IMX specific.
>>
>> Signed-off-by: Vladimir Zapolskiy 
> Acked-by: Andy Yan 
>> ---
>>   drivers/gpu/drm/bridge/dw_hdmi.h | 8 
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/dw_hdmi.h 
>> b/drivers/gpu/drm/bridge/dw_hdmi.h
>> index 175dbc8..ee7f7ed 100644
>> --- a/drivers/gpu/drm/bridge/dw_hdmi.h
>> +++ b/drivers/gpu/drm/bridge/dw_hdmi.h
>> @@ -7,8 +7,8 @@
>>* (at your option) any later version.
>>*/
>>   
>> -#ifndef __IMX_HDMI_H__
>> -#define __IMX_HDMI_H__
>> +#ifndef __DW_HDMI_H__
>> +#define __DW_HDMI_H__
>>   
>>   /* Identification Registers */
>>   #define HDMI_DESIGN_ID  0x
>> @@ -525,7 +525,7 @@
>>   
>>   /* I2C Master Registers (E-DDC) */
>>   #define HDMI_I2CM_SLAVE 0x7E00
>> -#define HDMI_I2CMESS0x7E01
>> +#define HDMI_I2CM_ADDRESS   0x7E01
>>   #define HDMI_I2CM_DATAO 0x7E02
>>   #define HDMI_I2CM_DATAI 0x7E03
>>   #define HDMI_I2CM_OPERATION 0x7E04
>> @@ -1031,4 +1031,4 @@ enum {
>>  HDMI_A_VIDPOLCFG_HSYNCPOL_ACTIVE_LOW = 0x0,
>>   };
>>   
>> -#endif /* __IMX_HDMI_H__ */
>> +#endif /* __DW_HDMI_H__ */
> 
> 

what would be the next action regarding these two patches? If review is
done, should they go to drm-dwhdmi-devel or drm-next ?

--
With best wishes,
Vladimir

[PATCH v2 2/2] drm: bridge/dw_hdmi: add dw hdmi i2c bus adapter support

[Vladimir Zapolskiy]

The change adds support of internal HDMI I2C master controller, this
subdevice is used by default, if "ddc-i2c-bus" DT property is omitted.

The main purpose of this functionality is to support reading EDID from
an HDMI monitor on boards, which don't have an I2C bus connected to
DDC pins.

Signed-off-by: Vladimir Zapolskiy 
---
Changes since v1:
- fixed a devm_kfree() signature
- split completions for read and write operations

 drivers/gpu/drm/bridge/dw_hdmi.c | 341 ++-
 1 file changed, 335 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c
index 49cafb6..7f64e73 100644
--- a/drivers/gpu/drm/bridge/dw_hdmi.c
+++ b/drivers/gpu/drm/bridge/dw_hdmi.c
@@ -1,15 +1,17 @@
 /*
+ * DesignWare High-Definition Multimedia Interface (HDMI) driver
+ *
+ * Copyright (C) 2013-2015 Mentor Graphics Inc.
  * Copyright (C) 2011-2013 Freescale Semiconductor, Inc.
+ * Copyright (C) 2010, Guennadi Liakhovetski 
  *
  * 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.
  *
- * Designware High-Definition Multimedia Interface (HDMI) driver
- *
- * Copyright (C) 2010, Guennadi Liakhovetski 
  */
+
 #include 
 #include 
 #include 
@@ -28,6 +30,26 @@

 #include "dw_hdmi.h"

+/* HDMI_IH_I2CM_STAT0 and HDMI_IH_MUTE_I2CM_STAT0 register bits */
+#define HDMI_IH_I2CM_STAT0_ERROR   BIT(0)
+#define HDMI_IH_I2CM_STAT0_DONEBIT(1)
+
+/* HDMI_I2CM_OPERATION register bits */
+#define HDMI_I2CM_OPERATION_READ   BIT(0)
+#define HDMI_I2CM_OPERATION_READ_EXT   BIT(1)
+#define HDMI_I2CM_OPERATION_WRITE  BIT(4)
+
+/* HDMI_I2CM_INT register bits */
+#define HDMI_I2CM_INT_DONE_MASKBIT(2)
+#define HDMI_I2CM_INT_DONE_POL BIT(3)
+
+/* HDMI_I2CM_CTLINT register bits */
+#define HDMI_I2CM_CTLINT_ARB_MASK  BIT(2)
+#define HDMI_I2CM_CTLINT_ARB_POL   BIT(3)
+#define HDMI_I2CM_CTLINT_NAC_MASK  BIT(6)
+#define HDMI_I2CM_CTLINT_NAC_POL   BIT(7)
+
+
 #define HDMI_EDID_LEN  512

 #define RGB0
@@ -102,6 +124,19 @@ struct hdmi_data_info {
struct hdmi_vmode video_mode;
 };

+struct dw_hdmi_i2c {
+   struct i2c_adapter  adap;
+
+   spinlock_t  lock;
+   struct completion   cmp_r;
+   struct completion   cmp_w;
+   u8  stat;
+
+   u8  operation_reg;
+   u8  slave_reg;
+   boolis_regaddr;
+};
+
 struct dw_hdmi {
struct drm_connector connector;
struct drm_encoder *encoder;
@@ -111,6 +146,7 @@ struct dw_hdmi {
struct device *dev;
struct clk *isfr_clk;
struct clk *iahb_clk;
+   struct dw_hdmi_i2c *i2c;

struct hdmi_data_info hdmi_data;
const struct dw_hdmi_plat_data *plat_data;
@@ -179,6 +215,249 @@ static void hdmi_mask_writeb(struct dw_hdmi *hdmi, u8 
data, unsigned int reg,
hdmi_modb(hdmi, data << shift, mask, reg);
 }

+static void dw_hdmi_i2c_init(struct dw_hdmi *hdmi)
+{
+   unsigned long flags;
+
+   spin_lock_irqsave(>i2c->lock, flags);
+
+   /* Set Fast Mode speed */
+   hdmi_writeb(hdmi, 0x0b, HDMI_I2CM_DIV);
+
+   /* Software reset */
+   hdmi_writeb(hdmi, 0x00, HDMI_I2CM_SOFTRSTZ);
+
+   /* Set done, not acknowledged and arbitration interrupt polarities */
+   hdmi_writeb(hdmi, HDMI_I2CM_INT_DONE_POL, HDMI_I2CM_INT);
+   hdmi_writeb(hdmi, HDMI_I2CM_CTLINT_NAC_POL | HDMI_I2CM_CTLINT_ARB_POL,
+   HDMI_I2CM_CTLINT);
+
+   /* Clear DONE and ERROR interrupts */
+   hdmi_writeb(hdmi, HDMI_IH_I2CM_STAT0_ERROR | HDMI_IH_I2CM_STAT0_DONE,
+   HDMI_IH_I2CM_STAT0);
+
+   /* Mute DONE and ERROR interrupts */
+   hdmi_writeb(hdmi, HDMI_IH_I2CM_STAT0_ERROR | HDMI_IH_I2CM_STAT0_DONE,
+   HDMI_IH_MUTE_I2CM_STAT0);
+
+   spin_unlock_irqrestore(>i2c->lock, flags);
+}
+
+static int dw_hdmi_i2c_read(struct dw_hdmi *hdmi,
+   unsigned char *buf, int length)
+{
+   int stat;
+   unsigned long flags;
+   struct dw_hdmi_i2c *i2c = hdmi->i2c;
+
+   spin_lock_irqsave(>lock, flags);
+
+   i2c->operation_reg = HDMI_I2CM_OPERATION_READ;
+
+   if (!i2c->is_regaddr) {
+   dev_dbg(hdmi->dev, "set read register address to 0\n");
+   i2c->slave_reg = 0x00;
+   i2c->is_regaddr = true;
+   }
+
+   while (length--) {
+   reinit_completion(>cmp_r);
+   i2c->stat = 0;
+
+   hdmi_writeb(hdmi, i2c->slave_r

[PATCH 1/2] drm: bridge/dw_hdmi: fix register I2CM_ADDRESS register name

[Vladimir Zapolskiy]

I2CM_ADDRESS became a MESS, fix it, also change guarding define
to __DW_HDMI_H__ , since the driver is not IMX specific.

Signed-off-by: Vladimir Zapolskiy 
---
 drivers/gpu/drm/bridge/dw_hdmi.h | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw_hdmi.h b/drivers/gpu/drm/bridge/dw_hdmi.h
index 175dbc8..ee7f7ed 100644
--- a/drivers/gpu/drm/bridge/dw_hdmi.h
+++ b/drivers/gpu/drm/bridge/dw_hdmi.h
@@ -7,8 +7,8 @@
  * (at your option) any later version.
  */

-#ifndef __IMX_HDMI_H__
-#define __IMX_HDMI_H__
+#ifndef __DW_HDMI_H__
+#define __DW_HDMI_H__

 /* Identification Registers */
 #define HDMI_DESIGN_ID  0x
@@ -525,7 +525,7 @@

 /* I2C Master Registers (E-DDC) */
 #define HDMI_I2CM_SLAVE 0x7E00
-#define HDMI_I2CMESS0x7E01
+#define HDMI_I2CM_ADDRESS   0x7E01
 #define HDMI_I2CM_DATAO 0x7E02
 #define HDMI_I2CM_DATAI 0x7E03
 #define HDMI_I2CM_OPERATION 0x7E04
@@ -1031,4 +1031,4 @@ enum {
HDMI_A_VIDPOLCFG_HSYNCPOL_ACTIVE_LOW = 0x0,
 };

-#endif /* __IMX_HDMI_H__ */
+#endif /* __DW_HDMI_H__ */
-- 
2.1.4

[PATCH v2 0/2] drm: bridge/dw_hdmi: add I2C bus adapter support

[Vladimir Zapolskiy]

This change adds support of internal HDMI I2C master controller,
originally the controller has its own separate driver written from
scratch http://patchwork.ozlabs.org/patch/405100 but due to shared
register space and interrupt with HDMI driver, it makes sense to
merge the code of both drivers.

The main purpose of this functionality is to support reading EDID from
an HDMI monitor on boards, which don't have an I2C bus connected to
DDC pins.

To use/test the change "ddc-i2c-bus" DT property must be omitted and
pin settings must be updated accordingly, here is an example for
iMX6 SabreLite:

---8<---
diff --git a/arch/arm/boot/dts/imx6qdl-sabrelite.dtsi 
b/arch/arm/boot/dts/imx6qdl-sabrelite.dtsi
index 0b28a9d..22d4431 100644
--- a/arch/arm/boot/dts/imx6qdl-sabrelite.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sabrelite.dtsi
@@ -174,7 +174,6 @@
 };

  {
-   ddc-i2c-bus = <>;
+   pinctrl-names = "default";
+   pinctrl-0 = <_hdmi>;
status = "okay";
 };

@@ -193,13 +192,6 @@
};
 };

- {
-   clock-frequency = <10>;
-   pinctrl-names = "default";
-   pinctrl-0 = <_i2c2>;
-   status = "okay";
-};
-
  {
clock-frequency = <10>;
pinctrl-names = "default";
@@ -284,10 +276,10 @@
>;
};

-   pinctrl_i2c2: i2c2grp {
+   pinctrl_hdmi: hdmigrp {
fsl,pins = <
-   MX6QDL_PAD_KEY_COL3__I2C2_SCL   
0x4001b8b1
-   MX6QDL_PAD_KEY_ROW3__I2C2_SDA   
0x4001b8b1
+   MX6QDL_PAD_KEY_COL3__HDMI_TX_DDC_SCL
0x4001b8b1
+   MX6QDL_PAD_KEY_ROW3__HDMI_TX_DDC_SDA
0x4001b8b1
>;
};

---8<---

Changes since v1:
- fixed a devm_kfree() signature
- split completions for read and write operations

Vladimir Zapolskiy (2):
  drm: bridge/dw_hdmi: fix register I2CM_ADDRESS register name
  drm: bridge/dw_hdmi: add dw hdmi i2c bus adapter support

 drivers/gpu/drm/bridge/dw_hdmi.c | 341 ++-
 drivers/gpu/drm/bridge/dw_hdmi.h |   8 +-
 2 files changed, 339 insertions(+), 10 deletions(-)

-- 
2.1.4

[PATCH 2/2] drm: bridge/dw_hdmi: add dw hdmi i2c bus adapter support

[Vladimir Zapolskiy]

On 17.05.2015 21:03, Vladimir Zapolskiy wrote:
> The change adds support of internal HDMI I2C master controller, this
> subdevice is used by default, if "ddc-i2c-bus" DT property is omitted.
> 
> The main purpose of this functionality is to support reading EDID from
> an HDMI monitor on boards, which don't have an I2C bus connected to
> DDC pins.
> 
> Signed-off-by: Vladimir Zapolskiy 
> ---
>  drivers/gpu/drm/bridge/dw_hdmi.c | 332 
> ++-
>  1 file changed, 326 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c 
> b/drivers/gpu/drm/bridge/dw_hdmi.c
> index 49cafb6..2a52449 100644
> --- a/drivers/gpu/drm/bridge/dw_hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw_hdmi.c

[snip]

> + ret = i2c_add_adapter(adap);
> + if (ret) {
> + dev_warn(hdmi->dev, "cannot add %s I2C adapter\n", adap->name);
> + devm_kfree(hdmi->i2c);

Immediately found a compilation problem in last minute update, I'll
resend the change, sorry.

> + hdmi->i2c = NULL;
> + return ERR_PTR(ret);
> + }
> +

--
With best wishes,
Vladimir

[PATCH 2/2] drm: bridge/dw_hdmi: add dw hdmi i2c bus adapter support

[Vladimir Zapolskiy]

The change adds support of internal HDMI I2C master controller, this
subdevice is used by default, if "ddc-i2c-bus" DT property is omitted.

The main purpose of this functionality is to support reading EDID from
an HDMI monitor on boards, which don't have an I2C bus connected to
DDC pins.

Signed-off-by: Vladimir Zapolskiy 
---
 drivers/gpu/drm/bridge/dw_hdmi.c | 332 ++-
 1 file changed, 326 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c
index 49cafb6..2a52449 100644
--- a/drivers/gpu/drm/bridge/dw_hdmi.c
+++ b/drivers/gpu/drm/bridge/dw_hdmi.c
@@ -1,15 +1,17 @@
 /*
+ * DesignWare High-Definition Multimedia Interface (HDMI) driver
+ *
+ * Copyright (C) 2013-2015 Mentor Graphics Inc.
  * Copyright (C) 2011-2013 Freescale Semiconductor, Inc.
+ * Copyright (C) 2010, Guennadi Liakhovetski 
  *
  * 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.
  *
- * Designware High-Definition Multimedia Interface (HDMI) driver
- *
- * Copyright (C) 2010, Guennadi Liakhovetski 
  */
+
 #include 
 #include 
 #include 
@@ -28,6 +30,26 @@

 #include "dw_hdmi.h"

+/* HDMI_IH_I2CM_STAT0 and HDMI_IH_MUTE_I2CM_STAT0 register bits */
+#define HDMI_IH_I2CM_STAT0_ERROR   BIT(0)
+#define HDMI_IH_I2CM_STAT0_DONEBIT(1)
+
+/* HDMI_I2CM_OPERATION register bits */
+#define HDMI_I2CM_OPERATION_READ   BIT(0)
+#define HDMI_I2CM_OPERATION_READ_EXT   BIT(1)
+#define HDMI_I2CM_OPERATION_WRITE  BIT(4)
+
+/* HDMI_I2CM_INT register bits */
+#define HDMI_I2CM_INT_DONE_MASKBIT(2)
+#define HDMI_I2CM_INT_DONE_POL BIT(3)
+
+/* HDMI_I2CM_CTLINT register bits */
+#define HDMI_I2CM_CTLINT_ARB_MASK  BIT(2)
+#define HDMI_I2CM_CTLINT_ARB_POL   BIT(3)
+#define HDMI_I2CM_CTLINT_NAC_MASK  BIT(6)
+#define HDMI_I2CM_CTLINT_NAC_POL   BIT(7)
+
+
 #define HDMI_EDID_LEN  512

 #define RGB0
@@ -102,6 +124,17 @@ struct hdmi_data_info {
struct hdmi_vmode video_mode;
 };

+struct dw_hdmi_i2c {
+   struct i2c_adapter  adap;
+
+   spinlock_t  lock;
+   struct completion   cmp;
+   u8  stat;
+
+   u8  slave_reg;
+   boolis_regaddr;
+};
+
 struct dw_hdmi {
struct drm_connector connector;
struct drm_encoder *encoder;
@@ -111,6 +144,7 @@ struct dw_hdmi {
struct device *dev;
struct clk *isfr_clk;
struct clk *iahb_clk;
+   struct dw_hdmi_i2c *i2c;

struct hdmi_data_info hdmi_data;
const struct dw_hdmi_plat_data *plat_data;
@@ -179,6 +213,242 @@ static void hdmi_mask_writeb(struct dw_hdmi *hdmi, u8 
data, unsigned int reg,
hdmi_modb(hdmi, data << shift, mask, reg);
 }

+static void dw_hdmi_i2c_init(struct dw_hdmi *hdmi)
+{
+   unsigned long flags;
+
+   spin_lock_irqsave(>i2c->lock, flags);
+
+   /* Set Fast Mode speed */
+   hdmi_writeb(hdmi, 0x0b, HDMI_I2CM_DIV);
+
+   /* Software reset */
+   hdmi_writeb(hdmi, 0x00, HDMI_I2CM_SOFTRSTZ);
+
+   /* Set done, not acknowledged and arbitration interrupt polarities */
+   hdmi_writeb(hdmi, HDMI_I2CM_INT_DONE_POL, HDMI_I2CM_INT);
+   hdmi_writeb(hdmi, HDMI_I2CM_CTLINT_NAC_POL | HDMI_I2CM_CTLINT_ARB_POL,
+   HDMI_I2CM_CTLINT);
+
+   /* Clear DONE and ERROR interrupts */
+   hdmi_writeb(hdmi, HDMI_IH_I2CM_STAT0_ERROR | HDMI_IH_I2CM_STAT0_DONE,
+   HDMI_IH_I2CM_STAT0);
+
+   /* Mute DONE and ERROR interrupts */
+   hdmi_writeb(hdmi, HDMI_IH_I2CM_STAT0_ERROR | HDMI_IH_I2CM_STAT0_DONE,
+   HDMI_IH_MUTE_I2CM_STAT0);
+
+   spin_unlock_irqrestore(>i2c->lock, flags);
+}
+
+static int dw_hdmi_i2c_read(struct dw_hdmi *hdmi,
+   unsigned char *buf, int length)
+{
+   int stat;
+   unsigned long flags;
+   struct dw_hdmi_i2c *i2c = hdmi->i2c;
+
+   spin_lock_irqsave(>lock, flags);
+
+   if (!i2c->is_regaddr) {
+   dev_dbg(hdmi->dev, "set read register address to 0\n");
+   i2c->slave_reg = 0x00;
+   i2c->is_regaddr = true;
+   }
+
+   while (length--) {
+   hdmi_writeb(hdmi, i2c->slave_reg++, HDMI_I2CM_ADDRESS);
+   hdmi_writeb(hdmi,
+   HDMI_I2CM_OPERATION_READ, HDMI_I2CM_OPERATION);
+   i2c->stat = 0;
+
+   spin_unlock_irqrestore(>lock, flags);
+
+   stat = wait_for_completion_interruptible_timeout(>cmp,
+ 

[PATCH 1/2] drm: bridge/dw_hdmi: fix register I2CM_ADDRESS register name

[Vladimir Zapolskiy]

I2CM_ADDRESS became a MESS, fix it, also change guarding define
to __DW_HDMI_H__ , since the driver is not IMX specific.

Signed-off-by: Vladimir Zapolskiy 
---
 drivers/gpu/drm/bridge/dw_hdmi.h | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw_hdmi.h b/drivers/gpu/drm/bridge/dw_hdmi.h
index 175dbc8..ee7f7ed 100644
--- a/drivers/gpu/drm/bridge/dw_hdmi.h
+++ b/drivers/gpu/drm/bridge/dw_hdmi.h
@@ -7,8 +7,8 @@
  * (at your option) any later version.
  */

-#ifndef __IMX_HDMI_H__
-#define __IMX_HDMI_H__
+#ifndef __DW_HDMI_H__
+#define __DW_HDMI_H__

 /* Identification Registers */
 #define HDMI_DESIGN_ID  0x
@@ -525,7 +525,7 @@

 /* I2C Master Registers (E-DDC) */
 #define HDMI_I2CM_SLAVE 0x7E00
-#define HDMI_I2CMESS0x7E01
+#define HDMI_I2CM_ADDRESS   0x7E01
 #define HDMI_I2CM_DATAO 0x7E02
 #define HDMI_I2CM_DATAI 0x7E03
 #define HDMI_I2CM_OPERATION 0x7E04
@@ -1031,4 +1031,4 @@ enum {
HDMI_A_VIDPOLCFG_HSYNCPOL_ACTIVE_LOW = 0x0,
 };

-#endif /* __IMX_HDMI_H__ */
+#endif /* __DW_HDMI_H__ */
-- 
2.1.4

[PATCH 0/2] drm: bridge/dw_hdmi: add I2C bus adapter support

[Vladimir Zapolskiy]

This change adds support of internal HDMI I2C master controller,
originally the controller has its own separate driver written from
scratch http://patchwork.ozlabs.org/patch/405100 but due to shared
register space and interrupt with HDMI driver, it makes sense to
merge the code of both drivers.

The main purpose of this functionality is to support reading EDID from
an HDMI monitor on boards, which don't have an I2C bus connected to
DDC pins.

To use/test the change "ddc-i2c-bus" DT property must be omitted and
pin settings must be updated accordingly, here is an example for
iMX6 SabreLite:

---8<---
diff --git a/arch/arm/boot/dts/imx6qdl-sabrelite.dtsi 
b/arch/arm/boot/dts/imx6qdl-sabrelite.dtsi
index 0b28a9d..22d4431 100644
--- a/arch/arm/boot/dts/imx6qdl-sabrelite.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sabrelite.dtsi
@@ -174,7 +174,6 @@
 };

  {
-   ddc-i2c-bus = <>;
+   pinctrl-names = "default";
+   pinctrl-0 = <_hdmi>;
status = "okay";
 };

@@ -193,13 +192,6 @@
};
 };

- {
-   clock-frequency = <10>;
-   pinctrl-names = "default";
-   pinctrl-0 = <_i2c2>;
-   status = "okay";
-};
-
  {
clock-frequency = <10>;
pinctrl-names = "default";
@@ -284,10 +276,10 @@
>;
};

-   pinctrl_i2c2: i2c2grp {
+   pinctrl_hdmi: hdmigrp {
fsl,pins = <
-   MX6QDL_PAD_KEY_COL3__I2C2_SCL   
0x4001b8b1
-   MX6QDL_PAD_KEY_ROW3__I2C2_SDA   
0x4001b8b1
+   MX6QDL_PAD_KEY_COL3__HDMI_TX_DDC_SCL
0x4001b8b1
+   MX6QDL_PAD_KEY_ROW3__HDMI_TX_DDC_SDA
0x4001b8b1
>;
};

---8<---

Vladimir Zapolskiy (2):
  drm: bridge: fix register I2CM_ADDRESS register name
  drm: bridge: add dw hdmi i2c bus adapter support

 drivers/gpu/drm/bridge/dw_hdmi.c | 332 ++-
 drivers/gpu/drm/bridge/dw_hdmi.h |   8 +-
 2 files changed, 330 insertions(+), 10 deletions(-)

-- 
2.1.4