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 <[email protected]>
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?
...
