On 14.11.22 04:16, Marek Vasut wrote: > On 11/14/22 02:11, Nicolas Boichat wrote: >> On Sun, Nov 13, 2022 at 8:29 AM Marek Vasut <marex@xxxxxxx> wrote: >>> >>> On 11/11/22 13:12, Nicolas Boichat wrote: >>> >>> [...] >>> >>>>>> BTW, are you sure DSIM_HSE_MODE is correct now? >>>>> >>>>> Yes, we have tested in imx8m platforms as well. Sébastien Szymanski >>>>> initially observed this issue on the imx8m platform. >>>> >>>> I'll repeat, are you sure about HSE specifically? You invert the >>>> polarity for HBP, HFP, and HSA, which makes sense given your patch >>>> 02/14. >>>> >>>> I'm concerned about HSE. Is the bit really a disable bit? >>>> MIPI_DSI_MODE_VIDEO_HSE is supposed to be an enable flag, so you >>>> should not do `reg |= DSIM_HSE_DISABLE;`, probably. >>> >>> I suspect the HSE bit is a misnomer, but its handling in the driver is >>> correct. >>> >>> i.MX 8M Plus Applications Processor Reference Manual, Rev. 1, 06/2021 >>> Page 5436 >>> >>> 23 HseDisableMode >>> >>> In Vsync pulse and Vporch area, MIPI DSI master transfers only Hsync >>> start packet to MIPI DSI slave at MIPI DSI spec 1.1r02. This bit >>> transfers Hsync end packet in Vsync pulse and Vporch area (optional). >>> >>> 0 = Disables transfer >>> 1 = Enables transfer >>> >>> In command mode, this bit is ignored. >> >> Okay. I'd suggest adding a comment in the code, it'd be so tempting to >> attempt to "fix" this as the if/or pattern looks different from the >> others. >> >> But it's up to you all. > > I agree. Clearly the discrepancy is confusing and leads to mistakes. +1 for a comment in the code that explains the misnamed bit. Otherwise: Reviewed-By: Frieder Schrempf <frieder.schrempf@xxxxxxxxxx>
