On 21-07-21, 00:08, Martin Blumenstingl wrote:

Hi Vinod,

On Tue, Jul 20, 2021 at 10:37 AM Vinod Koul [off-list ref] wrote:
[...]

quoted

quoted

+     if (clk_get_rate(priv->tmds_clk) >= 2970UL * 1000 * 1000)
+             hdmi_ctl0 = 0x1e8b;
+     else
+             hdmi_ctl0 = 0x4d0b;

magic numbers..? I guess these are register offsets, would be better to
define..

Unfortunately these are register values, not offsets
The only "documentation" I have is:
- documentation for the bits/bit-fields in these registers [0]
- some reference code with magic values from the vendor BSP: [1]

HDMI_CTL0/HDMI_CTL1 (the names from the datasheet) is not very
specific and I could not find any other explanation on what the values
mean.
That's why I cannot offer more than these magic values (same situation
for your finding below).

Ok, Can you add a comment that register documentation not available ...

quoted

quoted

+     ret = device_property_read_u32_array(&pdev->dev, "reg", reg,
+                                          ARRAY_SIZE(reg));

we have reg as single property, why array with 2 entries here?

My thought when Rob requested a "reg" property in the dt-bindings was
that I should use offset and size.
I am not validating the size here, which would be in reg[1].
If it's fine for Rob as well then I'll switch the dt-bindings to just
have the offset inside the reg property.

So the property is reg address and size. Two would imply you are using
two reg values.

So I would recommend to use:
        reg_offset = platform_get_resource(pdev, IORESOURCE_MEM, 0);

and skip this reg array.

[...]

quoted

quoted

+static const struct of_device_id phy_meson8_hdmi_tx_of_match[] = {
+     { .compatible = "amlogic,meson8-hdmi-tx-phy" },
+     { .compatible = "amlogic,meson8b-hdmi-tx-phy" },
+     { .compatible = "amlogic,meson8m2-hdmi-tx-phy" },
+     { /* sentinel */ }

I see that all three are handled similarly, no difference!

So far this is correct, they're all treated the same.
However, it happened to me (multiple times) in the past that later on
I would spot a difference hidden in the vendor BSP.
One example is commit f004be596c28f9 ("phy: amlogic: meson8b-usb2: Add
a compatible string for Meson8m2").
I know that other parts of the graphics pipeline are different on
Meson8 compared to the other two SoCs (because Meson8b/Meson8m2 have
some reset lines which need to be toggled after updating the video
clocks. these resets don't exist on Meson8).
So I decided to play safe and add compatible strings for every SoC so
I can easily handle any differences in the future (in case I find
any).

Correct, that is why you need to *keep* the SoC specific compatible and
document them. But use a generic one when you don't have any delta

Above would become:
        { .compatible = "amlogic,meson8-hdmi" },

with DTS specifying:
        compatible = "amlogic,meson8-hdmi-tx-phy", "amlogic,meson8-hdmi";

That way if required you can always use the specific one

-- 
~Vinod