On Sun, Oct 16, 2022 at 2:53 AM Marek Vasut <marex@xxxxxxx> wrote:
>
> On 10/5/22 17:13, Jagan Teki wrote:
> > Look like an explicit fixing up of mode_flags is required for DSIM IP
> > present in i.MX8M Mini/Nano SoCs.
> >
> > At least the LCDIF + DSIM needs active low sync polarities in order
> > to correlate the correct sync flags of the surrounding components in
> > the chain to make sure the whole pipeline can work properly.
> >
> > On the other hand the i.MX 8M Mini Applications Processor Reference Manual,
> > Rev. 3, 11/2020 says.
> > "13.6.3.5.2 RGB interface
> > Vsync, Hsync, and VDEN are active high signals."
> >
> > No clear evidence about whether it can be documentation issues or
> > something, so added a comment FIXME for this and updated the active low
> > sync polarities using SAMSUNG_DSIM_TYPE_IMX8MM hw_type.
>
> [...]
>
> > +static int samsung_dsim_atomic_check(struct drm_bridge *bridge,
> > + struct drm_bridge_state *bridge_state,
> > + struct drm_crtc_state *crtc_state,
> > + struct drm_connector_state *conn_state)
> > +{
> > + struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> > + struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
> > +
> > + if (dsi->plat_data->hw_type == SAMSUNG_DSIM_TYPE_IMX8MM) {
> > + /**
> > + * FIXME:
> > + * At least LCDIF + DSIM needs active low sync,
> > + * but i.MX 8M Mini Applications Processor Reference Manual,
> > + * Rev. 3, 11/2020 says
> > + *
> > + * 13.6.3.5.2 RGB interface
> > + * Vsync, Hsync, and VDEN are active high signals.
> > + */
> > + adjusted_mode->flags |= (DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC);
> > + adjusted_mode->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
> > + }
>
> It would be good to explain what exactly is going on here in the
> comment, the comment says "Vsync, Hsync, and VDEN are active high
> signals." and the code below does exact opposite and sets NxSYNC flags.
>
> Yes, the MX8MM/MN does need HS/VS/DE active LOW, it is a quirk of that
> MXSFB-DSIM glue logic. The MX8MP needs the exact opposite on all three,
> active HIGH.
This is what exactly is mentioned in the comments.
2nd line mentioned the active low of signals.
> > + * At least LCDIF + DSIM needs active low sync,
from 3rd line onwards it mentioned what reference manual is referring
Not quite understand what is misleading here.
>
> It would also be good to mention both MX8MM and MX8MN are affected, not
> only MX8MM.
I think I can do this once I refer to the MX8MN manual.
Jagan.
