Re: [PATCH] clk: zynqmp: fix compile testing without ZYNQMP_FIRMWARE

From: Michal Simek <michal.simek@xilinx.com>
To: Stephen Boyd <sboyd@kernel.org>,
	Michal Simek <michal.simek@xilinx.com>,
	Punit Agrawal <punit1.agrawal@toshiba.co.jp>
Cc: Arnd Bergmann <arnd@kernel.org>,
	Michael Turquette <mturquette@baylibre.com>,
	Arnd Bergmann <arnd@arndb.de>, Rajan Vaja <rajan.vaja@xilinx.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Jolly Shah <jolly.shah@xilinx.com>,
	Quanyang Wang <quanyang.wang@windriver.com>,
	<linux-clk@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] clk: zynqmp: fix compile testing without ZYNQMP_FIRMWARE
Date: Mon, 21 Jun 2021 15:15:55 +0200	[thread overview]
Message-ID: <8cc5409f-a8e4-87f9-87f8-79c5467b1faa@xilinx.com> (raw)
In-Reply-To: <161974903429.177949.6659170601321970979@swboyd.mtv.corp.google.com>

On 4/30/21 4:17 AM, Stephen Boyd wrote:
> Quoting Punit Agrawal (2021-04-22 23:37:25)
>> Michal Simek <michal.simek@xilinx.com> writes:
>>>
>>>
>>>>>  
>>>>>     rate =  parent_rate * fbdiv;
>>>>>     if (zynqmp_pll_get_mode(hw) == PLL_MODE_FRAC) {
>>>>
>>>> The changes make sense in that the functions error out sensibly when the
>>>> zynqmp firmware driver is not enabled.
>>>>
>>>> Acked-by: Punit Agrawal <punit1.agrawal@toshiba.co.jp>
>>>
>>> I think code should be checked that these error values are handled how
>>> they should be handled.
>>
>> I only looked at it from the point of view of getting rid of the
>> warnings - based on the commit log, Arnd's patch is only taking care of
>> the compiler warnings when the driver is built with
>> CONFIG_COMPILE_TEST=y and likely CONFIG_ZYNQMP_FIRMWARE=n.
> 
> The subject line basically says this.
> 
>>
>> In practice, the code should not be hit at runtime due to the dependency
>> on the firmware driver. Even then, a better fix would indeed be taking
>> the returned values at call sites into account.
> 
> Still needs to be fixed. If a better patch is sent I would apply it, but
> if that isn't going to happen I'll apply this one.

I have sent v2 version based on what I have found how that error values
should look like. Please take a look at v2.
https://lore.kernel.org/linux-clk/fdee3a286defb103aa07b5493b805d1987885165.1624281224.git.michal.simek@xilinx.com/

Thanks,
Michal