Re: [PATCH] clk: vc5: Add support for IDT VersaClock 5P49V5923B

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

On 01/10/2017 01:44 AM, Stephen Boyd wrote:
> On 01/10, Marek Vasut wrote:
>> On 01/10/2017 01:23 AM, Stephen Boyd wrote:
>>> On 01/05, Marek Vasut wrote:
>>>> On 01/05/2017 03:13 PM, Laurent Pinchart wrote:
>>>>> Hi Marek,
>>>>
>>>> Hi!
>>>>
>>>> [...]
>>>>
>>>>>>>>> +static unsigned long vc5_mux_recalc_rate(struct clk_hw *hw,
>>>>>>>>> +                                        unsigned long parent_rate)
>>>>>>>>> +{
>>>>>>>>> +       struct vc5_driver_data *vc5 =
>>>>>>>>> +               container_of(hw, struct vc5_driver_data, clk_mux);
>>>>>>>>> +       unsigned long idiv;
>>>>>>>>> +       u8 div;
>>>>>>>>> +
>>>>>>>>> +       /* FIXME: Needs locking ? */
>>>>>>>
>>>>>>> Let's fix it then :-)
>>>>>>
>>>>>> I would like to get feedback on this one, does it ?
>>>>>
>>>>> That's a question for Mike or Stephen I believe.
>>>>
>>>> OK
>>>
>>> What's the question?
>>
>> Whether or not I need a lock around the code in vc5_mux_recalc_rate(),
>> since I am also tweaking the predivider bits there to assure the
>> (downstream) PLL supplied from the mux always gets clock in range it can
>> handle. This tweaking is mostly inspired by clk-si5351.c driver.
> 
> Don't rely on locking in the core for register locking within a
> device. That makes a dependency headache if we want to rework the
> locking scheme in the core, which we want to do eventually.
> 
> Also, please don't cause side effects during recalc_rate(). The
> callback is meant to calculate the rate of the clock, not adjust
> hardware based on what arguments are passed to it.

Got it and fixed. I'll send V3 now.

-- 
Best regards,
Marek Vasut