On Fri, Oct 06, 2023 at 01:41:00PM +0200, Arkadiusz Kubalewski wrote:
> Implement new callback ops related to measurment and adjustment of
> signal phase for pin-dpll in ice driver.
>
> Signed-off-by: Arkadiusz Kubalewski <arkadiusz.kubalewski@xxxxxxxxx>
Hi Arkadiusz,
some minor feedback from my side.
If you do end up re-spinning the series, please consider
running checkpatch.pl --codespell.
> ---
> drivers/net/ethernet/intel/ice/ice_dpll.c | 224 +++++++++++++++++++++-
> drivers/net/ethernet/intel/ice/ice_dpll.h | 10 +-
> 2 files changed, 230 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ice/ice_dpll.c b/drivers/net/ethernet/intel/ice/ice_dpll.c
...
> +/**
> + * ice_dpll_phase_offset_get - callback for get dpll phase shift value
> + * @pin: pointer to a pin
> + * @pin_priv: private data pointer passed on pin registration
> + * @dpll: registered dpll pointer
> + * @dpll_priv: private data pointer passed on dpll registration
> + * @phase_adjust: on success holds pin phase_adjust value
nit: The parameter is called phase_offset, not phase_adjust in the code below
> + * @extack: error reporting
> + *
> + * Dpll subsystem callback. Handler for getting phase shift value between
> + * dpll's input and output.
> + *
> + * Context: Acquires pf->dplls.lock
> + * Return:
> + * * 0 - success
> + * * negative - error
> + */
> +static int
> +ice_dpll_phase_offset_get(const struct dpll_pin *pin, void *pin_priv,
> + const struct dpll_device *dpll, void *dpll_priv,
> + s64 *phase_offset, struct netlink_ext_ack *extack)
> +{
> + struct ice_dpll *d = dpll_priv;
> + struct ice_pf *pf = d->pf;
> +
> + mutex_lock(&pf->dplls.lock);
> + if (d->active_input == pin)
> + *phase_offset = d->phase_offset * ICE_DPLL_PHASE_OFFSET_FACTOR;
> + else
> + *phase_offset = 0;
> + mutex_unlock(&pf->dplls.lock);
> +
> + return 0;
> +}
> +
> /**
> * ice_dpll_rclk_state_on_pin_set - set a state on rclk pin
> * @pin: pointer to a pin
...
> @@ -1656,6 +1867,15 @@ ice_dpll_init_info_direct_pins(struct ice_pf *pf,
> return ret;
> pins[i].prop.capabilities |=
> DPLL_PIN_CAPABILITIES_PRIORITY_CAN_CHANGE;
> + pins[i].prop.phase_range.min =
> + pf->dplls.input_phase_adj_max;
> + pins[i].prop.phase_range.max =
> + -pf->dplls.input_phase_adj_max;
> + } else {
> + pins[i].prop.phase_range.min =
> + pf->dplls.output_phase_adj_max,
nit: It probably doesn't make any difference, but perhaps ',' should be ';'.
As flagged by clang-16 with -Wcomma
> + pins[i].prop.phase_range.max =
> + -pf->dplls.output_phase_adj_max;
> }
> pins[i].prop.capabilities |=
> DPLL_PIN_CAPABILITIES_STATE_CAN_CHANGE;
...
