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
