> + * This driver was written as a replacement for the vendor provided
> + * rtl8723au driver. As the Realtek 8xxx chips are very similar in
> + * their programming interface, I have started adding support for
> + * additional 8xxx chips like the 8192cu, 8188cus, etc.
That last sentence here seems like it might be more suitable in the
commit message then here - you'll surely forget to update it ;)
> +> > /*
> +> > * MBOX ready?
> +> > */
> +> > retry = 100;
> +> > do {
> +> > > val8 = rtl8xxxu_read8(priv, REG_HMTFR);
> +> > > if (!(val8 & BIT(mbox_nr)))
> +> > > > break;
> +> > } while (retry--);
Seems fishy without any delay in the loop?
> +> > val32 = rtl8xxxu_read32(priv, REG_OFDM0_TRX_PATH_ENABLE);
> +> > val32 &= ~OFDM_RF_PATH_TX_MASK;
> +> > if (priv->tx_paths == 2)
"TX path" is very uncommon language for this... I'd suggest changing
all that to "TX stream" or "TX chain"?
> +> > if (priv->rf_paths == 2)
Similar here - and should that be RX not RF?
> +static int rtl8723a_channel_to_group(int channel)
> +{
> +> > int group;
> +
> +> > if (channel < 4)
> +> > > group = 0;
> +> > else if (channel < 10)
> +> > > group = 1;
> +> > else
> +> > > group = 2;
> +
> +> > return group;
> +}
Could remove the group variable, it's kinda pointless - just return
immediately?
if (channel < 4)
return 0;
if (channel < 10)
return 1;
return 2;
> +> > /* Poll for data read */
> +> > val32 = rtl8xxxu_read32(priv, REG_EFUSE_CTRL);
> +> > for (i = 0; i < RTL8XXXU_MAX_REG_POLL; i++) {
> +> > > val32 = rtl8xxxu_read32(priv, REG_EFUSE_CTRL);
> +> > > if (val32 & BIT(31))
> +> > > > break;
> +> > }
>
Hmm, similar poll loop like above w/o any delay?
A few more seem to exist too.
> +> > > > for (i = 0; i < EFUSE_MAX_WORD_UNIT; i++) {
> +> > > > > /* Check word enable condition in the section */
> +> > > > > if (!(word_mask & BIT(i))) {
> +> > > > > > ret = rtl8xxxu_read_efuse8(priv,
> +> > > > > > > > > efuse_addr++,
> +> > > > > > > > > &val8);
> +> > > > > > if (ret)
> +> > > > > > > goto exit;
> +> > > > > > priv->efuse_wifi.raw[map_addr++] = val8;
> +
> +> > > > > > ret = rtl8xxxu_read_efuse8(priv,
> +> > > > > > > > > efuse_addr++,
> +> > > > > > > > > &val8);
> +> > > > > > if (ret)
> +> > > > > > > goto exit;
> +> > > > > > priv->efuse_wifi.raw[map_addr++] = val8;
> +> > > > > } else
> +> > > > > > map_addr += 2;
> +> > > > }
seems like it might better be in a helper function :)
> +static int rtl8xxxu_init_phy_regs(struct rtl8xxxu_priv *priv,
> +> > > > > struct rtl8xxxu_reg32val *array)
> +{
> +> > int i, ret;
> +> > u16 reg;
> +> > u32 val;
> +
> +> > for (i = 0; ; i++) {
> +> > > reg = array[i].reg;
> +> > > val = array[i].val;
> +
> +> > > if (reg == 0xffff && val == 0xffffffff)
> +> > > > break;
Maybe passing ARRAY_SIZE to these would be nicer than having to they're
terminated? Dunno though, might be a lot of infrastructure to do that.
Ugh, getting too long for me - anything in particular I should look at?
:)
johannes
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
