Re: [PATCH] net: ethernet: stmmac: add ARP management

[Andy Shevchenko]

On Tue, Jan 17, 2017 at 6:56 PM, Christophe Roullier
<christophe.roullier@xxxxxx> wrote:

> +static int dwmac4_arp_enable(struct mac_device_info *hw)
> +{
> +       void __iomem *ioaddr = hw->pcsr;

__iomem *config = hw->pcsr + GMAC_CONFIG;

> +       u32 value = readl(ioaddr + GMAC_CONFIG);
> +
> +       value |= GMAC_CONFIG_ARPEN;
> +
> +       writel(value, ioaddr + GMAC_CONFIG);

u32 value;

value = readl();
writel(value | ...);

?

> +
> +       value = readl(ioaddr + GMAC_CONFIG);
> +
> +       return !!(value & GMAC_CONFIG_ARPEN);
> +}

> +/* Set ARP Address */
> +static void dwmac4_set_arp_addr(void __iomem *ioaddr, bool set, u32 addr)
> +{

__iomem *arp_addr = ioaddr + GMAC_ARP_ADDR;

> +       u32 value;
> +
> +       value = readl(ioaddr + GMAC_ARP_ADDR);

Care to explain why do you need dummy readl() here?

> +
> +       if (set) {
> +               /* set arp address */
> +               value = addr;
> +       } else {
> +               /* unset arp address */
> +               value = 0;
> +       }

value = set ? addr : 0;


> +
> +       writel(value, ioaddr + GMAC_ARP_ADDR);
> +       value = readl(ioaddr + GMAC_ARP_ADDR);
> +}


> +       if ((priv->plat->arp_en) && (priv->dma_cap.arpoffsel)) {
> +               ret = priv->hw->mac->arp_en(priv->hw);
> +               if (!ret) {

Hmm... Most would expect

if (ret) {
 doing something
} else {
 doing something else
}

> +                       pr_warn(" ARP feature disabled\n");
> +               } else {

> +                       pr_info(" ARP feature enabled\n");

Wouldn't be too noisy?

pr_* -> dev_*

> +                       /* Copy MAC addr into MAC_ARP_ADDRESS register*/
> +                       priv->hw->dma->set_arp_addr(priv->ioaddr, 1,
> +                                                   priv->dev->dev_addr);
> +               }
> +       }

-- 
With Best Regards,
Andy Shevchenko