Re: [PATCH v2 2/5] rtw88: 8821c: add power tracking

[Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>]

On 2020-06-03 17:42:15 [+0800], yhchuang@xxxxxxxxxxx wrote:
> diff --git a/drivers/net/wireless/realtek/rtw88/rtw8821c.c b/drivers/net/wireless/realtek/rtw88/rtw8821c.c
> index 904eb494ccad..aa2457046ad1 100644
> --- a/drivers/net/wireless/realtek/rtw88/rtw8821c.c
> +++ b/drivers/net/wireless/realtek/rtw88/rtw8821c.c
> @@ -61,6 +61,46 @@ static int rtw8821c_read_efuse(struct rtw_dev *rtwdev, u8 *log_map)
>  	return 0;
>  }
>  
> +static const u32 rtw8821c_txscale_tbl[] = {
> +	0x081, 0x088, 0x090, 0x099, 0x0a2, 0x0ac, 0x0b6, 0x0c0, 0x0cc, 0x0d8,
> +	0x0e5, 0x0f2, 0x101, 0x110, 0x120, 0x131, 0x143, 0x156, 0x16a, 0x180,
> +	0x197, 0x1af, 0x1c8, 0x1e3, 0x200, 0x21e, 0x23e, 0x261, 0x285, 0x2ab,
> +	0x2d3, 0x2fe, 0x32b, 0x35c, 0x38e, 0x3c4, 0x3fe
> +};
> +
> +static const u8 rtw8821c_get_swing_index(struct rtw_dev *rtwdev)
> +{
> +	u8 i = 0;
> +	u32 swing, table_value;
> +
> +	swing = rtw_read32_mask(rtwdev, REG_TXSCALE_A, 0xffe00000);
> +	for (i = 0; i < ARRAY_SIZE(rtw8821c_txscale_tbl); i++) {
> +		table_value = rtw8821c_txscale_tbl[i];
> +		if (swing == table_value)
> +			break;
> +	}
> +
> +	return i;
> +}

This matches rtw8822b_get_swing_index() and rtw8822b_txscale_tbl. How
often are the lookups performed and how likely is it that the value is
not found?
With something like this:

 static int rtw8821c_get_swing_index(unsigned int swing)
 {
         unsigned short val;
         int start, end;
         int candidate;
 
         start = 0;
         end = ARRAY_SIZE(rtw8821c_txscale_tbl);
         while (start < end) {
 
                 candidate = start + (end - start) / 2;
                 val = rtw8821c_txscale_tbl[candidate];
                 if (swing == val)
                         return candidate;
                 if (swing < val)
                         end = candidate;
                 else
                         start = candidate + 1;
         }
         return -EINVAL;
 }

you would end up with more less constant lookup time with ~6 lookups in worst
case. I guess it is not a hot path but still…
Can the table become u16?

…
> +const struct rtw_pwr_track_tbl rtw8821c_rtw_pwr_track_tbl = {
> +	.pwrtrk_5gb_n[0] = rtw8821c_pwrtrk_5gb_n[0],
> +	.pwrtrk_5gb_n[1] = rtw8821c_pwrtrk_5gb_n[1],
> +	.pwrtrk_5gb_n[2] = rtw8821c_pwrtrk_5gb_n[2],

so the other driver use RTW_PWR_TRK_5G_1…3. Using constants here isn't
better because a line like

	.pwrtrk_5gb_n[2] = rtw8821c_pwrtrk_5gb_n[22],

will not trigger a warning. This is just an observation.

Sebastian