From: Jiri Pirko <jiri@resnulli.us>
Sent: Monday, July 10, 2023 2:10 PM

Mon, Jul 10, 2023 at 12:07:30PM CEST, arkadiusz.kubalewski@intel.com wrote:

quoted

quoted

From: Jiri Pirko <jiri@resnulli.us>
Sent: Wednesday, June 28, 2023 1:16 PM
Wed, Jun 28, 2023 at 11:15:11AM CEST, arkadiusz.kubalewski@intel.com

wrote:

quoted

quoted

quoted

quoted

From: Jiri Pirko <jiri@resnulli.us>
Sent: Tuesday, June 27, 2023 12:18 PM

Fri, Jun 23, 2023 at 02:38:10PM CEST, arkadiusz.kubalewski@intel.com
wrote:

quoted

v8 -> v9:

Could you please address all the unresolved issues from v8 and send v10?
I'm not reviewing this one.

Thanks!

Sure, will do, but first missing to-do/discuss list:
1) remove mode_set as not used by any driver

I have implemented in ice (also added back the DPLL_MODE_FREERUN).

Uh :/ Why exactly is it needed in this initial submission?

Without mode-set there is no need for device-set at all, right?
So it is better to implement at least one set command, so we don't
need remove device-set command entirely.

quoted

quoted

quoted

2) remove "no-added-value" static functions descriptions in
  dpll_core/dpll_netlink

Removed.

quoted

quoted

3) merge patches [ 03/10, 04/10, 05/10 ] into patches that are compiling
  after each patch apply

Hope Vadim will decide on this, the thing is merging in two patches
doesn't make much sense as there won't be any linking until both patches
are there, so most sense it would be if 3 are merged into one, but
then we will be back to one big blob patch issue.

quoted

quoted

4) remove function return values descriptions/lists

Fixed.

quoted

quoted

5) Fix patch [05/10]:
  - status Supported
  - additional maintainers
  - remove callback:
    int (*source_pin_idx_get)(...) from `struct dpll_device_ops`
6) Fix patch [08/10]: rethink ice mutex locking scheme

Fixed.

quoted

quoted

7) Fix patch [09/10]: multiple comments on
https://lore.kernel.org/netdev/ZIQu+%2Fo4J0ZBspVg@nanopsycho/#t (local)
8) add PPS DPLL phase offset to the netlink get-device API

Added few things on this matter
- 1 dpll level attribute:
 - phase-shift - measuring the phase difference between dpll input
   and it's output
- 1 dpll-pin tuple level attribute:
 - pin-phase-adjust - set/get phase adjust of a pin on a dpll
- 2 pin level attributes:
 - pin-phase-adjust-min - provide user with min value that can be set
 - pin-phase-adjust-max - provide user with max value that can be set
- a constant:
 - DPLL_PHASE_SHIFT_DIVIDER similar to DPLL_TEMP_DIVIDER for producing
   fraction value of measured DPLL_A_PHASE_SHIFT

Again, why do we need this in this initial submission? Why it can't be a
follow-up patchset to extend this? This way we never converge :/
Please focus on what we have now and bring it in. Let the extensions to
be addressed later on, please.

Well AFAIK, RHEL is doing some monitoring software, so the end-users need this.

quoted

- implemented in dpll netlink and in ice

quoted

You are missing removal of pin->prop.package_label = dev_name(dev); in
ice.

I didn't touch it, as we still need to discuss it, Jakub didn't respond
on v8 thread.
I don't see why we shall not name it the way. This is most meaningful
label for those pins for the user right now.

This is not meaningful, at all. dev_name() changes upon which pci slot
you plug the card into. package_label should be an actual label on a
silicon package. Why you think this two are related in aby way, makes me
really wonder. Could you elaborate the meaningfulness of this?

Without this, from end-user perspective, it would be very confusing.
As in ice without any label there would 4 pins which differs only with id.
What names would you suggest?

Thank you!
Arkadiusz

quoted

Thank you!
Arkadiusz

quoted

quoted

Thank you!
Arkadiusz