Re: [PATCH v2 2/2] phy: amlogic: Add a new driver for the HDMI TX PHY on Meson8/8b/8m2

[Martin Blumenstingl <martin.blumenstingl@googlemail.com>]

Hi Vinod,

On Tue, Jul 20, 2021 at 10:37 AM Vinod Koul <vkoul@kernel.org> wrote:
[...]
> > +     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).

[...]
> > +     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.

[...]
> > +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).


Best regards,
Martin


[0] https://dn.odroid.com/S805/Datasheet/S805_Datasheet%20V0.8%2020150126.pdf
[1] https://github.com/endlessm/linux-meson/blob/d6e13c220931110fe676ede6da69fc61a7cb04b6/arch/arm/mach-meson8/hdmi_tx_hw/hdmi_tx_hw.c#L2350-L2381