Re: [PATCH net-next 00/11] Create common DPLL configuration API

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

On 21.07.2023 12:14, Jiri Pirko wrote:
> There are couple of issues that came up during our internal ci run:
> 
> 10:16:04  error: drivers/dpll/dpll_netlink.c:452:5: error: no previous prototype for '__dpll_device_change_ntf' [-Werror=missing-prototypes]
> 10:16:04  error: drivers/dpll/dpll_netlink.c:1283:13: error: no previous prototype for 'dpll_netlink_fini' [-Werror=missing-prototypes]
> 10:16:04  error: drivers/dpll/dpll_core.c:221:1: error: no previous prototype for 'dpll_xa_ref_dpll_find' [-Werror=missing-prototypes]
> 
> 10:27:31  error: drivers/dpll/dpll_core.c:220:21: warning: symbol 'dpll_xa_ref_dpll_find' was not declared. Should it be static?
> 10:27:31  error: drivers/dpll/dpll_netlink.c:452:5: warning: symbol '__dpll_device_change_ntf' was not declared. Should it be static?
> 10:27:31  error: drivers/dpll/dpll_netlink.c:1283:13: warning: symbol 'dpll_netlink_fini' was not declared. Should it be static?
> 10:27:41  error: drivers/net/ethernet/intel/ice/ice_dpll.c:461:3: error: a label can only be part of a statement and a declaration is not a statement
> 
> I believe that you didn't run make with C=2, otherwise you would hit
> these.

Yeah, I'll re-run the set patch-by-patch with C=2 next time.

> 
> Checkpatch issue:
> 10:29:30  CHECK: struct mutex definition without comment
> 10:29:30  #6581: FILE: drivers/net/ethernet/intel/ice/ice_dpll.h:85:
> 10:29:30  +	struct mutex lock;

Arkadiusz will take care of "ice" part.


> Spelling errors:
> 10:45:08  error: Documentation/netlink/specs/dpll.yaml:165: prority ==> priority
> 10:45:08  error: include/uapi/linux/dpll.h:128: prority ==> priority
> 10:45:08  error: drivers/net/ethernet/intel/ice/ice_dpll.c:2008: userpsace ==> userspace
> 10:45:08  error: drivers/net/ethernet/intel/ice/ice_dpll.h:20: properities ==> properties
> 

Will fix it.

> 
> Thu, Jul 20, 2023 at 11:18:52AM CEST, vadim.fedorenko@linux.dev wrote:
>> Implement common API for clock/DPLL configuration and status reporting.
>> The API utilises netlink interface as transport for commands and event
>> notifications. This API aim to extend current pin configuration and
> 
> s/API aim/API aims/
> 
> 
>> make it flexible and easy to cover special configurations.
> 
> I don't follow. How this could "aim to extend current pin configuration" ?
> This is a new thing. Could you re-phrase?

Not really new. PTP devices have already simple pin configurations, mlx5 is 
using it for some cards with external pins. The problem is that PTP subsystem
covers only simple configuration of the pin and doesn't cover DPLL part at all.

> 
> What's "special configuration"? Sounds odd.
> 

Yeah, "complex configurations" sounds better, will change it.

> 
>>
>> Netlink interface is based on ynl spec, it allows use of in-kernel
>> tools/net/ynl/cli.py application to control the interface with properly
>> formated command and json attribute strings. Here are few command
>> examples of how it works with `ice` driver on supported NIC:
>>
>> - dump dpll devices
>> $# ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml \
> 
> "$#" looks a bit odd. Just "$" with "sudo" when you want to emphasize
> root is needed to perform the command.
> 
> 
>> --dump device-get
>> [{'clock-id': 282574471561216,
>>   'id': 0,
>>   'lock-status': 'unlocked',
>>   'mode': 'automatic',
>>   'module-name': 'ice',
>>   'type': 'eec'},
>> {'clock-id': 282574471561216,
>>   'id': 1,
>>   'lock-status': 'unlocked',
>>   'mode': 'automatic',
>>   'module-name': 'ice',
>>   'type': 'pps'}]
>>
>> - get single pin info:
>> $# ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml \
>> --do pin-get --json '{"pin-id":2}'
>> {'clock-id': 282574471561216,
>> 'module-name': 'ice',
>> 'pin-board-label': 'C827_0-RCLKA',
>> 'pin-dpll-caps': 6,
>> 'pin-frequency': 1953125,
>> 'pin-id': 2,
>> 'pin-parenti-device': [{'id': 0,
> 
> This looks like manual edit went wrong :)
> s/parenti/parent/
> 

Ahhh... yeah :)

> 
>>                          'pin-direction': 'input',
>>                          'pin-prio': 11,
>>                          'pin-state': 'selectable'},
>>                         {'id': 1,
>>                          'pin-direction': 'input',
>>                          'pin-prio': 9,
>>                          'pin-state': 'selectable'}],
>> 'pin-type': 'mux'}
>>
>> - set pin's state on dpll:
>> $# ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml \
>> --do pin-set --json '{"pin-id":2, "pin-parent-device":{"id":1, "pin-state":2}}'
>>
>> - set pin's prio on dpll:
>> $# ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml \
>> --do pin-set --json '{"pin-id":2, "pin-parent-device":{"id":1, "pin-prio":4}}'
>>
>> - set pin's state on parent pin:
>> $# ./tools/net/ynl/cli.py --spec Documentation/netlink/specs/dpll.yaml \
>> --do pin-set --json '{"pin-id":13, \
>>                       "pin-parent-pin":{"pin-id":2, "pin-state":1}}'
>>
> 
> [...]