Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 6 Mar 2022 19:13:30 +0200
From:   Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To:     Xin Ji <xji@analogixsemi.com>
Cc:     a.hajda@samsung.com, narmstrong@baylibre.com,
        dan.carpenter@oracle.com, robert.foss@linaro.org, jonas@kwiboo.se,
        jernej.skrabec@gmail.com, airlied@linux.ie, daniel@ffwll.ch,
        sam@ravnborg.org, pihsun@chromium.org, tzungbi@google.com,
        maxime@cerno.tech, drinkcat@google.com, hsinyi@chromium.org,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
        bliang@analogixsemi.com, qwen@analogixsemi.com, robh+dt@kernel.org,
        devicetree@vger.kernel.org
Subject: Re: [PATCH v12 3/4] drm/bridge: anx7625: add MIPI DPI input feature
Message-ID: <YiTruiCIkyxs3jTC@pendragon.ideasonboard.com>
References: <20211105031904.2641088-1-xji@analogixsemi.com>
 <20211105031904.2641088-3-xji@analogixsemi.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20211105031904.2641088-3-xji@analogixsemi.com>
Precedence: bulk

Hello Xin,

(Question for Rob below, and I'm afraid this is urgent as we need to
merge a fix in v5.17).

On Fri, Nov 05, 2021 at 11:19:03AM +0800, Xin Ji wrote:
> The basic anx7625 driver only support MIPI DSI rx signal input.
> This patch add MIPI DPI rx input configuration support, after apply
> this patch, the driver can support DSI rx or DPI rx by adding
> 'bus-type' in DT.
> 
> Reviewed-by: Robert Foss <robert.foss@linaro.org>
> Signed-off-by: Xin Ji <xji@analogixsemi.com>
> ---
>  drivers/gpu/drm/bridge/analogix/anx7625.c | 247 ++++++++++++++++------
>  drivers/gpu/drm/bridge/analogix/anx7625.h |  18 +-
>  2 files changed, 205 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> index f48e91134c20..f7c3386c8929 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> @@ -32,6 +32,7 @@
>  #include <drm/drm_print.h>
>  #include <drm/drm_probe_helper.h>
>  
> +#include <media/v4l2-fwnode.h>
>  #include <video/display_timing.h>
>  
>  #include "anx7625.h"
> @@ -152,18 +153,18 @@ static int anx7625_write_and(struct anx7625_data *ctx,
>  	return anx7625_reg_write(ctx, client, offset, (val & (mask)));
>  }
>  
> -static int anx7625_write_and_or(struct anx7625_data *ctx,
> -				struct i2c_client *client,
> -				u8 offset, u8 and_mask, u8 or_mask)
> +static int anx7625_config_bit_matrix(struct anx7625_data *ctx)
>  {
> -	int val;
> +	int i, ret;
>  
> -	val = anx7625_reg_read(ctx, client, offset);
> -	if (val < 0)
> -		return val;
> +	ret = anx7625_write_or(ctx, ctx->i2c.tx_p2_client,
> +			       AUDIO_CONTROL_REGISTER, 0x80);
> +	for (i = 0; i < 13; i++)
> +		ret |= anx7625_reg_write(ctx, ctx->i2c.tx_p2_client,
> +					 VIDEO_BIT_MATRIX_12 + i,
> +					 0x18 + i);
>  
> -	return anx7625_reg_write(ctx, client,
> -				 offset, (val & and_mask) | (or_mask));
> +	return ret;
>  }
>  
>  static int anx7625_read_ctrl_status_p0(struct anx7625_data *ctx)
> @@ -221,38 +222,6 @@ static int anx7625_video_mute_control(struct anx7625_data *ctx,
>  	return ret;
>  }
>  
> -static int anx7625_config_audio_input(struct anx7625_data *ctx)
> -{
> -	struct device *dev = &ctx->client->dev;
> -	int ret;
> -
> -	/* Channel num */
> -	ret = anx7625_reg_write(ctx, ctx->i2c.tx_p2_client,
> -				AUDIO_CHANNEL_STATUS_6, I2S_CH_2 << 5);
> -
> -	/* FS */
> -	ret |= anx7625_write_and_or(ctx, ctx->i2c.tx_p2_client,
> -				    AUDIO_CHANNEL_STATUS_4,
> -				    0xf0, AUDIO_FS_48K);
> -	/* Word length */
> -	ret |= anx7625_write_and_or(ctx, ctx->i2c.tx_p2_client,
> -				    AUDIO_CHANNEL_STATUS_5,
> -				    0xf0, AUDIO_W_LEN_24_24MAX);
> -	/* I2S */
> -	ret |= anx7625_write_or(ctx, ctx->i2c.tx_p2_client,
> -				AUDIO_CHANNEL_STATUS_6, I2S_SLAVE_MODE);
> -	ret |= anx7625_write_and(ctx, ctx->i2c.tx_p2_client,
> -				 AUDIO_CONTROL_REGISTER, ~TDM_TIMING_MODE);
> -	/* Audio change flag */
> -	ret |= anx7625_write_or(ctx, ctx->i2c.rx_p0_client,
> -				AP_AV_STATUS, AP_AUDIO_CHG);
> -
> -	if (ret < 0)
> -		DRM_DEV_ERROR(dev, "fail to config audio.\n");
> -
> -	return ret;
> -}
> -
>  /* Reduction of fraction a/b */
>  static void anx7625_reduction_of_a_fraction(unsigned long *a, unsigned long *b)
>  {
> @@ -431,7 +400,7 @@ static int anx7625_dsi_video_timing_config(struct anx7625_data *ctx)
>  	ret |= anx7625_write_and(ctx, ctx->i2c.rx_p1_client,
>  			MIPI_LANE_CTRL_0, 0xfc);
>  	ret |= anx7625_write_or(ctx, ctx->i2c.rx_p1_client,
> -				MIPI_LANE_CTRL_0, 3);
> +				MIPI_LANE_CTRL_0, ctx->pdata.mipi_lanes - 1);
>  
>  	/* Htotal */
>  	htotal = ctx->dt.hactive.min + ctx->dt.hfront_porch.min +
> @@ -615,6 +584,76 @@ static int anx7625_dsi_config(struct anx7625_data *ctx)
>  	return ret;
>  }
>  
> +static int anx7625_api_dpi_config(struct anx7625_data *ctx)
> +{
> +	struct device *dev = &ctx->client->dev;
> +	u16 freq = ctx->dt.pixelclock.min / 1000;
> +	int ret;
> +
> +	/* configure pixel clock */
> +	ret = anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> +				PIXEL_CLOCK_L, freq & 0xFF);
> +	ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> +				 PIXEL_CLOCK_H, (freq >> 8));
> +
> +	/* set DPI mode */
> +	/* set to DPI PLL module sel */
> +	ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p1_client,
> +				 MIPI_DIGITAL_PLL_9, 0x20);
> +	/* power down MIPI */
> +	ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p1_client,
> +				 MIPI_LANE_CTRL_10, 0x08);
> +	/* enable DPI mode */
> +	ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p1_client,
> +				 MIPI_DIGITAL_PLL_18, 0x1C);
> +	/* set first edge */
> +	ret |= anx7625_reg_write(ctx, ctx->i2c.tx_p2_client,
> +				 VIDEO_CONTROL_0, 0x06);
> +	if (ret < 0)
> +		DRM_DEV_ERROR(dev, "IO error : dpi phy set failed.\n");
> +
> +	return ret;
> +}
> +
> +static int anx7625_dpi_config(struct anx7625_data *ctx)
> +{
> +	struct device *dev = &ctx->client->dev;
> +	int ret;
> +
> +	DRM_DEV_DEBUG_DRIVER(dev, "config dpi\n");
> +
> +	/* DSC disable */
> +	ret = anx7625_write_and(ctx, ctx->i2c.rx_p0_client,
> +				R_DSC_CTRL_0, ~DSC_EN);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(dev, "IO error : disable dsc failed.\n");
> +		return ret;
> +	}
> +
> +	ret = anx7625_config_bit_matrix(ctx);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(dev, "config bit matrix failed.\n");
> +		return ret;
> +	}
> +
> +	ret = anx7625_api_dpi_config(ctx);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(dev, "mipi phy(dpi) setup failed.\n");
> +		return ret;
> +	}
> +
> +	/* set MIPI RX EN */
> +	ret = anx7625_write_or(ctx, ctx->i2c.rx_p0_client,
> +			       AP_AV_STATUS, AP_MIPI_RX_EN);
> +	/* clear mute flag */
> +	ret |= anx7625_write_and(ctx, ctx->i2c.rx_p0_client,
> +				 AP_AV_STATUS, (u8)~AP_MIPI_MUTE);
> +	if (ret < 0)
> +		DRM_DEV_ERROR(dev, "IO error : enable mipi rx failed.\n");
> +
> +	return ret;
> +}
> +
>  static void anx7625_dp_start(struct anx7625_data *ctx)
>  {
>  	int ret;
> @@ -625,9 +664,10 @@ static void anx7625_dp_start(struct anx7625_data *ctx)
>  		return;
>  	}
>  
> -	anx7625_config_audio_input(ctx);
> -
> -	ret = anx7625_dsi_config(ctx);
> +	if (ctx->pdata.is_dpi)
> +		ret = anx7625_dpi_config(ctx);
> +	else
> +		ret = anx7625_dsi_config(ctx);
>  
>  	if (ret < 0)
>  		DRM_DEV_ERROR(dev, "MIPI phy setup error.\n");
> @@ -1075,6 +1115,7 @@ static void anx7625_start_dp_work(struct anx7625_data *ctx)
>  		return;
>  	}
>  
> +	ctx->hpd_status = 1;
>  	ctx->hpd_high_cnt++;
>  
>  	/* Not support HDCP */
> @@ -1084,8 +1125,10 @@ static void anx7625_start_dp_work(struct anx7625_data *ctx)
>  	ret |= anx7625_write_or(ctx, ctx->i2c.rx_p1_client, 0xec, 0x10);
>  	/* Interrupt for DRM */
>  	ret |= anx7625_write_or(ctx, ctx->i2c.rx_p1_client, 0xff, 0x01);
> -	if (ret < 0)
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(dev, "fail to setting HDCP/auth\n");
>  		return;
> +	}
>  
>  	ret = anx7625_reg_read(ctx, ctx->i2c.rx_p1_client, 0x86);
>  	if (ret < 0)
> @@ -1104,6 +1147,10 @@ static void anx7625_hpd_polling(struct anx7625_data *ctx)
>  	int ret, val;
>  	struct device *dev = &ctx->client->dev;
>  
> +	/* Interrupt mode, no need poll HPD status, just return */
> +	if (ctx->pdata.intp_irq)
> +		return;
> +
>  	ret = readx_poll_timeout(anx7625_read_hpd_status_p0,
>  				 ctx, val,
>  				 ((val & HPD_STATUS) || (val < 0)),
> @@ -1131,6 +1178,21 @@ static void anx7625_remove_edid(struct anx7625_data *ctx)
>  	ctx->slimport_edid_p.edid_block_num = -1;
>  }
>  
> +static void anx7625_dp_adjust_swing(struct anx7625_data *ctx)
> +{
> +	int i;
> +
> +	for (i = 0; i < ctx->pdata.dp_lane0_swing_reg_cnt; i++)
> +		anx7625_reg_write(ctx, ctx->i2c.tx_p1_client,
> +				  DP_TX_LANE0_SWING_REG0 + i,
> +				  ctx->pdata.lane0_reg_data[i] & 0xFF);
> +
> +	for (i = 0; i < ctx->pdata.dp_lane1_swing_reg_cnt; i++)
> +		anx7625_reg_write(ctx, ctx->i2c.tx_p1_client,
> +				  DP_TX_LANE1_SWING_REG0 + i,
> +				  ctx->pdata.lane1_reg_data[i] & 0xFF);
> +}
> +
>  static void dp_hpd_change_handler(struct anx7625_data *ctx, bool on)
>  {
>  	struct device *dev = &ctx->client->dev;
> @@ -1146,9 +1208,8 @@ static void dp_hpd_change_handler(struct anx7625_data *ctx, bool on)
>  	} else {
>  		DRM_DEV_DEBUG_DRIVER(dev, " HPD high\n");
>  		anx7625_start_dp_work(ctx);
> +		anx7625_dp_adjust_swing(ctx);
>  	}
> -
> -	ctx->hpd_status = 1;
>  }
>  
>  static int anx7625_hpd_change_detect(struct anx7625_data *ctx)
> @@ -1225,20 +1286,72 @@ static irqreturn_t anx7625_intr_hpd_isr(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> +static int anx7625_get_swing_setting(struct device *dev,
> +				     struct anx7625_platform_data *pdata)
> +{
> +	int num_regs;
> +
> +	if (of_get_property(dev->of_node,
> +			    "analogix,lane0-swing", &num_regs)) {
> +		if (num_regs > DP_TX_SWING_REG_CNT)
> +			num_regs = DP_TX_SWING_REG_CNT;
> +
> +		pdata->dp_lane0_swing_reg_cnt = num_regs;
> +		of_property_read_u32_array(dev->of_node, "analogix,lane0-swing",
> +					   pdata->lane0_reg_data, num_regs);
> +	}
> +
> +	if (of_get_property(dev->of_node,
> +			    "analogix,lane1-swing", &num_regs)) {
> +		if (num_regs > DP_TX_SWING_REG_CNT)
> +			num_regs = DP_TX_SWING_REG_CNT;
> +
> +		pdata->dp_lane1_swing_reg_cnt = num_regs;
> +		of_property_read_u32_array(dev->of_node, "analogix,lane1-swing",
> +					   pdata->lane1_reg_data, num_regs);
> +	}
> +
> +	return 0;
> +}
> +
>  static int anx7625_parse_dt(struct device *dev,
>  			    struct anx7625_platform_data *pdata)
>  {
> -	struct device_node *np = dev->of_node;
> +	struct device_node *np = dev->of_node, *ep0;
>  	struct drm_panel *panel;
>  	int ret;
> +	int bus_type, mipi_lanes;
> +
> +	anx7625_get_swing_setting(dev, pdata);
>  
> +	pdata->is_dpi = 1; /* default dpi mode */
>  	pdata->mipi_host_node = of_graph_get_remote_node(np, 0, 0);
>  	if (!pdata->mipi_host_node) {
>  		DRM_DEV_ERROR(dev, "fail to get internal panel.\n");
>  		return -ENODEV;
>  	}
>  
> -	DRM_DEV_DEBUG_DRIVER(dev, "found dsi host node.\n");
> +	bus_type = V4L2_FWNODE_BUS_TYPE_PARALLEL;
> +	mipi_lanes = MAX_LANES_SUPPORT;
> +	ep0 = of_graph_get_endpoint_by_regs(np, 0, 0);
> +	if (ep0) {
> +		if (of_property_read_u32(ep0, "bus-type", &bus_type))
> +			bus_type = 0;
> +
> +		mipi_lanes = of_property_count_u32_elems(ep0, "data-lanes");
> +	}
> +
> +	if (bus_type == V4L2_FWNODE_BUS_TYPE_PARALLEL) /* bus type is Parallel(DSI) */

This is not correct *at all*. V4L2_FWNODE_BUS_TYPE_PARALLEL has nothing
to do with DSI. DSI stands for Digital *Serial* Interface. If anything,
the V4L2_FWNODE_BUS_TYPE_PARALLEL type would map better to DPI, even if
it's not an exact match.

This patch has landed in v5.17-rc1, along with the corresponding
bindings. As DT bindings are an ABI, we should really fix this before
v5.17 is released. There is no DSI bus types defined in DT, and adding
one as a fix so late in the v5.17-rc cycle seems a bit of a stretch to
me (unless Rob disagrees).

It would seem best to revert this series and the corresponding bindings,
and retry in v5.18.

> +		pdata->is_dpi = 0;
> +
> +	pdata->mipi_lanes = mipi_lanes;
> +	if (pdata->mipi_lanes > MAX_LANES_SUPPORT || pdata->mipi_lanes <= 0)
> +		pdata->mipi_lanes = MAX_LANES_SUPPORT;
> +
> +	if (pdata->is_dpi)
> +		DRM_DEV_DEBUG_DRIVER(dev, "found MIPI DPI host node.\n");
> +	else
> +		DRM_DEV_DEBUG_DRIVER(dev, "found MIPI DSI host node.\n");
>  
>  	ret = drm_of_find_panel_or_bridge(np, 1, 0, &panel, NULL);
>  	if (ret < 0) {
> @@ -1301,9 +1414,13 @@ static enum drm_connector_status anx7625_sink_detect(struct anx7625_data *ctx)
>  {
>  	struct device *dev = &ctx->client->dev;
>  
> -	DRM_DEV_DEBUG_DRIVER(dev, "sink detect, return connected\n");
> +	DRM_DEV_DEBUG_DRIVER(dev, "sink detect\n");
>  
> -	return connector_status_connected;
> +	if (ctx->pdata.panel_bridge)
> +		return connector_status_connected;
> +
> +	return ctx->hpd_status ? connector_status_connected :
> +				     connector_status_disconnected;
>  }
>  
>  static int anx7625_attach_dsi(struct anx7625_data *ctx)
> @@ -1332,7 +1449,7 @@ static int anx7625_attach_dsi(struct anx7625_data *ctx)
>  		return -EINVAL;
>  	}
>  
> -	dsi->lanes = 4;
> +	dsi->lanes = ctx->pdata.mipi_lanes;
>  	dsi->format = MIPI_DSI_FMT_RGB888;
>  	dsi->mode_flags = MIPI_DSI_MODE_VIDEO	|
>  		MIPI_DSI_MODE_VIDEO_SYNC_PULSE	|
> @@ -1460,6 +1577,10 @@ static bool anx7625_bridge_mode_fixup(struct drm_bridge *bridge,
>  
>  	DRM_DEV_DEBUG_DRIVER(dev, "drm mode fixup set\n");
>  
> +	/* No need fixup for external monitor */
> +	if (!ctx->pdata.panel_bridge)
> +		return true;
> +
>  	hsync = mode->hsync_end - mode->hsync_start;
>  	hfp = mode->hsync_start - mode->hdisplay;
>  	hbp = mode->htotal - mode->hsync_end;
> @@ -1835,14 +1956,22 @@ static int anx7625_i2c_probe(struct i2c_client *client,
>  
>  	platform->bridge.funcs = &anx7625_bridge_funcs;
>  	platform->bridge.of_node = client->dev.of_node;
> -	platform->bridge.ops = DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_HPD;
> -	platform->bridge.type = DRM_MODE_CONNECTOR_eDP;
> +	platform->bridge.ops = DRM_BRIDGE_OP_EDID;
> +	if (!platform->pdata.panel_bridge)
> +		platform->bridge.ops |= DRM_BRIDGE_OP_HPD |
> +					DRM_BRIDGE_OP_DETECT;
> +	platform->bridge.type = platform->pdata.panel_bridge ?
> +				    DRM_MODE_CONNECTOR_eDP :
> +				    DRM_MODE_CONNECTOR_DisplayPort;
> +
>  	drm_bridge_add(&platform->bridge);
>  
> -	ret = anx7625_attach_dsi(platform);
> -	if (ret) {
> -		DRM_DEV_ERROR(dev, "Fail to attach to dsi : %d\n", ret);
> -		goto unregister_bridge;
> +	if (!platform->pdata.is_dpi) {
> +		ret = anx7625_attach_dsi(platform);
> +		if (ret) {
> +			DRM_DEV_ERROR(dev, "Fail to attach to dsi : %d\n", ret);
> +			goto unregister_bridge;
> +		}
>  	}
>  
>  	DRM_DEV_DEBUG_DRIVER(dev, "probe done\n");
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.h b/drivers/gpu/drm/bridge/analogix/anx7625.h
> index 6dcf64c703f9..3ef1d8f4e575 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.h
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.h
> @@ -141,12 +141,20 @@
>  #define  HORIZONTAL_BACK_PORCH_H      0x22  /* Bit[7:4] are reserved */
>  
>  /******** END of I2C Address 0x72 *********/
> +
> +/***************************************************************/
> +/* Register definition of device address 0x7a */
> +#define DP_TX_SWING_REG_CNT		0x14
> +#define DP_TX_LANE0_SWING_REG0		0x00
> +#define DP_TX_LANE1_SWING_REG0		0x14
> +/******** END of I2C Address 0x7a *********/
> +
>  /***************************************************************/
>  /* Register definition of device address 0x7e */
>  
>  #define  I2C_ADDR_7E_FLASH_CONTROLLER  0x7E
>  
> -#define FLASH_LOAD_STA 0x05
> +#define FLASH_LOAD_STA          0x05
>  #define FLASH_LOAD_STA_CHK	BIT(7)
>  
>  #define  XTAL_FRQ_SEL    0x3F
> @@ -349,12 +357,20 @@ struct s_edid_data {
>  
>  /***************** Display End *****************/
>  
> +#define MAX_LANES_SUPPORT	4
> +
>  struct anx7625_platform_data {
>  	struct gpio_desc *gpio_p_on;
>  	struct gpio_desc *gpio_reset;
>  	struct regulator_bulk_data supplies[3];
>  	struct drm_bridge *panel_bridge;
>  	int intp_irq;
> +	int is_dpi;
> +	int mipi_lanes;
> +	int dp_lane0_swing_reg_cnt;
> +	int lane0_reg_data[DP_TX_SWING_REG_CNT];
> +	int dp_lane1_swing_reg_cnt;
> +	int lane1_reg_data[DP_TX_SWING_REG_CNT];
>  	u32 low_power_mode;
>  	struct device_node *mipi_host_node;
>  };

-- 
Regards,

Laurent Pinchart