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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAFBinCCJ5DjvjkfjYg7SveDB0bvJ-XV6BwcF_yEeUqxRN9awiQ@mail.gmail.com>
Date:   Wed, 21 Jul 2021 00:08:55 +0200
From:   Martin Blumenstingl <martin.blumenstingl@...glemail.com>
To:     Vinod Koul <vkoul@...nel.org>
Cc:     robh+dt@...nel.org, linux-phy@...ts.infradead.org,
        devicetree@...r.kernel.org, linux-amlogic@...ts.infradead.org,
        kishon@...com, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] phy: amlogic: Add a new driver for the HDMI TX PHY
 on Meson8/8b/8m2

Hi Vinod,

On Tue, Jul 20, 2021 at 10:37 AM Vinod Koul <vkoul@...nel.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