Re: [PATCH] clk: vc5: Abort clock configuration without upstream clock

[Marek Vasut <marek.vasut@gmail.com>]

On 12/16/2018 08:19 AM, Laurent Pinchart wrote:
> Hi Marek,
> 
> Thank you for the patch.
> 
> On Saturday, 15 December 2018 02:55:19 EET Marek Vasut wrote:
>> In case the upstream clock are not set, which can happen in case the
>> VC5 has no valid upstream clock, the $src variable is used uninited
>> by regmap_update_bits(). Check for this condition and return -EINVAL
>> in such case.
> 
> Note that the probe() function will fail in this case, so vc5_mux_set_parent() 
> won't be reached.
> 
>> Note that in case the VC5 has no valid upstream clock, the VC5 can
>> not operate correctly. That is a hardware property of the VC5. The
>> internal oscilator present in some VC5 models is also considered
>> upstream clock.
>>
>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>> Cc: Alexey Firago <alexey_firago@mentor.com>
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Cc: Stephen Boyd <sboyd@kernel.org>
>> Cc: linux-renesas-soc@vger.kernel.org
>> ---
>> NOTE: This is an updated version of:
>>       https://patchwork.kernel.org/patch/10731699/
>> ---
>>  drivers/clk/clk-versaclock5.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/clk-versaclock5.c b/drivers/clk/clk-versaclock5.c
>> index 5b393e711e94..b10801506518 100644
>> --- a/drivers/clk/clk-versaclock5.c
>> +++ b/drivers/clk/clk-versaclock5.c
>> @@ -262,8 +262,10 @@ static int vc5_mux_set_parent(struct clk_hw *hw, u8
>> index)
>>
>>  		if (vc5->clk_mux_ins == VC5_MUX_IN_XIN)
>>  			src = VC5_PRIM_SRC_SHDN_EN_XTAL;
>> -		if (vc5->clk_mux_ins == VC5_MUX_IN_CLKIN)
>> +		else if (vc5->clk_mux_ins == VC5_MUX_IN_CLKIN)
>>  			src = VC5_PRIM_SRC_SHDN_EN_CLKIN;
>> +		else
>> +			return -EINVAL;
>>  	}
> 
> I'd rather go for Stephen's approach if the goal is just to silence a warning 
> for a condition that can't happen in practice.

Sure, probe will fail, but it's safer to handle the possibility that
probe() is broken and this code is reached by properly handling the
failure instead of doing something obviously wrong (like configuring the
hardware with value 0).

-- 
Best regards,
Marek Vasut