From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f67.google.com ([74.125.82.67]:36247 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750853AbdAJSBl (ORCPT ); Tue, 10 Jan 2017 13:01:41 -0500 Subject: Re: [PATCH] clk: vc5: Add support for IDT VersaClock 5P49V5923B To: Stephen Boyd References: <20161228000045.4540-1-marek.vasut@gmail.com> <2015482.2Xx0FSfo4q@avalon> <7398143.JfNZUDgVyk@avalon> <69c22af5-717c-ce9d-5f4c-c1751894b885@gmail.com> <20170110002327.GI17126@codeaurora.org> <71b654d2-e24e-f09b-ca61-03dd7a27a24b@gmail.com> <20170110004427.GO17126@codeaurora.org> Cc: Laurent Pinchart , Geert Uytterhoeven , linux-clk , Michael Turquette , Linux-Renesas From: Marek Vasut Message-ID: Date: Tue, 10 Jan 2017 18:42:25 +0100 MIME-Version: 1.0 In-Reply-To: <20170110004427.GO17126@codeaurora.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-renesas-soc-owner@vger.kernel.org List-ID: 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