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

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@analogixsemi.com 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@analogixsemi.com

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?

•   hactive = mode->hdisplay;
  

This is never used, 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.

•   if (hfp & 0x1) {
  

•           adj_hfp = hfp + 1;
  

adj_hfp -= 1 for consistency?

•           adj_hblanking += 1;
  

•   }
  

•   /* minus 1 if hbp is odd */
  

•   if (hbp & 0x1) {
  

•           adj_hbp = hbp - 1;
  

ditto, adj_hbp -= 1;

•           adj_hblanking -= 1;
  

•   }
  

•   /* plus 1 if hsync is odd */
  

•   if (hsync & 0x1) {
  

•           if (adj_hblanking < hblanking)
  

•                   adj_hsync = hsync + 1;
  

ditto

•           else
  

•                   adj_hsync = hsync - 1;
  

ditto

•   }
  

•   /*
  

•    * 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?

•           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?

•   } 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"

•            * adjust HSYNC length, otherwize adjust HBP
  

otherwise

•            */
  

•           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).

•                        adj_hsync,
  

•                        adj_hfp,
  

•                        adj_hbp,
  

•                        adj->clock);
  

Put these 4 on a single line.

•   /* 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]