Message-ID: <6ae67215-28b6-04bf-b61d-1ba99ffdf6fb@linaro.org>
Date:   Sun, 12 Mar 2023 03:10:10 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH] drm/bridge: adv7533: Fix adv7533_mode_valid for adv7533
 and adv7535
Content-Language: en-GB
To:     Adam Ford <aford173@gmail.com>, dri-devel@lists.freedesktop.org
Cc:     aford@beaconembedded.com, quic_abhinavk@quicinc.com,
        Andrzej Hajda <andrzej.hajda@intel.com>,
        Neil Armstrong <neil.armstrong@linaro.org>,
        Robert Foss <rfoss@kernel.org>,
        Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
        Jonas Karlman <jonas@kwiboo.se>,
        Jernej Skrabec <jernej.skrabec@gmail.com>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>, linux-kernel@vger.kernel.org
References: <20230311231007.46174-1-aford173@gmail.com>
From:   Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
In-Reply-To: <20230311231007.46174-1-aford173@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 12/03/2023 01:10, Adam Ford wrote:
> When dynamically switching lanes was removed, the intent of the code
> was to check to make sure that higher speed items used 4 lanes, but
> it had the unintended consequence of removing the slower speeds for
> 4-lane users.
> 
> This attempts to remedy this by doing a check to see that the
> max frequency doesn't exceed the chip limit, and a second
> check to make sure that the max bit-rate doesn't exceed the
> number of lanes * max bit rate / lane.
> 
> Fixes: 9a0cdcd6649b ("drm/bridge: adv7533: remove dynamic lane switching from adv7533 bridge")
> 

Please remove the empty line here. There should be no empty lines 
between the tags


> Signed-off-by: Adam Ford <aford173@gmail.com>
> 
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7533.c b/drivers/gpu/drm/bridge/adv7511/adv7533.c
> index fdfeadcefe80..10a112d36945 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7533.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7533.c
> @@ -103,13 +103,9 @@ void adv7533_dsi_power_off(struct adv7511 *adv)
>   enum drm_mode_status adv7533_mode_valid(struct adv7511 *adv,
>   					const struct drm_display_mode *mode)
>   {
> -	int lanes;
> +	unsigned long max_lane_freq;
>   	struct mipi_dsi_device *dsi = adv->dsi;
> -
> -	if (mode->clock > 80000)
> -		lanes = 4;
> -	else
> -		lanes = 3;
> +	u8 bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
>   
>   	/*
>   	 * TODO: add support for dynamic switching of lanes

Drop the comment please. It makes little sense with your new implementation.

> @@ -117,8 +113,16 @@ enum drm_mode_status adv7533_mode_valid(struct adv7511 *adv,
>   	 * out the modes which shall need different number of lanes
>   	 * than what was configured in the device tree.
>   	 */
> -	if (lanes != dsi->lanes)
> -		return MODE_BAD;
> +
> +	/* Check max clock for either 7533 or 7535 */
> +	if (mode->clock > (adv->type == ADV7533 ? 80000 : 148500))
> +		return MODE_CLOCK_HIGH;
> +
> +	/* Check max clock for each lane */
> +	max_lane_freq = (adv->type == ADV7533 ? 800000 : 891000);
> +
> +	if (mode->clock * bpp > max_lane_freq * adv->num_dsi_lanes)
> +		return MODE_CLOCK_HIGH;
>   
>   	return MODE_OK;
>   }

-- 
With best wishes
Dmitry