Question regarding clocks in the DW-HDMI DT bindings

Commit Message

Fabio Estevam Nov. 25, 2016, 12:29 p.m. UTC

Hi Vladimir,

On Thu, Nov 24, 2016 at 8:16 PM, Vladimir Zapolskiy
<[e-mail]> 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?

                return PTR_ERR(hdmi->regmap);

Then we can remove the gpr from the device tree files.

Comments

Vladimir Zapolskiy Nov. 25, 2016, 1 p.m. UTC | #1

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
> <[e-mail]> 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

Fabio Estevam Nov. 25, 2016, 1:06 p.m. UTC | #2

Hi Vladimir,

On Fri, Nov 25, 2016 at 11:00 AM, Vladimir Zapolskiy
<[e-mail]> 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.

> 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.

Vladimir Zapolskiy Nov. 25, 2016, 1:36 p.m. UTC | #3

On 11/25/2016 03:06 PM, Fabio Estevam wrote:
> Hi Vladimir,
>
> On Fri, Nov 25, 2016 at 11:00 AM, Vladimir Zapolskiy
> <[e-mail]> 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

Patch

--- 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");