Re: [PATCH net-next] dpll: zl3073x: Allow to use custom phase measure averaging factor

[Ivan Vecera <ivecera@redhat.com>]

On 10. 09. 25 7:06 odp., Andrew Lunn wrote:
> On Wed, Sep 10, 2025 at 06:50:47PM +0200, Ivan Vecera wrote:
>> On 10. 09. 25 6:13 odp., Andrew Lunn wrote:
>>> On Wed, Sep 10, 2025 at 12:32:21PM +0200, Ivan Vecera wrote:
>>>> The DPLL phase measurement block uses an exponential moving average,
>>>> calculated using the following equation:
>>>>
>>>>                          2^N - 1                1
>>>> curr_avg = prev_avg * --------- + new_val * -----
>>>>                            2^N                 2^N
>>>>
>>>> Where curr_avg is phase offset reported by the firmware to the driver,
>>>> prev_avg is previous averaged value and new_val is currently measured
>>>> value for particular reference.
>>>>
>>>> New measurements are taken approximately 40 Hz or at the frequency of
>>>> the reference (whichever is lower).
>>>>
>>>> The driver currently uses the averaging factor N=2 which prioritizes
>>>> a fast response time to track dynamic changes in the phase. But for
>>>> applications requiring a very stable and precise reading of the average
>>>> phase offset, and where rapid changes are not expected, a higher factor
>>>> would be appropriate.
>>>>
>>>> Add devlink device parameter phase_offset_avg_factor to allow a user
>>>> set tune the averaging factor via devlink interface.
>>>>
>>>> Tested-by: Prathosh Satish <Prathosh.Satish@microchip.com>
>>>> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
>>>> ---
>>>>    Documentation/networking/devlink/zl3073x.rst |  4 ++
>>>>    drivers/dpll/zl3073x/core.c                  |  6 +-
>>>>    drivers/dpll/zl3073x/core.h                  |  8 ++-
>>>>    drivers/dpll/zl3073x/devlink.c               | 67 ++++++++++++++++++++
>>>>    4 files changed, 82 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/Documentation/networking/devlink/zl3073x.rst b/Documentation/networking/devlink/zl3073x.rst
>>>> index 4b6cfaf386433..ddd159e39e616 100644
>>>> --- a/Documentation/networking/devlink/zl3073x.rst
>>>> +++ b/Documentation/networking/devlink/zl3073x.rst
>>>> @@ -20,6 +20,10 @@ Parameters
>>>>         - driverinit
>>>>         - Set the clock ID that is used by the driver for registering DPLL devices
>>>>           and pins.
>>>> +   * - ``phase_offset_avg_factor``
>>>> +     - runtime
>>>> +     - Set the factor for the exponential moving average used by DPLL phase
>>>> +       measurement block. The value has to be in range <0, 15>.
>>>
>>> Maybe put the text in the commit message here as well?
>>
>> Do you mean to put the equation and details from commit message here?
>> This is pretty long.
> 
> So what if it is long? At the moment, it is hiding in the commit
> message. It is not easy to find, you effectively need to be a kernel
> developer to find it. If it is in the documentation of the device, it
> will be much easier to find and understand what this knob actually
> does.
> 

Agree. I will describe it in more detail in the documentation.
+ optimization proposed by Vadim.

Thanks for the review.

Ivan