Re: [PATCH v2 1/4] drm/bridge: nwl-dsi: Limit LCDIF specific sync override

"Luca Ceresoli" <[email protected]> writes:

> On Mon, 03 Aug 2026 16:56:19 +0200, Esben Haabendal <[email protected]> wrote:
>
> Hi Esben,
>
>> When using DCSS with NWL, overriding the mode flags to enforce active high
>> sync is preventing the use of active low with downstream bridges, such as
>> ti-sn65dsi83, which will not see such mode flags set by the panel.
>>
>> Signed-off-by: Esben Haabendal <[email protected]>
>
> Is this a fix? If so, it should have Fixes: + Cc: stable.

Yes. I will add tags.

>> diff --git a/drivers/gpu/drm/bridge/nwl-dsi.c b/drivers/gpu/drm/bridge/nwl-dsi.c
>> index 09992529f3d8..0fb10b686d1f 100644
>> --- a/drivers/gpu/drm/bridge/nwl-dsi.c
>> +++ b/drivers/gpu/drm/bridge/nwl-dsi.c
>> @@ -821,10 +823,13 @@ static int nwl_dsi_bridge_atomic_check(struct drm_bridge *bridge,
>>  				       struct drm_connector_state *conn_state)
>>  {
>>  	struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
>> +	struct nwl_dsi *dsi = bridge_to_dsi(bridge);
>>
>>  	/* At least LCDIF + NWL needs active high sync */
>> -	adjusted_mode->flags |= (DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
>> -	adjusted_mode->flags &= ~(DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC);
>> +	if (dsi->endpoint == NWL_DSI_ENDPOINT_LCDIF) {
>> +		adjusted_mode->flags |= (DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
>> +		adjusted_mode->flags &= ~(DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC);
>> +	}
>
> In some sense, this patch is doing two things. One is the hunk above, doing
> what the commit message says...
>
>> @@ -1103,11 +1107,12 @@ static int nwl_dsi_select_input(struct nwl_dsi *dsi)
>>  				      "No valid input endpoint found\n");
>>  			return -EINVAL;
>>  		}
>> +		dsi->endpoint = NWL_DSI_ENDPOINT_DCSS;
>>  	}
>
> ...and this is a somewhat orthogonal change, changing the way the endpoint
> selection is stored: either as a bool (false = LCDIF, true = DCSS) or as an
> index (0 = LCDIF, 1 = DCSS). No problem with this change, I think it makes
> code more readable. Just I'd rather keep it as a separate cleanup
> patch.

I will split it into two patches.

>>  	DRM_DEV_INFO(dsi->dev, "Using %s as input source\n",
>> -		     (use_dcss) ? "DCSS" : "LCDIF");
>> -	ret = mux_control_try_select(dsi->mux, use_dcss);
>> +		     (dsi->endpoint == NWL_DSI_ENDPOINT_DCSS) ? "DCSS" : "LCDIF");
>> +	ret = mux_control_try_select(dsi->mux, dsi->endpoint == NWL_DSI_ENDPOINT_DCSS);
>
> The code here is not formally correct. mux_control_try_select() takes a
> state argument, which is an integer, but you are passing a bool. It would
> still work just because bools are mapped to 0/1. Looks like should simply
> become:
>
> 	ret = mux_control_try_select(dsi->mux, dsi->endpoint);

Fixing, thanks.

/Esben