Re: [PATCH v7 2/2] drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to DP bridge driver

[Xin Ji]

On Thu, Apr 23, 2020 at 07:55:15PM +0800, Nicolas Boichat wrote:
> Hi,
> 
> Just commenting on the mode_fixup function that was added in v7.
> 
> On Tue, Feb 25, 2020 at 2:15 PM Xin Ji <xji@xxxxxxxxxxxxxxxx> wrote:
> >
> > The ANX7625 is an ultra-low power 4K Mobile HD Transmitter designed
> > for portable device. It converts MIPI DSI/DPI to DisplayPort 1.3 4K.
> >
> > The ANX7625 can support both USB Type-C PD feature and MIPI DSI/DPI
> > to DP feature. This driver only enabled MIPI DSI/DPI to DP feature.
> >
> > Signed-off-by: Xin Ji <xji@xxxxxxxxxxxxxxxx>
> > ---
> >  drivers/gpu/drm/bridge/Makefile           |    2 +-
> >  drivers/gpu/drm/bridge/analogix/Kconfig   |    6 +
> >  drivers/gpu/drm/bridge/analogix/Makefile  |    1 +
> >  drivers/gpu/drm/bridge/analogix/anx7625.c | 2172 +++++++++++++++++++++++++++++
> >  drivers/gpu/drm/bridge/analogix/anx7625.h |  410 ++++++
> >  5 files changed, 2590 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/gpu/drm/bridge/analogix/anx7625.c
> >  create mode 100644 drivers/gpu/drm/bridge/analogix/anx7625.h
> >
> > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> > index 4934fcf..bcd388a 100644
> > --- a/drivers/gpu/drm/bridge/Makefile
> > +++ b/drivers/gpu/drm/bridge/Makefile
> [snip]
> > +static bool anx7625_bridge_mode_fixup(struct drm_bridge *bridge,
> > +                                     const struct drm_display_mode *mode,
> > +                                     struct drm_display_mode *adj)
> > +{
> > +       struct anx7625_data *ctx = bridge_to_anx7625(bridge);
> > +       struct device *dev = &ctx->client->dev;
> > +       u32 hsync, hfp, hbp, hactive, hblanking;
> > +       u32 adj_hsync, adj_hfp, adj_hbp, adj_hblanking, delta_adj;
> > +       u32 vref, adj_clock;
> > +
> > +       DRM_DEV_DEBUG_DRIVER(dev, "drm mode fixup set\n");
> > +
> > +       mutex_lock(&ctx->lock);
> 
> Why do you need this lock?
Seems no need this lock, I'll remove it.
> 
> > +
> > +       hactive = mode->hdisplay;
> 
> This is never used, drop it?
OK, I'll drop it.
> 
> > +       hsync = mode->hsync_end - mode->hsync_start;
> > +       hfp = mode->hsync_start - mode->hdisplay;
> > +       hbp = mode->htotal - mode->hsync_end;
> > +       hblanking = mode->htotal - mode->hdisplay;
> > +
> > +       DRM_DEV_DEBUG_DRIVER(dev, "before mode fixup\n");
> > +       DRM_DEV_DEBUG_DRIVER(dev, "hsync(%d),hfp(%d),hbp(%d),clock(%d)\n",
> > +                            hsync,
> > +                            hfp,
> > +                            hbp,
> > +                            adj->clock);
> > +       DRM_DEV_DEBUG_DRIVER(dev, "hsync_start(%d),hsync_end(%d),htotal(%d)\n",
> > +                            adj->hsync_start,
> > +                            adj->hsync_end,
> > +                            adj->htotal);
> > +
> > +       adj_hfp = hfp;
> > +       adj_hsync = hsync;
> > +       adj_hbp = hbp;
> > +       adj_hblanking = hblanking;
> > +
> > +       /* plus 1 if hfp is odd */
> 
> A better way to word these comments is to say "hfp needs to be even",
> otherwise, you're just repeating what we can already see in the code.
OK
> 
> > +       if (hfp & 0x1) {
> > +               adj_hfp = hfp + 1;
> 
> adj_hfp -= 1 for consistency?
OK
> 
> > +               adj_hblanking += 1;
> > +       }
> > +
> > +       /* minus 1 if hbp is odd */
> > +       if (hbp & 0x1) {
> > +               adj_hbp = hbp - 1;
> 
> ditto, adj_hbp -= 1;
OK
> 
> > +               adj_hblanking -= 1;
> > +       }
> > +
> > +       /* plus 1 if hsync is odd */
> > +       if (hsync & 0x1) {
> > +               if (adj_hblanking < hblanking)
> > +                       adj_hsync = hsync + 1;
> 
> ditto
OK
> 
> > +               else
> > +                       adj_hsync = hsync - 1;
> 
> ditto
OK
> 
> > +       }
> > +
> > +       /*
> > +        * once illegal timing detected, use default HFP, HSYNC, HBP
> > +        */
> > +       if (hblanking < HBLANKING_MIN || (hfp < HP_MIN && hbp < HP_MIN)) {
> 
> should this be adj_hblanking/adj_hfp/adj_hbp?
NO, need check original HFP and HBP, if they are not legal, driver need
set default value to adj_hsync, adj_hfp, adj_hbp.
> 
> > +               adj_hsync = SYNC_LEN_DEF;
> > +               adj_hfp = HFP_HBP_DEF;
> > +               adj_hbp = HFP_HBP_DEF;
> > +               vref = adj->clock * 1000 / (adj->htotal * adj->vtotal);
> > +               if (hblanking < HBLANKING_MIN) {
> > +                       delta_adj = HBLANKING_MIN - hblanking;
> > +                       adj_clock = vref * delta_adj * adj->vtotal;
> > +                       adj->clock += DIV_ROUND_UP(adj_clock, 1000);
> > +               } else {
> > +                       delta_adj = hblanking - HBLANKING_MIN;
> > +                       adj_clock = vref * delta_adj * adj->vtotal;
> > +                       adj->clock -= DIV_ROUND_UP(adj_clock, 1000);
> > +               }
> > +
> > +               DRM_WARN("illegal hblanking timing, use default.\n");
> > +               DRM_WARN("hfp(%d),hbp(%d),hsync(%d).\n", hfp, hbp, hsync);
> 
> How likely is it that this mode is going to work? Can you just return
> false here to reject the mode?
We want to set the default minimal Hblancking value, then it may display,
otherwise. If we just return false, there is no display for sure.
> 
> > +       } else if (adj_hfp < HP_MIN) {
> > +               /* adjust hfp if hfp less than HP_MIN */
> > +               delta_adj = HP_MIN - adj_hfp;
> > +               adj_hfp = HP_MIN;
> > +
> > +               /*
> > +                * balance total HBlanking pixel, if HBP hasn't enough space,
> 
> "does not have enough space"
OK
> 
> > +                * adjust HSYNC length, otherwize adjust HBP
> 
> otherwise
OK
> 
> > +                */
> > +               if ((adj_hbp - delta_adj) < HP_MIN)
> > +                       /* hbp not enough space */
> > +                       adj_hsync -= delta_adj;
> > +               else
> > +                       adj_hbp -= delta_adj;
> > +       } else if (adj_hbp < HP_MIN) {
> > +               delta_adj = HP_MIN - adj_hbp;
> > +               adj_hbp = HP_MIN;
> > +
> > +               /*
> > +                * balance total HBlanking pixel, if HBP hasn't enough space,
> > +                * adjust HSYNC length, otherwize adjust HBP
> > +                */
> > +               if ((adj_hfp - delta_adj) < HP_MIN)
> > +                       /* hbp not enough space */
> > +                       adj_hsync -= delta_adj;
> > +               else
> > +                       adj_hfp -= delta_adj;
> > +       }
> > +
> > +       DRM_DEV_DEBUG_DRIVER(dev, "after mode fixup\n");
> > +       DRM_DEV_DEBUG_DRIVER(dev, "hsync(%d),hfp(%d),hbp(%d),clock(%d)\n",
> 
> Add spaces after commas in your debug strings (same above and below).
OK
> 
> > +                            adj_hsync,
> > +                            adj_hfp,
> > +                            adj_hbp,
> > +                            adj->clock);
> 
> Put these 4 on a single line.
OK
> 
> > +
> > +       /* reconstruct timing */
> > +       adj->hsync_start = adj->hdisplay + adj_hfp;
> > +       adj->hsync_end = adj->hsync_start + adj_hsync;
> > +       adj->htotal = adj->hsync_end + adj_hbp;
> > +       DRM_DEV_DEBUG_DRIVER(dev, "hsync_start(%d),hsync_end(%d),htotal(%d)\n",
> > +                            adj->hsync_start,
> > +                            adj->hsync_end,
> > +                            adj->htotal);
> > +
> > +       mutex_unlock(&ctx->lock);
> > +
> > +       return true;
> > +}
> > +
> > [snip]