Re: [PATCH v7 iwl-next 1/6] ice: Remove unncecessary ice_is_e8xx() functions

[Simon Horman <horms@kernel.org>]

On Tue, Aug 20, 2024 at 12:21:48PM +0200, Karol Kolacinski wrote:
> Remove unnecessary ice_is_e8xx() functions and PHY model. Instead, use
> MAC type where applicable.
> 
> Signed-off-by: Karol Kolacinski <karol.kolacinski@intel.com>

Hi Karol,

Sorry for waiting until v7 until raising this. But I feel that this patch
is doing a bit more than what is set out in patch description. So I'd like
to suggest some combination of splitting the patch and enhancing the patch
description.

...

> diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.c b/drivers/net/ethernet/intel/ice/ice_gnss.c
> index f02e8ca55375..dd65b2db9856 100644
> --- a/drivers/net/ethernet/intel/ice/ice_gnss.c
> +++ b/drivers/net/ethernet/intel/ice/ice_gnss.c
> @@ -386,27 +386,22 @@ void ice_gnss_exit(struct ice_pf *pf)
>   */
>  bool ice_gnss_is_gps_present(struct ice_hw *hw)
>  {
> +#if IS_ENABLED(CONFIG_PTP_1588_CLOCK)
> +	int err;
> +	u8 data;
> +
>  	if (!hw->func_caps.ts_func_info.src_tmr_owned)
>  		return false;
>  
>  	if (!ice_is_gps_in_netlist(hw))
>  		return false;
>  
> -#if IS_ENABLED(CONFIG_PTP_1588_CLOCK)
> -	if (ice_is_e810t(hw)) {
> -		int err;
> -		u8 data;
> -
> -		err = ice_read_pca9575_reg(hw, ICE_PCA9575_P0_IN, &data);
> -		if (err || !!(data & ICE_P0_GNSS_PRSNT_N))
> -			return false;
> -	} else {
> +	err = ice_read_pca9575_reg(hw, ICE_PCA9575_P0_IN, &data);
> +	if (err || !!(data & ICE_P0_GNSS_PRSNT_N))
>  		return false;
> -	}
> -#else
> -	if (!ice_is_e810t(hw))
> -		return false;
> -#endif /* IS_ENABLED(CONFIG_PTP_1588_CLOCK) */
>  
>  	return true;
> +#else
> +	return false;
> +#endif /* IS_ENABLED(CONFIG_PTP_1588_CLOCK) */
>  }

I understand that the above relates to the patch description in the sense
that it removes calls to ice_is_e810t(), a function that is removed
entirely by this patch. But the above is not a simple substitution of one
check for E810T for another. Indeed, it seems far more complex than that.

I think that warrants an explanation, e.g. why is it ok to always return
false if CONFIG_PTP_1588_CLOCK. Perhaps a separate patch is appropriate for
this change.

> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c b/drivers/net/ethernet/intel/ice/ice_ptp.c

...

> @@ -2759,7 +2766,7 @@ static void ice_ptp_maybe_trigger_tx_interrupt(struct ice_pf *pf)
>  	bool trigger_oicr = false;
>  	unsigned int i;
>  
> -	if (ice_is_e810(hw))
> +	if (!pf->ptp.port.tx.has_ready_bitmap)
>  		return;
>  
>  	if (!ice_pf_src_tmr_owned(pf))

Likewise, this doesn't really match the patch description.
Sure it is a simple change. And yes, after scanning the code,
I agree that has_ready_bitmap is set other than for E810.
But it is not a check against the MAC type as described in the patch
description.

A separate patch would be nice. Or, if not, an enhanced patch description.


> @@ -2898,14 +2905,12 @@ static int ice_ptp_rebuild_owner(struct ice_pf *pf)
>  	 */
>  	ice_ptp_flush_all_tx_tracker(pf);
>  
> -	if (!ice_is_e810(hw)) {
> -		/* Enable quad interrupts */
> -		err = ice_ptp_cfg_phy_interrupt(pf, true, 1);
> -		if (err)
> -			return err;
> +	/* Enable quad interrupts */
> +	err = ice_ptp_cfg_phy_interrupt(pf, true, 1);
> +	if (err)
> +		return err;
>  
> -		ice_ptp_restart_all_phy(pf);
> -	}
> +	ice_ptp_restart_all_phy(pf);
>  
>  	/* Re-enable all periodic outputs and external timestamp events */
>  	ice_ptp_enable_all_perout(pf);

Here too. Why is it ok to unconditionally enable quad interrupts?

...