Hi Paul, Thank you for your feedback!
On 6/27/2024 5:37 PM, Paul Menzel wrote: > > @@ -1708,6 +1709,15 @@ static int ice_ptp_write_perout(struct ice_hw *hw, > > unsigned int chan, > > /* 0. Reset mode & out_en in AUX_OUT */ > > wr32(hw, GLTSYN_AUX_OUT(chan, tmr_idx), 0); > > > > + if (ice_is_e825c(hw)) { > > + int err; > > + > > + /* Enable/disable CGU 1PPS output for E825C */ > > + err = ice_cgu_ena_pps_out(hw, !!period); > > + if (err) > > + return err; > > + } > > Does only E825C products support this feature? Yes, it's only necessary for E825C, other products don't need or support it. > > +/** > > + * ice_cgu_ena_pps_out - Enable/disable 1PPS output > > + * @hw: pointer to the HW struct > > + * @ena: Enable/disable 1PPS output > > + */ > > +int ice_cgu_ena_pps_out(struct ice_hw *hw, bool ena) > > Is `ena` short for enable? Yes. I guess for the function argument 'enable' would be better. > > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h > > b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h > > index ff98f76969e3..382e84568256 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.h > > +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.h > > @@ -331,6 +331,7 @@ extern const struct ice_vernier_info_e82x > > e822_vernier[NUM_ICE_PTP_LNK_SPD]; > > > > /* Device agnostic functions */ > > u8 ice_get_ptp_src_clock_index(struct ice_hw *hw); > > +int ice_cgu_ena_pps_out(struct ice_hw *hw, bool ena); > > If *ena* means “enable”, I do not like this pattern very much, and I’d > prefer an enable and a disable function. Good point regarding the name, I guess I should use 'cfg' instead of 'ena' in this case to be clear that this function doesn't only enable PPS output. That said, I don't see a point to use two separate functions for a simple enable/disable functionality. From my point of view, ~20 lines for 2 lines of actual code difference would be unnecessarily redundant. Kind regards, Karol