[U-Boot] [PATCH 3/7] net: sh_eth: Split sh_eth_recv

[Joe Hershberger]

On Wed, Jan 24, 2018 at 4:20 PM, Marek Vasut <marek.vasut at gmail.com> wrote:
> Split sh_eth_recv into two functions, one which checks whether
> a packet was received and one which handles the received packet.
> This is done in preparation for DM support, which handles these
> two parts separately.
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas at gmail.com>
> Cc: Nobuhiro Iwamatsu <iwamatsu at nigauri.org>
> Cc: Joe Hershberger <joe.hershberger at ni.com>
> ---
>  drivers/net/sh_eth.c | 73 +++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 46 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/net/sh_eth.c b/drivers/net/sh_eth.c
> index 7b11a5a0d3..99eab4c688 100644
> --- a/drivers/net/sh_eth.c
> +++ b/drivers/net/sh_eth.c
> @@ -118,39 +118,58 @@ static int sh_eth_send_legacy(struct eth_device *dev, void *packet, int len)
>         return sh_eth_send_common(eth, packet, len);
>  }
>
> -static int sh_eth_recv_common(struct sh_eth_dev *eth)
> +static int sh_eth_recv_start(struct sh_eth_dev *eth)
>  {
>         int port = eth->port, len = 0;

This "port" local variable seems pretty useless.

>         struct sh_eth_info *port_info = &eth->port_info[port];
> -       uchar *packet;
>
>         /* Check if the rx descriptor is ready */
>         invalidate_cache(port_info->rx_desc_cur, sizeof(struct rx_desc_s));
> -       if (!(port_info->rx_desc_cur->rd0 & RD_RACT)) {
> -               /* Check for errors */
> -               if (!(port_info->rx_desc_cur->rd0 & RD_RFE)) {
> -                       len = port_info->rx_desc_cur->rd1 & 0xffff;
> -                       packet = (uchar *)
> -                               ADDR_TO_P2(port_info->rx_desc_cur->rd2);
> -                       invalidate_cache(packet, len);
> -                       net_process_received_packet(packet, len);
> -               }
> -
> -               /* Make current descriptor available again */
> -               if (port_info->rx_desc_cur->rd0 & RD_RDLE)
> -                       port_info->rx_desc_cur->rd0 = RD_RACT | RD_RDLE;
> -               else
> -                       port_info->rx_desc_cur->rd0 = RD_RACT;
> -
> -               flush_cache_wback(port_info->rx_desc_cur,
> -                                 sizeof(struct rx_desc_s));
> -
> -               /* Point to the next descriptor */
> -               port_info->rx_desc_cur++;
> -               if (port_info->rx_desc_cur >=
> -                   port_info->rx_desc_base + NUM_RX_DESC)
> -                       port_info->rx_desc_cur = port_info->rx_desc_base;
> -       }
> +       if (port_info->rx_desc_cur->rd0 & RD_RACT)
> +               return -EINVAL;
> +
> +       /* Check for errors */
> +       if (port_info->rx_desc_cur->rd0 & RD_RFE)
> +               return -EINVAL;
> +
> +       len = port_info->rx_desc_cur->rd1 & 0xffff;

Just return here directly and delete the "len" variable.

> +
> +       return len;
> +}
> +
> +static void sh_eth_recv_finish(struct sh_eth_dev *eth)
> +{
> +       struct sh_eth_info *port_info = &eth->port_info[eth->port];
> +
> +       /* Make current descriptor available again */
> +       if (port_info->rx_desc_cur->rd0 & RD_RDLE)
> +               port_info->rx_desc_cur->rd0 = RD_RACT | RD_RDLE;
> +       else
> +               port_info->rx_desc_cur->rd0 = RD_RACT;
> +
> +       flush_cache_wback(port_info->rx_desc_cur,
> +                         sizeof(struct rx_desc_s));
> +
> +       /* Point to the next descriptor */
> +       port_info->rx_desc_cur++;
> +       if (port_info->rx_desc_cur >=
> +           port_info->rx_desc_base + NUM_RX_DESC)
> +               port_info->rx_desc_cur = port_info->rx_desc_base;
> +}
> +
> +static int sh_eth_recv_common(struct sh_eth_dev *eth)
> +{
> +       int port = eth->port, len = 0;

I would also remove "port" here.

> +       struct sh_eth_info *port_info = &eth->port_info[port];
> +       uchar *packet = (uchar *)ADDR_TO_P2(port_info->rx_desc_cur->rd2);
> +
> +       len = sh_eth_recv_start(eth);
> +       if (len > 0) {
> +               invalidate_cache(packet, len);
> +               net_process_received_packet(packet, len);
> +               sh_eth_recv_finish(eth);
> +       } else

I thought checkpatch would want you to have matching braces here. I
think it looks cleaner at least. Probably a moot point though since...

> +               len = 0;

Are you expecting "len" to be negative? If not, it will already be 0.
>From the function above, it looks like it will always be >= 0.

>
>         /* Restart the receiver if disabled */
>         if (!(sh_eth_read(port_info, EDRRR) & EDRRR_R))
> --
> 2.15.1
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot