Re: [PATCH v2 4/7] usb: dwc3: Program GFLADJ

[Felipe Balbi <balbi@kernel.org>]

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