Re: [PATCH net-next 1/2] dpll: add phase-adjust-gran pin attribute

[Ivan Vecera <ivecera@redhat.com>]

On 10/29/25 11:20 AM, Vadim Fedorenko wrote:
> On 24/10/2025 15:49, Ivan Vecera wrote:
>> Phase-adjust values are currently limited by a min-max range. Some
>> hardware requires, for certain pin types, that values be multiples of
>> a specific granularity, as in the zl3073x driver.
>>
>> Add a `phase-adjust-gran` pin attribute and an appropriate field in
>> dpll_pin_properties. If set by the driver, use its value to validate
>> user-provided phase-adjust values.
>>
>> Reviewed-by: Michal Schmidt <mschmidt@redhat.com>
>> Reviewed-by: Petr Oros <poros@redhat.com>
>> Tested-by: Prathosh Satish <Prathosh.Satish@microchip.com>
>> Signed-off-by: Ivan Vecera <ivecera@redhat.com>
>> ---
>>   Documentation/driver-api/dpll.rst     | 36 +++++++++++++++------------
>>   Documentation/netlink/specs/dpll.yaml |  7 ++++++
>>   drivers/dpll/dpll_netlink.c           | 12 ++++++++-
>>   include/linux/dpll.h                  |  1 +
>>   include/uapi/linux/dpll.h             |  1 +
>>   5 files changed, 40 insertions(+), 17 deletions(-)
>>
>> @@ -1261,7 +1265,13 @@ dpll_pin_phase_adj_set(struct dpll_pin *pin, 
>> struct nlattr *phase_adj_attr,
>>       if (phase_adj > pin->prop.phase_range.max ||
>>           phase_adj < pin->prop.phase_range.min) {
>>           NL_SET_ERR_MSG_ATTR(extack, phase_adj_attr,
>> -                    "phase adjust value not supported");
>> +                    "phase adjust value of out range");
>> +        return -EINVAL;
>> +    }
>> +    if (pin->prop.phase_gran && phase_adj % pin->prop.phase_gran) {
>> +        NL_SET_ERR_MSG_ATTR_FMT(extack, phase_adj_attr,
>> +                    "phase adjust value not multiple of %u",
>> +                    pin->prop.phase_gran);
> 
> That is pretty strict on the uAPI input. Maybe it's better to allow any
> value, but report back the one that is really applied based on driver's
> understanding of hardware? I mean the driver is doing some math before
> applying the math, it can potentially round any value to the closest
> acceptable and report it back?

I’d prefer to use the same approach as for phase-adjust-{min,max}.
Because we could treat them the same way - the user sets a value
above/below the max/min, and the driver clamps it.

Would it be better? I don't think so.

Let’s say the granularity is 1000, and the user sets a value of 499...
then the driver rounds it and sets it to 0. The user then reads the
current value via pin-get and finds that it hasn’t changed - quite
confusing, I’d say. If the user knows the granularity in advance,
they can adjust accordingly.

IMHO, strict behavior is better than smart behavior.

Ivan