Hi Lucas, [+Cc: Sandor] On Wed, 6 Sep 2023 20:42:11 +0200 Lucas Stach <l.stach@xxxxxxxxxxxxxx> wrote: > This adds the driver for the Samsung HDMI PHY found on the > i.MX8MP SoC. Based on downstream implementation from > Sandor Yu <Sandor.yu@xxxxxxx>. > > Tested-by: Luca Ceresoli <luca.ceresoli@xxxxxxxxxxx> (v2) Also for v3: [On custom board based on MSC SM2S-IMX8PLUS SMARC module] Tested-by: Luca Ceresoli <luca.ceresoli@xxxxxxxxxxx> I have a few notes however, see below. > +#define PHY_REG_14 0x38 > +#define REG14_TOL_MASK GENMASK(7, 4) > +#define REG14_RP_CODE_MASK GENMASK(2, 1) According to the latest reference manual currently available on the NXP website (Rev. 1, 06/2021), this should be GENMASK(3, 1). This is somewhat nitpicking as the only possible value documented is 2. But let's continue... > +#define PHY_REG_33 0x84 > +#define REG33_MODE_SET_DONE BIT(7) > +#define REG33_FIX_DA BIT(1) Here the reference manual is very different: MODE_SET_DONE BIT(4) TX_INV2 BIT(3) TX_INV1 BIT(2) TX_INV0 BIT(1) MON_RXD BIT(0) bits 7-5 are reserved ...which is strange: in the code you are always writing 0 in bit 4, which according to the docs means MODE_SET_DONE is always "Assert forced global reset". Thus I guess your definitions come from the downstream driver which, as it sadly happens, is more authoritative than the docs. :-/ Sandor, can you confirm this, or provide any clarifications? Otherwise LGTM. Luca -- Luca Ceresoli, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
