Re: [PATCH net-next v2 08/10] net: dsa: lan9303: Added ALR/fdb/mdb handling

[Egil Hjelmeland]

On 26. juli 2017 19:41, Andrew Lunn wrote:

Hi Egil

+/* This function will wait a while until mask & reg == value */
+/* Otherwise, return timeout */
+static int lan9303_csr_reg_wait(struct lan9303 *chip, int regno,
+                               int mask, char value)
+{
+       int i;
+
+       for (i = 0; i < 0x1000; i++) {
+               u32 reg;
+
+               lan9303_read_switch_reg(chip, regno, &reg);
+               if ((reg & mask) == value)
+                       return 0;
+       }
+       return -ETIMEDOUT;

Busy looping is probably not a good idea. Can you add a usleep()?

Yes

+}
+
+static int _lan9303_alr_make_entry_raw(struct lan9303 *chip, u32 dat0, u32 
dat1)

What does the _ indicate. I could understand having it when you have
lan9303_alr_make_entry_raw() call _lan9303_alr_make_entry_raw() after
taking a lock, but i don't see anything like that here.

Just my sloppy convention for something private, deep down. I can remove
the _.

+{
+       struct lan9303_alr_cache_entry *entr = lan9303_alr_cache_find_mac(
+               chip, mac);

A long line like this should be split into a declaration and an
assignment.

OK

I would probably make this a separate patch.

   Andrew

Got it.