Re: [PATCH 1/3] staging: imx-drm: document internal HDMI I2C master controller DT binding

[Andy Yan <andy.yan@rock-chips.com>]

Hi Vladimir:

  I am working on convert imx-hdmi to dw_hdmi now:
  https://lkml.org/lkml/2014/12/1/190
  I also have a plan to use  the internal HDMI I2C master under the I2c 
framework,
and I also have a patch to do this work. So glad to see your work.
  Please also Cc me<and.yan@rock-chips.com> and Zubair.Kakakhel@imgtec.com,
maybe Zubair also have interests on your future patch.
On 2014年12月02日 00:22, Philipp Zabel wrote:
> Hi Vladimir,
>
> [Added Andy Yan to Cc:, because imx-hdmi->dw-hdmi]
>
> Am Montag, den 01.12.2014, 17:39 +0200 schrieb Vladimir Zapolskiy:
>> On 01.12.2014 17:11, Philipp Zabel wrote:
>>> Am Montag, den 01.12.2014, 16:54 +0200 schrieb Vladimir Zapolskiy:
>>>> Hi Philipp and Shawn,
>>>>
>>>> On 15.11.2014 19:49, Vladimir Zapolskiy wrote:
>>>>> Provide information about how to bind internal iMX6Q/DL HDMI DDC I2C
>>>>> master controller. The property is set as optional one, because iMX6
>>>>> HDMI DDC bus may be represented by one of general purpose I2C busses
>>>>> found on SoC.
>>>>>
>>>>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
>>>>> Cc: Wolfram Sang <wsa@the-dreams.de>
>>>>> Cc: Philipp Zabel <p.zabel@pengutronix.de>
>>>>> Cc: Shawn Guo <shawn.guo@linaro.org>
>>>>> Cc: devicetree@vger.kernel.org
>>>>> Cc: linux-media@vger.kernel.org
>>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>>> Cc: linux-i2c@vger.kernel.org
>>>>> ---
>>>>>   Documentation/devicetree/bindings/staging/imx-drm/hdmi.txt |   10 +++++++++-
>>>>>   1 file changed, 9 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/staging/imx-drm/hdmi.txt b/Documentation/devicetree/bindings/staging/imx-drm/hdmi.txt
>>>>> index 1b756cf..43c8924 100644
>>>>> --- a/Documentation/devicetree/bindings/staging/imx-drm/hdmi.txt
>>>>> +++ b/Documentation/devicetree/bindings/staging/imx-drm/hdmi.txt
>>>>> @@ -10,6 +10,8 @@ Required properties:
>>>>>    - #address-cells : should be <1>
>>>>>    - #size-cells : should be <0>
>>>>>    - compatible : should be "fsl,imx6q-hdmi" or "fsl,imx6dl-hdmi".
>>>>> +   If internal HDMI DDC I2C master controller is supposed to be used,
>>>>> +   then "simple-bus" should be added to compatible value.
>>>>>    - gpr : should be <&gpr>.
>>>>>      The phandle points to the iomuxc-gpr region containing the HDMI
>>>>>      multiplexer control register.
>>>>> @@ -22,6 +24,7 @@ Required properties:
>>>>>   
>>>>>   Optional properties:
>>>>>    - ddc-i2c-bus: phandle of an I2C controller used for DDC EDID probing
>>>>> + - ddc: internal HDMI DDC I2C master controller
>>>>>   
>>>>>   example:
>>>>>   
>>>>> @@ -32,7 +35,7 @@ example:
>>>>>           hdmi: hdmi@0120000 {
>>>>>                   #address-cells = <1>;
>>>>>                   #size-cells = <0>;
>>>>> -                compatible = "fsl,imx6q-hdmi";
>>>>> +                compatible = "fsl,imx6q-hdmi", "simple-bus";
>>>>>                   reg = <0x00120000 0x9000>;
>>>>>                   interrupts = <0 115 0x04>;
>>>>>                   gpr = <&gpr>;
>>>>> @@ -40,6 +43,11 @@ example:
>>>>>                   clock-names = "iahb", "isfr";
>>>>>                   ddc-i2c-bus = <&i2c2>;
>>>>>   
>>>>> +                hdmi_ddc: ddc {
>>>>> +                        compatible = "fsl,imx6q-hdmi-ddc";
>>>>> +                        status = "disabled";
>>>>> +                };
>>>>> +
>>>>>                   port@0 {
>>>>>                           reg = <0>;
>>>>>   
>>>>>
>>>> knowing in advance that I2C framework lacks a graceful support of non
>>>> fully compliant I2C devices, do you have any objections to the proposed
>>>> iMX HDMI DTS change?
>>> I'm not sure about this. Have you seen "drm: Decouple EDID parsing from
>>> I2C adapter"? I feel like in the absence of a ddc-i2c-bus property the
>>> imx-hdmi/dw-hdmi driver should try to use the internal HDMI DDC I2C
>>> master controller, bypassing the I2C framework altogether.
>>>
>> My idea is exactly not to bypass the I2C framework, briefly the
>> rationale is that
>> * it allows to reuse I2C UAPI/tools naturally applied to the internal
>> iMX HDMI DDC bus,
>> * it allows to use iMX HDMI DDC bus as an additional feature-limited I2C
>> bus on SoC (who knows, I absolutely won't be surprised, if anyone needs
>> it on practice),
>> * if an HDMI controller supports an external I2C bus, the integration
>> with HDMI DDC bus driver based on I2C framework is seamless.
>>
>> However I agree that the selected approach may look odd, the question is
>> if the oddness comes from the technical side or from the fact that
>> nobody has done it before this way.
>>
>> I'm open to any critique, if the proposal of creating an I2C bus from
>> HDMI DDC bus is lame, then I suppose the shared iMX HDMI DDC bus driver
>> should be converted to something formless and internally used by
>> imx-hdmi. The negative side-effects of such a change from my point of
>> view are
>> * more or less natural modularity is lost,
>> * a number of I2C framework API/functions should be copy-pasted to the
>> updated HDMI DDC bus driver to support a subset of I2C read/write
>> transactions.
> If Wolfram is happy to accomodate such feature limited, 'I2C master'
> devices in i2c/drivers/busses in principle, I won't disagree.
>
> But then it should be abstracted properly. The dw-hdmi-tx core on i.MX6
> has the DDC I2C master register space at 0x7e00 - 0x7e12. What are the
> offsets on the Rockchip version? If the "simple-bus" compatible is to be
> set on the hdmi driver, the ddc driver should do its own register
> access, and therefore needs a reg property. I suspect for the ddc-i2cm
> we should get away with a common compatible like "snps,dw-hdmi-i2c".
>
> 	hdmi: hdmi@120000 {
> 		/* ... */
> 		compatible = "fsl,imx6q-hdmi", "snps,dw-hdmi";
> 		ddc-i2c-bus = <&hdmi_ddc>;
>
> 		hdmi_ddc: i2c@127e00 {
> 			compatible = "snps,dw-hdmi-i2c";
> 			reg = <0x1207e00 0x13>
> 		};
>
> 		/* could add phy-i2cm, cec, ... here */
> 	};
>
> Also there's an i2c bus for communication with the phy at 0x3020 -
> 0x3032, should that be handled in a similar way, then?

    Rockchip RK3288 has the same DDC I2C master and phy i2c master
    register offset as imx hdmi.

     what my suggestion is: make the ddc-i2c-bus property optional, if 
the dw_hdmi driver
    can't found the ddc-i2c-bus node, then use the internal i2cm:
        ddc_node = of_parse_phandle(np, "ddc-i2c-bus", 0);
         if (ddc_node) {
                 hdmi->ddc = of_find_i2c_adapter_by_node(ddc_node);
                 of_node_put(ddc_node);
                 if (!hdmi->ddc) {
                         dev_dbg(hdmi->dev, "failed to read ddc node\n");
                         return -EPROBE_DEFER;
                 }

         } else {
                 hdmi->ddc = dw_hdmi_ddc_adapter_register(hdmi).
         }

         and we can have a file put with dw_hdmi.c in a same dir, which
        implement  standard i2c adapter interface .
       I found that other hdmi platform such as msm(/msm/hdmi/hdmi_i2c.c) ,
       radeon(radeon_i2c.c) also do like this


>
> Do we need to make the hdmi driver a proper interrupt controller? At
> least the two i2c masters have two interrupts each, "done" and "error".
>
> regards
> Philipp
>
>
>
>