Date:   Tue, 30 Nov 2021 05:13:06 +0200
From:   Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To:     Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Cc:     linux-renesas-soc@vger.kernel.org, dri-devel@lists.freedesktop.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/4] drm: rcar-du: mipi-dsi: Ensure correct fout is
 reported
Message-ID: <YaWWwhadGi8Hx/Ar@pendragon.ideasonboard.com>
References: <20211126101518.938783-1-kieran.bingham+renesas@ideasonboard.com>
 <20211126101518.938783-4-kieran.bingham+renesas@ideasonboard.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20211126101518.938783-4-kieran.bingham+renesas@ideasonboard.com>
Precedence: bulk

Hi Kieran,

Thank you for the patch.

On Fri, Nov 26, 2021 at 10:15:17AM +0000, Kieran Bingham wrote:
> The debug reporting for the clock calculations was erroneously reporting
> the last calculation of fout, rather than the fout that was determined
> to have the least error, and therefore be the values chosen to operate
> with.
> 
> Fix the reporting to show the correct output by storing the determined
> fout, along with the error value.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
> 
> I spent /way/ too long confused why my clock values didn't make sense
> before I noticed this.. :-(

Oops :-S Sorry about that.

>  drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c
> index e94245029f95..833f4480bdf3 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_mipi_dsi.c
> @@ -168,6 +168,7 @@ static int rcar_mipi_dsi_phtw_test(struct rcar_mipi_dsi *dsi, u32 phtw)
>   */
>  
>  struct dsi_setup_info {
> +	unsigned long fout;
>  	unsigned int err;
>  	u16 vco_cntrl;
>  	u16 prop_cntrl;
> @@ -247,6 +248,7 @@ static void rcar_mipi_dsi_parameters_calc(struct rcar_mipi_dsi *dsi,
>  				setup_info->m = m - 2;
>  				setup_info->n = n - 1;
>  				setup_info->err = err;
> +				setup_info->fout = fout;
>  				if (err == 0)
>  					goto done;
>  			}
> @@ -256,7 +258,7 @@ static void rcar_mipi_dsi_parameters_calc(struct rcar_mipi_dsi *dsi,
>  done:
>  	dev_dbg(dsi->dev,
>  		"%pC %lu Hz -> Fout %lu Hz (target %lu Hz, error %d.%02u%%), PLL M/N/DIV %u/%u/%u\n",
> -		clk, fin, fout, fout_target, setup_info->err / 100,
> +		clk, fin, setup_info->fout, fout_target, setup_info->err / 100,

We don't need the fout in the caller, so it could be a local variable
(best_fout for instance). I can however imagine that we the frequency
could become useful in the caller, so

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  		setup_info->err % 100, setup_info->m,
>  		setup_info->n, setup_info->div);
>  	dev_dbg(dsi->dev,

-- 
Regards,

Laurent Pinchart