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

[Andrew Lunn]

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