Re: [PATCH net-next v3 4/5] ice: dpll: implement phase related callbacks

[Jiri Pirko <jiri@resnulli.us>]

Fri, Oct 06, 2023 at 02:33:34PM CEST, horms@kernel.org 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@intel.com>
>
>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;
>
>...