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?
> +
> + 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]
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
