Re: [RFC PATCH v7 5/8] ice: implement dpll interface to control cgu

Thu, May 18, 2023 at 06:07:33PM CEST, arkadiusz.kubalewski@intel.com wrote:

quoted

From: Jiri Pirko <jiri@resnulli.us>
Sent: Tuesday, May 16, 2023 1:47 PM

Tue, May 16, 2023 at 11:22:37AM CEST, arkadiusz.kubalewski@intel.com wrote:

quoted

quoted

From: Jiri Pirko <jiri@resnulli.us>
Sent: Monday, May 15, 2023 7:13 PM

Fri, Apr 28, 2023 at 02:20:06AM CEST, vadfed@meta.com wrote:

[...]

quoted

+static const enum dpll_lock_status
+ice_dpll_status[__DPLL_LOCK_STATUS_MAX] = {
+	[ICE_CGU_STATE_INVALID] = DPLL_LOCK_STATUS_UNSPEC,
+	[ICE_CGU_STATE_FREERUN] = DPLL_LOCK_STATUS_UNLOCKED,
+	[ICE_CGU_STATE_LOCKED] = DPLL_LOCK_STATUS_CALIBRATING,

This is a bit confusing to me. You are locked, yet you report
calibrating? Wouldn't it be better to have:
DPLL_LOCK_STATUS_LOCKED
DPLL_LOCK_STATUS_LOCKED_HO_ACQ

?

Sure makes sense, will add this state.

Do you need "calibrating" then? I mean, the docs says:
 ``LOCK_STATUS_CALIBRATING``   dpll device calibrates to lock to the
                               source pin signal

Yet you do: [ICE_CGU_STATE_LOCKED] = DPLL_LOCK_STATUS_CALIBRATING
Seems like you should have:
[ICE_CGU_STATE_LOCKED] = DPLL_LOCK_STATUS_LOCKED
[ICE_CGU_STATE_LOCKED_HO_ACQ] = DPLL_LOCK_STATUS_LOCKED_HO_ACQ,

and remove DPLL_LOCK_STATUS_CALIBRATING as it would be unused?

Also, as a sidenote, could you use the whole names of enum value names
in documentation? Simple reason, greppability.

Yes, removed CALIBRATING.
Fixed the docs.

Cool, thanks!

Thank you!
Arkadiusz

quoted

Thanks!

quoted

quoted

quoted

+	[ICE_CGU_STATE_LOCKED_HO_ACQ] = DPLL_LOCK_STATUS_LOCKED,
+	[ICE_CGU_STATE_HOLDOVER] = DPLL_LOCK_STATUS_HOLDOVER,
+};

[...]