Re: [PATCH net 5/5] net: aquantia: added err var into AQ_HW_WAIT_FOR construct

[Andrew Lunn <andrew@lunn.ch>]

On Wed, Jan 23, 2019 at 09:49:25AM +0000, Igor Russkikh wrote:
> 
> > 
> > Hi Igor
> > 
> > 	 err = readx_poll_timeout(hw_atl_itr_res_irq_get, self, alt_itr_res,
> > 	       		          alt_itr_res == 0, 10, 1000);
> > 
> > The advantage of using readx_poll_timeout is that it is used by lots
> > of other drivers and works. It is much better to use core
> > infrastructure, then build your own.
> 
> Hi Andrew, agreed, but driver have more incompatible places with constructs like
> 
> 	AQ_HW_WAIT_FOR(orig_stats_val !=
> 		       (aq_hw_read_reg(self, HW_ATL_FW2X_MPI_STATE2_ADDR) &
> 				       BIT(CAPS_HI_STATISTICS)),
> 		       1U, 10000U);

You can define a little helper:

static u32 aq_hw_read_mpi_state2_addr(struct aq_hw_s *self)
{
	return aq_hw_read_reg(self, HW_ATL_FW2X_MPI_STATE2_ADDR);
}

Then

	readx_poll_timeout(u32 aq_hw_read_mpi_state2_addr, self,
			   stats_val, orig_stats_val != stats_val & BIT(CAPS_HI_STATISTICS));

Given you have the comment:

        /* Wait FW to report back */

You think the current code is not very readable. So you could actually
have:

static int sq_fw2_update_stats_fw_wait(aq_hw_s *self, u32 orig_stats_val)
{
	u32 stats_val;

	return readx_poll_timeout(u32 aq_hw_read_mpi_state2_addr, self,
			           stats_val,
				   orig_stats_val != stats_val & BIT(CAPS_HI_STATISTICS),
				   1, 1000);
}

You see this sort of construct in quite a lot of drivers.

> Found duplicating readl_poll_timeout declaration here:
> https://elixir.bootlin.com/linux/latest/source/drivers/phy/qualcomm/phy-qcom-ufs-i.h#L27
> Not sure what's the reason, but may worth cleaning it up.

Thanks.

	Andrew