Re: [PATCH net-next v1 4/8] net: dsa: microchip: ksz8: Refactor ksz8_r_dyn_mac_table() for readability

[Arun.Ramadoss]

Hi Oleksij,

On Tue, 2024-04-02 at 15:13 +0200, Oleksij Rempel wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> Move the code out of a long if statement scope in
> ksz8_r_dyn_mac_table()
> to improve code readability.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>
> ---
>  drivers/net/dsa/microchip/ksz8795.c | 60 +++++++++++++++----------
> ----
>  1 file changed, 31 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz8795.c
> b/drivers/net/dsa/microchip/ksz8795.c
> index b70aa2c0a85ec..1962195e16a6f 100644
> --- a/drivers/net/dsa/microchip/ksz8795.c
> +++ b/drivers/net/dsa/microchip/ksz8795.c
> @@ -416,7 +416,9 @@ static int ksz8_r_dyn_mac_table(struct ksz_device
> *dev, u16 addr, u8 *mac_addr,
>         const u32 *masks;
>         const u16 *regs;
>         u16 ctrl_addr;
> +       u64 buf = 0;
>         u8 data;
> +       int cnt;
>         int rc;

If rc is initialized with 0, we don't need to assign rc = 0 in the
success path.

But otherwise
Acked-by: Arun Ramadoss <arun.ramadoss@xxxxxxxxxxxxx>

> 
>         shifts = dev->info->shifts;
> @@ -432,38 +434,38 @@ static int ksz8_r_dyn_mac_table(struct
> ksz_device *dev, u16 addr, u8 *mac_addr,
>         if (rc == -EAGAIN) {
>                 if (addr == 0)
>                         *entries = 0;
> +               goto unlock_alu;
>         } else if (rc == -ENXIO) {
>                 *entries = 0;
> -       /* At least one valid entry in the table. */
> -       } else {
> -               u64 buf = 0;
> -               int cnt;
> -
> -               ksz_read64(dev, regs[REG_IND_DATA_HI], &buf);
> -               data_hi = (u32)(buf >> 32);
> -               data_lo = (u32)buf;
> -
> -               /* Check out how many valid entry in the table. */
> -               cnt = data & masks[DYNAMIC_MAC_TABLE_ENTRIES_H];
> -               cnt <<= shifts[DYNAMIC_MAC_ENTRIES_H];
> -               cnt |= (data_hi & masks[DYNAMIC_MAC_TABLE_ENTRIES])
> >>
> -                       shifts[DYNAMIC_MAC_ENTRIES];
> -               *entries = cnt + 1;
> -
> -               *fid = (data_hi & masks[DYNAMIC_MAC_TABLE_FID]) >>
> -                       shifts[DYNAMIC_MAC_FID];
> -               *src_port = (data_hi &
> masks[DYNAMIC_MAC_TABLE_SRC_PORT]) >>
> -                       shifts[DYNAMIC_MAC_SRC_PORT];
> -
> -               mac_addr[5] = (u8)data_lo;
> -               mac_addr[4] = (u8)(data_lo >> 8);
> -               mac_addr[3] = (u8)(data_lo >> 16);
> -               mac_addr[2] = (u8)(data_lo >> 24);
> -
> -               mac_addr[1] = (u8)data_hi;
> -               mac_addr[0] = (u8)(data_hi >> 8);
> -               rc = 0;
> +               goto unlock_alu;
>         }
> +
> +       ksz_read64(dev, regs[REG_IND_DATA_HI], &buf);
> +       data_hi = (u32)(buf >> 32);
> +       data_lo = (u32)buf;
> +
> +       /* Check out how many valid entry in the table. */
> +       cnt = data & masks[DYNAMIC_MAC_TABLE_ENTRIES_H];
> +       cnt <<= shifts[DYNAMIC_MAC_ENTRIES_H];
> +       cnt |= (data_hi & masks[DYNAMIC_MAC_TABLE_ENTRIES]) >>
> +               shifts[DYNAMIC_MAC_ENTRIES];
> +       *entries = cnt + 1;
> +
> +       *fid = (data_hi & masks[DYNAMIC_MAC_TABLE_FID]) >>
> +               shifts[DYNAMIC_MAC_FID];
> +       *src_port = (data_hi & masks[DYNAMIC_MAC_TABLE_SRC_PORT]) >>
> +               shifts[DYNAMIC_MAC_SRC_PORT];
> +
> +       mac_addr[5] = (u8)data_lo;
> +       mac_addr[4] = (u8)(data_lo >> 8);
> +       mac_addr[3] = (u8)(data_lo >> 16);
> +       mac_addr[2] = (u8)(data_lo >> 24);
> +
> +       mac_addr[1] = (u8)data_hi;
> +       mac_addr[0] = (u8)(data_hi >> 8);
> +       rc = 0;
> +
> +unlock_alu:
>         mutex_unlock(&dev->alu_mutex);
> 
>         return rc;
> --
> 2.39.2
>