RE: [PATCH net-next v3 1/2] r8152: add helper functions for PLA/USB OCP registers

[Chih Kai Hsu]

On 24/03/26 2:04 pm, Hayes Wang wrote:
> 
> > @@ -7166,21 +6942,17 @@ static void r8153_init(struct r8152 *tp)
> >  	r8153_u1u2en(tp, true);
> >  	usb_enable_lpm(tp->udev);
> >
> > -	ocp_data = ocp_read_byte(tp, MCU_TYPE_PLA, PLA_CONFIG6);
> > -	ocp_data |= LANWAKE_CLR_EN;
> > -	ocp_write_byte(tp, MCU_TYPE_PLA, PLA_CONFIG6, ocp_data);
> > +	ocp_byte_set_bits(tp, MCU_TYPE_PLA, PLA_CONFIG6,
> > LANWAKE_CLR_EN);
> >
> > -	ocp_data = ocp_read_byte(tp, MCU_TYPE_PLA, PLA_LWAKE_CTRL_REG);
> > -	ocp_data &= ~LANWAKE_PIN;
> > -	ocp_write_byte(tp, MCU_TYPE_PLA, PLA_LWAKE_CTRL_REG, ocp_data);
> > +	ocp_byte_clr_bits(tp, MCU_TYPE_PLA, PLA_LWAKE_CTRL_REG,
> > LANWAKE_PIN);
> >
> >  	/* rx aggregation */
> > -	ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_USB_CTRL);
> > -	ocp_data &= ~(RX_AGG_DISABLE | RX_ZERO_EN);
> > -	if (tp->dell_tb_rx_agg_bug)
> > -		ocp_data |= RX_AGG_DISABLE;
> > +	ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_USB_CTRL,
> > +			  RX_AGG_DISABLE | RX_ZERO_EN);
> >
> > -	ocp_write_word(tp, MCU_TYPE_USB, USB_USB_CTRL, ocp_data);
> > +	if (tp->dell_tb_rx_agg_bug)
> > +		ocp_word_set_bits(tp, MCU_TYPE_USB, USB_USB_CTRL,
> > +				  RX_AGG_DISABLE);
> 
> I think you could combine above, such as
> 
> 	if (tp->dell_tb_rx_agg_bug)
> 		ocp_word_w0w1(tp, MCU_TYPE_USB, USB_USB_CTRL,
> RX_ZERO_EN,
> 			      RX_AGG_DISABLE);
> 	else
> 		ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_USB_CTRL,
> 				  RX_AGG_DISABLE | RX_ZERO_EN);
> 
Indeed, I will fix this in v4.

> >  	rtl_tally_reset(tp);
> >
> > @@ -7200,7 +6972,6 @@ static void r8153_init(struct r8152 *tp)
> >
> >  static void r8153b_init(struct r8152 *tp)  {
> > -	u32 ocp_data;
> >  	u16 data;
> >  	int i;
> >
> [...]
> > @@ -8306,48 +8028,39 @@ static void r8156b_init(struct r8152 *tp)
> >
> >  	usb_enable_lpm(tp->udev);
> >
> > -	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_RCR);
> > -	ocp_data &= ~SLOT_EN;
> > -	ocp_write_word(tp, MCU_TYPE_PLA, PLA_RCR, ocp_data);
> > +	ocp_word_clr_bits(tp, MCU_TYPE_PLA, PLA_RCR, SLOT_EN);
> >
> > -	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_CPCR);
> > -	ocp_data |= FLOW_CTRL_EN;
> > -	ocp_write_word(tp, MCU_TYPE_PLA, PLA_CPCR, ocp_data);
> > +	ocp_word_set_bits(tp, MCU_TYPE_PLA, PLA_CPCR, FLOW_CTRL_EN);
> >
> >  	/* enable fc timer and set timer to 600 ms. */
> >  	ocp_write_word(tp, MCU_TYPE_USB, USB_FC_TIMER,
> >  		       CTRL_TIMER_EN | (600 / 8));
> >
> > -	ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_FW_CTRL);
> >  	if (!(ocp_read_word(tp, MCU_TYPE_PLA, PLA_POL_GPIO_CTRL) &
> > DACK_DET_EN))
> > -		ocp_data |= FLOW_CTRL_PATCH_2;
> > -	ocp_data &= ~AUTO_SPEEDUP;
> > -	ocp_write_word(tp, MCU_TYPE_USB, USB_FW_CTRL, ocp_data);
> > +		ocp_word_set_bits(tp, MCU_TYPE_USB, USB_FW_CTRL,
> > +				  FLOW_CTRL_PATCH_2);
> > +
> > +	ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_FW_CTRL, AUTO_SPEEDUP);
> 
> I think you could combine above, such as
> 
> 	if (!(ocp_read_word(tp, MCU_TYPE_PLA, PLA_POL_GPIO_CTRL) &
> DACK_DET_EN))
> 		ocp_word_w0w1(tp, MCU_TYPE_USB, USB_FW_CTRL,
> AUTO_SPEEDUP,
> 			      FLOW_CTRL_PATCH_2);
> 	else
> 		ocp_word_clr_bits(tp, MCU_TYPE_USB, USB_FW_CTRL,
> AUTO_SPEEDUP);
> 
Will do.

Thanks for your review.

Best Regards,
Chih-Kai