Received-SPF: pass (google.com: domain of linux-kernel+bounces-62195-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <4efca2ec-b078-4421-8956-e3add15d8f62@collabora.com>
Date: Mon, 12 Feb 2024 20:36:03 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 2/2] phy: rockchip: Add Samsung HDMI/eDP Combo PHY
 driver
Content-Language: en-US
To: Luis de Arquer <ldearquer@gmail.com>, Rob Herring <robh+dt@kernel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
 Conor Dooley <conor+dt@kernel.org>, Heiko Stuebner <heiko@sntech.de>,
 Johan Jonker <jbx6244@gmail.com>,
 Sebastian Reichel <sebastian.reichel@collabora.com>,
 Sascha Hauer <s.hauer@pengutronix.de>, Andy Yan <andy.yan@rock-chips.com>,
 Vinod Koul <vkoul@kernel.org>, Kishon Vijay Abraham I <kishon@kernel.org>,
 Philipp Zabel <p.zabel@pengutronix.de>, Algea Cao <algea.cao@rock-chips.com>
Cc: devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
 linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org,
 linux-phy@lists.infradead.org, kernel@collabora.com
References: <20240205-phy-hdptx-v2-0-a7150814c047@collabora.com>
 <20240205-phy-hdptx-v2-2-a7150814c047@collabora.com>
 <61af7b121b23fe8ed06df5348692f862b9b125bf.camel@gmail.com>
From: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
In-Reply-To: <61af7b121b23fe8ed06df5348692f862b9b125bf.camel@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

Hi Luis,

On 2/12/24 18:44, Luis de Arquer wrote:
> Hi Cristian,
> 
> On Mon, 2024-02-05 at 13:24 +0200, Cristian Ciocaltea wrote:
>> +
>> +static bool hdptx_phy_clk_pll_calc(unsigned int data_rate,
>> +				   struct ropll_config *cfg)
>> +{
>> +	const unsigned int fout = data_rate / 2, fref = 24000;
>> +	unsigned long k = 0, lc, k_sub, lc_sub;
>> +	unsigned int fvco, sdc;
>> +	u32 mdiv, sdiv, n = 8;
>> +
>> +	for (sdiv = 16; sdiv >= 1; sdiv--) {
>> +		if (sdiv % 2 && sdiv != 1)
>> +			continue;
>> +
>> +		fvco = fout * sdiv;
>> +
>> +		if (fvco < 2000000 || fvco > 4000000)
>> +			continue;
>> +
> 
> What about adding a check to data_rate, maybe like
> 
> if (fout > 0x0FFFFFFF)
> 	return false;
> 
> or similar, before the for loop, to keep the multiplication safe?
> 
> Right now it would be redundant, given that data_rate was, at some
> point, encoded in 28 bits within bus_width. But can prevent future pain,
> especially after changing to phy_configure_opts_hdmi.

Indeed, it makes sense to do the check, thanks for the heads up!
I was almost ready to submit v3, so this arrived just in time. :-)

Regards,
Cristian