Fri, Oct 06, 2023 at 02:33:34PM CEST, horms@xxxxxxxxxx wrote:
>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
Yeah, the non-sense static function docs and how buggy they are. Nobody
reads them anyway. Same old story for ice I guess....
>
>> + * @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;
>
>...
