Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
References: <20220119002438.106079-1-sean.anderson@seco.com>
 <20220119002438.106079-5-sean.anderson@seco.com>
 <4696c5a4-5921-f7cb-196c-5ad956e696f9@synopsys.com>
 <f528aeb5-6155-a75e-9d35-9bf473e0bbc7@seco.com>
 <3ca6fb9e-94cf-6483-26a6-ae2682d1f55e@synopsys.com>
 <2831e216-737f-41b2-1aa0-4af56615e3b7@seco.com>
 <524027b7-1361-7cc2-842f-7c20cd4cb016@synopsys.com>
User-agent: mu4e 1.6.10; emacs 28.0.90
From:   Felipe Balbi <balbi@kernel.org>
To:     Thinh Nguyen <Thinh.Nguyen@synopsys.com>
Cc:     Sean Anderson <sean.anderson@seco.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        Balaji Prakash J <bjagadee@codeaurora.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Robert Hancock <robert.hancock@calian.com>,
        Baruch Siach <baruch@tkos.co.il>
Subject: Re: [PATCH v2 4/7] usb: dwc3: Program GFLADJ
Date:   Wed, 26 Jan 2022 12:56:04 +0200
In-reply-to: <524027b7-1361-7cc2-842f-7c20cd4cb016@synopsys.com>
Message-ID: <87ilu67okj.fsf@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk


Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>>> On 1/24/22 5:46 PM, Thinh Nguyen wrote:
>>>>> Sean Anderson wrote:
>>>>>> GUCTL.REFCLKPER can only account for clock frequencies with integer
>>>>>> periods. To address this, program REFCLK_FLADJ with the relative error
>>>>>> caused by period truncation. The formula given in the register reference
>>>>>> has been rearranged to allow calculation based on rate (instead of
>>>>>> period), and to allow for fixed-point arithmetic.
>>>>>>
>>>>>> Additionally, calculate a value for 240MHZDECR. This configures a
>>>>>> simulated 240Mhz clock using a counter with one fractional bit (PLS1).
>>>>>>
>>>>>> This register is programmed only for versions >= 2.50a, since this is
>>>>>> the check also used by commit db2be4e9e30c ("usb: dwc3: Add frame length
>>>>>> adjustment quirk").
>>>>>>
>>>>>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>>>>>> ---
>>>>>>
>>>>>> Changes in v2:
>>>>>> - Also program GFLADJ.240MHZDECR
>>>>>> - Don't program GFLADJ if the version is < 2.50a
>>>>>>
>>>>>>  drivers/usb/dwc3/core.c | 37 +++++++++++++++++++++++++++++++++++--
>>>>>>  drivers/usb/dwc3/core.h |  3 +++
>>>>>>  2 files changed, 38 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>>> index 5214daceda86..883e119377f0 100644
>>>>>> --- a/drivers/usb/dwc3/core.c
>>>>>> +++ b/drivers/usb/dwc3/core.c
>>>>>> @@ -348,7 +348,7 @@ static void dwc3_frame_length_adjustment(struct dwc3 *dwc)
>>>>>>  static void dwc3_ref_clk_period(struct dwc3 *dwc)
>>>>>>  {
>>>>>>  	u32 reg;
>>>>>> -	unsigned long rate, period;
>>>>>> +	unsigned long decr, fladj, rate, period;
>>>>>
>>>>> Minor style nit: Felipe prefers to keep the declaration on separate
>>>>> lines, let's keep it consistent with the rest in this driver.
>>>>
>>>> So 
>>>>
>>>> unsigned int decr;
>>>> unsigned int fladj;
>>>> unsigned int rate;
>>>> unsigned int period;
>>>>
>>>> ?
>>>>
>>>> Frankly that seems rather verbose.
>>>
>>> A couple of the benefits of having it like this is to help with viewing
>>> git-blame if we introduce new variables and help with backporting fix
>>> patch a bit simpler. Mainly I'm just following Felipe's style and keep
>>> it consistent in this driver, but I don't think it's a big deal.
>> 
>> *shrug*
>> 
>> If it's the subsystem style I will rewrite it.
>> 
>
> Felipe also prefers Reverse Christmas Tree style.

that's also part of the coding style and probably documented somewhere

-- 
balbi